From 7a20f868d8fdcca0ae6888d9b20a427ec2e3a63b Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Thu, 16 Jul 2026 15:45:47 +0800 Subject: [PATCH] fix(miner): expire claims orphaned by a dead process at claim time 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 #1 is just as stuck whether or not anyone is claiming #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 #6156 --- packages/loopover-miner/lib/claim-ledger.d.ts | 3 + packages/loopover-miner/lib/claim-ledger.js | 12 ++ .../miner-claim-ledger-sweep-wiring.test.ts | 117 ++++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 test/unit/miner-claim-ledger-sweep-wiring.test.ts diff --git a/packages/loopover-miner/lib/claim-ledger.d.ts b/packages/loopover-miner/lib/claim-ledger.d.ts index c18d3e5499..b148def326 100644 --- a/packages/loopover-miner/lib/claim-ledger.d.ts +++ b/packages/loopover-miner/lib/claim-ledger.d.ts @@ -25,7 +25,10 @@ export type ListClaimsFilter = { export type ClaimLedger = { dbPath: string; recordClaim(claim: RecordClaimInput): ClaimEntry; + /** Claims the issue, expiring any claim orphaned by a dead process first (#6156). */ claimIssue(repoFullName: string, issueNumber: number, note?: string, apiBaseUrl?: string): ClaimEntry; + /** Expire claims orphaned by a crashed/killed process, returning the transitioned rows (#6156). */ + reclaimExpiredClaims(maxAgeMs?: number): ClaimEntry[]; releaseClaim(repoFullName: string, issueNumber: number, apiBaseUrl?: string): ClaimEntry | null; expireClaim(repoFullName: string, issueNumber: number, apiBaseUrl?: string): ClaimEntry | null; listClaims(filter?: ListClaimsFilter): ClaimEntry[]; diff --git a/packages/loopover-miner/lib/claim-ledger.js b/packages/loopover-miner/lib/claim-ledger.js index 07507e8f6a..2bf2e8fd1c 100644 --- a/packages/loopover-miner/lib/claim-ledger.js +++ b/packages/loopover-miner/lib/claim-ledger.js @@ -1,4 +1,5 @@ import { DatabaseSync } from "node:sqlite"; +import { DEFAULT_MAX_CLAIM_AGE_MS, sweepExpiredClaims } from "./claim-ledger-expiry.js"; import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; import { isValidRepoSegment } from "./repo-clone.js"; @@ -220,7 +221,18 @@ export function openClaimLedger(dbPath = resolveClaimLedgerDbPath()) { } return rows.map(rowToClaim); }, + /** Expire claims orphaned by a crashed/killed process, returning the transitioned rows (#6156). The explicit + * counterpart to the sweep claimIssue runs on its own, mirroring reclaimStuckItems (portfolio-queue-manager.js). */ + reclaimExpiredClaims(maxAgeMs = DEFAULT_MAX_CLAIM_AGE_MS) { + return sweepExpiredClaims(ledger, Date.now(), maxAgeMs); + }, claimIssue(repoFullName, issueNumber, note, apiBaseUrl) { + // Expire orphaned claims first, so an issue stranded 'active' by a dead process becomes claimable again + // instead of blocking indefinitely (#6156). Without this, recordClaim's `WHERE status <> 'active'` guard + // makes re-claiming an active row a no-op, so a claim whose owning process died keeps winning forever -- + // there is no other path to expireClaim. Mirrors claimNextBatch's sweep-then-claim + // (portfolio-queue-manager.js), where a lease stranded by a dead process would otherwise starve the queue. + sweepExpiredClaims(ledger, Date.now(), DEFAULT_MAX_CLAIM_AGE_MS); return ledger.recordClaim({ repoFullName, issueNumber, note, apiBaseUrl }); }, listActiveClaims(repoFullName) { diff --git a/test/unit/miner-claim-ledger-sweep-wiring.test.ts b/test/unit/miner-claim-ledger-sweep-wiring.test.ts new file mode 100644 index 0000000000..20f02cbccf --- /dev/null +++ b/test/unit/miner-claim-ledger-sweep-wiring.test.ts @@ -0,0 +1,117 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_MAX_CLAIM_AGE_MS } from "../../packages/loopover-miner/lib/claim-ledger-expiry.js"; +import { closeDefaultClaimLedger, openClaimLedger } from "../../packages/loopover-miner/lib/claim-ledger.js"; + +// #6156: sweepExpiredClaims existed and worked, but nothing ever called it -- so a claim left behind by a +// crashed/killed miner never expired. That matters because recordClaim's `WHERE status <> 'active'` guard makes +// re-claiming an already-active row a deliberate no-op: with no sweep, the dead process's claim keeps winning +// forever and there is no path that reaches expireClaim. claimIssue now sweeps first, mirroring +// claimNextBatch's sweep-then-claim (portfolio-queue-manager.js). +const roots: string[] = []; +const ledgers: Array<{ close(): void }> = []; + +function tempLedger() { + const root = mkdtempSync(join(tmpdir(), "loopover-miner-claim-sweep-")); + roots.push(root); + const ledger = openClaimLedger(join(root, "claim-ledger.sqlite3")); + ledgers.push(ledger); + return ledger; +} + +/** Move the clock past the default window, so a claim recorded before it is stale by exactly one hour. */ +function advancePastDefaultWindow(fromIso: string) { + vi.setSystemTime(new Date(Date.parse(fromIso) + DEFAULT_MAX_CLAIM_AGE_MS + 60 * 60 * 1000)); +} + +afterEach(() => { + for (const ledger of ledgers.splice(0)) ledger.close(); + closeDefaultClaimLedger(); + vi.useRealTimers(); + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +describe("loopover-miner claim ledger sweep wiring (#6156)", () => { + it("REGRESSION: a claim stranded by a dead process is expired, so the issue can be claimed again", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 7, "attempt:crashed-process"); + + advancePastDefaultWindow("2026-01-01T00:00:00.000Z"); + const reclaimed = ledger.claimIssue("acme/widgets", 7, "attempt:new-process"); + + // The whole point: the new attempt now OWNS the claim. Before the sweep was wired, recordClaim's no-op guard + // left the dead process's row untouched -- same claimedAt, same note -- while reporting success. + expect(reclaimed.note).toBe("attempt:new-process"); + expect(reclaimed.status).toBe("active"); + expect(reclaimed.claimedAt).toBe(new Date().toISOString()); + expect(ledger.listClaims({ status: "active" })).toEqual([expect.objectContaining({ issueNumber: 7, note: "attempt:new-process" })]); + }); + + it("expires a stale claim on a DIFFERENT issue too -- the sweep is not scoped to the issue being claimed", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 1, "attempt:crashed-process"); + + advancePastDefaultWindow("2026-01-01T00:00:00.000Z"); + ledger.claimIssue("acme/widgets", 2, "attempt:new-process"); + + expect(ledger.listClaims({ status: "expired" }).map((entry) => entry.issueNumber)).toEqual([1]); + expect(ledger.listClaims({ status: "active" }).map((entry) => entry.issueNumber)).toEqual([2]); + }); + + it("leaves a claim inside the window alone -- claiming does not expire a live sibling", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 1, "attempt:still-running"); + + // One hour short of the window: still the live process's claim. + vi.setSystemTime(new Date(Date.parse("2026-01-01T00:00:00.000Z") + DEFAULT_MAX_CLAIM_AGE_MS - 60 * 60 * 1000)); + ledger.claimIssue("acme/widgets", 2, "attempt:other"); + + expect(ledger.listClaims({ status: "expired" })).toEqual([]); + expect(ledger.listClaims({ status: "active" }).map((entry) => entry.issueNumber)).toEqual([1, 2]); + }); + + it("reclaimExpiredClaims() expires stale claims on demand and returns the transitioned rows", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 1, "attempt:crashed-process"); + advancePastDefaultWindow("2026-01-01T00:00:00.000Z"); + + // Default window -- the no-argument branch. + expect(ledger.reclaimExpiredClaims()).toEqual([expect.objectContaining({ issueNumber: 1, status: "expired" })]); + expect(ledger.reclaimExpiredClaims()).toEqual([]); + }); + + it("reclaimExpiredClaims(maxAgeMs) honours an explicit window", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 1, "attempt:recent"); + + // Two hours on: far inside the 14-day default, but past an explicit one-hour window -- so the argument is + // what decides, not the default. + vi.setSystemTime(new Date(Date.parse("2026-01-01T02:00:00.000Z"))); + expect(ledger.reclaimExpiredClaims(DEFAULT_MAX_CLAIM_AGE_MS)).toEqual([]); + expect(ledger.reclaimExpiredClaims(60 * 60 * 1000)).toEqual([expect.objectContaining({ issueNumber: 1, status: "expired" })]); + }); + + it("a released claim is re-claimable as before -- the sweep doesn't disturb the normal lifecycle", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); + const ledger = tempLedger(); + ledger.claimIssue("acme/widgets", 1, "attempt:first"); + ledger.releaseClaim("acme/widgets", 1); + + const again = ledger.claimIssue("acme/widgets", 1, "attempt:second"); + expect(again).toEqual(expect.objectContaining({ status: "active", note: "attempt:second" })); + expect(ledger.listClaims({ status: "expired" })).toEqual([]); + }); +});