Evict stale per-device history series in the collector

[LarsLaskowski]

📖 Description

fastTick in internal/collector/collector.go lazily creates a ring buffer per mountpoint (diskHist) and per network interface (rxHist/txHist) but never removed one. Devices that disappear (e.g. veth* interfaces from Docker/Podman container churn, USB drives, autofs mounts) kept their full ring buffer in memory forever and kept being emitted by History() / GET /api/v1/metrics/history, causing slow unbounded memory growth and an ever-growing history payload.

This is a fix, not a breaking change: no REST API shape changes.

🎫 Issues

Closes #24

👩‍💻 Reviewer Notes

• The eviction rule lives in a new package-level helper, evictStaleSeries (internal/collector/collector.go), called from fastTick for diskHist, rxHist, and txHist after their per-device Add loops, still under c.mu.Lock().
• A device's series is only dropped once its newest sample falls outside the retained history window (FastInterval * HistoryCapacity) — not merely for being absent from a single tick. This matches DiskCollector.Collect, which already skips mountpoints that transiently fail to stat, so a one-tick miss must not lose history.
• Added RingBuffer.Newest() to read the most recently added sample (needed to check staleness) without exposing internal layout.
• Focus review on evictStaleSeries and its two call sites in fastTick; the rest of the diff is plumbing (diskKeys/netKeys sets built alongside the existing per-device loops).

📑 Test Plan

• New TestCollector_History_EvictsStaleDeviceSeries (internal/collector/collector_test.go): with HistoryCapacity: 3 (a 3s window) and fake tick timestamps, a device missing for a single tick keeps its history, while a device still absent once its newest sample is older than the window is evicted from History(); a continuously-present device is unaffected.
• New RingBuffer.Newest() unit tests (internal/collector/ringbuffer_test.go): empty buffer, basic case, and after wraparound.
• go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run all pass locally.

✅ Checklist

General

• I have added/updated tests for my changes (go test ./... -race -cover passes locally).
• go vet ./... and golangci-lint run are clean.
• I have tested my changes.
• I have read the CONTRIBUTING documentation and followed the project's code style guidelines.
• I have updated ARCHITECTURE.md if this changes a documented design decision. (not applicable — no design-decision change, just closing a memory leak within the existing history-window design)

REST API / configuration / packaging

Not applicable — no REST API, configuration, or packaging change.

⏭ Next Steps

None.

[LarsLaskowski]

Reviewed against the project's Go/security/API conventions. go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run (0 issues) all pass on ac1c234. No new dependencies, no exec.Command or privilege changes, no /api/v1 shape change, and RingBuffer.Newest() correctly handles the wraparound and post-Fill index cases.

Three findings; the first is blocking under this repo's test policy (submitting as a comment rather than "request changes" only because GitHub does not allow that on one's own PR).

1. Inline on collector_test.go — the new test bypasses fastTick and re-implements its bookkeeping, so the actual production change is uncovered. Two mutations of collector.go keep the suite green, including dropping the rxHist/txHist eviction entirely.
2. Inline on collector.go:369 — the eviction window is computed from the unclamped FastInterval, disabling eviction in the case Run's clampInterval already defends against.
3. docs/ARCHITECTURE.md (not in the diff): the "Snapshot + history" paragraph still describes the per-device maps as "created lazily the first time a given device is seen", with nothing about removal — that sentence is now half the lifecycle. The checklist marks ARCHITECTURE.md not applicable, but the file already documents the analogous alert-side rule at length ("Disks are pruned, not garbage-collected implicitly."), and the collector's new rule deliberately differs from it: pruneDisks drops a mountpoint after a single absent sample, while history now waits a full window. That divergence — and its reason, which is well argued in the PR description — is exactly what the next reader will come to that file for. A sentence or two there would cover it.

[LarsLaskowski]

Pushed e5ee2a3 addressing all three findings:

1. Blocking (test coverage) — replaced the reimplemented-bookkeeping test with TestCollector_FastTick_EvictsStaleDeviceSeries / TestCollector_FastTick_KeepsRecentlyMissingDeviceSeries, which seed stale/recent entries directly into diskHist/rxHist/txHist and then call the real fastTick, so the actual production call sites are exercised (including rxHist/txHist, previously uncovered). Kept a direct table-driven TestEvictStaleSeries for the evictStaleSeries helper covering present / within-window / exactly-at-window / past-window, plus a zero-window no-op case. Verified locally that reverting the rx/tx eviction calls now fails TestCollector_FastTick_EvictsStaleDeviceSeries.
2. Unclamped FastInterval — the eviction window is now derived from a clamped fastInterval stored once on Collector at construction (same value Run's ticker uses), instead of the raw cfg.FastInterval. Added TestCollector_FastTick_EvictsWithNonPositiveFastInterval; verified it fails against the old unclamped computation.
3. ARCHITECTURE.md — added a paragraph documenting the eviction rule and explaining why it deliberately differs from pruneDisks (full window vs. single absent sample).

go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run all pass on e5ee2a3.

[LarsLaskowski]

Re-reviewed e5ee2a3. All three points from the previous round are addressed properly. go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run (0 issues) pass; collector coverage 87.2% → 87.6%.

I re-ran the mutation checks against the new tests, and they now catch what they need to:

Mutation of collector.go	Result
drop the rxHist/txHist eviction calls	TestCollector_FastTick_EvictsStaleDeviceSeries fails
revert historyWindow to the raw c.cfg.FastInterval	TestCollector_FastTick_EvictsWithNonPositiveFastInterval fails
> → >= on the window comparison	TestEvictStaleSeries fails
evict on first absence (drop the age check)	TestEvictStaleSeries + TestCollector_FastTick_KeepsRecentlyMissingDeviceSeries fail

Clamping FastInterval once in New and having both Run's ticker and fastTick's window read c.fastInterval is the right shape — grep confirms no raw cfg.FastInterval use is left in the collector, and New is the only constructor. The ARCHITECTURE.md paragraph lands in the right place and the contrast it draws with pruneDisks is exactly what was missing.

One correction to my previous review: I cited "passing netKeys instead of diskKeys to the disk call stays green" as evidence of the coverage gap. That mutant is still green on e5ee2a3, but it is an equivalent mutant, not a gap — every device in current has just had a sample stamped now added in the same critical section, so the age check alone already keeps it and the current membership check can never change the outcome at these call sites. It's worth keeping as defensive intent; nothing to do. The genuine evidence was the dropped rxHist/txHist calls, which the new test does catch.

One new finding inline: TestCollector_FastTick_KeepsRecentlyMissingDeviceSeries bounds a real fastTick inside a 3s window, while that tick can block for 5s+ on vcgencmd on actual Pi hardware.

[LarsLaskowski]

Pushed e301729 addressing the new review comment and the SonarCloud finding:

1. Flaky test timing (collector_test.go:269) — TestCollector_FastTick_KeepsRecentlyMissingDeviceSeries stamped its seeded sample with time.Now() right before calling fastTick, then compared against a 3s window. Since fastTick's own now is taken after the full collection prologue (which can legitimately run several seconds on real Pi hardware — vcgencmd calls, statfsWithTimeout per mountpoint), a slow tick could push the sample outside the window and fail the assertion. Widened HistoryCapacity to 3600 (1h window), as suggested, so the test no longer depends on tick duration.
2. SonarCloud go:S3776 — fastTick's cognitive complexity was 25 (limit 15). Extracted the collect-and-log-errors prologue into collectFastTickSamples (returns a new fastTickSamples struct) and the per-device history/eviction bookkeeping into recordDeviceHistory, leaving fastTick itself as orchestration only. No behavior change — same collectors, same error logging, same lock scope.

go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run all pass on e301729.

[LarsLaskowski]

Reviewed e301729. go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run (0 issues) pass locally; all five CI checks including the SonarCloud gate are green.

The one finding is a retraction of my own previous comment — see inline. The flakiness I flagged does not exist: fastTick takes its timestamp before collection, so the test never depended on tick duration, and the 3s window it replaced was correct. The change is harmless but its new comment states something untrue about the code, so I'd revert that one line.

The fastTick split checks out as behavior-preserving. What I verified rather than eyeballed:

• Order of operations is unchanged: collect (unlocked) → c.mu.Lock() → latest assignment → scalar history → per-device history + eviction → alert evaluation. recordDeviceHistory sits exactly where the two device loops and the three evictStaleSeries calls used to be.
• The consolidated err variable is safe: load, throttled, network, and uptime all log their error immediately and never read it again. The four errors that outlive their call site (cpuErr, tempErr, memErr, diskErr) are carried on the struct, and each still feeds the same alert validity flag as before (SwapValid still keyed off memErr, matching the original).
• No log line was lost or reworded — the set of c.log.Warn messages is byte-identical to e5ee2a3.
• netIfaces stays nil when NetworkEnabled is false, which TestCollector_FastTick_NetworkDisabled still pins.
• The moved eviction call sites are still covered. Re-ran the mutation checks against the refactored code: dropping the rxHist/txHist calls, reverting to the raw cfg.FastInterval, flipping > to >=, and skipping the recordDeviceHistory call each fail the expected test. Coverage holds at 87.6%.

recordDeviceHistory documenting that it runs with c.mu already held is the right call, since that's no longer visible from the function itself.

[LarsLaskowski]

Reverted in 8a11900 — you're right, and no worries about the churn.

I confirmed the retraction independently before reverting. Reproduced your experiment on this branch (3s window restored, 4s time.Sleep injected at the top of the collection sequence in collectFastTickSamples, i.e. a tick well past the hypothetical failure threshold):

--- PASS: TestCollector_FastTick_KeepsRecentlyMissingDeviceSeries (4.00s)

s.now = time.Now() is the first statement of collectFastTickSamples, so every vcgencmd/statfs timeout elapses after the timestamp is taken — tick duration never enters the comparison.

Also re-checked that the 3s window doesn't weaken the test: mutating evictStaleSeries to evict on first absence still fails it with expected fastTick to keep a disk series missing for only one tick.

go build ./..., go vet ./..., go test ./... -race -cover, and golangci-lint run all pass on 8a11900.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud