fix(vn): prevent possible inf loop on shutdown - #1625
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors many broadcast channels from strong Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Worker as HotstuffWorker
participant Handler as Handler (e.g., OnMessageValidate)
participant Weak as broadcast::WeakSender
participant Temp as temporary Sender(1)
participant Shutdown as ShutdownSignal
App->>Worker: new(tx_events)
Worker->>Worker: store strong tx_events
Worker->>Handler: new(tx_events.downgrade())
Note over Handler,Weak: publishing an event
Handler->>Weak: upgrade()
alt upgrade succeeds
Weak->>Worker: send event (via upgraded Sender)
else upgrade fails
Handler->>Temp: create Sender::new(1)
Temp->>Handler: subscribe/send on temp
end
Note over App,Shutdown: shutdown flow
App->>Shutdown: trigger
App->>Worker: join_all()
Worker->>Handler: signal shutdown
Worker->>App: all tasks joined / exit
sequenceDiagram
participant Mempool as Mempool Service
participant Requests as mempool_requests
participant Gossip as gossip.next_message()
participant Consensus as Consensus events
participant Node as Node controller
loop service loop
alt request arrives
Requests->>Mempool: req
Mempool->>Mempool: handle req
else gossip message
Gossip->>Mempool: message
Mempool->>Mempool: handle gossip
else consensus event
Consensus->>Mempool: event
alt event == Closed or None
Mempool->>Node: log and break (shutdown)
else other
Mempool->>Mempool: process
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/epoch_oracles/src/configured/real_time_ticker.rs (1)
100-100: Comment could be more precise about timing.The comment references
self.epoch, but at this point in the code,self.epochhas already been incremented (line 95). The catching-up condition was actually checked earlier at line 94 using the original value. Consider clarifying the comment to avoid confusion about which epoch value is being referenced.crates/template_manager/src/implementation/template_sync_task.rs (1)
195-199: Consider logging failed clients before clearing.The code clears
recently_failed_clientsbefore returning the error, which discards debugging information about which validators were attempted and failed. Consider logging the failed clients or the count before clearing to aid troubleshooting.Also, the
shrink_to(100)capacity limit appears arbitrary. Consider adding a named constant with documentation explaining why 100 was chosen.else { // No more committee members to try - no real choice but to clear the failed list and try again if this // is re-attempted + debug!(target: LOG_TARGET, "Exhausted all validators. Failed clients count: {}", self.recently_failed_clients.len()); self.recently_failed_clients.clear(); self.recently_failed_clients.shrink_to(100); return Err(TemplateSyncError::NoMoreSyncValidators); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
applications/tari_indexer/src/network_state_sync/committee_client.rs(1 hunks)applications/tari_validator_node/src/bootstrap.rs(1 hunks)applications/tari_validator_node/src/consensus/mod.rs(1 hunks)applications/tari_validator_node/src/event_subscription.rs(1 hunks)applications/tari_validator_node/src/node.rs(4 hunks)applications/tari_validator_node/src/p2p/services/mempool/service.rs(2 hunks)crates/consensus/src/hotstuff/on_message_validate.rs(5 hunks)crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(3 hunks)crates/consensus/src/hotstuff/on_receive_local_proposal.rs(3 hunks)crates/consensus/src/hotstuff/state_machine/idle.rs(2 hunks)crates/consensus/src/hotstuff/state_machine/worker.rs(2 hunks)crates/consensus/src/hotstuff/worker.rs(4 hunks)crates/epoch_manager/src/service/epoch_manager_service.rs(2 hunks)crates/epoch_manager/src/service/handle.rs(2 hunks)crates/epoch_oracles/src/configured/real_time_ticker.rs(2 hunks)crates/template_manager/src/implementation/downloader.rs(2 hunks)crates/template_manager/src/implementation/template_sync_task.rs(1 hunks)networking/core/src/builder.rs(1 hunks)networking/core/src/handle.rs(2 hunks)networking/core/src/worker.rs(2 hunks)networking/libp2p-substream/src/behaviour.rs(4 hunks)networking/libp2p-substream/src/event.rs(0 hunks)networking/libp2p-substream/src/metrics.rs(1 hunks)
💤 Files with no reviewable changes (1)
- networking/libp2p-substream/src/event.rs
🧰 Additional context used
🧬 Code graph analysis (10)
applications/tari_validator_node/src/event_subscription.rs (3)
crates/consensus/src/hotstuff/on_message_validate.rs (2)
new(55-77)new(477-482)crates/consensus/src/hotstuff/worker.rs (1)
new(111-235)crates/epoch_manager/src/service/handle.rs (2)
new(38-48)subscribe(131-138)
networking/core/src/handle.rs (3)
networking/core/src/worker.rs (1)
new(109-141)applications/tari_validator_node/src/event_subscription.rs (1)
new(34-36)crates/consensus/src/hotstuff/worker.rs (1)
new(111-235)
crates/epoch_manager/src/service/epoch_manager_service.rs (3)
applications/tari_validator_node/src/event_subscription.rs (1)
new(34-36)crates/consensus/src/hotstuff/worker.rs (1)
new(111-235)crates/epoch_manager/src/service/handle.rs (2)
new(38-48)current_epoch(283-285)
applications/tari_validator_node/src/p2p/services/mempool/service.rs (4)
applications/tari_validator_node/src/p2p/services/mempool/gossip.rs (4)
next_message(73-86)msg(153-158)subscribe(88-104)unsubscribe(106-112)applications/tari_validator_node/src/event_subscription.rs (1)
subscribe(38-41)crates/epoch_manager/src/service/handle.rs (1)
subscribe(131-138)applications/tari_validator_node/src/p2p/services/consensus_gossip/service.rs (2)
subscribe(123-141)unsubscribe(143-151)
crates/consensus/src/hotstuff/state_machine/idle.rs (1)
crates/consensus/src/hotstuff/worker.rs (1)
discard_messages(631-642)
applications/tari_validator_node/src/consensus/mod.rs (1)
applications/tari_validator_node/src/event_subscription.rs (1)
new(34-36)
crates/epoch_manager/src/service/handle.rs (3)
applications/tari_validator_node/src/event_subscription.rs (1)
new(34-36)crates/consensus/src/hotstuff/worker.rs (1)
new(111-235)networking/core/src/handle.rs (2)
new(112-114)new(175-185)
networking/core/src/builder.rs (2)
networking/core/src/worker.rs (1)
new(109-141)networking/core/src/handle.rs (3)
new(112-114)new(175-185)local_peer_id(292-294)
crates/consensus/src/hotstuff/on_message_validate.rs (3)
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
publish_event(1535-1539)crates/consensus/src/hotstuff/on_receive_local_proposal.rs (1)
publish_event(542-546)crates/consensus/src/hotstuff/worker.rs (1)
publish_event(1096-1098)
applications/tari_validator_node/src/node.rs (1)
applications/tari_validator_node/src/bootstrap.rs (1)
join_all(425-431)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (33)
crates/epoch_oracles/src/configured/real_time_ticker.rs (1)
174-177: Verify these test adjustments align with the PR's shutdown fix.The changes to this ignored test appear to be developer testing adjustments. However, this file shows only comment and test changes, with no modifications to shutdown logic, channel handling, or WeakSender usage mentioned in the PR objectives. Please confirm whether the main shutdown fixes are in other files not included in this review.
applications/tari_validator_node/src/p2p/services/mempool/service.rs (2)
99-141: Excellent shutdown handling—addresses the infinite loop fix.The explicit channel closure handling for all three event sources (mempool_requests, gossip, consensus_events) properly prevents the infinite loop mentioned in the PR objectives. Each path now:
- Logs the shutdown reason for debugging
- Cleanly breaks the event loop
- Ensures cleanup at line 149 executes
The
Laggedevent handling (line 133-135) appropriately logs and continues, whileClosed(line 136-139) correctly triggers shutdown.
151-151: Clear shutdown logging.The final shutdown message appropriately confirms service termination after cleanup completes.
crates/template_manager/src/implementation/downloader.rs (2)
28-28: LGTM: Logging infrastructure added.The addition of logging import and target constant provides good observability for the shutdown behavior.
Also applies to: 35-36
74-77: Shutdown logging improves observability, but verify error handling.The shutdown log message is a good addition for observability. However, note that line 81 uses
unwrap()when sending completed downloads. If thecompleted_downloadsreceiver is dropped during shutdown while pending downloads are still completing, this will panic.Please verify the shutdown ordering to ensure the
completed_downloadsreceiver remains open while this worker is processing, or consider handling the send error gracefully:if let Err(_) = self.completed_downloads.send(result).await { info!(target: LOG_TARGET, "Completed downloads channel closed, dropping result"); break; }networking/libp2p-substream/src/behaviour.rs (1)
58-68: LGTM: Clean refactoring of stream ID tracking.The rename from
next_outbound_stream_idtonext_stream_idis consistent throughout and preserves the original logic. The more generic name may accommodate future enhancements or simply provide clearer semantics.Also applies to: 91-91, 116-119
crates/consensus/src/hotstuff/state_machine/worker.rs (2)
137-137: LGTM! Clear shutdown logging.The log message provides clear feedback when the consensus state machine is shutting down, aiding observability.
147-147: Good use of biased select for responsive shutdown.The
biasedmodifier ensures the shutdown signal is checked first, preventing delays when shutdown is triggered. This directly addresses the infinite loop issue described in the PR.crates/consensus/src/hotstuff/state_machine/idle.rs (1)
61-85: Excellent shutdown handling for closed epoch manager stream.The biased select combined with explicit handling of the
Closedevent ensures the idle state transitions to shutdown when the epoch manager event stream closes. The debug logging provides clear observability.applications/tari_validator_node/src/event_subscription.rs (1)
31-41: WeakSender migration correctly implemented.The upgrade-with-fallback pattern ensures subscribers get a receiver even if the original sender is dropped. The dummy channel (capacity 1) will immediately close, which is the expected behavior for shutdown scenarios.
networking/core/src/worker.rs (2)
200-200: LGTM! Consistent shutdown logging.The log message aligns with shutdown logging patterns across other modules.
1037-1039: Critical fix: Inbound substream failures are now properly reported.Previously, inbound substream failures were silently ignored. This change correctly propagates the error to the waiting caller, matching the pattern used for outbound failures (lines 1048-1049).
applications/tari_validator_node/src/consensus/mod.rs (1)
117-117: LGTM! Consistent with WeakSender migration.The downgrade() call aligns with the updated EventSubscription API and the project-wide shift to WeakSender-based event broadcasting.
applications/tari_validator_node/src/bootstrap.rs (1)
425-431: LGTM! Clean consolidation of service joins.The method correctly awaits all service handles and propagates any errors. Consuming
selfprevents misuse after shutdown.crates/epoch_manager/src/service/epoch_manager_service.rs (1)
87-87: LGTM! WeakSender usage prevents shutdown deadlocks.Passing
events.downgrade()ensures handles don't prevent the epoch manager from shutting down by holding strong references to the event channel.applications/tari_validator_node/src/node.rs (3)
66-69: LGTM! First SIGINT triggers graceful shutdown.The first Ctrl+C now triggers the shutdown signal and breaks to the waiting phase, allowing services to clean up gracefully.
83-87: LGTM! Service exit handling improved.Now checks if shutdown was already triggered before logging a warning, reducing noise when shutdown is intentional.
98-110: Excellent two-phase shutdown implementation.The shutdown flow is well-designed:
- First phase: Services are signaled to shut down and the main loop exits.
- Second phase: Waits for all services via
join_all()with clear logging.- Escape hatch: Second Ctrl+C forces immediate exit for unresponsive scenarios.
The
process::exit(1)on second SIGINT is appropriate here as a last-resort mechanism when services fail to shut down gracefully.networking/core/src/builder.rs (1)
147-147: LGTM! Clean WeakSender migration.The change correctly passes a downgraded (weak) sender to
NetworkingHandle::new. The worker retains the strong sender (line 130), ensuring the broadcast channel remains open as long as the worker is running, while handles can safely hold weak references.crates/consensus/src/hotstuff/on_receive_local_proposal.rs (2)
78-78: LGTM! Correct WeakSender field and constructor signature.The field and constructor have been updated to accept
broadcast::WeakSender<HotstuffEvent>, aligning with the broader migration pattern to avoid strong reference cycles.Also applies to: 92-92
542-546: LGTM! Safe event publication with guarded upgrade.The
publish_eventhelper correctly upgrades theWeakSenderbefore sending and silently ignores send failures when the sender has been dropped (i.e., during shutdown). This prevents potential deadlocks and ensures clean shutdown behavior.crates/epoch_manager/src/service/handle.rs (2)
34-34: LGTM! Correct WeakSender field and constructor signature.The field and constructor have been properly updated to use
broadcast::WeakSender<EpochManagerEvent>, consistent with the shutdown-safety improvements across the codebase.Also applies to: 40-40
131-138: LGTM! Robust subscribe implementation with shutdown fallback.The implementation correctly handles the case where the epoch manager has shut down by creating a dummy sender with capacity 1. Any subscriber will receive an immediately-closed receiver, which is the expected behavior when subscribing after shutdown.
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (2)
74-74: LGTM! Correct WeakSender field and constructor signature.The field and constructor have been updated to accept
broadcast::WeakSender<HotstuffEvent>, consistent with the pattern applied across all consensus handlers.Also applies to: 85-85
1535-1539: LGTM! Safe event publication with guarded upgrade.The
publish_eventhelper follows the established pattern of upgrading theWeakSenderbefore sending, gracefully handling shutdown scenarios where the sender has been dropped.crates/consensus/src/hotstuff/worker.rs (3)
159-159: LGTM! Correct WeakSender distribution to handlers.The worker correctly retains the strong
broadcast::Sender<HotstuffEvent>(line 82, 232) while passing downgraded weak references to internal handlers (lines 159, 176). This ownership model ensures the broadcast channel remains open as long as the worker is alive, while handlers can safely hold weak references that won't prevent shutdown.Also applies to: 176-176, 232-232
626-628: LGTM! Useful shutdown signal accessor.The new
shutdown_signal()accessor provides external access to the worker's shutdown handle, which may be useful for coordinating shutdown across components.
631-642: LGTM! Enhanced shutdown handling in discard_messages.The addition of explicit shutdown handling with a biased
select!ensures that shutdown signals are prioritized over message discarding. Thebiasedannotation ensures the shutdown branch is checked first, preventing potential message-processing delays during shutdown.networking/core/src/handle.rs (2)
171-171: LGTM! Correct WeakSender field and constructor signature.The field and constructor have been properly updated to use
broadcast::WeakSender<NetworkingEvent>, aligning with the shutdown-safety improvements.Also applies to: 178-178
187-192: LGTM! Robust subscribe_events implementation with shutdown fallback.The implementation correctly handles the case where the networking service has shut down by creating a dummy sender with capacity 1. This ensures any subscriber receives an immediately-closed receiver, which is appropriate for post-shutdown subscriptions.
crates/consensus/src/hotstuff/on_message_validate.rs (3)
48-48: LGTM! Correct WeakSender field and constructor signature.The field and constructor have been updated to accept
broadcast::WeakSender<HotstuffEvent>, consistent with the pattern applied across all consensus handlers.Also applies to: 63-63
207-209: LGTM! Event publications routed through guarded helper.Both
ParkedBlockReadyandProposedBlockParkedevents are now published via thepublish_eventhelper (lines 207, 273), which ensures safe shutdown behavior through the guarded upgrade pattern.Also applies to: 273-278
444-448: LGTM! Safe event publication with guarded upgrade.The
publish_eventhelper follows the established pattern of upgrading theWeakSenderbefore sending, gracefully handling shutdown scenarios where the sender has been dropped.
2 participants
Description
fix(vn): prevent possible inf loop on shutdown
fix: rpc session never reporting failure to caller
Motivation and Context
In some cases the idle state would cause an infinite loop, preventing tokio from exiting. This was caused by the epoch event broadcast channel not closing as expected. This PR fixes this by using WeakSender's in handles, while maintaining the sender in the epoch manager. Once the epoch manager exits, the sole sender is dropped and any subscriptions will close.
The mempool service now shuts down cleanly.
improved force shutdown handling on second SIGINT (ctrl+c)
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Chores