Skip to content

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

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9681-clone-lock-toctou
Jul 29, 2026
Merged

fix(miner): close the clone-lock TOCTOU on a still-empty lockfile (#9681)#9915
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9681-clone-lock-toctou

Conversation

@kai392

@kai392 kai392 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What

acquireRepoCloneLock creates the lockfile empty (openSync(.., 'wx') → 0-byte) and writes the owner record several statements later. isRepoCloneLockStale treated that intermediate state as immediately reclaimable — its catch (unparseable JSON) and !meta arms returned true unconditionally. 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, and its own open 'wx' now succeeds. P1's later writeSync lands in an already-unlinked inode, and both processes run git fetch/checkout/reset --hard on the same .git dir — precisely the 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.

How

  • 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 exactly 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; the docstring no longer describes an unparseable lock as unconditionally stale.

Tests

  • Unparseable / non-object locks are not stale within the grace window but are past it; a throwing stat is stale.
  • acquireRepoCloneLock polls a fresh empty peer lock (within grace) instead of reclaiming it (the double-hold path is closed).
  • It still reclaims an over-grace empty lock through the real default statSync path.

The reclaim/poll assertions fail against the current code. All existing isRepoCloneLockStale/acquireRepoCloneLock tests still pass.

Closes #9681

@kai392
kai392 requested a review from JSONbored as a code owner July 29, 2026 22:22
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-29 22:29:09 UTC

2 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This closes a real TOCTOU: an empty/unparseable clone lockfile (the normal 0-byte gap between openSync 'wx' and the owner-record write) was previously always treated as stale, letting a concurrent process unlink and re-acquire a lock its peer still legitimately holds, causing a double-hold and concurrent git mutation on the same .git dir. The fix introduces a grace window (`DEFAULT_LOCK_INCOMPLETE_GRACE_MS`, 5s) driven by the lockfile's own mtime via an injectable `statLock` seam, threaded consistently from `acquireRepoCloneLock` through to `isRepoCloneLockStale`, and the logic (mtime-based grace, throw-on-vanish means reclaimable) is sound and traced correctly to the source of the race. Tests cover both arms of the grace boundary, the poll-instead-of-reclaim path, and the real default statSync path, which is solid coverage of the new branches.

Nits — 4 non-blocking
  • packages/loopover-miner/lib/repo-clone.ts:229 — the two `isIncompleteLockReclaimable` call sites are functionally identical fallbacks (unparseable JSON vs non-object); consider collapsing them into a single guard early in `isRepoCloneLockStale` for a touch less duplication (purely stylistic).
  • test/unit/miner-repo-clone-lock.test.ts — the grace-boundary tests use `#` comments as the primary explanation of intent rather than descriptive assertions/variable names; fine as-is, but a helper like `withinGrace`/`pastGrace` booleans could make the boundary case more self-evident at a glance.
  • Consider extracting a small named constant/comment for the '5s well past microseconds, well under 15min stale' rationale directly next to `DEFAULT_LOCK_INCOMPLETE_GRACE_MS` if not already fully covered by the existing comment (it is, so this is optional).
  • No functional changes needed; the diff is well-scoped to the TOCTOU fix and its regression tests.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9681
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 95 registered-repo PR(s), 50 merged, 7 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kai392; Gittensor profile; 95 PR(s), 7 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The diff exports DEFAULT_LOCK_INCOMPLETE_GRACE_MS=5_000, adds the statLock seam threaded through acquireRepoCloneLock into isRepoCloneLockStale exactly as required, gates the catch/non-object arms on mtimeMs vs the grace window, returns true when stat throws, updates the docstring, and adds named regression tests covering within-grace/past-grace/non-object/stat-throws plus an acquisition-level pol

Review context
  • Author: kai392
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Cuda, JavaScript, Kotlin, MDX, Perl, Ruby, TypeScript
  • Official Gittensor activity: 95 PR(s), 7 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

…ONbored#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 JSONbored#9681

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.56%. Comparing base (84553fc) to head (9d92a19).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9915       +/-   ##
===========================================
- Coverage   91.86%   79.56%   -12.30%     
===========================================
  Files         921      282      -639     
  Lines      113256    58715    -54541     
  Branches    27300     8683    -18617     
===========================================
- Hits       104039    46719    -57320     
- Misses       7929    11706     +3777     
+ Partials     1288      290      -998     
Flag Coverage Δ
backend 100.00% <100.00%> (+4.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-miner/lib/repo-clone.ts 100.00% <100.00%> (ø)

... and 772 files with indirect coverage changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 1e1b004 into JSONbored:main Jul 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants