fix(lock): only one process takes over a stale write lock (#3056)

Issue #2979: FileLock.acquire() read the lock file, saw a dead PID,
unlinked the file and created its own with an exclusive create. Two
processes that read the same dead PID could both succeed: the first one
replaced the stale lock, then the second one, acting on what it had read
a moment earlier, unlinked the first one's live lock and created its own.
Both returned from acquire() and wrote to the index as its only writer.
The same steal happened in two narrower ways: a lock file read in the
instant between its exclusive create and the write of its PID was empty,
so it counted as stale, and a PID the process may not signal (EPERM:
another user's process, or an elevated one on Windows) counted as dead.

The fix keeps the lock file and its format (a bare PID) and changes how
it is taken:
- A free lock costs one create, as before. The lock file is created with
  its PID already in it: a private file hard-linked into place, which
  fails if the lock exists. Where hard links are missing it falls back
  to the exclusive create, and an empty lock younger than 10 s counts as
  held, since its creator is still writing its PID.
- A stale lock (dead PID, malformed, or empty and old) is replaced only
  by the process holding <lock>.break, itself a FileLock, after reading
  the lock again there, the way Mercurial breaks its locks. A break lock
  a crashed process left is broken the same way, one level up, at most
  three levels.
- EPERM from a signal-0 probe means the holder is alive.

Validation: __tests__/filelock-stale-takeover.test.ts runs two real
processes with a barrier: the slow one stops right after reading the
dead PID and resumes only once the other holds the lock. On main both
held it, 5 runs of 5; with the fix the slow one is refused and the lock
stays the holder's. The other cases cover the slow one getting in once
the lock is free, a live and a dead break lock, the empty-lock window,
EPERM (mocked, and for real on Windows with the System process), and no
leftover files.

Fixes #2979

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -68,6 +68,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - A file is no longer saved with no symbols when its language parser can't be loaded, which is what happened to every file a background server re-indexed after an upgrade removed its install: the file keeps what it had and is indexed again once the parser loads, and files an earlier version emptied this way are re-indexed by the next sync. A background server also exits on its own once its install is upgraded or removed, so the next session starts one from the current install. Thanks @lipchey for the report. (#2335)
 - `codegraph index` no longer stops with advice to run `codegraph daemon stop`, a command that doesn't exist, when the project's background-server lock names a process that isn't CodeGraph, such as a lock left by a crash whose process number the system has since given to another program: it now clears that stale lock, leaving the process alone, and rebuilds, and `codegraph unlock` clears it too. When a CodeGraph process that doesn't answer holds the lock, such as an older background server, the message names that process and the command that stops it, and the message about another server already serving a project now points to `codegraph daemon`. Thanks @inth3shadows for the report and @danusha2345 for the first fix. (#2770)
 - A program that uses CodeGraph as a library and indexes chosen files with `indexFiles()` now keeps other CodeGraph processes, like a `codegraph sync` run by a git hook, out of the index until those files are stored. Before, it let go of the write lock that keeps two writers apart as soon as indexing started, so another process could write to the index at the same time.
+- When several CodeGraph processes start at the same moment after one of them crashed, like the MCP server, a git hook's `codegraph sync` and `codegraph index`, only one of them now takes over the write lock the crashed process left behind. Before, two of them could both take it, the second one deleting the first one's lock as it did, and both went on to write to the index at once. A write lock held by another user's CodeGraph, or on Windows by one running as administrator, is no longer mistaken for a crashed one's and taken over either. Thanks @inth3shadows for the report and @vedantb7 for the first fix. (#2979)
 - A program that uses CodeGraph as a library and runs `indexAll()` again on a project it has already indexed now keeps every symbol findable by the words in its name, which the prompt hook and `codegraph_explore` use to match a question like "how does the order state machine work" to `OrderStateMachine`. Before, only the files changed since the last index kept their names' words, and a cancelled run lost them all; `codegraph index` was not affected. Thanks @rudycelekli.
 - The shared background server is no longer killed with "Main thread unresponsive" while it brings another project's index up to date, such as one a session names with `projectPath`: only its own project's index counted as progress, so a long update of any other looked like a hang, and the server was stopped and restarted again and again. Every index the server keeps up to date now counts, and so does the one a session updates on its own when the background server is unavailable. Thanks @mradwankhalil for the report and the first fix. (#2404)
 - A long-running session, like the background server behind your agent, now notices a framework that arrives or goes while it runs, such as Express in a new `package.json`, or one app replaced by another with as many files. Files it indexes from then on are read with the frameworks the project has now; before, they were read with the ones it had when the session started, so a new Express app's routes were missing until a full `codegraph index`. Files indexed before the change are not read again, so `codegraph status` now says which frameworks changed since the last full index and recommends running one. Thanks @nk33-dev for the report and the first fix. (#2516, #2519)

__tests__/filelock-stale-takeover.test.ts

@@ -0,0 +1,212 @@
+/**
+ * Taking over a stale `codegraph.lock` must leave exactly one holder (#2979).
+ *
+ * `FileLock.acquire()` used to read the lock, see a dead PID, unlink the file
+ * and create its own. Two processes that both read the same dead PID could
+ * then both succeed: the first replaced the stale lock, and the second, acting
+ * on what it had read a moment earlier, unlinked the first one's live lock and
+ * created its own. Both then wrote to the index as its only writer.
+ *
+ * The two-process cases pin that interleaving with a barrier rather than
+ * timing: the slow contender stops right after reading the stale lock and
+ * resumes only once the fast one holds it (see fixtures/filelock-contender.cjs).
+ */
+import { describe, it, expect, beforeEach, afterEach, vi } 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 { FileLock } from '../src/utils';
+
+const UTILS = path.resolve(__dirname, '../dist/utils.js');
+const CONTENDER = path.resolve(__dirname, 'fixtures/filelock-contender.cjs');
+/** Above every platform's PID range (and odd, which no Windows PID is): never a live process. */
+const DEAD_PID = '99999999';
+
+interface Contender {
+  child: ChildProcess;
+  name: string;
+  stderr: () => string;
+  exited: Promise<number | null>;
+}
+
+describe('FileLock stale takeover (#2979)', () => {
+  let dir: string;
+  let lockPath: string;
+  let markers: string;
+  let contenders: Contender[] = [];
+
+  beforeEach(() => {
+    dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-filelock-'));
+    lockPath = path.join(dir, 'codegraph.lock');
+    markers = path.join(dir, 'markers');
+    fs.mkdirSync(markers);
+  });
+
+  afterEach(async () => {
+    for (const { child, exited } of contenders) {
+      if (child.exitCode === null && child.signalCode === null) child.kill();
+      await exited;
+    }
+    contenders = [];
+    fs.rmSync(dir, { recursive: true, force: true });
+  });
+
+  function track(name: string, args: string[]): Contender {
+    const child = spawn(process.execPath, args, {
+      // Off the fixture, so a child still exiting cannot hold it (Windows EBUSY).
+      cwd: os.tmpdir(),
+      stdio: ['ignore', 'ignore', 'pipe'],
+      windowsHide: true,
+    });
+    let stderr = '';
+    child.stderr!.on('data', (chunk) => { stderr += String(chunk); });
+    const exited = new Promise<number | null>((resolve) => child.once('exit', (code) => resolve(code)));
+    const contender = { child, name, stderr: () => stderr, exited };
+    contenders.push(contender);
+    return contender;
+  }
+
+  const start = (name: string, mode: 'pause' | 'run') =>
+    track(name, [CONTENDER, UTILS, lockPath, markers, name, mode]);
+  /** A live process that never touches the lock: its PID names a holder that is alive. */
+  const idle = () => track('idle', ['-e', 'setInterval(() => {}, 1000)']);
+
+  /** The first of `names` (marker files) to appear, and its content. */
+  async function firstMarker(names: string[], from: Contender[]): Promise<{ name: string; text: string }> {
+    const deadline = Date.now() + 20_000;
+    for (;;) {
+      for (const name of names) {
+        const file = path.join(markers, name);
+        if (fs.existsSync(file)) return { name, text: fs.readFileSync(file, 'utf8') };
+      }
+      if (Date.now() > deadline) {
+        const detail = from.map((c) => `${c.name} (exit ${c.child.exitCode}): ${c.stderr()}`).join('\n');
+        throw new Error(`none of ${names.join(', ')} appeared\n${detail}`);
+      }
+      await new Promise((resolve) => setTimeout(resolve, 10));
+    }
+  }
+
+  const release = (who: Contender) => fs.writeFileSync(path.join(markers, `${who.name}.release`), '');
+  const go = () => fs.writeFileSync(path.join(markers, 'go'), '');
+
+  it('lets only one of two processes take over a stale lock', async () => {
+    fs.writeFileSync(lockPath, DEAD_PID);
+
+    // The slow process reads the dead PID, then stalls before acting on it.
+    const slow = start('slow', 'pause');
+    const read = await firstMarker(['slow.read'], [slow]);
+    expect(read.text.trim()).toBe(DEAD_PID);
+
+    // Meanwhile another process takes the stale lock over, and holds it.
+    const fast = start('fast', 'run');
+    expect((await firstMarker(['fast.acquired', 'fast.refused'], [fast])).name).toBe('fast.acquired');
+
+    // The slow one resumes with the dead PID it read: it must not replace a live lock.
+    go();
+    const slowOutcome = await firstMarker(['slow.acquired', 'slow.refused'], [slow]);
+    expect(slowOutcome.name, 'both processes held the lock at once').toBe('slow.refused');
+    expect(slowOutcome.text).toMatch(/locked by another process/);
+    expect(fs.readFileSync(lockPath, 'utf8').trim()).toBe(String(fast.child.pid));
+
+    release(fast);
+    expect(await fast.exited).toBe(0);
+    expect(await slow.exited).toBe(0);
+    expect(fs.existsSync(lockPath)).toBe(false);
+  }, 60_000);
+
+  it('still lets the slow process in once the other one has released the lock', async () => {
+    fs.writeFileSync(lockPath, DEAD_PID);
+
+    const slow = start('slow', 'pause');
+    await firstMarker(['slow.read'], [slow]);
+    const fast = start('fast', 'run');
+    expect((await firstMarker(['fast.acquired', 'fast.refused'], [fast])).name).toBe('fast.acquired');
+    release(fast);
+    await firstMarker(['fast.released'], [fast]);
+    expect(await fast.exited).toBe(0);
+
+    // The lock is free now: what the slow one read is out of date in the other direction.
+    go();
+    expect((await firstMarker(['slow.acquired', 'slow.refused'], [slow])).name).toBe('slow.acquired');
+    expect(fs.readFileSync(lockPath, 'utf8').trim()).toBe(String(slow.child.pid));
+    release(slow);
+    expect(await slow.exited).toBe(0);
+    expect(fs.existsSync(lockPath)).toBe(false);
+  }, 60_000);
+
+  it('leaves a stale lock to the process already taking it over', async () => {
+    // A live process holds the takeover sentinel: it is replacing the stale lock right now.
+    const breaker = idle();
+    fs.writeFileSync(lockPath, DEAD_PID);
+    fs.writeFileSync(`${lockPath}.break`, String(breaker.child.pid));
+
+    const lock = new FileLock(lockPath);
+    expect(() => lock.acquire()).toThrow(new RegExp(`locked by another process \\(PID ${breaker.child.pid}\\)`));
+    expect(fs.readFileSync(lockPath, 'utf8')).toBe(DEAD_PID);
+    expect(fs.readFileSync(`${lockPath}.break`, 'utf8')).toBe(String(breaker.child.pid));
+  });
+
+  it('takes over a stale lock whose takeover sentinel a dead process left behind', () => {
+    fs.writeFileSync(lockPath, DEAD_PID);
+    fs.writeFileSync(`${lockPath}.break`, DEAD_PID);
+
+    const lock = new FileLock(lockPath);
+    lock.acquire();
+    try {
+      expect(fs.readFileSync(lockPath, 'utf8')).toBe(String(process.pid));
+      expect(fs.readdirSync(dir).sort()).toEqual(['codegraph.lock', 'markers']);
+    } finally {
+      lock.release();
+    }
+    expect(fs.existsSync(lockPath)).toBe(false);
+  });
+
+  it('treats an empty lock file its creator may still be writing as held, and an old one as stale', () => {
+    // A filesystem without hard links creates the lock first and writes the PID after.
+    fs.writeFileSync(lockPath, '');
+    const lock = new FileLock(lockPath);
+    expect(() => lock.acquire()).toThrow(/locked by another process/);
+    expect(fs.readFileSync(lockPath, 'utf8')).toBe('');
+
+    // Left empty by a crash long ago: stale.
+    const old = new Date(Date.now() - 60_000);
+    fs.utimesSync(lockPath, old, old);
+    lock.acquire();
+    expect(fs.readFileSync(lockPath, 'utf8')).toBe(String(process.pid));
+    lock.release();
+    expect(fs.existsSync(lockPath)).toBe(false);
+  });
+
+  it('treats a PID it may not signal (EPERM) as a live holder', () => {
+    // Another user's process, or an elevated one on Windows: alive, just not ours to signal.
+    const foreign = 4242;
+    fs.writeFileSync(lockPath, String(foreign));
+    const kill = vi.spyOn(process, 'kill').mockImplementation(((pid: number, signal?: string | number) => {
+      if (pid === foreign && signal === 0) throw Object.assign(new Error('kill EPERM'), { code: 'EPERM' });
+      return true;
+    }) as typeof process.kill);
+    try {
+      expect(() => new FileLock(lockPath).acquire()).toThrow(/locked by another process \(PID 4242\)/);
+      expect(fs.readFileSync(lockPath, 'utf8')).toBe(String(foreign));
+    } finally {
+      kill.mockRestore();
+    }
+  });
+
+  it.runIf(process.platform === 'win32')('keeps a lock whose holder Windows will not let us open, like an elevated process', () => {
+    // PID 4 is always the System process: opening it to signal is refused (EPERM), as for an elevated one.
+    fs.writeFileSync(lockPath, '4');
+    expect(() => new FileLock(lockPath).acquire()).toThrow(/locked by another process \(PID 4\)/);
+    expect(fs.readFileSync(lockPath, 'utf8')).toBe('4');
+  });
+
+  it('leaves nothing but the lock file while held, and nothing after release', () => {
+    const lock = new FileLock(lockPath);
+    lock.acquire();
+    expect(fs.readdirSync(dir).sort()).toEqual(['codegraph.lock', 'markers']);
+    lock.release();
+    expect(fs.readdirSync(dir)).toEqual(['markers']);
+  });
+});

__tests__/fixtures/filelock-contender.cjs

@@ -0,0 +1,57 @@
+/**
+ * One contender for a `FileLock`, run as its own process by
+ * filelock-stale-takeover.test.ts (#2979).
+ *
+ *   node filelock-contender.cjs <utils.js> <lock> <markers> <name> <pause>
+ *
+ * It takes the lock with the built `FileLock`, writes `<name>.acquired` (and
+ * holds the lock until `<name>.release` appears) or `<name>.refused`, then
+ * exits. With `<pause>` = `pause`, it stops right after its first read of the
+ * lock file, with whatever that read returned in hand, writes `<name>.read`,
+ * and waits for `go`: the moment a slow process has decided a lock is stale
+ * but has not acted on it yet. Every wait is on a marker file the test writes,
+ * so the interleaving is the same on every run. A contender gives up after
+ * 30 s of waiting, so none outlives a test that stopped driving it.
+ */
+const fs = require('fs');
+const path = require('path');
+
+const [utilsJs, lockPath, markers, name, pause] = process.argv.slice(2);
+const marker = (suffix) => path.join(markers, `${name}.${suffix}`);
+const mark = (suffix, text = String(process.pid)) => fs.writeFileSync(marker(suffix), text);
+const sleep = (ms) => Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+const waitFor = (file) => {
+  const deadline = Date.now() + 30_000;
+  while (!fs.existsSync(file)) {
+    if (Date.now() > deadline) process.exit(3);
+    sleep(5);
+  }
+};
+
+if (pause === 'pause') {
+  const target = path.resolve(lockPath);
+  const read = fs.readFileSync;
+  let paused = false;
+  fs.readFileSync = function readThenPause(file, ...rest) {
+    const content = read.call(this, file, ...rest);
+    if (!paused && typeof file === 'string' && path.resolve(file) === target) {
+      paused = true;
+      mark('read', String(content));
+      waitFor(path.join(markers, 'go'));
+    }
+    return content;
+  };
+}
+
+const { FileLock } = require(utilsJs);
+const lock = new FileLock(lockPath);
+try {
+  lock.acquire();
+} catch (err) {
+  mark('refused', err instanceof Error ? err.message : String(err));
+  process.exit(0);
+}
+mark('acquired');
+waitFor(marker('release'));
+lock.release();
+mark('released');

src/utils.ts

@@ -32,6 +32,7 @@
 import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
+import { threadId } from 'worker_threads';
 
 // ============================================================
 // SECURITY UTILITIES
@@ -425,6 +426,26 @@ export function escapeRegExp(text: string): string {
   return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
 }
 
+/**
+ * An empty lock file younger than this is still being written by the process
+ * that created it (#2979): where hard links are missing, the lock is created
+ * first and the PID written a moment later. Only a crash leaves one empty for
+ * long, and FAT's 2 s timestamps stay well inside this.
+ */
+const EMPTY_LOCK_GRACE_MS = 10_000;
+
+/**
+ * How many `.break` levels a takeover may climb. Each level is needed only if
+ * a process died inside the level above it, within a few file operations.
+ */
+const MAX_LOCK_BREAK_DEPTH = 3;
+
+/** What a lock file says about its holder. */
+type LockHolder =
+  | { state: 'free' }
+  | { state: 'held'; pid?: number }
+  | { state: 'stale' };
+
 /**
  * Cross-process file lock using a lock file with PID tracking.
  *
@@ -441,49 +462,115 @@ export class FileLock {
 
   /**
    * Acquire the lock. Throws if the lock is held by another live process.
+   *
+   * A free lock costs one create. A stale one, whose PID is dead or missing,
+   * is replaced only by the process holding `<lock>.break` (itself a
+   * FileLock), after reading it again there (#2979). Before, two processes
+   * that read the same dead PID both unlinked the lock and created their own,
+   * the second one deleting the first one's live lock, and both went on to
+   * write as the only writer.
    */
   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);
-        // A long sync can hold the lock for hours. Its age says nothing about
-        // whether the writer is still alive; only a dead PID is stale.
-        if (!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 malformed PID) - remove it
-        fs.unlinkSync(this.lockPath);
-      } catch (err) {
-        if (err instanceof Error && err.message.includes('locked by another')) {
-          throw err;
-        }
-        // Other errors reading lock file - try to remove it
-        try { fs.unlinkSync(this.lockPath); } catch { /* ignore */ }
-      }
+    const refusal = this.tryAcquire(0);
+    if (refusal) {
+      const who = refusal.state === 'held' && refusal.pid !== undefined ? ` (PID ${refusal.pid})` : '';
+      throw new Error(
+        `CodeGraph database is locked by another process${who}. ` +
+        `If this is stale, run 'codegraph unlock' or delete ${this.lockPath}`
+      );
     }
+  }
 
-    // Write our PID to the lock file using exclusive create flag
+  /** Null once the lock is ours; otherwise what kept us out. */
+  private tryAcquire(depth: number): LockHolder | null {
+    if (this.create()) return this.taken();
+    let holder = this.readHolder();
+    if (holder.state === 'free') {
+      // Released between our create and our read.
+      if (this.create()) return this.taken();
+      holder = this.readHolder();
+    }
+    // A long sync can hold the lock for hours. Its age says nothing about
+    // whether the writer is still alive; only a dead PID is stale.
+    if (holder.state !== 'stale') return holder;
+    if (depth >= MAX_LOCK_BREAK_DEPTH) return { state: 'held' };
+    const breaker = new FileLock(`${this.lockPath}.break`);
+    // Refused: a live process is replacing this stale lock right now.
+    const breaking = breaker.tryAcquire(depth + 1);
+    if (breaking) return breaking;
     try {
-      fs.writeFileSync(this.lockPath, String(process.pid), { flag: 'wx' });
-      this.held = true;
-    } 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}`
-        );
+      // What we read before holding the break lock may be out of date: another
+      // process may have replaced the stale lock, or released its own, since.
+      const current = this.readHolder();
+      if (current.state === 'held') return current;
+      // Nobody else replaces a stale lock while we hold the break lock, and no
+      // create succeeds while the file exists, so this unlinks the stale lock.
+      if (current.state === 'stale') {
+        try { fs.unlinkSync(this.lockPath); } catch { /* gone, or the create below says who has it */ }
       }
+      return this.create() ? this.taken() : this.readHolder();
+    } finally {
+      breaker.release();
+    }
+  }
+
+  private taken(): null {
+    this.held = true;
+    return null;
+  }
+
+  /**
+   * Create the lock file with our PID already in it, so no reader ever finds
+   * it empty: write a private file, then hard-link it into place, which fails
+   * if the lock exists. Where hard links are missing, fall back to an
+   * exclusive create, whose PID lands a moment later (see
+   * EMPTY_LOCK_GRACE_MS). False when the lock already exists.
+   */
+  private create(): boolean {
+    const pid = String(process.pid);
+    const tmp = `${this.lockPath}.${process.pid}.${threadId}.tmp`;
+    try {
+      fs.writeFileSync(tmp, pid);
+      fs.linkSync(tmp, this.lockPath);
+      return true;
+    } catch (err) {
+      if ((err as NodeJS.ErrnoException).code === 'EEXIST') return false;
+      // No hard links here (or no private file): create in place instead.
+    } finally {
+      try { fs.unlinkSync(tmp); } catch { /* never written */ }
+    }
+    try {
+      fs.writeFileSync(this.lockPath, pid, { flag: 'wx' });
+      return true;
+    } catch (err) {
+      if ((err as NodeJS.ErrnoException).code === 'EEXIST') return false;
       throw err;
     }
   }
 
+  private readHolder(): LockHolder {
+    let content: string;
+    try {
+      content = fs.readFileSync(this.lockPath, 'utf-8').trim();
+    } catch (err) {
+      // Unreadable is not proof the holder is gone: leave it to `codegraph unlock`.
+      return (err as NodeJS.ErrnoException).code === 'ENOENT' ? { state: 'free' } : { state: 'held' };
+    }
+    if (content === '') {
+      let age: number;
+      try {
+        age = Date.now() - fs.statSync(this.lockPath).mtimeMs;
+      } catch {
+        return { state: 'free' };
+      }
+      return age < EMPTY_LOCK_GRACE_MS ? { state: 'held' } : { state: 'stale' };
+    }
+    const pid = parseInt(content, 10);
+    // Malformed: no process owns it.
+    if (!(pid > 0)) return { state: 'stale' };
+    return this.isProcessAlive(pid) ? { state: 'held', pid } : { state: 'stale' };
+  }
+
   /**
    * Release the lock
    */
@@ -532,8 +619,9 @@ export class FileLock {
     try {
       process.kill(pid, 0);
       return true;
-    } catch {
-      return false;
+    } catch (err) {
+      // EPERM: it exists but is not ours to signal (another user's, or elevated on Windows).
+      return (err as NodeJS.ErrnoException).code === 'EPERM';
     }
   }
 }