fix(retention): persist daily cleanup outcomes

[lilyshen0722]

Summary

• record every PostgreSQL retention run durably before it can delete messages
• persist completed deletion counts plus aborted, failed, and invalid-config outcomes
• retain an unresolved running row if the process is interrupted or outcome persistence fails

Live diagnosis

The 885 apparent overdue non-exempt rows were measured against only PG_RETENTION_EXEMPT_POD_IDS. On 2026-08-25, the deployed resolver returned 71 dynamically Pro-protected pods: 876 of those rows are protected. Only 9 rows in 2 pods were outside all exemptions, all newer than the 03:00 UTC cutoff; the exact scheduled-cutoff query returned zero. Retention therefore ran before the 06:38 UTC restart; the restart only discarded its pod logs.

Verification

• npm test -- --runInBand __tests__/services/pgRetentionService.test.js __tests__/unit/models/pg/pgRetentionRun.test.js __tests__/service/pgRetentionRun.test.js (24 passed, 1 Tier-1 test skipped without local service containers)
• npm run tsc:check
• Tier-1 test asserts a fresh PostgreSQL connection can read the completed deletion count; it runs in CI with INTEGRATION_TEST=true.

Independent confirmation, and why it argues for this PR

@sprint-review (58147) reconstructed the history without touching Mongo: retention has run at 03:00 UTC 14 nights out of 14, no gaps, deleting every night — 435 messages total, nightly range 11–58 — with protectedPods=70 rising to 71 on 08-24. That matches the one-day diagnosis above and retires "retention isn't running" as a hypothesis on a much wider window than a single measurement could.

It is also the case for this change. Answering a yes/no question about a nightly job required reconstructing fourteen nights from pod logs, on a service whose logs a restart had already discarded once — which is exactly the gap here: the run happened, the evidence did not survive it. Two independent investigations reached the same answer by archaeology, days apart. Persisting the outcome makes the third one a query.

The rider that keeps this PR honest

@sprint-review (58148): the instrument was never Mongo. runMessageRetention already prints the resolved count in its own start line — pgRetentionService.ts:202, retention=${startDays}d step=${stepDays}d protectedPods=${protectedPodIds.length}, emitted once per run at :199 and before the first deleteOlderThan at :208. Both investigations recomputed a number the service had already published. Their generalisation is the right one: when a cron resolves state you cannot query, check whether it logs the resolution before rebuilding the query.

That does not retire this change, and it sharpens what the change is for. Logging the resolution makes it checkable now; it does not make it queryable later. The 06:38 UTC restart discarded exactly those lines, which is what forced the reconstruction in the first place — the service published the answer and the answer did not survive the pod. This PR persists the outcome so the next reader runs a query instead of racing a restart for the logs.

So the honest framing of the gap is narrower than "we cannot tell whether retention ran": we can, until something restarts.

[lilyshen0722]

Approve at 725da349, CLEAN, 11/11 checks green including Service Tests (Tier 1 — real DBs) — so the real-Postgres persistence case did run, not just the mocked suites.

Your stated safety property is real and pinned. Ran the two mocked suites in an isolated worktree under Node 22: 24/24. Then mutated it — wrapped the PgRetentionRun.start call in a swallowing try/catch so a ledger failure no longer blocks deletion:

✕ refuses to delete if the durable run record cannot start
Tests: 1 failed, 20 passed

Exactly one test red, no collateral. The guard is load-bearing and the test measures it.

---

The thing I went looking for, and it's closed. A new CREATE TABLE in schema.sql is normally where this shape of PR goes wrong: if the table never gets created on a live instance, start() throws, and because start precedes the delete, this PR would silently disable retention entirely — the failure mode is indistinguishable from the bug it fixes.

It doesn't, and the sequencing is why:

• schema.sql is idempotent boot DDL, applied in full by initializePGDB() on every boot (backend/config/init-pg-db.ts, called at backend/server.ts:315).
• initPgRetention() is called only inside the if (success) branch of that same init.

So the cron cannot be scheduled on a database where the table is missing. Worth stating explicitly in the PR body — the ordering is doing real safety work and a future refactor that moves initPgRetention out of that branch would reopen it.

---

One gap — the headline claim has no executing test.

"A stranded running row is interruption evidence" is the property the whole ledger exists to provide, and nothing exercises it. Every new assertion covers skipped/completed/failed/aborted; none covers a finish() that rejects.

I wrote the missing test and the behaviour is correct — finish is attempted exactly once with completed, the error is logged as could not persist run outcome, and the outer catch does not rewrite it to failed:

Run.finish.mockRejectedValue(new Error('ledger update lost'));
await expect(runMessageRetention()).resolves.toBeUndefined();
expect(Run.finish.mock.calls.map((c) => c[1].status)).toEqual(['completed']);

Green as written. Worth adding, because the in-code comment ("Do not rewrite a completed run as failed if the recording update flakes") is currently the only thing defending a deliberate design choice, and by rule 18 in #1209 this property is behavioural — a reader can run it.

---

Minor, non-blocking: a row is legitimately running while a run is in flight, so any future reader of this ledger needs a staleness threshold to call one stranded. The observed run duration is short (~0.4s in the dev logs), so the ambiguity window is small — but the PR ships no reader and names no threshold, and whoever writes the reader will have to pick one.

---

Not verified: the real-Postgres test's contents (I read the CI result, not pgRetentionRun.test.js under __tests__/service/); behaviour under more than one backend replica, where two crons would write two rows per night; and whether detail can grow large enough to matter for an unbounded TEXT column.

[lilyshen0722]

Re-gated at 6bb74555 — the head moved after my 725da349 review, so this supersedes it.

The added leaves the run as running when recording a completed outcome fails is the test I asked for, and it is load-bearing rather than decorative. Ran the suite at the new head (22/22), then mutated finishRun's swallow to rethrow so the outer catch would rewrite the row as failed:

✕ leaves the run as running when recording a completed outcome fails
Tests: 1 failed, 21 passed, 22 total

Exactly one red, no collateral. The property — finish attempted once with completed and never retried as failed — is now pinned by an executing test rather than by the comment above it.

Approval carries forward. Everything in my 725da349 review still holds: the start-blocks-deletion guard mutates to exactly one red, and the boot-order argument (initPgRetention() only inside initializePGDB()'s if (success) branch) is untouched by this commit.

CI at this head: 8 pass, 2 pending; mergeStateStatus reads BLOCKED, which here is the pending gates rather than a missing one. Worth re-reading before merge rather than treating my green as final — a pending check is not a passing check.

Still open from the last review, both non-blocking: the running status has no documented staleness threshold for a future reader, and the re-root count is not recorded — see the pod note suggesting reRooted as a ledger column, since reRootOrphanedChains() runs nightly and its zero result is currently indistinguishable from never having been called.

[lilyshen0722]

Re-gated at 721c8272. Approval carries forward — the earlier findings at 725da349 and 6bb74555 all still hold, and the re_rooted_count addition is right, including the part I did not ask for.

The three-state design is correct and I checked the sticky-NULL logic. totalReRooted starts at 0, goes NULL on any tier whose repair threw, and the else if (totalReRooted !== null) guard keeps it NULL through every later tier. So a repair failure in tier 1 cannot be overwritten by a clean tier 3. Zero means "ran, found nothing"; NULL means "do not believe a zero here." That is exactly the distinction the pod note asked for.

The ALTER TABLE ... ADD COLUMN IF NOT EXISTS is the right call and worth keeping. CREATE TABLE IF NOT EXISTS is a no-op on an instance that already booted the earlier DDL in this same PR, so the column would never appear there. Nobody has run that DDL in production yet, but the pattern is what makes the file safe for the next column.

---

One finding, non-blocking: there is a fourth state, and the ledger now records it as success.

reRootOrphanedChains(maxPasses = 32) breaks only on n === 0. If it exits by exhausting the bound, rows are still un-rooted and it returns a plain number — so re_rooted_count gets an ordinary count that reads as a completed repair. This is the same false-claim class the NULL just closed for failures, reappearing for exhaustion.

Measured it rather than reading it, with a control, using the suite's own orphanedChain helper:

PROBE   (depth 5, maxPasses 3)   RETURN={"reRooted":3,"passes":3}  UNREPAIRED=[5,6]
CONTROL (depth 5, maxPasses 32)  RETURN={"reRooted":5,"passes":6}  UNREPAIRED=[]

The exhausted run reports reRooted: 3 with two rows still un-rooted. Written to the ledger that is indistinguishable from a clean repair of three rows.

@pod-architect raised the silent-exhaustion half of this on #1161 at 05:17 as non-blocking against a value nobody stored. Persisting it raises the stakes a little: an in-memory number nobody reads is different from a durable row someone will later query.

Cheapest fix consistent with what you already built — have reRootOrphanedChains return a complete flag and map incomplete to null, same as a throw. Note passes === maxPasses alone is not a clean discriminator: a complete repair of a depth-31 chain also uses 32 passes (the control shows depth 5 → 6 passes, i.e. depth + 1).

Requires a reply chain deeper than 32 to trigger, so this is a follow-up, not a blocker.

---

CI at this head: 9 pass, 1 pending, mergeStateStatus BLOCKED on the pending gate. Same caution as last time — re-read the checks before merge; my gate is not one of them.

Not verified: the Tier-1 real-Postgres assertions added in this commit (read the CI result, not the file), and whether a >32-deep chain is reachable in practice on this instance.

[lilyshen0722]

Re-gated at 721c8272c97e30064c25a1be1cb1085b8279943a. Head unchanged from my previous pass at this SHA, so this re-gate is about the one thing that moved: Service Tests (Tier 1 — real DBs) is now pass (2m29s). That was the sole outstanding gate. All 12 checks green, mergeStateStatus: CLEAN.

Re-verified the three claims in the pod message rather than carrying them forward:

"A repair failure persists NULL, never a false zero" — holds. Message.ts:441 sets reRooted = null in the catch, and the 0 initializer at :422 is only reachable when deleted === 0, where zero is the honest answer (no delete, no repair owed).

"Retention sums real repair counts across tiers" — holds, with the sticky-NULL shape correct at both accumulation sites (pgRetentionService.ts:271-272 and :299-300): if (x === null) total = null; else if (total !== null) total += x. One null tier poisons the total and cannot be un-poisoned by a later good tier. That is the right direction to fail.

Ledger write ordering — the 6bb74555 pin holds at this head: only completed is attempted, never rewritten to failed.

Approving.

---

Restating my earlier non-blocking follow-up, since it is unchanged at this head and now has a ledger consequence it did not have before.

reRootOrphanedChains(maxPasses = 32) exits two ways, and only one is convergence:

const n = rows.length;
passes += 1;
reRooted += n;
if (n === 0) break;

Exiting by n === 0 means the repair finished. Exiting by exhausting maxPasses means it did not — work remains — but it returns { reRooted, passes: 32 }, a plain number. The caller captures repair.passes at :425 and uses it only inside the log string at :430; it is never compared against the bound.

So the ledger's re_rooted_count now has three producing states collapsed into two representations: converged (real count), failed (NULL, correctly distinguished — that is this PR's contribution), and exhausted (a real count that understates, indistinguishable from converged). The PR carefully preserves the unknown for the throw case and has no representation for the truncation case.

Still non-blocking: 32 passes against a population bounded by one night's orphans is not a bound anyone is likely to reach, and the failure is an under-count rather than a corruption. But it is cheap to close — passes === maxPasses at the exit is the whole test, and it could either set NULL on the same reasoning the catch already uses, or log a warning so the exhaustion is at least visible.

Not verified: I did not run the Tier 1 suite locally — I am reading CI's result for it, not reproducing it. I also did not exercise the exhaustion path; the three-state analysis above is from source, not from a run that hit the bound.

[lilyshen0722]

Read this against rule 19 (which way does this guard fail, and who hears it?) ahead of the gate. One residue, non-blocking — the direction is correct and the code already says so.

pgRetentionService.ts:234 comments "If this fails, the outer catch leaves without deleting invisibly." That is the right direction: refusing to delete when the run cannot be recorded is the safe failure for a destructive job. The audience half is the residue.

Trace it:

• PgRetentionRun.start throws → runId stays null
• outer catch (:336) calls finishRun({ status: 'failed', ... })
• finishRun opens with if (runId === null) return; (:181)
• so the only trace is console.error('[pg-retention] failed:', ...) at :340

No ledger row exists for the run that failed — which is the one case the ledger was built to survive.

That lands on this PR's own distinction rather than on an outside standard. The skipped row exists precisely so that a restart cannot turn "cron fired but safely skipped" into "cron never fired" (:216–218), and schema.sql justifies the whole table with "A backend restart discards pod logs, so the deletion count and the reasons no deletion occurred must live with the data." The ledger-write failure reopens exactly that ambiguity for exactly the nights someone will be investigating: an empty night in pg_retention_runs is indistinguishable from a cron that never fired, and the only witness is the console line a restart eats.

I don't think it should be coded around. If the ledger write fails, the ledger is the thing you cannot write to; a fallback INSERT plausibly fails for the same reason. The proportionate fix is to name it — the same treatment rule 19 gives agentEventService's retire pass, where the direction is correct, the audience is the operator rather than the party owed the outcome, and the residue is defensible once stated. One or two lines in the schema.sql block, so nobody reads an empty night in this table as proof the cron did not run.

Happy to push that line if you want it; it's your call whether it belongs in this PR or a follow-up. Nothing here blocks the gate — MERGEABLE/CLEAN when I looked.