amadan
metrics: fix CPUFrac uint64 wrap when Busy regresses under advancing Total
CPUFrac only guarded cur.Total <= prev.Total, not cur.Busy < prev.Busy. With Total still advancing but Busy alone going backwards, the unsigned subtraction cur.Busy-prev.Busy wrapped into a huge garbage fraction instead of nil, contradicting the package's own "fail toward nil" contract. Adds the missing guard and a regression test pinning the exact values (Busy:500,Total:1000 -> Busy:100,Total:1100) that produced the garbage reading.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+17
−5
internal/metrics/parse.go+9 −5internal/metrics/parse_test.go+8 −0
diff --git a/internal/metrics/parse.go b/internal/metrics/parse.go| index f53fcee73..0900ab587 100644 |
| --- a/internal/metrics/parse.go |
| +++ b/internal/metrics/parse.go |
| @@ -56,12 +56,16 @@ func ParseProcStat(text string) *CPUCounters { |
| } |
| // CPUFrac turns two /proc/stat readings into a fraction of CPU busy over the |
| -// interval between them. Both readings must be non-nil and cur.Total must |
| -// exceed prev.Total — counters that went backwards mean the box rebooted or |
| -// the reading is otherwise invalid, and reporting a fraction from that would |
| -// be a guess dressed up as a measurement, so it's nil instead. |
| +// interval between them. Both readings must be non-nil, cur.Total must |
| +// exceed prev.Total, and cur.Busy must not be behind prev.Busy — counters |
| +// that went backwards mean the box rebooted or the reading is otherwise |
| +// invalid, and reporting a fraction from that would be a guess dressed up as |
| +// a measurement, so it's nil instead. The Busy check matters on its own: |
| +// Total can still advance while Busy alone regresses, and since both are |
| +// unsigned, cur.Busy-prev.Busy would silently wrap into a huge garbage |
| +// value rather than go negative. |
| func CPUFrac(prev, cur *CPUCounters) *float64 { |
| - if prev == nil || cur == nil || cur.Total <= prev.Total { |
| + if prev == nil || cur == nil || cur.Total <= prev.Total || cur.Busy < prev.Busy { |
| return nil |
| } |
| frac := float64(cur.Busy-prev.Busy) / float64(cur.Total-prev.Total) |
diff --git a/internal/metrics/parse_test.go b/internal/metrics/parse_test.go| index 726caf91e..c9bcbbff1 100644 |
| --- a/internal/metrics/parse_test.go |
| +++ b/internal/metrics/parse_test.go |
| @@ -34,6 +34,14 @@ func TestCPUFrac(t *testing.T) { |
| if CPUFrac(cur, prev) != nil { // counters went backwards (reboot) — no guess |
| t.Fatal("backwards counters must be nil") |
| } |
| + |
| + // Total advances but Busy alone regresses: uint64 subtraction of |
| + // cur.Busy-prev.Busy would wrap into a huge garbage value instead of |
| + // going negative, so this must be caught explicitly, not just inferred |
| + // from the Total check. |
| + if got := CPUFrac(&CPUCounters{Busy: 500, Total: 1000}, &CPUCounters{Busy: 100, Total: 1100}); got != nil { |
| + t.Fatalf("Busy regressing while Total advances must be nil, got %v", *got) |
| + } |
| } |
| func TestParseLoadAvg(t *testing.T) { |