Skip to content

Fix remaining chunkErrorRecovery.spec.ts hydration/guard races under CI shard contention #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
WilfordGrimley merged 1 commit into from
Jul 23, 2026
Merged

Fix remaining chunkErrorRecovery.spec.ts hydration/guard races under CI shard contention #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 66 additions & 22 deletions docs/troubleshooting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1557,15 +1557,18 @@ this entry's cause with the confirmed one.

## `chunkErrorRecovery.spec.ts`'s `expect.poll(() => reloadRequests).toBe(1)` fails with "Received: 0" in CI, passes locally

**Symptom**: one of the first three `chunkErrorRecovery.spec.ts` tests
(the ones that dispatch a synthetic `ChunkLoadError` and expect exactly
one intercepted reload request) times out at 0 reload requests on a CI
shard, including on a clean re-run, while passing reliably on a local
machine. No error is thrown anywhere in the test itself (`page.route()`
registers fine, `page.evaluate()` dispatching the synthetic error
completes fine) — the reload request simply never arrives.

**Cause**: `useChunkErrorRecovery`'s guard
**Symptom**: one of the `chunkErrorRecovery.spec.ts` tests (the ones
that dispatch a synthetic `ChunkLoadError` and expect exactly one
intercepted reload request) times out at 0 reload requests on a CI
shard - specifically shard 1/4, which is where this spec lands (verify
with `npx playwright test --list --shard=1/4`) - including on a clean
re-run, while passing reliably on a local machine run in isolation. No
error is thrown anywhere in the test itself (`page.route()` registers
fine, `page.evaluate()` dispatching the synthetic error completes
fine) - the reload request simply never arrives. Two independent root
causes were found across two rounds of this; both had to be fixed.

**Cause #1** (PR #397): `useChunkErrorRecovery`'s guard
(`chunkErrorRecovery.ts`'s `CHUNK_RELOAD_GUARD_KEY`, a real 10s
sessionStorage-backed "only one reload per window" debounce, working
exactly as designed) gets consumed by a **real** chunk hiccup before
Expand All @@ -1582,16 +1585,57 @@ test body reaches its own dispatch, and any real transient chunk error
during that churn legitimately consumes the guard first. This is a
dev-server/test-harness artifact only — the deployed static export has
no on-demand compilation or HMR at all, so it can't happen in
production.

**Fix**: the test now imports `CHUNK_RELOAD_GUARD_KEY` from
`chunkErrorRecovery.ts` and calls
`page.evaluate((key) => window.sessionStorage.removeItem(key), CHUNK_RELOAD_GUARD_KEY)`
immediately before each test's own synthetic dispatch, establishing a
clean precondition regardless of whatever real dev-server chunk noise
happened during page load. The one test that specifically exercises the
guard-suppression behaviour (two dispatches in one test) only clears it
once, before the first dispatch, so the guard is still genuinely
exercised for real on the second one. See `chunkErrorRecovery.spec.ts`'s
own comment above `clearReloadGuard` for the full trace-based diagnosis
(root-caused against CI run 30039392833, shard 1/4).
production. Fixed by clearing the guard immediately before each test's
own dispatch - but this alone wasn't sufficient (see cause #2).

**Cause #2** (found after #397 merged, PR #395's CI run 30043836352):
same symptom recurred on shard 1/4, all 3 attempts including retries.
Two independent gaps remained: (a) #397 cleared the guard and
dispatched the synthetic error as two _separate_ `page.evaluate()`
round-trips, leaving a real (if narrow) window between them for the
same class of dev-server chunk noise cause #1 already identified to
land in; (b) `loadPageWithDefaultBackend()`'s own "Choose Art" click is
a raw DOM click - Chromium dispatches it whether or not React has
actually hydrated and `useChunkErrorRecovery`'s `useEffect` has run
yet, so a successful click is not proof the recovery listener is live.
Reproduced locally: this spec failed intermittently at
`--workers=4` (parallel Playwright workers contending for CPU with the
dev server's own on-demand compilation, closely matching CI's shard-1
composition after PR #395 un-skipped 58 ported parity tests into the
same 4 shards - see `git log` on `playwright.config.ts`, whose
`fullyParallel`/`workers: undefined` settings predate this spec and
were never the trigger) but passed reliably at `--workers=1`. A native
DOM event dispatched before a listener is attached is simply lost, so
this can't be fixed by polling for longer.

**Fix**: (a) folds the guard-clear and the synthetic dispatch into ONE
`page.evaluate()` call per test (`clearGuardAndDispatchChunkError(Error)`
`AsRejection`) so nothing on the page's event loop can interleave
between them. (b) adds `awaitHydrated()`, which retries clicking
"Choose Art" (mirroring `test-utils.ts`'s own established
`openAddCardsDropdown()` pattern for the identical symptom) until
`ProjectEditor.tsx`'s "editor" tab content - specifically
`CardGrid.tsx`'s "Your project is empty at the moment." empty-state
text - actually becomes _visible_, which can only happen once
`Tab.Container`'s `onSelect` handler has bound and fired, i.e. once the
same hydration/effect-flush pass that mounts
`useChunkErrorRecovery`'s own listeners has completed. This needed no
extra network dispatch or navigation - two earlier attempts that DID
add one (a page.route()-intercepted warm-up reload, and a real
uncontrolled warm-up reload followed by re-navigating) were each
independently found to destabilise the page even further under this
same `--workers=4` stress (a `SecurityError: ... Access is denied for this document` on the aborted-navigation path, and a `Navigation ... is interrupted by another navigation` on the real-reload path - the
self-referential "Editor" nav-link prefetch from cause #1 is a
genuinely recurring background navigation under load, not a one-off,
and collides with any _additional_ top-level navigation this file
issues). The one test that specifically exercises the guard-suppression
behaviour (two dispatches in one test) only clears the guard once,
before the first dispatch, so the guard is still genuinely exercised
for real on the second one. See `chunkErrorRecovery.spec.ts`'s own
comments for the full trace-based diagnosis of all three rounds
(root-caused against CI runs 30039392833 and 30043836352, both shard
1/4, plus local repro at `--workers=4`/cold `.next` cache/CI=true).
Verify any future fix here the same way: `--shard=1/4` (not the file in
isolation) against a cold `.next` cache, since shard composition (not
just raw worker count) drives how much dev-server contention this spec
actually sees.
146 changes: 111 additions & 35 deletions frontend/tests/chunkErrorRecovery.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,44 +27,124 @@ const dispatchChunkError = (page: Page) =>
);
});

// CI diagnosis (PR #395, run 30039392833, shard 1/4 - failed twice consecutively, including a
// clean re-run, while passing locally): the recovery mechanism's own sessionStorage guard
// (chunkErrorRecovery.ts's CHUNK_RELOAD_GUARD_KEY, a real 10s "don't loop" debounce, not a bug)
// can legitimately already be set by the time a test gets around to dispatching its own synthetic
// error. `npm run dev`'s webserver compiles /editor on demand (its own network trace showed a
// second, ~800ms recompile of pages/editor.js mid-test, triggered by the navbar's own "Editor"
// nav link - visible while already ON /editor - being prefetched by next/link's default viewport
// IntersectionObserver behaviour), and that on-demand-compilation churn is exactly the class of
// transient chunk hiccup this whole mechanism exists to recover from - it's plausible for a real
// one to fire and consume the guard before a slow/cold CI runner's test body gets to its own
// dispatch. This is a dev-server/test-harness artifact only: the deployed static export has no
// on-demand compilation or HMR at all, so this can't happen in production, and the guard
// suppressing a second reload within its window is the product working exactly as designed - see
// docs/troubleshooting.md's chunkErrorRecovery.spec.ts entry. Clearing the guard immediately
// before each test's own dispatch establishes the clean precondition the assertion actually means
// to test ("a synthetic ChunkLoadError triggers exactly one reload"), independent of whatever
// unrelated real chunk noise the dev server produced getting the page ready - it does not touch
// the guard-suppression behaviour itself, which the last test below still exercises for real via
// two dispatches inside the same clean window.
// CI diagnosis #1 (PR #395, run 30039392833, shard 1/4 - failed twice consecutively, including a
// clean re-run, while passing locally; fixed by PR #397): the recovery mechanism's own
// sessionStorage guard (chunkErrorRecovery.ts's CHUNK_RELOAD_GUARD_KEY, a real 10s "don't loop"
// debounce, not a bug) can legitimately already be set by the time a test gets around to
// dispatching its own synthetic error - `npm run dev`'s on-demand page compilation, triggered by
// the navbar's own "Editor" nav link being prefetched by next/link while already ON /editor, is
// exactly the class of transient chunk hiccup this mechanism exists to recover from, and a real
// one firing first consumes the guard before the test's own dispatch. PR #397 fixed this by
// clearing the guard immediately before each test's own dispatch.
//
// CI diagnosis #2 (PR #395, run 30043836352, shard 1/4, after #397 was merged - all 3 attempts
// including retries failed, and reproduced intermittently on a local box too): #397's fix wasn't
// sufficient on its own, for two independent reasons.
//
// (a) #397 called clearReloadGuard(page) and dispatchChunkError(page) as two SEPARATE
// page.evaluate() round-trips, leaving a real (if narrow) gap between them for the exact same
// class of dev-server chunk noise diagnosis #1 already identified to land in. Fixed below by
// folding the clear and the dispatch into one page.evaluate() call
// (clearGuardAndDispatchChunkError / clearGuardAndDispatchChunkErrorAsRejection) - a single
// synchronous browser-side task nothing else on the page's event loop can interleave with.
//
// (b) loadPageWithDefaultBackend()'s own "Choose Art" click is a raw DOM click, which Chromium
// dispatches whether or not React has actually hydrated and useChunkErrorRecovery's useEffect has
// run yet - a successful click is not proof the recovery listener is live. Locally, this spec
// failed intermittently at `--workers=4` (parallel Playwright workers contending for CPU with the
// dev server's own on-demand compilation) but passed reliably at `--workers=1` - that
// serial-vs-parallel signature points squarely at hydration timing. A native DOM event dispatched
// before a listener is attached is simply lost (never delivered once the listener does attach),
// so this can't be worked around by polling for longer - it has to be worked around by proving
// hydration completed *before* the test's own real dispatch.
//
// CI diagnosis #3 (this box, reproduced locally at `--workers=4 --repeat-each=10` against a cold
// `.next` cache) - two dead ends before landing on the fix below, both worth recording since
// they're each individually tempting to re-derive:
// - A disposable warm-up ChunkLoadError dispatch, intercepted and aborted via page.route()
// (mirroring how every other dispatch in this file is guarded): under this exact stress, EVERY
// test started failing deterministically (40/40) with `page.evaluate: SecurityError: Failed to
// read the 'sessionStorage' property from 'Window': Access is denied for this document` on the
// very next page.evaluate() call after that abort - evidence the document was transiently in
// an about:blank-like state right after the abort completed (Chromium's bookkeeping for an
// aborted top-level navigation isn't fully synchronous under this much contention).
// - Letting that same warm-up reload complete for real (no interception), then re-running
// loadPageWithDefaultBackend() to get back to a configured page: this traded the SecurityError
// for `page.goto: Navigation ... is interrupted by another navigation to ".../editor"` -
// confirming the self-referential "Editor" nav-link prefetch (diagnosis #1) is a genuinely
// recurring background navigation under this stress, not a one-off, and collides with ANY
// explicit page.goto()/reload this file issues, not just the first one.
// Both dead ends required an EXTRA top-level navigation beyond what PR #397's baseline already
// safely did (confirmed 40/40 stable under the identical stress in an A/B before either attempt).
// The fix below requires none: react-bootstrap's uncontrolled `Tab.Container` in
// ProjectEditor.tsx defaults `editorPanel` to `"import"` (the "Add Cards" tab), so the "editor"
// tab's content - including CardGrid.tsx's "Your project is empty at the moment." empty-state
// text - is present in the DOM either way but only becomes *visible* once Tab.Container's
// onSelect handler actually runs and flips `activeKey` to `"editor"`, which needs the same
// hydration/effect-flush pass that mounts useChunkErrorRecovery's own listeners (React flushes a
// commit's effects top-down in one pass, and Layout wraps ProjectEditor). Waiting for that text to
// become visible is therefore direct, hydration-dependent proof, entirely via DOM state - no
// dispatch, no route, no navigation, so nothing left to race.
//
// loadPageWithDefaultBackend() (test-utils.ts) already clicks "Choose Art" once, but if that
// single click lands before hydration finishes, it's a dead click - React never sees it, the tab
// never switches, and no later click replays it (a native DOM event dispatched into an inert,
// not-yet-hydrated element is simply gone). So this can't just *wait* for the earlier click's
// result; it has to be prepared to *retry* the click itself until one lands after hydration -
// exactly the established pattern test-utils.ts's own openAddCardsDropdown() already uses for the
// identical symptom ("sometimes playwright is too 'fast' and clicking doesn't open the dropdown").
// See docs/troubleshooting.md's chunkErrorRecovery.spec.ts entry for the full trace-based
// diagnosis of all three rounds.
const awaitHydrated = (page: Page) =>
expect(async () => {
await page.getByText("Choose Art").click();
await expect(
page.getByText("Your project is empty at the moment.")
).toBeVisible();
}).toPass({ timeout: 10_000 });

const clearReloadGuard = (page: Page) =>
page.evaluate(
(key) => window.sessionStorage.removeItem(key),
CHUNK_RELOAD_GUARD_KEY
);

const clearGuardAndDispatchChunkError = (page: Page) =>
page.evaluate((key) => {
window.sessionStorage.removeItem(key);
const error = new Error("Loading chunk 5 failed.");
error.name = "ChunkLoadError";
window.dispatchEvent(
new ErrorEvent("error", { error, message: error.message })
);
}, CHUNK_RELOAD_GUARD_KEY);

const clearGuardAndDispatchChunkErrorAsRejection = (page: Page) =>
page.evaluate((key) => {
window.sessionStorage.removeItem(key);
const error = new Error("Loading CSS chunk 2 failed.");
// PromiseRejectionEvent isn't constructible directly in most browsers - a plain object
// with the same shape the real listener reads (`.reason`) is sufficient here since the
// hook only ever reads that one property.
window.dispatchEvent(
Object.assign(new Event("unhandledrejection"), { reason: error })
);
}, CHUNK_RELOAD_GUARD_KEY);

test.describe("Chunk-load-error recovery", () => {
test("a ChunkLoadError dispatched as a window 'error' event triggers a reload", async ({
page,
}) => {
await loadPageWithDefaultBackend(page);
await awaitHydrated(page);

let reloadRequests = 0;
await page.route(page.url(), async (route) => {
reloadRequests++;
await route.abort();
});

await clearReloadGuard(page);
await dispatchChunkError(page);
await clearGuardAndDispatchChunkError(page);

await expect.poll(() => reloadRequests).toBe(1);
});
Expand All @@ -73,28 +153,23 @@ test.describe("Chunk-load-error recovery", () => {
page,
}) => {
await loadPageWithDefaultBackend(page);
await awaitHydrated(page);

let reloadRequests = 0;
await page.route(page.url(), async (route) => {
reloadRequests++;
await route.abort();
});

await clearReloadGuard(page);
await page.evaluate(() => {
const error = new Error("Loading CSS chunk 2 failed.");
// PromiseRejectionEvent isn't constructible directly in most browsers - a plain object
// with the same shape the real listener reads (`.reason`) is sufficient here since the
// hook only ever reads that one property.
window.dispatchEvent(
Object.assign(new Event("unhandledrejection"), { reason: error })
);
});
await clearGuardAndDispatchChunkErrorAsRejection(page);

await expect.poll(() => reloadRequests).toBe(1);
});

test("an unrelated error never triggers a reload", async ({ page }) => {
await loadPageWithDefaultBackend(page);
await awaitHydrated(page);

let reloadRequests = 0;
await page.route(page.url(), async (route) => {
reloadRequests++;
Expand All @@ -118,20 +193,21 @@ test.describe("Chunk-load-error recovery", () => {
page,
}) => {
await loadPageWithDefaultBackend(page);
await awaitHydrated(page);

let reloadRequests = 0;
await page.route(page.url(), async (route) => {
reloadRequests++;
await route.abort();
});

await clearReloadGuard(page);
await dispatchChunkError(page);
await clearGuardAndDispatchChunkError(page);
await expect.poll(() => reloadRequests).toBe(1);

// The aborted reload never completed, so the same document/listeners are still live -
// dispatch a second chunk error and confirm the guard window suppresses a second attempt.
// (Deliberately no clearReloadGuard() call here - this second dispatch is exactly what's
// meant to hit the still-live guard from the first one above.)
// (Deliberately no guard-clear here - this second dispatch is exactly what's meant to hit
// the still-live guard from the first one above.)
await dispatchChunkError(page);
await page.waitForTimeout(1_000);
expect(reloadRequests).toBe(1);
Expand Down
Loading