From c84b5c74861d8c8e99fa0d105f134a844e2c4de9 Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 06:06:49 +0800 Subject: [PATCH] fix(miner): guard runAttempt's worktree cleanup and hosted claim release in the finally block (#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 #9677 Co-Authored-By: Claude Opus 4.8 --- packages/loopover-miner/lib/attempt-cli.ts | 23 +++++- test/unit/miner-attempt-cli.test.ts | 86 ++++++++++++++++++++++ 2 files changed, 105 insertions(+), 4 deletions(-) diff --git a/packages/loopover-miner/lib/attempt-cli.ts b/packages/loopover-miner/lib/attempt-cli.ts index 2c83968a3b..6dc4335ec6 100644 --- a/packages/loopover-miner/lib/attempt-cli.ts +++ b/packages/loopover-miner/lib/attempt-cli.ts @@ -1097,8 +1097,16 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} // the worktree to postmortem -- those are the cases that default to `true` (nothing to retain), matching // cleanupAttemptWorktree's own retention policy (a failed REAL attempt is what gets retained). if (worktreeResult?.ok) { - const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree; - await cleanupWorktree(worktreeResult.repoPath, worktreeResult.worktreePath, worktreeResult.attemptOk ?? true); + // #9677: cleanupWorktree spawns `git worktree remove` -- a locked worktree, a git failure, or a timeout must + // never abort the rest of this teardown (the claim release, allocator release, and every close() below). + // Catch-and-capture, mirroring the DB-fork discard block below; an unremoved worktree is a lesser harm than + // a stranded `active` claim that blocks a sibling miner for the ledger's full expiry window. + try { + const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree; + await cleanupWorktree(worktreeResult.repoPath, worktreeResult.worktreePath, worktreeResult.attemptOk ?? true); + } catch (error) { + captureMinerError(error, { kind: "attempt_worktree_cleanup_failed", repoFullName: parsed.repoFullName, attemptId }); + } } // Every terminal outcome past the claim point (submitted/abandon/stale/blocked/governed, or an // unexpected throw) releases the soft-claim -- a claim that outlives its own attempt process would @@ -1108,8 +1116,15 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} // the initial claim submission actually ran (claimRecord is only set once claimedIssue is), so a run that // never reached the claim point (e.g. blocked_max_concurrent_claims) has nothing to release remotely. if (claimedIssue && claimRecord && isDiscoveryPlaneEnabled(env)) { - const submitClaim = options.submitSoftClaim ?? submitSoftClaim; - await submitClaim({ ...claimRecord, status: "released" } as Parameters[0], { env }); + // #9677: the hosted release is a network POST -- a down discovery plane or a non-2xx response must not + // strand the local claim release / allocator release / close()s that follow. Catch-and-capture like the + // DB-fork discard block below rather than letting the finally throw. + try { + const submitClaim = options.submitSoftClaim ?? submitSoftClaim; + await submitClaim({ ...claimRecord, status: "released" } as Parameters[0], { env }); + } catch (error) { + captureMinerError(error, { kind: "attempt_hosted_claim_release_failed", repoFullName: parsed.repoFullName, attemptId }); + } } if (allocation && allocator) allocator.release(attemptId); // #7858: discard the disposable DB fork on every terminal outcome, mirroring the worktree release above. diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index ce4c9e8031..f1b9d5975c 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -2567,6 +2567,92 @@ describe("runAttempt: hosted soft-claim submission (#7168)", () => { expect(releaseCall![0]).toMatchObject({ repoFullName: "acme/widgets", issueNumber: 7, status: "released" }); }); + it("REGRESSION (#9677): a rejecting cleanupAttemptWorktree still runs the claim release, allocator release + close, and returns the same exit code as a clean cleanup", async () => { + const submittedOutcome = () => ({ + outcome: "submitted" as const, + spec: { command: "gh pr create", cwd: "/tmp/wt", timeoutMs: 1000 }, + execResult: { code: 0 }, + loopResult: fakeLoopResult({ outcome: "handoff" }), + }); + vi.spyOn(console, "log").mockImplementation(() => undefined); + + // Control run with a clean cleanup, to pin the exit code the failing run must match. + const ok = tempLedgers(); + const controlExit = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + attemptId: "attempt-ctl", + openWorktreeAllocator: () => ok.allocator, + openClaimLedger: () => ok.claimLedger, + initEventLedger: () => ok.eventLedger, + initAttemptLog: () => ok.attemptLog, + initGovernorLedger: () => ok.governorLedger, + ...readyPipelineOptions({ runMinerAttempt: async () => submittedOutcome() }), + }); + + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + const releaseClaimSpy = vi.spyOn(claimLedger, "releaseClaim"); + const allocatorReleaseSpy = vi.spyOn(allocator, "release"); + const allocatorCloseSpy = vi.spyOn(allocator, "close"); + const cleanupAttemptWorktree = vi.fn().mockRejectedValue(new Error("git worktree remove failed")); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + attemptId: "attempt-cleanupfail", + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + ...readyPipelineOptions({ runMinerAttempt: async () => submittedOutcome(), cleanupAttemptWorktree }), + }); + + // Before the fix the rejecting cleanup escaped the finally and skipped every step below it. + expect(cleanupAttemptWorktree).toHaveBeenCalled(); + expect(releaseClaimSpy).toHaveBeenCalledWith("acme/widgets", 7); + expect(allocatorReleaseSpy).toHaveBeenCalledWith("attempt-cleanupfail"); + expect(allocatorCloseSpy).toHaveBeenCalled(); + expect(exitCode).toBe(controlExit); + }); + + it("REGRESSION (#9677): a rejecting hosted claim-release still runs the allocator release and closes every store", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const allocatorReleaseSpy = vi.spyOn(allocator, "release"); + const allocatorCloseSpy = vi.spyOn(allocator, "close"); + const claimLedgerCloseSpy = vi.spyOn(claimLedger, "close"); + const eventLedgerCloseSpy = vi.spyOn(eventLedger, "close"); + const attemptLogCloseSpy = vi.spyOn(attemptLog, "close"); + const governorLedgerCloseSpy = vi.spyOn(governorLedger, "close"); + // The initial "active" claim POST succeeds; only the "released" POST rejects (the discovery plane went down + // between claiming and releasing). + const submitSoftClaim = vi.fn(async (claim: { status?: string }) => { + if (claim.status === "released") throw new Error("discovery plane unreachable"); + return { sent: true }; + }); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop", LOOPOVER_MINER_DISCOVERY_PLANE: "true" }, + attemptId: "attempt-releasefail", + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + submitSoftClaim, + ...readyPipelineOptions({ runMinerAttempt: async () => ({ outcome: "abandon", loopResult: fakeLoopResult() }) }), + }); + + expect(exitCode).toBe(7); // abandon -- unchanged by the release failure + // Both the active claim and the (rejecting) release POST fired, and teardown continued past the rejection. + expect(submitSoftClaim).toHaveBeenCalledTimes(2); + expect(allocatorReleaseSpy).toHaveBeenCalledWith("attempt-releasefail"); + expect(allocatorCloseSpy).toHaveBeenCalled(); + expect(claimLedgerCloseSpy).toHaveBeenCalled(); + expect(eventLedgerCloseSpy).toHaveBeenCalled(); + expect(attemptLogCloseSpy).toHaveBeenCalled(); + expect(governorLedgerCloseSpy).toHaveBeenCalled(); + }); + it("does not submit a release soft-claim when the claim point was never reached (blocked_max_concurrent_claims)", async () => { const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); vi.spyOn(console, "log").mockImplementation(() => undefined);