From 08bb2e485d56115d2ecd641a15659ff912b69d38 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:12:40 -0700 Subject: [PATCH 1/2] chore(release): open dev at 2.86.0 before releasing 2.85.0 (#7093) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- desktop/src-tauri/Cargo.lock | 2 +- desktop/src-tauri/Cargo.toml | 2 +- desktop/src-tauri/tauri.conf.json | 2 +- package.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index 35894c7cd1e..b994d029dc7 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -2645,7 +2645,7 @@ dependencies = [ [[package]] name = "opencodex-desktop" -version = "2.85.0" +version = "2.86.0" dependencies = [ "base64 0.22.1", "dbus", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index ede2061caef..a65970d63d9 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "opencodex-desktop" -version = "2.85.0" +version = "2.86.0" description = "OpenCodex desktop shell" authors = ["OpenCodex contributors"] license = "MIT" diff --git a/desktop/src-tauri/tauri.conf.json b/desktop/src-tauri/tauri.conf.json index 186419427d8..c1bb2b933d3 100644 --- a/desktop/src-tauri/tauri.conf.json +++ b/desktop/src-tauri/tauri.conf.json @@ -1,7 +1,7 @@ { "$schema": "https://schema.tauri.app/config/2", "productName": "OpenCodex", - "version": "2.85.0", + "version": "2.86.0", "identifier": "com.opencodex.desktop", "build": { "frontendDist": "../ui", diff --git a/package.json b/package.json index f17dad45fbf..db3a06aee47 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@bitkyc08/opencodex", - "version": "2.85.0", + "version": "2.86.0", "description": "Universal provider proxy for OpenAI Codex & Claude Code — use any LLM with Codex CLI/App/SDK and Claude Code", "type": "module", "main": "./bin/package-main.mjs", From 494c50d02f9773b7da9c47fb345e7a8540a87b90 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 14 Oct 2026 13:08:15 -0700 Subject: [PATCH 2/2] fix(ci): remove the shared root causes of flaky Windows CI (#7107) * fix(test-teardown): settle un-awaited background work before removing a tree The pre-route pool quota prime runs without being awaited and holds the native-main claim database open across ACL hardening and a WHAM read. Teardown retried removal with Bun.sleepSync, which blocks the event loop the prime needs to reach its finally, so on Windows every retry failed EBUSY until the 15 s budget ran out (#7088 and #7091 stubbed the prime per test). Track fire-and-forget work in src/lib/background-work, register the pool prime and its lazy-import callers, settle it in a global afterEach, and add removeTreeAfterBackgroundWork (settle, reap icacls, then retry with waits that yield). The compaction fixture uses it, and the per-test prime stubs from #7088 and #7091 are reverted so the real prime runs again. * fix(responses): serialize snapshot follow-up scheduling with shutdown * fix(config): track the config-dir ACL harden as background work icacls holds the config directory open while it runs; a config write or removal that lands on it before the child exits fails with a sharing violation on Windows. Registering the flight lets the per-case drain settle it, and the V2 refusal test now prints the CLI errors when the handler refuses before the native toggle. * fix(cli): reject occupied auxiliary ports before private startup work * fix(tray): retain timed-out probes until asynchronous reaping Remove WaitForExit from startup-health timeout ticks and retain process ownership until exit and both readers settle. Run lifecycle fixtures with the production Bun engine instead of a nested PowerShell cold start, which failed readiness before maintenance in Windows run 38503986851. * fix(config): preserve Windows ACL memo across repeated reads * test: bound child exit and pipe waits in slow-spawn cases * fix(ci): share one transpiler cache per shard; widen the DPAPI PowerShell bound Run 2 and 3 of the Windows proof failed on first-spawn timeouts (writeJournal child 40s, launcher attest 15s) and a 10s DPAPI PowerShell call. Each CI batch is its own bun test process, and with BUN_RUNTIME_TRANSPILER_CACHE_PATH unset the preload gives every batch an empty private cache, so the first child each batch spawns re-transpiles its whole module graph. The batch runner now creates one private cache per shard and exports it. The DPAPI helper's PowerShell bound moves from 10s to 20s: it is async and off the request path, and a cold PowerShell loading System.Security exceeded 10s on a loaded runner. * test(desktop-egress): give each proxy variable one spelling on Windows Windows environment names are case-insensitive, so the child env that set ALL_PROXY to the fixture proxy and all_proxy to "" defined one variable twice. Env-block ordering decided the winner; adding the shard transpiler cache variable flipped it, and the -all cases reached the real chatgpt.com (403). Clear every spelling and set only the intended route. * fix(test-teardown): never remove or exit under live tracked work; address review findings - removeTreeAfterBackgroundWork keeps the tree and names pending work instead of deleting under a live writer. - Final sandbox cleanup bounds background-work settlement (15 s) and leaves the root for stale recovery with the labels when work never settles. - spawnTestChild rejects when the child never started (ENOENT) instead of resolving an empty result. - Auxiliary preflight rejects loopback and hub management configured on one port. - Tray shutdown keeps a probe tracked until it has actually exited and waits for its pipe readers before disposal. - The free-socket preflight test binds inside try so a lost race still stops the first server. --------- Co-authored-by: jun --- scripts/ci/run-bun-test-batches.sh | 12 ++ scripts/test-layout/layout.json | 2 + scripts/test-temp.ts | 29 +++++ src/cli/auxiliary-listener-preflight.ts | 37 ++++++ src/cli/index.ts | 1 + src/codex/auth-api/pool-mode-gate.ts | 7 +- src/codex/auth-context.ts | 6 +- .../windows-key-protection.ts | 6 +- src/config/load-degrade.ts | 5 +- src/config/paths.ts | 13 +- src/lib/background-work.ts | 65 ++++++++++ src/responses/state.ts | 61 ++++----- src/server/index.ts | 5 +- src/tray/windows-tray.ps1 | 67 ++++++---- structure/codex-home.md | 2 +- structure/config.md | 2 +- structure/ops/test-sandbox-cleanup.md | 11 ++ structure/runtime.md | 4 +- tests/ci-workflows/cold-spawn-warmup.test.ts | 30 ++++- tests/cli/cli-route-policy-write.test.ts | 20 ++- tests/cli/cli-start-auxiliary-bind.test.ts | 45 ++++++- tests/cli/cli-status-json.test.ts | 34 ++--- tests/cli/cli-v2-runtime.test.ts | 3 +- .../desktop-compatibility-authority.test.ts | 2 +- .../catalog-auto-refresh-scheduler.test.ts | 40 +++--- tests/fixtures/test-layout-expected.json | 2 + tests/helpers/cold-spawn-warmup.ts | 103 +-------------- tests/helpers/compaction-routing-fixtures.ts | 6 +- tests/helpers/remove-tree.ts | 36 +++++- tests/helpers/spawn-test-child.ts | 122 ++++++++++++++++++ tests/helpers/windows-tray-probe-child.ts | 12 ++ .../windows-tray-probe-lifecycle-driver.ps1 | 6 +- tests/lib/optional-desktop-upstream.test.ts | 10 +- tests/lib/remove-tree-helper.test.ts | 47 ++++++- tests/preload.ts | 14 +- .../responses-compaction-routing.test.ts | 20 +-- .../responses-state-flush-race.test.ts | 90 +++++++++++++ .../windows-config-read-acl-memo.test.ts | 100 ++++++++++++++ tests/windows/windows-tray.test.ts | 87 ++++++++++--- 39 files changed, 902 insertions(+), 262 deletions(-) create mode 100644 src/cli/auxiliary-listener-preflight.ts create mode 100644 src/lib/background-work.ts create mode 100644 tests/helpers/spawn-test-child.ts create mode 100644 tests/helpers/windows-tray-probe-child.ts create mode 100644 tests/responses/responses-state-flush-race.test.ts create mode 100644 tests/windows/windows-config-read-acl-memo.test.ts diff --git a/scripts/ci/run-bun-test-batches.sh b/scripts/ci/run-bun-test-batches.sh index 9786ddaf898..f8aa18e80af 100644 --- a/scripts/ci/run-bun-test-batches.sh +++ b/scripts/ci/run-bun-test-batches.sh @@ -326,6 +326,18 @@ fallback_ms="$( )" readonly FALLBACK_MS="$fallback_ms" +# One private Bun transpiler cache for every batch in this shard. Left unset, each batch's +# preload creates an empty cache of its own, so the first child a batch spawns re-transpiles +# its whole module graph from nothing (0.67s cold against 0.20s warm for the journal/CLI graph +# on macOS). On Windows runners that cold load is what pushed first spawns to 15-40s and past +# their deadlines. mktemp -d creates a fresh directory only this job can write, which is the +# protected explicit cache createIsolatedTestEnvironment accepts; an operator's value is kept. +if [[ -z "${BUN_RUNTIME_TRANSPILER_CACHE_PATH+x}" ]]; then + shard_transpiler_cache="$(mktemp -d -t ocx-bun-transpiler-cache.XXXXXX)" + trap 'rm -rf -- "$shard_transpiler_cache"' EXIT + export BUN_RUNTIME_TRANSPILER_CACHE_PATH="$shard_transpiler_cache" +fi + assignment_file="$(mktemp -t ocx-bun-test-shards.XXXXXX)" printf '%s\n' "${GENERAL_FILES[@]}" \ | awk -F '\t' -v table="$durations_source" -v fallback="$FALLBACK_MS" ' diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 3696b56564c..16227fe5cf1 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1525,6 +1525,7 @@ "responses-spill-inspection.test.ts": "responses", "responses-spill-orphan-sweep.test.ts": "responses", "responses-spill-shutdown-clock.test.ts": "responses", + "responses-state-flush-race.test.ts": "responses", "responses-state-snapshot-select.test.ts": "responses", "responses-state-snapshot-stubs.test.ts": "responses", "responses-state-store-false.test.ts": "responses", "responses-state-write-amplification.test.ts": "responses", @@ -1849,6 +1850,7 @@ "windows-process-priority.test.ts": "windows", "windows-scheduler-install-verification.test.ts": "windows", "windows-secret-acl.test.ts": "windows", "windows-secret-acl-removal-flight.test.ts": "windows", + "windows-config-read-acl-memo.test.ts": "windows", "windows-service-mutation-lock.test.ts": "windows", "windows-service-wrappers.test.ts": "windows", "windows-text-decoding.test.ts": "windows", diff --git a/scripts/test-temp.ts b/scripts/test-temp.ts index 72369d46b13..b7a430eec13 100644 --- a/scripts/test-temp.ts +++ b/scripts/test-temp.ts @@ -210,6 +210,35 @@ export function removeTestTempTree(path: string, options: RemoveTreeOptions = {} } } +/** + * The same policy, but the waits yield to the event loop. + * + * The synchronous loop is only correct when the handle holder is another process. When the + * holder is async work in THIS process -- an un-awaited quota prime holding a claim database + * across an icacls child or a fetch -- `Bun.sleepSync` stops the loop that work needs to reach + * its `finally`, so every retry fails until the budget is gone. Awaiting lets the owner finish. + */ +export async function removeTestTempTreeAsync( + path: string, + options: Omit & { sleep?: (milliseconds: number) => Promise } = {}, +): Promise { + const delays = options.delays ?? removeRetrySchedule(options.budgetMs); + const remove = options.remove ?? (target => rmSync(target, { recursive: true, force: true })); + const sleep = options.sleep ?? (milliseconds => Bun.sleep(milliseconds)); + for (let attempt = 0; attempt <= delays.length; attempt += 1) { + try { + remove(path); + return; + } catch (error) { + const transient = TRANSIENT_REMOVE_CODES.has(errorCode(error)); + const finalAttempt = !transient || attempt === delays.length; + if (finalAttempt && quarantineFinalEperm(path, error, options.lockDiagnostic, options.lockEnv)) return; + if (finalAttempt) throw error; + await sleep(delays[attempt]!); + } + } +} + /** Stamp a newly created root so future runs can prove its OpenCodex test ownership. */ export function writeTestTempOwner(root: string, runId?: string): void { const owner: TestTempOwner = { diff --git a/src/cli/auxiliary-listener-preflight.ts b/src/cli/auxiliary-listener-preflight.ts new file mode 100644 index 00000000000..45026fba10e --- /dev/null +++ b/src/cli/auxiliary-listener-preflight.ts @@ -0,0 +1,37 @@ +import type { OcxConfig } from "../types"; +import { AuxiliaryListenerBindError } from "../server/ports"; +import { createServer } from "node:net"; + +/** + * Reject impossible auxiliary binds before startup creates private state. On Windows + * that publication requires several synchronous ACL subprocesses. This is only an + * early rejection: startServer still owns the actual bind and transactional rollback + * if another process takes the port between this probe and startup. + */ +export async function assertAuxiliaryListenerPortsAvailable(config: OcxConfig): Promise { + const loopback = config.unauthenticatedLoopbackListener; + const management = config.runtimeRole === "hub" ? config.hub?.managementIngress : undefined; + const targets = [ + { listener: "unauthenticatedLoopbackListener", port: loopback?.enabled ? loopback.port : undefined }, + { listener: "hub.managementIngress", port: management?.enabled ? management.port : undefined }, + ] as const; + // Each probe releases its socket, so two listeners configured for one port would both + // pass and startup would still fail on the second bind. Reject the collision directly. + if (targets[0].port !== undefined && targets[0].port === targets[1].port) { + throw new AuxiliaryListenerBindError( + "hub.managementIngress", targets[1].port, "127.0.0.1", + new Error("port is also configured for unauthenticatedLoopbackListener"), + ); + } + for (const { listener, port } of targets) { + // The port-less companion shares the public bind; it has no independent target. + if (port === undefined) continue; + await new Promise((resolve, reject) => { + const probe = createServer(socket => socket.destroy()); + const fail = (cause: unknown) => reject(new AuxiliaryListenerBindError(listener, port, "127.0.0.1", cause)); + probe.once("error", fail); + probe.once("listening", () => probe.close(error => error ? fail(error) : resolve())); + try { probe.listen({ port, host: "127.0.0.1" }); } catch (error) { fail(error); } + }); + } +} diff --git a/src/cli/index.ts b/src/cli/index.ts index 8f512847864..708698bf9a0 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -283,6 +283,7 @@ async function chooseListenPort( `Port ${preferred} is reserved for unauthenticatedLoopbackListener; choose a different proxy port.`, ); } + await (await import("./auxiliary-listener-preflight")).assertAuxiliaryListenerPortsAvailable(config); // Soft start: brief prefer-retry then ephemeral hop. // Explicit `--port` (service wrappers / update restart): wait for the pinned port // to free without killing any listener (healthy ocx / foreign). Never hop. diff --git a/src/codex/auth-api/pool-mode-gate.ts b/src/codex/auth-api/pool-mode-gate.ts index 99f96d01e7a..92b02e0ed2f 100644 --- a/src/codex/auth-api/pool-mode-gate.ts +++ b/src/codex/auth-api/pool-mode-gate.ts @@ -25,6 +25,7 @@ import type { PoolQuotaResult } from "./pool-quota-probe"; import { fetchMainAccountInfoAttempt, fetchMainAccountInfo, MAIN_CACHE_TTL } from "./main-account-probe"; import { fetchPoolAccountQuota, PoolQuotaProbeBusyError, POOL_CACHE_TTL, POOL_QUOTA_REFRESH_CONCURRENCY } from "./pool-quota-probe"; import { getRuntimeConfig, configuredPoolAccount, mapWithConcurrency } from "./runtime-config"; +import { trackBackgroundWork } from "../../lib/background-work"; let primeInFlight: Promise | null = null; let lastPriorityFailbackPrimeAt: number | undefined; @@ -259,7 +260,9 @@ export async function primeCodexPoolQuotas( && now - lastPriorityFailbackPrimeAt < CODEX_PRIORITY_FAILBACK_REFRESH_MS) return; lastPriorityFailbackPrimeAt = now; } - primeInFlight = (async () => { + // Tracked because most callers do not await it, yet it holds the native-main claim + // database open; whoever removes that directory settles it first (see background-work). + primeInFlight = trackBackgroundWork(`codex-pool-quota-prime:${reason}`, (async () => { const pool = (runtimeConfig.codexAccounts ?? []).filter(isSelectableCodexPoolAccount); const stale = pool.filter(a => { const q = getAccountQuota(a.id); @@ -344,7 +347,7 @@ export async function primeCodexPoolQuotas( if (process.env.OPENCODEX_DEBUG_QUOTA === "1") { console.warn(`[codex-quota] prime done (reason=${reason}, pool=${pool.length}, refreshed=${stale.length})`); } - })().finally(() => { primeInFlight = null; }); + })().finally(() => { primeInFlight = null; })); return primeInFlight; } diff --git a/src/codex/auth-context.ts b/src/codex/auth-context.ts index 93a9d4bddbd..76438d04ac3 100644 --- a/src/codex/auth-context.ts +++ b/src/codex/auth-context.ts @@ -51,6 +51,7 @@ import { // The half-open TRANSIENT-HOLD lease (#4701). Not the quota-cooldown probe lease imported from // ./routing above -- different module, different domain, and a request never holds both. import { releaseTransientProbe } from "../routing/probe-lease"; +import { trackBackgroundWork } from "../lib/background-work"; import { codexConversationIdentity, recordCodexThreadLineage, @@ -1547,9 +1548,10 @@ async function resolveCodexAuthContextUnannotated( if (options.primeCodexPoolQuotas) { void options.primeCodexPoolQuotas(config, reason).catch(() => {}); } else { - import("./auth-api") + // Tracked from the import on: the prime registers itself only once the module resolves. + void trackBackgroundWork(`codex-pool-quota-prime-import:${reason}`, import("./auth-api") .then(({ primeCodexPoolQuotas }) => primeCodexPoolQuotas(config, reason)) - .catch(() => {}); + .catch(() => {})); } } // Snapshot (not just the deadline) so a refused request can report WHY it is cooled: diff --git a/src/codex/desktop-compatibility/windows-key-protection.ts b/src/codex/desktop-compatibility/windows-key-protection.ts index 6c12cce52a7..a40c64d408e 100644 --- a/src/codex/desktop-compatibility/windows-key-protection.ts +++ b/src/codex/desktop-compatibility/windows-key-protection.ts @@ -4,6 +4,10 @@ import { resolveTrustedWindowsPowerShellExe } from "../../lib/windows-elevation" const MAX_INPUT = 65_536; const MAX_OUTPUT = 131_072; const PURPOSE = "opencodex/codex-desktop-compatibility/authority/v1"; +// A cold Windows PowerShell that must also load System.Security has been measured past 10s on a +// loaded machine (Windows CI run 38591626343). The call is async and off the request path, so a +// wider bound costs nothing while healthy and turns a slow start into success instead of a refusal. +const DPAPI_TIMEOUT_MS = 20_000; const SCRIPT = [ "$ErrorActionPreference='Stop'", "Add-Type -AssemblyName System.Security", @@ -31,7 +35,7 @@ function runDpapi(operation: "protect" | "unprotect", data: Uint8Array): Promise if (data.byteLength === 0 || data.byteLength > MAX_INPUT) return Promise.reject(new Error("desktop_compatibility_key_size")); return new Promise((resolve, reject) => { const child = execFile(resolveTrustedWindowsPowerShellExe(), ["-NoProfile", "-NonInteractive", "-Command", SCRIPT], { - timeout: 10_000, maxBuffer: MAX_OUTPUT, windowsHide: true, encoding: "utf8", + timeout: DPAPI_TIMEOUT_MS, maxBuffer: MAX_OUTPUT, windowsHide: true, encoding: "utf8", }, (error, stdout) => { // Do not include PowerShell stderr, command output or the input in a propagated error. if (error || !/^[A-Za-z0-9+/]+={0,2}$/.test(stdout.trim())) { diff --git a/src/config/load-degrade.ts b/src/config/load-degrade.ts index ccfb186f462..2dd4c46644f 100644 --- a/src/config/load-degrade.ts +++ b/src/config/load-degrade.ts @@ -52,9 +52,12 @@ import { hasWarnedInheritedFastWireConflict, markWarnedInheritedFastWireConflict export function hardenExistingSecret(path: string): void { if (existsSync(path) && !isPassiveRead()) { - try { chmodSync(path, 0o600); } catch { /* best-effort */ } if (process.platform === "win32") { + // Preserve the ACL helper's metadata observation across reads. Windows chmod does + // not establish a private DACL and can invalidate that observation before every load. hardenSecretPath(path, { required: false }); + } else { + try { chmodSync(path, 0o600); } catch { /* best-effort */ } } } } diff --git a/src/config/paths.ts b/src/config/paths.ts index e241d026959..d20166733d8 100644 --- a/src/config/paths.ts +++ b/src/config/paths.ts @@ -4,6 +4,7 @@ import { join, resolve } from "node:path"; import { flushWindowsSecretAclReapsBeforeRemoval, hardenSecretDirAsync, windowsSecretAclApplies } from "../lib/windows-secret-acl"; import { assertNotRealHomeUnderTest } from "../lib/test-home-guard"; import { isPassiveRead } from "../lib/passive-read"; +import { trackBackgroundWork } from "../lib/background-work"; /** * Expand a leading `~` in user-supplied paths without interpreting shell @@ -37,17 +38,23 @@ export function hardenConfigDir(): void { // have changed the protected directory used by the test-home boundary. assertNotRealHomeUnderTest(dir); if (!existsSync(dir)) return; - try { chmodSync(dir, 0o700); } catch { /* best-effort */ } + // Windows privacy is enforced by NTFS ACLs. chmod only changes DOS attributes there; + // doing it before the ACL lookup can invalidate its ctime-based successful observation. + if (!windowsSecretAclApplies()) { + try { chmodSync(dir, 0o700); } catch { /* best-effort */ } + } if (windowsSecretAclApplies() && !configDirHardeningFlights.has(dir)) { // This is an optional read-path harden. Waiting synchronously here used to stop the Bun // event loop (including /healthz) for the full icacls timeout. Required mutation paths keep // their own awaited/fail-closed hardening; ordinary config reads only start one soft flight. - const flight = hardenSecretDirAsync(dir, { required: false }) + // Tracked: icacls holds the directory open, and a config write or removal that lands on + // it before the child exits fails with a sharing violation on Windows. + const flight = trackBackgroundWork("config-dir-acl-harden", hardenSecretDirAsync(dir, { required: false }) .then(() => undefined) .catch(() => undefined) .finally(() => { if (configDirHardeningFlights.get(dir) === flight) configDirHardeningFlights.delete(dir); - }); + })); configDirHardeningFlights.set(dir, flight); } } diff --git a/src/lib/background-work.ts b/src/lib/background-work.ts new file mode 100644 index 00000000000..4bdc560306d --- /dev/null +++ b/src/lib/background-work.ts @@ -0,0 +1,65 @@ +/** + * Fire-and-forget work the process started and nobody awaits. + * + * A lazy quota prime or a startup reconcile is deliberately left running behind a + * request so the request never waits on it. The work still owns files: the pool + * prime holds the native-main claim database open across ACL hardening and a WHAM + * read. Whoever later removes the directory those files live in has to wait for the + * owner. Without a handle on the work there was nothing to wait for: test teardown + * spun a synchronous retry loop, which blocks the very event loop the owner needs to + * reach its `finally` and close the file. On Windows every overlap became a 15 s EBUSY. + * + * Registering costs one Set entry per in-flight task and changes nothing about how + * the work runs. Settling is opt-in: test teardown calls it; requests never do. + */ + +interface BackgroundWork { + readonly label: string; + readonly settled: Promise; +} + +const inFlight = new Set(); + +/** Record `work` until it settles and return it unchanged. Rejections stay the caller's. */ +export function trackBackgroundWork(label: string, work: Promise): Promise { + const entry: BackgroundWork = { + label, + settled: work.then(() => { inFlight.delete(entry); }, () => { inFlight.delete(entry); }), + }; + inFlight.add(entry); + return work; +} + +/** Labels of work still running, for diagnostics. */ +export function pendingBackgroundWork(): string[] { + return [...inFlight].map(entry => entry.label); +} + +export interface BackgroundWorkSettlement { + readonly settled: boolean; + readonly pending: string[]; +} + +/** + * Wait until no tracked work is in flight, including work started by work that was + * already running. A bounded wait reports what is still running instead of throwing, + * so the caller decides whether that is fatal. + */ +export async function settleBackgroundWork(options: { timeoutMs?: number } = {}): Promise { + const deadline = options.timeoutMs === undefined ? Infinity : Date.now() + options.timeoutMs; + while (inFlight.size > 0) { + const remaining = deadline - Date.now(); + if (remaining <= 0) break; + const batch = Promise.all([...inFlight].map(entry => entry.settled)).then(() => false); + if (remaining === Infinity) { + await batch; + continue; + } + let timer: ReturnType | undefined; + const expired = new Promise(resolve => { timer = setTimeout(() => resolve(true), remaining); }); + const timedOut = await Promise.race([batch, expired]); + clearTimeout(timer); + if (timedOut) break; + } + return { settled: inFlight.size === 0, pending: pendingBackgroundWork() }; +} diff --git a/src/responses/state.ts b/src/responses/state.ts index 7696f2d2414..602d19a3724 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -710,11 +710,6 @@ export function sweepOrphanedResponseSpills(): number { type SnapshotWriteOutcome = "stable" | "unstable" | "failed"; async function writeBoundedSnapshot(path: string, attemptLimit: number): Promise { - // Serialize writers so concurrent flush + debounce cannot race on temps / ACL (#612). - const previous = persistGate; - let release!: () => void; - persistGate = new Promise(resolve => { release = resolve; }); - await previous; try { for (let attempt = 0; attempt < attemptLimit; attempt += 1) { const revision = stateRevision; @@ -751,13 +746,19 @@ async function writeBoundedSnapshot(path: string, attemptLimit: number): Promise lastSnapshotTarget = resolveWriteTarget(path); } persistAttemptHookForTests?.(); - if (revision === stateRevision) return "stable"; + if (revision === stateRevision) { + // Cancel only the timer whose mutation this stable revision includes, + // before an awaiting caller's microtask can record a newer revision. + if (persistTimer) clearTimeout(persistTimer); + persistTimer = null; + pendingPersistPath = null; + drainPendingSpillUnlinks(); + return "stable"; + } } return "unstable"; } catch { return "failed"; - } finally { - release(); } } @@ -792,21 +793,30 @@ function schedulePersistAt(path: string, replace = false): void { } async function persistNow(path: string, awaitFollowUp = false): Promise { - if (persistTimer) { - clearTimeout(persistTimer); - persistTimer = null; - } - pendingPersistPath = null; - const attemptLimit = awaitFollowUp ? MAX_SNAPSHOT_REWRITE_ATTEMPTS : 1; - let outcome = await writeBoundedSnapshot(path, attemptLimit); - if (outcome === "unstable" && awaitFollowUp) { - if (persistTimer) clearTimeout(persistTimer); - persistTimer = null; + // Include follow-up scheduling in the gate: releasing after only the disk write + // lets shutdown observe an empty timer before the background pass re-arms it. + const previous = persistGate; + let release!: () => void; + persistGate = new Promise(resolve => { release = resolve; }); + await previous; + try { + if (persistTimer) { + clearTimeout(persistTimer); + persistTimer = null; + } pendingPersistPath = null; - outcome = await writeBoundedSnapshot(path, attemptLimit); + const attemptLimit = awaitFollowUp ? MAX_SNAPSHOT_REWRITE_ATTEMPTS : 1; + let outcome = await writeBoundedSnapshot(path, attemptLimit); + if (outcome === "unstable" && awaitFollowUp) { + if (persistTimer) clearTimeout(persistTimer); + persistTimer = null; + pendingPersistPath = null; + outcome = await writeBoundedSnapshot(path, attemptLimit); + } + if (outcome === "unstable" && !awaitFollowUp) schedulePersistAt(path, true); + } finally { + release(); } - if (outcome === "stable") drainPendingSpillUnlinks(); - else if (outcome === "unstable" && !awaitFollowUp) schedulePersistAt(path, true); } function schedulePersist(): void { @@ -816,15 +826,8 @@ function schedulePersist(): void { } async function flushResponseSnapshot(): Promise { - if (persistTimer) { - await persistNow(pendingPersistPath ?? snapshotPath(), true); - return; - } - // No pending timer: still await any in-flight write so shutdown does not race (#612). + // The gate includes the background pass's timer decision, not just its write. await persistGate; - // A bounded background pass may have scheduled its same-path follow-up while - // this flush was waiting on the single-flight gate. Shutdown owns one awaited - // bounded follow-up rather than returning behind that unref'd timer. if (persistTimer) await persistNow(pendingPersistPath ?? snapshotPath(), true); } diff --git a/src/server/index.ts b/src/server/index.ts index 69b2f61d1e4..1abb994f766 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -25,6 +25,7 @@ import { websocketsEnabled, } from "../config"; import { flushConfigDirHardeningAndReaps } from "../config/paths"; +import { trackBackgroundWork } from "../lib/background-work"; import { migrateStartupSubagentModels } from "./subagent-models-startup"; import { migrateStartupXaiResponses } from "./xai-responses-startup"; import { migrateStartupZaiResponses } from "./zai-responses-startup"; @@ -843,7 +844,7 @@ function startServerWithSpendLedgerOwner(port: number | undefined, deps: StartSe && isCanonicalOpenAiForwardProvider(openAiProvider) && providerCodexAccountMode("openai", openAiProvider) === "pool" ) { - import("../codex/plan-from-token") + void trackBackgroundWork("codex-pool-quota-prime-startup", import("../codex/plan-from-token") .then(({ reconcileCodexPlansFromTokens }) => { try { reconcileCodexPlansFromTokens(config); @@ -853,7 +854,7 @@ function startServerWithSpendLedgerOwner(port: number | undefined, deps: StartSe return import("../codex/auth-api"); }) .then(({ primeCodexPoolQuotas }) => primeCodexPoolQuotas(config, "startup")) - .catch(() => {}); + .catch(() => {})); } // Opt-in storage policy (default OFF). Never blocks listen; cancellable on shutdown. diff --git a/src/tray/windows-tray.ps1 b/src/tray/windows-tray.ps1 index 3112ee6fdc9..8aab9bdd900 100644 --- a/src/tray/windows-tray.ps1 +++ b/src/tray/windows-tray.ps1 @@ -201,14 +201,10 @@ function Start-StartupHealthProbe { if ($null -ne $script:startupProbeProcess) { # Process.Start succeeded but the async pipe setup failed: Dispose alone # would leave the Bun child running, so terminate it before cleanup. - try { - $script:startupProbeProcess.Kill() - [void]$script:startupProbeProcess.WaitForExit(3000) - } catch { - Write-ActionLog "startup-health probe launch cleanup failed: $($_.Exception.GetType().Name)" - } + Stop-StartupHealthProbe + } else { + Complete-StartupHealthProbe } - Complete-StartupHealthProbe } } @@ -217,8 +213,11 @@ function Complete-StartupHealthProbe { $script:startupProbeProcess = $null $script:startupProbeOutputTask = $null $script:startupProbeErrorTask = $null + $script:startupProbeTerminating = $false if ($null -ne $process) { try { + $process.StandardOutput.Dispose() + $process.StandardError.Dispose() $process.Dispose() } catch { Write-ActionLog "startup-health probe dispose failed: $($_.Exception.GetType().Name)" @@ -226,6 +225,31 @@ function Complete-StartupHealthProbe { } } +function Stop-StartupHealthProbe([switch]$Shutdown) { + if ($null -eq $script:startupProbeProcess) { return } + $script:startupProbeTerminating = $true + try { + if (-not $script:startupProbeProcess.HasExited) { $script:startupProbeProcess.Kill() } + } catch { Write-ActionLog "startup-health probe kill failed: $($_.Exception.GetType().Name)" } + if ($Shutdown) { + # Shutdown alone may wait. Timeout ticks keep the child tracked until actual exit. + try { [void]$script:startupProbeProcess.WaitForExit(500) } catch { $null = $_ } + $exited = $false + try { $exited = $script:startupProbeProcess.HasExited } catch { $null = $_ } + if ($exited) { + # Exit does not mean both pipe readers finished; give them the same bounded wait. + $readers = @($script:startupProbeOutputTask, $script:startupProbeErrorTask) | Where-Object { $null -ne $_ } + if ($readers.Count -gt 0) { + try { [void][System.Threading.Tasks.Task]::WaitAll([System.Threading.Tasks.Task[]]$readers, 500) } catch { $null = $_ } + } + Complete-StartupHealthProbe + } else { + # Releasing the only reference would orphan a live child; keep it tracked. + Write-ActionLog "startup-health probe still running at shutdown; leaving it tracked" + } + } +} + function Initialize-UpdateBadgeReader { if (([System.Management.Automation.PSTypeName]"TrayUpdateBadgeReader").Type) { return } Add-Type -TypeDefinition @' @@ -433,6 +457,7 @@ $script:startupRefreshMs = 20000L $script:startupProbeProcess = $null $script:startupProbeOutputTask = $null $script:startupProbeErrorTask = $null +$script:startupProbeTerminating = $false $script:startupProbeStarted = 0L $script:startupProbeTimeoutMs = 30000L $script:updateAvailable = $false @@ -520,10 +545,16 @@ function Update-TrayState { try { $probeExited = $script:startupProbeProcess.HasExited } catch { - $probeExited = $true + # Failed inspection is not proof of exit; keep ownership and request termination. + Stop-StartupHealthProbe } $probeTimedOut = ($now - $script:startupProbeStarted) -gt $script:startupProbeTimeoutMs - if ($probeExited -and $null -ne $script:startupProbeOutputTask -and $script:startupProbeOutputTask.IsCompleted) { + $probeDrained = ($null -eq $script:startupProbeOutputTask -or $script:startupProbeOutputTask.IsCompleted) -and + ($null -eq $script:startupProbeErrorTask -or $script:startupProbeErrorTask.IsCompleted) + if ($script:startupProbeTerminating) { + if ($probeExited -and $probeDrained) { Complete-StartupHealthProbe } + elseif (-not $probeExited) { Stop-StartupHealthProbe } + } elseif ($probeExited -and $probeDrained -and $null -ne $script:startupProbeOutputTask) { $probeStartup = $null try { $probeStartup = Parse-StartupHealthText $script:startupProbeOutputTask.Result @@ -537,14 +568,8 @@ function Update-TrayState { # A hung diagnostic must not survive its refresh slot: terminate it so the # next refresh cannot stack another orphaned Bun process behind it. Write-ActionLog "startup-health probe timed out; terminating it" - try { - $script:startupProbeProcess.Kill() - [void]$script:startupProbeProcess.WaitForExit(3000) - } catch { - Write-ActionLog "startup-health probe kill failed: $($_.Exception.GetType().Name)" - } + Stop-StartupHealthProbe $script:startupHealth = $null - Complete-StartupHealthProbe $script:startupHealthCheckedAt = $now } } @@ -710,14 +735,8 @@ try { # A probe still running when the tray shuts down would outlive it: Dispose() # releases the handle but does not terminate the child, so kill the active # probe, wait briefly for exit, then release the probe state. - try { - Write-ActionLog "terminating active startup-health probe on tray shutdown" - $script:startupProbeProcess.Kill() - [void]$script:startupProbeProcess.WaitForExit(3000) - } catch { - Write-ActionLog "startup-health probe shutdown kill failed: $($_.Exception.GetType().Name)" - } - Complete-StartupHealthProbe + Write-ActionLog "terminating active startup-health probe on tray shutdown" + Stop-StartupHealthProbe -Shutdown } Stop-UpdateBadgeProbe -Shutdown $notify.Dispose() diff --git a/structure/codex-home.md b/structure/codex-home.md index 06e63acca16..5d07f5c55c4 100644 --- a/structure/codex-home.md +++ b/structure/codex-home.md @@ -445,7 +445,7 @@ queue or be misreported as evictable memory. One caller-owned retry is allowed a `ETIMEDOUT`; the first timeout does not install a `spill-failed` tombstone. Required ACL failures remain fail-closed after that bounded recovery. Optional config-directory hardening uses a separate per-directory async single-flight, while required config mutation writers retain their existing -awaited or synchronous fail-closed boundary. +awaited or synchronous fail-closed boundary. Snapshot persistence serializes the entire background pass, including its follow-up timer decision. Shutdown awaits that gate before consuming a pending pass; a stable bounded rewrite cancels the mutation timer whose bytes it incorporated. `tests/responses/responses-state-flush-race.test.ts` covers overlap with a background rewrite and leaves no redundant persistence timer after flush. Each ordinary async spill write attempt owns one 30-second ACL budget shared across directory, temp, and exclusive-copy destination hardening; the single timeout retry receives one fresh whole-attempt diff --git a/structure/config.md b/structure/config.md index feda166f9c5..f803d408c5c 100644 --- a/structure/config.md +++ b/structure/config.md @@ -62,7 +62,7 @@ test seam and the mutation types remain re-exported through `src/config.ts`. Suc `src/config/paths.ts` is the single owner of `OPENCODEX_HOME` expansion and resolution. It exposes the config directory and `config.json` path and retains the existing cache rule: a relative home is resolved once for each distinct raw environment value, so a later working-directory change cannot -silently move the active installation. +silently move the active installation. Config reads in this leaf and `src/config/load-degrade.ts` use NTFS ACLs on Windows and private POSIX modes elsewhere. Windows reads omit `chmod` before ACL hardening so repeated loads preserve successful object/freshness observations; external metadata changes still invalidate them. Directory flights keep their existing flush/reap lifecycle, and file/required-write failure policy is unchanged. `tests/windows/windows-config-read-acl-memo.test.ts` covers repeated loads, external invalidation and POSIX permissions. `src/config/process-state.ts` derives `ocx.pid` and `runtime-port.json` from that resolved directory. It owns their byte-compatible writes, parsing, expected-PID filters, cheap liveness, full OCX command diff --git a/structure/ops/test-sandbox-cleanup.md b/structure/ops/test-sandbox-cleanup.md index 531aa18f48b..daed1ef575f 100644 --- a/structure/ops/test-sandbox-cleanup.md +++ b/structure/ops/test-sandbox-cleanup.md @@ -39,3 +39,14 @@ blocks every removal; a root-specific drain failure preserves that root while ot can be removed. Removal failures do not stop later roots. Environment restoration runs in finally, and the first failure is rethrown. Undrained roots remain for run-level sandbox cleanup. `tests/ci-workflows/fixture-teardown-helper.test.ts` verifies ordering and failure gating with injected owners. + +`tests/helpers/spawn-test-child.ts` owns behavior and warm-up children on the parent's live +event loop. It captures output continuously, kills at the child deadline, and bounds exit/pipe +observation with a separate five-second grace. Results distinguish deadline expiry, observed +exit (`reaped`) and pipe closure; an unobserved exit does not prove fixture handles were released. +The affected catalog/status/routing fixtures retain roots after timeout or unproven exit/EOF +for sandbox recovery rather than deleting against uncertain child handles. Explicit environments +are complete allowlists. Catalog tick children drain their config ACL +flights/reaps before normal exit. Child work deadlines remain separate from outer test budgets, +which reserve time for termination and teardown. The shared warm-up regressions exercise a live +interval after work completes and a descendant retaining stdout after its parent exits. diff --git a/structure/runtime.md b/structure/runtime.md index bdea85e470e..2a31215cb52 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -192,7 +192,7 @@ described in [OpenAI quota ownership](providers/openai-tiers.md#public-provider- until shutdown. Normal shutdown restores native Codex; a sibling instance beside a live proxy ([Codex home](codex-home.md#codex-home)) syncs and restores nothing, and `ocx stop` of a runtime whose record carries `siblingOfPort` skips the shared teardown. `src/cli/index.ts` resolves same-home ownership, then checks shared client hints through the cross-home owner helper, then reconciles the journal only when no sibling is marked. Service mode sets `OCX_SERVICE=1`, so managed restarts do not repeatedly restore/reinject; explicit service stop and uninstall still restore. `src/service/cli.ts` removes the service token on uninstall only when persisted client state is disconnected and no pending connect marker owns the newly issued key. `src/client/connect.ts` publishes that fingerprint marker before the key, then clears it with the connection commit or rollback under the client lifecycle and config mutation locks. Connected, invalid, or mismatched client state retains an existing token. A valid pending marker retains only its matching fingerprint; an older marker does not own a replacement service key. An absent token is reported as absent; unsafe, malformed, or unreadable markers and lock, state-read, or deletion failures leave cleanup unverified. -`src/cli/tray-proxy.ts` coordinates `ocx restart` and tray restart. A second start requires proof the prior attempt never launched or its child exited, plus confirmed absence under a fresh bounded discovery window; a missing health response alone is insufficient. After an accepted restart, polling continues through uncertain observations until its deadline and requires a new runtime PID on the same port. If the old proxy then disappears, explicit recovery starts it regardless of the Codex autostart preference; a skipped recovery fails rather than reporting success. +`src/cli/tray-proxy.ts` coordinates `ocx restart` and tray restart. A second start requires proof the prior attempt never launched or its child exited, plus confirmed absence under a fresh bounded discovery window; a missing health response alone is insufficient. After an accepted restart, polling continues through uncertain observations until its deadline and requires a new runtime PID on the same port. If the old proxy then disappears, explicit recovery starts it regardless of the Codex autostart preference; a skipped recovery fails rather than reporting success. The Windows tray startup-health probe in `src/tray/windows-tray.ps1` requests termination at its 30-second deadline without waiting on the UI thread. It retains the process through later ticks until actual exit and both redirected readers settle; replacements cannot start while that reference is held. Launch setup failures follow the same cleanup. Tray shutdown alone waits at most 500 ms before disposal. The package-tree integrity fence for live package replacement follows the [update transaction contract](ops/docs-and-release.md#package-tree-integrity-fence). After client journal reconciliation, `ocx ensure` in `src/cli/dispatch.ts` treats validated connected-client state as a successful no-op without local startup or a remote health probe; invalid or mismatched state still fails, and disconnected state keeps local ensure. `tests/cli/cli-ensure-client-state.test.ts` pins this dispatch contract so the Codex shim does not warn merely because its proxy is delegated to a hub. Startup journal reconciliation, including replacement after restart/drain, follows the [Codex-home recovery contract](codex-home.md#codex-home): already-original recovery is silent, incomplete recovery retains its journal and warns without claiming restoration, and only complete recovery that rewrites or removes an artifact reports a restore; restart admission, drain deadlines and replacement ownership are unchanged. A busy preferred port is never resolved by starting somewhere else. Both questions a start asks @@ -208,7 +208,7 @@ is reported as such rather than called foreign, because an identity probe cannot foreign server from an unreachable one. An explicit `--port` still never hops — it waits for the pin through `src/server/port-reclaim.ts` — and a configured `port: 0` still means "ask the OS". A restart replacement that finds its own draining parent waits for it instead ([restart handoff](ops/service-and-sidecars.md#restart-handoff)). During drain, `src/server/index.ts` answers new data-plane work with HTTP 503 JSON `server_restarting`, `Retry-After: 5`, and receiving-listener CORS; the same handoff section owns that response contract. -Every `startServer` invocation acquires the `src/lib/spend-ledger-owner.ts` SQLite writer lease +CLI port selection in `src/cli/auxiliary-listener-preflight.ts` first probes explicitly ported auxiliary listeners on IPv4 loopback. A failed bind reports `AuxiliaryListenerBindError` before public-port reclaim, spend-owner acquisition, or management-token publication can run their Windows ACL subprocesses. The port-less companion has no independent probe, and hub management ingress is probed only in the hub role. This early rejection does not reserve a socket: `startServer` retains the actual bind and failed-start rollback for a port taken after preflight. Every `startServer` invocation acquires the `src/lib/spend-ledger-owner.ts` SQLite writer lease for its resolved OpenCodex state directory before loading configuration or binding a listener. References share one lease only inside one process and one directory; a different directory in that process is refused while the lease is held, because the shared ledger is process-wide. The diff --git a/tests/ci-workflows/cold-spawn-warmup.test.ts b/tests/ci-workflows/cold-spawn-warmup.test.ts index 367b14edacc..25cece9211f 100644 --- a/tests/ci-workflows/cold-spawn-warmup.test.ts +++ b/tests/ci-workflows/cold-spawn-warmup.test.ts @@ -12,6 +12,7 @@ import { } from "../helpers/cold-spawn-warmup"; import { repoPath, repoRoot } from "../helpers/repo-root"; import { INTERNAL_DEADLINE_MS, SPAWN_BUDGET_MS } from "../helpers/test-budget"; +import { spawnTestChild } from "../helpers/spawn-test-child"; import { analyzeWarmupRegistration, dispositionComplaints, @@ -285,16 +286,23 @@ describe("warm-up failure policy", () => { // SIGKILL at the deadline, then settle. const startedAt = performance.now(); const result = await spawnModuleGraphWarmupChild( - "setInterval(() => undefined, 60_000)", + 'process.stdout.write("work-complete\\n"); setInterval(() => undefined, 60_000)', repoRoot(), undefined, 1_000, ); expect(performance.now() - startedAt).toBeLessThan(INTERNAL_DEADLINE_MS); expect(result.timedOut).toBe(true); + expect(result.reaped).toBe(true); + expect(result.stdout).toContain("work-complete"); expect(result.exitCode).not.toBe(0); }); + test("a child that never starts rejects instead of resolving an empty result", async () => { + await expect(spawnTestChild([join(repoRoot(), "no-such-ocx-test-binary")], { cwd: repoRoot(), deadlineMs: 5_000 })) + .rejects.toThrow("the child could not be spawned"); + }); + test("a descendant holding the child's pipes does not turn exit into a wait for EOF", async () => { resetColdSpawnWarmupForTests(); // `close` is what a clean exit earns. A grandchild that keeps the write end open must not @@ -313,6 +321,8 @@ describe("warm-up failure policy", () => { expect(result.exitCode).toBe(0); expect(result.timedOut).toBe(false); expect(result.stdout).toContain("ok"); + expect(result.reaped).toBe(true); + expect(result.pipesClosed).toBe(false); }, INTERNAL_DEADLINE_MS); test("a real module graph loads, and reports what it loaded", async () => { @@ -324,3 +334,21 @@ describe("warm-up failure policy", () => { }); }, COLD_SPAWN_WARMUP_HOOK_BUDGET_MS); }); + +test("behavior-child runner preserves argv and a complete environment allowlist", async () => { + const previous = process.env.OCX_CHILD_EXCLUDED_FIXTURE; + process.env.OCX_CHILD_EXCLUDED_FIXTURE = "synthetic"; + try { + const result = await spawnTestChild([process.execPath, "--eval", + 'console.log(JSON.stringify({ argument: process.argv.at(-1), excluded: !!process.env.OCX_CHILD_EXCLUDED_FIXTURE })); console.error("fixture-error"); process.exit(7);', + "fixture-argument"], { + cwd: repoRoot(), env: { SystemRoot: process.env.SystemRoot }, deadlineMs: INTERNAL_DEADLINE_MS, + }); + expect(result).toMatchObject({ exitCode: 7, timedOut: false, reaped: true, pipesClosed: true }); + expect(JSON.parse(result.stdout)).toEqual({ argument: "fixture-argument", excluded: false }); + expect(result.stderr.trim()).toBe("fixture-error"); + } finally { + if (previous === undefined) delete process.env.OCX_CHILD_EXCLUDED_FIXTURE; + else process.env.OCX_CHILD_EXCLUDED_FIXTURE = previous; + } +}, SPAWN_BUDGET_MS); diff --git a/tests/cli/cli-route-policy-write.test.ts b/tests/cli/cli-route-policy-write.test.ts index ec98a2a1bac..8df7f104681 100644 --- a/tests/cli/cli-route-policy-write.test.ts +++ b/tests/cli/cli-route-policy-write.test.ts @@ -8,10 +8,13 @@ import type { RuntimeApiDeps } from "../../src/cli/runtime-api"; import { installIsolatedCodexHome, type IsolatedCodexHome } from "../helpers/isolated-codex-home"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { repoPath, repoRoot } from "../helpers/repo-root"; +import { spawnTestChild } from "../helpers/spawn-test-child"; +import { SPAWN_BUDGET_MS } from "../helpers/test-budget"; const sentinel = "synthetic-private-profile-value"; const committed = { status: "committed", changed: true, degraded: false, notices: [] }; let home: string, previousHome: string | undefined, codexHome: IsolatedCodexHome; +let childCompleted = true; let stdout: string[], stderr: string[], calls: { url: string; init: RequestInit }[], probes: number; let response: unknown, status: number; let logSpy: ReturnType, errorSpy: ReturnType, networkSpy: ReturnType; @@ -38,6 +41,7 @@ function deps(extra: Partial = {}): RuntimeApiDeps { } function noEcho() { expect(stdout.join("\n") + stderr.join("\n")).not.toContain(sentinel); } beforeEach(() => { + childCompleted = true; previousHome = process.env.OPENCODEX_HOME; home = mkdtempSync(join(tmpdir(), "ocx-cli-policy-write-")); process.env.OPENCODEX_HOME = home; codexHome = installIsolatedCodexHome("ocx-cli-policy-write-codex-"); @@ -49,7 +53,9 @@ beforeEach(() => { afterEach(() => { logSpy.mockRestore(); errorSpy.mockRestore(); networkSpy.mockRestore(); codexHome.restore(); if (previousHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousHome; - removeTreeWithRetry(home); + // A killed/unreaped child may still have descendants holding this root. Preserve it for + // run-level sandbox recovery instead of racing Windows removal against those handles. + if (childCompleted) removeTreeWithRetry(home); }); describe("routing profile write input", () => { @@ -252,12 +258,14 @@ test("actual routing editor preserves stale/create/update/alias/delete authority } finally { resetLabActivationForTests(); } `; const childHome = join(home, "real-handler"); - const child = Bun.spawn([process.execPath, "--eval", script], { cwd: repoRoot(), stdout: "pipe", stderr: "pipe", + childCompleted = false; + const result = await spawnTestChild([process.execPath, "--eval", script], { cwd: repoRoot(), deadlineMs: 20_000, env: { PATH: process.env.PATH, SystemRoot: process.env.SystemRoot, NODE_ENV: "test", HOME: childHome, USERPROFILE: childHome, APPDATA: childHome, XDG_CONFIG_HOME: childHome, OPENCODEX_HOME: childHome, CODEX_HOME: join(childHome, "codex"), NO_COLOR: "1" }, }); - const [out, err, exit] = await Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text(), child.exited]); - expect({ exit, err }).toEqual({ exit: 0, err: "" }); - expect(JSON.parse(out.trim())).toEqual({ scenarios: 9, saves: 3, refreshes: 3, requests: 9, automationEnabled: false }); -}, 20_000); + childCompleted = !result.timedOut && result.reaped && result.pipesClosed; + // Keep child diagnostics in the failure instead of leaving a live process after Bun times out. + expect(result).toMatchObject({ exitCode: 0, stderr: "", timedOut: false, reaped: true, pipesClosed: true }); + expect(JSON.parse(result.stdout.trim())).toEqual({ scenarios: 9, saves: 3, refreshes: 3, requests: 9, automationEnabled: false }); +}, SPAWN_BUDGET_MS); diff --git a/tests/cli/cli-start-auxiliary-bind.test.ts b/tests/cli/cli-start-auxiliary-bind.test.ts index 7f371130142..6c00cb0ad89 100644 --- a/tests/cli/cli-start-auxiliary-bind.test.ts +++ b/tests/cli/cli-start-auxiliary-bind.test.ts @@ -1,11 +1,12 @@ import { expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { findAvailablePort } from "../../src/server/ports"; import { repoPath } from "../helpers/repo-root"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { watchdogMs } from "../helpers/ci-watchdog"; +import { assertAuxiliaryListenerPortsAvailable } from "../../src/cli/auxiliary-listener-preflight"; const DEADLINE = watchdogMs(20_000); @@ -47,6 +48,10 @@ for (const listener of ["unauthenticatedLoopbackListener", "hub.managementIngres expect(output).toContain(`${listener} at 127.0.0.1:${auxiliaryPort}`); expect(output).not.toContain("picking another"); expect(output).not.toContain("waiting to retry the same port"); + // An impossible auxiliary bind must fail before unrelated private-state + // publication (and its synchronous Windows icacls subprocesses). + expect(existsSync(join(ocxHome, "spend-ledger-owner.sqlite"))).toBe(false); + expect(existsSync(join(ocxHome, "admin-api-token"))).toBe(false); const rebound = Bun.serve({ port: publicPort, hostname: "127.0.0.1", fetch: () => new Response("free") }); await rebound.stop(true); } finally { @@ -58,3 +63,41 @@ for (const listener of ["unauthenticatedLoopbackListener", "hub.managementIngres }, DEADLINE + 10_000); } } + +test("auxiliary preflight leaves free sockets available for the actual bind", async () => { + const loopbackPort = await findAvailablePort(0); + const managementPort = await findAvailablePort(0, "127.0.0.1", { reservedPort: loopbackPort }); + await assertAuxiliaryListenerPortsAvailable({ + port: 0, providers: {}, defaultProvider: "openai", runtimeRole: "hub", + unauthenticatedLoopbackListener: { enabled: true, port: loopbackPort }, + hub: { managementIngress: { enabled: true, port: managementPort } }, + }); + // Bind inside try: if another process takes the second port, the first server still stops. + const listeners: ReturnType[] = []; + try { + for (const port of [loopbackPort, managementPort]) { + listeners.push(Bun.serve({ hostname: "127.0.0.1", port, fetch: () => new Response("ok") })); + } + expect(listeners.map(server => server.port)).toEqual([loopbackPort, managementPort]); + } finally { await Promise.all(listeners.map(server => server.stop(true))); } +}); + +test("auxiliary preflight rejects two listeners configured for one port", async () => { + const port = await findAvailablePort(0); + await expect(assertAuxiliaryListenerPortsAvailable({ + port: 0, providers: {}, defaultProvider: "openai", runtimeRole: "hub", + unauthenticatedLoopbackListener: { enabled: true, port }, + hub: { managementIngress: { enabled: true, port } }, + })).rejects.toThrow(`hub.managementIngress at 127.0.0.1:${port}`); +}); + +test("auxiliary preflight ignores the companion, disabled listeners, and non-hub ingress", async () => { + const occupied = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: () => new Response("occupied") }); + try { + const base = { port: 0, providers: {}, defaultProvider: "openai", hub: { managementIngress: { enabled: true, port: occupied.port! } } }; + await assertAuxiliaryListenerPortsAvailable({ ...base, port: occupied.port!, unauthenticatedLoopbackListener: { enabled: true } }); + await assertAuxiliaryListenerPortsAvailable({ ...base, unauthenticatedLoopbackListener: { enabled: false, port: occupied.port! } }); + await assertAuxiliaryListenerPortsAvailable({ ...base, runtimeRole: "hub", hub: { managementIngress: { enabled: false, port: occupied.port! } } }); + expect(occupied.port).toBeGreaterThan(0); + } finally { await occupied.stop(true); } +}); diff --git a/tests/cli/cli-status-json.test.ts b/tests/cli/cli-status-json.test.ts index a7f05aef3e6..2260eca75f5 100644 --- a/tests/cli/cli-status-json.test.ts +++ b/tests/cli/cli-status-json.test.ts @@ -20,12 +20,14 @@ import { findDeadPid } from "../helpers/dead-pid"; import { COLD_SPAWN_WARMUP_HOOK_BUDGET_MS, warmColdSpawn } from "../helpers/cold-spawn-warmup"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { INTERNAL_DEADLINE_MS, SPAWN_BUDGET_MS, STORE_BUDGET_MS } from "../helpers/test-budget"; +import { spawnTestChild } from "../helpers/spawn-test-child"; import { inspectClientRotationRecoveryGate, readClientConnectionState } from "../../src/client/state"; import * as lifecycleLock from "../../src/client/lifecycle-lock"; import { writeDesktopDisconnectReceipt } from "../../src/claude/desktop-remote-store"; const repoRoot = dirname(fileURLToPath(new URL("../../package.json", import.meta.url))); const cliPath = join(repoRoot, "src", "cli", "index.ts"); +const undrainedStatusHomes = new Set(); function runStatusJson(opencodexHome: string) { return spawnSync(process.execPath, [cliPath, "status", "--json"], { @@ -62,7 +64,7 @@ async function withStatusVersionFixture( try { await server?.stop(true); } finally { - removeTreeWithRetry(home); + if (!undrainedStatusHomes.has(home)) removeTreeWithRetry(home); } } } @@ -72,28 +74,16 @@ async function runTimedStatus( codexHome: string, json: boolean, deadlineMs: number, -): Promise<{ stdout: string; stderr: string; exitCode: number; timedOut: boolean; elapsedMs: number }> { - const startedAt = performance.now(); - const child = Bun.spawn([process.execPath, cliPath, "status", ...(json ? ["--json"] : [])], { +): ReturnType { + // Until exit and pipe closure are observed, fixture removal has no release evidence. + undrainedStatusHomes.add(home); + const result = await spawnTestChild([process.execPath, cliPath, "status", ...(json ? ["--json"] : [])], { cwd: repoRoot, env: { ...process.env, OPENCODEX_HOME: home, CODEX_HOME: codexHome }, - stdout: "pipe", stderr: "pipe", + deadlineMs, }); - let timedOut = false; - const timer = setTimeout(() => { - timedOut = true; - child.kill("SIGKILL"); - }, deadlineMs); - try { - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(child.stdout).text(), new Response(child.stderr).text(), child.exited, - ]); - return { stdout, stderr, exitCode, timedOut, elapsedMs: performance.now() - startedAt }; - } finally { - clearTimeout(timer); - if (child.exitCode === null) child.kill("SIGKILL"); - await child.exited; - } + if (!result.timedOut && result.reaped && result.pipesClosed) undrainedStatusHomes.delete(home); + return result; } describe("status version skew projection", () => { @@ -106,7 +96,7 @@ describe("status version skew projection", () => { await warmColdSpawn("cli-index/status", async deadlineMs => { await withStatusVersionFixture(packageVersion(), "ocx-status-skew-cold-", async ({ home, codexHome }) => { const result = await runTimedStatus(home, codexHome, true, deadlineMs); - expect(result.timedOut).toBe(false); + expect(result).toMatchObject({ timedOut: false, reaped: true, pipesClosed: true }); expect({ exitCode: result.exitCode, stderr: result.stderr }).toEqual({ exitCode: 0, stderr: "" }); const parsed = JSON.parse(result.stdout); expect(parsed.schemaVersion).toBe(1); @@ -137,7 +127,7 @@ describe("status version skew projection", () => { `[status-version-skew] proxy=${proxyVersion ?? "undefined"} ` + `format=${json ? "json" : "human"} elapsedMs=${result.elapsedMs.toFixed(0)}`, ); - expect(result.timedOut).toBe(false); + expect(result).toMatchObject({ timedOut: false, reaped: true, pipesClosed: true }); // Preserve both gates while surfacing the child error when startup fails. expect({ exitCode: result.exitCode, stderr: result.stderr }).toEqual({ exitCode: 0, stderr: "" }); if (json) { diff --git a/tests/cli/cli-v2-runtime.test.ts b/tests/cli/cli-v2-runtime.test.ts index 9425a29b6f7..d8e516ba263 100644 --- a/tests/cli/cli-v2-runtime.test.ts +++ b/tests/cli/cli-v2-runtime.test.ts @@ -288,7 +288,8 @@ describe("live V2 real-handler refusal and incomplete convergence", () => { }) as typeof fetch }; expect(await cmdV2(["on", "--live", "--json"], { runtimeApi, execFile: () => { throw new Error("Local fallback is forbidden"); } })).toBe(1); - expect(requests).toBe(1); expect(toggles).toBe(1); + // Errors ride along so a refusal before the native toggle names its cause in CI. + expect({ requests, toggles, errors }).toMatchObject({ requests: 1, toggles: 1 }); if (failure === "native-failure") { expect(out).toEqual([]); expect(readFileSync(path, "utf8")).toBe(before); } else expect(readOutput()).toMatchObject({ enabled: true, catalogRefresh: { status: "failed", partialWrite: true } }); expect(out.join("\n") + errors.join("\n")).not.toContain(privateText); diff --git a/tests/clients/desktop-compatibility-authority.test.ts b/tests/clients/desktop-compatibility-authority.test.ts index f23e2ae70a0..8b3c142693c 100644 --- a/tests/clients/desktop-compatibility-authority.test.ts +++ b/tests/clients/desktop-compatibility-authority.test.ts @@ -170,5 +170,5 @@ describe("Codex Desktop compatibility authority lifecycle", () => { expect(Buffer.from(await windowsAuthorityKeyProtection.unprotect(encrypted))).toEqual(original); const corrupted = Uint8Array.from(encrypted); corrupted[corrupted.length - 1] ^= 1; await expect(windowsAuthorityKeyProtection.unprotect(corrupted)).rejects.toThrow("desktop_compatibility_key_protection_failed"); - }, 35_000); + }, 65_000); // three PowerShell calls, each bounded at 20s, plus margin }); diff --git a/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts b/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts index 167a0b24a7a..407b020ff22 100644 --- a/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts +++ b/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { spawnSync } from "node:child_process"; +import { spawnTestChild } from "../helpers/spawn-test-child"; +import { SPAWN_BUDGET_MS } from "../helpers/test-budget"; import { mkdirSync, mkdtempSync, readFileSync, realpathSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -687,7 +688,7 @@ describe("catalog auto-refresh drift heal", () => { expect(selectDriftHealCatalogPath(journalPath, defaultPath, path => join(openCodexHome, path))).toBe(alternatePath); }); - test("the drift branch passes a journaled non-default catalog to the injector", () => { + test("the drift branch passes a journaled non-default catalog to the injector", async () => { const root = mkdtempSync(join(tmpdir(), "ocx-heal-catalog-child-")); const home = join(root, "home"); const ocx = join(root, "ocx"); @@ -734,22 +735,25 @@ describe("catalog auto-refresh drift heal", () => { spyOn(console, "info").mockImplementation(() => {}); await scheduler.runCatalogAutoRefreshTickForTests(); process.stdout.write(JSON.stringify({ received })); + await require(${JSON.stringify(fileURLToPath(new URL("../../src/config/paths.ts", import.meta.url)))}).flushConfigDirHardeningAndReaps(process.env.OPENCODEX_HOME); `; + let childCompleted = false; try { - const result = spawnSync(process.execPath, ["--eval", script], { + const result = await spawnTestChild([process.execPath, "--eval", script], { cwd: process.cwd(), - env: { ...process.env, HOME: home, OPENCODEX_HOME: ocx, CODEX_HOME: codex, TMPDIR: tmp }, - encoding: "utf8", - timeout: 20_000, + env: { ...process.env, HOME: home, USERPROFILE: home, APPDATA: home, + OPENCODEX_HOME: ocx, CODEX_HOME: codex, TMPDIR: tmp, TMP: tmp, TEMP: tmp }, + deadlineMs: 20_000, }); - expect(result.status).toBe(0); + childCompleted = !result.timedOut && result.reaped && result.pipesClosed; + expect(result).toMatchObject({ exitCode: 0, timedOut: false, reaped: true, pipesClosed: true }); expect(JSON.parse(result.stdout)).toEqual({ received: join(realpathSync.native(codex), "alternate-catalog.json") }); } finally { - removeTreeWithRetry(root); + if (childCompleted) removeTreeWithRetry(root); } - }); + }, SPAWN_BUDGET_MS); - test("a missing catalog defers injection until catalog convergence creates one", () => { + test("a missing catalog defers injection until catalog convergence creates one", async () => { const root = mkdtempSync(join(tmpdir(), "ocx-heal-missing-catalog-")); const home = join(root, "home"); const ocx = join(root, "ocx"); @@ -795,23 +799,27 @@ describe("catalog auto-refresh drift heal", () => { deferred: info.some(line => line.includes("not re-injected this tick")) }; await scheduler.runCatalogAutoRefreshTickForTests(); process.stdout.write(JSON.stringify({ first, received, converges })); + await require(${JSON.stringify(fileURLToPath(new URL("../../src/config/paths.ts", import.meta.url)))}).flushConfigDirHardeningAndReaps(process.env.OPENCODEX_HOME); `; + let childCompleted = false; try { - const result = spawnSync(process.execPath, ["--eval", script], { + const result = await spawnTestChild([process.execPath, "--eval", script], { cwd: process.cwd(), - env: { ...process.env, HOME: home, OPENCODEX_HOME: ocx, CODEX_HOME: codex, TMPDIR: tmp }, - encoding: "utf8", timeout: 20_000, + env: { ...process.env, HOME: home, USERPROFILE: home, APPDATA: home, + OPENCODEX_HOME: ocx, CODEX_HOME: codex, TMPDIR: tmp, TMP: tmp, TEMP: tmp }, + deadlineMs: 20_000, }); - expect(result.status).toBe(0); + childCompleted = !result.timedOut && result.reaped && result.pipesClosed; + expect(result).toMatchObject({ exitCode: 0, timedOut: false, reaped: true, pipesClosed: true }); expect(JSON.parse(result.stdout)).toEqual({ first: { received: [], converges: 1, catalogExists: true, rootsStillMissing: true, deferred: true }, received: [join(realpathSync.native(codex), "opencodex-catalog.json")], converges: 2, }); } finally { - removeTreeWithRetry(root); + if (childCompleted) removeTreeWithRetry(root); } - }); + }, SPAWN_BUDGET_MS); test("invalid journal bytes and unsafe catalogs are read without mutation", () => { const journalPath = join(openCodexHome, "journal.json"); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index d662008849b..210221f4d50 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1010,6 +1010,7 @@ "responses-spill-inspection.test.ts": "responses", "responses-spill-orphan-sweep.test.ts": "responses", "responses-spill-shutdown-clock.test.ts": "responses", "responses-state-snapshot-select.test.ts": "responses", "responses-state-snapshot-stubs.test.ts": "responses", "responses-state-store-false.test.ts": "responses", "responses-state-write-amplification.test.ts": "responses", + "responses-state-flush-race.test.ts": "responses", "responses-state.test.ts": "responses", "responses-stateless-dangling-call-repair.test.ts": "responses", "responses-stream-tool-events.test.ts": "responses", "responses-terminal-repair.test.ts": "responses", "responses-tool-conformance.test.ts": "responses", "responses-tool-groups.test.ts": "responses", @@ -1186,6 +1187,7 @@ "windows-process-priority.test.ts": "windows", "windows-scheduler-install-verification.test.ts": "windows", "windows-secret-acl.test.ts": "windows", "windows-secret-acl-removal-flight.test.ts": "windows", + "windows-config-read-acl-memo.test.ts": "windows", "windows-service-mutation-lock.test.ts": "windows", "windows-service-wrappers.test.ts": "windows", "windows-text-decoding.test.ts": "windows", diff --git a/tests/helpers/cold-spawn-warmup.ts b/tests/helpers/cold-spawn-warmup.ts index 796a489d8de..d81310f510c 100644 --- a/tests/helpers/cold-spawn-warmup.ts +++ b/tests/helpers/cold-spawn-warmup.ts @@ -1,4 +1,4 @@ -import { spawn } from "node:child_process"; +import { spawnTestChild, TEST_CHILD_REAP_RESERVE_MS, type TestChildResult } from "./spawn-test-child"; import { readFileSync } from "node:fs"; import { dirname, isAbsolute, resolve } from "node:path"; import { repoRoot } from "./repo-root"; @@ -92,7 +92,7 @@ import { SPAWN_BUDGET_MS } from "./test-budget"; const WARMUP_TEARDOWN_RESERVE_MS = 15_000; /** Reaping the warm-up child after its deadline. */ -const WARMUP_REAP_RESERVE_MS = 5_000; +const WARMUP_REAP_RESERVE_MS = TEST_CHILD_REAP_RESERVE_MS; /** * Budget for the hook that performs a warm-up. This is `SPAWN_BUDGET_MS` because a warm-up IS a real @@ -202,108 +202,17 @@ export async function warmModuleGraph(options: ColdSpawnWarmup): Promise { return warmColdSpawn(options.graph, deadlineMs => runModuleGraphWarmup(options, deadlineMs)); } -export interface ModuleGraphWarmupResult { - stdout: string; - stderr: string; - exitCode: number | null; - signal: NodeJS.Signals | null; - timedOut: boolean; -} +export type ModuleGraphWarmupResult = TestChildResult; -/** - * Spawn the warm-up child asynchronously and bound it on a live event loop. - * - * A blocking `Bun.spawnSync` made its own `timeout` the only bound it could honour, and that - * turned out to be no bound at all: while the synchronous wait runs, the event loop is dead, so - * the calling hook's budget and the suite's per-test timeout freeze inside the same wait and - * nothing can report anything. Run 35511743422's macos 2/2 leg held that shape for eighteen - * silent minutes inside tests/clients/client-connect.test.ts before the job ceiling cut it and - * reported `cancelled` — a result the `ci` gate reads as failure rather than evidence. Whether - * the child or the spawn primitive wedged is not observable from the outside, so the bound here - * does not depend on either: SIGKILL at the deadline, a short reap grace, and the call settles - * with or without the child's exit or EOF. A child that outlives its kill — or a descendant - * holding its pipes — cannot turn a warm-up into an unbounded wait. - */ +/** Warm-up compatibility surface; bounded child ownership is shared with behavior tests. */ export function spawnModuleGraphWarmupChild( script: string, cwd: string, env: Record | undefined, deadlineMs: number, ): Promise { - const maxCaptureBytes = 1024 * 1024; - return new Promise((resolve, reject) => { - let child: ReturnType; - try { - child = spawn(process.execPath, ["--eval", script], { - cwd, - env: { ...process.env, ...env }, - stdio: ["ignore", "pipe", "pipe"], - }); - } catch { - reject(new Error("[cold-spawn-warmup] the warm-up child could not be spawned")); - return; - } - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - let bytes = 0; - let settled = false; - let timedOut = false; - let exitCode: number | null = null; - let signal: NodeJS.Signals | null = null; - let deadline: ReturnType | undefined; - let reap: ReturnType | undefined; - const finish = () => { - if (settled) return; - settled = true; - clearTimeout(deadline); - clearTimeout(reap); - child.stdout?.destroy(); - child.stderr?.destroy(); - child.unref(); - resolve({ - stdout: Buffer.concat(stdoutChunks).toString("utf8"), - stderr: Buffer.concat(stderrChunks).toString("utf8"), - exitCode, - signal, - timedOut, - }); - }; - const beginReapGrace = () => { - if (settled) return; - reap ??= setTimeout(finish, WARMUP_REAP_RESERVE_MS); - }; - const stop = () => { - if (settled || timedOut) return; - timedOut = true; - clearTimeout(deadline); - beginReapGrace(); - try { child.kill("SIGKILL"); } catch { /* The kill's own failure must not extend the wait. */ } - }; - const capture = (chunk: Buffer, into: Buffer[]) => { - if (settled || timedOut) return; - bytes += chunk.length; - if (bytes > maxCaptureBytes) { stop(); return; } - into.push(chunk); - }; - child.stdout?.on("data", (chunk: Buffer) => capture(chunk, stdoutChunks)); - child.stderr?.on("data", (chunk: Buffer) => capture(chunk, stderrChunks)); - child.stdout?.on("error", stop); - child.stderr?.on("error", stop); - // The child was never started or died at launch; there is nothing to reap. - child.on("error", finish); - child.once("exit", (code, exitSignal) => { - exitCode = code; - signal = exitSignal; - clearTimeout(deadline); - // A descendant retaining a pipe must not turn a clean exit into a wait for EOF. - beginReapGrace(); - }); - child.once("close", (code, exitSignal) => { - exitCode = code; - signal = exitSignal; - finish(); - }); - deadline = setTimeout(stop, deadlineMs); + return spawnTestChild([process.execPath, "--eval", script], { + cwd, env: { ...process.env, ...env }, deadlineMs, }); } diff --git a/tests/helpers/compaction-routing-fixtures.ts b/tests/helpers/compaction-routing-fixtures.ts index 1c2688692a0..3ae7cef893a 100644 --- a/tests/helpers/compaction-routing-fixtures.ts +++ b/tests/helpers/compaction-routing-fixtures.ts @@ -4,7 +4,7 @@ import { clearResponseStateForTests, flushResponseState } from "../../src/respon import { flushConfigDirHardeningForTests } from "../../src/config/paths"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import { closeRequestHistoryIndex } from "../../src/routing/history/indexer"; -import { removeTreeWithRetry } from "./remove-tree"; +import { removeTreeAfterBackgroundWork } from "./remove-tree"; /** * Config, request and upstream-response fixtures for the compaction-routing suite. @@ -39,7 +39,9 @@ export async function drainCompactionResponseState(): Promise { export async function removeCompactionFixture(path: string): Promise { await drainCompactionResponseState(); - removeTreeWithRetry(path); + // Routing through a pool account fires the pre-route quota prime without awaiting it, and + // that prime holds the native-main claim database under this tree until it settles. + await removeTreeAfterBackgroundWork(path); } export function keyProviderConfig(overrides: Partial = {}): OcxConfig { diff --git a/tests/helpers/remove-tree.ts b/tests/helpers/remove-tree.ts index 651b92b6fba..5dbf5f2e622 100644 --- a/tests/helpers/remove-tree.ts +++ b/tests/helpers/remove-tree.ts @@ -1,5 +1,7 @@ import { assertRemovalOutsideProtectedTrees } from "../../src/lib/test-home-guard"; -import { removeTestTempTree } from "../../scripts/test-temp"; +import { pendingBackgroundWork, settleBackgroundWork } from "../../src/lib/background-work"; +import { flushWindowsSecretAclReapsBeforeRemoval } from "../../src/lib/windows-secret-acl"; +import { REMOVE_RETRY_BUDGET_MS, removeTestTempTree, removeTestTempTreeAsync } from "../../scripts/test-temp"; type RemoveTreeWithRetryOptions = Readonly<{ remove?: (path: string) => void; @@ -18,11 +20,41 @@ type RemoveTreeWithRetryOptions = Readonly<{ * The retry policy itself lives in `scripts/test-temp` so the wrapper's own cleanup and every * fixture teardown wait the same way. They were separate schedules for one release, and the * duplicate is what let the shard-load failure in #4789 be fixed in one place and not the other. + * + * This variant blocks the event loop between attempts, so it cannot outwait a holder that is + * async work in this process. A failure while such work is in flight names it: that case + * belongs to {@link removeTreeAfterBackgroundWork}. */ export function removeTreeWithRetry( path: string, options: RemoveTreeWithRetryOptions = {}, ): void { assertRemovalOutsideProtectedTrees(path); - removeTestTempTree(path, options); + try { + removeTestTempTree(path, options); + } catch (error) { + const pending = pendingBackgroundWork(); + if (pending.length > 0 && error instanceof Error) { + error.message += ` (in-process background work still running: ${pending.join(", ")}; ` + + "use removeTreeAfterBackgroundWork so it can finish and release its handles)"; + } + throw error; + } +} + +/** + * Teardown for a case that started work it did not await (a lazy quota prime, a startup + * reconcile): settle that work, wait for any icacls child under the tree to be reaped, then + * remove with waits that yield, so an in-process handle holder can still close its files. + */ +export async function removeTreeAfterBackgroundWork(path: string): Promise { + assertRemovalOutsideProtectedTrees(path); + const { settled, pending } = await settleBackgroundWork({ timeoutMs: REMOVE_RETRY_BUDGET_MS }); + // Removing under a live writer either fails with a sharing violation or lets the writer + // recreate files after removal. Keep the tree and name the work instead. + if (!settled) { + throw new Error(`[test] not removing ${path}: in-process background work still running: ${pending.join(", ")}`); + } + await flushWindowsSecretAclReapsBeforeRemoval(path); + await removeTestTempTreeAsync(path); } diff --git a/tests/helpers/spawn-test-child.ts b/tests/helpers/spawn-test-child.ts new file mode 100644 index 00000000000..e0ce658a647 --- /dev/null +++ b/tests/helpers/spawn-test-child.ts @@ -0,0 +1,122 @@ +import { spawn } from "node:child_process"; + +/** Fits inside the caller's outer test/hook budget, separately from the child deadline. */ +export const TEST_CHILD_REAP_RESERVE_MS = 5_000; + +export interface TestChildResult { + stdout: string; + stderr: string; + exitCode: number | null; + signal: NodeJS.Signals | null; + timedOut: boolean; + reaped: boolean; + pipesClosed: boolean; + elapsedMs: number; +} + +/** + * Own a test child on the parent's live event loop. Kill at the deadline, then bound exit/EOF + * observation separately: descendants retaining pipes must not make a completed child hang. + * A result with reaped=false is NOT permission to remove a handle-bearing child's fixture. + * env, when supplied, is the complete environment; do not merge a caller's allowlist. + * Capture continuously (at most 1 MiB) so failure diagnostics survive a timeout. + */ +export function spawnTestChild( + argv: readonly string[], + options: { cwd: string; env?: NodeJS.ProcessEnv; deadlineMs: number }, +): Promise { + const { cwd, env, deadlineMs } = options; + const startedAt = performance.now(); + const maxCaptureBytes = 1024 * 1024; + return new Promise((resolve, reject) => { + let child: ReturnType; + try { + child = spawn(argv[0], argv.slice(1), { + cwd, + env, + stdio: ["ignore", "pipe", "pipe"], + }); + } catch { + reject(new Error("[test-child] the child could not be spawned")); + return; + } + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let bytes = 0; + let settled = false; + let timedOut = false; + let reaped = false; + let pipesClosed = false; + let exitCode: number | null = null; + let signal: NodeJS.Signals | null = null; + let spawned = false; + let deadline: ReturnType | undefined; + let reap: ReturnType | undefined; + const finish = () => { + if (settled) return; + settled = true; + clearTimeout(deadline); + clearTimeout(reap); + child.stdout?.destroy(); + child.stderr?.destroy(); + child.unref(); + resolve({ + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + exitCode, + signal, + timedOut, + reaped, + pipesClosed, + elapsedMs: performance.now() - startedAt, + }); + }; + const beginReapGrace = () => { + if (settled) return; + reap ??= setTimeout(finish, TEST_CHILD_REAP_RESERVE_MS); + }; + const stop = () => { + if (settled || timedOut) return; + timedOut = true; + clearTimeout(deadline); + beginReapGrace(); + try { child.kill("SIGKILL"); } catch { /* The kill's own failure must not extend the wait. */ } + }; + const capture = (chunk: Buffer, into: Buffer[]) => { + if (settled || timedOut) return; + bytes += chunk.length; + if (bytes > maxCaptureBytes) { stop(); return; } + into.push(chunk); + }; + child.stdout?.on("data", (chunk: Buffer) => capture(chunk, stdoutChunks)); + child.stderr?.on("data", (chunk: Buffer) => capture(chunk, stderrChunks)); + child.stdout?.on("error", stop); + child.stderr?.on("error", stop); + child.once("spawn", () => { spawned = true; }); + // Before "spawn" the child never started (missing executable): report it as a failure + // rather than an empty result. After it, an error is a failed kill; the reap grace bounds that. + child.on("error", (error: Error) => { + if (settled) return; + if (spawned) { beginReapGrace(); return; } + settled = true; + clearTimeout(deadline); + clearTimeout(reap); + reject(new Error(`[test-child] the child could not be spawned: ${error.message}`, { cause: error })); + }); + child.once("exit", (code, exitSignal) => { + reaped = true; + exitCode = code; + signal = exitSignal; + clearTimeout(deadline); + // A descendant retaining a pipe must not turn a clean exit into a wait for EOF. + beginReapGrace(); + }); + child.once("close", (code, exitSignal) => { + pipesClosed = true; + exitCode = code; + signal = exitSignal; + finish(); + }); + deadline = setTimeout(stop, deadlineMs); + }); +} diff --git a/tests/helpers/windows-tray-probe-child.ts b/tests/helpers/windows-tray-probe-child.ts new file mode 100644 index 00000000000..6a7163c86a2 --- /dev/null +++ b/tests/helpers/windows-tray-probe-child.ts @@ -0,0 +1,12 @@ +import { appendFileSync } from "node:fs"; + +const pidFile = process.env.OCX_PROBE_TEST_PID_FILE; +if (!pidFile) throw new Error("Missing OCX_PROBE_TEST_PID_FILE"); +const scenario = process.env.OCX_PROBE_TEST_SCENARIO; +// Readiness remains observable, including deliberately delayed initialization. +if (scenario === "Online") await Bun.sleep(1500); +appendFileSync(pidFile, `${process.pid}\n`); +if (scenario === "BadgeOverflowStdout") process.stdout.write("x".repeat(32768)); +if (scenario === "BadgeOverflowStderr") process.stderr.write("x".repeat(32768)); +if (scenario === "BadgeStaleFailure") process.exit(1); +await Bun.sleep(120_000); diff --git a/tests/helpers/windows-tray-probe-lifecycle-driver.ps1 b/tests/helpers/windows-tray-probe-lifecycle-driver.ps1 index 0101ad5190f..9a6f12621b7 100644 --- a/tests/helpers/windows-tray-probe-lifecycle-driver.ps1 +++ b/tests/helpers/windows-tray-probe-lifecycle-driver.ps1 @@ -43,6 +43,7 @@ $wanted = @( "Parse-StartupHealthText", "Start-StartupHealthProbe", "Complete-StartupHealthProbe", + "Stop-StartupHealthProbe", "Initialize-UpdateBadgeReader", "Parse-UpdateBadgeText", "Complete-UpdateBadgeProbe", @@ -116,6 +117,7 @@ $script:startupRefreshMs = 20000 $script:startupProbeProcess = $null $script:startupProbeOutputTask = $null $script:startupProbeErrorTask = $null +$script:startupProbeTerminating = $false $script:startupProbeStarted = 0 $script:startupProbeTimeoutMs = 30000 $script:updateAvailable = $false @@ -227,8 +229,8 @@ try { $observedAtAfterMaintenance = $script:updateBadgeObservedAt $updateItemVisibleAfterMaintenance = $updateItem.Visible $updateItemEnabledAfterMaintenance = $updateItem.Enabled - $terminatingAfterMaintenance = if ($badgeCase) { $script:updateBadgeTerminating } else { $false } - $trackedAfterMaintenance = if ($badgeCase) { $null -ne $script:updateBadgeProcess } else { $false } + $terminatingAfterMaintenance = if ($badgeCase) { $script:updateBadgeTerminating } else { $script:startupProbeTerminating } + $trackedAfterMaintenance = if ($badgeCase) { $null -ne $script:updateBadgeProcess } else { $null -ne $script:startupProbeProcess } $maxTickMs = $maintenanceMs $reapDeadline = [DateTimeOffset]::UtcNow.ToUnixTimeMilliseconds() + 2000 do { diff --git a/tests/lib/optional-desktop-upstream.test.ts b/tests/lib/optional-desktop-upstream.test.ts index 19d181b1649..820ceea9c67 100644 --- a/tests/lib/optional-desktop-upstream.test.ts +++ b/tests/lib/optional-desktop-upstream.test.ts @@ -60,9 +60,15 @@ for (const kind of ["http", "https", "socks5", "http-all", "https-all"] as const const origin = await listenFixture(upstream), proxy = await forwardProxy(transport, origin.port, proxyLeaf); const root = mkdtempSync(join(tmpdir(), "ocx-desktop-fetch-")), certificate = join(root, "fixture-ca.pem"); writeFileSync(certificate, ca.certPem); + // Windows environment names are case-insensitive: ALL_PROXY and all_proxy are ONE variable, so + // giving them different values left the winner to env-block ordering, and an unrelated new + // variable once flipped it and sent this child straight to the real chatgpt.com. Clear every + // spelling, then set only the routes this case means. + const env: Record = Object.fromEntries(Object.entries(process.env) + .filter(([name]) => !/^(https?_proxy|all_proxy|no_proxy)$/i.test(name))); + Object.assign(env, { NODE_EXTRA_CA_CERTS: certificate }, kind.endsWith("-all") ? { ALL_PROXY: proxy.url } : { HTTPS_PROXY: proxy.url }); const child = Bun.spawn([process.execPath, repoPath("tests/helpers/desktop-egress-worker.ts")], { - cwd: repoRoot(), stdout: "pipe", stderr: "pipe", - env: { ...process.env, NODE_EXTRA_CA_CERTS: certificate, HTTPS_PROXY: kind.endsWith("-all") ? "" : proxy.url, https_proxy: "", HTTP_PROXY: "", http_proxy: "", ALL_PROXY: kind.endsWith("-all") ? proxy.url : "", all_proxy: "", NO_PROXY: "", no_proxy: "" }, + cwd: repoRoot(), stdout: "pipe", stderr: "pipe", env, }); const timer = setTimeout(() => child.kill(), 10000); try { diff --git a/tests/lib/remove-tree-helper.test.ts b/tests/lib/remove-tree-helper.test.ts index a814d0b65fc..c1927e4ebef 100644 --- a/tests/lib/remove-tree-helper.test.ts +++ b/tests/lib/remove-tree-helper.test.ts @@ -1,10 +1,15 @@ import { describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { settleBackgroundWork, trackBackgroundWork } from "../../src/lib/background-work"; import { REMOVE_RETRY_BUDGET_MS, REMOVE_RETRY_MAX_DELAY_MS, removeRetrySchedule, + removeTestTempTreeAsync, } from "../../scripts/test-temp"; -import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { removeTreeAfterBackgroundWork, removeTreeWithRetry } from "../helpers/remove-tree"; function codedError(code: string, message = code): Error & { code: string } { return Object.assign(new Error(message), { code }); @@ -85,3 +90,43 @@ describe("removeRetrySchedule", () => { expect(removeRetrySchedule(10)).toEqual([]); }); }); +// The Windows EBUSY in native Codex pool compaction (#7088, #7091) was an in-process holder: an +// un-awaited quota prime kept a claim database open across an await, and teardown retried with +// Bun.sleepSync, which stops the loop the holder needed to close it. These cases pin that shape. +describe("removal while in-process work holds the tree", () => { + test("a blocking retry loop cannot outwait an async holder, and says what is running", async () => { + let held = true; + const holder = trackBackgroundWork("fixture-holder", Bun.sleep(1).then(() => { held = false; })); + expect(() => removeTreeWithRetry("ignored", { + remove: () => { if (held) throw codedError("EBUSY", "held"); }, + sleep: () => undefined, + })).toThrow(/in-process background work still running: fixture-holder/); + await holder; + expect(held).toBe(false); + }); + + test("the async removal yields between attempts so the holder can release", async () => { + let held = true; + void Bun.sleep(1).then(() => { held = false; }); + let attempts = 0; + await removeTestTempTreeAsync("ignored", { + delays: [5, 5, 5, 5, 5, 5, 5, 5], + remove: () => { attempts += 1; if (held) throw codedError("EBUSY", "held"); }, + }); + expect(held).toBe(false); + expect(attempts).toBeGreaterThan(1); + }); + + test("teardown settles tracked work before the first removal", async () => { + const root = mkdtempSync(join(tmpdir(), "ocx-remove-after-work-")); + let wroteLate = false; + void trackBackgroundWork("late-writer", Bun.sleep(10).then(() => { + writeFileSync(join(root, "late.json"), "{}"); + wroteLate = true; + })); + await removeTreeAfterBackgroundWork(root); + expect(wroteLate).toBe(true); + expect(existsSync(root)).toBe(false); + expect((await settleBackgroundWork({ timeoutMs: 0 })).pending).toEqual([]); + }); +}); diff --git a/tests/preload.ts b/tests/preload.ts index 53b112b5826..ee4f990913c 100644 --- a/tests/preload.ts +++ b/tests/preload.ts @@ -26,7 +26,7 @@ * what HOME says. `assertLiveServiceManagerAllowed` in `src/service.ts` is the guard for * that, armed by the same flag set below. */ -import { afterAll } from "bun:test"; +import { afterAll, afterEach } from "bun:test"; import { isTestHomeGuardArmed, protectedHomeForTests } from "../src/lib/test-home-guard"; import { createIsolatedTestEnvironment, LIVE_INSTALL_CREDENTIAL_ENV } from "../scripts/test"; import { @@ -131,8 +131,13 @@ const { flushWindowsSecretAclReapsBeforeRemoval, windowsSecretAclReapPendingAtOr // afterAll hook. Cleanup must drain existing producers rather than initialize their graph. const { flushConfigDirHardeningForTests } = await import("../src/config/paths"); const { flushNativeMainStartupReleases } = await import("../src/codex/native-profile-startup"); +const { settleBackgroundWork } = await import("../src/lib/background-work"); const cleanup = createTestSandboxCleanup({ drainProducers: async () => { + // Bounded: a task that never settles must not hang the worker. Throwing skips removal, + // so the root stays for ownership-checked stale recovery and the labels are reported. + const { settled, pending } = await settleBackgroundWork({ timeoutMs: 15_000 }); + if (!settled) throw new Error(`[test] background work never settled before final cleanup: ${pending.join(", ")}`); await flushNativeMainStartupReleases(); await flushConfigDirHardeningForTests(); }, @@ -142,3 +147,10 @@ const cleanup = createTestSandboxCleanup({ }); afterAll(cleanup.afterAll); process.on("exit", cleanup.onExit); +// Work a case started without awaiting (a lazy quota prime) settles inside that case. Left +// running, it writes the next case's state and holds files the next teardown has to remove. +// Bounded and non-fatal: a case that leaks work pays at most this wait and is named. +afterEach(async () => { + const { settled, pending } = await settleBackgroundWork({ timeoutMs: 2_000 }); + if (!settled) console.warn(`[test] background work outlived its case: ${pending.join(", ")}`); +}); diff --git a/tests/responses/responses-compaction-routing.test.ts b/tests/responses/responses-compaction-routing.test.ts index 301913d3a76..e1837a95816 100644 --- a/tests/responses/responses-compaction-routing.test.ts +++ b/tests/responses/responses-compaction-routing.test.ts @@ -17,7 +17,6 @@ import { externalTaskInputContent } from "../../src/responses/task-input"; import { looksLikeBackendCiphertext } from "../../src/server/responses/encrypted-payload"; import * as adapterResolveModule from "../../src/server/adapter-resolve"; import * as visionModule from "../../src/vision"; -import * as authApiModule from "../../src/codex/auth-api"; import { saveCodexAccountCredential } from "../../src/codex/account-store"; import { CODEX_QUOTA_PROBE_INTERVAL_MS, @@ -337,21 +336,6 @@ describe("native compact usage reporting", () => { describe("native Codex pool compaction", () => { beforeEach(takeSpendHome); - // handleResponsesCompact resolves its account through resolveCodexAuthContext, which fires the - // pre-route quota prime without awaiting it. That prime opens the native-main claim file under - // this case's CODEX_HOME and then awaits ACL hardening, which is an async subprocess on Windows. - // Teardown removes the directory with a synchronous retry loop, so the hardening can never finish - // and close the file, and every retry fails with EBUSY. The prime is single-flight across cases, - // so the failure lands on whichever case is tearing down while it runs. None of these cases - // assert on priming, so the module export the lazy import resolves through is stubbed here. - let primeSpy: ReturnType | undefined; - beforeEach(() => { - primeSpy = spyOn(authApiModule, "primeCodexPoolQuotas").mockResolvedValue(undefined); - }); - afterEach(() => { - primeSpy?.mockRestore(); - primeSpy = undefined; - }); test("ignores a retired Spark reset without cooling later compact requests", async () => { const testDir = mkdtempSync(join(tmpdir(), "ocx-compact-scope-")); const previousOpencodexHome = process.env.OPENCODEX_HOME; @@ -481,7 +465,7 @@ describe("native Codex pool compaction", () => { new Headers({ authorization: "Bearer main-token" }), config, "pool", - { modelId: "gpt-5.6-sol", primeCodexPoolQuotas: async () => {} }, + { modelId: "gpt-5.6-sol" }, ); expect(nextProbe).toMatchObject({ probeQuotaScope: "shared" }); releaseCodexAuthContextProbeLease(nextProbe); @@ -551,7 +535,7 @@ describe("native Codex pool compaction", () => { new Headers({ authorization: "Bearer main-token" }), config, "pool", - { modelId: "gpt-5.6-sol", primeCodexPoolQuotas: async () => {} }, + { modelId: "gpt-5.6-sol" }, ); expect(nextProbe).toMatchObject({ probeQuotaScope: "shared" }); releaseCodexAuthContextProbeLease(nextProbe); diff --git a/tests/responses/responses-state-flush-race.test.ts b/tests/responses/responses-state-flush-race.test.ts new file mode 100644 index 00000000000..abf2bb1ddab --- /dev/null +++ b/tests/responses/responses-state-flush-race.test.ts @@ -0,0 +1,90 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { + clearResponseStateForTests, + flushResponseState, + rememberResponseState, + responseStatePersistPendingForTests, + runPendingResponseStatePersistForTests, + setResponseStatePersistAttemptHookForTests, +} from "../../src/responses/state"; +import { createTempHome, type TempHome } from "../helpers/temp-home"; + +let home: TempHome; +beforeEach(() => { + home = createTempHome("ocx-state-flush-race-"); + clearResponseStateForTests(); +}); +afterEach(async () => { + setResponseStatePersistAttemptHookForTests(null); + await flushResponseState(); + clearResponseStateForTests(); + home.remove(); +}); + +function remember(id: string): void { + rememberResponseState( + { model: "test/model", input: id, store: false }, + { id, output: [{ type: "message", role: "assistant", content: id }], status: "completed" }, + undefined, + { force: true }, + ); +} + +test("shutdown consumes a background writer's follow-up before returning", async () => { + remember("resp_before_background"); + setResponseStatePersistAttemptHookForTests(() => { + setResponseStatePersistAttemptHookForTests(null); + remember("resp_during_background"); + }); + const background = runPendingResponseStatePersistForTests(); + await flushResponseState(); + await background; + + expect(responseStatePersistPendingForTests()).toBe(false); + const snapshot = JSON.parse(readFileSync(home.path("responses-state.json"), "utf8")); + expect(snapshot.states.map(([id]: [string, unknown]) => id)).toContain("resp_during_background"); +}); + +test("a stable shutdown rewrite consumes the timer armed by its own mutation", async () => { + remember("resp_before_flush"); + setResponseStatePersistAttemptHookForTests(() => { + setResponseStatePersistAttemptHookForTests(null); + remember("resp_during_flush"); + }); + await flushResponseState(); + + expect(responseStatePersistPendingForTests()).toBe(false); + const snapshot = JSON.parse(readFileSync(home.path("responses-state.json"), "utf8")); + expect(snapshot.states.map(([id]: [string, unknown]) => id)).toContain("resp_during_flush"); +}); + +test("concurrent shutdown flushes both settle behind the background timer decision", async () => { + remember("resp_concurrent_seed"); + setResponseStatePersistAttemptHookForTests(() => { + setResponseStatePersistAttemptHookForTests(null); + remember("resp_concurrent_mutation"); + }); + const background = runPendingResponseStatePersistForTests(); + await Promise.all([flushResponseState(), flushResponseState()]); + await background; + + expect(responseStatePersistPendingForTests()).toBe(false); + const snapshot = JSON.parse(readFileSync(home.path("responses-state.json"), "utf8")); + expect(snapshot.states.map(([id]: [string, unknown]) => id)).toContain("resp_concurrent_mutation"); +}); + +test("a mutation after the stable revision check retains its own pending pass", async () => { + remember("resp_before_stable_check"); + setResponseStatePersistAttemptHookForTests(() => { + setResponseStatePersistAttemptHookForTests(null); + queueMicrotask(() => remember("resp_after_stable_check")); + }); + await runPendingResponseStatePersistForTests(); + + expect(responseStatePersistPendingForTests()).toBe(true); + await flushResponseState(); + const snapshot = JSON.parse(readFileSync(home.path("responses-state.json"), "utf8")); + expect(snapshot.states.map(([id]: [string, unknown]) => id)).toContain("resp_after_stable_check"); + expect(responseStatePersistPendingForTests()).toBe(false); +}); diff --git a/tests/windows/windows-config-read-acl-memo.test.ts b/tests/windows/windows-config-read-acl-memo.test.ts new file mode 100644 index 00000000000..ec7ed5160a2 --- /dev/null +++ b/tests/windows/windows-config-read-acl-memo.test.ts @@ -0,0 +1,100 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { chmodSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { loadConfig } from "../../src/config"; +import { hardenExistingSecret } from "../../src/config/load-degrade"; +import { flushConfigDirHardeningForTests, hardenConfigDir } from "../../src/config/paths"; +import * as acl from "../../src/lib/windows-secret-acl"; +import * as principal from "../../src/lib/windows-user-principal"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +const originalPlatform = process.platform; +let home: string; +let previousHome: string | undefined; +let previousVerify: string | undefined; +const calls: string[] = []; + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + previousVerify = process.env.OPENCODEX_ACL_VERIFY_EXISTING; + home = mkdtempSync(join(tmpdir(), "ocx-config-read-acl-")); + process.env.OPENCODEX_HOME = home; + delete process.env.OPENCODEX_ACL_VERIFY_EXISTING; + writeFileSync(join(home, "config.json"), JSON.stringify({ port: 19481, defaultProvider: "openai", providers: {} })); + writeFileSync(join(home, "auth.json"), "{}"); + acl.resetHardenedStateForTests(); + acl.setPlatformForTests("win32"); + Object.defineProperty(process, "platform", { value: "win32", configurable: true }); + const identity = () => ({ success: true, exitCode: 0, timedOut: false, + stdout: "S-1-5-21-1-2-3-1001\nfixture\\user\n" }); + principal.setWindowsPrincipalRunnerForTests(identity); + principal.setAsyncWindowsPrincipalRunnerForTests(async () => identity()); + calls.length = 0; + const run = (args: string[], _timeout?: number): acl.IcaclsResult => { + calls.push(args[0]); + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }; + acl.setIcaclsRunnerForTests(run); + acl.setAsyncIcaclsRunnerForTests(async (args, timeout) => run(args, timeout)); +}); + +afterEach(async () => { + await flushConfigDirHardeningForTests(); + Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }); + acl.setIcaclsRunnerForTests(null); + acl.setAsyncIcaclsRunnerForTests(null); + acl.setPlatformForTests(null); + principal.setWindowsPrincipalRunnerForTests(null); + principal.setAsyncWindowsPrincipalRunnerForTests(null); + principal.resetWindowsPrincipalForTests(); + acl.setStatForTests(null); + acl.resetHardenedStateForTests(); + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + if (previousVerify === undefined) delete process.env.OPENCODEX_ACL_VERIFY_EXISTING; + else process.env.OPENCODEX_ACL_VERIFY_EXISTING = previousVerify; + removeTreeWithRetry(home); +}); + +test("repeated Windows config reads preserve successful ACL observations", async () => { + expect(loadConfig().port).toBe(19481); + await flushConfigDirHardeningForTests(); + const first = [...calls]; + for (const path of [home, join(home, "config.json"), join(home, "auth.json")]) { + expect(first.filter(value => value === path).length).toBeGreaterThan(0); + } + const before = readFileSync(join(home, "config.json"), "utf8"); + expect(loadConfig().port).toBe(19481); + await flushConfigDirHardeningForTests(); + expect(calls).toEqual(first); + expect(readFileSync(join(home, "config.json"), "utf8")).toBe(before); +}); + +test("an external metadata change still invalidates a Windows file ACL observation", () => { + const path = join(home, "auth.json"); + let freshness = 1n; + acl.setStatForTests(() => ({ dev: 1n, ino: 1n, ctimeNs: freshness })); + hardenExistingSecret(path); + const first = calls.length; + // Exercise the production freshness check instead of weakening its cache identity. + freshness += 1n; + hardenExistingSecret(path); + expect(calls.length).toBeGreaterThan(first); +}); + +test("POSIX config reads still apply private directory and file permissions", () => { + Object.defineProperty(process, "platform", { value: "darwin", configurable: true }); + acl.setPlatformForTests("darwin"); + chmodSync(home, 0o755); + const path = join(home, "auth.json"); + chmodSync(path, 0o644); + hardenConfigDir(); + hardenExistingSecret(path); + // Windows does not implement these mode bits; that is the branch being simulated above. + if (originalPlatform !== "win32") { + expect(statSync(home).mode & 0o777).toBe(0o700); + expect(statSync(path).mode & 0o777).toBe(0o600); + } + expect(calls).toEqual([]); +}); diff --git a/tests/windows/windows-tray.test.ts b/tests/windows/windows-tray.test.ts index 59e3127f362..cce64c7fafa 100644 --- a/tests/windows/windows-tray.test.ts +++ b/tests/windows/windows-tray.test.ts @@ -413,6 +413,8 @@ describe("Windows tray packaging and command safety", () => { // child whose stdout pipe is drained asynchronously; the 3s tick only touches the // completed read task, so a slow or hung diagnostic can never freeze the tray menu. expect(source).toContain("ReadToEndAsync()"); + const tick = source.slice(source.indexOf("function Update-TrayState"), source.indexOf("$timer = New-Object")); + expect(tick).not.toContain(".WaitForExit("); expect(source).toContain("$script:startupProbeProcess"); // A timed-out diagnostic must be terminated, not left to become an orphaned // Bun process on the next refresh. @@ -425,7 +427,9 @@ describe("Windows tray packaging and command safety", () => { // A probe still in flight when the tray shuts down must not outlive it: the // finally block kills the active probe, waits briefly, then completes it. expect(source).toContain("terminating active startup-health probe on tray shutdown"); - expect(source).toContain("startupProbeProcess.WaitForExit(3000)"); + expect(source).toContain("Stop-StartupHealthProbe -Shutdown"); + const stopProbe = source.slice(source.indexOf("function Stop-StartupHealthProbe"), source.indexOf("function Initialize-UpdateBadgeReader")); + expect(stopProbe).toMatch(/if \(\$Shutdown\) \{[\s\S]*WaitForExit\(500\)/); // Placement proof that runs on every platform: the behavioral test below is // win32-only and the Windows CI leg runs on dispatch rather than on PRs, // so merge-time coverage needs a lightweight check here too. The timeout @@ -448,9 +452,10 @@ describe("Windows tray packaging and command safety", () => { // server-side parser instead of accepting any non-null rebootSafe value. expect(source).toContain("($parsed.rebootSafe -is [bool])"); // If the async pipe setup fails after the child started, the child must be - // terminated, not merely disposed and lost. That catch block logs a - // distinct string, which breaks if the branch is deleted. - expect(source).toContain("startup-health probe launch cleanup failed"); + // terminated, not merely disposed and lost. The catch retains the process + // through the same later-tick reap as an ordinary timeout. + const launchCatch = source.slice(source.indexOf('Write-ActionLog "startup-health probe launch failed'), source.indexOf("function Complete-StartupHealthProbe")); + expect(launchCatch).toContain("Stop-StartupHealthProbe"); }); test("drops CODEX_HOME for tray children only when the default home is still missing", () => { @@ -514,19 +519,9 @@ describe("Windows tray packaging and command safety", () => { const openCodexHome = join(directory, "ohome"); mkdirSync(codexHome, { recursive: true }); mkdirSync(openCodexHome, { recursive: true }); - // The fake CLI name stays simple ASCII on purpose: the child engine here - // is powershell.exe (not bun), and its -Command parsing mangles paths - // with spaces or `&` even when quoted. - const hangChild = join(directory, "hangchild.ps1"); - writeFileSync(hangChild, [ - // Prove readiness is observed, not guessed from a fixed 500ms sleep. - ...(scenario === "Online" ? ["Start-Sleep -Milliseconds 1500"] : []), - "$pidFile = $env:OCX_PROBE_TEST_PID_FILE", - "if ($pidFile) { Add-Content -LiteralPath $pidFile -Value $PID }", - ...(scenario === "BadgeOverflowStdout" ? ["[Console]::Out.Write('x' * 32768)"] : []), - ...(scenario === "BadgeOverflowStderr" ? ["[Console]::Error.Write('x' * 32768)"] : []), - ...(scenario === "BadgeStaleFailure" ? ["exit 1"] : ["Start-Sleep -Seconds 120"]), - ].join("\r\n")); + // Production probes run Bun. A nested PowerShell cold start used to consume + // the readiness deadline before any lifecycle assertion (run 38503986851). + const hangChild = helperPath("windows-tray-probe-child.ts"); const pidFile = join(directory, "pids.txt"); const resultPath = join(directory, "verdict.json"); let server: ReturnType | undefined; @@ -558,7 +553,7 @@ describe("Windows tray packaging and command safety", () => { "-NoLogo", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-File", driver, "-TrayScriptPath", trayScript, - "-ChildEnginePath", psExe, + "-ChildEnginePath", process.execPath, "-HangChildPath", hangChild, "-CodexHome", codexHome, "-OpenCodexHome", openCodexHome, @@ -567,7 +562,7 @@ describe("Windows tray packaging and command safety", () => { ], { stdout: "pipe", stderr: "pipe", - env: { ...process.env, OCX_PROBE_TEST_PID_FILE: pidFile }, + env: { ...process.env, OCX_PROBE_TEST_PID_FILE: pidFile, OCX_PROBE_TEST_SCENARIO: scenario }, }); const stdoutPromise = new Response(child.stdout).text(); const stderrPromise = new Response(child.stderr).text(); @@ -613,6 +608,10 @@ describe("Windows tray packaging and command safety", () => { expect(verdict.childTerminated, `${scenario}: hung probe child ${verdict.childPid} survived the timeout`).toBe(true); expect(verdict.probeCleared, `${scenario}: probe reference was not released after the kill`).toBe(true); expect(verdict.launches, `${scenario}: expected exactly 1 probe launch, saw ${verdict.launches}`).toBe(1); + if (scenario !== "BadgeStaleFailure") { + expect(verdict.terminatingAfterMaintenance, `${scenario}: no terminating state after kill`).toBe(true); + expect(verdict.trackedAfterMaintenance, `${scenario}: disposed on the timeout tick`).toBe(true); + } if (scenario.startsWith("Badge")) { expect(verdict.maxTickMs, `${scenario}: UI tick blocked`).toBeLessThan(250); if (scenario === "BadgeStaleFailure") { @@ -627,8 +626,7 @@ describe("Windows tray packaging and command safety", () => { expect(verdict.updateItemVisibleAfterMaintenance).toBe(false); expect(verdict.updateItemEnabledAfterMaintenance).toBe(false); } else { - expect(verdict.terminatingAfterMaintenance, `${scenario}: no terminating state after kill`).toBe(true); - expect(verdict.trackedAfterMaintenance, `${scenario}: disposed on the timeout tick`).toBe(true); + expect(verdict.observedAtAfterMaintenance).toBe(verdict.observedAtBeforeMaintenance); } } else { expect(verdict.totalMs, `${scenario}: two ticks took ${verdict.totalMs}ms; a UI-thread block would hang until the 120s sleeper exits`).toBeLessThan(20_000); @@ -650,6 +648,53 @@ describe("Windows tray packaging and command safety", () => { } }, { timeout: SPAWN_BUDGET_MS }); + test("Bun probe fixture reports readiness and real overflow/failure on every platform", async () => { + for (const scenario of ["Offline", "Online", "BadgeOverflowStdout", "BadgeOverflowStderr", "BadgeStaleFailure"]) { + const directory = mkdtempSync(join(tmpdir(), "ocx-tray-fixture-")); + const pidFile = join(directory, "pids.txt"); + const child = Bun.spawn([process.execPath, helperPath("windows-tray-probe-child.ts"), "__startup-health"], { + env: { ...process.env, OCX_PROBE_TEST_PID_FILE: pidFile, OCX_PROBE_TEST_SCENARIO: scenario }, + stdout: "pipe", stderr: "pipe", + }); + let overflowReady!: () => void; + const overflow = new Promise(resolve => { overflowReady = resolve; }); + const drain = async (stream: ReadableStream) => { + let size = 0; + for await (const chunk of stream) { + size += chunk.length; + if (size >= 32768) overflowReady(); + } + return size; + }; + const stdout = drain(child.stdout); + const stderr = drain(child.stderr); + try { + const deadline = Date.now() + INTERNAL_DEADLINE_MS; + while (!existsSync(pidFile) && child.exitCode === null && Date.now() < deadline) await Bun.sleep(10); + expect(existsSync(pidFile), scenario).toBe(true); + expect(readFileSync(pidFile, "utf8").trim(), scenario).toBe(String(child.pid)); + if (scenario.startsWith("BadgeOverflow")) { + expect(await Promise.race([overflow.then(() => true), Bun.sleep(INTERNAL_DEADLINE_MS).then(() => false)]), scenario).toBe(true); + } + if (scenario === "BadgeStaleFailure") expect(await child.exited).toBe(1); + else { + expect(child.exitCode, scenario).toBeNull(); + child.kill(); + await child.exited; + } + const sizes = await Promise.all([stdout, stderr]); + expect(sizes).toEqual([ + scenario === "BadgeOverflowStdout" ? 32768 : 0, + scenario === "BadgeOverflowStderr" ? 32768 : 0, + ]); + } finally { + if (child.exitCode === null) child.kill(); + await Promise.all([child.exited, stdout, stderr]); + removeTreeWithRetry(directory); + } + } + }, SPAWN_BUDGET_MS); + // This test really does launch PowerShell, which really does launch a Bun child, and // then rebinds the port to prove the child did not inherit the listen socket. Those // processes ARE the assertion — there is no version of this proof that fakes them.