Skip to content

fix(miner): enforce maxConcurrentClaims atomically across sibling processes - #6912

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
e11734937-beep:fix-6758-atomic-claim-cap
Jul 17, 2026
Merged

fix(miner): enforce maxConcurrentClaims atomically across sibling processes#6912
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
e11734937-beep:fix-6758-atomic-claim-cap

Conversation

@e11734937-beep

Copy link
Copy Markdown
Contributor

Context

maxConcurrentClaims was enforced as check-then-act: attempt-cli.js read the active-claim count via claimLedger.listActiveClaims(...), then recorded the claim in a separate claimLedger.claimIssue(...) call with no shared transaction. claimIssue only did sweepExpiredClaims + recordClaim, and recordClaim is atomic only per-issue-uniqueness (INSERT ... ON CONFLICT), never a repo-wide cap. Two sibling miner processes racing the same repo could both read the same sub-cap count before either committed, both claim, and exceed the cap.

Fix

  • claim-ledger.js: new claimIssueWithinCap(repoFullName, issueNumber, note, apiBaseUrl, maxConcurrentClaims) that sweeps orphaned claims, counts the repo active claims, and records the new claim only while strictly under the cap, all inside one BEGIN IMMEDIATE transaction. With node:sqlite shared busy_timeout, the loser waits for the winner commit, sees the committed count, and is cleanly rejected with claimed: false (never silently dropped) so the caller still logs the cap violation.
  • attempt-cli.js: calls the atomic count-and-claim instead of the split listActiveClaims pre-check + separate claimIssue. The blocked path (log/event/JSON/human message/exit 11) is byte-for-byte unchanged; only its activeClaimCount source moved to the atomic result.

Tests

  • miner-claim-ledger.test.ts: under-cap success, at-cap rejection, per-repo scoping, two sibling connections to one DB file racing the cap (only one wins), sweep-frees-a-slot inside the transaction, invalid-cap rejection, and transaction rollback on a mid-transaction throw.
  • miner-attempt-cli.test.ts: existing cap-enforcement cases updated to the atomic entry point.

100% patch line + branch coverage on the changed source.

Closes #6758

…cesses

The cap was check-then-act: attempt-cli.js read the active-claim count via
listActiveClaims, then recorded the claim in a separate claimLedger.claimIssue
call with no shared transaction. Two sibling miner processes racing the same
repo could both pass the sub-cap count check before either committed, both
claim, and exceed maxConcurrentClaims.

Add claimIssueWithinCap to the claim ledger: it sweeps orphaned claims, counts
the repo active claims, and records the new claim only while under the cap, all
inside one BEGIN IMMEDIATE transaction. With node:sqlite shared busy_timeout the
loser waits for the winner commit, sees the committed count, and is cleanly
rejected (claimed: false) so the caller still logs the cap violation.
attempt-cli.js now calls this instead of the split check-then-claim.

Closes JSONbored#6758
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.70%. Comparing base (64abfab) to head (36e1558).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6912   +/-   ##
=======================================
  Coverage   93.70%   93.70%           
=======================================
  Files         685      685           
  Lines       68313    68331   +18     
  Branches    18723    18723           
=======================================
+ Hits        64014    64032   +18     
  Misses       3302     3302           
  Partials      997      997           
Flag Coverage Δ
shard-1 43.60% <4.76%> (-0.07%) ⬇️
shard-2 36.81% <85.71%> (+0.03%) ⬆️
shard-3 32.57% <85.71%> (-0.08%) ⬇️
shard-4 34.40% <0.00%> (-0.43%) ⬇️
shard-5 31.63% <4.76%> (-0.29%) ⬇️
shard-6 45.87% <4.76%> (+0.39%) ⬆️

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

Files with missing lines Coverage Δ
packages/loopover-miner/lib/attempt-cli.js 98.08% <100.00%> (ø)
packages/loopover-miner/lib/claim-ledger.js 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 17, 2026
@loopover-orb

loopover-orb Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-17 12:39:13 UTC

5 files · 1 AI reviewer · no blockers · readiness 88/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR closes the check-then-act race in maxConcurrentClaims by fusing the count and the insert into a single BEGIN IMMEDIATE transaction (claimIssueWithinCap), replacing the split listActiveClaims-then-claimIssue pattern in attempt-cli.js. The logic is sound: the cap check happens inside the same transaction as the insert, so a racing sibling connection blocks on the write lock and sees the committed count, and the rejected-claim path correctly commits (not rolls back) so a sweep-expired claim's transition persists. Tests include a genuine two-connection race against a shared DB file, which is the right way to prove this, and the blocked-path JSON/log output is preserved byte-for-byte per the description.

Nits — 5 non-blocking
  • packages/loopover-miner/lib/claim-ledger.js:255 (countActiveRepoStatement) — confirm this doesn't need api_base_url scoping long-term; the comment explains it matches prior behavior, but if cross-forge same-name repos become common this could undercount/overcount, worth a follow-up issue rather than blocking here.
  • The external brief's 'magic number 6758' flag is just an issue-number reference in a comment, not a real magic-number smell — not actionable.
  • attempt-cli.js is flagged as exceeding a 400-line file-size threshold, but the diff itself doesn't add much net length here; consider a follow-up split if the file keeps growing.
  • Consider extracting the BEGIN IMMEDIATE/COMMIT/ROLLBACK pattern in claim-ledger.js:255-282 into a small transaction helper if more atomic multi-statement operations get added later, to avoid repeating the try/catch/rollback boilerplate.
  • claim-ledger.d.ts's ClaimWithinCapResult discriminated union is a nice touch for callers — worth the same treatment for claimIssue's return type if similar ambiguity ever arises there.

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 #6758
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High 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: 60 registered-repo PR(s), 35 merged, 2 issue(s).
Contributor context ✅ Confirmed Gittensor contributor e11734937-beep; Gittensor profile; 60 PR(s), 2 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The diff replaces the check-then-act pattern with a new claimIssueWithinCap that fuses the count and insert inside a single BEGIN IMMEDIATE transaction in claim-ledger.js, and attempt-cli.js now calls this atomic method and still logs/reports the cap violation via the existing blocked-path logic. A regression test opens two sibling DatabaseSync connections to the same DB file and asserts only one

Review context
  • Author: e11734937-beep
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript
  • Official Gittensor activity: 60 PR(s), 2 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; 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

@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 b1a2bbd into JSONbored:main Jul 17, 2026
16 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.

maxConcurrentClaims cap is check-then-act, not atomic, across sibling miner processes

1 participant