diff --git a/apps/loopover-ui/content/docs/what-you-can-verify.mdx b/apps/loopover-ui/content/docs/what-you-can-verify.mdx index 939d99d781..a419e28e3c 100644 --- a/apps/loopover-ui/content/docs/what-you-can-verify.mdx +++ b/apps/loopover-ui/content/docs/what-you-can-verify.mdx @@ -29,8 +29,24 @@ hash, so any edit to history breaks the chain at a point you can locate. curl -s "https://api.loopover.ai/v1/public/decision-ledger/verify" | jq ``` -Returns `{ ok, checked, nextAfterSeq, tipSeq, tipHash, totalCount }`, and a `break` object with a -`409` status if the chain is inconsistent. No API key — **anyone** can run it. +Returns `{ ok, checked, nextAfterSeq, tipSeq, tipHash, totalCount, prunedRecords }`, and a `break` +object with a `409` status if the chain is inconsistent. No API key — **anyone** can run it. + +Two response fields encode deliberate, published semantics rather than tolerance for tampering: + +- **`prunedRecords`** — decision *records* (the preimages) are retained for 180 days; ledger rows are + kept forever. A ledger row whose hash-chained `createdAt` is older than that window may therefore + reference a pruned record: the chain checks still hold for it, only the content re-check is + impossible, and it is counted here instead of reported as `missing_record`. The tolerance keys on + the *ledger row's* timestamp, which is inside the hash chain — backdating it to sneak a fresh + deletion under the window breaks `row_hash_mismatch` first. What pruning genuinely gives up: the + row's committed digest stays published, but only a challenger holding the original preimage can + still prove a historical rewrite of it. +- **Append grace (5 minutes)** — a record and its chain row are two writes moments apart, so a + record younger than the grace window with no chain entry is "append in flight", not a break. Older + than that, it is reported: past the verified tip as `short_tail` (the truncated-tail signature), + or behind it as `unchained_record` (the failed-append signature — interior orphans are found by an + anti-join over the whole record set, not just the tail). **Trust assumption: tamper-evident, externally anchored.** The check above still catches sequence diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 8ff09caf01..1be226d847 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -20104,10 +20104,10 @@ "summary": "Verify a window of the hash-chained decision ledger (resumable via afterSeq)", "responses": { "200": { - "description": "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing." + "description": "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing, and prunedRecords — the count of rows whose record preimage was legitimately pruned by the published retention window (chain checks still hold for them; only the content re-check is impossible, and the committed digest stays published)." }, "409": { - "description": "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | short_tail (a record exists past the verified tip with no chain entry)" + "description": "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | missing_record | content_mismatch | short_tail (a record newer than the verified tip has no chain entry — the truncated-tail signature) | unchained_record (an INTERIOR record has no chain entry — the failed-append signature). Records younger than the 5-minute append grace window are not reported: the record insert and its chain append are two writes moments apart, and a verify landing between them is not evidence of tampering." } }, "security": [ diff --git a/migrations/0198_orb_outcome_rollups.sql b/migrations/0198_orb_outcome_rollups.sql new file mode 100644 index 0000000000..6eca2218f5 --- /dev/null +++ b/migrations/0198_orb_outcome_rollups.sql @@ -0,0 +1,25 @@ +-- #9474: durable running totals for orb_pr_outcomes' CUMULATIVE public consumer. +-- +-- #9415 gave orb_pr_outcomes a 90-day retention window, but getOrbGlobalStats SUMs the ENTIRE table and +-- public-stats folds that into the homepage's all-time merged/closed/handled counters. Once rows began aging +-- past 90 days (~2026-10-25 given #9415's merge date) the "all-time" numbers would have plateaued and then +-- visibly DECREASED -- a published cumulative counter going backwards. +-- +-- The retention prune now folds each about-to-be-deleted row into this table first, atomically (same batch +-- transaction as the delete -- see pruneExpiredRecords' orb_pr_outcomes special case), and getOrbGlobalStats +-- adds these totals to its live scan. Keyed per LOWERCASED account_login (matching the stats query's own +-- LOWER() comparison; '' for a NULL login) so its excludeAccount de-dup keeps working after the raw rows are +-- gone. Only rows the live query would have counted are folded: registered installations, no published +-- review surface. +CREATE TABLE IF NOT EXISTS orb_outcome_rollups ( + account_login TEXT PRIMARY KEY, + merged INTEGER NOT NULL DEFAULT 0, + closed INTEGER NOT NULL DEFAULT 0, + total INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL +); + +-- #9489/#9474: verifyDecisionLedger's completeness reconciliation now asks "does ANY ledger row vouch for +-- this record" (a NOT EXISTS anti-join finding interior orphans, not just tail ones); without this index +-- that is a full ledger scan per candidate record. +CREATE INDEX IF NOT EXISTS decision_ledger_record_id ON decision_ledger (record_id); diff --git a/scripts/check-schema-drift.ts b/scripts/check-schema-drift.ts index 602de96dda..3976ebfe5c 100644 --- a/scripts/check-schema-drift.ts +++ b/scripts/check-schema-drift.ts @@ -54,6 +54,9 @@ export const RAW_SQL_ONLY_TABLES: Set = new Set([ "orb_export_cursor", "orb_github_installations", "orb_instances", + // #9474: durable running totals folded from orb_pr_outcomes (itself raw-SQL-only, below) by the + // retention prune, and summed back in by getOrbGlobalStats -- both via env.DB.prepare, no Drizzle use. + "orb_outcome_rollups", "orb_pr_outcomes", "orb_relay_failures", "orb_reuse_counters", diff --git a/src/db/retention.ts b/src/db/retention.ts index 0ac1dc98dc..a922bb25c2 100644 --- a/src/db/retention.ts +++ b/src/db/retention.ts @@ -14,6 +14,14 @@ export type RetentionRule = { table: string; column: string; days: number }; const DURABLE_AUDIT_EVENT_TYPES = ["github_app.pr_public_surface_published"] as const; export const RETENTION_POLICY: readonly RetentionRule[] = [ + // #9474: MUST stay ahead of audit_events. This table's prune first FOLDS the rows it is about to delete + // into the durable orb_outcome_rollups running totals (see pruneExpiredRecords' special case), and that + // fold reuses getOrbGlobalStats' exact counting semantics -- including the LEFT JOIN that excludes + // outcomes whose PR already published a review surface (a durable audit_events row). Both tables share a + // 90-day window, so if audit_events pruned first within the same pass, the very audit rows that exclusion + // needs would be gone by the time the fold ran, and the rollup would permanently over-count exactly the + // PRs the live query never counted. + { table: "orb_pr_outcomes", column: "occurred_at", days: 90 }, { table: "audit_events", column: "created_at", days: 90 }, { table: "ai_usage_events", column: "created_at", days: 90 }, { table: "product_usage_events", column: "occurred_at", days: 180 }, @@ -74,7 +82,9 @@ export const RETENTION_POLICY: readonly RetentionRule[] = [ { table: "pull_request_files", column: "updated_at", days: 30 }, { table: "repo_github_totals_snapshots", column: "fetched_at", days: 30 }, { table: "recent_merged_pull_requests", column: "updated_at", days: 30 }, - { table: "orb_pr_outcomes", column: "occurred_at", days: 90 }, + // orb_pr_outcomes was #9415's fifth entry here; #9474 moved it to the TOP of this policy (ordering + // constraint documented there) and gave it a fold-before-delete so the cumulative public counter it + // feeds can never shrink. // #9473: four more members of the same re-derivable/per-event class #9415 bounded, found by an audit sweep // for tables written per event with NO delete path anywhere in src/. Two have a pruned sibling, which is // what makes the omission clearly unintentional rather than a retention decision: @@ -157,6 +167,20 @@ export const RETENTION_COMPOSITE_PK_TABLES: ReadonlySet = new Set([ "linked_issue_satisfaction_cache", ]); +/** + * The retention cutoff for `table` as of `nowMs` -- rows with a timestamp strictly BELOW this are eligible + * for pruning -- or null when the table has no retention rule at all. #9474: exported so consumers whose + * correctness depends on a table's permanence can reason about its IMPERMANENCE instead of silently assuming. + * verifyDecisionLedger uses this to tell "this record was legitimately pruned by the published retention + * policy" apart from "this record is missing and should not be": the distinction is keyed on the LEDGER row's + * hash-chained created_at (which cannot be backdated without breaking the chain), so an operator cannot use + * the tolerance to hide a fresh deletion. + */ +export function retentionCutoffIsoForTable(table: string, nowMs: number = Date.parse(nowIso())): string | null { + const rule = RETENTION_POLICY.find((candidate) => candidate.table === table); + return rule ? cutoffIso(rule.days, nowMs) : null; +} + function pkColumnFor(table: string): string { return RETENTION_PK_COLUMN[table] ?? "rowid"; } @@ -219,6 +243,84 @@ export async function pruneExpiredRecords( continue; } + // #9474: orb_pr_outcomes feeds a CUMULATIVE public counter (getOrbGlobalStats -> the homepage "all-time" + // merged/closed totals), so its rows must be folded into the durable orb_outcome_rollups totals in the + // same transaction that deletes them -- a fold and delete that could commit separately would either + // double-count (fold landed, delete didn't, next run re-folds) or under-count (delete landed, fold + // didn't). One atomic batch, both statements scoped to the identical cutoff, sidesteps both. The delete + // is deliberately UNBATCHED for this one table: the aging cohort is one row per fleet-wide PR terminal + // per day (hundreds at most, vs the six-figure log tables the batching exists for), and a bounded delete + // would reintroduce the split-commit problem for whatever the bound left behind. + if (rule.table === "orb_pr_outcomes") { + // #9474: this table feeds a CUMULATIVE public counter (getOrbGlobalStats -> the homepage "all-time" + // merged/closed totals), so every row must be folded into the durable orb_outcome_rollups totals in the + // SAME transaction that deletes it. A fold and delete that could commit separately would either + // double-count (fold landed, delete didn't, next run re-folds) or under-count (delete landed, fold + // didn't). + // + // BOUNDED, like every other table here. An earlier revision deleted the whole aged cohort in one + // unbatched statement, arguing the daily volume is small -- true today, and exactly the argument that + // ages badly (one fleet-wide backfill and it is a multi-million-row statement). Instead each slice + // picks a TIMESTAMP boundary and both statements share the identical `occurred_at` predicate, so they + // provably see the same rows without either depending on rowid/ctid (which pg-dialect rewrites to a + // PHYSICAL location -- the #9470 hazard) or on row-value tuple syntax (uneven SQLite support). + // + // Slice boundary = the `occurred_at` of the row at OFFSET batchSize-1 among expired rows, ascending, + // taken INCLUSIVELY (`<=`). Inclusive is what makes progress guaranteed even when many rows share one + // timestamp: an exclusive `<` boundary against a run of ties selects zero rows and spins forever (there + // is a dedicated regression test for exactly that). The slice is therefore batchSize rows plus any ties + // at the boundary, and the final slice (no row at that offset) takes the remainder with the real cutoff. + let deleted = 0; + for (;;) { + const boundaryRow = await env.DB.prepare( + `SELECT ${rule.column} AS boundary FROM ${rule.table} WHERE ${retentionWhere(rule)} ORDER BY ${rule.column} LIMIT 1 OFFSET ${batchSize - 1}`, + ) + .bind(cutoff) + .first<{ boundary: string }>(); + // `== null` deliberately: D1 drivers disagree on .first() returning null vs undefined for no-row. + const sliceIsFinal = boundaryRow == null; + const sliceWhere = sliceIsFinal ? `${rule.column} < ?1` : `${rule.column} <= ?1`; + const sliceBound = sliceIsFinal ? cutoff : boundaryRow.boundary; + const batchResults = await env.DB.batch([ + // Fold EXACTLY the population getOrbGlobalStats counts: registered installations only, and only + // outcomes whose PR never published a review surface (those are already counted by the own ledger). + // Rows failing either filter are deleted WITHOUT folding -- the live query never counted them, so + // folding them would make the public total jump on prune day. Keyed per lowercased account_login so + // the stats query's excludeAccount de-dup keeps working against the rollup after the raw rows are gone. + env.DB.prepare( + `INSERT INTO orb_outcome_rollups (account_login, merged, closed, total, updated_at) + SELECT LOWER(COALESCE(i.account_login, '')) AS account_login, + SUM(CASE WHEN o.outcome = 'merged' THEN 1 ELSE 0 END) AS merged, + SUM(CASE WHEN o.outcome = 'closed' THEN 1 ELSE 0 END) AS closed, + COUNT(*) AS total, + ?2 AS updated_at + FROM orb_pr_outcomes o + JOIN orb_github_installations i ON i.installation_id = o.installation_id AND i.registered = 1 + LEFT JOIN audit_events ae + ON ae.target_key = o.repository_full_name || '#' || o.pr_number + AND ae.event_type = 'github_app.pr_public_surface_published' + WHERE o.${sliceWhere} AND ae.id IS NULL + GROUP BY LOWER(COALESCE(i.account_login, '')) + ON CONFLICT(account_login) DO UPDATE SET + merged = orb_outcome_rollups.merged + excluded.merged, + closed = orb_outcome_rollups.closed + excluded.closed, + total = orb_outcome_rollups.total + excluded.total, + updated_at = excluded.updated_at`, + ).bind(sliceBound, nowIso()), + env.DB.prepare(`DELETE FROM ${rule.table} WHERE ${sliceWhere}`).bind(sliceBound), + ]); + /* v8 ignore next 2 -- defensive: batch() returns exactly one result per statement on both backends, so + * the `?.`/`?? 0` arms only satisfy the driver types; a missing meta degrades the COUNT, never the prune. */ + const changes = Number(batchResults[1]?.meta?.changes ?? 0); + deleted += changes; + // The final slice always ends the loop; a non-final slice that somehow removed nothing would too, + // so a driver anomaly can never spin here. + if (sliceIsFinal || changes === 0 || deleted >= maxPerTable) break; + } + results.push({ table: rule.table, column: rule.column, cutoff, deleted }); + continue; + } + let deleted = 0; // Batched delete by a real indexable PK (see RETENTION_PK_COLUMN) ordered by the retention column, so // each statement is bounded AND the inner SELECT is an index range scan on Postgres, not a ctid-keyed diff --git a/src/openapi/spec.ts b/src/openapi/spec.ts index 42a42c1672..7f49cb953f 100644 --- a/src/openapi/spec.ts +++ b/src/openapi/spec.ts @@ -1641,8 +1641,8 @@ export function buildOpenApiSpec() { path: "/v1/public/decision-ledger/verify", summary: "Verify a window of the hash-chained decision ledger (resumable via afterSeq)", responses: { - 200: { description: "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing." }, - 409: { description: "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | short_tail (a record exists past the verified tip with no chain entry)" }, + 200: { description: "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing, and prunedRecords — the count of rows whose record preimage was legitimately pruned by the published retention window (chain checks still hold for them; only the content re-check is impossible, and the committed digest stays published)." }, + 409: { description: "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | missing_record | content_mismatch | short_tail (a record newer than the verified tip has no chain entry — the truncated-tail signature) | unchained_record (an INTERIOR record has no chain entry — the failed-append signature). Records younger than the 5-minute append grace window are not reported: the record insert and its chain append are two writes moments apart, and a verify landing between them is not evidence of tampering." }, }, }); registry.registerPath({ diff --git a/src/orb/outcomes.ts b/src/orb/outcomes.ts index befbec7119..b00e20728d 100644 --- a/src/orb/outcomes.ts +++ b/src/orb/outcomes.ts @@ -64,10 +64,19 @@ export async function getOrbGlobalStats(env: Env, opts: { excludeAccount?: strin // excludeAccount de-dups an account already counted by another source. "" = include all. const exclude = (opts.excludeAccount ?? "").toLowerCase(); let row: { merged: number | null; closed: number | null; total: number | null } | null; + let rollup: { merged: number | null; closed: number | null; total: number | null } | null; // #8879: guard ONLY the query. A D1 error degrades to zeros, mirroring computeFleetAnalytics's try/catch // (src/orb/analytics.ts) so a failure on this join drops just the orb aggregate instead of 503-ing the entire // /v1/public/stats payload (accuracyTrend/reuseRateTrend/reviewVolumeTrend/rulePrecision) via the route catch. try { + // #9474: rows older than the 90-day retention window are folded into orb_outcome_rollups by the prune + // (atomically with their deletion -- see pruneExpiredRecords) precisely so THIS cumulative total never + // shrinks. Rollup rows key on the lowercased account_login, so the same excludeAccount de-dup applies. + rollup = await env.DB.prepare( + `SELECT SUM(merged) AS merged, SUM(closed) AS closed, SUM(total) AS total FROM orb_outcome_rollups WHERE (?1 = '' OR account_login <> ?1)`, + ) + .bind(exclude) + .first<{ merged: number | null; closed: number | null; total: number | null }>(); row = await env.DB.prepare( `SELECT SUM(CASE WHEN o.outcome = 'merged' THEN 1 ELSE 0 END) AS merged, @@ -88,5 +97,10 @@ export async function getOrbGlobalStats(env: Env, opts: { excludeAccount?: strin } /* v8 ignore next -- an aggregate query always returns exactly one row; this guards the nullable .first() type only */ if (!row) return { merged: 0, closed: 0, total: 0 }; - return { merged: row.merged ?? 0, closed: row.closed ?? 0, total: row.total ?? 0 }; + // The SUM() arms are genuinely reachable NULLs, not defensive: an aggregate over zero rows returns NULL. + return { + merged: (row.merged ?? 0) + (rollup?.merged ?? 0), + closed: (row.closed ?? 0) + (rollup?.closed ?? 0), + total: (row.total ?? 0) + (rollup?.total ?? 0), + }; } diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index f1e9897452..a2ed02838b 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -41,6 +41,7 @@ // #9135 (also v4): `divertedByHoldout` records when the randomized close-audit holdout (#8831) converted this // decision's plan from a heuristic close to a hold — see that field's own doc comment. import { errorMessage, nowIso } from "../utils/json"; +import { retentionCutoffIsoForTable } from "../db/retention"; /** Bump when the record's FIELD SET changes meaning — consumers compare records only within a version. */ export const DECISION_RECORD_SCHEMA_VERSION = "5"; // v5 (#8834): + aiAgreement (inter-run agreement folded with the verbalized confidence); v4 (#9124/#9135): configDigest digests the resolved policy (+ settingsDigest split out), promptDigest digests the actual sent prompt, modelId -> modelIds (real identities), ciState populated, + divertedByHoldout; v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments @@ -397,7 +398,27 @@ export type LedgerBreak = // #9078: a ledger row commits to a decision_records id that no longer has a row at all — the one preimage // the chain vouched for is simply gone (a direct-DB deletion, or some other operation none of the // gap/predecessor/hash checks above can see, since those only ever compare ledger rows against each other). - | { kind: "missing_record"; atSeq: number; recordId: string }; + // #9474: NOT reported for a record whose hash-chained ledger created_at is older than the published + // decision_records retention window — that absence is the retention policy doing its job, and reporting it + // as tampering would make legitimate pruning indistinguishable from evidence destruction. The verify result + // counts such rows in `prunedRecords` instead. + | { kind: "missing_record"; atSeq: number; recordId: string } + // #9489: a record older than the append grace window with NO ledger row vouching for it, at a created_at at + // or before the verified tail — i.e. an INTERIOR orphan the short_tail check could never see, because that + // check only ever compared the tail. A permanently failed appendDecisionLedger call, or a targeted deletion + // of one interior ledger row... except the latter also breaks the seq chain, so in practice this is the + // failed-append signature. + | { kind: "unchained_record"; atSeq: number; recordId: string }; + +/** + * #9489: how old a decision_records row must be before its lack of a ledger row is treated as a break rather + * than an append still in flight. persistDecisionRecord inserts the record and appends its chain row in the + * same call, ordinarily milliseconds apart — but a verification running in that gap used to report + * `short_tail`, i.e. "tampering", on a PUBLIC endpoint, for a state every healthy write passes through. Five + * minutes is orders of magnitude beyond any healthy insert-to-append latency while still bounding how long a + * genuinely failed append (its own error-level alarm fires at the moment it happens) can hide from verify. + */ +export const LEDGER_APPEND_GRACE_MS = 5 * 60 * 1000; /** * Verify a window of the chain, resumable via `afterSeq` (0 = genesis). Reports the FIRST break with its @@ -417,8 +438,13 @@ export async function verifyDecisionLedger( env: Env, afterSeq = 0, limit = 500, -): Promise<{ ok: boolean; checked: number; nextAfterSeq: number | null; tipSeq: number; tipHash: string; totalCount: number; break?: LedgerBreak }> { +): Promise<{ ok: boolean; checked: number; nextAfterSeq: number | null; tipSeq: number; tipHash: string; totalCount: number; prunedRecords: number; break?: LedgerBreak }> { const bounded = Math.max(1, Math.min(1000, limit)); + // #9474: rows whose record preimage was legitimately pruned by the published retention policy (see the + // missing-record branch below). Surfaced in the result so "the chain is clean but N old preimages are no + // longer independently checkable" is an explicit, countable statement rather than silent. + let prunedRecords = 0; + const decisionRecordsPruneCutoff = retentionCutoffIsoForTable("decision_records"); const [totalRow, globalTip, prior] = await Promise.all([ env.DB.prepare("SELECT COUNT(*) AS n FROM decision_ledger").first<{ n: number }>(), env.DB.prepare("SELECT seq, row_hash AS rowHash FROM decision_ledger ORDER BY seq DESC LIMIT 1").first<{ seq: number; rowHash: string }>(), @@ -430,7 +456,7 @@ export async function verifyDecisionLedger( const tipSeq = globalTip?.seq ?? 0; const tipHash = globalTip?.rowHash ?? LEDGER_GENESIS_HASH; // `== null` deliberately: D1 drivers disagree on .first() returning null vs undefined for no-row. - if (afterSeq > 0 && prior == null) return { ok: false, checked: 0, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "sequence_gap", atSeq: afterSeq, expectedSeq: afterSeq } }; + if (afterSeq > 0 && prior == null) return { ok: false, checked: 0, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "sequence_gap", atSeq: afterSeq, expectedSeq: afterSeq } }; let prevHash = prior?.rowHash ?? LEDGER_GENESIS_HASH; let expectedSeq = afterSeq + 1; const { results } = await env.DB.prepare( @@ -456,23 +482,39 @@ export async function verifyDecisionLedger( // from) so a call that finds ZERO new rows still has an anchor to reconcile against. let lastVerifiedCreatedAt = prior?.createdAt ?? null; for (const row of results) { - if (row.seq !== expectedSeq) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "sequence_gap", atSeq: row.seq, expectedSeq } }; - if (row.prevHash !== prevHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "predecessor_mismatch", atSeq: row.seq } }; + if (row.seq !== expectedSeq) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "sequence_gap", atSeq: row.seq, expectedSeq } }; + if (row.prevHash !== prevHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "predecessor_mismatch", atSeq: row.seq } }; const recomputed = await ledgerRowHash(prevHash, { seq: row.seq, recordId: row.recordId, recordDigest: row.recordDigest, createdAt: row.createdAt }); - if (recomputed !== row.rowHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "row_hash_mismatch", atSeq: row.seq } }; + if (recomputed !== row.rowHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "row_hash_mismatch", atSeq: row.seq } }; // #9078: the promised record/ledger reconciliation — a row_hash chained cleanly can still commit to a // digest whose CONTENT has since been rewritten (or whose preimage is simply gone). Neither is visible to // the chain-only checks above, since those only ever compare ledger rows against each other. const storedRecord = recordsById.get(row.recordId); - if (!storedRecord) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "missing_record", atSeq: row.seq, recordId: row.recordId } }; - let recomputedContentDigest: string | null = null; - try { - recomputedContentDigest = await contentDigest(JSON.parse(storedRecord.recordJson)); - } catch { - // Unparseable record_json is itself proof the content no longer matches whatever the chain committed to. - recomputedContentDigest = null; + if (!storedRecord) { + // #9474: decision_records carries a 180-day retention window while ledger rows are kept forever, so + // roughly 180 days after that rule first bit, EVERY full-chain verification would have reported + // missing_record at the first pruned row -- a false tamper signal manufactured by a legitimate, + // published retention policy, on the one endpoint whose whole point is that a skeptic can trust it. + // The tolerance keys on the LEDGER row's created_at, which is inside the hash chain: backdating it to + // sneak a fresh deletion under the cutoff breaks row_hash_mismatch above first. What is genuinely + // given up is exactly what pruning gives up -- the content reconciliation for that row -- while the + // chain checks (already passed above) still hold; the digest the chain committed to remains published, + // so a challenger holding the original preimage can still prove a historical rewrite by hand. + if (decisionRecordsPruneCutoff !== null && row.createdAt < decisionRecordsPruneCutoff) { + prunedRecords += 1; + } else { + return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "missing_record", atSeq: row.seq, recordId: row.recordId } }; + } + } else { + let recomputedContentDigest: string | null = null; + try { + recomputedContentDigest = await contentDigest(JSON.parse(storedRecord.recordJson)); + } catch { + // Unparseable record_json is itself proof the content no longer matches whatever the chain committed to. + recomputedContentDigest = null; + } + if (recomputedContentDigest !== row.recordDigest) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, prunedRecords, break: { kind: "content_mismatch", atSeq: row.seq, recordId: row.recordId } }; } - if (recomputedContentDigest !== row.recordDigest) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "content_mismatch", atSeq: row.seq, recordId: row.recordId } }; prevHash = row.rowHash; lastVerifiedCreatedAt = row.createdAt; expectedSeq = row.seq + 1; @@ -490,15 +532,46 @@ export async function verifyDecisionLedger( // (`nextAfterSeq === null`; a paginated window still has more to verify first) and only when there is an // actual tip to anchor the comparison on (an entirely empty, never-yet-populated ledger has nothing to // truncate FROM, and predates this reconciliation by definition). + // + // #9489 reshaped this reconciliation twice over: + // 1. GRACE — the record INSERT and its ledger append are two writes milliseconds apart, and a verify + // landing between them used to report short_tail ("tampering") on a public endpoint for a state every + // healthy write passes through. A record younger than LEDGER_APPEND_GRACE_MS is now simply not yet + // due for reconciliation; a genuinely failed append still surfaces here on the next verify after the + // grace lapses (and fired its own error-level alarm at the moment it happened). + // 2. INTERIOR ORPHANS — the old check only compared created_at against the verified TAIL, so the moment + // any newer record chained cleanly, an unchained record behind it became invisible forever. The + // NOT EXISTS anti-join (indexed via decision_ledger_record_id, migration 0198) asks the real + // question — "does ANY ledger row vouch for this record?" — which catches both positions. The newest + // orphan decides the break kind: past the tail it is the truncated-tail signature short_tail always + // meant; at or before the tail it is an interior unchained_record. + // Records legitimately pruned by retention cannot false-positive here in either direction: pruning deletes + // the RECORD row, and this reconciliation only ever reports records that still exist. if (nextAfterSeq === null && lastVerifiedCreatedAt !== null) { - const orphaned = await env.DB.prepare("SELECT COUNT(*) AS n FROM decision_records WHERE created_at > ?").bind(lastVerifiedCreatedAt).first<{ n: number }>(); - /* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row (even {n: 0}); the `?? 0` - * only satisfies .first()'s optional-by-signature TS return type. */ - if ((orphaned?.n ?? 0) > 0) { - return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "short_tail", atSeq: expectedSeq - 1 } }; + const graceCutoffIso = new Date(Date.parse(nowIso()) - LEDGER_APPEND_GRACE_MS).toISOString(); + const orphan = await env.DB.prepare( + "SELECT id, created_at AS createdAt FROM decision_records r WHERE r.created_at <= ? AND NOT EXISTS (SELECT 1 FROM decision_ledger l WHERE l.record_id = r.id) ORDER BY r.created_at DESC LIMIT 1", + ) + .bind(graceCutoffIso) + .first<{ id: string; createdAt: string }>(); + // `== null` deliberately: D1 drivers disagree on .first() returning null vs undefined for no-row. + if (orphan != null) { + return { + ok: false, + checked, + nextAfterSeq: null, + tipSeq, + tipHash, + totalCount, + prunedRecords, + break: + orphan.createdAt > lastVerifiedCreatedAt + ? { kind: "short_tail", atSeq: expectedSeq - 1 } + : { kind: "unchained_record", atSeq: expectedSeq - 1, recordId: orphan.id }, + }; } } - return { ok: true, checked, nextAfterSeq, tipSeq, tipHash, totalCount }; + return { ok: true, checked, nextAfterSeq, tipSeq, tipHash, totalCount, prunedRecords }; } /** One ledger row, exactly as chained -- the shape `GET /v1/public/decision-ledger/row/:seq` returns. */ diff --git a/test/integration/orb-outcomes.test.ts b/test/integration/orb-outcomes.test.ts index 1eaf2b6751..caa795bacc 100644 --- a/test/integration/orb-outcomes.test.ts +++ b/test/integration/orb-outcomes.test.ts @@ -130,3 +130,30 @@ describe("getOrbGlobalStats", () => { await expect(getOrbGlobalStats({ DB: nullRowDb } as unknown as Env)).resolves.toEqual({ merged: 0, closed: 0, total: 0 }); }); }); + +// #9474: the rollup read's `rollup?.` arms -- a driver anomaly resolving NO row for the rollup aggregate must +// degrade that term to zero, never null-poison the live totals it is added to. +it("degrades the rollup term to zero when its query resolves without a row, keeping the live totals intact (#9474)", async () => { + const anomalyDb = { + prepare: (sql: string) => ({ + bind: () => ({ + first: () => Promise.resolve(sql.includes("orb_outcome_rollups") ? null : { merged: 2, closed: 1, total: 3 }), + }), + }), + } as unknown as Env["DB"]; + await expect(getOrbGlobalStats({ DB: anomalyDb } as unknown as Env)).resolves.toEqual({ merged: 2, closed: 1, total: 3 }); +}); + +// The mirror-image anomaly: the LIVE row resolves with all-NULL fields (a degenerate driver reply) while the +// rollup carries real folded history -- the rollup must still be reported, with the null live terms at zero. +it("degrades all-NULL live fields to zero while still reporting folded rollup history (#9474)", async () => { + const anomalyDb = { + prepare: (sql: string) => ({ + bind: () => ({ + first: () => + Promise.resolve(sql.includes("orb_outcome_rollups") ? { merged: 1, closed: 2, total: 3 } : { merged: null, closed: null, total: null }), + }), + }), + } as unknown as Env["DB"]; + await expect(getOrbGlobalStats({ DB: anomalyDb } as unknown as Env)).resolves.toEqual({ merged: 1, closed: 2, total: 3 }); +}); diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index 63944ddbe2..76cce78e10 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -384,7 +384,7 @@ describe("decision ledger (#8837)", () => { it("verifying a completely empty ledger returns ok:true with a zero tip, and skips the tail-truncation check (nothing to anchor against yet)", async () => { const env = createTestEnv(); const verified = await verifyDecisionLedger(env); - expect(verified).toEqual({ ok: true, checked: 0, nextAfterSeq: null, tipSeq: 0, tipHash: LEDGER_GENESIS_HASH, totalCount: 0 }); + expect(verified).toEqual({ ok: true, checked: 0, nextAfterSeq: null, tipSeq: 0, tipHash: LEDGER_GENESIS_HASH, totalCount: 0, prunedRecords: 0 }); }); it("TAIL TRUNCATION now breaks verify instead of passing clean (#9122): dropping the newest ledger rows leaves an orphaned decision_records tail", async () => { @@ -532,3 +532,111 @@ describe("decision ledger (#8837)", () => { vi.restoreAllMocks(); }); }); + +// #9474 + #9489: the verifier's two relationships with ABSENCE. A record can be absent because the published +// retention policy pruned it (legitimate, must NOT read as tampering) or because something deleted it out of +// band (must). And a record can lack a chain row because the append is milliseconds in flight (legitimate) or +// because the append failed / the tail was truncated (must be reported -- including INTERIOR orphans, which +// the old tail-only comparison lost forever the moment any newer row chained). +describe("verifier vs absence (#9474 pruned records, #9489 grace + interior orphans)", () => { + const persist = async (env: Env, pull: number, action = "close") => { + const { record, recordDigest } = await buildDecisionRecord(recordInput({ pullNumber: pull, action })); + await persistDecisionRecord(env, record, recordDigest); + return recordDigest; + }; + /** Persist with the system clock frozen at `at` so the row's hash-chained created_at is genuinely old -- + * a post-hoc UPDATE of created_at would desync row_hash and turn every test below into row_hash_mismatch. */ + const persistAt = async (env: Env, pull: number, at: Date) => { + vi.useFakeTimers(); + try { + vi.setSystemTime(at); + await persist(env, pull); + } finally { + vi.useRealTimers(); + } + }; + const daysAgo = (days: number) => new Date(Date.now() - days * 24 * 60 * 60 * 1000); + + it("REGRESSION (#9474): a record pruned by the 180-day retention window verifies clean, counted in prunedRecords -- not reported as tampering", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(200)); // older than the decision_records window: prunable + await persistAt(env, 2, daysAgo(10)); // young: its record must survive + // Simulate exactly what pruneExpiredRecords does: delete the RECORD, never the ledger row. + const first = await env.DB.prepare("SELECT record_id AS id FROM decision_ledger WHERE seq = 1").first<{ id: string }>(); + await env.DB.prepare("DELETE FROM decision_records WHERE id = ?").bind(first!.id).run(); + + const verified = await verifyDecisionLedger(env); + + expect(verified.ok).toBe(true); + expect(verified.break).toBeUndefined(); + expect(verified.prunedRecords).toBe(1); + expect(verified.checked).toBe(2); // the chain checks still ran over BOTH rows + }); + + it("INVARIANT (#9474): a RECENT record deleted out of band is still missing_record -- the tolerance keys on the hash-chained ledger timestamp, so it cannot launder a fresh deletion", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(10)); // far inside the retention window + const first = await env.DB.prepare("SELECT record_id AS id FROM decision_ledger WHERE seq = 1").first<{ id: string }>(); + await env.DB.prepare("DELETE FROM decision_records WHERE id = ?").bind(first!.id).run(); + + const verified = await verifyDecisionLedger(env); + + expect(verified.ok).toBe(false); + expect(verified.break).toEqual({ kind: "missing_record", atSeq: 1, recordId: first!.id }); + expect(verified.prunedRecords).toBe(0); + }); + + it("REGRESSION (#9489): a verify landing between the record INSERT and its ledger append reports ok -- an in-flight write is not a tamper signal", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(1)); // an established, chained tip to reconcile against + // The in-flight state: record row present (created JUST now, inside the grace window), no ledger row yet. + await env.DB.prepare("INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) SELECT 'in-flight', repo_full_name, 99, head_sha, action, reason_code, record_digest, record_json, ? FROM decision_records LIMIT 1") + .bind(new Date().toISOString()) + .run(); + + const verified = await verifyDecisionLedger(env); + + expect(verified.ok).toBe(true); + expect(verified.break).toBeUndefined(); + }); + + it("INVARIANT (#9489): past the grace window the same unchained record IS reported -- the grace bounds the blind spot, it does not remove the check", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(2)); + // An orphan NEWER than the verified tail but well past the 5-minute grace: the truncated-tail signature. + await env.DB.prepare("INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) SELECT 'stale-orphan', repo_full_name, 99, head_sha, action, reason_code, record_digest, record_json, ? FROM decision_records LIMIT 1") + .bind(daysAgo(1).toISOString()) + .run(); + + const verified = await verifyDecisionLedger(env); + + expect(verified.ok).toBe(false); + expect(verified.break).toEqual({ kind: "short_tail", atSeq: 1 }); + }); + + it("REGRESSION (#9489): an INTERIOR orphan -- a failed append with newer rows chained cleanly after it -- is unchained_record, no longer invisible forever", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(3)); + await persistAt(env, 3, daysAgo(1)); // the newer, cleanly chained row that used to hide the orphan + // The failed-append signature: a record BETWEEN them (created_at behind the verified tail) with no chain row. + await env.DB.prepare("INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) SELECT 'interior-orphan', repo_full_name, 99, head_sha, action, reason_code, record_digest, record_json, ? FROM decision_records LIMIT 1") + .bind(daysAgo(2).toISOString()) + .run(); + + const verified = await verifyDecisionLedger(env); + + expect(verified.ok).toBe(false); + expect(verified.break).toEqual({ kind: "unchained_record", atSeq: 2, recordId: "interior-orphan" }); + }); + + it("INVARIANT: a fully healthy chain reports prunedRecords 0 and no break -- both new mechanisms are inert when nothing is absent", async () => { + const env = createTestEnv(); + await persistAt(env, 1, daysAgo(2)); + await persistAt(env, 2, daysAgo(1)); + + const verified = await verifyDecisionLedger(env); + + expect(verified).toMatchObject({ ok: true, checked: 2, prunedRecords: 0 }); + expect(verified.break).toBeUndefined(); + }); +}); diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index 5179d16b6f..97fc270a28 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -710,7 +710,8 @@ describe("getPublicStats — live aggregate over the review ledger", () => { const env = { DB: { prepare: (sql: string) => { - if (sql.includes("orb_pr_outcomes")) { + // #9474: getOrbGlobalStats now ALSO reads the durable orb_outcome_rollups fold (empty here). + if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) { return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }), }; @@ -731,6 +732,13 @@ describe("getPublicStats — live aggregate over the review ledger", () => { const env = { DB: { prepare: (sql: string) => { + // #9474: the rollup read must return the no-rows aggregate shape (all-NULL SUMs), so this test + // also pins that live-scan totals are NOT double-counted through the rollup arm. + if (sql.includes("orb_outcome_rollups")) { + return { + bind: () => ({ first: async () => ({ merged: null, closed: null, total: null }) }), + }; + } if (sql.includes("orb_pr_outcomes")) { return { bind: () => ({ first: async () => ({ merged: 12, closed: 8, total: 20 }) }), diff --git a/test/unit/retention.test.ts b/test/unit/retention.test.ts index 98b6fca21a..fda8607286 100644 --- a/test/unit/retention.test.ts +++ b/test/unit/retention.test.ts @@ -1,7 +1,8 @@ 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 } from "../../src/db/retention"; +import { dedupeSignalSnapshots, pruneExpiredRecords, RETENTION_COMPOSITE_PK_TABLES, RETENTION_PK_COLUMN, RETENTION_POLICY, retentionCutoffIsoForTable } from "../../src/db/retention"; +import { getOrbGlobalStats } from "../../src/orb/outcomes"; import { agentContextSnapshots, aiReviewCache, aiSlopCache, aiUsageEvents, groundingFileContentCache, linkedIssueSatisfactionCache, webhookEvents } from "../../src/db/schema"; import { processJob, runRetentionPrune } from "../../src/queue/processors"; import { REPO_FOCUS_MANIFEST_SIGNAL, REPO_PUBLIC_FOCUS_MANIFEST_SIGNAL } from "../../src/signals/focus-manifest-loader"; @@ -788,3 +789,171 @@ describe("dedupeSignalSnapshots survivor selection (#9470)", () => { expect(await countSignalSnapshots(env, "repo-culture-profile")).toBe(1); }); }); + +// #9474: orb_pr_outcomes feeds a CUMULATIVE public counter -- getOrbGlobalStats SUMs the whole table and +// public-stats folds it into the homepage's all-time merged/closed/handled totals. #9415's 90-day window, +// left alone, would have made those "all-time" numbers plateau and then visibly DECREASE (~2026-10-25). The +// prune now folds every row it deletes into the durable orb_outcome_rollups totals in the SAME transaction, +// and getOrbGlobalStats adds them back -- so retention never changes the meaning of a published number. +describe("orb_pr_outcomes fold-before-delete (#9474)", () => { + const RULE = { table: "orb_pr_outcomes", column: "occurred_at", days: 90 } as (typeof RETENTION_POLICY)[number]; + const seedOutcomes = async (env: Env) => { + await env.DB.prepare("INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (1, 'acme', 1), (2, 'JSONbored', 1), (3, 'stranger', 0)").run(); + await env.DB.prepare( + `INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES + ('acme/widgets', 1, 1, 'merged', ?1), + ('acme/widgets', 2, 1, 'closed', ?1), + ('jsonbored/loopover', 3, 2, 'merged', ?1), + ('stranger/repo', 4, 3, 'merged', ?1), + ('acme/widgets', 5, 1, 'merged', ?2)`, + ) + .bind(daysAgo(100), daysAgo(1)) + .run(); + }; + + it("REGRESSION: the cumulative public total is IDENTICAL before and after the prune, and the aged raw rows are gone", async () => { + const env = createTestEnv(); + await seedOutcomes(env); + const before = await getOrbGlobalStats(env); + expect(before).toEqual({ merged: 3, closed: 1, total: 4 }); // unregistered install 3 never counted + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE] }); + + expect(results[0]?.deleted).toBe(4); // ALL aged rows deleted, including the never-counted unregistered one + const remaining = await env.DB.prepare("SELECT pr_number FROM orb_pr_outcomes").all<{ pr_number: number }>(); + expect(remaining.results.map((row) => row.pr_number)).toEqual([5]); + expect(await getOrbGlobalStats(env)).toEqual(before); + }); + + it("INVARIANT: rows the live query never counted are deleted WITHOUT folding — an unregistered install and an own-ledger-published PR do not join the total on prune day", async () => { + const env = createTestEnv(); + await seedOutcomes(env); + // acme/widgets#1 was already counted by the own ledger (published surface): the live query excludes it, + // so the fold must too -- otherwise the public total would JUMP by one the day retention runs. + await env.DB.prepare( + "INSERT INTO audit_events (id, event_type, target_key, outcome, created_at) VALUES ('evt-1', 'github_app.pr_public_surface_published', 'acme/widgets#1', 'success', ?)", + ) + .bind(daysAgo(100)) + .run(); + const before = await getOrbGlobalStats(env); + expect(before).toEqual({ merged: 2, closed: 1, total: 3 }); + + await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE] }); + + expect(await getOrbGlobalStats(env)).toEqual(before); + const rollup = await env.DB.prepare("SELECT SUM(total) AS n FROM orb_outcome_rollups").first<{ n: number }>(); + expect(rollup?.n).toBe(2); // acme/widgets#2 + jsonbored/loopover#3; NOT the published #1, NOT the unregistered #4 + }); + + it("INVARIANT: a second prune run does not double-count — the fold and delete are one transaction, so re-running folds nothing new", async () => { + const env = createTestEnv(); + await seedOutcomes(env); + const before = await getOrbGlobalStats(env); + + await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE] }); + const second = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE] }); + + expect(second[0]?.deleted).toBe(0); + expect(await getOrbGlobalStats(env)).toEqual(before); + }); + + it("INVARIANT: excludeAccount still de-dups against FOLDED totals — the rollup keys on lowercased account_login", async () => { + const env = createTestEnv(); + await seedOutcomes(env); + const beforeExcluded = await getOrbGlobalStats(env, { excludeAccount: "jsonbored" }); + expect(beforeExcluded).toEqual({ merged: 2, closed: 1, total: 3 }); + + await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE] }); + + expect(await getOrbGlobalStats(env, { excludeAccount: "jsonbored" })).toEqual(beforeExcluded); + }); + + // Review feedback on #9532: the first revision deleted the whole aged cohort in ONE unbatched statement, + // arguing the daily volume is small. True today, and exactly the argument that ages badly -- one fleet-wide + // backfill makes it a multi-million-row statement, removing the batching safety net this file enforces for + // every other table. Slices are bounded now, and each slice's fold+delete still commit together. + it("REGRESSION: deletes in BOUNDED slices, not one unbatched statement — and the cumulative total is still exact", async () => { + const env = createTestEnv(); + await env.DB.prepare("INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (1, 'acme', 1)").run(); + // 25 aged rows with DISTINCT timestamps, so slicing has real boundaries to find. + const values = Array.from({ length: 25 }, (_, i) => `('acme/widgets', ${i + 1}, 1, '${i % 2 === 0 ? "merged" : "closed"}', '${daysAgo(200 - i)}')`).join(","); + await env.DB.prepare(`INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES ${values}`).run(); + const before = await getOrbGlobalStats(env); + expect(before.total).toBe(25); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE], batchSize: 10 }); + + expect(results[0]?.deleted).toBe(25); // every aged row removed, across multiple bounded slices + expect((await env.DB.prepare("SELECT COUNT(*) AS n FROM orb_pr_outcomes").first<{ n: number }>())?.n).toBe(0); + expect(await getOrbGlobalStats(env)).toEqual(before); // the public counter is untouched by the slicing + }); + + it("INVARIANT: a run of rows sharing ONE timestamp still makes progress — the inclusive slice boundary cannot spin", async () => { + // An exclusive `<` boundary against a tie run selects zero rows and loops forever. All 12 rows here share + // one occurred_at, so the first slice's boundary IS that timestamp: inclusive is what guarantees progress. + const env = createTestEnv(); + await env.DB.prepare("INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (1, 'acme', 1)").run(); + const tied = daysAgo(150); + const values = Array.from({ length: 12 }, (_, i) => `('acme/widgets', ${i + 1}, 1, 'merged', '${tied}')`).join(","); + await env.DB.prepare(`INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES ${values}`).run(); + + const results = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE], batchSize: 5 }); + + expect(results[0]?.deleted).toBe(12); + expect((await env.DB.prepare("SELECT COUNT(*) AS n FROM orb_pr_outcomes").first<{ n: number }>())?.n).toBe(0); + expect((await getOrbGlobalStats(env)).total).toBe(12); // all 12 folded exactly once, no double-count + }); + + it("INVARIANT: maxPerTable still caps one run, leaving the remainder (and its rollup) for the next", async () => { + const env = createTestEnv(); + await env.DB.prepare("INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (1, 'acme', 1)").run(); + const values = Array.from({ length: 20 }, (_, i) => `('acme/widgets', ${i + 1}, 1, 'merged', '${daysAgo(200 - i)}')`).join(","); + await env.DB.prepare(`INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES ${values}`).run(); + + const first = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE], batchSize: 5, maxPerTable: 10 }); + expect(first[0]?.deleted).toBe(10); + expect((await env.DB.prepare("SELECT COUNT(*) AS n FROM orb_pr_outcomes").first<{ n: number }>())?.n).toBe(10); + // The cumulative total is exact at EVERY point, not only once the cohort is fully drained. + expect((await getOrbGlobalStats(env)).total).toBe(20); + + const second = await pruneExpiredRecords(env, { nowMs: NOW, policy: [RULE], batchSize: 5, maxPerTable: 10 }); + expect(second[0]?.deleted).toBe(10); + expect((await getOrbGlobalStats(env)).total).toBe(20); + }); + + it("INVARIANT: dryRun counts without folding or deleting", async () => { + const env = createTestEnv(); + await seedOutcomes(env); + + const results = await pruneExpiredRecords(env, { dryRun: true, nowMs: NOW, policy: [RULE] }); + + expect(results[0]?.deleted).toBe(4); + expect((await env.DB.prepare("SELECT COUNT(*) AS n FROM orb_pr_outcomes").first<{ n: number }>())?.n).toBe(5); + expect((await env.DB.prepare("SELECT COUNT(*) AS n FROM orb_outcome_rollups").first<{ n: number }>())?.n).toBe(0); + }); + + it("ORDERING GUARD: orb_pr_outcomes prunes BEFORE audit_events, so the fold's own-ledger exclusion still sees the audit rows it needs", () => { + const tables = RETENTION_POLICY.map((rule) => rule.table); + expect(tables.indexOf("orb_pr_outcomes")).toBeGreaterThanOrEqual(0); + expect(tables.indexOf("orb_pr_outcomes")).toBeLessThan(tables.indexOf("audit_events")); + }); +}); + +// #9474: the exported cutoff helper is what lets a consumer reason about a table's IMPERMANENCE instead of +// silently assuming permanence -- verifyDecisionLedger keys its pruned-record tolerance on it, so the two can +// never drift apart the way the retention rule and the verifier originally did. +describe("retentionCutoffIsoForTable (#9474)", () => { + it("returns the policy cutoff for a covered table and null for a table with no rule", () => { + const cutoff = retentionCutoffIsoForTable("decision_records", NOW); + expect(cutoff).toBe(daysAgo(180)); // the published decision_records window, not a second hand-typed copy + expect(retentionCutoffIsoForTable("decision_ledger", NOW)).toBeNull(); // ledger rows are kept forever + expect(retentionCutoffIsoForTable("no_such_table", NOW)).toBeNull(); + }); + + it("defaults to the current clock when nowMs is omitted", () => { + const cutoff = retentionCutoffIsoForTable("decision_records"); + expect(cutoff).not.toBeNull(); + // 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); + }); +}); diff --git a/test/unit/selfhost-pg-retention.test.ts b/test/unit/selfhost-pg-retention.test.ts index 6d74653504..fa6fd4c472 100644 --- a/test/unit/selfhost-pg-retention.test.ts +++ b/test/unit/selfhost-pg-retention.test.ts @@ -52,9 +52,22 @@ function makeRetentionPgPool(remaining: Record = {}): MockPgPool if (/^insert into "?audit_events"?/i.test(q)) return { rows: [], rowCount: 1 }; + // #9474: the orb_pr_outcomes fold-then-delete pair. The fold INSERT..SELECT aggregates rows this mock + // never materializes (harmless: rowCount 0), and its unbatched DELETE drains the table's counter. + const orbDeleteMatch = /^DELETE FROM orb_pr_outcomes WHERE occurred_at < \$\d+$/i.exec(q); + if (orbDeleteMatch) { + const have = remaining["orb_pr_outcomes"] ?? 0; + remaining["orb_pr_outcomes"] = 0; + return { rows: [], rowCount: have }; + } + return { rows: [], rowCount: 0 }; }); - return { pool: { query: fn } as unknown as Pool, calls, remaining }; + // #9474: orb_pr_outcomes' fold-before-delete goes through the adapter's batch(), which checks out a + // dedicated client (BEGIN/COMMIT). The client delegates to the same query fn, so the rowid guard and the + // per-table row accounting above see batch-issued statements exactly like plain ones. + const client = { query: fn, release: vi.fn() }; + return { pool: { query: fn, connect: async () => client } as unknown as Pool, calls, remaining }; } function makeEnv(remaining: Record = {}): { env: Env; mock: MockPgPool } {