From 72ea534073ff4062e2c257015824533c6698c771 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 23 Jul 2026 21:51:27 +0000 Subject: [PATCH] Fix remaining chunkErrorRecovery.spec.ts hydration/guard races under CI shard contention --- docs/troubleshooting.md | 88 +++++++++---- frontend/tests/chunkErrorRecovery.spec.ts | 146 ++++++++++++++++------ 2 files changed, 177 insertions(+), 57 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 1c0c6514e..ad2dba99b 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -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 @@ -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. diff --git a/frontend/tests/chunkErrorRecovery.spec.ts b/frontend/tests/chunkErrorRecovery.spec.ts index 02496f93d..d9d4e2e7f 100644 --- a/frontend/tests/chunkErrorRecovery.spec.ts +++ b/frontend/tests/chunkErrorRecovery.spec.ts @@ -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); }); @@ -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++; @@ -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);