diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index 3c66bc914..872075080 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -914,7 +914,15 @@ durable storage. Any future change that reduces that batching headroom, makes WAL checkpoints more aggressive again, reopens duplicate-write failures, or removes the metrics DB path/cadence controls must re-prove the metrics-store hot path with the owned store tests rather than assuming the earlier vacuum -fixes are sufficient. +fixes are sufficient. Retention must also return freed SQLite pages to the OS +proportionally to the current freelist, bounded per cycle, and on every +retention cycle rather than only when that cycle deleted rows: a fixed small +incremental-vacuum batch lets the freelist outpace reclaim on busy instances, +so `metrics.db` bloats to GBs of free pages over tens of MB of live data even +while row retention works correctly. The reclaim stays a once-per-cycle bounded +operation (it skips the checkpoint entirely when the freelist is empty) so WAL +cadence is not made more aggressive, and `TestStoreRetentionReclaimsFreePages` +guards that a pre-existing backlog drains and the file shrinks. contract instead of inventing an infrastructure-local summary filter branch. For shared line charts on that hot path, the shared sparkline primitive may isolate the selected series inside the existing render budget, but that diff --git a/pkg/metrics/store.go b/pkg/metrics/store.go index 87231f205..f3cb0177f 100644 --- a/pkg/metrics/store.go +++ b/pkg/metrics/store.go @@ -1735,16 +1735,44 @@ func (s *Store) runRetention() { Int64("deleted", totalDeleted). Dur("duration", time.Since(start)). Msg("Metrics retention cleanup completed") + } - // Reclaim disk space from deleted rows. Without this, the database - // file never shrinks — a setup with 50+ resources can bloat to 5GB+ - // while only holding ~60MB of live data. - if _, err := s.db.Exec(`PRAGMA incremental_vacuum(5000)`); err != nil { + // Reclaim freed pages every cycle, even in an hour where nothing was + // deleted, so a pre-existing freelist backlog still drains over time. + s.reclaimFreePages() +} + +// maxReclaimPages bounds how many freed SQLite pages reclaimFreePages returns +// to the OS per retention cycle (~50k pages * 4KiB ≈ 200MiB). A large backlog +// is drained over several hourly cycles instead of holding the write lock for +// minutes at once, while steady-state freelists (well under the cap) drain +// fully every cycle. +const maxReclaimPages = 50000 + +// reclaimFreePages returns freed pages to the OS. auto_vacuum is INCREMENTAL, +// so deleted-row pages sit on the freelist until reclaimed here. A previous +// fixed 5000-page batch could not keep up on busy instances: when an hourly +// retention pass frees more pages than the batch reclaims, the freelist grows +// net-positive every cycle and the database file bloats unboundedly (5GB+ of +// free pages over ~60MB of live data) even though row retention is working. +// Draining proportionally to the freelist, capped, fixes that. +func (s *Store) reclaimFreePages() { + var freelist int64 + if err := s.db.QueryRow(`PRAGMA freelist_count`).Scan(&freelist); err != nil { + log.Debug().Err(err).Msg("Failed to read freelist_count") + freelist = maxReclaimPages // fall back to a bounded reclaim + } + if freelist > 0 { + pages := freelist + if pages > maxReclaimPages { + pages = maxReclaimPages + } + if _, err := s.db.Exec(fmt.Sprintf(`PRAGMA incremental_vacuum(%d)`, pages)); err != nil { log.Debug().Err(err).Msg("Incremental vacuum failed") } - if _, err := s.db.Exec(`PRAGMA wal_checkpoint(TRUNCATE)`); err != nil { - log.Debug().Err(err).Msg("WAL checkpoint failed") - } + } + if _, err := s.db.Exec(`PRAGMA wal_checkpoint(TRUNCATE)`); err != nil { + log.Debug().Err(err).Msg("WAL checkpoint failed") } } diff --git a/pkg/metrics/store_additional_test.go b/pkg/metrics/store_additional_test.go index 91b52f386..b8bdd7d35 100644 --- a/pkg/metrics/store_additional_test.go +++ b/pkg/metrics/store_additional_test.go @@ -828,3 +828,81 @@ func TestStoreQueryMetricTypesBatchFiltersMetricTypes(t *testing.T) { t.Fatalf("expected metric filter to exclude memory for agent-2, got %+v", result["agent-2"]) } } + +// TestStoreRetentionReclaimsFreePages guards the fix for unbounded metrics.db +// growth. A pre-existing freelist backlog must drain on a normal retention pass +// even in an hour where nothing new was deleted. The previous code reclaimed +// only when that cycle deleted rows, and only 5000 pages at a time, so on busy +// instances the freelist outpaced reclaim and the file bloated to GBs over +// ~60MB of live data. Reclaiming every cycle, proportional to the freelist, +// returns the freed pages to the OS so the file shrinks. +func TestStoreRetentionReclaimsFreePages(t *testing.T) { + dir := t.TempDir() + cfg := DefaultConfig(dir) + cfg.DBPath = filepath.Join(dir, "metrics-reclaim.db") + cfg.RetentionRaw = time.Minute + cfg.FlushInterval = time.Hour + + store, err := NewStore(cfg) + if err != nil { + t.Fatalf("NewStore returned error: %v", err) + } + defer store.Close() + + // Build a freelist backlog: insert many rows (kept unique by timestamp) then + // delete them directly, so the rows are already gone before runRetention. + tx, err := store.db.Begin() + if err != nil { + t.Fatalf("begin tx: %v", err) + } + stmt, err := tx.Prepare(`INSERT INTO metrics (resource_type, resource_id, metric_type, value, timestamp, tier) VALUES ('vm', 'vm-101', 'cpu', 1.0, ?, 'raw')`) + if err != nil { + t.Fatalf("prepare: %v", err) + } + base := time.Now().Add(-72 * time.Hour).Unix() + for i := 0; i < 20000; i++ { + if _, err := stmt.Exec(base + int64(i)); err != nil { + t.Fatalf("insert: %v", err) + } + } + stmt.Close() + if err := tx.Commit(); err != nil { + t.Fatalf("commit: %v", err) + } + if _, err := store.db.Exec(`DELETE FROM metrics`); err != nil { + t.Fatalf("delete: %v", err) + } + if _, err := store.db.Exec(`PRAGMA wal_checkpoint(TRUNCATE)`); err != nil { + t.Fatalf("checkpoint: %v", err) + } + + var freelistBefore, pagesBefore int64 + if err := store.db.QueryRow(`PRAGMA freelist_count`).Scan(&freelistBefore); err != nil { + t.Fatalf("freelist before: %v", err) + } + if err := store.db.QueryRow(`PRAGMA page_count`).Scan(&pagesBefore); err != nil { + t.Fatalf("page_count before: %v", err) + } + if freelistBefore == 0 { + t.Fatalf("test precondition failed: expected a freelist backlog, got 0 free pages") + } + + // Nothing to prune this cycle (table is already empty), but the backlog must + // still be reclaimed. The old code skipped reclaim entirely when nothing was + // deleted, so the file would not shrink and this would fail. + store.runRetention() + + var freelistAfter, pagesAfter int64 + if err := store.db.QueryRow(`PRAGMA freelist_count`).Scan(&freelistAfter); err != nil { + t.Fatalf("freelist after: %v", err) + } + if err := store.db.QueryRow(`PRAGMA page_count`).Scan(&pagesAfter); err != nil { + t.Fatalf("page_count after: %v", err) + } + if pagesAfter >= pagesBefore { + t.Fatalf("expected database file to shrink after reclaim: pages before=%d after=%d (freelist %d -> %d)", pagesBefore, pagesAfter, freelistBefore, freelistAfter) + } + if freelistAfter >= freelistBefore { + t.Fatalf("expected freelist to shrink after reclaim: %d -> %d", freelistBefore, freelistAfter) + } +}