From 3d86bcc0371079070d25901830fb2102100434c3 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Fri, 2 Oct 2026 18:03:28 +0000 Subject: [PATCH] fix(windows): the query pool never terminates a worker still starting up (#2307) Terminating a worker thread while it is still loading its modules can crash the whole process on Windows (exit 3221225477 / 0xC0000005, no error, no dump). QueryPool.destroy() terminated every worker at once, and the daemon destroys its pool whenever it stops - a session that ends within a few seconds of starting had its eager worker in exactly that state. MCPEngine.stop() didn't even wait for the pool before the daemon called process.exit(). - QueryPool tracks each worker's start (settled on 'ready', error or exit); destroy() answers outstanding calls first, then terminates each worker once it has started, capped at 15 s - the parse pool's #2301 mechanism. - MCPEngine.stop() awaits the pool. - #2301's explanation is corrected: on the Windows VM, terminating workers mid grammar compile never crashed (0 in 2,161), but terminating them while they load CodeGraph's extraction modules did. The parse pool's wait already covers that window, so only its comment and changelog wording change. Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 3 +- __tests__/query-pool.test.ts | 101 +++++++++++++++++++++++++++++++++++ src/extraction/parse-pool.ts | 14 ++--- src/mcp/engine.ts | 10 ++-- src/mcp/query-pool.ts | 62 +++++++++++++++++++-- 5 files changed, 173 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0669709589..86258f27b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -174,8 +174,9 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - On Windows, a project opened with a different drive-letter or folder-name case, like `d:\work\app` and `D:\Work\App`, now joins the CodeGraph background server that is already running for it. Before, each spelling started a server of its own that exited at once, so the session quietly served the graph by itself with no shared file watching or auto-sync, and `codegraph list` showed the project twice. Thanks @greatjackzhou. (#2278) - An agent session that lost its connection to CodeGraph's shared background server, or couldn't reach it at startup, no longer stops that server from running for the rest of the session. It reconnects on its own once the server can start, so other sessions on the same project get auto-sync back instead of running read-only. Thanks @ijbranch for the report. (#2277) - On Windows, `codegraph upgrade` no longer breaks the install while an agent session is using CodeGraph. It used to stop partway and leave CodeGraph unable to start; it now swaps the new files in with CodeGraph running and puts the previous version back if anything fails, and re-running the PowerShell installer works the same way. If an earlier upgrade already broke your install, re-run `irm https://raw.githubusercontent.com/colbymchenry/codegraph/main/install.ps1 | iex` to repair it. Thanks @tippmar-nr for the report. (#2185) -- On Windows, `codegraph init`, `codegraph index` and the background server's syncs no longer sometimes crash right after parsing on a busy machine (exit code 3221225477, an access violation). Shutting down the parsing threads could stop one while it was still loading its language grammars, which took the whole process down; they now finish loading first. +- On Windows, `codegraph init`, `codegraph index` and the background server's syncs no longer sometimes crash right after parsing on a busy machine (exit code 3221225477, an access violation). Shutting down the parsing threads could stop one while it was still starting up, which took the whole process down; they now finish starting first. - On a busy machine, indexing or syncing a small project no longer starts extra background threads for resolving references that it can't use. Under heavy load the slower pace made a ten-file project look big enough to need them, which added seconds to the run and held extra memory. +- On Windows, the CodeGraph background server no longer sometimes crashes as it shuts down (exit code 3221225477, an access violation) when it stops shortly after starting. Shutting down could stop one of its background threads while that thread was still starting up; it now lets the thread finish starting first. ## [1.6.1] - 2026-09-29 diff --git a/__tests__/query-pool.test.ts b/__tests__/query-pool.test.ts index 2087e6af47..7c28f48f04 100644 --- a/__tests__/query-pool.test.ts +++ b/__tests__/query-pool.test.ts @@ -182,6 +182,63 @@ describe('QueryPool', () => { expect(pool.healthy).toBe(false); }); + describe('destroy never terminates a worker still starting up', () => { + // Terminating a worker while it is still loading its modules can crash the + // whole process on Windows (0xC0000005), and the daemon destroys its pool + // whenever it stops. FakeWorker(…, null) never posts 'ready' by itself. + it('answers callers at once, then waits for the worker to start', async () => { + let worker!: FakeWorker; + const pool = new QueryPool({ + root: '/x', size: 1, softTimeoutMs: 10_000, + createWorker: () => (worker = new FakeWorker(() => ({ hang: true }), null)), + }); + const pending = pool.run('codegraph_explore', { query: 'q' }); + let destroyed = false; + const down = pool.destroy().then(() => { destroyed = true; }); + expect((await pending).isError).toBe(true); // not held behind the start + await sleep(50); + expect(destroyed).toBe(false); + expect(worker.alive).toBe(true); + worker.emitMessage({ type: 'ready', ok: true }); + await down; + expect(worker.alive).toBe(false); + }); + + it('terminates a started worker at once', async () => { + let worker!: FakeWorker; + const pool = new QueryPool({ root: '/x', size: 1, startSettleMs: 10_000, createWorker: () => (worker = new FakeWorker(() => ({ result: ok('r') }))) }); + await sleep(5); + const started = Date.now(); + await pool.destroy(); + expect(worker.alive).toBe(false); + expect(Date.now() - started).toBeLessThan(1_000); + }); + + it('gives up waiting on a start that never finishes', async () => { + let worker!: FakeWorker; + const pool = new QueryPool({ root: '/x', size: 1, startSettleMs: 60, createWorker: () => (worker = new FakeWorker(() => ({ hang: true }), null)) }); + const started = Date.now(); + await pool.destroy(); + expect(worker.alive).toBe(false); + expect(Date.now() - started).toBeGreaterThanOrEqual(55); + }); + + it('does not wait on a worker that died while starting', async () => { + const workers: FakeWorker[] = []; + const pool = new QueryPool({ + root: '/x', size: 1, startSettleMs: 10_000, + // The first never starts; its replacement does. + createWorker: () => { const w = new FakeWorker(() => ({ result: ok('r') }), workers.length ? true : null); workers.push(w); return w; }, + }); + workers[0].emitExit(); + await sleep(5); + const started = Date.now(); + await pool.destroy(); + expect(Date.now() - started).toBeLessThan(1_000); + expect(workers.every((w) => !w.alive)).toBe(true); + }); + }); + it('is not `ready` until a worker completes its cold start (#662 first-call stall)', async () => { // A worker cold start is seconds (tens under load); a call queued behind it // waits for the 45s busy backstop with nothing served. The ToolHandler must @@ -408,6 +465,50 @@ describe('MCP query pool with real projects (#1465)', () => { } }, 30000); + it('never terminates a real worker before it has started', async () => { + const alpha = await indexProject('alpha', 'alphaSymbol'); + const startedWhenEnded: boolean[] = []; + pool = new QueryPool({ + root: alpha, size: 1, + createWorker: () => { + const worker = new Worker(path.resolve(__dirname, '../dist/mcp/query-worker.js'), { workerData: { root: alpha } }); + let started = false; + worker.on('message', (m: { type?: string }) => { if (m?.type === 'ready') started = true; }); + const terminate = worker.terminate.bind(worker); + worker.terminate = () => { + startedWhenEnded.push(started); + return terminate(); + }; + return worker; + }, + }); + await pool.destroy(); // its eager worker is still loading + expect(startedWhenEnded).toEqual([true]); + }, 30000); + + it('engine stop waits for the query pool to shut down', async () => { + const alpha = await indexProject('alpha', 'alphaSymbol'); + const activeEngine = await start(alpha); + expect(pool).not.toBeNull(); + const realPool = pool!; + let release!: () => void; + const destroy = vi.spyOn(realPool, 'destroy').mockReturnValue(new Promise((r) => { release = r; })); + try { + let stopped = false; + const stopping = activeEngine.stop().then(() => { stopped = true; }); + // Everything else stop() closes is done well inside this; only the pool is held. + await Promise.race([stopping, sleep(1_500)]); + expect(stopped).toBe(false); + release(); + await stopping; + expect(stopped).toBe(true); + } finally { + release(); + destroy.mockRestore(); + await realPool.destroy(); + } + }, 30000); + it('honors size=0 for projectPath-only sessions', async () => { const alpha = await indexProject('alpha', 'alphaSymbol'); const workspace = path.join(tempDir, 'workspace'); diff --git a/src/extraction/parse-pool.ts b/src/extraction/parse-pool.ts index 62967a3dfb..b406a0916d 100644 --- a/src/extraction/parse-pool.ts +++ b/src/extraction/parse-pool.ts @@ -76,13 +76,13 @@ const MAX_SCALED_PARSE_TIMEOUT_MS = 20_000; */ const HARD_KILL_MULTIPLIER = 3; /** - * How long `destroy()` waits for a worker that is still loading its grammars - * before terminating it anyway. Terminating a worker whose WebAssembly - * compiles are still in flight can take the whole process down with an access - * violation (0xC0000005, seen on Windows): a pool torn down right after a short - * index often has a late-spawned worker in exactly that state. A load finishes - * in well under a second normally and a few seconds under heavy load; the cap - * only bounds a load that is wedged. + * How long `destroy()` waits for a worker that is still starting up — loading + * its modules, then its grammars — before terminating it anyway. Terminating a + * worker while it is still loading its modules can take the whole process down + * with an access violation (0xC0000005, seen on Windows): a pool torn down + * right after a short index often has a late-spawned worker in exactly that + * state. A start finishes in well under a second normally and a few seconds + * under heavy load; the cap only bounds a start that is wedged. */ const GRAMMAR_LOAD_SETTLE_MS = 15_000; /** diff --git a/src/mcp/engine.ts b/src/mcp/engine.ts index 5409c36d48..62a4c8041c 100644 --- a/src/mcp/engine.ts +++ b/src/mcp/engine.ts @@ -271,13 +271,13 @@ export class MCPEngine { // Detach + terminate the worker pool first so no tool call routes to a // worker mid-teardown; outstanding pool calls resolve with graceful guidance. + // Stopping waits for the workers to end: the daemon exits right after, and + // exiting while a worker is still starting up can crash the process. this.toolHandler.setQueryPool(null); - if (this.queryPool) { - void this.queryPool.destroy(); - this.queryPool = null; - } + const poolDown = this.queryPool ? this.queryPool.destroy() : Promise.resolve(); + this.queryPool = null; const drained = this.toolHandler.closeAll(); - this.stopPromise = drained.then(async () => { + this.stopPromise = Promise.all([drained, poolDown]).then(async () => { if (this.initPromise) await this.initPromise; if (this.defaultLease) { await this.defaultLease.release(); diff --git a/src/mcp/query-pool.ts b/src/mcp/query-pool.ts index b3a0f5322e..6b1f473991 100644 --- a/src/mcp/query-pool.ts +++ b/src/mcp/query-pool.ts @@ -69,6 +69,17 @@ const CRASH_BUDGET = 12; */ const MAX_CONCURRENT_SPAWN = 2; +/** + * How long `destroy()` waits for a worker that is still starting up before + * terminating it anyway. Terminating a worker while it is still loading its + * modules can take the whole process down with an access violation + * (0xC0000005, seen on Windows), and the daemon shuts its pool down whenever + * it stops — a session that ends within a few seconds of starting has a + * worker in exactly that state. A start takes about a second normally and a + * few under heavy load; the cap only bounds a start that is wedged. + */ +const WORKER_START_SETTLE_MS = 15_000; + /** Shape of a message a worker posts back (ready handshake or a tool result). */ interface WorkerMessage { type?: string; @@ -99,6 +110,8 @@ export interface QueryPoolOptions { maxRetries?: number; /** Worker factory (tests inject a fake). Defaults to a real `worker_threads` Worker. */ createWorker?: () => PoolWorker; + /** How long destroy() waits on a worker still starting up (tests shorten it). Default 15s. */ + startSettleMs?: number; } /** @@ -150,6 +163,10 @@ export class QueryPool { // a large DB open) saturate the box and starve the main loop. Grow only when // the queue outstrips idle + pending. private pendingWorkers = new Set(); + // Each worker's start, settled when it posts 'ready' (either way) or goes + // away — what destroy() waits on before terminating it (see + // WORKER_START_SETTLE_MS). + private starts = new Map; settle: () => void }>(); private nextId = 1; private totalCrashes = 0; private destroyed = false; @@ -158,6 +175,7 @@ export class QueryPool { private readonly softTimeoutMs: number; private readonly maxRetries: number; private readonly createWorker: () => PoolWorker; + private readonly startSettleMs: number; constructor(opts: QueryPoolOptions) { this.root = opts.root; @@ -165,6 +183,7 @@ export class QueryPool { this.softTimeoutMs = opts.softTimeoutMs ?? resolveBusyTimeoutMs(); this.maxRetries = opts.maxRetries ?? 1; this.createWorker = opts.createWorker ?? (() => new Worker(WORKER_FILE, { workerData: { root: this.root } })); + this.startSettleMs = opts.startSettleMs ?? WORKER_START_SETTLE_MS; this.spawnOne(); // one eager warm worker, ready for the first call } @@ -211,12 +230,18 @@ export class QueryPool { } this.workers.add(w); this.pendingWorkers.add(w); + let settle!: () => void; + const settled = new Promise((resolve) => { settle = resolve; }); + this.starts.set(w, { settled, settle }); w.on('message', (m) => this.onMessage(w, (m ?? {}) as WorkerMessage)); - w.on('error', () => this.onWorkerGone(w)); - w.on('exit', (code) => { if (code !== 0) this.onWorkerGone(w); }); + w.on('error', () => { this.startSettled(w); this.onWorkerGone(w); }); + w.on('exit', (code) => { this.startSettled(w); if (code !== 0) this.onWorkerGone(w); }); } private onMessage(w: PoolWorker, m: WorkerMessage): void { + // Before the retired-worker check: destroy() waits on this for a worker + // it has already let go of. + if (m?.type === 'ready') this.startSettled(w); if (!m || !this.workers.has(w)) return; // ignore late messages from retired workers if (m.type === 'ready') { if (!this.pendingWorkers.delete(w)) return; // already handled this handshake @@ -311,7 +336,36 @@ export class QueryPool { }); } - /** Terminate all workers and answer any outstanding calls gracefully. */ + private startSettled(w: PoolWorker): void { + const start = this.starts.get(w); + if (!start) return; + this.starts.delete(w); + start.settle(); + } + + /** + * Terminate a worker, but not while it is still starting up: wait for it to + * post 'ready' (or go away), up to `startSettleMs` (WORKER_START_SETTLE_MS — + * see that constant for the crash this avoids). + */ + private async terminateStarted(w: PoolWorker): Promise { + const start = this.starts.get(w); + if (start) { + let cap: NodeJS.Timeout | undefined; + await Promise.race([ + start.settled, + new Promise((resolve) => { cap = setTimeout(resolve, this.startSettleMs); cap.unref?.(); }), + ]); + clearTimeout(cap); + } + try { await w.terminate(); } catch { /* already gone */ } + } + + /** + * Terminate all workers and answer any outstanding calls gracefully. A worker + * still starting up is terminated once it has started (see + * WORKER_START_SETTLE_MS); outstanding calls are answered first. + */ async destroy(): Promise { if (this.destroyed) return; this.destroyed = true; @@ -324,6 +378,6 @@ export class QueryPool { } this.inflight.clear(); this.queue = []; - await Promise.all(ws.map((w) => Promise.resolve(w.terminate()).catch(() => { /* already gone */ }))); + await Promise.all(ws.map((w) => this.terminateStarted(w))); } }