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(