Persist metric history to disk (survive restarts)#44
Merged
Conversation
RingBuffer history was in-memory only, so every service restart or reboot wiped the dashboard sparklines - on a Pi that reboots for updates this made history empty right when it was most useful. The collector now snapshots all ring buffers to a single compact binary file (data_dir/history.bin, PIMH v1 format) via an atomic temp-file+rename write, flushed on the existing slow tick and once more on clean shutdown to minimize SD-card wear. On startup the file is reloaded, dropping points older than history_window_minutes. A binary format was chosen over JSON per issue #2 for a smaller footprint and less SD-card wear; corrupt or truncated files are detected (magic/version/bounds checks) and ignored with a warning so the service always starts. New config keys: history_persist_enabled (default true) and data_dir (default /var/lib/pimonitor). The systemd unit gains StateDirectory=pimonitor and ReadWritePaths=/var/lib/pimonitor so the hardened service can write its state directory. No API shape change.
LarsLaskowski
commented
Jul 14, 2026
Owner
Author
There was a problem hiding this comment.
Reviewed against the project's Go/security/API-stability conventions (/review-pr). Everything checks out — no blocking issues.
Verification (on the PR branch)
go build ./...,go vet ./...— cleango test ./...andgo test -raceon the touched packages — all passgolangci-lint run— 0 issues,gofmtclean
Checklist review
- Parsing robustness: the
PIMHdecoder is well done — sticky-error bounds checking, magic/version/series/key-length/point-count limits, and (importantly) point slices are only allocated aftertake()confirms the bytes exist, so allocation is bounded by actual file size, not by attacker-declared counts.maxPointsPerSeries * pointSize(2²⁵) also fits in a 32-bitint, so the GOARM=6 builds are safe. Negative-path tests cover bad magic, wrong version, truncation, trailing data, and unknown kinds. - Error handling: missing file, corrupt file, and unwritable
data_dirall degrade to warnings with the collector still starting — matches the project's graceful-degradation convention. - Resource leaks: temp file is closed/removed on every failure path of
writeFileAtomic;mainnow waits for the collector goroutine (bounded by the shutdown context) so the final flush isn't orphaned. - REST API stability: no
/api/v1/...shape change;docs/API.mdwording updated accordingly. - Dependencies: none added. Language: all English. Command execution: none.
- Config/packaging: defaults-then-YAML load order means
history_persist_enabled: falseworks;StateDirectory=+ReadWritePaths=is the right pairing forProtectSystem=strict.
Non-blocking observations
- Directory durability:
writeFileAtomicfsyncs the file but not the parent directory after the rename, so on power loss the latest snapshot may be lost (the previous one survives). For best-effort sparkline history on an SD card that's a reasonable trade-off against extra wear — fine as is, just noting it's a known gap. - Encode side doesn't enforce
maxSeries: a host with >4096 mountpoints/interfaces would write a file its own decoder rejects on next start. Unreachable on any real Pi, so not worth code; mentioning only for completeness. - Brief empty-history window at startup: the HTTP server can serve
/api/v1/metrics/historybefore the collector goroutine finishesloadHistory(). Benign (same as pre-PR behavior of starting empty) and properly locked, so no race — just a few milliseconds where restored points aren't visible yet.
Nice, focused change — the format comment doubling as a spec and the SD-wear-conscious flush cadence are both appreciated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RingBufferhistory was in-memory only, so every service restart or reboot wiped the dashboard sparklines. The collector now periodically snapshots all ring buffers to a single compact binary file and reloads it on startup, soGET /api/v1/metrics/historyspans restarts.PIMHv1) in a newinternal/collector/persist.go: magic + version + per-series records (kind byte, mountpoint/interface key, points as Unix-ms timestamp + float64 pairs), all little-endian. Decoding is bounds-checked (magic, version, series/key/point-count limits) so a corrupt or truncated file is logged and ignored rather than crashing or over-allocating; unknown series kinds are skipped for forward compatibility.fsync+ rename, so a crash mid-write never leaves a partial file.Collector.Run(default every 15 min, keeping SD-card wear low) plus a final flush on clean shutdown;mainnow waits for the collector goroutine so that last flush completes before the process exits.history_window_minutesare trimmed, and each buffer keeps at most its capacity (newest points win). Map series that end up empty after trimming are omitted, matching live-collection behavior.RingBuffer.Fill: the import counterpart toSnapshot, restoring buffer contents oldest-first.history_persist_enabled(defaulttrue) anddata_dir(default/var/lib/pimonitor); validation rejects an emptydata_dirwhen persistence is enabled. Persistence failures are warnings only — metric collection and the API keep working (verified against an unwritabledata_dir).StateDirectory=pimonitor(creates/var/lib/pimonitorowned by the service user) +ReadWritePaths=/var/lib/pimonitorso the hardened unit (ProtectSystem=strict) can write its state dir.No
/api/v1/...response shape changes.Verified end-to-end on this branch: started the service, sampled
/api/v1/metrics/history, sent SIGTERM, restarted, and confirmed the pre-restart points were returned after the restart (restored timestamps are truncated to millisecond precision by design). Also exercised: corrupthistory.bin(warns, starts empty), unwritabledata_dir(warns, keeps serving), persistence disabled (writes nothing), emptydata_dirwith persistence enabled (rejected at startup). Note: verified on x86-64 Linux (plus arm/arm64 cross-compile); not yet run on physical Pi hardware.Related Issue
Closes #2
Checklist
go test ./...passes locally)go vet ./...andgolangci-lint runare cleandocs/API.md),configuration (
README.md,packaging/pimonitor.example.yaml), orinstallation/packaging (
packaging/install.sh, systemd units)/api/v1/...response shapes, or a new APIversion was introduced instead