Skip to content

retention: submitter_outcome_log and alert_dedup_claims grow without bound, in the same class #9473 already bounded #10058

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

RETENTION_POLICY (src/db/retention.ts:16) is this repo's only delete path for append-only tables. #9473
added four entries found by "an audit sweep for tables written per event with NO delete path anywhere in
src/", and stated the two qualifying criteria in its own comment
(src/db/retention.ts:100src/db/retention.ts:110): a table written once per event, and every reader
already windowed, so aged rows are "pure dead weight".

Replaying migrations/*.sql and cross-referencing every INSERT INTO <table> in src/ against
RETENTION_POLICY and against every DELETE FROM <table> / .delete(<table>) in src/ finds two tables
that meet both criteria and were missed:

1. submitter_outcome_log (migrations/0189_submitter_outcome_log.sql:9) — one row per
(project, submitter, pull_number, outcome), appended on every PR terminal:

// src/review/submitter-reputation.ts:289
.prepare(`INSERT OR IGNORE INTO submitter_outcome_log (project, submitter, pull_number, outcome) VALUES (?, ?, ?, ?)`)

Its only reader is explicitly windowed:

-- src/review/submitter-reputation.ts:348
SELECT COUNT(*) AS submissions, ...
   FROM submitter_outcome_log
  WHERE project = ? AND submitter = ? AND recorded_at >= datetime('now', ?)

and the migration's own header says recorded_at exists precisely to give "the burst-detection signal a real
WINDOW to decay against". Nothing in src/ ever deletes from it. This is structurally identical to
contributor_gate_history (src/db/retention.ts:109, 90 days), which #9473 added with the reasoning
"every reader is already windowed … aged rows are pure dead weight".

2. alert_dedup_claims (migrations/0181_alert_dedup_claims.sql:9) — a pure hourly-expiring idempotency
claim. Its uniqueness key embeds an hour bucket, so a brand-new row is minted every hour, forever:

// src/review/alerts.ts:225
const hourBucket = new Date().toISOString().slice(0, 13); // YYYY-MM-DDTHH
const checkClaim = await storage(env).prepare(
  `INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status)
   VALUES (?, ?, '__healthcheck__', ?, 'sent')
   ON CONFLICT(project, target_id, notification_key) DO NOTHING`,
).bind(newId("hc"), config.slug, await sha256Hex(`healthcheck:${config.slug}:${hourBucket}`)).run();

plus a second row per distinct anomaly condition-set per hour (src/review/alerts.ts:244). No claim older
than the current hour is ever read again, and nothing deletes them. That is exactly the shape
RETENTION_POLICY already describes for webhook_events / orb_webhook_events
"short-lived idempotency lookups, not durable history" (src/db/retention.ts:75) — both bounded at 14 days.

Neither is a rebuildable cache and neither is an evidentiary trail; both are the "grew without bound … which
is what actually filled the hosted D1 to its 10GB ceiling" class documented at
src/db/retention.ts:79src/db/retention.ts:84, and both are on the self-host SQLite/Postgres backends too,
where nothing reclaims them either.

One trap the implementer must handle. Unlike every other policy table, both of these store their
timestamp in SQLite's CURRENT_TIMESTAMP format, not ISO-8601: recorded_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP (migrations/0189_submitter_outcome_log.sql:14) and created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP (migrations/0181_alert_dedup_claims.sql:15), and both writers omit the column, so the
DB default supplies 'YYYY-MM-DD HH:MM:SS'. pruneExpiredRecords binds an ISO cutoff
(cutoffIso, src/db/retention.ts:241) and compares as text, so on the cutoff date itself a
'2026-04-30 12:00:00' row sorts before '2026-04-30T00:00:00.000Z' (space 0x20 < T 0x54) and is
deleted up to a day early. That is acceptable inside a 90-day window but must be deliberate and tested, not
discovered later.

Requirements

  • Add two entries to RETENTION_POLICY (src/db/retention.ts:16), each with a comment stating why the table
    qualifies (per-event write, no delete path, windowed-only readers):

    • { table: "submitter_outcome_log", column: "recorded_at", days: 90 } — matching
      contributor_gate_history's window (src/db/retention.ts:109), which is the same windowed-reader shape.
    • { table: "alert_dedup_claims", column: "created_at", days: 14 } — matching webhook_events /
      orb_webhook_events (src/db/retention.ts:46, src/db/retention.ts:75), the same short-lived
      idempotency-claim shape.
  • Satisfy the drift guard at test/unit/retention.test.ts:716 ("every policy table is either PK-mapped or
    explicitly listed as composite-PK"):

    • alert_dedup_claims has id TEXT PRIMARY KEY (migrations/0181_alert_dedup_claims.sql:10) ⇒ add
      alert_dedup_claims: "id" to RETENTION_PK_COLUMN (src/db/retention.ts:128).
    • submitter_outcome_log has PRIMARY KEY (project, submitter, pull_number, outcome)
      (migrations/0189_submitter_outcome_log.sql:15) ⇒ add it to RETENTION_COMPOSITE_PK_TABLES
      (src/db/retention.ts:178) with the cost note that set's own doc comment requires.
  • Satisfy the drift guard at test/unit/retention.test.ts:730 ("every policy table has an index leading with
    its retention column somewhere in migrations/") by adding one new migration file
    migrations/0209_retention_column_indexes_round_three.sql creating:

    • 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);

    following migrations/0193_retention_column_indexes.sql and
    migrations/0196_retention_column_indexes_round_two.sql exactly in shape and header-comment style. 0209
    is the next free contiguous number (migrations/0208_manual_review_label_provenance.sql is the current
    highest).

  • A shipped migrations/*.sql file is immutable in this repo — do NOT edit 0181, 0189, 0193, 0196
    or any other existing migration, not even a comment. A new contiguous migrations/NNNN_*.sql is the only
    legal path for this schema change.
    Neither table's DDL may be altered; the timestamp columns keep their
    existing DEFAULT CURRENT_TIMESTAMP and their existing format.

  • Must NOT change: any existing RETENTION_POLICY entry's table, column or window; the ordering constraint
    that orb_pr_outcomes stays ahead of audit_events (src/db/retention.ts:17src/db/retention.ts:24);
    MAX_DELETED_PER_TABLE / BATCH_SIZE; or the two fold-before-delete special cases in
    pruneExpiredRecords (src/db/retention.ts:250).

⚠️ Required pattern: mirror #9473's own additions — the four entries at
src/db/retention.ts:100src/db/retention.ts:112 plus their RETENTION_PK_COLUMN mappings and the
migrations/0196_retention_column_indexes_round_two.sql index migration. What does NOT satisfy this
issue: (a) adding the policy entries without the index migration, which is precisely the #9472 regression
the drift guard at test/unit/retention.test.ts:730 exists to catch; (b) adding a bespoke delete path for
either table instead of a RETENTION_POLICY entry — a second mechanism beside the one prune job;
(c) editing migrations/0189 or migrations/0181 to change the column default or add the index there;
(d) a test-only PR.

Deliverables

  • RETENTION_POLICY (src/db/retention.ts) contains the two new entries with the windows above.
  • RETENTION_PK_COLUMN contains alert_dedup_claims: "id"; RETENTION_COMPOSITE_PK_TABLES contains
    submitter_outcome_log.
  • A new migrations/0209_retention_column_indexes_round_three.sql creates the two leading-column indexes
    named above, and no existing migration file is modified.
  • A test in test/unit/retention.test.ts seeding submitter_outcome_log with one row at
    recorded_at = '2026-01-01 00:00:00' (the DB-default format) and one at
    '2026-06-12 00:00:00', running pruneExpiredRecords(env, { nowMs: NOW }), and asserting the aged row
    is deleted, the recent one survives, and the returned PruneResult for the table reports
    deleted: 1.
  • The equivalent test for alert_dedup_claims at its 14-day window, seeded with created_at values in
    the same 'YYYY-MM-DD HH:MM:SS' format the writer actually produces.
  • A regression test named for this bug at test/unit/retention.test.ts asserting that
    retentionDaysForTable("submitter_outcome_log") is 90 and
    retentionDaysForTable("alert_dedup_claims") is 14 (src/db/retention.ts:202) — so a future edit
    that drops either entry fails loudly rather than silently restoring unbounded growth.
  • The three existing drift guards at test/unit/retention.test.ts:716,
    test/unit/retention.test.ts:725 and test/unit/retention.test.ts:730 still pass unmodified.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the two policy entries and the PK mappings but ships no index migration — does not resolve this
issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/db/retention.ts is measured. The two
policy entries add no new branch of their own, but they route through two existing branches that must each be
exercised by the new tests: pkColumnFor's RETENTION_PK_COLUMN[table] ?? "rowid"
(src/db/retention.ts:211) — alert_dedup_claims takes the mapped arm and submitter_outcome_log takes the
rowid fallback arm — and the generic batched-delete loop's changes < batchSize exit
(src/db/retention.ts:420). Seed enough rows in at least one of the two tests to prove the loop terminates on
the changes < batchSize condition rather than only on an empty first pass. The migration file itself is not
measured by Codecov, but the drift guard at test/unit/retention.test.ts:730 reads migrations/ directly and
will fail without it.

Expected Outcome

The two remaining per-event tables with no delete path anywhere in src/ are bounded by the same policy that
bounds their structurally identical siblings: submitter_outcome_log at 90 days (matching
contributor_gate_history, whose readers are windowed the same way) and alert_dedup_claims at 14 days
(matching the other short-lived idempotency logs). Both get a leading-column index so their batched delete is
an index range scan rather than a full scan, and the completeness guards keep all three sites in step.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions