fix: report a stopped shared host helper only after its exit is observed

docs/reference/shared-host-lifecycle.md

@@ -155,13 +155,45 @@ Under the same lock, remove the holder:
 | --- | --- | --- |
 | others remain | `retained` | Claim rewritten; helper untouched. A holder that was not recorded changes nothing. |
 | none, adopted | `released-adopted` | Claim removed; the adopted process is never signalled. |
-| none, owned, live | `stopped` | TERM, bounded grace, then KILL for the captured tree; each signal re-checks the birth time. |
+| none, owned, live | `stopped` | TERM, bounded grace, then KILL for the captured tree; each signal re-checks the birth time. Claim removed only after every member's exit was observed. |
 | none, owned, stale | `stale-cleared` | Claim removed; nothing is signalled. |
 | none, owned, stop unproven | `stop-unconfirmed` | Claim kept with no holders. |
 
 A release that cannot take the lock within its timeout returns `lock-unavailable`
 and changes nothing.
 
+### What `stopped` means
+
+`stopped` is a statement about observed exits, not about signals sent. Release
+reports it only after a process census, taken after the signals, shows that every
+member of the captured tree has exited. The wait is bounded and named:
+
+1. TERM to the captured tree, then poll every 50 ms for up to `graceMs`
+   (`SHARED_HOST_RELEASE_GRACE_MS`, 2000 ms, when omitted; a failed start uses
+   1000 ms).
+2. KILL to the members still running, then poll for up to
+   `SHARED_HOST_KILL_CONFIRM_MS` (1000 ms).
+3. If a member is still running when that wait ends, the result is
+   `stop-unconfirmed` and the claim is kept, so the process stays recorded.
+
+A member's exit is observed when its PID is gone from the census or is a zombie
+(`ps` state `Z`: exited, not yet reaped). Zombie handling is the same on Linux and
+macOS. On Linux an orphaned helper (its starter died) is reaped by init or a
+subreaper, which can lag behind the stop by any amount. A process that has exited
+holds no listener or open file, so waiting for the reaper would add nothing.
+
+A PID that still runs never counts as exited, even when the census shows another
+birth time for it. Linux `lstart` can drift with the wall clock (see "Not covered
+here"), so that row may be the same helper. PID reuse inside one stop wait is not
+plausible, so the only cost is an honest `stop-unconfirmed`. A census that fails
+during the wait proves nothing either way. Polling continues until the deadline.
+
+Everything that checks whether a claimed process is running uses one predicate,
+`isSharedHostProcessRunning`: the exact PID and birth, not a zombie. The test
+fixtures use the same predicate. A zombie-blind check could see a helper that
+release had correctly reported `stopped` as still alive. That explains the
+Linux-only fixture failure tracked as HACK-1336.
+
 Hack never kills by port number, process name or pattern (`pkill`, `lsof -t | xargs
 kill`). It only signals processes in the captured tree of a birth-verified claim.
 
@@ -222,8 +254,11 @@ Linux pass is a receipt only once that hosted job has run it.
 | lost parent after ready | The helper stays claimed; another client reuses it; the release by both holders stops it. |
 | owned descendants | Release stops the helper and its grandchild; a bystander survives. |
 | SIGTERM ignored | Release escalates to SIGKILL after the grace period: `stopped`. |
+| SIGTERM ignored briefly | A helper that keeps running for 600 ms after TERM, with a 10 s grace: release returns `stopped` only after the helper exits through its own TERM path. No KILL is sent, and the helper is no longer running when release returns. |
+| exit observed by census | Simulated census: the helper keeps running for three polls after TERM and then shows as a zombie. Release waits for the fourth poll, sends no KILL, and reports `stopped`. |
+| running PID with a new birth | Simulated census: after TERM the PID keeps running with another birth time (`lstart` drift). Release sends KILL, then reports `stop-unconfirmed` and keeps the claim. |
 | stop unconfirmed | With a simulated census and signal boundary (no real process can portably survive KILL), a tree that stays in the census gets TERM then KILL, gives `stop-unconfirmed` and keeps the claim with no holders. |
-| unreaped zombie | A helper whose starter is frozen with `SIGSTOP` exits on TERM and stays an unreaped zombie; release reports `stopped` and removes the claim. |
+| unreaped zombie | A helper whose starter is frozen with `SIGSTOP` exits on TERM and stays an unreaped zombie; release reports `stopped` and removes the claim, and the fixture liveness check reads the zombie as not alive. |
 | session and environment | The helper leads its own process group and does not see a runner-only variable. |
 | impostor identity | A live `ready` claim whose port is answered by another PID fails `foreign-listener`; neither process is signalled. |
 | invalid claim / no census | Malformed, other-port, group-readable and oversized claims, and an unavailable census, fail closed; nothing is spawned or signalled; the claim is kept. |
@@ -247,6 +282,7 @@ fail when that check is removed.
   move when the wall clock is changed, so a live helper's birth can change. Start
   then fails closed if the helper reports its PID in the identity answer, but a
   release would read the claim as stale and clear it without stopping the helper.
+  A drift during a stop is handled: it gives `stop-unconfirmed`, never `stopped`.
   macOS records the start time directly and is not affected.
 - Wiring into `hack.config.json`, `hack up`/`down` or the daemon, and migrating
   existing `lifecycle.singleton` configurations.

src/lib/shared-host-lifecycle.ts

@@ -48,6 +48,13 @@ import { withProjectsRegistryLock } from "./projects-registry-lock.ts";
  * - Release removes one holder. Only when no holder remains is an owned helper
  *   stopped, through its birth-verified process tree. Adopted helpers are
  *   never signalled.
+ * - `stopped` means the exit of every member of the captured tree was
+ *   observed within one bounded wait: TERM grace, then
+ *   {@link SHARED_HOST_KILL_CONFIRM_MS} after KILL. Exit is observed when the
+ *   PID is gone from the census or is a zombie (exited, not yet reaped).
+ *   Anything still running under that PID, even with a different birth time,
+ *   is not exit. When the wait ends without that proof, the result is
+ *   `stop-unconfirmed` and the claim is kept.
  *
  * Diagnostics are closed: every failure is one of {@link SharedHostFailure}
  * with a fixed message that never includes paths, argv, env or response
@@ -231,6 +238,16 @@ const MAX_CLAIM_BYTES = 8192;
 const MAX_IDENTITY_BYTES = 4096;
 const MAX_HOLDERS = 256;
 const STOP_POLL_MS = 50;
+/** TERM grace used by {@link releaseSharedHostHelper} when `graceMs` is omitted. */
+export const SHARED_HOST_RELEASE_GRACE_MS = 2000;
+/** TERM grace used when a start fails and Hack stops the helper it started. */
+const FAILED_START_GRACE_MS = 1000;
+/**
+ * How long a stop waits, after KILL, to observe the exit of every member of
+ * the captured tree before it reports `unconfirmed`. The whole stop is bounded
+ * by the TERM grace plus this wait (plus one census per poll).
+ */
+export const SHARED_HOST_KILL_CONFIRM_MS = 1000;
 /**
  * Exec gate: the helper argv runs only after the starter writes `open`. EOF
  * (starter lost before its claim was durable) exits without running it. `exec`
@@ -514,15 +531,39 @@ async function readCensus(
   return rows.filter((row) => !row.zombie);
 }
 
-function isLive(
+/**
+ * The liveness rule for a claimed process: the census shows this exact PID
+ * and birth, and the row is not a zombie. A zombie has exited (see
+ * {@link readCensus}), so it never counts as running. This is the one
+ * predicate Hack and its fixtures use, so a Linux zombie waiting for a slow
+ * reaper reads the same way everywhere.
+ */
+export function isSharedHostProcessRunning(
   census: readonly ProcessIdentityRow[],
-  claim: { readonly pid: number; readonly birth: string }
+  identity: { readonly pid: number; readonly birth: string }
 ): boolean {
   return census.some(
-    (row) => row.pid === claim.pid && row.birth === claim.birth
+    (row) =>
+      row.pid === identity.pid && row.birth === identity.birth && !row.zombie
   );
 }
 
+const isLive = isSharedHostProcessRunning;
+
+/**
+ * Exit of a signalled tree member is observed only when its PID is gone from
+ * the census or is a zombie. The same PID running with another birth time is
+ * not proof: Linux `lstart` can drift with the wall clock, so it may be the
+ * same process. PID reuse inside one stop wait is not plausible, so this
+ * costs nothing but an honest `unconfirmed` in that case.
+ */
+function exitObserved(
+  census: readonly ProcessIdentityRow[],
+  member: ProcessIdentityRow
+): boolean {
+  return !census.some((row) => row.pid === member.pid && !row.zombie);
+}
+
 async function readBounded(response: Response): Promise<string | null> {
   const reader = response.body?.getReader();
   if (!reader) {
@@ -630,10 +671,15 @@ function classifyIdentity(
 }
 
 /**
- * Stop a birth-verified process tree: TERM, bounded grace, then KILL for the
- * members that still carry their captured birth time. Nothing is signalled
- * when the root no longer matches its claim. A member that has exited counts
- * as stopped even before its parent reaps it (see {@link readCensus}).
+ * Stop a birth-verified process tree: TERM, `graceMs` to observe every
+ * member's exit, then KILL for the members still running (each signal
+ * re-checks the birth time) and {@link SHARED_HOST_KILL_CONFIRM_MS} more.
+ * Nothing is signalled when the root no longer matches its claim.
+ *
+ * `stopped` is returned only after a census observed every member exited
+ * (see {@link exitObserved}); a zombie counts as exited, a running PID never
+ * does. A census that fails during the wait proves nothing, so polling goes
+ * on until the deadline. Otherwise the result is `unconfirmed`.
  */
 export async function stopVerifiedSharedHostProcess(opts: {
   readonly pid: number;
@@ -655,26 +701,27 @@ export async function stopVerifiedSharedHostProcess(opts: {
     if (live === null) {
       return null;
     }
-    return tree.filter((row) => isLive(live, row));
+    return tree.filter((row) => !exitObserved(live, row));
+  };
+  const awaitExit = async (waitMs: number) => {
+    const deadline = performance.now() + waitMs;
+    let left = await remaining();
+    while ((left === null || left.length > 0) && performance.now() < deadline) {
+      await delay(STOP_POLL_MS);
+      left = await remaining();
+    }
+    return left;
   };
   await processes.signalTree(tree, "SIGTERM");
-  const termDeadline = performance.now() + opts.graceMs;
-  let left = await remaining();
-  while (left !== null && left.length > 0 && performance.now() < termDeadline) {
-    await delay(STOP_POLL_MS);
-    left = await remaining();
-  }
-  if (left !== null && left.length === 0) {
+  const afterTerm = await awaitExit(opts.graceMs);
+  if (afterTerm !== null && afterTerm.length === 0) {
     return "stopped";
   }
-  await processes.signalTree(left ?? tree, "SIGKILL");
-  const killDeadline = performance.now() + 1000;
-  left = await remaining();
-  while (left !== null && left.length > 0 && performance.now() < killDeadline) {
-    await delay(STOP_POLL_MS);
-    left = await remaining();
-  }
-  return left !== null && left.length === 0 ? "stopped" : "unconfirmed";
+  await processes.signalTree(afterTerm ?? tree, "SIGKILL");
+  const afterKill = await awaitExit(SHARED_HOST_KILL_CONFIRM_MS);
+  return afterKill !== null && afterKill.length === 0
+    ? "stopped"
+    : "unconfirmed";
 }
 
 function withHolder(holders: readonly string[], holder: string): string[] {
@@ -794,7 +841,7 @@ async function stopFailedStart(opts: {
 }): Promise<void> {
   const stopped = await stopVerifiedSharedHostProcess({
     ...opts.owned,
-    graceMs: 1000,
+    graceMs: FAILED_START_GRACE_MS,
   });
   if (stopped === "stopped" || stopped === "absent") {
     await removeClaim(opts.context.spec);
@@ -1227,7 +1274,7 @@ async function releaseLocked(opts: {
   const stopped = await stopVerifiedSharedHostProcess({
     pid: claim.pid,
     birth: claim.birth,
-    graceMs: opts.graceMs ?? 2000,
+    graceMs: opts.graceMs ?? SHARED_HOST_RELEASE_GRACE_MS,
     processes: opts.processes,
   });
   switch (stopped) {

tests/fixtures/shared-host-helper.ts

@@ -7,6 +7,9 @@
  *   {helper, upstream, pid}; `/` answers `served:<upstream>`;
  *   `/__fixture/env` lists this process's environment variable names.
  * - `stubborn`: like `identity`, but ignores SIGTERM (only SIGKILL stops it).
+ * - `slow-term`: like `identity`, but on SIGTERM keeps running for
+ *   {@link SLOW_TERM_MS}, then writes `<HOME>/term-exited` and exits 0 (a
+ *   helper that ignores TERM briefly; the marker proves it was not killed).
  * - `oversized`: like `identity`, but pads the identity answer past 4 KiB.
  * - `exit`: exits immediately (a helper that dies during startup).
  * - `tcp`: a plain TCP listener that never answers (a foreign port owner).
@@ -18,7 +21,11 @@
  * Every mode exits once its private HOME (the test's scratch root) is gone,
  * so no fixture outlives its test even when a failing run skips cleanup.
  */
-import { existsSync } from "node:fs";
+import { existsSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+
+/** How long `slow-term` keeps running after SIGTERM (the test mirrors it). */
+const SLOW_TERM_MS = 600;
 
 const [mode, portText, helper, upstream] = process.argv.slice(2);
 const port = Number(portText);
@@ -61,6 +68,17 @@ switch (mode) {
     });
     serveIdentity();
     break;
+  case "slow-term":
+    process.on("SIGTERM", () => {
+      setTimeout(() => {
+        if (home) {
+          writeFileSync(join(home, "term-exited"), `${process.pid}\n`);
+        }
+        process.exit(0);
+      }, SLOW_TERM_MS);
+    });
+    serveIdentity();
+    break;
   case "oversized":
     serveIdentity({ padding: 8192 });
     break;