Fix duplicate inline step execution on mid-step wake via message ownership#2848
Conversation
…rship (#2780) An inline step has no queue message of its own, so any wake (hook resume, elapsed wait) that replayed the run mid-step enqueued a first message for its correlation id; that message's handler bare-started the still-running step and executed the body a second time, concurrently. The lazy step_started now stamps eventData.ownerMessageId with the queue message ID of the invocation running the body (re-stamped on owner-recovery bare starts; cleared by unstamped retry starts; permanently lapsed at step_retrying). The pending-step dispatch loop replaces the unconditional immediate requeue with a decision table: - owner == this message -> re-execute in this invocation (crash recovery via queue redelivery of the owning message) - ownership active, other message -> ensure a DELAYED backstop message (delaySeconds = lease remaining, idempotencyKey = correlationId), which doubles as the escape hatch for lost/exhausted owner messages - unowned / lapsed / expired -> immediate enqueue, unchanged The lease is a fixed 860s (WORKFLOW_INLINE_OWNERSHIP_LEASE_SECONDS): no runtime/build-time signal for the resolved maxDuration exists, and 'max' resolves to at most 800s on the platform. An in-process single-flight map around step execution makes a backstop firing mid-step harmless on worlds with no invocation kill bound (world-local). Kill switch: WORKFLOW_INLINE_OWNERSHIP=0 reverts dispatch to the previous behavior. The workbench repro (hook + 1.5s inline step + resume at ~400ms) executes the side effect exactly once with ownership on, and twice with the kill switch — reproducing the bug. Requires the world-vercel backend to persist and re-emit the new step_started meta field (deployed ahead of this change; older backends safely degrade to today's behavior). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (5)0943aa5Thu, 09 Jul 2026 23:31:30 GMT · run logs
fe0c1deThu, 09 Jul 2026 19:43:34 GMT · run logs
da20e17Thu, 09 Jul 2026 17:01:41 GMT · run logs
1ec5e6eThu, 09 Jul 2026 01:47:45 GMT · run logs
63c46e4Thu, 09 Jul 2026 01:17:01 GMT · run logs
Avg deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body execution · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (time outside step bodies, client start → last step body exit) · SL: stream latency (first chunk write → visible to the reader) Scenarios — stream: one step that streams chunks back to the client; no hooks, so the run stays in turbo mode · hook + stream: registers a hook before the same streaming step, which exits turbo mode · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges 🟢/🔴 mark percentiles within/above target. Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 TTFS/WO compare client vs deployment clocks and SL compares the step runner’s clock vs the client’s (NTP-synced in CI). WO ends at the last step body exit, the closest observable proxy for the final step-completion request. |
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
🦋 Changeset detectedLatest commit: c01b3da The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed the ownership model, the dispatch decision table, the single-flight layer, and the wire changes; ran all the suites plus the workbench regression test. Approving — this is a well-engineered fix for a nasty class of bug, and the design writeups in the code are exemplary.
The core idea holds up under adversarial reading. Reusing the queue's own delivery semantics as the liveness lease avoids inventing a heartbeat mechanism, and the three safety layers compose correctly:
- Ownership lease — the 860s constant's justification chain is solid: builders emit
maxDuration: 'max'which resolves to ≤800s by platform rule, so the lease dominates any invocation lifetime, while staying under the 900s queue delay cap so one delayed message always suffices. The doc comment flags exactly the condition that would invalidate it (the 30-minute beta becoming reachable via'max'). - Single-flight — the subtle part is the loser awaiting settlement rather than ack-and-skip, and the reasoning is right: an early ack followed by a crash could consume the only message left to drive the step. Awaiting keeps the at-least-once envelope intact and degrades to polling if the loser times out.
- Delayed backstop as a run continuation (not a step message) — this is the right shape: when it fires, the replay re-derives ownership from the log instead of blindly bare-starting, so a step that completed in the meantime costs nothing, and the
:backstopidempotency suffix keeps it from colliding with the step's own message key.
Ownership derivation is deterministic-safe: it's computed from replayed events but only feeds dispatch decisions (operational, never recorded), and the latest-start-wins / step_retrying-lapses-permanently semantics are implemented identically in both places that derive it (the replay consumer in step.ts and the raw-event fast-path scan), with the invariant guard on ack paths as a nice canary against future ordering refactors.
Graceful degradation is covered in every direction I checked: kill switch reverts to today's dispatch while stamping stays inert; worlds with unstable message IDs never match the owner check and fall to the backstop path (documented in the Queue contract as SHOULD with exactly this analysis); backends that don't persist the field yield unowned steps = today's behavior; missing timestamps zero the lease = immediate enqueue; old SDKs never send the field and new SDKs tolerate its absence.
Verified locally: core 1408/1408, world 71/71, world-vercel 231/231, and the workbench regression test (hook resume landing mid-step-body, asserting the marker side effect executes exactly once) passes against the local world — which also exercises the single-flight layer, since that's what protects the no-kill-bound single-process world. CI is fully green including all e2e lanes.
Two non-blocking questions/notes:
- Backstop idempotency dedupe window: a double-orphan sequence (owner crashes → backstop fires → recovery re-owns under a new message → that crashes too) re-enqueues
${correlationId}:backstopwith the same key. If that lands inside the queue's dedupe window, the second backstop is dropped — recovery then rests on the owner message's own redelivery (the primary mechanism) or any later wake. That layering looks sufficient to me, but it'd be worth a sentence in the changelog doc confirming the dedupe-window interplay was considered, since the backstop is billed as the escape hatch for exhausted owner messages. - The
assertNoInFlightOwnedStepsguard logs an error but doesn't throw — right call for production, just noting that a CI-only hard-fail mode could make regressions louder in e2e lanes.
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Requesting changes for one liveness hole in the backstop path. I checked the ownership state derivation, dispatch decision table, owner recovery, retry handoff, single-flight behavior, world queue deduplication semantics, CI/E2E coverage, and the latency report. The overall ownership approach looks sound, but the fixed backstop idempotency key means the documented refreshed-lease re-arm cannot actually create a replacement message. Please address the inline finding and add a regression test for the full re-arm sequence.
Review finding (liveness hole): queues dedupe an idempotency key for the
original message's lifetime — world-local retains it while a delivery is
in flight, and Vercel Queues behaves the same — so a fixed
`${correlationId}:backstop` key meant a backstop firing during a
refreshed lease (owner recovery re-stamped after the backstop was armed)
could never publish its own replacement: the re-arm deduped against the
in-flight backstop message and was dropped. If the recovered owner then
died with its redelivery budget exhausted, no escape hatch remained and
the run wedged until an unrelated external wake.
The key is now `${correlationId}:backstop:${lastStartedAt}` — scoped to
the ownership epoch (the latest step_started's persisted timestamp, so
every replayer derives the same key). Wakes within one epoch still
dedupe to a single pending backstop; an owner-recovery re-stamp changes
the epoch and gives the re-arm a fresh key. Pending backstops stay
bounded by the queue's redelivery budget for the owning message.
The ownership helpers move from runtime.ts into
runtime/step-ownership.ts so the key derivation and dispatch predicates
are unit-testable; step-ownership.test.ts covers the state machine, the
lease math, and the full re-arm sequence from the review (wake arms →
re-stamp → backstop fires mid-lease → replacement accepted despite
in-flight key retention) against a dedupe model matching world-local's
inflightMessages behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Re-reviewed at da20e17. My previous blocking finding — the fixed backstop idempotency key that couldn't re-arm after an owner-recovery lease refresh — is resolved by the epoch-scoped key (correlationId:backstop:lastStartedAt), and the new regression test in step-ownership.test.ts models the in-flight key-retention behavior and walks the full re-arm sequence. Approving.
What I checked on this pass:
- Epoch-key fix: the key changes on every owner-recovery re-stamp (new
lastStartedAt), stays stable within an epoch (fan-out capped at one pending backstop), and never collides with the step message's owncorrelationIdkey, so the retry handoff can't be absorbed. Verified world-local retains idempotency keys while a delivery is in flight (inflightMessagesreleased post-loop), which is exactly the hazard the epoch suffix escapes. - Ownership state derivation: latest-start-wins in both the replay consumer (
step.ts) and the raw-event scan (hasPendingStepOwnedByMessage), withstep_retryingas a permanent lapse in both — the two derivations agree. The queue items carrying the new fields are the same objects surfaced aspendingSteps, so state flows through the suspension handler untouched. - Dispatch decision table: owner match → owned recovery (in-invocation re-execution); live foreign lease → delayed backstop; lease expired / unstamped / retrying / kill-switched → the exact pre-PR immediate enqueue. Kill switch reverts dispatch while stamping stays inert.
- Type-level hazards:
meta.messageIdis required (non-optional) in the Queue contract, so noundefined === undefinedfalse ownership match;Event.createdAtisz.coerce.date()and world-vercel's read path explicitly coerces beforeEventSchema.safeParse, so+event.createdAtin the consumer is a real ms timestamp on all worlds;{type:'skipped'}is a pre-existingStepExecutionResultmember and lands in the terminal bucket (loop back to replay), which is correct for a single-flight loser. - Interactions: inline-delta fast path and turbo forced-optimistic-start both exclude owned recovery (
ownedRecoverySteps.length === 0guards); latency tracking restricted to lazy first steps; retry results from owned-recovery steps flow into the existing keyed delayed-retry handoff afterstep_retryinglapses ownership; the background fast-path fall-through only fires when this message owns a pending step, otherwise the early return is unchanged. - world-local messageId stability: one ID minted per enqueued message, reused across the delivery retry loop (
x-vqs-message-idconstant, attempt counter separate) — the lease's redelivery-recovery assumption holds there. - Test verification: ran the new unit suites (66 tests),
@workflow/world(71),@workflow/world-vercel(231), and the fullworkbench/vitestsuite (39) — all green (core e2e/* failures in my sandbox are missing-build-artifact environmental, not PR-related). Confirmed the #2780 repro is faithful: withWORKFLOW_INLINE_OWNERSHIP=0it fails with the marker written exactly twice; with the fix it passes.
Noted, not blocking: the deploy-order dependency (backend must persist/re-emit meta.ownerMessageId before this SDK ships) is documented in the rollout section; the benchmark deltas (hook+stream TTFS +8.6% avg) are within the run-to-run spread of the three posted runs. One non-blocking robustness nit inline.
lastStartedAt is the server-stamped event createdAt while nowMs is the local clock, so a client running behind the server computed a lease remainder longer than the lease itself. With the lease tuned to the 900s cap that yields a delaySeconds above the queue per-message maximum, which SQS-backed community worlds reject — the wake replay's backstop enqueue would throw and ride the redelivery loop until the lease drained. Clamp the remainder to the configured lease so skew is strictly harmless. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Observability (design doc Phase 7, previously unimplemented): span attributes workflow.inline_ownership.owned_recovery_steps and workflow.inline_ownership.backstop_wakes_armed on the workflow.execute span, and warn-level (always printed) logs for owned recovery and single-flight absorption — debug/info logs are invisible without DEBUG, so an operator triaging a duplicate-step or wedged-run report previously could not tell whether recovery ran or a backstop was armed. Also documents two load-bearing subtleties found in re-review: - turbo's attempt-1 gate is what makes its reinvoke() paths (hook conflict, throttle) safe with inline ownership: owned-recovery steps only exist on redeliveries, so turbo can never ack a message that inline-owns a non-terminal step. Comment added so a future change to turbo's engagement conditions doesn't silently break this. - clock-skew clamp rationale in the design doc's lease formula. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the phase-by-phase implementation plan (scaffolding that served its purpose) with a rationale-first document: the liveness problem underneath #2780, why the queue messageId is the ownership identity, why ownership must be lease-bounded and why the lease is a fixed 860s constant, why the non-owner action is an epoch-keyed delayed backstop wake (including both rejected backstop shapes and the evidence that killed them), why in-process single-flight is a required layer, and why the alternatives — queue serialization, inline-eligibility latch, inline retries, heartbeat/lock stores, lease derivation, a backend-side fix — were rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous framing led with redelivery exhaustion, but that path fails the run via the SDK's max-deliveries check even without a lease. Replace it with what the lease actually protects against: owner-message loss the SDK never observes (queue-side DLQ below the SDK budget, retention expiry, operator purge), unstable-message-ID worlds (where the lease is the entire recovery mechanism), insurance on the ack-while-owned invariant, and step-level vs run-level failure granularity for poison steps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Fixes #2780
TLDR: tie steps to a queue message ID which needs to execute them, preventing all cases of competing/concurrent invocations trying to execute the same step.
Note: fixing local world still. Working in vercel/postgres
Problem
An inline step has no queue message of its own — that is the point of inline execution. But the pending-step dispatch on every wake replay queued every created, non-terminal step unconditionally, relying on
idempotencyKey = correlationIdto dedupe. For an inline step there is no prior message to dedupe against, so any wake mid-step (hook_received, an elapsed wait continuation, cancellation) enqueued a first message for that correlation id. Its consumer bare-step_startedtherunningstep (allowed — retries legitimately restart non-terminal steps) and ran the body a second time, concurrently. Onestep_completedwins; every side effect happened twice.Fix: message-ID ownership
Rather than inventing a liveness mechanism, this reuses the queue's: a delivery of the owning message is the liveness lease.
step_startedthat creates an inline step stampseventData.ownerMessageId= the handler's queue message ID (meta.messageId, stable across redeliveries — now documented in the Queue contract).step_startedduring replay: an unstamped bare start (a retry attempt) clears it; an owner-recovery start re-stamps it;step_retryinglapses it permanently (queue-owned from there).delaySeconds = lease remaining,idempotencyKey = correlationIdThe delayed backstop folds the expiry escape hatch into the existing mechanism: a lost or exhausted owner message is bailed out after the lease, with step-level (catchable) failure semantics for poison steps, and the idempotency key caps it at one pending backstop regardless of wake count.
Lease + single-flight
WORKFLOW_INLINE_OWNERSHIP_LEASE_SECONDS, clamped ≤900 = queue max delay). No runtime/build-time signal for the resolvedmaxDurationexists (builders emit'max', platform-resolved per plan); the bound comes from the platform rule that'max'resolves ≤800s. The rationale + revisit caveat live in a comment on the constant.step-single-flight.ts) around step execution for the background/backstop/owned-recovery paths: the loser awaits the winner's settlement, then acks without executing (never early). This is what makes a mid-step backstop harmless on worlds with no invocation kill bound (world-local, self-hosted single process).reinvoke) runs while an owned step is mid-body — load-bearing for crash recovery, guards future refactors.Kill switch
WORKFLOW_INLINE_OWNERSHIP=0reverts dispatch to the unconditional immediate requeue (stamping stays — inert data).Tests
workbench/vitest/test/inline-step-ownership.test.ts): open hook + 1.5s inline step + resume at ~400ms → side-effect marker fires exactly once. Verified the test faithfully reproduces the bug: withWORKFLOW_INLINE_OWNERSHIP=0it fails with the marker fired twice.step_startedschema round-trip, v4 wire split (splitEventDataForV4) for stamped lazy + bare re-stamp + unstamped starts.@workflow/coresuite (1408), world packages (incl. world-testing inline batches), full workbench/vitest suite — all green.Backend dependency & rollout
Known residual (documented in code + design doc): a redelivery-while-alive of the owning message passes the owner check → duplicate. Equals the queue's at-least-once envelope, strictly rarer than today's every-wake duplication. Cross-instance long-step duplicates on multi-instance self-hosted worlds remain post-lease (raise the lease env to mitigate).
Docs
docs/content/docs/v5/changelog/step-message-ownership.mdx(motivation, decision table, phases, edge cases)WORKFLOW_INLINE_OWNERSHIP+WORKFLOW_INLINE_OWNERSHIP_LEASE_SECONDSdocumented inconfiguration/runtime-tuning.mdxmeta.messageIdredelivery-stability expectation documented oncreateQueueHandler(worlds with unstable IDs degrade to backstop-lease behavior, never wedge)🤖 Generated with Claude Code
Docs Preview
WORKFLOW_INLINE_OWNERSHIP/WORKFLOW_INLINE_OWNERSHIP_LEASE_SECONDSmessageIdstability note