fix(world-postgres): make step creation atomic - #3575
Conversation
🦋 Changeset detectedLatest commit: 3fb90ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@shin4141 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
b9727fe to
82be579
Compare
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| .onConflictDoNothing() | ||
| .returning(); | ||
| if (!stepValue) { | ||
| throw new EntityConflictError( |
There was a problem hiding this comment.
AI Review: Blocking
Keying the conflict on the step row instead of on the step_created event deletes the only recovery path that exists today for an already-orphaned step row, and replaces it with a corrupted event log.
On the base commit, a zero-row onConflictDoNothing() fell through to the event insert, so an orphan (row committed, event lost) was completed by the next step_created. This branch throws before the event insert, so the orphan can never be completed.
Measured against an isolated Postgres 15: seed one orphan row for stepId = step_orphan, then send the matching step_created, then the consumer's plain step_started.
step_created |
resulting event log for that step | |
|---|---|---|
base (b0adb50) |
resolves | [step_created, step_started] |
| this PR | EntityConflictError: Step "step_orphan" already created |
[step_started] |
Both runtime engines swallow EntityConflictError on step_created (quickjs-entrypoint.ts: "Concurrent invocation wrote it first — the message is already out"; suspension-handler.ts: "Step already exists, continuing") and dispatch the step regardless. The consumer's non-lazy step_started then succeeds, because getStepForValidation finds the orphan row. So the run does not error: it produces a step whose log has step_started and no step_created, permanently. The replay consumer re-derives that step as uncreated on every replay while the step also executes.
That is strictly worse than the state #3081 describes. The population the issue is about is the pre-existing orphans (it reports 15 accumulated over 11 days on one database). After this change none of them can be drained, and every retry against one corrupts its run's log.
The issue's suggested fix (2) is the missing half, and the hook path a few hundred lines up is already the precedent: on a zero-row claim, look up whether step_created exists for (runId, correlationId). Present → EntityConflictError, as here. Absent → orphaned partial write, so skip the row insert and complete the event insert in the same transaction. That closes the new window and drains the existing orphans, which is what makes this a fix for the reported issue rather than a change of failure mode.
Two smaller points in the same area:
- The error message loses the run id. Base produced
step_created for correlationId "X" already exists in run "Y"via the23505translation below; this producesStep "X" already created. DuplicatecorrelationIds are per-run, so the run id is the useful half for an operator reading self-hosted logs. wait_createdstill has exactly the shape this branch introduces: non-transactional insert,EntityConflictErrorkeyed on the entity row, no event-existence check. step_created entity row and event are written non-transactionally — a crash between them wedges the run permanently #3081 names it andattr_setas sharing the pattern. Not this PR's scope, but it means the codebase now has one path that is atomic-and-unrecoverable and one that is neither.
| let inserted: Awaited<ReturnType<typeof insertEventRow>>; | ||
| if (data.eventType === 'step_created') { | ||
| const eventData = data.eventData; | ||
| const created = await drizzle.transaction(async (tx) => { |
There was a problem hiding this comment.
AI Review: Note
Wrapping the fan-out write in an explicit transaction has a width-dependent cost. N concurrent step_created calls on one run, isolated Postgres 15, pg pool max: 30, 3 independent runs of 15 rounds each per side, first round of each run discarded, n=42 per cell:
| width | base median | this PR median | factor |
|---|---|---|---|
| 1 | 2.0 ms | 1.8 ms | 0.90x |
| 5 | 3.8 ms | 4.7 ms | 1.24x |
| 10 | 6.3 ms | 9.6 ms | 1.52x |
| 20 | 16.1 ms (10.9–35) | 36.5 ms (19.7–90.9) | 2.27x |
Width 1 is unchanged, so this is not the BEGIN/COMMIT round trips. I read it as contention amplification, though I have not instrumented it to prove the mechanism: INSERT … ON CONFLICT DO NOTHING blocks on a conflicting uncommitted row, so the window in which a concurrent slot attempt stalls grows from a single autocommit statement to event insert → COMMIT. Each stalled writer that returns zero rows re-enters insertEventRow's retry loop (SLOT_INSERT_BASE_DELAY_MS, doubling past SLOT_INSERT_IMMEDIATE_ATTEMPTS), so the chain lengthens faster than linearly in width.
The existing suite cannot surface this: the harness pool is max: 1, so every "concurrent" case in storage.test.ts serialises at the driver before it reaches Postgres.
Worth deciding explicitly rather than by accident, since fan-out is the hottest events.create path in this world. If the cost is unwelcome, a single statement is atomic without an explicit transaction and keeps the blocking window at one statement:
WITH s AS (
INSERT INTO workflow.workflow_steps (...) VALUES (...)
ON CONFLICT DO NOTHING RETURNING step_id
)
INSERT INTO workflow.workflow_events (...) SELECT ... FROM sThe trade-off is that the slot-retry loop then re-runs the step insert too, so distinguishing "row conflict" from "slot retry" needs a follow-up read.
| stepName: 'test-step', | ||
| input: new Uint8Array(), | ||
| }) | ||
| ).rejects.toThrow(); |
There was a problem hiding this comment.
AI Review: Nit
rejects.toThrow() accepts any error. It happens to be the right one today, but it would also pass if the code threw before reaching the event insert at all, which is the failure mode worth guarding here. Asserting the forced failure specifically (rejects.toThrow(/forced step_created event insert failure/)) plus "no step_created event landed" alongside the existing row assertion would make the test prove atomicity rather than just absence.
|
@shin4141 See review above. Also, all commits on a PR must have a signature in order for us to merge it. Please squash+sign+force push after addressing comments |
82be579 to
75fffb4
Compare
Still not passing |
Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
75fffb4 to
3fb90ec
Compare
|
Thanks — the commit is now cryptographically signed and GitHub reports it as Verified. I preserved the tree unchanged and force-pushed the single signed commit. New head: |
Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
|
Backport PR opened against |
2 participants
What
step_createdevent in one transaction@workflow/world-postgresWhy
step_createdpreviously committed theworkflow_stepsrow before attempting theworkflow_eventsinsert. If the process or database failed between those writes, replay had no creation event while the exactly-once entity claim remained. Subsequent replay could only rediscover the same conflict and leave the run wedged.This fixes the
step_createdpartial-write window reported in #3081 without changing the public event contract or expanding the repair to other event types.Root cause and impact
The entity mutation and its replay evidence used separate autocommit statements. The repair moves only this pair behind a single transaction. A failed event write now rolls the entity claim back; a successful call still returns the same step/event result and preserves dense event slots.
Checks
{ stepId: 'step_partial_write' }inworkflow_stepspackages/world-postgres/test/storage.test.ts: 141 passed against an isolated PostgreSQL 17.10 database with repository migrations@workflow/world-postgresbuild: passed@workflow/world-postgrestypecheck: passed@workflow/world-postgrespatch detectedThe repository-standard Testcontainers spec suite is left to CI because Docker is not available in the local execution environment.