From 8dbd917718a1a2f2dc373e505c3517da41c7001a Mon Sep 17 00:00:00 2001 From: John McLear Date: Fri, 15 May 2026 09:22:45 +0100 Subject: [PATCH] test(ci): kill Windows + Node 24 backend-test flake; capture native crashes (#7748) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(ci): kill Windows + Node 24 backend-test flake; capture native crashes The Backend tests suite has a ~22% silent-failure rate on Windows + Node 24 specifically (Linux 22/24/25 ✓, Windows 22/25 ✓). Two prior PRs instrumented the failure — common.ts handlers (#7663), then an unconditional diagnostics.ts (#7665) — and confirmed it's a hard kill: diagnostics.ts:23-27 documents the matrix, and every recurrence (run 25279692065, 25754938013, 25906496503) shows only `[diag +0ms] diagnostics loaded`, no beforeExit / exit / unhandledRejection / uncaughtException / signal handlers. Process dies 700–900 ms after the last passing test, in a varying spec each time. That's a native crash in V8 / libuv / the tsx loader, not anything reachable from JS. This PR ships two independent attacks at the failure: 1. Mitigation — add --exit to the mocha command in src/package.json. Mocha's default (--exit=false) waits for the event loop to drain after tests complete. The hard kill happens during that drain or the inter-spec transition. With --exit, mocha calls process.exit(failures) directly once the run finishes, closing the cleanup-race window. Linux/Windows-22/25 are green today, so the natural-drain path is not surfacing real leaks worth preserving. Verified locally: `cd src && pnpm test` -> 1121 passing, 0 failing, 23s. 2. Capture — set NODE_OPTIONS in each Backend tests step to --report-on-fatalerror, --report-uncaught-exception, --report-on-signal, --report-compact, plus --report-directory=${{ github.workspace }}/node-report. If Node crashes at the C++ level (segfault, V8 abort, libuv panic) the runtime writes a JSON diagnostic report with the V8 stack, libuv handle table, JS heap state, and OS info. A new "Upload Node diagnostic reports on failure" step (actions/upload-artifact@v7, `if: failure()`, `if-no-files-found: ignore`) uploads that directory as an artifact per matrix cell — the data we have been unable to capture from JS instrumentation alone. If (1) eliminates the flake on the next push to develop, great. If not, (2) finally gives us the crash dump and we can fix the root cause. Touches all four Backend tests jobs (Linux × 2, Windows × 2). The Windows steps now also set `shell: bash` so the same `mkdir -p ...` line works under git-bash; the existing `working-directory: src` is preserved. NODE_OPTIONS is scoped to the test step only, so the existing vitest step is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) * test(ci): address Qodo review on PR #7748 Three findings from Qodo's review of the Windows + Node 24 flake fix: 1. (bug) "mocha --exit masks handle leaks". --exit removes the post-suite event-loop drain that surfaces leaked timers / sockets — exactly the class of regression that lowerCasePadIds.ts notes is otherwise visible. Linux/local runs are currently green on natural drain, so dropping that signal everywhere would silence real leaks to fix a Windows-only flake. Fix: scope --exit to Windows only. - Remove --exit from src/package.json's "test" script (shared with local dev + Linux CI; both keep natural-drain behaviour). - Append `pnpm test -- --exit` in just the two Windows backend-test steps so the mitigation only runs where the flake actually lives. 2. (observability) "diagnostics exit-matrix misleading". With --exit on Windows, "only exit fires" becomes the EXPECTED pattern there, not a sign of unexpected process.exit(). Update the matrix comment in tests/backend/diagnostics.ts to spell out: clean drain on Linux/local → beforeExit + exit; Windows under --exit → only exit; "only exit" elsewhere still implies an unexpected process.exit somewhere. 3. (rule violation) "no regression test for the mitigation". Repo convention (see admin-i18n-source-lint.test.ts) is to pin policy-bearing config with a source-lint spec so a future refactor can't silently revert it. Add src/tests/backend-new/specs/backend-tests-flake-mitigation.test.ts: - Asserts every "Run the backend tests" step sets NODE_OPTIONS with the report-on-fatalerror diag flags AND is followed by an Upload Node diagnostic reports step (4 of each in the current matrix). - Asserts exactly 2 Windows jobs invoke `pnpm test -- --exit`. - Asserts the shared mocha "test" script in src/package.json does NOT bake in --exit globally. Verified locally: - cd src && pnpm exec vitest run tests/backend-new/specs/backend-tests-flake-mitigation.test.ts → 3 passed (3). - Stream.ts spec without --exit → 31 passing, diag prints "beforeExit code=0" + "exit code=0" (clean drain restored). - Existing admin-i18n-source-lint suite still passes. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/backend-tests.yml | 71 ++++++++++++++++-- .../backend-tests-flake-mitigation.test.ts | 72 +++++++++++++++++++ src/tests/backend/diagnostics.ts | 8 ++- 3 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 src/tests/backend-new/specs/backend-tests-flake-mitigation.test.ts diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 96fed8321..a4bfed79e 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -66,7 +66,24 @@ jobs: run: pnpm build - name: Run the backend tests - run: pnpm test + env: + # --report-on-fatalerror and friends write a Node diagnostic report + # (V8 stack, libuv handles, OS info) on fatal errors that bypass JS + # handlers — the failure mode we've been chasing on Windows + Node + # 24 since PR #7663. Reports land in node-report/ and are uploaded + # as an artifact if the step fails. + NODE_OPTIONS: "--report-on-fatalerror --report-uncaught-exception --report-on-signal --report-compact --report-directory=${{ github.workspace }}/node-report" + run: | + mkdir -p "${{ github.workspace }}/node-report" + pnpm test + - name: Upload Node diagnostic reports on failure + if: ${{ failure() }} + uses: actions/upload-artifact@v7 + with: + name: node-diagnostic-report-${{ runner.os }}-node${{ matrix.node }}-${{ github.job }} + path: node-report/ + if-no-files-found: ignore + retention-days: 7 - name: Run the new vitest tests working-directory: src run: pnpm run test:vitest @@ -136,7 +153,19 @@ jobs: ep_table_of_contents - name: Run the backend tests - run: pnpm test + env: + NODE_OPTIONS: "--report-on-fatalerror --report-uncaught-exception --report-on-signal --report-compact --report-directory=${{ github.workspace }}/node-report" + run: | + mkdir -p "${{ github.workspace }}/node-report" + pnpm test + - name: Upload Node diagnostic reports on failure + if: ${{ failure() }} + uses: actions/upload-artifact@v7 + with: + name: node-diagnostic-report-${{ runner.os }}-node${{ matrix.node }}-${{ github.job }} + path: node-report/ + if-no-files-found: ignore + retention-days: 7 - name: Run the new vitest tests working-directory: src run: pnpm run test:vitest @@ -186,8 +215,25 @@ jobs: powershell -Command "(gc settings.json.holder) -replace '\"points\": 10', '\"points\": 1000' | Out-File -encoding ASCII settings.json" - name: Run the backend tests + shell: bash working-directory: src - run: pnpm test + env: + NODE_OPTIONS: "--report-on-fatalerror --report-uncaught-exception --report-on-signal --report-compact --report-directory=${{ github.workspace }}/node-report" + run: | + mkdir -p "${{ github.workspace }}/node-report" + # --exit forces process.exit(failures) after the suite completes, + # closing the post-suite event-loop drain window where Windows + + # Node 24 hard-kills the process. Scoped to Windows so Linux/local + # runs still surface real handle leaks via natural drain. + pnpm test -- --exit + - name: Upload Node diagnostic reports on failure + if: ${{ failure() }} + uses: actions/upload-artifact@v7 + with: + name: node-diagnostic-report-${{ runner.os }}-node${{ matrix.node }}-${{ github.job }} + path: node-report/ + if-no-files-found: ignore + retention-days: 7 - name: Run the new vitest tests working-directory: src run: pnpm run test:vitest @@ -265,8 +311,25 @@ jobs: powershell -Command "(gc settings.json.holder) -replace '\"points\": 10', '\"points\": 1000' | Out-File -encoding ASCII settings.json" - name: Run the backend tests + shell: bash working-directory: src - run: pnpm test + env: + NODE_OPTIONS: "--report-on-fatalerror --report-uncaught-exception --report-on-signal --report-compact --report-directory=${{ github.workspace }}/node-report" + run: | + mkdir -p "${{ github.workspace }}/node-report" + # --exit forces process.exit(failures) after the suite completes, + # closing the post-suite event-loop drain window where Windows + + # Node 24 hard-kills the process. Scoped to Windows so Linux/local + # runs still surface real handle leaks via natural drain. + pnpm test -- --exit + - name: Upload Node diagnostic reports on failure + if: ${{ failure() }} + uses: actions/upload-artifact@v7 + with: + name: node-diagnostic-report-${{ runner.os }}-node${{ matrix.node }}-${{ github.job }} + path: node-report/ + if-no-files-found: ignore + retention-days: 7 - name: Run the new vitest tests working-directory: src run: pnpm run test:vitest diff --git a/src/tests/backend-new/specs/backend-tests-flake-mitigation.test.ts b/src/tests/backend-new/specs/backend-tests-flake-mitigation.test.ts new file mode 100644 index 000000000..964e752cb --- /dev/null +++ b/src/tests/backend-new/specs/backend-tests-flake-mitigation.test.ts @@ -0,0 +1,72 @@ +'use strict'; + +// Source-level lint pinning the Windows + Node 24 backend-test flake +// mitigations from PR #7748. Two independent attacks at the failure: +// +// 1. Mocha --exit on the Windows CI jobs so the post-suite event-loop +// drain — where Windows + Node 24 hard-kills the process — never +// executes. Scoped to Windows so Linux/local runs still surface +// real handle leaks via natural drain. +// 2. NODE_OPTIONS=--report-on-fatalerror (and friends) on every +// Backend tests step, with the resulting node-report/ directory +// uploaded as an artifact on failure. If the flake recurs we +// finally get a V8 stack + libuv handle table. +// +// Both pieces are easy to silently revert in a workflow refactor; this +// test fails fast if either disappears. + +import {readFileSync} from 'fs'; +import {join} from 'path'; +import {describe, it, expect} from 'vitest'; + +const repoRoot = join(__dirname, '..', '..', '..', '..'); +const read = (rel: string) => readFileSync(join(repoRoot, rel), 'utf8'); + +const workflow = read('.github/workflows/backend-tests.yml'); + +describe('backend-tests flake mitigation (PR #7748)', () => { + it('every Backend tests step exposes Node diagnostic reports via NODE_OPTIONS', () => { + // Count the "Run the backend tests" steps so the expected-count is + // explicit — if a job is added later, this test reminds the author + // to wire the diag flags into it too. + const runStepCount = (workflow.match(/name: Run the backend tests/g) || []).length; + expect(runStepCount, 'expected 4 Backend tests step blocks (Linux × 2, Windows × 2)') + .toBe(4); + const nodeOptionsCount = (workflow.match( + /--report-on-fatalerror --report-uncaught-exception --report-on-signal --report-compact/g, + ) || []).length; + expect(nodeOptionsCount, + 'every Backend tests step must set NODE_OPTIONS with the report-on-fatalerror diag flags') + .toBe(runStepCount); + const uploadCount = (workflow.match(/name: Upload Node diagnostic reports on failure/g) || []) + .length; + expect(uploadCount, + 'every Backend tests step must be followed by an Upload Node diagnostic reports step') + .toBe(runStepCount); + }); + + it('Windows backend-test steps invoke pnpm test with --exit', () => { + // --exit is the Windows-only mitigation. Linux still runs natural-drain + // so leaked-handle regressions stay visible there. + const exitCount = (workflow.match(/pnpm test -- --exit/g) || []).length; + expect(exitCount, 'Windows × 2 jobs must pass --exit to pnpm test') + .toBe(2); + // Negative check: Linux jobs must NOT use --exit so handle-leak + // detection stays alive on the natural-drain platforms. + expect(workflow.includes('runs-on: ubuntu-latest'), + 'workflow no longer has any Linux jobs (sanity check)').toBe(true); + }); + + it('mocha test script does not bake --exit in globally', () => { + // Counterpart to the workflow check: if a future refactor moves + // --exit back into src/package.json it would silently apply to + // Linux + local runs too, masking handle leaks. Keep --exit out of + // the shared script. + const pkg = JSON.parse(read('src/package.json')) as { + scripts: Record, + }; + expect(pkg.scripts.test, + 'mocha test script must not include --exit — apply --exit per-platform in CI') + .not.toMatch(/(^|\s)--exit(\s|$)/); + }); +}); diff --git a/src/tests/backend/diagnostics.ts b/src/tests/backend/diagnostics.ts index ac29ab198..67e4f3624 100644 --- a/src/tests/backend/diagnostics.ts +++ b/src/tests/backend/diagnostics.ts @@ -21,8 +21,12 @@ // 3. Tracks the last-seen test via a mocha root afterEach hook so the // death point is identified. // 4. Logs exit-related events so we can discriminate: -// beforeExit + exit -> clean event-loop drain -// only exit -> process.exit() called somewhere +// beforeExit + exit -> clean event-loop drain (Linux CI, local) +// only exit -> process.exit() called — expected when mocha +// is launched with --exit (the Windows CI +// jobs do this to mitigate a hard-kill flake; +// elsewhere "only exit" still means something +// else called process.exit unexpectedly) // neither -> hard kill (SIGKILL/OOM/runner) // signal lines -> SIGTERM / SIGINT / SIGBREAK received //