feat(collector): storj warns when satellites cannot reach the node (CashPilot-1gmy) - #321
Conversation
…ashPilot-1gmy) The node's own dashboard API states reachability directly: lastPinged is the last successful satellite dial-back, quicStatus whether UDP 28967 got through. The Aug 2026 incident node carried the zero-value lastPinged for FOUR MONTHS while every dashboard surface looked healthy — the balance keeps ticking from held/storage components while inbound work is dead, which is exactly why the warning rides on a SUCCESSFUL collection. - _reachability_warning probes /api/sno/ after collecting earnings: never-pinged (Go zero time), lastPinged staler than 3h (satellites ping well under hourly; a fixed node was pinged within two minutes), and quicStatus == Misconfigured — the one value SEEN to mean broken; 'Refreshing' is a startup transient and warning on it would cry wolf. - A failing probe is silence, never a verdict: reachability is a bonus reading, and inventing 'unreachable' from a probe that itself failed is the same false confidence this feature exists to remove. - The warning flows through EarningsResult.warning into the existing kind=notice alert pipeline, so it reaches the notification bell with no UI changes. Tests: 8 reachability cases with 4 negative controls (recent ping, Refreshing, absent field, failing probe — all must stay silent) plus the Go timestamp forms (nanosecond fractions, offsets, naive-as-UTC).
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughStorj payout collection now probes node reachability, parses node timestamps, detects stale or missing satellite contact, reports explicit QUIC misconfiguration, and preserves successful earnings results when reachability data is unavailable. ChangesStorj reachability monitoring
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
==========================================
- Coverage 95.57% 95.49% -0.08%
==========================================
Files 51 51
Lines 6685 6866 +181
==========================================
+ Hits 6389 6557 +168
- Misses 296 309 +13
🚀 New features to boost your workflow:
|
…robe diagnostics Round 2 from independent review: - A node started minutes ago legitimately carries the zero-value lastPinged — warning then makes the user's FIRST reading of a healthy new node an unreachability alert, teaching them to ignore the one notice that matters. Nodes younger than 15 minutes (startedAt, verified present on a live node) get grace; an absent startedAt degrades to the warning, never to silence. - The probe gets its own 5s deadline instead of inheriting the collection's 15s — a stalling dashboard must not double the cost of the reading it decorates. - Probe failures now log at debug: silence stays the product behaviour, but a probe that never fires was previously indistinguishable from a healthy node — the invisibility class this feature exists to end. The review's notice-pipeline finding (warnings re-emit every cycle and never reach notify.send) is real, pre-existing, and filed as a bead rather than widened into this PR.
|
@coderabbitai full review |
|
|
@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/collectors/storj.py`:
- Around line 42-45: Update the lastPinged parsing logic around
datetime.fromisoformat to accept only string values containing an explicit time
component, returning None before parsing for date-only values such as
"2026-08-10" or other unsupported shapes. Preserve timezone normalization for
valid timestamps, and add a regression test covering the date-only input.
🪄 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: 045ea9ac-5525-4c29-976c-fd65573626f1
📒 Files selected for processing (2)
app/collectors/storj.pytests/test_collectors_deep.py
…s midnight fromisoformat parses '2026-08-10' as 00:00:00, which would fabricate an hours-stale ping time out of a value that never claimed a time and emit a false offline warning. Go's time.Time marshalling always writes a time component, so anything without one is not a node timestamp — no claim. Flagged by CodeRabbit; regression test added.
1 participant
Why
Fix 3 of the storj dial-back incident review (follows #318, #320). The incident node's dashboard API had been saying "no satellite has ever reached me" (
lastPingedzero-value,quicStatus: Misconfigured) for four months — nothing read it. The balance keeps ticking from held/storage components while inbound work is dead, so container health, earnings movement, and the dashboard all stayed green.What
After each successful earnings collection, the storj collector probes the node's
/api/sno/and attaches a warning to the reading when:lastPinged),quicStatusisMisconfigured— the one value observed to mean broken;Refreshingis a startup transient and warning on it would cry wolf.A failing probe produces silence, never a verdict — reachability is a bonus reading on top of a successful collection. The warning rides
EarningsResult.warninginto the existingkind=noticealert pipeline, so it reaches the notification bell with zero UI changes.Testing
8 reachability cases including 4 negative controls (recent ping,
Refreshing, absent field, failing probe — all must stay silent), Go timestamp parsing (nanosecond fractions,Z/offset forms, naive-as-UTC), and a guard on every case that the earnings figure itself is untouched. Full suite: 4544 passed, 6 skipped, coverage 95.61%; ruff check + format clean.Summary by CodeRabbit