V75: plan content gets its own retention horizon (#2316)

[erikdarlingdata]

Implements the decision recorded on #2316 (Finding 1). The measurements that drove it are in that comment; the short version:

Why the existing bound cannot save this store

The dimension GC's fact-coupled horizon is a correctness guarantee (no live fact ever references deleted content) with a blind spot: it cannot bound a store younger than the fact retention. Measured on the use2 box: query_plan_dim hit 127 GB (63% of the store) in its first 22 days, ~6 GB/day of parameter-sniffing recompile churn — 344k distinct plan XMLs/day from 5,327 plan shapes (65 variants per shape; the worst single shape produced 57,402 in one day) — with the first coupled-GC delete mathematically impossible before ~Oct 27, about a month after projected disk-full (~late Sept at 249.8 GB free). Orphan pruning already exists and is healthy (n_tup_del=0 because nothing is eligible, not because it's broken); compression is spent (every row already app-gzipped, avg 9,750 B).

What this does

config_service.plan_content_retention_days — V75, default 21, clamps [7,365], 0 = disabled = the old behavior byte-for-byte. The dimension cutoff becomes the newer of the fact-coupled cutoff and now − (knob + 1) (the same one-day margin as the measured floor, covering the same hourly last_seen refresh guard).

• Facts keep their full 90-day retention — metrics, hashes, and text stay analyzable.
• A plan older than the window renders as the missing plan every reader already handles (ResolveContent → null).
• A knob wider than the fact horizon is deliberately a no-op — it must not become a way to keep XML nothing can reference.
• On the use2 box's exact shape, the first post-upgrade sweep starts deleting immediately (verified arithmetic below), and steady state lands near ~140 GB at current churn instead of ~500 GB.

Deliberately not done: shape-keyed latest-wins storage. It would shrink this 65×, but it breaks the historical-fact → exact-XML contract #1767 preserves on purpose — parameter-variant plans are the product's diagnostic bread and butter. If churn outgrows the horizon lever, that trade gets its own issue.

Recipe compliance

All four rung steps: Scripts entry + SchemaVersion 75; the three pin forms (the new PlanContentRetentionTests carries top-of-ladder + probe-maps-75; the V74 file is demoted to its keeps-true-forever form, including its InvokeMap arity — it appends the new parameter as false so its facts keep exercising the V74/V73 arms); viewer probe sentinel + ordinal 50 + newest-first arm above V74's. Knob plumbing follows the V59 pattern end to end: darling.json → seed → ReadServiceRowAsync (clamped) → StoreConfigView → ApplyToConfig → both PurgeAsync call sites (purge_now now receives the config it previously didn't need).

Verification scope

• Cutoff arithmetic + clamps ran against the real build via a net10.0 harness on this machine — 10/10, including the disabled-is-byte-identical fact and the field prediction for the use2 box's shape.
• Service, Viewer, and Darling.Tests build clean on macOS; the Windows suite and the live-PG job are the arbiters for the migration, probe, and pins in CI.
• Post-merge, the dogfood box's next nightly is the live proof: expect the daily purge to start reporting query_plan_dim deletions on its first sweep and disk-free to turn around within days.

🤖 Generated with Claude Code

[claude]

Correctness: plan_content_retention_days also shortens query_text_dim retention, not just plan XML.

dimensionCutoff computed here from ComputeDimensionCutoff(..., planContentRetentionDays) is applied uniformly to every table in PayloadDimensions.DimTables a few lines down (the foreach (var dimTable in PayloadDimensions.DimTables) loop), which is { query_text_dim, query_plan_dim } (PayloadDimensions.cs:114). Nothing scopes the dedicated horizon to the plan dimension only.

That means with the shipped default (21 days, enabled out of the box on every new store), a distinct SQL statement text row in query_text_dim that hasn't been re-seen in 22 days gets GC'd exactly like a plan — even though its facts (query_stats/procedure_stats) still have up to 90 days of retention left. From day ~23 to day 90, those facts would resolve to missing query text, not just a missing plan.

Every piece of this PR's own framing — the knob's name (plan_content_retention_days/PlanContentRetentionDays), the doc comments ("stored plan XML", "a plan older than the window renders as the missing plan"), the CHANGELOG entry ("gives plan CONTENT its own retention horizon" / "sets how long a stored plan XML outlives its last sighting"), and PlanContentRetentionTests.cs — describes this as scoped to plan content only. The #2316 measurement that motivated it was query_plan_dim-specific (127 GB from parameter-sniffing plan churn); nothing measured or justifies shrinking query_text_dim too.

If the intent really is "all payload dimension content shares one horizon," the knob's name/docs/tests should say so explicitly. If the intent is plan-only (as documented), the loop needs to special-case query_plan_dim with the dedicated cutoff and leave query_text_dim on the fact-coupled cutoff alone.

[erikdarlingdata]

Fixed in the follow-up commit: a pure router (ComputeDimTableCutoff) sends only query_plan_dim to the dedicated cutoff — query_text_dim (and any future dimension) keeps the fact-coupled one, so text stays resolvable for the facts' full retention. Pinned by TheRouterScopesTheKnobToThePlanDimensionOnly, and the CHANGELOG entry now states the scoping.

[claude]

The new V75 doc comment/field got inserted between the pre-existing V74 doc comment and the V74Sql field it documents, orphaning it: lines 1693-1711 ("V74 — where the query-text fetch lands statement text...") now sit directly above V75Sql instead of V74Sql, stacked with a second <summary> block for the same field. Net effect: V74Sql (line 1729) ends up with no doc comment at all, and V75Sql ends up with two concatenated <summary> elements — the first one describing V74's design, not V75's.

Move the V75 doc block (1712-1724) to sit directly above private const string V75Sql with nothing in between, and leave the V74 doc block where it originally was, directly above V74Sql.

[erikdarlingdata]

Fixed two commits back (the CI hygiene guard caught it in the same round): the V75 block now sits above V74's doc comment, both consts carry their own summary, and the local reimplementation of the detector's rule reports no stacked openings.

[claude]

Reviewed the V75 plan-content-retention change. This PR only touches the Darling app (PostgreSQL-backed service); Lite stores data locally in DuckDB/Parquet with no equivalent shared-store GC concept, so no parity gap there.

Left two inline comments:

1. Correctness (significant): the plan_content_retention_days cutoff in DarlingRetention.PurgeAsync is applied uniformly to all PayloadDimensions.DimTables, which includes both query_plan_dim and query_text_dim. The knob's name, docs, CHANGELOG entry, and tests all describe it as scoped to plan XML only, but as implemented it also GCs deduplicated SQL statement text after the same 21-day default window — even while the facts referencing that text still have up to 90 days left. That's a behavior change beyond what's documented/tested, shipped enabled-by-default on every new store.
2. Doc bug (minor): in PgMigrations.cs, the new V75 doc comment/field got inserted between the existing V74 doc comment and the V74Sql field, orphaning that comment and leaving V74Sql undocumented.

Everything else — the clamp semantics, the disabled=byte-identical-old-behavior guarantee, the migration/schema-version/viewer-probe rung mechanics, and the V59-pattern config plumbing (darling.json → seed → read → view → apply → both purge call sites) — looks correct and is well covered by the new tests.

[claude]

Correctness: this can prune query_plan_dim rows a live query_store_plan_map row still points at — the exact silent-missing-plan failure the map/dim ordering exists to prevent.

ComputeDimensionCutoff's result feeds the GC for every table in PayloadDimensions.DimTables, which includes query_plan_dim — the same dimension table Query Store's plan fetch writes into via the digest in collect.query_store_plan_map (see QueryStorePlanMap.cs). That map's own doc comment is explicit that Query Store facts carry no digest column, so they are invisible to the "measured floor" safety net here, and the only protection they get is (a) TouchSql refreshing the dim row's last_seen in the same pass as the map row's, and (b) mapCutoff in PurgeAsync (line ~414, utcNow.AddDays(-(widestFactRetentionDays + QueryStorePlanMap.PruneMarginDays))) being calibrated to always be later (more aggressive) than dimensionCutoff, so the map is always pruned before the dim it points to. That ordering is pinned by DarlingDimensionGcBoundTests.DimensionOutlivesTheMap_AtEveryFactRetention — but that test (untouched by this PR) never passes planContentRetentionDays, so it keeps passing even though the invariant it guards is now broken in production.

With the shipped default (planContentRetentionDays = 21, applied to every existing store on upgrade via the V75 DEFAULT 21) and the default 30-day query_stats/procedure_stats retention:

• dedicated = now - 22
• coupled (old behavior) = now - 32
• so the new dimensionCutoff = now - 22 (the newer of the two wins per line 684)
• but mapCutoff is untouched by this change and stays now - 31

That's a 9-day window (and on the dogfood fleet's actual 90-day fact retention that the CHANGELOG cites, a 69-day window: now-22 vs now-91) where a query_plan_dim row is eligible for deletion while the query_store_plan_map row — and any Query Store fact — that still references it survives. Per QueryStorePlanMap's own docs, that's the one unrecoverable outcome this design was built to avoid: "a reader resolving a live fact to absent content, silently, weeks after the cause."

Since mapCutoff is derived from widestFactRetentionDays alone and never learns about planContentRetentionDays, this isn't an edge case — it reproduces on any store with Query Store plan capture enabled, right out of the box on upgrade. Should mapCutoff (and the MarginOrderingHolds check) fold in planContentRetentionDays too, or should the new knob's dedicated cutoff be excluded from query_plan_dim specifically when Query Store references are in play?

[erikdarlingdata]

Fixed in the follow-up commit — and thank you, this was the sharpest catch of the round: the ordering pin never passed the knob, so it guarded an invariant the PR had already broken. ComputeMapCutoff now folds the knob in (map at knob, dim at knob+1 — the one-day gap covers the stamp skew from TouchSql's eager map refresh vs the dim's hourly guard), both cutoff components are strictly ordered so the max-of-newer composition preserves dimension-outlives-the-map under every knob value, and the new NeitherPruneOrder_CanLeaveAMapRowResolvingToAnAbsentDigest_UnderTheKnob sweeps every age across (retention × knob) including the shipped default and both clamp edges. The visible consequence is deliberate and now documented at the call site: a QS plan fetch older than the knob misses, same as the dim itself.

[claude]

Review summary

Reviewed this against CONTRIBUTING.md's style guide (no CLAUDE.md in this repo) and the Lite/Darling parity requirement. This PR is Darling-only, which is correct — the payload-dimension GC and config_service PostgreSQL store this feature touches have no Lite counterpart (Lite is DuckDB-backed with no dimensional star schema), so there's no parity drift to flag here.

One correctness finding, left as an inline comment on DarlingRetention.cs: the new planContentRetentionDays knob feeds ComputeDimensionCutoff, which governs GC for all PayloadDimensions.DimTables — including query_plan_dim, the table Query Store's plan fetch also writes into via collect.query_store_plan_map. The map's cutoff (mapCutoff in PurgeAsync) was deliberately calibrated to always prune before the dimension cutoff, so a live map row can never point at deleted content — an invariant pinned by DarlingDimensionGcBoundTests.DimensionOutlivesTheMap_AtEveryFactRetention. This PR doesn't touch mapCutoff, so with the shipped default (21 days, applied to every existing store on upgrade) the dimension cutoff can now land later than the map cutoff — reproducible with just the default 30-day fact retention, and dramatically so at the 90-day retention the CHANGELOG entry's own numbers cite. That reopens the exact "reader resolves a live fact to absent content, silently" failure mode the map/dim ordering exists to prevent, and none of the new tests (nor the untouched DarlingDimensionGcBoundTests) exercise the interaction, so nothing in CI currently catches it.

Everything else — the migration (schema-qualified, additive, DEFAULT 21 for byte-identical-when-0 semantics), the clamp bounds, the V59-pattern config plumbing (darling.json → seed → ReadServiceRowAsync → StoreConfigView → ApplyToConfig → both PurgeAsync call sites), the viewer schema probe/rung numbering, and the QueryStoreTextStoreTests demotion to its "keeps true forever" form — looks consistent and well-tested in isolation.

[claude]

planContentRetentionDays isn't clamped at the destructive sink, unlike its sibling parameter right above it in this same function.

retentionDaysFor gets a documented belt-and-suspenders clamp a few lines down (DarlingRetention.cs:173-176: "Clamp at the destructive sink (belt-and-suspenders...): a retention of 0/negative would flip the cutoff into the present/future... Never purge with a horizon under 1 day."), and RunPurgeNowAsync's doc comment (DarlingWorker.cs:3167) explicitly relies on that guarantee: "PurgeAsync clamps a sub-1-day horizon at its destructive sink, so a bad custom-N can never wipe a table."

planContentRetentionDays gets no equivalent treatment here — it flows straight into ComputeDimensionCutoff/ComputeMapCutoff unclamped. Both call sites in DarlingWorker.cs (lines 1405-1408 and 3182-3184) pass config.PlanContentRetentionDays raw, whereas the two other live-swappable knobs in this file are defensively re-clamped at every point of use (StoreConfigProvider.ClampTextBudgetMb(config.QueryStoreTextBudgetMb) at DarlingWorker.cs:1032/1173, StoreConfigProvider.ClampConcurrentSweeps(config.MaxConcurrentSweeps) at DarlingWorker.cs:1194/1223).

The value only reaches PurgeAsync pre-clamped when configProvider.LoadViewAsync has succeeded and ApplyToConfig has run (its ReadServiceRowAsync calls ClampPlanContentRetentionDays) — but if the store is unreachable at startup, initialView is null, ApplyToConfig never runs, and config.PlanContentRetentionDays keeps whatever raw value came from darling.json (default 21, but a user-set value is untouched) until the next successful control-plane reload. If that raw value is e.g. 1-6 (below the documented [7,365] floor), the daily/on-demand purge runs with a horizon narrower than the codebase's own stated minimum — the failure direction here is data loss (plan XML deleted earlier than the [7,365] contract promises), not merely a display/UX issue.

Suggest clamping inside PurgeAsync itself (ComputeDimensionCutoff/ComputeMapCutoff call sites) via StoreConfigProvider.ClampPlanContentRetentionDays(planContentRetentionDays), matching the "clamp at the destructive sink" precedent already documented in this function.

[erikdarlingdata]

Fixed: PurgeAsync clamps first thing via ClampPlanContentRetentionDays, with the same belt-and-suspenders framing as retentionDaysFor's sink clamp below it — the sink no longer trusts its callers, and the store-unreachable-boot path can't prune below the [7,365] contract.

[claude]

Minor: this last sentence doesn't match the actual control flow. The disabled case (planContentRetentionDays <= 0) takes an early return coupled; a few lines up — it never reaches a dedicated-vs-coupled comparison, so DateTime.MinValue never enters the picture at all (no dedicated value is even computed on that path). The comment reads as if disabling degrades via losing a max-comparison against MinValue, which would only be true if the early return were removed in favor of always computing dedicated. Given this codebase's comments are treated as load-bearing documentation of the actual reasoning, worth tightening so a future reader doesn't infer a mechanism the code doesn't have.

[erikdarlingdata]

Fixed — the sentence now states the actual mechanism (early return before any dedicated value is computed) instead of the MinValue comparison the code never performs.

[claude]

Reviewed. This PR is Darling-only (C#/PostgreSQL retention knob) — no T-SQL, and Lite has no analog to flag for parity: Lite never stores plan XML at all (fetched live, never persisted), so there's nothing on that side for plan_content_retention_days to mirror. No SQL injection/secrets/network concerns — all DB access goes through parameterized Npgsql commands and a schema-qualified migration, consistent with the store's existing conventions.

The cutoff-ordering math (dimension-vs-map invariant, plan-vs-text dimension routing, disabled-is-byte-identical) checks out — I worked through the algebra by hand (ComputeDimensionCutoff/ComputeMapCutoff differ by a constant 1-day offset pointwise in both the coupled and dedicated components, which structurally guarantees dim < map for every knob value, not just the tested cases) and it holds.

Left two inline comments on DarlingRetention.cs:

• Correctness (the one worth fixing before merge): planContentRetentionDays skips the "clamp at the destructive sink" treatment this same function already gives its sibling retentionDaysFor parameter, and that RunPurgeNowAsync's own doc comment advertises as a guarantee. The other two live-swappable knobs in DarlingWorker.cs (text budget, max concurrent sweeps) get re-clamped at every usage site for exactly this reason; this one doesn't. It's a narrow window (store unreachable at boot + an out-of-range darling.json value), but the failure direction is silent premature deletion of plan XML below the documented 7-day floor.
• Minor: a stale comment in ComputeDimensionCutoff's doc referencing a DateTime.MinValue comparison that doesn't match the actual early-return code path.

Everything else — migration, clamps, config plumbing, viewer probe/schema-version pin, test coverage for the review-catch fixes — looks solid and well-tested.

[claude]

planContentRetentionDays is never clamped before use here — the 4th commit's message describes a fix that isn't actually in the diff.

The last commit ("Clamp the knob at the destructive sink, and say what disabled actually does") claims:

The knob arrives pre-clamped only when a store read succeeded and ApplyToConfig ran. On a store-unreachable boot the worker passes darling.json's RAW value, and a file value of 1-6 would prune plan content below the [7,365] contract - the failure direction is data loss, so PurgeAsync now clamps first thing...

But the actual diff for that commit only reworded a code comment inside ComputeDimensionCutoff — no Math.Clamp/StoreConfigProvider.ClampPlanContentRetentionDays call was added anywhere in PurgeAsync. planContentRetentionDays still flows straight from the caller into ComputeDimensionCutoff (line ~376) and ComputeMapCutoff (line ~422) unclamped.

Concretely: DarlingWorker holds one long-lived DarlingConfig config for the service's lifetime. It's only overwritten with the clamped store value via StoreConfigProvider.ApplyToConfig when a store read succeeds (worker startup comment: "Store-unreachable degrades to the darling.json-loaded config (never worse than before)"). DarlingConfig.Parse/Validate never clamp or reject PlanContentRetentionDays either. So if the store is unreachable on the very first sweep and an operator's darling.json has, say, "planContentRetentionDays": 3 (below the documented floor of 7), both the daily purge (DarlingWorker.cs ~line 1408) and purge_now (RunPurgeNowAsync, ~line 3184) will call PurgeAsync with the raw 3, pruning query_plan_dim content after only ~4 days instead of the guaranteed minimum 7 — silent, unrecoverable data loss, exactly the failure direction the commit message says it closed.

None of the new tests catch this either — PlanContentRetentionTests exercises ComputeDimensionCutoff/ComputeMapCutoff/ClampPlanContentRetentionDays directly with already-valid inputs, never PurgeAsync with an out-of-range raw value.

Fix: clamp at the top of PurgeAsync, e.g. planContentRetentionDays = StoreConfigProvider.ClampPlanContentRetentionDays(planContentRetentionDays); before it's used.

[erikdarlingdata]

You were right, and the miss had a mechanism worth recording: the batch edit script applies substitutions in-memory and writes only after all anchors match — the comment anchor failed its assert, which silently discarded the already-applied clamp, and the comment half was then re-applied by hand without noticing. The clamp is now actually in PurgeAsync (before both cutoff computations), and because it's unreachable by any executable test, it carries a source pin (PurgeAsyncClampsTheKnobAtTheDestructiveSink) that asserts existence AND ordering — proven to fail against the unclamped code before landing.

[claude]

Minor consistency nit: every other knob in this class (CapturePlans, QueryStoreBackfillEnabled, QueryStoreTextBudgetMb, PlanXmlCompression, MaxConcurrentSweeps, …) has an explicit [JsonPropertyName("...")] attribute even though PropertyNameCaseInsensitive = true would make it redundant for matching. PlanContentRetentionDays is missing one. It still deserializes correctly today (case-insensitive matching tolerates the casing difference), so this isn't a functional bug, but if this config is ever serialized back out (e.g. a future settings-writer), it would emit "PlanContentRetentionDays" instead of the camelCase convention every other property follows. Worth adding [JsonPropertyName("planContentRetentionDays")] for consistency.

[erikdarlingdata]

Added — [JsonPropertyName("planContentRetentionDays")], matching every sibling knob.

[claude]

Reviewed the V75 plan-content-retention series (all 4 commits). This is a Darling-only change — Lite never stores plan XML (per Darling/README.md: "Execution plans: Not stored" for Lite), so there's no Lite/Darling parity gap here, and I did not flag one.

Main finding (posted inline on DarlingRetention.cs)

The 4th commit's message ("Clamp the knob at the destructive sink...") describes adding a clamp for planContentRetentionDays in PurgeAsync to cover the store-unreachable-at-boot path, where the worker runs with the raw, unvalidated darling.json value instead of the store-clamped one. Looking at the actual diff, that commit only reworded a comment — no clamp call was added. planContentRetentionDays still flows unclamped into ComputeDimensionCutoff/ComputeMapCutoff inside PurgeAsync. A darling.json value like 3 (below the documented [7,365] floor), combined with the store being unreachable on first boot, would cause query_plan_dim rows to be pruned after ~4 days instead of the guaranteed 7 — silent data loss in exactly the direction the commit claims to have closed. None of the new tests exercise PurgeAsync with an out-of-range raw value, so this slipped through. Suggested fix is a one-liner: clamp planContentRetentionDays via StoreConfigProvider.ClampPlanContentRetentionDays at the top of PurgeAsync.

Everything else checked out

• Migration rung (V75), StorageVersion.SchemaVersion bump, and the viewer probe's new arm/ordinal are all consistent and covered by PlanContentRetentionTests.
• The cutoff arithmetic (dim vs. map, coupled vs. dedicated, disabled=byte-identical, wider-than-facts-is-a-no-op) is correct and well covered, including the two real review catches already folded into commits 3–4 (scoping the knob to query_plan_dim only, and fixing the map/dim prune ordering invariant).
• Config plumbing (darling.json → seed → ReadServiceRowAsync → StoreConfigView → ApplyToConfig → both PurgeAsync call sites) follows the established V59 knob pattern correctly.
• No SQL injection surface — all new SQL uses parameterized Npgsql commands; the migration SQL is a static constant.

One minor style nit posted inline on DarlingConfig.cs: the new PlanContentRetentionDays property is missing the [JsonPropertyName(...)] attribute every sibling knob in that class has (harmless today due to case-insensitive matching, but inconsistent).

[claude]

Review

Went through the V75 plan-content retention change in detail — the cutoff/margin arithmetic, migration wiring, config plumbing, and the viewer's schema probe.

Correctness — verified by hand, not just by the tests:

• ComputeDimensionCutoff/ComputeMapCutoff: proved algebraically (not just for the tested (factRetentionDays, knobDays) pairs) that dim cutoff < map cutoff holds for every combination of widestFactRetentionDays, planContentRetentionDays, and oldestSurvivingDigestFact — including when the measured floor is older than the assumed horizon. The 1-day margin between dedicated_dim = now-(knob+1) and dedicated_map = now-knob mirrors the existing coupled-pair margin (ChunkIntervalDays+1 vs PruneMarginDays), so the "dim must outlive the map" invariant can't invert under the new knob.
• ComputeDimTableCutoff correctly scopes the tightened cutoff to query_plan_dim only; query_text_dim keeps the fact-coupled cutoff, matching the stated rationale (~40 MB, not worth shortening).
• Destructive-sink re-clamp in PurgeAsync is real (not just the pinned string-match test) — it runs before both ComputeDimensionCutoff/ComputeMapCutoff calls, which matters for the store-unreachable boot path where config.PlanContentRetentionDays arrives as darling.json's raw, unclamped value.
• Seed INSERT (SeedServiceRowAsync) and ReadServiceRowAsync's column list / tuple / $n parameter numbering all line up correctly (I traced every ordinal by hand) — no off-by-one in the new plan_content_retention_days slot.
• Viewer schema probe: new EXISTS check is index 50 of 51, MapProbedSchemaVersion gets the matching 51st positional arg, and the new if (hasPlanContentRetentionKnob) return 75; arm sits correctly above the V74 arm for newest-first resolution. QueryStoreTextStoreTests was correctly updated to append false for the new trailing parameter so its V74/V73 pins keep exercising the old arms.
• Only call site of RunPurgeNowAsync (WorkerCommandHost.PurgeNowAsync) was updated for the new config parameter; no other caller missed.

Lite/Darling parity: No gap. Lite has no equivalent — it archives aged rows to Parquet (ArchiveService) rather than running a normalized-dimension GC coupled to fact retention (query_plan_dim, PayloadDimension, dimension last_seen watermark are Darling-only concepts). Nothing to mirror here.

Security: All new values flow through parameterized Npgsql queries; no string-built SQL from the new config value. Clamped at both read (ReadServiceRowAsync) and the destructive sink (PurgeAsync).

Operational note (not a bug, just worth having on record): the migration ships DEFAULT 21 rather than DEFAULT 0, so any store upgrading to V75 will have its first post-upgrade purge start deleting query_plan_dim rows older than 21 days immediately — this is explicitly the intended fix (per the linked #2316 measurements) and the delete is time-sliced (TimeSlicedDeleteSql, 300s per-slice timeout) so a large backlog won't blow a single transaction/timeout budget.

Didn't find anything to flag as a defect. Nice, thoroughly self-documented change — the "review catch" comments in DarlingRetention.cs/tests match what I'd have caught independently (map-cutoff knob-awareness, dim-table scoping, destructive-sink clamp).

[claude]

Review: V75 plan-content retention horizon

Went through the retention arithmetic, the migration/probe wiring, and Lite/Darling parity. No correctness, security, or performance issues found — this lands unusually clean, likely because the PR description's "review catch" trail (destructive-sink clamp, map-ordering, text-dim scoping) shows the obvious bugs were already caught and fixed in earlier iterations. Specifically verified:

• Cutoff arithmetic (ComputeDimensionCutoff / ComputeMapCutoff / ComputeDimTableCutoff): the disabled (0) path is byte-identical to pre-knob behavior, the knob correctly overrides both the assumed and measured-floor safeties only for query_plan_dim (by design — that's the whole point of decoupling plan content from fact retention), and the dim-outlives-map invariant holds across the full swept knob range (dim dedicated = now - (knob+1), map dedicated = now - knob, same 1-day margin shape as the existing coupled pair).
• Clamp defense-in-depth: ClampPlanContentRetentionDays is applied at every boundary that matters — store read (ReadServiceRowAsync), seed write, and the PurgeAsync destructive sink itself (which doesn't trust its caller, correctly, since a store-unreachable boot passes the raw darling.json value straight through).
• Migration: V75 is schema-qualified (config.config_service), additive (ADD COLUMN IF NOT EXISTS), and the viewer probe/MapProbedSchemaVersion 51-parameter wiring checks out against the SQL's actual column count.
• Both PurgeAsync call sites (daily sweep + purge_now) thread config.PlanContentRetentionDays correctly; the interface (IDarlingCommandHost.PurgeNowAsync) is unchanged since the config is captured on the adapter, matching how the sibling V59 knobs already flow.
• Lite/Darling parity: not a gap. Lite never persists plan XML long-term (Query Store plans are fetched live and discarded; the query_plan_xml-shaped DuckDB columns added for schema parity with [FEATURE] Add a headless Windows Service collector mode for Lite (gMSA-compatible), with the existing UI as a read-only viewer #1262 are dormant and always NULL since Lite never sets CapturePlanXml), so the "plan churn outpaces fact retention" problem this PR fixes structurally cannot occur on the Lite side.

One thing worth a sanity check but not a blocker: PlanContentRetentionDays isn't wired into the Viewer's Settings window / ViewerDataService.ServiceConfig.cs (unlike QueryStoreTextBudgetMb/MaxConcurrentSweeps, which are user-editable there) — so today it's only settable via darling.json or a direct store edit. That said, PlanXmlCompression (V62, same "store-backed knob" family) has the identical gap, so this looks like existing precedent rather than an oversight introduced here.