From f18f15bf7b1fbdf1078bf412754c059694c8b996 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 25 Aug 2026 16:55:20 +0100 Subject: [PATCH] Release seed-pinned metrics history backing arrays on trim The retention and max-points trims re-sliced the series front (metrics[i:]), which keeps the entire backing array reachable while hiding the trimmed prefix from cap(). After the dense mock trends seed (~2,800 points/series) aged out of its 6h window, every series held a few hundred live points while pinning its seed-sized array - roughly 700MB across the demo estate's ~8,000 series. HeapSys drifted over the droplet's memory.high, kernel reclaim throttling starved the accept loop, and demo.pulserelay.pro served 502s (2026-08-25 outage; verified live via pprof: appendMetric-attributed live heap grew +117MB in six minutes after restart while goroutines stayed at 80). Trims now compact in place so cap() reflects the true array size, and releaseTrimmedCapacity copies the window into a right-sized array when it occupies under a quarter of the backing array. A healthy sliding window sits between 1x and 2x capacity and never pays the copy. Regression tests pin both paths: a live append after an aged-out seed backfill, and Cleanup on a mostly-expired series, including an aliasing assertion that catches the cap()-blind front re-slice. --- internal/monitoring/metrics_history.go | 56 ++++++++++---- .../metrics_history_memory_regression_test.go | 74 +++++++++++++++++++ 2 files changed, 115 insertions(+), 15 deletions(-) diff --git a/internal/monitoring/metrics_history.go b/internal/monitoring/metrics_history.go index c4e009493..3fee8361b 100644 --- a/internal/monitoring/metrics_history.go +++ b/internal/monitoring/metrics_history.go @@ -224,27 +224,48 @@ func (mh *MetricsHistory) appendMetric(metrics []MetricPoint, point MetricPoint) metrics = append(metrics, point) } - // Remove old points beyond retention time + // Remove old points beyond retention time. Compact in place instead of + // re-slicing the front: `metrics[i:]` hides the trimmed prefix from + // cap() while keeping the whole backing array reachable, so seed-sized + // arrays stayed pinned long after their points aged out. cutoffTime := time.Now().Add(-mh.retentionTime) - found := false + firstRetained := len(metrics) for i, p := range metrics { if p.Timestamp.After(cutoffTime) { - metrics = metrics[i:] - found = true + firstRetained = i break } } - if !found { + if firstRetained == len(metrics) { metrics = metrics[:0] + } else if firstRetained > 0 { + metrics = append(metrics[:0], metrics[firstRetained:]...) } - // Ensure we don't exceed max data points - if len(metrics) > mh.maxDataPoints { - // Keep the most recent points - metrics = metrics[len(metrics)-mh.maxDataPoints:] + // Ensure we don't exceed max data points, keeping the most recent. + if over := len(metrics) - mh.maxDataPoints; over > 0 { + metrics = append(metrics[:0], metrics[over:]...) } - return metrics + return releaseTrimmedCapacity(metrics) +} + +// releaseTrimmedCapacity copies the retained window into a right-sized +// backing array when the window occupies a small fraction of it. After a +// dense historical seed (~2,800 points/series) ages out of retention, every +// series would otherwise keep its seed-sized array while holding a few +// hundred live points — across thousands of series that pinned ~700MB on +// the public demo and wedged it against its memory cgroup. The trims above +// compact in place (never re-slice the front) so cap() here reflects the +// full backing array. The 4x threshold keeps the copy amortized: a healthy +// sliding window sits between 1x and ~2x capacity and never triggers it. +func releaseTrimmedCapacity(metrics []MetricPoint) []MetricPoint { + if cap(metrics) <= 64 || cap(metrics) <= 4*len(metrics) { + return metrics + } + resized := make([]MetricPoint, len(metrics), len(metrics)+len(metrics)/4+8) + copy(resized, metrics) + return resized } // appendMetricSeries applies the same retention, duplicate-tail, and capacity @@ -275,13 +296,13 @@ func (mh *MetricsHistory) appendMetricSeries(metrics []MetricPoint, values []flo if firstRetained == len(metrics) { metrics = metrics[:0] } else if firstRetained > 0 { - metrics = metrics[firstRetained:] + metrics = append(metrics[:0], metrics[firstRetained:]...) } - if len(metrics) > mh.maxDataPoints { - metrics = metrics[len(metrics)-mh.maxDataPoints:] + if over := len(metrics) - mh.maxDataPoints; over > 0 { + metrics = append(metrics[:0], metrics[over:]...) } - return metrics + return releaseTrimmedCapacity(metrics) } func (mh *MetricsHistory) addGuestMetricSeries(guestID, metricType string, values []float64, timestamps []time.Time) { @@ -828,7 +849,12 @@ func (mh *MetricsHistory) Cleanup() { func (mh *MetricsHistory) cleanupMetrics(metrics []MetricPoint, cutoffTime time.Time) []MetricPoint { for i, p := range metrics { if p.Timestamp.After(cutoffTime) { - return metrics[i:] + if i > 0 { + // Compact rather than re-slice so the backing array's true + // size stays visible to the release check below. + metrics = append(metrics[:0], metrics[i:]...) + } + return releaseTrimmedCapacity(metrics) } } // Return nil instead of metrics[:0] to release the backing array diff --git a/internal/monitoring/metrics_history_memory_regression_test.go b/internal/monitoring/metrics_history_memory_regression_test.go index 55b77fd4b..815120e0f 100644 --- a/internal/monitoring/metrics_history_memory_regression_test.go +++ b/internal/monitoring/metrics_history_memory_regression_test.go @@ -52,3 +52,77 @@ func TestMetricsHistoryMemoryStability(t *testing.T) { } } } + +// TestAppendMetricReleasesSeedSizedCapacity pins the demo-wedge fix: a dense +// historical seed builds a large backing array, and once retention trims the +// window down, the live append path must copy into a right-sized array +// rather than re-slicing and pinning the seed-sized one. Pre-fix, every +// series retained its full seed capacity (~2,800 points) forever while +// holding a few hundred live points, which pinned ~700MB across the demo +// estate's series and wedged the process against its memory cgroup. +func TestAppendMetricReleasesSeedSizedCapacity(t *testing.T) { + mh := NewMetricsHistory(3500, time.Hour) + + const seedPoints = 2820 + values := make([]float64, seedPoints) + timestamps := make([]time.Time, seedPoints) + // Seed entirely outside the retention window so the first live append + // trims it away, mirroring a seed that has aged out. + start := time.Now().Add(-3 * time.Hour) + for i := range values { + values[i] = float64(i % 100) + timestamps[i] = start.Add(time.Duration(i) * time.Second) + } + mh.addGuestMetricSeries("guest-1", "cpu", values, timestamps) + + series := mh.guestMetrics["guest-1"].CPU + if len(series) != 0 { + t.Fatalf("expected aged-out seed to be trimmed on backfill, got len=%d", len(series)) + } + + mh.AddGuestMetric("guest-1", "cpu", 42, time.Now()) + series = mh.guestMetrics["guest-1"].CPU + if len(series) != 1 { + t.Fatalf("expected exactly the live point after trim, got len=%d", len(series)) + } + if cap(series) > 4*len(series)+64 { + t.Fatalf("append pinned an oversized backing array: len=%d cap=%d", len(series), cap(series)) + } +} + +// TestCleanupReleasesSeedSizedCapacity covers the same pinning through the +// periodic Cleanup path: a mostly-expired series must not keep its original +// backing array just because a few points survive the cutoff. +func TestCleanupReleasesSeedSizedCapacity(t *testing.T) { + mh := NewMetricsHistory(3500, time.Hour) + + now := time.Now() + metrics := &GuestMetrics{} + mh.guestMetrics["guest-1"] = metrics + // 2000 points spanning ~20h up to now; only ~100 fall inside the 1h + // retention window, so cleanup must shed the array sized for all 2000. + for i := 0; i < 2000; i++ { + metrics.CPU = append(metrics.CPU, MetricPoint{ + Value: float64(i % 100), + Timestamp: now.Add(-20 * time.Hour).Add(time.Duration(i) * 36 * time.Second), + }) + } + before := cap(metrics.CPU) + orig := metrics.CPU + + mh.Cleanup() + + series := mh.guestMetrics["guest-1"].CPU + if len(series) == 0 { + t.Fatal("expected some points to survive the cleanup cutoff") + } + if cap(series) > 4*len(series)+64 { + t.Fatalf("cleanup pinned an oversized backing array: len=%d cap=%d (was %d)", len(series), cap(series), before) + } + // A front re-slice would alias the tail of the original array while + // keeping all of it reachable; cap() alone cannot see that, so assert + // the surviving window no longer lives inside the oversized array. + if &series[0] == &orig[len(orig)-len(series)] { + t.Fatalf("cleanup still aliases the seed-sized backing array (len=%d cap-was=%d)", len(series), before) + } +}