Uh oh!
There was an error while loading. Please reload this page.
[core] Fix hook token reuse after dispose() (same-run and cross-run) - #2779
Merged
Conversation
- core: process hook operations per token in workflow-code order during suspension handling, so a dispose() of an earlier hook releases the token before a later same-token hook's creation is validated. A hook created and disposed within the same suspension is still created before it is disposed. Fixes the same-run recreate self-conflict (#2777). - world-local: retry the token claim when the observed claim vanished mid-check or is held by a hook whose disposal is committed / a run that is terminal or missing; never rebuild a claim from the event log for a hook whose dispose lock exists. Fixes spurious HookConflictError against an already-disposed hook during fast run-to-run token handoff (#2778). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 899a288 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)
workflow with 1 step💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
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 6, 2026 17:04
This was referenced Jul 6, 2026
karthikscale3
approved these changes
Jul 7, 2026
karthikscale3
left a comment
Contributor
There was a problem hiding this comment.
ai review: Approving — no blockers or regressions found.
What I checked:
@workflow/coresuspension handler: verified the per-token grouping preserves queue-insertion (workflow-code) order, so dispose-before-recreate and create-before-dispose orderings both hold; cross-token parallelism and the hooks-before-steps/waits ordering are unchanged.hasHookEvents(hooksNeedingCreation.length > 0) is equivalent to the oldhookEvents.length > 0. Skipping disposal for a conflicted creation is a behavior improvement (previously it attempted the dispose and swallowedHookNotFoundError). Abort processing (hooksNeedingAbort) is unaffected.@workflow/world-localclaim loop: the loop is bounded (10 attempts, ≤2×10ms sleeps), a genuinely live claim breaks out on the first iteration into the unchanged conflict path, and the own-claim dedup/recovery path is preserved (existingClaimcarried out of the loop matches the old post-failure re-read). On loop exhaustion, behavior degrades to the pre-PR single-attempt semantics rather than something worse.- Rebuild/resurrection interaction:
findLiveHookCreatedEventalready excludes terminal-run hooks (isTerminalRunCache) and now dispose-committed hooks, so a force-released claim is not resurrected by the event-log rebuild in the disposed-hook and terminal-run cases (matching the two new storage tests). - Dispose-lock keying: the lock path/naming is byte-identical to the old inline construction (backward compatible with locks already on disk), and hook correlation IDs are
hook_${ulid}so keying by hookId alone is safe — same keying the dispose path already relied on. - Tests: the new
@workflow/vitestworkbench tests are race-free becausewaitForHookexcludes hooks that already have ahook_receivedevent, so each round can only observe the next round's hook; the e2e helper's newexcludeHookIdhandles the same race for the token-lookup-based poll. CI is green including the required E2E check.
Left two non-blocking inline notes on narrow crash/race windows in the world-local force-release path.
| // The releaser is not coming. Release the stale claim and | ||
| // the hook entity it points at, mirroring the hook_disposed | ||
| // cleanup. | ||
| await deleteJSON(constraintPath); |
Contributor
There was a problem hiding this comment.
ai review (non-blocking): there's a narrow cross-process window where this force-release can lead to a transient token-uniqueness violation. Both in-flight releasers (hook_disposed handler and deleteAllHooksForRun) delete the claim file unconditionally after reading the hook entity. Sequence: releaser writes the dispose lock (or run goes terminal) and reads the hook entity, then stalls; this claimant observes the stale claim releasable 3×, deletes it, and writes its own fresh claim; the stalled releaser then executes its deleteJSON(constraintPath) — deleting the new claimant's live claim, so a third claimant can also claim the token. The 2×10ms grace period narrows this but doesn't close it. A cheap hardening would be for the releasers to re-read the claim and only delete it if it still points at their own (runId, hookId) (still TOCTOU, but the window shrinks from "stall of any length" to adjacent file ops), or to record the claim's identity here and have releasers skip deletion when the hook entity is already gone — which hook_disposed already partially does via the existingHook guard.
| WorkflowRunSchema, | ||
| tag | ||
| ); | ||
| if (!owningRun) { |
Contributor
There was a problem hiding this comment.
ai review (non-blocking): the missing-run semantics here disagree with the event-log rebuild. This treats a claim whose run file is absent as releasable, but findLiveHookCreatedEvent's isTerminalRunCache returns false for a missing run file, i.e. the rebuild treats that same hook as live and re-writes the claim. If a claim + surviving hook_created event ever exist without a run file (crash-lost run cache), the claim loop deletes the claim, the next iteration's rebuild resurrects it, and the loop ping-pongs to exhaustion — ending in the conflict branch with existingClaim = null, i.e. a durable hook_conflict with conflictingRunId: undefined, the exact symptom #2778 fixes. Unreachable today AFAICT (world-local never deletes run files), but worth aligning the two predicates — either both treat a missing run as terminal, or drop this branch.
Uh oh!
There was an error while loading. Please reload this page.
VaguelySerious
commented
Jul 7, 2026
MemberAuthor
Addressing review comment in a follow-up PR |
Contributor
Backport to This is usually an infrastructure problem (e.g. the configured AI model could not be found, an AI Gateway error, or an opencode crash) rather than a merge conflict. Check the job logs linked above for details. Once the underlying issue is fixed, re-run the Backport to stable workflow manually via |
TooTallNate
approved these changes
Jul 7, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Reviewed both halves in depth, ran the full affected suites plus the new regression tests against a live dev server. Approving.
Core (same-run, #2777): The per-token grouping is the right shape. I traced the ordering semantics:
- Queue-insertion order within a token group = workflow code order, so dispose-of-earlier flushes before create-of-later is validated, while a created-and-disposed-in-one-suspension hook still creates first. Both directions are covered by the new unit tests.
- The new event-log order (
hook_disposedbefore the successor'shook_created) also matches replay consumption order — under the old code the log recorded them inverted relative to code order, so this is strictly better for replay, not just for conflict avoidance. hasHookConflict/hasAwaitedHookCreationaccumulation reaches the same set of creations as before, andhasHookEventsis computed from the same population — no semantic drift in the V2 re-invoke signaling.- The
creationConflictedgate on dispose is correct belt-and-braces: a conflicted creation has nothing to dispose, and skipping avoids even the benignHookNotFoundErrorround-trip. - Cross-token parallelism is preserved, so unrelated hooks don't serialize.
world-local (cross-run, #2778): The claim loop's force-release is the risky part, so I audited its failure modes:
readJSONreturns null only on ENOENT (anything else throws), soisHookTokenClaimReleasablecan't misclassify a live claim due to a transient read error — a "missing" owning run is genuinely missing, and terminal status is monotonic.- Racing force-releasers and the in-flight disposer are all safe:
deleteJSONis ENOENT-idempotent, and re-claim arbitration still lands onwriteExclusive's EEXIST semantics, so exactly one claimant wins and the loser correctly conflicts against the winner's live claim. - The dispose lock as earliest-durable-disposal-marker works because the lock write already preceded the destructive deletes pre-PR — the change formalizes existing ordering rather than introducing new ordering requirements. The
findLiveHookCreatedEventlock check closing the rebuild-resurrection window is a nice catch. - I also confirmed the asymmetry with world-postgres is justified: there the hook row is the token claim and
hook_disposedreleases it via atomicDELETE ... RETURNING, so no separate-release window exists — the core fix alone covers it.
Validation: 1354/1354 core unit tests, 432/432 world-local (after full build — the 17 initial failures were unbuilt-SWC-plugin/e2e-env artifacts), 2/2 new workbench/vitest regression tests, and against a live turbopack dev server: the new hookTokenReuseLoopWorkflow e2e plus all 26 existing hook e2e tests pass. Workbench conventions followed (workflow added to workbench/example and propagated via symlink; the vitest workbench's local workflow matches its local-file convention). Changesets correctly split per package. CI: 105 pass, 1 fail = the known Benchmark Vercel (nextjs-turbopack) flake.
Two non-blocking notes:
- If the claim loop exhausts all 10 attempts via repeated vanish races (claim deleted between the exclusive-create attempt and every read), it falls through with
existingClaim = nulland still records ahook_conflictwith noconflictingRunId— the #2778 symptom, now requiring ~10 consecutive lost races to reproduce. Practically unreachable, but aruntimeLogger.warnon loop exhaustion would make it diagnosable if it ever fires. - The dispose locks in
.locks/hooks/are now load-bearing durable state (consulted by the claim path and the rebuild), not just transient mutexes. Worth a note somewhere discoverable that they must never be garbage-collected — a future ".locks cleanup" change would silently reintroduce the resurrection bug. The comment athookDisposeLockPathcovers the what; the hazard is for whoever writes a cleanup without reading it.
This was referenced Jul 7, 2026
VaguelySerious added a commit
that referenced
this pull request
Jul 7, 2026
…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>
This was referenced Jul 8, 2026
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#2777, closes#2778.
Problem
Reusing a hook token after
hook.dispose()deterministically or intermittently failed with a spuriousHookConflictErrornaming an already-disposed hook:hook_createdevents before anyhook_disposed, so recreating a hook with the same token afterdispose()was validated while the disposed hook still held the token claim — the first reuse always self-conflicted.conflictingRunId: undefined). The event-log rebuild could also resurrect a claim for a hook whose teardown was in progress.Fix
@workflow/core:handleSuspensionnow processes hook operations grouped per token, in workflow-code order: a dispose of an earlier hook flushes before a later same-token hook's creation is validated, while a hook created and disposed within one suspension is still created first. Different tokens keep processing in parallel, and hooks still process before steps/waits.@workflow/world-local: thehook_createdtoken claim runs in a short bounded loop — it retries when the observed claim vanished mid-check, and treats a claim as vacant when its hook's disposal is committed (dispose lock exists) or its owning run is terminal/missing, releasing the stale claim if the in-flight releaser never finishes. The event-log rebuild (rebuildLiveHookByTokenFromEventLog/hooks.get*) no longer considers a hook live once its dispose lock exists. Live claims still conflict exactly as before.