Skip to content

Commit 5bddeba

Browse files
committed
keyviz: clamp HistoryColumns to MaxHistoryColumns upper bound
Round-1 review fix for PR #651 (Gemini medium): newRingBuffer pre-allocates a slice of capacity HistoryColumns at construction. A misconfiguration like --keyvizHistoryColumns=100000000 (operator typo) would reserve gigabytes upfront and likely OOM the node before the heatmap returned its first column. Add MaxHistoryColumns = 100_000 (~70 days at 60s Step) and clamp opts.HistoryColumns in NewMemSampler so excessive values silently land at the cap instead of being trusted as-is. The cap lives in the keyviz package (next to the existing Default* constants) because the risk is a property of the data structure, not the caller. Operators wanting longer retention should use the Phase 3 persistence path (per-Raft-group `!admin|keyviz|*` namespace) — not a giant in-memory ring. Test TestHistoryColumnsClampedAboveMax confirms both above-cap input clamps to MaxHistoryColumns and exactly-at-cap input is preserved.
1 parent 88423f6 commit 5bddeba

2 files changed

Lines changed: 37 additions & 0 deletions

File tree

keyviz/sampler.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@ const (
8282
DefaultMaxMemberRoutesPerSlot = 256
8383
)
8484

85+
// MaxHistoryColumns is the upper bound on opts.HistoryColumns. The
86+
// ring buffer pre-allocates a slice of capacity HistoryColumns at
87+
// construction; misconfiguration (e.g. an operator typo of
88+
// 100_000_000) would otherwise reserve gigabytes up front. 100 000
89+
// columns at the default 60s Step is ~70 days of history — longer
90+
// retention is the Phase 3 persistence path's job, not the in-memory
91+
// ring's.
92+
const MaxHistoryColumns = 100_000
93+
8594
// MemSamplerOptions configures NewMemSampler. Zero values fall back to
8695
// the Default* constants above; passing a struct literal with only the
8796
// fields you care about is the expected call style.
@@ -303,6 +312,9 @@ func NewMemSampler(opts MemSamplerOptions) *MemSampler {
303312
if opts.HistoryColumns <= 0 {
304313
opts.HistoryColumns = DefaultHistoryColumns
305314
}
315+
if opts.HistoryColumns > MaxHistoryColumns {
316+
opts.HistoryColumns = MaxHistoryColumns
317+
}
306318
if opts.MaxTrackedRoutes <= 0 {
307319
opts.MaxTrackedRoutes = DefaultMaxTrackedRoutes
308320
}

keyviz/sampler_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,6 +869,31 @@ func TestNonPositiveOptionsFallBackToDefaults(t *testing.T) {
869869
}
870870
}
871871

872+
// TestHistoryColumnsClampedAboveMax pins the upper bound on
873+
// HistoryColumns. The ring buffer pre-allocates a slice of capacity
874+
// HistoryColumns at construction; an operator typo (e.g.
875+
// 100_000_000) would otherwise reserve gigabytes up front. Confirm
876+
// that values above MaxHistoryColumns are silently clamped to
877+
// MaxHistoryColumns instead of trusted as-is.
878+
func TestHistoryColumnsClampedAboveMax(t *testing.T) {
879+
t.Parallel()
880+
s, _ := newTestSampler(t, MemSamplerOptions{
881+
Step: time.Second,
882+
HistoryColumns: MaxHistoryColumns * 10,
883+
})
884+
if s.opts.HistoryColumns != MaxHistoryColumns {
885+
t.Fatalf("HistoryColumns clamp = %d, want %d", s.opts.HistoryColumns, MaxHistoryColumns)
886+
}
887+
// At the cap exactly: preserved.
888+
s2, _ := newTestSampler(t, MemSamplerOptions{
889+
Step: time.Second,
890+
HistoryColumns: MaxHistoryColumns,
891+
})
892+
if s2.opts.HistoryColumns != MaxHistoryColumns {
893+
t.Fatalf("HistoryColumns at cap = %d, want %d", s2.opts.HistoryColumns, MaxHistoryColumns)
894+
}
895+
}
896+
872897
// TestStepAccessor pins the Step() accessor contract: returns the
873898
// configured Step (after defaulting) for a constructed sampler, and
874899
// returns DefaultStep for a typed nil so callers wiring RunFlusher

0 commit comments

Comments
 (0)