feat(alerts): give collector notices the same dedupe and push that errors get (CashPilot-vb78) - #326
Merged
Merged
Conversation
…rors get (CashPilot-vb78)
A notice — a caveat on a SUCCESSFUL reading, like 'satellites cannot
reach this node' — only ever reached the bell, which is read by whoever
happens to open the UI. 'Nobody looked for days' is the incident that
class of warning comes from, while errors already pushed out-of-band on
first occurrence.
Notices now go through database.record_alert('notice', ...) with the
same once-per-window dedupe before notify.send, and a warning-free
success clears the stored row so the NEXT warning pushes again instead
of being deduped into silence forever. The bell keeps its per-cycle
behaviour; only the out-of-band push is gated.
Tests: fresh notice pushes once with kind=notice; a deduped notice stays
silent (control); a warning-free success clears the stored notice; a
success with no prior notice clears nothing (control).
📝 WalkthroughWalkthroughCollection runs now track platform notices separately from collector failures. New warnings create deduplicated ChangesPlatform Notice Tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
=======================================
Coverage 95.50% 95.51%
=======================================
Files 51 51
Lines 6878 6883 +5
=======================================
+ Hits 6569 6574 +5
Misses 309 309
🚀 New features to boost your workflow:
|
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/main.py`:
- Around line 643-645: Filter previously_alerting in app/main.py lines 643-645
to include only entries whose kind is "collector", while keeping
previously_noticing limited to "notice" entries. In tests/test_beads_batch_25.py
lines 243-248, replace the broad assert_any_await check with an exact assertion
that only clear_alerts("notice", "storj") is called.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7e41746-0261-4b2a-b6ca-40f1475d9860
📒 Files selected for processing (2)
app/main.pytests/test_beads_batch_25.py
GeiserX
added a commit
that referenced
this pull request
Aug 13, 2026
* feat(alerts): a worker going offline now notifies, not just logs The 42-hour incident had two halves. #330 fixed the worker's half — docker ps now tells the truth. This is the UI's half: it KNEW within 3 minutes (it wrote 'offline' to its own database) and told nobody. The only witness was the fleet page, and the whole incident happened because nobody was looking at it. The online->offline transition now records a durable alert and pushes out-of-band with the same once-per-window dedupe collectors get, the bell shows every currently offline worker (re-derived from the workers table on each rebuild, so the hourly wholesale replacement of the list cannot silently un-report a dead machine, and it survives a UI restart), and the recovery heartbeat clears the stored alert so the NEXT outage notifies again instead of being deduped into silence. Also restores 'notice' bell entries across UI restarts: #326 made notices durable, but the warm filter dropped them, so a recovery inside the gap skipped clear_alerts and the stale row swallowed the next genuine warning within the cooldown window. The pre-heartbeat state read is best-effort by design: it failing must never reject a heartbeat — the upsert one line below is the loud path. 10 new tests incl. negative controls; the transition mechanism is mutation-verified (disabling the alert path turns the test red). Full suite 4674 passed, coverage 95.64%. * fix(alerts): worker alert identity, atomic transition, retryable lifecycle Review round on #331, all four findings verified then fixed: - Identity is client_id, not the display name: workers.name is the container hostname — cosmetic, mutable across recreates, shareable by two hosts — so keying alerts on it let one worker suppress or clear another's. Alert rows and pushes key on client_id; the name travels in the message/title; bell entries carry both; warm restoration resolves stored ids to current display names (a deleted worker keeps the raw id — still actionable, never wrong). - The offline transition is atomic with its evidence: the sweep marks offline via a conditional UPDATE on the exact heartbeat it decided from, so a recovery heartbeat landing in the read-write gap wins instead of being alerted offline at its exact moment of coming back. - The lifecycle is retryable end to end: a still-offline worker re-attempts the durable alert every sweep (the record_alert cooldown makes the successful case a no-op, so a failed insert or push is retried in 2 minutes instead of being lost to the already-offline state); the heartbeat route's recovery clear is best-effort (a DB hiccup must never fail a HEARTBEAT — and on failure the bell entry deliberately stays so memory and disk cannot diverge); the sweep reconciles any lingering alert it finds on an online worker. - Tests read the fleet key from the environment instead of embedding a second copy of the literal. 15 tests in the lifecycle file (+ race, retry, reconciliation, failed-clear-never-fails-heartbeat cases); two pre-existing sweep tests updated to the conditional-mark call shape they now exercise. Full suite 4679 passed, coverage 95.55%.
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
1 participant
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.
Why
A notice — a caveat on a successful reading, like #321's "satellites cannot reach this node" — only ever reached the notification bell, which is read by whoever happens to open the UI. "Nobody looked for days" is the incident that class of warning comes from. Errors already push out-of-band once per window; notices got nothing.
What
database.record_alert("notice", ...)— the same once-per-window dedupe errors use — beforenotify.send(kind="notice"). A node broken for a week does not notify every hour.Testing
Four tests including two negative controls: a deduped notice must stay silent, and a success with no prior notice must clear nothing. Full suite on the rebased head: see below. ruff clean.
Summary by CodeRabbit
New Features
Bug Fixes