fix(miner): close the clone-lock TOCTOU on a still-empty lockfile (#9681)

packages/loopover-miner/lib/repo-clone.ts

@@ -1,6 +1,6 @@
 import { execFile } from "node:child_process";
 import { randomUUID } from "node:crypto";
-import { closeSync, existsSync, mkdirSync, openSync, readFileSync, unlinkSync, writeSync } from "node:fs";
+import { closeSync, existsSync, mkdirSync, openSync, readFileSync, statSync, unlinkSync, writeSync } from "node:fs";
 import { homedir, hostname } from "node:os";
 import { dirname, join } from "node:path";
 import { promisify } from "node:util";
@@ -33,6 +33,7 @@ export type RepoCloneLockOptions = {
   isProcessAlive?: (pid: number) => boolean;
   openLock?: (lockPath: string) => number;
   writeLock?: (fd: number, data: string) => void;
+  statLock?: (lockPath: string) => { mtimeMs: number };
 };
 
 type EnsureRepoClonedOptions = {
@@ -146,33 +147,63 @@ async function withRepoCloneLock<T>(repoPath: string, fn: () => Promise<T>): Pro
 const DEFAULT_LOCK_TIMEOUT_MS = 10 * 60 * 1000; // comfortably past a slow clone/fetch sequence
 const DEFAULT_LOCK_STALE_MS = 15 * 60 * 1000; // a lock older than this is presumed crashed
 const DEFAULT_LOCK_POLL_MS = 100;
+// #9681: an empty/unparseable lockfile is the NORMAL mid-acquire state -- open(.., 'wx') creates it 0-byte and
+// the owner record is written a few statements later, so a concurrent process that hits EEXIST in that window
+// must NOT reclaim it as a crash. Only reclaim once it has sat unwritten longer than this grace window (well past
+// the microseconds between create and owner-write, but far below DEFAULT_LOCK_STALE_MS so a genuinely crashed
+// mid-write lock is still cleared promptly).
+export const DEFAULT_LOCK_INCOMPLETE_GRACE_MS = 5_000;
 const defaultLockSleep = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms));
 
 function repoCloneLockPath(repoPath: string): string {
   return `${repoPath}.clone.lock`;
 }
 
 /**
- * Decide whether an existing clone lockfile is stale (reclaimable): true when the file is missing or its JSON is
- * unreadable/partial (a crash mid-write), when its owner pid is confirmed dead within the SAME host's namespace,
- * or -- ONLY for an owner this host cannot probe (a different host/container, or a malformed record with no
- * usable pid) -- when it is older than `staleMs`. A same-host owner whose pid IS probeable is judged purely by
- * liveness: a live one is never stale no matter how long its clone legitimately runs (age reclaim there would
- * yank the lock out from under an in-progress clone -- a double-holder bug), and a dead one is stale at once.
+ * An empty/unparseable lockfile is BOTH the mid-write crash case AND the normal mid-acquire state of every
+ * successful acquisition (open 'wx' creates it 0-byte, the owner record lands a few statements later, #9681), so
+ * it must not be reclaimed on sight -- that lets a concurrent process delete a live holder's lock in the window
+ * before its owner-write and double-hold. Reclaim only once it has sat unwritten past DEFAULT_LOCK_INCOMPLETE_GRACE_MS.
+ * A stat that throws means the file vanished between the read and here -- nothing left to hold, so reclaimable.
+ */
+function isIncompleteLockReclaimable(
+  lockPath: string,
+  nowMs: number,
+  statLock: (lockPath: string) => { mtimeMs: number },
+): boolean {
+  let mtimeMs: number;
+  try {
+    mtimeMs = statLock(lockPath).mtimeMs;
+  } catch {
+    return true;
+  }
+  return nowMs - mtimeMs > DEFAULT_LOCK_INCOMPLETE_GRACE_MS;
+}
+
+/**
+ * Decide whether an existing clone lockfile is stale (reclaimable): true when the file is missing, when its owner
+ * pid is confirmed dead within the SAME host's namespace, or -- ONLY for an owner this host cannot probe (a
+ * different host/container, or a malformed record with no usable pid) -- when it is older than `staleMs`. A same-host
+ * owner whose pid IS probeable is judged purely by liveness: a live one is never stale no matter how long its clone
+ * legitimately runs (age reclaim there would yank the lock out from under an in-progress clone -- a double-holder
+ * bug), and a dead one is stale at once. An unreadable/partial JSON payload is NOT unconditionally stale (#9681):
+ * it is the normal mid-acquire state, so it is reclaimed only once its own mtime is older than the incomplete-lock
+ * grace window (see {@link isIncompleteLockReclaimable}).
  */
 export function isRepoCloneLockStale(
   lockPath: string,
   nowMs: number,
   staleMs: number,
   isAlive: (pid: number) => boolean = isProcessAlive,
+  statLock: (lockPath: string) => { mtimeMs: number } = (path) => statSync(path),
 ): boolean {
   let meta: unknown;
   try {
     meta = JSON.parse(readFileSync(lockPath, "utf8"));
   } catch {
-    return true;
+    return isIncompleteLockReclaimable(lockPath, nowMs, statLock);
   }
-  if (!meta || typeof meta !== "object") return true;
+  if (!meta || typeof meta !== "object") return isIncompleteLockReclaimable(lockPath, nowMs, statLock);
   const record = meta as RepoCloneLockMeta;
   // Owner we can directly probe (same host, usable pid): trust liveness exclusively -- alive => held (never
   // age-reclaim a still-running local clone), dead => reclaim now. The age backstop below is reserved for an
@@ -202,6 +233,7 @@ export async function acquireRepoCloneLock(repoPath: string, options: RepoCloneL
   const isAlive = typeof options.isProcessAlive === "function" ? options.isProcessAlive : isProcessAlive;
   const openLock = typeof options.openLock === "function" ? options.openLock : (path: string) => openSync(path, "wx", 0o600);
   const writeLock = typeof options.writeLock === "function" ? options.writeLock : (fd: number, data: string) => writeSync(fd, data);
+  const statLock = typeof options.statLock === "function" ? options.statLock : (path: string) => statSync(path);
 
   mkdirSync(dirname(lockPath), { recursive: true, mode: 0o700 });
   const deadline = now() + timeoutMs;
@@ -212,7 +244,7 @@ export async function acquireRepoCloneLock(repoPath: string, options: RepoCloneL
     } catch (error: unknown) {
       const err = error as NodeJS.ErrnoException | null | undefined;
       if (!err || err.code !== "EEXIST") throw error;
-      if (isRepoCloneLockStale(lockPath, now(), staleMs, isAlive)) {
+      if (isRepoCloneLockStale(lockPath, now(), staleMs, isAlive, statLock)) {
         try {
           unlinkSync(lockPath);
         } catch {

test/unit/miner-repo-clone-lock.test.ts

@@ -1,8 +1,8 @@
-import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { existsSync, mkdirSync, mkdtempSync, openSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { hostname, tmpdir } from "node:os";
 import { dirname, join } from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
-import { acquireRepoCloneLock, ensureRepoCloned, isRepoCloneLockStale } from "../../packages/loopover-miner/lib/repo-clone";
+import { acquireRepoCloneLock, DEFAULT_LOCK_INCOMPLETE_GRACE_MS, ensureRepoCloned, isRepoCloneLockStale } from "../../packages/loopover-miner/lib/repo-clone";
 import {
   cleanupResourceCount,
   closeAllCleanupResources,
@@ -31,19 +31,39 @@ function writeLockFile(lockPath: string, meta: unknown) {
 const at1000 = new Date(1000).toISOString();
 
 describe("isRepoCloneLockStale (#7084)", () => {
-  it("treats a missing or unreadable lockfile as stale", () => {
+  // A statLock stub that reports a fixed mtime, so incomplete-lock staleness is driven purely by nowMs vs mtime.
+  const statAt = (mtimeMs: number) => () => ({ mtimeMs });
+
+  it("treats a missing lockfile as stale (nothing to hold)", () => {
     const { lockPath } = tempRepoPath();
-    expect(isRepoCloneLockStale(lockPath, 1000, 5000)).toBe(true); // never created
-    writeLockFile(lockPath, "{ not valid json");
+    // Never created: readFileSync throws, then the default statSync also throws -> reclaimable.
     expect(isRepoCloneLockStale(lockPath, 1000, 5000)).toBe(true);
   });
 
-  it("treats a non-object payload as stale", () => {
+  it("REGRESSION (#9681): an unparseable/partial lockfile is NOT stale within the incomplete-grace window, but IS past it", () => {
     const { lockPath } = tempRepoPath();
+    writeLockFile(lockPath, "{ not valid json"); // the 0-byte / mid-write state open('wx') leaves before writeLock
+    const now = 10_000;
+    // mtime == now: a concurrent holder is probably still mid-acquire -> must NOT be reclaimed (was: always stale).
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(now))).toBe(false);
+    // At the grace bound exactly (5s old) -> still held; just past it (6s old) -> genuinely abandoned -> reclaim.
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(now - DEFAULT_LOCK_INCOMPLETE_GRACE_MS))).toBe(false);
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(now - 6_000))).toBe(true);
+  });
+
+  it("REGRESSION (#9681): a non-object payload is grace-windowed the same way, and a stat that throws is stale", () => {
+    const { lockPath } = tempRepoPath();
+    const now = 10_000;
     writeLockFile(lockPath, "null");
-    expect(isRepoCloneLockStale(lockPath, 1000, 5000)).toBe(true);
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(now))).toBe(false); // within grace
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(1_000))).toBe(true); // 9s old > 5s grace
     writeLockFile(lockPath, "123");
-    expect(isRepoCloneLockStale(lockPath, 1000, 5000)).toBe(true);
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statAt(now))).toBe(false);
+    // The file vanished between the failed read and the stat -> nothing to hold -> reclaimable.
+    const statThrows = () => {
+      throw new Error("ENOENT");
+    };
+    expect(isRepoCloneLockStale(lockPath, now, 5000, () => true, statThrows)).toBe(true);
   });
 
   it("reclaims a same-host lock whose owner PID is dead", () => {
@@ -202,6 +222,50 @@ describe("acquireRepoCloneLock (#7084)", () => {
     release();
   });
 
+  it("REGRESSION (#9681): POLLS a fresh empty peer lock (within grace) instead of reclaiming and re-acquiring it", async () => {
+    const { repoPath, lockPath } = tempRepoPath();
+    writeLockFile(lockPath, ""); // a peer's freshly-created, not-yet-owner-written 0-byte lock
+    const now = 5_000_000;
+    const sleeps: number[] = [];
+    let opens = 0;
+    const release = await acquireRepoCloneLock(repoPath, {
+      nowMs: () => now,
+      lockPollMs: 7,
+      lockSleep: async (ms) => {
+        sleeps.push(ms);
+        // The peer finishes its acquisition and releases after our first poll, so our retry can take the lock.
+        rmSync(lockPath, { force: true });
+      },
+      openLock: (path) => {
+        opens += 1;
+        if (opens === 1) {
+          const error = new Error("EEXIST") as NodeJS.ErrnoException;
+          error.code = "EEXIST";
+          throw error; // the peer holds the freshly-created empty lock
+        }
+        return openSync(path, "wx", 0o600);
+      },
+      // mtime == now -> the empty peer lock is inside the incomplete-grace window -> NOT stale.
+      statLock: () => ({ mtimeMs: now }),
+      isProcessAlive: () => true,
+    });
+    // Before the fix the empty lock read as stale, so acquire UNLINKED it and `continue`d WITHOUT sleeping.
+    expect(sleeps).toEqual([7]); // it polled instead of reclaiming
+    expect(opens).toBe(2); // retried after the poll
+    release();
+  });
+
+  it("REGRESSION (#9681): reclaims an over-grace empty lock through the real default statLock (statSync) path", async () => {
+    const { repoPath, lockPath } = tempRepoPath();
+    writeLockFile(lockPath, ""); // an abandoned 0-byte lock a crashed peer left mid-acquire
+    // No injected statLock/openLock: exercises the real default statSync-backed incomplete-lock check. The real
+    // file mtime is ~now, so an injected clock well past the grace window makes the default check reclaim it.
+    const future = Date.now() + DEFAULT_LOCK_INCOMPLETE_GRACE_MS + 60_000;
+    const release = await acquireRepoCloneLock(repoPath, { nowMs: () => future, isProcessAlive: () => true });
+    expect(JSON.parse(readFileSync(lockPath, "utf8")).pid).toBe(process.pid); // reclaimed and re-owned
+    release();
+  });
+
   it("rethrows a non-EEXIST open error and a non-Error thrown value", async () => {
     const { repoPath } = tempRepoPath();
     await expect(