fix(test-helpers): kill the whole watch process group so teardown stops leaking orphans

packages/agent-memory-sync/tests/helpers/watch-process.ts

@@ -32,12 +32,195 @@ const WATCH_READY_PATTERN = /watching \d+ path\(s\) under/;
 const READY_TIMEOUT_MS = 10000;
 const TICK_TIMEOUT_MS = 20000;
 
+// How long to wait for a graceful SIGINT/SIGTERM to a watch process group to
+// take effect before escalating to SIGKILL — see stopWatchProcessGroup below.
+const GROUP_KILL_GRACE_MS = 2000;
+const GROUP_KILL_POLL_INTERVAL_MS = 25;
+
+// spawnWatch's `child` is the `tsx` launcher process, NOT the real node
+// process that ends up running src/main.ts: tsx's own CLI (node_modules/tsx/
+// dist/cli.mjs) always spawns a second, genuine node child (with
+// `--import`/`--loader` pointed at its loader) to actually execute the file,
+// passing it `stdio: ["inherit","inherit","inherit"]` so that grandchild
+// writes directly into the SAME stdout/stderr pipe fds this helper's own
+// spawn() call below opened. `detached: true` puts the tsx launcher (and,
+// because child processes stay in their parent's process group unless they
+// explicitly opt out, its node grandchild too) into a process group whose id
+// equals the launcher's own pid — letting cleanup target the whole group
+// with a single process.kill(-pid, signal) call instead of only the
+// launcher. Killing only the launcher with SIGKILL (e.g. a bare
+// child.kill("SIGKILL")) leaves the grandchild running: SIGKILL specifically
+// cannot be caught or relayed, so the launcher dies before it can forward
+// anything, and the grandchild is reparented to pid 1 and keeps the
+// inherited pipe's write end open, so nothing reading that pipe (this
+// helper's own child.stdout/stderr, and transitively whatever spawned this
+// whole test file) ever sees the stream close — measured directly: a stuck
+// grandchild here is what stalls test-file teardown and the wider suite run
+// long after every test in the file has already gone green. SIGINT/SIGTERM
+// to just the launcher happen to be relayed to the grandchild by tsx today
+// (and src/commands/watch.ts installs its own handlers for both), measured
+// to leave 0 survivors — but that relaying is tsx's implementation detail,
+// not a guarantee this helper should rely on, hence targeting the whole
+// group unconditionally below instead of trusting any one signal to
+// propagate on its own.
 function spawnWatch(args: string[], env: NodeJS.ProcessEnv) {
-  return spawn(
+  registerLastResortGroupKillHandlers();
+  const child = spawn(
     path.resolve(process.cwd(), "node_modules", ".bin", "tsx"),
     ["src/main.ts", ...args],
-    { env, stdio: ["ignore", "pipe", "pipe"] }
+    { env, stdio: ["ignore", "pipe", "pipe"], detached: true }
   );
+  if (typeof child.pid === "number") {
+    liveGroupPids.add(child.pid);
+  }
+  return child;
+}
+
+// Every process group spawnWatch() has started and not yet torn down via
+// stopWatchProcessGroup, keyed by the group's leader pid (== the tsx
+// launcher's own pid, see spawnWatch's `detached: true` comment). Populated
+// in spawnWatch, pruned in stopWatchProcessGroup's `finally` below — see
+// registerLastResortGroupKillHandlers for what still reads it once a test
+// itself can no longer reach the group.
+const liveGroupPids = new Set<number>();
+
+// `detached: true` (spawnWatch's comment) traded away a free cleanup
+// pre-change code had for nothing: back when the launcher and grandchild
+// stayed in the RUNNER's own process group, a Ctrl-C on `npm test` (a group
+// SIGINT to the runner's pgid) reached and killed them too, for free, with
+// zero survivors. Once they moved into their own group, that stops working
+// — any path that ends the test-file process without ever reaching a test's
+// own `finally`/stopWatchProcessGroup call (Ctrl-C, a node:test file-level
+// abort, an exception thrown between a bare spawnWatch() call and the
+// `.finally()` that would tear it down — see watch-mirror-delete.test.ts's
+// two `spawnWatch` call sites, or a hard process.exit()) now leaks a group
+// nothing can reach any more. This function registers the deliberate
+// last-resort replacement — a synchronous, best-effort SIGKILL of every
+// still-tracked group — on "exit" and on SIGINT/SIGTERM (re-raised
+// afterwards so the process still actually terminates on the signal), and
+// is idempotent so calling it from every spawnWatch() invocation never
+// accumulates duplicate listeners. Zero effect on the green path: by the
+// time a passing test file's process actually exits, every group it spawned
+// has already been removed from liveGroupPids by stopWatchProcessGroup, so
+// these handlers iterate an empty set and do nothing.
+let lastResortHandlersRegistered = false;
+function registerLastResortGroupKillHandlers(): void {
+  if (lastResortHandlersRegistered) {
+    return;
+  }
+  lastResortHandlersRegistered = true;
+
+  const killAllLiveGroups = () => {
+    for (const pid of liveGroupPids) {
+      try {
+        process.kill(-pid, "SIGKILL");
+      } catch {
+        // ESRCH (already gone) or EPERM (not ours any more, e.g. the pid was
+        // reused) — either way, nothing more this last resort can do for it.
+      }
+    }
+  };
+
+  process.once("exit", killAllLiveGroups);
+  for (const signal of ["SIGINT", "SIGTERM"] as const) {
+    // `once` self-unregisters before invoking the listener (Node's
+    // EventEmitter semantics), so re-sending the signal to ourselves from
+    // inside the handler falls through to Node's default disposition
+    // (terminate) instead of looping back into this same handler.
+    process.once(signal, () => {
+      killAllLiveGroups();
+      process.kill(process.pid, signal);
+    });
+  }
+}
+
+// True if any process in child's process group (see spawnWatch's
+// `detached: true` comment) is still alive. Uses signal 0, which only checks
+// for existence/permission — it never actually signals anything. EPERM
+// (refused for permission reasons — e.g. this pid was reused by an
+// unrelated process this test runner has no standing to signal) is treated
+// the same as ESRCH: either way, this is no longer a process group cleanup
+// can find or safely act on.
+function isProcessGroupAlive(child: ReturnType<typeof spawn>): boolean {
+  if (typeof child.pid !== "number") {
+    return false;
+  }
+  try {
+    process.kill(-child.pid, 0);
+    return true;
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException)?.code;
+    if (code === "ESRCH" || code === "EPERM") {
+      return false;
+    }
+    throw err;
+  }
+}
+
+// Best-effort signal delivery to every process in child's group (the tsx
+// launcher AND the real node process it spawns internally — see spawnWatch),
+// not just `child` itself. A process-group-scoped kill(2) via the negated
+// pid, not a name/pattern-based pkill: it can only ever reach descendants of
+// this specific spawnWatch() call. ESRCH (nothing left to signal) and EPERM
+// (no longer ours to signal, e.g. pid reuse) are both treated as success,
+// not an error — retrying or rethrowing here cannot fix either, and letting
+// either surface would risk an uncaught exception out of a timer/finally
+// callback (see withTickDeadline and runWatchTick) instead of just leaving
+// cleanup a no-op for a group that is not reachable any more.
+function signalProcessGroup(child: ReturnType<typeof spawn>, signal: NodeJS.Signals): void {
+  if (typeof child.pid !== "number") {
+    return;
+  }
+  try {
+    process.kill(-child.pid, signal);
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException)?.code;
+    if (code !== "ESRCH" && code !== "EPERM") {
+      throw err;
+    }
+  }
+}
+
+// Deterministic teardown for a spawnWatch()'d child: sends `signal`
+// (graceful by default) to its whole process group, waits up to
+// GROUP_KILL_GRACE_MS for the group to actually exit, then SIGKILLs
+// whatever remains. Centralizing this here — rather than each test/helper
+// separately checking `child.exitCode`/`signalCode` on the launcher alone —
+// means cleanup always attempts the group kill, not only when the immediate
+// child looks unfinished: `child` having already reported exit says nothing
+// about whether its node grandchild is still alive (see spawnWatch's
+// comment). The leak that originally motivated group-wide kills was
+// measured via withTickDeadline's SIGKILL-on-timeout path (see its
+// comment), not through this function's own graceful-signal path — the
+// common case here (both processes already gone on their own) returns
+// immediately, since isProcessGroupAlive's first check is false, so this
+// adds no measurable latency to a normal passing tick. Always prunes
+// `child`'s pid from liveGroupPids on the way out, whether or not the group
+// actually died — see registerLastResortGroupKillHandlers for who else
+// reads that set.
+async function stopWatchProcessGroup(
+  child: ReturnType<typeof spawn>,
+  signal: NodeJS.Signals = "SIGINT"
+): Promise<void> {
+  try {
+    if (!isProcessGroupAlive(child)) {
+      return;
+    }
+    signalProcessGroup(child, signal);
+
+    const deadline = Date.now() + GROUP_KILL_GRACE_MS;
+    while (Date.now() < deadline && isProcessGroupAlive(child)) {
+      await new Promise((resolve) => setTimeout(resolve, GROUP_KILL_POLL_INTERVAL_MS));
+    }
+
+    if (isProcessGroupAlive(child)) {
+      signalProcessGroup(child, "SIGKILL");
+    }
+  } finally {
+    if (typeof child.pid === "number") {
+      liveGroupPids.delete(child.pid);
+    }
+  }
 }
 
 // Polls `getStderr()` until it matches WATCH_READY_PATTERN, i.e. until the
@@ -67,10 +250,10 @@ function waitForWatcherReady(getStderr: () => string, timeoutMs = READY_TIMEOUT_
 
 // Bounds `fn` (expected to await a spawned watch child reaching some end
 // state — typically process exit) to `timeoutMs`: if it has not settled in
-// time, force-kills `child` (SIGKILL — a deliberate last-resort tier,
-// distinct from the graceful SIGINT a test's own cleanup uses once the
-// child has already exited on its own) and rejects with a clear message
-// instead of hanging indefinitely.
+// time, force-kills `child`'s whole process group (SIGKILL — a deliberate
+// last-resort tier, distinct from the graceful SIGINT stopWatchProcessGroup
+// uses once a tick has already completed on its own) and rejects with a
+// clear message instead of hanging indefinitely.
 async function withTickDeadline<T>(
   child: ReturnType<typeof spawn>,
   fn: () => Promise<T>,
@@ -79,10 +262,27 @@ async function withTickDeadline<T>(
   let timer: NodeJS.Timeout | null = null;
   const timeout = new Promise<never>((_, reject) => {
     timer = setTimeout(() => {
-      if (child.exitCode === null && child.signalCode === null) {
-        child.kill("SIGKILL");
-      }
+      // Reject FIRST: that rejection is what actually fails the test, so it
+      // must run even if signaling the process group below throws for some
+      // reason signalProcessGroup doesn't already swallow (it treats ESRCH/
+      // EPERM as success) — otherwise an uncaught exception here would kill
+      // the whole test-file process instead of just failing this one tick,
+      // and this rejection would never fire at all.
       reject(new Error(`watch tick did not complete within ${timeoutMs}ms — killed the child process`));
+      try {
+        // Already past the deadline, so there is no grace period left to
+        // spend on a graceful signal: SIGKILL the whole process group
+        // immediately (see spawnWatch's `detached: true` comment) instead of
+        // just `child` — SIGKILL specifically cannot be caught or relayed,
+        // so a bare child.kill("SIGKILL") here would only ever reach the tsx
+        // launcher, orphaning its real node grandchild (reparented to pid 1)
+        // to keep running and hold the inherited stdout/stderr pipe open.
+        signalProcessGroup(child, "SIGKILL");
+      } catch {
+        // Best-effort: the rejection above has already failed the test —
+        // don't let a further error here replace it with an uncaught
+        // exception instead.
+      }
     }, timeoutMs);
   });
 
@@ -144,15 +344,17 @@ async function runWatchTick(
       return { exitCode, stderr };
     });
   } finally {
-    if (child.exitCode === null && child.signalCode === null) {
-      child.kill("SIGINT");
-    }
+    // Always attempt teardown, not just when the launcher itself looks
+    // unfinished — see stopWatchProcessGroup's comment for why `child`
+    // having already exited says nothing about its node grandchild.
+    await stopWatchProcessGroup(child);
   }
 }
 
 module.exports = {
   spawnWatch,
   waitForWatcherReady,
   withTickDeadline,
-  runWatchTick
+  runWatchTick,
+  stopWatchProcessGroup
 };

packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts

@@ -35,7 +35,13 @@ const {
   writeProjectConfig,
   writeText
 } = require("../helpers/cli.ts");
-const { spawnWatch, waitForWatcherReady, withTickDeadline, runWatchTick } = require("../helpers/watch-process.ts");
+const {
+  spawnWatch,
+  waitForWatcherReady,
+  withTickDeadline,
+  runWatchTick,
+  stopWatchProcessGroup
+} = require("../helpers/watch-process.ts");
 
 function createConfig(workspaceRoot: string, remoteDir: string) {
   return {
@@ -203,11 +209,7 @@ test("watch tick queues locally when the remote is unreachable, then replays the
     return new Promise<number>((resolve) => {
       offlineChild.on("exit", (code: number | null) => resolve(code ?? -1));
     });
-  }).finally(() => {
-    if (offlineChild.exitCode === null && offlineChild.signalCode === null) {
-      offlineChild.kill("SIGINT");
-    }
-  });
+  }).finally(() => stopWatchProcessGroup(offlineChild));
 
   assert.equal(offlineExitCode, 0, `watch exited non-zero while offline. stderr: ${offlineStderr}`);
   assert.match(offlineStderr, /queued locally/);
@@ -253,11 +255,7 @@ test("watch tick queues locally when the remote is unreachable, then replays the
     return new Promise<number>((resolve) => {
       onlineChild.on("exit", (code: number | null) => resolve(code ?? -1));
     });
-  }).finally(() => {
-    if (onlineChild.exitCode === null && onlineChild.signalCode === null) {
-      onlineChild.kill("SIGINT");
-    }
-  });
+  }).finally(() => stopWatchProcessGroup(onlineChild));
 
   assert.equal(onlineExitCode, 0, `watch exited non-zero while replaying. stderr: ${onlineStderr}`);
   assert.deepEqual(fs.readdirSync(queueDir), [], "expected the queue to be empty after a successful replay");

packages/agent-memory-sync/tests/integration/watch-teardown-guard.test.ts

@@ -0,0 +1,86 @@
+// Regression guard for the MEDIUM review finding on
+// ../helpers/watch-process.ts's `detached: true` + negated-pid
+// process.kill(-pid, sig) group-kill (task c71de504, review round 2):
+// reverting either half silently — `detached: true` alone, or the
+// negated-pid kill alone — leaves every other existing test green, because
+// process.kill(-pid, sig) then throws ESRCH (no such process group), which
+// signalProcessGroup already swallows as "success". A silently no-op
+// teardown is worse than the pre-change code it replaced (which at least
+// got a free kill from the runner's own process-group SIGINT on Ctrl-C).
+//
+// The observable that actually distinguishes "the group died" from "it
+// didn't": tsx's node grandchild inherits this helper's own child.stdout/
+// child.stderr pipe fds (see spawnWatch's comment) and holds their write
+// end open for as long as it is alive, whether or not the immediate tsx
+// launcher is still around. So the parent-side pipe closing is a direct,
+// cheap proxy for "the grandchild is actually dead", not just "the
+// launcher exited" — which is exactly the gap the pre-fix code got wrong.
+//
+// Mutation-tested outside this repo (not part of this automated suite —
+// see this task's final report): reverting `detached: true` alone, and
+// reverting only the negated-pid kill in signalProcessGroup, each made this
+// test fail (stderr never closed within the deadline, and/or
+// process.kill(-pid, 0) did not throw ESRCH). Both arms pass on the
+// unmutated code below.
+const test = require("node:test");
+const assert = require("node:assert/strict");
+const path = require("node:path");
+const { createSandbox, initBareRemote, runCli, writeProjectConfig, writeText } = require("../helpers/cli.ts");
+const { spawnWatch, withTickDeadline, stopWatchProcessGroup } = require("../helpers/watch-process.ts");
+
+test("stopWatchProcessGroup actually kills the whole watch process group, not just the tsx launcher", async () => {
+  const root = createSandbox("watch-teardown-guard");
+  const remoteDir = initBareRemote(root);
+  const workspaceRoot = path.join(root, "workspace");
+  const configPath = path.join(root, "config.json");
+
+  writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n");
+  writeProjectConfig(configPath, {
+    rootDir: workspaceRoot,
+    remoteUrl: remoteDir,
+    branch: "main",
+    repositorySubdir: "shared",
+    stateDir: ".agent-memory-sync/default",
+    syncPaths: [{ source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }]
+  });
+  runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]);
+
+  // A watch that will never tick: no trigger edit is ever applied, so the
+  // child just sits armed (or still arming) until we force it down below.
+  const child = spawnWatch(
+    ["watch", "default", "--config", configPath, "--debounce-ms", "300", "--max-runs", "1", "--verbose", "--output", "json"],
+    process.env
+  );
+
+  // Force the deadline path (500ms) rather than waiting out TICK_TIMEOUT_MS;
+  // `fn` never settles on its own, so this always times out and SIGKILLs the
+  // group. The rejection is expected and irrelevant to this test.
+  await withTickDeadline(child, () => new Promise(() => {}), 500).catch(() => {});
+
+  // Idempotent on top of the deadline path's own kill — exercises the same
+  // teardown callers actually use (runWatchTick's finally).
+  await stopWatchProcessGroup(child);
+
+  const stderrClosed = await new Promise((resolve) => {
+    if (child.stderr.destroyed || child.stderr.closed) {
+      resolve(true);
+      return;
+    }
+    const timer = setTimeout(() => resolve(false), 1000);
+    child.stderr.once("close", () => {
+      clearTimeout(timer);
+      resolve(true);
+    });
+  });
+  assert.equal(
+    stderrClosed,
+    true,
+    "parent-side stderr pipe never closed within 1s after teardown — the grandchild is likely still alive and orphaned"
+  );
+
+  assert.throws(
+    () => process.kill(-(child.pid as number), 0),
+    (err: NodeJS.ErrnoException) => err.code === "ESRCH",
+    "expected the whole process group to be gone (ESRCH) after teardown"
+  );
+});