From a9e6c32dc921cd744814f9b00de3ced21fb144e3 Mon Sep 17 00:00:00 2001 From: Christopher Beaulieu Date: Thu, 10 Sep 2026 20:42:27 -0400 Subject: [PATCH] fix(daemon): preserve live legacy locks (#1834) --- CHANGELOG.md | 2 + __tests__/cli-unlock.test.ts | 31 ++++++ __tests__/daemon-manager.test.ts | 29 +++++ __tests__/daemon-registry.test.ts | 85 ++++++++++++++ __tests__/daemon-socket-fallback.test.ts | 42 +++++++ __tests__/mcp-daemon.test.ts | 135 ++++++++++++++++++++--- __tests__/writer-lock.test.ts | 30 +++++ src/mcp/daemon-manager.ts | 14 +++ src/mcp/daemon-paths.ts | 16 ++- src/mcp/daemon-registry.ts | 89 +++++++++++---- src/mcp/daemon.ts | 82 +++++++++----- src/mcp/engine.ts | 15 ++- src/mcp/index.ts | 85 ++++++++++++-- 13 files changed, 576 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9b68b091..6ea62ee52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -151,6 +151,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). #### MCP / indexing +- Daemon startup and cleanup now preserve live legacy PID-only locks while still reclaiming dead or identity-disproved records, preventing two writers from serving the same project. + - The prompt hook no longer injects unrelated projects when run from your home directory or a broader directory containing a stray workspace manifest. (#1454) - Indexing now succeeds when Node.js's SQLite lacks FTS5, with search falling back to name and fuzzy matching; thanks @aniruddhaadak80. (#1532) diff --git a/__tests__/cli-unlock.test.ts b/__tests__/cli-unlock.test.ts index 9db3b7a02..56799f385 100644 --- a/__tests__/cli-unlock.test.ts +++ b/__tests__/cli-unlock.test.ts @@ -99,4 +99,35 @@ describe('codegraph unlock — daemon artifact recovery (#1553)', () => { await new Promise((resolve) => server.close(() => resolve())); } }); + + it('preserves a live legacy lock whose daemon identity cannot be probed', () => { + const pidPath = getDaemonPidPath(tempDir); + fs.writeFileSync(pidPath, `${process.pid}\n`); + + const output = runCodegraph(['unlock', tempDir], tempDir); + + expect(output).toContain('No stale lock files found'); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(`${process.pid}\n`); + expect(() => process.kill(process.pid, 0)).not.toThrow(); + }); + + it('removes a legacy lock whose PID is dead', () => { + const pidPath = getDaemonPidPath(tempDir); + fs.writeFileSync(pidPath, '999999\n'); + + const output = runCodegraph(['unlock', tempDir], tempDir); + + expect(output).toContain('Removed stale lock artifacts'); + expect(fs.existsSync(pidPath)).toBe(false); + }); + + it('removes a malformed daemon lock', () => { + const pidPath = getDaemonPidPath(tempDir); + fs.writeFileSync(pidPath, 'not-a-lock\n'); + + const output = runCodegraph(['unlock', tempDir], tempDir); + + expect(output).toContain('Removed stale lock artifacts'); + expect(fs.existsSync(pidPath)).toBe(false); + }); }); diff --git a/__tests__/daemon-manager.test.ts b/__tests__/daemon-manager.test.ts index 28fec9558..af000d662 100644 --- a/__tests__/daemon-manager.test.ts +++ b/__tests__/daemon-manager.test.ts @@ -98,6 +98,35 @@ describe('runDaemonPicker', () => { expect(h.getDone()).toBe('Done.'); }); + it('does not report an unverified daemon as stopped', async () => { + const h = harness([rec('/p/a', 42, 1)], ['/p/a', CANCEL]); + h.deps.stop = async (root): Promise => ({ + root, + pid: 42, + outcome: 'unverified', + }); + + await runDaemonPicker(h.deps); + + expect(h.notes).toEqual([ + 'Could not verify daemon (pid 42); left it running with its artifacts intact — /p/a', + ]); + expect(h.getDone()).toContain('Cancelled'); + }); + + it.each([ + ['not-running', 42, 'Daemon was no longer running; removed stale artifacts — /p/a'], + ['no-daemon', null, 'No daemon was found — /p/a'], + ] as const)('reports the %s race outcome accurately', async (outcome, pid, message) => { + const h = harness([rec('/p/a', 42, 1)], ['/p/a', CANCEL]); + h.deps.stop = async (root): Promise => ({ root, pid, outcome }); + + await runDaemonPicker(h.deps); + + expect(h.notes).toEqual([message]); + expect(h.getDone()).toContain('Cancelled'); + }); + it('Cancel (and Esc/Ctrl-C) stop nothing', async () => { const h1 = harness([rec('/p/a', 1, 1)], [CANCEL]); await runDaemonPicker(h1.deps); diff --git a/__tests__/daemon-registry.test.ts b/__tests__/daemon-registry.test.ts index aa13ec100..881d215fd 100644 --- a/__tests__/daemon-registry.test.ts +++ b/__tests__/daemon-registry.test.ts @@ -11,10 +11,12 @@ import { deregisterDaemon, listDaemons, listVerifiedDaemons, + clearStaleDaemonArtifacts, stopDaemonAt, type DaemonRecord, } from '../src/mcp/daemon-registry'; import { encodeLockInfo, getDaemonPidPath } from '../src/mcp/daemon-paths'; +import { releaseWriterLock, tryAcquireWriterLock } from '../src/mcp/writer-lock'; /** A pid that's guaranteed dead: spawn a trivial process, let it exit, reap it. */ async function deadPid(): Promise { @@ -155,4 +157,87 @@ describe('daemon-registry', () => { expect(isProcessAlive(process.pid)).toBe(true); expect(fs.existsSync(pidPath)).toBe(false); }); + + it('preserves a live legacy lock when stop cannot verify daemon identity', async () => { + const root = fs.mkdtempSync(path.join(tmpHome, 'legacy-stop-')); + const pidPath = getDaemonPidPath(root); + fs.mkdirSync(path.dirname(pidPath), { recursive: true }); + fs.writeFileSync(pidPath, `${process.pid}\n`); + + const result = await stopDaemonAt(root); + + expect(result).toMatchObject({ pid: process.pid, outcome: 'unverified' }); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(`${process.pid}\n`); + expect(isProcessAlive(process.pid)).toBe(true); + }); + + it('preserves a replacement lock written while stale identity is probed', async () => { + const root = fs.mkdtempSync(path.join(tmpHome, 'probe-race-')); + const pidPath = getDaemonPidPath(root); + const socketPath = process.platform === 'win32' + ? `\\\\.\\pipe\\cg-race-old-${process.pid}-${Date.now()}` + : path.join(tmpHome, 'probe-race-old.sock'); + const replacementSocketPath = process.platform === 'win32' + ? `\\\\.\\pipe\\cg-race-new-${process.pid}-${Date.now()}` + : path.join(tmpHome, 'probe-race-new.sock'); + let acceptConnection!: () => void; + const connected = new Promise((resolve) => { acceptConnection = resolve; }); + let acceptedSocket: net.Socket | null = null; + const server = net.createServer((socket) => { + acceptedSocket = socket; + acceptConnection(); + }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(socketPath, resolve); + }); + const original = encodeLockInfo({ + pid: process.pid, + version: '1.5.0', + socketPath, + startedAt: 1, + }); + const replacement = encodeLockInfo({ + pid: process.pid, + version: '1.5.0', + socketPath: replacementSocketPath, + startedAt: 2, + }); + fs.mkdirSync(path.dirname(pidPath), { recursive: true }); + fs.writeFileSync(pidPath, original); + + try { + const clearing = clearStaleDaemonArtifacts(root); + await connected; + fs.writeFileSync(pidPath, replacement); + acceptedSocket!.end('{"protocol":0}\n'); + + expect(await clearing).toBe(false); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(replacement); + } finally { + acceptedSocket?.destroy(); + await new Promise((resolve) => server.close(() => resolve())); + } + }); + + it('does not clean daemon artifacts while another writer owns the project', async () => { + const root = fs.mkdtempSync(path.join(tmpHome, 'writer-claim-')); + const pidPath = getDaemonPidPath(root); + const lock = encodeLockInfo({ + pid: process.pid, + version: '1.5.0', + socketPath: path.join(root, '.codegraph', 'not-listening.sock'), + startedAt: 1, + }); + fs.mkdirSync(path.dirname(pidPath), { recursive: true }); + fs.writeFileSync(pidPath, lock); + expect(tryAcquireWriterLock(root, 'daemon').kind).toBe('acquired'); + + try { + expect(await clearStaleDaemonArtifacts(root)).toBe(false); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + } finally { + releaseWriterLock(root); + } + }); }); diff --git a/__tests__/daemon-socket-fallback.test.ts b/__tests__/daemon-socket-fallback.test.ts index 3862b4c92..02af370c5 100644 --- a/__tests__/daemon-socket-fallback.test.ts +++ b/__tests__/daemon-socket-fallback.test.ts @@ -43,6 +43,7 @@ import { decodeLockInfo } from '../src/mcp/daemon-paths'; import { acquireLockViaExclusiveOpen, bindFirstUsableSocket, + clearStaleDaemonLock, tryAcquireDaemonLock, } from '../src/mcp/daemon'; @@ -244,3 +245,44 @@ describe('lock acquisition without hard links (#997)', () => { expect(decodeLockInfo(fs.readFileSync(pidPath, 'utf8'))).toEqual(winner); }); }); + +describe('legacy daemon lock decoding', () => { + it('decodes a plain decimal PID as a legacy lock record', () => { + expect(decodeLockInfo('4242\n')).toEqual({ + pid: 4242, + version: 'unknown', + socketPath: '', + startedAt: 0, + }); + }); + + it.each(['1e3', '0x3e8', '1000.0'])('rejects non-decimal PID syntax %s', (raw) => { + expect(decodeLockInfo(raw)).toBeNull(); + }); +}); + +describe('stale daemon lock snapshot validation', () => { + it('does not delete a same-PID replacement whose identity was never probed', () => { + const pidPath = path.join(os.tmpdir(), `cg-snapshot-${process.pid}-${Date.now()}.pid`); + tmpFiles.push(pidPath); + const original = JSON.stringify({ + pid: process.pid, + version: '1.5.0', + socketPath: '/old.sock', + startedAt: 1, + }); + const replacement = JSON.stringify({ + pid: process.pid, + version: '1.5.0', + socketPath: '/new.sock', + startedAt: 2, + }); + fs.writeFileSync(pidPath, replacement); + + expect(clearStaleDaemonLock(pidPath, process.pid, { + allowLivePid: true, + expectedLockContents: original, + })).toBe(false); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(replacement); + }); +}); diff --git a/__tests__/mcp-daemon.test.ts b/__tests__/mcp-daemon.test.ts index c73ac564c..a39feb1b2 100644 --- a/__tests__/mcp-daemon.test.ts +++ b/__tests__/mcp-daemon.test.ts @@ -337,7 +337,7 @@ describe('Shared MCP daemon (issue #411)', () => { expect(isAlive(livePid!)).toBe(true); }, 40000); - it('takes over after SIGKILL even when the stale PID has been reused (#1553)', async () => { + it('preserves paired daemon/writer locks when their live PID may have been reused', async () => { const env = { CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '30000' }; const first = spawnServer(tempDir, env); servers.push(first); @@ -351,30 +351,116 @@ describe('Shared MCP daemon (issue #411)', () => { // Model OS PID reuse without risking another process: the stale lock now // names this live vitest worker, but no daemon answers the leftover socket. - fs.writeFileSync( - path.join(realRoot, '.codegraph', 'daemon.pid'), - JSON.stringify({ - pid: process.pid, - version: CodeGraphPackageVersion, - socketPath: getDaemonSocketPath(realRoot), - startedAt: Date.now() - 60_000, - }), - ); + const daemonPath = path.join(realRoot, '.codegraph', 'daemon.pid'); + const writerPath = path.join(realRoot, '.codegraph', 'writer.pid'); + const staleDaemonLock = JSON.stringify({ + pid: process.pid, + version: CodeGraphPackageVersion, + socketPath: getDaemonSocketPath(realRoot), + startedAt: Date.now() - 60_000, + }); + const staleWriterLock = JSON.stringify({ + pid: process.pid, + mode: 'daemon', + startedAt: Date.now() - 60_000, + }) + '\n'; + fs.writeFileSync(daemonPath, staleDaemonLock); + fs.writeFileSync(writerPath, staleWriterLock); const second = spawnServer(tempDir, env); servers.push(second); sendInitialize(second.child, `file://${tempDir}`, 2); const response = await waitFor(() => findResponse(second.stdout, 2), 12000); expect(response.result.serverInfo.name).toBe('codegraph'); - await waitFor(() => countListeningLines(realRoot) >= 2, 10000); + await waitFor( + () => second.stderr.some((line) => + line.includes('Attached to shared daemon') || line.includes('Shared daemon unavailable') + ), + 12000, + 25, + 'the proxy to attach or fall back', + ); - const replacementPid = readLockPid(realRoot)!; - expect(replacementPid).not.toBe(killedPid); - expect(replacementPid).not.toBe(process.pid); - expect(isAlive(replacementPid)).toBe(true); + expect(second.stderr.some((line) => line.includes('Attached to shared daemon'))).toBe(false); + expect(countListeningLines(realRoot)).toBe(1); + expect(fs.readFileSync(daemonPath, 'utf8')).toBe(staleDaemonLock); + expect(fs.readFileSync(writerPath, 'utf8')).toBe(staleWriterLock); expect(isAlive(process.pid)).toBe(true); + + sendMessage(second.child, { + jsonrpc: '2.0', + id: 3, + method: 'tools/call', + params: { name: 'codegraph_status', arguments: {} }, + }); + const toolResponse = await waitFor(() => findResponse(second.stdout, 3), 5000); + expect(toolResponse).toMatchObject({ + error: { message: expect.stringContaining('writer lock held') }, + }); }, 50000); + it('does not replace a live legacy lock with a second daemon', async () => { + const pidPath = path.join(realRoot, '.codegraph', 'daemon.pid'); + fs.writeFileSync(pidPath, `${process.pid}\n`); + + const server = spawnServer(tempDir, { CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '15000' }); + servers.push(server); + sendInitialize(server.child, `file://${tempDir}`, 1); + const response = await waitFor(() => findResponse(server.stdout, 1), 12000); + expect(response.result.serverInfo.name).toBe('codegraph'); + + await waitFor( + () => server.stderr.some((line) => + line.includes('Attached to shared daemon') || line.includes('Shared daemon unavailable') + ), + 12000, + 25, + 'the proxy to attach or fall back', + ); + + expect(server.stderr.some((line) => line.includes('Attached to shared daemon'))).toBe(false); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(`${process.pid}\n`); + expect(countListeningLines(realRoot)).toBe(0); + expect(isAlive(process.pid)).toBe(true); + + sendMessage(server.child, { + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'codegraph_status', arguments: {} }, + }); + const toolResponse = await waitFor(() => findResponse(server.stdout, 2), 5000); + expect(toolResponse).toMatchObject({ + error: { message: expect.stringContaining('live legacy daemon') }, + }); + }, 30000); + + it('does not start a fallback writer when the daemon lock is unreadable', async () => { + const pidPath = path.join(realRoot, '.codegraph', 'daemon.pid'); + fs.mkdirSync(pidPath); + + const server = spawnServer(tempDir); + servers.push(server); + sendInitialize(server.child, `file://${tempDir}`, 1); + await waitFor( + () => server.stderr.some((line) => line.includes('Shared daemon unavailable')), + 12000, + 25, + 'the proxy to fall back', + ); + + sendMessage(server.child, { + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'codegraph_status', arguments: {} }, + }); + const toolResponse = await waitFor(() => findResponse(server.stdout, 2), 5000); + expect(toolResponse).toMatchObject({ + error: { message: expect.stringContaining('daemon lock could not be read') }, + }); + }, 30000); + it('proxy falls back to direct mode on a daemon version mismatch', async () => { const net = await import('net'); const sockPath = getDaemonSocketPath(realRoot); @@ -385,7 +471,12 @@ describe('Shared MCP daemon (issue #411)', () => { JSON.stringify({ pid: process.pid, version: '0.0.0-mismatch', socketPath: sockPath, startedAt: Date.now() }), ); const miniServer = net.createServer((sock) => { - sock.write(JSON.stringify({ codegraph: '0.0.0-mismatch', pid: 1, socketPath: sockPath, protocol: 1 }) + '\n'); + sock.write(JSON.stringify({ + codegraph: '0.0.0-mismatch', + pid: process.pid, + socketPath: sockPath, + protocol: 1, + }) + '\n'); }); await new Promise((resolve) => miniServer.listen(sockPath, () => resolve())); @@ -402,6 +493,18 @@ describe('Shared MCP daemon (issue #411)', () => { () => server.stderr.some((l) => l.includes('serving this session in-process')), 6000, ); + + sendMessage(server.child, { + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'codegraph_status', arguments: {} }, + }); + const toolResponse = await waitFor(() => findResponse(server.stdout, 2), 5000); + expect(toolResponse).toMatchObject({ + error: { message: expect.stringContaining('live daemon') }, + }); + expect(fs.existsSync(path.join(realRoot, '.codegraph', 'writer.pid'))).toBe(false); } finally { await new Promise((resolve) => miniServer.close(() => resolve())); } diff --git a/__tests__/writer-lock.test.ts b/__tests__/writer-lock.test.ts index c3ba44723..3dd1e1b80 100644 --- a/__tests__/writer-lock.test.ts +++ b/__tests__/writer-lock.test.ts @@ -4,9 +4,11 @@ */ import { afterEach, describe, expect, it } from 'vitest'; +import { spawn, type ChildProcess } from 'child_process'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; +import { MCPEngine } from '../src/mcp/engine'; import { decodeWriterLockInfo, getWriterPidPath, @@ -17,8 +19,11 @@ import { describe('writer lock (#1740)', () => { let dir: string; + let holder: ChildProcess | null = null; afterEach(() => { + try { holder?.kill('SIGKILL'); } catch { /* already gone */ } + holder = null; if (dir) { releaseWriterLock(dir); try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } @@ -84,4 +89,29 @@ describe('writer lock (#1740)', () => { expect(r.kind).toBe('acquired'); releaseWriterLock(root); }); + + it('lets a fallback engine atomically claim and release writer ownership', () => { + const root = makeProject(); + const engine = new MCPEngine({ writerLockRoot: root }); + + expect(decodeWriterLockInfo(fs.readFileSync(getWriterPidPath(root), 'utf8'))).toMatchObject({ + pid: process.pid, + mode: 'fallback', + }); + + engine.stop(); + expect(fs.existsSync(getWriterPidPath(root))).toBe(false); + }); + + it('rejects a fallback engine before opening when another process owns writer.pid', () => { + const root = makeProject(); + holder = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' }); + if (!holder.pid) throw new Error('Failed to spawn writer-lock holder'); + fs.writeFileSync( + getWriterPidPath(root), + JSON.stringify({ pid: holder.pid, mode: 'daemon', startedAt: Date.now() }) + '\n', + ); + + expect(() => new MCPEngine({ writerLockRoot: root })).toThrow(/writer lock held/i); + }); }); diff --git a/src/mcp/daemon-manager.ts b/src/mcp/daemon-manager.ts index 0c1a991a1..634e633b5 100644 --- a/src/mcp/daemon-manager.ts +++ b/src/mcp/daemon-manager.ts @@ -109,6 +109,20 @@ export async function runDaemonPicker(deps: PickerDeps): Promise { } const result = await deps.stop(String(choice)); + if (result.outcome === 'unverified') { + deps.note( + `Could not verify daemon (pid ${result.pid}); left it running with its artifacts intact — ${choice}` + ); + continue; + } + if (result.outcome === 'not-running') { + deps.note(`Daemon was no longer running; removed stale artifacts — ${choice}`); + continue; + } + if (result.outcome === 'no-daemon') { + deps.note(`No daemon was found — ${choice}`); + continue; + } const forced = result.outcome === 'kill' ? ', forced' : ''; deps.note(`Stopped daemon (pid ${result.pid}${forced}) — ${choice}`); // Loop: the next iteration re-lists; if more remain it re-prompts, otherwise diff --git a/src/mcp/daemon-paths.ts b/src/mcp/daemon-paths.ts index c860ee76c..34751814f 100644 --- a/src/mcp/daemon-paths.ts +++ b/src/mcp/daemon-paths.ts @@ -102,13 +102,23 @@ export interface DaemonLockInfo { startedAt: number; } +/** Whether a lock record contains enough identity data for a socket hello. */ +export function canProbeDaemonIdentity(info: DaemonLockInfo): boolean { + return ( + Number.isInteger(info.pid) && + info.pid > 0 && + typeof info.socketPath === 'string' && + info.socketPath.length > 0 + ); +} + /** * Verify that the process named by a lockfile is the CodeGraph daemon serving * its socket. A bare PID liveness probe is insufficient because OSes reuse PIDs * after an OOM/SIGKILL (#1553). */ export function probeDaemonIdentity(info: DaemonLockInfo, timeoutMs = 1_000): Promise { - if (!Number.isInteger(info.pid) || info.pid <= 0 || !info.socketPath) return Promise.resolve(false); + if (!canProbeDaemonIdentity(info)) return Promise.resolve(false); return new Promise((resolve) => { let socket: net.Socket; let buffer = ''; @@ -178,12 +188,12 @@ export function decodeLockInfo(raw: string): DaemonLockInfo | null { ) { return parsed as DaemonLockInfo; } - return null; } catch { // Fall through to legacy plain-pid handling. } + if (!/^[1-9]\d*$/.test(trimmed)) return null; const pid = Number(trimmed); - if (Number.isFinite(pid) && pid > 0) { + if (Number.isSafeInteger(pid)) { return { pid, version: 'unknown', socketPath: '', startedAt: 0 }; } return null; diff --git a/src/mcp/daemon-registry.ts b/src/mcp/daemon-registry.ts index f731563cf..0d3b7cf4f 100644 --- a/src/mcp/daemon-registry.ts +++ b/src/mcp/daemon-registry.ts @@ -26,9 +26,11 @@ import { getDaemonPidPath, getDaemonSocketCandidates, decodeLockInfo, + canProbeDaemonIdentity, probeDaemonIdentity, type DaemonLockInfo, } from './daemon-paths'; +import { readWriterLock, releaseWriterLock, tryAcquireWriterLock } from './writer-lock'; export interface DaemonRecord { /** Realpath'd project root the daemon serves. */ @@ -140,18 +142,45 @@ export async function listVerifiedDaemons(opts: { prune?: boolean } = {}): Promi return verified; } -/** Remove a stopped daemon's leftover lockfile + socket + registry record. */ -function cleanupDaemonArtifacts(root: string): void { - try { fs.unlinkSync(getDaemonPidPath(root)); } catch { /* gone */ } - // POSIX sockets are real files; Windows named pipes vanish with the process. - // Sweep every candidate — a daemon that relocated past an unusable in-project - // FS (ExFAT/FAT; #997) left its socket at the tmpdir fallback, not candidate 0. - if (process.platform !== 'win32') { - for (const candidate of getDaemonSocketCandidates(root)) { - try { fs.unlinkSync(candidate); } catch { /* gone */ } +/** Remove stale artifacts while holding the project writer slot exclusively. */ +function cleanupDaemonArtifacts( + root: string, + expectedLockContents: string | null, +): boolean { + const pidPath = getDaemonPidPath(root); + // A daemon owns writer.pid before binding or relocating its socket. Claiming + // the writer slot therefore freezes every legitimate daemon artifact writer + // while we compare the inspected lock snapshot and clean it up. + if (readWriterLock(root)?.pid === process.pid) return false; + const claim = tryAcquireWriterLock(root, 'cleanup'); + if (claim.kind === 'taken') return false; + + try { + if (expectedLockContents === null) { + if (fs.existsSync(pidPath)) return false; + } else { + try { + if (fs.readFileSync(pidPath, 'utf8') !== expectedLockContents) return false; + } catch { + return false; + } + } + // POSIX sockets are real files; Windows named pipes vanish with the process. + // Sweep every candidate before releasing daemon.pid, so no successor can + // acquire the lock and bind a socket that this cleanup then removes. + if (process.platform !== 'win32') { + for (const candidate of getDaemonSocketCandidates(root)) { + try { fs.unlinkSync(candidate); } catch { /* gone */ } + } + } + deregisterDaemon(root); + try { fs.unlinkSync(pidPath); } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return false; } + return true; + } finally { + releaseWriterLock(root); } - deregisterDaemon(root); } /** Remove daemon artifacts only when no matching daemon answers the socket hello. */ @@ -162,10 +191,17 @@ export async function clearStaleDaemonArtifacts(root: string): Promise ); if (!hadArtifacts) return false; let info: DaemonLockInfo | null = null; - try { info = decodeLockInfo(fs.readFileSync(pidPath, 'utf8')); } catch { /* missing/corrupt */ } - if (info && isProcessAlive(info.pid) && await probeDaemonIdentity(info)) return false; - cleanupDaemonArtifacts(root); - return true; + let lockContents: string | null = null; + try { + lockContents = fs.readFileSync(pidPath, 'utf8'); + info = decodeLockInfo(lockContents); + } catch { /* missing/corrupt */ } + if (info && isProcessAlive(info.pid)) { + // A live legacy holder has no socket path to probe. That is inconclusive, + // not proof of PID reuse, so preserve its lock rather than risk two writers. + if (!canProbeDaemonIdentity(info) || await probeDaemonIdentity(info)) return false; + } + return cleanupDaemonArtifacts(root, lockContents); } const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); @@ -182,8 +218,8 @@ async function waitForDeath(pid: number, timeoutMs: number): Promise { export interface StopResult { root: string; pid: number | null; - /** 'term' graceful, 'kill' force, 'not-running' stale lock, 'no-daemon' none found. */ - outcome: 'term' | 'kill' | 'not-running' | 'no-daemon'; + /** 'term' graceful, 'kill' force, 'not-running' stale, 'no-daemon' absent, 'unverified' preserved. */ + outcome: 'term' | 'kill' | 'not-running' | 'no-daemon' | 'unverified'; } /** @@ -195,8 +231,10 @@ export interface StopResult { export async function stopDaemonAt(root: string): Promise { let pid: number | null = null; let identity: DaemonLockInfo | null = null; + let lockContents: string | null = null; try { - identity = decodeLockInfo(fs.readFileSync(getDaemonPidPath(root), 'utf8')); + lockContents = fs.readFileSync(getDaemonPidPath(root), 'utf8'); + identity = decodeLockInfo(lockContents); pid = identity?.pid ?? null; } catch { /* no lockfile */ @@ -210,18 +248,21 @@ export async function stopDaemonAt(root: string): Promise { } if (pid == null) { - cleanupDaemonArtifacts(root); + cleanupDaemonArtifacts(root, lockContents); return { root, pid: null, outcome: 'no-daemon' }; } if (!isProcessAlive(pid)) { - cleanupDaemonArtifacts(root); - return { root, pid, outcome: 'not-running' }; + const removed = cleanupDaemonArtifacts(root, lockContents); + return { root, pid, outcome: removed ? 'not-running' : 'unverified' }; } // Never signal a process merely because it reused a stale daemon PID. The // daemon's immediate hello is the process-identity proof (#1553). - if (!identity || !await probeDaemonIdentity(identity)) { - cleanupDaemonArtifacts(root); - return { root, pid, outcome: 'not-running' }; + if (!identity || !canProbeDaemonIdentity(identity)) { + return { root, pid, outcome: 'unverified' }; + } + if (!await probeDaemonIdentity(identity)) { + const removed = cleanupDaemonArtifacts(root, lockContents); + return { root, pid, outcome: removed ? 'not-running' : 'unverified' }; } // POSIX: SIGTERM runs the daemon's graceful shutdown. Windows: TerminateProcess @@ -233,7 +274,7 @@ export async function stopDaemonAt(root: string): Promise { await waitForDeath(pid, 2000); outcome = 'kill'; } - cleanupDaemonArtifacts(root); + cleanupDaemonArtifacts(root, lockContents); return { root, pid, outcome }; } diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index a5735d168..d80dad5be 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -55,7 +55,11 @@ import { getDaemonSocketPath, } from './daemon-paths'; import { CodeGraphPackageVersion } from './version'; -import { releaseWriterLock, tryAcquireWriterLock, writerLockHeldMessage } from './writer-lock'; +import { + releaseWriterLock, + tryAcquireWriterLock, + writerLockHeldMessage, +} from './writer-lock'; import { registerDaemon, deregisterDaemon } from './daemon-registry'; /** Default idle linger after the last client disconnects. */ @@ -161,9 +165,8 @@ export interface DaemonStartResult { * * Race-safe: callers must first call `tryAcquireDaemonLock(projectRoot)` and * only construct a Daemon if they got the lock (`kind: 'acquired'`). The atomic - * `O_EXCL` create inside the acquire helper — which now also writes the full - * record before returning — is the only synchronization between competing - * daemons. + * create/link inside the acquire helper elects one candidate. The project + * writer lock then fences bind/ownership refresh against stale-artifact cleanup. */ export class Daemon { private server: net.Server | null = null; @@ -199,10 +202,9 @@ export class Daemon { } /** - * Bind the socket, kick off engine init, and register signal handlers. The - * lockfile body was already written atomically by `tryAcquireDaemonLock`, so - * there is nothing to write here. The promise resolves once the server is - * listening — the daemon then sticks around until idle/shutdown. + * Bind the socket, refresh the ownership record, kick off engine init, and + * register signal handlers. The promise resolves once the server is listening + * — the daemon then sticks around until idle/shutdown. */ async start(): Promise { // #1740: claim the project writer lock before opening/watching so a @@ -215,10 +217,16 @@ export class Daemon { throw new Error(msg); } - // Engine init is deliberately backgrounded — see #172. The first session - // to land waits on `ensureInitialized` either way, and unloaded sessions - // (cross-project tool calls only) shouldn't pay any open cost. - void this.engine.ensureInitialized(this.projectRoot); + let initialLockContents: string; + try { + initialLockContents = fs.readFileSync(this.pidPath, 'utf8'); + if (decodeLockInfo(initialLockContents)?.pid !== process.pid) { + throw new Error('daemon lock belongs to another process'); + } + } catch { + releaseWriterLock(this.projectRoot); + throw new Error('Lost daemon lock ownership before startup.'); + } // Walk the ordered socket candidates and bind the first that works. The // in-project path comes first; the deterministic tmpdir path is the fallback @@ -282,19 +290,27 @@ export class Daemon { startedAt: Date.now(), }; - // `tryAcquireDaemonLock` wrote the pidfile with the PREFERRED path (candidate - // 0) before we knew which one would bind. If we relocated, rewrite it so the - // per-project record is honest. Atomic temp+rename; safe because we hold the - // lock and we're alive — `clearStaleDaemonLock` pid-verifies, so no racing - // candidate clears or clobbers a live daemon's lock. - if (this.socketPath !== candidates[0]) { - try { - const tmpPid = `${this.pidPath}.${process.pid}.relocate`; - fs.writeFileSync(tmpPid, encodeLockInfo(lock), { mode: 0o600 }); - fs.renameSync(tmpPid, this.pidPath); - } catch { /* best-effort; the registry record below carries the real path */ } + // Refresh the lock on every successful bind, not only relocation. The + // writer lock prevents stale-artifact cleanup from racing this ownership + // check, and the exact snapshot prevents overwriting a replacement record. + try { + if (fs.readFileSync(this.pidPath, 'utf8') !== initialLockContents) { + throw new Error('Lost daemon lock ownership after binding.'); + } + const tmpPid = `${this.pidPath}.${process.pid}.bound`; + fs.writeFileSync(tmpPid, encodeLockInfo(lock), { mode: 0o600 }); + fs.renameSync(tmpPid, this.pidPath); + } catch (err) { + try { bound.server.close(); } catch { /* best-effort */ } + this.cleanupLockfile(); + throw err; } + // Engine init is deliberately backgrounded — see #172. It starts only + // after bind and ownership refresh, so a delayed daemon that lost election + // can never open a second watcher or writer. + void this.engine.ensureInitialized(this.projectRoot); + // Drop a discovery record so `codegraph list` / `stop --all` can find us. // Best-effort; a missing record only means list's liveness prune covers it. registerDaemon({ root: this.projectRoot, ...lock }); @@ -531,7 +547,13 @@ export class Daemon { */ export type AcquireResult = | { kind: 'acquired'; pidPath: string; info: DaemonLockInfo } - | { kind: 'taken'; existing: DaemonLockInfo | null; pidPath: string }; + | { + kind: 'taken'; + existing: DaemonLockInfo | null; + /** Exact record read after losing acquisition; null when it was unreadable. */ + lockContents: string | null; + pidPath: string; + }; /** * Atomically create the daemon pidfile with its full record already in place. @@ -609,10 +631,12 @@ export function tryAcquireDaemonLock(projectRoot: string): AcquireResult { // record — `existing` is null only for a genuinely corrupt leftover, never a // mid-write race. let existing: DaemonLockInfo | null = null; + let lockContents: string | null = null; try { - existing = decodeLockInfo(fs.readFileSync(pidPath, 'utf8')); + lockContents = fs.readFileSync(pidPath, 'utf8'); + existing = decodeLockInfo(lockContents); } catch { /* unreadable lockfile — treat as malformed */ } - return { kind: 'taken', existing, pidPath }; + return { kind: 'taken', existing, lockContents, pidPath }; } /** @@ -655,10 +679,14 @@ export function acquireLockViaExclusiveOpen(pidPath: string, info: DaemonLockInf export function clearStaleDaemonLock( pidPath: string, expectedDeadPid?: number, - opts: { allowLivePid?: boolean } = {} + opts: { allowLivePid?: boolean; expectedLockContents?: string } = {} ): boolean { try { const raw = fs.readFileSync(pidPath, 'utf8'); + // The identity record changed after the caller inspected it. Even the same + // PID may now advertise a newly-bound socket, so this snapshot was never + // disproved and must not be deleted. + if (opts.expectedLockContents !== undefined && raw !== opts.expectedLockContents) return false; const info = decodeLockInfo(raw); if (info) { // A different pid took over since we read it — not ours to clear. diff --git a/src/mcp/engine.ts b/src/mcp/engine.ts index 734fafa4f..b88f1b121 100644 --- a/src/mcp/engine.ts +++ b/src/mcp/engine.ts @@ -47,6 +47,12 @@ export interface MCPEngineOptions { * disables it even in daemon mode. */ queryPool?: boolean; + /** + * Project root whose writer slot must be claimed synchronously before this + * engine can open the graph. Used by proxy fallback to fence catch-up sync, + * not just the later file watcher. + */ + writerLockRoot?: string; } /** @@ -70,7 +76,7 @@ export class MCPEngine { private watcherStarted = false; /** Set when this engine holds writer.pid (#1740). */ private writerLockRoot: string | null = null; - private opts: Required; + private opts: Required>; private closed = false; // Off-loop read-tool pool (daemon mode only). Created lazily once the default // project is open — workers each hold their own WAL read connection. @@ -79,6 +85,13 @@ export class MCPEngine { constructor(opts: MCPEngineOptions = {}) { this.opts = { watch: opts.watch ?? true, queryPool: opts.queryPool ?? false }; this.toolHandler = new ToolHandler(null); + if (opts.writerLockRoot) { + const writer = tryAcquireWriterLock(opts.writerLockRoot, 'fallback'); + if (writer.kind === 'taken') { + throw new Error(writerLockHeldMessage(writer.existing, writer.pidPath)); + } + this.writerLockRoot = opts.writerLockRoot; + } } /** diff --git a/src/mcp/index.ts b/src/mcp/index.ts index e4a14a4fb..c12ab8105 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -47,9 +47,22 @@ import { isProcessAlive, tryAcquireDaemonLock, } from './daemon'; +import { clearStaleDaemonArtifacts } from './daemon-registry'; import { connectWithHello, runLocalHandshakeProxy } from './proxy'; -import { releaseWriterLock, tryAcquireWriterLock, writerLockHeldMessage } from './writer-lock'; -import { getDaemonSocketCandidates, probeDaemonIdentity } from './daemon-paths'; +import { + getWriterPidPath, + readWriterLock, + releaseWriterLock, + tryAcquireWriterLock, + writerLockHeldMessage, +} from './writer-lock'; +import { + canProbeDaemonIdentity, + decodeLockInfo, + getDaemonPidPath, + getDaemonSocketCandidates, + probeDaemonIdentity, +} from './daemon-paths'; import { getTelemetry } from '../telemetry'; import { checkForUpdateInBackground } from '../upgrade/update-check'; import { EARLY_PPID } from './early-ppid'; @@ -75,6 +88,42 @@ const DAEMON_INTERNAL_ENV = 'CODEGRAPH_DAEMON_INTERNAL'; const TAKEOVER_MAX_RETRIES = 5; const TAKEOVER_RETRY_DELAY_MS = 100; +/** + * Create an in-process fallback only when it cannot conflict with a live + * legacy daemon. Plain-PID locks cannot prove daemon identity, but they still + * prove that a process owns the legacy writer slot. + */ +function makeFallbackEngine(root: string): MCPEngine { + let existing: ReturnType = null; + try { + existing = decodeLockInfo(fs.readFileSync(getDaemonPidPath(root), 'utf8')); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'ENOENT') { + throw new Error(`The daemon lock could not be read (${code ?? 'unknown error'}); refusing an in-process fallback.`); + } + } + if ( + existing && + isProcessAlive(existing.pid) && + !canProbeDaemonIdentity(existing) + ) { + throw new Error( + `Cannot start an in-process fallback while live legacy daemon pid ${existing.pid} holds the project lock.` + ); + } + const writer = readWriterLock(root); + if (writer && writer.pid > 0 && isProcessAlive(writer.pid)) { + throw new Error(writerLockHeldMessage(writer, getWriterPidPath(root))); + } + if (existing && isProcessAlive(existing.pid)) { + throw new Error( + `Cannot start an in-process fallback while live daemon pid ${existing.pid} holds the project lock.` + ); + } + return new MCPEngine({ writerLockRoot: root }); +} + /** * How long a launcher waits for a freshly-spawned daemon to bind its socket * before giving up and running in-process. The daemon binds the socket *before* @@ -449,23 +498,43 @@ export class MCPServer { // Taken. If the holder is alive, another daemon already serves (or is // binding) — we're redundant; exit cleanly so the launcher proxies to it. const existing = lock.existing; + let disprovedLiveIdentity = false; if (existing && existing.pid > 0 && isProcessAlive(existing.pid)) { // Give a newly-elected daemon time to bind, then require its socket hello // to match the lock PID/version. PID existence alone accepts an unrelated // process after OS PID reuse and permanently wedges startup (#1553). const age = Date.now() - existing.startedAt; - const stillStarting = existing.startedAt > 0 && age >= 0 && age < 10_000; - if (stillStarting || await probeDaemonIdentity(existing)) { + const startupGraceMs = 10_000; + const stillStarting = existing.startedAt > 0 && age >= 0 && age < startupGraceMs; + // Legacy plain-PID locks have no socket identity to test. Preserve those + // live holders: an inconclusive probe is not permission to create a + // second writer. + if ( + !canProbeDaemonIdentity(existing) || + stillStarting || + await probeDaemonIdentity(existing) + ) { process.stderr.write( `[CodeGraph daemon] Another daemon (pid ${existing.pid}) already holds the lock; exiting.\n` ); process.exit(0); } + disprovedLiveIdentity = true; } - // Holder is dead (or the record is unreadable) — clear it (pid-verified, - // so we never delete a live daemon's lock) and retry the acquire. - clearStaleDaemonLock(lock.pidPath, existing?.pid, { allowLivePid: true }); + // The holder is dead, the record is unreadable, or a completed socket + // hello disproved a live PID's identity. Revalidate the exact record and + // retry the acquire only after cleanup succeeds safely. + if (disprovedLiveIdentity) { + // Re-probe and claim writer.pid before cleanup. A daemon that is merely + // delayed already owns that writer lock, and a paired live-PID record is + // ambiguous under the legacy lock format, so both cases fail closed. + await clearStaleDaemonArtifacts(root); + } else if (lock.lockContents !== null) { + clearStaleDaemonLock(lock.pidPath, existing?.pid, { + expectedLockContents: lock.lockContents, + }); + } await sleep(TAKEOVER_RETRY_DELAY_MS); } @@ -515,7 +584,7 @@ export class MCPServer { } return null; // never bound — the proxy serves this session in-process }; - await runLocalHandshakeProxy({ getDaemonSocket, makeEngine: () => new MCPEngine(), root }); + await runLocalHandshakeProxy({ getDaemonSocket, makeEngine: () => makeFallbackEngine(root), root }); } /** Standard SIGINT/SIGTERM handlers that route to our `stop()` (direct mode). */