[pull] main from vercel:main - #532
Merged
Merged
Conversation
…3728) * fix(world-vercel,world-local): hold process-wide state on globalThis Both packages are bundled into the host application's server build, and a bundler keys module identity on (resource, layer) — Next.js alone builds `instrument`, app-route, `ssr` and `edge` layers, so one process holds one copy of each of these modules per layer. Every module-scope `const`/`let` in them was therefore per-copy state wearing the costume of a process singleton. #3493 made `@workflow/world-vercel` bundled rather than external and the events WebSocket transport regressed to HTTP for exactly this reason: the queue consumer registered its channel in the `instrument` copy's `Map` and the write path looked it up in the route copy's empty one. A deterministic miss, for the life of the process. `@workflow/world-local` had the same exposure all along — including `runFileLocks`, where a duplicated mutex simply stops mutually excluding. Add `globalSingleton()` to `@workflow/utils` (the primitive `@workflow/core` already hand-rolls for its World cache) and route every mutable module-scope binding in both worlds through it. Regression cover, in three layers: - `global-singleton.test.ts` pins the primitive's semantics. - `ws-transport-module-copies.test.ts` imports the module twice in one process and asserts a transport registered by one copy is found by the other — it fails on a plain module-scope `Map`, which is the shipped bug. - `scripts/lint/module-scope-state.mjs` fails the class: an AST rule banning mutable module-scope state in these packages, with `// per-copy-ok: <why>` as the deliberate escape. Wired into both packages' `vitest run src`, with fixture self-tests so it cannot rot into a no-op. * test(world-postgres): pin the module-scope-state rule for the postgres world It is deduped today only because `getRuntimeRequire()` loads it — a property of how it is loaded, not how it is written, and exactly what changed for world-vercel in #3493. The package is already clean; this keeps it that way. * docs(worlds): codify "a world must not hold mutable module state" A world package is loaded one of two ways, and only one of them gives it a single module instance: a runtime `require()` (deduped by Node) or the host's bundler (one copy per layer). Which one you get is a property of how the world is loaded, not of how it is written, and it changed under `world-vercel` in #3493 — so the rule has to be "never rely on module scope", not "rely on it until someone flips a config". Written down in the four places someone can meet it: - `docs/content/worlds/{v4,v5}/building-a-world.mdx` — a "Process-wide state" section for custom-world authors, with the loading modes spelled out and a nudge to prefer World-instance state over a global. - `packages/world/README.md` — the same constraint on the contract package. - `CLAUDE.md` — so the next contributor working in these packages sees it. - `packages/core/src/runtime/world.ts` — at the two static imports, which is where the difference between a bundled world and a required one originates. The rule's own error message now teaches it too, rather than naming a helper. Consolidates the guard while here: `@workflow/utils` owns the rule and its fixture self-tests, and sweeps every *published* `packages/world-*` discovered at runtime, so a world package added later is covered without anyone remembering. Each world keeps a one-assertion mirror for locality. * style: drop prose em dashes from this branch's new text #3704 landed a repo-wide writing pass hours after this branch was written and took `world-vercel/src` from 406 em dashes to 130 (`ws-transport.ts` alone went 35 to 1). This branch's docs section, README, comments and lint messages were written before that and would have put 36 of them straight back into the files that were just cleaned. Rewritten sentence by sentence rather than by substitution: an em dash becomes a colon, a comma, a full stop or a parenthetical depending on what it was doing. Also fixes a real defect the sweep surfaced: `world-postgres`'s guard test was generated through a shell heredoc and had literal backslash-backticks in its doc comment. * Update .changeset/world-module-scope-state.md Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> * fix(core): build the entrypoint's queue handler from getWorld() Adopted from #3666 by @MintedKenny, which implements #3665 and could not run CI as a fork PR. One line of behavior: `workflowEntrypoint`'s lazy handler init calls `getWorld()` rather than `getWorldHandlers()`. `getWorldHandlers()` owns a second, build-time-safe cache, so calling it from the runtime route built a *second* World in the same process. That costs a stateful World duplicate resources on every instance — world-postgres eagerly constructs a `pg.Pool` (default `max: 10`) and a nested world-local World in `createWorld()`, so self-hosted users have been paying for two of each — and, for a bundled world package, the two Worlds are built by two different module copies, which is the mechanism behind the WS transport regression the rest of this branch contains. The public `getWorldHandlers()` and its separate build-time cache are unchanged; only the runtime route stops using it. Kept from the original: the regression test asserting the factory runs exactly once, and the api-reference wording (re-applied over #3704's list punctuation). Not taken: renaming the `workflow.route.get_world_handlers` span. It is a distinct span from the per-request `workflow.route.get_world` at the top of the flow route, and reusing that name would collide with it in traces and in `runtime-trace-mode.test.ts`; a comment records why the name outlived the call. Co-authored-by: Kenneth <kenneth@standardforensics.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: address AI review on the module-scope work Two blocking findings, both real: - **Cross-version state sharing** (`ws-transport.ts`). A process can hold two *published versions* of `@workflow/world-vercel` (a transitive dependency pinning an older `@workflow/core`, which depends on this package by exact version). Both wrote to the same unversioned `Symbol.for` key, so one version's write path could be handed a `WsEventsTransport` built by the other's class and frame against a protocol it may not share — with no version negotiation on the socket to catch it. `shapeVersion` cannot express this: the container is stable, the hazard is its contents. The registry and the events dispatcher recycler are now keyed by package version. The plain connection pools stay unversioned; sharing those across copies is the point. - **The documented pattern failed the rule this PR adds.** The custom-world docs teach `store[StateKey] ??= …`, which the rule flagged as a field write. It now recognizes state rooted at `globalThis`, following one alias hop, which is also what `core/private.ts:23` and `next/src/index.ts:58` are already doing correctly (core drops 26 findings to 22, next 7 to 6). The docs also now say outright that `globalSingleton()` is the same thing, since AGENTS.md prescribes it and the page did not mention it. Rule precision, from the review's probes: - `.mts`/`.cts` are scanned. `@workflow/world-testing` is authored in `.mts`, so its entry in the sweep was passing vacuously — with the walk fixed it reports a real finding, now annotated (it is a standalone `serve()` entry). - Mutations in top-level statements no longer count. A table filled at module evaluation is identical in every copy; divergence needs a later write. - `static` class fields are collected, attributed to the class name. - An *exported* binding initialized to an empty collection is a finding on its own, which approximates the cross-file case the walk cannot resolve. Six fixtures pin the new behavior. The rule's header now states what it does not see, and AGENTS.md states where the sweep stops and why core is not gated yet. Also tags `resetGlobalSingletonForTest` `@internal`. * fix(lint): attribute a static-field write to the field, not the class The static-field support added in the previous commit keyed `declared` on the class name, so a class carrying more than one mutable static reported one finding instead of one per field, and labelled the survivor with whichever mutation was seen first. On a two-static fixture it reported `static Registry.latch (`.set()`)`: the name of one field, the reason belonging to the other, pointing the reader at the wrong line. Key static fields `Class.field` and resolve a write to the same shape, via a new `memberPath()` that takes the first two segments of a member chain and tries that key before the bare root identifier. Two follow-ons fall out of having the path: - `this.field` inside a `static` member resolves to the class, which is the ordinary way to write the mutation. `staticClassOf()` returns nothing for an instance member, where `this` is an instance and the state is per-instance rather than per-copy, and nothing inside a nested `function`, which rebinds `this`. - `state.count++` is now a finding, like the `state.count += 1` that `assignment()` already reported. Fixtures pin all four, including the instance-field case that must stay clean. The four world packages still report zero, and the extracted `recordMutation()` keeps the file at its previous two Biome complexity warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: make module duplication inert across every bundled package `@workflow/core` is bundled into the host server build the same way the worlds are, and always has been — the original repro measured three live copies in every arm, including the pre-#3493 external one. One instance is not reachable: layers cannot share a module, and core cannot be external because it *is* workflow code (`runtime/start.ts:253` and nine methods in `runtime/run.ts` are `'use step'`), so it must go through the SWC loader. The Next integration already encodes that rule by removing workflow-bearing packages from `serverExternalPackages`. So the duplication stays and the hazard is removed instead, everywhere the duplication can happen. `@workflow/core` (22 findings to 0): warn-once latches in `constants.ts`, `start.ts` and `telemetry.ts`; the source-map tracer cache; the VM script cache; the QuickJS compiled-assets and baseline caches; the dev-server port cache (its own comment already said "per process"); the text codecs; the zstd browser decoder; and the `useStep` closure brand, where a function marked by one copy was invisible to another. The one with teeth was `step-single-flight.ts`: a per-copy map is not single-flight. Two invocations reaching it through different layers would each believe they were alone in the process and both run the step body, silently degrading in-process dedup to the cross-process residual its own doc scopes out to the ownership lease. Also `@workflow/world` (a warn-once set, hand-rolled onto `globalThis` to keep that package dependency-free), `@workflow/ai` (the lazy OTel API), and `@workflow/nest` (bootstrap config in a module-level `let` and two static class fields — configure one copy, read another, and the controller is unconfigured for the life of the process). Five sites are deliberately per-copy and now say why: state keyed on objects that never cross copies (the barrier safety-net `WeakSet`, the QuickJS pending byte `WeakMap`), the synchronously-scoped guest-code sink, and the OTel diagnostic that reports what *this* copy sees. The sweep now covers all of it. Packages with a single module graph stay out (build-time code, the CLI, the o11y UI, the test runner) and AGENTS.md records which and why. Found while doing this: two static fields on one class collapsed into a single entry in the rule, so `WorkflowModule.options` was invisible behind `WorkflowModule.outDir`. Statics are now keyed `Class.field`. * fix(world): suppress noAssignInExpressions on the globalThis idiom The hand-rolled form trips Biome, as it does in `packages/core/src/private.ts`, which carries the same suppression. Restructuring it into a helper function instead would hide the state behind a call the module-scope rule cannot follow, so the binding would stop being recognized as off-module and the package would report a finding for correct code. * fix: sweep every bundled package, and mark utils side-effect free @shalabhc asked on review whether `@workflow/utils` needs this too. It does, and so do three others: `utils`, `errors`, `serde` and `workflow` all end up in the host application's server build and none were in the sweep. All four report zero today, which is exactly the state `world-testing` appeared to be in before the `.mts` walk was fixed and it turned out to have a real finding. Being clean and being *checked* are different properties, and only the second one survives the next contributor. `sideEffects: false` on `@workflow/utils`: verified that every module in the package only declares (no import-time work), so a bundler can now drop the unused parts of the barrel instead of keeping all ~64 KB of it because three packages import one 476-byte function. --------- Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> Co-authored-by: Peter Wielander <mittgfu@gmail.com> Co-authored-by: Kenneth <kenneth@standardforensics.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Peter Wielander <peter.wielander@vercel.com>
* Default the events transport to WebSockets WORKFLOW_EVENTS_TRANSPORT=http is the opt-out. Only that exact value disables it, so a typo'd or empty value fails toward the default rather than quietly pinning a deployment to HTTP. The prerequisite the gate named for defaulting on is met: postEventFrameOverWs opens a client span per frame. What is still missing is Vercel's outgoing-requests view, which reads instrumented fetch calls rather than spans and so cannot show a transport that issues no request. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * docs: WORKFLOW_EVENTS_TRANSPORT defaults to ws Three places still documented http as the default. Each now states the opt-out is the exact value http, rather than leaving 'default: ws' to imply that anything non-ws disables it — the asymmetry is deliberate in the code and is the part a reader would otherwise get wrong. Also drops 'Experimental' from the Vercel World page: a setting that is on for everyone by default is not opt-in experimental, whatever else it is. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * Fix the gate's own unit tests for the flipped default Five tests in ws-transport.test.ts still encoded the opt-in semantics. Three were the isWsEventsTransportEnabled table itself; the other two (openWsChannel 'does nothing when the gate is off', and the channel release equivalent) relied on the suite's ambient unset environment meaning 'off', which it no longer does. Both now set http explicitly. Two tests in ws-transport-spans.test.ts asserted HTTP-side span behaviour the same way. The write one would have kept passing by falling through resolveWsTransport's null rather than because the gate was off - passing for the wrong reason, which is what this file exists to catch. Also makes the opt-out case-insensitive and trimmed. The gate is deliberately asymmetric - unrecognized values take the default - but that asymmetry should not extend to swallowing HTTP or ' http '. Whoever reaches for the escape hatch is plausibly mid-incident, and silently ignoring their opt-out over a capital letter is the same class of silent-wrong-transport bug this flip is meant to stop shipping. 554 tests pass in packages/world-vercel. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * ci: add a required forced-HTTP e2e lane (#3703) Flipping the default makes e2e-vercel-prod a WebSocket lane: it sets no WORKFLOW_EVENTS_TRANSPORT, and unset now means ws. Nothing in the file would exercise the HTTP events transport against a real deployment any more, so this is not additive coverage — it replaces coverage the flip silently removed. Unconditional and required rather than label-gated like the WS lane. HTTP is now the fallback, and the fallback is silent: resolveWsTransport returning null costs a write nothing and logs nothing, which is the shape of the durabench bug this stack came out of. Two apps rather than the WS lane's four, since every row is a real vercel deploy charged to every PR. nextjs-turbopack is the only fixture emitting OTEL spans, so it is the one that can show which transport actually ran; express covers the non-Next server path. Also corrects the WS lane's docblock, which claimed every other job exercises HTTP only. That stopped being true one commit ago. Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * Fail loudly when step_completed falls back to HTTP under a strict flag The WS e2e lane asserts that the transport is harmless, not that it is used: an event written over HTTP produces the same run outcome as one written over the socket, so the lane stayed green through the entire period the transport was silently demoted. WORKFLOW_INTERNAL_EVENTS_TRANSPORT_STRICT turns that one case into a failed run, and the WS lane now sets it. Scoped to step_completed alone, because most fallback is legitimate: run_created is written outside any invocation that opens a channel; run_started routinely lands before the channel is registered (34% HTTP on a healthy deployment); step_created and wait_created mostly fold into events.createBatch, which is not wired to the socket; and a write after the invocation released its claim falls back by design. step_completed is issued after a step body has run, and was 100% ws across every WS-enabled deployment measured on two SDK versions. The flag reads as off unless the value is exactly 1 or true - the opposite asymmetry from the transport gate, which treats an unrecognized value as on. That gate risks a deployment sitting quietly on the wrong transport; this one fails runs, and should not be acquired by a typo. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * ci: run the WS transport lane on every PR It was opt-in behind ws-transport-test because four real vercel deploys were too much to charge an unrelated PR for a transport that was off by default. Flipping the default expires that reasoning from both ends: the cost is no longer for someone else's feature, and this is now the only lane that asserts the socket carried the events. e2e-vercel-prod inherits the new default but checks nothing, so behind a label the average PR would move every deployment onto WebSockets with nothing verifying they were used. Drops WS_REQUIRED from the gate along with it. That existed only to let the lane be legitimately skipped on an unlabelled PR; with no label the lane is required unconditionally, like e2e-vercel-prod and the HTTP lane, and the skipped case is now a failure rather than a warning. Gate script extracted and run against the cases that matter: ws skipped fails on a standard PR, ws skipped fails under workflow-server-test, and all-green passes. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> * ci: widen the HTTP transport lane to six server shapes Before the flip, HTTP was the default and all 28 e2e-vercel-prod lane-runs covered it. After the flip they cover WebSockets instead, and this lane is the entirety of the HTTP coverage - two apps was too thin for a transport that is still supported. Six, not the full 14, because every row is a real vercel deploy charged to every PR. Chosen by server shape rather than count: example (baseline), nextjs-turbopack (Next, and the only fixture emitting OTEL spans), vite (Vite SSR), express (Node req/res), nitro (h3, also covers nuxt) and hono (fetch-API Request/Response, a different mount shape from express). The rest duplicate a shape already covered; python is left out because it has no conformance gate and needs routes this suite does not serve. The first four match the WS lane's matrix on purpose, so the same fixture runs on both transports and a failure on one can be read against the other. Project ids and slugs are copied from e2e-vercel-prod and verified equal to it; both lanes already use the same team and token. Co-Authored-By: opencode <opencode@vercel.com> Co-Authored-By: shalabhc <shalabh.chaturvedi@vercel.com> --------- Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
* Revert "[world] Make the sealed log opt-in instead of default-on (#3735)" Reverts b2cac62. New runs are stamped at spec 7 again, now that a read which cannot see past an unfilled position waits for it instead of reporting a log that ends there (workflow-server: derive the in-request seal poll budget from the staleness bound). Two things are kept from #3735 rather than reverted: - the world-testing conformance floor at mintedSpecVersion(), which was wrong for any staged bump and not specific to this default - a note on mintedSpecVersion recording what default-on rests on: the events density requirement, and that a sealed log meets it by repair rather than by construction, so the READ has to wait Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * TEMPORARY: point world-vercel at workflow-server#839 preview Validating the seal-poll-budget fix end to end with spec 7 on. Reverted before merge; the override lint guard is expected to fail meanwhile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Revert "TEMPORARY: point world-vercel at workflow-server#839 preview" This reverts commit 5e17cc9. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )