Unit & Component Tests job exits 1 from vitest-browser teardown race — every test green but CI red #535
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What the user sees
Every push to a branch with frontend changes turns the
Unit & Component Testsjob red even though every test in the run reports green. The failure surfaces only at the very end of theRun unit and component tests with coveragestep as an Unhandled Rejection, not as an assertion failure.First observed run: https://git.raddatz.cloud/marcel/familienarchiv/actions/runs/1520/jobs/0 (job id 4344).
Failing job — exact symptom
Tail of the failing step (ANSI stripped):
Root cause (what the trace actually says)
The coverage step runs
vitest run -c vitest.client-coverage.config.ts --coverage, which uses the browser project powered by@vitest/browser-playwright(Chromium headless). In this mode:vi.mock('module', () => ({ … }))factories register asManualMockedModuleinstances.resolveManualMock) to evaluate the factory and return the module body.[birpc] rpc is closed, cannot call "resolveManualMock"→ unhandled rejection → process exit 1.The hint vitest prints ("top-level variables inside the factory") is misleading for browser mode. The actual cause is a teardown race between the worker shutdown and a pending playwright route.
Timing — pinpoints the offending spec
PersonMentionEditor.svelte.spec.ts(22 tests, 8755 ms — slowest in the run, last to finish)PersonMentionEditor.svelte.spec.ts(src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts) is the prime suspect:/api/persons?q=…on every keystroke (debounced),vitest-browser-svelte+vitest/browserand usesvito spy/mock.The likely sequence: the test's last
userEvent.typetriggers a debounced fetch, the test ends, the worker begins teardown, the debounced fetch resolves against a mocked module after birpc is gone → boom.What is noise (do not be distracted by these in the log)
TypeError: Cannot read properties of undefined (reading 'wrapDynamicImport')atget_hooks (.svelte-kit/generated/server/internal.js:37). This is SvelteKit's dev runtime trying to loadhooks.server.tswhile the test page is being navigated. It is stderr only, does not fail any test, and is unrelated to this issue.Failed to load transcription blocks: [Error: network]etc. — these are intentional negative-path log lines from passing tests.Error loading data: TypeError: Cannot read properties of undefined (reading 'response')frompage.server.spec.ts— intentional, the test asserts the fallback path.Repro plan
feat/prod-import-bind-mount(or whichever branch shows the same redUnit & Component Testsjob).frontend/:vi.mock(..., () => …)factories in browser tests — collect with:Acceptance criteria
Unit & Component Testsjob is green on at least 20 consecutive CI runs of an untouched branch.npx vitest run -c vitest.client-coverage.config.ts --coverageis green on at least 10 consecutive runs.[birpc] rpc is closed, cannot call "resolveManualMock"line appears in any of those runs.Candidate fix paths (in order of cheapest → most invasive)
1. Make the suspect spec await all in-flight work before exiting
In
PersonMentionEditor.svelte.spec.ts, ensure every test:/api/persons?q=…fetch (e.g. viaawait expect.poll(...)orawait waitFor(...)),cleanup()fromvitest-browser-svelteinafterEach(already imported — verify it actually runs and the TipTap editor'sdestroy()is invoked),This is the highest-likelihood, lowest-risk fix.
2. Bump the vitest-browser stack
Upstream has landed several teardown-race fixes in
@vitest/browser,@vitest/browser-playwright, and@vitest/mocker. Bump all four together (vitest,@vitest/browser,@vitest/browser-playwright,@vitest/mocker) to the latest patch, re-run the coverage config 10× locally.3. Catch the unhandled rejection in vitest config
Last-resort mitigation only. Adds
dangerouslyIgnoreUnhandledErrors: trueor aonUnhandledErrorfilter that swallows the specific[birpc] rpc is closedrejection. Do not ship this without 1 or 2 — it hides real failures.4. Replace
vi.mock(...)factories in the slowest browser tests with vitest-browser-svelte's render-time prop injectionFor the typeahead test specifically, you can avoid
vi.mockby providing apersonSearchprop toPersonMentionEditorHostand feeding canned results from the test. This sidesteps the ManualMockedModule code path entirely.Out of scope (file separately if you want them)
actions/upload-artifact@v4failure (GHESNotSupportedError) on the same run is a real but unrelated CI gap. It currently produces❌ Failure - Main Upload coverage reports / exitcode '1': failureafter the test step has already failed. Even if tests are green, this step will fail on Gitea Actions until pinned to v3 or replaced with a Gitea-native artifact action. → suggest a separatedevopsissue.wrapDynamicImportstderr spam is cosmetic but could be cleaned up by gating the dev server'sget_hooksin tests.References
node_modules/@vitest/mocker/dist/chunk-registry.js:189node_modules/@vitest/browser-playwright/dist/index.js:977👨💻 Felix Brandt — Senior Fullstack Developer
Observations
PersonMentionEditor.svelte.spec.tsusesvi.stubGlobal('fetch', …)— notvi.mock(MOD, factory). The unhandled rejection trace specifically namesManualMockedModule.factory/resolveManualMock, which only fires forvi.mock(MOD, factory). The offending mock factory is therefore in a different spec; this spec is just the one that happens to be running when birpc closes.afterEachalready callscleanup()+vi.unstubAllGlobals(), and the unmount chain looks right: Svelte unmount →onDestroy(PersonMentionEditor.svelte:255-257) →editor.destroy()→ suggestion plugin'sonExit→unmount(MentionDropdown). So path 1 in this spec alone won't fix the bug.mount()-ed todocument.body(PersonMentionEditor.svelte:194-200), outside the vitest-browser-svelte container.cleanup()doesn't reach it; onlyeditor.destroy()does. That chain is intact, but worth holding onto if path 4 ever touches the dropdown mount path.Recommendations
vi.mock(MOD, factory)in browser specs (I count ~25 sites:pdfjs-dist,$app/navigation,$app/forms,$app/state,$app/stores,$lib/paraglide/runtime,$env/static/public). The culprit is whichever module is lazy-loaded (dynamic import, async chunk) such that Chromium fetches it after birpc has torn down. The slow spec just widens the race window; it is not the source.PdfViewer.svelte.spec.ts, which mockspdfjs-dist+ the?urlworker import; that is exactly the kind of late-resolving import the trace points at).vi.mockfactory synchronous and closure-free —vi.mock('mod', () => ({ default: 0 }))with novi.fn(), no top-level vars, no async. Static factories don't round-trip through birpc on resolution.*.test-host.sveltepattern in this spec is exactly the right injection seam — extend it where path 4 lands. Tests should bind props, not mock modules.dangerouslyIgnoreUnhandledErrors: true) without 1 or 2. It permanently silences a whole class of failures including real component bugs.Open Decisions (none)
🏛️ Markus Keller — Senior Application Architect
Observations
vi.mock(MOD, factory)sites across browser specs. In browser mode each one is a manual-mock served via a playwright route handler that calls Node over birpc — effectively an intra-process network call. Teardown surface area grows linearly with the count; this race is not a one-off, it's the predictable outcome of the chosen test-mocking style.PersonMentionEditorHost/*.test-host.sveltepattern is exactly the architectural seam tests should use: thread the dependencies the test cares about as props, isolate the SUT, avoid module-level mocking. It's currently used in two or three places. It should be the default, not the exception.PersonMentionEditor.svelte:127-140documents why client-side fetch is acceptable for the suggestion plugin and references an open ADR follow-up. Good discipline. The same discipline is missing from the test-mocking strategy — there is no ADR explaining whenvi.mockis appropriate vs when prop injection is.Recommendations
vi.mock(MOD, factory)in browser specs as architectural debt with a clear migration target (prop injection via test-host). Path 4 in the issue body is the right category, not just one fix. Apply it project-wide, not only toPersonMentionEditor.vi.stubGlobalovervi.mock(MOD, factory)." Capture the why (birpc teardown race, lazy-resolved mock factories), the seam (*.test-host.svelte), and the residual exceptions (e.g.$app/navigationmay need avi.mocksince SvelteKit doesn't expose a DI seam there).pdfjs-distand$app/*mocks: those are framework boundaries where prop injection isn't feasible. Audit and minimise everything else.Open Decisions (none)
🛠️ Tobias Wendt — DevOps & Platform Engineer
Observations
actions/upload-artifact@v4(.gitea/workflows/ci.yml:53,87). The "pinned to v3 or replaced with a Gitea-native artifact action" framing in the issue body is half-right: downgrading to v3 is the wrong direction (deprecated, security patches stopped). The right move is the Gitea-native artifact action.mcr.microsoft.com/playwright:v1.58.2-noble).node_modulescache is keyed bypackage-lock.jsonhash. The pipeline shape is sound — failure is in the test harness, not the runner.npm run test:coverageat line 46) runs the full browser-mode suite a second time. Two passes through the race window per CI run roughly doubles the probability of tripping it. Worth knowing while debugging — if the unit-test step is green and only the coverage step is red, the race is order-dependent in a way the slower run exposes.Recommendations
devopsissue per the body): swapactions/upload-artifact@v4forhttps://gitea.com/actions/upload-artifact@v4(Gitea's port that speaks the v1 artifact protocol Gitea Actions implements). Do not downgrade to@v3— it's deprecated.[birpc] rpc is closedappears anywhere in the test-step log. That makes any future regression visible even if some runner change swallows the exit code:npm run test:coverage10× per the repro plan and trigger the CI workflow 20+ times against an untouched branch (see Elicit's clarification request on what "untouched branch" means in practice).Open Decisions (none)
🔒 Nora Steiner — Application Security Engineer
Observations
dangerouslyIgnoreUnhandledErrors: true, or anyonUnhandledErrorfilter) would suppress every unhandled rejection in browser-mode tests, not just[birpc] rpc is closed. Unhandled rejections in tests are also how leaked promises from auth handlers, swallowed sanitisation errors, and post-test-teardown DOM mutations surface. A blanket silencer is a permanent defense-in-depth regression.PersonMentionEditor.svelte:138-139already references an open security follow-up (Nora #5618 #3) for theGET /api/personsresponse-shape audit —PersonSummaryDTOmay leaknotes. That is not this issue, but the fix here must not collide with that work, and the existingXSS resistancedescribe block (PersonMentionEditor.svelte.spec.ts:345-374) must keep passing through whichever fix path is taken.Recommendations
rpc is closed/resolveManualMockonly, reviewed in PR, with a code comment explaining the threat model (perReadable & Clean Code §1). A blanket flag is unacceptable.No "[birpc] rpc is closed" line appears in any of those runs— that is exactly the right shape. Back it with a permanent CI grep guard (per Tobias). That guard also doubles as a security smoke test: a future leaked promise that happens to print "rpc is closed" anywhere fails the build.displayNamecontaining<img src=x onerror=alert(1)>round-trips through ProseMirror's DOMSerializer as text, not HTML. Re-wire the test against the new injection seam, do not delete or weaken it.PersonMentionEditor.svelte's/api/personscallsite — that work may move the fetch out of the suggestion plugin entirely, which would resolve a different attack surface at the same time.Open Decisions (none)
🧪 Sara Holt — Senior QA Engineer
Observations
green on at least 20 consecutive CI runsis testable but statistically weak. If the residual flake rate after the fix is 5%, P(20 green | 5% fail) ≈ 0.36 — passing the gate is consistent with shipping the bug. For 95% confidence the flake rate is < 5%, you want closer to 60 consecutive green runs.afterEachhere already runscleanup()+vi.unstubAllGlobals(). Hygiene is fine. The trace'sresolveManualMock/ManualMockedModule.factorylines mean the offender is avi.mock(MOD, factory)somewhere else — and there are ~25 such sites across the browser specs.vitest.client-coverage.config.ts:43-48) only applies if the suite exits 0. Every teardown-race exit-1 today silently skips the coverage delta check — so a regression that drops coverage could ride alongside the race without being noticed.aria-disabled,min-h-[44px]on options, ARIA roles, XSS resistance) are exactly the load-bearing checks that mustn't regress through any fix path.Recommendations
npm run test:coverageruns, both with zero[birpc] rpc is closed. 20 is too few for a known-flaky path; 30 is a cheap-but-meaningful uplift. (See open decision for the trade.)globalSetuplistener that registers anunhandledrejectionhandler and fails the run if the rejection message containsrpc is closedorresolveManualMock. Then the bug is caught at the layer it actually lives in, not via a side-channel exit code.--sequence.shuffleand--no-isolatelocally as part of investigation. If the symptom moves with shuffle, the fix has to touch the racing mock, notPersonMentionEditor.svelte.spec.ts. If it stays put, the test order is the trigger and isolation is the durable fix.it.skip) this spec to make CI green. A skipped flake corrodes trust in the suite faster than a red one.Open Decisions
🎨 Leonie Voss — UX & Accessibility Lead
No UX or accessibility concerns from my angle — this is a test-infrastructure bug with no user-visible surface.
I did verify that the load-bearing a11y tests in
PersonMentionEditor.svelte.spec.ts(contenteditable=false on disable,aria-disabled=trueon the wrapper,min-h-[44px]on every result row per WCAG 2.2 AA, ARIA rolestextbox/option/listbox, and the XSS-resistance test fordisplayNamerendering) survive each of the four candidate fix paths intact. None of the paths require touching them. If path 4 (prop injection) is taken, please rewire those tests against the new host seam rather than dropping them — they are the regression net that keeps the touch target and contrast guarantees from rotting silently.📋 Elicit — Requirements Engineer
Observations
16:18:37.876 → 16:18:38.110), four prioritised fix paths with explicit cost ordering, an "Out of scope" section that separates concerns cleanly, and four measurable acceptance criteria. Labels (P1-high,bug,devops,test) are appropriate. No backlog-hygiene issues.Root cause is documented in the closing commit message — which fix path of the four below was applied, and why) is unusual but correct for an investigative bug. It prevents the lossy "fixed in #535" outcome where the fix is opaque six months later.20 consecutive CI runs of an untouched branch— which untouched branch and what's the trigger? A no-op push tomain? Aworkflow_dispatchagainstfeat/prod-import-bind-mount? A push to a fresh branch off ofmainpost-fix? Testability requires a single, unambiguous trigger.Recommendations
workflow_dispatchruns of theCIworkflow againstmainafter the fix is merged, all green, with[birpc] rpc is closedabsent from every log." Pair with Sara's recommendation to lift 20 → 30 if you want a tighter statistical guarantee.devopsissue for the Gitea-Actions /actions/upload-artifact@v4mismatch now — it is independently actionable, P2 at minimum, and waiting for this fix to land first only obscures whether the coverage upload works on green runs. Cross-link from here.wrapDynamicImportstderr noise and the "intentional negative-path log lines" are correctly scoped as out-of-scope/noise. Resist scope creep into them during this fix; both can become their own low-priority hygiene issues if anyone has time later.Open Decisions (none)
🗳️ Decision Queue — Action Required
1 decision needs your input before implementation starts.
Quality Gates
Sample size for the green-run acceptance criterion. The current AC requires 20 consecutive green CI runs. That gate is consistent with a residual flake rate of ~15% at 95% confidence — passing it does not strongly prove the fix held. Options:
(Raised by: Sara, seconded by Elicit who also asks you to specify which trigger counts —
workflow_dispatchagainstmainpost-merge is the recommended unambiguous form.)60 runs
✅ Implementation complete — branch
feat/issue-535-birpc-teardown-raceWhat was done
Root cause confirmed:
PdfViewer.svelte.spec.tsregistered twovi.mock(module, factory)calls —pdfjs-distandpdfjs-dist/build/pdf.worker.min.mjs?url. In vitest browser mode, each factory becomes aManualMockedModuleserved via a playwright route handler over birpc. SinceusePdfRenderer.svelte.ts::init()loads both modules via dynamicimport()insideonMount, Chromium can request the module after the birpc worker channel has closed →[birpc] rpc is closed, cannot call "resolveManualMock"→ unhandled rejection →exit 1.Fix applied: Path 4 (prop injection) — 4 commits
49171e59libLoaderparam tocreatePdfRenderer()+ failing test (red)7a4295d4libLoaderprop toPdfViewer.svelte; remove bothvi.mockcalls from spec; pass fake loader via prop (green)67f53fccrpc is closedappearsb9e2ed4aWhy this fixes the race
The two
ManualMockedModuleregistrations forpdfjs-distare never created. With no registered factory, the playwright route handler is never installed, so there is nothing for birpc to serve during teardown. The race condition is structurally impossible.All other
vi.mocksites in browser specs ($app/*,$env/*) mock modules loaded statically at spec module-eval time — these are not race candidates and are unchanged.Acceptance criterion
User decision: 60 consecutive green
workflow_dispatchCI runs againstmainafter merge, with zerorpc is closedlines in any log. The new CI guard step will fail the build if the pattern ever reappears.60-run quality gate — operationalization
Responding to Elicit's question in PR #536 round-4 review about how the 60-run gate works in practice.
Proposed answers:
workflow_dispatchonmain, or any push/PR trigger countsmainafter the merge counts toward the 60[birpc] rpc is closedline in any log resets the count.mainwith zero[birpc] rpc is closedlines in any coverage log. Issue closes when that milestone is reached.The CI artifact
coverage-test-<run_id>.logis uploaded for every run, making it straightforward to audit.I'll close this issue once 60 clean runs are accumulated. No dedicated tracking issue needed — the existing birpc guard step is the measurement instrument.