Skip to content

miner(attempt): claimLedger.releaseClaim, allocator.release, and every store close() are unguarded in runAttempt's finally block #10335

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

packages/loopover-miner/lib/attempt-cli.ts's runAttempt has a finally block (approximately
lines 1092-1148) that tears down every resource an attempt acquired: worktree cleanup, the
hosted soft-claim release, the local claim-ledger release, the worktree-allocator slot release, the
disposable DB-fork discard, and five store .close() calls, always in that order.

A prior fix (referenced in the file's own comments as #9677) deliberately wrapped three of these
steps — cleanupAttemptWorktree, the hosted submitSoftClaim release, and discardAttemptDbFork
— in individual try { ... } catch (error) { captureMinerError(...) } blocks, with an explicit
rationale: "a locked worktree, a git failure, or a timeout must never abort the rest of this
teardown ... an unremoved worktree is a lesser harm than a stranded active claim that blocks a
sibling miner for the ledger's full expiry window."

That same discipline was never extended to the remaining steps in the same finally block:

if (claimedIssue && claimLedger) claimLedger.releaseClaim(parsed.repoFullName, parsed.issueNumber);  // unguarded
...
if (allocation && allocator) allocator.release(attemptId);                                            // unguarded
...
allocator?.close();       // unguarded
claimLedger?.close();     // unguarded
eventLedger?.close();     // unguarded
attemptLog?.close();      // unguarded
governorLedger?.close();  // unguarded

claimLedger.releaseClaim is a real SQLite write (it can throw on a lock contention error, a disk
error, or a row already swept by the stale-claim expiry job) that runs BEFORE the hosted release,
the allocator release, the DB-fork discard, and all five .close() calls in the same block. If it
throws, none of the later steps run: the exception propagates out of the finally block entirely
(unrecoverable — a throw from inside finally masks whatever the try/catch above it already
decided, and it propagates to runAttempt's own caller). In loop-cli.ts's runLoop, this is only
caught by the outermost loop-level try/catch, meaning a single transient claim-release error
terminates the entire long-running loop daemon process — rather than being logged via
captureMinerError (as every other guarded step in this same finally block already does) and
letting cleanup continue.

test/unit/miner-attempt-cli.test.ts has two REGRESSION (#9677) tests proving a rejecting
cleanupAttemptWorktree and a rejecting hosted claim-release don't abort downstream cleanup — but
no test simulates a throwing claimLedger.releaseClaim, allocator.release, or any of the five
.close() calls.

Requirements

  • Wrap claimLedger.releaseClaim(...) in its own try { ... } catch (error) { captureMinerError(error, { kind: "attempt_claim_release_failed", repoFullName: parsed.repoFullName, attemptId }) } (choose a kind string consistent with the existing
    attempt_worktree_cleanup_failed / attempt_hosted_claim_release_failed /
    attempt_db_fork_discard_failed naming convention already used in this same finally block).
  • Wrap allocator.release(attemptId) the same way, with its own distinct kind.
  • Wrap each of the five .close() calls (allocator?.close(), claimLedger?.close(),
    eventLedger?.close(), attemptLog?.close(), governorLedger?.close()) so that a throw from any
    one of them does not prevent the remaining .close() calls from running — either individual
    try/catch blocks per call (matching the pattern above), or a shared loop/helper that
    catches-and-continues per store; either is acceptable as long as one store's close failure cannot
    skip another store's close.
  • Preserve the exact existing execution ORDER of all steps in the finally block — this issue is
    about guarding failures, not reordering cleanup.
  • Do not change the try/catch block that precedes the finally (the outcome-determination
    logic and exit-code switch) — this issue is scoped to the finally block only.

Deliverables

  • claimLedger.releaseClaim is wrapped in try/catch with captureMinerError, matching the
    #9677 pattern already used for the three other guarded steps in the same block.
  • allocator.release(attemptId) is wrapped the same way.
  • All five .close() calls are guarded so that one failing close cannot prevent the others
    from running.
  • A new regression test (in the style of the existing REGRESSION (#9677) tests in
    test/unit/miner-attempt-cli.test.ts) asserting that a throwing claimLedger.releaseClaim
    does not prevent allocator.release, the DB-fork discard, and all five .close() calls from
    still running.
  • A second new regression test asserting that a throwing allocator.release does not prevent
    the subsequent .close() calls from still running.
  • A third new regression test asserting that a throwing .close() call on one store (e.g.
    claimLedger.close()) does not prevent the other stores' .close() calls from still running.

All three code changes and all three new regression tests are required in this single PR — a PR
that guards the claim/allocator release but leaves the .close() calls unguarded (or vice versa)
does not resolve this issue.

Test Coverage Requirements

This repo's Codecov patch gate is 99%+ (branch-counted) on packages/loopover-miner/lib/**. Add
the three new regression tests to test/unit/miner-attempt-cli.test.ts (the existing test file
that already has the #9677 precedent tests to mirror) — this package's tests live in the shared
root test/ directory, not packages/loopover-miner/test/**. Every new catch branch you add must
be exercised by a test that makes the wrapped call throw.

Expected Outcome

Every step in runAttempt's finally block tears down independently: a failure in any single
cleanup step (claim release, allocator release, or any store's close) is captured via
captureMinerError and logged, but never prevents the remaining cleanup steps from running or
propagates out to terminate the long-running loop daemon process.

Links & Resources

  • packages/loopover-miner/lib/attempt-cli.tsrunAttempt's finally block, approximately
    lines 1092-1148.
  • test/unit/miner-attempt-cli.test.ts — the existing REGRESSION (#9677) tests to mirror for the
    new tests.

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