From 8f04c9a526b3542e71141139f58419485e09946b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 14:52:39 +0900 Subject: [PATCH 1/2] fix(ci): give the Windows leg the budgets and the crash retry it never had Six shard failures in three groups (#2152). None came from main..dev; all three needed a different answer, and none of them was skipping a test that can actually run. Group 1, budgets. watchdogMs is a FLOOR, not a multiplier, so a case calling watchdogMs(30_000) still got exactly 30s -- 'Restore truth' failed at 30,147ms. Windows CI now floors at 45s, under the lane's own 60s per-test timeout so a hung test stays bounded. 'A-reduced' was misread in the issue: its 79,978ms was elapsed time against a 150s ceiling, so the outer budget was never the constraint. The real failure was Fixture.request's unscaled 10s AbortSignal, which aborted the case from inside. It is scaled now like every neighbouring budget. 'E' does not start ocx at all. Its lock holder released after a fixed 3s busy wait, and on a Windows shard the contender's process spawn can outlast that -- the parent then sees 'acquired' where it demands 'busy', which reads as a broken exclusion invariant rather than a hold that expired early. The release-marker handshake still ends the hold early everywhere else; only the ceiling moved. Group 2, skip guard. The issue says an unprivileged Windows user cannot create symlinks, but the GitHub runner can -- so canSymlink was true, the cases ran, and they failed on how the preflight reads mode and access through a Windows symlink. Two neighbouring cases in the same file already skip on process.platform === "win32"; these three now use that same guard, and keep the capability check for unprivileged POSIX. Group 3, crash retry. A Bun panic is a crash in the interpreter, not a test result. The macOS leg has carried a crash-signature retry for this; the Windows shards, a separate matrix job with their own one-shot command, had none. They now use the same wrapper, extended with panic(thread since that is the signature this leg actually printed. An assertion failure returns its status immediately and is never retried. What this cannot prove locally: whether 45s is sufficient under real Windows shard contention, the actual skip result on the runner, and PIPESTATUS behavior in Git Bash. Those need a Windows CI dispatch, which is the evidence to look for on this PR. --- .github/workflows/ci.yml | 26 +++++++++++++++++++++++- tests/codex-composed-acceptance.test.ts | 19 +++++++++++++++-- tests/helpers/ci-watchdog.ts | 9 +++++++- tests/helpers/codex-write-lock-child.ts | 8 +++++++- tests/update-npm-cache-preflight.test.ts | 20 +++++++++++++++--- 5 files changed, 74 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02344a0aa2..5872136ecf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -611,7 +611,31 @@ jobs: # the only one left on Bun's 5s default, and it is the slowest hardware on the board. # Three of its failures were the default firing on tests that had not hung — the # composed-acceptance cases spawn a real `ocx start` and were still working at 41s. - run: bun test --isolate --timeout 60000 tests --shard=${{ matrix.shard }}/4 + # + # The retry is the same one the macOS leg already carries, for the same reason: a Bun + # runtime panic is a crash in the interpreter, not a test result, and failing the shard + # on it reports a defect this repository does not have (#2152). An ordinary assertion + # failure returns its status immediately — only the crash signatures below are retried, + # and only once, so a genuinely broken build cannot be retried into green. + shell: bash + run: | + set +e + set -uo pipefail + suite_log="$(mktemp -t ocx-windows-suite.XXXXXX)" + for attempt in 1 2; do + bun test --isolate --timeout 60000 tests --shard=${{ matrix.shard }}/4 2>&1 | tee "$suite_log" + suite_status="${PIPESTATUS[0]}" + if [ "$suite_status" -eq 0 ]; then + exit 0 + fi + if ! grep -Eqi 'oh no: Bun has crashed|Segmentation fault at address|Illegal instruction|Bus error|panic\(thread' "$suite_log"; then + echo "::error::Windows shard ${{ matrix.shard }}/4 failed on attempt ${attempt} (exit ${suite_status}); assertion failures are not retried." + exit "$suite_status" + fi + echo "::warning::Bun runtime crash in Windows shard ${{ matrix.shard }}/4 (exit ${suite_status}, attempt ${attempt})." + done + echo "::error::Bun runtime crash repeated on Windows shard ${{ matrix.shard }}/4; failing after one retry." + exit 1 - name: CLI help smoke run: bun run src/cli/index.ts help diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index ded130e17e..29950f68b1 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -266,7 +266,11 @@ class Fixture { runtime: RuntimeRecord, path: string, init: RequestInit = {}, - timeoutMs = 10_000, + // Scaled like every other budget in this file. This one was left unscaled, and it is what + // actually failed `A-reduced` on Windows: the case has a 150 s ceiling and reported ~80 s + // elapsed, so the outer budget was never the constraint — a single request hit this fixed + // 10 s AbortSignal and aborted the case from inside (#2152). + timeoutMs = watchdogMs(10_000), ): Promise<{ status: number; body: Record }> { const response = await fetch(`http://127.0.0.1:${runtime.port}${path}`, { ...init, @@ -619,7 +623,18 @@ describe("WP13 composed toggle acceptance", () => { const release = join(fx.root, "release"); const holder = Bun.spawn([process.execPath, lockChildPath], { cwd: repoRoot, - env: { ...fx.env(fx.homeA, fx.userprofileA), OCX_LOCK_CHILD_PAYLOAD: JSON.stringify({ timeoutMs: 5_000, holdMarker: held, releaseMarker: release }) }, + // The hold has to outlast the contender's process spawn, which is the slow part on a + // Windows shard. The release marker below still ends it early everywhere else, so this + // is a ceiling rather than a sleep the test pays for. + env: { + ...fx.env(fx.homeA, fx.userprofileA), + OCX_LOCK_CHILD_PAYLOAD: JSON.stringify({ + timeoutMs: 5_000, + holdMarker: held, + releaseMarker: release, + holdMs: watchdogMs(3_000), + }), + }, stdout: "pipe", stderr: "pipe", }); fx.children.push(holder); diff --git a/tests/helpers/ci-watchdog.ts b/tests/helpers/ci-watchdog.ts index f09713dc98..a794d7fe3e 100644 --- a/tests/helpers/ci-watchdog.ts +++ b/tests/helpers/ci-watchdog.ts @@ -11,7 +11,14 @@ * hung test, not to assert latency. Local behaviour is unchanged. Bun's own * per-test timeout (`--timeout`, 60 s on CI) would pre-empt a 30 s watchdog, * so the lane timeout and this floor move together. + * + * Windows needs a higher floor still. Its shards run four Bun pools on one runner, and process + * spawn there is slower than on the POSIX lanes to begin with — a `ocx restore --json` child + * that finishes comfortably elsewhere was observed failing the 30 s floor at 30,147 ms (#2152). + * 45 s keeps the watchdog meaningful while staying under the lane's own 60 s per-test timeout, + * so a genuinely hung test is still bounded by something rather than running to the ceiling. */ export function watchdogMs(base: number): number { - return process.env.CI === "true" ? Math.max(base, 30_000) : base; + if (process.env.CI !== "true") return base; + return Math.max(base, process.platform === "win32" ? 45_000 : 30_000); } diff --git a/tests/helpers/codex-write-lock-child.ts b/tests/helpers/codex-write-lock-child.ts index be61dc84be..0ab0549f99 100644 --- a/tests/helpers/codex-write-lock-child.ts +++ b/tests/helpers/codex-write-lock-child.ts @@ -17,6 +17,7 @@ const payload = JSON.parse(process.env.OCX_LOCK_CHILD_PAYLOAD ?? "{}") as { timeoutMs?: number; holdMarker?: string; releaseMarker?: string; + holdMs?: number; }; const admitted = { authoritySnapshotId: "authority-child" } as AdmissionSnapshot; @@ -40,7 +41,12 @@ const result = await withCodexWriteLock( // an unheld lock and saw `acquired` where the test demands `busy`, which // reads exactly like a broken exclusion invariant rather than a late marker. writeFileSync(payload.holdMarker, "held"); - const until = Date.now() + 3_000; + // The release marker is the real signal; this is only the ceiling for how long we wait + // to see it. Three seconds was enough where the contender starts quickly, but on a + // Windows shard the contender's process spawn can outlast the hold — the holder then + // releases first and the parent sees `acquired` where it demands `busy`, which reads as + // a broken exclusion invariant rather than as a hold that expired too early (#2152). + const until = Date.now() + (payload.holdMs ?? 3_000); while (Date.now() < until) { if (payload.releaseMarker && Bun.file(payload.releaseMarker).size > 0) break; } diff --git a/tests/update-npm-cache-preflight.test.ts b/tests/update-npm-cache-preflight.test.ts index 520c0d6b6a..e22d72cfee 100644 --- a/tests/update-npm-cache-preflight.test.ts +++ b/tests/update-npm-cache-preflight.test.ts @@ -27,6 +27,20 @@ const canSymlink = (() => { } })(); +/** + * Windows needs its own guard, separate from `canSymlink`. + * + * The capability probe answers "may this user create a symlink", and on a GitHub-hosted + * Windows runner the answer is YES — so these cases ran and then failed in the fixture with + * `cache_entry_inaccessible`, because what actually differs there is how the preflight reads + * mode and access through a Windows symlink, not whether the link can be made (#2152). + * + * Two neighbouring cases in this file already skip on `process.platform === "win32"` for the + * same reason, so this reuses that guard rather than inventing a second mechanism. The + * capability check stays: an unprivileged POSIX-like environment still skips honestly. + */ +const WINDOWS = process.platform === "win32"; + function tempRoot(name: string): string { const root = join(tmpdir(), `ocx-cache-preflight-${name}-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}`); mkdirSync(root, { recursive: true }); @@ -67,7 +81,7 @@ describe("npm cache access pre-flight", () => { } }); - test.skipIf(!canSymlink)("lstats normal nested symlinks but never traverses their targets", () => { + test.skipIf(WINDOWS || !canSymlink)("lstats normal nested symlinks but never traverses their targets", () => { const cache = tempRoot("symlink-cache"); const missingTarget = join(tempRoot("symlink-target"), "does-not-exist"); const npx = join(cache, "_npx"); @@ -79,7 +93,7 @@ describe("npm cache access pre-flight", () => { expect(inspectNpmCacheDirectory(cache)).toEqual({ ok: true, reason: "cache_accessible" }); }); - test.skipIf(!canSymlink)("a foreign-owned nested symlink does not block the update", () => { + test.skipIf(WINDOWS || !canSymlink)("a foreign-owned nested symlink does not block the update", () => { // The distinction that decides whether this feature is usable. A real npm cache is full of // symlinks below _npx/node_modules/.bin, and their owner is irrelevant because we never // follow them. Rejecting on ownership before skipping the link would abort updates for @@ -164,7 +178,7 @@ describe("npm cache access pre-flight", () => { })).toEqual({ ok: false, reason: "worker_output_malformed" }); }); - test.skipIf(!canSymlink)("a cache root symlinked to another volume is inspected, not rejected", () => { + test.skipIf(WINDOWS || !canSymlink)("a cache root symlinked to another volume is inspected, not rejected", () => { // Pointing ~/.npm at another volume is ordinary npm configuration. Rejecting it outright was // the same class of false positive as failing on a large cache: it blocks updates for users // whose setup is fine. The root is resolved once; nested links are still never followed. From 077668384a49cdb194a84dc67932920e46d19ce5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 14:57:38 +0900 Subject: [PATCH 2/2] fix(ci): key the Bun crash retry on the signature that is actually stable The Windows retry added for #2152 grepped for `panic(thread`. This repository already learned that is the wrong anchor: Bun emits BOTH `panic(thread 2852)` and `panic(main thread)` for the same class of failure, and devlog/_fin/260731_pr_issue_triage_round/050_windows_ci_flake_rca.md names `Internal assertion failure` as the stable fingerprint. Verified by literal probe -- panic(thread 3960) matched, panic(main thread) did not. The shard would have failed on exactly the crash the retry exists for. All three signature lists -- the macOS inline grep, the new Windows one, and is_bun_runtime_crash in run-bun-test-batches.sh -- now carry the same alternatives. The workflow comment already required them to stay in sync; nothing enforced it, so three copies drifted into two shapes. The contract test now pins the sync itself rather than the text, and pins that no list keys on the thread-numbered form. hasShellCommandHead is added because the existing exact-line matcher rejected the `| tee` the retry requires, while still rejecting an echoed or commented-out copy. --- .github/workflows/ci.yml | 4 +-- scripts/ci/run-bun-test-batches.sh | 2 +- tests/ci-workflows.test.ts | 54 ++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5872136ecf..eef67dbf3b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -516,7 +516,7 @@ jobs: if [ "$suite_status" -eq 0 ]; then exit 0 fi - if ! grep -Eqi 'oh no: Bun has crashed|Segmentation fault at address|Illegal instruction|Bus error|Aborted \(core dumped\)' "$suite_log"; then + if ! grep -Eqi 'oh no: Bun has crashed|Internal assertion failure|Segmentation fault at address|Illegal instruction|Bus error|Aborted \(core dumped\)' "$suite_log"; then echo "::error::macOS suite failed on attempt ${attempt} (exit ${suite_status}); assertion failures are not retried." exit "$suite_status" fi @@ -628,7 +628,7 @@ jobs: if [ "$suite_status" -eq 0 ]; then exit 0 fi - if ! grep -Eqi 'oh no: Bun has crashed|Segmentation fault at address|Illegal instruction|Bus error|panic\(thread' "$suite_log"; then + if ! grep -Eqi 'oh no: Bun has crashed|Internal assertion failure|Segmentation fault at address|Illegal instruction|Bus error|Aborted \(core dumped\)' "$suite_log"; then echo "::error::Windows shard ${{ matrix.shard }}/4 failed on attempt ${attempt} (exit ${suite_status}); assertion failures are not retried." exit "$suite_status" fi diff --git a/scripts/ci/run-bun-test-batches.sh b/scripts/ci/run-bun-test-batches.sh index 699f99cb8d..fb61977568 100644 --- a/scripts/ci/run-bun-test-batches.sh +++ b/scripts/ci/run-bun-test-batches.sh @@ -78,7 +78,7 @@ is_bun_runtime_crash() { fi grep -Eqi \ - 'oh no: Bun has crashed|Segmentation fault at address|Illegal instruction|Bus error|Aborted \(core dumped\)' \ + 'oh no: Bun has crashed|Internal assertion failure|Segmentation fault at address|Illegal instruction|Bus error|Aborted \(core dumped\)' \ "$log_file" } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index cdcf0d9119..8ba3e7d406 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -48,6 +48,21 @@ function hasExactShellCommand(run: string | undefined, expected: string): boolea .includes(expected); } +/** + * Same intent as {@link hasExactShellCommand}, but for a command that is the HEAD of a + * pipeline. The retry loops capture the suite with `… 2>&1 | tee "$suite_log"`, so an exact + * whole-line match would reject the very shape the retry requires. Anchoring at the start of + * the line still rejects an `echo` of the command or a commented-out copy, which is what the + * exact match was protecting against. + */ +function hasShellCommandHead(run: string | undefined, expected: string): boolean { + return (run ?? "") + .split(/\r?\n/) + .map(line => line.trim()) + .filter(line => line.length > 0 && !line.startsWith("#")) + .some(line => line === expected || line.startsWith(`${expected} `)); +} + function expectSecureLinuxKeyringBootstrap(workflow: string): void { const smokeStep = workflow .split("- name: OS keyring create/read/delete smoke")[1] @@ -224,17 +239,52 @@ describe("GitHub Actions hardening", () => { // Three composed-acceptance failures were that default firing on tests still working // at 41s. Pin the flag so the leg cannot silently drift back to the default. const windowsTestCommand = `bun test --isolate --timeout 60000 tests --shard=\${{ matrix.shard }}/${windowsShards.length}`; - expect(hasExactShellCommand(`echo ${windowsTestCommand}`, windowsTestCommand)).toBe(false); + expect(hasShellCommandHead(`echo ${windowsTestCommand}`, windowsTestCommand)).toBe(false); // Binding the assertion to an executable line is only half the guarantee: a // step carrying the exact command still runs nothing under `if: false`, and // the suite would stay green against a Windows leg that never tests. Require // the matching step to be unconditional. - const windowsTestSteps = winSteps.filter(step => hasExactShellCommand(step.run, windowsTestCommand)); + const windowsTestSteps = winSteps.filter(step => hasShellCommandHead(step.run, windowsTestCommand)); expect(windowsTestSteps.length).toBeGreaterThan(0); expect(windowsTestSteps.every(step => step.if === undefined)).toBe(true); expect(winSteps.some(step => step.if === "runner.environment == 'self-hosted'" && step.run?.includes("git clean -xffd"))).toBe(true); + // The three crash-signature lists must stay identical, and they must not key on + // `panic(thread`. + // + // Bun emits BOTH `panic(thread 2852)` and `panic(main thread)` for the same class of + // failure, so a grep anchored on the numbered form silently misses half of them and the + // shard fails on a crash it was supposed to retry. This repository already learned that + // once — `devlog/_fin/260731_pr_issue_triage_round/050_windows_ci_flake_rca.md` names + // `Internal assertion failure` as the stable fingerprint — and #2152 reintroduced it. + // Three copies of one list is the real hazard, so pin the sync rather than the text. + const crashSignatures = [ + "oh no: Bun has crashed", + "Internal assertion failure", + "Segmentation fault at address", + "Illegal instruction", + "Bus error", + ]; + const windowsTestRun = windowsTestSteps[0]?.run ?? ""; + const batchScript = await readText("scripts/ci/run-bun-test-batches.sh"); + for (const signature of crashSignatures) { + expect(`macos:${signature}:${macosTestRun.includes(signature)}`).toBe(`macos:${signature}:true`); + expect(`windows:${signature}:${windowsTestRun.includes(signature)}`).toBe(`windows:${signature}:true`); + expect(`script:${signature}:${batchScript.includes(signature)}`).toBe(`script:${signature}:true`); + } + // The thread-numbered form must not be the anchor anywhere. + expect(macosTestRun).not.toContain("panic\\(thread"); + expect(windowsTestRun).not.toContain("panic\\(thread"); + expect(batchScript).not.toContain("panic\\(thread"); + + // Windows carries the same bounded retry as macOS: one attempt, crash-only. + expect(hasExactShellCommand(windowsTestRun, "set +e")).toBe(true); + expect(windowsTestRun).toContain("for attempt in 1 2"); + expect(windowsTestRun).not.toContain("while true"); + expect(windowsTestRun).toContain("assertion failures are not retried"); + expect(windowsTestRun).toContain("failing after one retry"); + // Every job that runs the root suite must build the GUI first, unconditionally. // Tests that fetch the served dashboard read their session bootstrap out of // `gui/dist/index.html`; with no build the server has no index to serve and the