fix(core): close the replay-engine determinism gaps (barrier retirement, quiescence, buffered-hook claim ordering) - #3196
fix(core): close the replay-engine determinism gaps (barrier retirement, quiescence, buffered-hook claim ordering)#3196pranaygp wants to merge 15 commits into
Conversation
🦋 Changeset detectedLatest commit: 3275f4f The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests📦 Local Production (1 failed)vite-stable (1 failed):
📋 Other (1 failed)e2e-local-dev-nest-stable (1 failed):
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
❌ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 422746ms → this run 1395421ms (Δ +972675ms, +230%) 1020 steps (queue-hop) Cumulative STSO time: main 8176ms → this run 41090ms (Δ +32914ms, +403%) 📜 Previous results (6)6f1d3deFri, 31 Jul 2026 23:59:38 GMT · run logs
ca8a316Thu, 30 Jul 2026 23:51:33 GMT · run logs
b397eeaThu, 30 Jul 2026 22:53:48 GMT · run logs
b58fdcfThu, 30 Jul 2026 21:19:11 GMT · run logs
dc0d25eThu, 30 Jul 2026 16:59:59 GMT · run logs
51de584Wed, 29 Jul 2026 22:08:53 GMT · run logs
ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: 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) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) 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) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 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 |
There was a problem hiding this comment.
Pull request overview
Adds several new test-only suites in packages/core to act as executable documentation for known replay-engine determinism gaps (barrier idle collapse/starvation and buffered hook claim ordering), plus a passing characterization suite for below-watermark event divergence. This helps keep a stable inventory of repros/expected-fail cases so future fixes can flip it.fails back to it and regressions become visible.
Changes:
- Add expected-fail suites reproducing delivery-barrier idle collapse and idle starvation behaviors via
private.tsprimitives. - Add an end-to-end expected-fail suite reproducing buffered-hook claim-time deferral causing step-vs-hook ordering divergence across varying hop counts.
- Add passing characterization tests showing how below-watermark events can yield an irrecoverably divergent log.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/delivery-barrier-idle-starvation.test.ts | New expected-fail suite documenting idle-safety-net starvation under unrelated delivery traffic. |
| packages/core/src/delivery-barrier-idle-collapse.test.ts | New expected-fail suite documenting premature barrier retirement (“idle collapse”) and overtakes. |
| packages/core/src/buffered-hook-claim-ordering.test.ts | New expected-fail end-to-end repro for buffered-hook claim-time ordering causing step delivery to overtake earlier-in-log hook/wait. |
| packages/core/src/below-watermark-event-divergence.test.ts | New passing characterization suite showing divergence when an event commits below the reader’s cursor/watermark. |
| .changeset/spicy-impalas-battle.md | Empty changeset to satisfy changeset requirements for a test-only PR. |
Comments suppressed due to low confidence (1)
packages/core/src/delivery-barrier-idle-starvation.test.ts:11
- This comment points to
scheduleWhenIdleatprivate.ts:321-336, but the function currently lives elsewhere inprivate.ts. To keep this suite as durable documentation, it’s better to avoid hard-coded line ranges here too.
* `scheduleWhenIdle` (`private.ts:321-336`) is an unbounded poll:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * A barrier registered by `registerDeliveryBarrier` (`private.ts:266-299`) has | ||
| * exactly two retirement paths: `markDelivered()`, called when the delivery | ||
| * reaches workflow code, and the idle safety net armed at registration — | ||
| * `scheduleWhenIdle(ctx, finish)` at `private.ts:296`. For a delivery the | ||
| * workflow never observes (its branch was not taken, or the run is suspending), |
There was a problem hiding this comment.
Fixed in 977cd33 — every private.ts / step.ts / hook.ts / sleep.ts line reference in the four new suites is now a symbol reference (awaitEarlierDeliveries, scheduleBarrierRetirement, "the step_completed branch of step.ts", and so on). They were already stale by the time you read them, and the fix in this PR moved those lines again, so the general point holds regardless of the specific numbers.
Event Log Race ReproNo result file was produced by the latest repro job. Run History
|
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| // The next payload arrives, then the previous one's slot ends: the counter | ||
| // dips to 1 but never to 0. | ||
| ctx.pendingDeliveries++; | ||
| ctx.pendingDeliveries--; |
There was a problem hiding this comment.
AI Review: Blocking
This harness does not model what the file comment claims, and the suite's central empirical claim does not survive an end-to-end check.
The comment above startPokeStorm says "the next payload's hydration begins before the previous one's slot ends, so pendingDeliveries never touches zero." The code does not do that. Lines 98-99 are ctx.pendingDeliveries++ immediately followed by ctx.pendingDeliveries-- — a no-op pair. The counter is held above zero entirely by the single unmatched ctx.pendingDeliveries++ on line 90, which is never released until stopStorm(). So what is under test is a permanently unreleased delivery (a leak, or a hydration that never completes), not overlapping delivery traffic.
That distinction decides whether the suite documents a real defect. I built the cited production shape against the real machinery — N buffered payloads for a hook the workflow never reads, plus a wait_completed that per DEFER_BEHIND.wait gates on every one of those unclaimed hook barriers, whose only retirement path is the idle net:
pokes=1 elapsed=5ms liveBarriers=0 pendingDeliveries=0
pokes=8 elapsed=4ms liveBarriers=0 pendingDeliveries=0
pokes=64 elapsed=4ms liveBarriers=0 pendingDeliveries=0
pokes=228 elapsed=6ms liveBarriers=0 pendingDeliveries=0
228 payloads — the number from the live observation in the PR description — retires every barrier in 6ms with the gated wait delivered. Replay-log traffic alone cannot starve the net, which is consistent with the execution model: each poke is a separate invocation replaying a static log, so per-invocation traffic is finite and every increment has a matching decrement in a finally.
So the "zero progress for 3m19s under 228 pokes" attribution is not established by this suite. The only way to pin pendingDeliveries the way line 90 does is a hydration that never completes — which is a hang in its own right, with barrier non-retirement as a symptom rather than the cause. I'd drop this suite until that run is traced to an actual unreleased increment, and then repro that.
Happy to share the end-to-end harness.
There was a problem hiding this comment.
You are right about the harness, and the fix is in 977cd33.
Lines 98-99 were a no-op pair and the counter was held up entirely by the unmatched increment, so the suite modelled a stuck hydration rather than overlapping traffic — a hang in its own right, as you say. startPokeStorm now overlaps genuinely: each poke increments, schedules the next poke, and only then schedules its own release, so the counter oscillates between 1 and 2 with every increment matched. The control asserts both halves — lowWaterMark() > 0 while running, and pendingDeliveries === 0 after stop() — so a future edit that reintroduces a leak fails the test rather than passing on it.
The attribution is walked back too. The header now says the 3m19s observation is consistent with the mechanism but not established by it, and that pinning that run needs its own trace; the deadline is justified as a bound on a loop that provably has none. Your 228-payload measurement is the reason — a finite burst drains, so the claim can only ever be about traffic that is sustained while the barrier waits.
I did keep the suite rather than dropping it, for two reasons. The mechanism it pins is real and now stated exactly (no escape while consumption keeps the counter above zero), and scheduleWhenIdle also gates WorkflowSuspension, where an unbounded poll means a dormant run rather than a slow one. Your fan-out shape is in the suite now as a liveness guard — see the reply on the collapse thread.
| ).toBe(true); | ||
| }); | ||
|
|
||
| it.fails('bounds scheduleWhenIdle rather than polling without a deadline', async () => { |
There was a problem hiding this comment.
AI Review: Note
This one is true by inspection of scheduleWhenIdle — the poll has no deadline, that is visible in five lines of source. As a test it asserts a design change (add a deadline) that hasn't been agreed, rather than reproducing a defect. it.fails here reads as "this is a bug main has" when it is really "we propose this API grow a bound."
There was a problem hiding this comment.
Agreed as written, and the framing has changed: the deadline is no longer a proposal, it is in this PR (977cd33), and it is narrower than what the test originally implied. It bounds the unarmed path only — an unclaimed buffered hook payload. An armed barrier is never retired by the net on any schedule, because a timer that can retire a committed delivery is the collapse defect at a longer timescale (production hydration is 10-500ms; the tick budget is tens of ms).
The scheduleWhenIdle deadline in the third test stands on its own ground: that function schedules WorkflowSuspension, so an unbounded poll is a dormant run, not just a slow one.
| // FAILS on `main`: `scheduleWhenIdle(ctx, finish)` at `private.ts:461` ran | ||
| // `finish()` for a delivery that is still in flight, so nothing consumed | ||
| // from here on can be ordered behind it. | ||
| expect(barriers.has(1)).toBe(true); |
There was a problem hiding this comment.
AI Review: Blocking
This assertion (and the one at line 253) asserts the exact inverse of a deliberate, documented invariant. step.ts:281-283, on why the counter is released inside the hydration slot rather than after the detached defer:
pendingDeliveriesis likewise released inside the slot, before the detached defer, soscheduleWhenIdlecan still reach idle and retire the barriers this deferral may be waiting on.
Retiring a barrier that a deferral is parked on is the deadlock-avoidance tradeoff, not an oversight. "A barrier stays registered while its delivery is parked in awaitEarlierDeliveries" is precisely the property that comment says must NOT hold.
To be clear about what I am and am not disputing: the window is real and reachable, and the PR under-sells that by not demonstrating it. I instrumented registerDeliveryBarrier to report idle retirements of barriers with a deferral actively parked on them, and the existing green suite hits it in a real replay:
IDLE-RETIRE-WITH-PARKED-WAITERS idx=4 kind=step waiters=1 laterLive=1
IDLE-RETIRE-WITH-PARKED-WAITERS idx=5 kind=step waiters=1 laterLive=1
(from delivery-barrier-coverage.test.ts → "delivers the earlier step_completed first when only it defers"; the other 40 hits in that file come from the pre-existing synthetic scan-cost test and should be discounted). So this is not a fake-context artifact. The harness itself is faithful — I checked deliverStepResult and deliverLater line by line against step.ts:284-330 and sleep.ts:102-130, including sleep never touching the counter.
The problem is what the two registry-membership assertions commit main to. Merged as it.fails, they say "bug, flip on fix." Someone flipping them green by holding barriers until real delivery reintroduces the deadlock the net exists to prevent, and nothing in this PR guards that direction — the starvation control only covers retirement once traffic stops, not that an abandoned delivery can't wedge a later one.
Two concrete asks:
- Reframe tests 1 and 3 as a documented design tension (passing characterization of current behavior, or prose) rather than expected-fail defects. Tests 2 and 4 are the substantive ordering claims and don't have this problem — keep those as-is.
- Add a liveness test in the opposite direction before any of these get flipped: many unclaimed hook barriers gating a later delivery, asserting the run still progresses. I have one that passes on main today (6ms for 228 barriers) and would go red on a naive "keep barriers registered" fix.
There was a problem hiding this comment.
This was the most useful comment on the PR — it is the reason the fix has the shape it does. 977cd33.
On the invariant: you are right that step.ts:281-283 documented "idle may retire the barriers this deferral is waiting on" as a deliberate tradeoff, and right that a naive "hold barriers until real delivery" reintroduces the deadlock. The resolution is not to hold everything, it is to split the two cases. The net now retires unarmed barriers only — an unclaimed buffered hook payload, the one delivery that can be abandoned at the root. Every other barrier is committed: a wait completion, a step result, a claimed payload and an abort each call markDelivered() from a chain that runs unconditionally, so the only thing that can hold one up is an earlier unclaimed payload, and retiring that releases the chain in log order. I audited all five call sites against that. The comments you quoted are updated in the same commit — they described what the net used to do, and leaving them would have left the code contradicting itself.
Your two asks, both taken:
- Tests 1 and 3 are no longer "expected-fail defects" in tension with a documented rule — they assert the rule the engine now follows, and the rule is stated where you would look for it (
scheduleBarrierRetirement, and the INVARIANT onregisterDeliveryBarrier). - The liveness test is in:
delivery-barrier-idle-starvation.test.ts→ "drains a fan-out of unclaimed payloads gating a later delivery" — 228 unclaimed payloads gating a later wait, asserted to drain within a handful of ticks (not one per barrier, and nowhere near the abandon deadline). It passes onmainand here, which is exactly the guard you asked for.
I also added the guard in the third direction, which your comment implies but does not name: a case where hydration is held open well past the abandon deadline, asserting the armed barrier survives and a later delivery still orders behind it. Without it, "the deadline does not apply to committed deliveries" would have been unguarded — every other case in the file hydrates for less than a tick, which is the #3139 hop-count trap in a new costume.
| } | ||
|
|
||
| describe('delivery-barrier idle safety net vs. in-flight deliveries', () => { | ||
| it.fails('keeps a barrier registered while its delivery is parked in awaitEarlierDeliveries', async () => { |
There was a problem hiding this comment.
AI Review: Note
it.fails passes on any throw, so these six markers will stay green through harness breakage as well as through the defect. Change registerDeliveryBarrier's signature and all of them throw TypeError and keep reporting "expected fail" indefinitely — which is the failure mode that matters most for a suite whose entire purpose is to be a durable inventory.
buffered-hook-claim-ordering.test.ts:271-278 already solves this: it checks the outcome explicitly and rethrows carrying the real message, so a harness break is distinguishable from the documented failure. Worth applying that pattern to the six markers here and the two in the starvation suite.
Separately: it.fails already carries two incompatible meanings in this package — hook-sleep-interaction.test.ts:816 is marked KNOWN-INVALID (kept as it.fails), i.e. "this scenario is wrong and will never be fixed." A reader hitting one of these markers cannot tell "flip on fix" from "invalid, ignore" without reading the prose. A shared naming convention in the test title would help.
There was a problem hiding this comment.
Moot as of 977cd33 — all 12 markers are plain it now, so nothing in these suites passes by throwing.
The underlying point is worth keeping on the record though: it.fails is indistinguishable from harness breakage, and this package already overloads it with the KNOWN-INVALID cases in hook-sleep-interaction.test.ts. The one place the explicit-outcome pattern survives is buffered-hook-claim-ordering.test.ts, which still rethrows the real message rather than asserting a bare throw — that is now a readability win rather than a marker workaround.
|
|
||
| describe('buffered hook payload vs. a later step result', () => { | ||
| for (const extraHops of [0, 1, 2, 4, 8, 16]) { | ||
| it.fails(`keeps the recorded ULID allocation with ${extraHops} extra step-branch hops`, async () => { |
There was a problem hiding this comment.
AI Review: Note
No change requested — recording independent validation, since this is the suite I'd most want to survive review.
Mechanism confirmed as described. Instrumenting awaitEarlierDeliveries to dump the deferral set and the live registry:
DEFER idx=5 kind=wait deferralSet=1 registry=4:hook(unarmed),5:wait
DEFER idx=6 kind=step deferralSet=0 registry=4:hook(unarmed),5:wait,6:step
The step at index 6 computes an empty deferral set with two live earlier barriers present — index 4 skipped as unarmed, index 5 skipped because resolvesOnItsOwn is transitively false through it. Exactly the two-rule interaction in the header, at the mechanism level, not just the error level.
I also tried to falsify the ordering framing, since the obvious objection is that any log with a buffered payload racing a concurrent step might diverge, which would make the log order decorative. Mirroring the log so step_completed precedes wait_completed and afterStep legitimately draws ULIDS[3]:
✓ replays the stepFirst log without divergence
× replays the waitFirst log without divergence → ReplayDivergenceError at evnt_7
Order-sensitive. The engine unconditionally lets the step result win, so it reproduces exactly one of the two possible committed logs and corrupts the other. That plus the fact that the log is one a correct engine would itself produce makes this a genuine determinism defect rather than a constructed input. Consider adding the mirrored log as a passing control in this file — it's cheap and it pins the claim against a fix that just flips the bias.
There was a problem hiding this comment.
Thanks for the independent instrumentation — the deferralSet=0 registry=4:hook(unarmed),5:wait dump is the mechanism at exactly the level the header claims, and it matches what the fix targets: the transitive skip is gone (977cd33), so a step gates on an earlier armed wait or hook whatever that delivery is itself waiting for, and only the unclaimed payload is skipped.
The mirrored log is in as a passing control, at 0/4/16 hops: buildEventLog(stepFirst) emits the interleaving where step_completed precedes wait_completed and afterStep legitimately draws ULID[3]. It passes on main and here, so the waitFirst assertions can no longer be satisfied by flipping the bias toward hooks — which was your point, and it is the control I would have most wanted too.
| expect(boundStepName(ctx, ULIDS[2])).toBe('afterA'); | ||
| }); | ||
|
|
||
| it('binds the same ULID to afterB when that event is missing from the view', async () => { |
There was a problem hiding this comment.
AI Review: Note
This test locks in behavior that isn't a contract. "Binds the contended ULID to afterB when the event is missing from the view" is what the engine happens to do while it has no way to notice the hole. A fix that detects the gap and refuses to proceed — which is the direction the count-guard work points — turns this red for a good reason, and the next person has to reconstruct from the prose whether that's a regression. Worth saying so in the test name or dropping it; tests 1 and 3 carry the argument on their own.
Also on the header: it measures the postgres world and then generalizes to "a World that mints event IDs client-side (ULID) but reads them back with ORDER BY id + WHERE id > cursor." Whether world-vercel's read path has that same shape is what decides if this documents a production class or a world-postgres class, and the file doesn't say. Please state it either way — it changes how much weight the storm forensics in the PR description can carry.
There was a problem hiding this comment.
Both taken, in 977cd33.
Test 2 is renamed to binds the same ULID to afterB when that event is missing from the view (characterization) with a comment saying outright that a guard which detects the gap and refuses to proceed would turn it red for a good reason — a fix, not a regression.
On scope, the answer is definite and it is not a world-postgres class. world-vercel reads through workflow-server, which stores events in DynamoDB keyed by the event ULID and paginates with an eid:<eventId> cursor: between({eventId: cursor}, {eventId: MAX_EVENT_ID}) on the byWorkflowRunId index — an ID-ordered range strictly after the cursor, the same shape. The IDs are minted per server instance (EventId.make()), and the server documents the residual gap itself, in the comment on its monotonic factory: it "only fixes intra-instance ordering. Cross-instance ordering for the same run still depends on wall-clock agreement; an append-tail fence on the run is a separate follow-up." Two instances appending for one run is the ordinary case under the storm repro. That is now in the suite header, and it is why the fence in workflow-server#670 matters rather than being postgres-only hygiene.
| const FIXED_TIMESTAMP = 1753481739458; | ||
| const RESUME_AT = new Date(FIXED_TIMESTAMP + 5_000); | ||
|
|
||
| function setupWorkflowContext(events: Event[]): WorkflowOrchestratorContext { |
There was a problem hiding this comment.
AI Review: Nit
setupWorkflowContext, deterministicUlids, replay and event are duplicated verbatim between this file and below-watermark-event-divergence.test.ts, and near-verbatim in three existing suites. That's ~150 lines per copy of a harness that has to stay in sync with WorkflowOrchestratorContext — and it already has to, since every field added to that interface has to be threaded through each copy. Worth extracting to a shared test helper while there are only five copies.
There was a problem hiding this comment.
Agreed, and deferred rather than declined — it is real duplication and it does have to track WorkflowOrchestratorContext (this PR added two fields to that interface and had to thread one of them through the copies, which is your argument made concrete).
Not doing it here: extracting it edits three currently-green suites inside a fix PR whose whole value is that those suites stayed honest, and a shared harness is a change I would want reviewed on its own terms rather than buried under a barrier-retirement diff. Happy to open it as a follow-up unless you would rather it ride along.
| @@ -0,0 +1,2 @@ | |||
| --- | |||
| --- | |||
There was a problem hiding this comment.
AI Review: Note
On the merge strategy rather than the changeset itself (empty is correct here).
The description cites "Same convention as #3137 → #3139." That isn't what happened. #3137 is still open and was never merged; its tests landed inside the fix PR as regular passing it — git log --diff-filter=A -- packages/core/src/step-delivery-ordering.test.ts gives 2941b1c360, which is #3139. So the established convention in this repo is "repro tests land green alongside the fix," not "merge the repro as expected-fail documentation first."
That's worth resolving explicitly rather than by precedent, because merging 12 it.fails into main means main permanently carries assertions that pass by throwing, and two of them (delivery-barrier-idle-collapse.test.ts lines 174 and 253) assert the inverse of an invariant step.ts:281-283 documents as deliberate. If the goal is a CI-visible inventory of known gaps, the buffered-hook suite plus the two ordering cases in the collapse suite deliver that on their own.
There was a problem hiding this comment.
You were right on the facts and the correction has been applied — #3137 is open and unmerged, and #3139 landed its tests green, so "repro tests land green alongside the fix" is the convention. That is what this PR now does: the fix is here, all 12 markers are flipped to it (977cd33 and its parent), and nothing expected-fail is being merged into main. The empty changeset is gone too, replaced by a @workflow/core patch.
|
Correcting the measurement section above, since I had it wrong twice and the second version blamed this PR. The hook-storm regression is not this PR's. Two runs of this branch carrying test files only, with no
The first engine commit here ( It reproduces on The window is The two defects this PR found while chasing the wrong cause are real and unit-test-proven, and they stay. They just do not explain these numbers. |
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Verification review, per the ask in Slack: are the claimed deterministic-replay bugs real, and do the fixes fix them? Short answer: yes to both, verified adversarially — plus one finding that changes how the still-open merge gate should be read: the head-commit CI carnage and the partial head-commit storm are both contaminated by 429 rate limiting from concurrent storm jobs, so the storm this PR is waiting on needs to run in isolation to mean anything.
The claimed bugs are real
At the branch point dc0d25eaf I reproduced the description's suite accounting verbatim: 4 failed | 1655 passed | 12 expected fail. Running just the four new suites there: the 4 failures are exactly the 3 idle-collapse repros #3198 already fixes (marked it.fails, passing) plus the armed-barrier starvation control — and 9 markers fail-as-expected, i.e. 9 real engine defects reproduced on synthetic, perfectly-ordered logs. That's the claim Pranay wanted checked, and it holds: these corruptions need no backend misordering at all. The buffered-claim suite in particular reproduces the production ReplayDivergenceError at every consumer hop count with a flawless log.
The fixes are load-bearing, individually
I reverted each mechanism one at a time on the head commit and confirmed its guard — and only its guard — fires:
| revert | result |
|---|---|
| net retires armed barriers again (mechanism 1) | 9 failures — idle-collapse suite and all 6 buffered-claim hop cases |
budget reset on delivery progress (b397eeafc) |
the 40-wide-batch mid-fire guard fails |
| quiescing map not gated on (mechanism 3) | the continuation-overtake guard fails |
pendingOrderedDeliveries dropped from busy (mechanism 4) |
the parked-on-quiescing-entry guard (the hookWithSleepWorkflow e2e's unit twin) fails |
transitive resolvesOnItsOwn skip restored (mechanism 5) |
all 6 buffered-claim hop cases fail |
abandon clock starts pre-claimability (c8235f495) |
its red-then-green test fails |
None of the 22 new tests is vacuous. The R1/R5 results are the important ones: they show the two heavy design decisions — never retiring an armed barrier, and narrowing the step skip to the unclaimed payload itself — are each independently necessary for the actual corruption repro, not just for their own subsuites.
Mechanism-5 termination I audited separately: a step gating on an armed wait that is itself parked behind an unclaimed payload drains because scheduleBarrierRetirement polls on raw ticks, outside scheduleWhenIdle — and abandonableAfter always settles (hydrated.resolve() sits in the buffered branch's finally; the waiting-consumer branch never registers it). claim() arms synchronously before its first await, so the retirement poll stops racing a claim that has already happened.
Local validation
- Full
@workflow/coresuite on the head commit: 79 files, 1676 passed | 3 expected fail (the 3 are the pre-existing pair inhook-sleep-interaction+ the Date one inworkflow.test.ts). Typecheck clean. - The 14 ordering/suspension-sensitive files 8× consecutively: identical results every run.
- Full local e2e: 135/135, and the timing-sensitive isolated run the description calls out as the reliable signal —
-t hookWithSleep— 5× consecutively green.
The CI reading is wrong without this context
The head commit's Tests run (30586473832) fails all 12 E2E Vercel Prod apps plus 3 local jobs, which looks like a catastrophic regression from b397eeafc — a commit that only makes the idle deadline more conservative. It isn't the code. The failures are:
HTTP 429 Too Many Requests — api-workflow-deployment-key
"Too many requests - try again in 60 seconds (more than 2000)"
Two storm jobs (30585608092 at 22:15, 30586473664 at 22:31 — 1400 runs each at concurrency 40) ran concurrently with the full test matrix against the same vercel-labs team and exhausted the deployment-key rate limit. The per-commit picture is consistent with that and not with a code regression: dc0d25eaf 5 failed jobs (chronic-flake baseline), b58fdcf0d 57 (its defects were real), 977cd332b 2, head 15 — where the head run is the only one fully overlapped by two storms.
This also contaminates the partial head-commit storm (30586473664: hook-storm 127 corrupted + 28 infra of ~155 attempted). Rate-limited key fetches and a saturated backend change hydration timing wholesale, which is the very variable these scenarios measure. I would not conclude anything from that run — for better or worse. The merge gate this PR sets for itself (head-commit storm: hook-storm ≤ baseline, step-storm improved, control clean, zero stuck) is still the right gate, but the run needs to be the only storm in flight, ideally with the Tests matrix quiet too. A concurrency group on the repro workflow keyed to the shared backend — not just to the ref — would make this class of false signal structurally impossible; the same collision is likely part of why the pre/post #3198 isolation runs (900→1160) and Peter's drifting ~700 baseline have been hard to interpret all week.
Residuals, agreed non-blocking
- The 16-round vs 32-tick ordering residual the description flags: I confirmed the common shape escapes it — an unclaimed payload with a parked chain retires via the
pendingDeliveries === 0route on the first quiet tick, well before any suspension budget expires. It only bites under sustained no-progress traffic, which is the poke-storm shape where firing is intended. Deserves the follow-up issue the description promises, not a hold. - Two inline notes on documentation of deliberate asymmetries (
scheduleBarrierRetirement's idle route ignoringpendingOrderedDeliveries;beginQuiescingrunning for net retirements). - Hot-path perf note in passing:
awaitEarlierDeliveriesdropping the transitive walk makes the per-delivery path strictly cheaper than what #3139/#3198 shipped. The walk survives only in the suspension gate, once per idle poll round.
Verdict
Approving the code. Every claimed bug verified real, every fix mechanism verified individually load-bearing, suite/e2e evidence solid, and the one alarming CI signal traced to infrastructure with receipts. The PR's own do-not-merge gate stands — an isolated head-commit storm — and per the above it should be treated as unmet until a storm runs without competition, not because the current numbers are bad but because they are unreadable.
| return; | ||
| } | ||
| if ( | ||
| ctx.pendingDeliveries === 0 || |
There was a problem hiding this comment.
Non-blocking, please document: the idle route here checks pendingDeliveries === 0 but deliberately does not consult pendingOrderedDeliveries — and that asymmetry is load-bearing, not an oversight. A chain of committed deliveries parked on this unclaimed payload holds pendingOrderedDeliveries > 0 the whole time it waits; if this route counted that as busy, the payload could never retire by the idle path while anything was gated on it, and every parked chain would ride the full 32-tick deadline. Checking only hydration is what lets the root retire the moment the system quiets and release the chain in log order.
Since scheduleWhenIdle twenty lines down does consult pendingOrderedDeliveries, the next reader will reasonably assume this one forgot to. One sentence in the docblock would prevent that "fix".
There was a problem hiding this comment.
Documented in 7ab411223, in your words — a reader arriving twenty lines above scheduleWhenIdle and seeing it consult pendingOrderedDeliveries really would "fix" this one, and the fix would be a deadlock.
The docblock now says the check is pendingDeliveries === 0 and deliberately not the full idle predicate, with the reason: a chain of committed deliveries parked on this very payload holds that counter above zero for as long as it waits, so counting it as busy would mean the payload could never retire by the idle route while anything was gated on it — every parked chain would ride the full deadline instead. Checking only hydration is what lets the root retire the moment the system quiets and release the chain in log order.
Your comment also turned out to be load-bearing for the other review thread. Peter found that the round budget can expire before that 32-tick deadline and preempt exactly the parked chain you are describing here; the fix (92831b16f) resets the budget while a committed delivery is waiting on an unarmed barrier. Both routes out of that state are now written down next to each other, which they were not before.
| barriers.delete(eventIndex); | ||
| } | ||
| resolve(); | ||
| beginQuiescing(ctx, eventIndex, kind); |
There was a problem hiding this comment.
Non-blocking observation: finish() runs beginQuiescing for safety-net retirements too, not just real deliveries — so an abandoned payload that never reached the workflow still parks a quiescing entry that later deliveries gate one macrotask on. I think that's actually correct (the retirement releases a chain whose members then need their quiesce window, and a uniform rule is simpler than a special case), but it's a subtlety worth a clause here, since the recentlyDeliveredBarriers docblock describes entries as deliveries "handed to the workflow", which a net-retired payload never was.
There was a problem hiding this comment.
Your reading is right, and it is now a clause on beginQuiescing as of 7ab411223.
The reason it is correct is the one you gave: retiring the root of a chain releases every member behind it, and those members do need their quiesce window. I added the second half of the argument — a special case here would have to be right about which retirements woke a branch, and being wrong about that reintroduces the overtake recentlyDeliveredBarriers exists to remove, so the uniform rule is not just simpler but the safer default.
Good catch on the docblock mismatch: recentlyDeliveredBarriers describes its entries as deliveries "handed to the workflow", which a net-retired payload never was. The beginQuiescing comment now names that case explicitly so the two do not read as contradicting each other.
Head-commit storm verdictRun 30586473664, attempt 2,
Against the same-night hook-storm is at parity with step-storm is not separable in one run each. 545 of 563 verdicted (96.8%) here; the closest The one registered prediction this branch's fixes made, held. Where it misses. hook-storm shows 5 stuck. Summary against the exit bar: met on hook-storm (at parity), met on the control, unresolvable on step-storm, missed on the letter of "zero stuck" by 5 runs at Two commits pushed since the storm started, neither of them runtime code: |
|
@VaguelySerious ready for another look when you have a moment — the do-not-merge hold is off and everything you raised is folded in. What changed since your review:
Separately, one thing from your |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| * because this function also schedules workflow suspensions, where firing early | ||
| * preempts data delivery. | ||
| */ | ||
| const IDLE_POLL_DEADLINE_ROUNDS = 16; |
There was a problem hiding this comment.
AI Review: Blocking
IDLE_POLL_DEADLINE_ROUNDS (16 rounds) expires before BARRIER_ABANDON_DEADLINE_TICKS (32 ticks) under exactly the traffic the abandon deadline was added for, so the suspension wins and preempts the parked committed delivery that pendingOrderedDeliveries was added to protect.
The state where both deadlines are the only ways out:
- index 0: a buffered hook payload nobody claims (unarmed). Sustained traffic keeps
pendingDeliveriesabove zero, soscheduleBarrierRetirementcan only retire it via its 32 raw ticks. - index 1: a committed
wait_completeddelivery parked on it inawaitEarlierDeliveries—DEFER_BEHIND.waitincludes'hook', and the!entry.armedskip is on the step path only, not the wait path. scheduleWhenIdlearmed by another pending consumer. Nothing callsmarkDelivered()while that wait is parked, sodeliveryProgressnever advances, the budget is never reset, and it fires at round 16.
A round costs 1–2 ticks, so 16 rounds is always reached before 32 ticks. Measured on this branch with your own harness (makeCtx + startPokeStorm from delivery-barrier-idle-starvation.test.ts, plus a sleep.ts-shaped wait delivery); numbers are the tick each event landed on:
macrotask-backed queue (your startPokeStorm): firedAt=32 deliveredAt=33
microtask-only hydration queue: firedAt=16 deliveredAt=33
Not a knife-edge — with cheap hydration the suspension fires 17 ticks early. runtime.ts:1568 goes straight into handleSuspension without draining pendingOrderedDeliveries, so the run suspends carrying none of the work that delivery was about to create, which is the failure delivery-barrier-idle-starvation.test.ts:264 calls a defect. Neither existing test covers it: in :264 every round of the 40-wide batch delivers, so deliveryProgress resets the budget, and in :181 nothing arms scheduleWhenIdle. The gap is only visible with both clocks running against each other.
The scoping caveat you put on the starvation suite header applies here too — this shows the mechanism, not its production incidence. Reachability needs an unclaimed buffered payload, a later wait/hook delivery gated on it, sustained traffic, and some other pending consumer to arm the suspension: the consumer that consumed wait_completed returns Finished, so it is not that one. A loop over Promise.race([hook, sleep]) supplies it.
The relationship between the two budgets wants to be expressed rather than left to two independently chosen literals — either derive the round budget so it cannot expire before the abandon deadline, or exempt pendingOrderedDeliveries > 0 from the round budget when the barrier blocking it is unarmed (that barrier's retirement is already bounded, so waiting on it is bounded too, and the starvation this budget guards against cannot be what is happening).
Repro, on top of this file's existing makeCtx/startPokeStorm:
// makeCtx() also needs recentlyDeliveredBarriers/pendingOrderedDeliveries/deliveryProgress
function deliverWaitResult(ctx, eventIndex, onDelivered) {
const barrier = registerDeliveryBarrier(ctx, eventIndex, 'wait');
const earlierDelivered = awaitEarlierDeliveries(ctx, eventIndex, 'wait');
void ctx.promiseQueue
.then(() => earlierDelivered)
.then(() => { barrier.markDelivered(); onDelivered(); });
}
const ctx = makeCtx();
const storm = startPokeStorm(ctx);
registerDeliveryBarrier(ctx, 0, 'hook', { armed: false });
let ticks = 0, deliveredAt = null, firedAt = null;
deliverWaitResult(ctx, 1, () => { deliveredAt = ticks; });
scheduleWhenIdle(ctx, () => { if (firedAt === null) firedAt = ticks; });
while (ticks < 200 && (deliveredAt === null || firedAt === null)) { ticks++; await macrotask(); }
expect(storm.lowWaterMark()).toBeGreaterThan(0);
expect(deliveredAt).not.toBeNull(); // liveness: the abandon deadline does release it
expect(firedAt === null || deliveredAt <= firedAt).toBe(true); // fails: 33 <= 32There was a problem hiding this comment.
Reproduced verbatim and fixed in 92831b16f. Your repro is now a test in the suite (does not let the round budget preempt a delivery waiting on the abandon deadline), and it fails on the parent commit for exactly the reason you give.
Which of your two options, and why. The second — exempt the parked delivery — not the derived budget. Deriving cannot be made reliable, and your own numbers are the argument: the abandon clock does not start until abandonableAfter settles, so the head start the round budget needs is however long the payload's hydration took, which in production is an S3 fetch plus a decrypt. Your macrotask-backed measurement already lands firedAt=32 deliveredAt=33 — a budget derived to "not expire before 32 ticks" still loses by one, and any margin on top of that is a number chosen against a hydration latency rather than against anything structural. Re-tuning the pair keeps the race and moves it.
What it does instead. The budget resets — the same treatment deliveryProgress gets, not a skipped increment — while isAwaitingBoundedRetirement(ctx) holds:
(ctx.pendingOrderedDeliveries ?? 0) > 0 && someRegisteredBarrierIsUnarmed(ctx)Both conjuncts are load-bearing, and the docstring says so at length because each one on its own is a bug:
pendingOrderedDeliveries > 0keeps the escape hatch. In the storm this budget exists for, a hook defers behind earlier waits and steps only, so a run of pure pokes parks nothing and the predicate stays false however many unclaimed payloads pile up. Without it, "an unarmed barrier exists" is true forever under that storm and the budget never expires — the 3m19s stall, restored.- An unarmed barrier being registered is what makes the wait bounded. A parked delivery's blocking set is fixed when it parks, every edge points to a strictly earlier index, and an unarmed entry retires within
BARRIER_ABANDON_DEADLINE_TICKSraw ticks on a timer no traffic can starve. Once those entries are gone the predicate goes false and the budget counts again — so a call site that breaks the INVARIANT and leaks an armed barrier still reaches the callback on the budget, exactly as before. That mattered: it is the difference between your exemption and a blanket one, and it is the reason I took your narrower wording rather than exemptingpendingOrderedDeliveriesoutright.
The approximation to "the barrier blocking it" is "an unarmed barrier is registered" — we do not record wait-for edges. It is self-correcting in the direction that matters: a delivery parked behind both a leaked armed barrier and an unarmed one keeps the exemption only until the unarmed one retires, then the budget resumes.
The test asserts both liveness directions before the ordering claim, which your sketch left open — firedAt === null || deliveredAt <= firedAt passes vacuously if the callback simply never fires, and "never fires" is the starvation the suite opens on. It now asserts deliveredAt is not null, firedAt is not null, and only then that the delivery came first.
| * delivery is abandoned (the workflow took a different branch or is | ||
| * suspending and never observes it), the barrier auto-resolves at idle. | ||
| * suspending and never observes it), the barrier is retired by the safety net | ||
| * in {@link scheduleBarrierRetirement}: at idle if it is UNARMED, and on a |
There was a problem hiding this comment.
AI Review: Note
ca8a316e0 corrected this claim in step.ts, but the same sentence is still here — on the docstring for the invariant itself: "retired by the safety net in scheduleBarrierRetirement: at idle if it is UNARMED, and on a deadline either way." There is no deadline for an armed barrier; check() returns early on entry.armed before the tick budget is consulted, and that it never retires one is what the rest of the design rests on.
Line 578 has the same problem from the other side: "an armed barrier with no unconditional chain stalls every idle check in the run, its own retirement included, until the deadlines fire." Its retirement never comes — IDLE_POLL_DEADLINE_ROUNDS releases the idle callbacks, not the barrier — so a call site that violates the invariant leaks that entry for the rest of the run and every later gated delivery waits on it indefinitely. Worth stating plainly here, since this docstring is where the next call site will look.
There was a problem hiding this comment.
Both corrected in 7ab411223, and you are right that the second one was wrong in a worse direction than the first.
registerDeliveryBarrier now reads that the net retires a barrier "ONLY while it is UNARMED, by either of that net's two conditions", with the mechanism named — check() returns early on entry.armed before it looks at hydration or at the tick budget — so an armed barrier leaves the registry from its own delivery chain and from nothing else. ca8a316e0 fixed the copy of that sentence in step.ts and left the original standing, which is the more embarrassing of the two places to leave it.
On line 578: the old text said a leaked armed barrier stalls idle checks "until the deadlines fire" and that "the deadlines bound that damage". Neither is true, and the docstring now says the opposite plainly — violating the INVARIANT is unrecoverable, not merely slow. IDLE_POLL_DEADLINE_ROUNDS releases the idle callbacks, letting suspensions fire again, while the barrier and everything gated on it stay stuck on a promise that will never settle. What the run loses is every delivery ordered after the leaked one, permanently.
That correction also changed the fix on the blocking comment. Knowing the budget is the only thing that still happens after an armed leak — and not a bound on the leak itself — is why the exemption there is conjoined on an unarmed barrier being registered rather than on pendingOrderedDeliveries alone.
|
|
||
| // Comfortably wider than the round budget, and narrower than the bursts | ||
| // the hook-storm scenario produces. | ||
| const batch = 40; |
There was a problem hiding this comment.
AI Review: Note
batch = 40 is chosen as "comfortably wider than the round budget", but the budget is a private literal in private.ts and 40 is a private literal here, with nothing tying them. Raise IDLE_POLL_DEADLINE_ROUNDS to 64 and this test still passes — vacuously, because a 40-wide batch can no longer overrun the budget whose flat counting it exists to catch. The regression would be silently unguarded. Same for batch = 32 at :321 and the macrotasks(60)/macrotasks(20)/macrotasks(8) budgets, each sized against 16 or 32. Exporting both constants and deriving the test numbers from them keeps them honest, and matches the repo rule about not restating constants in tests.
Separately: the harness fix checks out — every bump(+1) in startPokeStorm now has a matching -1 one tick later, the control test asserts both the low-water mark and the drain to zero, and the suite header's scoping of what the 3m19s observation does and does not establish is the right claim. The two tests I asked for last round are here and pass: :346 (228 unclaimed payloads, still single-digit ticks) and buffered-hook-claim-ordering.test.ts:334 (the mirrored log).
There was a problem hiding this comment.
Taken, in 6f1d3de62. Both constants are exported now and every budget in the suite derives from them:
const IDLE_BUDGET_TICKS = 2 * IDLE_POLL_DEADLINE_ROUNDS; // a round costs at most two ticks
const PAST_BOTH_DEADLINES = 2 * Math.max(BARRIER_ABANDON_DEADLINE_TICKS, IDLE_BUDGET_TICKS);
const BEFORE_ABANDON_DEADLINE = Math.floor(BARRIER_ABANDON_DEADLINE_TICKS / 2);
const IDLE_ROUTE_TICKS = Math.floor(Math.min(BARRIER_ABANDON_DEADLINE_TICKS, IDLE_BUDGET_TICKS) / 4);Both batch literals are now 2 * IDLE_POLL_DEADLINE_ROUNDS, so the flat-count regression fires at the batch's halfway mark whatever the budget is, and the macrotasks(60)/(40)/(20)/(8) sites are PAST_BOTH_DEADLINES / BEFORE_ABANDON_DEADLINE / IDLE_ROUTE_TICKS by which side of a deadline each one has to land on. private.ts is not a public entrypoint and only getStepFunction is imported out of it, so exporting the two constants adds no API surface.
The header comment on those derivations states the failure mode in your words, because it is the one that would otherwise recur silently: a suite about deadlines is exactly where a restated constant stops being able to overrun the thing it guards.
And thanks for re-checking the harness independently — the matched bump(+1)/bump(-1) accounting and the two tests from last round are the ones I'd have wanted a second pair of eyes on.
| * improvement on retiring at the first idle tick, which is what a barrier used | ||
| * to do while its own delivery was still in flight. | ||
| */ | ||
| const BARRIER_ABANDON_DEADLINE_TICKS = 32; |
There was a problem hiding this comment.
AI Review: Note
Neither deadline is env-overridable, unlike every other timing knob in this package — WORKFLOW_DEFERRED_CHECK_DELAY_MS (events-consumer.ts:24), WORKFLOW_MAX_QUEUE_DELIVERIES, WORKFLOW_REPLAY_TIMEOUT_MS, all through envNumber. These two decide whether a run drops a committed delivery, and per the blocking comment their ratio is what decides it, which is the kind of value worth being able to move without a release. Skew protection cuts the other way here: runs stay on the deployment they started on, so a code-only correction never reaches the runs already in the bad state.
There was a problem hiding this comment.
Done in 6f1d3de62, and the skew argument is what decided it — a code-only correction reaching none of the runs already in the bad state is the part that makes these different from a knob you can just get right.
WORKFLOW_BARRIER_ABANDON_DEADLINE_TICKS default 32, min 1
WORKFLOW_IDLE_POLL_DEADLINE_ROUNDS default 16, min 1
Both go through envNumber with integer: true, same as the others. Two details worth stating:
- Read once per clock, not per poll. The abandon deadline is read at barrier registration and the round budget at the
scheduleWhenIdlecall, so a single clock's budget cannot change underneath it mid-flight, and neither one puts aprocess.envread in a per-tick loop. - Floored at 1, not 0. A 0-tick abandon deadline retires every unclaimed payload on its first tick, which is precisely the collapse behavior this net replaced; a 0-round budget fires the callback unconditionally and preempts every in-flight delivery. Both are one-character overrides away from the two defects this PR fixes, so the floor is doing real work rather than being defensive boilerplate.
Documented in configuration/runtime-tuning, including the constraint you identified in the blocking comment — the round budget has to outlast the abandon deadline — since an operator moving one of these without the other is the failure mode the docs exist to prevent. The blocking fix means the default pair is no longer relying on that constraint holding, but an override still can break it.
| '@workflow/core': patch | ||
| --- | ||
|
|
||
| Fix replay-engine determinism defects that could surface as `CORRUPTED_EVENT_LOG`: delivery barriers were retired while their own delivery was still in flight (by a global idle tick, or by an abandon deadline that ran through the payload's own hydration), an abandoned barrier could never retire under continuous unrelated delivery traffic, and a barrier stopped being visible to the registry one statement before the branch it woke had run. Workflow suspensions now wait on delivery progress rather than a fixed number of poll rounds, so a wide batch of deliveries is never preempted part-way through. |
There was a problem hiding this comment.
AI Review: Note
CI state, so nobody chases it — none of the red is this PR.
- 13/13
E2E Vercel Prod Testslanes fail onHTTP 429 ... {"code":"rate_limited","name":"api-workflow-deployment-key"}raised frompackages/world-vercel/src/encryption.ts:134while fetching the run key: the 2000/min deployment-key quota, exhausted by all 13 lanes hitting it at once. Every listed test failure is that same error, and most fail in under 100ms rather than timing out. Lintfails on anorganizeImportserror inpackages/core/src/runtime.ts(aCOMPUTE_INSTANCE_IDimport) that arrived with themainmerge indc0d25eaand is no longer onmain;Lintwas green one commit earlier onb397eeaf. A rebase clears it.mainhadTestsred on every commit between 19:23 and 22:32 UTC, and the twoE2E Local Devlanes here match its failures.
Locally on ca8a316e0: pnpm typecheck green; full packages/core suite 1736 passed / 3 expected-fail, the only failure being e2e/route-bundle-isolation.test.ts, which needs a built workbench; 42/42 across the five barrier suites.
There was a problem hiding this comment.
Thanks for triaging it — and the Lint diagnosis was exactly right. The branch is now rebased onto main (ee944d247, which includes #3145, #3208 and #3244), which drops the dc0d25ea merge commit along with the organizeImports violation it carried, and gives the repo the linear history it wants anyway. The rebase was clean; no conflicts in any of the barrier files.
Post-rebase, locally on 6f1d3de62:
| check | result |
|---|---|
packages/core full suite |
82 files, 1750 passed / 3 expected-fail / 0 failed |
| five barrier suites | 43/43 (the new deadline-race test is the 43rd) |
pnpm typecheck |
clean |
biome ci --max-diagnostics=200 |
exit 0 |
isolated hookWithSleepWorkflow e2e |
4/4 runs green against a local nextjs-turbopack |
One trap worth recording since you hit the same tree: main added computeInstanceId to CreateEventParams in #3186, so packages/core typecheck reds with 12 phantom errors until packages/world is rebuilt. It is stale dist, not a real break.
On the 13/13 E2E Vercel Prod lanes — agreed that is the deployment-key quota rather than anything in the diff, and worth its own issue independent of this PR, since it means a 13-lane matrix cannot get a clean read on any PR. The push above re-triggers the full matrix; I will rerun failed lanes if they come back on that same 429.
…tirement Four failing tests against the barrier primitives in private.ts: - the idle safety net (private.ts:461) retires a barrier whose delivery is still parked inside awaitEarlierDeliveries, because pendingDeliveries only counts the hydration window (released at step.ts:322, before the detached continuation at step.ts:324); - one idle tick retires EVERY live barrier, not just abandoned ones; - a later-in-log delivery consequently computes an empty deferral set, skips the macrotask yield at private.ts:392, and is handed to workflow code before an earlier-in-log one; - the same inversion without the idle net at all: markDelivered() runs one statement before resolve(), so the registry stops mentioning a delivery while the branch it woke is still hops away from its next useStep. Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
End-to-end reproduction on current main, from the event log alone — no artificial hydration latency, no shared ReplayPayloadCache, failing at every consumer hop count from 0 to 16 with the production error shape: Replay divergence: step event step_created for step_<ULID> belongs to "afterHook", but the current step consumer is "afterStep" An unclaimed buffered hook payload makes resolvesOnItsOwn (private.ts:296-319) report false for itself, and transitively for the armed wait barrier that defers behind it. A later step result therefore skips BOTH via the kind === 'step' escape at private.ts:368-371, computes an empty deferral set, and resolves on microtasks while the wait is still parked — so the step branch draws the ULID the log assigns to the hook branch. Verified mechanism: neutralising that skip makes all six cases pass (and keeps the 72 existing delivery-ordering assertions green), so the skip is the cause. No existing suite covers this because every hook case in step-delivery-ordering / step-delivery-hop-count / delivery-barrier-coverage registers its awaiter before the drain, taking the armed path in hook.ts rather than claim(). Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
…ivery traffic Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
The 12 tests that fail on main are right-reason failures documenting the delivery-barrier idle collapse/starvation windows and the buffered-hook claim-ordering race. Mark them it.fails so the suites merge as executable documentation (the #3137 -> #3139 convention); the fix PR flips the markers back to it. The 4 controls that pass on main keep plain it. Also adds an empty changeset (test-only change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Three defects in the delivery-barrier registry could hand branch-deciding deliveries to workflow code out of event-log order, surfacing as CORRUPTED_EVENT_LOG on replay. All three are fixed here, and the twelve expected-fail repros that documented them are flipped back to plain `it`. Barriers no longer retire on a global idle tick. #3198 stopped the idle CHECK from observing idle while a committed delivery is parked; this stops the same predicate from retiring the barriers themselves. `pendingDeliveries` tracks only the host-side hydration window — released inside the promiseQueue slot, before the detached continuation that hands the value over, and never touched at all by a wait_completed — so one idle tick used to retire every live barrier while their deliveries were still parked in awaitEarlierDeliveries. Retirement moves to its own poll, which retires only an UNARMED barrier: an unclaimed buffered hook payload, the one delivery that can be abandoned at the root. Every other kind is committed and retires from its own chain. Both polls get a deadline. Without one neither has any escape from continuous unrelated traffic: an abandoned barrier under a stream of deliveries to a never-read hook starved indefinitely, observed live as a run making no progress for 3m19s across 228 pokes. The barrier deadline is counted in raw ticks, so the traffic keeping the system busy cannot stretch it; scheduleWhenIdle's is counted in poll rounds, each of which waits out a full promiseQueue drain, because that function also schedules suspensions, where firing early preempts data delivery. A retired delivery stays visible to the registry for one more macrotask. markDelivered() resolved a barrier one statement before the resolve() that wakes the branch, so anything reading the registry in between — a delivery consumed in a later drain window, or a buffered payload's claim() — computed an empty deferral set and overtook the branch it was meant to follow. The live registry still drops the entry immediately; a second short-lived map carries ordering visibility across the gap. A step result's buffered-hook skip is narrowed to the payload itself. A step still never gates on an unclaimed payload, where the claim commonly sits downstream of the step result, but it now gates on an earlier armed wait or hook whatever that delivery is itself waiting for. Skipping those as well, via the transitive resolvesOnItsOwn walk, was the buffered-claim corruption: in Promise.all([step, sleep-then-read-hook]) the wait completion is precisely what wakes the branch that goes on to claim the payload. awaitEarlierDeliveries no longer consults that walk, leaving the per-delivery path a single linear pass; the walk itself survives only in hasParkedCommittedDelivery, which gates suspensions rather than deliveries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
…rked deliveries as in-flight Follow-up to the previous commit, from review of #3196 and from an e2e regression that review work exposed. Retirement by the safety net is now UNARMED-only. The abandon deadline used to fire for any barrier, which reintroduced the collapse at a longer timescale: production hydration (object-storage fetch plus decrypt) runs 10-500ms, far past any tick budget worth setting, so the slowest deliveries would have been exactly the ones force-retired mid-flight. Only an unclaimed buffered hook payload — the one delivery that can be abandoned at the root — is retirable by the net; every other barrier leaves the registry from its own delivery chain. All five call sites were audited against that invariant: step_completed, step_failed, wait_completed, the waiting-consumer and claim() hook paths, and the abort hook all attach their chain unconditionally. Deliveries parked in awaitEarlierDeliveries are now counted, and the count gates scheduleWhenIdle. A delivery whose only remaining gate is a recentlyDeliveredBarriers entry is invisible to both pendingDeliveries (already released in its hydration slot) and to a scan of the live registry (already empty), so a suspension armed in that window preempted it and the run suspended carrying none of the work the delivery was about to create. That is how the second payload of the hookWithSleepWorkflow e2e went missing. Guarded by a unit test that fails without the counter. Review fixes: - Starvation suite: the poke storm held pendingDeliveries above zero with a single unmatched increment, so it modelled a stuck hydration rather than overlapping traffic. It now overlaps genuinely, leaks nothing, and asserts both properties. The header no longer attributes the observed 3m19s stall to this mechanism, which the suite does not establish. - Added the liveness guard asked for in review: 228 unclaimed payloads gating a later delivery still drain in a handful of ticks. - Added the mirrored-log control asked for in review: the interleaving where the step result legitimately precedes the wait completion must keep replaying cleanly, so the ordering assertions cannot be satisfied by flipping the bias. - Below-watermark suite: the read shape is not a world-postgres class. world-vercel paginates DynamoDB by event ULID with an `eid:` cursor over a strictly-after range, and workflow-server documents its IDs as monotonic only intra-instance. Recorded in the header; the second test is renamed to say it characterizes current behavior rather than pinning a contract. - Call-site comments that documented idle retirement of parked barriers as a deliberate tradeoff now describe what the net actually does. - Stale private.ts / step.ts line references replaced with symbol names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
…imed An unarmed delivery barrier is retired on the inference that no consumer has claimed the payload. That inference is not available until a consumer could have: `claim()` cannot hand anything over until the payload's own hydration resolves, so while it is still hydrating, "nobody has claimed it" carries no information. Left unguarded, the two retirement conditions conspire during that window. The payload's own hydration holds `pendingDeliveries` above zero, so the idle route cannot fire, and the abandon deadline retires the barrier because its hydration was slow. Against a production hydration (object storage fetch plus decrypt, 10-500ms) and a deadline of a few dozen `setTimeout(0)` ticks, that is the common case for a buffered payload rather than an edge one. `registerDeliveryBarrier` now takes `abandonableAfter` and does not start the retirement poll until it settles; `workflow/hook.ts` passes the buffered payload's own hydration. The waiting-consumer path is unaffected: it registers armed, and an armed barrier leaves the poll on its first check either way. This is the same slow-hydration trap b58fdcf had on the armed side, found by re-auditing the deadline against production latency after that commit's storm run. The regression test holds hydration open well past the deadline and asserts both that the barrier survives and that a later step result still orders behind the claim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Counted flat, `IDLE_POLL_DEADLINE_ROUNDS` is not a deadline but a cap on how many events one drain window may deliver. Delivering a window in log order costs one macrotask per deferring delivery, so a window of K events needs K poll rounds to clear. Past the budget `scheduleWhenIdle` fires anyway, into the middle of the batch — and for a hook consumer that callback raises a `WorkflowSuspension`, so the run suspends carrying none of the work the remaining deliveries were about to create. A new test in `delivery-barrier-idle-starvation.test.ts` puts 40 deliveries in one window and shows the callback landing after exactly 16 of them. The budget now restarts whenever a delivery reaches workflow code, tracked as `deliveryProgress` and incremented by `markDelivered()` only. Excluding the abandonment safety net's retirements is what keeps the escape hatch working: a stream of pokes to a hook nobody reads retires barrier after barrier and delivers nothing, so the budget still runs out and the suspension still fires. That is the 3m19s stall the deadline was added for, and its test still passes. Ordering machinery that slows delivery down is only safe if the liveness timers around it measure progress rather than elapsed rounds; this is the one that did not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
The budget bounds rounds without delivery progress, so the suite should say what that means in both directions and at the boundary between them. It already covered firing on a stall and not firing mid-batch; this adds the case that separates "reset the budget on progress" from "disable the budget once anything delivered" — deliver for a while, then stall with traffic still in flight, and the callback must still be reached. Verified as a guard rather than a passing assertion: mutating the reset to grant permanent immunity fails this case and only this case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
The comment offered the abandon deadline as a second exit. There isn't one: `scheduleBarrierRetirement` returns early for an armed entry, and that it never retires one is the invariant the rest of the design rests on. An armed barrier leaves only from its own delivery chain. Reported by Vercel Agent on #3196. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
…andon deadline The two deadlines added in this PR had no relationship to each other, and in the one state where either decides anything they raced — with the wrong one winning. A committed `wait_completed` parked on an unclaimed buffered payload can only be released by that payload's abandon deadline: sustained traffic keeps hydration busy, so `scheduleBarrierRetirement` cannot take the idle route, and `DEFER_BEHIND.wait` includes `hook` while the unarmed skip in `awaitEarlierDeliveries` is on the step path only. Nothing calls `markDelivered()` while that wait is parked, so `deliveryProgress` never advances and the round budget is never reset. A round costs 1-2 ticks, so `IDLE_POLL_DEADLINE_ROUNDS` expires well before `BARRIER_ABANDON_DEADLINE_TICKS` does and the suspension preempts the delivery `pendingOrderedDeliveries` was added to protect — the same loss the round budget was added to prevent, arriving by the other route. Re-tuning the literals cannot fix it: the abandon clock does not start until the payload is claimable, so no fixed head start is enough. The budget instead resets while a committed delivery is parked behind an unarmed barrier, which is a state that cannot make progress by construction and is bounded by a timer no traffic can starve. Both conjuncts of that predicate are load-bearing and documented: without `pendingOrderedDeliveries > 0` a pure poke storm would never expire the budget, and without an unarmed barrier a leaked ARMED barrier would extend it forever instead of reaching the callback as it does today. The test asserts BOTH liveness directions before the ordering claim, so a fix that simply stopped firing the callback cannot pass it. Reported by @VaguelySerious with a repro, reproduced verbatim. Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Three comment corrections in private.ts, all on invariants the next call site will read before it gets them wrong. `registerDeliveryBarrier` still claimed a barrier is retired by the net "at idle if it is UNARMED, and on a deadline either way". There is no deadline for an armed barrier — `check()` returns early on `entry.armed` before it looks at hydration or the tick budget — and ca8a316 corrected the same sentence in step.ts while leaving it standing here, on the docstring for the invariant itself. The consequence stated below it was wrong in the same direction, and understated: it said a leaked armed barrier "stalls every idle check ... until the deadlines fire", and that the deadlines bound the damage. They do not. IDLE_POLL_DEADLINE_ROUNDS releases the idle CALLBACKS, not the barrier, so the entry leaks for the rest of the run and every delivery ordered after it waits on a promise that will never settle. Violating the invariant is unrecoverable, not merely slow, and the docstring now says so. `scheduleBarrierRetirement` checks `pendingDeliveries === 0` rather than the full idle predicate, and deliberately does not consult `pendingOrderedDeliveries` — a chain parked on this very payload holds that counter up for as long as it waits, so counting it would mean the payload could never retire by the idle route while anything was gated on it. Twenty lines down `scheduleWhenIdle` does consult it, so the next reader would reasonably "fix" this one. `beginQuiescing` runs for safety-net retirements as well as real hand-overs, so a payload that never reached the workflow still parks an entry later deliveries gate on. That is correct and intended, but the `recentlyDeliveredBarriers` docblock describes entries as deliveries handed to the workflow, which a net-retired payload never was. Raised by @TooTallNate (the last two) and @VaguelySerious (the first two). Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
…ests from them Two changes to the same pair of constants. They are now env-overridable, like every other timing knob in this package (WORKFLOW_DEFERRED_CHECK_DELAY_MS, WORKFLOW_MAX_QUEUE_DELIVERIES, WORKFLOW_REPLAY_TIMEOUT_MS) and for a sharper reason than any of them: their interaction is what decides whether a run drops a committed delivery, and skew protection cuts the wrong way here — a run stays on the deployment it started on, so a code-only correction never reaches the runs already in the bad state. Each is read once per barrier / once per poll so a single clock's budget cannot change underneath it, and both are floored at 1 (a 0-tick abandon deadline retires every payload on its first tick; a 0-round budget fires the callback unconditionally). Documented in configuration/runtime-tuning, including the constraint that binds them to each other. They are also exported, and the starvation suite now derives its budgets from them instead of restating 40 / 32 / 60 / 20 / 8 as literals. Those literals were each sized against a 16 or a 32 with nothing tying them: raise IDLE_POLL_DEADLINE_ROUNDS to 64 and a 40-wide batch can no longer overrun the budget whose flat counting it exists to catch, so the test guarding that regression would pass vacuously and the regression would ship unguarded. Raised by @VaguelySerious. Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Closes the engine-side determinism gaps behind the residual
CORRUPTED_EVENT_LOGfailures, and ships the four test suites that pin them. Started life as expected-fail documentation (12it.fails+ 4 controls); the fix landed in the same PR, so all 12 markers are flipped back toitand the suites are ordinary regression guards. Review added six more cases, including two guards against the fix itself.Measurement status
The do-not-merge hold is lifted. It was on while it was ambiguous whether this branch's saturated storm numbers were its own doing. They are not, and the cause is now attributed with matched-in-time evidence to #3198 — escalated with every number on that PR's thread: vercel/workflow#3198 (comment). Merging stays gated on review and required checks as always; what is being removed is our hold.
Where this branch actually stands, per criterion, with nothing rounded in its favour:
IDLE_POLL_DEADLINE_ROUNDSacting as a cap on batch width, and the abandon clock starting before a payload could be claimed.b58fdcf0dproduced 12 stuck step-storm runs; I registered before the rerun thatc8235f495'sabandonableAftershould take that to zero. It did: 0 stuck step-storm runs in the head storm (30586473664).main. 593/600 here against 195/200 onmain32ac8e73fthe same night — parity by both being equally bad. That saturation ismain's window, not this branch's.mainpreview (94.9%). One run each cannot separate those, and I am not claiming an improvement from them.mainruns 1 in 200 in the same shape, so this is rate parity rather than the clean zero the original bar asked for. It is 5, not 0.hook-sleep.The regression this branch inherited is documented below, along with two attributions I published and withdrew — including one that blamed this PR.
It predates every engine change on this branch
Both runs below are this branch carrying test files only, with no
packages/core/srcchange of mine at all:51de58490maine8934ade9dc0d25eafmaindc0d25eafis aMerge branch 'main'commit. The first engine commit on this branch,b58fdcf0d, lands four hours after that 596.The same flip on
peter/event-count-guard, which shares no changes with this PR:1e08d029dmainb19197debmainTwo branches with different content, one shared cause.
The window
e8934ade9..b12f248b6. Four commits, two of them on runtime paths:fix(core): don't observe idle while a committed delivery is parked behind its deferral,packages/core/src/private.ts(+75).[world-vercel] Make HTTP/2 actually multiplex on the events path,packages/world-vercel/src/http-client.ts(+254). Event-path transport timing, which this corruption class is directly sensitive to.(#3197 Streams UI and #2548 nitro dashboard are the other two; neither is on a runtime path.)
Separating them takes one storm per revert, and it is not work this PR can do from inside itself.
Two attributions I published and withdrew
Kept visible because the corrections matter as much as the conclusion:
b58fdcf0d's two hook-path defects caused it." Wrong: run 30583620477 carries977cd332b, with both of them fixed, and hook-storm is still 591/600. Every column here is attributed withgh run view <id> --json headSharather than by run order, because the storm cancels in-progress runs on unrelated repo activity — the run executing at any moment is frequently not the commit you just pushed. My head-commit storm was killed that way by another PR mid-run.What a storm run can and cannot support
Same-day runs on near-identical trees swing widely —
peter/event-count-guardmoved 65 → 539 on step-storm inside an hour — and several storms run concurrently against shared infrastructure. One run is enough to show that a scenario is broadly broken. It is not enough to attribute a delta to a commit, and that cuts against my own claims first.Two real defects, found while chasing the wrong thing
Neither explains the storm numbers. Both are genuine, both would have bitten at production hydration latency, and both are proven by a test that fails before the fix and passes after.
The idle poll's deadline was a cap on batch width.
IDLE_POLL_DEADLINE_ROUNDS(16) is mine, added for the starvation half of this PR. Counted flat it is not a deadline. Delivering a drain window in log order costs one macrotask per deferring delivery — that yield is mechanism 3 below, and it is the point of it — so a window of K events needs K poll rounds to clear. Past 16 the callback fires into the middle of the batch, and for a hook consumer that callback raises aWorkflowSuspension: every delivery after it is preempted and the run suspends carrying none of the work they were about to create. A case indelivery-barrier-idle-starvation.test.tsputs 40 deliveries in one window with an idle check armed, and the suspension lands after exactly 16 of them. Fixed inb397eeafcby restarting the budget whenever a delivery reaches workflow code — adeliveryProgresscounter bumped bymarkDelivered()and nothing else. Excluding the safety net's own retirements is what preserves the escape hatch: a stream of pokes to a hook nobody reads retires barrier after barrier and delivers nothing, so the budget still runs out and the suspension still fires. A third case pins the boundary between those two, so that resetting on progress cannot quietly become never expiring.The abandon clock started before the payload could be claimed. A buffered payload's barrier is retired on the inference that no consumer claimed it, but that inference is unavailable until a consumer could have —
claim()cannot hand anything over until the payload's own hydration resolves. During that window the two retirement conditions conspire: the hydration holdspendingDeliveriesabove zero so the idle route cannot fire, and the deadline retires the barrier because the hydration was slow. Fixed inc8235f495withabandonableAfter, which holds the retirement poll until the payload is claimable.Still open
A head-commit storm.Done — attempt 2 of 30586473664 ran the full 1400 onb397eeafc: step-storm 545 corrupted / 18 completed (37 infra), hook-storm 593 corrupted / 2 completed / 5 stuck, control 200/200 clean, 0 stuck on step-storm. Read against the same-nightmainbaseline in Measurement status above.A valid same-dayNow measured, and it settles the comparison:mainbaseline.main32ac8e73f, tonight, preview environment, hook-first shape — hook-storm 195 corrupted / 0 completed of 200, control 50/50 clean (30588748635). Every verdicted hook-storm run onmainis corrupt. The first attempt at this (30582373523) is void — all 838 attempts came backstuck, including controls that have never failed all week. That run targetedmain's production deployment (workflow_dispatchon themainref selectsenvironment: production); the same tree on preview suspends and resumes normally, so the wedge is that deployment, notmain's code.A residual in the same family, not addressed here.Fixed in92831b16f, after @VaguelySerious escalated it to blocking with a repro.BARRIER_ABANDON_DEADLINE_TICKS(32 raw ticks) is longer than the idle budget (16 rounds at 1-2 ticks each), so a committed delivery gated on an unclaimed payload saw the suspension fire before that payload retired — and progress-resetting could not help, because nothing is delivered in that state by construction. Ordering the two constants turned out not to be the fix either: the abandon clock does not start until the payload is claimable, so no fixed head start is enough. The budget now resets while a committed delivery is parked behind an unarmed barrier, which is bounded by a timer no traffic can starve. See mechanism 6.The bar for taking the do-not-merge note off was: this branch's storm no worse than a valid same-day
mainbaseline on both scenarios, control clean, zero stuck. With the baseline in hand that bar reads honest but weak —maincorrupts essentially every verdicted hook-storm run, so "no worse thanmain" means "still bad, in the waymainis currently bad". It is met on hook parity, step parity and control; missed on the letter of zero-stuck, by 5, atmain's own rate. Assessment in Measurement status above.The regression is attributed. Forward-carried onto
e8934ade9: #3238 (base + #3198 only) reproduces it at hook-storm 591/600, while the same base unmodified 40 minutes later sits at 59/200 (30589943739) — and @VaguelySerious's own three matched A/B pairs, run before any of this bisecting, show 323/210/167 without #3198 against 596/596/598 with it. #3242 ruled out the transport hypothesis: currentmainwith #3190's kill switch (WORKFLOW_H2_MULTIPLEX=0) still corrupts 199/200. Full write-up on #3198.What each suite pins
buffered-hook-claim-ordering.test.tsclaim()evaluates its deferral set at claim time against the live registry. Reproduces the productionReplayDivergenceErrorshape from a synthetic, perfectly-ordered event log at every consumer hop count (0/1/2/4/8/16) — the engine was nondeterministic even given a flawless backend.delivery-barrier-idle-collapse.test.tspendingDeliveries === 0, which tracks only hydration slots — one idle tick retired every live barrier while deliveries were still parked inawaitEarlierDeliveries; plus themarkDelivered()-before-continuation-quiesce overtake that needs no idle tick at all.delivery-barrier-idle-starvation.test.tsbelow-watermark-event-divergence.test.tsThe suites drive the
private.tsregistry primitives and a hand-authored in-orderEvent[]directly — no world, no backend, no VM scheduling noise — so the ordering contract they assume is stronger than any real backend provides, and the failures were engine-intrinsic.The fix
All of it is in
packages/core/src/private.ts, plus context init inworkflow.ts; the changes tostep.tsandworkflow/abort-controller.tsare comments only.1. An armed barrier can never be retired by the safety net. This is the load-bearing property. It replaces the global idle predicate outright — not "idle plus a smarter threshold", but "the net does not act on committed deliveries at all". The net retires unarmed barriers only: an unclaimed buffered hook payload, which is the single delivery that can be abandoned at the root. Everything else is committed — a wait completion, a step result, a claimed payload and an abort each call
markDelivered()from a chain that runs unconditionally — so the only thing that can hold one up is an earlier unclaimed payload, and retiring that one releases the rest in log order. All five call sites were audited against that invariant.Why it has to be absolute: hydration is already quiet while a delivery sits in
awaitEarlierDeliverieswaiting its turn, so under the old rule a single idle tick handed ordering back to the microtask race the registry exists to remove. A timer has the same defect at a longer timescale — production hydration (object-storage fetch plus decrypt) runs 10-500ms, far past any tick budget worth setting, so a deadline that applied to armed barriers would collapse precisely the deliveries whose hydration was slowest, and no test with sub-tick hydration would notice.delivery-barrier-idle-collapse.test.tsnow carries a case that holds hydration open past the deadline and asserts both that the barrier survives and that a later delivery still orders behind it.2. Deadlines bound the unarmed path only. An unclaimed payload's barrier retires after
BARRIER_ABANDON_DEADLINE_TICKS(32)setTimeout(0)ticks even under sustained traffic, andscheduleWhenIdlefires anyway afterIDLE_POLL_DEADLINE_ROUNDS(16) poll rounds. Both are backstops; the idle condition remains the normal route out, which the starvation control pins. Both are now overridable (WORKFLOW_BARRIER_ABANDON_DEADLINE_TICKS,WORKFLOW_IDLE_POLL_DEADLINE_ROUNDS), read once per clock rather than per tick, and documented inconfiguration/runtime-tuning; and their interaction is handled structurally by mechanism 6 rather than by the choice of literals. The units differ on purpose: the barrier's in raw ticks, so it cannot be stretched by the very traffic keeping the system non-idle;scheduleWhenIdle's in poll rounds, each of which waits out a fullpromiseQueuedrain, because that function also schedules workflow suspensions, where firing early preempts data delivery.3. "Delivered" now means the woken branch quiesced.
markDelivered()drops the entry from the live registry immediately, as before, but also records it in a newrecentlyDeliveredBarriersmap for one macrotask — the same yield a deferring delivery would have paid.awaitEarlierDeliveriesgates on both maps. That closes the window betweenmarkDelivered()and the woken branch reaching its next suspension point, for deliveries consumed in a later drain window and for the bufferedclaim()path, without makingclaim()capture its deferral at consumption time (that variant was tried previously and rejected — it stalls thehookWithSleepWorkflowe2e).4. Parked deliveries count as in-flight.
awaitEarlierDeliveriesmaintainspendingOrderedDeliverieswhile a delivery is gated or paying its macrotask yield, andscheduleWhenIdletreats that as busy. Without it, a delivery whose only remaining gate is a quiescing entry is invisible to everything:pendingDeliverieswas released in its hydration slot, and a scan of the live registry finds it empty. A suspension armed in that window preempts the delivery and the run suspends carrying none of the work it was about to create. This is not hypothetical — it is what mechanism 3 cost on its own, and it is how the second payload of thehookWithSleepWorkflowe2e went missing (found in a full local e2e run, see below). Gating on it cannot deadlock, because barrier retirement no longer goes throughscheduleWhenIdle: the unarmed payload at the root of any parked chain is retired by its own poll while the counter is up.5. The step delivery's buffered-hook skip is narrowed to the payload itself. A step still never gates on a buffered payload no consumer has claimed — that is the case where the claim commonly sits downstream of the step result, and gating there would stall the run. But it now gates on an earlier armed wait or hook whatever that delivery is itself waiting for. The old rule skipped those too, via a transitive
resolvesOnItsOwnwalk, and that is precisely the buffered-claim corruption: in thePromise.all([step, sleep-then-read-hook])shape the wait completion is what wakes the branch that goes on to claim the payload, so skipping it let the step result overtake a hook read the log had ordered first.awaitEarlierDeliveriesno longer consults the transitive walk at all, so the per-delivery path is a single linear pass; the walk survives only in #3198'shasParkedCommittedDelivery, which gates suspensions rather than deliveries.6. The round budget does not count against a delivery waiting on the abandon deadline. The two deadlines are the only ways out of one specific state — an unclaimed payload, a committed
wait_completedgated on it (DEFER_BEHIND.waitincludeshook, and the unarmed skip in mechanism 5 is on the step path only), sustained traffic so the idle route cannot retire the payload, and some other consumer armingscheduleWhenIdle. Nothing callsmarkDelivered()while that wait is parked, sodeliveryProgressnever advances and the budget from mechanism 2 runs flat to exhaustion before 32 ticks elapse. The suspension then preempts exactly the delivery mechanism 4 exists to protect.scheduleWhenIdlenow also resets its budget whilependingOrderedDeliveries > 0and an unarmed barrier is registered. Both conjuncts are required: without the first, a pure poke storm (hooks defer behind waits and steps only, so nothing parks) would keep an unarmed barrier registered forever and the budget would never expire — the starvation this PR added the deadline for; without the second, a call site that leaks an armed barrier would extend the budget indefinitely instead of reaching the callback as it does today.Mechanisms 1 and 5 are load-bearing for each other: the step → wait → unclaimed-payload chain that 5 creates is not a deadlock only because 1 leaves the payload at its root retirable.
Validation
hook-sleep-interaction,step-delivery-ordering,step-delivery-hop-count,delivery-barrier-coverage@workflow/coresuiteee944d247; 1677 / 3 / 0 (79 files) before it, against 1655 passed / 12 expected fail / 4 failed at the branch pointdc0d25eaf. The 12 expected-fails there are this PR's markers plus the 3 that survive here (2 hook-sleep known-invalid, 1 unrelatedworkflow.test.tsDate-determinism); the 4 failures are explained below. The passing count rises by 22: 12 flipped markers, 6 cases added in review, 1 e2e regression guard, and 3 added while investigating the storm.nextjs-turbopackdev, port 3400)hookWithSleepWorkflow— which failed until mechanism 4 landed. Re-run isolated on the final tree: 4/4 greenresolvesOnItsOwnwalk is off that path. Performance Benchmarks CI is the arbiter.An e2e regression the unit suites could not see. The first full local e2e run after the review changes failed on
hookWithSleepWorkflow: the second hook payload was never delivered. Bisecting againstdc0d25eaf(passes) and the first fix commit (fails) confirmed it was mine, and tracing the barrier registry showed the claim's only remaining gate was a quiescing entry, with the live registry empty andpendingDeliveriesat 0 — so the suspension fired first. Mechanism 4 is the fix, with a unit test that fails without it. Worth stating plainly because an earlier full-suite e2e run had passed 135/135 with the bug present: the test only fails at certain timings, so the isolated-trun is the reliable signal.Note on the branch point
dc0d25eaf, which is why they show as failures there while still markedit.fails.IDLE_POLL_DEADLINE_ROUNDS— bounded, and suspensions still fire. The starvation suite now uses the unclaimed payload, which is the delivery that genuinely has no chain of its own.Review
Everything actionable from @VaguelySerious's review is folded in, on merit:
pendingDeliverieswas held above zero by one unmatched increment, i.e. a stuck hydration, not overlapping traffic. Rewritten to overlap genuinely with no leak, and the control now asserts both properties (never zero while running, back to zero after). The header no longer attributes the observed 3m19s stall to this mechanism; the suite does not establish that, and says so.mainand here.step.tsdocuments the old tradeoff as deliberate. It did, and the fix changes it — those comments are corrected rather than left to contradict the code.world-postgresclass? No.world-vercelreads through workflow-server, which keys events in DynamoDB by event ULID and paginates with aneid:cursor —between({eventId: cursor}, {eventId: MAX})onbyWorkflowRunId, an ID-ordered range strictly after the cursor. The IDs are minted per server instance, and the server's own comment says its monotonic factory "only fixes intra-instance ordering. Cross-instance ordering for the same run still depends on wall-clock agreement." Recorded in the suite header.it.failspasses on any throw. Moot — all 12 are flipped.setupWorkflowContext/replay/eventharness shared by five suites. Worth doing, but it edits three currently-green suites in a fix PR; better as its own change.Round 2 (2026-07-31)
Everything from @VaguelySerious's second review and @TooTallNate's verification review is folded in. The branch is rebased onto
mainee944d247(dropping thedc0d25eamerge commit and theorganizeImportslint failure it carried), so the history is linear again.92831b16f— mechanism 6 above. Peter's repro is now a test, and it fails on the parent commit.ca8a316e0had fixed the copy instep.tsand left the original standing. Corrected in7ab411223, along with the consequence stated below it, which was wrong in a worse direction: a leaked armed barrier is not bounded byIDLE_POLL_DEADLINE_ROUNDSat all — that releases the idle callbacks, not the barrier — so violating the INVARIANT is unrecoverable rather than merely slow, and the docstring now says so.6f1d3de62). RaisingIDLE_POLL_DEADLINE_ROUNDScan no longer make the flat-count test pass vacuously.envNumberlike every other timing knob, read once per clock and floored at 1 (6f1d3de62). The skew argument is what decided it: a run stays on the deployment it started on, so a code-only correction never reaches the runs already in the bad state.scheduleBarrierRetirementchecks onlypendingDeliveries, unlikescheduleWhenIdletwenty lines down (@TooTallNate). The asymmetry is required — counting parked deliveries there would mean a payload could never retire by the idle route while anything was gated on it — and is now documented so the next reader does not "fix" it into a deadlock.beginQuiescingruns for safety-net retirements too (@TooTallNate). Correct and intended; now stated, since therecentlyDeliveredBarriersdocblock describes its entries as deliveries handed to the workflow, which a net-retired payload never was.E2E Vercel Prodfailures are the deployment-key 429 quota, and theLintfailure came in with the merge commit. The rebase clears the latter; the former is a matrix-wide problem worth its own issue.Scope — what this does not fix
This closes the engine class. Two other contributors to the storm are tracked separately and are expected to keep some corruption on the board:
The measurement to watch is the step-storm number, where the engine class dominates: baseline on
mainis 583/600 corrupted (overall 698/1400; hook-storm 115/600, hook-sleep 0/200 — PR #3189, run 30481289312). This PR carries theevent-log-race-reprolabel, so each push triggers the ~90 min storm job; the delta will be reported here when it lands. Substantial improvement is the expectation, not zero.🤖 Generated with Claude Code