From ee7657862034ce9901e905f95aec7e47016ddb94 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:50:33 +0900 Subject: [PATCH] fix(test): bound the native-profile teardown and stop parsing partial JSON (#1061) The teardown awaited child.exited with no deadline. A child stalled in server.stop(true) hung the run until CI killed the job - a 30-minute wait for a test that had already finished its work. The waits are bounded now, including the ones after a signal: an ignored SIGTERM would otherwise reproduce the same hang one layer down, so it escalates to SIGKILL and observes the reap before throwing. Second half, same issue: waitFor proves a file exists, which is not the precondition a caller that immediately parses it needs. A half-written document satisfied the wait and then threw Unexpected EOF. The settled file is now published through atomicWriteFile, and the reader waits for something parseable rather than something present. Both paths are driven, not just present. An opt-in test-only stall makes the child hang exactly where the reported failure occurred, and the teardown has to give up and reap it; removing the bound makes that test time out at 30s instead of passing in 1.2s. The partial-write race is reproduced directly. --- tests/helpers/native-profile-startup-child.ts | 12 +- tests/native-profile-crash-boundaries.test.ts | 115 +++++++++++++++++- 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/tests/helpers/native-profile-startup-child.ts b/tests/helpers/native-profile-startup-child.ts index b0b1e6f49e..e60adffdb3 100644 --- a/tests/helpers/native-profile-startup-child.ts +++ b/tests/helpers/native-profile-startup-child.ts @@ -3,7 +3,7 @@ import { appendFileSync, existsSync, writeFileSync } from "node:fs"; import { NativeProfileManager } from "../../src/codex/native-profile-manager"; import { isCodexAccountUsable } from "../../src/codex/account-usability"; import { isMainAccountTokenLive, MAIN_CODEX_ACCOUNT_ID } from "../../src/codex/main-account"; -import { loadConfig } from "../../src/config"; +import { atomicWriteFile, loadConfig } from "../../src/config"; import { nativeMainStartupGateSnapshot, waitForNativeMainStartupGate, @@ -68,17 +68,23 @@ const server = startServer(0, { }); writeFileSync(portPath, String(server.port)); +// #1061: the parent parses this file as soon as it exists, so a partial write +// surfaces as `Unexpected EOF`. atomicWriteFile publishes through a rename, so a +// reader sees either nothing or the whole document. void waitForNativeMainStartupGate().then(() => { - writeFileSync(settledPath, JSON.stringify({ + atomicWriteFile(settledPath, JSON.stringify({ gate: nativeMainStartupGateSnapshot(), mainTokenLive: isMainAccountTokenLive(), mainUsable: isCodexAccountUsable(loadConfig(), MAIN_CODEX_ACCOUNT_ID), })); }).catch((error: unknown) => { - writeFileSync(settledPath, JSON.stringify({ + atomicWriteFile(settledPath, JSON.stringify({ error: error instanceof Error ? `${error.message}\n${error.stack ?? ""}` : String(error), })); }); while (!existsSync(stopPath)) await Bun.sleep(10); +// Test-only stall, opt-in. It exists so the parent's bounded teardown can be shown +// firing (#1061) — without it the timeout branch is present but never exercised. +if (process.env.OCX_TEST_STALL_ON_STOP === "1") await new Promise(() => {}); await server.stop(true); diff --git a/tests/native-profile-crash-boundaries.test.ts b/tests/native-profile-crash-boundaries.test.ts index 4c61fc2a38..66bc308936 100644 --- a/tests/native-profile-crash-boundaries.test.ts +++ b/tests/native-profile-crash-boundaries.test.ts @@ -9,6 +9,7 @@ import { NativeProfileManager, type NativeProfileSwitchBoundary } from "../src/c import { readNativeProfileJournal, readNativeProfileVault } from "../src/codex/native-profile-store"; import type { NativeProfileKey, NativeProfileKeyProvider } from "../src/codex/native-profile-types"; import type { OcxConfig } from "../src/types"; +import { INTERNAL_DEADLINE_MS } from "./helpers/test-budget"; const roots: string[] = []; const oldOcx = process.env.OPENCODEX_HOME; @@ -83,6 +84,59 @@ async function waitFor(path: string, timeout = 10_000): Promise { if (!existsSync(path)) throw new Error(`timed out waiting for ${path}`); } +/* + * #1061. `waitFor` proves a file EXISTS, which is not the precondition a caller + * that immediately parses it actually needs — a partially written document + * satisfies the wait and then throws `Unexpected EOF`. This waits for the real + * precondition instead. + */ +async function waitForJson(path: string, timeout = 10_000): Promise { + const deadline = Date.now() + timeout; + let lastError: unknown; + while (Date.now() < deadline) { + if (existsSync(path)) { + try { + return JSON.parse(readFileSync(path, "utf8")) as T; + } catch (error) { + lastError = error; + } + } + await Bun.sleep(10); + } + throw new Error(`timed out waiting for parseable JSON in ${path}`, { cause: lastError }); +} + +const KILL_GRACE_MS = 2_000; + +/* + * #1061. The teardown used to `await child.exited` with no deadline, so a child + * stalled in `server.stop(true)` hung the run until CI killed the job — a + * 30-minute wait for a test that had already done its work. Every wait here is + * bounded, including the ones after a signal: an ignored SIGTERM would otherwise + * reproduce the same hang one layer down. + */ +async function stopStartup( + child: Bun.Subprocess, + paths: { release: string; stop: string }, + timeoutMs: number = INTERNAL_DEADLINE_MS, +): Promise { + writeFileSync(paths.release, "recover"); + writeFileSync(paths.stop, "stop"); + const exit = await Promise.race([child.exited, Bun.sleep(timeoutMs).then(() => null)]); + if (exit === null) { + child.kill(); + const killed = await Promise.race([child.exited, Bun.sleep(KILL_GRACE_MS).then(() => null)]); + if (killed === null) { + child.kill("SIGKILL"); + // Observe the escalation before throwing, so a caller asserting on + // `exitCode` is not racing the reap. + await Promise.race([child.exited, Bun.sleep(KILL_GRACE_MS).then(() => null)]); + } + throw new Error("startup child did not stop"); + } + if (exit !== 0) throw new Error(`startup child exited ${exit}`); +} + function spawnSwitch(f: Awaited>, options: { boundary?: NativeProfileSwitchBoundary; marker?: string; release?: string; contention?: string; result: string }) { return Bun.spawn([process.execPath, join(import.meta.dir, "helpers", "native-profile-switch-child.ts")], { cwd: join(import.meta.dir, ".."), @@ -109,7 +163,11 @@ function startupPaths(f: Awaited>) { return { port: join(f.root, "port"), release: join(f.root, "recover"), settled: join(f.root, "settled"), upstream: join(f.root, "upstream"), stop: join(f.root, "stop") }; } -function spawnStartup(f: Awaited>, p: ReturnType) { +function spawnStartup( + f: Awaited>, + p: ReturnType, + extraEnv: Record = {}, +) { return Bun.spawn([process.execPath, join(import.meta.dir, "helpers", "native-profile-startup-child.ts")], { cwd: join(import.meta.dir, ".."), env: { @@ -119,6 +177,7 @@ function spawnStartup(f: Awaited>, p: ReturnType { expect(existsSync(p.upstream)).toBe(false); writeFileSync(p.release, "recover"); } - await waitFor(p.settled); - expect(JSON.parse(readFileSync(p.settled, "utf8"))).toMatchObject({ gate: { status: "ready" } }); + expect(await waitForJson(p.settled)).toMatchObject({ gate: { status: "ready" } }); expect((await mainRequest(port)).status).toBe(200); await waitFor(p.upstream); const receipt = JSON.parse(readFileSync(p.upstream, "utf8").trim().split("\n").at(-1)!); @@ -192,9 +250,7 @@ describe("native profile OpenCodex process-exit phases", () => { expect(recoveredVault.revision).toBe(f.initialRevision + (finalTarget ? 1 : 0)); expect(readNativeProfileJournal(f.manager.context)).toBeNull(); } finally { - writeFileSync(p.release, "recover"); - writeFileSync(p.stop, "stop"); - expect(await restart.exited).toBe(0); + await stopStartup(restart, p); } } }, 90_000); @@ -230,4 +286,51 @@ describe("native profile OpenCodex process-exit phases", () => { if (second) await second.exited; } }, 20_000); + + /* + * #1061 activation evidence for the teardown deadline. A green suite says nothing + * about a timeout branch nobody drives, so this drives it: the child is told to + * stall exactly where the reported hang occurred (before `server.stop(true)`), + * and the teardown must give up and reap it instead of waiting forever. + * + * The reap assertion is the part that matters — it proves cleanup happened, not + * merely that a deadline was noticed. A signalled child reports `signalCode` + * rather than `exitCode`, so this checks that the process settled either way. + */ + test("a stalled startup child is killed by the bounded teardown instead of hanging", async () => { + const f = await fixture(); + const p = startupPaths(f); + const child = spawnStartup(f, p, { OCX_TEST_STALL_ON_STOP: "1" }); + try { + await waitFor(p.port); + await expect(stopStartup(child, p, 1_000)).rejects.toThrow("startup child did not stop"); + expect(child.killed).toBe(true); + expect(child.exitCode ?? child.signalCode).not.toBeNull(); + } finally { + if (child.exitCode === null) { + child.kill("SIGKILL"); + await child.exited; + } + } + }, 30_000); + + /* + * #1061 the other half: the settled file is parsed the moment it appears, so a + * reader that only checks existence sees a partial document. This drives that + * exact sequence — partial content first, then the real document — and asserts + * the wait holds out for something parseable. + */ + test("waitForJson holds out for a complete document instead of parsing a partial write", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-settled-race-")); + const target = join(dir, "settled.json"); + try { + writeFileSync(target, "{\"gate\":"); // what a half-finished write looks like + const pending = waitForJson<{ gate: { status: string } }>(target, 5_000); + await Bun.sleep(50); + writeFileSync(target, JSON.stringify({ gate: { status: "ready" } })); + expect(await pending).toMatchObject({ gate: { status: "ready" } }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, 15_000); });