[core] Add atomic start Hook admission - #3426
Conversation
🦋 Changeset detectedLatest commit: 91d04bb 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✅ All tests passed E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 183209ms → this run 167452ms (Δ -15757ms, -9%) 📜 Previous results (7)45239c8Tue, 11 Aug 2026 21:59:50 GMT · run logs
adf58ebTue, 11 Aug 2026 05:11:23 GMT · run logs
7825269Tue, 11 Aug 2026 00:39:16 GMT · run logs
f9614bfMon, 10 Aug 2026 23:58:39 GMT · run logs
77ce876Mon, 10 Aug 2026 23:17:12 GMT · run logs
06bd237Mon, 10 Aug 2026 21:45:13 GMT · run logs
3da6597Mon, 10 Aug 2026 21:05:53 GMT · run logs
ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
NathanColosimo
changed the title
5ea20bb to
3da6597
Compare
06bd237 to
176b429
Compare
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Sim WorldSimulated world deterministic testing for races. Traces 🟠 Mint-ordered log — 6 fail of 41 total
Full trace: 🟢 Append-only log — 0 fail of 41 total
Full trace: |
| if ( | ||
| (err instanceof WorkflowWorldError || | ||
| WorkflowWorldError.is(err)) && | ||
| !isRetryableWorldError(err) |
There was a problem hiding this comment.
Should this not check for the specific error? This seems like any non-retriable error will surface as a hook admission rejected
There was a problem hiding this comment.
AI: Blocking The new runInput?.startHook !== undefined block sits before the EntityConflictError/RunExpiredError check and before the getWorkflowSetupErrorCode → recordFatalRunError path. EntityConflictError, RunExpiredError, and PreconditionFailedError all extend WorkflowWorldError and all classify as non-retryable, so the prelude shadows every one of them.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
Human pass LGTM. Agent pass coming in a few minutes probably
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| return; | ||
| } | ||
| } catch (err) { | ||
| if (runInput?.startHook !== undefined) { |
There was a problem hiding this comment.
AI Review: Blocking
This prelude sits before the EntityConflictError/RunExpiredError check and before the getWorkflowSetupErrorCode -> recordFatalRunError path. EntityConflictError, RunExpiredError, and PreconditionFailedError all extend WorkflowWorldError and all classify as non-retryable, so err instanceof WorkflowWorldError && !isRetryableWorldError(err) shadows every one of them.
For a world-contract error the pre-existing path writes run_failed; the atomic path logs at ERROR and acks the message. The run stays in pending forever with no terminal event and no retry.
I confirmed this against the driveTurbo harness in this PR (scratch test, not committed):
non-atomic run + WorkflowWorldError{code: WORLD_CONTRACT_ERROR} -> 1 run_failed
atomic-start run + the identical error -> 0 run_failed
A second scratch test showed RunExpiredError in the atomic path now logs error: "Atomic start Hook admission rejected queued candidate" and never logs the pre-existing info: "Run already finished during setup, skipping" — same outcome, wrong severity, misleading message.
The second branch is strictly worse than falling through for contract errors, since the fall-through already stops the retry loop and records the failure. Narrowing the swallow to HookConflictError.is(err) alone fixes the case that matters. If you also want to stop queue retries for non-contract, non-retryable world errors, route those through recordFatalRunError rather than a bare return.
There was a problem hiding this comment.
AI Review: Note
Resolved by 91d04bb. Narrowing to WorkflowWorldError.is(err) && err.code === START_HOOK_ADMISSION_REJECTED fixes it, and using the name-exact .is() rather than instanceof is the right call: subclasses no longer match, so the terminal-run families reach their own branch.
Verified with the same paired scratch tests, run against both heads (45239c8 pre-fix, 91d04bb post-fix):
45239c8 (pre) 91d04bb (post)
EntityConflictError rejectedLog=true skipLog=false rejectedLog=false skipLog=true
RunExpiredError rejectedLog=true skipLog=false rejectedLog=false skipLog=true
PreconditionFailedError acked 204, no rethrow, no event rethrown (queue retries)
All three assertions fail on the pre-fix head and pass here, so the fix is load-bearing rather than incidental. PreconditionFailedError was the clearest strand: previously acked with neither a terminal event nor a retry.
The two tests added alongside the fix cover the tagged-rejection and unrelated-contract-error cases. Neither covers the terminal-run families, which are what the original prelude actually shadowed. A parametrized case over EntityConflictError/RunExpiredError/PreconditionFailedError asserting the INFO path would keep a future re-widening of this condition from going unnoticed. Note that runtimeLogger.info only reaches a sink when DEBUG matches workflow:runtime:info, so such a test has to set it.
| // No probe channel to the target — cannot attest the consumer honors | ||
| // `hookInput`, so leave the marker off (fail closed to sequential). | ||
| targetHookResumeInputVersion = undefined; | ||
| } else { |
There was a problem hiding this comment.
AI Review: Blocking
The else if (typeof world.streams?.get !== 'function') branch and its comment were deleted here. This fires for every cross-deployment start, with or without hook.
A world with no stream channel now enters healthCheck(), which enqueues a health-check message nobody can answer, then loops world.streams.get(...) -> TypeError -> sleep HEALTH_CHECK_POLL_INTERVAL, 20 times, for the full CROSS_DEPLOYMENT_CAPABILITY_PROBE_TIMEOUT_MS of 2000ms, before falling back to exactly the values the deleted branch set synchronously.
Measured on start.test.ts, same file, base vs head: 34ms -> 8.15s total, with four pre-existing tests each at ~2020ms:
should pass explicit deploymentId from opts to getEncryptionKeyForRun 2022ms
should resolve "latest" to the actual deployment ID via resolveLatestDeploymentId 2015ms
should pass the resolved deployment ID to getEncryptionKeyForRun when using "latest" 2017ms
should not call resolveLatestDeploymentId when a normal deploymentId is provided 2019ms
The guard was added deliberately in 303b6da (#1853). Nothing in the PR body explains removing it, so this reads as collateral from the refactor.
There was a problem hiding this comment.
AI Review: Blocking
Still open on 91d04bb. packages/core/src/runtime/start.ts is unchanged between 45239c8 and this head, and the else if (typeof world.streams?.get !== 'function') branch is still absent while it is present on the PR base (codex/atomic-start-capabilities, 459e34b, line 357).
Re-measured start.test.ts on both, same install and same build:
459e34b (PR base) 58 tests 36ms of test time
91d04bb (this head) 72 tests 8.16s of test time
with the same four pre-existing tests pinned at the probe timeout:
✓ should pass explicit deploymentId from opts to getEncryptionKeyForRun 2021ms
✓ should resolve "latest" to the actual deployment ID via resolveLatestDeploymentId 2023ms
✓ should pass the resolved deployment ID to getEncryptionKeyForRun when using "latest" 2029ms
✓ should not call resolveLatestDeploymentId when a normal deploymentId is provided 2029ms
None of those tests involve hook. Every cross-deployment start on a World without a stream channel now pays 2s and one undeliverable queue write to arrive at the values the deleted branch set synchronously. The suite passes, so CI will not catch it.
| specVersion: _specVersion, | ||
| ...data | ||
| } = runInput; | ||
| runCreationData = data; |
There was a problem hiding this comment.
AI Review: Note
Behavior change that applies to every run, not just atomic-start ones.
Replacing the explicit six-field pick with {...runInput} minus environment/specVersion means run_started now also carries encryptionPublicKey on the resilient-start path. Verified empirically by porting the new assertion on line 2348 back to the base branch:
base: expected undefined to be 'test-public-key'
head: passes
This looks like a fix, and it matches what the (now-deleted) RunStartedEventSchema comment described: on the resilient path the run is created from this event, and without the key it silently loses the ability to receive sealed writes. But it ships unannounced — the changeset doesn't mention it, and the only coverage is an added assertion inside a test named for turbo optimistic start. Worth its own changeset line and a test that names the behavior, so a future refactor doesn't drop it again.
The spread also means any field added to RunInput later is auto-forwarded into run_started rather than opted in. That's the mechanism that just quietly changed the payload here.
| deploymentId: z.string(), | ||
| workflowName: z.string(), | ||
| export const RunInputSchema = RunCreationDataSchema.extend({ | ||
| specVersion: z.number(), |
There was a problem hiding this comment.
AI Review: Note
RunInputSchema moving from a standalone object to RunCreationDataSchema.extend(...) changes input from optional to required at the type level. z.unknown() infers input?: unknown; SerializedDataSchema infers input: unknown.
Verified both halves:
- Type: constructing a
RunInputwithoutinputnow fails withTS2741: Property 'input' is missing. - Runtime:
RunInputSchema.safeParse({deploymentId, workflowName, specVersion})still succeeds, so no parse behavior changed.
RunInputSchema and RunInput are public exports of @workflow/world, so this is a source-breaking type change for external consumers (community worlds, test fixtures) riding a minor bump. Either restore optionality with .partial({ input: true })-style handling, or call it out in the changeset.
| }).catch(() => undefined); | ||
| if ( | ||
| startHook && | ||
| probe?.capabilities?.atomicStartHook?.active !== true |
There was a problem hiding this comment.
AI Review: Note
healthCheck(...).catch(() => undefined) collapses a network blip, a slow target, and a genuinely old deployment into the same probe === undefined, which then throws WORLD_CONTRACT_ERROR: "The target deployment does not support atomic start Hooks."
A transient probe failure is exactly the uncertainty WorkflowStartError was added for. Classifying it as a deterministic contract error tells the caller not to retry something that is retryable, and the message points at a capability gap that may not exist. Worth distinguishing "probe returned and said no" from "probe did not return".
| meta.executionContext = input.executionContext; | ||
| } | ||
| if (input.attributes !== undefined) meta.attributes = input.attributes; | ||
| if (input.startHook !== undefined) meta.startHook = input.startHook; |
There was a problem hiding this comment.
AI Review: Note
The v4 meta contract is two-sided: a field only survives if the receiving parser also knows it, and unknown meta keys are dropped silently rather than rejected. I checked the receiving side and it has no startHook handling today.
Inert on merge because no world advertises atomicStartHook, but the world-enabling PR has to land both halves together or admission data vanishes on the wire with no error. Worth a note in the follow-up PR description so it isn't discovered at E2E time.
| (error as Error & { cause?: unknown }).cause = value.cause; | ||
| } | ||
| return error; | ||
| return makeWebError(value.name, value); |
There was a problem hiding this comment.
AI Review: Note
Unrelated to startHook, and it changes every hydrated error in the o11y UI, not just the new one.
The old base Error reviver assigned error.cause = value.cause after construction; makeWebError passes {cause} to the constructor instead. InstallErrorCause defines cause as non-enumerable, a plain assignment defines it as enumerable:
old: {"writable":true,"enumerable":true,"configurable":true} Object.keys -> ['name','cause']
new: {"writable":true,"enumerable":false,"configurable":true} Object.keys -> ['name']
Anything that enumerates own properties (spread, Object.keys, JSON.stringify) stops seeing cause. This is arguably a consistency fix, since FatalError and the other revivers already used the constructor form. Flagging it because it's a silent semantic change in a shared hydration path and nothing in the PR calls it out.
| ); | ||
| } | ||
| // Pin the run to the VM engine selected when it starts. | ||
| const workflowVm = getWorkflowVmFromEnv(); |
There was a problem hiding this comment.
AI Review: Note
Two ordering changes here apply to all starts, not just atomic ones. Both look like improvements; noting them because neither is mentioned and both change observable behavior:
- Attribute/lineage/
replayedFromRunId/WORKFLOW_VMvalidation now runs beforeworld.getDeploymentId(). When both would fail, the validation error now wins where the deployment lookup used to. safeWaitUntil(Promise.all(ops), ...)moved ahead of admission, so the stream flush is registered even if the code between the old and new positions throws.
| attributes: z.record(z.string(), z.string()).optional(), | ||
| allowReservedAttributes: z.literal(true).optional(), | ||
| startHook: StartHookSchema.optional(), | ||
| /** Public key used by cross-run writers to seal payloads to this run. */ |
There was a problem hiding this comment.
AI Review: Note
The extraction into RunCreationDataSchema dropped several accurate explanatory comments: the X25519/sealed-envelope rationale on run_created.encryptionPublicKey, the resilient-start rationale on run_started.encryptionPublicKey, and in queue.ts the allowReservedAttributes mirroring note and "Initial plaintext run attributes, for resilient run creation".
start.ts lost a similar batch in the same refactor: the "<=1% of cases" note on the 409 branch, the 429/5xx/transport rationale on the retryable branch, "Queue failure is always fatal", the per-region queue routing note on opts.region, and the WORKFLOW_VM pinning note.
None of it was stale — it documents current behavior, which is what the repo guidance asks comments to do. Worth carrying forward into the new shapes.
| } | ||
|
|
||
| function normalizeStartHook(options: StartHookOptions): StartHook { | ||
| if (options.token.length === 0) { |
There was a problem hiding this comment.
AI Review: Nit
options.token.length === 0 doesn't reject a non-string. A JS caller passing a number gets undefined === 0 -> false and the value flows through to the world. typeof options.token !== 'string' || options.token.length === 0 matches what the doc comment on line 111 already promises.
|
AI Review: Note Not anchorable inline since these files aren't in the diff. Four docs pages still describe the workaround this PR is replacing, each with the phrase "until native atomic start-and-hook registration exists":
Fine to leave while the capability is inert everywhere, but |
There was a problem hiding this comment.
Additional Suggestion:
Removal of the world.streams?.get guard in the cross-deployment start path forces every streams-less world (minimal worlds / test mocks) through healthCheck(), which busy-loops until the 2s probe timeout and leaves an orphaned health-check queue message.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| } | ||
| if ( | ||
| WorkflowWorldError.is(err) && | ||
| err.code === START_HOOK_ADMISSION_REJECTED |
There was a problem hiding this comment.
AI Review: Note
Follow-up on the narrowed condition (which is the right fix). The escape hatch only fires when the World tags the rejection, and no shipped World sets START_HOOK_ADMISSION_REJECTED today. The constant appears in exactly three places: its declaration in @workflow/errors, this check, and the prose contract on Storage.events.create. No implementation produces it.
So for any World that rejects admission with a plain 400 — the shape a validation failure naturally takes — the error falls past this check, past the EntityConflictError/RunExpiredError branch, and into getWorkflowSetupErrorCode, which returns null because isWorldContractError requires the absence of a status. That rethrows, and the queue retries a permanently-invalid request forever.
Scratch test on this head:
atomic start + WorkflowWorldError{status: 400} -> threw WorkflowWorldError, runFailed=0
atomic start + WorkflowWorldError{status: 400, code: START_HOOK_ADMISSION_REJECTED} -> acked 204, runFailed=0
Pre-existing behavior for 400s in general, so not a regression. Flagging it because this PR introduces a new category of permanent 400 and the handling for it is inert until a World opts in. Worth stating in the PR body that the code is a contract for World implementors to adopt, so the follow-up doesn't ship admission rejection without it.
|
AI Review: Note Status of the earlier findings on 91d04bb. The head was force-pushed to a single squashed commit on the same base; the only files that changed since the last review are Addressed:
Still open, all re-verified on this head:
No reply on any of those threads yet, so I have left them as-is rather than reposting. CI on this head is green apart from |
2 participants
API
A successful call returns the existing
Runtype. A duplicate throwsHookConflictErrorwithconflictingRunId. If queueing or admission cannot be confirmed,WorkflowStartErroridentifies the candidate run and uncertain stage.Implementation
start()queues the candidate first, then callsevents.create()to atomically reserve the token and create the run. The direct event and durable queue input carry the same normalized Hook data, so either path can complete admission. A queued loser exits before creating a run or executing user code.The caller World and a cross-deployment target must advertise
atomicStartHook. Requests withexperimental_minRetentionmust also advertisehookRetention. This PR defines the public API, wire format, runtime behavior, and World contract; no World advertises support yet.Plan