diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02344a0aa2..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 @@ -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|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 + 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/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 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.