Skip to content

Comments

refactor: add ensure_not_started() guard#466

Open
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/ensure-not-started
Open

refactor: add ensure_not_started() guard#466
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/ensure-not-started

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Feb 21, 2026

Add a state guard that returns an error if a manager's sync has already been started, and use it in start_sync() implementations instead of inline state checks.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Enhanced error handling for concurrent sync attempts with clearer error messages that identify which specific sync manager is already active.
    • Added centralized guards across sync components to prevent re-entrancy and duplicate sync operations.

Add a state guard that returns an error if a manager's sync
has already been started, and use it in `start_sync()` implementations
instead of inline state checks.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 21, 2026

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

Changes enhance sync error handling by refactoring SyncError::SyncInProgress to carry a ManagerIdentifier and consolidating duplicate inline state guards across multiple sync manager implementations into a centralized ensure_not_started helper function. This improves error specificity and reduces code duplication.

Changes

Cohort / File(s) Summary
Error Type Definition
dash-spv/src/error.rs
Updated SyncInProgress enum variant from unit variant to data variant carrying ManagerIdentifier payload. Updated error formatting to include manager context in messages like "{0} already started".
Sync Manager Implementations
dash-spv/src/sync/block_headers/sync_manager.rs, dash-spv/src/sync/blocks/sync_manager.rs, dash-spv/src/sync/filters/sync_manager.rs
Replaced inline state validation guards with calls to ensure_not_started helper function, removing previous warning logs and early returns in favor of error propagation.
Core Sync Manager
dash-spv/src/sync/sync_manager.rs
Added new pub(super) ensure_not_started helper function that validates state is WaitingForConnections, returning SyncError::SyncInProgress with ManagerIdentifier if not. Updated default start_sync to use this helper instead of inline checks.
Sync Coordinator
dash-spv/src/sync/sync_coordinator.rs
Changed error return from SyncError::SyncInProgress to SyncError::InvalidState in start method guard.
Test Updates
dash-spv/tests/error_types_test.rs
Updated test cases to construct and pattern-match SyncInProgress(ManagerIdentifier) variant. Adjusted error message expectations to reflect manager-specific context in messages.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A scattered warren of guards became one,
ManagerIdentifiers now carry the tale,
Where each sync manager bows to the sun,
Of centralized wisdom that won't ever fail.
Hop, hop! The errors now speak true and clear! 🌟

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main refactoring change: introducing a centralized ensure_not_started() guard function used across multiple sync manager implementations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/ensure-not-started

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@xdustinface xdustinface requested a review from ZocoLini February 21, 2026 14:42
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