diff --git a/packages/loopover-miner/lib/repo-clone.d.ts b/packages/loopover-miner/lib/repo-clone.d.ts index 8701705d91..0ed6d77d08 100644 --- a/packages/loopover-miner/lib/repo-clone.d.ts +++ b/packages/loopover-miner/lib/repo-clone.d.ts @@ -12,6 +12,29 @@ export type EnsureRepoClonedResult = { ok: boolean; repoPath: string; error?: st export type RunGitFn = (args: string[], cwd: string, timeoutMs: number) => Promise<{ ok: boolean; stdout: string; stderr: string }>; +export function parseRepoCloneLock(raw: string): { acquiredAtMs: number; pid: number | null } | null; + +export function isRepoCloneLockStale(info: { acquiredAtMs: number } | null, nowMs: number, staleMs: number): boolean; + +export type RepoCloneLockFs = { + open: (path: string) => number; + write: (fd: number, data: string) => void; + close: (fd: number) => void; + read: (path: string) => string; + unlink: (path: string) => void; +}; + +export type RepoCloneLockOptions = { + timeoutMs?: number; + staleMs?: number; + pollMs?: number; + now?: () => number; + sleep?: (ms: number) => Promise; + fs?: RepoCloneLockFs; +}; + +export function acquireRepoCloneLock(lockPath: string, options?: RepoCloneLockOptions): Promise<() => void>; + export function ensureRepoCloned( repoFullName: string, options?: { @@ -21,5 +44,6 @@ export function ensureRepoCloned( timeoutMs?: number; remoteUrl?: string; runGit?: RunGitFn; + lock?: RepoCloneLockOptions; }, ): Promise; diff --git a/packages/loopover-miner/lib/repo-clone.js b/packages/loopover-miner/lib/repo-clone.js index 9dd60197d8..258cefa87f 100644 --- a/packages/loopover-miner/lib/repo-clone.js +++ b/packages/loopover-miner/lib/repo-clone.js @@ -1,8 +1,9 @@ import { execFile } from "node:child_process"; -import { existsSync, mkdirSync } from "node:fs"; +import { closeSync, existsSync, mkdirSync, openSync, readFileSync, unlinkSync, writeSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; import { promisify } from "node:util"; +import { registerCleanupResource } from "./process-lifecycle.js"; // Per-repo base-clone cache (#5132, Wave 3.5 follow-up). packages/loopover-engine/src/miner/ // worktree-allocator.ts's real `addWorktree` primitive (git worktree add -b ) @@ -103,16 +104,162 @@ async function withRepoCloneLock(repoPath, fn) { } } +// Cross-process serialization for ensureRepoCloned (#7084). The in-process `repoCloneLocks` Map above only +// serializes calls inside ONE Node process's event loop. DEPLOYMENT.md documents "fleet mode" -- multiple +// container processes sharing the SAME bind-mounted clone volume -- and claim-ledger.js already treats +// "two sibling miner processes racing the same repo" as a real concurrency model. Two fleet workers hitting +// the same repoPath each start with their own empty Map, so #6762's guard does nothing between them: both can +// run `git checkout`/`reset --hard` on the shared working tree at once and corrupt it / trip .git/index.lock. +// An OS-level exclusive lockfile (`open(path, 'wx')` -- an atomic create-or-fail) closes that gap: it is +// visible across processes because the filesystem, not process memory, arbitrates it. The lock records the +// acquiring process's wall-clock so a lock left behind by a crashed process self-expires after `staleMs` +// (mirroring worktree-allocator.js's age-based orphan reclaim), and it is registered as a cleanup resource so +// process-lifecycle.js's SIGINT/SIGTERM handler removes it on a clean exit (mirroring local-store.js). +const DEFAULT_LOCK_TIMEOUT_MS = 10 * 60_000; +const DEFAULT_LOCK_STALE_MS = 15 * 60_000; +const DEFAULT_LOCK_POLL_MS = 250; + +function defaultLockSleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +// Real filesystem primitives behind the lock, grouped so tests can inject a deterministic double instead of +// racing real timing. `open` uses the "wx" flag (O_CREAT | O_EXCL): it throws EEXIST when the file already +// exists, which is exactly the atomic "someone else holds the lock" signal. +const defaultLockFs = { + open: (path) => openSync(path, "wx"), + write: (fd, data) => writeSync(fd, data), + close: (fd) => closeSync(fd), + read: (path) => readFileSync(path, "utf8"), + unlink: (path) => unlinkSync(path), +}; + +/** + * Parse a lockfile's JSON payload back into `{ acquiredAtMs, pid }`, or null when the content is missing, + * corrupt, or lacks a finite timestamp (all of which mean "no trustworthy owner", so the caller treats it as + * reclaimable). Exported so the staleness decision is unit-testable without touching the filesystem. + * + * @param {string} raw + * @returns {{ acquiredAtMs: number, pid: number | null } | null} + */ +export function parseRepoCloneLock(raw) { + try { + const parsed = JSON.parse(raw); + const acquiredAtMs = Number(parsed?.acquiredAtMs); + if (!Number.isFinite(acquiredAtMs)) return null; + return { acquiredAtMs, pid: Number.isFinite(Number(parsed?.pid)) ? Number(parsed.pid) : null }; + } catch { + return null; + } +} + +/** + * Decide whether an existing lock may be reclaimed: an unparseable lock (crash mid-write, corruption) is always + * reclaimable, and a parseable one is reclaimable once it is older than `staleMs` -- the self-expiry that stops + * a crashed holder from wedging every future attempt against that repo forever. + * + * @param {{ acquiredAtMs: number } | null} info + * @param {number} nowMs + * @param {number} staleMs + * @returns {boolean} + */ +export function isRepoCloneLockStale(info, nowMs, staleMs) { + if (info === null) return true; + return nowMs - info.acquiredAtMs > staleMs; +} + +function readExistingLock(fs, lockPath) { + try { + return parseRepoCloneLock(fs.read(lockPath)); + } catch { + // The holder released between our failed `open` and this `read` (TOCTOU): the lock is gone, so it is + // trivially reclaimable -- fall through to the unlink (a no-op) and retry the `open`. + return null; + } +} + +function forceUnlinkLock(fs, lockPath) { + try { + fs.unlink(lockPath); + } catch (error) { + // Another waiter reclaimed the same stale lock first; that is the outcome we wanted, so ignore ENOENT and + // surface anything else (e.g. EPERM) rather than looping on an unremovable lock. + if (error?.code !== "ENOENT") throw error; + } +} + +function createLockRelease(fs, lockPath, fd) { + let released = false; + const release = () => { + if (released) return; + released = true; + unregister(); + fs.close(fd); + forceUnlinkLock(fs, lockPath); + }; + // Registered so process-lifecycle.js's SIGINT/SIGTERM/crash handler drops the lockfile on a clean shutdown + // instead of leaving it for `staleMs` to expire; `release` unregisters itself so the happy path never + // double-frees. + const unregister = registerCleanupResource(release); + return release; +} + +/** + * Take an OS-level exclusive lock on `lockPath`, blocking (bounded by `timeoutMs`) until a holder in ANOTHER + * process releases it, reclaiming a stale/orphaned lock left by a crashed process, and failing closed with a + * `repo_clone_lock_timeout` error if it cannot be acquired in time. Returns an idempotent release function. + * Every dependency (`fs`, `now`, `sleep`, the timeouts) is injectable so all branches are deterministically + * testable without real cross-process timing. + * + * @param {string} lockPath + * @param {{ + * timeoutMs?: number, staleMs?: number, pollMs?: number, now?: () => number, sleep?: (ms: number) => Promise, + * fs?: { open: (path: string) => number, write: (fd: number, data: string) => void, close: (fd: number) => void, read: (path: string) => string, unlink: (path: string) => void }, + * }} [options] + * @returns {Promise<() => void>} + */ +export async function acquireRepoCloneLock(lockPath, options = {}) { + const timeoutMs = Number.isFinite(options.timeoutMs) ? options.timeoutMs : DEFAULT_LOCK_TIMEOUT_MS; + const staleMs = Number.isFinite(options.staleMs) ? options.staleMs : DEFAULT_LOCK_STALE_MS; + const pollMs = Number.isFinite(options.pollMs) ? options.pollMs : DEFAULT_LOCK_POLL_MS; + const now = typeof options.now === "function" ? options.now : Date.now; + const sleep = typeof options.sleep === "function" ? options.sleep : defaultLockSleep; + const fs = options.fs ?? defaultLockFs; + + const deadline = now() + timeoutMs; + for (;;) { + let fd; + try { + fd = fs.open(lockPath); + } catch (error) { + if (error?.code !== "EEXIST") throw error; + } + if (fd !== undefined) { + fs.write(fd, JSON.stringify({ pid: process.pid, acquiredAtMs: now() })); + return createLockRelease(fs, lockPath, fd); + } + if (isRepoCloneLockStale(readExistingLock(fs, lockPath), now(), staleMs)) { + forceUnlinkLock(fs, lockPath); + continue; + } + if (now() >= deadline) throw new Error("repo_clone_lock_timeout"); + await sleep(pollMs); + } +} + /** * Serialize the git mutations of {@link ensureRepoClonedUnlocked} per resolved repo path so concurrent - * same-repo attempts never race the shared base clone (#6762), while different repos still run in parallel. - * Resolves the same `repoPath` the unlocked step computes and uses it as the mutex key; throws (before - * locking) on a malformed `repoFullName`, matching the prior behaviour. + * same-repo attempts never race the shared base clone -- both WITHIN one Node process (the in-process + * `repoCloneLocks` Map, #6762) and ACROSS separate OS processes such as fleet-mode's sibling containers (an + * OS-level exclusive lockfile, #7084), while different repos still run fully in parallel. Resolves the same + * `repoPath` the unlocked step computes and uses it as both the mutex key and the lockfile location; throws + * (before locking) on a malformed `repoFullName`, matching the prior behaviour. * * @param {string} repoFullName * @param {{ * baseBranch?: string, cloneBaseDir?: string, env?: Record, timeoutMs?: number, * remoteUrl?: string, runGit?: (args: string[], cwd: string, timeoutMs: number) => Promise<{ ok: boolean, stdout: string, stderr: string }>, + * lock?: { timeoutMs?: number, staleMs?: number, pollMs?: number, now?: () => number, sleep?: (ms: number) => Promise, fs?: object }, * }} [options] * @returns {Promise<{ ok: boolean, repoPath: string, error?: string }>} */ @@ -120,7 +267,26 @@ export async function ensureRepoCloned(repoFullName, options = {}) { const target = normalizeRepoFullName(repoFullName); const cloneBaseDir = typeof options.cloneBaseDir === "string" && options.cloneBaseDir.trim() ? options.cloneBaseDir.trim() : resolveRepoCloneBaseDir(options.env); const repoPath = join(cloneBaseDir, target.owner, target.repo); - return withRepoCloneLock(repoPath, () => ensureRepoClonedUnlocked(repoFullName, options)); + const lockPath = `${repoPath}.clone.lock`; + return withRepoCloneLock(repoPath, async () => { + // The lockfile lives beside the clone, so its parent (cloneBaseDir/owner) must exist before `open` runs -- + // on a first-ever clone that directory hasn't been created yet. + mkdirSync(join(cloneBaseDir, target.owner), { recursive: true, mode: 0o700 }); + let releaseLock; + try { + releaseLock = await acquireRepoCloneLock(lockPath, options.lock ?? {}); + } catch (error) { + // A lock-acquire failure (timeout on a busy lock, or an fs error taking it) is an operational failure, + // not a programmer error: surface it via the same { ok:false, error } contract as a git failure so the + // caller (attempt-worktree.js) fails the attempt closed instead of throwing an unhandled rejection. + return { ok: false, repoPath, error: error instanceof Error ? error.message : String(error) }; + } + try { + return await ensureRepoClonedUnlocked(repoFullName, options); + } finally { + releaseLock(); + } + }); } /** diff --git a/test/unit/miner-repo-clone.test.ts b/test/unit/miner-repo-clone.test.ts index 9ede31c80a..8bfd2deff7 100644 --- a/test/unit/miner-repo-clone.test.ts +++ b/test/unit/miner-repo-clone.test.ts @@ -1,9 +1,9 @@ import { execFileSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { ensureRepoCloned, resolveRepoCloneBaseDir, resolveRepoCloneDir } from "../../packages/loopover-miner/lib/repo-clone.js"; +import { acquireRepoCloneLock, ensureRepoCloned, isRepoCloneLockStale, parseRepoCloneLock, resolveRepoCloneBaseDir, resolveRepoCloneDir } from "../../packages/loopover-miner/lib/repo-clone.js"; const roots: string[] = []; @@ -308,3 +308,271 @@ describe("ensureRepoCloned per-repo concurrency guard (#6762)", () => { expect((await ensureRepoCloned("acme/widgets", { cloneBaseDir, runGit: failOn("reset", "boom-reset") })).error).toBe("boom-reset"); }); }); + +type LockFs = { + open: (path: string) => number; + write: (fd: number, data: string) => void; + close: (fd: number) => void; + read: (path: string) => string; + unlink: (path: string) => void; +}; + +/** A filesystem error carrying a `.code`, as Node's fs throws (EEXIST / ENOENT / EPERM / ...). */ +function codedError(code: string, message = code): NodeJS.ErrnoException { + const error = new Error(message) as NodeJS.ErrnoException; + error.code = code; + return error; +} + +describe("parseRepoCloneLock / isRepoCloneLockStale (#7084)", () => { + it("parses a well-formed lock payload, keeping a finite pid", () => { + expect(parseRepoCloneLock(JSON.stringify({ acquiredAtMs: 42, pid: 1234 }))).toEqual({ acquiredAtMs: 42, pid: 1234 }); + }); + + it("keeps the timestamp but nulls a missing/non-numeric pid", () => { + expect(parseRepoCloneLock(JSON.stringify({ acquiredAtMs: 42 }))).toEqual({ acquiredAtMs: 42, pid: null }); + expect(parseRepoCloneLock(JSON.stringify({ acquiredAtMs: 42, pid: "nope" }))).toEqual({ acquiredAtMs: 42, pid: null }); + }); + + it("returns null when the timestamp is absent/non-finite or the JSON is corrupt", () => { + expect(parseRepoCloneLock(JSON.stringify({ pid: 1 }))).toBeNull(); + expect(parseRepoCloneLock("{ not json")).toBeNull(); + }); + + it("treats a null (unparseable/vanished) lock as always stale", () => { + expect(isRepoCloneLockStale(null, 1000, 500)).toBe(true); + }); + + it("treats a parseable lock as stale only once it is older than staleMs", () => { + expect(isRepoCloneLockStale({ acquiredAtMs: 0 }, 1000, 500)).toBe(true); + expect(isRepoCloneLockStale({ acquiredAtMs: 600 }, 1000, 500)).toBe(false); + }); +}); + +describe("acquireRepoCloneLock (#7084)", () => { + it("acquires immediately when the lockfile does not yet exist, writing an owner record", () => { + // Real fs against a temp path: proves the happy path creates the file and the release removes it. + const root = tempRoot("loopover-miner-repo-clone-lock-fresh-"); + const lockPath = join(root, "repo.clone.lock"); + return acquireRepoCloneLock(lockPath).then((release) => { + const held = parseRepoCloneLock(readFileSync(lockPath, "utf8")); + expect(held?.pid).toBe(process.pid); + expect(existsSync(lockPath)).toBe(true); + release(); + expect(existsSync(lockPath)).toBe(false); + }); + }); + + it("rethrows an open error that is not EEXIST", async () => { + const fs: LockFs = { open: () => { throw codedError("EACCES", "denied"); }, write: () => {}, close: () => {}, read: () => "", unlink: () => {} }; + await expect(acquireRepoCloneLock("/whatever.lock", { fs })).rejects.toThrow("denied"); + }); + + it("reclaims a stale lock (age > staleMs) then acquires; release is idempotent", async () => { + let opens = 0; + let closes = 0; + const unlinked: string[] = []; + const fs: LockFs = { + open: () => { + opens += 1; + if (opens === 1) throw codedError("EEXIST"); + return 7; + }, + write: () => {}, + close: () => { + closes += 1; + }, + read: () => JSON.stringify({ pid: 999, acquiredAtMs: 0 }), + unlink: (path) => { + unlinked.push(path); + }, + }; + const release = await acquireRepoCloneLock("/x.clone.lock", { fs, now: () => 1_000_000, staleMs: 100 }); + // The stale lock was removed before the second open succeeded. + expect(unlinked).toEqual(["/x.clone.lock"]); + release(); + release(); // idempotent: the guard short-circuits, so close/unlink run exactly once. + expect(closes).toBe(1); + expect(unlinked).toEqual(["/x.clone.lock", "/x.clone.lock"]); + }); + + it("waits (sleeps) while a fresh lock is held, then acquires once it is gone", async () => { + let opens = 0; + let sleeps = 0; + const fs: LockFs = { + open: () => { + opens += 1; + if (opens <= 2) throw codedError("EEXIST"); + return 9; + }, + write: () => {}, + close: () => {}, + read: () => JSON.stringify({ pid: 1, acquiredAtMs: 100 }), + unlink: () => {}, + }; + const release = await acquireRepoCloneLock("/held.clone.lock", { + fs, + now: () => 100, // never advances past the deadline, so the loop keeps waiting rather than timing out + timeoutMs: 10_000, + staleMs: 10_000, + pollMs: 5, + sleep: async () => { + sleeps += 1; + }, + }); + expect(sleeps).toBe(2); + release(); + }); + + it("fails closed with repo_clone_lock_timeout when a fresh lock outlives the deadline", async () => { + const fs: LockFs = { + open: () => { + throw codedError("EEXIST"); + }, + write: () => {}, + close: () => {}, + read: () => JSON.stringify({ pid: 1, acquiredAtMs: 0 }), + unlink: () => {}, + }; + // timeoutMs 0 => deadline == start; the not-stale lock is never reclaimable, so the first pass throws. + await expect(acquireRepoCloneLock("/busy.clone.lock", { fs, now: () => 0, timeoutMs: 0, staleMs: 10_000_000 })).rejects.toThrow("repo_clone_lock_timeout"); + }); + + it("treats a lock whose content vanished mid-check (read throws) as reclaimable", async () => { + let opens = 0; + const unlinked: string[] = []; + const fs: LockFs = { + open: () => { + opens += 1; + if (opens === 1) throw codedError("EEXIST"); + return 4; + }, + write: () => {}, + close: () => {}, + read: () => { + throw codedError("ENOENT", "gone"); + }, + unlink: (path) => { + unlinked.push(path); + }, + }; + const release = await acquireRepoCloneLock("/vanished.clone.lock", { fs }); + expect(unlinked).toEqual(["/vanished.clone.lock"]); + release(); + }); + + it("ignores ENOENT from the reclaim unlink (another waiter won the race)", async () => { + let opens = 0; + const fs: LockFs = { + open: () => { + opens += 1; + if (opens === 1) throw codedError("EEXIST"); + return 2; + }, + write: () => {}, + close: () => {}, + read: () => JSON.stringify({ acquiredAtMs: 0 }), + unlink: () => { + throw codedError("ENOENT"); + }, + }; + const release = await acquireRepoCloneLock("/raced.clone.lock", { fs, now: () => 10_000, staleMs: 1 }); + expect(typeof release).toBe("function"); + release(); + }); + + it("surfaces a non-ENOENT unlink error while reclaiming a stale lock", async () => { + const fs: LockFs = { + open: () => { + throw codedError("EEXIST"); + }, + write: () => {}, + close: () => {}, + read: () => "corrupt-not-json", // unparseable => stale => reclaim attempt => unlink throws EPERM + unlink: () => { + throw codedError("EPERM", "operation not permitted"); + }, + }; + await expect(acquireRepoCloneLock("/wedged.clone.lock", { fs, staleMs: 0 })).rejects.toThrow("operation not permitted"); + }); +}); + +describe("ensureRepoCloned cross-process lock (#7084)", () => { + it("REGRESSION: a real on-disk lock blocks a second independent acquirer until the first releases", async () => { + // The two acquirers share NOTHING but the lockfile itself -- exactly the fleet-mode "sibling containers on a + // shared volume" model the in-process Map cannot serialize. The second must not acquire while the first holds. + const root = tempRoot("loopover-miner-repo-clone-xproc-"); + const lockPath = join(root, "repo.clone.lock"); + const first = await acquireRepoCloneLock(lockPath); + + let secondAcquired = false; + const secondPromise = acquireRepoCloneLock(lockPath, { pollMs: 5 }).then((release) => { + secondAcquired = true; + return release; + }); + + await new Promise((resolve) => setTimeout(resolve, 40)); + expect(secondAcquired).toBe(false); // still blocked on the real lockfile + + first(); // first process releases + const secondRelease = await secondPromise; + expect(secondAcquired).toBe(true); + secondRelease(); + }, 60000); + + it("returns ok:false with repo_clone_lock_timeout (not a throw) when the cross-process lock can't be taken", async () => { + // A lock held by a live foreign process that never releases: ensureRepoCloned must fail closed via the + // { ok:false, error } contract so attempt-worktree.js can mark the attempt failed rather than crash. + const root = tempRoot("loopover-miner-repo-clone-locktimeout-"); + const cloneBaseDir = join(root, "cache"); + const heldFs: LockFs = { + open: () => { + throw codedError("EEXIST"); + }, + write: () => {}, + close: () => {}, + read: () => JSON.stringify({ pid: 1, acquiredAtMs: 0 }), + unlink: () => {}, + }; + const result = await ensureRepoCloned("acme/widgets", { + cloneBaseDir, + remoteUrl: "unused", + lock: { fs: heldFs, now: () => 0, timeoutMs: 0, staleMs: 10_000_000 }, + }); + expect(result.ok).toBe(false); + expect(result.error).toBe("repo_clone_lock_timeout"); + expect(result.repoPath).toBe(join(cloneBaseDir, "acme", "widgets")); + }); + + it("stringifies a non-Error lock failure into the error field", async () => { + // The lock fs throws a bare string (not an Error); the { ok:false } contract still surfaces it as a string. + const root = tempRoot("loopover-miner-repo-clone-locknonerror-"); + const cloneBaseDir = join(root, "cache"); + const nonErrorFs: LockFs = { + open: () => { + throw "lock-subsystem-exploded"; // eslint-disable-line no-throw-literal + }, + write: () => {}, + close: () => {}, + read: () => "", + unlink: () => {}, + }; + const result = await ensureRepoCloned("acme/widgets", { cloneBaseDir, remoteUrl: "unused", lock: { fs: nonErrorFs } }); + expect(result.ok).toBe(false); + expect(result.error).toBe("lock-subsystem-exploded"); + }); + + it("proceeds past a stale orphaned clone lock left by a crashed process", async () => { + // A lockfile with an ancient timestamp simulates a process that died mid-clone; a subsequent call must + // reclaim it and clone rather than wedging forever. + const root = tempRoot("loopover-miner-repo-clone-staleclone-"); + const originPath = initOriginRepo(root); + const cloneBaseDir = join(root, "cache"); + mkdirSync(join(cloneBaseDir, "acme"), { recursive: true }); + writeFileSync(join(cloneBaseDir, "acme", "widgets.clone.lock"), JSON.stringify({ pid: 999999, acquiredAtMs: 0 })); + + const result = await ensureRepoCloned("acme/widgets", { cloneBaseDir, remoteUrl: originPath, lock: { staleMs: 1 } }); + expect(result.ok).toBe(true); + expect(readFileSync(join(result.repoPath, "README.md"), "utf8")).toBe("hello\n"); + }, 60000); +});