Skip to content

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

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

acquireRepoCloneLock in packages/loopover-miner/lib/repo-clone.ts is the cross-process lock that exists specifically because fleet mode runs multiple separate OS processes against one bind-mounted clone volume (module comment, lines 137-145): "create-and-hold is atomic across processes, so exactly one holder mutates the clone".

That guarantee does not hold. The lockfile is created empty (openSync(path, "wx", 0o600) produces a 0-byte file) and the owner record is only written several statements later via writeLock(fd, JSON.stringify({ pid, host, at, token })).

isRepoCloneLockStale treats exactly that intermediate state as reclaimable:

try {
  meta = JSON.parse(readFileSync(lockPath, "utf8"));
} catch {
  return true;
}

So a genuinely concurrent process P2 that hits EEXIST while P1 is between openSync and writeSync reads "", JSON.parse throws, P2 declares the lock stale, unlinkSyncs it, continues, and its own openSync(..., "wx") now succeeds. P1's later writeSync lands in an already-unlinked inode. Both processes then run git fetch / git checkout / git reset --hard concurrently on the same .git directory — precisely the index/HEAD/refs corruption the lock was added to prevent. The per-acquire token does not help: it only stops release() from deleting a peer's lock, not double-holding.

The docstring at lines 156-159 calls the unparseable case "a crash mid-write", but it is also the normal mid-acquire state of every successful acquisition, so the stale heuristic fires on live holders.

Requirements

  • An unparseable/empty lockfile must no longer be reclaimable immediately. isRepoCloneLockStale must, in its catch arm (and its !meta || typeof meta !== "object" arm), consult the lockfile's own mtimeMs and return true only when nowMs - mtimeMs exceeds a new exported constant DEFAULT_LOCK_INCOMPLETE_GRACE_MS, and false otherwise.
  • DEFAULT_LOCK_INCOMPLETE_GRACE_MS must be exported from repo-clone.ts alongside DEFAULT_LOCK_STALE_MS and set to 5_000.
  • The stat call must go through a new optional injectable seam on RepoCloneLockOptions (statLock?: (path: string) => { mtimeMs: number }, defaulting to node:fs's statSync), threaded from acquireRepoCloneLock into isRepoCloneLockStale exactly as the existing isProcessAlive, openLock, and writeLock seams are.
  • A statSync that itself throws (the lockfile vanished between the read and the stat) must return true — nothing is there to hold.
  • The same-host-live-pid and the age-backstop branches must be unchanged.
  • The docstring at lines 156-163 must be updated so it no longer describes an unparseable lock as unconditionally stale.

⚠️ Required pattern: add the grace window as a third injectable-clock-driven branch inside the existing isRepoCloneLockStale, mirroring how staleMs + nowMs already drive the age backstop at lines 184-186. It does NOT satisfy this issue to rewrite the acquisition to a temp-file + linkSync scheme (that removes the openLock/writeLock seams the tests depend on), to hold a second lock, to retry the JSON read in a loop, or to leave the grace window as a bare inline magic number instead of an exported constant.

Deliverables

  • repo-clone.ts exports DEFAULT_LOCK_INCOMPLETE_GRACE_MS = 5_000 and isRepoCloneLockStale returns false for an unparseable/non-object lockfile whose mtimeMs is within that window of nowMs.
  • RepoCloneLockOptions gains an injectable statLock seam, defaulted to statSync, threaded from acquireRepoCloneLock.
  • isRepoCloneLockStale returns true when the stat throws.
  • The docstring above isRepoCloneLockStale no longer states that an unreadable/partial lock is stale unconditionally.
  • A new named regression test in test/unit/miner-repo-clone.test.ts asserts that an empty lockfile whose mtimeMs is nowMs is NOT stale, and that the same file at nowMs - 6_000 IS stale.
  • A second new test drives acquireRepoCloneLock with an injected openLock that simulates a peer's EEXIST against a freshly-created empty lock and asserts the caller waits (polls) rather than unlinking it.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example adding the grace window without the acquisition-level test, which proves the double-hold path is actually closed — does not resolve this issue.

Test Coverage Requirements

packages/loopover-miner/lib/**/*.ts IS inside Codecov's coverage.include in vitest.config.ts, so the 99%+ branch-counted codecov/patch gate applies exactly as for src/**. Every arm of the new logic needs a test: within-grace (not stale), past-grace (stale), non-object JSON within grace, and stat throwing. The fix needs named regression tests that fail against the current code.

Expected Outcome

Two miner processes sharing a clone volume can no longer both hold the same repo's clone lock during the window between lock creation and owner-record write; the module's stated "exactly one holder mutates the clone" guarantee holds in fleet mode.

Links & Resources

packages/loopover-miner/lib/repo-clone.ts:137-145, :156-187, :196-264, test/unit/miner-repo-clone.test.ts.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions