Retention gate: re-hold a policy whose coverage list grew, on a measured shortfall only (#1877)

[erikdarlingdata]

Closes #1877.

The gap

EnsureRetentionPoliciesAsync could only ARM. It creates each policy paused, evaluates coverage, and arms when the verdict is safe; when the verdict is unsafe it logs HELD PAUSED and moves on — but add_retention_policy(if_not_exists => true) returned -1 for the already-existing policy, so nothing paused it and an already-armed policy stayed armed.

That is right for a restart (#1680/#1759: re-pausing on every start would undo an operator's backfill). It was wrong for a policy whose coverage list GREW, which did not exist as a case until #1869 added a third consumer to a gate stores had already armed under #1849. Those stores log the hold every start and keep purging, which caps how deep the new day-grain daily can ever be backfilled at the interval layer's 7 days rather than at the store's own history.

Option chosen, and why

The issue offered three. This implements the first — disarm only on a positive measurement, never on an indeterminate verdict — because it is the only one of the three that satisfies all of the decision rule. The third (WARN at startup naming --backfill-rollups) does not re-hold at all, so the purge keeps outrunning the new tier while the operator reads the warning. The second (record the coverage list a policy was armed against) re-holds, but needs schema/state, and re-holds on a list CHANGE rather than on a coverage gap — so it would fire on a store where the new consumer was already caught up, and would miss a gap that arose any other way.

Option 1 needs no new state. The probe already had all three answers in it; they were being collapsed into a bool at the return statement. RetentionCoverage is Covered / Short / Unknown:

• Covered — arm, exactly as before.
• Short — a MEASUREMENT: the probe ran, the source holds rows, and a named consumer either holds none or starts later than the source's oldest row. This is the only verdict that may stop a running policy. A consumer that exists and answers with zero rows counts here, which is precisely the state a newly-added consumer is born in.
• Unknown — a throw, a timeout, an empty result. Neither arms nor holds; the policy is left in whatever state it is already in. A new one stays paused (fail-closed, unchanged), an armed one keeps purging.

That third value is the whole fix. The issue's own argument against "unsafe implies disarm" is that the probe is deliberately fail-closed, so one bad probe on a busy store would stop purging across every tier at once and grow disk until someone noticed. Separating the failed measurement from the measured shortfall is what removes that, and nothing else in the change would work without it.

The release path is the existing arming path, untouched — a re-held policy arms itself on the first start after the backfill reaches, with no manual step, exactly as a first-time hold has always released. So it is a gate, not a latch.

What it does to the #1793 property

Nothing, by construction: IsRawTierDropSafeAsync collapses Short and Unknown back to "not safe", which is byte-for-byte the old behaviour, so both purge paths still judge the same drop through the same map. It actually removes a case where they could disagree — for the three raw tiers, an armed policy used to keep dropping the very chunks the catalog sweep was refusing to touch.

Verification (live, PostgreSQL 18.4 / TimescaleDB 2.28.1)

Two new live tests in QueryStoreCorrectedRollupLiveTests, both against a scratch store.

The #1877 scenario end to end. Nine days of raw Query Store history, every consumer materialized over all of it, sweep → L1's purge armed and running (the state the arm-only gate could not reach). Then the coverage list grows the way #1869 grew it: the consumer is DROPped and rebuilt WITH NO DATA, CASCADEing to the day-grain daily, and the ensure sweep recreates both — the real upgrade shape, not a simulation of it. Next sweep: re-held. Backfill the consumer past L1's oldest bucket, next sweep: armed again, by itself.

The fail-closed property. A genuinely missing relation makes the probe throw 42P01 rather than measure — the strongest available stand-in for the timeout the rule exists for. Three assertions, because a hold-nothing implementation would pass the first alone: the armed policy whose probe threw stays armed, a different policy (query_stats, an unrelated rollup family) is still judged on its own merits in the same sweep, and a policy that is not yet armed when its probe throws stays paused — the original fail-closed direction, which the tristate must not have inverted. Plus a direct assertion that IsRawTierDropSafeAsync still answers "not safe" on an unreadable state.

Watched-red, per mutation:

mutation	result
Short branch skips its hold statement (i.e. back to arm-only)	re-hold test red at step 3, on its own message; other 5 green
probe's catch returns Short instead of Unknown	fail-closed test red on the disarm assertion

Three consecutive clean runs of the class after restoring, so the new tests are not load-sensitive.

Suites: Darling 3988 passed / 0 failed / 9 skipped with the live rig on; Lite 1849 passed / 0 failed. -t:Rebuild clean on Darling.Tests and Lite, 0 warnings each. No Installer.Tests.

One thing measured rather than assumed

The re-hold makes the horizon ordering load-bearing in a new way: an inverted pair would now stop a purge on a healthy store instead of merely failing to arm it. Steady state is safe by construction and both halves were checked rather than recalled — every consumer outlives its source (pinned), and materialization hypertables measured at a 10-day chunk interval against the raw tables' 1 day, so a consumer never retains less than its source at a chunk boundary. (My recollection had been "10x the bucket width", which the measurement contradicted for the hourly shape.) The test that pins the ordering now says what it is protecting, and gained the raw/hourly leg it was missing.

Filed rather than folded in

• Retention: the 'every consumer outlives its source' invariant is pinned by hand, not derived from the policy list — a new policy on a daily tier could stop a healthy purge #1905 — the ordering above is still asserted by hand for the pairs that exist today, not derived from the policy list, because the list is a local in the method body that Ungated catalog sweep bypasses the #1680 coverage gate: drop_chunks destroys rollup-uncovered raw history at 30 days #1784's source-parsing guard reads. A future policy on a daily tier would be green in CI and could reintroduce the risk from the other side.
• Retention: a RE-hold reads the same as a first-time hold in the log — the one that stopped a running purge is not distinguishable #1906 — a re-hold and a first-time hold produce the same WARN, so the one that stopped a running purge is not distinguishable. The prior armed state is not free to know and the clean single-statement form depends on a snapshot-timing argument that is the wrong risk to take for a log line; the operator action is identical either way.

🤖 Generated with Claude Code

[claude]

Review

Scope: Darling-only (TimescaleDB retention gate in TimescaleSupport.cs), plus matching tests and CHANGELOG. No Lite-side counterpart is needed — Lite's RetentionService is a wholly separate mechanism (deleting aged Parquet archive files by filename date), unrelated to continuous-aggregate coverage gating. No parity drift.

Correctness

• The RetentionCoverage tristate (Covered/Short/Unknown) is threaded through cleanly. MeasureRetentionCoverageAsync's column ordinals (source_oldest, then coverage_oldest_{i}) still line up 1:1 with RetentionArmSafetySql's generated column list and the coverageRelations indexing used to report ShortConsumer — verified by hand, looks correct.
• IsRawTierDropSafeAsync collapses Short and Unknown back to "not safe," preserving the Make the catalog sweep honor the #1680 coverage gate (#1784) #1793 property that both purge paths judge a drop identically. Good.
• The Unknown branch in EnsureRetentionPoliciesAsync genuinely touches nothing (no ALTER JOB call), so a probe failure can't disarm a running policy — matches the stated fail-closed intent, and the new RetentionSweep_TreatsAFailedProbeAsUnknown_NeverAsACoverageRegression live test exercises exactly this with a real 42P01 from a dropped relation.
• ArmRetentionPolicySql/HoldRetentionPolicySql sharing SetRetentionScheduleSql is a clean mirror-image refactor; the new HoldRetentionPolicy_IsTheMirrorOfArming_SameTargetOppositeFlag test pins that the two statements can't drift apart except on the boolean.
• Minor/non-blocking: in the Short branch, HoldRetentionPolicySql now runs unconditionally, including for a policy that was just created and already paused moments earlier in the same transaction (PauseJobSql). That's a redundant ALTER JOB round-trip in the newly-created case, but it's cheap, idempotent, and only runs at startup, so not worth special-casing.

Security

• SetRetentionScheduleSql/RetentionArmSafetySql interpolate relation directly into SQL text rather than parameterizing. This is pre-existing (unchanged by this PR — it was already string-interpolated in ArmRetentionPolicySql) and the values only ever come from the hardcoded policies tuple list in EnsureRetentionPoliciesAsync, never external input, so no new injection surface here.

Tests / docs

• The two new live tests are well-targeted (arm→re-hold→self-release, and fail-closed-not-disarm with a second independent policy to prove isolation), and the PR description documents mutation testing on both, which is good rigor.
• CHANGELOG entry accurately reflects the code changes (per-policy warning now names the specific short tier, startup summary reports the new indeterminate count).

Nothing blocking found. Nice, thorough piece of work — the fail-closed/measured-shortfall distinction is the right fix for the described problem, and the tests actually pin the failure modes the PR claims to guard against.