Cap history capacity to prevent OOM from misconfigured window/interval#83
Conversation
HistoryCapacity() was the unchecked ratio of history_window_minutes to poll_interval_seconds. Since NewRingBuffer allocates its capacity eagerly for each of the 7 scalar history series, an extreme ratio (e.g. a typo like history_window_minutes: 525600 with poll_interval_seconds: 0.05) could allocate several GiB at startup and OOM a Pi. Validate() now rejects any configuration whose implied history capacity exceeds 1,000,000 points per series, failing fast with a descriptive error instead of letting the allocator die. Closes #65
There was a problem hiding this comment.
Reviewed against the project checklist. Build, go vet, go test ./..., and golangci-lint run all pass on the PR branch; boundary tests (at-cap and one-over) are in place; no API-shape or dependency changes. One finding inline: the validation can be bypassed on amd64 via platform-dependent float→int overflow in HistoryCapacity().
| if cap := c.HistoryCapacity(); cap > maxHistoryCapacity { | ||
| return fmt.Errorf("history_window_minutes (%v) / poll_interval_seconds (%v) implies %d history points per series, exceeding the %d limit; increase poll_interval_seconds or reduce history_window_minutes", c.HistoryWindowMinutes, c.PollIntervalSeconds, cap, maxHistoryCapacity) | ||
| } |
There was a problem hiding this comment.
This check can be silently bypassed on amd64 for ratios beyond int64 range, because HistoryCapacity() does int(c.HistoryWindowMinutes * 60 / c.PollIntervalSeconds) and Go's out-of-range float→int conversion is platform-dependent: on amd64 an overflowing value converts to min-int64, so HistoryCapacity()'s capacity < 1 clamp kicks in and returns 1 — validation passes and the service starts with a useless 1-point history buffer. On arm64 the same conversion saturates to max-int64 and is correctly rejected. Verified on amd64:
int(1e300 * 60 / 1.0) // == -9223372036854775808So e.g. history_window_minutes: 1e300 fails startup on a Pi but silently "works" on an amd64 dev machine — the exact class of typo this PR is meant to fail fast on.
Doing the comparison in float64 before any int conversion closes the gap on all platforms:
if ratio := c.HistoryWindowMinutes * 60 / c.PollIntervalSeconds; ratio > maxHistoryCapacity {
return fmt.Errorf("history_window_minutes (%v) / poll_interval_seconds (%v) implies %v history points per series, exceeding the %d limit; increase poll_interval_seconds or reduce history_window_minutes", c.HistoryWindowMinutes, c.PollIntervalSeconds, ratio, maxHistoryCapacity)
}There was a problem hiding this comment.
Good catch, fixed in a3e2f8c: the cap is now checked on the float64 ratio before any int conversion, and added a regression test with history_window_minutes: 1e300.
Validate() called HistoryCapacity(), which converts the window/interval ratio to int before the cap check. Converting an out-of-range float64 to int is implementation-specific in Go and differs by architecture: amd64 wraps an overflowing value to a negative int64, while arm64 saturates to math.MaxInt64. Since HistoryCapacity() also clamps negative results up to 1, an extreme ratio (e.g. history_window_minutes: 1e300) would wrap to a negative number on amd64 and pass validation with a useless 1-point buffer, while correctly failing on arm64 (the Pi's actual architecture) - the exact class of typo this cap exists to catch. Comparing the ratio in float64 before any int conversion closes the gap on all platforms.
2 participants
Summary
HistoryCapacity()(internal/config/config.go) is the unchecked ratio ofhistory_window_minutestopoll_interval_seconds. SinceNewRingBufferallocates its capacity eagerly for each of the 7 scalar history series, an extreme ratio (e.g. a typo likehistory_window_minutes: 525600withpoll_interval_seconds: 0.05) could allocate several GiB at startup and OOM a Pi.Validate()now rejects any configuration whose implied history capacity exceeds a sanity cap of 1,000,000 points per series, failing fast at startup with a descriptive error instead of letting the allocator die.Related Issue
Closes #65
Checklist
go test ./...passes locally)go vet ./...andgolangci-lint runare cleandocs/API.md), configuration (README.md,packaging/pimonitor.example.yaml), or installation/packaging (packaging/install.sh, systemd units) — not needed; no new config field, no documented numeric limit changed/api/v1/...response shapes, or a new API version was introduced instead