Skip to content

fix: require sustained threshold before alerting on CPU and memory (#66) - #68

Merged
eliau2005 merged 1 commit into
mainfrom
fix/66-cpu-sustain
Sep 13, 2026
Merged

eliau2005 merged 1 commit into
mainfrom
fix/66-cpu-sustain

Conversation

@eliau2005

Copy link
Copy Markdown
Owner

What & why

Fixes #66.

The cpu and mem rules in evalSystem fired on the first sample over threshold. At the default 15s sample_interval, any cron job, backup, apt upgrade or build that fills one sample window produced an "above threshold" push followed by a "recovered" push 15s later. The 10-minute cooldown doesn't prevent that pair, because it only blocks a second fire.

The alert engine already supports a consecutive-sample sustain (ThresholdOpts.Sustain, added for temperature in #25). This PR makes the system rules use it:

  • New systemSustain = time.Minute constant and a sustainSamples(d, interval) helper in internal/agent/agent.go that converts the window to a sample count (ceil, min 1).
    • 15s interval → 4 samples. CPU% is an interval average, so that's exactly 60s of real saturation.
    • 5s interval → 12 samples. A shorter interval no longer shrinks the window.
  • cpu and mem pass Sustain computed from the configured interval. Disk and battery are unchanged.
  • No engine changes and no new config key, so there's no rollback hazard from unknown keys (Config is forward-incompatible: unknown keys are fatal, so a rollback to an older binary kills the agent #29).

Behavior after this change:

Scenario Before After
One 15s sample at 97%, then 18% alert + recovery nothing
≥ 1 minute over threshold alert on first sample one alert after ~60s
Recovery after a fired alert immediate immediate (unchanged)
Cooldown / snooze — unchanged

Trade-off: a real CPU/memory incident is reported about a minute later, and a load oscillating inside the hysteresis band (e.g. 85↔95%) doesn't build a streak. Both match the existing temperature semantics.

Also updated the outdated tempSustainSamples comment, docs/ARCHITECTURE.md (Sustain bullet), and the README "Core system" description.

How it was tested

  • gofmt -l . is clean. go vet ./... is clean. go test -race ./... passes for all packages.
  • New tests in internal/agent/agent_test.go:
    • TestSustainSamples: conversion table (60s at 15s/5s/1s/45s/2m, zero interval, zero duration).
    • TestCPUBurstIsSilent: the CPU threshold alerts fire on single-sample transient spikes without a sustain window #66 timeline [12%, 97%, 18%], plus a run one sample short of the window. No alert or recovery, and the key is not left active.
    • TestCPUSustainedFiresOnce: fires only on the N-th sample, stays quiet while held, resolves on the first cool sample.
    • TestMemSustained: a memory burst is silent, sustained pressure fires.
    • TestSustainScalesWithInterval: at sample_interval = 15s it fires on sample 4, not 3.
  • Updated TestThresholdAlertPushed, the tuned-threshold check in the settings test, and TestSnoozeCallback to run the full sustain window (via a small evalSustained helper), so they still exercise a real fire.
  • Mutation check: with Sustain forced back to 1, the four new behavior tests fail. With the fix, they pass.

Checklist

  • go test ./... passes
  • go vet ./... is clean
  • Code is gofmt-formatted
  • New behavior is covered by tests (and fixes include a regression test)
  • Docs updated where behavior, commands, config, or the security surface changed
  • The change keeps the agent featherweight and respects the OS-abstraction boundary

The CPU and memory rules fired on the first sample over threshold. At the
default 15s interval a cron job, backup, package upgrade or build that
fills one sample window sent "above threshold" and then "recovered" a
sample later: two pushes for something that is not an incident.

The engine already supports a consecutive-sample sustain (added for
temperature in #25); the system rules now use it. The window is defined as
a duration (one minute) and converted to samples from sample_interval, so
a shorter interval does not shrink it. Because CPU is an interval average,
N violating samples mean at least N x interval of real saturation.

A burst shorter than the window now sends neither an alert nor a recovery
notice. Recovery, cooldown and snooze behave as before. No config key is
added.
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c792dd5f-91e0-4122-9233-82d1d8d5aab4


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@eliau2005
eliau2005 merged commit 919cb1b into main Sep 13, 2026
3 checks passed
@eliau2005
eliau2005 deleted the fix/66-cpu-sustain branch September 13, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CPU threshold alerts fire on single-sample transient spikes without a sustain window

1 participant