Skip to content

fix(miner): expire claims orphaned by a dead process at claim time - #6441

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/miner-claim-ledger-sweep
Jul 16, 2026
Merged

fix(miner): expire claims orphaned by a dead process at claim time#6441
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/miner-claim-ledger-sweep

Conversation

@luciferlive112116

Copy link
Copy Markdown
Contributor

Summary

sweepExpiredClaims/findExpiredClaims worked but had zero callers, so no claim ever expired: a claim left behind by a crashed or killed miner kept its issue locked indefinitely, and expireClaim — which works — was unreachable in normal operation.

That is worse than a claim merely lingering. recordClaim re-activates a row only WHERE status <> 'active', so re-claiming an already-active issue is a deliberate no-op. A stale claim therefore doesn't just sit there — it wins: a later attempt calls claimIssue, gets a success back, and silently inherits the dead process's claimedAt and note.

claimIssue now sweeps before recording, mirroring claimNextBatch's sweep-then-claim in portfolio-queue-manager.js (whose own comment describes reclaiming a lease "stranded 'in_progress' by a dead process" so it doesn't "permanently consume a WIP slot and starve the queue" — the same failure, one layer down). The store also gains reclaimExpiredClaims(maxAgeMs?), the explicit counterpart to reclaimStuckItems for an operator or a scheduled caller.

Design choices, verified against the sibling pattern

  • Wired inside the store's claimIssue, not at one CLI call site. attempt-cli.js and claim-ledger-cli.js both reach the ledger through openClaimLedger(), and the module-level claimIssue delegates to the same store — so this covers every path to a claim, exactly as claimNextBatch covers every batch claim.
  • The sweep is global, not scoped to the issue being claimed — matching sweepStuckItems. A stale claim on feat(docs): add install site and mcp diagnostics #1 is just as stuck whether or not anyone is claiming feat(docs): add install site and mcp diagnostics #1 right now, and claiming any issue is the natural moment the miner is already touching the ledger.
  • expireClaim is untouched, per the issue — this only gives it a caller.
  • The shipped ClaimLedger .d.ts gains reclaimExpiredClaims, so the published type matches the store.

Validation

  • New suite miner-claim-ledger-sweep-wiring.test.ts — 6/6 pass.
  • 4 of the 6 fail against the unwired store, verified by reverting only the source file to main. The diagnostic is the bug itself: expected 'attempt:crashed-process' to be 'attempt:new-process' — the dead process's claim was still winning. The 2 that pass either way are the guards that claiming does not expire a live sibling's claim, nor disturb the normal release → re-claim lifecycle.
  • Patch coverage is 100% — measured from the v8 JSON report: my changed statements are hit (4 and 19 times), and the one branch I introduce (reclaimExpiredClaims's maxAgeMs default-arg) has both sides covered. Clears the 99% codecov/patch wall on packages/loopover-miner/lib/**.
  • npm run typecheck — 0 errors (the .d.ts now declares reclaimExpiredClaims, which the TS test calls). npm run build:miner + npm run test:miner-pack — pass. npm run docs:drift-check — ok.
  • 46/46 across 6 claim-ledger + adjudication suites; the claim-ledger README canary still matches.
  • git diff --check — clean. Rebased on latest main, no base conflict.

Pre-existing failures on clean main (my commit absent), so not introduced here: two Windows-only miner-claim-ledger cases (DB-path resolution, owner-only file permissions) and two miner-attempt-cli cases — all pass on CI's Linux.

Scope

  • Three files, one coherent change: the wiring (claim-ledger.js), its published type (claim-ledger.d.ts), and the regression suite. Wanted paths (packages/, test/).
  • No secrets/tokens/wallet/trust-score/reward terms; no changelog, site/, CNAME, or lovable changes.

Safety

  • Behaviour change is strictly corrective: a stale claim now expires where before it blocked forever. A live claim inside the 14-day window is untouched — asserted.
  • Per-host correctness preserved: the sweep flows through sweepExpiredClaims, which already echoes each row's own apiBaseUrl (Scope local ledger keys by forge host, not bare repoFullName #5563), so it can't expire the wrong forge host's claim.

Closes #6156

sweepExpiredClaims/findExpiredClaims worked but had zero callers, so no
claim ever expired: a claim left behind by a crashed or killed miner kept
its issue locked indefinitely, and expireClaim -- which works -- was
unreachable in normal operation.

That is worse than a claim simply lingering. recordClaim re-activates a row
only `WHERE status <> 'active'`, which makes re-claiming an already-active
issue a deliberate no-op. So a stale claim doesn't just sit there: it wins.
A later attempt calls claimIssue, gets a success back, and silently inherits
the dead process's claimedAt and note.

claimIssue now sweeps before recording, mirroring claimNextBatch's
sweep-then-claim in portfolio-queue-manager.js, where a lease stranded by a
dead process would otherwise starve the queue. Same reasoning, same shape:
the sweep is the only thing standing between a dead process and a permanent
lock. The store also gains reclaimExpiredClaims(maxAgeMs?), the explicit
counterpart to reclaimStuckItems for an operator or a scheduled caller.

The sweep is global rather than scoped to the issue being claimed, matching
sweepStuckItems: a stale claim on issue JSONbored#1 is just as stuck whether or not
anyone is claiming JSONbored#1 right now, and only claiming it again would ever
notice.

expireClaim itself is untouched, per the issue -- this only gives it a
caller. The shipped ClaimLedger .d.ts gains reclaimExpiredClaims so the
published type matches the store.

Regression tests prove the claim actually changes hands: a stale claim's
issue re-claimed by a new attempt now carries the new note and timestamp,
where before the fix it kept the dead process's. Four of the six fail
against the unwired store; the two that pass either way are the guards that
claiming does NOT expire a live sibling's claim or disturb the normal
release/re-claim lifecycle.

Closes JSONbored#6156
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.61%. Comparing base (8b85ce8) to head (7a20f86).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6441   +/-   ##
=======================================
  Coverage   95.61%   95.61%           
=======================================
  Files         598      598           
  Lines       47308    47312    +4     
  Branches    15053    15056    +3     
=======================================
+ Hits        45233    45237    +4     
  Misses       1291     1291           
  Partials      784      784           
Flag Coverage Δ
shard-1 43.92% <0.00%> (-0.01%) ⬇️
shard-2 36.92% <50.00%> (+0.03%) ⬆️
shard-3 32.28% <50.00%> (+0.05%) ⬆️
shard-4 34.38% <0.00%> (+0.05%) ⬆️
shard-5 31.63% <100.00%> (+0.18%) ⬆️
shard-6 45.46% <50.00%> (+0.04%) ⬆️

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

Files with missing lines Coverage Δ
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 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 08:10:34 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This wires the previously-orphaned sweepExpiredClaims into claimIssue (called on every claim attempt) and adds an explicit reclaimExpiredClaims accessor, fixing the described bug where a claim left by a dead process would win forever due to recordClaim's `WHERE status <> 'active'` no-op guard. The fix runs at the correct layer (inside the store's claimIssue, covering every caller via openClaimLedger), mirrors the cited claimNextBatch pattern, and is backed by a well-targeted regression test plus a d.ts update to keep the type in sync. This is a small, correctly-scoped fix with a real, traceable defect and a real regression test (not a fabricated one, since claimIssue's own no-op guard genuinely reproduces the described behavior before the fix).

Nits — 3 non-blocking
  • claim-ledger.js:225-228 — sweepExpiredClaims now runs on every claimIssue call (a full table scan/update via findExpiredClaims), which is fine at current scale but worth a comment or follow-up if claim volume grows significantly.
  • The default reclaimExpiredClaims(maxAgeMs = DEFAULT_MAX_CLAIM_AGE_MS) and the hardcoded DEFAULT_MAX_CLAIM_AGE_MS inside claimIssue are two separate call sites for the same default — consider having claimIssue call `ledger.reclaimExpiredClaims()` internally instead of calling sweepExpiredClaims directly, to avoid the default drifting between the two sites.
  • packages/loopover-miner/lib/claim-ledger.js:227 — DRY: have claimIssue call `ledger.reclaimExpiredClaims()` (already defined right above) instead of duplicating the sweepExpiredClaims(ledger, Date.now(), DEFAULT_MAX_CLAIM_AGE_MS) call.

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 #6156
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: 149 registered-repo PR(s), 88 merged, 31 issue(s).
Contributor context ✅ Confirmed Gittensor contributor luciferlive112116; Gittensor profile; 149 PR(s), 31 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Review context
  • Author: luciferlive112116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 149 PR(s), 31 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 &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 4d326e4 into JSONbored:main Jul 16, 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.

fix(miner): claim-ledger-expiry.js's sweepExpiredClaims/findExpiredClaims are dead; stale claims never auto-expire

1 participant