lidge-jun · Sep 16, 2026 · Sep 16, 2026 · Sep 16, 2026 · Sep 16, 2026 · Sep 16, 2026
@@ -1309,6 +1309,7 @@
     "sse-payload-rewrite.test.ts": "responses",
     "sse-unspaced-data-fields.test.ts": "responses",
     "stale-state-purge.test.ts": "service",
+    "stall-subprocess-exit.test.ts": "lib",
     "stall-timeout.test.ts": "lib",
     "star-deferral.test.ts": "cli",
     "startup-action-control-elevation.test.ts": "server",

@@ -308,6 +308,47 @@ function observeOwner(entry: StartupEntry, owner: NativeMainOwnerSnapshot): void
  * Retain process ownership for one live server. The first reference acquires the
  * canonical-home SQLite lease and owns recovery; later same-process references share it.
  */
+/**
+ * Releases nobody is awaiting.
+ *
+ * A release closes the native-main owner's SQLite lease and its stable lock file, both of which
+ * live under CODEX_HOME. The normal shutdown path awaits it: `server.stop` goes through
+ * `releaseNativeMainStartupLifecycle`, which awaits the flight. The FAILED-start path does not —
+ * `startServer` must stay synchronous, so its rollback can only fire `void lifecycle.release()`
+ * and rethrow. Nothing could then wait for those handles to close, and on Windows an open handle
+ * does not delay an unlink, it refuses it outright with EPERM.
+ *
+ * That is invisible in production, where a failed start is followed by exit rather than by
+ * deleting the home. It is not invisible to a test whose cleanup removes the home it just used:
+ * `tests/server/server-management-auth.test.ts` binds a management ingress on the fixed port
+ * 10101, which nine other test files also use, so a collision on the six-shard Windows leg turns
+ * a passing start into the rollback path. The failure followed that collision across shards 1, 2
+ * and 3 while staying on the same file and line, which is what a shard-independent trigger looks
+ * like.
+ *
+ * Tracking the flight here rather than at the call site keeps `startServer` synchronous and
+ * unchanged, and gives anyone who needs the handles closed something to await.
+ */
+const pendingStartupReleases = new Set<Promise<void>>();
+
+function trackStartupRelease(release: Promise<void>): Promise<void> {
+  const tracked = release.finally(() => { pendingStartupReleases.delete(tracked); });
+  pendingStartupReleases.add(tracked);
+  return tracked;
+}
+
+/**
+ * Settle every native-main startup release still in flight, including ones nobody awaited.
+ *
+ * Loops rather than awaiting a single snapshot: a release can retire an owner whose own teardown
+ * starts another, and draining only the first batch would return with handles still open.
+ */
+export async function flushNativeMainStartupReleases(): Promise<void> {
+  while (pendingStartupReleases.size > 0) {
+    await Promise.allSettled([...pendingStartupReleases]);
+  }
+}
+
 export function startNativeMainStartupLifecycle(
   deps: NativeMainStartupGateDeps = {},
 ): NativeMainStartupLifecycle {
@@ -352,29 +393,32 @@ export function startNativeMainStartupLifecycle(
   }
   entry.refs += 1;
   let released = false;
+  const performRelease = async (): Promise<void> => {
+    if (released) return;
+    released = true;
+    entry!.refs = Math.max(0, entry!.refs - 1);
+    if (entry!.refs !== 0) return;
+    entry!.epoch += 1;
+    entry!.sweepStopping = true;
+    if (entry!.sweepTimer) clearTimeout(entry!.sweepTimer);
+    entry!.sweepTimer = undefined;
+    entry!.unsubscribe();
+    startupEntries.delete(homeId);
+    entry!.resolveAcquisition?.(snapshot);
+    entry!.resolveAcquisition = undefined;
+    // Startup convergence can transition from the exclusive recovery claim
+    // into a stage sweep. Keep the owner registered until that entire chain
+    // settles so no cleanup transaction starts untracked after owner detach.
+    await Promise.allSettled([entry!.settled]);
+    if (entry!.sweepInFlight) await Promise.allSettled([entry!.sweepInFlight]);
+    await entry!.owner.release();
+  };
   return {
     homeId,
     get settled() { return entry!.settled; },
-    async release() {
-      if (released) return;
-      released = true;
-      entry!.refs = Math.max(0, entry!.refs - 1);
-      if (entry!.refs !== 0) return;
-      entry!.epoch += 1;
-      entry!.sweepStopping = true;
-      if (entry!.sweepTimer) clearTimeout(entry!.sweepTimer);
-      entry!.sweepTimer = undefined;
-      entry!.unsubscribe();
-      startupEntries.delete(homeId);
-      entry!.resolveAcquisition?.(snapshot);
-      entry!.resolveAcquisition = undefined;
-      // Startup convergence can transition from the exclusive recovery claim
-      // into a stage sweep. Keep the owner registered until that entire chain
-      // settles so no cleanup transaction starts untracked after owner detach.
-      await Promise.allSettled([entry!.settled]);
-      if (entry!.sweepInFlight) await Promise.allSettled([entry!.sweepInFlight]);
-      await entry!.owner.release();
-    },
+    // Tracked so a caller that cannot await -- the synchronous rollback in `startServer` -- still
+    // leaves the flight drainable through `flushNativeMainStartupReleases`.
+    release: () => trackStartupRelease(performRelease()),
   };
 }
 

@@ -9,28 +9,78 @@ export interface BoundedSubprocessExit {
   timedOut: boolean;
 }
 
-/** Kill at the deadline and abandon immediately; late exit/rejection remains observed. */
+/**
+ * How long to wait for a killed child to actually be reaped before abandoning it.
+ *
+ * `kill()` only REQUESTS termination. It returns before the kernel has torn the process down, and
+ * every handle that process holds stays held until it does. On Windows that is not a detail: file
+ * locking is mandatory, so a directory an abandoned `icacls.exe` still has open cannot be removed
+ * by anyone, and the removal fails with EPERM rather than waiting.
+ */
+export const SUBPROCESS_KILL_GRACE_MS = 2_000;
+
+/**
+ * Wait for a child, bounded. At the deadline, kill it AND wait for it to actually die.
+ *
+ * This used to kill, `unref`, and resolve in the same tick, which made every caller's "I waited
+ * for my child" guarantee false precisely when it mattered. `flushConfigDirHardening` exists so
+ * shutdown owns every `icacls.exe` it started; it awaited a promise that had already settled while
+ * the child was still alive, so the contract read as satisfied and the directory stayed locked.
+ *
+ * That cost three failed fixes. #4789 blamed the removal retry budget and asked for more than
+ * 2.5s; #4796 gave it a 15s exponential schedule; a later change awaited the hardening flight from
+ * the test hook. Windows shard 1/6 failed identically through all three, because none of them
+ * addressed a live process holding the handle -- run 35108652486 burned the full 15s budget and
+ * still threw `EPERM ... rm ocx-management-auth-fDchUb`, with two
+ * `ACL hardening timed out (ETIMEDOUT) - transient icacls stall` lines logged beside it.
+ *
+* The grace is bounded and abandonment is still the fallback, so a genuinely unkillable child
+* cannot hang shutdown. The classification does not move: a child that missed its deadline is
+* reported as timed out whether or not it dies during the grace, because it did time out. Only the
+* moment of resolution changes.
+ *
+ * Pass `0` to opt out. The grace buys exactly one thing -- a handle released before somebody
+ * removes the path holding it -- so a caller whose child holds nothing anyone deletes should not
+ * pay for it. `windows-user-principal` is that caller: its PowerShell lookup sits on the startup
+ * critical path, where seconds are the scarce resource and no directory is waiting on the reap.
+ */
 export function waitForSubprocessExit(
   proc: KillableSubprocess,
   timeoutMs: number,
+  killGraceMs: number = SUBPROCESS_KILL_GRACE_MS,
 ): Promise<BoundedSubprocessExit> {
   return new Promise(resolve => {
     let settled = false;
+    let deadlineFired = false;
     let timer: ReturnType<typeof setTimeout> | undefined;
+    let graceTimer: ReturnType<typeof setTimeout> | undefined;
     const finish = (result: BoundedSubprocessExit): void => {
       if (settled) return;
       settled = true;
       if (timer !== undefined) clearTimeout(timer);
+      if (graceTimer !== undefined) clearTimeout(graceTimer);
       resolve(result);
     };
     timer = setTimeout(() => {
+      deadlineFired = true;
       try { proc.kill(); } catch { /* already exited */ }
-      try { proc.unref?.(); } catch { /* abandonment is still authoritative */ }
-      finish({ exitCode: null, timedOut: true });
+      if (killGraceMs <= 0) {
+        try { proc.unref?.(); } catch { /* abandonment is still authoritative */ }
+        finish({ exitCode: null, timedOut: true });
+        return;
+      }
+      graceTimer = setTimeout(() => {
+        // The child outlived its own kill. Abandon it -- but only now, and only after having
+        // given the OS a real chance to release what it holds.
+        try { proc.unref?.(); } catch { /* abandonment is still authoritative */ }
+        finish({ exitCode: null, timedOut: true });
+      }, Math.max(1, killGraceMs));
     }, Math.max(1, timeoutMs));
     void proc.exited.then(
-      exitCode => finish({ exitCode, timedOut: false }),
-      () => finish({ exitCode: null, timedOut: false }),
+      exitCode => finish(deadlineFired
+        ? { exitCode: null, timedOut: true }
+        : { exitCode, timedOut: false }),
+      () => finish({ exitCode: null, timedOut: deadlineFired }),
     );
   });
 }
@@ -31,7 +31,7 @@
 
 import { existsSync, statSync } from "node:fs";
 import { env, platform } from "node:process";
-import { waitForSubprocessExit } from "./bounded-subprocess";
+import { SUBPROCESS_KILL_GRACE_MS, waitForSubprocessExit } from "./bounded-subprocess";
 import { resolveTrustedWindowsIcaclsExe } from "./windows-elevation";
 import {
   cachedCurrentWindowsIdentity,
@@ -48,6 +48,8 @@ const hardenedPaths = new Map<string, HardenedIdentity>();
  * that attempt was consumed. Ordinary callers never consume it.
  */
 const timedOutPaths = new Map<string, boolean>();
+/** Slack above the kill grace so the outer belt can never fire before the runner settles. */
+const ASYNC_ICACLS_BELT_MARGIN_MS = 250;
 
 /**
  * The memo value: `object:freshness` for a file a harden was actually attributed
@@ -366,9 +368,13 @@ function awaitAsyncIcaclsRunner(args: string[], timeoutMs: number): Promise<Icac
       if (timer !== undefined) clearTimeout(timer);
       resolve(result);
     };
+    // The belt has to outlast the runner it is guarding, or it is not a belt -- it is the
+    // deadline. At exactly `timeoutMs` this used to resolve while `waitForSubprocessExit` was
+    // still killing the child, which reintroduced the abandonment that helper now avoids: the
+    // caller saw a settled flight and started removing a directory `icacls.exe` still held.
     timer = setTimeout(
       () => finish({ success: false, exitCode: null, timedOut: true, stdout: "" }),
-      Math.max(1, timeoutMs),
+      Math.max(1, timeoutMs) + SUBPROCESS_KILL_GRACE_MS + ASYNC_ICACLS_BELT_MARGIN_MS,
     );
     void asyncIcaclsRunner(args, timeoutMs).then(finish, () => finish(spawnFailedResult()));
   });

@@ -160,7 +160,11 @@ async function defaultAsyncWindowsPrincipalRunner(
     stderr: "ignore",
     windowsHide: true,
   });
-  const { exitCode, timedOut } = await waitForSubprocessExit(proc, timeoutMs);
+  // No kill grace. The grace exists so a dying child releases a path someone is about to
+  // remove; this lookup holds no such path, and it runs during `ocx start`, where the composed
+  // acceptance cases already measure real startups at up to 38.8s against a bounded watchdog.
+  // Paying two extra seconds per timed-out resolution there buys nothing and costs margin.
+  const { exitCode, timedOut } = await waitForSubprocessExit(proc, timeoutMs, 0);
   // `.bytes()` rather than `.text()`, for the same reason as the sync runner above.
   const stdout: string | Uint8Array = !timedOut && proc.stdout
     ? await new Response(proc.stdout).bytes().catch(() => new Uint8Array())

@@ -900,22 +900,50 @@ describe("status reports stale process records end to end", () => {
   let freePort: number;
   beforeEach(async () => { freePort = await allocateFreePort(); });
 
-  test("a dead owner record surfaces in --json and in human output", () => {
+  test("a dead owner record surfaces in --json and in human output", async () => {
     const home = mkdtempSync(join(tmpdir(), "ocx-stale-json-"));
     try {
-      seed(home, { runtime: true, port: freePort });
+      // Same hazard the fallback-port case below already guards, and for the same reason:
+      // `probeUncleanExitState` only reports a stale record when the recorded port REFUSES, and
+      // `allocateFreePort` hands back a port it has already released. This case spawns the CLI
+      // TWICE, so it was exposed for the whole gap between the two.
+      //
+      // Dispatch run 35121570658 lost the race there. The --json run saw the refusal and
+      // reported true; the human run a moment later found the port answering and correctly said
+      // nothing about a previous exit. Both reports were right. The fixture was asserting
+      // against a port it no longer owned, so it read a correct report as a lost signal.
+      //
+      // Confirm refusal around every probe and re-allocate when something takes it, so a stolen
+      // port retries the setup instead of failing an assertion it never exercised.
+      let parsed: { proxy?: { staleProcessState?: unknown } } | undefined;
+      let humanStdout: string | undefined;
+      for (let attempt = 0; attempt < 5 && humanStdout === undefined; attempt++) {
+        const port = await allocateFreePort();
+        if (!await refusesConnection(port)) continue;
+        seed(home, { runtime: true, port });
+
+        const json = runStatusJson(home);
+        expect(json.status).toBe(0);
+        const observed = JSON.parse(json.stdout) as { proxy?: { staleProcessState?: unknown } };
+        if (!await refusesConnection(port)) continue;
+        // A /healthz probe can abort without ECONNREFUSED even while the port is empty; that
+        // leaves the field false without anything having taken the port. Retry rather than
+        // treat a timed-out probe as a verdict.
+        if (observed?.proxy?.staleProcessState !== true) continue;
 
-      const json = runStatusJson(home);
-      expect(json.status).toBe(0);
-      const parsed = JSON.parse(json.stdout) as { proxy?: { staleProcessState?: unknown } };
-      expect(parsed.proxy?.staleProcessState).toBe(true);
+        const human = spawnSync(process.execPath, [cliPath, "status"], {
+          cwd: repoRoot,
+          env: { ...process.env, OPENCODEX_HOME: home },
+          encoding: "utf8",
+        });
+        if (!await refusesConnection(port)) continue;
+        parsed = observed;
+        humanStdout = human.stdout;
+      }
 
-      const human = spawnSync(process.execPath, [cliPath, "status"], {
-        cwd: repoRoot,
-        env: { ...process.env, OPENCODEX_HOME: home },
-        encoding: "utf8",
-      });
-      expect(human.stdout).toContain("may have exited unexpectedly");
+      expect(humanStdout, "no allocated port stayed refused across both status probes").toBeDefined();
+      expect(parsed?.proxy?.staleProcessState).toBe(true);
+      expect(humanStdout).toContain("may have exited unexpectedly");
     } finally {
       removeTreeWithRetry(home);
     }

@@ -26,6 +26,27 @@ import { createHash } from "node:crypto";
 import { Database } from "bun:sqlite";
 
 import { watchdogMs } from "../helpers/ci-watchdog";
+
+/**
+ * How long a real `ocx start` child may take to publish runtime-port.json on CI.
+ *
+ * The repository CI floor is 45s on Windows, and that is not a margin here, it is the answer.
+ * Dispatch 35124906412 measured this file's own passing cases on one shard at 5.0s, 7.4s, 8.1s,
+ * 10.7s, 14.8s and 38.8s. The largest healthy startup consumed 86% of the budget meant to bound
+ * a hang, and B-reduced then spent the whole 45s with `child exit=null`, no pid record, no
+ * runtime record and not one byte on either stream — a child still starting, which is exactly
+ * what the diagnostics were added to distinguish from a wedged one.
+ *
+* 120s is roughly three times the slowest healthy start observed, so a hang is still bounded and
+* still reported with the diagnostics rather than by Bun's blunt per-test kill. The per-test
+ * budget already in place, CASE_TIMEOUT_MS at 150s on CI, still exceeds it, so the watchdog keeps
+ * reporting first and the diagnostics survive. That 150s ceiling was never the constraint here;
+ * this 45s floor was.
+ *
+ * Local runs keep the short watchdog: this is a property of the loaded six-shard Windows leg,
+ * not of the code, and waiting two minutes for a hang on a developer machine helps nobody.
+ */
+const CHILD_START_WATCHDOG_MS = process.env.CI === "true" ? 120_000 : watchdogMs(10_000);
 import { removeTreeWithRetry } from "../helpers/remove-tree";
 
 /**
@@ -133,7 +154,7 @@ async function waitFor<T>(
   // while the child was still alive and still working — `child exit=null` with both streams
   // open, which is a slow start, not a crash. The watchdog exists to bound a hung test, not
   // to assert startup latency, so it takes the repository's CI floor.
-  timeoutMs = watchdogMs(10_000),
+  timeoutMs = CHILD_START_WATCHDOG_MS,
 ): Promise<T> {
   const deadline = Date.now() + timeoutMs;
   while (Date.now() < deadline) {

@@ -7,6 +7,17 @@ import { removeTreeWithRetry } from "../helpers/remove-tree";
 
 const roots: string[] = [];
 
+/**
+ * The reclaim case builds a real SQLite log, fragments it, and incrementally vacuums it, and the
+ * cost of that is set by the disk it lands on rather than by anything the test controls. Measured
+ * on three Windows shard runs: 1.6s, 20.2s, and 67.0s. The last one exceeded the suite-wide 60s
+ * per-test ceiling in dispatch 35124906412 while the other cases in the same file finished in 0.4s
+ * to 5.0s, so the work itself varies by more than an order of magnitude with contention on the
+ * six-shard Windows leg. Give this one case room for that spread rather than letting the shared
+ * default decide, and keep it bounded well inside the 30-minute job ceiling.
+ */
+const RECLAIM_CASE_TIMEOUT_MS = process.env.CI === "true" ? 180_000 : 60_000;
+
 function makeRoot(): string {
   const root = mkdtempSync(join(tmpdir(), "ocx-log-guard-reclaim-"));
   roots.push(root);
@@ -143,7 +154,7 @@ describe("Codex Log Guard reclaim", () => {
     expect(result.report.after.databaseBytes).toBeLessThanOrEqual(beforeBytes);
     expect(result.report.integrity).toEqual({ before: "ok", after: "ok" });
     expect(logicalSnapshot(databasePath)).toEqual(beforeLogical);
-  });
+  }, RECLAIM_CASE_TIMEOUT_MS);
 
   test("is a safe no-op when there is nothing reclaimable", async () => {
     const mod = await import("../../src/codex/log-guard/maintenance").catch(() => null);

@@ -1137,6 +1137,7 @@
   "sse-payload-rewrite.test.ts": "responses",
   "sse-unspaced-data-fields.test.ts": "responses",
   "stale-state-purge.test.ts": "service",
+  "stall-subprocess-exit.test.ts": "lib",
   "stall-timeout.test.ts": "lib",
   "star-deferral.test.ts": "cli",
   "startup-action-control-elevation.test.ts": "server",