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 19 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: 54efccb 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 |
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests💻 Local Development (1 failed)nextjs-webpack-canary-quickjs (1 failed):
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 124965ms → this run 165538ms (Δ +40573ms, +32%) 1020 steps (queue-hop) Cumulative STSO time: main 3102ms → this run 2404ms (Δ -698ms, -23%) ℹ️ 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 ReproNo event-log regressions in the latest repro job. Run History
Latest Scenario Breakdown
|
…tity # Conflicts: # docs/content/docs/v5/configuration/runtime-tuning.mdx
Correlation ids always draw from a sequence per entity family. The WORKFLOW_PER_KIND_CORRELATION_IDS opt-out is gone, along with the shared run-wide sequence it selected and the test helper that pinned suites to it. Test fixtures that hardcoded ids the shared sequence minted are regenerated against their kind's own sequence, and the two sites in workflow.test.ts that derived expected ids from a raw monotonicFactory now derive them from the run's generator. Folds the per-kind changeset into the slot-event-identity one.
Fixtures that pinned correlation ids to the shared run-wide sequence now derive them from the kind's own sequence, and the two places in workflow.test.ts that built expected ids from a raw monotonicFactory derive them from the run's generator instead. Also documents hooks.list() ordering: a hook the runtime creates on your behalf draws from a separate sequence, so it sorts at an arbitrary position among the ones a workflow created.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| // The slots this attempt drew are not the writer's, and the tail is at | ||
| // least as high as the claim that lost. Rewinding onto the occupied range | ||
| // is what makes the rest of the batch fail with it. | ||
| log.nextSlot = log.maxSlot + 1; |
There was a problem hiding this comment.
AI Review: Note
The rewind makes the rest of the batch fail, but it makes each of them fail at the World, one at a time. Once a claim on this log has lost, every claim behind it is already decided: nextSlot is pinned to maxSlot + 1, which is either taken or at-or-below the tail, so each sibling re-proposes the same occupied slot and gets the same answer. They are still sent, and because claims are serialized they are sent sequentially.
I measured this against the branch with a throwaway test: a 20-way batch whose first claim loses issues 20 sequential creates, and attempts 2..20 all propose the identical occupied slot. Each is a full round-trip whose only outcome is a rejection the client discards, and each one makes the World build and send a conflict delta that is thrown away. On the suspension flush that is the difference between one round-trip and twenty before the replay can restart.
The semantics you want (none of the batch lands) are unchanged if the rejection is latched on the log and later claims fail locally: record the rejection alongside the nextSlot rewind and have withSerializedClaim rethrow it instead of calling op. Scope the latch to the log instance so a restart's fresh log starts clean.
It also shortens the Promise.allSettled wait on the inline-claim path, which currently blocks the restart until every doomed sibling has made its own round-trip.
There was a problem hiding this comment.
Done in 36a789b. The rejection is latched on the log (claimRejection) next to the nextSlot rewind, and withSerializedClaim rethrows it before calling op, so a 20-way batch whose first claim loses issues one create instead of twenty and the restart no longer waits on nineteen doomed round-trips.
The latch is gated on isStaleWriteRejection (412 or 409) rather than on any create failure. An EntityConflictError means the event never landed, so the slot is still free and the nextSlot rewind alone is the right recovery; latching it would break the wait-completion loop in runtime.ts, which swallows entity conflicts and keeps going. Scope is the log instance, so a restart's fresh log starts clean.
| const result = await op(fence); | ||
| log.maxSlot = Math.max( | ||
| log.maxSlot, | ||
| maxSlotOf([{ eventId: fence.eventId ?? '' }]) |
There was a problem hiding this comment.
AI Review: Nit
Routing a known slot id through maxSlotOf by wrapping it in a synthetic one-element event array reads as a workaround for not having a parse helper. slotOf(fence.eventId) (or reusing whatever maxSlotOf calls internally) would say the same thing directly, and would not need the ?? '' that only exists to satisfy the array's element type.
There was a problem hiding this comment.
Done in 36a789b. reserveSlotFence now returns { fence, slot }, so withSerializedClaim advances maxSlot from the integer it already had. No synthetic event array, no ?? ''.
| // rejection. | ||
| // Trust it only on the FIRST restart: its completeness proof | ||
| // leans on the backend's own bookkeeping, so if that | ||
| // under-counts, a "complete" delta can still leave a hole. |
There was a problem hiding this comment.
AI Review: Note
The reason given for bounding the delta to the first restart is that its completeness proof leans on the World's own bookkeeping. That is the watermark scheme's problem. Under slot numbering the client can check the merged result itself: a dense log from slot 1 holds exactly maxSlot events, which is the test already applied to the slot top-up path a few lines below.
So for a slot-numbered run the delta could be trusted on every restart, with maxSlotOf(events) !== events.length after the merge as the backstop that falls through to a full reload when the merge did not close the gap. As written, restarts 2..N each spend a list round-trip re-fetching what the rejection already handed over for free, and the in-process budget for slot runs is 12 - so up to 11 avoidable round-trips inside one invocation, on exactly the runs that are already losing races.
preconditionEventDelta returns null when hasMore is set, so a truncated delta cannot reach this path to begin with.
There was a problem hiding this comment.
Done in 36a789b. For a run that numbers its events the delta is trusted on every restart, with maxSlotOf(events) !== events.length after the merge as the backstop that falls through to a full reload when the merge left a hole. The flag that carries that check is renamed slotDensityCheckPending to match what it now guards.
A run guarded by the watermark keeps the first-restart bound, since it has no equivalent local check.
…tity # Conflicts: # packages/world-local/src/storage/events-storage.ts # packages/world-local/src/storage/helpers.ts # packages/world-postgres/src/drizzle/migrations/meta/_journal.json # packages/world-postgres/src/index.ts
…ition A second replay's hook_created adopts the canonical position recorded in the token claim. Under slot identity that write had also picked a position of its own, so losing the canonical one was read as ordinary slot contention and the writer reallocated around it, publishing a second hook_created for one hook and leaving its first pick as a hole. Pin a converged write to the canonical position: losing it means the hook exists, which the runtime already swallows as a benign concurrent replay. Release the position the write gave up so the log stays dense.
… does A claim asserts the log is complete up to the position it names, so the client takes the batch's claims one at a time, each against the tail its writer has actually seen. The test issued all ten at once, and when they arrived out of order the tail fence rejected a claim that was genuinely free — intermittently failing on a rejection the client cannot provoke. Serializing costs the test nothing: numbering the second event off the log rather than off the claim still leaves the batch's first reserved slot a hole, which the density assertion still catches. Also covers the fence itself, which had no direct test here.
…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.
| // 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.
| } | ||
| 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.
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.
| // `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).
| @@ -484,7 +498,7 @@ export function createEventsStorage(drizzle: Drizzle): Storage['events'] { | |||
| // 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.
| * 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.
| @@ -276,19 +276,19 @@ export async function handleSuspension({ | |||
| 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.
| // 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).
| @@ -488,6 +566,37 @@ export async function executeStep( | |||
| }); | |||
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.
| ) { | ||
| 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).
| 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.
| @@ -109,6 +117,7 @@ async function drainPendingQueueItems( | |||
| 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.
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.
withEventCreateFencepicks 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.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), migration0018), 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. 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.