From 3691eb50f4065d43bdead7b6e58df1c27c7439d3 Mon Sep 17 00:00:00 2001 From: chenyuxuan <458254969@qq.com> Date: Wed, 27 May 2026 21:16:04 +0800 Subject: [PATCH] $(cat <<'EOF' fix(lock): harden FileLock ownership semantics Store structured lock metadata, stop treating long-held live locks as stale, --- __tests__/security.test.ts | 136 +++++++++++++++++++++++++++++--- src/index.ts | 21 +++-- src/utils.ts | 154 ++++++++++++++++++++++++++++--------- 3 files changed, 261 insertions(+), 50 deletions(-) diff --git a/__tests__/security.test.ts b/__tests__/security.test.ts index 75ac84320..eeeadc9c3 100644 --- a/__tests__/security.test.ts +++ b/__tests__/security.test.ts @@ -8,11 +8,12 @@ * - Atomic writes */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; import { FileLock, validateProjectPath } from '../src/utils'; +import { setLogger, silentLogger, type Logger } from '../src/errors'; import CodeGraph from '../src/index'; import { ToolHandler, tools } from '../src/mcp/tools'; import { scanDirectory, isSourceFile } from '../src/extraction'; @@ -32,13 +33,25 @@ function cleanupTempDir(dir: string): void { describe('FileLock', () => { let tempDir: string; let lockPath: string; + let logger: Logger; + let debugSpy: ReturnType; + let warnSpy: ReturnType; beforeEach(() => { tempDir = createTempDir(); lockPath = path.join(tempDir, 'test.lock'); + debugSpy = vi.fn(); + warnSpy = vi.fn(); + logger = { + debug: debugSpy, + warn: warnSpy, + error: vi.fn(), + }; + setLogger(logger); }); afterEach(() => { + setLogger(silentLogger); cleanupTempDir(tempDir); }); @@ -47,8 +60,16 @@ describe('FileLock', () => { lock.acquire(); expect(fs.existsSync(lockPath)).toBe(true); - const content = fs.readFileSync(lockPath, 'utf-8').trim(); - expect(parseInt(content, 10)).toBe(process.pid); + const content = JSON.parse(fs.readFileSync(lockPath, 'utf-8')) as { + pid: number; + token: string; + createdAt: number; + hostname: string; + }; + expect(content.pid).toBe(process.pid); + expect(content.token).toEqual(expect.any(String)); + expect(content.createdAt).toEqual(expect.any(Number)); + expect(content.hostname).toEqual(expect.any(String)); lock.release(); expect(fs.existsSync(lockPath)).toBe(false); @@ -60,20 +81,33 @@ describe('FileLock', () => { lock1.acquire(); - // Second lock should fail because our PID is alive - expect(() => lock2.acquire()).toThrow(/locked by another process/); + expect(() => lock2.acquire()).toThrow(/locked by another process \(PID/); lock1.release(); }); it('should detect and remove stale locks from dead processes', () => { - // Write a lock file with a PID that doesn't exist - // PID 99999999 is extremely unlikely to be a real process fs.writeFileSync(lockPath, '99999999'); const lock = new FileLock(lockPath); - // Should succeed because the PID is dead expect(() => lock.acquire()).not.toThrow(); + expect(debugSpy).toHaveBeenCalledWith( + 'Removing stale lock from dead process', + expect.objectContaining({ lockPath, pid: 99999999 }) + ); + + lock.release(); + }); + + it('should remove unreadable lock files before acquiring', () => { + fs.writeFileSync(lockPath, '{bad json'); + + const lock = new FileLock(lockPath); + expect(() => lock.acquire()).not.toThrow(); + expect(warnSpy).toHaveBeenCalledWith( + 'Removing unreadable lock file before reacquiring', + expect.objectContaining({ lockPath }) + ); lock.release(); }); @@ -130,9 +164,93 @@ describe('FileLock', () => { const lock = new FileLock(lockPath); lock.acquire(); lock.release(); - // Second release should not throw expect(() => lock.release()).not.toThrow(); }); + + it('should not remove a lock it no longer owns', () => { + const lock = new FileLock(lockPath); + lock.acquire(); + const current = JSON.parse(fs.readFileSync(lockPath, 'utf-8')) as { + pid: number; + token: string; + createdAt: number; + hostname: string; + }; + fs.writeFileSync(lockPath, JSON.stringify({ ...current, token: 'different-token' })); + + lock.release(); + + expect(fs.existsSync(lockPath)).toBe(true); + }); +}); + +describe('CodeGraph lock error propagation', () => { + let testDir: string; + let cg: CodeGraph; + let lockPath: string; + + beforeEach(() => { + testDir = createTempDir(); + fs.writeFileSync(path.join(testDir, 'example.ts'), 'export const value = 1;\n'); + cg = CodeGraph.initSync(testDir); + lockPath = path.join(testDir, '.codegraph', 'codegraph.lock'); + }); + + afterEach(() => { + if (cg) cg.close(); + cleanupTempDir(testDir); + }); + + it('indexAll should return the specific file lock error', async () => { + fs.writeFileSync(lockPath, JSON.stringify({ + pid: process.pid, + token: 'held-by-other', + createdAt: Date.now() - 1234, + hostname: 'test-host', + })); + + const result = await cg.indexAll(); + + expect(result.success).toBe(false); + expect(result.errors[0]?.message ?? '').toContain('PID'); + expect(result.errors[0]?.message ?? '').toContain(lockPath); + expect(result.errors[0]?.message ?? '').toContain('test-host'); + }); + + it('indexFiles should return the specific file lock error', async () => { + fs.writeFileSync(lockPath, JSON.stringify({ + pid: process.pid, + token: 'held-by-other', + createdAt: Date.now() - 567, + hostname: 'test-host', + })); + + const result = await cg.indexFiles([path.join(testDir, 'example.ts')]); + + expect(result.success).toBe(false); + expect(result.errors[0]?.message ?? '').toContain('PID'); + expect(result.errors[0]?.message ?? '').toContain(lockPath); + }); + + it('sync should preserve the zero-result sentinel on lock contention', async () => { + fs.writeFileSync(lockPath, JSON.stringify({ + pid: process.pid, + token: 'held-by-other', + createdAt: Date.now() - 250, + hostname: 'test-host', + })); + + const result = await cg.sync(); + + expect(result).toEqual({ + filesChecked: 0, + filesAdded: 0, + filesModified: 0, + filesRemoved: 0, + nodesUpdated: 0, + durationMs: 0, + }); + }); }); describe('Path Traversal Prevention', () => { diff --git a/src/index.ts b/src/index.ts index 14b0fb0a6..545a49481 100644 --- a/src/index.ts +++ b/src/index.ts @@ -47,6 +47,7 @@ import { GraphTraverser, GraphQueryManager } from './graph'; import { ContextBuilder, createContextBuilder } from './context'; import { Mutex, FileLock } from './utils'; import { FileWatcher, WatchOptions, PendingFile, LockUnavailableError } from './sync'; +import { logDebug } from './errors'; // Re-export types for consumers export * from './types'; @@ -321,8 +322,11 @@ export class CodeGraph { return this.indexMutex.withLock(async () => { try { this.fileLock.acquire(); - } catch { - return { success: false, filesIndexed: 0, filesSkipped: 0, filesErrored: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message: 'Could not acquire file lock - another process may be indexing', severity: 'error' as const }], durationMs: 0 }; + } catch (err) { + const message = err instanceof Error + ? err.message + : 'Could not acquire file lock - another process may be indexing'; + return { success: false, filesIndexed: 0, filesSkipped: 0, filesErrored: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message, severity: 'error' as const }], durationMs: 0 }; } try { const before = this.queries.getNodeAndEdgeCount(); @@ -393,8 +397,11 @@ export class CodeGraph { return this.indexMutex.withLock(async () => { try { this.fileLock.acquire(); - } catch { - return { success: false, filesIndexed: 0, filesSkipped: 0, filesErrored: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message: 'Could not acquire file lock - another process may be indexing', severity: 'error' as const }], durationMs: 0 }; + } catch (err) { + const message = err instanceof Error + ? err.message + : 'Could not acquire file lock - another process may be indexing'; + return { success: false, filesIndexed: 0, filesSkipped: 0, filesErrored: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message, severity: 'error' as const }], durationMs: 0 }; } try { return this.orchestrator.indexFiles(filePaths); @@ -413,7 +420,11 @@ export class CodeGraph { return this.indexMutex.withLock(async () => { try { this.fileLock.acquire(); - } catch { + } catch (err) { + logDebug('Skipping sync because file lock is unavailable', { + error: err instanceof Error ? err.message : String(err), + projectRoot: this.projectRoot, + }); return { filesChecked: 0, filesAdded: 0, filesModified: 0, filesRemoved: 0, nodesUpdated: 0, durationMs: 0 }; } try { diff --git a/src/utils.ts b/src/utils.ts index 1ee1c9372..0ab0e4498 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -31,6 +31,9 @@ import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; +import { randomUUID } from 'crypto'; +import { logDebug, logWarn } from './errors'; // ============================================================ // SECURITY UTILITIES @@ -175,17 +178,25 @@ export function normalizePath(filePath: string): string { } /** - * Cross-process file lock using a lock file with PID tracking. + * Cross-process file lock using a lock file with explicit ownership metadata. * * Prevents multiple processes (e.g., git hooks, CLI, MCP server) from * writing to the same database simultaneously. */ +interface FileLockInfo { + pid: number; + token?: string; + createdAt?: number; + hostname?: string; +} + export class FileLock { private lockPath: string; private held = false; + private token: string | null = null; + private acquiredAtMs: number | null = null; - /** Locks older than this are considered stale regardless of PID status */ - private static readonly STALE_TIMEOUT_MS = 2 * 60 * 1000; // 2 minutes + private static readonly WARN_HOLD_MS = 30_000; constructor(lockPath: string) { this.lockPath = lockPath; @@ -195,44 +206,39 @@ export class FileLock { * Acquire the lock. Throws if the lock is held by another live process. */ acquire(): void { - // Check for existing lock if (fs.existsSync(this.lockPath)) { - try { - const content = fs.readFileSync(this.lockPath, 'utf-8').trim(); - const pid = parseInt(content, 10); - const stat = fs.statSync(this.lockPath); - const lockAge = Date.now() - stat.mtimeMs; - - // Treat locks older than the timeout as stale, regardless of PID - if (lockAge < FileLock.STALE_TIMEOUT_MS && !isNaN(pid) && this.isProcessAlive(pid)) { - throw new Error( - `CodeGraph database is locked by another process (PID ${pid}). ` + - `If this is stale, run 'codegraph unlock' or delete ${this.lockPath}` - ); - } - - // Stale lock (dead process or timed out) - remove it - fs.unlinkSync(this.lockPath); - } catch (err) { - if (err instanceof Error && err.message.includes('locked by another')) { - throw err; + const existing = this.readLockInfo(); + if (existing) { + if (this.isProcessAlive(existing.pid)) { + throw new Error(this.buildLockedMessage(existing)); } - // Other errors reading lock file - try to remove it - try { fs.unlinkSync(this.lockPath); } catch { /* ignore */ } + this.removeLockFile('Removing stale lock from dead process', existing, 'debug'); + } else { + this.removeLockFile('Removing unreadable lock file before reacquiring', null, 'warn'); } } - // Write our PID to the lock file using exclusive create flag + const token = randomUUID(); + const info: FileLockInfo = { + pid: process.pid, + token, + createdAt: Date.now(), + hostname: os.hostname(), + }; + try { - fs.writeFileSync(this.lockPath, String(process.pid), { flag: 'wx' }); + fs.writeFileSync(this.lockPath, JSON.stringify(info), { flag: 'wx' }); this.held = true; + this.token = token; + this.acquiredAtMs = info.createdAt; + logDebug('Acquired file lock', { + lockPath: this.lockPath, + pid: process.pid, + }); } catch (err: any) { if (err.code === 'EEXIST') { - // Race condition: another process grabbed the lock between our check and write - throw new Error( - 'CodeGraph database is locked by another process. ' + - `If this is stale, run 'codegraph unlock' or delete ${this.lockPath}` - ); + const existing = this.readLockInfo(); + throw new Error(existing ? this.buildLockedMessage(existing) : this.buildLockedMessage()); } throw err; } @@ -244,15 +250,26 @@ export class FileLock { release(): void { if (!this.held) return; try { - // Only remove if we still own it (check PID) - const content = fs.readFileSync(this.lockPath, 'utf-8').trim(); - if (parseInt(content, 10) === process.pid) { + const info = this.readLockInfo(); + if (this.ownsLock(info)) { fs.unlinkSync(this.lockPath); } } catch { // Lock file already gone - that's fine + } finally { + if (this.acquiredAtMs !== null) { + const heldMs = Date.now() - this.acquiredAtMs; + const context = { lockPath: this.lockPath, pid: process.pid, heldMs }; + if (heldMs >= FileLock.WARN_HOLD_MS) { + logWarn('Released file lock after long hold', context); + } else { + logDebug('Released file lock', context); + } + } + this.held = false; + this.token = null; + this.acquiredAtMs = null; } - this.held = false; } /** @@ -279,6 +296,71 @@ export class FileLock { } } + private readLockInfo(): FileLockInfo | null { + try { + const content = fs.readFileSync(this.lockPath, 'utf-8').trim(); + if (!content) return null; + const legacyPid = parseInt(content, 10); + if (!Number.isNaN(legacyPid) && String(legacyPid) === content) { + const stat = fs.statSync(this.lockPath); + return { pid: legacyPid, createdAt: Number.isFinite(stat.mtimeMs) ? stat.mtimeMs : undefined }; + } + const parsed = safeJsonParse | null>(content, null); + if (!parsed || typeof parsed !== 'object' || typeof parsed.pid !== 'number') { + return null; + } + return { + pid: parsed.pid, + token: typeof parsed.token === 'string' ? parsed.token : undefined, + createdAt: typeof parsed.createdAt === 'number' ? parsed.createdAt : undefined, + hostname: typeof parsed.hostname === 'string' ? parsed.hostname : undefined, + }; + } catch { + return null; + } + } + + private ownsLock(info: FileLockInfo | null): boolean { + if (!info || info.pid !== process.pid) return false; + if (!info.token) return true; + return info.token === this.token; + } + + private buildLockedMessage(info?: FileLockInfo | null): string { + if (!info) { + return 'CodeGraph database is locked by another process. ' + + `If this is stale, run 'codegraph unlock' or delete ${this.lockPath}`; + } + const details = [`PID ${info.pid}`]; + if (info.hostname) details.push(`host ${info.hostname}`); + if (typeof info.createdAt === 'number') { + details.push(`held for ${Math.max(0, Date.now() - info.createdAt)}ms`); + } + return `CodeGraph database is locked by another process (${details.join(', ')}). ` + + `If this is stale, run 'codegraph unlock' or delete ${this.lockPath}`; + } + + private removeLockFile(message: string, info: FileLockInfo | null, level: 'debug' | 'warn'): void { + const context: Record = { lockPath: this.lockPath }; + if (info) { + context.pid = info.pid; + if (info.hostname) context.hostname = info.hostname; + if (typeof info.createdAt === 'number') { + context.ageMs = Math.max(0, Date.now() - info.createdAt); + } + } + try { + fs.unlinkSync(this.lockPath); + } catch { + return; + } + if (level === 'warn') { + logWarn(message, context); + } else { + logDebug(message, context); + } + } + /** * Check if a process is still running */