Default WORKFLOW_PRECONDITION_GUARD on#2946
Conversation
Follow-up to #2266 (review pullrequestreview-4699625704). - **Guard on by default:** `isPreconditionGuardEnabled()` now returns true unless `WORKFLOW_PRECONDITION_GUARD` is explicitly falsy. Validated by the #2266 race-repro (2000 runs guard-on: 0 corruptions). - **Env-var footgun:** boolean flags accept `1`/`true`/`yes`/`on` (and `0`/`false`/`no`/`off` for the guard's opt-out) case-insensitively instead of only `1`, so `=true` no longer silently reads as off. Mirrored across `isPreconditionGuardEnabled` (core) and `isSequentialReplaysEnabled` (builders + world-vercel). - **run_failed asymmetry:** comment documenting why the terminal `run_failed` is intentionally not guarded (fail-open, re-runnable) unlike `run_completed`. - **Concurrent-reload cursor:** comment on the benign last-write-wins cursor in withPreconditionRetry. - **World-authoring docs:** CreateEventParams.stateUpdatedAt JSDoc now documents the backend marker/reject/equal-passes contract for custom-world authors. - **Diagnostics:** debug log when a non-decodable event id disarms the guard. - **retryAfter:** JSDoc noting it is forward-compat and currently unused. - Docs: runtime-tuning guard entry updated to "Default: enabled" + opt-out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 304f594 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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:
📜 Previous results (1)45a9425Wed, 15 Jul 2026 19:57:19 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
✅ vercel-multi-region
❌ Some E2E test jobs failed:
Check the workflow run for details. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
VaguelySerious
changed the title
Per follow-up direction, remove the "accept common truthy/falsy spellings" change from this PR; it will ship on its own later. The guard's default-on behavior stays (opt out with WORKFLOW_PRECONDITION_GUARD=0), and WORKFLOW_SEQUENTIAL_REPLAYS reverts to `=== '1'` (back to main), so @workflow/builders and @workflow/world-vercel are no longer touched by this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed both shapes of this PR — the original (default-flip + env-var spelling hardening) and the narrowed head after 304f594bd split the spelling changes out. Approving the narrowed version.
The flip itself is sound and consistent with the merged #2266 state. I re-verified the post-approval history: the WORKFLOW_SAFE_MODE umbrella was removed from #2266 before merge ("configure WORKFLOW_SEQUENTIAL_REPLAYS directly"), so isPreconditionGuardEnabled() collapsing to a single !== '0' check has no orphaned umbrella semantics to worry about. Fail-open properties hold in every direction: unsupporting backends ignore the snapshot (no-op, today's behavior), a non-decodable event id disarms the guard for that create (now with the debug log so a fleet-wide silent disarm is diagnosable), and an unrecognized env value fails toward the guard — the correct direction for an opt-out safety flag.
The review-feedback items are all well executed:
- The
run_failedasymmetry comment captures both the principled reason (spurious failure is re-runnable; spurious completion commits a wrong result) and the practical one (the event log isn't in scope in that catch) — exactly what a future reader needs to not "fix" it naively. - The
stateUpdatedAtbackend-contract JSDoc is the most valuable addition here: the externally-originated-events-advance-the-marker / replay-origin-events-must-not distinction is the crux of the whole design, and it was previously only implemented, not specified. Custom-World authors now have something to build against, including the equal-passes anti-livelock rule. - The concurrent-reload cursor comment (benign last-write-wins, refetch-only consequence) and the
retryAfterforward-compat note close out the remaining items.
Verified locally on the narrowed head: guard + replay + runtime suites 66/66; full core suite was 1482 green on the pre-narrowing head and the narrowing only removed code. Docs were correctly narrowed along with the code ("Set 0 to disable" — no spelling promises the code doesn't keep). Changeset properly trimmed to workflow/@workflow/core minor + world/errors patch. The one CI failure is the nextjs-webpack HMR rebuild-count test — the same flake family that's hit at least four unrelated PRs recently, and it watches dev-server rebuild logs, mechanistically untouched by event-creation guarding.
Two follow-through asks, non-blocking:
- With the spelling hardening split out,
WORKFLOW_PRECONDITION_GUARD=falsecurrently leaves the guard on silently — the exact footgun class the split-out PR fixes, now pointed in the opposite direction. The docs only promise0, so nothing is inconsistent — but please land the spelling PR promptly behind this one so the window where=falsemisleads is short. - For the record on rollout sequencing: the Slack plan was a few days of internal-project soak before the opt-out flip. The 2000-run race-repro (0 corrupted, 13.8% recovered 412s) plus the fully-green forced-ON e2e round from #2266 are strong evidence, and the failure mode on unsupporting backends is a no-op — so I'm comfortable approving. Just confirm the backend guard support is fully rolled out in production before this reaches a published beta, since default-ON makes the 412-recovery path production-hot immediately.
2 participants
Summary
Follow-up to #2266. Two things:
WORKFLOW_PRECONDITION_GUARDis now on by default. The event-creation guard was opt-in; it now runs unless explicitly disabled withWORKFLOW_PRECONDITION_GUARD=0. Backends that don't support the guard ignore thestateUpdatedAtsnapshot, so this is backward-compatible and fails open. Validated by the Add stateUpdatedAt precondition guard to event creation #2266 race-repro (2000 runs guard-on: 0CORRUPTED_EVENT_LOG, 13.8% of runs hit a 412 and recovered).Addresses the non-blocking feedback from this review
Review items addressed
1/true/yes/on,0/false/no/off) instead of only1will ship on its own, so this PR leaves the flag parsing (and@workflow/builders/@workflow/world-vercel) untouched.run_failedasymmetry — added a comment documenting why the terminalrun_failedis intentionally not guarded likerun_completed(fail-open: a spurious failure is re-runnable, a spurious completion commits a wrong result; and the loaded event log isn't in scope on that catch path). Guarding it symmetrically would be a larger change — happy to do it as its own follow-up if preferred.CreateEventParams.stateUpdatedAtJSDoc now documents the backend contract (which events advance the per-run marker, strictly-older rejects, equal-passes anti-livelock) so custom-World authors have something to implement against.withPreconditionRetry.runtimeLogger.debugwhen a non-decodable event id disarms the guard for a create.retryAfternit — JSDoc noting it's forward-compat and currently unread by the runtime.