[world] Guard hook_received against a concurrent run termination#2987
Conversation
Add a terminal-run status re-check for hook_received at the same linearization point as its event write, closing the race where the run reaches a terminal state (run_completed/failed/cancelled) between hook_received's earlier validation reads and its write. - world-local: re-read the run status immediately before the event publish, under the same in-process hookLocks lock already held for hook_received, and throw RunExpiredError if terminal. - world-postgres: wrap the status check and event insert in a single transaction, taking a FOR UPDATE lock on the run row so the insert is linearized against a concurrent terminal-transition UPDATE. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 710c9bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
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 |
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (4)b5801f2Tue, 21 Jul 2026 20:18:21 GMT · run logs
91eeac7Tue, 21 Jul 2026 19:44:27 GMT · run logs
5a4616dFri, 17 Jul 2026 18:29:14 GMT · run logs
3bbe1f8Fri, 17 Jul 2026 17:40:02 GMT · run logs
Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
| // the terminal-run guard every other event type gets earlier in | ||
| // this function (hook_received has no branch there because it | ||
| // doesn't transition the run or create an entity). | ||
| if (data.eventType === 'hook_received') { |
There was a problem hiding this comment.
[P1] Serialize this check with the event write
This re-read does not eliminate the race because terminal lifecycle writes are serialized with withRunFileLock, while this check and the later writeExclusive are outside that lock. hook_received can read running, a concurrent run_completed can acquire the run lock and write completed, and then hook_received can still publish its event. Please put the status re-read and event publication in the same withRunFileLock(effectiveRunId, ...) critical section used by terminal transitions. The new test directly makes the run terminal before calling events.create, so it verifies stale-status detection but cannot catch this check-to-write interleaving; a deterministic concurrent test should cover both lock orderings.
There was a problem hiding this comment.
Good catch — fixed in 5a4616d. The re-read is now inside withRunFileLock(effectiveRunId, ...), the same critical section run_completed/run_failed/run_cancelled use for their own terminal-transition write (via writeRunUnderLifecycleLock), and the writeExclusive publish happens inside that same lock. So hook_received either waits behind an in-flight terminal transition and observes the committed status, or completes its own check-then-publish before a transition's critical section can begin — no interleaving where both proceed.
Also added a deterministic test that manually occupies the run-file lock and asserts hook_received cannot settle until the hold is released (proving it actually queues on the shared lock, not just that it happens to see a pre-set status), plus a genuine concurrent Promise.allSettled race test that fuzzes both lock orderings.
Address review feedback: re-reading run status outside the shared withRunFileLock used by run_completed/run_failed/run_cancelled did not actually close the race, since a concurrent terminal transition could still acquire that lock and commit between the unlocked re-read and the event write. Move the check and the writeExclusive publish inside the same withRunFileLock(effectiveRunId, ...) critical section so the two are mutually exclusive with a run's terminal transition. Add a deterministic test that manually occupies the run-file lock and asserts hook_received cannot settle until the hold releases, proving it actually queues on the shared lock rather than merely observing a status set before it started. Also add a genuine concurrent race test (Promise.allSettled) fuzzing both lock orderings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lication Replaces the publish-then-rollback protocol: a published event file is immediately visible to events.list() in other processes, so deleting it after observing the terminal marker cannot undo the observation. hook_received now stages its event at a non-reader-visible path under .locks, re-checks the terminal marker, and promotes via atomic hard link into events/. Terminal transitions write the marker and then reap the staging directory before writing terminal state — the reap's unlink and the resume's link race on the same staged file, so the filesystem atomically picks a single winner and a rejected event is never visible. Also applies the guard to legacy (pre-event-sourcing) runs, which previously bypassed it entirely via handleLegacyEvent in both world-local (same staged protocol) and world-postgres (same FOR UPDATE transaction as the current-spec path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
(AI) Retrospective on why this PR took three rounds of correctness feedback, and what the root problem was. The root problem: this guard needs two-object atomicity — "check the run is not terminal" and "append the event" must be decided together — but world-local's only commit primitive is a single-file atomic create (
The fix that ended the cycle stopped trying to combine two files atomically and instead made visibility itself the linearization point: the event is staged at a non-reader-visible path, and a single atomic filesystem operation — the promote hard-link vs. the terminal transition's reap unlink, both targeting the same staged file — decides its fate. Either the event was visible before the termination proceeded, or it never becomes visible at all. Rejections only ever touch invisible paths, so there is nothing to roll back. Process takeaway: the invariant ("no |
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
The staging/promote protocol addresses the two previous findings, but there is still one local-world ordering race that can put the accepted hook after the terminal event during replay.
| // so the reap's unlink and the resume's link race on the SAME | ||
| // staged file and the filesystem decides a single winner: | ||
| // either the resume's event was already visible before this | ||
| // transition proceeded (it legitimately precedes the |
There was a problem hiding this comment.
[P1] Stamp the terminal event at the arbitration point
The promote-vs-reap protocol decides whether hook_received precedes termination, but replay order is still based on createdAt/eventId, both allocated at createImpl() entry (currently lines 681-682), before this marker/reap block. A terminal call can allocate the older timestamp/ULID and stall here; a later hook_received gets a newer timestamp/ULID and promotes before the marker, legitimately winning the filesystem arbitration. When the terminal call resumes, its event is published afterward, but events.list() sorts the older terminal createdAt first (and uses the event ID as the tie-breaker), so replay observes terminal then hook_received anyway. Please defer the terminal event's timestamp/ID allocation until after marker+reap (or otherwise derive log order from the same linearization point), and cover the case where a terminal invocation pauses after initial allocation while a later hook promotes.
There was a problem hiding this comment.
Fixed in b5801f2. You're right — acceptance was linearized but the replay-ordering key wasn't: createdAt/eventId were still allocated at createImpl() entry, so a terminal call that stalled after allocation could append with a key older than a hook that legitimately won the arbitration in the meantime.
Terminal transitions now re-mint eventId + createdAt immediately after marker + reap (mintRunDominantEventKey), choosing a key that strictly dominates every reader-visible event of the run: the timestamp is bumped past the max visible eventId's ULID timestamp when the wall clock hasn't advanced, so the minted ULID compares lexicographically greater than every visible eventId even against another process's random ULID bits (a plain "allocate later" re-stamp would still lose same-millisecond cross-process ties). Since every accepted hook is visible by the end of the reap, the terminal event is guaranteed to sort last under the (createdAt, eventId) comparator.
Covered with the interleaving you described: a test intercepts the terminal marker's writeExclusive (the transition's first durable step, i.e. a stall after entry-time allocation) and runs a full hook_received to completion inside the interception, then asserts the accepted hook replays strictly before run_completed and that the terminal event is last. Verified it fails with the re-stamp removed.
world-postgres needs no change here: it orders by eventId, but terminal events allocate theirs lazily after the conditional run UPDATE (the row-lock linearization point) from the same in-process monotonic factory — the ordering guarantee the hook_received transaction already documents.
…erminal-run-guard
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
…point events.list() sorts by (createdAt, eventId), both allocated at createImpl() entry. A terminal invocation could allocate an older key, stall before its marker write, lose the promote arbitration to a later hook_received (legitimately), and then append its terminal event with the stale key — replaying the accepted hook AFTER the terminal event. Terminal transitions now re-mint eventId/createdAt after the marker + reap, choosing a key that strictly dominates every reader-visible event of the run (bumping the ULID timestamp past the max visible one, so dominance holds against another process's random ULID bits). All accepted hooks are visible by the end of the reap, so the terminal event is guaranteed to replay last. world-postgres needs no change: terminal events allocate their eventId lazily after the conditional run UPDATE (the row-lock linearization point), the same ordering guarantee hook_received's in-transaction allocation documents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
The replay-key re-mint addresses the previous ordering finding, but the filesystem protocol still has correctness-critical error paths that are silently treated as success.
| let entries: string[]; | ||
| try { | ||
| entries = await fs.readdir(dir); | ||
| } catch { |
There was a problem hiding this comment.
[P1] Abort the terminal transition when the reap fails
This catch treats every readdir failure as an absent staging directory, and line 177 similarly ignores every unlink failure. If a hook_received has staged and passed its marker re-check, the terminal writer can commit the marker, hit an EACCES/EMFILE/I/O error here, then continue to publish the terminal state/event while the staged inode remains; when the stalled hook resumes, its hard-link promotion succeeds and becomes visible after termination. These operations are the correctness-critical half of the arbitration, not cleanup. Please ignore only ENOENT (and use the existing Windows retry behavior where applicable), while propagating other readdir/unlink failures so the terminal transition cannot proceed until the reap has actually completed.
There was a problem hiding this comment.
Fixed in 710c9bf. The reap now treats only ENOENT as benign: on readdir that means "nothing was ever staged", and on unlink it means the arbitration was already decided for that file (the resume promoted and cleaned up, or a concurrent reaper won). Every other failure propagates and aborts the terminal transition before the state write — the marker write and reap are idempotent, so the transition's retry re-runs them. Unlinks now also go through the shared withWindowsRetry helper for the transient EPERM/EBUSY cases. Only the trailing rmdir of the (now empty) staging directory remains best-effort, since a leftover empty directory has no correctness effect.
Also applied the same tightening to isRunTerminalCommitted's fs.access probe — it had the identical defect, and it is what rejects a resume that staged AFTER the reap passed, so a swallowed EACCES there could equally let an event become visible post-termination.
Tests (POSIX-only via chmod): a staged file plus an unreadable staging directory makes run_completed reject with EACCES before the terminal state write, the staged file survives, and a retry completes the transition and reaps it. Both verified to fail with the swallow-all catches restored.
| let files: string[] = []; | ||
| try { | ||
| files = await fs.readdir(path.join(basedir, 'events')); | ||
| } catch { |
There was a problem hiding this comment.
[P1] Do not silently lose the dominance scan
The new ordering guarantee depends on scanning every visible event, but this catch converts any events/ read failure into an empty log and mints from the wall clock. On a permission, descriptor-exhaustion, or I/O error that key is no longer guaranteed to dominate an already-visible accepted hook, recreating the replay-ordering bug this helper is meant to close. Catch only the expected ENOENT case; other errors need to abort the terminal write.
There was a problem hiding this comment.
Fixed in 710c9bf. The scan now swallows only ENOENT ("no events directory yet" — provably nothing to dominate) and propagates every other readdir failure, aborting the terminal transition before any state is written; the retry re-runs the scan. Covered with a chmod-based test (POSIX-only): an unreadable events/ directory makes run_completed reject with EACCES while the run state stays untouched, and a retry completes normally — verified it fails with the swallow-all catch restored.
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Approving with the expectation that the two outstanding filesystem error-handling P1 threads are addressed before merge.
The reap and the dominance scan are correctness-critical halves of the arbitration, but swallowed every readdir/unlink failure: an EACCES/ EMFILE/EIO during the reap let the terminal transition proceed with a staged inode still linkable (the stalled resume's later promote would become visible after termination), and a failed dominance scan silently minted a wall-clock key with no ordering guarantee. The run-terminal marker probe had the same defect, which guards the staged-after-reap rejection. All three now treat only ENOENT as the benign "not present" case and propagate anything else, aborting the terminal transition (or failing the resume) so a retry re-runs the idempotent step. Reap unlinks go through the shared Windows retry helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Backport PR opened against |
2 participants
Summary
hook_receivedhad no branch in the terminal-run guard (it doesn't transition the run or create an entity), so a run reaching a terminal state (run_completed/run_failed/run_cancelled) concurrently with an in-flighthook_receivedcould still get its event appended after the run was already done.world-local
The filesystem provides no multi-file transaction, so the guard is built around a single atomic operation that decides both facts at once — the event stays invisible to readers until the arbitration has committed:
.locks/runs/<runId>.terminal), then reap the run's staging directory, both before writing the terminal run state or appending their terminal event.hook_received(1) fast-path rejects if the run is already terminal, (2) stages its event at a non-reader-visible path under.locks, (3) re-checks the marker, (4) promotes the staged file intoevents/with an atomic hard link.The reap's
unlinkand the promote'slinktarget the same staged file, so the filesystem serializes them and exactly one wins: either the event was reader-visible before the terminal transition proceeded (acceptance happened-before termination), or the promotion fails and the event is never visible to any reader. There is no in-memory lock (cross-process safe) and no rollback of reader-visible state.world-postgres
hook_receivedruns inside its owndrizzle.transaction, taking aFOR UPDATElock on the run row before checking terminal status and inserting — the same linearization techniquestep_startedalready uses, adapted to guard against a concurrent terminal run transition (whose ownUPDATEtakes the same row lock).Legacy runs
Both backends route legacy (pre-event-sourcing) runs through a separate handler that previously bypassed the guard entirely. The same protection now applies there: world-local's legacy
run_cancelledcommits the marker and reaps before its state write and legacyhook_receiveduses the same stage → check → promote protocol; world-postgres's legacyhook_receiveduses the sameFOR UPDATEtransaction.Test plan
world-local: deterministic tests for each protocol step — fast-path rejection on terminal state, fast-path rejection on a marker committed by another process, rejection in the stage-to-check window, rejection when the reap wins the check-to-promote arbitration (promote returns'missing'), the accept side (a promoted event survives a subsequent termination), and the transition-side reap of a stalled other-process resume. Each verified to fail with its mechanism removed.world-locallegacy: accepts on a live legacy run, rejects on a cancelled legacy run (marker committed by the legacy cancel path), rejects on a marker-only (cross-process, state stillrunning) legacy run. Verified to fail with the legacy guard removed.world-postgres: rejection when the run row is terminal at theFOR UPDATEre-check; legacy accepts-on-live and rejects-on-cancelled. Verified to fail with the guards removed.pnpm vitest runsuites pass:@workflow/world-local(475) and@workflow/world-postgres(150, via testcontainers).pnpm typecheckandpnpm buildpass for both packages.🤖 Generated with Claude Code