fix(update): retire exited pinned-start children before cleanup

src/update/job.ts

@@ -983,6 +983,11 @@ export interface RestartIo {
   spawnStart?: (job: UpdateJobState, installer: Installer, port?: number, launcher?: string) => void;
   /** The package launcher verified after a pnpm group switch or rollback. */
   packageLauncherPathFn?: () => string;
+  /** Exercise pinned-start retries without spawning, reclaiming, or killing real processes. */
+  spawnDetachedStartFn?: typeof spawnDetachedStart;
+  preparePortForPinnedStartFn?: typeof preparePortForPinnedStart;
+  waitForGhostListenClearFn?: typeof waitForGhostListenClear;
+  killProxyFn?: typeof killProxy;
   serviceInstalledFn?: () => boolean;
   /**
    * After a service reinstall exits 0, only trust the service path when this is true.
@@ -1332,9 +1337,20 @@ async function restartAfterUpdate(
     }
   }
   const attempts = 3;
+  const now = io.now ?? (() => Date.now());
+  const spawnPinnedStart = io.spawnDetachedStartFn ?? spawnDetachedStart;
+  const preparePort = io.preparePortForPinnedStartFn ?? preparePortForPinnedStart;
+  const waitForGhost = io.waitForGhostListenClearFn ?? waitForGhostListenClear;
   // Longer than published hard-pin reclaim (30s) so a slow start can still report healthy.
   const perAttemptHealthMs = 70_000;
   let lastChild: ChildProcess | null = null;
+  const killSpawnAttempt = (child: ChildProcess | null): void => {
+    // A numeric PID can be reused once this particular child has exited.
+    if (!child?.pid || child.exitCode !== null || child.signalCode !== null) return;
+    if (aliveFn(child.pid)) {
+      try { (io.killProxyFn ?? killProxy)(child.pid); } catch { /* best-effort */ }
+    }
+  };
   for (let attempt = 1; attempt <= attempts; attempt++) {
     if (attempt > 1) {
       updateJob(
@@ -1343,13 +1359,11 @@ async function restartAfterUpdate(
         `Pinned start attempt ${attempt - 1} did not become healthy on port ${port}; `
           + `retrying (${attempt}/${attempts}).`,
       );
-      if (lastChild?.pid && aliveFn(lastChild.pid)) {
-        try { killProxy(lastChild.pid); } catch { /* best-effort */ }
-      }
+      killSpawnAttempt(lastChild);
       lastChild = null;
     }
-    preparePortForPinnedStart(job, port, listPids, aliveFn, verifyOcx);
-    const ready = await waitForGhostListenClear(
+    preparePort(job, port, listPids, aliveFn, verifyOcx);
+    const ready = await waitForGhost(
       port,
       hostname,
       listPids,
@@ -1365,17 +1379,19 @@ async function restartAfterUpdate(
       );
       continue;
     }
-    lastChild = spawnDetachedStart(job, job.installer, port, launcher);
-    const healthDeadline = Date.now() + perAttemptHealthMs;
-    while (Date.now() < healthDeadline) {
+    const child = spawnPinnedStart(job, job.installer, port, launcher);
+    lastChild = child;
+    child.once("exit", () => {
+      if (lastChild === child) lastChild = null;
+    });
+    const healthDeadline = now() + perAttemptHealthMs;
+    while (now() < healthDeadline) {
       if (await probe(port, hostname)) return;
       await sleep(500);
     }
   }
   // Exhausted retries: do not leave a hung pinned-start child owning the port.
-  if (lastChild?.pid && aliveFn(lastChild.pid)) {
-    try { killProxy(lastChild.pid); } catch { /* best-effort */ }
-  }
+  killSpawnAttempt(lastChild);
 }
 
 /** Compact listen-holder summary for update-job logs when reclaim fails. */

structure/runtime.md

@@ -93,6 +93,13 @@ it does not add process-instance proof or change the classification cache.
 
 > Decision record: [ADR-0003](decisions/ADR-0003-lifecycle.md)
 
+Pinned-start retries in `src/update/job.ts` retain the spawned child object for cleanup.
+An observed exit retires only that object, and recorded exit or signal status prevents a PID
+liveness check or termination attempt for the retired child. A previous child's late exit cannot
+retire a newer child with the same numeric PID. Live children are still cleaned up before a retry
+and after the final health timeout; a successful health probe leaves the current child running.
+`tests/update/update-job.test.ts` exercises these transitions with injected process and port I/O.
+
 An installed Codex shim is checked on ordinary CLI startup with a regular-file/1 MiB state bound plus
 bounded metadata and prefix reads. A complete replacement must produce identical fingerprints and
 prefixes across a 100 ms observation interval; changing launchers are silently deferred, while mixed

tests/update/update-job.test.ts

@@ -1,4 +1,6 @@
 import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import type { ChildProcess } from "node:child_process";
+import { EventEmitter } from "node:events";
 import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -42,6 +44,120 @@ afterEach(() => {
   removeTreeWithRetry(dir);
 });
 
+describe("pinned-start child cleanup", () => {
+  // exitCode/signalCode are readonly on ChildProcess, but these fakes must move a child from
+  // "running" to "exited" mid-test. Redeclare them as mutable rather than widening each
+  // assignment with a cast, so the transitions stay type-checked.
+  type FakeChild = EventEmitter & Pick<ChildProcess, "pid"> & {
+    exitCode: number | null;
+    signalCode: NodeJS.Signals | null;
+  };
+
+  async function exhaustRetries(options: {
+    spawned?: (child: FakeChild) => void;
+    healthWait?: (children: FakeChild[]) => void;
+    reusePid?: boolean;
+    healthyOnLastAttempt?: boolean;
+  } = {}) {
+    let now = 0;
+    const children: FakeChild[] = [];
+    const killed: number[] = [];
+    const livenessChecks: number[] = [];
+    const job: UpdateJobState = {
+      id: "pinned-child-cleanup", status: "restarting",
+      startedAt: new Date().toISOString(), updatedAt: new Date().toISOString(),
+      currentVersion: "2.49.0", latestVersion: "2.50.0", channel: "latest",
+      installer: "npm", restart: true, command: "", releaseNotesUrl: "", log: [],
+    };
+    writeFileSync(updateJobPath(), JSON.stringify(job));
+    await restartAfterUpdateForTests(job, { port: 19111, hostname: "127.0.0.1" }, {
+      serviceInstalledFn: () => false,
+      waitForPort: async () => true,
+      listListenPidsFn: () => [],
+      preparePortForPinnedStartFn: () => {},
+      waitForGhostListenClearFn: async () => ({ ok: true, accessDenied: false }),
+      probeProxyIdentity: async () => null,
+      probeProxy: async () => !!options.healthyOnLastAttempt && children.length === 3,
+      now: () => now,
+      sleepMs: async ms => {
+        options.healthWait?.(children);
+        now += ms;
+      },
+      isAliveFn: pid => {
+        livenessChecks.push(pid);
+        // A reused numeric PID may be live even after our own child has exited.
+        return true;
+      },
+      spawnDetachedStartFn: () => {
+        const child: FakeChild = Object.assign(new EventEmitter(), {
+          pid: options.reusePid ? 4241 : 4241 + children.length, exitCode: null, signalCode: null,
+        });
+        children.push(child);
+        options.spawned?.(child);
+        return child as ChildProcess;
+      },
+      killProxyFn: pid => { killed.push(pid); },
+    });
+    expect(children).toHaveLength(3);
+    return { killed, livenessChecks };
+  }
+
+  test.each([
+    { name: "successful exit", exitCode: 0, signalCode: null },
+    { name: "failed exit", exitCode: 1, signalCode: null },
+    { name: "signal exit", exitCode: null, signalCode: "SIGTERM" as const },
+  ])("never reuses a child PID after $name", async ({ exitCode, signalCode }) => {
+    const result = await exhaustRetries({
+      spawned: child => { child.exitCode = exitCode; child.signalCode = signalCode; },
+    });
+    expect(result.killed).toEqual([]);
+    expect(result.livenessChecks).toEqual([]);
+  });
+
+  test("retires a child when its exit event is observed during the health wait", async () => {
+    const observed = new Set<FakeChild>();
+    const result = await exhaustRetries({
+      healthWait: children => {
+        const child = children.at(-1)!;
+        if (observed.has(child)) return;
+        observed.add(child);
+        // Keep the fixture fields unset to exercise the event retirement independently.
+        child.emit("exit", 0, null);
+      },
+    });
+    expect(observed.size).toBe(3);
+    expect(result.killed).toEqual([]);
+    expect(result.livenessChecks).toEqual([]);
+  });
+
+  test("still cleans up live children before retries and after the final health timeout", async () => {
+    const result = await exhaustRetries();
+    expect(result.killed).toEqual([4241, 4242, 4243]);
+    expect(result.livenessChecks).toEqual([4241, 4242, 4243]);
+  });
+
+  test("a previous child's late exit does not retire the current live child", async () => {
+    const observed = new Set<FakeChild>();
+    const result = await exhaustRetries({
+      reusePid: true,
+      healthWait: children => {
+        const previous = children.at(-2);
+        if (!previous || observed.has(previous)) return;
+        observed.add(previous);
+        previous.exitCode = 0;
+        previous.emit("exit", 0, null);
+      },
+    });
+    expect(observed.size).toBe(2);
+    expect(result.killed).toEqual([4241, 4241, 4241]);
+  });
+
+  test("leaves the current child running when its health probe succeeds", async () => {
+    const result = await exhaustRetries({ healthyOnLastAttempt: true });
+    expect(result.killed).toEqual([4241, 4242]);
+  });
+});
+
 describe("GUI update check", () => {
   test("surfaces an npm update with the launcher-safe command", () => {
     const result = checkForUpdate("latest", {

tests/windows/windows-deploy-close-regressions.test.ts

@@ -59,7 +59,8 @@ describe("update-job restart avoids the shell-less .cmd EINVAL (Windows, bun/sou
     // matching rules themselves are covered by windows-service-wrappers.test.ts.
     expect(src).toContain("killWindowsSchedulerWrappers");
     expect(read("src/lib/windows-service-wrappers.ts")).toContain("$_.ProcessId -eq $PID");
-    expect(src).toContain("lastChild?.pid && aliveFn(lastChild.pid)");
+    // Pinned-child cleanup is exercised through the retry loop in update/update-job.test.ts,
+    // including exited children, live retry/final cleanup, and successful health probes.
   });
 });