Slot event identity: number a run's events by position - #3305
Slot event identity: number a run's events by position#3305VaguelySerious wants to merge 32 commits into
Conversation
Number a run's events by dense per-run position instead of by ULID, and make the runtime claim its own event ids. - `@workflow/world`: slot id format/parse helpers, `SPEC_VERSION_SLOT_IDENTITY`, `mintedSpecVersion()` (default on, `WORKFLOW_SLOT_IDENTITY=0` opts out), `eventId`/`maxSlot` on `CreateEventParams`. - `@workflow/core`: contiguous slot reservation off the mutable event log, tail-tight one-at-a-time claims, and a merge/replay/re-claim loop on rejection with a per-slot-run restart budget and randomized backoff. - `@workflow/errors`: `SlotConflictError` carrying the inline event delta. - `@workflow/world-vercel`: sends the claimed id and decodes the 409 delta. - `@workflow/world-local`, `@workflow/world-postgres`: slot allocators, plus a run-scoped events primary key, since `evnt_…001` now exists once per run. Correlation ids are untouched: steps, waits, hooks and attributes keep their seeded ULIDs.
🦋 Changeset detectedLatest commit: f5a2301 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❌ Some tests failed ❌ Failed E2E Tests▲ Vercel Production (6 failed)astro-node (1 failed):
express-node (1 failed):
fastify-node (1 failed):
hono-node (1 failed):
nest-node (1 failed):
sveltekit-node (1 failed):
E2E Test SummarySummary
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 242330ms → this run 152504ms (Δ -89826ms, -37%) 📜 Previous results (2)89482cfTue, 11 Aug 2026 18:15:29 GMT · run logs
fcb6404Tue, 11 Aug 2026 02:38:21 GMT · run logs
ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: 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, 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 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 |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // TEMPORARY — revert to '' before merge. Points e2e at the slot-identity | ||
| // backend branch deployment. | ||
| export const WORKFLOW_SERVER_URL_OVERRIDE = | ||
| 'https://workflow-server-git-peter-slot-event-identity.vercel.sh'; |
There was a problem hiding this comment.
WORKFLOW_SERVER_URL_OVERRIDE is hardcoded to a branch/preview deployment URL instead of '', forcing all world-vercel clients to route traffic to an ephemeral preview backend.
There was a problem hiding this comment.
Temporary, and marked with a comment: it points the client at this stack's server preview so the paired e2e run exercises both halves. Reverted to '' before merge.
VaguelySerious
added
the
event-log-race-repro
Event Log Race Repro12 of 14 latest repro runs hit event-log regressions. Run History
Latest Scenario Breakdown
Latest Non-Completed Runs
|
…for Hooks on Worlds that don't support hook retention, silently dropping the requested retention instead of failing closed like the node:vm engine.
This commit fixes the issue reported at packages/core/src/runtime/quickjs-runtime.ts:569
## Bug
The node:vm workflow engine gates Hook retention on a World capability in `packages/core/src/workflow/hook.ts` (~L89):
```ts
if (
options.experimental_minRetention !== undefined &&
ctx.worldCapabilities?.hookRetention?.active !== true
) {
throw new FatalError(
'The configured World does not support `experimental_minRetention` for Hooks.'
);
}
```
`ctx.worldCapabilities` is populated from `world.capabilities` (`packages/core/src/runtime.ts:2399`). The `WorldCapabilities.hookRetention` contract (`packages/world/src/interfaces.ts:315`) explicitly states this must **fail closed**: "Missing or inactive means the runtime rejects retained Hooks before registration."
The QuickJS engine (opt-in `WORKFLOW_VM=quickjs`) replicated the webhook rejection (commit `342c64c`) and the retention-deadline computation (commit `98692e4`) inside `WORKFLOW_CREATE_HOOK`, but **not** the world-capability gate. It simply computed `tokenRetentionUntil = Date.now() + retentionMs` and passed it through to `world.events.create` in the entrypoint.
### Concrete trigger
- Capability declarations: `world-local` and `world-postgres` declare `hookRetention: { active: true }`; **`world-vercel` does not** (verified in `packages/world-vercel/src/index.ts:36` capabilities block).
- A workflow running under the QuickJS engine on the Vercel world calling `createHook({ experimental_minRetention: '1h' })` would throw a `FatalError` up front on the node engine, but the QuickJS engine silently accepted it. The requested minimum token retention was therefore never enforced by the backend — the token could be reused earlier than requested — a silent divergence from the documented fail-closed contract.
## Fix
Added the missing capability gate at the same synchronous point as the existing webhook check, so it fails closed by default:
1. `WORKFLOW_CREATE_HOOK` (in `VM_BOOTSTRAP`) now throws `'The configured World does not support `experimental_minRetention` for Hooks.'` (same message as the node engine) when `experimental_minRetention` is set but `globalThis.__worldSupportsHookRetention !== true`.
2. Added `worldSupportsHookRetention?: boolean` to `QuickJSRuntimeOptions`, injected per-run into the VM as `globalThis.__worldSupportsHookRetention` (defaults to `false` → unsupported when omitted, i.e. fail closed).
3. The QuickJS entrypoint passes `world.capabilities?.hookRetention?.active === true`, mirroring the node engine's `ctx.worldCapabilities` source.
4. Updated the existing test `preserves a Hook minimum-retention deadline across the VM boundary` to pass `worldSupportsHookRetention: true` so the supported path stays green; the webhook-rejection test is unaffected.
This preserves behavior for `world-local`/`world-postgres` (which declare the capability) while making QuickJS reject retention on `world-vercel`, matching the node engine.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: VaguelySerious <mittgfu@gmail.com>
The QuickJS engine dispatched hook_created concurrently with step_created and the step's queue message, so a step that calls abort() on a signal it received could reach the hook resume before the hook row existed. The resume throws HookNotFoundError and is swallowed as best-effort, so the abort was lost with no error. The node:vm engine already settles its hook phase before its step phase; this restores parity.
A suspension flushes its creates concurrently, so once one of them loses its slot every sibling behind it is proposing into the same taken range. The rejection is recorded on the log and the siblings rethrow it without issuing a create, which turns an N-way fan-out's N round-trips after a conflict into one. The merged delta is also trusted on every restart of a run that numbers its events, since a dense log holds exactly `maxSlot` events and a delta that left a hole is caught by that count instead of by a reload.
…tity # Conflicts: # packages/core/src/runtime/quickjs-runtime.test.ts # packages/core/src/runtime/quickjs-runtime.ts
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
(AI) AI review: blocking issues found
| // What this world stamps on new runs: slot identity (spec v6). It reads | ||
| // every earlier version too, so this only decides how the runs it creates | ||
| // from here on are numbered. | ||
| specVersion: SPEC_VERSION_SLOT_IDENTITY, |
There was a problem hiding this comment.
AI Review: Blocking
This world now advertises spec 6 unconditionally, and nothing on the client ever checks that the backend honored a claim. If a run gets stamped spec 6 while it is served by a backend that predates slot identity, that run silently loses its concurrency fence entirely:
- the backend ignores
eventId/maxSlotin the create meta and mints its own ULID id, answering 201; - the runtime reads the mode off the run's persisted
specVersion, sousesSlotIdentityis true andeventCreateFenceForreturns{eventId, maxSlot}instead ofpreconditionSnapshotParams— thestateUpdatedAtwatermark stops being sent; maxSlotOf(events)is 0 over a ULID log, so every replay's first write claims slot 1 again, forever, against a log that already holds N events;world.events.createnever comparesbody.event.eventIdtoparams.eventId, so there is no error and no log line.
Net effect: those runs run with neither fence. That is the exact failure mode this PR exists to remove, arriving without a signal. Reproduced against the real helpers:
const events = [/* 4 ULID-numbered events */];
expect(maxSlotOf(events)).toBe(0);
const fence = eventCreateFenceFor(
toMutableEventLog(events, 'eid:cursor'),
SPEC_VERSION_SLOT_IDENTITY
);
expect(fence.eventId).toBe(`evnt_${'0'.repeat(25)}1`); // claims slot 1 over 4 events
expect('stateUpdatedAt' in fence).toBe(false); // watermark disarmedThe same test drives claimFenceFor(...)((f) => create(f?.eventId)) against a create that returns a ULID-numbered event: it resolves normally, and log.maxSlot is then 1 while the log holds no slot at all. All assertions pass on this branch.
The PR body covers the SDK-upgrade-across-an-in-flight-run direction, but not this one. Deploy ordering and rollback are load-bearing here in a way a reader cannot infer from the diff. One of:
- verify the claim on the run's first fenced write — compare the returned event id to
params.eventIdand fail loudly (or fall back to the watermark for the run's life) when they differ; - gate the stamp on a negotiated backend capability rather than a constant;
- at minimum, state the requirement explicitly (backend deployed first, no rollback once spec-6 runs exist) in the PR body and in this comment, so the constraint survives the next person who reads this line.
There was a problem hiding this comment.
AI Review: Blocking
Still open on f029086. packages/world-vercel/src/index.ts:36 stamps SPEC_VERSION_SLOT_IDENTITY unconditionally, and the create path still forwards eventId/maxSlot (events.ts:773-774) without ever comparing the returned event.eventId to the claim, so the "neither fence" case is still silent.
Of the three options, (1) is the cheapest that removes the silence: on the first fenced write of a run, compare the returned id to params.eventId and either throw or fall back to the watermark for the run's life. (3) alone leaves the failure undetectable in production, which is the part I would not ship.
| // the batch is abandoned and re-invoked for a fresh | ||
| // replay, so a stale view can never commit a step). | ||
| // A slot claim gets there differently: it merges the | ||
| // missed events and retries in place, so the same |
There was a problem hiding this comment.
AI Review: Note
This comment says a slot claim "merges the missed events and retries in place, so the same events are observed without discarding the batch. See claimFenceFor." claimFenceFor documents and implements the opposite: "Neither scheme re-issues a rejected claim at a free number ... The rejection propagates and the run replays over the corrected log." withSerializedClaim latches the rejection on the log and rethrows for every claim behind it, and executeStep's slot branch either skips (benign duplicate) or propagates.
So the comment describes behavior that is not there and points the reader at the function that contradicts it — the worst combination for whoever reads this next. The surrounding paragraph's rewrap also left a stray // Hooks created line on its own.
There was a problem hiding this comment.
(AI) Fixed. The paragraph now ends: "A slot claim reaches the same place by the same route: it rejects, and the batch is abandoned for a replay over the corrected log. Neither fence retries a rejected write in place." The rewrap that orphaned the // Hooks created line is repaired too.
| } | ||
| const body = String(slot).padStart(SLOT_ID_WIDTH, '0'); | ||
| if (body.length > SLOT_ID_WIDTH) { | ||
| throw new Error(`Slot ${slot} does not fit in ${SLOT_ID_WIDTH} digits`); |
There was a problem hiding this comment.
AI Review: Nit
This post-check can never fire for the inputs that need it. String(slot) goes to exponential notation at 1e21, so slotIdBody(1e21) produces '0000000000000000000001e+21' — exactly 26 characters, so body.length > SLOT_ID_WIDTH is false and a body containing e+ is returned as a slot id. It passes the slot >= FIRST_SLOT guard above too.
Unreachable in practice (no run reaches 1e21 events), but the check reads as if it bounds the width and it doesn't. Asserting SLOT_BODY_PATTERN.test(body), or bounding slot to Number.MAX_SAFE_INTEGER, makes it actually hold.
There was a problem hiding this comment.
AI Review: Nit
Still open, and now asymmetric with the backend, which bounds its own slot parsing at Number.MAX_SAFE_INTEGER.
Two halves on this file: slotIdBody has no upper bound and its length post-check cannot fire (as above), and slotFromId accepts any 26-digit body, including values above Number.MAX_SAFE_INTEGER where Number(body) is imprecise, so two distinct bodies can resolve to the same slot. Bounding both at Number.MAX_SAFE_INTEGER closes the pair and makes the two ends of the same id agree on what a slot is.
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed at 54efccb. All suites green locally: core 1959 passed / 3 expected fail, world-local 572, world 100, world-vercel 342; root build + typecheck clean. (No docker here for the Postgres container suite or the race-repro rig, so those rest on CI plus your measured 2-of-14 → 0-of-14, whose variance caveat you stated honestly.)
The adversarial questions I brought to this all have answers in the code:
- Can a permanent hole wedge a run? No, twice over: claims are minted from
maxSlot + 1(never from count), so a hole can't cause a claim loop; and the density check falls back to exactly one authoritative reload before the replay proceeds (slotDensityCheckPending→loadWorkflowRunEvents), so an unrecoverable hole costs a reload, not a livelock. - Is the serialized claim chain actually tight? Yes.
withSerializedClaimswaps the chain promise synchronously before awaiting its predecessor (no interleave gap), theclaimRejectionlatch fails the rest of the batch locally with the correct reasoning (their fences all name the tail the rejection just proved wrong), and the slot rewind on failure (nextSlot = maxSlot + 1) is right for both entity-conflict retries and lost claims. The unknown-outcome case (network error where the write may have landed) converges: the next claimant collides, restarts, and the merged log carries the truth. slotFloorfor turbo — numbering claims from a snapshot that predates the backgroundedrun_startedwould make every turbo invocation's first write a guaranteed conflict; threading the floor throughtoMutableEventLogquietly removes a whole class of warmup 409s.- Delta discipline — a truncated delta is discarded (
hasMore→ null) rather than trusted, the slot-top-up path covers it from the cursor, and the ULID-mode full-reload rationale (holes defined by ULID time vs cursor's lexicographic filter) is exactly the lesson from the earlier watermark work, correctly carried forward. - Trace context — the v4 write path routes through
instrumentedFetch, which injects W3C context centrally (http-core), and the 5 trace-propagation tests pass. The historical v4 regression stayed fixed through this rewrite. - Per-kind correlation ids: the hashed per-kind bases with the lower-half leading character (so
incrementBase32can't overflow) are sound, and the module header's honest scoping — per-kind ordinals still shift within their own kind — is the right level of claim.
Three asks, none blocking approval:
- Rollout coupling deserves louder documentation. New runs are proposed at the slot-identity spec version unconditionally (
world.specVersion), and there is no downgrade path: a backend that doesn't accept spec-6 runs rejectsstart()outright with a 400. The backend acceptance has to be live everywhere before this SDK reaches users, and anyone operating a backend kill switch should know it hard-fails new starts from this SDK rather than degrading them to ULID runs. A sentence in theslot-event-identitychangeset (or runtime-tuning docs) would put that where operators will find it. (opts.specVersionas a manual pin is the escape hatch — worth mentioning too.) - The in-flight-run caveat should reach the changeset. The PR body owns that correlation-id numbering isn't spec-pinned, so upgrading the SDK across an in-flight run breaks that run on
world-local/world-postgres/self-hosted. That's release-note material, not just PR-body material — the folded-in deletion of the old per-kind changeset makesslot-event-identity.mdthe only place users will see this change described. - Trivial: the body says Postgres migration
0018; the file is0019_run_scoped_event_keys.sql(data-preserving PK swap + redundant-index drop — the SQL itself is fine).
Known CI state, for the record: the nextjs-webpack HMR rebuild-count failure is the long-standing baseline flake; the No Test Overrides failure is your disclosed temporary server-URL pointer doing its job — reminder that reverting it is the merge gate.
This is the strongest piece of engineering in the series — the scalar-watermark → event-count → positional-identity progression finally lands on a design where completeness is provable instead of approximated. Approving.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Deep review + local empirical validation (details in the inline comments; full storm data below). The design is right and the writing is excellent — position-claims-as-identity, the tail check over free-position, SPEC_VERSION_MAX_SUPPORTED, mode pinned to the persisted run, the epoch-0 ulidToDate defense, and the EventsConsumer delivery-in-flight grace are all correct calls, and the last one is worth landing regardless of the slot work.
Requesting changes on two blockers (silent loss of all fencing against a server without the slot half; turbo deriving mode from client-sent specVersion while the server's SLOT_IDENTITY_ENABLED can stamp differently → 400 storm) plus a set of important gaps where the description promises more than the code delivers (the rejection latch, delta-trust-on-every-restart, local-world's no-orphan claim) — inline.
Local empirical validation (storm harness, 24 step-storm attempts, conc 8, DB ground truth):
- world-postgres: 0/24 corrupted, 24/24 completed. Density 24/24, zero commit-order inversions (
pg_xact_commit_timestamp), zero causal-order violations across 5,224 steps, and every one of 9,542 reloads reporteddropped 0— the cursor-skip/non-prefix read class is empirically gone. The PR's "all 24 stuck" is a harness artifact: labels fire at the 240srunTimeoutMswhile p50 completion is 521s (max 841s) — every run was structurally guaranteed to be mislabeled. (Also: run the harness withWORKFLOW_POSTGRES_WORKER_CONCURRENCYpinned in both processes or the rig OOMs at 60 workers — plausibly the PR's earlier all-stuck run.) - Restart economics are the cost: 6,123 in-process restarts + 12,283 slot conflicts for those 24 runs (p50 completion ~3-4x lighter-concurrency reference), consistent with the missing latch + benign-conflict rewind + page-1 delta findings inline. The promised batched create matters more than "follow-up" suggests.
- world-local: corruption eliminated (A/B vs baseline: 1 corrupted + 175 divergences → 0 and 0) but replaced by a measured livelock: under the storm's poke load, replay restarts spin at ~8.8/s with zero forward log progress (657 restarts / 0 non-poke events in one 75s window); conc=8 is infeasible (~6% progress in 9 min) and runs die on REPLAY_TIMEOUT. Out-of-band writes move the tail faster than a replay can land, and the 12-restart budget burns inside one invocation's replay-timeout budget. I'd hold the local half until this has an answer (batched claims, fact/decision fence asymmetry, or admission control on pokes).
Verdict: postgres half is validated and close to landable once the two blockers and the delta/latch/economics set are addressed; local half needs the livelock resolved. Happy to share the full validation artifacts and the harness-correction runbook.
Findings on lines outside the diff hunks
-
packages/core/src/runtime.ts:2103— Important — the density check runs on only one of four merge paths, and skips the one that matters.maxSlotOf(events) !== events.lengthis gated onslotTopUpPending: nothing asserts density after the initial full load, the preload path, the success-path inline delta, or the 409-delta restart — the last one mergesslot > maxSlotonly, so a hole below the client'smaxSlotis neither repaired nor detected before re-deriving. Also the PR body's "trust the inline delta on every restart, not just the first" doesn't match the code (allowDelta && preconditionRestarts === 1); one of the two should change. -
packages/world-postgres/src/drizzle/migrations/0018_run_scoped_event_keys.sql:1— Important (operator impact) — this PK swap takes an ACCESS EXCLUSIVE lock with no operator note.DROP CONSTRAINT+ADD PRIMARY KEY+DROP INDEX, noneCONCURRENTLY: on a largeworkflow_eventstable that's a full PK index rebuild while every read and write blocks. Either split it (build the new unique index CONCURRENTLY, then swap) or add a migration note so self-hosters schedule it. (The index-usage claims themselves check out — all seven event queries filter runId first.) -
docs/content/worlds/v5/building-a-world.mdx:96— Important — the new World obligations aren't documented for world authors, and the conformance suite has zero slot coverage. This page still documents only the watermark guard; a community/self-hosted world author (Platformatic, SurrealDB, Gusto) can't learn from docs that declaring specVersion 6 requires honoring client-mintedeventId, enforcing(runId, eventId)uniqueness, the tail check, 409SlotConflictError+ delta, and mode-mismatch 400s. The excellent contract text onCreateEventParams.eventIdmostly needs lifting into this page, plus aworld-testingconformance case so partial implementations fail loudly.
| * | ||
| * The reservation has to happen here rather than at the World or its backend. | ||
| * Slots are handed out for a whole concurrent batch synchronously, before any of | ||
| * it lands, so a second event numbered off the log as the backend sees it would |
There was a problem hiding this comment.
Blocking — a server without the slot half silently removes ALL fencing. eventCreateFenceFor is either/or: slot runs emit {eventId, maxSlot} and stop sending stateUpdatedAt/stateEventCount. The v4 meta parser drops unknown keys silently, so against a server that predates slot support the create lands with no fence of either kind — no 409, no 412 — and the degradation cascades quietly (maxSlotOf = 0 on the resulting ULID log, nextSlot resets to 1 every replay, density check permanently true → full reload every restart). WorldCapabilities.preconditionGuard exists precisely to prevent "runtime relies on a fence the backend doesn't enforce" (interfaces.ts:328) — the slot fence has no equivalent.
Two cheap fixes, ideally both: (a) send the watermark snapshot alongside the claim so an old server still enforces 412s; (b) in withSerializedClaim, compare result.event.eventId against fence.eventId and fail loudly on mismatch/absence (also catches the relocation case, see the maxSlot comment below). Verified against the merged v4 parser behavior.
There was a problem hiding this comment.
(AI) Took (b), and (a) is not available.
(a) would have the runtime send a watermark snapshot derived from the log's ULID times, and a slotted log has none. latestEventStateUpdatedAt over padded slots decodes to epoch 0, so the snapshot would be a value a guard-enforcing backend acts on and every write of the run would be judged against 1970. Sending a fabricated fence is worse than sending none.
(b) is in: withSerializedClaim reads the created event's id back out of the EventResult and assertClaimLanded throws WORLD_CONTRACT_ERROR when it differs from the claim or is absent. A backend that drops the field mints its own id and answers 200, which is indistinguishable from a claim that won until you look at the id, so the run now fails loudly on its first write instead of degrading into the cascade you describe.
| // `run_created` from start(), then the `run_started` | ||
| // in flight above. Both are certain before any write of | ||
| // this invocation, and turbo replays against the empty | ||
| // snapshot skipped just above — so seed the floor with |
There was a problem hiding this comment.
Blocking (cross-PR integration) — turbo takes the run's mode from the client-sent specVersion, but the server can stamp it differently. The paired server (#692) has SLOT_IDENTITY_ENABLED gating run_created (config.ts:178): with the flag off it stamps new runs ≤5 while this path sets knownSlotFloor and claims slots off runInput.specVersion — every create of every new turbo run then 400s on mode mismatch (server I3). The mode a run actually got is only knowable from the server's response/persisted row; trusting the client-side intent makes the server's own kill switch an outage switch for turbo. Suggest deriving mode from the run the server returns (or a capability probe), mirroring how hook-resume dedup deliberately avoids trusting a client-supplied mode (world-vercel/index.ts:44).
There was a problem hiding this comment.
(AI) Not reproducible as described. The backend does not downgrade a run's mode: one that will not create a run at this spec version rejects run_created with a 400 rather than stamping it lower. start() then throws, and there is no run, no queue message and no turbo replay to mis-number.
By the time this path runs, runInput.specVersion is the spec of a run whose creation already succeeded, so it agrees with the persisted row by construction. The kill switch is a gate on what gets created, not a way to move an existing run between modes. Keeping as-is.
| .prepare('events_get_hook_by_token'); | ||
|
|
||
| // Used to distinguish a real same-hook duplicate from an orphaned | ||
| // hook row left behind by a process / database interruption between |
There was a problem hiding this comment.
Important — the 409 delta is computed from page 1 of the log, not from the client's maxSlot, so it's empty for any run past one page. eventsAfterClaim pages from params.sinceCursor — which claim creates never set — then filters slot > maxSlot in memory. Past 100 events (20 on world-local), the filter yields [] with hasMore: true, preconditionEventDelta returns null, and every recovery becomes a full reload; isBenignDuplicateStart also never sees its evidence. Validation data agrees: 9,460 slot-top-up reloads vs 80 inline-delta in a 24-run storm. maxSlot is already on the wire — page from slotEventId(maxSlot) instead.
There was a problem hiding this comment.
(AI) Correct, fixed in both worlds. eventsAfterClaim now pages from slotEventId(maxSlot) when the claim named one, falling back to sinceCursor only for a claim that did not. Dense slots make that exact rather than approximate: the caller cannot be missing an event at or below the highest position it can name.
| * we were about to claim. The step is then someone else's to finish and we | ||
| * can skip, exactly as we do on the `EntityConflictError` the unfenced path | ||
| * would have raised instead. | ||
| * |
There was a problem hiding this comment.
Important — isBenignDuplicateStart can never match on the lazy inline path. Both worlds strip input from the persisted step_started (postgres storage.ts:1336, local events-storage.ts:1572), so sameSerializedInput(lazyStepInput, undefined) is always false exactly where the strict check was meant to apply — every lazy duplicate start pays a full replay restart instead of the intended skip. The unit tests pass because startedEvent() synthesizes events carrying input, which no world persists. Compare against the companion step_created's input (it's in the same delta), or drop the input comparison and rely on name+correlationId.
There was a problem hiding this comment.
(AI) Right, and the tests were the reason it read as covered. isBenignDuplicateStart now takes the input off the companion step_created in the same delta, which is the event that carries it. Added a case for a delta whose start has no companion, so the fixture can no longer supply an input no World persists.
| // non-run_created events on this run's `runId`. | ||
| const reporter = replayRecoveryReporter ?? ReplayRecoveryReporter.inert(); | ||
| const createEvent: EventCreator = (data, params) => | ||
| reporter.withEventCreate(params, (p) => |
There was a problem hiding this comment.
Important — settlePhase prefers a 412 but not a 409. Under slot identity the recoverable rejection is SlotConflictError; a phase producing both a slot conflict and a FatalError throws the FatalError, isStaleWriteRejection is false for it, and the run takes the failure path instead of the restart. The comment's rationale for preferring 412 applies verbatim to 409 — isStaleWriteRejection already exists; use it here.
There was a problem hiding this comment.
(AI) Fixed. settlePhase picks with isStaleWriteRejection, so a phase producing both a slot conflict and a FatalError takes the restart.
| // backend names its machine-readable code `error`; that field is read only | ||
| // here, so every other error keeps the status → type mapping below unchanged. | ||
| if (statusCode === 409 && decoded?.error === V4_SLOT_CONFLICT_CODE) { | ||
| return slotConflictFromBody(message, responseHeaders, decoded); |
There was a problem hiding this comment.
Important — 409 classification is body-decode-dependent and fails in the unsafe direction. Only decoded?.error === V4_SLOT_CONFLICT_CODE yields SlotConflictError; a slot-taken 409 whose content-type gets rewritten or body truncated decodes to undefined and arrives as EntityConflictError, which nearly every call site reads as "my write already landed" and skips — but it never landed. The delta payload was hardened against exactly this (slotConflictFromBody reads the conflicting id from a header "so the error is still actionable when the body failed to decode"); the classification deserves the same: an x-wf-error-code header, or defaulting an undecodable 409 to SlotConflictError with an empty delta (forces a full reload — always correct).
There was a problem hiding this comment.
(AI) Fixed the way you suggested. Classification now leads with the x-wf-event-id header the slot-conflict response carries, which survives a truncated or re-encoded body; the body's error code is the fallback for a response that lost its headers. The asymmetry is spelled out in the comment: an entity conflict misread as a slot conflict costs a replay restart that was always safe, the reverse drops the write for good.
| span?.setAttributes({ | ||
| ...Attribute.StepSkipped(true), | ||
| ...Attribute.StepSkipReason('completed'), | ||
| }); |
There was a problem hiding this comment.
Important — a real correlation-id collision (conflict, not slot-conflict) is invisible and can wedge a run as silent success. EntityConflictError is swallowed as "already landed" at every site (here, and suspension-handler:179/355/507) with no counter, no restart, no divergence report. The paired server's own test documents the misread as unclosed (slot-identity.integration.ts:403 on the server branch): entity materialized, event write lost its slot, re-post trips the entity conditional → conflict → SDK maps to skipped → no step_created ever lands and the replay believes the step is owned by a writer that doesn't exist. On a slot-mode run, an EntityConflictError whose merged log contains no event for that correlationId is provably NOT "mine already landed" — escalate it, and count these either way.
There was a problem hiding this comment.
(AI) Logged, not escalated, and the reasoning is in the code now.
There is no local signal that separates the two causes. A step row says nothing about which writer created it, so "someone else owns this step" and "my own row, left behind by a write that lost its position" arrive as the same error. Failing the run on the suspicion trades a rare wedge for a frequent false alarm, since skipping is right in the common case.
So on a slot-numbered run this reports at warn with slotNumbered on the record, which makes the population countable. The durable fix is on the World side: the entity row and its event have to land together, and then a lost position leaves nothing behind to trip over.
| ) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Important (upgrade ordering) — old core + new world-postgres fails at startup. This relaxation (>= CURRENT && <= MAX_SUPPORTED) exists only on this branch; main's core still requires an exact match, so a self-hoster bumping the world package alone gets a hard failure. Worth stating the core-first upgrade order in the world-postgres changeset (and arguably a major bump).
There was a problem hiding this comment.
(AI) Added a changeset saying it: upgrade @workflow/core together with your World package, because a World declaring the new spec version is refused at startup by an older runtime. Left it at minor rather than major: nothing in the World's own API changes, and the failure is a loud startup refusal rather than a run that misbehaves.
| preloadedEvents = undefined; | ||
| preloadedEventsCursor = undefined; | ||
| pendingInlineDelta = null; | ||
| slotDensityCheckPending = false; |
There was a problem hiding this comment.
Telemetry — an operator can't tell 409 churn from 412 churn. Both fences are live at once and isStaleWriteRejection unions them; the restart warn and workflow.precondition_restarts don't record the rejection class, and backoff waits are unmeasured. Adding the class dimension (slot-conflict vs precondition-failed) plus batch width makes the rollout legible — especially since a wide flush can exhaust the 12-restart budget with no writer being wrong, and the failure message will misdiagnose the run.
There was a problem hiding this comment.
(AI) Added. The restart warn now carries rejectionClass (slot-conflict / precondition-failed / none) and backoffMs, and the span carries workflow.stale_write_rejection_class next to workflow.precondition_restarts.
Batch width is not on the record. The restart is raised from the loop, which sees one rejected write and not the flush it came from, so plumbing the width there means threading it through the suspension result purely for the tag. The class plus the restart count is enough to tell the two fences apart during the rollout, which was the part that could not be reconstructed after the fact.
| await handleSuspension({ | ||
| suspension: synthesized, | ||
| world, | ||
| run: workflowRun, |
There was a problem hiding this comment.
Minor (but it compounds) — a swallowed drain rejection guarantees the terminal write loses its slot. A 409 in drainPendingQueueItems is caught and warned, rewinding nextSlot onto a slot the world has written; the run_completed claim below aims at it and 409s too. It's also the one path where a 409 escapes both restart budgets entirely. Recoverable, but the drain's events are silently dropped and the terminal write reliably pays a restart under contention.
There was a problem hiding this comment.
(AI) Addressed by removing the cause rather than the catch. The replay's event log is threaded into the drain, so its writes claim from the same source the terminal write draws from and the two no longer propose the same position.
If the drain does lose a claim, the rejection latches on the log, so the terminal write fails without a round trip and the run restarts over the corrected log, which retries the drain. Recorded, one replay long, and no longer a path where a 409 escapes both budgets. The catch stays because a drain failure must not turn a completed run into a failed one, and its rejectionClass is on the warn.
Keeps main's `__worldCapabilities` gate for QuickJS hook retention and drops the parallel `worldSupportsHookRetention` plumbing this branch had added for the same check.
A step's terminal event, and the `step_created` a lazy start defers, carried no client-assigned slot, so the backend placed them at the tail. That tail is the position the next start had already reserved for its own deferred create, so that start lost a claim no foreign writer had contended: the replay restarted, abandoning the siblings whose bodies had already run. On the recursive `fibonacciWorkflow` e2e it turned 25 runs into 64, with 32 restarts. `orderedCreateFor` draws those writes from the same serialized chain the claims use, so the two cannot name the same slot. Its claim is allocation rather than a guard: a genuine conflict re-issues the write unnumbered for the backend to place, since a step's terminal event is identified by its correlation id, fixed already by the start that landed, and the work it records must not be dropped over a position that carries no meaning.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| log.nextSlot = log.maxSlot + 1; | ||
| if (!SlotConflictError.is(error)) throw error; | ||
| const delta = preconditionEventDelta(error, runId); | ||
| if (delta) mergeLoadedEvents(log, delta.events); |
There was a problem hiding this comment.
AI Review: Blocking
orderedCreateFor's conflict path resolves its own 409 by merging the delta, and mergeLoadedEvents ends with log.claimRejection = undefined (helpers.ts:815). That clears a different write's latched rejection, and it undoes the fast-fail added in 36a789b in exactly the interleaving this commit exists to handle.
The doc on mergeLoadedEvents justifies the clear with "the merged events are the ones a rejected claim was missing, so the log is usable again and its recorded rejection is cleared." That premise does not hold at the only live call site, and line 1278 is the only caller outside tests. Here the merged events are the ones the ordered create was missing; the latched rejection belongs to a claim that is still doomed, whose caller is already restarting the replay.
Sequence, all on one log:
- a
step_startedclaim loses its slot, throws, latchesclaimRejection; - a sibling's terminal write, numbered off the same chain, also loses, merges its delta, re-issues unnumbered and succeeds, clearing the latch;
- the next sibling claim no longer fails locally. It issues a create, can succeed, runs its step body inline, and step 1's restart then abandons it: the
step_started-with-no-terminal shape, plus an extra write and a round trip.
Reproduced (not in the PR; fails on this head, passes with the fix):
const log = toMutableEventLog([slotEvent(1), slotEvent(2)], 'eid:cursor');
const claim = claimFenceFor(log, SPEC_VERSION_SLOT_IDENTITY);
const ordered = orderedCreateFor(log, runId, SPEC_VERSION_SLOT_IDENTITY);
// A claim in the batch loses its slot and latches the rejection.
await expect(claim(() => Promise.reject(conflict(3)))).rejects.toThrow(/slot taken/);
expect(log.claimRejection).toBeDefined();
// A sibling terminal write off the same chain loses, merges, re-issues unnumbered.
let orderedAttempts = 0;
const result = await ordered!((fence) => {
orderedAttempts++;
if (fence) return Promise.reject(conflict(3, [slotEvent(3, 'step_started')]));
return Promise.resolve({ event: slotEvent(4) } as any);
});
expect(orderedAttempts).toBe(2);
// The batch is still doomed, so a sibling claim must not reach the World.
let siblingCreates = 0;
await expect(
claim(() => {
siblingCreates++;
return Promise.resolve({ event: slotEvent(5) } as any);
})
).rejects.toThrow(/slot taken/);
expect(siblingCreates).toBe(0);On f029086 the last claim resolves with evnt_00000000000000000000000005 instead of rejecting, so siblingCreates === 1.
Either fix works. Save and restore log.claimRejection around the merge here, or drop the clear from mergeLoadedEvents — no other caller wants it and no test pins it. I ran both: the test above passes, and all 98 tests in helpers.test.ts stay green.
There was a problem hiding this comment.
(AI) Fixed by deleting mergeLoadedEvents outright. A rejected claim restarts the replay over a freshly loaded log rather than patching the log it decided from, so nothing needs to merge a delta in place and nothing needs to clear the latch. toMutableEventLog carries a note on why in-place merging is deliberately absent, so it does not come back.
| ): OrderedCreate | undefined { | ||
| if (!usesSlotIdentity(specVersion)) return undefined; | ||
| return (op) => | ||
| onWriteChain(log, async () => { |
There was a problem hiding this comment.
AI Review: Note
onWriteChain is held across the whole op round trip, so on a slot-numbered run every write in a batch is now strictly serialized, including the terminal writes that previously went out concurrently. The payload travels in the same request, so this serializes payload transfer too, not just the id exchange.
The trade looks right to me: releasing the chain at reservation time would let a later write commit ahead of an earlier reservation, and the earlier one would then pay a wasted round trip to discover it. Two asks rather than a change:
- the fan-out numbers in the PR body predate this commit, and fan-out is the shape this affects most. Worth re-measuring width 10 and width 40 before the rollout decision.
- say in the body (or on
OrderedCreate) that slotted writes are serialized per run, because it is a per-run throughput ceiling a reader would not infer from "numbering writes off the same chain".
There was a problem hiding this comment.
(AI) Both taken. The serialization and its cost are documented on onWriteChain, including that batch create is the way out of the per-run ceiling. Re-measuring width 10 and 40 on this head before the rollout decision and updating the body with those numbers; the ones there now predate the chain.
A step's terminal write named the slot it was ordered onto, and losing that claim destroyed the event: a backend that materializes an entity before it publishes has already applied the transition the lost event described, so the re-issue is refused as a duplicate. The step's entity ends up terminal with no terminal event in the log, and every later replay re-runs the owned recovery start against it forever. Ordering the write against the batch's claims is what removed the collision; naming a position for it was never needed. Folding the backend's answer back in before the turn ends leaves the next claim drawing above it.
`slotIdBody` checked only that the padded body was not too long. `1e21` is an integer whose `String` form is `1e+21`, which pads to exactly 26 characters, so the check passed and the function returned `0000000000000000000001e+21` as an id. `slotFromId` had the mirror gap: a 26-digit body of nines parses to a number a double cannot separate from its neighbours, and seeding `maxSlotOf` with one yields reservations this module mints and cannot parse back. Both now bound at `MAX_SLOT` (`Number.MAX_SAFE_INTEGER`), the point past which `slot + 1 === slot` becomes possible, matching the bound the backend already applies to the ids it accepts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tity # Conflicts: # packages/core/src/runtime.ts # packages/core/src/runtime/resume-hook.consumer-preload.test.ts # packages/world-vercel/src/events.ts # packages/world/src/events.ts
|
Superseeded by #3389, parking for now |
Prove the World honored a slot claim by reading the created event's id back: a backend that drops the field numbers the event itself and answers 200, which reads exactly like a claim that won, and a slotted log has no ULID watermark to fall back on. Check density once per invocation on the initial load, not only on the restart paths, and reload behind a preload that comes up short. A log dense from slot 1 holds exactly `maxSlot` events, so a short count is either a load that was skipped or a hole, and only the first is fixable. Read a duplicate start's input off its companion `step_created` rather than off `step_started`, which under slot identity no longer carries it. Page a conflict delta from the log's high-water mark, and drop `mergeLoadedEvents`: a rejected claim restarts the replay over a fresh snapshot rather than patching the log it decided from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tity # Conflicts: # packages/core/src/runtime.test.ts # packages/core/src/runtime.ts # packages/core/src/runtime/step-executor.ts # packages/core/src/runtime/suspension-handler.ts # packages/core/src/step-delivery-ordering.test.ts # packages/world-local/src/storage/events-storage.ts # packages/world-vercel/src/events-v4.test.ts # packages/world-vercel/src/events-v4.ts # packages/world-vercel/src/events.ts
…tity # Conflicts: # packages/core/src/events-consumer.test.ts # packages/core/src/events-consumer.ts # packages/core/src/private.ts # packages/core/src/workflow.ts
main shipped server-allocated slot identity (#3389), which covers most of what this branch built and does it with different names. Rather than merge 43 files hunk by hunk, this takes main's tree wholesale so the branch baseline is exactly main; the client-allocation delta lands on top as its own commit.
Sim WorldSimulated world deterministic testing for races. Traces 🟠 Mint-ordered log — 6 fail of 41 total
Full trace: 🟢 Append-only log — 0 fail of 41 total
Full trace: |
main lets the World allocate: a write sends the position it read the log at, the World takes the next free slot from the tail, and if that is not the position the writer asked for it commits anyway and reports back the events occupying the slots in between. Bind the write to the position instead. On a slot-numbered run `eventCount + 1` is where the event goes or nowhere, so the insert that occupies it settles in one operation both that nothing else took the position and that the log the write was decided from is still the whole log. A position already taken answers 412 with the events the writer had not seen, which is the path the runtime already merges and replays from. That needs no new wire field and no new error class. What it does need is that each write of a batch names its own position: the suspension flush is a concurrent fan-out of twenty writes reading one log, and the inline step batch shared one snapshot across every claim, so a single position per phase would be one winner and nineteen rejections every time. `reserveEventSlots` hands out consecutive positions off the log's maximum, rebasing when that maximum moves, and a write that persists more than one event takes the whole span it needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A backend cannot tell a write that names the slot it takes from one that reports how much of the log it holds, because both are the same number. Reusing the reporting field for the claim would bind every SDK that only reports, and those reuse one value across a whole flush of writes, so all but the first would be rejected on a slot they never claimed.
Reserving consecutive positions locally let a write commit two above a log it never re-read, so a sibling's rejection could not stop it: the decision had already landed without the event it was missing. It also left permanent holes, because a reserved position whose write never lands is never filled while reserved siblings above it commit. The event-log race repro caught both as one hole per run. A writer now names max(loaded) + 1 and nothing else. Writes issued from one snapshot name one position between them, one takes it, and the rest merge their 412 delta and name the position above that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 participants
Events are numbered by position instead of by ULID:
evnt_…001is a run's first event,evnt_…002its second, no gaps. The runtime mints the id itself, and the id is its claim on that spot in the log. A World inserts it under a uniqueness constraint, so a 409SlotConflictErrortells the writer someone else got there first, and therefore that it replayed from an incomplete log.That removes ULID clock re-ordering, gaps, misplaced cursors, and silent contention as failure modes.
Correlation ids also get one sequence per entity type (steps, waits, hooks, attributes, abort controllers, streams) instead of one shared across the run. With a single sequence an id is an ordinal over the whole run, so one extra
sleep()in one replay renames every id after it and two replays' writes land side by side. Shipped opt-in asWORKFLOW_PER_KIND_CORRELATION_IDSin #3301; here it is the only scheme and the flag is gone.One claim at a time
Numbering a concurrent batch up front only fences the first write in it. The rest sit above positions their own siblings have not filled yet, so a foreign event can slip into that space without tripping anything, and the batch commits decisions taken without it. That was still corrupting logs with positional ids on.
Claims are now drawn one at a time off a per-log chain, so every write names the position right after the tail its writer saw. A rejection stops the whole batch, since the batch was decided from a log missing an event, and it is latched on the log so the other 19 writes in a 20-way flush don't each rediscover the same taken position.
Worlds check a claim against the log's tail, not against the position being free. A failed write leaves its position empty forever, so a log can carry holes below its tail, and a writer numbering from a stale snapshot aims straight at one. Accepting it would land an event below events another replay has already consumed.
Recovery is a replay, not a resend
Re-sending would lose the same position again, and the event we were missing may send the workflow down another branch. So each attempt merges what it missed (inline off the 409 body, topped up from the World when truncated), restarts the replay, and claims whatever position that replay lands on.
eventCreateFenceForpicks the run's fence: its event position, or thestateUpdatedAtwatermark for a run that predates this. The two loops stay separate. A 409 and a 412 don't prove the same thing, and both are live while older runs drain.run_completedand the inlinestep_startedclaims don't retry in place; a rejection escapes to a fresh replay.stateUpdatedAtForCreatetakes the mode explicitly. Inferring it gives a wrong answer rather than none: a padded position is valid Crockford base32, so decoding it yields epoch 0.assertClaimLanded). A World that ignores the field numbers the event itself and answers 200, which is indistinguishable from a claim that won until you compare the ids, and a positional log has no watermark to fall back on. The mismatch fails the run instead.WORKFLOW_PRECONDITION_RESTART_BACKOFF_MS) and a larger budget, since a restart reads only the page past its cursor.Worlds
SPEC_VERSION_MAX_SUPPORTEDsplits the newest version a World can read from the one it stamps. Without the split, every World would reject the runs it had just created.(run_id, id), migration0019), sinceevnt_…001now exists once per run. The run leads the key so existing range scans stay one index seek, which makes the standalonerun_idindex redundant.Both schemes are unconditional. A run keeps the numbering it was created with, because the mode is read from the persisted
specVersionand never from the build, so existing runs keep their ULID event ids. Correlation ids aren't pinned that way (the replaying build mints them), so upgrading the SDK across an in-flight run breaks that run onworld-local,world-postgresand self-hosted.world-vercelskew protection prevents it there.Testing
Unit: core 1950, world-local 554, world-vercel 342, world 103, world-postgres 183 (testcontainer). All green. New coverage for per-kind determinism and cross-kind independence, dense numbering, mode pinning both directions, the conflict delta, the tail check, the no-orphan guarantee, and 2/8/50-way contention against real Postgres.
pnpm run test:e2e:event-log-race-repro:localagainst Postgres at the default 14-run scale:CORRUPTED_EVENT_LOGmain(ba2cddc861)The rest here were 13
completedand 1stuck. Earliermainpasses at this scale hit 9 of 14, so the 2 is the low end of a wide spread rather than a rate. Thestuckis the rig, not the branch: one Next.js process carries every replay, and at 24 attempts everything came backstuckwith the logs dense and intact. Positions were dense in every run I inspected, either way.The two ordering-sensitive hook tests (
hookWithSleepWorkflowracing a sleep,hookTokenReuseLoopWorkflow) also ran against a productionexpressbuild onworld-local: 126 passed, 7 skipped, 2webhookWorkflowfailures that reproduce with positional ids off.Known cost: serializing claims turns an N-event suspension flush into N round-trips, and the chain is per run, so that is a per-run write ceiling for every positional run, terminal writes included. The fence can't be pipelined, since each claim has to name the tail its writer actually saw, so the fix is a batched create allocating N contiguous positions in one request. Follow-up, along with retiring the
stateUpdatedAtwatermark once no older runs remain.Docs
SlotConflictErrorhooks.list()ordering(Behind deployment protection, so the links need Vercel team access.)
WORKFLOW_SERVER_URL_OVERRIDEpoints at a preview deployment and will be reverted before merge. No need to flag it or its lint failure in review.