Просмотр исходного кода

fix(mcp): first tool call awaits catch-up sync (no stale rows for deleted files)

`MCPEngine.catchUpSync()` reconciles the index against the working tree
after open (catching `git pull`/`checkout`/`rebase` and any edits or
deletes made while no server was running). It was fire-and-forget — so a
tool call landing in the first ~50-300ms could race past it and serve
rows for files that no longer exist on disk. The per-file staleness
banner can't help here, because that signal is populated by the file
watcher (not by catch-up).

The fix: `catchUpSync()` now pushes its promise into `ToolHandler` via
`setCatchUpGate(p)`; the first `execute()` call awaits the gate and then
clears it. Subsequent calls pay nothing. Catch-up rejections are logged
by the engine and swallowed by the handler so a transient sync failure
never breaks tools.

Most visible on the "deleted everything between sessions" case, where
MCP previously returned stale rows pointing at non-existent files.
Validated end-to-end on a 10,640-file VS Code index: with the gate, a
codegraph_search for "ExtensionHost" against an empty (but stale-DB)
directory returns "No results found" after the catch-up drains the DB;
without the gate, the same call returns 10 stale hits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Colby McHenry 3 недель назад
Родитель
Сommit
f48b3129f9
4 измененных файлов с 171 добавлено и 2 удалено
  1. 12 0
      CHANGELOG.md
  2. 122 0
      __tests__/mcp-catchup-gate.test.ts
  3. 8 2
      src/mcp/engine.ts
  4. 29 0
      src/mcp/tools.ts

+ 12 - 0
CHANGELOG.md

@@ -90,6 +90,18 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   now sees the four anonymous overrides in its trail without a Read.
 
 ### Fixed
+- **MCP tools no longer return rows for files deleted while no server was
+  running.** The post-open catch-up sync that reconciles the index against
+  the working tree (catching `git pull`/`checkout`/`rebase` and any edits
+  or deletes made between sessions) was fire-and-forget — so a tool call
+  that landed in the first ~50–300ms could race past it and serve rows
+  for files that no longer exist on disk. The per-file staleness banner
+  couldn't help here, because that signal is populated by the file
+  watcher (which doesn't see pre-startup changes). Now the first tool
+  call of the session awaits the catch-up before serving; subsequent
+  calls pay nothing. Most visible on the "deleted everything between
+  sessions" case, where MCP now returns the correct empty index instead
+  of stale rows. Validated end-to-end on a 10,640-file VS Code index.
 - **`codegraph index` / `init -i` summary now reports the true edge count.**
   The per-file counter in the orchestrator only saw extraction-phase edges,
   so resolution and synthesizer edges (often >50% of the graph on

+ 122 - 0
__tests__/mcp-catchup-gate.test.ts

@@ -0,0 +1,122 @@
+/**
+ * MCP catch-up gate — first tool call blocks on the engine's post-open
+ * filesystem reconcile so it never serves rows for files that were
+ * deleted (or edited) while no MCP server was running.
+ *
+ * Background: `MCPEngine.catchUpSync()` fires `cg.sync()` in the background.
+ * Before this fix it was fire-and-forget — a tool call could race past it
+ * and return rows for files that no longer exist on disk. The per-file
+ * staleness banner (`withStalenessNotice`) couldn't help, because
+ * `getPendingFiles()` is populated by the watcher, not by catch-up.
+ *
+ * The fix: `catchUpSync()` pushes its promise into the `ToolHandler` via
+ * `setCatchUpGate(p)`; the first `execute()` call awaits the gate and then
+ * clears it. These tests exercise the gate directly (deterministic) and
+ * the engine-driven path (proves the engine actually pokes the gate).
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import * as fs from 'fs';
+import * as path from 'path';
+import * as os from 'os';
+import CodeGraph from '../src/index';
+import { ToolHandler } from '../src/mcp/tools';
+
+describe('MCP catch-up gate', () => {
+  let testDir: string;
+  let cg: CodeGraph;
+  let handler: ToolHandler;
+
+  beforeEach(async () => {
+    testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-catchup-gate-'));
+    fs.mkdirSync(path.join(testDir, 'src'));
+    fs.writeFileSync(
+      path.join(testDir, 'src', 'survivor.ts'),
+      'export function survivor() { return 1; }\n',
+    );
+    fs.writeFileSync(
+      path.join(testDir, 'src', 'deleted-later.ts'),
+      'export function deletedLater() { return 2; }\n',
+    );
+
+    cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
+    await cg.indexAll();
+    handler = new ToolHandler(cg);
+  });
+
+  afterEach(() => {
+    try { cg.unwatch(); } catch { /* ignore */ }
+    try { cg.close(); } catch { /* ignore */ }
+    if (fs.existsSync(testDir)) fs.rmSync(testDir, { recursive: true, force: true });
+  });
+
+  it('awaits the gate before serving the first tool call', async () => {
+    let gateResolved = false;
+    const gate = new Promise<void>((resolve) => {
+      setTimeout(() => { gateResolved = true; resolve(); }, 80);
+    });
+    handler.setCatchUpGate(gate);
+
+    const res = await handler.execute('codegraph_search', { query: 'survivor' });
+    expect(gateResolved).toBe(true);
+    expect(res.isError).toBeFalsy();
+    expect(res.content[0].text).toMatch(/survivor/);
+  });
+
+  it('drops the gate after first await — second call does not re-wait', async () => {
+    let awaitCount = 0;
+    const gate = new Promise<void>((resolve) => {
+      awaitCount++;
+      setTimeout(resolve, 20);
+    });
+    handler.setCatchUpGate(gate);
+
+    await handler.execute('codegraph_search', { query: 'survivor' });
+    const before = awaitCount;
+    await handler.execute('codegraph_search', { query: 'survivor' });
+    // The promise body runs once when constructed; second execute never
+    // resubscribes to a fresh promise because the gate field was nulled.
+    expect(awaitCount).toBe(before);
+  });
+
+  it('catch-up reconciles a deleted file before the first tool call sees it', async () => {
+    // Simulate the empty-project / deleted-files startup case: file is in
+    // the DB (we indexed it above) but vanishes from disk before the MCP
+    // server's first query. The catch-up sync, awaited via the gate,
+    // must remove the row so the first tool call returns no hit.
+    fs.unlinkSync(path.join(testDir, 'src', 'deleted-later.ts'));
+
+    // Push the actual catch-up sync as the gate — same flow the MCP engine
+    // uses (`cg.sync()` returns a Promise<SyncResult>, the wrapper voids it).
+    handler.setCatchUpGate(cg.sync().then(() => undefined));
+
+    const res = await handler.execute('codegraph_search', { query: 'deletedLater' });
+    expect(res.isError).toBeFalsy();
+    const text = res.content[0].text;
+    expect(text).not.toMatch(/src\/deleted-later\.ts/);
+  });
+
+  it('catch-up that converges the project to 0 files clears all rows', async () => {
+    // Worst case: every source file is gone between sessions. Without the
+    // gate, the first tool call serves whatever was in the DB. With the
+    // gate + the orchestrator's filesystem reconcile, the DB drains.
+    fs.unlinkSync(path.join(testDir, 'src', 'survivor.ts'));
+    fs.unlinkSync(path.join(testDir, 'src', 'deleted-later.ts'));
+
+    handler.setCatchUpGate(cg.sync().then(() => undefined));
+
+    const res = await handler.execute('codegraph_search', { query: 'survivor' });
+    expect(res.isError).toBeFalsy();
+    expect(cg.getStats().fileCount).toBe(0);
+  });
+
+  it('gate that rejects does not break the tool call', async () => {
+    // A catch-up sync failure (lock contention, transient FS error) must
+    // not poison tool dispatch — the engine logs it, the handler proceeds.
+    handler.setCatchUpGate(Promise.reject(new Error('simulated sync failure')));
+
+    const res = await handler.execute('codegraph_search', { query: 'survivor' });
+    expect(res.isError).toBeFalsy();
+    expect(res.content[0].text).toMatch(/survivor/);
+  });
+});

+ 8 - 2
src/mcp/engine.ts

@@ -222,12 +222,17 @@ export class MCPEngine {
   /**
    * Reconcile the index with the current filesystem once, right after open —
    * catches edits, adds, deletes, and `git pull`/`checkout` changes made while
-   * no watcher was running. Background, never awaited.
+   * no watcher was running. Runs in the background, but the returned promise
+   * is pushed into the ToolHandler as a one-shot gate so the *first* tool
+   * call awaits completion before serving (without this, a tool call that
+   * races past sync returns rows for files that no longer exist on disk —
+   * and the per-file staleness banner can't help because `getPendingFiles()`
+   * is populated by the watcher, not by catch-up).
    */
   private catchUpSync(): void {
     const cg = this.cg;
     if (!cg) return;
-    void cg
+    const p = cg
       .sync()
       .then((result) => {
         const changed = result.filesAdded + result.filesModified + result.filesRemoved;
@@ -239,6 +244,7 @@ export class MCPEngine {
         const msg = err instanceof Error ? err.message : String(err);
         process.stderr.write(`[CodeGraph MCP] Catch-up sync failed: ${msg}\n`);
       });
+    this.toolHandler.setCatchUpGate(p);
   }
 }
 

+ 29 - 0
src/mcp/tools.ts

@@ -624,6 +624,14 @@ export class ToolHandler {
   // once and every later tool call reuses the result — never shelling out to
   // git on the hot path. `undefined` = not computed yet; `null` = no mismatch.
   private worktreeMismatchCache: Map<string, WorktreeIndexMismatch | null> = new Map();
+  // Gate that the MCP engine pokes after `cg.open()` so the first tool call
+  // blocks on the post-open filesystem reconcile (catch-up sync). Without
+  // this, a tool call that races past `catchUpSync()` serves rows for files
+  // that were deleted (or edited) while no MCP server was running — and the
+  // per-file staleness banner can't help, because `getPendingFiles()` is
+  // populated by the watcher, not by catch-up. Cleared on first await so
+  // subsequent calls don't pay any cost.
+  private catchUpGate: Promise<void> | null = null;
 
   constructor(private cg: CodeGraph | null) {}
 
@@ -634,6 +642,17 @@ export class ToolHandler {
     this.cg = cg;
   }
 
+  /**
+   * Engine-only: register the catch-up sync promise so the next `execute()`
+   * call awaits it before serving. The handler swallows rejections (the
+   * engine logs them) so a sync failure never propagates as a tool error;
+   * we still want to serve a best-effort result over the same potentially-
+   * stale data, which is what would have happened without the gate.
+   */
+  setCatchUpGate(p: Promise<void> | null): void {
+    this.catchUpGate = p;
+  }
+
   /**
    * Record the directory the server tried to resolve the default project from.
    * Used only to make the "no default project" error actionable.
@@ -999,6 +1018,16 @@ export class ToolHandler {
    */
   async execute(toolName: string, args: Record<string, unknown>): Promise<ToolResult> {
     try {
+      // Block the first tool call on the engine's post-open reconcile so we
+      // never serve rows for files deleted/edited while no MCP server was
+      // running. The gate is cleared after first await — subsequent calls
+      // pay nothing. Catch-up failures are logged by the engine; we
+      // proceed regardless so a transient sync error never breaks tools.
+      if (this.catchUpGate) {
+        const gate = this.catchUpGate;
+        this.catchUpGate = null;
+        try { await gate; } catch { /* engine already logged */ }
+      }
       // Honor the optional tool allowlist (CODEGRAPH_MCP_TOOLS): a trimmed
       // surface rejects ablated tools defensively even if a client cached them.
       if (!this.isToolAllowed(toolName)) {