From f161ba3c343ea3ae9e5c2da27a0dc591d50cf5f3 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:28:28 +0900 Subject: [PATCH 1/4] fix(update): retire exited pinned-start children before cleanup --- src/update/job.ts | 38 +++++++---- tests/update/update-job.test.ts | 110 ++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index b7e51ff8c5..dd89d4028f 100644 --- a/src/update/job.ts +++ b/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. */ diff --git a/tests/update/update-job.test.ts b/tests/update/update-job.test.ts index 9ba42b8150..c35d0ae29a 100644 --- a/tests/update/update-job.test.ts +++ b/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,114 @@ afterEach(() => { removeTreeWithRetry(dir); }); +describe("pinned-start child cleanup", () => { + type FakeChild = EventEmitter & Pick; + + 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: "", log: [], + }; + writeFileSync(updateJobPath(job.id), 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(); + 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(); + 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", { From 6c99ae46bddbb7d398d4bfaeb2ca53d5f519c759 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:41:54 +0900 Subject: [PATCH 2/4] test(update): align fixtures and replace stale cleanup source check --- tests/update/update-job.test.ts | 4 ++-- tests/windows/windows-deploy-close-regressions.test.ts | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/update/update-job.test.ts b/tests/update/update-job.test.ts index c35d0ae29a..507ff702d8 100644 --- a/tests/update/update-job.test.ts +++ b/tests/update/update-job.test.ts @@ -61,9 +61,9 @@ describe("pinned-start child cleanup", () => { 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: "", log: [], + installer: "npm", restart: true, command: "", releaseNotesUrl: "", log: [], }; - writeFileSync(updateJobPath(job.id), JSON.stringify(job)); + writeFileSync(updateJobPath(), JSON.stringify(job)); await restartAfterUpdateForTests(job, { port: 19111, hostname: "127.0.0.1" }, { serviceInstalledFn: () => false, waitForPort: async () => true, diff --git a/tests/windows/windows-deploy-close-regressions.test.ts b/tests/windows/windows-deploy-close-regressions.test.ts index e3bae4fc0b..d9f80c8d19 100644 --- a/tests/windows/windows-deploy-close-regressions.test.ts +++ b/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. }); }); From 623097b869b4ca9712fceeaff9fff8729b13e616 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 11 Sep 2026 21:41:49 +0900 Subject: [PATCH 3/4] test(update): keep FakeChild exit fields mutable Pick carries the readonly modifiers, so the fake's running-to-exited transitions relied on the test file sitting outside the typecheck scope. Redeclare both fields as mutable and keep pid/EventEmitter typing as-is. --- tests/update/update-job.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/update/update-job.test.ts b/tests/update/update-job.test.ts index 507ff702d8..c5ac2b523e 100644 --- a/tests/update/update-job.test.ts +++ b/tests/update/update-job.test.ts @@ -45,7 +45,13 @@ afterEach(() => { }); describe("pinned-start child cleanup", () => { - type FakeChild = EventEmitter & Pick; + // 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 & { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + }; async function exhaustRetries(options: { spawned?: (child: FakeChild) => void; From fda4690f86287c723a1911e43f44a83e0b644467 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 12 Sep 2026 14:47:20 +0900 Subject: [PATCH 4/4] docs(update): document pinned child cleanup lifetime --- structure/runtime.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/structure/runtime.md b/structure/runtime.md index 833d2fe66b..ad73fb2869 100644 --- a/structure/runtime.md +++ b/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