Uh oh!
There was an error while loading. Please reload this page.
[codex] Capture world contract failures as fatal - #2060
Conversation
🦋 Changeset detectedLatest commit: a85d52c 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 |
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
workflow with 1 step💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
744b7a6 to
16a2f7bCompare
TooTallNate
left a comment
There was a problem hiding this comment.
Approve — sound fix for a real production problem
The bug is real and the fix is targeted: when world-vercel returns a malformed run_started response, the runtime catches WorkflowWorldError with a schema validation message/Zod cause, records run_failed with the new WORLD_CONTRACT_ERROR code, and returns 204 to ack the queue message. No more redelivery loops to max deliveries on a non-retryable failure.
The shape mirrors the just-merged #2046 (CORRUPTED_EVENT_LOG): new code in error-codes.ts, classification in the appropriate runtime catch block, description coverage in describe-error.ts. Clean separation.
What I verified
pnpm turbo build --filter @workflow/core✓pnpm vitest run runtime.test.ts describe-error.test.ts classify-error.test.ts✓ (974 tests pass including the new regression)- Traced the
WorkflowWorldErrorthrow sites inworld-vercel/src/utils.ts: the newisWorldSchemaValidationErrorhelper correctly distinguishes schema validation (status=undefined + matching message/cause) from transient failures (timeouts, HTTP errors with status set). - The narrow detection (
message.startsWith('Schema validation failed for ') || cause.name === 'ZodError') does NOT match timeouts (which also havestatus === undefinedbut a different message) — so we don't accidentally treat retryable failures as fatal. - CI: 34 success, 1 failure (
E2E Local Prod Tests (nextjs-turbopack - canary)— unrelated; main is also failing it), 75 in-progress.
Five things worth thinking about (none are blockers)
1. Narrow scope: schema validation only, not parse-body failures
world-vercel/src/utils.ts throws WorkflowWorldError with status === undefined in two non-retryable cases:
// utils.ts:424 — body parse errorthrownewWorkflowWorldError(`Failed to parse response body for ${request.method}${endpoint}...`,{ url,cause: error});// utils.ts:443 — schema validation error (caught by this PR)thrownewWorkflowWorldError(`Schema validation failed for ${method}${endpoint}...`,{ url,cause: validationResult.error});Both are equally non-retryable (the server is sending malformed data; retrying doesn't help). The PR catches only the second. The body-parse case will still loop to max deliveries.
Worth either expanding the detection to include 'Failed to parse response body for ', or filing a follow-up to extend.
2. No WorldContractError class — asymmetric with CorruptedEventLogError
PR #2046 introduced a dedicated CorruptedEventLogError class. This PR adds the code but no class. The asymmetry has one practical consequence: describeError(liveError) cannot recognize a world-contract failure from a live WorkflowWorldError instance — only describeRunError({errorCode}) can detect it post-persistence.
In the current code path that's fine (the runtime translates to the code at emit time), but a class would make describeError symmetric and would localize the message-prefix matching to one place. Could be a follow-up.
3. getWorkflowSetupErrorCode uses instanceof, not name-based checks
functiongetWorkflowSetupErrorCode(err: unknown): RunErrorCode|null{if(errinstanceofWorkflowRuntimeError){returnRUN_ERROR_CODES.RUNTIME_ERROR;}if(isWorldSchemaValidationError(err)){returnRUN_ERROR_CODES.WORLD_CONTRACT_ERROR;}returnnull;}Compare to classifyRunError, which checks CorruptedEventLogError.is(err)first before falling through to WorkflowRuntimeError (because CorruptedEventLogError extends WorkflowRuntimeError, an instanceof check on the parent would mask the more specific class).
For the setup block today this doesn't matter because CorruptedEventLogError isn't thrown from setup paths — it's thrown from step.ts/hook.ts/sleep.ts/abort-controller.ts during replay, after setup completes. So the regression is theoretical. But if someone in the future throws CorruptedEventLogError from setup, they'd silently get RUNTIME_ERROR instead of CORRUPTED_EVENT_LOG.
Easiest fix: check CorruptedEventLogError.is(err) first (consistent with classifyRunError).
4. Setup-only scope
Schema validation can happen on ANY world call — events.list, events.create for wait_completed, etc. Those failures currently still loop to max deliveries. Worth a follow-up to broaden the fatal-error handling, or at least note the scope limitation in the PR body.
5. Code name WORLD_CONTRACT_ERROR vs detection logic
The code name is broad ("contract" covers schema + parse + other client-side world errors). The detection only catches schema validation. The name is slightly aspirational vs. the implementation.
That's OK — better to introduce a forward-looking code now than have to introduce another code later. But worth a code comment like // Today only detects schema validation; extend isWorldSchemaValidationError to cover other contract violations as they're discovered.
Backport stance
Not addressed in PR body. This is a bug fix touching error classification and runtime control flow — a candidate for backport to stable per AGENTS.md ("Bug fixes to existing functionality that at least partially exists on stable"). The backport action's AI should pick this up automatically; worth checking after merge.
Approving.
karthikscale3
left a comment
There was a problem hiding this comment.
ai review: posting inline comments from a regression-focused pass. Nothing I'd block on — five follow-ups to consider.
| runtimeLogger.error( | ||
| 'Fatal runtime error during workflow setup', | ||
| { workflowRunId: runId, error: err.message } | ||
| 'Fatal error during workflow setup', |
There was a problem hiding this comment.
ai review: the log message rename from 'Fatal runtime error during workflow setup' → 'Fatal error during workflow setup' is a silent observability regression. Any external alert or dashboard keyed on the old literal string will stop firing without warning. Low blast radius, but worth a quick check that nothing in datadog/elastic/etc. matches on the old wording before this lands.
There was a problem hiding this comment.
Good catch. Restored the original Fatal runtime error during workflow setup literal while preserving the errorCode field for distinguishing WORLD_CONTRACT_ERROR from regular runtime setup failures.
| const cause = 'cause' in err ? err.cause : undefined; | ||
| return ( | ||
| err.message.startsWith('Schema validation failed for ') || |
There was a problem hiding this comment.
ai review: this string-prefix check is tightly coupled to the throw site at packages/world-vercel/src/utils.ts:444. If that message is ever reworded, only the ZodError-cause fallback below remains, and detection silently degrades. A sturdier shape would be a typed marker — e.g. a dedicated WorldSchemaValidationError subclass of WorkflowWorldError, or a code: 'SCHEMA_VALIDATION' field on the existing class — so the runtime detects it structurally rather than by message prefix. Not blocking.
There was a problem hiding this comment.
Done. The runtime no longer depends only on the string prefix for the primary world-vercel path: schema failures now carry code: "SCHEMA_VALIDATION", parse failures carry code: "PARSE_ERROR", and the string/cause checks remain as compatibility fallbacks.
| return; | ||
| } else if (err instanceof WorkflowRuntimeError) { | ||
| } else { | ||
| const errorCode = getWorkflowSetupErrorCode(err); |
There was a problem hiding this comment.
ai review: the fix is scoped to the run_started setup path, but the same WorkflowWorldError schema-validation failure can come out of other world calls in this handler — events.list, runs.get, and subsequent events.create calls during replay. Those still bubble up and redrive the queue to max deliveries, which is the same bug class. Worth a sentence in the PR description acknowledging the scope, or a follow-up issue to extend the same treatment to the other call sites.
There was a problem hiding this comment.
Broadened the handling in a85d52c: world contract errors now classify through the normal run failure path, so events.list and later replay events.create failures record run_failed with WORLD_CONTRACT_ERROR; the background-step prep path also records the fatal run error instead of redriving. Added an events.list regression test.
| setWorld, | ||
| } from './runtime/world.js'; | ||
| function isWorldSchemaValidationError(err: unknown): err is WorkflowWorldError { |
There was a problem hiding this comment.
ai review: the sibling throw at packages/world-vercel/src/utils.ts:424 (Failed to parse response body for ...) has the same shape as schema validation — WorkflowWorldError with no status, non-retryable in practice (the response body is malformed CBOR/JSON, retrying won't help) — but isn't caught here because the cause is a JSON/CBOR parse error rather than a ZodError. If we're declaring "world contract violation" as a fatal category, parse failures probably belong in it too. Worth deciding explicitly whether to include them now or in a follow-up.
There was a problem hiding this comment.
Included this now. world-vercel marks malformed response bodies with WorkflowWorldError.code = "PARSE_ERROR", and core treats that as WORLD_CONTRACT_ERROR alongside schema validation. Added a run-start parse failure regression test.
| /** Workflow replay exceeded the maximum allowed duration */ | ||
| REPLAY_TIMEOUT: 'REPLAY_TIMEOUT', | ||
| /** World response violated the SDK contract and cannot be retried safely */ | ||
| WORLD_CONTRACT_ERROR: 'WORLD_CONTRACT_ERROR', |
There was a problem hiding this comment.
ai review: classifyRunError in packages/core/src/classify-error.ts isn't updated to recognize WORLD_CONTRACT_ERROR. The describe-error path is fine in this PR because the runtime passes the explicit code, but if a schema-validation WorkflowWorldError ever flows through classifyRunError from outside the setup path, it'll be classified as USER_ERROR (wrong attribution). Symmetry would be nice — adding a WorkflowWorldError.is-style check there that returns WORLD_CONTRACT_ERROR when the schema-validation heuristic matches.
There was a problem hiding this comment.
Done in a85d52c: classifyRunError now uses a shared isWorldContractError helper and returns WORLD_CONTRACT_ERROR for schema/parse world contract failures. Added classifier coverage for both shapes.
ec28267 to
a85d52cCompare| const MAX_DELIVERIES_HINT = | ||
| 'The workflow queue exceeded its max-delivery budget. This usually indicates a persistent runtime failure — check the most recent stack traces for the underlying cause.'; | ||
| const WORLD_CONTRACT_HINT = | ||
| 'The workflow backend returned data that violated the SDK contract. This is not retryable; please report it with the stack trace and runId.'; |
There was a problem hiding this comment.
"This usually indicates a version incompatibility bug"
There was a problem hiding this comment.
Agreed. PR #2060 is already merged, so I cannot update this branch in-place anymore; this should be a small follow-up wording cleanup to change the world-contract hint toward “This usually indicates a version incompatibility bug”.
Backport PR opened against |
Summary
WORLD_CONTRACT_ERRORas a distinct run failure code for non-retryable world/SDK contract failuresWorkflowWorldError.codevalues (SCHEMA_VALIDATION/PARSE_ERROR) and classify them asWORLD_CONTRACT_ERRORrun_failedinstead of letting the queue callback redrive until max deliveriesWhy
When the Vercel world validates a malformed response, it throws a non-retryable
WorkflowWorldError. The workflow queue handler previously let some of those errors escape, so Q kept redelivering the message until max deliveries. This failure is not user code and not a transient backend retry case; it is a fatal world/client contract mismatch.Conflict resolution
Rebased onto
origin/mainand preserved the newCORRUPTED_EVENT_LOGclassification from base alongside the newWORLD_CONTRACT_ERRORhandling.Validation
pnpm exec biome check --write packages/core/src/classify-error.ts packages/core/src/classify-error.test.ts packages/core/src/runtime.ts packages/core/src/runtime.test.ts packages/world-vercel/src/utils.tspnpm turbo build --filter=@workflow/core --filter=@workflow/world-vercelpnpm vitest run packages/core/src/runtime.test.ts packages/core/src/describe-error.test.ts packages/core/src/classify-error.test.tspnpm changeset status --since=mainBackport
This needs to be backported after merge in main.
Note: local commands emitted the repo engine warning because this machine is on Node v25.2.1 while the repo supports Node 18/20/22/24.