chore: properly send job update to go engine#698
Conversation
WalkthroughAdds a workspace engine SDK dependency and updates the GitHub workflow_run webhook handler to derive a jobId, pre-dispatch JobUpdated events (with Run/Workflow metadata) to all workspaces when jobId is found, then persist job status and metadata to the database. Changes
Sequence Diagram(s)sequenceDiagram
actor GitHub
participant Handler as Webhook Handler
participant SDK as Workspace Engine SDK
participant DB as Database
GitHub->>Handler: workflow_run event
Handler->>Handler: extractUuid -> jobId?
alt jobId derived
Handler->>Handler: build Run/Workflow links & metadata
Handler->>Handler: map JobStatus -> engine status
Handler->>DB: getAllWorkspaceIds
loop per workspace
Handler->>SDK: dispatch JobUpdated (jobId, status, metadata)
SDK-->>Handler: ack
end
end
Handler->>DB: upsert/update job (status, externalId, timestamps, metadata)
DB-->>Handler: OK
Handler-->>GitHub: 200/response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-01-21T19:52:22.838ZApplied to files:
🧬 Code graph analysis (1)apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts (1)
75-100: Optional: Add exhaustiveness check for future-proofing.The switch statement lacks a default case. While TypeScript's type checking should catch missing cases at compile time, adding an exhaustiveness check provides a runtime safeguard.
const convertStatusToOapiStatus = ( status: JobStatus, ): WorkspaceEngine["schemas"]["JobStatus"] => { switch (status) { case JobStatus.Successful: return "successful"; // ... other cases ... case JobStatus.Failure: return "failure"; + default: + const exhaustiveCheck: never = status; + throw new Error(`Unhandled JobStatus: ${exhaustiveCheck}`); } };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/webservice/package.json(1 hunks)apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.tsapps/webservice/package.json
🧠 Learnings (1)
📚 Learning: 2025-01-21T19:52:22.838Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#287
File: packages/validators/src/jobs/conditions/status-condition.ts:4-4
Timestamp: 2025-01-21T19:52:22.838Z
Learning: In apps/webservice/src/app/api/github/webhook/workflow/handler.ts, the external "completed" status is intentionally mapped to JobStatus.Successful to handle GitHub workflow status conversions.
Applied to files:
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts
🧬 Code graph analysis (1)
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts (3)
packages/db/src/client.ts (1)
db(18-18)packages/db/src/schema/job.ts (2)
JobStatus(149-149)job(75-108)packages/events/src/kafka/client.ts (1)
sendGoEvent(35-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/webservice/package.json (1)
29-29: LGTM! Clean dependency addition.The workspace-engine-sdk dependency is properly added to support the new workspace event dispatching functionality.
apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts (2)
22-26: LGTM! Clean implementation.The function correctly queries all workspace IDs. Error handling follows the project's pattern of allowing errors to propagate naturally.
178-179: ****The review comment identifies a non-existent duplication concern. The
updateJobfunction does not persist metadata to the database—it only queries the job with its metadata and dispatches a JobUpdated event. In contrast,updateLinksdirectly inserts/updates into the jobMetadata table. These functions serve different purposes:updateLinksmodifies database state, whileupdateJobtriggers event dispatching. There is no metadata operation duplication or conflict.Likely an incorrect or invalid review comment.
| if (jobId != null) { | ||
| const workspaceIds = await getAllWorkspaceIds(); | ||
| const jobUpdateEvent: WorkspaceEngine["schemas"]["JobUpdateEvent"] = { | ||
| job: { | ||
| id: jobId, | ||
| externalId, | ||
| createdAt: startedAt.toISOString(), | ||
| updatedAt: updatedAt.toISOString(), | ||
| completedAt: completedAt?.toISOString() ?? undefined, | ||
| startedAt: startedAt.toISOString(), | ||
| status: convertStatusToOapiStatus(status as JobStatus), | ||
| releaseId: "", | ||
| jobAgentConfig: {}, | ||
| jobAgentId: "", | ||
| metadata, | ||
| }, | ||
| fieldsToUpdate: [ | ||
| "externalId", | ||
| "updatedAt", | ||
| "completedAt", | ||
| "startedAt", | ||
| "status", | ||
| "metadata", | ||
| ], | ||
| }; | ||
|
|
||
| for (const workspaceId of workspaceIds) { | ||
| await sendGoEvent({ | ||
| workspaceId, | ||
| eventType: Event.JobUpdated, | ||
| data: jobUpdateEvent, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Events sent to all workspaces before job validation.
Two critical issues with this event dispatching logic:
-
Broadcasting to ALL workspaces: The code sends
JobUpdatedevents to every workspace in the system (line 136). Jobs typically belong to a specific workspace - broadcasting to all workspaces could be a security/privacy issue and at minimum is inefficient. The job's workspace should be determined first. -
Race condition - events before validation: Events are dispatched (lines 161-168) before verifying the job exists (lines 171-173). If
getJobreturns null, the handler throws an error, but events have already been sent to all workspaces with potentially invalid data, creating inconsistent state.
Additionally, the sequential loop (lines 161-168) with await will be slow with many workspaces.
Consider this refactor:
const jobId = extractUuid(name);
+
+ const job = await getJob(id, name);
+ if (job == null)
+ throw new Error(`Job not found: externalId=${id} name=${name}`);
// ... build metadata ...
if (jobId != null) {
- const workspaceIds = await getAllWorkspaceIds();
+ // TODO: Determine the correct workspace for this job
+ // const workspaceId = await getJobWorkspaceId(job.id);
const jobUpdateEvent: WorkspaceEngine["schemas"]["JobUpdateEvent"] = {
// ... event data ...
};
- for (const workspaceId of workspaceIds) {
- await sendGoEvent({
- workspaceId,
- eventType: Event.JobUpdated,
- data: jobUpdateEvent,
- timestamp: Date.now(),
- });
- }
+ await sendGoEvent({
+ workspaceId, // single workspace
+ eventType: Event.JobUpdated,
+ data: jobUpdateEvent,
+ timestamp: Date.now(),
+ });
}
- const job = await getJob(id, name);
- if (job == null)
- throw new Error(`Job not found: externalId=${id} name=${name}`);Committable suggestion skipped: line range outside the PR's diff.
| const jobUpdateEvent: WorkspaceEngine["schemas"]["JobUpdateEvent"] = { | ||
| job: { | ||
| id: jobId, | ||
| externalId, | ||
| createdAt: startedAt.toISOString(), | ||
| updatedAt: updatedAt.toISOString(), | ||
| completedAt: completedAt?.toISOString() ?? undefined, | ||
| startedAt: startedAt.toISOString(), | ||
| status: convertStatusToOapiStatus(status as JobStatus), | ||
| releaseId: "", | ||
| jobAgentConfig: {}, | ||
| jobAgentId: "", | ||
| metadata, | ||
| }, | ||
| fieldsToUpdate: [ | ||
| "externalId", | ||
| "updatedAt", | ||
| "completedAt", | ||
| "startedAt", | ||
| "status", | ||
| "metadata", | ||
| ], | ||
| }; |
There was a problem hiding this comment.
Major: Incomplete job data in event payload.
The JobUpdateEvent is constructed with placeholder values for critical fields:
releaseId: ""(line 146)jobAgentConfig: {}(line 147)jobAgentId: ""(line 148)
Since the job isn't fetched until line 171, the actual values aren't available. Downstream consumers of this event will receive incomplete data, which could cause issues in workspace engines expecting full job context.
Fetch the job first, then populate the event with actual data:
+ const job = await getJob(id, name);
+ if (job == null)
+ throw new Error(`Job not found: externalId=${id} name=${name}`);
+
const jobUpdateEvent: WorkspaceEngine["schemas"]["JobUpdateEvent"] = {
job: {
id: jobId,
externalId,
// ...
- releaseId: "",
- jobAgentConfig: {},
- jobAgentId: "",
+ releaseId: job.releaseId ?? "",
+ jobAgentConfig: job.jobAgentConfig,
+ jobAgentId: job.jobAgentId ?? "",
metadata,
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts around
lines 137 to 159, the JobUpdateEvent is being built with placeholder values
(releaseId: "", jobAgentConfig: {}, jobAgentId: "") before the actual job is
fetched later; fetch the job first (the fetch occurs around line 171), then
populate job.releaseId, job.jobAgentConfig and job.jobAgentId from the fetched
job object when constructing the event, and ensure fieldsToUpdate includes any
of those fields you intend to update so the payload contains real job context
instead of empty placeholders.
| for (const workspaceId of workspaceIds) { | ||
| await sendGoEvent({ | ||
| workspaceId, | ||
| eventType: Event.JobUpdated, | ||
| data: jobUpdateEvent, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Major: Missing error handling for event dispatch failures.
If sendGoEvent fails for any workspace, the entire webhook handler will throw and exit, preventing:
- Events to remaining workspaces
- Database updates (lines 176-179)
This creates inconsistent state where some workspaces are notified while the database isn't updated.
Wrap event sending in try/catch or use Promise.allSettled:
- for (const workspaceId of workspaceIds) {
- await sendGoEvent({
- workspaceId,
- eventType: Event.JobUpdated,
- data: jobUpdateEvent,
- timestamp: Date.now(),
- });
- }
+ await Promise.allSettled(
+ workspaceIds.map((workspaceId) =>
+ sendGoEvent({
+ workspaceId,
+ eventType: Event.JobUpdated,
+ data: jobUpdateEvent,
+ timestamp: Date.now(),
+ }).catch((err) => {
+ console.error(`Failed to send event to workspace ${workspaceId}:`, err);
+ })
+ )
+ );This also parallelizes the event sending for better performance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const workspaceId of workspaceIds) { | |
| await sendGoEvent({ | |
| workspaceId, | |
| eventType: Event.JobUpdated, | |
| data: jobUpdateEvent, | |
| timestamp: Date.now(), | |
| }); | |
| } | |
| await Promise.allSettled( | |
| workspaceIds.map((workspaceId) => | |
| sendGoEvent({ | |
| workspaceId, | |
| eventType: Event.JobUpdated, | |
| data: jobUpdateEvent, | |
| timestamp: Date.now(), | |
| }).catch((err) => { | |
| console.error(`Failed to send event to workspace ${workspaceId}:`, err); | |
| }) | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In apps/webservice/src/app/api/github/webhook/handlers/workflow_run.ts around
lines 161 to 168, the loop that calls sendGoEvent can throw and abort the
handler, preventing events to remaining workspaces and the subsequent DB
updates; change it to send events in parallel using Promise.allSettled (or
individually wrap each sendGoEvent in try/catch) so failures are captured
per-workspace, log or record any errors for failed sends, and ensure the
database update block at lines ~176-179 always runs regardless of event dispatch
outcomes.
d41608e to
c1fda4a
Compare
Summary by CodeRabbit