fix(spv): add distinct error state for SPV sync failures#650
Open
fix(spv): add distinct error state for SPV sync failures#650
Conversation
The SpvManager's progress watcher and sync event handler did not detect fatal sync errors, leaving the UI stuck in "Syncing" status indefinitely. Two detection paths are now in place: 1. spawn_sync_event_handler handles SyncEvent::ManagerError to transition SpvStatus to Error and store the error message. This is the primary path since dash-spv always emits this event on manager failure. 2. spawn_progress_watcher checks SyncState::Error from the progress channel as defense-in-depth (currently blocked by upstream bug dashpay/rust-dashcore#469 where try_emit_progress is not called on error paths). Once SpvStatus::Error is set, the existing ConnectionStatus logic maps it to OverallConnectionState::Disconnected, showing a red indicator icon with "SPV: Error" in the tooltip. Related upstream issues: - dashpay/rust-dashcore#469 (missing progress emit on error) - dashpay/rust-dashcore#470 (QRInfo chain lock retry robustness) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds SPV sync error detection and propagation from the SPV manager into connection status and UI, plus a manual test scenario doc for verifying SPV error display and diagnostic logging. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant SPV as SPV Library
end
rect rgba(200,255,200,0.5)
participant Manager as SPV Manager
end
rect rgba(255,200,200,0.5)
participant Status as ConnectionStatus
end
rect rgba(255,230,200,0.5)
participant UI as TopPanel/UI
end
SPV->>Manager: progress update (SyncState)
alt SyncState::Error
Manager->>Manager: set last_error (Arc<Mutex>)
Manager->>Status: update SpvStatus::Error (+error detail)
Status->>UI: refresh overall state -> OverallConnectionState::Error
UI->>UI: render indicator (magenta + "!")
Manager->>Status: emit diagnostic log (detailed message)
else SyncState::Running/Complete
Manager->>Status: update SpvStatus::Running/Synced
Status->>UI: refresh overall state -> Synced/Syncing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spv/manager.rs`:
- Around line 1082-1087: The progress-watcher branch currently unconditionally
sets last_error to "Sync failed (reported by SPV library)" when is_error is
true, which can overwrite a more detailed error previously set by the sync event
handler; change the logic in the is_error branch (where SpvStatus::Error is
assigned) to open last_error.write(), inspect whether it is None, and only
assign the generic message if err_guard.is_none(), leaving an existing detailed
message intact (keep the existing status_guard = SpvStatus::Error assignment).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/ai-design/2026-02-24-spv-sync-error-status/manual-test-scenarios.mdsrc/spv/manager.rs
Only set the generic "Sync failed" message in last_error when no detailed message has already been stored by the sync event handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/spv/manager.rs (1)
1082-1089: Past concern resolved —is_none()guard correctly preserves detailed error message.The
err_guard.is_none()check ensures the generic fallback message does not overwrite a detailed message already stored by the sync event handler, which is the fix requested in the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 1082 - 1089, The code correctly avoids overwriting a detailed error by checking last_error.write() and err_guard.is_none() before setting a generic message; no code change needed—ensure the conditional around SpvStatus::Error uses the same pattern (is_error branch setting *status_guard = SpvStatus::Error and only writing to last_error when err_guard.is_none()) to preserve previously stored detailed errors from the sync event handler.
🧹 Nitpick comments (1)
src/spv/manager.rs (1)
1082-1089: Consider adding inline unit tests for the new error-transition paths.Neither the
is_errorbranch inspawn_progress_watchernor theSyncEvent::ManagerErrorhandler inspawn_sync_event_handlerhas test coverage. The status/last_error mutation logic can be tested independently of the async spawn by extracting it into a small helper or usingtokio::test. As per coding guidelines, unit tests should be written inline using#[test].Also applies to: 1155-1163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 1082 - 1089, Add inline unit tests covering the error-transition logic by extracting the status/last_error mutation into a small helper (e.g., set_spv_error_status or update_status_on_error) and then write #[test] (or #[tokio::test] if async) cases that exercise the is_error branch used by spawn_progress_watcher and the SyncEvent::ManagerError handling used by spawn_sync_event_handler; tests should call the helper (or invoke the handler functions directly) to assert SpvStatus becomes SpvStatus::Error and last_error is set to Some(...) when an error condition is simulated, and also include a case where last_error is already Some to ensure it is not overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/spv/manager.rs`:
- Around line 1082-1089: The code correctly avoids overwriting a detailed error
by checking last_error.write() and err_guard.is_none() before setting a generic
message; no code change needed—ensure the conditional around SpvStatus::Error
uses the same pattern (is_error branch setting *status_guard = SpvStatus::Error
and only writing to last_error when err_guard.is_none()) to preserve previously
stored detailed errors from the sync event handler.
---
Nitpick comments:
In `@src/spv/manager.rs`:
- Around line 1082-1089: Add inline unit tests covering the error-transition
logic by extracting the status/last_error mutation into a small helper (e.g.,
set_spv_error_status or update_status_on_error) and then write #[test] (or
#[tokio::test] if async) cases that exercise the is_error branch used by
spawn_progress_watcher and the SyncEvent::ManagerError handling used by
spawn_sync_event_handler; tests should call the helper (or invoke the handler
functions directly) to assert SpvStatus becomes SpvStatus::Error and last_error
is set to Some(...) when an error condition is simulated, and also include a
case where last_error is already Some to ensure it is not overwritten.
Distinguish "connected but sync failed" from "never connected" by adding an `OverallConnectionState::Error` variant. SPV sync errors now show a magenta pulsating circle with "!" glyph instead of the same red static circle used for disconnected state. Tooltip shows the specific SPV error message for easier debugging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
- Move last_error write outside status write guard in spawn_progress_watcher to maintain consistent lock ordering (status → release → last_error), eliminating latent deadlock risk. - Update manual test scenarios to match actual implementation: magenta pulsating "!" indicator (not red/static/Disconnected). - Add Scenario 4 to explicitly verify Error vs Disconnected distinction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Audit SummaryReviewed by: Claude Code with a 2-agent team:
Overall Risk: LOW — well-scoped status propagation change with no new attack surface. Findings
Pre-existing / Out-of-scope
Positive Observations
RedundancyBoth agents flagged the nested lock issue (code-reviewer as "overwrite asymmetry", security as "nested lock acquisition"). Security's framing was more precise — the lock ordering is the root concern. Redundancy ratio: 1 finding out of 8 unique = 12.5%. Verdict: Approve. The two MEDIUM findings have been fixed. Remaining LOW/INFO items are acceptable trade-offs for a desktop application. 🤖 Co-authored by Claudius the Magnificent AI Agent |
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.
Issue being fixed or feature implemented
SPV sync gets permanently stuck in "Syncing" status when a fatal error occurs
(e.g., masternode QRInfo failure). The connectivity icon stays orange instead
of turning red, and no error message is shown to the user.
Symptoms observed in logs:
Despite these errors, the UI showed no indication of failure.
Root cause: Two gaps in error detection:
spawn_sync_event_handlerignoredSyncEvent::ManagerErrorevents entirely.spawn_progress_watcheronly checkedis_synced()and defaulted everythingelse to
SpvStatus::Syncing, never checking forSyncState::Error.Additionally, an upstream bug in dash-spv (dashpay/rust-dashcore#469) means
the progress channel never receives the error state update, making gap #2
a dead path until the library is fixed.
What was done?
Error detection (manager.rs)
Added two error detection paths in
src/spv/manager.rs:spawn_sync_event_handler(primary path): Now handlesSyncEvent::ManagerError— transitionsSpvStatustoErrorand storesthe error message in
last_error. This works reliably because dash-spvalways emits this event on manager failure.
spawn_progress_watcher(defense-in-depth): Now checkswatch_progress.state() == SyncState::Errorand transitions accordingly.Currently blocked by upstream bug but will activate once
bug: SyncManager does not emit progress update on manager error path rust-dashcore#469 is fixed.
Distinct Error UI state (connection_status.rs, top_panel.rs, theme.rs)
Previously,
SpvStatus::Errormapped toOverallConnectionState::Disconnected—the same red static circle as "never connected". Users couldn't distinguish
between the two. Now:
OverallConnectionState::Errorvariant (repr3)SpvStatus::Errormaps toOverallConnectionState::Errorexplicitlyfrom
spv_last_error(populated fromSpvManager::status().last_error)DashColors::sync_error_color()for the magenta-red indicator colorErrorarm ("Connection error")Related upstream issues
SyncManagerdoes not emit progress update onerror path
robust
How has this been tested?
cargo clippy --all-features --all-targets -- -D warnings— cleancargo +nightly fmt --all— cleandocs/ai-design/2026-02-24-spv-sync-error-status/manual-test-scenarios.mdBreaking Changes
None
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Documentation