From 2f0fb1fb6edfaa96ac48a8d98d7accf2301aa01d Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:22:28 -0400 Subject: [PATCH 1/4] Bound the force-plan bot's decision journal with a retention horizon collect.plan_force_actions is append-only, is not in CollectorCatalog.All and is not a hypertable, so neither the catalog purge loop nor chunk dropping ever reached it and nothing else bounded its size. Enrolled at PlanForceLedgerRetentionDays (365) through the same batched DELETE the config_alert_log and config.config_command purges use, on its own action_time column, failure-isolated like every sibling. A year is deliberately the longest horizon in the store: this is the audit trail of a bot writing to production servers, so it has to outlive the metrics that motivated each decision. A DELETE rather than a hypertable conversion because action_id is a PRIMARY KEY that related_action_id points back to, and TimescaleSupport already excludes PK-bearing tables for exactly that reason. The V107 doc comment claimed the table was deliberately not enrolled in retention on the grounds that the bot's cooldowns bound its volume. Those cooldowns bound the arrival rate, which is not a size bound, so the comment now states the horizon that does bound it. --- .../Darling.Tests/DarlingRetentionTests.cs | 93 +++++++++++++++++++ .../DarlingRetention.cs | 55 ++++++++++- .../PgMigrations.cs | 11 ++- 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/Darling/Darling.Tests/DarlingRetentionTests.cs b/Darling/Darling.Tests/DarlingRetentionTests.cs index ab1316be5..09d68ad32 100644 --- a/Darling/Darling.Tests/DarlingRetentionTests.cs +++ b/Darling/Darling.Tests/DarlingRetentionTests.cs @@ -141,6 +141,58 @@ anchoring a slice on an ineligible row would delete nothing and terminate the dr Assert.Equal("status IN ('succeeded', 'failed')", DarlingRetention.TerminalCommandStatuses); } + [Fact] + public void PlanForceLedgerRetention_IsAYear_AndIsTheLongestHorizonInTheStore() + { + /* collect.plan_force_actions (the force-plan bot's decision journal) is not a collector and not a + hypertable, so it purges through the same batched DELETE builder as config_alert_log, on its own + action_time column. A year: it is the audit trail of a bot WRITING to production servers, so it + has to outlive the metrics that motivated each decision by enough that "why did this plan change" + is still answerable releases later. */ + Assert.Equal(365, DarlingRetention.PlanForceLedgerRetentionDays); + + /* The LONGEST horizon in the store, deliberately — strictly greater than every sibling, including + the alert log's already-generous 90 days. This is the assertion that catches a copy-paste of a + shorter sibling's constant into the purge call. */ + Assert.True( + DarlingRetention.PlanForceLedgerRetentionDays > DarlingRetention.AlertHistoryRetentionDays, + "the plan-force ledger must outlive alert history"); + Assert.True( + DarlingRetention.PlanForceLedgerRetentionDays > DarlingRetention.CollectionLogRetentionDays, + "the plan-force ledger must outlive the collection log"); + Assert.True( + DarlingRetention.PlanForceLedgerRetentionDays > DarlingRetention.CommandHistoryRetentionDays, + "the plan-force ledger must outlive command history"); + + /* SCHEMA-QUALIFIED to match the V107 DDL and PgPlanForceActionStore, which both name + collect.plan_force_actions explicitly. No extra predicate: unlike config.config_command every row + here is eligible once it is past the horizon — the journal is append-only, so there is no live-row + state a purge could strand. */ + Assert.Equal( + "DELETE FROM collect.plan_force_actions WHERE action_time < $1" + + " AND action_time >= (SELECT min(action_time) FROM collect.plan_force_actions WHERE action_time < $1)" + + " AND action_time < (SELECT min(action_time) FROM collect.plan_force_actions WHERE action_time < $1) + INTERVAL '1 days'", + DarlingRetention.TimeSlicedDeleteSql("collect.plan_force_actions", "action_time")); + } + + [Fact] + public void ThePlanForceLedger_IsNotACollectorTable_SoTheCatalogLoopCannotReachIt() + { + /* Why the purge needs its own explicit block rather than an entry in the shared catalog: the journal + is written by the service's post-analysis bot pass, not by a collector, so nothing in + CollectorCatalog.All names it and the catalog-driven loop skips it entirely. If it ever DOES gain a + catalog entry, this test fails and the explicit block becomes a double-purge to delete. */ + Assert.DoesNotContain( + CollectorCatalog.All, + d => d.TargetTable.Contains("plan_force_actions", StringComparison.OrdinalIgnoreCase)); + + /* Positive control for that negative: the same predicate DOES find a real collector table, so the + assertion above is a fact about the catalog rather than a mis-spelled probe that can never match. */ + Assert.Contains( + CollectorCatalog.All, + d => d.TargetTable.Equals("file_io_stats", StringComparison.OrdinalIgnoreCase)); + } + [Fact] public void TimeSlicedDelete_WithoutAnExtraPredicate_IsUnchanged() { @@ -409,6 +461,32 @@ the two NOT NULL value columns are required (the rest of the V3 columns default) await insert.ExecuteNonQueryAsync(ct); } + /* collect.plan_force_actions (the force-plan bot's decision journal) purges on its own 365-day + horizon. The two ages are chosen to DISCRIMINATE that horizon rather than merely exercise it: a + 400-day row is past it and 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). So this fails + if the purge is wired to a shorter sibling's constant by copy-paste, and it fails if the table + is not purged at all. Only the NOT NULL columns without defaults are supplied; action_id is + GENERATED ALWAYS AS IDENTITY, so it is never written. */ + foreach (var (ageDays, decision) in new[] { (400, "would_force"), (100, "blocked") }) + { + using var insert = new NpgsqlCommand( + "INSERT INTO collect.plan_force_actions" + + " (action_time, server_id, server_name, database_name, query_id, plan_id, action, mode, decision, outcome)" + + " VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)", connection); + insert.Parameters.AddWithValue(utcNow.AddDays(-ageDays)); + insert.Parameters.AddWithValue(TestServerId); + insert.Parameters.AddWithValue("retention-e2e"); + insert.Parameters.AddWithValue("retention_e2e_db"); + insert.Parameters.AddWithValue(1L); + insert.Parameters.AddWithValue(2L); + insert.Parameters.AddWithValue("force"); + insert.Parameters.AddWithValue("dry_run"); + insert.Parameters.AddWithValue(decision); + insert.Parameters.AddWithValue("journaled"); + await insert.ExecuteNonQueryAsync(ct); + } + /* At least our three expired rows go (40-day wait_stats, 70-day collection_log, 100-day config_alert_log); a shared dev store may shed more. The extension-free DELETE path on purpose (timescaleAvailable: false) — it must keep working even on a store whose tables ARE hypertables, @@ -477,6 +555,20 @@ the two NOT NULL value columns are required (the rest of the V3 columns default) Assert.DoesNotContain(survivors, s => s.Status == "failed"); } + using (var read = new NpgsqlCommand( + "SELECT action_time FROM collect.plan_force_actions WHERE server_id = $1", connection)) + { + read.Parameters.AddWithValue(TestServerId); + using var reader = await read.ExecuteReaderAsync(ct); + Assert.True(await reader.ReadAsync(ct), + $"the 100-day plan_force_actions row (inside the 365-day horizon) did not survive the purge; {purgeLog.Joined}"); + var survivor = reader.GetDateTime(0); + Assert.True(survivor < utcNow.AddDays(-99) && survivor > utcNow.AddDays(-101), + $"the surviving journal row should be the 100-day one, got {survivor:O}; {purgeLog.Joined}"); + Assert.False(await reader.ReadAsync(ct), + $"the 400-day plan_force_actions row survived past the 365-day horizon; {purgeLog.Joined}"); + } + /* The purge writes ONE auditable run-record under the fleet sentinel server_id — SUCCESS here (every table purged cleanly on this store). Never attributed to a real monitored server. */ using (var read = new NpgsqlCommand( @@ -644,6 +736,7 @@ private static async Task DeleteTestRowsAsync(NpgsqlConnection connection, Cance $"DELETE FROM collection_log WHERE server_id = {TestServerId}; " + $"DELETE FROM config_alert_log WHERE server_id = {TestServerId}; " + $"DELETE FROM config.config_command WHERE target_server_id = {TestServerId}; " + + $"DELETE FROM collect.plan_force_actions WHERE server_id = {TestServerId}; " + $"DELETE FROM collection_log WHERE server_id = {DarlingObservability.FleetServerId} AND collector_name = 'data_retention';", connection); await cleanup.ExecuteNonQueryAsync(ct); diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs index 5b28fe5a3..c9e83f8e9 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs @@ -97,6 +97,26 @@ public static class DarlingRetention /// internal const int CommandHistoryRetentionDays = DataRetentionBaseDays; + /// + /// collect.plan_force_actions (the auto force-plan bot's decision journal) keeps its rows this long. Like + /// config_alert_log it is neither a collector (no horizon) nor a + /// hypertable, and it is APPEND-ONLY with no other purge path — so this horizon is the only thing bounding + /// it. The bot's own cooldowns bound the arrival RATE, which is not a size bound: a bounded rate over + /// unbounded time is unbounded. + /// A full year — deliberately the longest horizon in the store, because this is the audit trail of a + /// bot WRITING to production servers and it has to outlive the metrics that motivated each decision + /// () by enough that "why did this plan change" is still answerable + /// releases later. That is 4x the alert log's already-generous and + /// 12x the metric window, and it is 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 rather than a + /// sample per collection cycle. It also clears, by a wide margin, the longest window the bot itself reads + /// back when judging eligibility (a week, for the two-taken-back-forces cooldown), so retention can never + /// make the bot forget a decision it is still bound by. Generous, but BOUNDED — a monitoring store is + /// sized for rolling windows, and "forever" is not a horizon. No operator setting governs this, so the + /// constant is the single source of truth. + /// + internal const int PlanForceLedgerRetentionDays = 365; + /// /// The terminal-status filter for the command purge — the two states /// ViewerDataService.IsTerminal recognizes, which are also the only two @@ -116,8 +136,9 @@ keeps a large first purge from ever hitting a timeout at all — a single unboun /// /// Purges every collector table past its shared /// RetentionDays, plus collection_log past , - /// config_alert_log past , and terminal - /// config.config_command rows past . + /// config_alert_log past , terminal + /// config.config_command rows past , and + /// collect.plan_force_actions past . /// When (the worker's startup detection), the /// collector tables purge via drop_chunks () with a /// per-table DELETE fallback so a table that failed hypertable conversion still honors its @@ -579,6 +600,36 @@ every night into a warning nobody reads. Keyed on created_at (NOT NULL, so no ro tablesFailed++; } + /* collect.plan_force_actions (the force-plan bot's decision journal) purges on its own action_time + column at PlanForceLedgerRetentionDays — the longest horizon in the store. NOT in + CollectorCatalog.All (it is written by the service's post-analysis bot pass, not a collector), so + the loop above skips it, and it is append-only with no other purge path, which makes this the + only thing bounding it. + A batched DELETE, never a hypertable: action_id is a PRIMARY KEY that related_action_id points + back to (a self-review row references the force row it re-judges), and TimescaleSupport already + excludes PK-bearing tables for exactly that reason — conversion would reject the key or force it + onto the partition column, breaking the self-reference the append-only design is built on. + idx_plan_force_actions_time (server_id, action_time) serves both min(action_time) probes and the + slice range scan, so the delete is cheap without one. + SCHEMA-QUALIFIED to match the V107 DDL and PgPlanForceActionStore, which both name + collect.plan_force_actions explicitly. Unlike config.config_command a bare name would also + resolve here (search_path = collect, config, public), but naming the schema keeps the purge and + the writer readable against each other. + Failure-isolated like every sibling: a failed statement is warned + counted, the sweep goes on. */ + var forceLedgerDeleted = await PurgeOneAsync( + postgres, "collect.plan_force_actions", + TimeSlicedDeleteSql("collect.plan_force_actions", "action_time"), + utcNow.AddDays(-PlanForceLedgerRetentionDays), logger, cancellationToken); + if (forceLedgerDeleted is not null) + { + tablesPurged++; + totalRowsDeleted += forceLedgerDeleted.Value; + } + else + { + tablesFailed++; + } + var summary = new PurgeSummary(tablesPurged, totalRowsDeleted, totalChunksDropped); logger?.LogInformation( "Retention purge: {Tables} table(s) purged, {Rows} row(s) deleted, {Chunks} chunk(s) dropped, {Failed} failed, {ElapsedMs}ms", diff --git a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs index f3a2a677c..65dd4e5fb 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs @@ -2297,9 +2297,14 @@ CREATE INDEX IF NOT EXISTS idx_pg_buffer_usage_time /// generator-parity pin applies): it is written by the service's post-analysis bot pass, the /// store_metrics/collector_cost pattern. APPEND-ONLY by design — reviews and outcomes are their own rows /// pointing back via related_action_id, never UPDATEs, so the trail cannot be rewritten by the - /// thing it audits. Deliberately NOT enrolled in retention: an audit of writes to production servers is - /// the one series that should outlive the metrics that motivated it, and its volume is bounded by the - /// bot's own cooldowns (at most one decision per query per cooldown window). + /// thing it audits. Enrolled in retention at the LONGEST horizon in the store + /// (DarlingRetention.PlanForceLedgerRetentionDays, a year), because an audit of writes to + /// production servers is the one series that should outlive the metrics that motivated it. The bot's own + /// cooldowns (at most one decision per query per cooldown window) bound the arrival RATE, which is not a + /// size bound — a bounded rate over unbounded time is unbounded — so the horizon is what keeps it + /// finite. A batched DELETE on action_time, not drop_chunks: the identity PRIMARY KEY that + /// related_action_id points back to is exactly what + /// TimescaleSupport excludes PK-bearing tables for. /// /// reasons is a comma-joined text of the named gate/blocker reasons (the same strings the /// MCP structured_remediation blockers carry) rather than text[], so a future Lite twin From 2ba5b9083f54ba64531e3beb02ea8c6488f87e2e Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:30:12 -0400 Subject: [PATCH 2/4] Name the journal's horizon in the bounded-delete list The retention section enumerates the non-hypertable bounded deletes, so it understated the set while the journal's purge was absent from it. --- docs/how-collection-works.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/how-collection-works.md b/docs/how-collection-works.md index e231d0742..8a5613ebc 100644 --- a/docs/how-collection-works.md +++ b/docs/how-collection-works.md @@ -143,7 +143,7 @@ Three independent mechanisms: - **Daily service purge** — horizons come from `CollectorScheduleDefaults` (7 days for snapshot-ish collectors, 30 for most, 90 for size/index/PVS, 365 for `server_properties` and `job_history`). With TimescaleDB this is `drop_chunks`, which is metadata-only; without it, a time-sliced `DELETE` that is safe against compressed chunks. Failure-isolated per table, with an auditable run-record under `server_id = 0`. - **TimescaleDB retention policies** for the rollup tiers — raw `query_stats` at 4 days, hourly aggregates at 90, daily kept indefinitely. Every policy is created *paused* and arms itself only once it can prove each downstream consumer has already captured the range it would drop. The governing rule, stated in the code: never drop what your consumer has not captured yet. -- **Bounded deletes** for the non-hypertable tables (alert history at 90 days, terminal commands at 30 — a pending command is never purged at any age). +- **Bounded deletes** for the non-hypertable tables (alert history at 90 days, terminal commands at 30 — a pending command is never purged at any age, and the force-plan bot's decision journal at 365, the longest horizon in the store because it audits writes to production servers rather than measuring them). Darling deliberately does not archive before deleting; compression is the archive. From 05d670ef559038c67f00b97d9dc437686866c543 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:36:51 -0400 Subject: [PATCH 3/4] Document the journal's horizon in the operator-facing table, and stop overstating what its index does for the purge The Darling README's retention table is what an operator reads to learn how long anything is kept, so the journal gets a row there, labelled as a fixed horizon rather than a per-collector one because that table's other rows come from CollectorScheduleDefaults and this one does not. The purge comment claimed idx_plan_force_actions_time serves the min(action_time) probes and the slice range scan. It cannot: the index leads with server_id and the DELETE has no server_id predicate, so it is not seekable here. The work bound is the one-day slice, which is what makes the purge cheap, and the comment now says that instead. --- .../PerformanceMonitor.Darling.Service/DarlingRetention.cs | 7 +++++-- Darling/README.md | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs index c9e83f8e9..2b7c8b1a2 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs @@ -609,8 +609,11 @@ only thing bounding it. back to (a self-review row references the force row it re-judges), and TimescaleSupport already excludes PK-bearing tables for exactly that reason — conversion would reject the key or force it onto the partition column, breaking the self-reference the append-only design is built on. - idx_plan_force_actions_time (server_id, action_time) serves both min(action_time) probes and the - slice range scan, so the delete is cheap without one. + The work bound is the one-day SLICE, not an index seek: idx_plan_force_actions_time leads with + server_id and this DELETE has no server_id predicate, so that index cannot be seeked here. Same + shape as config_alert_log's purge against idx_config_alert_log_time(server_id, metric_name, + alert_time), and adequate for the same reason — the bot's per-query cooldown and per-server + daily budget cap arrivals at a few rows per server per day, so there is never much to scan. SCHEMA-QUALIFIED to match the V107 DDL and PgPlanForceActionStore, which both name collect.plan_force_actions explicitly. Unlike config.config_command a bare name would also resolve here (search_path = collect, config, public), but naming the schema keeps the purge and diff --git a/Darling/README.md b/Darling/README.md index 54b9513b8..1831ab34c 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -862,6 +862,7 @@ A purge runs on the first sweep after startup and then daily, driven by the same | 30 days | Most collector tables (wait/query/procedure/Query Store stats, CPU, memory, file I/O, tempdb, perfmon, deadlocks, blocking, sessions, config snapshots), plus `collection_log` and `analysis_findings` | | 90 days | `database_size_stats`, `index_object_stats`, `pvs_stats` | | 365 days | `server_properties` | +| 365 days | `collect.plan_force_actions`, the auto force-plan bot's decision journal — a **fixed** horizon (`DarlingRetention.PlanForceLedgerRetentionDays`) rather than a per-collector one, and deliberately the longest in the store: it audits a bot *writing* to production servers rather than measuring them, so it has to outlive the metrics that motivated each decision. Append-only, never a hypertable, so it purges by bounded DELETE | On plain PostgreSQL the purge is DELETE-based. With TimescaleDB it switches to `drop_chunks` — a metadata-only detach of whole expired chunks (rows inside a partially-expired chunk survive until the whole chunk ages out; up to ~1 day of grace at the 1-day chunk width), with a per-table DELETE fallback for any table that is not a hypertable. Failure-isolated per table: one stuck purge is logged and retried the next day without stopping the sweep. From f4b639b3382710278c4435e1cbbaf42805739ab7 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:38:57 -0400 Subject: [PATCH 4/4] Name the journal in the class doc's list of DELETE-based tables The class summary enumerates which tables never convert to hypertables and so stay on the batched DELETE, and which are owned elsewhere. The journal belongs in the first list, for its own reason: it keeps an identity PRIMARY KEY its own rows reference, where the other two are config-side registry tables. --- .../PerformanceMonitor.Darling.Service/DarlingRetention.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs index 2b7c8b1a2..18bf30d39 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs @@ -28,8 +28,9 @@ namespace PerformanceMonitor.Darling.Service; /// purge via hypertable drop_chunks instead, which detaches whole expired chunks in O(1) /// instead of scanning rows. collection_log — a hypertable since V23, though converted directly by the /// V23 migration rather than the catalog loop — purges the SAME way (drop_chunks with a DELETE fallback for -/// a plain-PostgreSQL store). config_alert_log and config.config_command stay DELETE-based either way -/// (never converted — plain config-side registry tables), as do the analysis tables +/// a plain-PostgreSQL store). config_alert_log, config.config_command and collect.plan_force_actions stay +/// DELETE-based either way (never converted — the first two are plain config-side registry tables, and the +/// journal keeps an identity PRIMARY KEY its own rows reference), as do the analysis tables /// (PgFindingStore.CleanupOldFindingsAsync owns those). Retention horizons are the shared /// per-collector (identity-pinned to Lite's /// ScheduleManager table), so both SKUs keep the same data horizons out of the box. NOTE: Lite