Add support for 'noop' event type - spec version 7 - #3634
Conversation
🦋 Changeset detectedLatest commit: 62084a5 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 |
📊 Workflow Benchmarkscommit Backend:
Streams
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 261936ms → this run 203083ms (Δ -58853ms, -22%) 📈 CRTT drill-down vs main (RTT distributions & profiles)RTT over stream progress (avg per tenth of stream, bars scaled min→max): RTT by chunk size (avg per log size bin, ~160B → ~12KB serialized, bars scaled min→max): Delivery jitter over stream progress (avg positive CDV per tenth of stream, bars scaled min→max): ℹ️ Metric definitions & methodologyStreams: first-chunk RTT (the stream-open path, before any buffering/backpressure), CRTT percentiles, and worst delivery stall (CDV max). Cells are medians across iterations; per-run values in the artifacts. No 🔴/🟢 marks until targets attach. The collapsed STSO distribution section above buckets every step gap, split inline (same warm process — pure framework overhead) vs queue-hop (fresh process — dispatch, reinit, replay). The collapsed CRTT drill-down: per-variant RTT histograms (fixed log bins, 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) · Fan-out TTFS: fan-out time to first step (in-deployment start() → first of the parallel step bodies to complete) · Fan-out TTLS: fan-out time to last step (in-deployment start() → last of the parallel step bodies to complete, i.e. when the Promise.all resolves) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · CRTT: chunk round-trip time (per-chunk write → read latency, one clock domain: deployment → stream backend → same deployment) · CDV: chunk delay variation / delivery jitter (inter-arrival gap minus inter-write gap per seq-adjacent pair; skew-free; the row is each run's MAX positive value, so one stall moves it) 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 · Promise.all(100 steps): 100 trivial no-op steps started together in a single Promise.all; Fan-out TTFS is the first of them to complete and Fan-out TTLS the last, both from the in-deployment clientStart, so their gap is the spread the runtime adds across the fan-out · paced control (100/s, 60B): the control: 300 tiny (~60B) deltas metronome-paced at 100/s — zero workload structure, so it reads the transport floor and flush cadence, and disambiguates transport-wide vs workload-specific when a replay row moves · size sweep (100/s, 160B-12KB): same pacing as the control with deltas padded in rotation across seven log-spaced sizes (~160B–12KB) — rotation decouples size from stream position, so it isolates whether chunk size causes latency · replay gateway-gpt-5.4-nano-2000t (1x): raw provider SSE cadence captured at the AI gateway boundary (gpt-5.4-nano, the most popular gateway model; per-token deltas p50 208B = the modal production chunk size), replayed exactly as measured — the typical customer's workload; its CDV is the typical customer's real delivery jitter · replay eve-gpt-5.6-sol-2000t (1x): a captured eve turn (gpt-5.6-sol, the most-used demanding eve model; ~2000 output tokens = production p50 turn length) replayed exactly as measured — eve's envelope protocol re-ships the cumulative message so sizes ramp 142B→13KB; the demanding outlier tenant's reality · replay eve-gpt-5.6-sol-2000t (2x): the same eve capture at 2x — the headroom/stress row; real fast-tier models emit the same chunk sizes at proportionally higher rate, so time compression is a faithful speed model · first chunk (pooled): every run's seq-0 RTT pooled across all stream scenarios — the first chunk precedes any workload differentiation, so pooling samples one shared stream-open path with exact percentiles Replay cadences (semantic sha256) — eve-gpt-5.6-sol-2000t 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 All timestamps are deployment-side; runs are triggered in-deployment, so the CI runner and api.vercel.com sit outside every measured window. TTFS = Cold starts stay in the numbers (real bursty-workload latency, inflates P75+); Best is the warm floor. |
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests▲ Vercel Production (8 failed)python-node (8 failed):
🌐 Cross-language Conformance (9 failed)python (9 failed):
🛠 Infra Events (absorbed by the harness)Platform anomalies the e2e harness detected and worked around (e.g. a run the queue never picked up, replaced by a fresh run). Clustered timestamps indicate a backend blip; a steady drip indicates a platform issue worth escalating. 33 infra events
E2E Test SummarySummary
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌐 Cross-language Conformance
✅ vercel-multi-region
|
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 41 total
Full trace: |
Event Log Race Repro
Run History
Config14 runs / step-storm 6, hook-storm 6, hook-sleep 2 / c8 / 6x8 / watchdog 2500ms / step 2200±250ms / stagger 400ms / poke 750ms / poke max 64 / timeout 240000ms |
… are The o11y UI now understands spec-7 noop seals, mirroring how it already treats replay duplicates: real rows of the log, shown greyed in every event list with the reason on hover, and excluded from everything that charts the run's behavior. - trace-builder keeps noops out of span grouping AND the latest-known-time bound: a noop's createdAt is the SEALER's wall clock, which can postdate every real event around it, and letting it in would stretch spans and the trace's known duration to the sealer's schedule. - Event lists (main table + sidebar) grey noop rows and explain them via the new EventNoticeTooltip, the generalized duplicate tooltip. - noop groups and labels with the run itself, like attr_set: its correlationId (noop_<position>) is positional, not a child entity. - Neutral gray palette + status dot, one step quieter than pending. Verified against a seeded world-local run in the real web UI: dimmed rows with tooltips in the Events tab, dense event ids straight through the seals, and a trace waterfall whose geometry ignores them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestion:
Module packages/web-shared/src/lib/sealed-events.ts is missing, but commit 214dd09 imports isSealedNoopEvent and SEALED_EVENT_MESSAGE from it in four places, causing a module-not-found build/typecheck failure.
…it imports 214dd09 referenced ../lib/sealed-events but the new file itself was never staged. Lands with the lightened tooltip copy: "No-op events may be added by Workflow SDK to ensure correctness" — the mechanism belongs in the event-sourcing docs, not a hover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Restores the three VERCEL_WORKFLOW_SERVER_URL expressions to main's secrets-based form, byte-identical to 37e1d9e. The pin existed so this PR's e2e exercised spec-7 sequencing + noop sealing against the workflow-server PR's preview; shipping the client against today's production server is safe — it treats spec-7 runs as slot identity (usesSlotIdentity is >= 6) and never writes a noop, so the client's new reading capability simply lies dormant until the server lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review sweep, getting this ready to land:
Ship-order note: this client is safe to land before the server-side sealed-log PR (vercel/workflow-server#805). Today's production server treats spec-7 runs as slot identity ( 🤖 Generated with Claude Code |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| * which can even postdate later slots' events; letting it touch the clock | ||
| * would leak the sealer's wall clock into replay.) | ||
| */ | ||
| private skipSealedNoop(event: Event) { |
There was a problem hiding this comment.
AI Review: Blocking
The QuickJS engine still advances the deterministic clock on a noop. This skip covers the node:vm engine only.
EventsConsumer is one of two replay paths. The node:vm engine feeds its clock from onConsumedEvent (workflow.ts:417), so bypassing offer() here correctly starves it. The QuickJS engine has its own loop over the raw event array:
// runtime/quickjs-runtime.ts:1802
for (const event of events) {
advanceClock(+event.createdAt); // <- before any filtering
const cid = event.correlationId;
if (!cid) continue;
switch (event.eventType) { /* no noop arm, no default */ }Two things make this bite rather than glance:
advanceClockisMath.max(:1391), so the seal's timestamp ratchets the VM clock forward permanently, and a seal'screatedAtis exactly the value the PR says "can even postdate events at higher positions".noopcarriescorrelationId = noop_<slot>, so theif (!cid) continueguard does not skip it.
Reproduced against this branch. Log = run_created@1, wait_created@2, noop@3 (sealed at 01:00:00Z), wait_completed@4 (00:00:11Z), against the workflow from the existing deterministic replay clock suite:
expected 1735693200000 not to be 1735693200000 // == +SEAL_AT
The noop-free twin returns endTime === +waitCompletedAt; the sealed log returns the seal time. So Date.now() inside the workflow jumps an hour, and the same log replays differently before and after the hole is sealed — the exact divergence skipSealedNoop exists to prevent.
The fix is a continue on noop ahead of advanceClock in processEvents. Worth pinning with a QuickJS twin of the 'never advances the deterministic clock off a noop' test added here — that test is what makes the invariant real on node:vm, and its absence on the other engine is why this got through.
| }); | ||
| }); | ||
|
|
||
| describe('sealed-log noop events', () => { |
There was a problem hiding this comment.
AI Review: Blocking
These tests never run. packages/world/package.json has no test script, so nothing executes this file:
$ npx turbo run test --filter=@workflow/world
No tasks were executed as part of this run.
Tasks: 0 successful, 0 total
Every other package in the change has one (world-vercel, world-local, world-postgres, core, web-shared all declare vitest run); @workflow/world is the only gap. That matters more here than it usually would: these three cases plus the spec-version.test.ts additions are the only automated check that the read union accepts what a sealing backend writes and that CreateEventSchema refuses it. The PR body's "156 ✓" was a local run.
Independent evidence the file has been dead for a while: spec-version.test.ts on main still asserts SPEC_VERSION_CURRENT === SPEC_VERSION_SUPPORTS_COMPRESSION (5) while spec-version.ts has said 6 since #3542. A failing assertion has been sitting on main unnoticed, which is only possible because nothing runs it.
Adding "test": "vitest run src" to packages/world/package.json turns all of this on. Please check the rest of the file passes when it does.
Separately, while these are the wire-contract tests: neither read-union case sets correlationId, so nothing asserts noop_<slot> survives parsing. The event-list grouping added in this PR reads event.correlationId, so it is worth asserting.
| @@ -72,7 +86,7 @@ export const SPEC_VERSION_SUPPORTS_SLOT_IDENTITY = 6 as SpecVersion; | |||
| * run's identity scheme from what is stored rather than from this constant. | |||
| */ | |||
| export const SPEC_VERSION_CURRENT = | |||
There was a problem hiding this comment.
AI Review: Blocking
The bump to 7 breaks the Python SDK, and that is what is reddening E2E Required Check — not a flake. All three failures share one root cause:
1 validation error for RunStartedEvent
specVersion
Input should be less than or equal to 6 [type=less_than_equal, input_value=7]
workbench/python resolves its runtime from the released vercel-workflow distribution, which bounds specVersion <= 6. Every run 500s at run_started and retries to exhaustion, so E2E Python Conformance and E2E Vercel Prod Tests (python - node) both fail and the aggregate reports:
PYTHON_STATUS: failure
VERCEL_STATUS: failure
The following E2E jobs did not succeed: e2e-vercel-prod (failure), e2e-python (failure)
Everything else is green, including all 30+ quickjs lanes.
So landing spec 7 has a release dependency outside this repo: a published Python runtime that accepts 7 has to go out first, or the bound has to be relaxed there. The PR body's "Cross-world coverage" section covers the JS Worlds but not this, and it is the one consumer that hard-fails rather than degrading.
| @@ -86,7 +100,7 @@ export const SPEC_VERSION_CURRENT = | |||
| * impossible to express. | |||
| */ | |||
| export const SPEC_VERSION_MAX_SUPPORTED = | |||
There was a problem hiding this comment.
AI Review: Note
Moving both dials together narrows the accepted World range to exactly {7}, because assertWorldSupportsRuntimeProtocol gates on declared >= SPEC_VERSION_CURRENT && declared <= SPEC_VERSION_MAX_SUPPORTED. A World package declaring 6 is now refused at startup with "Install a World package version compatible with the current Workflow runtime."
Worth making that a deliberate choice rather than one inherited from the constant, because the justification written above SPEC_VERSION_CURRENT no longer covers it:
a World declaring anything below this allocates event ids the runtime cannot read positions out of
That is an argument about slot identity, i.e. version 6. A spec-6 World numbers by slot, produces no noops, and this runtime reads it correctly — the sealed-log contract is a reader capability, not something the World has to provide. Pinning the floor at SPEC_VERSION_SUPPORTS_SLOT_IDENTITY and letting only CURRENT/MAX_SUPPORTED move would keep the actual hard requirement as the floor and stop rejecting Worlds the runtime can serve. Duplicate-@workflow/world-in-node_modules installs are the realistic way this surfaces, and it presents as a startup crash rather than anything diagnostic.
| The backend restores the dense log at read time by **sealing** such positions: once a hole is provably abandoned (bounded by the commit time of later positions — positions are handed out in order, so a committed later position proves how long the hole has been open), the backend writes a `noop` event into it. A `noop` occupies its position — length-based completeness checks, cursors, and pagination all count it — and means nothing: | ||
|
|
||
| - It is **never offered to any consumer** during replay. The walk steps over it in the same synchronous pass that delivers the events around it, so its presence cannot perturb delivery order, promise scheduling, or which branch of a `Promise.all` resumes first. | ||
| - It **never advances the deterministic clock**. A `noop`'s `createdAt` is the *sealer's* wall clock — it can even postdate events at higher positions — and letting it feed the replay clock would make a log containing a seal replay differently from one whose hole was filled by its original writer. Same rule, and same mechanism, as skipped duplicates above. |
There was a problem hiding this comment.
AI Review: Note
The invariants listed here hold for replay, but "means nothing" is not yet true of the latency telemetry, where a seal reads as run activity and silently suppresses a measurement. Two sites, both confirmed by running them:
TTFS and RSFS — runtime.ts:2927 allowlists three types:
invocationStartedClean ??= eventLog.events.every(
(e) => e.eventType === 'run_created' || e.eventType === 'run_started' || e.eventType === 'attr_set'
);A seal before the first step flips this to false, and RSFS shares TTFS's eligibility exactly, so both go unreported.
STSO — runtime/step-latency.ts:249 keys on the log's last event:
const lastEvent = events[events.length - 1];
if (... && (lastEvent.eventType === 'step_completed' || lastEvent.eventType === 'step_failed'))A trailing seal makes prevStepEndMs undefined. I reproduced this with a back-to-back step pair: measured at T0+500 without the seal, undefined with a seal appended.
Neither is a corner case — a seal at the tail is precisely what a fanout whose last writer died leaves behind. The consequence is that rolling spec 7 out quietly shrinks the TTFS/STSO sample population, which reads as a data change rather than a seal artifact when you are watching those numbers for regressions. noop should be admitted in both places for the same reason attr_set already is: it is not something the run did.
(Same class, lower stakes: runtime/quickjs-entrypoint.ts:104 isFirstInvocation uses the same two-type every, so a seal in the preload costs the first-invocation fast path a redundant fetch.)
| * Sealed-log filler event (specVersion >= 7). Written ONLY by the World's | ||
| * backend when it seals a slot whose writer allocated the position and died | ||
| * before committing (see `SPEC_VERSION_SUPPORTS_SEALED_LOG`). It occupies its | ||
| * slot — so density arithmetic and cursors count it — but carries no workflow |
There was a problem hiding this comment.
AI Review: Note
"density arithmetic and cursors count it" is right, and there is one more counter it lands in that is worth deciding about deliberately: the per-run event ceiling.
// core/src/runtime.ts:2909
if (maxEventsLimit !== undefined && eventLog.events.length >= maxEventsLimit) {
throw new MaxEventsExceededError(eventLog.events.length, maxEventsLimit);
}MAX_EVENTS_EXCEEDED is attributed to the customer, with a hint that names their code as the cause:
// core/src/describe-error.ts:137
if (errorCode === RUN_ERROR_CODES.MAX_EVENTS_EXCEEDED) {
return { attribution: 'user', errorCode, hint: MAX_EVENTS_HINT };
}
// "This usually means unbounded work in the workflow function ..."So backend-written rows can push a run over a limit whose failure blames the user. At the ~1% seal rate quoted in the design that is a rounding error, but the direction is wrong, and the runs that accumulate seals fastest are exactly the ones repeatedly losing writers. Either exclude seals from that count or, if they should count, say so where the ceiling is documented.
|
|
||
| #### Optional: pre-assigned positions and `noop` sealing | ||
|
|
||
| Spec version 7 legitimizes one alternative to allocate-at-commit, for Worlds whose store makes commit-time allocation a contention bottleneck: hand positions out from a per-run atomic counter **before** the commit, and restore density at read time. Pre-assignment means concurrent writers hold distinct positions and never race for one — but a writer that claims a position and dies leaves a permanent hole. A World that allocates this way MUST therefore **seal** provably abandoned positions by writing a `noop` event into them (racing the original writer at the same uniqueness fence — losing that race means the real event landed, which is success), and MUST NOT return a page with an interior hole: return the dense prefix below the hole and let the caller's next page pick up past it once the position resolves to an event or a seal. |
There was a problem hiding this comment.
AI Review: Note
This is the right place for the contract, and it is missing the half that the SDK enforces most harshly. The paragraph tells a pre-assigning World to "return the dense prefix below the hole and let the caller's next page pick up past it" but says nothing about what hasMore must be when that prefix is empty.
The runtime gives no latitude there. loadWorkflowRunEvents runs two guards per page (runtime/helpers.ts):
if (requestedCursors.has(cursor)) throw eventPaginationContractError(runId, 'repeated a cursor');
// and, on the request side:
if (requestedCursors.has(cursor)) throw eventPaginationContractError(runId, 'did not advance');Both raise WORLD_CONTRACT_ERROR, which is non-retryable and whose hint asks the user to file a bug with the runId. A World that truncates at a hole, makes no progress, and still reports hasMore: true with the cursor the caller just sent therefore fails the run outright — and "made no progress because the hole is still young" is the normal steady state under sustained fanout, not an edge.
Since this section is the contract a World author implements against, it should state the rule: a truncated page that returned nothing must report hasMore: false and let the caller come back, rather than re-offering the same cursor. Worth adding a world-testing conformance case too — the suite currently has nothing that exercises a sealed or truncated page, so a World author has no way to check either half of this new contract.
| function logEvent(eventType: Event['eventType'], id: string): Event { | ||
| return createMockEvent({ id, eventId: id, eventType } as Partial<Event>); | ||
| } | ||
|
|
There was a problem hiding this comment.
AI Review: Note
These are good tests and they are also, after d30f7e5a0d, close to the whole of the coverage. Unpinning e2e from the server preview is correct for merge, but it means nothing in CI now sees a noop end to end: world-local and world-postgres never seal, so the only paths that produce one are unit tests — and the schema half of those does not run at all (see my note on packages/world/src/events.test.ts).
That is the structural reason the QuickJS clock bug survived a fully green e2e matrix including every quickjs lane: no lane can produce the event that would expose it.
Two cheap ways to close it, either is enough:
- a
world-testingconformance case that stores a seal and replays across it, which runs against every World and both engines; or - keep the server-preview pin alive on a scratch branch and record the result here, so the sealed path is exercised at least once before this lands.
| * the run's. | ||
| */ | ||
| export const SEALED_EVENT_MESSAGE = | ||
| 'No-op events may be added by Workflow SDK to ensure correctness'; |
There was a problem hiding this comment.
AI Review: Note
This is the only user-visible string in the change, and it credits the wrong component: the SDK never writes a noop. Everything else in this PR is careful about exactly that distinction (event-retry.ts calls it "server-originated; never POSTed by the SDK", the schema comment says "written ONLY by the World's backend"), so the one sentence a customer actually reads contradicts all of it.
Something like "Added by the workflow backend to seal an abandoned log position" keeps the actor right and tells the reader what they are looking at. The current wording also invites the support question it is meant to prevent — "why is the SDK adding events to my run?".
| duplicateEventIds.size === 0 | ||
| ? events | ||
| : events.filter((event) => !duplicateEventIds.has(event.eventId)); | ||
| const actedOnEvents = events.filter( |
There was a problem hiding this comment.
AI Review: Nit
This drops the no-copy fast path: the previous form returned events unchanged when there were no duplicates, and now every buildTrace allocates a filtered array. buildTrace runs on the trace tab for runs whose logs can reach tens of thousands of events, so it is worth keeping the early out — e.g. compute both predicates first and only filter when either matched.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found (follow-up pass, cross-repo and second-order consumers)
| this.drainParked(); | ||
| const currentEvent = this.events[this.eventIndex] ?? null; | ||
| if (currentEvent !== null && currentEvent.eventType === 'noop') { | ||
| this.skipSealedNoop(currentEvent); |
There was a problem hiding this comment.
AI Review: Blocking
Second issue in this file, from the other direction: handleEndOfLog decides whether a parked event is orphaned by looking at the log's last row, and a trailing seal defeats it.
// :598
const last = this.events.at(-1);
if (!last || !TERMINAL_EVENT_TYPES.has(last.eventType)) {
// "A later replay is still expected" -> debug log, returnTERMINAL_EVENT_TYPES is {run_completed, run_failed, run_cancelled}. With a noop as the last element, a parked event on an already terminated run stops reaching scheduleUnconsumedCheck(..., false) and is downgraded to a debug line. That swaps a loud ReplayDivergenceError for a silent stall — the run just stops, holding an event no consumer will ever come for.
The "a seal always has a committed position above it" argument does not rescue this, because this.events is a loaded prefix, not the whole log. A page truncated below a young hole ends on whatever sits under that hole, and if that position was itself sealed earlier, at(-1) is a noop. Incremental appends can land the same way.
The shape was clearly on your mind — events-consumer.test.ts adds 'handles a log that ends on a noop' — but this reader of at(-1) was not audited alongside it. findLast((e) => e.eventType !== 'noop') fixes it, and that test is one assertion away from covering it.
| // attr_set and noop carry a dedup/positional correlationId rather than a | ||
| // child entity ID, so they group and label with the run itself. | ||
| eventType === 'attr_set' || | ||
| eventType === 'noop' |
There was a problem hiding this comment.
AI Review: Note
The grouping change is right, but one thing further down this file undoes the effect the PR description shows in its screenshot. sortedEvents orders rows by timestamp, not by position:
// :1344
return [...sourceEvents].sort(
(a, b) => dir * (getEffectiveEventTime(a) - getEffectiveEventTime(b))
);getEffectiveEventTime is createdAt/occurredAt. A seal's createdAt is the sealer's wall clock, which this PR correctly says "can postdate every real event around it" — so a seal row sorts to wherever its clock lands, not to its slot. The visible event-ID column then reads non-monotonically, which is the opposite of the "event IDs stay dense straight through them (1–14, no gaps)" claim in the description. That screenshot holds only because the seed's seal timestamps happened to fall between their neighbours.
trace-builder.ts got the corresponding treatment (seals excluded from geometry and time bounds) but this sort was left alone. Since a slot id is the log order written down, sorting by eventId when the run is slot-numbered would be both correct and cheaper than parsing dates.
|
|
||
| A sealed position races its original writer at the same uniqueness fence as every other write, and losing that race is the good outcome: the real event landed first, and readers get it instead. A live writer that gets sealed over simply re-derives a fresh position and commits there — the same recovery as losing any other write race — so sealing can cost a retry, never a wrong log. | ||
|
|
||
| `noop` is not user-creatable: it does not exist in the create schemas, and backends reject it on every create endpoint. Only a backend's own read path writes one. |
There was a problem hiding this comment.
AI Review: Note
"backends reject it on every create endpoint" is stronger than what ships. CreateEventSchema is never .parse()d anywhere in the repo — it is consumed only as a type, via AnyEventRequest, which is what events.create takes in world-local, world-postgres, world-vercel and world-sim. So the exclusion is a compile-time guarantee for SDK callers, not a runtime rejection by the World.
This PR's own tests demonstrate it: both world-postgres/test/spec.test.ts and world-local/src/storage/slot-identity.test.ts create a noop through world.events.create(runId, {eventType: 'noop', ...} as any) and assert it succeeds. That is deliberate (they are testing tolerance), and it is also the counterexample to the sentence above.
Two ways to make them agree: narrow the claim to the backends that validate on the wire, or add a runtime guard on the create path so the sentence is enforceable. The second is worth considering on its own merits — a noop accepted from a caller burns a slot the caller never allocated, which is exactly the failure the sealer exists to repair.
| * would fail to parse the unknown event type, which is exactly what | ||
| * `requiresNewerWorld` exists to catch. | ||
| */ | ||
| export const SPEC_VERSION_SUPPORTS_SEALED_LOG = 7 as SpecVersion; |
There was a problem hiding this comment.
AI Review: Note
world-sim now declares this version while modelling the thing it names incorrectly, so the simulator's guarantees quietly stop applying to spec-7 logs. It stamps SPEC_VERSION_CURRENT (store.ts:705, world.ts:732), and then:
store.ts:1310—allEventssortsa.createdAt - b.createdAt || a.eventId.localeCompare(b.eventId). The real Worlds sort by position (world-postgres/src/storage.ts.orderBy(Schema.events.eventId);world-local/src/storage/events-storage.ts:321comments "log order for a slot-numbered run is slot order, notcreatedAtorder"). The sim is the outlier, so a postdating seal lands at the wrong log position there and nowhere else.invariants.ts:68—log.monotonic-orderasserts commit order equals(createdAt, eventId)order. A seal commits with a lower eventId and a latercreatedAtthan events already committed, violating it in both dimensions. That is an invariant spec 7 retires, still asserted.replay.ts:122— anchors the virtual clock onterminal.createdAt, which a postdating seal can exceed.
None of this fires today, because writers.ts/scenario.ts/tempo.ts have no seal generator. That is the point worth deciding: either the sim should not declare 7, or it needs a seal generator plus the ordering fix, so the invariant it enforces is the one the spec actually promises.
| ### System Events | ||
|
|
||
| | Event | Description | | ||
| |-------|-------------| |
There was a problem hiding this comment.
AI Review: Nit
Good addition. Its twin was missed: docs/content/docs/v5/api-reference/workflow-runtime/world/storage.mdx:123-130 has an "Event Types" table with the same Run/Attribute/Step/Hook/Wait breakdown and no System row. That page documents events.list returning Event[], so a World author reads a closed set there that no longer matches EventTypeSchema.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| * not advance, so a log that happens to contain one produces the same | ||
| * timestamps as a log that does not. (Its `createdAt` is the seal time, | ||
| * which can even postdate later slots' events; letting it touch the clock | ||
| * would leak the sealer's wall clock into replay.) |
There was a problem hiding this comment.
AI Review: Blocking
The clock rule this docstring establishes is implemented for the node:vm engine only. The QuickJS engine has its own event loop — processEvents in packages/core/src/runtime/quickjs-runtime.ts — and at line 1808 it calls:
advanceClock(+event.createdAt);for every row in the log, before the if (!cid) continue guard and before the switch (event.eventType). A noop carries a correlationId (noop_<position>) so it passes the guard, matches no case, and is otherwise inert — but it has already moved the clock. grep -c noop packages/core/src/runtime/quickjs-runtime.ts is 0 on this head.
Reproduced with the existing runQuickJSWorkflow harness: two logs for the same run, identical except position 3 is a writer-filled attr_set (in-order clock) vs a noop sealed an hour later, with the sleep's wait_completed above it.
- noopSealed: "2025-01-01T00:00:11.000Z" (expected)
+ noopSealed: "2025-01-01T01:00:00.000Z" (actual)
writerFilled: "2025-01-01T00:00:11.000Z"
Date.now() after the wait jumps to seal time, and because the replay clock is monotonic every subsequent read in the run is poisoned. Concretely that means: the same log replays with different timestamps on the two engines, and a replay taken before the seal (log truncated at the hole) disagrees with one taken after. Both engines ship and both run on every E2E matrix row.
Worth noting the same reasoning is already applied correctly one layer up — packages/web-shared/src/lib/sealed-events.ts excludes noops from span geometry and latestKnownTime for exactly this reason. The replay clock needs the same treatment. The events-consumer.test.ts and step-delivery-ordering.test.ts coverage added here is good but is node:vm-only, so it cannot catch this; a QuickJS case belongs alongside the deterministic replay clock suite in quickjs-runtime.test.ts.
| */ | ||
| export const SPEC_VERSION_CURRENT = | ||
| SPEC_VERSION_SUPPORTS_SLOT_IDENTITY as SpecVersion; | ||
| SPEC_VERSION_SUPPORTS_SEALED_LOG as SpecVersion; |
There was a problem hiding this comment.
AI Review: Blocking
Moving SPEC_VERSION_CURRENT to 7 breaks the Python runtime outright. This is not speculation — it is this PR's own CI on the current head (run 32316424719):
1 validation error for RunStartedEvent
specVersion
Input should be less than or equal to 6 [type=less_than_equal, input_value=7, input_type=int]
8,444 occurrences in the E2E Python Conformance log. Every event the Python handler receives 500s, the queue retries it to exhaustion, and every run times out. E2E Python Conformance and E2E Vercel Prod Tests (python - node) both fail on Run E2E Tests, which is what is holding E2E Required Check red — so these are real failures caused by this diff, not the usual Python-lane flakes.
The ceiling lives in the external vercel-workflow distribution that workbench/python/pyproject.toml pulls in via the vercel umbrella (vercel-workflow>=0.9.0,<1.1.0), so it cannot be fixed in this repo. This needs a Python runtime release that both raises the accepted specVersion to 7 and skips noop on replay (same skip-and-do-not-advance-the-clock contract as EventsConsumer), landed and pinned here, before the constant can move.
Given SPEC_VERSION_CURRENT is what every World stamps, it is also worth asking whether the bump wants a gate of its own for the rollout — the plan's §6 shape was "client spec bump after, behind its own gate", and as written there is no client-side way to back out of spec 7 short of publishing a new SDK.
| * without advancing the deterministic clock. NOT user-creatable, and absent | ||
| * from `CreateEventSchema` for that reason. | ||
| */ | ||
| const NoopEventSchema = BaseEventSchema.extend({ |
There was a problem hiding this comment.
AI Review: Note
A noop occupies a slot, so it counts toward the client-side event ceiling: packages/core/src/runtime.ts fails a run on eventLog.events.length >= maxEventsLimit, and that length includes noops. Small in practice (seals are a low single-digit percentage of positions), but it is a backend-written filler spending the user's event budget, and the design notes elsewhere say noops are excluded from the event-count guard. Either exclude them from that comparison or state that they deliberately count.
Same file, smaller: invocationStartedClean in runtime.ts is computed with events.every(e => e.eventType === 'run_created' | 'run_started' | 'attr_set'), so a noop anywhere in the log silently disqualifies the run from TTFS telemetry. Metrics-only, but it means sealed runs quietly drop out of the latency dataset — which is the dataset you'd want them in.
| export const SEALED_EVENT_MESSAGE = | ||
| 'No-op events may be added by Workflow SDK to ensure correctness'; | ||
|
|
||
| /** Whether `event` is a backend-written seal for an abandoned position. */ |
There was a problem hiding this comment.
AI Review: Note
This and the trace-builder change are the right treatment, and the clock rationale in the docstring is exactly right.
One consumer-side gap: the dashboard pins @workflow/web-shared at a fixed version and also keeps its own local EventType union for the events it renders, which has no noop member. So until that consumer bumps to a release containing this commit and adds noop to its union, noops reach the run-details view through the un-updated path and render unlabelled rather than greyed-with-tooltip. Worth tracking as an explicit follow-up so the UI half actually lands with the protocol half.
Also: SEALED_EVENT_MESSAGE says "added by Workflow SDK", but the SDK never writes one — the World's backend does. Users reading that tooltip will look in the wrong place.
| * reader at this version knows a `noop` occupies its slot and carries no | ||
| * workflow meaning, and skips it during replay without advancing the | ||
| * deterministic clock (see `EventsConsumer`). A reader below this version | ||
| * would fail to parse the unknown event type, which is exactly what |
There was a problem hiding this comment.
AI Review: Note
SPEC_VERSION_CURRENT is what every World stamps, so this bump puts spec 7 on world-local, world-postgres and every community World too, not just world-vercel. The building-a-world.mdx addition covers the contract and the reasoning is sound (a World that allocates positions at commit time never leaves a hole, so it never needs to seal), but the docstring here reads as though the version is about the Vercel backend specifically. Worth one sentence saying commit-time-allocating Worlds are spec-7 compliant by construction, since that is the thing a World author will want to check first.
A `noop` occupies a position whose writer allocated it and died; the run never observed it, and its `createdAt` is the SEALER's wall clock, which can postdate every real event around it. The node:vm engine skips it in `EventsConsumer` before `onConsumedEvent` feeds the clock. The QuickJS engine's own event loop did not: `processEvents` advanced the clock from every row before it looked at the event type, so a sealed log replayed with a different `Date.now()` than the same log whose hole its own writer filled -- and than itself on the other engine. Monotonic clock, so every later read in the run inherited the skew. - `isSealedNoopEvent` moves to `@workflow/world`, the one place both engines and the trace viewer now test through. Three independent copies of "is this a seal" is what let the engines drift in the first place. - Sealed positions stop counting toward the event ceiling: the limit exists to stop a runaway workflow, and charging the user's budget for the backend's bookkeeping made the ceiling a run hits depend on how much write contention it happened to see. - A seal no longer disqualifies a run from TTFS telemetry, which had been dropping exactly the contended runs worth measuring. Regression test sits with the existing deterministic-replay-clock suite, with the hole between wait_created and wait_completed -- where a fanout actually leaves one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(AI) Pushed Fixed — QuickJS replay clock (was blocking). Fixed — the two accounting notes. Sealed positions no longer count toward the event ceiling (the limit exists to stop a runaway workflow; charging the user's budget for backend bookkeeping made the ceiling a run hits depend on how much write contention it saw), and no longer flip Fixed — wording and docs. Verification: Not fixed — the Python spec ceiling. This one cannot be closed from this repo. The ceiling is a Pydantic bound in the external So |
Moving SPEC_VERSION_CURRENT to 7 stamped spec 7 on every run every World created, with no way back short of publishing another SDK. That took the Python runtime down outright -- it pins its own accepted range and rejects 7, so every event 500s and every run times out (8,444 validation errors in one E2E Python Conformance run, which is what held E2E Required Check red). The fix is the shape slot identity itself shipped behind before going unconditional: SPEC_VERSION_CURRENT stays the floor, `mintedSpecVersion()` answers what a World actually stamps, and the three Worlds declare that instead of the constant so they keep moving together. Opt-IN rather than opt-out, because stamping a version is not a local decision -- it changes what every other reader of those runs has to understand, including readers that do not ship on this release train. A spec-7 log may hold `noop` rows and a reader that does not skip them cannot replay it at all. Default-off makes the rollout a deployment setting instead of a publish. SPEC_VERSION_MAX_SUPPORTED stays at 7: every build READS sealed logs and skips noops regardless of the flag, which is what keeps the two dials honest -- and puts them back in the staged relationship they document, ceiling ahead of floor. That also restores the start.test.ts case the bump had to weaken when the two collapsed onto the same number. Runs are unaffected either way: a run's version is stamped once at creation and read from the run for life, so flipping this changes only what new runs get. Docs: WORKFLOW_SEALED_LOG in runtime-tuning, plus the gate noted in event-sourcing and building-a-world, where spec 7 read as unconditional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client half of the sealed-log (Corfu-style) design.
Server half: vercel/workflow-server#805 (design doc
SEALED-LOG-PLAN.mdthere).The server side optimization linked above allows event creation to post sparse event IDs (we no longer have to block on claiming and filling a monotonically increasing event log). However, at read time, a sparse event log is still invalid, so readers will wait for empty sparse slots to fill in, before eventually considering them abandoned and just writing a "noop" event instead - thereby making the event log retain it's dense ordering nature without writers bottlenecked on a single queue to ensure strict ordering.
So this client side PR adds the noop event type to the runtime as a pre-requisite for the server side optimization.
Design of the
noopeventeventTypenoopeventIdevnt_<26-digit slot>) - it occupies a real slot, so length-based completeness checks, cursors, and pagination all count itcorrelationIdnoop_<26-digit slot>- a pure function of the position (mirrorsslotToEventId), so any two sealers racing for one hole mint the identical row; deterministic at process time, and the 4-char prefix matches thewrun_/step_/evnt_id conventioneventData{ sealed: true }, inline, no payload refscreatedAtCreateEventSchema, never POSTed by the SDK, rejected 400 by the server if anything triesThe "advances no clock" is an important design nit - noop event should truly be noop and not affect the deterministic clock in the VM used to calculate
Date.now. In other words, there should be no difference in the outcome of replaying an event log, vs. repalying the same event log with an extra NOOP at the end. To this end, noop events should also not affect microtask ordering and replaying issues, which is soemthing I steered in this PR but is worth validating in code reviews.Docs
docs/content/docs/v5/how-it-works/event-sourcing.mdx: new System Events table entry + a Sealed Positions (noop events) section (why holes exist, skip semantics, clock rule, correlation-id design, why losing the seal race is the good outcome).docs/content/worlds/v5/building-a-world.mdx: the World-author contract gains Optional: pre-assigned positions andnoopsealing — spec 7's amendment to the allocate-at-the-commit rule, what a pre-assigning World MUST do (seal, never return interior holes), and why commit-time-allocating Worlds never need it.Observability rendering
Captured from a real dashboard serving a seeded world-local run with seals at positions 6 and 12:
Events tab — seal rows render dimmed with the deterministic
noop_<position>correlation ID, and event IDs stay dense straight through them (1–14, no gaps). Seals group and label with the run itself, likeattr_set, since their correlation ID is positional rather than a child entity:Hover — the same tooltip pattern duplicates use:
Trace waterfall — no span or marker for either seal, and the trace's known duration is bounded by the last real event: a seal's
createdAtis the sealer's wall clock, which can postdate every event around it, so letting it into the geometry would stretch spans to the sealer's schedule (pinned by a unit test intrace-builder.test.ts):"Full AI details below"
What spec 7 means for the SDK
On the server, a spec-7 run's slot positions come from a per-run atomic counter instead of writers racing conditional creates — concurrent writers hold distinct positions, so fanout writes proceed fully parallel with no retries and no queueing. The price: a writer that allocates a position and dies leaves a hole, and the server's read path fills provably-abandoned holes with server-written
noopevents so returned pages stay dense prefixes. The client contract change is exactly that one thing: a reader must know anoopoccupies its slot and means nothing.Design of the
noopeventeventTypenoopeventIdevnt_<26-digit slot>) — it occupies a real slot, so length-based completeness checks, cursors, and pagination all count itcorrelationIdnoop_<26-digit slot>— a pure function of the position (mirrorsslotToEventId), so any two sealers racing for one hole mint the identical row; deterministic at process time, and the 4-char prefix matches thewrun_/step_/evnt_id conventioneventData{ sealed: true }, inline, no payload refscreatedAtCreateEventSchema, never POSTed by the SDK, rejected 400 by the server if anything triesScheduling determinism (the microtask question)
The skip is a synchronous
continueinsideEventsConsumer's walk pass — noawait, nonextTick, noqueueMicrotask. So a noop doesn't even deterministically cost a tick: the events around it are offered in the same order, inside the same synchronous pass, on the same single post-subscribetick, as a log without it. Same log → same schedule on every replay, and additionally noop-bearing ≡ noop-free for scheduling purposes. This is pinned by tests, not asserted:events-consumer.test.ts: a log with noops at head, middle (consecutive), and tail finishes after the same single tick with a byte-identical offer sequence to its noop-free twin; plus all-noop logs, trailing noops, and a noop whosecreatedAtis 60s in the future never reachingonConsumedEvent(the clock feed).step-delivery-ordering.test.ts: noops injected into the exact microtask-hop-sensitive scenario from the production ordering incident this file exists for — including into thewait_completed/step_completedadjacency where branch-resume order decides ULID draws — replay resolves branch order, correlation-id assignment, and suspension state identically, on first replays and on payload-cache-sharing later replays (the hop-count-asymmetric case).Why the clock rule matters: a noop's
createdAtis seal time, which can postdate events at higher positions. Letting it advance the replay clock would make a sealed log replay differently from one whose hole was filled by its writer. Same rule, same mechanism, as the existingskipDuplicateEvent.Cross-world coverage
Only the vercel backend seals, but spec-7 logs are legal residents of every World's storage:
noopentries in the exhaustive retry-policy and v4 response-schema maps; page parsing accepts noops via the sharedEventSchema; the server-side integration suite (workflow-server#805) covers the full protocol (fanout density, sealing, truncation cursors, counter repair, create rejection).EventsConsumerand full-replay tests above are world-independent.Docs
docs/content/docs/v5/how-it-works/event-sourcing.mdx: new System Events table entry + a Sealed Positions (noop events) section (why holes exist, skip semantics, clock rule, correlation-id design, why losing the seal race is the good outcome).docs/content/worlds/v5/building-a-world.mdx: the World-author contract gains Optional: pre-assigned positions andnoopsealing — spec 7's amendment to the allocate-at-the-commit rule, what a pre-assigning World MUST do (seal, never return interior holes), and why commit-time-allocating Worlds never need it.Spec constants
SPEC_VERSION_SUPPORTS_SEALED_LOG = 7;SPEC_VERSION_CURRENTandSPEC_VERSION_MAX_SUPPORTEDmove to it together. An old server receiving spec 7 already treats it as slot identity (open-ended>=checks), so this client degrades gracefully against un-upgraded servers; old clients never see noops because only spec-7 runs contain them..github/workflows/tests.ymlhardcodesVERCEL_WORKFLOW_SERVER_URL: https://workflow-server-git-pgp-sealed-log-sequencer.vercel.sh(3 e2e jobs, each markedTODO(sealed-log)) so this PR's e2e suite exercises spec-7 sequencing + sealing against the actual server implementation. Restore thesecrets.VERCEL_WORKFLOW_SERVER_URLexpression before landing.Testing
@workflow/world: 156 ✓ (noop parses in the read union with/without eventData; rejected byCreateEventSchema; spec constants)@workflow/core: full suite 2,186 ✓ (incl. 50 consumer tests + 9 delivery-ordering tests with the new determinism coverage)@workflow/world-vercel: 528 ✓@workflow/world-local: slot-identity suite 14 ✓ (incl. noop round-trip)@workflow/world-postgres: noop round-trip ✓ against testcontainer@workflow/world,@workflow/world-vercel,@workflow/coreminor)Observability rendering
The dashboard (
@workflow/web/web-shared) treatsnoopseals the way it already treats replay duplicates: real rows of the log, greyed in every event list with the reason on hover, and excluded from everything that charts the run's behavior. Captured from a real dashboard serving a seeded world-local run with seals at positions 6 and 12:Events tab — seal rows render dimmed with the deterministic
noop_<position>correlation ID, and event IDs stay dense straight through them (1–14, no gaps). Seals group and label with the run itself, likeattr_set, since their correlation ID is positional rather than a child entity:Hover — the same tooltip pattern duplicates use:
Trace waterfall — no span or marker for either seal, and the trace's known duration is bounded by the last real event: a seal's
createdAtis the sealer's wall clock, which can postdate every event around it, so letting it into the geometry would stretch spans to the sealer's schedule (pinned by a unit test intrace-builder.test.ts):Screenshots hosted on the
pgp/noop-o11y-shotsassets branch — delete it after this PR lands.🤖 Generated with Claude Code