From dea076fd1e9fa3e236e93fa915bb0feebe73579d Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Mon, 5 Oct 2026 15:08:14 +0000 Subject: [PATCH] fix(mcp,extraction): exit a daemon whose install changed; never store a grammar-load failure (#2335, #2336) (#2346) * fix(mcp,extraction): a daemon whose install changed exits; grammar-load failures are never stored (#2335, #2336) A daemon that outlived an upgrade loaded grammars lazily from an install that no longer existed, and stored every file its watcher re-indexed as a zero-node row under the file's new content hash: no hash-based sync ever revisited those rows, and `codegraph status` called the index up to date. - The daemon checks its own package.json every 30 s (CODEGRAPH_DAEMON_INSTALL_CHECK_MS, 0 turns it off) and exits once the file is gone or carries another version, so the next session starts a daemon from the current install. - A result whose grammar failed to load (`parser_error`) is never stored: the file keeps its previous data and the next sync or index retries it, and a row an older engine stored that way is re-indexed even though its hash matches. The tag-based CFML path reports a missing grammar with the same code. - A launcher whose own install was deleted no longer crashes on the spawn error of the daemon it tried to start. - `codegraph status` and `status --json` (filesNeedingReindex, filesWithParseErrors) report indexed files whose symbols are missing although their content is current (#2336), and `files --json` carries each file's recorded errors. Co-Authored-By: Claude Opus 5.5 * fix(mcp): a session whose own install changed never claims the writer lock (#2335) A daemon now exits once its install is upgraded or removed, and nobody holds the writer slot for a successor at that point. Its sessions run the same replaced code and serve themselves in-process the moment it goes; one that found the slot free claimed it as its writer even though its engine could not load (a real npm upgrade: "Cannot find module './sqlite-adapter'"), and so kept a daemon from the current install from starting. Such a session now falls back read-only and says to restart it. Also adds the CHANGELOG entries for the install check and grammar-load failures (#2335) and for the status report (#2336). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 2 + __tests__/daemon-install-check.test.ts | 83 ++++++++++++ __tests__/daemon-older-version.test.ts | 45 +++++++ __tests__/grammar-load-failure.test.ts | 167 +++++++++++++++++++++++++ __tests__/status-index-health.test.ts | 159 +++++++++++++++++++++++ src/bin/codegraph.ts | 49 +++++++- src/db/queries.ts | 12 ++ src/extraction/cfml-extractor.ts | 4 +- src/extraction/grammars.ts | 17 ++- src/extraction/index.ts | 40 ++++-- src/index.ts | 30 +++++ src/mcp/daemon.ts | 72 ++++++++++- src/mcp/index.ts | 17 ++- src/mcp/version.ts | 6 +- src/types.ts | 18 +++ 15 files changed, 706 insertions(+), 15 deletions(-) create mode 100644 __tests__/daemon-install-check.test.ts create mode 100644 __tests__/grammar-load-failure.test.ts create mode 100644 __tests__/status-index-health.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 32db6b23..f280890b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In VB.NET and C#, what a property's `Get` and `Set` code calls, creates and reads now belongs to that property, as do C#'s `get => …` accessors and `=> …` property bodies. Before, it was dropped, so a method used only from a property looked unused. - In VB.NET, a field or property initializer like `= Compute()` or `As New List(Of Order)` now links what it calls and creates, and so do a `Custom Event`'s `AddHandler`, `RemoveHandler` and `RaiseEvent` blocks. Re-index VB.NET and C# projects after upgrading. - Upgrading CodeGraph while an agent session is open no longer leaves the old version's background server in charge of your project: the first session started from the new install stops it and starts a current one in its place, even while sessions opened before the upgrade are still running. That old server could no longer load the language parsers the upgrade removed, so it saved every file it re-indexed with no symbols, while new sessions could only read the index beside it without keeping it up to date. A background server from a newer install is never stopped, and sessions opened before the upgrade keep the old version until you restart them. Thanks @lipchey for the report. (#2335) +- A file is no longer saved with no symbols when its language parser can't be loaded, which is what happened to every file a background server re-indexed after an upgrade removed its install: the file keeps what it had and is indexed again once the parser loads, and files an earlier version emptied this way are re-indexed by the next sync. A background server also exits on its own once its install is upgraded or removed, so the next session starts one from the current install. Thanks @lipchey for the report. (#2335) +- `codegraph status` no longer says the index is up to date while indexed files are missing their symbols: it now names files the parser couldn't read and files stored without their symbols (which `codegraph sync` repairs), `status --json` counts both, and `codegraph files --json` lists each file's recorded errors. Thanks @lipchey for the report. (#2336) ## [1.6.2] - 2026-10-03 diff --git a/__tests__/daemon-install-check.test.ts b/__tests__/daemon-install-check.test.ts new file mode 100644 index 00000000..614afb6e --- /dev/null +++ b/__tests__/daemon-install-check.test.ts @@ -0,0 +1,83 @@ +/** + * A daemon whose install was upgraded or deleted underneath it exits (#2335). + * + * The daemon loads grammars (and the native kernel, and worker scripts) lazily + * from the directory it was started from. An upgrade replaces or deletes that + * directory while the daemon keeps running, so every later load fails; the + * 1.6.1 daemon in #2335 went on re-indexing files as empty for as long as it + * lived. It now checks its own package.json periodically and makes way for a + * daemon started from the current install. The check is pure and the Daemon's + * reaction is tested with it injected; neither starts a real daemon. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { Daemon, installChangedReason } from '../src/mcp/daemon'; + +describe('installChangedReason', () => { + let dir: string; + let pkg: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-install-check-')); + pkg = path.join(dir, 'package.json'); + fs.writeFileSync(pkg, JSON.stringify({ name: '@colbymchenry/codegraph', version: '1.6.2' })); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('is null while the install still carries the running version', () => { + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + }); + + it('reports an install upgraded in place', () => { + fs.writeFileSync(pkg, JSON.stringify({ version: '1.6.3' })); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/replaced by v1\.6\.3/); + }); + + it('reports an install that was deleted, file or whole directory', () => { + fs.rmSync(pkg); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/removed/); + fs.rmSync(dir, { recursive: true, force: true }); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/removed/); + }); + + it('waits for a package.json caught mid-write instead of exiting on it', () => { + fs.writeFileSync(pkg, '{"name": "@colbymchenry/codeg'); + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + fs.writeFileSync(pkg, ''); + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + }); +}); + +describe('Daemon.checkInstall', () => { + // A Daemon is safe to construct (nothing binds until start()); stop() is + // replaced so a positive check can't tear down the test process. + const makeDaemon = () => { + const d = new Daemon('/tmp/codegraph-install-check-unit-test', { idleTimeoutMs: 0 }) as any; + d.stop = vi.fn(async () => {}); + return d; + }; + + it('stops the daemon when its install changed', () => { + const d = makeDaemon(); + expect(d.checkInstall(() => 'Install removed')).toBe(true); + expect(d.stop).toHaveBeenCalledWith('install changed'); + }); + + it('keeps running while the install is unchanged', () => { + const d = makeDaemon(); + expect(d.checkInstall(() => null)).toBe(false); + expect(d.stop).not.toHaveBeenCalled(); + }); + + it('does nothing once the daemon is already stopping', () => { + const d = makeDaemon(); + d.stopping = true; + expect(d.checkInstall(() => 'Install removed')).toBe(false); + expect(d.stop).not.toHaveBeenCalled(); + }); +}); diff --git a/__tests__/daemon-older-version.test.ts b/__tests__/daemon-older-version.test.ts index 188cd6cf..b29cbba3 100644 --- a/__tests__/daemon-older-version.test.ts +++ b/__tests__/daemon-older-version.test.ts @@ -713,4 +713,49 @@ describe('a launcher meeting a daemon of another version (#2335)', () => { expect(writerLock()).toMatchObject({ pid: newer.pid, mode: 'daemon' }); await status(newer.session, 3, 'a tool call through the newer daemon'); }, 90000); + + it('a session whose own install was upgraded serves reads only once its daemon exits', async () => { + // An upgrade in place: the daemon notices its package.json changed and + // exits (its install check), and its session falls back in-process. As + // the project's writer that session — running code the upgrade replaced — + // would only keep a daemon from the new install from starting. + const install = makeInstall('0.0.2'); + try { + const before = await sessionWithDaemon(install, '0.0.2', { + CODEGRAPH_DAEMON_INSTALL_CHECK_MS: '200', + CODEGRAPH_DAEMON_RETRY_MS: '0', + }); + fs.writeFileSync(path.join(install, 'package.json'), JSON.stringify({ name: '@colbymchenry/codegraph', version: '0.0.3' }) + '\n'); + expect(await waitProcessExit(before.pid, 10000)).toBe(true); + expect(daemonLog()).toContain('Install replaced by v0.0.3'); + await waitFor( + () => before.session.stderr.some((l) => l.includes('Shared daemon connection lost')), + 10000, 'the session to lose its daemon', + ); + + await status(before.session, 3, 'a tool call after the daemon exited'); + const stderr = before.session.stderr.join('\n'); + expect(stderr).toContain("this session's CodeGraph install was upgraded or removed"); + expect(stderr).not.toContain('File watcher active'); + expect(writerLock()).toBeNull(); + + // So a session from the current install gets a daemon straight away. + const after = startSession(null); + await waitFor(() => attachedLine(after), 20000, 'the new session to attach'); + const lock = daemonLock(); + daemonPids.add(lock.pid); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + } finally { + // On Windows a process still running from the install blocks its removal. + for (const { child } of sessions) { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } + try { fs.unlinkSync(path.join(install, 'node_modules')); } catch { /* not created */ } + await fs.promises.rm(install, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } + }, 90000); }); diff --git a/__tests__/grammar-load-failure.test.ts b/__tests__/grammar-load-failure.test.ts new file mode 100644 index 00000000..1f285f0f --- /dev/null +++ b/__tests__/grammar-load-failure.test.ts @@ -0,0 +1,167 @@ +/** + * A grammar that fails to load is an environment failure, not a parse result + * (#2335). + * + * A long-running daemon loads tree-sitter grammars lazily from its install + * directory. When an upgrade deletes that directory underneath it, every later + * grammar load fails (ENOENT) and the extractor answers `parser_error` with no + * nodes. That used to be STORED: the file's good symbols were replaced by a + * zero-node row carrying the new content hash, so `sync` never revisited it + * and `status` stayed green. Now nothing is stored for such a result — the + * file keeps its previous index data and the next sync or index retries it — + * and rows an older engine already wrote that way are re-indexed by `sync`. + * + * The failure is simulated at `getParser` (what a failed load leaves behind: + * no parser for the language), with the native kernel switched off so every + * file goes through the grammar path regardless of whether a kernel is built. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { CodeGraph } from '../src'; +import type { Language } from '../src/types'; + +const { failing } = vi.hoisted(() => ({ failing: new Set() })); + +vi.mock('../src/extraction/grammars', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getParser: (language: Language) => (failing.has(language) ? null : actual.getParser(language)), + }; +}); + +const MATH_TS = + 'export function add(a: number, b: number): number {\n' + + ' return a + b;\n' + + '}\n' + + 'export class Calculator {\n' + + ' total = 0;\n' + + ' plus(n: number): this { this.total = add(this.total, n); return this; }\n' + + '}\n'; + +const VIEW_TS = + "import { Calculator } from './math';\n" + + 'export function total(n: number): number {\n' + + ' return new Calculator().plus(n).total;\n' + + '}\n'; + +const SUB_TS = '\nexport function sub(a: number, b: number): number {\n return a - b;\n}\n'; + +function names(cg: CodeGraph, file: string): string[] { + return cg.getNodesInFile(file).map((n) => n.name); +} + +describe('grammar load failures are never stored (#2335)', () => { + let dir: string; + let cg: CodeGraph; + let kernelEnv: string | undefined; + + beforeEach(() => { + kernelEnv = process.env.CODEGRAPH_KERNEL; + process.env.CODEGRAPH_KERNEL = '0'; + failing.clear(); + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-grammar-load-failure-')); + fs.mkdirSync(path.join(dir, 'src')); + fs.writeFileSync(path.join(dir, 'src', 'math.ts'), MATH_TS); + fs.writeFileSync(path.join(dir, 'src', 'view.ts'), VIEW_TS); + }); + + afterEach(() => { + failing.clear(); + cg?.destroy(); + if (kernelEnv === undefined) delete process.env.CODEGRAPH_KERNEL; + else process.env.CODEGRAPH_KERNEL = kernelEnv; + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('sync keeps the previous symbols of a file whose grammar failed to load, then retries it', async () => { + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/math.ts')!; + expect(before.nodeCount).toBeGreaterThan(1); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + + // The upgrade removed the grammar; the watcher then re-indexes an edit. + failing.add('typescript'); + fs.appendFileSync(path.join(dir, 'src', 'math.ts'), SUB_TS); + const failed = await cg.sync(); + + expect(failed.failedFilePaths).toContain('src/math.ts'); + const kept = cg.getFile('src/math.ts')!; + expect(kept.nodeCount).toBe(before.nodeCount); + expect(kept.contentHash).toBe(before.contentHash); + expect(kept.errors ?? []).toEqual([]); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + + // Grammar available again (a fresh process from the new install): the + // next sync sees the file as still changed and indexes the edit. + failing.clear(); + const retried = await cg.sync(); + expect(retried.filesModified).toBe(1); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator', 'sub'])); + expect(cg.getFile('src/math.ts')!.nodeCount).toBeGreaterThan(before.nodeCount); + }); + + it('a full index stores nothing for files whose grammar failed to load, and sync adds them later', async () => { + failing.add('typescript'); + cg = await CodeGraph.init(dir); + const result = await cg.indexAll(); + + expect(result.filesErrored).toBe(2); + expect(result.errors.filter((e) => e.code === 'parser_error')).toHaveLength(2); + expect(cg.getFile('src/math.ts')).toBeNull(); + expect(cg.getFile('src/view.ts')).toBeNull(); + + failing.clear(); + const synced = await cg.sync(); + expect(synced.filesAdded).toBe(2); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + expect(names(cg, 'src/view.ts')).toContain('total'); + }); + + it('sync re-indexes rows an older engine stored while the grammar was missing', async () => { + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/math.ts')!; + + // What v1.6.2 wrote for a file it re-indexed with no grammar: no nodes, + // the parser error, and the CURRENT content hash — so a hash-based + // reconcile considers it up to date forever. + const db = (cg as unknown as { db: { getDb(): { prepare(sql: string): { run(...args: unknown[]): unknown } } } }).db.getDb(); + db.prepare('DELETE FROM nodes WHERE file_path = ?').run('src/math.ts'); + db.prepare('UPDATE files SET node_count = 0, errors = ? WHERE path = ?').run( + JSON.stringify([{ message: 'Failed to get parser for language: typescript', filePath: 'src/math.ts', severity: 'error', code: 'parser_error' }]), + 'src/math.ts' + ); + expect(cg.getFile('src/math.ts')!.nodeCount).toBe(0); + + await cg.sync(); + + const healed = cg.getFile('src/math.ts')!; + expect(healed.nodeCount).toBe(before.nodeCount); + expect(healed.errors ?? []).toEqual([]); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + }); + + it('a component whose script grammar failed keeps its previous symbols too', async () => { + fs.writeFileSync( + path.join(dir, 'src', 'Counter.svelte'), + '\n\n' + ); + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/Counter.svelte')!; + expect(names(cg, 'src/Counter.svelte')).toContain('increment'); + + failing.add('typescript'); + fs.appendFileSync(path.join(dir, 'src', 'Counter.svelte'), '

edited

\n'); + await cg.sync(); + + // The component node alone would have been stored before: the script's + // symbols gone, with a nonzero node count hiding it. + expect(cg.getFile('src/Counter.svelte')!.nodeCount).toBe(before.nodeCount); + expect(names(cg, 'src/Counter.svelte')).toContain('increment'); + }); +}); diff --git a/__tests__/status-index-health.test.ts b/__tests__/status-index-health.test.ts new file mode 100644 index 00000000..b94562c8 --- /dev/null +++ b/__tests__/status-index-health.test.ts @@ -0,0 +1,159 @@ +/** + * `codegraph status` must not call an index "up to date" while indexed files + * are missing their symbols (#2336, #2335). + * + * "Up to date" used to mean only "every file's content hash matches the + * index". Two kinds of file pass that check with nothing usable in the graph: + * + * - a file the parser could not read — `export type * from` is valid + * TypeScript 5.0 the bundled grammar has no rule for, so the file is stored + * with a parse error and no symbols (#2336); + * - a row stored without its symbols — by an engine whose grammar failed to + * load (#2335), or the #1541 wipe — which `codegraph sync` re-indexes. + * + * Both are counted in `status` (human and `--json`), and `files --json` carries + * each file's recorded errors. Exercised against the built CLI so the output + * and the JSON field names are what users and scripts actually see. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + +function spawn(cwd: string, args: string[]) { + return spawnSync(process.execPath, [BIN, ...args], { + cwd, + encoding: 'utf-8', + timeout: 30_000, + env: { + ...process.env, + CODEGRAPH_NO_DAEMON: '1', + CODEGRAPH_TELEMETRY: '0', + CODEGRAPH_NO_UPDATE_CHECK: '1', + NO_COLOR: '1', + }, + }); +} + +function run(cwd: string, args: string[]): { status: number | null; out: string } { + const result = spawn(cwd, args); + return { status: result.status, out: (result.stdout ?? '') + (result.stderr ?? '') }; +} + +function runJson(cwd: string, args: string[]): any { + const result = spawn(cwd, args); + expect(result.status, (result.stdout ?? '') + (result.stderr ?? '')).toBe(0); + return JSON.parse(result.stdout); +} + +describe('status reports files indexed without their symbols (#2336, #2335)', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-status-health-')); + fs.mkdirSync(path.join(dir, 'src', 'constants'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src', 'types'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'constants', 'index.ts'), + 'export const MAX_ITEMS = 10;\nexport function clamp(n: number): number { return Math.min(n, MAX_ITEMS); }\n' + ); + fs.writeFileSync( + path.join(dir, 'src', 'types', 'database.ts'), + 'export interface Row { id: number; name: string }\nexport type RowId = Row["id"];\n' + ); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + async function index(): Promise { + const cg = CodeGraph.initSync(dir); + await cg.indexAll(); + cg.destroy(); + } + + it('a clean index is up to date, with zero counts and empty error lists', async () => { + await index(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).toContain('Index is up to date'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesNeedingReindex).toBe(0); + expect(json.index.filesWithParseErrors).toBe(0); + + const files = runJson(dir, ['files', '--json']); + expect(files).toHaveLength(2); + for (const f of files) expect(f.errors).toEqual([]); + }); + + it('a file the parser could not read keeps status from reporting "up to date"', async () => { + fs.writeFileSync( + path.join(dir, 'src', 'index.ts'), + 'export * from "./constants/index.js";\nexport type * from "./types/database.js";\n' + ); + await index(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).not.toContain('Index is up to date'); + expect(human.out).toMatch(/1 file could not be parsed/); + expect(human.out).toContain('src/index.ts'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesWithParseErrors).toBe(1); + expect(json.index.filesNeedingReindex).toBe(0); + + const files = runJson(dir, ['files', '--json']); + const broken = files.find((f: any) => f.path === 'src/index.ts'); + expect(broken.errors).toHaveLength(1); + expect(broken.errors[0]).toMatchObject({ severity: 'warning', code: 'parse_error' }); + expect(broken.errors[0].message).toContain('parse produced no symbols'); + expect(files.find((f: any) => f.path === 'src/constants/index.ts').errors).toEqual([]); + }); + + it('rows stored without their symbols ask for a sync, and the sync repairs them', async () => { + await index(); + + // A row v1.6.2 wrote with its grammar missing (#2335), and a #1541 wipe: + // both carry the current content hash, so the hash check calls them current. + const { DatabaseSync } = require('node:sqlite'); + const db = new DatabaseSync(path.join(dir, '.codegraph', 'codegraph.db')); + db.prepare('DELETE FROM nodes WHERE file_path IN (?, ?)').run('src/constants/index.ts', 'src/types/database.ts'); + db.prepare('UPDATE files SET node_count = 0, errors = ? WHERE path = ?').run( + JSON.stringify([{ message: 'Failed to get parser for language: typescript', severity: 'error', code: 'parser_error' }]), + 'src/constants/index.ts' + ); + db.prepare('UPDATE files SET node_count = 0 WHERE path = ?').run('src/types/database.ts'); + db.close(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).not.toContain('Index is up to date'); + expect(human.out).toMatch(/2 files are missing their symbols/); + expect(human.out).toContain('codegraph sync'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesNeedingReindex).toBe(2); + expect(json.index.filesWithParseErrors).toBe(0); + expect(json.pendingChanges).toEqual({ added: 0, modified: 0, removed: 0 }); + + const synced = run(dir, ['sync']); + expect(synced.status, synced.out).toBe(0); + + const after = runJson(dir, ['status', '--json']); + expect(after.index.filesNeedingReindex).toBe(0); + expect(run(dir, ['status']).out).toContain('Index is up to date'); + const files = runJson(dir, ['files', '--json']); + for (const f of files) { + expect(f.nodeCount).toBeGreaterThan(1); + expect(f.errors).toEqual([]); + } + }); +}); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index e5f8eacf..de33c1a4 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -1056,6 +1056,33 @@ program } }); +/** + * The `status` warnings for indexed files whose symbols are missing although + * their content is current (#2335, #2336): one line per group, saying what is + * wrong, what to do, and naming up to three of the files. + */ +function describeFilesMissingSymbols(health: { needsReindex: string[]; parseErrors: string[] }): string[] { + const dash = getGlyphs().dash; + const sample = (paths: string[]): string => + paths.slice(0, 3).join(', ') + (paths.length > 3 ? ` (+${formatNumber(paths.length - 3)} more)` : ''); + const lines: string[] = []; + const reindex = health.needsReindex.length; + if (reindex > 0) { + lines.push( + `${formatNumber(reindex)} ${reindex === 1 ? 'file is missing its' : 'files are missing their'} symbols ${dash} ` + + `run "codegraph sync" to re-index ${reindex === 1 ? 'it' : 'them'}: ${sample(health.needsReindex)}` + ); + } + const unparsed = health.parseErrors.length; + if (unparsed > 0) { + lines.push( + `${formatNumber(unparsed)} ${unparsed === 1 ? 'file could not be parsed, so its symbols are' : 'files could not be parsed, so their symbols are'} ` + + `missing: ${sample(health.parseErrors)} ${dash} "codegraph files --json" shows the errors` + ); + } + return lines; +} + /** * codegraph status [path] */ @@ -1103,6 +1130,9 @@ program // Zero on a healthy index; non-zero at rest means a resolution pass was // interrupted, so some files' call edges are missing (#1187). const pendingRefs = cg.getPendingReferenceCount(); + // Files whose content is current but whose symbols are missing — the + // content-hash check behind "up to date" cannot see them (#2336). + const health = cg.getIndexHealth(); // JSON output mode if (options.json) { @@ -1143,6 +1173,12 @@ program // interrupted resolution pass left edges missing; the next // sync sweeps them (#1187). pendingRefs, + // Files stored without their symbols (e.g. while their grammar + // could not load, #2335); the next sync re-indexes them. + filesNeedingReindex: health.needsReindex.length, + // Files with a recorded parse error and no symbols from it + // (#2336); unchanged until the file or the parser changes. + filesWithParseErrors: health.parseErrors.length, }, })); cg.destroy(); @@ -1235,7 +1271,10 @@ program console.log(` Removed: ${changes.removed.length} files`); } info('Run "codegraph sync" to update the index'); - } else { + } + const missingSymbols = describeFilesMissingSymbols(health); + for (const line of missingSymbols) warn(line); + if (totalChanges === 0 && missingSymbols.length === 0) { success('Index is up to date'); } console.log(); @@ -1751,6 +1790,14 @@ program language: f.language, nodeCount: f.nodeCount, size: f.size, + // What extraction recorded for the file — a parse error, a skip + // reason — so a health check needn't read the database (#2336). + errors: (f.errors ?? []).map((e) => ({ + severity: e.severity, + code: e.code, + message: e.message, + line: e.line, + })), })); console.log(JSON.stringify(output, null, 2)); cg.destroy(); diff --git a/src/db/queries.ts b/src/db/queries.ts index a12e7c26..704cfd5b 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -3253,6 +3253,18 @@ export class QueryBuilder { return rows.map(rowToFileRecord); } + /** + * Files stored with no nodes or with recorded extraction errors — the only + * rows that can be missing their symbols (`CodeGraph.getIndexHealth`). A + * healthy index returns few or none, so this stays cheap on a large one. + */ + getFilesWithoutNodesOrWithErrors(): FileRecord[] { + const rows = this.db + .prepare('SELECT * FROM files WHERE node_count = 0 OR errors IS NOT NULL ORDER BY path') + .all() as FileRow[]; + return rows.map(rowToFileRecord); + } + /** * Most recent index timestamp (ms since epoch) across all tracked files, or * null when nothing is indexed yet. One indexed aggregate, no per-row scan. (#329) diff --git a/src/extraction/cfml-extractor.ts b/src/extraction/cfml-extractor.ts index efd2ea6e..a1a66870 100644 --- a/src/extraction/cfml-extractor.ts +++ b/src/extraction/cfml-extractor.ts @@ -147,10 +147,12 @@ export class CfmlExtractor { private extractTagBased(): void { const parser = getParser('cfml'); if (!parser) { + // Same code as TreeSitterExtractor's missing parser, so a grammar that + // failed to load is never stored as this file's index data (#2335). this.errors.push({ message: 'cfml grammar not loaded', severity: 'error', - code: 'unsupported_language', + code: 'parser_error', }); return; } diff --git a/src/extraction/grammars.ts b/src/extraction/grammars.ts index 6a05b96c..0a12e61d 100644 --- a/src/extraction/grammars.ts +++ b/src/extraction/grammars.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import * as fsp from 'fs/promises'; import { Parser, Language as WasmLanguage } from 'web-tree-sitter'; -import { Language } from '../types'; +import { ExtractionError, Language } from '../types'; export type GrammarLanguage = Exclude; @@ -828,6 +828,21 @@ export function getUnavailableGrammarErrors(): Partial> return out; } +/** + * Whether these extraction errors say the file was never parsed because its + * grammar was not available (`parser_error`, recorded by the extractor when + * `getParser` has nothing for the language). + * + * That is a fact about the running process, not about the file: a daemon whose + * install was upgraded or deleted underneath it fails every lazy grammar load + * (#2335). Such a result must never replace a file's index data, and a row an + * older engine stored that way must be re-indexed — not treated as current + * because its content hash still matches. + */ +export function hasGrammarLoadFailure(errors: readonly ExtractionError[] | undefined): boolean { + return !!errors && errors.some((e) => e.code === 'parser_error'); +} + /** * Get language display name */ diff --git a/src/extraction/index.ts b/src/extraction/index.ts index 0ca3145b..ecb946ea 100644 --- a/src/extraction/index.ts +++ b/src/extraction/index.ts @@ -25,7 +25,7 @@ import { ParseWorkerPool, resolveParsePoolSize, resolveParseTimeoutMs } from './ import { StoreWriter, StoreBundle, finalizeStoreBundle } from './store-writer'; import { materializeKernelResult } from './kernel'; import { detectGeneratedFile } from './generated-detection'; -import { detectLanguage, isSourceFile, isLanguageSupported, isFileLevelOnlyLanguage, initGrammars, loadGrammarsForLanguages, readGrammarWasmBytes, isMpegTransportStream, hasMpegTsExtension, MPEG_TS_SNIFF_BYTES } from './grammars'; +import { detectLanguage, isSourceFile, isLanguageSupported, isFileLevelOnlyLanguage, initGrammars, loadGrammarsForLanguages, readGrammarWasmBytes, isMpegTransportStream, hasMpegTsExtension, MPEG_TS_SNIFF_BYTES, hasGrammarLoadFailure } from './grammars'; import { loadExtensionOverrides, loadIncludeIgnoredPatterns, loadExcludePatterns, loadIncludePatterns, PROJECT_CONFIG_FILENAME } from '../project-config'; import { isCodeGraphDataDir } from '../directory'; import { logDebug, logWarn } from '../errors'; @@ -2289,11 +2289,17 @@ export class ExtractionOrchestrator { const nodeCount = result.kernelCounts?.nodes ?? result.nodes.length; const edgeCount = result.kernelCounts?.edges ?? result.edges.length; + // A file whose grammar failed to load was never parsed (#2335). + const grammarUnavailable = hasGrammarLoadFailure(result.errors); + // Store: on the writer thread when active (fresh DB — bundles applied // in the same file order this chain dispatches them), else on the main // thread (SQLite connections are per-thread). const language = detectLanguage(filePath, content, overrides); - if (storeWriter) { + if (grammarUnavailable) { + // Store nothing: a row from an earlier run keeps its data, and with no + // row (or an older hash) the next sync or index retries the file. + } else if (storeWriter) { if (result.kernelBuffers) { // Buffers go to the writer as-is; the worker decodes + finalizes. // The main thread's only per-file work stays O(1) + the content hash. @@ -2321,7 +2327,9 @@ export class ExtractionOrchestrator { errors.push(...result.errors); } - if (nodeCount > 0) { + if (grammarUnavailable) { + filesErrored++; + } else if (nodeCount > 0) { filesIndexed++; totalNodes += nodeCount; totalEdges += edgeCount; @@ -2735,7 +2743,9 @@ export class ExtractionOrchestrator { errors.push(...result.errors); } - if (result.nodes.length > 0) { + if (hasGrammarLoadFailure(result.errors)) { + filesErrored++; // nothing was stored (#2335) + } else if (result.nodes.length > 0) { filesIndexed++; totalNodes += result.nodes.length; totalEdges += result.edges.length; @@ -2918,6 +2928,12 @@ export class ExtractionOrchestrator { result: ExtractionResult, onYield?: MaybeYield ): Promise { + // The file was never parsed: its grammar failed to load (#2335). Storing + // this would replace the file's symbols with an empty row under the new + // content hash, which no hash-based sync revisits. Keep whatever the index + // has; the stale (or missing) row makes the next sync retry the file. + if (hasGrammarLoadFailure(result.errors)) return; + // A kernel result can arrive as an undecoded buffer transport (empty // node/edge arrays, tables riding in kernelBuffers). Decode it before // storing — persisting the transport as-is records the file as having no @@ -2944,7 +2960,10 @@ export class ExtractionOrchestrator { const existingIsMarker = existingFile.nodeCount === 0 && (existingFile.errors?.length ?? 0) > 0; const incomingHasContent = result.nodes.length > 0; - if (!existingIsMarker || !incomingHasContent) { + // A row an older engine stored while the grammar could not load + // (#2335) records no parse at all: any real result replaces it. + const existingNeverParsed = hasGrammarLoadFailure(existingFile.errors); + if (!existingNeverParsed && (!existingIsMarker || !incomingHasContent)) { return; // No changes } } @@ -3400,13 +3419,18 @@ export class ExtractionOrchestrator { } const fullPath = path.join(this.rootDir, filePath); const tracked = trackedMap.get(filePath); + // A row an older engine stored while the file's grammar could not load + // (#2335) holds no parse of these bytes: re-index it even though its + // size, mtime and hash all match. Rows with a real parse error are + // deterministic and are not retried. + const neverParsed = tracked !== undefined && hasGrammarLoadFailure(tracked.errors); // Cheap pre-filter: an already-indexed file whose size AND mtime both match // the DB is unchanged — skip it without reading or hashing. (A content // change that preserves both exactly is the blind spot every mtime-based // incremental tool accepts; `index --force` is the escape hatch. Git bumps // mtime on every file it writes during checkout/merge, so pulls are caught.) - if (tracked) { + if (tracked && !neverParsed) { try { const stat = fs.statSync(fullPath); if (stat.size === tracked.size && Math.floor(stat.mtimeMs) === Math.floor(tracked.modifiedAt)) { @@ -3442,7 +3466,7 @@ export class ExtractionOrchestrator { filesToIndex.push(filePath); changedFilePaths.push(filePath); filesAdded++; - } else if (tracked.contentHash !== contentHash) { + } else if (tracked.contentHash !== contentHash || neverParsed) { onFileChange?.(filePath, content); filesToIndex.push(filePath); changedFilePaths.push(filePath); @@ -3477,7 +3501,7 @@ export class ExtractionOrchestrator { const result = await this.indexFile(filePath); if (result.errors.some(e => e.severity === 'error')) failedFilePaths.push(filePath); - nodesUpdated += result.nodes.length; + if (!hasGrammarLoadFailure(result.errors)) nodesUpdated += result.nodes.length; // else nothing was stored (#2335) const pause = backpressure?.(); if (pause) await pause; diff --git a/src/index.ts b/src/index.ts index 47330503..c1e5d25e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -25,6 +25,7 @@ import { BuildContextOptions, FindRelevantContextOptions, UnresolvedReference, + IndexHealth, } from './types'; import { DatabaseConnection, getDatabasePath, removeDatabaseFiles } from './db'; import { WalCheckpointValve, resolveWalValveMb } from './db/wal-valve'; @@ -43,6 +44,7 @@ import { extractFromSource, initGrammars, } from './extraction'; +import { hasGrammarLoadFailure, isFileLevelOnlyLanguage } from './extraction/grammars'; import { ReferenceResolver, createResolver, @@ -2065,6 +2067,34 @@ export class CodeGraph { return this.queries.countGeneratedFiles(); } + /** + * Indexed files whose symbols are missing although their content is current + * — rows a content-hash comparison calls up to date (#2336). Reported by + * `status`; `sync` repairs the `needsReindex` group. + */ + getIndexHealth(): IndexHealth { + const needsReindex: string[] = []; + const parseErrors: string[] = []; + for (const file of this.queries.getFilesWithoutNodesOrWithErrors()) { + const errors = file.errors ?? []; + if (hasGrammarLoadFailure(errors)) { + // Stored without being parsed — its grammar could not load (#2335). + needsReindex.push(file.path); + } else if (file.nodeCount === 0) { + // Every parse stores at least the file node, so zero nodes means a + // wiped row (#1541) or a recorded failure — except file-level-only + // languages and files over the size limit, which are empty on purpose. + if (isFileLevelOnlyLanguage(file.language)) continue; + if (errors.length === 0) needsReindex.push(file.path); + else if (errors.some((e) => e.severity === 'error' || e.code === 'parse_error')) parseErrors.push(file.path); + } else if (errors.some((e) => e.code === 'parse_error')) { + // Parsed, but the tree had errors and no symbols survived. + parseErrors.push(file.path); + } + } + return { needsReindex, parseErrors }; + } + // =========================================================================== // Graph Query Methods // =========================================================================== diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index 73a49695..db38f97f 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -54,7 +54,7 @@ import { getDaemonSocketCandidates, getDaemonSocketPath, } from './daemon-paths'; -import { CodeGraphPackageVersion } from './version'; +import { CodeGraphPackageJsonPath, CodeGraphPackageVersion } from './version'; import { releaseWriterLock, swapWriterLock, @@ -120,6 +120,38 @@ export function finalizeDaemonExit( /** How often the daemon sweeps connected clients for a dead peer process (#692). */ const DEFAULT_CLIENT_SWEEP_MS = 30_000; +/** How often the daemon checks that the install it runs from is still in place (#2335). */ +const DEFAULT_INSTALL_CHECK_MS = 30_000; + +/** + * Why the install this daemon was started from no longer matches it, or null + * while it still does. An upgrade replaces or deletes the package directory + * under a running daemon (npm reinstalling the package, the launcher pruning + * an old bundle), and everything the daemon loads lazily from there afterwards + * fails — grammars among them, so every file its watcher re-indexed came out + * empty (#2335). Such a daemon should make way for one started from the + * current install. Any other read failure, or a package.json caught mid-write, + * answers null: the next check looks again. Exported for testing. + */ +export function installChangedReason(packageJsonPath: string, runningVersion: string): string | null { + let raw: string; + try { + raw = fs.readFileSync(packageJsonPath, 'utf8'); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + return code === 'ENOENT' || code === 'ENOTDIR' ? `Install removed (${packageJsonPath} is gone)` : null; + } + let version: unknown; + try { + version = (JSON.parse(raw) as { version?: unknown } | null)?.version; + } catch { + return null; + } + return typeof version === 'string' && version !== runningVersion + ? `Install replaced by v${version} (this daemon is v${runningVersion})` + : null; +} + /** How long the daemon waits for the optional client-hello before proceeding without it. */ const CLIENT_HELLO_TIMEOUT_MS = 3_000; @@ -185,6 +217,7 @@ export class Daemon { private lastActivityAt = Date.now(); private maxIdleTimer: NodeJS.Timeout | null = null; private clientSweepTimer: NodeJS.Timeout | null = null; + private installCheckTimer: NodeJS.Timeout | null = null; private engine: MCPEngine; private stopping = false; private socketPath: string; @@ -372,6 +405,10 @@ export class Daemon { clearInterval(this.clientSweepTimer); this.clientSweepTimer = null; } + if (this.installCheckTimer) { + clearInterval(this.installCheckTimer); + this.installCheckTimer = null; + } process.stderr.write(`[CodeGraph daemon] Shutting down (${reason}; clients=${this.clients.size}).\n`); for (const session of [...this.clients]) { try { session.stop(); } catch { /* best-effort */ } @@ -491,6 +528,31 @@ export class Daemon { this.clientSweepTimer = setInterval(() => this.reapDeadClients(isProcessAlive), sweepMs); this.clientSweepTimer.unref?.(); } + // An install whose version could not be read at startup has nothing to + // compare against; leave that daemon alone. + const installMs = resolveInstallCheckMs(); + if (installMs > 0 && CodeGraphPackageVersion !== '0.0.0-unknown') { + this.installCheckTimer = setInterval(() => { this.checkInstall(); }, installMs); + this.installCheckTimer.unref?.(); + } + } + + /** + * Exit once the install this daemon runs from has been upgraded or removed + * (#2335; see {@link installChangedReason}). Its clients' proxies fall back + * and reconnect, and the next launch starts a daemon from the current + * install. Returns whether the daemon is exiting. The check is injected for + * tests; the timer passes the real one. + */ + checkInstall( + changed: () => string | null = () => installChangedReason(CodeGraphPackageJsonPath, CodeGraphPackageVersion), + ): boolean { + if (this.stopping) return false; + const reason = changed(); + if (!reason) return false; + process.stderr.write(`[CodeGraph daemon] ${reason}; exiting so the next session starts a daemon from the current install.\n`); + void this.stop('install changed'); + return true; } /** @@ -865,6 +927,14 @@ function resolveClientSweepMs(): number { return Math.floor(parsed); // 0 disables the sweep } +function resolveInstallCheckMs(): number { + const raw = process.env.CODEGRAPH_DAEMON_INSTALL_CHECK_MS; + if (raw === undefined || raw === '') return DEFAULT_INSTALL_CHECK_MS; + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed < 0) return DEFAULT_INSTALL_CHECK_MS; + return Math.floor(parsed); // 0 disables the check +} + /** * Parse one client-hello line. Returns the peer pids if `line` is a well-formed * client-hello (carries the `codegraph_client` marker), or null otherwise — in diff --git a/src/mcp/index.ts b/src/mcp/index.ts index ac7aa1d0..e41bd43e 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -44,12 +44,13 @@ import { MCPSession } from './session'; import { Daemon, clearStaleDaemonLock, + installChangedReason, isProcessAlive, tryAcquireDaemonLock, } from './daemon'; import { clearStaleDaemonArtifacts, stopOlderDaemon } from './daemon-registry'; import { connectWithHello, runLocalHandshakeProxy } from './proxy'; -import { CodeGraphPackageVersion } from './version'; +import { CodeGraphPackageJsonPath, CodeGraphPackageVersion } from './version'; import { readWriterLock, assertNoRebuild, @@ -150,6 +151,14 @@ function makeFallbackEngine(root: string): MCPEngine { if (existing && isProcessAlive(existing.pid)) { return readOnlyFallback(`live daemon PID ${existing.pid} holds the project lock`); } + // This session's own install was upgraded or removed underneath it (#2335): + // its code no longer loads whole, so as the project's writer it would only + // keep a daemon from the current install from starting. Its daemon exits + // for the same reason, which is how it got here. + if (CodeGraphPackageVersion !== '0.0.0-unknown' && + installChangedReason(CodeGraphPackageJsonPath, CodeGraphPackageVersion) !== null) { + return readOnlyFallback('this session\'s CodeGraph install was upgraded or removed; restart the session to use the current one'); + } return new MCPEngine({ writerLockRoot: root, queryPool: true, queryPoolDefaultMax: DIRECT_QUERY_POOL_MAX }); } @@ -295,6 +304,12 @@ function spawnDetachedDaemon(root: string, handover = false): void { env, }, ); + // An upgrade can delete the install this launcher runs from — its daemon + // then exits (#2335) and a respawn finds no executable. spawn reports that + // as an asynchronous 'error' event, which would crash this process if + // nobody listened. No daemon binds either way; the caller's poll gives up + // and the session is served in-process. + child.on('error', () => { /* no daemon — see above */ }); child.unref(); } finally { // The child holds its own dup of the log fd now; the launcher doesn't need it. diff --git a/src/mcp/version.ts b/src/mcp/version.ts index b6c919f0..e2ee11ea 100644 --- a/src/mcp/version.ts +++ b/src/mcp/version.ts @@ -20,10 +20,12 @@ import * as fs from 'fs'; import * as path from 'path'; +/** The `package.json` of the install this process runs from. */ +export const CodeGraphPackageJsonPath = path.join(__dirname, '..', '..', 'package.json'); + function readPackageVersion(): string { try { - const pkgPath = path.join(__dirname, '..', '..', 'package.json'); - const raw = fs.readFileSync(pkgPath, 'utf8'); + const raw = fs.readFileSync(CodeGraphPackageJsonPath, 'utf8'); const parsed = JSON.parse(raw); if (typeof parsed?.version === 'string' && parsed.version.length > 0) { return parsed.version; diff --git a/src/types.ts b/src/types.ts index 44ffaf4e..f3008000 100644 --- a/src/types.ts +++ b/src/types.ts @@ -265,6 +265,24 @@ export interface FileRecord { generated?: boolean; } +/** + * Indexed files whose symbols are missing even though their content hash is + * current — what a hash comparison alone calls "up to date" (#2336). Paths are + * project-relative and sorted. + */ +export interface IndexHealth { + /** + * Files `sync` re-indexes: stored while their grammar could not load (#2335) + * or stored with no nodes and no recorded reason (#1541). + */ + needsReindex: string[]; + /** + * Files with a recorded parse error and no symbols from it. Deterministic: + * they stay this way until the file (or the parser) changes. + */ + parseErrors: string[]; +} + // ============================================================================= // Extraction Types // =============================================================================