The shared daemon's inactivity backstop (#692) reaped the daemon after maxIdleMs (default 30 min) of no inbound query bytes whenever a client was still connected — without ever checking whether that client was actually alive. lastActivityAt is fed only by inbound socket data and MCP has no keepalive, so a genuinely-live session that just hadn't queried CodeGraph in 30 min tripped it. The daemon then exited, and the proxy's onDaemonLost degrades that session (and every other session sharing the daemon) to an in-process engine for the rest of its life. On one dev machine over a day the backstop fired 20 times on live sessions (clients=1) and the liveness sweep caught 0 real dead peers — net harm. The backstop exists only to catch a phantom client (one counted but gone, whose socket-close was never delivered). It now consults the peer pids the daemon already tracks: after the inactivity window it sweeps provably-dead peers, then reaps the daemon only if NO remaining client can be proven alive (every one is an unknown-pid connection the sweep can't verify — the sole phantom class it can't catch). One provably-alive client keeps the daemon up. Extracted the decision into Daemon.backstopShouldExit(isAlive) so it's unit- testable with an injected liveness probe, mirroring reapDeadClients. All #692 guarantees preserved; the only behavior change is that a provably-alive quiet session is no longer reaped. - daemon-client-liveness.test.ts: 7 new deterministic cases for backstopShouldExit (live kept, phantom reaped, mixed protects the live one, dead-peer swept-then-held, within-window, zero-client). - mcp-daemon.test.ts: the integration test that asserted the backstop reaps a live connected client (it encoded the bug) now asserts the opposite — a live-but-quiet session survives several backstop windows with its lockfile intact and no backstop shutdown logged. Validated end-to-end on the built bundle: a quiet session's daemon stayed up across 4 backstop windows (maxIdle=3s), same pid throughout, zero backstop fires. Found while fixing #1185. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
1b13d79d1d
commit
356f5f7659
@@ -108,3 +108,74 @@ describe('Daemon.reapDeadClients', () => {
|
||||
expect(d.clients.has(s)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// The inactivity backstop (#692) must reap a phantom daemon but NEVER a
|
||||
// live-but-quiet session — reaping the latter silently degraded that session
|
||||
// (and any others sharing the daemon) to an in-process engine, and on a real
|
||||
// machine it fired far more often on live sessions than on actual phantoms.
|
||||
describe('Daemon.backstopShouldExit', () => {
|
||||
// maxIdleMs small; idleTimeoutMs:0 so a sweep that empties the set doesn't arm
|
||||
// a real timer. Force the inactivity window open by backdating lastActivityAt.
|
||||
const makeDaemon = () => {
|
||||
const d = new Daemon('/tmp/codegraph-backstop-unit-test', { idleTimeoutMs: 0, maxIdleMs: 1000 }) as any;
|
||||
d.lastActivityAt = Date.now() - 60_000; // long past the 1000ms window
|
||||
return d;
|
||||
};
|
||||
const fakeSession = () => ({ stopped: false, stop() { this.stopped = true; } });
|
||||
|
||||
it('does NOT reap while a provably-alive client stays connected (the fix)', () => {
|
||||
const d = makeDaemon();
|
||||
const live = fakeSession();
|
||||
d.clients.add(live); d.clientPeers.set(live, { pid: 222, hostPid: null });
|
||||
|
||||
expect(d.backstopShouldExit(() => true)).toBe(false); // 222 alive → keep the daemon
|
||||
expect(d.clients.has(live)).toBe(true);
|
||||
});
|
||||
|
||||
it('reaps when only an unknown-pid client remains (the phantom the sweep cannot catch)', () => {
|
||||
const d = makeDaemon();
|
||||
const phantom = fakeSession();
|
||||
d.clients.add(phantom); d.clientPeers.set(phantom, { pid: null, hostPid: null });
|
||||
|
||||
// Unknown pid → the sweep leaves it, and after the window it's a probable phantom.
|
||||
expect(d.backstopShouldExit(() => false)).toBe(true);
|
||||
});
|
||||
|
||||
it('protects a live session even when a phantom is also connected', () => {
|
||||
const d = makeDaemon();
|
||||
const live = fakeSession();
|
||||
const phantom = fakeSession();
|
||||
d.clients.add(live); d.clientPeers.set(live, { pid: 222, hostPid: null });
|
||||
d.clients.add(phantom); d.clientPeers.set(phantom, { pid: null, hostPid: null });
|
||||
|
||||
// 222 alive, phantom unknown → ANY alive keeps the daemon; the live one wins.
|
||||
expect(d.backstopShouldExit((pid: number) => pid === 222)).toBe(false);
|
||||
expect(d.clients.has(live)).toBe(true);
|
||||
});
|
||||
|
||||
it('sweeps a dead-peer client first; if that empties the set it does not exit', () => {
|
||||
const d = makeDaemon();
|
||||
const dead = fakeSession();
|
||||
d.clients.add(dead); d.clientPeers.set(dead, { pid: 111, hostPid: null });
|
||||
|
||||
// 111 dead → swept by backstopShouldExit; empty set → idle timer owns it, no backstop exit.
|
||||
expect(d.backstopShouldExit(() => false)).toBe(false);
|
||||
expect(d.clients.has(dead)).toBe(false);
|
||||
expect(dead.stopped).toBe(true);
|
||||
});
|
||||
|
||||
it('does not exit before the inactivity window elapses', () => {
|
||||
const d = makeDaemon();
|
||||
d.lastActivityAt = Date.now(); // fresh — inside the 1000ms window
|
||||
const phantom = fakeSession();
|
||||
d.clients.add(phantom); d.clientPeers.set(phantom, { pid: null, hostPid: null });
|
||||
|
||||
expect(d.backstopShouldExit(() => false)).toBe(false);
|
||||
expect(d.clients.has(phantom)).toBe(true); // not even swept yet
|
||||
});
|
||||
|
||||
it('does not exit with zero clients (the idle timer owns that case)', () => {
|
||||
const d = makeDaemon();
|
||||
expect(d.backstopShouldExit(() => false)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -362,15 +362,21 @@ describe('Shared MCP daemon (issue #411)', () => {
|
||||
}
|
||||
}, 30000);
|
||||
|
||||
// The over-the-wire client-hello → record → sweep path is covered by the
|
||||
// deterministic `Daemon.reapDeadClients` unit test in daemon-client-liveness
|
||||
// (a raw-socket variant here was flaky under heavy parallel load), plus the
|
||||
// client-hello round-trip exercised by every test above (the real proxy now
|
||||
// sends it). What stays here is the lifecycle behavior that needs real procs.
|
||||
it('exits on the inactivity backstop even while a client stays connected (#692)', async () => {
|
||||
// The over-the-wire client-hello → record → sweep path, and the inactivity
|
||||
// backstop's liveness gate, are covered by the deterministic unit tests in
|
||||
// daemon-client-liveness (`reapDeadClients`, `backstopShouldExit`) — a
|
||||
// raw-socket variant here was flaky under heavy parallel load. What stays
|
||||
// here is the lifecycle behavior that needs real procs: a live-but-quiet
|
||||
// client must SURVIVE the inactivity backstop. Reaping it used to silently
|
||||
// degrade the session (and any others sharing the daemon) to an in-process
|
||||
// engine; on a real machine the backstop fired on live sessions far more
|
||||
// often than on the phantoms it exists for. The phantom case it still covers
|
||||
// (an unknown-pid connection) is the `backstopShouldExit` unit test.
|
||||
it('does NOT reap a live-but-quiet client on the inactivity backstop (#692)', async () => {
|
||||
// Backstop short, idle timeout long: with a client connected the idle timer
|
||||
// never arms, so only the inactivity backstop can take the daemon down.
|
||||
const env = { CODEGRAPH_DAEMON_MAX_IDLE_MS: '1500', CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '60000' };
|
||||
// never arms, so the inactivity backstop is the only thing that could take
|
||||
// the daemon down — and it must not, because the client's peer is alive.
|
||||
const env = { CODEGRAPH_DAEMON_MAX_IDLE_MS: '1200', CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '60000' };
|
||||
const server = spawnServer(tempDir, env);
|
||||
servers.push(server);
|
||||
sendInitialize(server.child, `file://${tempDir}`, 1);
|
||||
@@ -379,11 +385,13 @@ describe('Shared MCP daemon (issue #411)', () => {
|
||||
const daemonPid = readLockPid(realRoot)!;
|
||||
expect(isAlive(daemonPid)).toBe(true);
|
||||
|
||||
// Send nothing further — the client stays connected but idle. The backstop
|
||||
// should fire and the daemon should exit and clean up its lockfile.
|
||||
expect(await waitProcessExit(daemonPid, 12000)).toBe(true);
|
||||
expect(readDaemonLog(realRoot)).toContain('inactivity backstop');
|
||||
expect(fs.existsSync(path.join(realRoot, '.codegraph', 'daemon.pid'))).toBe(false);
|
||||
// Stay silent well past several backstop windows. The live session's peer is
|
||||
// provably alive, so the daemon must keep running (and never log a backstop
|
||||
// shutdown), with its lockfile intact.
|
||||
await new Promise((r) => setTimeout(r, 4000)); // > 3× maxIdle
|
||||
expect(isAlive(daemonPid)).toBe(true);
|
||||
expect(readDaemonLog(realRoot)).not.toContain('inactivity backstop');
|
||||
expect(readLockPid(realRoot)).toBe(daemonPid);
|
||||
}, 30000);
|
||||
|
||||
it('daemon idle-times-out after the last client disconnects', async () => {
|
||||
|
||||
Reference in New Issue
Block a user