[world] Guard hook_received against a concurrent run termination#2987
[world] Guard hook_received against a concurrent run termination#2987VaguelySerious wants to merge 2 commits into
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: 5a4616d 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 (1)3bbe1f8Fri, 17 Jul 2026 17:40:02 GMT · run logs
Avg deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body execution · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (time outside step bodies, client start → last step body exit) · SL: stream latency (first chunk write → visible to the reader) Scenarios — stream: one step that streams chunks back to the client; no hooks, so the run stays in turbo mode · hook + stream: registers a hook before the same streaming step, which exits turbo mode · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges 🟢/🔴 mark percentiles within/above target. 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 TTFS/WO compare client vs deployment clocks and SL compares the step runner’s clock vs the client’s (NTP-synced in CI). WO ends at the last step body exit, the closest observable proxy for the final step-completion request. |
🧪 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>
VaguelySerious
changed the title
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.hookLockslock already held forhook_received, and throwRunExpiredErrorif terminal — mirroring the existing last-instanthook_disposed-vs-hook_receivedordering check right above it.hook_receivedpreviously fell through to the generic, unguarded events INSERT. It now runs inside its owndrizzle.transaction, taking aFOR UPDATElock on the run row before inserting — the same linearization techniquestep_startedalready uses against a concurrent terminal step event, adapted here to guard against a concurrent terminal run transition (whose own conditionalUPDATEtakes the same row lock).Test plan
world-local: added a "linearization guard" test that writes a terminal run status directly to disk (bypassing the run-completion hook cleanup) so the hook still exists, then confirmshook_receivedis rejected withRunExpiredError. Verified it fails without the fix.world-postgres: added the equivalent test, updating therunsrow directly viadrizzle(bypassing the run-completion cleanup) so the hook row still exists, then confirmshook_receivedis rejected withRunExpiredError. Verified it fails without the fix.HookNotFoundErrorinstead, since terminal-run transitions already delete all hooks for the run.pnpm vitest runsuites pass for both@workflow/world-local(216 tests) and@workflow/world-postgres(119 tests, via testcontainers).pnpm typecheckandpnpm buildpass for both packages.🤖 Generated with Claude Code