mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-16 20:53:57 +08:00
* fix(mcp): share one connection across two spellings of one root (#1057) A symlinked checkout, or a case-variant of a root on a case-insensitive mount (macOS, NTFS, WSL DrvFs), resolved to two path strings and opened two connections to the same .codegraph/codegraph.db. The connection cache stays keyed by path, so a root recreated at the same path still heals in place (#925). On a miss, an open root that is the same index NOW (both data directories stat to one dev:ino, read as bigints) is reused and the new spelling filed as its alias. Comparing live identities means a deleted root never matches, even if a new directory reuses its inode. Windows has no usable inode, so it compares the case-folded realpath.native instead. statInode moves to directory.ts so the DB layer and the cache share one reading, and closeAll closes an aliased instance once. * fix(mcp): keep root aliases out of the connection cache (#1057) A second spelling of an open root was filed in projectCache under its own key, so one CodeGraph sat under two keys. Anything that drops one key and closes its connection (a bounded cache's eviction, as proposed in #1929) then left the other key holding a closed handle: every query on that spelling, and through the alias scan on the original spelling too, failed with "database is not open" until restart. Aliases now live in their own map (spelling -> projectCache key). projectCache is back to one key per instance, and an alias whose entry is gone is dropped and re-resolved instead of served. * fix(mcp): deduplicate equivalent project paths (#1057) Equivalent project paths could open duplicate cached connections. Compare current filesystem identities using bigint inodes and native Windows paths. Revalidate aliases and pin cached connection owners to symlink targets. Preserve recreation recovery and single-owner cleanup. Co-authored-by: Eric Minish <eric.minish@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Eric Minish <eric.minish@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Colby Mchenry
@@ -0,0 +1,257 @@
|
||||
/**
|
||||
* One index, two spellings (#1057).
|
||||
*
|
||||
* The MCP server caches its open-DB connections by resolved-root PATH STRING, so
|
||||
* two spellings of one physical repo — a symlinked checkout, or a case-variant
|
||||
* on a case-insensitive mount (macOS, NTFS, WSL DrvFs `/mnt/c`) — each opened a
|
||||
* SEPARATE connection to the same `.codegraph/codegraph.db`. The cache stays
|
||||
* path-keyed (a same-path recreate must still heal in place, #925); a miss now
|
||||
* checks whether an open root is the same index as the new spelling, compared
|
||||
* as it is on disk NOW, without caching an alias that could later be retargeted.
|
||||
*
|
||||
* The symlink case is the deterministic, filesystem-agnostic stand-in for the
|
||||
* case-insensitive-mount scenario: both give two path strings for one inode.
|
||||
*/
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { getCodeGraphDir, isSameIndexRoot, statInode } from '../src/directory';
|
||||
import CodeGraph from '../src/index';
|
||||
import { ToolHandler, __setLoadCodeGraphForTests } from '../src/mcp/tools';
|
||||
|
||||
const posixOnly = it.runIf(process.platform !== 'win32');
|
||||
const windowsOnly = it.runIf(process.platform === 'win32');
|
||||
|
||||
describe('isSameIndexRoot (#1057)', () => {
|
||||
let tmp: string;
|
||||
|
||||
beforeEach(() => {
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-rootid-'));
|
||||
});
|
||||
afterEach(() => {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
function makeProject(name: string): string {
|
||||
const proj = path.join(tmp, name);
|
||||
fs.mkdirSync(path.join(proj, '.codegraph'), { recursive: true });
|
||||
fs.writeFileSync(path.join(proj, '.codegraph', 'codegraph.db'), 'x');
|
||||
return proj;
|
||||
}
|
||||
|
||||
it('treats a directory and a symlink that points at it as one index', () => {
|
||||
const real = makeProject('proj');
|
||||
const link = path.join(tmp, 'projLink');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
|
||||
expect(path.resolve(real)).not.toBe(path.resolve(link));
|
||||
expect(isSameIndexRoot(link, real)).toBe(true);
|
||||
});
|
||||
|
||||
it('keeps distinct projects apart', () => {
|
||||
expect(isSameIndexRoot(makeProject('a'), makeProject('b'))).toBe(false);
|
||||
});
|
||||
|
||||
it('never matches a root that is gone', () => {
|
||||
const real = makeProject('proj');
|
||||
const gone = path.join(tmp, 'does-not-exist');
|
||||
expect(isSameIndexRoot(gone, real)).toBe(false);
|
||||
expect(isSameIndexRoot(real, gone)).toBe(false);
|
||||
});
|
||||
|
||||
posixOnly('never matches a deleted root, even when its inode is handed to a new one', () => {
|
||||
// Inode reuse after delete is filesystem-dependent, so force the worst case
|
||||
// by comparing against what the old root WAS: once it is gone it no longer
|
||||
// stats, so no later directory can claim its identity.
|
||||
const old = makeProject('old');
|
||||
const oldId = statInode(getCodeGraphDir(old));
|
||||
fs.rmSync(old, { recursive: true, force: true });
|
||||
const fresh = makeProject('fresh');
|
||||
|
||||
expect(oldId).not.toBeNull();
|
||||
expect(isSameIndexRoot(old, fresh)).toBe(false);
|
||||
});
|
||||
|
||||
posixOnly('reads inodes exactly, as bigints', () => {
|
||||
const id = statInode(makeProject('proj'));
|
||||
expect(id).toMatch(/^\d+:\d+$/);
|
||||
const s = fs.statSync(path.join(tmp, 'proj'), { bigint: true });
|
||||
expect(id).toBe(`${s.dev}:${s.ino}`);
|
||||
});
|
||||
|
||||
windowsOnly('has no inode on Windows, and compares on-disk-cased real paths instead', () => {
|
||||
const real = makeProject('Proj');
|
||||
expect(statInode(real)).toBeNull();
|
||||
expect(isSameIndexRoot(real, real.toLowerCase())).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ToolHandler connection cache (#1057)', () => {
|
||||
let tmp: string;
|
||||
let handler: ToolHandler;
|
||||
const graphs: CodeGraph[] = [];
|
||||
|
||||
beforeEach(() => {
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-rootcache-'));
|
||||
__setLoadCodeGraphForTests(CodeGraph);
|
||||
handler = new ToolHandler(null);
|
||||
});
|
||||
afterEach(() => {
|
||||
handler.closeAll();
|
||||
__setLoadCodeGraphForTests(null);
|
||||
for (const cg of graphs.splice(0)) {
|
||||
try { cg.close(); } catch { /* already closed */ }
|
||||
}
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function makeIndexed(name: string): Promise<string> {
|
||||
const proj = path.join(tmp, name);
|
||||
fs.mkdirSync(path.join(proj, 'src'), { recursive: true });
|
||||
fs.writeFileSync(path.join(proj, 'src', 'a.ts'), 'export function a() { return 1; }\n');
|
||||
const cg = CodeGraph.initSync(proj);
|
||||
await cg.indexAll();
|
||||
cg.close();
|
||||
return proj;
|
||||
}
|
||||
|
||||
function open(p: string): CodeGraph {
|
||||
return (handler as unknown as { getCodeGraph(p: string): CodeGraph }).getCodeGraph(p);
|
||||
}
|
||||
|
||||
it('serves a symlinked spelling from the connection already open', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const link = path.join(tmp, 'projLink');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
|
||||
const first = open(real);
|
||||
expect(open(link)).toBe(first);
|
||||
// And the alias is remembered: the same spelling is served again.
|
||||
expect(open(link)).toBe(first);
|
||||
});
|
||||
|
||||
it('revalidates a previously seen alias after it is retargeted', async () => {
|
||||
const a = await makeIndexed('a');
|
||||
const b = await makeIndexed('b');
|
||||
const link = path.join(tmp, 'link');
|
||||
fs.symlinkSync(a, link, 'junction');
|
||||
const first = open(a);
|
||||
const second = open(b);
|
||||
expect(open(link)).toBe(first);
|
||||
|
||||
fs.unlinkSync(link);
|
||||
fs.symlinkSync(b, link, 'junction');
|
||||
expect(open(link)).toBe(second);
|
||||
expect(open(a)).toBe(first);
|
||||
});
|
||||
|
||||
it('keeps the connection owner stable when the first spelling is a symlink', async () => {
|
||||
const a = await makeIndexed('a');
|
||||
const b = await makeIndexed('b');
|
||||
const link = path.join(tmp, 'link');
|
||||
fs.symlinkSync(a, link, 'junction');
|
||||
const first = open(link);
|
||||
const second = open(b);
|
||||
expect(open(a)).toBe(first);
|
||||
|
||||
fs.unlinkSync(link);
|
||||
fs.symlinkSync(b, link, 'junction');
|
||||
expect(open(link)).toBe(second);
|
||||
expect(open(a)).toBe(first);
|
||||
});
|
||||
|
||||
posixOnly('revalidates an alias replaced by a distinct indexed directory', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const link = path.join(tmp, 'link');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
const first = open(real);
|
||||
expect(open(link)).toBe(first);
|
||||
|
||||
fs.unlinkSync(link);
|
||||
await makeIndexed('link');
|
||||
expect(open(link)).not.toBe(first);
|
||||
expect(open(real)).toBe(first);
|
||||
});
|
||||
|
||||
it('shares the default instance with another spelling of its root', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const link = path.join(tmp, 'projLink');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
|
||||
const def = CodeGraph.openSync(real);
|
||||
graphs.push(def);
|
||||
handler = new ToolHandler(def);
|
||||
expect(open(link)).toBe(def);
|
||||
});
|
||||
|
||||
it('opens distinct projects on distinct connections', async () => {
|
||||
const a = await makeIndexed('a');
|
||||
const b = await makeIndexed('b');
|
||||
expect(open(a)).not.toBe(open(b));
|
||||
});
|
||||
|
||||
it('closes an aliased connection once', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const link = path.join(tmp, 'projLink');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
|
||||
const cg = open(real);
|
||||
open(link);
|
||||
let closes = 0;
|
||||
const close = cg.close.bind(cg);
|
||||
cg.close = () => { closes++; close(); };
|
||||
handler.closeAll();
|
||||
expect(closes).toBe(1);
|
||||
});
|
||||
|
||||
it('never serves a connection that was evicted under another spelling', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const link = path.join(tmp, 'projLink');
|
||||
fs.symlinkSync(real, link, 'junction');
|
||||
|
||||
const first = open(real);
|
||||
expect(open(link)).toBe(first);
|
||||
|
||||
// Evict the entry the way a bounded cache does: drop the key, close the
|
||||
// connection. The other spelling must not be left holding a closed handle.
|
||||
const cache = (handler as unknown as { projectCache: Map<string, CodeGraph> }).projectCache;
|
||||
for (const [key, cg] of cache) {
|
||||
if (cg === first) {
|
||||
cache.delete(key);
|
||||
break;
|
||||
}
|
||||
}
|
||||
first.close();
|
||||
|
||||
const again = open(link);
|
||||
expect(again).not.toBe(first);
|
||||
expect(() => again.getStats()).not.toThrow();
|
||||
expect(open(real)).toBe(again);
|
||||
});
|
||||
|
||||
posixOnly('still heals a root recreated at the same path in place (#925)', async () => {
|
||||
const real = await makeIndexed('proj');
|
||||
const first = open(real);
|
||||
|
||||
fs.rmSync(getCodeGraphDir(real), { recursive: true, force: true });
|
||||
const cg = CodeGraph.initSync(real);
|
||||
await cg.indexAll();
|
||||
cg.close();
|
||||
|
||||
// Same path, same cached instance — reopened onto the new index, not a
|
||||
// second connection beside a leaked one.
|
||||
const again = open(real);
|
||||
expect(again).toBe(first);
|
||||
expect(again.searchNodes('a').length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
posixOnly('never hands a new root the connection of a deleted one', async () => {
|
||||
const old = await makeIndexed('old');
|
||||
const oldCg = open(old);
|
||||
fs.rmSync(old, { recursive: true, force: true });
|
||||
|
||||
const fresh = await makeIndexed('fresh');
|
||||
expect(open(fresh)).not.toBe(oldCg);
|
||||
});
|
||||
});
|
||||
+1
-18
@@ -9,7 +9,7 @@ import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import { SchemaVersion } from '../types';
|
||||
import { runMigrations, getCurrentVersion, CURRENT_SCHEMA_VERSION } from './migrations';
|
||||
import { getCodeGraphDir } from '../directory';
|
||||
import { getCodeGraphDir, statInode } from '../directory';
|
||||
|
||||
export { SqliteDatabase, SqliteBackend } from './sqlite-adapter';
|
||||
|
||||
@@ -836,23 +836,6 @@ export class DatabaseConnection {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* `dev:ino` for a path, or null if it can't be stat'd or the platform doesn't
|
||||
* report a usable inode. Windows st_ino is unreliable across handle reopens, so
|
||||
* we deliberately return null there — the deleted-but-open-inode hazard this
|
||||
* guards (#925) is a POSIX file-semantics issue that doesn't arise on Windows
|
||||
* (an open file can't be unlinked).
|
||||
*/
|
||||
function statInode(p: string): string | null {
|
||||
if (process.platform === 'win32') return null;
|
||||
try {
|
||||
const s = fs.statSync(p);
|
||||
return `${s.dev}:${s.ino}`;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Default database filename
|
||||
*/
|
||||
|
||||
@@ -155,6 +155,46 @@ export function unsafeIndexRootReason(projectRoot: string): string | null {
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* `dev:ino` for a path, or null if it can't be stat'd or the platform doesn't
|
||||
* report a usable inode. Read as bigints: WSL DrvFs (`/mnt/c`) reports inodes
|
||||
* above 2^53, where a plain number rounds nearby inodes onto one value. Windows
|
||||
* st_ino is unreliable across handle reopens, so we deliberately return null
|
||||
* there — the deleted-but-open-inode hazard this guards (#925) is a POSIX
|
||||
* file-semantics issue that doesn't arise on Windows (an open file can't be
|
||||
* unlinked).
|
||||
*/
|
||||
export function statInode(p: string): string | null {
|
||||
if (process.platform === 'win32') return null;
|
||||
try {
|
||||
const s = fs.statSync(p, { bigint: true });
|
||||
return `${s.dev}:${s.ino}`;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether two resolved index roots are one index spelled two ways — a symlinked
|
||||
* checkout, or a case-variant on a case-insensitive mount (macOS, NTFS, WSL
|
||||
* DrvFs `/mnt/c`), where `realpathSync` keeps the caller's casing (#1057).
|
||||
* Compares the identity of both data directories as they are NOW, so an inode
|
||||
* reused after a delete can't match: the deleted root no longer stats. Windows
|
||||
* has no usable inode, so it compares the on-disk-cased native realpaths.
|
||||
*/
|
||||
export function isSameIndexRoot(a: string, b: string): boolean {
|
||||
if (a === b) return true;
|
||||
if (process.platform === 'win32') {
|
||||
try {
|
||||
return fs.realpathSync.native(getCodeGraphDir(a)) === fs.realpathSync.native(getCodeGraphDir(b));
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
const id = statInode(getCodeGraphDir(a));
|
||||
return id !== null && id === statInode(getCodeGraphDir(b));
|
||||
}
|
||||
|
||||
export function findNearestCodeGraphRoot(startPath: string): string | null {
|
||||
let current = path.resolve(startPath);
|
||||
const root = path.parse(current).root;
|
||||
|
||||
+20
-5
@@ -6,7 +6,7 @@
|
||||
|
||||
import type CodeGraph from '../index';
|
||||
import type { QueryPool } from './query-pool';
|
||||
import { findNearestCodeGraphRoot } from '../directory';
|
||||
import { findNearestCodeGraphRoot, isSameIndexRoot } from '../directory';
|
||||
// Lazy-load the heavy CodeGraph chain off the MCP startup path — see the same
|
||||
// helper in engine.ts. ToolHandler must load to answer tools/list (static
|
||||
// schemas), but it must NOT drag in sqlite/query layers before the daemon binds;
|
||||
@@ -37,6 +37,7 @@ import { extractQueryPaths, queryMightContainPaths } from '../search/query-paths
|
||||
import {
|
||||
existsSync,
|
||||
readFileSync,
|
||||
realpathSync,
|
||||
statSync,
|
||||
} from 'fs';
|
||||
import { createHash } from 'crypto';
|
||||
@@ -1810,7 +1811,8 @@ export class ToolHandler {
|
||||
// support) that surfaces as intermittent
|
||||
// "database is locked" on concurrent tool calls. See issue #238. The
|
||||
// default instance is owned/closed by the server, so it's never cached.
|
||||
if (this.cg && this.cg.getProjectRoot() === resolvedRoot) {
|
||||
// Another spelling of the same root counts too (#1057).
|
||||
if (this.cg && isSameIndexRoot(this.cg.getProjectRoot(), resolvedRoot)) {
|
||||
return this.freshen(this.cg);
|
||||
}
|
||||
|
||||
@@ -1820,8 +1822,19 @@ export class ToolHandler {
|
||||
const cached = this.projectCache.get(resolvedRoot);
|
||||
if (cached) return this.freshen(cached);
|
||||
|
||||
const cg = loadCodeGraph().openSync(resolvedRoot);
|
||||
this.projectCache.set(resolvedRoot, cg);
|
||||
// Compare current identities on every cache miss: a previously seen alias
|
||||
// may have been retargeted or recreated since the last call (#1057).
|
||||
for (const [root, open] of this.projectCache) {
|
||||
if (isSameIndexRoot(root, resolvedRoot)) {
|
||||
return this.freshen(open);
|
||||
}
|
||||
}
|
||||
|
||||
// Pin the owner to the symlink target, so retargeting the first spelling
|
||||
// cannot move an existing connection onto another cached project.
|
||||
const ownerRoot = realpathSync.native(resolvedRoot);
|
||||
const cg = loadCodeGraph().openSync(ownerRoot);
|
||||
this.projectCache.set(ownerRoot, cg);
|
||||
return cg;
|
||||
}
|
||||
|
||||
@@ -1854,7 +1867,9 @@ export class ToolHandler {
|
||||
* Close all cached project connections
|
||||
*/
|
||||
closeAll(): void {
|
||||
for (const cg of this.projectCache.values()) {
|
||||
// One key per instance by design; closing through a Set keeps a second
|
||||
// close (which throws on node:sqlite) from ever stopping the loop.
|
||||
for (const cg of new Set(this.projectCache.values())) {
|
||||
cg.close();
|
||||
}
|
||||
this.projectCache.clear();
|
||||
|
||||
Reference in New Issue
Block a user