Skip to content

Commit d8199fb

Browse files
committed
fix(engine): drain scheduled work before a fake client drops its globals
--run-tests=linux React finishes an unmount on a later macrotask. Those tasks still resolve window through GlobalThisItemProxy, so removing this client's values first made the getter answer originalValue — undefined for window under Bun — and the late task threw reading window.event. Assertions all passed; the file went red on an unhandled error between tests.
1 parent fe01c49 commit d8199fb

2 files changed

Lines changed: 99 additions & 0 deletions

File tree

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
# React scheduled work outlives the test's DOM, and Bun 1.4 counts it
2+
3+
**Status:** open · **Area:** test-infra (int lane, Linux) · **Kind:** teardown
4+
leak
5+
6+
Seen twice on ubuntu right after the Bun 1.4 bump, in two different files, with
7+
one signature: react-dom's scheduler callback runs **after** the test's DOM is
8+
gone and dereferences `window`.
9+
10+
```
11+
# Unhandled error between tests
12+
17920 | schedulerEvent = window.event;
13+
TypeError: undefined is not an object (evaluating 'window.event')
14+
at react-dom-client.development.js:17920
15+
at performWorkUntilDeadline (scheduler.development.js:45)
16+
```
17+
18+
- **run 32402353586, `int-2`** —
19+
`engine/tests/subscription-lifecycle.int.test.tsx`: `3 pass, 0 fail, 1 error`.
20+
Every assertion passed; the file went red purely on the unhandled error, and
21+
took the job with it.
22+
- **run 32399811362, `int-3`** —
23+
`engine/tests/subscription-tracked.int.test.tsx`: the same `window.event`
24+
error, plus three tests failing with
25+
`Cannot access serverOnlyGlobal item "__POINT0_SERVER_LOGGER__" from client` —
26+
the store resolving a client variant in what should be a server context.
27+
28+
Neither reproduces on macOS: both files pass in isolation, and the whole `int-3`
29+
group passes locally (19 files, ~95 s).
30+
31+
## Why it is worth a card, not a rerun
32+
33+
`0 fail, 1 error` is the tell. The product code under test is fine — what fails
34+
is the boundary: React keeps a scheduler task queued past the end of the test,
35+
the harness tears down the happy-dom globals, and the task then lands in a world
36+
with no `window`. Bun 1.4 shifted the timing enough to make the overlap common;
37+
the shape has nothing to do with load, so a green rerun is luck, not evidence.
38+
39+
Both symptoms are the same boundary, crossed in opposite directions — and the
40+
boundary is the fake client's **async context**, not the DOM globals.
41+
42+
`GlobalThisItemProxy` (engine/src/fake-client.ts) does not assign
43+
`globalThis.window`; it installs a **getter** that asks
44+
`superstore.getFakeClient()` — i.e. `AsyncLocalStorage.getStore()` — who is
45+
asking, and answers `originalValue` (`undefined` for `window` under Bun) to
46+
anyone outside a fake-client context. So:
47+
48+
- **Context missing when it should be there** — React's scheduler task runs on a
49+
chain the ALS store doesn't reach, `window` resolves to `undefined`, and
50+
`window.event` throws. This is the `int-2` failure.
51+
- **Context present when it should not be** — `subscription-tracked` builds its
52+
engine through a plain `Engine.create()` at test level, outside any
53+
`fakeClient.run()`. If a previous test's context is still on the chain,
54+
`getFakeClient()` answers, `superstore` resolves `variant: 'fakeClient'`
55+
instead of `'server'`, and the `serverOnlyGlobal` write throws. This is the
56+
`int-3` failure.
57+
58+
Note that `variant` is decided in `super-store.ts` from `POINT0_SIDE`, the ALS
59+
store and the fake client — **not** from `window`. An earlier draft of this card
60+
blamed the DOM global for the variant confusion; that was wrong.
61+
62+
## What is fixed, and what is not
63+
64+
**The int-2 half is fixed** (`FakeClient.destroy`): teardown now drains the
65+
macrotask queue inside the client's context, before the `finally` drops the
66+
client's values, so React's late unmount work still resolves `window`.
67+
68+
**The int-3 half is not.** How a fake-client context outlives its `run()` and
69+
greets the next test's `Engine.create()` is still unexplained, and nothing here
70+
addresses it.
71+
72+
Neither half reproduces in isolation: `test-one.yml` ran each file ×10 on ubuntu
73+
green (runs 32458036398, 32458045167). It needs the fast lane's real load —
74+
dozens of files in parallel on one runner — so verification means pushing a
75+
branch with `--run-tests=linux`, not a point run.
76+
77+
## Related
78+
79+
- [ci-flakes](./ci-flakes.md) — the rule this follows: an unhandled error with
80+
clean assertions is not the loaded-runner flake profile.
81+
- [playwright-dom-queue-race](./playwright-dom-queue-race.md) — the other
82+
harness race the 1.4 bump surfaced, on Windows, unrelated in mechanism.

‎packages/engine/src/fake-client.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,16 @@ type CookieStoreGetter = {
3030
(): Record<string, string>
3131
}
3232

33+
/**
34+
* Yield the event loop a few times so work another library queued — React's scheduler, above all — has run before the
35+
* caller tears anything down. Three turns because a React unmount can chain: the cleanup task queues the next one.
36+
*/
37+
const drainScheduledWork = async (): Promise<void> => {
38+
for (let turn = 0; turn < 3; turn++) {
39+
await new Promise<void>((resolve) => setTimeout(resolve, 0))
40+
}
41+
}
42+
3343
class GlobalThisItemProxy {
3444
// item key -> item proxy
3545
static items = new Map<string, GlobalThisItemProxy>()
@@ -595,6 +605,13 @@ export class FakeClient<TState extends FakeClientState, TError extends ErrorPoin
595605
if (this.onDestroyInside) {
596606
await this.run(async () => {
597607
await this.onDestroyInside?.(this.state)
608+
// Teardown does not finish in the turn that starts it: React's scheduler lands an unmount's passive
609+
// cleanup on a later macrotask. Those tasks were scheduled inside this client's context, so they still
610+
// ask GlobalThisItemProxy for `window` — and the `finally` below drops this client's values, after which
611+
// the getter answers `originalValue`, which is `undefined` for `window` under Bun. The late task then
612+
// throws reading it, outside any test's stack, and bun reports an unhandled error that reddens the whole
613+
// file even though every assertion passed. So let the queue empty while the globals still resolve.
614+
await drainScheduledWork()
598615
})
599616
}
600617
} finally {

0 commit comments

Comments
 (0)