From 28bb1fb11e06d9866cb7ac62ccb54e98e00d9fe8 Mon Sep 17 00:00:00 2001
From: luvs01 <luvs01@hanmail.net>
Date: Fri, 7 Aug 2026 22:36:18 +0900
Subject: [PATCH] fix(update): avoid killing stale child PIDs

---
 src/update/job.ts        | 32 ++++++++++++++++++++--------
 tests/update-job.test.ts | 45 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/src/update/job.ts b/src/update/job.ts
index 3b9557f953..0d59ffaa18 100644
--- a/src/update/job.ts
+++ b/src/update/job.ts
@@ -679,6 +679,10 @@ export interface RestartIo {
   verifyOcxFn?: (pid: number) => number | null;
   /** Liveness check when deciding whether a reclaim timeout still has live holders. */
   isAliveFn?: (pid: number) => boolean;
+  /** Test seam for production pinned-start retries. */
+  spawnDetachedStartFn?: typeof spawnDetachedStart;
+  /** Test seam for terminating a still-running pinned-start attempt. */
+  killProxyFn?: typeof killProxy;
 }
 
 /**
@@ -937,6 +941,16 @@ async function restartAfterUpdate(
     !!(await proxyIdentityAt(p, { hostname: host }))
   ));
   const probeIdentity = io.probeProxyIdentity ?? defaultProbeProxyIdentity;
+  const now = io.now ?? (() => Date.now());
+  const spawnPinnedStart = io.spawnDetachedStartFn ?? spawnDetachedStart;
+  const killSpawnAttempt = (child: ChildProcess | null): void => {
+    // Once Node has observed this ChildProcess exit, its numeric PID is no longer
+    // owned by the start attempt and may already identify an unrelated process.
+    if (!child?.pid || child.exitCode !== null || child.signalCode !== null) return;
+    if (aliveFn(child.pid)) {
+      try { (io.killProxyFn ?? killProxy)(child.pid); } catch { /* best-effort */ }
+    }
+  };
   const expectedVersion = typeof job.latestVersion === "string" && job.latestVersion.length > 0
     ? job.latestVersion
     : null;
@@ -965,9 +979,7 @@ 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);
@@ -987,17 +999,19 @@ async function restartAfterUpdate(
       );
       continue;
     }
-    lastChild = spawnDetachedStart(job, job.installer, port);
-    const healthDeadline = Date.now() + perAttemptHealthMs;
-    while (Date.now() < healthDeadline) {
+    const child = spawnPinnedStart(job, job.installer, port);
+    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-job.test.ts b/tests/update-job.test.ts
index 7adcc513fb..6fa1ed0cb9 100644
--- a/tests/update-job.test.ts
+++ b/tests/update-job.test.ts
@@ -301,6 +301,51 @@ describe("GUI update execution decisions", () => {
     expect(readUpdateJob(job.id)?.log.some(line => line.includes("waiting for ghost LISTEN rows to clear before pinned start"))).toBe(true);
   });
 
+  test("restart retries never kill a PID retained from an exited pinned start", async () => {
+    let now = 0;
+    let spawnCount = 0;
+    const killed: number[] = [];
+    const job: UpdateJobState = {
+      id: "restart-stale-child-pid",
+      status: "restarting",
+      startedAt: new Date().toISOString(),
+      updatedAt: new Date().toISOString(),
+      currentVersion: "2.10.1",
+      latestVersion: "2.10.2",
+      channel: "latest",
+      installer: "npm",
+      restart: true,
+      command: "",
+      log: [],
+    };
+    writeFileSync(updateJobPath(job.id), JSON.stringify(job));
+
+    await restartAfterUpdateForTests(job, { port: 10100, hostname: "127.0.0.1" }, {
+      serviceInstalledFn: () => false,
+      waitForPort: async () => true,
+      listListenPidsFn: () => [],
+      probeProxy: async () => false,
+      probeProxyIdentity: async () => null,
+      now: () => now,
+      sleepMs: async ms => { now += ms; },
+      isAliveFn: () => true,
+      spawnDetachedStartFn: () => {
+        spawnCount += 1;
+        const child = {
+          pid: 4242,
+          exitCode: 1,
+          signalCode: null,
+          once: () => child,
+        };
+        return child as unknown as ReturnType<typeof import("node:child_process").spawn>;
+      },
+      killProxyFn: pid => { killed.push(pid); },
+    });
+
+    expect(spawnCount).toBe(3);
+    expect(killed).toEqual([]);
+  });
+
   test("service restart waits on the captured port and clears OCX_BAKE_PORT after install", async () => {
     const waited: Array<{ port: number; hostname: string }> = [];
     const bakeDuringInstall: string[] = [];