Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions migrations/0211_retention_column_indexes_round_three.sql
Original file line number Diff line number Diff line change
@@ -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);
16 changes: 16 additions & 0 deletions src/db/retention.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -168,6 +179,8 @@ export const RETENTION_PK_COLUMN: Readonly<Record<string, string>> = {
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",
};

/**
Expand All @@ -191,6 +204,9 @@ export const RETENTION_COMPOSITE_PK_TABLES: ReadonlySet<string> = 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",
]);

/**
Expand Down
72 changes: 71 additions & 1 deletion test/unit/retention.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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
Expand Down