fix(world-vercel,world-local): hold process-wide state on globalThis - #3728
Conversation
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.
🦋 Changeset detectedLatest commit: afa0302 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
…s 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.
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 41 total
Full trace: |
Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| const store = globalThis as typeof globalThis & | ||
| Record<symbol, WorldState | undefined>; | ||
|
|
||
| const state: WorldState = (store[StateKey] ??= { locks: new Map() }); |
There was a problem hiding this comment.
AI Review: Blocking
This snippet fails the lint rule added in this same PR. The rule accepts exactly two things: a globalSingleton(...) initializer, or a // per-copy-ok: annotation. store[StateKey] ??= { ... } is an element-access assignment, so collectDeclarations records store and assignment() reports it:
$ cat probe/src/docs-pattern.ts # copied verbatim from this section
type WorldState = { locks: Map<string, Promise<void>> };
const StateKey = Symbol.for('@your-org/world-foo//locks/v1');
const store = globalThis as typeof globalThis &
Record<symbol, WorldState | undefined>;
const state: WorldState = (store[StateKey] ??= { locks: new Map() });
$ node scripts/lint/module-scope-state.mjs probe
probe/src/docs-pattern.ts:4 const store (field written)
TOTAL 1
packages/utils/src/module-scope-state.test.ts discovers every published packages/world-* at runtime, so a world added to this repo that follows this section verbatim fails its own test, and the failure text tells the author to reach for globalSingleton(), which this section deliberately does not mention. AGENTS.md:446 prescribes globalSingleton(); this page prescribes the hand-rolled key. Same block at docs/content/worlds/v4/building-a-world.mdx:261.
Either teach globalSingleton() here (it is already exported from the published @workflow/utils), or teach the rule to recognize a write rooted at a globalThis alias. The second looks worth doing on its own: packages/core/src/private.ts:23 and packages/next/src/index.ts:58 are already correct globalThis-backed code and the rule flags both.
There was a problem hiding this comment.
Fixed, and I took the second option you suggested — the rule now recognizes state rooted at globalThis, following one alias hop, so the documented two-statement shape (const store = globalThis as … then store[StateKey] ??= …) passes.
You were right that it's worth doing on its own: packages/core/src/private.ts:23 and packages/next/src/index.ts:58 were both correct globalThis-backed code being flagged. Core drops from 26 findings to 22 and next from 7 to 6, purely false positives removed.
I also did the first thing, in a smaller way: the docs section now says outright that globalSingleton() from @workflow/utils does exactly this and is what the first-party worlds use, with the hand-rolled form written out so a world published outside this repo doesn't need the dependency, and notes the rule accepts either. That closes the AGENTS.md/docs inconsistency you flagged without pushing a @workflow/utils dependency onto third-party world authors.
| * Both branches they guard repeat on every event, so a per-copy log would be | ||
| * the same noise the latch exists to prevent. | ||
| */ | ||
| const wsState = globalSingleton( |
There was a problem hiding this comment.
AI Review: Blocking
Sharing this Map process-wide also shares it between two different published versions of the package, which is a coupling that did not exist before. @workflow/core depends on @workflow/world-vercel as workspace:*, which publishes as an exact version, so an install with workflow and @workflow/ai on different betas gives two copies of this package in one process. shapeVersion is 1 in both, so they meet on the same Symbol.for('@workflow/world-vercel//wsEventsTransports/v1') key, and one version's write path is handed a WsEventsTransport constructed by the other version's class.
Staged two copies of the built package (b gets one extra method to stand in for a version difference) and imported both in one process:
distinct module instances: true
A got the same object B created: true
object A holds is an instance of A's own class: false
object A holds carries B-only method: true
Replacing globalSingleton(...) with a plain object in both copies, which is main today:
A got the same object B created: false
object A holds is an instance of A's own class: true
events-v4.ts:1086 then calls transport.request(...) on that foreign instance and parses the reply against its own frames.ts schema. There is no protocol-version negotiation in this client, so a frame-format change between two co-resident betas has nothing to catch it. Pre-PR the same skew cost a second socket; post-PR it is a silent protocol mismatch.
shapeVersion cannot cover this: it versions the container ({ transports, loggedWsProxyFallback, loggedWsInUse }), which is stable, while the hazard is in the contents. The same applies to eventsRecycler in http-client.ts:539, where the shared recycler's createEventsDispatcher closure comes from whichever copy won the race, so one version's undici/H2 agent options silently apply to both.
The pools and latches genuinely want to be shared; state holding instances of module-local classes does not. ./version.js already exports the version string, so keying that one entry on it (`@workflow/world-vercel//wsEventsTransports@${version}`) keeps the layer fix and drops the cross-version handoff.
There was a problem hiding this comment.
Fixed in bd5b2a4-ish (latest push) — this was the right call and I had missed it entirely.
The registry and the events dispatcher recycler are now keyed by package version:
`@workflow/world-vercel//wsEventsTransports@${version}`
`@workflow/world-vercel//eventsDispatcherRecycler@${version}`Your framing of why shapeVersion can't cover it is exactly right and I've put it in the comment at the call site: the container shape is stable, the hazard is in the contents. Two co-resident versions now keep separate registries, which costs a second socket — the same thing that happened before this package was bundled, so it is not a regression against any state we've shipped.
I left the plain connection pools in http-client.ts:22 unversioned, per your own point that pools and latches genuinely want sharing: they hold no module-local behavior and cross-copy keep-alive reuse is the reason they exist. The recycler is different precisely because it closes over this copy's createEventsDispatcher, so I took that one.
| } | ||
|
|
||
| /** Module-scope `const`/`let` bindings in `source`, keyed by name. */ | ||
| function collectDeclarations(source) { |
There was a problem hiding this comment.
AI Review: Note
collectDeclarations plus the same-file visit walk means the rule catches the shape these two worlds happen to be written in, rather than the class. Three probes, all clean:
src/registry.ts: export const transports = new Map<string, number>();
src/consumer.ts: import { transports } from './registry.js';
export function open(id: string) { transports.set(id, 1); }
src/static-field.ts: export class Registry {
static transports = new Map<string, number>();
static open(id: string) { Registry.transports.set(id, 1); }
}
src/handed-off.ts: const cache = new Map<string, number>();
function fill(t: Map<string, number>, id: string) { t.set(id, 1); }
export function open(id: string) { fill(cache, id); }
$ node scripts/lint/module-scope-state.mjs probe
TOTAL 0
The first is the shipped bug exactly, with the registry and its mutators in separate files, which is ordinary module hygiene rather than an exotic case. The second is module-scope state the collector never looks at (source.statements only sees VariableStatement). Cross-file needs whole-package resolution, which is a lot for a heuristic, but the class-static case is a few lines here, and flagging any module-scope Map/Set/array literal that is exported regardless of where it is written would close the first without resolution.
There was a problem hiding this comment.
Fixed two of the three, and documented the third as a known gap.
Class statics: collectDeclarations now walks ClassDeclaration members for static property declarations, attributed to the class name so Registry.transports.set(…) resolves. Your probe reports static Registry.transports.
Cross-file: took your suggestion, narrowed slightly to keep false positives down. An exported binding initialized to an empty collection (new Map(), new Set(), []) is now a finding on its own, with no write required in the file. Emptiness plus the export is the signal — an exported non-empty new Map([['a', 1]]) is a lookup table and stays clean (also a fixture). Your registry.ts/consumer.ts probe now reports transports (exported empty collection).
Handed off to a function (fill(cache, id)) still passes. That needs real dataflow, so I've written it into the rule's header as an explicit limitation rather than leaving it implied. The header now has a 'what it does not see' paragraph.
All three probes plus the non-empty case are fixtures now, so the behavior is pinned in both directions.
| import ts from 'typescript'; | ||
|
|
||
| /** Methods that mutate the receiver in place. */ | ||
| const MUTATORS = new Set([ |
There was a problem hiding this comment.
AI Review: Note
The mutation check has no notion of when the mutation happens, so a table populated during module evaluation is indistinguishable from one written per request:
const BASE64_LOOKUP = new Uint8Array(256);
for (let i = 0; i < 64; i++) BASE64_LOOKUP[i] = i;
flags as const BASE64_LOOKUP (field written). That is packages/core/src/vm/uint8array-base64.ts:59, and per-copy is entirely harmless there: each copy computes the same bytes at init. Neither world has one of these today, so the rule reports zero, but the first contributor who adds a precomputed lookup or a frozen config map gets a failure whose only exit is // per-copy-ok:. Once that annotation starts appearing on declarations with no per-copy hazard it stops meaning anything, which is the failure mode for a rule whose whole value is that its escape hatch is rare. Restricting the walk to mutations reachable from a function body (skipping top-level statements) would keep the precision.
There was a problem hiding this comment.
Fixed. Mutations in top-level statements no longer count — only writes reachable from a function body do.
Your BASE64_LOOKUP example is the exact test I added (packages/utils/src/module-scope-state.test.ts, 'ignores a table filled once at module evaluation'). The reasoning I wrote into the rule header is yours: a write at module evaluation runs identically in every copy, so it costs memory and nothing else; divergence needs a write that can happen later, per request.
Agreed on the failure mode you describe — an escape hatch that starts appearing on harmless declarations stops meaning anything. That was the strongest argument for fixing this now rather than waiting for the first contributor to hit it.
| ### Observability data hydration | ||
| `packages/core/src/observability.ts` contains `hydrateResourceIO` which strips certain fields (like `executionContext`) before UI display. If you need to display data from stripped fields, extract it before the stripping occurs. | ||
|
|
||
| ### World packages must not hold mutable module state |
There was a problem hiding this comment.
AI Review: Note
Worth a sentence here on why the rule stops at packages/world-*. @workflow/core is statically imported into the same server build and has always been bundled, and this PR's own reasoning (a run started from a Server Component and consumed in a route handler puts live copies in both the ssr and app-route graphs) applies to it unchanged. It reports 26:
$ node scripts/lint/module-scope-state.mjs packages/core
TOTAL 26
packages/core/src/runtime/step-single-flight.ts:32 const inFlightSteps (`.set()`)
packages/core/src/vm/script-cache.ts:63 const scriptCache (`.delete()`)
packages/core/src/serialization/workflow-vm.ts:20 let _encoder (reassigned)
packages/core/src/runtime/start.ts:83 let hasWarnedLatestNoOp (reassigned)
...
I spot-checked several and they look wasteful rather than wrong: registeredSteps (private.ts:28) is already globalThis-backed, the compile and single-flight caches are only reached from /flow so they stay in one layer, and the duplicated cbor encoders and warn-once latches cost memory and a repeated log. So I am not asking for core in this PR. But as written, someone reading "enforces this across every published packages/world-*" alongside "the class of bug behind it" in the PR description will assume core is covered. packages/next reports 7 and packages/cli 4 for the same reason.
There was a problem hiding this comment.
Added. AGENTS.md now states where the sweep stops and why, rather than leaving 'every published packages/world-*' to be read as covering core.
I used your spot-check verbatim, since it's more useful than a bare caveat: the step registry is already globalThis-backed, the compile and single-flight caches are only reached from /flow so they stay in one layer, and the remainder cost a duplicated encoder or a repeated warn-once log — wasteful rather than wrong, which is why core isn't gated. @workflow/next and @workflow/cli noted as the same case. Widening the sweep is tracked in #3729, and the note ends by saying a new mutable module-scope binding in core should be treated as suspect even though nothing fails the build.
The globalThis fix from your other comment also brings core down to 22 and next to 6, so those numbers are now closer to the genuine ones.
| @@ -1,3 +1,7 @@ | |||
| export { | |||
| globalSingleton, | |||
| resetGlobalSingletonForTest, | |||
There was a problem hiding this comment.
AI Review: Nit
resetGlobalSingletonForTest lands on the public barrel of a published package. Its own doc comment says it is a test seam and that production code must not call it, because other copies keep writing to the object it discards. A ./test-support subpath export, or at least an @internal tag, keeps that out of the documented surface.
There was a problem hiding this comment.
Tagged @internal. I stopped short of a ./test-support subpath: @workflow/utils has no such split today, and adding one export condition for a single four-line function felt like more surface than it removes. Happy to do it if you'd rather have the hard boundary.
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>
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`.
There was a problem hiding this comment.
Looks like 'workflow/core' is not covered here.
Clanker found many guard violations:
The sweep covers published packages/world-*. @workflow/core is excluded — yet the PR's own doc comment says core "has always been bundled", which makes it the longest-standing instance of this bug class. I ran their rule against it:
packages/world-vercel — 0
packages/world-local — 0
packages/core — 26
packages/utils — 1
Most are caches (waste, not wrongness), but one looks like the same bug with worse consequences: step-single-flight.ts holds inFlightSteps as a module-scope Map, and its own doc scopes the guarantee to "already in flight in this process" while explicitly ruling cross-instance duplicates out of scope. Per-layer copies quietly break the in-process half — two layers could each run one "winner" for the same run + correlation id. I have not proven it reachable (that needs the same step driven from two layers in one process), but it is exactly the shape that bit the socket, and it deserves a look before this PR's rule declares the class covered. Worth noting registeredSteps in private.ts is already globalThis-backed, so core's authors knew the hazard — which makes core's omission from the sweep more surprising, not less.
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>
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues.
Re-reviewed c87dea39f5 and 3c628bad05. All six findings from the previous round are addressed, and I verified each against the code rather than the description.
Verified fixed
- Docs pattern vs. the rule.
isGlobalThisBackedaccepts an initializer that reachesglobalThis, including through the two-statement alias form. The section's snippet now scans clean, and so do the already-correctcore/src/private.ts:23andnext/src/index.ts:58. - Cross-version coupling. Staged two copies of the built package with distinct
version.jsvalues and confirmed both directions: different versions get separate registries and each keeps its own class's instances; same version across two module copies still shares, so the bundler-layer fix is intact.
a=beta.39 b=beta.40 same object: false own class: true
a=beta.39 b=beta.39 same object: true own class: false
- Init-time vs request-time. The
inFunctiongate drops the lookup-table false positive; theBASE64_LOOKUPshape is clean. .mts/.cts. Confirmedworld-testing/srcis entirely.mts, so that sweep was genuinely vacuous before. It now scans, and theper-copy-okon the test server's counter is honest: it is a standalone entry.- Cross-file registries, via the exported-empty-collection heuristic. The two-file split of the shipped bug is now reported.
- Core's scope boundary is written down in
AGENTS.mdwith the spot-check reasoning, which is what I was after.
Fixed directly in 00338ea
The static-field support keyed declared on the class name, so a class with more than one mutable static reported a single finding and labelled it with the wrong field's mutation:
static Registry.latch (`.set()`) # latch is assigned; transports is .set()
One went unreported, and the one that survived pointed at the wrong line. Now keyed Class.field, with a memberPath() that resolves a write to the same shape. Two things fell out of having the path: this.field inside a static member resolves to the class (and correctly does not for an instance member or a nested function), and state.count++ is now caught like the state.count += 1 that was already reported. Four fixtures pin those, including the instance-field case that must stay clean. All four worlds still report zero, and the extracted recordMutation() keeps the file at its pre-existing two Biome complexity warnings rather than adding a third.
On 3c628bad05
Reviewed separately since it is new scope. getWorld() and getWorldHandlers() are structurally identical apart from the cache symbol, and World satisfies Pick<World, 'createQueueHandler' | 'specVersion'>, so the handler's view is unchanged. It also drops a detached-method hazard: the old path passed createQueueHandler off an object literal, the new one passes the World itself. The lazy init is inside the request handler, so nothing populates the runtime cache at build time, and getWorldHandlers() stays exported with its own cache for build-time callers. Reusing the workflow.route.get_world_handlers span name is a little odd now, but the collision argument holds and the comment records it.
Local runs
utils 139, world-vercel 552, world-local 558, world-testing 16, core/runtime-world-singleton 1: all pass. Full turbo build green across 45 packages, typecheck clean. One world-postgres spec.test.ts > runs an addition failure locally that passed 15/15 on re-run; unrelated to the lint script, which nothing in that suite imports.
CI was still working through the E2E matrix with no failures when I looked. Approving on the strength of the above; worth a glance at the required aggregate before merge.
`@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`.
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.
shalabhc
left a comment
shalabhc
left a comment
There was a problem hiding this comment.
Approving, but do we also need @workflow/utils?
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues. Approval stands over the new head (9979bb56fe).
My earlier approval was on 00338ea32a, and the branch has since grown a lot, so here is a review of what came after it.
6b991a5317 — expansion to every bundled package
This takes the core-scope note further than I asked, converting @workflow/core (22 to 0), plus @workflow/world, @workflow/ai and @workflow/nest, and adds them to an explicit BUNDLED_RUNTIME_PACKAGES list. I checked the parts that carry risk rather than the count:
step-single-flight.tsis the one with teeth, and the commit is right that a per-copy map is not single-flight. The key isrunId:correlationId, fully qualified, so sharing introduces no cross-contamination.- The hot-path caches are keyed on content, not on anything copy-local:
scripts.byCodeandbaselines.byKeyon the bundle source,quickjsAssets.promiseon nothing. Sharing them is a straight win, andBASELINE_CACHE_MAX_ENTRIESnow bounds one cache instead of one per layer, so peak memory goes down rather than up. - The five
per-copy-oksites are correctly reasoned, andhardened.tsis the one I would have gotten wrong:activeStatsandreportedProxiesare armed and cleared around a synchronous call, so sharing them would actually be a bug (two copies recording concurrently would clobber each other's sink). Keeping them per-copy is right. useStepClosuresonglobalThisis correct: functions cross copies freely, so a brand applied by one copy has to be visible to another.
On the exclusion list: next is described as build-time code, but the package does have a ./runtime export. It happens not to matter, because packages/next/src/runtime.ts is a bare export * from '@workflow/core/runtime' and pulls in no local module, so none of its 6 remaining findings can reach a server bundle. The conclusion holds; the parenthetical is just looser than the reasoning behind it. Worth a word if you touch that paragraph again.
Also verified
The static-field keying you hit independently is the same defect I fixed in 00338ea32a; that commit is an ancestor here, and memberPath() plus Class.field keying is intact. WorkflowModule.options is no longer hidden behind WorkflowModule.outDir. I had a fix queued for the noAssignInExpressions error at env-config.ts and dropped it when c95c5ca02d landed the same single-line suppression. Worth knowing for next time: a suppression whose reason continues onto following comment lines is not recognized, so the reason has to sit on the biome-ignore line itself.
Local runs on 9979bb56fe
All 30 test tasks pass: core 106 files, world 13 (newly enabled by #3733), ai 12, nest 3, utils 11, world-vercel 28, world-local 18, world-testing 3. Full turbo build 45/45, turbo typecheck 43/43, repo-wide biome ci clean, and scripts/check-no-unrun-tests.mjs passes. The rule reports zero across all nine covered packages.
CI: the 16 red lanes are inherited from main, not from this branch
Every failure is packages/core/e2e/e2e.test.ts > e2e > AbortController, timing out at 60000ms rather than asserting. main's latest tests.yml run (32535198498) has 19 failing jobs across the same lane set, including E2E Required Check itself, and its last three runs are red. The failing test rotates within the group between runs, which is why it does not look identical lane to lane:
| failing test | |
|---|---|
this branch, vite - quickjs |
abortAnyInStepWorkflow, abortListenerWorkflow |
main, vite - quickjs |
abortDeterministicBranchFromStepWorkflow |
E2E Python Conformance and (python - node) are red in the same main runs for the separate reason that #3634 moved the JS spec version to 7 while the Python SDK still caps at 6.
So nothing here to fix on this branch, and the required aggregate cannot go green until main is. Worth deciding separately whether the AbortController group gets fixed or quarantined, since it is currently blocking every PR.
@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.
Yes, and thanks — that was a real gap. All four report zero today, which is exactly the state Also took the other half of the question, on the dependency itself: The sweep list is now |
|
No backport to This is a sprawling change that mixes a real fix with new API surface and new tooling: it adds a publicly exported To override, re-run the Backport to stable workflow manually via |
Generalizes #3699 from one symbol to the class of bug behind it.
What broke
serverExternalPackageswas the only thing making@workflow/world-vercela process singleton. #3493 removed it — for a real cold-start win — and every module-scopeconst/letin the package quietly became one copy per bundler layer.A bundler keys module identity on (resource, layer). Next.js alone builds
instrument, app-route,ssrandedgelayers, so a bundled module is compiled and evaluated once per layer inside one process; a package left inserverExternalPackagesis emitted as a runtimerequire()and Node's module cache dedupes it to one instance.What makes this bite rather than merely waste memory: core caches the World object on
globalThis, while the module state that World closes over stays layer-local. Anything a World reaches at request time therefore has to be process-wide too.The visible casualty was the WS events transport. Core has two independent world caches, each calling
createWorld():getWorldHandlers()→ queue consumer →openWsChannel→ registers in that copy'stransportsMapgetWorld()→ events write path →resolveWsTransport→ reads that copy's MapBefore #3493 both resolved into the same module instance. After, the socket opens in one Map and every lookup reads the other, empty one — a deterministic miss, silently demoting every event to HTTP for the life of the process.
Reproduction
Standalone app staged from tarballs (real
node_modules— a pnpm workspace link defeatsserverExternalPackages, so the workbench cannot show the counterfactual), Next 16.2.11,WORKFLOW_TARGET_WORLD=vercel,instrumentation.tsawaitinggetWorld(). Probes counted live module instances:createWorld()ran inTurbopack emits four byte-identical (8702 B) copies of
ws-transport.jsunder four module ids — one pulled byinstrumentation.js, one by everyapp/**/route.js, one underchunks/ssr/, one underedge/chunks/.Two things this establishes beyond the reported symptom:
ssr). An app that starts a run from a Server Component and consumes it in a route handler already has two worlds.@workflow/world-localhas the same exposure, and always has — it is a static import inworld.tstoo.@workflow/world-postgresand other custom worlds are safe: they load throughgetRuntimeRequire()withwebpackIgnore/turbopackIgnore, so Node dedupes them.Per #3493's own table, webpack + pnpm apps never got externalization even before it — so a revert would be both a regression in cold start and an incomplete fix.
The fix
globalSingleton(name, shapeVersion, create)in@workflow/utils— the primitive@workflow/corealready hand-rolls for its World cache — parks state onglobalThisunder aSymbol.for()key.shapeVersionis part of the key, so two releases of a package sharing one process don't meet on the same key with different expectations of the object.Fourteen files converted, every mutable module-scope binding in both worlds:
ws-transport(the channel registry + its two log latches),http-client(the undici keep-alive pools and the events dispatcher recycler: per-copy pools mean a warm-up warms a pool no route dispatches on, and each layer pays its own TLS handshake),runs(long-poll support negative cache),create-run-id(the monotonic ULID factory — two copies can mint the same run ID in one millisecond),telemetry,queue.storage/runs-storage(runFileLocks: a duplicated mutex stops mutually excluding),storage/hook-index,storage/helpersandstreamer(monotonic ULID factories forevnt_/chnk_ids),fs,init,telemetry,build-target-mismatch.State that is deliberately per-copy keeps a
// per-copy-ok: <why>annotation — the OTel diagnostic (which reports what this copy sees) and queue.ts'sAsyncLocalStorage(both ends live in one closure).Regression cover
Three layers, because a test for the WebSocket symptom would not stop the next module-scope
let:packages/utils/src/global-singleton.test.ts— the primitive's semantics: identity, factory-runs-once, cross-holder mutation,shapeVersionisolation.packages/world-vercel/src/ws-transport-module-copies.test.ts— the shipped bug in miniature. Imports the module twice in one process and asserts a transport registered by one copy is found by the other. Verified to fail on a plain module-scopeMap(2 of its 3 assertions) and pass with the fix; the third assertion pins that the two imports really are distinct instances, so the test cannot go vacuous.scripts/lint/module-scope-state.mjs— the class. A TypeScript-AST rule that flags any module-scope binding these packages mutate at runtime, withglobalSingleton(...)and// per-copy-ok: <reason>as the only two escapes (a bareper-copy-okwith no reason does not count). Surfaced as a vitest file in each package soturbo testgates it, with seven fixture self-tests pinning both directions — four sources that must be flagged, three that must stay clean — so the rule cannot rot into a no-op.Verification
pnpm build— 28/28 packages.@workflow/world-vercel559 tests,@workflow/world-local557,@workflow/utils117 — all pass; typecheck and Biome clean on the touched packages.Deliberately not in this PR
getWorld/getWorldHandlers— the comment inpackages/core/src/runtime/world.tsalready contemplates it. Two caches each callingcreateWorld()is what turned "duplicated module" into a deterministic cross-copy miss rather than a coin flip. Worth doing, separately.e2e-vercel-ws-transportpasses whether events go over WS or fall back to HTTP, and no workbenchinstrumentation.tswarms the world — which is why CI never saw this. A transport assertion plus a warm-up would give that lane the failing shape.resolveWsTransport. The silence here cost several investigations.Codifying the rule (added after review discussion)
The valid worry with the fix above is that it trades one invariant ("module scope is process-wide") for a discipline someone has to keep. There is no in-bundle way to restore the invariant — module identity is
(resource, layer), each Next server layer is its own compilation, and the server gets no shared runtime chunk (runtimeChunkis client-only in 16.2.11). The only two namespaces that cross layers at runtime are Node's require cache (be external) andglobalThis. So the rule is the mechanism, and it has to be written down where someone will meet it:docs/content/worlds/{v4,v5}/building-a-world.mdx— a Process-wide state section for custom-world authors: the two loading modes (runtimerequire(), deduped by Node — vs bundled, one copy per layer), why which one you get is not yours to assume, and a nudge to prefer World-instance state over a global.packages/world/README.md— the same constraint stated on the contract package.CLAUDE.md— for the next contributor working in these packages.packages/core/src/runtime/world.ts— at the two static imports, which is precisely where a bundled world diverges from a required one.The guard is consolidated so a future world is covered without anyone remembering:
@workflow/utilsowns the rule and its fixture self-tests, and sweeps every publishedpackages/world-*discovered at runtime (world-local,world-postgres,world-testing,world-vercel; privateworld-simis out of scope). Each world keeps a one-assertion mirror so the signal still arrives when you run just that package's tests.@workflow/world-postgresscans clean today and stays pinned — it is deduped only becausegetRuntimeRequire()loads it, which is a property of how it is loaded, not how it is written, and is exactly what flipped for world-vercel in #3493.Follow-ups, tracked
This PR contains the bleeding; the structural work is filed so it survives the merge:
#3665globalThisfor the genuinely process-wide remainder. Turns the rule into a property of the design.e2e-vercel-ws-transportpasses on silent HTTP fallback, no workbench warms the world ininstrumentation.ts, andresolveWsTransportmisses without a breadcrumb.@workflow/worldhas 13 test files and notestscript, so none run in CI — and one fails onmain(aSPEC_VERSION_CURRENTbump left a stale assertion nobody saw).Docs Preview
Also in this PR: #3666, adopted (Fixes #3665)
workflowEntrypoint's lazy handler init now callsgetWorld()instead ofgetWorldHandlers()— one line, originally written by @MintedKenny in #3666, which implements #3665 and could not run CI as a fork PR.It belongs here because it is the other half of the same bug.
getWorldHandlers()owns a second, build-time-safe cache, so the runtime route built a second World in the same process — and for a bundled world package those two Worlds are built by two different module copies, which is the mechanism behind the WS regression above.get-world-lazy.ts:25-45makes it deterministic rather than a race: the events write path resolves only fromWorldCacheand never readsStubbedWorldCache.The two changes are complementary, not redundant:
getWorldHandlers()stays public API, and@workflow/vitest, the CLI,packages/webandsetWorld()all create Worlds by design — socreate-run-id's monotonic factory, which must not fork its sequence, still needs to be process-wide.It also pays for itself independently of bundling:
world-postgres/src/index.ts:60-64eagerly constructs apg.Pool(defaultmax: 10) increateWorld(), andqueue.ts:91eagerly constructs a whole nestedworld-localWorld. TwocreateWorld()calls per process means self-hosted users have been getting 20 connections where they configured 10, plus two nested Worlds, on every instance.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). The public
getWorldHandlers()and its build-time cache are untouched.Not taken — worth a reviewer's opinion: renaming the
workflow.route.get_world_handlersspan, which now measures agetWorld()call. It is a distinct span from the per-requestworkflow.route.get_worldat the top of the flow route, so reusing that name would collide with it both in traces and inruntime-trace-mode.test.ts. A comment records why the name outlived the call.Every bundled package, not just the worlds
@workflow/coreis 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, and the reason is worth recording. Layers cannot share a module, and core cannot be made external because core is workflow code:
runtime/start.ts:253and nine methods inruntime/run.tsare'use step'functions, so it has to go through the SWC loader. An external package never does. The Next integration already encodes that rule generally — it strips workflow-bearing packages back out ofserverExternalPackagesand warns (packages/next/src/index.ts:217-221).So the duplication stays and the hazard is removed instead, everywhere it can occur:
@workflow/core@workflow/world@workflow/ai@workflow/nestMostly warn-once latches and lazy caches. Three had teeth:
step-single-flight.ts— a per-copy map is not single-flight. Two invocations arriving 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.@workflow/nest— bootstrap config in a module-levelletplus two static class fields. Configure one copy, read another, and the controller is unconfigured for the life of the process.hardened.ts'suseStepclosure brand — a function marked by one copy was invisible to another.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-byteWeakMap), the synchronously-scoped guest-code sink, and the OTel diagnostic that reports what this copy sees.Scope of the sweep. Every published
packages/world-*(discovered at runtime) pluscore,world,ai,nest, named inBUNDLED_RUNTIME_PACKAGES. Out: build-time code (next,builders,sveltekit), the CLI, the o11y UI (web,web-shared), the test runner (vitest), and private packages — all single-module-graph, where the hazard cannot occur. AGENTS.md records the criterion, because "does this run in the host's server bundle" is a judgement rather than something to infer from a directory name.Relationship to #3699
@shalabhc's #3699 fixes the ws registry and the two log latches with the same technique, arrived at independently. This PR subsumes it and extends the treatment to the remaining twelve files plus the lint rule. Happy to land #3699 first and rebase this on top if that's easier to review — the overlapping hunk is
ws-transport.ts's state block.