From 1ae44dc6b096f57f0339c434e9d23f216949c2e1 Mon Sep 17 00:00:00 2001 From: johnhldiver-beep Date: Fri, 30 Oct 2026 00:48:17 +0000 Subject: [PATCH] fix(writer-lock): reap a live writer stuck at ready:false past the timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A daemon that acquires writer.pid and then never finishes its first catch-up holds the project with `ready: false` forever: the pid is alive, so nothing clears the lock, and every query times out until the process is killed by hand (#2918). Treat an explicit `ready: false` record older than ten minutes as stale, clear it pid-verified (re-read, confirm the same pid), and retry once, the same way a dead pid is cleared. Override the threshold with CODEGRAPH_WRITER_READY_TIMEOUT_MS. The held-lock message now names the start time and says the writer has not reported ready. The steal is deliberately narrow: only readiness-publishing MCP writers (daemon/direct/fallback) can go stale this way, a ready writer is never stolen however old it is (#1978), and the holder is never signalled (#2819) — only the lock is cleared, so a wedged daemon is stopped through the existing `codegraph daemon` controls and its eventual exit cannot delete the successor's record. Tests cover age classification, the env override, stealing a live stale writer, refusing a fresh one, and the held message. --- CHANGELOG.md | 1 + README.md | 2 +- __tests__/writer-lock.test.ts | 65 +++++++++++++++++++++++++++++++++ src/mcp/writer-lock.ts | 67 ++++++++++++++++++++++++++++++++--- 4 files changed, 130 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89a4f9bfeb..f2af0ec8f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- A project's MCP server no longer stays stuck holding the writer lock with every query timing out when it never finishes its first catch-up: after ten minutes the next server reclaims the lock and logs the old process and how long it went without reporting ready, tunable with `CODEGRAPH_WRITER_READY_TIMEOUT_MS`, while a server that is serving normally keeps its lock however long it runs and the wedged process is never signalled. (#2918) - In TypeScript, an awaited call with type arguments like `await fetchApi("/me")` or `await api.get(url)` now links to the function it calls. Before, the call was dropped, so typed fetch wrappers looked like they had no callers. (#2586) - In JavaScript and TypeScript, a call or `new` in a parameter's default value, like `function renderWithNote(note = buildNote())`, a destructured `{ size = defaultSize() }` or `constructor(private svc = new Service())`, now counts as the function's own call. Before, it was dropped, so `buildNote` was missing that caller and impact on it came back incomplete. Re-index JavaScript and TypeScript projects after upgrading. Thanks @inth3shadows for the report and the pull request. (#2588) - In React, a component rendered at module scope, in a top-level constant or inside a test's `it(…)` callback, like `createRoot(el).render()` or `render()`, now counts as rendered there, so an app's entry reaches its root component and a component's tests show among its callers. (#2587) diff --git a/README.md b/README.md index baac93178c..88751becac 100644 --- a/README.md +++ b/README.md @@ -890,7 +890,7 @@ Framework routing is validated the same way, on a canonical app per framework: E **MCP server not connecting** — Your agent starts the server itself, so you don't launch it by hand. Make sure the project is initialized and indexed (`codegraph status`) and that the path in your MCP config is correct. If it still won't connect, re-run `codegraph install` to rewrite the config. -**Two `codegraph serve --mcp` on one project fight over the index / auto-sync stops** — CodeGraph allows one live MCP *writer* per project (the shared background daemon, or a single direct-mode process). Extra clients should proxy to that daemon. If you set `CODEGRAPH_NO_DAEMON=1`, run only one `serve --mcp` for that project; a second instance exits with a clear writer-lock error (see `writer.pid` under `.codegraph/`). Prefer leaving the daemon enabled so multiple MCP hosts share one watcher. +**Two `codegraph serve --mcp` on one project fight over the index / auto-sync stops** — CodeGraph allows one live MCP *writer* per project (the shared background daemon, or a single direct-mode process). Extra clients should proxy to that daemon. If you set `CODEGRAPH_NO_DAEMON=1`, run only one `serve --mcp` for that project; a second instance exits with a clear writer-lock error (see `writer.pid` under `.codegraph/`). Prefer leaving the daemon enabled so multiple MCP hosts share one watcher. A server that never finishes its first catch-up no longer holds the project forever: after ten minutes at `ready: false` the next server reclaims the lock and logs how long the old process went without reporting ready (tune with `CODEGRAPH_WRITER_READY_TIMEOUT_MS`). A server that is ready keeps its lock however long it runs. **MCP tool calls fail with `Transport closed` while `codegraph status`/`sync` are healthy** — almost always WSL2 with the project on a Windows drive (a `/mnt/c` or `/mnt/d` path), where the local socket CodeGraph uses to share one background server across sessions is unreliable. CodeGraph now falls back to serving the session in-process instead of dropping the connection, but if you still hit it, set `CODEGRAPH_NO_DAEMON=1` in your MCP server's environment to skip the shared server entirely (each session runs in its own process). Moving the project onto the Linux-native filesystem (e.g. under `~/` instead of `/mnt/`) restores the shared server. diff --git a/__tests__/writer-lock.test.ts b/__tests__/writer-lock.test.ts index 028f7f29ce..fc9da3d32a 100644 --- a/__tests__/writer-lock.test.ts +++ b/__tests__/writer-lock.test.ts @@ -12,11 +12,14 @@ import { MCPEngine } from '../src/mcp/engine'; import { decodeWriterLockInfo, getWriterPidPath, + isWriterReadyStale, markWriterReady, readWriterLock, releaseWriterLock, + resolveWriterReadyTimeoutMs, tryAcquireWriterLock, writerLockHeldMessage, + WRITER_READY_TIMEOUT_MS, } from '../src/mcp/writer-lock'; describe('writer lock (#1740)', () => { @@ -139,4 +142,66 @@ describe('writer lock (#1740)', () => { expect(() => new MCPEngine({ writerLockRoot: root })).toThrow(/writer lock held/i); }); + + it('classifies ready:false by age, not by pid liveness alone (#2918)', () => { + const now = Date.now(); + expect(isWriterReadyStale({ pid: 123, mode: 'fallback', startedAt: now - WRITER_READY_TIMEOUT_MS - 1, ready: false }, now)).toBe(true); + expect(isWriterReadyStale({ pid: 123, mode: 'fallback', startedAt: now, ready: false }, now)).toBe(false); + expect(isWriterReadyStale({ pid: 123, mode: 'fallback', startedAt: now - WRITER_READY_TIMEOUT_MS - 1, ready: true }, now)).toBe(false); + expect(isWriterReadyStale({ pid: 123, mode: 'fallback', startedAt: 0, ready: false }, now)).toBe(false); + expect(isWriterReadyStale(null, now)).toBe(false); + }); + + it('honours CODEGRAPH_WRITER_READY_TIMEOUT_MS', () => { + const prev = process.env.CODEGRAPH_WRITER_READY_TIMEOUT_MS; + process.env.CODEGRAPH_WRITER_READY_TIMEOUT_MS = '1000'; + try { + expect(resolveWriterReadyTimeoutMs()).toBe(1000); + expect(isWriterReadyStale({ pid: 1, mode: 'fallback', startedAt: Date.now() - 2000, ready: false })).toBe(true); + } finally { + if (prev === undefined) delete process.env.CODEGRAPH_WRITER_READY_TIMEOUT_MS; + else process.env.CODEGRAPH_WRITER_READY_TIMEOUT_MS = prev; + } + }); + + it('steals a live writer lock that stayed at ready:false past the timeout (#2918)', () => { + const root = makeProject(); + holder = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'ignore' }); + if (!holder.pid) throw new Error('Failed to spawn writer-lock holder'); + fs.writeFileSync( + getWriterPidPath(root), + JSON.stringify({ pid: holder.pid, mode: 'fallback', startedAt: Date.now() - WRITER_READY_TIMEOUT_MS - 1_000, ready: false }) + '\n', + { mode: 0o600 }, + ); + const r = tryAcquireWriterLock(root, 'daemon'); + expect(r.kind).toBe('acquired'); + if (r.kind === 'acquired') { + expect(r.info.pid).toBe(process.pid); + expect(r.info.ready).toBe(false); + } + expect(readWriterLock(root)?.pid).toBe(process.pid); + releaseWriterLock(root); + }); + + it('does not steal a live writer that is still within the ready timeout (#2918)', () => { + const root = makeProject(); + holder = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'ignore' }); + if (!holder.pid) throw new Error('Failed to spawn writer-lock holder'); + fs.writeFileSync( + getWriterPidPath(root), + JSON.stringify({ pid: holder.pid, mode: 'fallback', startedAt: Date.now(), ready: false }) + '\n', + ); + const r = tryAcquireWriterLock(root, 'daemon'); + expect(r.kind).toBe('taken'); + if (r.kind === 'taken') expect(r.existing?.pid).toBe(holder.pid); + }); + + it('held message names a not-ready writer, its pid, and the lock file (#2918)', () => { + const root = makeProject(); + const pidPath = getWriterPidPath(root); + const msg = writerLockHeldMessage({ pid: 20632, mode: 'fallback', startedAt: Date.now(), ready: false }, pidPath); + expect(msg).toContain('20632'); + expect(msg).toContain(pidPath); + expect(msg).toContain('has not reported ready'); + }); }); diff --git a/src/mcp/writer-lock.ts b/src/mcp/writer-lock.ts index d7b78f2351..c6c09a9a26 100644 --- a/src/mcp/writer-lock.ts +++ b/src/mcp/writer-lock.ts @@ -46,6 +46,48 @@ export interface WriterLockInfo { ready?: boolean; } +/** + * How long a writer may stay at `ready: false` before it is stale (#2918). + * + * Deliberately far longer than normal catch-up (and the 30 s reader wait in + * project-lifecycle.ts), and far shorter than the ~28 h wedged daemon in + * #2918. A live pid is not, by itself, proof of a live writer: one that never + * reports ready is indistinguishable from a wedged one once this long has + * passed. Overridable for tests and very large projects via + * `CODEGRAPH_WRITER_READY_TIMEOUT_MS`. + */ +export const WRITER_READY_TIMEOUT_MS = 10 * 60 * 1000; + +export function resolveWriterReadyTimeoutMs(): number { + const raw = Number(process.env.CODEGRAPH_WRITER_READY_TIMEOUT_MS); + return Number.isFinite(raw) && raw > 0 ? raw : WRITER_READY_TIMEOUT_MS; +} + +/** + * True only for a record that explicitly says `ready: false` and whose + * `startedAt` is at least the ready timeout old. `ready: true`, a missing + * `startedAt`, and a future `startedAt` are never stale here, so legacy + * records without a `ready` field are not reaped by this check. + */ +export function isWriterReadyStale(info: WriterLockInfo | null, now = Date.now()): boolean { + if (!info || info.ready !== false || !(info.startedAt > 0)) return false; + return now - info.startedAt >= resolveWriterReadyTimeoutMs(); +} + +/** + * Modes that run an MCP writer and publish `ready: true` once its first + * catch-up finishes. Only these can go stale by staying at `ready: false`: + * `rebuild`, `cleanup` and `handover` also take `writer.pid` (or its sibling + * `rebuild.pid`) but never publish readiness, so age alone is not evidence + * they are wedged — a full re-index stays `ready: false` for as long as it + * runs. Kept narrow on purpose: an unknown mode is never stolen. + */ +const READY_PUBLISHING_MODES = new Set(['daemon', 'direct', 'fallback']); + +function isReapableNotReadyWriter(info: WriterLockInfo | null): boolean { + return !!info && READY_PUBLISHING_MODES.has(info.mode) && isWriterReadyStale(info); +} + export type WriterAcquireResult = | { kind: 'acquired'; pidPath: string; info: WriterLockInfo } | { kind: 'taken'; existing: WriterLockInfo | null; pidPath: string }; @@ -71,7 +113,13 @@ export function decodeWriterLockInfo(raw: string): WriterLockInfo | null { /** * Atomically create `writer.pid` (link-into-place, O_EXCL fallback). If held - * by a dead PID, clear and retry once. Does not steal from a live holder. + * by a dead PID, clear and retry once. A live MCP writer that has stayed at + * `ready: false` past `WRITER_READY_TIMEOUT_MS` is also stale (#2918) and is + * cleared the same pid-verified way, with a stderr line naming its pid and + * the lock file. A holder that is ready, or still within the timeout, is + * never stolen — no matter how old — and the stale holder is never signalled + * (#1978, #2819): only the lock is cleared, so a live process that later + * exits still cannot delete the successor's record (see releaseWriterLock). */ export function tryAcquireWriterLock( projectRoot: string, @@ -134,14 +182,22 @@ export function tryAcquireWriterLock( } if (result.kind === 'taken') { const existing = result.existing; - if (!existing || existing.pid <= 0 || !isProcessAlive(existing.pid)) { + if (!existing || existing.pid <= 0 || !isProcessAlive(existing.pid) || isReapableNotReadyWriter(existing)) { // Stale — clear (pid-verified) and retry once. try { const raw = fs.readFileSync(pidPath, 'utf8'); const cur = decodeWriterLockInfo(raw); if (!cur || cur.pid === existing?.pid) { - if (!cur || cur.pid <= 0 || !isProcessAlive(cur.pid)) { + if (!cur || cur.pid <= 0 || !isProcessAlive(cur.pid) || isReapableNotReadyWriter(cur)) { fs.unlinkSync(pidPath); + // Only the lock is cleared — never the holder (#1978, #2819). The + // wedged process is left to the user's daemon controls. + if (cur && cur.pid !== process.pid && isReapableNotReadyWriter(cur)) { + console.error( + `CodeGraph writer lock: cleared stale not-ready writer PID ${cur.pid} (${cur.mode}) at ${pidPath} ` + + `after ${Math.round((Date.now() - cur.startedAt) / 1000)}s without reporting ready; a new writer may now acquire the lock.`, + ); + } } } } catch { /* ENOENT ok */ } @@ -235,12 +291,15 @@ export function writerLockHeldMessage( const who = existing && existing.pid > 0 ? `PID ${existing.pid} (${existing.mode || 'unknown'} mode)` : 'another process'; + const notReady = existing && existing.ready === false && existing.startedAt > 0 + ? ` It started at ${new Date(existing.startedAt).toISOString()} and has not reported ready.` + : ''; return ( 'CodeGraph writer lock held by ' + who + '. ' + 'Only one live MCP writer may serve a project (auto-sync / index). ' + 'Stop the other server (if it is the shared daemon, pick it in `codegraph daemon` to stop it; otherwise end the other MCP session), ' + 'or unset CODEGRAPH_NO_DAEMON so additional clients proxy to the shared daemon. ' + - 'If this is stale, delete ' + pidPath + 'If this is stale, delete ' + pidPath + '.' + notReady ); }