-
Notifications
You must be signed in to change notification settings - Fork 245
[core] Combine initial run fetch, event fetch, and run_started event creation #1569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| "@workflow/world": patch | ||
| "@workflow/core": patch | ||
| "@workflow/world-local": patch | ||
| "@workflow/world-postgres": patch | ||
| --- | ||
|
|
||
| Combine initial run fetch, event fetch, and run_started event creation |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,7 +190,15 @@ export function createEventsStorage( | |
| }; | ||
| } | ||
|
|
||
| // Run state transitions are not allowed on terminal runs | ||
| // For run_started on terminal runs, use RunExpiredError so the | ||
| // runtime knows to exit without retrying. | ||
| if (data.eventType === 'run_started') { | ||
| throw new RunExpiredError( | ||
| `Workflow run "${effectiveRunId}" is already in terminal state "${currentRun.status}"` | ||
| ); | ||
| } | ||
|
|
||
| // Other run state transitions are not allowed on terminal runs | ||
| if ( | ||
| runTerminalEvents.includes(data.eventType) || | ||
| data.eventType === 'run_cancelled' | ||
|
|
@@ -280,6 +288,10 @@ export function createEventsStorage( | |
| createdAt: now, | ||
| specVersion: effectiveSpecVersion, | ||
| }; | ||
| // Strip eventData from run_started — it belongs on run_created only. | ||
| if (data.eventType === 'run_started' && 'eventData' in event) { | ||
| delete (event as any).eventData; | ||
| } | ||
|
|
||
| // Track entity created/updated for EventResult | ||
| let run: WorkflowRun | undefined; | ||
|
|
@@ -316,6 +328,15 @@ export function createEventsStorage( | |
| } else if (data.eventType === 'run_started') { | ||
| // Reuse currentRun from validation (already read above) | ||
| if (currentRun) { | ||
| // If already running, return the run without inserting a | ||
| // duplicate event. This makes run_started idempotent for | ||
| // concurrent invocations. We omit preloaded events here | ||
| // because this is a rare race-condition path — the runtime | ||
| // falls back to getAllWorkflowRunEvents(). | ||
| if (currentRun.status === 'running') { | ||
| return { run: currentRun }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking (same concern as postgres): This early return for already-running also returns
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the comment — same rationale as the postgres path (rare race condition, runtime falls back to full event fetch). |
||
| } | ||
|
|
||
| run = { | ||
| runId: currentRun.runId, | ||
| deploymentId: currentRun.deploymentId, | ||
|
|
@@ -832,13 +853,29 @@ export function createEventsStorage( | |
| const resolveData = params?.resolveData ?? DEFAULT_RESOLVE_DATA_OPTION; | ||
| const filteredEvent = stripEventDataRefs(event, resolveData); | ||
|
|
||
| // For run_started: include all events so the runtime can skip | ||
| // the initial events.list call and reduce TTFB. | ||
| let events: Event[] | undefined; | ||
| if (data.eventType === 'run_started' && run) { | ||
| const allEvents = await paginatedFileSystemQuery({ | ||
| directory: path.join(basedir, 'events'), | ||
| schema: EventSchema, | ||
| filePrefix: `${effectiveRunId}-`, | ||
| sortOrder: 'asc', | ||
| getCreatedAt: getObjectCreatedAt('evnt'), | ||
| getId: (e) => e.eventId, | ||
| }); | ||
| events = allEvents.data; | ||
| } | ||
|
|
||
| // Return EventResult with event and any created/updated entity | ||
| return { | ||
| event: filteredEvent, | ||
| run, | ||
| step, | ||
| hook, | ||
| wait, | ||
| events, | ||
| }; | ||
| }, | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -444,7 +444,15 @@ export function createEventsStorage(drizzle: Drizzle): Storage['events'] { | |
| }; | ||
| } | ||
|
|
||
| // Run state transitions are not allowed on terminal runs | ||
| // For run_started on terminal runs, use RunExpiredError so the | ||
| // runtime knows to exit without retrying. | ||
| if (data.eventType === 'run_started') { | ||
| throw new RunExpiredError( | ||
| `Workflow run "${effectiveRunId}" is already in terminal state "${currentRun.status}"` | ||
| ); | ||
| } | ||
|
|
||
| // Other run state transitions are not allowed on terminal runs | ||
| if ( | ||
| runTerminalEvents.includes(data.eventType) || | ||
| data.eventType === 'run_cancelled' | ||
|
|
@@ -563,6 +571,23 @@ export function createEventsStorage(drizzle: Drizzle): Storage['events'] { | |
|
|
||
| // Handle run_started event: update run status | ||
| if (data.eventType === 'run_started') { | ||
| // If the run is already running, return it without inserting a | ||
| // duplicate run_started event. This makes run_started idempotent | ||
| // for concurrent invocations: replay is deterministic, so letting | ||
| // multiple callers proceed with the same run is safe. We skip | ||
| // preloaded events here because this is a rare race-condition path | ||
| // — the runtime falls back to getAllWorkflowRunEvents(). | ||
| if (currentRun?.status === 'running') { | ||
| const [fullRun] = await drizzle | ||
| .select() | ||
| .from(Schema.runs) | ||
| .where(eq(Schema.runs.runId, effectiveRunId)) | ||
| .limit(1); | ||
| if (fullRun) { | ||
| return { run: deserializeRunError(compact(fullRun)) }; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: When the run is already More importantly, this early return happens before the event insertion, so it short-circuits the rest of the If this is intentional (i.e., it's safe for multiple invocations to proceed since replay is deterministic), a comment explaining that would help. If not, this should at minimum return the preloaded events for consistency, or throw an
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment explaining the idempotency semantics. The early return is intentional — replay is deterministic so concurrent callers proceeding with the same run is safe. Skipping preloaded events here is acceptable since this is a rare race-condition path and the runtime falls back to |
||
| } | ||
|
|
||
| const [runValue] = await drizzle | ||
| .update(Schema.runs) | ||
| .set({ | ||
|
|
@@ -1135,29 +1160,66 @@ export function createEventsStorage(drizzle: Drizzle): Storage['events'] { | |
| } | ||
| } | ||
|
|
||
| // Strip eventData from run_started — it belongs on run_created only. | ||
| const storedEventData = | ||
| data.eventType === 'run_started' | ||
| ? undefined | ||
| : 'eventData' in data | ||
| ? data.eventData | ||
| : undefined; | ||
|
|
||
| const [value] = await drizzle | ||
| .insert(events) | ||
| .values({ | ||
| runId: effectiveRunId, | ||
| eventId, | ||
| correlationId: data.correlationId, | ||
| eventType: data.eventType, | ||
| eventData: 'eventData' in data ? data.eventData : undefined, | ||
| eventData: storedEventData, | ||
| specVersion: effectiveSpecVersion, | ||
| }) | ||
| .returning({ createdAt: events.createdAt }); | ||
| if (!value) { | ||
| throw new EntityConflictError(`Event ${eventId} could not be created`); | ||
| } | ||
| const result = { ...data, ...value, runId: effectiveRunId, eventId }; | ||
| const result = { | ||
| ...data, | ||
| ...value, | ||
| runId: effectiveRunId, | ||
| eventId, | ||
| ...(storedEventData !== undefined | ||
| ? { eventData: storedEventData } | ||
| : {}), | ||
| }; | ||
| if (data.eventType === 'run_started') { | ||
| delete (result as any).eventData; | ||
| } | ||
| const parsed = EventSchema.parse(result); | ||
| const resolveData = params?.resolveData ?? 'all'; | ||
|
|
||
| // For run_started: include all events so the runtime can skip | ||
| // the initial events.list call and reduce TTFB. | ||
| let allEvents: Event[] | undefined; | ||
| if (data.eventType === 'run_started' && run) { | ||
| const eventRows = await drizzle | ||
| .select() | ||
| .from(Schema.events) | ||
| .where(eq(Schema.events.runId, effectiveRunId)) | ||
| .orderBy(Schema.events.eventId); | ||
| allEvents = eventRows.map((e) => { | ||
| e.eventData ||= e.eventDataJson; | ||
| const parsed = EventSchema.parse(compact(e)); | ||
| return stripEventDataRefs(parsed, resolveData); | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| event: stripEventDataRefs(parsed, resolveData), | ||
| run, | ||
| step, | ||
| hook, | ||
| wait, | ||
| events: allEvents, | ||
| }; | ||
| }, | ||
| async get( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking observation: Now that
runs.getis removed and the runtime always callsevents.create(run_started), the behavior changes for runs that are alreadyrunning. Previously,runs.getwould succeed and theif (status === 'pending')guard would skip therun_startedcreation. Now, every invocation attemptsrun_startedregardless of status.This works because the world implementations have an early-return for already-running runs — but it's a semantic change worth noting in the PR description. The contract is now:
events.create('run_started')must be idempotent forrunningstatus (return the run without error), not just forpending → runningtransitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout — added a comment documenting this contract:
events.create('run_started')must be idempotent for runs already inrunningstatus, not just forpending → runningtransitions.