[core] Allow fast-path execution while there are open waits not close to resumeAt - #4124
Conversation
🦋 Changeset detectedLatest commit: 68ad8fa 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 |
📊 Workflow Benchmarkscommit Backend:
Streams
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 162443ms → this run 166827ms (Δ +4384ms, +3%) 📈 CRTT drill-down vs main (RTT distributions & profiles)RTT over stream progress (avg per tenth of stream, bars scaled min→max): RTT by chunk size (avg per log size bin, ~160B → ~12KB serialized, bars scaled min→max): Delivery jitter over stream progress (avg positive CDV per tenth of stream, bars scaled min→max): ℹ️ Metric definitions & methodologyStreams: first-chunk RTT (the stream-open path, before any buffering/backpressure), CRTT percentiles, and worst delivery stall (CDV max). Cells are medians across iterations; per-run values in the artifacts. No 🔴/🟢 marks until targets attach. The collapsed STSO distribution section above buckets every step gap, split inline (same warm process — pure framework overhead) vs queue-hop (fresh process — dispatch, reinit, replay). The collapsed CRTT drill-down: per-variant RTT histograms (fixed log bins, 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) · Fan-out TTFS: fan-out time to first step (in-deployment start() → first of the parallel step bodies to complete) · Fan-out TTLS: fan-out time to last step (in-deployment start() → last of the parallel step bodies to complete, i.e. when the Promise.all resolves) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · CRTT: chunk round-trip time (per-chunk write → read latency, one clock domain: deployment → stream backend → same deployment) · CDV: chunk delay variation / delivery jitter (inter-arrival gap minus inter-write gap per seq-adjacent pair; skew-free; the row is each run's MAX positive value, so one stall moves it) 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 · Promise.all(100 steps): 100 trivial no-op steps started together in a single Promise.all; Fan-out TTFS is the first of them to complete and Fan-out TTLS the last, both from the in-deployment clientStart, so their gap is the spread the runtime adds across the fan-out · paced control (100/s, 60B): the control: 300 tiny (~60B) deltas metronome-paced at 100/s — zero workload structure, so it reads the transport floor and flush cadence, and disambiguates transport-wide vs workload-specific when a replay row moves · size sweep (100/s, 160B-12KB): same pacing as the control with deltas padded in rotation across seven log-spaced sizes (~160B–12KB) — rotation decouples size from stream position, so it isolates whether chunk size causes latency · replay gateway-gpt-5.4-nano-2000t (1x): raw provider SSE cadence captured at the AI gateway boundary (gpt-5.4-nano, the most popular gateway model; per-token deltas p50 208B = the modal production chunk size), replayed exactly as measured — the typical customer's workload; its CDV is the typical customer's real delivery jitter · replay eve-gpt-5.6-sol-2000t (1x): a captured eve turn (gpt-5.6-sol, the most-used demanding eve model; ~2000 output tokens = production p50 turn length) replayed exactly as measured — eve's envelope protocol re-ships the cumulative message so sizes ramp 142B→13KB; the demanding outlier tenant's reality · replay eve-gpt-5.6-sol-2000t (2x): the same eve capture at 2x — the headroom/stress row; real fast-tier models emit the same chunk sizes at proportionally higher rate, so time compression is a faithful speed model · first chunk (pooled): every run's seq-0 RTT pooled across all stream scenarios — the first chunk precedes any workload differentiation, so pooling samples one shared stream-open path with exact percentiles Replay cadences (semantic sha256) — eve-gpt-5.6-sol-2000t 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 All timestamps are deployment-side; runs are triggered in-deployment, so the CI runner and api.vercel.com sit outside every measured window. TTFS = Cold starts stay in the numbers (real bursty-workload latency, inflates P75+); Best is the warm floor. |
🧪 E2E Test Results✅ All tests passed
|
| Passed | Failed | Skipped | Total | |
|---|---|---|---|---|
| ✅ ▲ Vercel Production | 3662 | 0 | 685 | 4347 |
| ✅ 💻 Local Development | 3998 | 0 | 510 | 4508 |
| ✅ 📦 Local Production | 3998 | 0 | 510 | 4508 |
| ✅ 🐘 Local Postgres | 3998 | 0 | 510 | 4508 |
| ✅ 🪟 Windows | 320 | 0 | 2 | 322 |
| ✅ 🌐 Cross-language Conformance | 68 | 0 | 74 | 142 |
| ✅ vercel-http-transport | 823 | 0 | 143 | 966 |
| ✅ vercel-multi-region | 27 | 0 | 0 | 27 |
| ✅ vercel-ws-transport | 557 | 0 | 87 | 644 |
| Total | 17451 | 0 | 2521 | 19972 |
Details by Category
✅ ▲ Vercel Production
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-node | 133 | 0 | 28 |
| ✅ astro-quickjs | 133 | 0 | 28 |
| ✅ example-node | 133 | 0 | 28 |
| ✅ example-quickjs | 133 | 0 | 28 |
| ✅ express-node | 133 | 0 | 28 |
| ✅ express-quickjs | 133 | 0 | 28 |
| ✅ fastify-node | 133 | 0 | 28 |
| ✅ fastify-quickjs | 133 | 0 | 28 |
| ✅ hono-node | 133 | 0 | 28 |
| ✅ hono-quickjs | 133 | 0 | 28 |
| ✅ nest-node | 133 | 0 | 28 |
| ✅ nest-quickjs | 133 | 0 | 28 |
| ✅ nextjs-turbopack-node | 158 | 0 | 3 |
| ✅ nextjs-turbopack-quickjs | 158 | 0 | 3 |
| ✅ nextjs-webpack-node | 158 | 0 | 3 |
| ✅ nextjs-webpack-quickjs | 158 | 0 | 3 |
| ✅ nitro-node | 133 | 0 | 28 |
| ✅ nitro-quickjs | 133 | 0 | 28 |
| ✅ nuxt-node | 133 | 0 | 28 |
| ✅ nuxt-quickjs | 133 | 0 | 28 |
| ✅ python-node | 66 | 0 | 95 |
| ✅ sveltekit-node | 152 | 0 | 9 |
| ✅ sveltekit-quickjs | 152 | 0 | 9 |
| ✅ tanstack-start-node | 133 | 0 | 28 |
| ✅ tanstack-start-quickjs | 133 | 0 | 28 |
| ✅ vite-node | 133 | 0 | 28 |
| ✅ vite-quickjs | 133 | 0 | 28 |
✅ 💻 Local Development
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 27 |
| ✅ astro-stable-quickjs | 134 | 0 | 27 |
| ✅ express-stable-node | 134 | 0 | 27 |
| ✅ express-stable-quickjs | 134 | 0 | 27 |
| ✅ fastify-stable-node | 134 | 0 | 27 |
| ✅ fastify-stable-quickjs | 134 | 0 | 27 |
| ✅ hono-stable-node | 134 | 0 | 27 |
| ✅ hono-stable-quickjs | 134 | 0 | 27 |
| ✅ nest-stable-node | 134 | 0 | 27 |
| ✅ nest-stable-quickjs | 134 | 0 | 27 |
| ✅ nextjs-turbopack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 160 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 27 |
| ✅ nitro-stable-quickjs | 134 | 0 | 27 |
| ✅ nuxt-stable-node | 134 | 0 | 27 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 27 |
| ✅ sveltekit-stable-node | 153 | 0 | 8 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 8 |
| ✅ tanstack-start-node | 134 | 0 | 27 |
| ✅ tanstack-start-quickjs | 134 | 0 | 27 |
| ✅ vite-stable-node | 134 | 0 | 27 |
| ✅ vite-stable-quickjs | 134 | 0 | 27 |
✅ 📦 Local Production
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 27 |
| ✅ astro-stable-quickjs | 134 | 0 | 27 |
| ✅ express-stable-node | 134 | 0 | 27 |
| ✅ express-stable-quickjs | 134 | 0 | 27 |
| ✅ fastify-stable-node | 134 | 0 | 27 |
| ✅ fastify-stable-quickjs | 134 | 0 | 27 |
| ✅ hono-stable-node | 134 | 0 | 27 |
| ✅ hono-stable-quickjs | 134 | 0 | 27 |
| ✅ nest-stable-node | 134 | 0 | 27 |
| ✅ nest-stable-quickjs | 134 | 0 | 27 |
| ✅ nextjs-turbopack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 160 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 27 |
| ✅ nitro-stable-quickjs | 134 | 0 | 27 |
| ✅ nuxt-stable-node | 134 | 0 | 27 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 27 |
| ✅ sveltekit-stable-node | 153 | 0 | 8 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 8 |
| ✅ tanstack-start-node | 134 | 0 | 27 |
| ✅ tanstack-start-quickjs | 134 | 0 | 27 |
| ✅ vite-stable-node | 134 | 0 | 27 |
| ✅ vite-stable-quickjs | 134 | 0 | 27 |
✅ 🐘 Local Postgres
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 27 |
| ✅ astro-stable-quickjs | 134 | 0 | 27 |
| ✅ express-stable-node | 134 | 0 | 27 |
| ✅ express-stable-quickjs | 134 | 0 | 27 |
| ✅ fastify-stable-node | 134 | 0 | 27 |
| ✅ fastify-stable-quickjs | 134 | 0 | 27 |
| ✅ hono-stable-node | 134 | 0 | 27 |
| ✅ hono-stable-quickjs | 134 | 0 | 27 |
| ✅ nest-stable-node | 134 | 0 | 27 |
| ✅ nest-stable-quickjs | 134 | 0 | 27 |
| ✅ nextjs-turbopack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 160 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 160 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 27 |
| ✅ nitro-stable-quickjs | 134 | 0 | 27 |
| ✅ nuxt-stable-node | 134 | 0 | 27 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 27 |
| ✅ sveltekit-stable-node | 153 | 0 | 8 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 8 |
| ✅ tanstack-start-node | 134 | 0 | 27 |
| ✅ tanstack-start-quickjs | 134 | 0 | 27 |
| ✅ vite-stable-node | 134 | 0 | 27 |
| ✅ vite-stable-quickjs | 134 | 0 | 27 |
✅ 🪟 Windows
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ nextjs-turbopack-node | 160 | 0 | 1 |
| ✅ nextjs-turbopack-quickjs | 160 | 0 | 1 |
✅ 🌐 Cross-language Conformance
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ python | 68 | 0 | 74 |
✅ vercel-http-transport
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ example | 133 | 0 | 28 |
| ✅ express | 133 | 0 | 28 |
| ✅ hono | 133 | 0 | 28 |
| ✅ nextjs-turbopack | 158 | 0 | 3 |
| ✅ nitro | 133 | 0 | 28 |
| ✅ vite | 133 | 0 | 28 |
✅ vercel-multi-region
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ nextjs-turbopack | 27 | 0 | 0 |
✅ vercel-ws-transport
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ example | 133 | 0 | 28 |
| ✅ express | 133 | 0 | 28 |
| ✅ nextjs-turbopack | 158 | 0 | 3 |
| ✅ vite | 133 | 0 | 28 |
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 41 total
Full trace: |
About these numbersSizes are gzip; parentheses show the change against
|
| ownedRecoverySteps.length === 0 && | ||
| !suspensionResult.waitTimeout && | ||
| !openHookWaitState.openWait; | ||
| !openWaitDueThisInvocation; |
There was a problem hiding this comment.
[P2] The remaining wait checks still disable both fast paths
A sleep() that loses Promise.race() stays in ctx.invocationsQueue until wait_completed. Subsequent suspensions therefore still have err.waitCount > 0 and a populated suspensionResult.waitTimeout. This gate rejects both, and forceOptimisticStart also still rejects waitTimeout, so returning false from openWaitDueThisInvocation does not enable either optimization for the motivating case.
I reproduced this through workflowEntrypoint on d0336a9: after await Promise.race([stepA(), sleep('24h')]); await stepB();, step B's terminal write has no sinceCursor. The same two-step workflow without the sleep requests the delta. All 68 existing tests in runtime.test.ts and open-hook-wait-state.test.ts passed, but this added regression failed. These remaining checks are unchanged at the current head.
Could these checks also account for already-created, far-future waits, with a runtime-level regression covering the losing-sleep case?
Local agent review (`openai/gpt-6-astra`)
There was a problem hiding this comment.
(AI) Confirmed and fixed in 5ebbff9. Reproduced first with the retained-vm-loop harness: Promise.race([sleep('1h'), s1()]) then s2() did 3 events.list calls vs 1 for two plain steps, so the deadline predicate on the log scan was never the binding term.
suspensionResult.waitTimeout now carries resumeAtMs, and the delta gate tests the soonest deadline across both the log scan and the suspension queue (waitDueThisInvocation); err.waitCount === 0 and !suspensionResult.waitTimeout are gone from that gate. Regression added at retained-vm-loop.test.ts ("a sleep that lost a race does not cost an events.list per step boundary"): listCalls === 1, both terminal writes carry sinceCursor. It fails with err.waitCount === 0 re-added.
forceOptimisticStart deliberately keeps rejecting waitTimeout and any open wait; see the reply to the other review for why.
pranaygp
left a comment
There was a problem hiding this comment.
Read the full diff, ran the tests, and built a repro against the existing retained-vm-loop harness. The reasoning in the description is careful and open-hook-wait-state.ts is clean and well tested in isolation, but I don't think the change does what it says on the tin yet, and relaxing the gate opens a hole on the manual "cancel sleep" path that the current write-up doesn't cover.
Correctness
1. Both gates are still shadowed by suspensionResult.waitTimeout / err.waitCount, so the change is inert
packages/core/src/runtime.ts:4276-4284:
const requestInlineDelta =
typeof eventLog.cursor === 'string' &&
err.stepCount === 1 &&
err.waitCount === 0 && // ← still gates on wait existence
...
!suspensionResult.waitTimeout && // ← still gates on wait existence
!openWaitDueThisInvocation; // ← the term this PR relaxedpackages/core/src/runtime.ts:4362-4369:
const forceOptimisticStart =
turbo &&
!suspensionResult.hasAttributeEvents &&
!suspensionResult.waitTimeout && // ← same
...
!openWaitDueThisInvocation;waitTimeout is derived from every wait item in the VM's suspension queue, not only newly created ones — suspension-handler.ts:578 filters suspension.items by type === 'wait' with no hasCreatedEvent filter, and suspension-handler.ts:1899-1910 takes the soonest resumeAt across all of them. err.waitCount (global.ts:108) counts the same items. A sleep('24h') that lost a Promise.race() stays in that queue for exactly as long as it stays open in the log — which is the premise of this PR. So at every later suspension waitTimeout is set and waitCount >= 1, and both conjunctions are false regardless of what openWaitDueThisInvocation says.
Measured, using the harness already in the repo. packages/core/src/retained-vm-loop.test.ts happens to define openWaitRaceWorkflow = Promise.race([sleep("1h"), s1()]) then s2(), and drive() returns a listCalls count. The fake world exposes no getRuntimeDeadline, so the window is the 2-minute fallback and a 1h wait sits well outside it:
| config | listCalls |
|---|---|
| this branch, default skew | 3 |
this branch, WORKFLOW_OPEN_WAIT_CLOCK_SKEW_MS=31536000000 (i.e. the old "gate on any open wait") |
3 |
| two plain steps, no wait at all | 1 |
Instrumenting the gate at the second suspension confirms the new predicate works and simply isn't the binding term:
{ waitCount: 1, waitTimeout: 3600, openWait: true, openWaitDue: false, lazyInlineSteps: 1, ... }
Relaxing err.waitCount === 0 and !suspensionResult.waitTimeout as well drops listCalls 3 → 1 with the run still returning the correct result (30). So the delta path does work for this shape; those two untouched conditions are what keep it off.
For turbo the shadowing is total rather than partial. Turbo is the start delivery only, so the log is empty at invocation start and an open wait can only exist because this invocation's workflow created one — which means that suspension and every later one in the invocation report waitTimeout. !openHookWaitState.openWait in the turbo conjunction was already dead code before this PR, and !openWaitDueThisInvocation is dead code after it.
Net: as it stands this adds a module, an env var, a docs section and a changeset promising a user-visible improvement, and changes no observable behaviour. The changeset text ("no longer disables the per-step event-log delta or turbo's optimistic start") will ship as false. Either apply the deadline predicate to the waitTimeout / waitCount terms too — gate on "soonest wait due within the window" rather than "any wait in the queue" — or land this as groundwork and drop the changeset claim.
2. A wait can be completed early from the UI, which is the premise the whole bound rests on
This is the part I'd most like reworked before finding 1 is fixed, because fixing finding 1 is what makes it live.
The bound assumes the only writer of a wait_completed is the wait's own timer, so a wait due after the inline window cannot produce one during this invocation. That is not true: wakeUpRun (runtime/runs.ts:294-360) walks every wait_created with no wait_completed and writes the completion immediately, ignoring resumeAt. It is not an operator escape hatch buried in an admin tool — it is:
- exported from
@workflow/core(runtime.ts:185), - the public
run.wakeUp()(runtime/run.ts:277), - a "Cancel sleeps" button on every run in the dashboard (
packages/web/app/components/run-actions.tsx:118-140, toast: "Cancelled N active sleeps and resumed the run"), - and a per-sleep wake button in the run detail view, which passes a single
correlationId(packages/web/app/components/run-detail-view.tsx:279-284).
So the exact shape this PR is optimising for — a long sleep() left open after losing a race — is also the shape whose "cancel this sleep" button a human is most likely to press.
The current write-up does address this, at runtime/constants.ts:536-545, but I think both halves of that argument are wrong:
Such a
wait_completedthat lands after the step's terminal write is absent from that write's delta and is observed by the next read instead … so it is absorbed late rather than lost.
(a) For the inline delta, "absorbed late" can mean "absorbed in 24 hours". A missed hook_received is safe to observe an iteration late because the hook's own delivery re-drives the run — that is exactly the argument at runtime.ts:4243-4262, and it holds. A missed wait_completed is different, and it's different for a reason the existing comment at runtime.ts:4225-4230 already half-states: when a replay reaches a pending wait it doesn't just carry on, it parks the run on a continuation armed for resumeAt (runtime/wait-continuation.ts). Concretely:
- Run is on the fast path, mid-inline-step, with a 24h sleep open.
- Someone clicks "Cancel sleeps".
wait_completedis durable; one wake message is enqueued (runs.ts:367-378— note it carries no idempotency key and nowaitContinuationmarker, so it is a single plain delivery). - The wake delivery finds the step inline-owned by a live lease, defers, and arms a backstop wake.
- The in-flight invocation finishes its step, takes a delta that does not contain the
wait_completed(it landed above the terminal write's slot), replays over the prefix, finds the sleep unresolved, and arms a wait continuation for the originalresumeAt— chained at 23h (WAIT_CONTINUATION_MAX_DELAY_SECONDS). - Recovery now rests entirely on the backstop from step 3 firing and re-driving.
If that backstop lands, the run resumes and the only cost is a wasted round. If it doesn't — it deferred again against a still-owned step, or the wake arrived before the claim so no backstop was armed — the run has a durable wait_completed in its log and nothing scheduled to look at it for up to 23 hours, which to the user is "I pressed the button and nothing happened". Worth noting too that continuation messages are keyed on the wait's correlationId with attempt = 0 (wait-continuation.ts:84-141), so a later suspension pass cannot simply arm a shorter one; the attempt counter only advances when an invocation recognises itself as a continuation for a still-pending wait.
I want to be straight about confidence here: I traced this mechanism, I did not reproduce the stuck run. The backstop may well close it in every real interleaving. But "does the button still work" currently depends on an untested race, where before this PR it depended on nothing at all (any open wait forced a real events.list()), and that regression isn't acknowledged anywhere in the PR.
(b) The turbo half of the argument is backwards.
The resume invocation it spawns is not turbo (turbo is the start delivery only), so it awaits its claims.
The hazard isn't that the peer is turbo — it's that this invocation forces a body ahead of its claim while some peer races for that claim. The description states it exactly: "if the resume's awaited claim wins, a body has run for a claim that was lost." An awaited-claim peer is the losing case, not the safe one. Before this PR any open wait latched turbo off, so a manual wake could never race a turbo-forced body. After it (once finding 1 is also fixed), a far-future sleep plus a dashboard click is precisely that race, and the failure mode is a double-executed step body rather than a delay.
Proposed solutions
I agree with the framing that a rare manual action shouldn't cost anything on the steady-state path. Options, roughly in the order I'd pick them:
A. Pay the read only where the run is about to park (recommended). Keep the deadline relaxation for ordinary step boundaries, but when a replay that consumed a delta (rather than a read) is about to suspend on a pending wait, do the events.list() then. The cost lands on exactly one boundary — the one that is a hand-off anyway, so it's amortised against a queue hop — and it is zero on every boundary that just runs the next step. This is the smallest change that makes "the button always works" true by construction rather than by race.
B. Narrow A further by continuation length. Same as A, but only re-read when the continuation about to be armed is long (say timeoutSeconds beyond the inline window, or beyond NEAR_ELAPSED_WAIT_THRESHOLD_SECONDS). A short sleep self-heals within seconds when the continuation fires, so it doesn't need the read; only the long ones can strand. Cheaper than A and targets the actual failure.
C. Keep turbo conservative; only relax the delta gate. The two failure modes aren't symmetric: the delta path risks a delayed wake (recoverable), turbo risks a double-executed body (not). Letting forceOptimisticStart keep latching off on any open wait costs almost nothing — turbo is the start delivery, and a run that opens a wait during its start delivery is not the hot path this PR is about — while the delta gate gets the full deadline treatment. Combines fine with A or B.
D. Restore the invariant at the writer. Make wakeUpRun durably record that the wait is now due before completing it, so "no wait_completed can exist before resumeAt" is readable from the log alone and the gate's premise is actually true. Note it can't just write wait_completed with resumeAt: now — workflow/sleep.ts:70-83 raises ReplayDivergenceError on a resumeAt mismatch, which is why the current code copies the original value forward (runs.ts:348). So this needs its own event type and a spec bump. Most correct, most expensive; probably not worth it for this.
E. Disable the UI button. I'd argue against it. Cancelling a stuck sleep is genuinely useful, and the same path is reachable as run.wakeUp() from the public API, so hiding the button narrows the blast radius without closing the hole — and it trades a real user-facing capability for an optimisation, which seems like the wrong side of that trade.
My vote: B + C, with a test for each.
Risks
3. noInlineReplayAfterMs isn't quite an upper bound on the last forced claim
The budget check sits at the top of the loop, before replay (runtime.ts:2787-2790). So the last forced step_started of an invocation can be issued at invocationStartTime + noInlineReplayAfterMs + (one replay pass) + (barrier/RTT), not at the window edge. The 30s skew silently absorbs that on top of its stated job (clock skew between this process, the timer queue and the wait_completed writer). Probably fine in practice, but the comment presents 30s as "seconds at most … an order of magnitude to spare" for clock skew alone, which understates what it's covering. Either say so, or take the bound at the end of the iteration rather than its start.
4. "Raising the function's duration widens the window" isn't true
getMaxInlineDurationMs (runtime.ts:630-658) is tiered and capped: deadline ≥ 25m → 10m, ≥ 10m → 5m, else 2m. Only WORKFLOW_V2_TIMEOUT_MS goes higher. So the window from function duration never exceeds 10 minutes, and "to hours" is reachable only via the env var. This appears in three places — the PR body, constants.ts:524-526, and docs/content/docs/v5/configuration/runtime-tuning.mdx:105. The error is in the conservative direction (the real window is shorter than claimed), so it's a docs fix, but the test named 'follows a longer inline window (multi-hour function durations)' bakes the misconception in.
Missing tests
5. Nothing tests the runtime wiring
open-hook-wait-state.test.ts is thorough (20 cases, all passing) but covers only the pure predicate. Neither gate site, nor the invocationStartTime + noInlineReplayAfterMs + skew deadline computation, has a test. retained-vm-loop.test.ts already has both the workflow shape (openWaitRaceWorkflow) and the assertion vocabulary (listCalls) — one case there asserting listCalls === 1 for a far-future open wait would have caught finding 1 immediately. That's the test this PR most needs.
6. No test for the out-of-band wait_completed the relaxed gate now admits
The existing { type: 'inject-wait' } mode injects wait_completed at step_started (retained-vm-loop.test.ts:618), i.e. inside the delta the terminal write returns, so it never exercises the case the comment reasons about. A mode that injects after step_completed — the wakeUpRun shape — is what would test the "absorbed late rather than lost" claim, and would pin down whether the backstop in finding 2(a) actually closes it.
Nits
hasOpenWaitDueBy(open-hook-wait-state.ts:110-118) checks bothopenWaitandearliestOpenWaitResumeAtMs === undefined; these are the same condition by construction. DroppingopenWaitfrom thePickremoves a second source of truth.getOpenWaitClockSkewMs()is re-read fromprocess.envon every suspension (runtime.ts:4206). Hoisting it next tonoInlineReplayAfterMsatruntime.ts:915-919would make the deadline an invocation-scoped constant, matching how the rest of the window is computed.envNumberrejects non-finite values, soWORKFLOW_OPEN_WAIT_CLOCK_SKEW_MS=Infinitysilently falls back to 30s rather than restoring the old gating. The docs' "a very large value" phrasing invites exactly that; worth naming a concrete value.
Verification performed
pnpm --filter @workflow/core test— 113 files passed, 2449 tests passed, 1 skipped, 3 expected-fail. Green.tsc --noEmitinpackages/core— clean.biome checkon the four changed/added source files — only pre-existingnoExcessiveCognitiveComplexitywarnings on untouched parts ofruntime.ts.- The
listCallsrepro in finding 1 (temporary edits, reverted). - (
pnpm typecheckat the root fails in@workflow/swc-plugin#buildin my environment — unrelated to this change.)
Recommendation
Request changes — the change is safe as written but is a no-op for the problem it describes, and the waitTimeout / waitCount terms need the same deadline treatment before the changeset claim is true. When that lands, the manually-cancelled-sleep path in finding 2 needs to be closed with it.
…parking The inline-delta gate also tested `err.waitCount === 0` and `!suspensionResult.waitTimeout`, both of which count every wait still in the workflow's queue. A `sleep()` that lost a `Promise.race` stays queued until its `wait_completed`, so the deadline predicate on the log scan never became the binding term and the losing-sleep shape still read the log at every boundary. `waitTimeout` now carries the soonest wait's `resumeAtMs`, and the gate tests the soonest deadline across both the log scan and the queue against the invocation's window. Turbo's forced optimistic start goes back to latching off on any open wait. A wake from `run.wakeUp()` is a peer that can race a forced body for its claim, and a body executed twice is not repairable the way a delayed wake is. `run.wakeUp()` also completes waits regardless of `resumeAt`. A completion it lands after a step's terminal write is above that write's inline delta, so before parking on a wait over a delta-extended log the runtime re-reads once; a completion that exists is replayed over instead of arming a continuation for the original deadline. Runtime-level regressions in retained-vm-loop.test.ts cover the losing-sleep list count, the gated variant, the wakeUp-shaped completion above the delta, and the park after an unchanged re-read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(AI) Addressing the review point by point. All changes in 5ebbff9; PR body rewritten to match. 1. Gates shadowed by 2.
3. 4. "Raising the function's duration widens the window." Fixed in the constant comment, the docs, and the test name: the tier tops out at 10 minutes, and only 5 / 6. Runtime-level tests. Four added in Nits. Not changed: the QuickJS engine's equivalent |
| // - No pending wait, whether created by this | ||
| // suspension or still queued from an earlier one, | ||
| // that can fire before this invocation's inline | ||
| // window ends (`waitDueThisInvocation`). A | ||
| // `wait_completed` is a resolution the replay is | ||
| // waiting on rather than an event it can observe | ||
| // one iteration late, so consuming a delta that | ||
| // predates it would settle the sleep from a view | ||
| // that does not contain its completion. A wait's | ||
| // timer cannot produce one before `resumeAt`, so a | ||
| // wait due after this invocation has handed the | ||
| // run off is no hazard here, and since nothing | ||
| // disposes a wait, a `sleep()` that lost a race | ||
| // against a hook would otherwise hold every later | ||
| // boundary of the run on the fetch path. The one | ||
| // writer that ignores `resumeAt`, `run.wakeUp()`, | ||
| // is covered by the re-read before parking above: | ||
| // a completion it lands after a step's terminal | ||
| // write is read before this invocation would arm | ||
| // a continuation over it. See | ||
| // `OPEN_WAIT_CLOCK_SKEW_MS` for the bound. |
There was a problem hiding this comment.
nit: this combo of verbose comments and deep nesting is really something to behold
There was a problem hiding this comment.
Maybe it's time for a refactor-only pass to reduce nesting and comments
The re-read fired on every `await step(); await sleep()` boundary, since the sleep created by the parking suspension sets `waitTimeout` while the preceding step's delta set the flag. A wait created after the delta's terminal write cannot have a completion above that delta, so the read protected nothing there. It now also requires an open wait in the log the replay ran over (`openHookAndWaitState().openWait`; wait creates do not fold a delta back, so a wait from this suspension is not in that scan), which is exactly the `Promise.race` loser the gate exists for. The suspension-queue deadline test is written as `!(resumeAt > deadline)` so a NaN deadline gates, matching the log scan. A span attribute, `workflow.inline_delta_over_pending_wait`, marks batches where the delta was taken while a wait is pending. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No backport to This is a performance optimization: it relaxes the inline-delta gate so a far-future pending wait no longer costs an extra To override, re-run the Backport to stable workflow manually via |
Part of #3916 (option 1). Not a full fix for that issue: the 20x regression reported there was dominated by VM retention being disabled, which #3892 addressed in beta.48. This PR covers one remaining item: a pending wait that cannot fire during the current invocation should not cost an extra event-log read per step boundary.
Problem
Promise.race([hook, sleep('24h')])where the hook wins leaves thesleepopen until its timer elapses. There is no event that disposes a wait. The inline-delta gate inruntime.ts(requestInlineDelta; when false, the runtime falls back to an incrementalevents.list()per step boundary) required no pending wait at all, from three terms:openHookAndWaitState(...).openWaitover the log,err.waitCount === 0over the suspension queue, and!suspensionResult.waitTimeout, also over the queue. A losing sleep stays in the queue until itswait_completed, so all three stayed true for the rest of the run.Measured with the
retained-vm-loopharness (Promise.race([sleep('1h'), s1()])thens2()): 3events.listcalls per run onmain, versus 1 for the same two steps without the sleep.Change
wait_created.eventData.resumeAtis durable and already in the log, and the suspension queue knows each pending wait'sresumeAttoo. The gate now asks whether the soonest pending wait, across both sources, is due before this invocation's inline window closes:noInlineReplayAfterMsis the existing inline budget fromgetMaxInlineDurationMs:WORKFLOW_V2_TIMEOUT_MS, else a tier offworld.getRuntimeDeadline()(capped at 10 minutes), else 2 minutes. It is the point past which the loop stops scheduling inline batches, so it is the last moment this invocation can still be exposed. A wait is gating when its deadline is at or before that point, already past, or unreadable.openHookAndWaitStatemoved toruntime/open-hook-wait-state.ts(exported for tests) and now records the earliest open wait deadline.openWaitkeeps its old meaning.suspensionResult.waitTimeoutcarriesresumeAtMsalongsideseconds.err.waitCount === 0and!suspensionResult.waitTimeoutare gone from the delta gate;!waitDueThisInvocationreplaces all three wait terms.getRetentionDecisiondoes not look at waits (since [core] Retain workflow VMs across waits #3892) and this PR does not add that back.hasPendingWaitgate inquickjs-entrypoint.ts; it is unchanged here.Turbo stays conservative
forceOptimisticStartstill latches off on any open wait, as before this PR. An earlier revision relaxed it too; review pointed out that a wake fromrun.wakeUp()is a peer that can race a forced body for its claim, and a body executed twice is not repairable the way a delayed wake is. Turbo is the start delivery only, so a run that opens a wait during it pays the await-then-run path for that one invocation, which is cheap next to the alternative.run.wakeUp()completes waits regardless ofresumeAtThe deadline bound is a statement about the wait's own timer.
run.wakeUp()(public API, and the dashboard's "cancel sleeps" action) writeswait_completedimmediately. If that lands after a step's terminal write, it is above the inline delta that write returned, and a replay that then reaches the sleep would park and arm a continuation for the originalresumeAt(chained at 23h for a long sleep), with recovery resting on the wake messagerun.wakeUp()sent not having deferred to this invocation's own in-flight step.So before an invocation parks on a wait (nothing to run or queue, only a wait timer to arm) over a log whose last extension was an inline delta, and that log holds a wait that was already open when the delta's step ran, it does one
events.listfrom the cursor and replays over the result. A completion that landed is acted on now; an unchanged log parks on the next pass. Paid once per park, at a queue hand-off boundary, and only for thePromise.raceloser shape: a wait created by the parking suspension itself (await step(); await sleep('5s'), the common polling loop) did not exist when the delta was taken, so nothing can sit above the delta for it, and it parks without the read. Tracked byeventLogFromInlineDelta, cleared by every real read.Skew allowance
OPEN_WAIT_CLOCK_SKEW_MS = 30_000(WORKFLOW_OPEN_WAIT_CLOCK_SKEW_MS) covers the clocks that are not this process's (the wait timer's queue can deliver early; the runtime already tolerates 2s viaNEAR_ELAPSED_WAIT_THRESHOLD_SECONDS), and the fact that the inline-window check sits at the top of the loop, so the last batch can start one replay pass plus a claim round trip after the window nominally closes. Seconds at most; 30s is an order of magnitude of margin.envNumberrejectsInfinity;31536000000restores the old unconditional gating.Conservative defaults, kill switch, observability
wait_createdwhoseresumeAtis missing or unparseable is treated as due now (-Infinity), and the suspension-queue deadline test is written as!(resumeAt > deadline)so aNaNgates too. The gate never fails open for a wait it cannot reason about.WORKFLOW_OPEN_WAIT_CLOCK_SKEW_MS=31536000000(one year) makes every pending wait "due within the window" and restores the gating onmainexactly. The pre-park re-read stays on under it, which is the conservative direction.workflow.inline_delta_over_pending_waiton batches where the delta was taken while a wait is pending (how often the relaxed gate is exercised), andworkflow.wait_park_rereadwhen the pre-park read fires.Tests
packages/core/src/retained-vm-loop.test.ts, throughworkflowEntrypointover the stateful fake World (no runtime deadline, so the 2-minute fallback window):listCalls === 1, both terminal writes carrysinceCursor(was 3 onmain)WORKFLOW_OPEN_WAIT_CLOCK_SKEW_MSset to a year:listCalls > 1(gated)inject-wait-after-step-completedmode (therun.wakeUp()shape):wait_completedappended above the delta; the pre-park read sees it and the run completes in the same invocation (listCalls === 2, no continuation re-armed)await step(); await sleep('5s'): parks with no extra read (the wait postdates the delta)Each of these fails if its corresponding change is reverted (checked by mutation).
packages/core/src/runtime/open-hook-wait-state.test.ts(20 tests) covers the pure predicate: boundary at the deadline, past-due, unparseable, completed, several waits, hooks unaffected, env override.cd packages/core && pnpm test: 115 files, 2488 tests passed.Docs
WORKFLOW_OPEN_WAIT_CLOCK_SKEW_MSindocs/content/docs/v5/configuration/runtime-tuning.mdx.🤖 Generated with Claude Code