Repository navigation
fix(dashboard): stabilise the vitest suite — pool starvation, a detached-node race, and a DOMMatrix collection crash - #23
Merged
Conversation
…hed-node race, and a DOMMatrix collection crash
The reported symptom was ONE intermittently failing test. Root-causing it found
FOUR defects across three causes. Full diagnosis:
.agents/audit/vitest-intermittency.md
## 1. Worker-pool starvation (the actual cause of the intermittency)
`pool: "threads"` opens up to cpus-1 workers - 7 on an 8-core machine. Under that
contention, timing-sensitive assertions failed in files that pass BOTH alone and
in small groups. Bound the pool and the suite goes green AND gets faster:
default pool -> 2 failed files / 10 failed tests / 597s
maxWorkers: 3 -> 253 passed / 1331 passed / 277s
Faster because the run is dominated by setup/environment time, not parallel
throughput: the workers were contending, not scaling.
## 2. A detached-node race in AuthGuard.test.tsx
Reported as flaky; it was deterministically broken under parallelism. The test
took an instantaneous snapshot - `await screen.findByText(...)` then
`.toBeInTheDocument()` - of a node it EXPECTS to churn (NavProbe navigates on
every render, so the shell is briefly replaced by the gate spinner). When that
landed in the window, findByText resolved, the node detached, and the matcher
failed in ~25ms.
That duration is what disproved my first hypothesis. findByText cannot resolve
early with a failure - @testing-library/dom retries until its deadline and then
reports "Timed out in waitFor" - so a 25ms failure meant the element WAS found and
then removed. My initial fix (raising the timeout to 5000ms) still flaked 1 in 3,
which is what sent me back to the timing evidence. Replaced with a `waitFor` that
re-queries, which is also what the test already did two lines below.
Mutation-verified: original form fails 8/8, waitFor form passes 8/8.
## 3. DOMMatrix missing, breaking whole files at COLLECTION
pdfjs-dist instantiates a transform at module scope
(node_modules/pdfjs-dist/build/pdf.mjs:9387, `const SCALE_MATRIX = new DOMMatrix()`).
jsdom ships no DOMMatrix, so merely importing react-pdf throws during collection -
reported as a whole-file failure with "Tests: no tests", so the real tests never
run at all.
This reached far past the PDF components: a plain .ts test with no DOM
(pages/Agent/Skills/skillMarkdown.test.ts) died because it imports a helper from
components/SkillDrawer, which renders react-pdf. Same for AuthGuard.test.tsx and
Settings/Models/ModelListEditor.test.tsx.
Stub added to src/test/setup.ts beside the existing matchMedia / ResizeObserver /
IntersectionObserver shims - that is what the file is for. Deliberately minimal:
the module-scope constructor is the no-arg form and only .a/.d are read (both
identity 1). A full transform implementation would be untested scaffolding.
Mutation-verified: removing it restores the ReferenceError.
## 4. A lazy-import latency vs waitFor's 1000ms default
TrajectoryInspector renders Preview through LazyMarkdown (React.lazy ->
import("./index")). A cold import of that chunk measures 2086ms; waitFor's default
is 1000ms (asyncUtilTimeout, @testing-library/dom config.js:15). The assertion
therefore could only pass when another file in the worker had already warmed the
chunk. Given an explicit 5000ms budget.
Proven causally with an artificial 2.2s import delay: the unpatched assertion
fails at 1140ms, the patched one passes.
## Rejected: raising testTimeout to 15000
Tried and reverted. The flagging failures were starved, not slow, and #2 is a race
no budget can fix. A generous timeout turns a hard failure into a slow pass
without removing the race - worse for diagnosis, not better.
## Verification
cd dashboard && npx tsc -b # clean
cd dashboard && npx vitest run # 253 files / 1331 tests, 277s
Baseline on pristine develop for comparison: 4 failed files / 1 failed test, and
three files that could not even collect.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The reported symptom was one flaky test. Root-causing it found four defects across three causes.
Full write-up in
.agents/audit/vitest-intermittency.md.develop1. Worker-pool starvation — the actual cause of the intermittency
pool: "threads"opens up tocpus - 1workers, i.e. 7 on this 8-core machine. Under that contention, timing-sensitive assertions failed in files that pass both alone and in small groups. Bounding tomaxWorkers: 3fixes it outright, and the suite gets ~2× faster (597 s → 277 s), because the run is dominated by setup/environment cost rather than parallel throughput — the workers were contending, not scaling.2. A detached-node race in
AuthGuard.test.tsxReported as flaky; it was deterministically broken under parallelism. The test took an instantaneous snapshot —
await screen.findByText(...)then.toBeInTheDocument()— of a node it expects to churn.NavProbenavigates on every render, so the shell is briefly replaced by the gate's spinner. When that lands in the window,findByTextresolves, the node detaches, and the matcher fails in ~25 ms.That duration is what disproved my own first hypothesis:
findByTextcannot resolve early with a failure (@testing-library/domretries until its deadline and reports "Timed out in waitFor"), so a 25 ms failure meant the element was found, then removed. My initial fix — raising the timeout to 5000 ms — still flaked 1 run in 3, which is what sent me back to the timing evidence. Replaced with awaitForthat re-queries.Mutation-verified: the original form fails 8/8; the
waitForform passes 8/8. The test already contained the correct assertion two lines below; the snapshot was both racy and redundant.3.
DOMMatrixmissing — whole files dying at collectionpdfjs-distinstantiates a transform at module scope:jsdom ships no
DOMMatrix, so merely importing react-pdf throws during collection. Vitest reports that as a whole-file failure with "Tests: no tests" — the real tests never run, and it reads like an infra error rather than a broken test.This reached far past the PDF components: a plain
.tstest with no DOM (pages/Agent/Skills/skillMarkdown.test.ts) died because it imports a helper fromcomponents/SkillDrawer, which renders react-pdf. Same forAuthGuard.test.tsxandSettings/Models/ModelListEditor.test.tsx. Three files could not have passed before this PR, whatever the timing.The stub sits in
src/test/setup.tsbeside the existingmatchMedia/ResizeObserver/IntersectionObservershims — that is what the file is for. Deliberately minimal: the module-scope constructor is the no-arg form and only.a/.dare read from it (both identity1). A full transform implementation would be untested scaffolding.4. Lazy-import latency vs
waitFor's 1000 ms defaultTrajectoryInspectorrenders the Preview tab viaLazyMarkdown(React.lazy→import("./index")). A cold import of that chunk measures 2086 ms;waitFor's default is 1000 ms (asyncUtilTimeout,@testing-library/dom/dist/config.js:15). The assertion could therefore only pass when another file in the same worker had already warmed the chunk — which is exactly why it looked order-dependent.Proven causally with an artificial 2.2 s import delay: the unpatched assertion fails at 1140 ms, the patched one passes.
Rejected: raising
testTimeoutto 15000I tried this and reverted it. The flagging failures were starved, not slow, and #2 is a race no budget can fix. A generous timeout turns a hard failure into a slow pass without removing the race — worse for diagnosis, not better. Recorded in the write-up so it is not re-tried.
The honest intermediate
After fixes 2–4 but before bounding the pool, the suite looked worse: 2 failed files / 10 failed tests (vs baseline's 4 / 1), 597 s. That is the real shape of this bug — fixing the collection crashes made more files actually execute, which added load and surfaced more starvation-sensitive failures that had been hidden behind them.
Verification
Verified via config alone (no CLI flags), since that is what CI would use.
Out of scope, flagged not fixed
Neither CI nor
make allruns vitest at all —make allisformat-all + lint + typecheck + test, andtestis pytest only, whiledashboard/package.jsonhas an uninvoked"test": "vitest run". So this suite is invisible to the ship bar. These fixes only hold ongoing if something actually runs them — but wiring a ~5-minute frontend suite into every commit is a ship-bar decision, not a bug fix, so I have left it for you to call rather than expanding scope unilaterally.