From 9d92a196b3f3f57a28486c3c272b7ab026b2e548 Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 06:27:13 +0800 Subject: [PATCH] fix(miner): close the clone-lock TOCTOU on a still-empty lockfile (#9681) acquireRepoCloneLock creates the lockfile EMPTY (open 'wx' -> 0-byte) and writes the owner record several statements later. isRepoCloneLockStale treated that intermediate state as immediately reclaimable: a concurrent process P2 that hit EEXIST while P1 was between openSync and writeSync read "", JSON.parse threw, P2 declared the lock stale, unlinked it, and its own open 'wx' then succeeded -- so P1's later write landed in an unlinked inode and both processes ran git fetch/checkout/reset on the same .git dir, the exact index/HEAD/refs corruption the lock exists to prevent. The per-acquire token doesn't help: it only stops release() from deleting a peer's lock, not double-holding. - Export DEFAULT_LOCK_INCOMPLETE_GRACE_MS = 5_000. - isRepoCloneLockStale's unparseable-JSON and non-object arms now consult the lockfile's own mtimeMs (via a new injectable statLock seam, defaulting to statSync, threaded from acquireRepoCloneLock like the existing openLock/ writeLock seams) and reclaim ONLY when nowMs - mtimeMs exceeds the grace window; within it the lock is treated as a live mid-acquire holder. A statSync that throws (the file vanished) returns stale. The same-host-live- pid and age-backstop branches are unchanged. - Docstring updated so an unparseable lock is no longer described as unconditionally stale. Tests: unparseable/non-object locks within grace are NOT stale but ARE past it; a throwing stat is stale; acquireRepoCloneLock POLLS a fresh empty peer lock instead of reclaiming it; and it reclaims an over-grace empty lock through the real default statSync path. The reclaim assertions fail against the current code. Closes #9681 Co-Authored-By: Claude Opus 4.8 --- packages/loopover-miner/lib/repo-clone.ts | 52 ++++++++++++--- test/unit/miner-repo-clone-lock.test.ts | 80 ++++++++++++++++++++--- 2 files changed, 114 insertions(+), 18 deletions(-) diff --git a/packages/loopover-miner/lib/repo-clone.ts b/packages/loopover-miner/lib/repo-clone.ts index f85116bca9..c3bbe128d8 100644 --- a/packages/loopover-miner/lib/repo-clone.ts +++ b/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,6 +147,12 @@ async function withRepoCloneLock(repoPath: string, fn: () => Promise): 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 => new Promise((resolve) => setTimeout(resolve, ms)); function repoCloneLockPath(repoPath: string): string { @@ -153,26 +160,50 @@ function repoCloneLockPath(repoPath: string): string { } /** - * 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 { diff --git a/test/unit/miner-repo-clone-lock.test.ts b/test/unit/miner-repo-clone-lock.test.ts index 64c8d94a30..e2bb88680b 100644 --- a/test/unit/miner-repo-clone-lock.test.ts +++ b/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(