feat: adding onboarding worker as standard pattern (CM-1171) - #4530
Conversation
PR SummaryMedium Risk Overview Onboarding activities load pending rows, re-check Schema/DAL: migration adds nullable Reviewed by Cursor Bugbot for commit 57c05b6. Bugbot is set up for automated code reviews on this repo. Configure here. |
2bb3a23 to
cc4700f
Compare
There was a problem hiding this comment.
Pull request overview
Adds scheduled Temporal-based onboarding for critical projects, including failure tracking in the project catalog.
Changes:
- Adds onboarding activities and a sequential batch workflow.
- Registers a daily Temporal schedule.
- Adds terminal error state and persisted onboarding errors.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/data-access-layer/src/project-catalog/types.ts |
Extends catalog types with onboarding errors. |
services/libs/data-access-layer/src/project-catalog/projectCatalog.ts |
Reads and updates onboarding errors. |
services/apps/automatic_onboarding_worker/src/workflows/onboardProjects.ts |
Implements batch onboarding workflow. |
services/apps/automatic_onboarding_worker/src/workflows/index.ts |
Removes obsolete barrel. |
services/apps/automatic_onboarding_worker/src/workflows.ts |
Exports workflow and input type. |
services/apps/automatic_onboarding_worker/src/types.ts |
Defines workflow input. |
services/apps/automatic_onboarding_worker/src/schedules/scheduleProjectsOnboarding.ts |
Registers daily schedule. |
services/apps/automatic_onboarding_worker/src/main.ts |
Registers schedule during startup. |
services/apps/automatic_onboarding_worker/src/activities/activities.ts |
Adds onboarding and failure activities. |
services/apps/automatic_onboarding_worker/src/activities.ts |
Exports activities. |
backend/src/database/migrations/V1787916364__onboarding-error-to-project-catalog.sql |
Adds onboarding error column. |
Suppressed comments (1)
services/libs/data-access-layer/src/project-catalog/types.ts:48
- This type advertises that create/upsert callers can persist
onboardingError, butinsertProjectCatalog,bulkInsertProjectCatalog,upsertProjectCatalog, andbulkUpsertProjectCatalogall omit it, so supplied values are silently discarded. Either implement it in those SQL paths or remove it from the create type.
onboardingError?: string | null
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cc4700f. Configure here.
…ror reason on db Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
ce23e77 to
d5336de
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Suppressed comments (2)
services/apps/automatic_onboarding_worker/src/activities/activities.ts:40
- This writer-side read does not make the activity retry-idempotent. If
/github-nango-connectsucceeds and the activity crashes or times out beforeonboardedAtis written, the retry seesNULLand calls the endpoint again; that endpoint creates a new integration whenever nointegrationIdis supplied (backend/src/services/integrationService.ts:857-869). Make the external operation idempotent with a stable key/persisted integration ID (and atomically claim the catalog row) before enabling retries.
// Guard: uses the writer connection to avoid replica lag missing a just-written onboardedAt.
const fresh = await findAlreadyOnboarded(qx, project.id)
services/apps/automatic_onboarding_worker/src/workflows/onboardProjects.ts:60
- This comment only narrates the behavior of the surrounding
try/catch, which is already explicit. Remove it to follow the repository's no-explanatory-comments convention (CLAUDE.md:72-84).
// Don't let a failure to record the error state abort the rest of the batch.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
services/apps/automatic_onboarding_worker/src/workflows/onboardProjects.ts:61
- This comment merely describes the immediately following catch behavior. Remove it; the nested catch and log already make the failure isolation clear.
// Don't let a failure to record the error state abort the rest of the batch.
services/apps/automatic_onboarding_worker/src/activities/activities.ts:41
- This read does not make the activity at-most-once. If
/github-nango-connectcreates the integration but the activity times out or the worker dies beforeonboardedAtis committed, Temporal retries with this field still null and posts again; that endpoint creates a new integration whenever nointegrationIdis supplied. Make the external operation idempotent with a stable key/integration ID that retries reuse (and persist that state before invoking it), rather than relying on the post-call timestamp.
// Guard: uses the writer connection to avoid replica lag missing a just-written onboardedAt.
const fresh = await findAlreadyOnboarded(qx, project.id)
services/apps/automatic_onboarding_worker/src/workflows/onboardProjects.ts:6
- This comment only restates the timeout configuration and the activity name. Remove it to follow the repository rule that straightforward code should be self-explanatory.
// Short timeout: just a DB read.
services/apps/automatic_onboarding_worker/src/schedules/scheduleProjectsOnboarding.ts:17
- This comment narrates the cron expression and PR intent rather than documenting a non-obvious invariant. Remove it to comply with the codebase's no-explanatory-comments convention.
// Daily: catches up on whatever landed in 'onboard' state, independent of the evaluation schedule's timing.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/apps/automatic_onboarding_worker/src/workflows/onboardProjects.ts:58
onboardAndUpdateProjectcan throw after the external onboarding has succeeded—for example, if the catalog update fails. This catch then marks the row as a terminal error even though its segment/integration was created. Distinguish onboarding failures from post-onboarding persistence failures; only mark the former terminal, and retry or reconcile the catalog write for the latter.
await failureActivities.markProjectOnboardingFailed(project.id, reason)

Summary
Adds the onboarding stage to the critical-projects catalog pipeline: a batch Temporal workflow that
picks up projects sitting in
action='onboard', calls the onboarder (same CDP API calls the manualonboard-projects.tsscript already uses), and marks them as onboarded — plus a daily schedule sothe queue drains automatically instead of requiring a manual trigger. This is PR7 of the critical
projects onboarding pipeline epic.
Changes
activities/activities.ts(new, moved out of the flatactivities.ts):fetchProjectsPendingOnboardingreads the queue;
onboardAndUpdateProjectre-checksonboardedAton the writer connection beforecalling the onboarder, to guarantee at-most-once side effects under Temporal activity retries.
workflows/onboardProjects.ts(new): batch workflow — fetches the pending batch, onboards eachproject sequentially, and never aborts the whole batch on a single project's failure.
onboardingErrorstate on failure: when an activity exhausts its retries, the workflow calls anew
markProjectOnboardingFailedactivity, settingaction='error'and persisting the root-causemessage (via Temporal's
rootCause(), which unwrapsActivityFailure.causeinstead of the genericwrapper message) in a new
onboardingErrorcolumn. This removes the project from the daily queueinstead of retrying it forever, and makes failures queryable/alertable.
onboardingError TEXTonprojectCatalog(nullable, additive).markProjectOnboardingFaileditself is wrapped in its own try/catch in the workflow, so afailure to record the error state can't abort the rest of the batch either.
schedules/scheduleProjectsOnboarding.ts(new): daily cron (0 8 * * *), independent of theweekly evaluation schedule's timing — deliberately not chained off
evaluateProjects, so it alwayscatches up on whatever is pending regardless of how long the previous stage took.
src/workflows.ts(new flat barrel): needed for type-safe client-side imports in the scheduleregistration code — mirrors the existing pattern in
projects_evaluation_workerexactly (flatbarrel file re-exporting from the specific workflow file, sitting alongside the
workflows/directory that the Temporal bundler resolves at runtime).
src/workflows/index.tswas removed asdead code: any
../workflowsimport resolves to the flat file first, so the directory'sindex.tswas permanently unreachable.
data-access-layer/src/project-catalog/):ProjectCatalogActiongains'error'as aterminal state;
onboardingErrorthreaded through create/update types andupdateProjectCatalog.Type of change
JIRA ticket
CM-1171