diff --git a/packages/gittensory-miner/lib/loop-cli.js b/packages/gittensory-miner/lib/loop-cli.js index 9b6ff15131..54e56d89ea 100644 --- a/packages/gittensory-miner/lib/loop-cli.js +++ b/packages/gittensory-miner/lib/loop-cli.js @@ -16,15 +16,11 @@ // // REAL, NOT FABRICATED: this loop is the first production caller of governor-state.js's `saveCapUsage` // (turnsTaken from runMinerAttempt's own real `loopResult.totalTurnsUsed`, elapsedMs from real wall-clock -// measurement) and of a genuine per-identifier convergence history (attempts/consecutiveFailures/reenqueues -// tracked in this process's own memory across its own cycles) -- both were previously honest zero/placeholder -// literals (see attempt-input-builder.js's own header) because a ONE-SHOT `attempt` CLI invocation has no -// cross-call history to draw on. A long-running loop genuinely does. -// -// DOCUMENTED GAP: convergence/cap-usage history is IN-MEMORY, scoped to this loop process's own lifetime (cap -// usage itself persists across restarts via governor-state.js; per-identifier convergence counters do not -- -// a durable version needs attempt-log.js to grow a repo+issue index, the same separate schema change -// attempt-input-builder.js's header already flags as out of scope here). +// measurement). Its per-identifier convergence history (attempts/consecutiveFailures/reenqueues) is the real, +// SQLite-persisted portfolio-queue attempt-history (portfolio-queue.js's getAttemptHistory, #5654) that the +// dequeueNext claim + markDone/markFailed calls below already maintain -- the same source a one-shot `attempt` +// invocation reads (#5654), so both share one source of truth and the counters survive a loop-daemon restart +// (crash/deploy/systemd bounce) instead of resetting with the process (#5677). import { checkMinerKillSwitch } from "./governor-kill-switch.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; @@ -169,14 +165,6 @@ function parseIssueNumberFromIdentifier(identifier) { return match ? Number(match[1]) : null; } -function convergenceKey(repoFullName, identifier) { - return `${repoFullName}:${identifier}`; -} - -function zeroConvergence() { - return { attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }; -} - /** * Run one full discover -> claim -> attempt -> observe -> reenter cycle repeatedly until a kill-switch trips, * the run-loop boundary gate halts (non-convergence or a real budget/turn/elapsed cap), re-entry is declined, @@ -290,7 +278,6 @@ export async function runLoop(args, options = {}) { } let usage = governorState.loadCapUsage(); - const convergenceHistory = new Map(); const cycles = []; let sinceSeq = eventLedger.readEvents({}).at(-1)?.seq ?? 0; let haltReason = null; @@ -352,9 +339,15 @@ export async function runLoop(args, options = {}) { continue; } - const key = convergenceKey(claimed.repoFullName, claimed.identifier); const amsPolicy = await resolveAmsPolicyFn(claimed.repoFullName, { env }); - const convergenceInput = convergenceHistory.get(key) ?? zeroConvergence(); + // Real, SQLite-persisted per-item convergence history (#5677): the dequeueNext claim above already recorded + // this attempt and the markDone/markFailed calls below record the outcome, so reading it back here shares one + // source of truth with attempt-cli.js (#5654) and survives a loop-daemon restart instead of resetting. + const convergenceInput = portfolioQueue.getAttemptHistory( + claimed.repoFullName, + claimed.identifier, + claimed.apiBaseUrl, + ); const boundary = evaluateBoundaryGateFn( { @@ -377,9 +370,6 @@ export async function runLoop(args, options = {}) { break; } - convergenceInput.attempts += 1; - convergenceHistory.set(key, convergenceInput); - const cycleStartMs = nowMsFn(); let lastResult = null; const attemptArgv = [ @@ -420,19 +410,15 @@ export async function runLoop(args, options = {}) { // (reenqueues threshold) rather than silently retried forever. const permanentBlock = attemptOutcome === "blocked_rejection_signaled"; - if (submitted) { - portfolioQueue.markDone(claimed.repoFullName, claimed.identifier, claimed.apiBaseUrl); - convergenceInput.reachedDone = true; - convergenceInput.consecutiveFailures = 0; - } else if (permanentBlock) { + if (submitted || permanentBlock) { + // Both terminal -- a submitted PR is done, and a repo-wide AI-usage-policy ban never resolves on retry -- + // so neither is re-queued. markDone also clears the persisted consecutive-failure streak. portfolioQueue.markDone(claimed.repoFullName, claimed.identifier, claimed.apiBaseUrl); - convergenceInput.consecutiveFailures += 1; } else { + // Any other blocked/abandoned/stale/governed outcome may resolve on a later retry, so requeue it; markFailed + // records the re-enqueue + consecutive failure the non-convergence detector reads on the next cycle. portfolioQueue.markFailed(claimed.repoFullName, claimed.identifier, claimed.apiBaseUrl); - convergenceInput.consecutiveFailures += 1; - convergenceInput.reenqueues += 1; } - convergenceHistory.set(key, convergenceInput); let reentryOutcome = "other"; let prNumber = null; diff --git a/test/unit/miner-loop-cli.test.ts b/test/unit/miner-loop-cli.test.ts index 59b6c0b97d..252b80fa97 100644 --- a/test/unit/miner-loop-cli.test.ts +++ b/test/unit/miner-loop-cli.test.ts @@ -367,6 +367,46 @@ describe("runLoop (#5135)", () => { expect(runDiscoverSpy).toHaveBeenCalledTimes(1); }); + it("REGRESSION (#5677): reads convergence history from the persisted portfolio queue, so a stuck item's re-enqueue streak survives a loop-daemon restart instead of resetting to fresh", async () => { + const { eventLedger, governorLedger, portfolioQueue, runState, governorState, paths } = tempStores(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const item = { repoFullName: "acme/widgets", identifier: "issue:7" }; + + // Prior loop cycles that never reached done: each claim -> markFailed persists one re-enqueue + consecutive + // failure on the queue row (portfolio-queue.js, #5654/#5661), taking the item to the non-convergence + // threshold -- the state a real long-running loop would have accumulated before a restart. + const { maxReenqueues } = DEFAULT_AMS_POLICY_SPEC.convergenceThresholds; + portfolioQueue.enqueue(item); + for (let cycle = 0; cycle < maxReenqueues; cycle += 1) { + portfolioQueue.dequeueNext(); + portfolioQueue.markFailed(item.repoFullName, item.identifier); + } + expect(portfolioQueue.getAttemptHistory(item.repoFullName, item.identifier).reenqueues).toBe(maxReenqueues); + for (const closer of [eventLedger, governorLedger, portfolioQueue, runState, governorState]) closer.close(); + + // Restart: a brand-new loop process -- fresh handles to the SAME on-disk files, with no in-memory Map to + // inherit. Pre-#5677 the fresh Map would read a zero convergence history and attempt the stuck item anyway. + const runAttemptSpy = vi.fn(); + const exitCode = await runLoop(["acme/widgets", "--miner-login", "alice", "--json"], { + openGovernorState: () => openGovernorState(paths.governorStatePath), + initEventLedger: () => initEventLedger(paths.eventLedgerPath), + initGovernorLedger: () => initGovernorLedger(paths.governorLedgerPath), + initPortfolioQueue: () => initPortfolioQueueStore(paths.portfolioQueuePath), + initRunStateStore: () => initRunStateStore(paths.runStatePath), + runDiscover: vi.fn(async () => 0), + runAttempt: runAttemptSpy, + ...readyLoopOptions(), + }); + + expect(exitCode).toBe(0); + // Halted before this run's FIRST attempt: the persisted re-enqueue streak (not a reset in-memory Map) was read + // straight away, so the stuck item is caught immediately after the restart rather than re-attempted from zero. + expect(runAttemptSpy).not.toHaveBeenCalled(); + const after = reopenAfterRun(paths); + expect(after.governorLedger.readGovernorEvents({}).some((event) => event.reason === "non_convergence_detected")).toBe(true); + expect(after.portfolioQueue.listQueue()[0]).toMatchObject({ status: "queued" }); + }); + it("REGRESSION: runs a full cycle end to end -- claims, attempts, polls real PR disposition, records the outcome, and re-enters", async () => { const { eventLedger, governorLedger, portfolioQueue, runState, governorState, paths } = tempStores(); const log = vi.spyOn(console, "log").mockImplementation(() => undefined);