Skip to content

fix(miner): guard runAttempt's worktree cleanup and hosted claim release in the finally block (#9677) - #9912

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9677-guard-finally-cleanup
Jul 29, 2026
Merged

fix(miner): guard runAttempt's worktree cleanup and hosted claim release in the finally block (#9677)#9912
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9677-guard-finally-cleanup

Conversation

@kai392

@kai392 kai392 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What

runAttempt's finally performs the whole teardown in order: cleanup worktree → release local claim → release hosted claim → release allocator slot → discard DB fork → close every store. Only the DB-fork discard was defensive. The two riskiest steps — cleanupAttemptWorktree (a git worktree remove subprocess) and the hosted submitSoftClaim(... "released") (a network POST) — sat before the claim release, the allocator release, and every close(), unguarded. If either rejected, the finally threw and the rest never ran: the soft-claim stayed active (wrongly telling a sibling miner the issue is in flight, for the ledger's full 14-day expiry window), the worktree pool slot stayed leased, and every open SQLite handle was left unclosed.

How

Wrap each of the two calls in its own try/catch that reports via captureMinerError with a distinct kind (attempt_worktree_cleanup_failed, attempt_hosted_claim_release_failed) and the repoFullName / attemptId context — exactly mirroring the DB-fork discard block below them (attempt-cli.ts:1120-1126). Neither catch changes the return value or swallows silently; the claim release, allocator release, and close()s are now reachable on every path through the finally. The SIGINT/SIGTERM handlers and the ordering are unchanged.

Tests

Two named regression tests in test/unit/miner-attempt-cli.test.ts:

  1. A rejecting cleanupAttemptWorktree still runs the local claim release, the allocator release + close(), and returns the same exit code as a clean cleanup (pinned by a control run).
  2. A rejecting hosted claim-release (only the "released" POST rejects; the initial "active" claim succeeds) still runs the allocator release and closes every store.

Both fail against the unguarded code; the two success arms are already covered by the existing end-to-end submitted-path and discovery-plane release tests.

Closes #9677

@kai392
kai392 requested a review from JSONbored as a code owner July 29, 2026 22:02
@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:08: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 PR wraps the two riskiest finally-block teardown steps in runAttempt (worktree cleanup via git subprocess, hosted claim-release POST) in their own try/catch with captureMinerError, matching the existing pattern used for the DB-fork discard block. The fix correctly traces to the real defect: previously either call rejecting would throw out of finally and skip the local claim release, allocator release, and every store close() below it — a genuine stranded-claim/leaked-resources bug. Two targeted regression tests exercise the exact failure paths (rejecting cleanup, rejecting hosted release) and assert the rest of teardown still runs and the exit code is unaffected, which is solid, non-fabricated coverage of a reachable path.

Nits — 5 non-blocking
  • Linked issue miner(attempt): guard every step of runAttempt's finally-block cleanup #9677 is only listed as 'partial' coverage per the external brief — worth confirming the issue's full scope (e.g. does it call for guarding additional finally steps beyond these two?) is addressed.
  • attempt-cli.ts:1105-1108 and 1119-1122 repeat the same try/catch-with-captureMinerError shape three times now (including the pre-existing DB-fork discard); consider a small helper if a fourth guarded step is ever added, though duplication at two/three call sites is acceptable as-is.
  • The new tests rely on tempLedgers() opening real SQLite-backed stores per test, which is consistent with existing conventions in this file but adds a bit of I/O overhead — not a concern given the rest of the suite already does this.
  • packages/loopover-miner/lib/attempt-cli.ts: no code change needed here — the guard mirrors the DB-fork discard block correctly and is the right layer to fix (inside runAttempt's finally, not upstream in cleanupAttemptWorktree/submitSoftClaim).
  • test/unit/miner-attempt-cli.test.ts: the two new regression tests are well-targeted; consider also asserting captureMinerError was called with the expected `kind` field in each, matching the discard-failure test's `captureSpy` assertion pattern already in the file.

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 #9677
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: moderate
Linked issue satisfaction

Addressed
The diff wraps both cleanupAttemptWorktree and submitSoftClaim("released") in their own try/catch blocks with captureMinerError calls using the exact required kinds and context fields, matching the DB-fork discard pattern, and adds two named regression tests covering both rejection paths with the specified assertions.

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.

…ase in the finally block (JSONbored#9677)

runAttempt's `finally` performs the whole teardown in order: cleanup
worktree -> release local claim -> release hosted claim -> release
allocator slot -> discard DB fork -> close every store. Only the DB-fork
discard was defensive. The two riskiest steps -- `cleanupAttemptWorktree`
(a `git worktree remove` subprocess) and the hosted `submitSoftClaim(...
"released")` (a network POST) -- sat before the claim release, the
allocator release, and every close(), unguarded. If either rejected, the
`finally` threw: the soft-claim stayed `active` (wrongly telling a sibling
miner the issue is in flight, for the ledger's full 14-day expiry window),
the worktree pool slot stayed leased, and every open SQLite handle was
left unclosed.

Wrap each of the two calls in its own try/catch that reports via
captureMinerError with a distinct kind (attempt_worktree_cleanup_failed,
attempt_hosted_claim_release_failed) and the repoFullName/attemptId
context -- exactly mirroring the DB-fork discard block below them. Neither
catch changes the return value or swallows silently; the claim release,
allocator release, and close()s are now reachable on every path.

Two named regression tests: a rejecting cleanupAttemptWorktree still runs
the claim release + allocator release/close and returns the same exit code
as a clean cleanup; a rejecting hosted release still runs the allocator
release and closes every store. Both fail against the unguarded code; the
success arms are already covered by the existing end-to-end tests.

Closes JSONbored#9677

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.62%. Comparing base (93e80ac) to head (c84b5c7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9912       +/-   ##
===========================================
- Coverage   91.86%   79.62%   -12.24%     
===========================================
  Files         921      282      -639     
  Lines      113221    58872    -54349     
  Branches    27287     6930    -20357     
===========================================
- Hits       104005    46876    -57129     
- Misses       7929    11706     +3777     
+ Partials     1287      290      -997     
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/attempt-cli.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 84553fc 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(attempt): guard every step of runAttempt's finally-block cleanup

2 participants