Files
thedotmack__claude-mem/tests/shared/kill-process-tree-modes.test.ts
T
Alex Newman e2421eb1a1 fix(process): make root identity checking the default, not opt-in
Greptile P1 #5/#6. The findings are two more instances, but the shape is
the bug: root identity was opt-in via expectedStartToken, so any call
site that omitted the option silently reopened the reuse hole. Eleven of
the thirteen real call sites omit it. That is why each review round found
another one, and patching these two would have set up a fifth.

killProcessTree now captures the root's start token ITSELF at entry when
the caller did not supply one, and revalidates before every signal it
sends — the initial root signal, the post-settle re-enumeration, and the
final SIGKILL. Omitting the option is safe; a call site can now only
weaken this by passing an explicitly wrong token, not by forgetting an
argument.

expectedStartToken remains, because it is a strictly stronger guarantee
and not redundant:
  - self-captured (default) detects reuse during THIS function's awaits;
  - caller-supplied additionally detects reuse that happened BEFORE
    entry, which self-capture cannot see — by then it would be reading
    the replacement's token. ChromaMcpManager needs that: its PID is
    captured before `await transport.close()`.

A reused root skips the root signal but does NOT abort the call: the
descendants enumerated earlier were the real root's children and still
have to be reaped. Re-enumeration after the settle IS skipped, since
walking a reused PID would list the replacement's children.

P1 #6 (session-reap initial teardown) now passes its earlier-captured
token too. That is belt-and-braces given the new default, but the
reaper captured the token before the loop's awaits, so it is the
stronger of the two guarantees.

Also fixes a genuinely timing-dependent fixture this exposed. The #3378
control asserted graceful mode delivers SIGTERM to the root, using a
`sleep 300 & wait` root — which exits on its own the moment the
descendant pass kills its sleep. The added revalidation (a `ps` spawn on
macOS) widened that window enough for the root to be gone before the
root signal landed, so the trap never fired. The fixture now self-loops
and outlives its child; it was measuring fixture timing, not delivery.

Cost note: revalidation adds a start-token read per signal point —
effectively free on Linux (/proc) and Windows (5s TTL cache), ~10-30ms
per read on macOS (`ps` spawn). Teardown is not a hot path and the
correctness is worth it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-19 02:04:18 -07:00

170 lines
6.1 KiB
TypeScript

import { describe, it, expect } from 'bun:test';
import { spawn } from 'child_process';
import fs from 'fs';
import os from 'os';
import path from 'path';
import {
killProcessTree,
collectDescendantPids,
ProcessTreeKillError,
} from '../../src/shared/kill-process-tree.js';
import { isPidAlive } from '../../src/supervisor/process-registry.js';
const isPosix = process.platform !== 'win32';
async function waitUntil(predicate: () => boolean, timeoutMs: number): Promise<boolean> {
const deadline = Date.now() + timeoutMs;
while (Date.now() < deadline) {
if (predicate()) return true;
await new Promise(resolve => setTimeout(resolve, 50));
}
return predicate();
}
/**
* A shell that forks a grandchild and then sleeps, so the tree is genuinely
* two levels deep — a single-PID kill leaves the grandchild running, which is
* the #3482 shape in miniature.
*/
function spawnTwoLevelTree(): { rootPid: number } {
// `& wait` keeps the shell itself alive as the parent instead of letting it
// exec away into the last command, so the tree really is two levels deep.
const child = spawn('/bin/sh', ['-c', 'sleep 300 & sleep 300 & wait'], {
stdio: 'ignore',
detached: false,
});
return { rootPid: child.pid! };
}
/** Give the shell time to actually fork before sampling the tree. */
function settle(ms = 500): Promise<void> {
return new Promise(resolve => setTimeout(resolve, ms));
}
describe.if(isPosix)('killProcessTree signal modes', () => {
it("'immediate' reaps the whole tree without sending SIGTERM (#3378)", async () => {
const { rootPid } = spawnTwoLevelTree();
await settle();
const descendants = await collectDescendantPids(rootPid);
expect(descendants.length).toBeGreaterThan(0);
await killProcessTree(rootPid, { signalMode: 'immediate' });
const allGone = await waitUntil(
() => !isPidAlive(rootPid) && descendants.every(pid => !isPidAlive(pid)),
10_000
);
expect(allGone).toBe(true);
}, 30_000);
it("'graceful' (default) also reaps the whole tree", async () => {
const { rootPid } = spawnTwoLevelTree();
await settle();
const descendants = await collectDescendantPids(rootPid);
expect(descendants.length).toBeGreaterThan(0);
await killProcessTree(rootPid);
const allGone = await waitUntil(
() => !isPidAlive(rootPid) && descendants.every(pid => !isPidAlive(pid)),
10_000
);
expect(allGone).toBe(true);
}, 30_000);
it('is not an error when the target is already gone', async () => {
const { rootPid } = spawnTwoLevelTree();
await settle();
await killProcessTree(rootPid, { signalMode: 'immediate' });
await waitUntil(() => !isPidAlive(rootPid), 10_000);
// Second call against a corpse must resolve, not throw.
await killProcessTree(rootPid, { signalMode: 'immediate' });
}, 30_000);
});
describe.if(isPosix)('the #3378 invariant is enforced, not just documented', () => {
// The recycle path must run ZERO stale-version shutdown code. That holds
// only if 'immediate' never sends SIGTERM — a catchable signal would let a
// stale worker execute its own shutdown/handoff logic and re-spawn itself,
// which is the restart storm (#3378) the invariant exists to prevent.
//
// Asserting that in a comment is worthless: a regression reinstating
// SIGTERM-then-SIGKILL would pass a comment unchanged. So the root here
// TRAPS SIGTERM and touches a marker file. The marker's absence is the
// proof; its presence means SIGTERM was delivered.
it("'immediate' never delivers SIGTERM to the root", async () => {
const marker = path.join(
os.tmpdir(),
`claude-mem-sigterm-marker-${process.pid}-${Date.now()}`
);
const child = spawn(
'/bin/sh',
// The root must OUTLIVE its child. `sleep 300 & wait` ends as soon as the
// descendant pass kills the sleep, and the root then exits on its own
// before the root signal lands — the trap never fires and the assertion
// measures fixture timing rather than delivered signals. A self-looping
// root stays alive until it is signalled directly.
['-c', `trap 'touch "${marker}"; exit 0' TERM; while :; do sleep 1; done`],
{ stdio: 'ignore' }
);
const rootPid = child.pid!;
await settle();
await killProcessTree(rootPid, { signalMode: 'immediate' });
await waitUntil(() => !isPidAlive(rootPid), 10_000);
// Give a delivered-but-slow handler room to write, so a pass cannot be
// an artifact of checking too early.
await settle(750);
const trapFired = fs.existsSync(marker);
fs.rmSync(marker, { force: true });
expect(trapFired).toBe(false);
}, 30_000);
it("'graceful' DOES deliver SIGTERM — proving the marker works", async () => {
// Control case. Without this, a marker that never fires for an unrelated
// reason (trap syntax, tmpdir perms) would make the test above vacuous.
const marker = path.join(
os.tmpdir(),
`claude-mem-sigterm-control-${process.pid}-${Date.now()}`
);
const child = spawn(
'/bin/sh',
// The root must OUTLIVE its child. `sleep 300 & wait` ends as soon as the
// descendant pass kills the sleep, and the root then exits on its own
// before the root signal lands — the trap never fires and the assertion
// measures fixture timing rather than delivered signals. A self-looping
// root stays alive until it is signalled directly.
['-c', `trap 'touch "${marker}"; exit 0' TERM; while :; do sleep 1; done`],
{ stdio: 'ignore' }
);
const rootPid = child.pid!;
await settle();
await killProcessTree(rootPid);
await waitUntil(() => !isPidAlive(rootPid), 10_000);
await settle(750);
const trapFired = fs.existsSync(marker);
fs.rmSync(marker, { force: true });
expect(trapFired).toBe(true);
}, 30_000);
});
describe('ProcessTreeKillError', () => {
it('carries the pid so callers can report which kill failed', () => {
const error = new ProcessTreeKillError(4242, 'taskkill failed');
expect(error).toBeInstanceOf(Error);
expect(error.name).toBe('ProcessTreeKillError');
expect(error.pid).toBe(4242);
});
});