diff --git a/CHANGELOG.md b/CHANGELOG.md index 21ced07..87ba283 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- Initializing or indexing a project now reliably releases its database and parser workers after both successful runs and unexpected failures, preventing stale Windows file locks from blocking worktree removal. (#241) - A VBA module that declares its own variable named `Error` no longer reports every `On Error GoTo` line as a read of it, so error-handling statements stop showing up as data access. (#292) - VBA: a call written with the `Call` keyword, or with an argument list, is now reported as a call rather than as an ambiguous bare-identifier read, so a genuinely missing procedure is no longer filtered out by the constant-lookup rules meant for plain identifier reads. A bare name with no `Call` keyword and no arguments still counts as an identifier read, because it really can be a constant. (#265) - A form or report whose file was saved under a different name than the module itself carries now gets its event handlers and control references wired up, instead of quietly coming through with none of them. (#249) diff --git a/__tests__/index-orphan-watchdog.test.ts b/__tests__/index-orphan-watchdog.test.ts index 8646fc4..a3d49e9 100644 --- a/__tests__/index-orphan-watchdog.test.ts +++ b/__tests__/index-orphan-watchdog.test.ts @@ -13,8 +13,75 @@ * there and the reparenting semantics the ppid watchdog relies on are POSIX-only * (same exclusion as mcp-ppid-watchdog.test.ts). */ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect, afterEach, vi } from 'vitest'; import { spawn, ChildProcessWithoutNullStreams } from 'child_process'; + +const parsePools = vi.hoisted(() => [] as Array<{ + destroy: ReturnType; +}>); + +vi.mock('../src/extraction/parse-pool', () => ({ + resolveParsePoolSize: () => 1, + resolveParseTimeoutMs: () => 30_000, + ParseWorkerPool: class { + readonly size = 1; + readonly destroy = vi.fn(); + async requestParse() { + return { nodes: [], edges: [], errors: [{ message: 'fixture warning', severity: 'warning' }] }; + } + recycleAll() {} + constructor() { parsePools.push(this); } + }, +})); + +vi.mock('fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + existsSync: (target: fs.PathLike) => String(target).endsWith('parse-worker.js') || actual.existsSync(target), + }; +}); + +vi.mock('ignore', () => ({ + default: () => ({ add() { return this; }, ignores: () => false }), +})); +vi.mock('../src/project-config', () => ({ + loadExtensionOverrides: () => ({}), + loadIncludeIgnoredPatterns: () => [], + loadExcludePatterns: () => [], + loadVbaConfig: () => ({}), + loadIncludePatterns: () => [], + loadDysflowExportConfig: () => true, +})); +vi.mock('../src/resolution/frameworks', () => ({ detectFrameworks: () => [] })); +vi.mock('../src/extraction/tree-sitter', () => ({ extractFromSource: vi.fn() })); + +vi.mock('../src/extraction/grammars', () => ({ + detectLanguage: () => 'typescript', + isSourceFile: (file: string) => file.endsWith('.ts'), + isLanguageSupported: () => true, + isFileLevelOnlyLanguage: () => false, + initGrammars: vi.fn(async () => undefined), + loadGrammarsForLanguages: vi.fn(async () => undefined), + readGrammarWasmBytes: vi.fn(async () => ({})), +})); + +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + execFileSync: vi.fn((command: string, args: string[]) => { + if (command !== 'git') return actual.execFileSync(command, args); + if (args[0] === 'rev-parse' && args[1] === '--show-toplevel') return process.cwd() + '\n'; + if (args[0] === 'rev-parse') return '.git\n'; + if (args[0] === 'ls-files' && args.includes('-s')) { + return '100644 0000000000000000000000000000000000000000 0\t__tests__/init-resource-cleanup.test.ts\0'; + } + if (args[0] === 'ls-files') return ''; + throw new Error(`Unexpected git invocation: ${args.join(' ')}`); + }), + }; +}); import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; @@ -118,3 +185,40 @@ describe.skipIf(process.platform === 'win32')('index/init orphan supervision (#9 expect(stderr).toMatch(/Parent process exited.*aborting/); }, 20000); }); + +/** + * The worker-pool cleanup half of #241 is source-level and cross-platform. It + * drives a real ExtractionOrchestrator over one controlled repository-visible + * file while replacing only the worker boundary. + */ +describe('index parse-pool cleanup (#241)', () => { + afterEach(() => { parsePools.length = 0; }); + + async function createOrchestrator() { + const { ExtractionOrchestrator } = await import('../src/extraction'); + return new ExtractionOrchestrator(process.cwd(), {} as never); + } + + it('destroys the parse pool exactly once after a successful index', async () => { + const orchestrator = await createOrchestrator(); + + const result = await orchestrator.indexAll(); + + expect(result.success).toBe(true); + expect(parsePools).toHaveLength(1); + expect(parsePools[0]!.destroy).toHaveBeenCalledTimes(1); + }); + + it('destroys the parse pool exactly once when a post-parse callback throws', async () => { + const orchestrator = await createOrchestrator(); + + await expect(orchestrator.indexAll((progress) => { + if (progress.phase === 'parsing' && progress.current === 1) { + throw new Error('synthetic post-parse failure'); + } + })).rejects.toThrow('synthetic post-parse failure'); + + expect(parsePools).toHaveLength(1); + expect(parsePools[0]!.destroy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/__tests__/init-resource-cleanup.test.ts b/__tests__/init-resource-cleanup.test.ts new file mode 100644 index 0000000..d26acb0 --- /dev/null +++ b/__tests__/init-resource-cleanup.test.ts @@ -0,0 +1,132 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const cli = vi.hoisted(() => ({ + initAction: undefined as undefined | ((pathArg: string | undefined, options: Record) => Promise), + actionPromise: undefined as undefined | Promise, +})); + +const graph = vi.hoisted(() => ({ + init: vi.fn(), + destroy: vi.fn(), + indexAll: vi.fn(), +})); + +vi.mock('commander', () => { + class FakeCommand { + private current = ''; + + name() { return this; } + description() { return this; } + version() { return this; } + option() { return this; } + aliases() { return this; } + hook() { return this; } + command(name: string) { this.current = name; return this; } + action(fn: (...args: any[]) => Promise) { + if (this.current.startsWith('init ')) cli.initAction = fn; + return this; + } + parse() { + if (!cli.initAction) throw new Error('init action was not registered'); + cli.actionPromise = cli.initAction('C:/safe-project', {}); + } + } + return { Command: FakeCommand }; +}); + +vi.mock('../src/mcp/early-ppid', () => ({})); +vi.mock('../src/bin/fatal-handler', () => ({ installFatalHandlers: vi.fn() })); +vi.mock('../src/extraction/wasm-runtime-flags', () => ({ relaunchWithWasmRuntimeFlagsIfNeeded: vi.fn() })); +vi.mock('../src/bin/command-supervision', () => ({ installCommandSupervision: () => ({ stop: vi.fn() }) })); +vi.mock('../src/directory', () => ({ + getCodeGraphDir: (root: string) => `${root}/.codegraph`, + isInitialized: () => false, + unsafeIndexRootReason: () => null, + findNearestCodeGraphRoot: () => null, + planFrontload: vi.fn(), + hasStructuralKeyword: vi.fn(), + extractCodeTokens: vi.fn(), +})); +vi.mock('../src/index', () => ({ + default: { init: graph.init }, + getDatabasePath: (root: string) => `${root}/.codegraph/codegraph.db`, +})); +vi.mock('../src/telemetry', () => ({ + TELEMETRY_DOCS: '', + getTelemetry: () => ({ recordUsage: vi.fn(), maybeFlush: vi.fn(), flushNow: vi.fn(async () => undefined) }), + recordIndexEvent: vi.fn(), +})); +vi.mock('../src/ui/shimmer-progress', () => ({ + createShimmerProgress: () => ({ onProgress: vi.fn(), stop: vi.fn(async () => undefined) }), +})); +vi.mock('../src/ui/glyphs', () => ({ getGlyphs: () => ({ err: 'x', ok: 'ok', dash: '-', rail: '|', info: 'i', warn: '!' }) })); +vi.mock('../src/bin/daemon-release', () => ({ registerDaemonStopCommand: vi.fn() })); +vi.mock('../src/sync/worktree', () => ({ detectWorktreeIndexMismatch: vi.fn(), worktreeMismatchWarning: vi.fn() })); +vi.mock('../src/search/identifier-segments', () => ({ extractProseCandidates: vi.fn() })); +vi.mock('../src/bin/node-version-check', () => ({ buildNodeTooOldBanner: vi.fn(), isBelowMinimumNodeVersion: () => false })); + +const successfulResult = { + success: true, + filesIndexed: 1, + filesSkipped: 0, + filesErrored: 0, + nodesCreated: 1, + edgesCreated: 0, + errors: [], + durationMs: 1, +}; + +async function runInit(): Promise { + vi.resetModules(); + cli.initAction = undefined; + cli.actionPromise = undefined; + process.argv = ['node', 'codegraph', 'init', 'C:/safe-project']; + process.env.CODEGRAPH_WASM_RELAUNCHED = '1'; + const log = { error: vi.fn(), info: vi.fn(), warn: vi.fn(), success: vi.fn() }; + const clack = { intro: vi.fn(), outro: vi.fn(), log, note: vi.fn(), confirm: vi.fn(), isCancel: () => false }; + vi.stubGlobal('Function', vi.fn(() => async () => clack)); + await import('../src/bin/codegraph'); + await cli.actionPromise; +} + +describe('init resource cleanup (#241)', () => { + const originalArgv = [...process.argv]; + const originalExitCode = process.exitCode; + + afterEach(() => { + process.argv = [...originalArgv]; + process.exitCode = originalExitCode; + graph.init.mockReset(); + graph.destroy.mockReset(); + graph.indexAll.mockReset(); + vi.restoreAllMocks(); + }); + + it('releases the initialized graph after a successful initial index', async () => { + graph.init.mockResolvedValue({ indexAll: graph.indexAll, destroy: graph.destroy }); + graph.indexAll.mockResolvedValue(successfulResult); + + await runInit(); + + expect(graph.destroy).toHaveBeenCalledTimes(1); + }); + + it('releases the initialized graph when indexing fails unexpectedly', async () => { + graph.init.mockResolvedValue({ indexAll: graph.indexAll, destroy: graph.destroy }); + graph.indexAll.mockRejectedValue(new Error('synthetic index failure')); + vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never); + + await runInit(); + + expect(graph.destroy).toHaveBeenCalledTimes(1); + }); + + it('does not attempt cleanup when graph construction fails before ownership is acquired', async () => { + graph.init.mockRejectedValueOnce(new Error('synthetic construction failure')); + vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never); + + await runInit(); + + expect(graph.destroy).not.toHaveBeenCalled(); + }); +}); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index 4c88449..19e98f8 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -562,6 +562,7 @@ program const clack = await importESM('@clack/prompts'); clack.intro('Initializing CodeGraph'); + let cg: Awaited> | undefined; try { // Refuse to index your home directory / a filesystem root — it pulls in @@ -588,7 +589,8 @@ program } const { default: CodeGraph, getDatabasePath } = await loadCodeGraph(); - const cg = await CodeGraph.init(projectPath, { index: false }); + const initializedGraph = await CodeGraph.init(projectPath, { index: false }); + cg = initializedGraph; clack.log.success(`Initialized in ${projectPath}`); // Indexing runs by default now. The legacy -i/--index flag is still @@ -604,11 +606,11 @@ program const supervision = installCommandSupervision('init', { progressPaths: [dbPath, `${dbPath}-wal`] }); try { if (options.verbose) { - return await cg.indexAll({ onProgress: createVerboseProgress(), verbose: true }); + return await initializedGraph.indexAll({ onProgress: createVerboseProgress(), verbose: true }); } process.stdout.write(`${colors.dim}${getGlyphs().rail}${colors.reset}\n`); const progress = createShimmerProgress(); - const r = await cg.indexAll({ onProgress: progress.onProgress }); + const r = await initializedGraph.indexAll({ onProgress: progress.onProgress }); await progress.stop(); return r; } finally { @@ -617,7 +619,7 @@ program }; const result = await runIndex(); printIndexResult(clack, result, projectPath); - await recordIndexTelemetry(cg, result); + await recordIndexTelemetry(initializedGraph, result); // An empty graph at a git super-repo usually means `.gitignore` excludes // the child repos that hold the code — surface them and offer to opt in @@ -632,10 +634,11 @@ program } catch { /* non-fatal */ } clack.outro('Done'); - cg.destroy(); } catch (err) { clack.log.error(`Failed: ${err instanceof Error ? err.message : String(err)}`); - process.exit(1); + process.exitCode = 1; + } finally { + cg?.destroy(); } }); diff --git a/src/extraction/index.ts b/src/extraction/index.ts index 1372ae5..8d76dab 100644 --- a/src/extraction/index.ts +++ b/src/extraction/index.ts @@ -1702,6 +1702,7 @@ export class ExtractionOrchestrator { const useWorker = fs.existsSync(parseWorkerPath); let pool: ParseWorkerPool | null = null; + try { if (useWorker) { // CODEGRAPH_PARSE_WORKERS: explicit worker count; 1 = the old single-worker // behaviour (the conservative rollback). Unset → clamp(cores-1, 1, 8). @@ -1961,7 +1962,6 @@ export class ExtractionOrchestrator { } if (signal?.aborted || aborted) { - if (pool) await pool.destroy(); return { success: false, filesIndexed, @@ -2098,9 +2098,6 @@ export class ExtractionOrchestrator { } } - // Shut down the parse worker pool. - if (pool) await pool.destroy(); - return { success: filesIndexed > 0 || errors.filter((e) => e.severity === 'error').length === 0, filesIndexed, @@ -2112,6 +2109,9 @@ export class ExtractionOrchestrator { errors, durationMs: Date.now() - startTime, }; + } finally { + if (pool) await pool.destroy(); + } } /**