Uh oh!
There was an error while loading. Please reload this page.
[world-local] Fix resumeHook racing hook disposal being journaled after hook_disposed - #2808
Merged
Conversation
A hook_received write only validated that the hook entity exists, and was not serialized against the hook_disposed write for the same hook. A resume that passed its existence check before the disposal began could therefore be journaled after hook_disposed — an order that makes every subsequent replay of the owning run diverge at that event and escalate to CorruptedEventLogError. hook_received and hook_disposed now share the per-hook in-process lock with hook_created, and the hook_received write re-validates the durable dispose lock (both at acceptance and immediately before the event append), rejecting with HookNotFoundError exactly like a resume that arrived after teardown finished. Closes#2781 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 0e77b7d 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 |
Contributor
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
VaguelySerious
marked this pull request as ready for review
July 7, 2026 21:51
…tall Both in-flight releasers (the hook_disposed handler and the terminal-run deleteAllHooksForRun cleanup) deleted the claim file unconditionally after reading the hook entity. A releaser stalled between those operations could outlive a claimant force-releasing its stale claim, and its deferred delete would then destroy the new claimant's live claim — transiently breaking token uniqueness (a third claimant could claim the token too). Releasers now re-read the claim and delete it only if it still points at their own (runId, hookId). Still TOCTOU, but the window shrinks from "a stall of any length" to adjacent file ops; a claim owned by someone else is left for the claimant-side force-release path to reap. Addresses post-merge feedback on #2779. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
karthikscale3
approved these changes
Jul 7, 2026
karthikscale3
left a comment
Contributor
There was a problem hiding this comment.
Reviewed and verified the fixes hands-on. Approving.
What I checked:
- Fix 1 mechanism: the per-hook in-process lock now wraps the entire
createImpl()for all three hook lifecycle events, so the resume's "validate → append" is atomic with the disposer's "dispose lock → entity delete → append" within an instance; the lock key (${runId}-${correlationId}.hook) matches on both sides. The disposer's whole sequence runs insidecreateImpl()with no nestedevents.create— no self-deadlock. Cross-instance, the durable dispose-lock re-check at acceptance + immediately before the append narrows the window to the single event write, as documented. - Fix 2: the ownership match on
(runId, hookId)is safe for normal releases — the claim writer has always persisted both fields — and a skipped release is reaped by the claimant-side force-release loop, so tokens still free up. - Counterfactual: reverted
events-storage.ts/hooks-storage.ts/helpers.tstomainwhile keeping the new tests — the mid-teardown acceptance test and both claim-takeover tests fail exactly as claimed, and pass again on the branch. - Suites: full
@workflow/world-local438/438 locally; CI fully green (103 checks, incl. the usually-flaky webpack lane).
Non-blocking observations:
- The 20-round race-loop test passed even with the fix reverted on my machine — it's a soak, not a reliable regression guard; the deterministic mid-teardown test is the real one.
- Corrupt (unparseable) claim files are no longer deleted by any path: the releaser now skips them (correctly — ownership is undeterminable), and the claimant loop's force-delete is only reachable with a parsed claim (
readHookTokenClaim→ null →continue), so an unreadable claim can block its token indefinitely. Very low likelihood; a "unparseable after N observations → delete" fallback in the claimant loop would close it if ever seen in practice. - Nit:
hook_createdstill builds the claim path inline (~L1631) instead of using the newhookTokenClaimPath()helper — worth unifying so the layout can't drift.
TooTallNate
approved these changes
Jul 7, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Reviewed both parts against the #2779 machinery. Approving.
Part 1 (hook_received vs hook_disposed, #2781): The two-layer approach is right: the per-hook in-process lock makes resume-vs-dispose atomic within a storage instance, and the durable dispose-lock re-validation — at acceptance and immediately before the event append — narrows the cross-instance window to the single event write. The comments are honest that this is a narrowing rather than full elimination; that matches the module's existing convention (on-disk state as source of truth, in-process locks as fast path), and going further would require holding an fs-level mutex across validate+append, which isn't worth it for this world. Rejecting with HookNotFoundError gives resumes that lose the race the same surface as resumes arriving after teardown — correct and unobservable to callers as a new state.
Part 2 (guarded claim release): Good catch — this closes a gap in the #2779 force-release design I didn't flag in my review: a releaser stalled between its entity read and claim delete could destroy the next claimant's live claim. The guarded release direction is right in both failure modes: a claim that's missing/unreadable/foreign is left alone (never delete what you can't verify), and genuinely stale debris still gets reaped by the claimant-side force-release. Both releasers (hook_disposed handler and deleteAllHooksForRun) are covered. The remaining read→unlink window is honestly documented and only reachable via a second takeover within adjacent fs ops — proportionate for this world.
I also checked the hookTokenClaimPath helper against the hook_created claim-write site: both use path.join with identical segments, so there's no absolute/relative path-construction mismatch (a hazard this module has hit before).
Verified locally: full world-local suite passes including the 4 new tests (both releaser-guard takeover tests, the mid-teardown acceptance rejection, and the resume-vs-dispose race loop asserting the journal-order invariant). CI fully green.
One non-blocking nit: the hook_created claim site still builds its constraintPath inline — now that hookTokenClaimPath exists, unifying on the helper would remove the last duplicated construction of that path.
Contributor
No backport to Both fixes in this commit build directly on the #2779 durable dispose-lock and force-release/claim infrastructure that exists only on To override, re-run the Backport to stable workflow manually via |
VaguelySerious added a commit
that referenced
this pull request
Jul 8, 2026
Address #2808 review: - The claimant loop now force-deletes a claim file that persists but never parses (after a few observations) so a corrupt/orphan claim can't block its token forever — the releaser correctly leaves such files alone, so nothing else reaped them. A live hook's claim is still rebuilt from the event log, so reaping a corrupt claim can't steal a token from a live hook. - hook_created now builds the claim path via hookTokenClaimPath() instead of inline, so the layout can't drift. - Drop the 20-round resume-vs-disposal soak test: both orderings are valid so it passed even with the fix reverted (a soak, not a regression guard). The deterministic mid-teardown test remains the real guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 8, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#2781. Also addresses the post-merge feedback on #2779 (guarded claim release).
1.
hook_receivedjournaled afterhook_disposed(#2781)A
hook_receivedwrite only validated that the hook entity exists, and was not serialized against thehook_disposedwrite for the same hook. A resume that passed its existence check before the disposal began could be journaled afterhook_disposed— once that order is in the log, every replay of the owning run diverges at the post-disposalhook_receivedand the run escalates toCorruptedEventLogError.hook_receivedandhook_disposednow share the per-hook in-process lock previously taken only byhook_created, making the resume's "validate, then append" atomic with the disposer's "dispose lock → entity delete → append" within a storage instance.hook_receivedwrite re-validates the durable dispose lock from [core] Fix hook token reuse after dispose() (same-run and cross-run) #2779 (at acceptance, and once more immediately before the event append, narrowing the cross-instance window to the single event write), rejecting withHookNotFoundError— the same surface as a resume that arrives after teardown finished. Acceptance now observes the same order replay will see.2. Guarded token claim release (#2779 follow-up)
Both in-flight releasers (the
hook_disposedhandler and the terminal-rundeleteAllHooksForRuncleanup) deleted the token claim file unconditionally after reading the hook entity. A releaser stalled between those operations could outlive a claimant force-releasing its stale claim, and its deferred delete would then destroy the new claimant's live claim — transiently breaking token uniqueness. Releasers now re-read the claim and delete it only if it still points at their own(runId, hookId); a claim owned by someone else is left for the claimant-side force-release path to reap.Tests
hook_receivedis never journaled afterhook_disposedand that a rejected resume leaves no event behind.(runId, hookId)and assert the release skips it; both fail without fix 2.🤖 Generated with Claude Code