[core] Don't count racing invocations' duplicate step_started events toward the maxRetries ceiling#3069
Conversation
…toward the maxRetries ceiling The retry-ceiling guard added in #3035 derives a step's attempt number from the number of step_started events in the log. But invocations racing on the same pending batch (stale replays, wake replays, step messages dispatched off a lost create-claim) can each write a step_started for the same logical attempt — worlds without an atomic start guard (world-local) let them all through — so a healthy step's count could cross the ceiling and fail the run with a false "exceeded max retries" (the world-testing inline-batches-debug CI flake). Scope the count to the starts each ceiling is actually about: - inline owned-recovery: only starts stamped with THIS message's ownerMessageId (each real (re)delivery of the owner re-stamps it; racers stamp their own IDs or none) - background steps: only bare/unstamped starts (that path never stamps ownership, and throttle/too-early redeliveries still write no start) Real timeout retries still bound: each recovery re-run writes another owner-stamped (or bare) start, so genuine exhaustion still trips the guard.
🦋 Changeset detectedLatest commit: 3a31e52 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✅ All tests passed E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (3)034e9a6Fri, 24 Jul 2026 21:18:29 GMT · run logs
95f6d0fFri, 24 Jul 2026 01:01:52 GMT · run logs
a0eed2fThu, 23 Jul 2026 20:34:21 GMT · run logs
ℹ️ Metric definitions & methodologyBest/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 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 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
Fixes a false-positive maxRetries exhaustion in @workflow/core caused by counting duplicate step_started events emitted by concurrent/racing invocations as separate attempts, especially in worlds without an atomic start guard (e.g. world-local). The change scopes “authoritative attempt” derivation to the specific class of starts each ceiling is intended to measure.
Changes:
- Add an ownership-aware filter to the
step_started-counting helper so inline owned-recovery counts only starts stamped with the currentownerMessageId, while background-step verification counts only bare/unstamped starts. - Update both the background-step retry ceiling verification and the inline owned-recovery ceiling to use the filtered counts.
- Add focused unit tests covering both filters and the CI-flake-shaped regression case; add a changeset for
@workflow/core.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/runtime.ts | Adds owner-filtered step_started counting and applies it to inline/background retry ceiling logic to avoid false maxRetries exhaustion from racing invocations. |
| packages/core/src/runtime.test.ts | Adds unit tests validating the new counting behavior (owned/unowned filters + regression/counter-case). |
| .changeset/inline-maxretries-race.md | Publishes the fix as a patch changeset for @workflow/core. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type StepStartOwnerFilter = | ||
| | { type: 'ownedBy'; messageId: string } | ||
| | { type: 'unowned' }; | ||
|
|
||
| /** | ||
| * Number of `step_started` events already recorded for a step, used as the | ||
| * authoritative attempt count for the inline retry ceiling. Each real attempt | ||
| * writes exactly one `step_started` (the atomic create-claim / single-flight | ||
| * prevents concurrent double-starts from inflating this), so the count equals | ||
| * the number of attempts that have begun. | ||
| * authoritative attempt count for the retry ceiling. | ||
| * | ||
| * IMPORTANT: the raw (unfiltered) count is NOT a reliable attempt number. | ||
| * Concurrent invocations racing on the same pending batch (stale replays, | ||
| * wake replays, a step message dispatched by a replay that lost the | ||
| * create-claim race, ...) can each write a `step_started` for the same | ||
| * logical attempt — worlds without an atomic start guard (world-local) let | ||
| * all of them through. Counting those duplicates as "attempts" made the | ||
| * maxRetries ceiling fire on healthy runs and fail them with a false | ||
| * "exceeded max retries" (see workflow#3048 CI flake). | ||
| * | ||
| * Callers must therefore scope the count to the starts their ceiling is | ||
| * actually about via `ownerFilter`: | ||
| * - the inline owned-recovery ceiling counts only THIS message's starts | ||
| * (`ownedBy`) — each real (re)delivery of the owning message stamps its | ||
| * `ownerMessageId` on the start it writes, while racing invocations stamp | ||
| * their own IDs (or none), so they no longer inflate the count; | ||
| * - the background-step ceiling counts only bare starts (`unowned`) — the | ||
| * background path never stamps ownership, so inline racers' stamped | ||
| * starts are excluded, and throttle/too-early redeliveries (which write | ||
| * no start at all) stay excluded as before. | ||
| */ | ||
| function countStepStartedEvents( | ||
| export function countStepStartedEvents( | ||
| events: Event[] | null | undefined, | ||
| stepId: string | ||
| stepId: string, | ||
| ownerFilter?: StepStartOwnerFilter | ||
| ): number { |
There was a problem hiding this comment.
Good catch — ./runtime is indeed a public subpath export. Moved the helper (and its filter type) to an internal module, src/runtime/count-step-started-events.ts, imported relatively from runtime.ts; the tests moved to a sibling test file and import it by relative path. No new public API surface. Done in 95f6d0f.
|
Huh, I thought this should already be safe under the assumptions that we only create |
|
(AI) Dug into the write path — the premise doesn't hold, on two counts: 1. 2. The only write-time exactly-once guard is the create-claim, and it covers step creation, not step starts. In world-local the atomic claim fires only on the lazy-start path (
So in the inline-batches scenario the steps are eagerly created and queued, then several concurrent invocations (stale replay, wake replay, a step message dispatched off a lost create-claim) each pass the not-terminal check and each append a Why not just gate the write by message ID instead? You can't distinguish the two cases at write time: the legitimate retry path is a different message re-starting the same non-terminal step, and a genuine owned-recovery re-run re-stamps the same (world-vercel's server-side guard makes the duplicate-non-terminal-start window much narrower, which is why the flake surfaced on world-local/world-testing — but the ceiling is computed in core against the event log, so the fix has to be world-agnostic.) |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| s.correlationId | ||
| s.correlationId, | ||
| { | ||
| type: 'ownedBy', |
There was a problem hiding this comment.
AI Review: Note
Splitting the count into two disjoint filters means neither ceiling sees the combined attempt total across a mixed owned→bare sequence — and the authoritativeAttempt guard is the only bound for a step that always times out (writes no step_failed, so the error-gated guard at step-executor.ts:661 never fires).
Concretely, with maxRetries=3 (ceiling = attempt > 4), a step that times out under owned recovery a few times and then transitions to the queued/bare path:
step_started ownerMessageId=msg_OWNER // owned attempt 1
step_started ownerMessageId=msg_OWNER // owned attempt 2 (crash recovery)
step_started ownerMessageId=msg_OWNER // owned attempt 3 (crash recovery)
step_started (bare) // attempt 4 (lease expired → queued)
step_started (bare) // attempt 5 (queued retry)
- unfiltered ([core] Enforce maxRetries for steps that time out #3035): attempt 6 → trips
ownedBy msg_OWNER: 3 → attempt 4 → does not tripunowned: 2 → attempt 3 → does not trip
I ran this through countStepStartedEvents on this branch to confirm (a throwaway probe, not committed). So the PR-description claim "genuine exhaustion still trips the guard at the same point" holds for pure-owned and pure-bare sequences but not strictly for a mixed one. It's still bounded overall (owned side by the queue's max-receive count, bare side by metadata.attempt), so this is not an infinite-loop regression and I'm not blocking on it — but the effective ceiling for an always-timing-out step that changes ownership mode can exceed maxRetries. Worth confirming that's acceptable, and ideally a test for the mixed case since the two current tests only cover the pure sequences.
Nit (folding in a second point to keep this on a diffable line): the comments here and near line 1953 call the owned-recovery start a "bare step_started", but ownerMessageId: metadata.messageId is stamped unconditionally just below — so it is not bare; it carries the owner stamp (which is what makes the ownedBy ceiling count it). Elsewhere "bare" means "no ownerMessageId" (the unowned background start). Reusing the word for both reads as contradictory.
There was a problem hiding this comment.
You're right on both counts — thanks for probing the mixed sequence, and I decided it's worth closing rather than just documenting. Addressed in 95f6d0f:
Mixed owned→bare gap: the background ceiling's scope is now totalAttempts = bare starts + the largest single owner's starts, instead of bare-only. Your exact sequence now computes attempt 3 (owned) + 2 (bare) + 1 = 6 > 4 → trips, restoring the #3035 behavior for genuine mixed lifecycles. The max-over-owners is what keeps the false positive fixed: a genuine lifecycle has at most one inline-ownership phase (ownership is claimed atomically at creation and lapses permanently at step_retrying or on any bare start), so the genuine owner's attempts are counted in full, while racing invocations' one-off stamped duplicates contribute at most their single largest count instead of accumulating. (If a racer's stamped start steals latest-start ownership mid-race, the superseded owner's attempts get shadowed by the max — that residual undercount is in the permissive direction and still bounded by the delivery-count gate.) Your sequence is committed as the mixed owned→bare timeout sequence trips the combined background ceiling test, and the racer-regression test now also asserts totalAttempts stays inside the ceiling.
The inline owned-recovery ceiling intentionally stays ownedBy: a mixed history can't precede owned recovery (a bare start clears ownership, so the owner's redelivery would no longer own the step and it falls through to the queued path, where the combined ceiling applies).
Terminology nit: fixed — the owned-recovery start is now described as a payload-less (but owner-stamped) start at both spots in runtime.ts, plus the two matching comments in step-executor.ts; "bare" is reserved for the background path's unstamped start.
|
No backport to This fixes the retry-ceiling guard from #3035, which is To override, re-run the Backport to stable workflow manually via |
Problem
The retry-ceiling guard added in #3035 derives a step's attempt number from the number of
step_startedevents recorded for it. That count is not a reliable attempt number: invocations racing on the same pending batch (stale replays, wake replays, a step message dispatched off a lost create-claim) can each write astep_startedfor the same logical attempt. Worlds without an atomic start guard (world-local) let all of them through, so a healthy step's count can crossmaxRetries + 1and the guard fails the run with a falseexceeded max retriesFatalError.This is the root cause of the
world-testing > inline-batches-debugunit-test flake on CI (e.g. this run on main and this one on #3048): the test deliberately provokes concurrent replays racing on parallel step batches; on slow runners enough duplicate starts accumulate that a recovery re-run computes an attempt above the ceiling →run_failed→ the test polls forcompleteduntil its 60s timeout. (The guard's error log was invisible in the test output becausestepLoggeruses theworkflow:step:*namespace while the test only enablesDEBUG=workflow:runtime:*.)Fix
Scope the
step_startedcount to the starts each ceiling is actually about, using theownerMessageIdstamp that inline starts already carry:ownerMessageId. Each real (re)delivery of the owning message re-stamps its ID on the start it writes, while racing invocations stamp their own IDs (or none) — so they no longer inflate this owner's attempt number.Real timeout retries are still bounded — the #3035 scenario is unchanged: each genuine recovery re-run writes another owner-stamped (or bare) start, so genuine exhaustion still trips the guard at the same point.
Testing
countStepStartedEventscovering both filters, including a regression case shaped like the CI flake (owner start + racer starts must not exhaust the ceiling) and a counter-case (4 genuine owner re-runs still exceed it).@workflow/coresuite: 1516 passed | 3 expected fail.@workflow/world-testingsuite (contains the flaky test): 15/15 × 4 consecutive runs. (The flake needs CI-slow timing to reproduce, so local runs can't fully prove it — but the new unit tests encode the failure mechanism directly.)