diff --git a/migrations/0211_retention_column_indexes_round_three.sql b/migrations/0211_retention_column_indexes_round_three.sql new file mode 100644 index 000000000..8e2a6eac8 --- /dev/null +++ b/migrations/0211_retention_column_indexes_round_three.sql @@ -0,0 +1,5 @@ +-- #10058: submitter_outcome_log and alert_dedup_claims gained a RETENTION_POLICY entry in the same change, +-- following 0193/0196's precedent of pairing every new policy table with a leading-column index on its +-- retention timestamp so the batched delete's inner SELECT is an index range scan rather than a full scan. +CREATE INDEX IF NOT EXISTS idx_submitter_outcome_log_recorded_at ON submitter_outcome_log(recorded_at); +CREATE INDEX IF NOT EXISTS idx_alert_dedup_claims_created_at ON alert_dedup_claims(created_at); diff --git a/src/db/retention.ts b/src/db/retention.ts index 16c9fb028..5b79ca59a 100644 --- a/src/db/retention.ts +++ b/src/db/retention.ts @@ -118,6 +118,17 @@ export const RETENTION_POLICY: readonly RetentionRule[] = [ // plus contributor content (the largest, most sensitive artifact in the replay family), the re-query mode // is a debugging tool for RECENT decisions, and the public promptDigest commitment outlives the text. { table: "decision_replay_prompts", column: "created_at", days: 30 }, + // #10058: two more members of the same re-derivable/per-event class #9473 bounded, found by the same audit + // sweep for tables written per event with NO delete path anywhere in src/: + // - submitter_outcome_log is per (project, submitter, pull_number, outcome), appended on every PR + // terminal (src/review/submitter-reputation.ts), and its only reader is already windowed + // (`recorded_at >= datetime('now', ?)`) -- same "windowed reader, aged rows are pure dead weight" shape + // as contributor_gate_history above, so it gets the same 90-day window. + // - alert_dedup_claims is a pure hourly-expiring idempotency claim (src/review/alerts.ts hashes an hour + // bucket into its dedup key), never read again once its hour passes -- same short-lived-idempotency-log + // shape as webhook_events / orb_webhook_events above, so it gets the same 14-day window. + { table: "submitter_outcome_log", column: "recorded_at", days: 90 }, + { table: "alert_dedup_claims", column: "created_at", days: 14 }, ]; // #9083: a real, single-column, indexable primary key for the ordered-range delete below, keyed by table @@ -168,6 +179,8 @@ export const RETENTION_PK_COLUMN: Readonly> = { decision_replay_inputs: "record_id", // Same key shape as decision_replay_inputs above, for the same reason. decision_replay_prompts: "record_id", + // #10058: alert_dedup_claims has a genuine single-column `id TEXT PRIMARY KEY` (migrations/0181). + alert_dedup_claims: "id", }; /** @@ -191,6 +204,9 @@ export const RETENTION_COMPOSITE_PK_TABLES: ReadonlySet = new Set([ "ai_review_cache", "ai_slop_cache", "linked_issue_satisfaction_cache", + // #10058: PRIMARY KEY (project, submitter, pull_number, outcome) -- no genuine single-column id, so this + // table pays the rowid/ctid cost noted above rather than getting a surrogate key added just for pruning. + "submitter_outcome_log", ]); /** diff --git a/test/unit/retention.test.ts b/test/unit/retention.test.ts index 682810694..663ac2a3f 100644 --- a/test/unit/retention.test.ts +++ b/test/unit/retention.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { createApp } from "../../src/api/routes"; import { getDb } from "../../src/db/client"; -import { dedupeSignalSnapshots, pruneExpiredRecords, RETENTION_COMPOSITE_PK_TABLES, RETENTION_PK_COLUMN, RETENTION_POLICY, retentionCutoffIsoForTable } from "../../src/db/retention"; +import { dedupeSignalSnapshots, pruneExpiredRecords, RETENTION_COMPOSITE_PK_TABLES, RETENTION_PK_COLUMN, RETENTION_POLICY, retentionCutoffIsoForTable, retentionDaysForTable } from "../../src/db/retention"; import { computeFleetAnalytics } from "../../src/orb/analytics"; import { listFleetInstances } from "../../src/orb/fleet-admin"; import { getOrbGlobalStats } from "../../src/orb/outcomes"; @@ -417,6 +417,69 @@ describe("pruneExpiredRecords", () => { const remaining = await env.DB.prepare("SELECT count(*) AS n FROM ai_review_cache").first<{ n: number }>(); expect(remaining?.n).toBe(0); }); + + // #10058: submitter_outcome_log and alert_dedup_claims both store CURRENT_TIMESTAMP-format timestamps + // (`'YYYY-MM-DD HH:MM:SS'`, no `T`, no zone) because both writers omit the column and let the DB default + // supply it -- unlike every other policy table's ISO-8601 `column`. pruneExpiredRecords binds an ISO cutoff + // and compares as text; seeding in the exact writer-produced format (not `daysAgo`'s ISO string) is what + // proves the comparison still resolves the right rows on both sides of the cutoff. + it("prunes submitter_outcome_log older than its 90-day window and keeps recent rows (#10058)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO submitter_outcome_log (project, submitter, pull_number, outcome, recorded_at) + VALUES + ('acme/widgets', 'alice', 1, 'merged', '2026-01-01 00:00:00'), + ('acme/widgets', 'alice', 2, 'merged', '2026-06-12 00:00:00')`, + ).run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "submitter_outcome_log"); + expect(rule).toEqual({ table: "submitter_outcome_log", column: "recorded_at", days: 90 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT pull_number FROM submitter_outcome_log").all<{ pull_number: number }>(); + expect(rows.results.map((row) => row.pull_number)).toEqual([2]); + }); + + it("prunes alert_dedup_claims older than its 14-day window and keeps recent rows (#10058)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status, created_at) + VALUES + ('adc-old', 'acme/widgets', '__healthcheck__', 'hash-old', 'sent', '2026-01-01 00:00:00'), + ('adc-recent', 'acme/widgets', '__healthcheck__', 'hash-recent', 'sent', '2026-06-12 00:00:00')`, + ).run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "alert_dedup_claims"); + expect(rule).toEqual({ table: "alert_dedup_claims", column: "created_at", days: 14 }); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!] }); + expect(results[0]?.deleted).toBe(1); + const rows = await env.DB.prepare("SELECT id FROM alert_dedup_claims").all<{ id: string }>(); + expect(rows.results.map((row) => row.id)).toEqual(["adc-recent"]); + }); + + // pkColumnFor's two arms (src/db/retention.ts:211): alert_dedup_claims has a genuine `id` mapping in + // RETENTION_PK_COLUMN, while submitter_outcome_log's composite PK means it falls back to the `?? "rowid"` + // arm. A small batchSize also proves the batched-delete loop's `changes < batchSize` exit (line 420) fires + // on a real partial-then-final pair of iterations, not only on an empty first pass. + it("submitter_outcome_log's composite PK falls back to rowid ordering across multiple batches (#10058)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO submitter_outcome_log (project, submitter, pull_number, outcome, recorded_at) + VALUES + ('acme/widgets', 'alice', 1, 'merged', '2026-01-01 00:00:00'), + ('acme/widgets', 'alice', 2, 'closed', '2026-01-02 00:00:00'), + ('acme/widgets', 'alice', 3, 'merged', '2026-01-03 00:00:00'), + ('acme/widgets', 'bob', 1, 'merged', '2026-06-12 00:00:00')`, + ).run(); + + const rule = RETENTION_POLICY.find((r) => r.table === "submitter_outcome_log"); + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [rule!], batchSize: 2 }); + expect(results[0]?.deleted).toBe(3); + const remaining = await env.DB.prepare("SELECT count(*) AS n FROM submitter_outcome_log").first<{ n: number }>(); + expect(remaining?.n).toBe(1); + }); }); describe("dedupeSignalSnapshots", () => { @@ -976,6 +1039,13 @@ describe("retentionCutoffIsoForTable (#9474)", () => { // Within a second of a locally computed 180-day cutoff -- pins the default-arg arm without clock flake. expect(Math.abs(Date.parse(cutoff!) - (Date.now() - 180 * 86_400_000))).toBeLessThan(1000); }); + + // #10058 regression: pins the two windows so a future edit that drops or shrinks either entry fails loudly + // rather than silently restoring unbounded growth on submitter_outcome_log or alert_dedup_claims. + it("submitter_outcome_log is 90 days and alert_dedup_claims is 14 days (#10058)", () => { + expect(retentionDaysForTable("submitter_outcome_log")).toBe(90); + expect(retentionDaysForTable("alert_dedup_claims")).toBe(14); + }); }); // #9783: orb_signals is a PUBLIC data source (computeFleetAnalytics' /fairness headline, #9775's weekly fleet