Skip to content

fix: disk alert escalation compares operands in the wrong order - #1592

Open
CaptainVirgil wants to merge 1 commit into
moghtech:mainfrom
CaptainVirgil:fix/disk-alert-escalation-operand-order
Open

fix: disk alert escalation compares operands in the wrong order#1592
CaptainVirgil wants to merge 1 commit into
moghtech:mainfrom
CaptainVirgil:fix/disk-alert-escalation-operand-order

Conversation

@CaptainVirgil

Copy link
Copy Markdown

Summary

In bin/core/src/monitor/alert/server.rs, the disk-alert escalation
check compares its operands in the wrong order relative to the CPU
and memory alert paths in the same file.

  • CPU (alert_servers, CPU block): alert.level < health.cpu.level
  • Memory (same fn, memory block): alert.level < health.mem.level
  • Disk (same fn, disk block): health.level < alert.level ← reversed

All three branches carry the identical comment "modify alert level
only if it has increased and not in maintenance", so the disk branch
is inconsistent with its own stated intent, not just with its
siblings.

SeverityLevel derives PartialOrd in declaration order (Ok < Warning < Critical), so the CPU/memory form reads naturally as "is
the freshly observed level higher than what's currently on the open
alert?". With the disk operands swapped, that question becomes
Critical < Warning, Critical < Ok, etc. - comparisons that are
never true once the alert has already opened at Warning or higher.
Practical effect: a disk alert that opens at WARNING and later
crosses into CRITICAL is never updated - it stays reported at
WARNING (or whatever level it first opened at) until it resolves back
to OK and a fresh alert cycle begins.

Fix

Flip the comparison to alert.level < health.level, matching the
CPU and memory branches exactly. One line changed, no behavior
change to the open/close paths.

Testing

  • cargo check -p komodo_core passes.
  • cargo fmt -p komodo_core -- --check passes (no formatting diff).
  • I looked for existing coverage of this escalation logic before
    deciding whether to add a test: there are currently no #[test]
    functions anywhere in the workspace, and alert_servers reads
    live state through db_client()/resource:: helpers with no
    mocking seam in place. Adding a unit test here would mean
    introducing test infrastructure the project doesn't currently have,
    which felt out of scope for a one-line operand fix - happy to add
    one if there's a preferred approach for exercising this path.

In the server disk-alert path, the comparison guarding alert-level
escalation was `health.level < alert.level`, the reverse of the CPU
and memory paths (`alert.level < health.cpu.level` /
`alert.level < health.mem.level`), both of which carry the comment
"modify alert level only if it has increased".

SeverityLevel derives PartialOrd in declaration order (Ok < Warning <
Critical), so escalating an open alert evaluates `alert.level <
health.level`: is the new level higher than the current one? With
the operands swapped, an open WARNING alert (alert.level = Warning)
checks `Critical < Warning`, which is always false. A disk that first
crosses the warning threshold and later crosses critical never has
its alert level raised - it stays reported at whatever severity it
first crossed until it resolves back to Ok.

Flips the comparison to match the CPU/memory pattern.
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.

1 participant