Răsfoiți Sursa

fix(daemon): gate the inactivity backstop on client liveness (#1200)

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>
Colby McHenry 2 luni în urmă
părinte
comite
10bbb3fb9b
4 a modificat fișierele cu 130 adăugiri și 20 ștergeri
  1. 1 0
      CHANGELOG.md
  2. 71 0
      __tests__/daemon-client-liveness.test.ts
  3. 21 13
      __tests__/mcp-daemon.test.ts
  4. 37 7
      src/mcp/daemon.ts

+ 1 - 0
CHANGELOG.md

@@ -36,6 +36,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 - `codegraph init` and `codegraph index` no longer get killed by the safety watchdog at the "Resolving refs" step on large method-name-heavy codebases (big Java/enterprise monorepos were the main victims, especially on slower machines). Resolution used to come up for air only every 500 references, so a dense stretch of expensive ones could starve the watchdog long enough for it to assume the process was stuck and kill a perfectly healthy index. Resolution now checkpoints after every reference, and two of the expensive steps got much cheaper: repeated method lookups on the same type are now cached, and source files are no longer re-split line-by-line for every call being resolved — indexing such repos is several times faster as a result. Generated or minified single-line files are also skipped during receiver-type inference instead of being scanned per call. Thanks @UchihaYong and @wangmeng-95 for the reports. (#1122)
 - An index left incomplete by an interrupted run now heals itself on the next sync instead of silently staying wrong forever. If indexing died partway through resolving references (a crash, Ctrl-C, or the watchdog kill fixed above), the affected files still looked indexed but their caller/impact edges were missing — a too-small blast radius clustering by package or module, e.g. a Spring `@Resource`-injected method reporting 3 of its 10 real caller files — and because incremental syncs only re-resolve files that changed, the damage was permanent until a full re-index. Any sync (a watched file change, or a bare `codegraph sync`) now detects the leftover references and finishes resolving them, `codegraph status` warns when an index is in that state instead of passing it off as healthy, and a rare early-stop that could abandon resolution on repos whose first files reference only external libraries is fixed too. Thanks @KnifeOfLife for the report and the package-correlation observation that pinned it down. (#1187)
+- The shared background server no longer shuts down out from under a live editor/agent session that simply hasn't queried CodeGraph in a while. A safety timer meant to reap an *abandoned* server — one whose client vanished without the connection ever closing — was reaping **any** server that saw no requests for 30 minutes, including a perfectly live session that just wasn't asking CodeGraph anything; that silently dropped the session (and every other session sharing the same background server) to a slower in-process mode for the rest of its life. On one machine over a day it fired 20 times on live sessions and caught zero real phantoms. The timer now checks whether the connected clients are actually still alive and only reaps when none of them are, so a quiet-but-live session keeps its shared server while a genuinely abandoned one is still cleaned up. (#1200)
 - CodeGraph's background server no longer leaves a lingering Node process behind when your editor or agent kills its launcher during startup. If the app that started CodeGraph (Codex, Claude Code, or another MCP client) was killed within the server's first fraction of a second — a config probe, a cancelled request, a startup timeout — while keeping the connection's pipes open, the server could be handed off to the system before its orphan-detection watchdog had captured a reference point, leaving it running (idle, ~30 MB) until the launching app itself exited; over a long day of repeated launches these accumulated. The server now records its parent at the earliest possible moment, and the npm and standalone launchers pass the real host's process id down to it so it can watch the host directly instead of only the launcher that may already be gone. As a final backstop, a server that never receives a single request after starting now shuts itself down instead of waiting for the host to exit — tunable with `CODEGRAPH_STARTUP_HANDSHAKE_TIMEOUT_MS` (0 disables). Thanks @ruslan33321 for the report. (#1185)
 - The automatic context hook for Claude Code now fires for structural questions asked in nearly thirty languages — French, Spanish, Portuguese, German, Italian, Dutch, Polish, Czech, Romanian, Hungarian, Greek, Swedish, Danish, Norwegian, Finnish, Russian, Ukrainian, Turkish, Indonesian, Vietnamese, Thai, Hindi, Arabic, Farsi, Hebrew, Japanese, Korean, and both simplified and traditional Chinese — instead of just English and simplified Chinese. Previously a natural question like "comment marche la state machine des commandes ?" injected nothing unless it happened to contain a code-shaped symbol name, making the hook look broken for non-English teams. English questions phrased with derived word forms ("explain the architecture…", "what are the dependencies…") now fire too, and prompts in any other language still fire when they name a symbol from the index. Thanks @anthonyle-roy-lgtm for the report. (#1126)
 - Lua and Luau method calls with capitalized names (`obj:Method()` — the standard Roblox convention) now link to the right method. Because Lua's method-call syntax looks identical to a Luau type annotation, a capitalized call like `lg:Log()` was misread as declaring the variable's type, so whenever two or more classes shared a method name (`Init`, `Update`, `Destroy`, …) the call was silently dropped from callers, impact/blast-radius, and flow traces. Lowercase method names were unaffected. Thanks @inth3shadows for the precise root-cause analysis and repro. (#1124)

+ 71 - 0
__tests__/daemon-client-liveness.test.ts

@@ -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);
+  });
+});

+ 21 - 13
__tests__/mcp-daemon.test.ts

@@ -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 () => {

+ 37 - 7
src/mcp/daemon.ts

@@ -418,9 +418,11 @@ export class Daemon {
   /**
    * Defense-in-depth against a daemon that outlives its clients (#692), for the
    * cases the refcount + idle timer miss because a socket close never arrives:
-   *   - **Inactivity backstop:** exit if no inbound traffic for `maxIdleMs` while
-   *     clients are still (nominally) connected. A phantom client sends nothing,
-   *     so it can't pin the daemon past this window.
+   *   - **Inactivity backstop:** after `maxIdleMs` with no inbound traffic, reap
+   *     the daemon — but ONLY if no connected client can be proven alive (see
+   *     {@link backstopShouldExit}). This is the sole phantom class the sweep
+   *     below can't catch: a client whose client-hello never arrived, so we have
+   *     no pid to check.
    *   - **Liveness sweep:** drop any client whose peer process has died (per the
    *     client-hello pids), which re-arms the idle timer once the last real
    *     client is gone. Catches a dead peer within one sweep instead of waiting
@@ -432,10 +434,7 @@ export class Daemon {
     if (this.maxIdleMs > 0) {
       const tick = Math.min(this.maxIdleMs, 60_000);
       this.maxIdleTimer = setInterval(() => {
-        if (this.stopping || this.clients.size === 0) return; // idle timer owns the no-client case
-        if (Date.now() - this.lastActivityAt >= this.maxIdleMs) {
-          void this.stop('inactivity backstop');
-        }
+        if (this.backstopShouldExit(isProcessAlive)) void this.stop('inactivity backstop');
       }, tick);
       this.maxIdleTimer.unref?.();
     }
@@ -446,6 +445,37 @@ export class Daemon {
     }
   }
 
+  /**
+   * Decide whether the inactivity backstop should reap the daemon right now.
+   * Public + `isAlive`-injected for deterministic tests; the timer calls it each
+   * tick with the real liveness probe.
+   *
+   * The backstop exists ONLY to catch a **phantom** client (#692) — one counted
+   * but actually gone, whose socket-close was never delivered. It must never
+   * reap a **live-but-quiet** session (connected, alive peer, just not querying):
+   * doing so silently severed the shared daemon and degraded that session — and
+   * any others sharing it — to an in-process engine. `lastActivityAt` only tracks
+   * inbound query bytes, and MCP has no keepalive, so a genuinely-live session
+   * trips the raw inactivity window after ~30 min of not being queried.
+   *
+   * So: once the inactivity window elapses, drop provably-dead peers (the same
+   * check the periodic sweep runs), then reap the daemon only when NOT ONE
+   * remaining client can be proven alive — i.e. every client left is an
+   * unknown-pid connection the sweep can't verify. A single provably-alive
+   * client keeps the daemon up. Has the sweep's side effect (drops dead peers).
+   */
+  backstopShouldExit(isAlive: (pid: number) => boolean): boolean {
+    if (this.stopping || this.clients.size === 0) return false; // idle timer owns the no-client case
+    if (Date.now() - this.lastActivityAt < this.maxIdleMs) return false; // still within the window
+    this.reapDeadClients(isAlive);
+    if (this.clients.size === 0) return false; // sweep cleared them — idle timer takes over
+    const anyProvablyAlive = [...this.clients].some((session) => {
+      const peers = this.clientPeers.get(session);
+      return peers != null && peers.pid !== null && !peerIsDead(peers, isAlive);
+    });
+    return !anyProvablyAlive;
+  }
+
   /**
    * Drop every connected client whose peer process is gone. Returns the count
    * reaped. `isAlive` is injected for testing. Clients with unknown pids (no