diff --git a/packages/agent-memory-sync/tests/helpers/watch-process.ts b/packages/agent-memory-sync/tests/helpers/watch-process.ts index 8739421..965fd00 100644 --- a/packages/agent-memory-sync/tests/helpers/watch-process.ts +++ b/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(); + +// `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): 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, 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, + signal: NodeJS.Signals = "SIGINT" +): Promise { + 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( child: ReturnType, fn: () => Promise, @@ -79,10 +262,27 @@ async function withTickDeadline( let timer: NodeJS.Timeout | null = null; const timeout = new Promise((_, 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,9 +344,10 @@ 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); } } @@ -154,5 +355,6 @@ module.exports = { spawnWatch, waitForWatcherReady, withTickDeadline, - runWatchTick + runWatchTick, + stopWatchProcessGroup }; diff --git a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts index b9e1986..fb2d6a8 100644 --- a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts +++ b/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((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((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"); diff --git a/packages/agent-memory-sync/tests/integration/watch-teardown-guard.test.ts b/packages/agent-memory-sync/tests/integration/watch-teardown-guard.test.ts new file mode 100644 index 0000000..1176d25 --- /dev/null +++ b/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" + ); +});