Bound the force-plan bot's decision journal with a retention horizon

[erikdarlingdata]

Part of #2948.

collect.plan_force_actions — the auto force-plan bot's decision journal — had no purge path. It is not in CollectorCatalog.All, so the catalog-driven loop in DarlingRetention skipped it, and it was never converted to a hypertable, so there was no chunk dropping either. Append-only by design, with nothing bounding it in either sense.

The gap is real rather than a grep artifact, positive-controlled both ways: plan_force_actions appears 0 times in DarlingRetention.cs and 0 times in CollectorCatalog.cs, while the same grep form finds collection_log 15 times and file_io_stats once in the former, and both in the latter.

The premise the issue got wrong, and why the fix still stands

The V107 doc comment did not merely omit a horizon — it asserted one should not exist: "Deliberately NOT enrolled in retention ... its volume is bounded by the bot's own cooldowns." The issue quotes that same comment but stops one clause short of it.

That justification does not hold. The cooldowns bound the arrival rate; a bounded rate over unbounded time is unbounded size. So the documented decision rested on a false premise, and the property it was actually protecting — an audit of writes to production outliving the metrics that motivated it — is preserved by a long horizon rather than by no horizon. The comment now states the horizon that does the bounding. Nothing pinned the old claim; it existed only as prose.

Horizon

PlanForceLedgerRetentionDays = 365, in DarlingRetention.cs beside its siblings — a named constant, so overriding it is a one-line change.

A fixed horizon independent of DataRetentionBaseDays, following AlertHistoryRetentionDays (90), which is the closest analogue in kind. Deliberately the longest horizon in the store, because this is the only series recording a bot writing to production servers: 4x alert history, 12x the metric base. Nearly free — volume is capped by the bot's per-query cooldown and per-server daily force budget, so the ceiling is a few decisions per server per day, not a sample per collection cycle. It also clears the longest window the bot itself reads back for eligibility (a week, for the two-taken-back-forces cooldown) by a wide margin, so retention cannot make the bot forget a decision it is still bound by.

Batched DELETE, not a hypertable

TimescaleSupport already excludes the registry tables from conversion because "registries keep their PRIMARY KEYs, which TimescaleDB would reject or force onto the partition column". This table has action_id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY, and related_action_id points back to it — that self-reference is how a review row references the force row it re-judges. Forcing the key onto (action_id, action_time) would break it. So conversion is not merely poor value for a table holding one row per force/unforce decision per server; it is the thing the repo already declines to do to PK-bearing tables, and it would need a data-moving migration rung against the census register on top.

On the index: idx_plan_force_actions_time leads with server_id and this DELETE has no server_id predicate, so it is not seekable here — #2948 says the existing index makes the delete cheap, and that part does not hold. What makes it cheap is the one-day slice, exactly as for config_alert_log's purge against idx_config_alert_log_time (server_id, metric_name, alert_time), and the bot's cooldowns cap arrivals at a few rows per server per day so there is never much to scan. The code comment says this rather than the original claim.

Where it lives

Beside the config_alert_log and config.config_command purges in PurgeAsync, matching that precedent exactly: PurgeOneAsync over TimeSlicedDeleteSql, counted into tablesPurged/totalRowsDeleted, and failure-isolated — a null return warns, increments tablesFailed and lets the sweep continue, so this table failing cannot abort the others. Schema-qualified to match the V107 DDL and PgPlanForceActionStore; unlike config.config_command a bare name would also resolve, but naming the schema keeps purge and writer readable against each other.

Tests

The failure mode worth pinning is a retention path that silently purges nothing, so the constant-and-SQL-string pin is deliberately not the evidence — it passes against a no-op. The behavioural assertion lives in the existing live-Postgres end-to-end purge test, whose two ages discriminate the horizon rather than just exercise it: a 400-day row goes, and a 100-day row survives even though it is past every other horizon in the store (90-day alert history, 60-day collection_log, 30-day base).

Verified red-first against a real PostgreSQL 17 + TimescaleDB 2.28.1 store, three mutations, each asserted applied by anchor count and content hash and each confirmed to change the output assembly:

Mutation	Caught by
purge block deleted (the never-enrolled implementation)	end-to-end only — pin stayed green
365 → 90	end-to-end and the pin
purge wired to AlertHistoryRetentionDays, constant left at 365	end-to-end only — pin stayed green

Two of the three leave the test assembly byte-identical and move only PerformanceMonitor.Darling.Service.dll, and two are invisible to the pin — which is the case for the behavioural test carrying the contract.

Docs

docs/how-collection-works.md's retention section enumerates the non-hypertable bounded deletes, so it understated the set while this purge was missing from it; it now names the journal's horizon. Darling/README.md's horizon table is what an operator reads to learn how long anything is kept, so the journal gets a row there too, labelled a fixed horizon rather than a per-collector one — every other row in that table comes from CollectorScheduleDefaults and this one does not.

That README table has two problems this PR deliberately does not fix, because another lane owns them: its preamble claims the whole table is "driven by the same shared per-collector horizons Lite uses" (it already mixes in non-collector tables), and it lists collection_log at 30 days when CollectionLogRetentionDays is 60. My row is written to be relocatable into whichever shape that lane lands.

Lite

No equivalent table, measured rather than assumed: the force-plan bot exists only under Darling/ (Service, Viewer, Tests — zero files under Lite/), and none of the 12 tables in Lite's Schema.GetAllTableStatements() is a plan-force journal. The ForcePlanFailure* hits under Lite/ are a different concept — reading a monitored server's own Query Store forcing failures, not a bot's audit trail of its own writes. So this is not the one-SKU half of a shared seam; the subsystem does not exist in Lite, and there is nothing to keep in parity.

Not verified

No schema change and no new migration rung, so no rung-census or ladder impact. The write path (#2138 / PR #2731) is treated as absent. The "one row per force/unforce decision per server" rate is from reading the writer, not observed traffic — the table is empty everywhere the bot is off, which is everywhere, so it cannot be measured. Landing it now is the point: the retention path arrives with no data at risk and before the writer ships.

[claude]

Reviewed. This is a clean, well-scoped addition — I couldn't find a correctness, parity, security, or performance issue to flag.

Specifically verified:

• SQL correctness: TimeSlicedDeleteSql("collect.plan_force_actions", "action_time") matches the pinned expected string, and the schema-qualified name is required here (search_path is collect, config, public, and the table really is under collect, so this one happens to also resolve unqualified — but qualifying it is right per the migration-authoring convention anyway).
• TimescaleDB exclusion claim: confirmed TimescaleSupport.HypertableTables => CollectorCatalog.All, so plan_force_actions (not in the catalog) was never going to be converted regardless of its PK — the updated doc comment's PK-based reasoning is consistent with the class's existing rationale for excluding registry tables in general.
• Failure isolation: the new PurgeOneAsync block for the ledger follows the exact same shape (increment tablesPurged/totalRowsDeleted or tablesFailed) as its config_alert_log and config.config_command neighbors — a failure here can't abort the sweep.
• Test realism: Decision: action in PlanForceBot.BuildRecord confirms the test's decision values (would_force/blocked) mirror real usage rather than being mismatched with the action column.
• Naive-UTC convention: utcNow is stripped to DateTimeKind.Unspecified once at the top of the test and reused via AddDays, so the new insert loop doesn't need its own SpecifyKind call.
• No migration rung added (this is a DDL-doc-comment-only change to V107, correctly not bumping StorageVersion), and the new e2e assertions discriminate the 365-day horizon against every shorter sibling (90/60/30 days), which is exactly the kind of test that would catch a copy-paste of a shorter constant.
• Lite parity: confirmed there is no force-plan-bot code anywhere under Lite/ (the ForcePlanFailure* hits there are an unrelated concept — reading a monitored server's own Query Store failures). Nothing to keep in parity, as the PR description states.

One pre-existing (not introduced by this PR) pattern worth noting for awareness only: idx_plan_force_actions_time leads with server_id, but the purge's DELETE filters only on action_time with no server_id predicate, so Postgres can't seek the index for this query — same shape as config_alert_log's existing purge (idx_config_alert_log_time(server_id, metric_name, alert_time) against an alert_time-only DELETE). Not a regression from this change, just consistent with how the sibling table already works, so I'm not flagging it as blocking.

[claude]

Reviewed. This is a clean, narrowly-scoped fix that follows the existing purge patterns exactly (same shape as config_alert_log/config.config_command: schema-qualified name, TimeSlicedDeleteSql, failure-isolated PurgeOneAsync, counted into tablesPurged/totalRowsDeleted).

Checked and confirmed:

• collect.plan_force_actions is Darling-only (no Lite/DuckDB equivalent — the force-plan bot doesn't exist on that SKU), so there's no Lite/Darling parity gap here.
• Test insert columns (action_time, server_id, server_name, database_name, query_id, plan_id, action, mode, decision, outcome) match the V107 DDL's NOT-NULL-without-default columns exactly, and match PgPlanForceActionStore's own INSERT column list.
• idx_plan_force_actions_time leads with server_id, so the claim that this global DELETE can't seek it (and instead relies on the one-day slice + low arrival rate) checks out.
• The rationale reversal (cooldowns bound arrival rate, not size, so "bounded by cooldowns" doesn't justify "no horizon") is sound and the old V107 doc comment is updated to match rather than left stale.
• No T-SQL touched, so the collector-query style rules (OPTION(RECOMPILE), etc.) don't apply here.
• action_time is a bare timestamp column, consistent with every other purge horizon column in the store (collection_time, alert_time, created_at) — no timezone-handling inconsistency introduced.

No correctness, security, or performance issues found. Nothing to flag.