fix: disk alert escalation compares operands in the wrong order - #1592
Open
CaptainVirgil wants to merge 1 commit into
Open
fix: disk alert escalation compares operands in the wrong order#1592CaptainVirgil wants to merge 1 commit into
CaptainVirgil wants to merge 1 commit into
Conversation
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.
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
In
bin/core/src/monitor/alert/server.rs, the disk-alert escalationcheck compares its operands in the wrong order relative to the CPU
and memory alert paths in the same file.
alert_servers, CPU block):alert.level < health.cpu.levelalert.level < health.mem.levelhealth.level < alert.level← reversedAll 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.
SeverityLevelderivesPartialOrdin declaration order (Ok < Warning < Critical), so the CPU/memory form reads naturally as "isthe 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 arenever true once the alert has already opened at
Warningor 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 theCPU and memory branches exactly. One line changed, no behavior
change to the open/close paths.
Testing
cargo check -p komodo_corepasses.cargo fmt -p komodo_core -- --checkpasses (no formatting diff).deciding whether to add a test: there are currently no
#[test]functions anywhere in the workspace, and
alert_serversreadslive state through
db_client()/resource::helpers with nomocking 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.