From 58dcacdc3d887a9710e1f03ca46ec5577fd21ab0 Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:10:43 +0800 Subject: [PATCH] fix(miner): purge portfolio-queue and run-state rows in the right-to-be-forgotten sweep (#6599) loopover-miner purge --repo swept only four of the six local stores that persist repo_full_name. portfolio-queue.js and run-state.js were left untouched, with no warning -- unlike attempt-log.js, which is deliberately reported as not-purgeable because it genuinely has no repo column. An operator honoring a right-to-be-forgotten request kept that repo's rows in both stores and was told the purge succeeded. Both stores gain purgeByRepo built on the shared purgeStoreByRepo, two purge specs are added, and both are wired into REAL_PURGE_TARGETS so --dry-run and the real purge report them alongside the other four with no special-casing. Keeping repo_full_name in a PRIMARY KEY rather than a plain column never made them unpurgeable; they were simply never wired up. Closes #6599 --- .../loopover-miner/lib/portfolio-queue.d.ts | 1 + .../loopover-miner/lib/portfolio-queue.js | 8 ++++ packages/loopover-miner/lib/purge-cli.d.ts | 4 ++ packages/loopover-miner/lib/purge-cli.js | 12 +++++- packages/loopover-miner/lib/run-state.d.ts | 1 + packages/loopover-miner/lib/run-state.js | 7 ++++ .../loopover-miner/lib/store-maintenance.d.ts | 2 + .../loopover-miner/lib/store-maintenance.js | 5 +++ test/unit/miner-portfolio-queue.test.ts | 28 +++++++++++++ test/unit/miner-purge-cli.test.ts | 37 ++++++++++++++--- test/unit/miner-run-state.test.ts | 40 +++++++++++++++++++ 11 files changed, 138 insertions(+), 7 deletions(-) diff --git a/packages/loopover-miner/lib/portfolio-queue.d.ts b/packages/loopover-miner/lib/portfolio-queue.d.ts index 9a316a0d10..7c2053e278 100644 --- a/packages/loopover-miner/lib/portfolio-queue.d.ts +++ b/packages/loopover-miner/lib/portfolio-queue.d.ts @@ -50,6 +50,7 @@ export type PortfolioQueueStore = { ) => Array<{ repoFullName: string; identifier: string; apiBaseUrl?: string }>, ): QueueEntry[]; getAttemptHistory(repoFullName: string, identifier: string, apiBaseUrl?: string): QueueAttemptHistory; + purgeByRepo(repoFullName: string): number; close(): void; }; diff --git a/packages/loopover-miner/lib/portfolio-queue.js b/packages/loopover-miner/lib/portfolio-queue.js index f93c43f9b3..8045f061f3 100644 --- a/packages/loopover-miner/lib/portfolio-queue.js +++ b/packages/loopover-miner/lib/portfolio-queue.js @@ -1,6 +1,7 @@ import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; import { applySchemaMigrations } from "./schema-version.js"; +import { PORTFOLIO_QUEUE_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js"; // The miner's local portfolio/queue store (#2292): a 100% client-side, prioritized backlog of candidate work // items across every repo the miner has been pointed at ("what should I look at next, across everything I'm @@ -385,6 +386,13 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) reachedDone: row.status === "done", }; }, + // Explicit, operator-invoked right-to-be-forgotten purge (#5564, wired here by #6599) — never runs + // automatically. Distinct from this store's normal enqueue/release/expire lifecycle: deletes every row for + // a repo outright. normalizeRepoFullName throws on a missing/malformed name rather than letting a typo + // silently purge nothing and report success. + purgeByRepo(repoFullName) { + return purgeStoreByRepo(db, PORTFOLIO_QUEUE_PURGE_SPEC, normalizeRepoFullName(repoFullName)); + }, close() { db.close(); }, diff --git a/packages/loopover-miner/lib/purge-cli.d.ts b/packages/loopover-miner/lib/purge-cli.d.ts index bbba9301bc..0f84a01e6d 100644 --- a/packages/loopover-miner/lib/purge-cli.d.ts +++ b/packages/loopover-miner/lib/purge-cli.d.ts @@ -2,6 +2,8 @@ import type { ClaimLedger } from "./claim-ledger.js"; import type { EventLedger } from "./event-ledger.js"; import type { GovernorLedger } from "./governor-ledger.js"; import type { PredictionLedger } from "./prediction-ledger.js"; +import type { PortfolioQueueStore } from "./portfolio-queue.js"; +import type { RunStateStore } from "./run-state.js"; export const ATTEMPT_LOG_NOT_PURGEABLE_NOTE: string; @@ -33,6 +35,8 @@ export type PurgeCliOptions = { initEventLedger?: () => EventLedger; initGovernorLedger?: () => GovernorLedger; initPredictionLedger?: () => PredictionLedger; + initPortfolioQueueStore?: () => PortfolioQueueStore; + initRunStateStore?: () => RunStateStore; resolveDbPaths?: Record string>; }; diff --git a/packages/loopover-miner/lib/purge-cli.js b/packages/loopover-miner/lib/purge-cli.js index 91b2bc81e1..4fc7fb8c4e 100644 --- a/packages/loopover-miner/lib/purge-cli.js +++ b/packages/loopover-miner/lib/purge-cli.js @@ -1,6 +1,8 @@ // `loopover-miner purge` (#5564): an explicit, operator-invoked right-to-be-forgotten path across the local -// ledgers. Deletes every row for one repo from the four stores that have a real `repoColumn` (claim-ledger, -// event-ledger, governor-ledger, prediction-ledger), via each store's own `purgeByRepo` method (which reuses +// ledgers. Deletes every row for one repo from the six stores that have a real `repoColumn` (claim-ledger, +// event-ledger, governor-ledger, prediction-ledger, portfolio-queue, run-state — the last two wired in by +// #6599, which is why the four ledgers above read as the whole set in older comments), via each store's own +// `purgeByRepo` method (which reuses // `store-maintenance.js`'s shared, identifier-guarded `purgeStoreByRepo`). `attempt-log.js` is deliberately // reported as not-purgeable rather than silently skipped or approximated: its payload is a free-form // `Record` with no dedicated repo column, so a precise per-repo match isn't possible there @@ -15,12 +17,16 @@ import { openClaimLedger, resolveClaimLedgerDbPath } from "./claim-ledger.js"; import { initEventLedger, resolveEventLedgerDbPath } from "./event-ledger.js"; import { initGovernorLedger, resolveGovernorLedgerDbPath } from "./governor-ledger.js"; import { initPredictionLedger, resolvePredictionLedgerDbPath } from "./prediction-ledger.js"; +import { initPortfolioQueueStore, resolvePortfolioQueueDbPath } from "./portfolio-queue.js"; +import { initRunStateStore, resolveRunStateDbPath } from "./run-state.js"; import { resolveAttemptLogDbPath } from "./attempt-log.js"; import { CLAIM_LEDGER_PURGE_SPEC, EVENT_LEDGER_PURGE_SPEC, GOVERNOR_LEDGER_PURGE_SPEC, PREDICTION_LEDGER_PURGE_SPEC, + PORTFOLIO_QUEUE_PURGE_SPEC, + RUN_STATE_PURGE_SPEC, countStoreByRepo, describeError, } from "./store-maintenance.js"; @@ -36,6 +42,8 @@ const REAL_PURGE_TARGETS = [ { name: "event-ledger", optionKey: "initEventLedger", opener: initEventLedger, resolveDbPath: resolveEventLedgerDbPath, spec: EVENT_LEDGER_PURGE_SPEC }, { name: "governor-ledger", optionKey: "initGovernorLedger", opener: initGovernorLedger, resolveDbPath: resolveGovernorLedgerDbPath, spec: GOVERNOR_LEDGER_PURGE_SPEC }, { name: "prediction-ledger", optionKey: "initPredictionLedger", opener: initPredictionLedger, resolveDbPath: resolvePredictionLedgerDbPath, spec: PREDICTION_LEDGER_PURGE_SPEC }, + { name: "portfolio-queue", optionKey: "initPortfolioQueueStore", opener: initPortfolioQueueStore, resolveDbPath: resolvePortfolioQueueDbPath, spec: PORTFOLIO_QUEUE_PURGE_SPEC }, + { name: "run-state", optionKey: "initRunStateStore", opener: initRunStateStore, resolveDbPath: resolveRunStateDbPath, spec: RUN_STATE_PURGE_SPEC }, ]; function parseRepoArg(value, usage) { diff --git a/packages/loopover-miner/lib/run-state.d.ts b/packages/loopover-miner/lib/run-state.d.ts index 2446e12142..fb1bed014d 100644 --- a/packages/loopover-miner/lib/run-state.d.ts +++ b/packages/loopover-miner/lib/run-state.d.ts @@ -19,6 +19,7 @@ export type RunStateStore = { getRunState(repoFullName: string, apiBaseUrl?: string): RunState | null; setRunState(repoFullName: string, state: RunState, apiBaseUrl?: string): RunStateWrite; listRunStates(): RunStateRow[]; + purgeByRepo(repoFullName: string): number; close(): void; }; diff --git a/packages/loopover-miner/lib/run-state.js b/packages/loopover-miner/lib/run-state.js index de9bfaa843..6fdddf29fb 100644 --- a/packages/loopover-miner/lib/run-state.js +++ b/packages/loopover-miner/lib/run-state.js @@ -1,6 +1,7 @@ import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; import { applySchemaMigrations } from "./schema-version.js"; +import { purgeStoreByRepo, RUN_STATE_PURGE_SPEC } from "./store-maintenance.js"; export const RUN_STATES = Object.freeze(["idle", "discovering", "planning", "preparing"]); @@ -133,6 +134,12 @@ export function initRunStateStore(dbPath = resolveRunStateDbPath()) { updatedAt: row.updated_at, })); }, + // Explicit, operator-invoked right-to-be-forgotten purge (#5564, wired here by #6599) — never runs + // automatically. Deletes every tracked-state row for a repo outright. normalizeRepoFullName throws on a + // missing/malformed name rather than letting a typo silently purge nothing and report success. + purgeByRepo(repoFullName) { + return purgeStoreByRepo(db, RUN_STATE_PURGE_SPEC, normalizeRepoFullName(repoFullName)); + }, close() { db.close(); }, diff --git a/packages/loopover-miner/lib/store-maintenance.d.ts b/packages/loopover-miner/lib/store-maintenance.d.ts index f54483b6bb..438c38d383 100644 --- a/packages/loopover-miner/lib/store-maintenance.d.ts +++ b/packages/loopover-miner/lib/store-maintenance.d.ts @@ -13,6 +13,8 @@ export const CLAIM_LEDGER_PURGE_SPEC: LedgerPurgeSpec; export const EVENT_LEDGER_PURGE_SPEC: LedgerPurgeSpec; export const GOVERNOR_LEDGER_PURGE_SPEC: LedgerPurgeSpec; export const PREDICTION_LEDGER_PURGE_SPEC: LedgerPurgeSpec; +export const PORTFOLIO_QUEUE_PURGE_SPEC: LedgerPurgeSpec; +export const RUN_STATE_PURGE_SPEC: LedgerPurgeSpec; export type StoreIntegrityResult = { name: string; ok: boolean; detail: string }; export type LedgerRetentionPolicy = { maxAgeMs?: number; maxRows?: number }; diff --git a/packages/loopover-miner/lib/store-maintenance.js b/packages/loopover-miner/lib/store-maintenance.js index 073d2a1bfe..faba3a8f28 100644 --- a/packages/loopover-miner/lib/store-maintenance.js +++ b/packages/loopover-miner/lib/store-maintenance.js @@ -32,6 +32,11 @@ export const CLAIM_LEDGER_PURGE_SPEC = { table: "miner_claims", repoColumn: "rep export const EVENT_LEDGER_PURGE_SPEC = { table: "miner_event_ledger", repoColumn: "repo_full_name" }; export const GOVERNOR_LEDGER_PURGE_SPEC = { table: "governor_events", repoColumn: "repo_full_name" }; export const PREDICTION_LEDGER_PURGE_SPEC = { table: "predictions", repoColumn: "repo_full_name" }; +// These two stores keep repo_full_name inside their PRIMARY KEY rather than as a plain column, but that is +// irrelevant to purging: a DELETE by repo works the same either way, so they are purgeable for the same reason +// the four above are, and were simply never wired up (#6599). +export const PORTFOLIO_QUEUE_PURGE_SPEC = { table: "miner_portfolio_queue", repoColumn: "repo_full_name" }; +export const RUN_STATE_PURGE_SPEC = { table: "miner_run_state", repoColumn: "repo_full_name" }; const SQL_IDENTIFIER = /^[A-Za-z_][A-Za-z0-9_]*$/; diff --git a/test/unit/miner-portfolio-queue.test.ts b/test/unit/miner-portfolio-queue.test.ts index 84f4b70d3d..646d4d5bd9 100644 --- a/test/unit/miner-portfolio-queue.test.ts +++ b/test/unit/miner-portfolio-queue.test.ts @@ -644,4 +644,32 @@ describe("loopover-miner portfolio/queue store (#2292)", () => { }).not.toThrow(); }); }); + + describe("purgeByRepo (#6599)", () => { + it("deletes every queued item for one repo and leaves other repos untouched", () => { + const store = tempStore(); + store.enqueue({ repoFullName: "owner/repo-a", identifier: "issue:1" }); + store.enqueue({ repoFullName: "owner/repo-a", identifier: "issue:2" }); + store.enqueue({ repoFullName: "owner/repo-b", identifier: "issue:3" }); + + expect(store.purgeByRepo("owner/repo-a")).toBe(2); + expect(store.listQueue("owner/repo-a")).toEqual([]); + expect(store.listQueue("owner/repo-b")).toHaveLength(1); + }); + + it("returns 0 when nothing matches the repo", () => { + const store = tempStore(); + store.enqueue({ repoFullName: "owner/repo-b", identifier: "issue:1" }); + expect(store.purgeByRepo("owner/repo-a")).toBe(0); + expect(store.listQueue("owner/repo-b")).toHaveLength(1); + }); + + it("rejects a missing/malformed repoFullName rather than silently no-opping", () => { + // A typo'd repo must not report a successful purge of nothing — the operator would believe the + // right-to-be-forgotten request was honored. + const store = tempStore(); + expect(() => store.purgeByRepo(undefined as never)).toThrow("invalid_repo_full_name"); + expect(() => store.purgeByRepo("no-slash")).toThrow("invalid_repo_full_name"); + }); + }); }); diff --git a/test/unit/miner-purge-cli.test.ts b/test/unit/miner-purge-cli.test.ts index 89f6d79385..b9909bc50d 100644 --- a/test/unit/miner-purge-cli.test.ts +++ b/test/unit/miner-purge-cli.test.ts @@ -6,6 +6,8 @@ import { openClaimLedger, closeDefaultClaimLedger } from "../../packages/loopove import { initEventLedger, closeDefaultEventLedger } from "../../packages/loopover-miner/lib/event-ledger.js"; import { initGovernorLedger, closeDefaultGovernorLedger } from "../../packages/loopover-miner/lib/governor-ledger.js"; import { initPredictionLedger, closeDefaultPredictionLedger } from "../../packages/loopover-miner/lib/prediction-ledger.js"; +import { initPortfolioQueueStore } from "../../packages/loopover-miner/lib/portfolio-queue.js"; +import { initRunStateStore } from "../../packages/loopover-miner/lib/run-state.js"; import { initAttemptLog, closeDefaultAttemptLog } from "../../packages/loopover-miner/lib/attempt-log.js"; import { ATTEMPT_LOG_NOT_PURGEABLE_NOTE, @@ -69,12 +71,14 @@ describe("parsePurgeArgs (#5564)", () => { }); describe("runPurge --dry-run (#5564)", () => { - it("counts matching rows across the four real stores without writing anything, and reports attempt-log as not-purgeable", async () => { + it("counts matching rows across the six real stores without writing anything, and reports attempt-log as not-purgeable", async () => { const root = tempDir(); const claimDbPath = join(root, "claim-ledger.sqlite3"); const eventDbPath = join(root, "event-ledger.sqlite3"); const governorDbPath = join(root, "governor-ledger.sqlite3"); const predictionDbPath = join(root, "prediction-ledger.sqlite3"); + const portfolioDbPath = join(root, "portfolio-queue.sqlite3"); + const runStateDbPath = join(root, "run-state.sqlite3"); const attemptLogDbPath = join(root, "attempt-log.sqlite3"); // never created — dry run must not touch it const claimLedger = openClaimLedger(claimDbPath); @@ -108,11 +112,26 @@ describe("runPurge --dry-run (#5564)", () => { }); predictionLedger.close(); + // #6599: both of these persist repo_full_name and were silently left out of the purge entirely. Each gets + // 2 rows for the target repo and 1 for another repo, so the count proves the sweep is repo-scoped. + const portfolioQueue = initPortfolioQueueStore(portfolioDbPath); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:1" }); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:2" }); + portfolioQueue.enqueue({ repoFullName: "acme/other", identifier: "issue:3" }); + portfolioQueue.close(); + + const runState = initRunStateStore(runStateDbPath); + runState.setRunState("acme/widgets", "planning"); + runState.setRunState("acme/other", "idle"); + runState.close(); + const resolveDbPaths = { "claim-ledger": () => claimDbPath, "event-ledger": () => eventDbPath, "governor-ledger": () => governorDbPath, "prediction-ledger": () => predictionDbPath, + "portfolio-queue": () => portfolioDbPath, + "run-state": () => runStateDbPath, "attempt-log": () => attemptLogDbPath, }; @@ -127,6 +146,8 @@ describe("runPurge --dry-run (#5564)", () => { { store: "event-ledger", wouldPurge: 1 }, { store: "governor-ledger", wouldPurge: 1 }, { store: "prediction-ledger", wouldPurge: 0 }, + { store: "portfolio-queue", wouldPurge: 2 }, + { store: "run-state", wouldPurge: 1 }, ], attemptLogNote: ATTEMPT_LOG_NOT_PURGEABLE_NOTE, attemptLogTotalRows: 0, @@ -283,11 +304,15 @@ describe("runPurge (real, #5564)", () => { const event = fakeStore(1); const governor = fakeStore(0); const prediction = fakeStore(3); + const portfolioQueue = fakeStore(4); // #6599 + const runState = fakeStore(1); // #6599 const options = { openClaimLedger: () => claim, initEventLedger: () => event, initGovernorLedger: () => governor, initPredictionLedger: () => prediction, + initPortfolioQueueStore: () => portfolioQueue, + initRunStateStore: () => runState, }; const log = vi.spyOn(console, "log").mockImplementation(() => undefined); @@ -296,28 +321,30 @@ describe("runPurge (real, #5564)", () => { expect(summary).toMatchObject({ outcome: "purged", repoFullName: "acme/widgets", - totalPurged: 6, + totalPurged: 11, // 6 from the four original stores + 5 newly covered by #6599 stores: [ { store: "claim-ledger", purged: 2 }, { store: "event-ledger", purged: 1 }, { store: "governor-ledger", purged: 0 }, { store: "prediction-ledger", purged: 3 }, + { store: "portfolio-queue", purged: 4 }, + { store: "run-state", purged: 1 }, { store: "attempt-log", purged: null, note: ATTEMPT_LOG_NOT_PURGEABLE_NOTE }, ], }); expect(typeof summary.purgedAt).toBe("string"); - for (const store of [claim, event, governor, prediction]) { + for (const store of [claim, event, governor, prediction, portfolioQueue, runState]) { expect(store.purgeByRepo).toHaveBeenCalledWith("acme/widgets"); } // Injected stores are caller-owned: runPurge must not close them. - for (const store of [claim, event, governor, prediction]) { + for (const store of [claim, event, governor, prediction, portfolioQueue, runState]) { expect(store.close).not.toHaveBeenCalled(); } log.mockClear(); expect(runPurge(["--repo", "acme/widgets"], options as never)).toBe(0); const text = String(log.mock.calls[0]?.[0]); - expect(text).toContain("Purged 6 row(s) for acme/widgets"); + expect(text).toContain("Purged 11 row(s) for acme/widgets"); expect(text).toContain("claim-ledger=2"); expect(text).toContain(ATTEMPT_LOG_NOT_PURGEABLE_NOTE); }); diff --git a/test/unit/miner-run-state.test.ts b/test/unit/miner-run-state.test.ts index 6d84e31f42..30380aeba4 100644 --- a/test/unit/miner-run-state.test.ts +++ b/test/unit/miner-run-state.test.ts @@ -359,4 +359,44 @@ describe("loopover-miner run-state store (#2289)", () => { }).not.toThrow(); }); }); + + describe("purgeByRepo (#6599)", () => { + // Closed via this block's own afterEach rather than per-test, so a failing assertion still releases the + // SQLite handle — an open handle makes the outer afterEach's rmSync fail on Windows. + const openStores: Array<{ close: () => void }> = []; + afterEach(() => { + for (const store of openStores.splice(0)) store.close(); + }); + + function tempStore() { + const store = initRunStateStore(join(tempRoot(), "nested", "run-state.sqlite3")); + openStores.push(store); + return store; + } + + it("deletes the tracked state for one repo and leaves other repos untouched", () => { + const store = tempStore(); + store.setRunState("owner/repo-a", "planning"); + store.setRunState("owner/repo-b", "preparing"); + + expect(store.purgeByRepo("owner/repo-a")).toBe(1); + expect(store.getRunState("owner/repo-a")).toBeNull(); + expect(store.listRunStates()).toHaveLength(1); + }); + + it("returns 0 when nothing matches the repo", () => { + const store = tempStore(); + store.setRunState("owner/repo-b", "planning"); + expect(store.purgeByRepo("owner/repo-a")).toBe(0); + expect(store.listRunStates()).toHaveLength(1); + }); + + it("rejects a missing/malformed repoFullName rather than silently no-opping", () => { + // A typo'd repo must not report a successful purge of nothing — the operator would believe the + // right-to-be-forgotten request was honored. + const store = tempStore(); + expect(() => store.purgeByRepo(undefined as never)).toThrow("invalid_repo_full_name"); + expect(() => store.purgeByRepo("no-slash")).toThrow("invalid_repo_full_name"); + }); + }); });