fix #413: move internal events from bounded P4 event_tx to unbounded P2 role_tx — eliminate deadlock risk#414
Merged
Conversation
After migrating 6 lifecycle events from bounded event_tx (P4) to unbounded role_tx (P2), BecomeFollower and these events now share the same channel. The ordering is a race: Follower receiving LogPurgeCompleted / PromoteReadyLearners / MembershipApplied after step-down is normal in-flight behavior, not a protocol violation. - follower_state: override 3 methods → Ok(()) instead of default RoleViolation - learner_state: override 2 methods → Ok(()) - raft.rs: add is_fatal() guard to StepDownSelfRemoved and LogPurgeCompleted dispatch arms (symmetry with other 4 arms) - role_state: fix hardcoded current_role "Learner" → "non-Leader" in default handle_log_purge_completed error message - Add dispatch-layer tests for the 6 migrated RoleEvent variants - Add LIFECYCLE_EVENT_MIGRATION_ANALYSIS.md for protocol correctness review Candidate retains RoleViolation — it cannot receive stale Leader events because role_tx is fully drained between Leader→Follower and Follower→Candidate.
…to InternalEvent
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR renames the event pipeline to ChangesInternal and inbound event refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
d-engine-core/src/raft_role/leader_state.rs (1)
1339-1400:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrain remaining pending client queues before returning fatal.
On fatal inbound errors, this branch notifies
pending_write_applyand read queues, but it does not drainpending_client_writesandpending_commit_actions. Since execution returnsErr(Fatal(...))immediately, those waiting senders can be left without a terminal response.💡 Proposed fix
InboundEvent::FatalError { source, error } => { error!("[Leader] Fatal error from {}: {}", source, error); let fatal_status = || tonic::Status::internal(format!("Node fatal error: {error}")); @@ if !self.eventual_read_queue.is_empty() { @@ for (_, sender) in self.eventual_read_queue.drain(..) { let _ = sender.send(Err(fatal_status())); } } + + // Writes waiting for quorum/commit should fail fast on fatal. + self.drain_pending_writes_with_error(ErrorCode::ProposeFailed); + + // Post-commit waiters (e.g. JoinCluster) should also be closed. + for (_, entry) in std::mem::take(&mut self.pending_commit_actions) { + if let PostCommitAction::NodeJoin { sender, .. } = entry.action { + let _ = sender.send(Err(fatal_status())); + } + } + return Err(crate::Error::Fatal(format!( "Fatal error from {source}: {error}" ))); }🤖 Prompt for 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. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 1339 - 1400, The InboundEvent::FatalError handler drains and notifies pending_write_apply, linearizable_read_buffer, pending_reads, lease_read_queue, and eventual_read_queue, but it fails to drain and notify pending_client_writes and pending_commit_actions. Since the handler returns Err(Fatal(...)) immediately after, those waiting senders are left without a terminal response. Add similar notification logic for pending_client_writes and pending_commit_actions before the final return statement, following the same pattern as the other queues by draining each collection with appropriate warn logs and sending the fatal_status() error to each waiting sender.d-engine-core/src/raft_role/follower_state.rs (1)
188-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReply with the updated term after granting a higher-term vote.
Line 233 and Line 250 use
my_termcaptured before Line 218 can advance the follower term, so a higher-term vote request can be answered with the old term.🐛 Proposed fix
- let response = VoteResponse { - term: my_term, + let response = VoteResponse { + term: self.current_term(), vote_granted: new_voted_for.is_some(), last_log_index, last_log_term, }; @@ - let response = VoteResponse { - term: my_term, + let response = VoteResponse { + term: self.current_term(), vote_granted: false, last_log_index, last_log_term, };🤖 Prompt for 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. In `@d-engine-core/src/raft_role/follower_state.rs` around lines 188 - 258, The VoteResponse being sent on lines 233 and 250 uses the stale my_term value captured at the beginning, which does not reflect any term update that may have occurred after the handle_vote_request call on line 218. After updating the current term via self.update_current_term(new_term) on line 225, refresh the my_term variable or directly obtain the current term again before constructing the VoteResponse objects to ensure they reply with the updated term value.
🧹 Nitpick comments (1)
d-engine-core/src/raft_role/candidate_state_test.rs (1)
180-190: ⚡ Quick winAssert the
BecomeLeaderevent in the successful tick test.This test now passes even if
CandidateState::tick()stops sendingInternalEvent::BecomeLeader; keep the receiver and verify the dispatch contract changed by this PR.Test assertion addition
- let (internal_event_tx, _internal_event_rx) = mpsc::unbounded_channel(); + let (internal_event_tx, mut internal_event_rx) = mpsc::unbounded_channel(); let (event_tx, _event_rx) = mpsc::channel(1); @@ assert!( state.tick(&internal_event_tx, &event_tx, &context).await.is_ok(), "Tick should succeed" ); + + assert!(matches!( + internal_event_rx.try_recv(), + Ok(InternalEvent::BecomeLeader) + ));🤖 Prompt for 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. In `@d-engine-core/src/raft_role/candidate_state_test.rs` around lines 180 - 190, The test for CandidateState::tick() in candidate_state_test.rs verifies the tick succeeds but doesn't actually assert that the InternalEvent::BecomeLeader event is dispatched as part of the contract. Remove the underscore prefix from _internal_event_rx to make it an active receiver, and after the tick call, add an assertion that verifies the InternalEvent::BecomeLeader event was received through the channel. This ensures the test properly validates the event dispatch contract and will fail if the event sending is accidentally removed from the CandidateState::tick() implementation.
🤖 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 `@d-engine-core/src/raft_role/follower_state.rs`:
- Line 5: The follower state implementation currently returns a
permission_denied error for DiscoverLeader requests instead of leveraging the
leader information that followers learn from AppendEntries messages. Modify the
follower's DiscoverLeader request handler (found in the code sections around
lines 412-423) to return the known leader information when available instead of
permission_denied, and return a clear unknown leader response when the follower
has not yet learned the leader identity. This allows clients to discover the
leader even when initially contacting a follower rather than the actual leader.
In `@d-engine-core/src/raft_role/leader_state_test/become_follower_test.rs`:
- Line 97: The calls to handle_inbound_event at lines 97, 151, 208, and 261 use
.await.ok() which silently discards any error results, causing tests to miss
regressions if the handler starts returning Err. Replace .await.ok() with
.await.expect() with a descriptive message (or alternatively use
assert!(result.is_ok())) to explicitly assert that handle_inbound_event
succeeds, so any errors will cause the test to fail and catch real regressions.
In
`@d-engine-core/src/raft_role/leader_state_test/pending_commit_actions_test.rs`:
- Around line 8-10: The module documentation contains a stale reference to an
internal event that does not exist. In the doc comments around the
drain_commit_actions description (specifically the line mentioning
`InternalEvent::JoinCommitted`), remove or update the documentation that claims
`drain_commit_actions` fires `InternalEvent::JoinCommitted` when a NodeJoin
entry commits, since the actual implementation handles NodeJoin entries through
direct join response delivery instead of this internal event. Update the docs to
accurately reflect the actual behavior used in the implementation.
In `@d-engine-core/src/raft_role/learner_state.rs`:
- Around line 550-552: The error log message in the `LearnerState`
implementation incorrectly refers to "Follower" instead of "Learner". In the
error logging statement where
`internal_event_tx.send(InternalEvent::SnapshotCreated(result))` fails, change
the error message from "Follower failed to send snapshot creation result" to
"Learner failed to send snapshot creation result" to accurately reflect the role
context of this code.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 648-654: The error context message in the RoleViolation error
block incorrectly identifies PromoteReadyLearners as an InboundEvent when it is
actually an InternalEvent. Update the format string in the context field to
change the text from "InboundEvent::PromoteReadyLearners" to
"InternalEvent::PromoteReadyLearners" to accurately reflect the event type being
handled.
- Around line 685-691: The error context message in the
handle_membership_applied method incorrectly references
InboundEvent::PromoteReadyLearners when it should reference the actual event
being handled. Update the context string in the format! macro to replace the
event name reference with MembershipApplied to accurately reflect which event
triggered this error condition.
In `@d-engine-core/src/raft_test/leader_discovered_tests.rs`:
- Line 136: The test function `test_internal_event_leader_discovered_creation`
is missing the `#[test]` attribute, which means it will not be recognized and
executed by the Rust test harness. Add the `#[test]` attribute directly above
the function definition to mark it as a test that should be executed.
In `@d-engine-core/src/raft.rs`:
- Around line 317-319: The current event processing order processes client
commands and inbound RPCs before ensuring all internal events (P2) are fully
drained, which can cause stale role handling after a required step-down. When
internal events like BecomeFollower are generated during processing of
BecomeLeader, StepDownSelfRemoved, or NotifyNewCommitIndex in the
process_internal_events() call, they should be processed immediately in the
current iteration before proceeding to process_client_cmds() and
process_inbound_events(). Restructure the event processing logic to continue
draining buffered_internal_event until it is completely empty before moving to
P3/P4 operations (client commands and inbound RPCs), ensuring generated internal
events are kept in buffered_internal_event and processed within the same drain
cycle. This applies to all related event processing sections at the mentioned
line ranges.
In `@d-engine-proto/proto/server/storage.proto`:
- Around line 52-59: In the ChunkStatus enum in the storage.proto file, restore
the original numeric values for all existing enum constants
(CHUNK_STATUS_ACCEPTED, CHUNK_STATUS_CHECKSUM_MISMATCH,
CHUNK_STATUS_OUT_OF_ORDER, CHUNK_STATUS_REQUESTED, and CHUNK_STATUS_FAILED) to
their pre-change wire values to maintain backward compatibility. The addition of
CHUNK_STATUS_UNSPECIFIED must not cause existing enum values to be renumbered,
as this breaks wire format compatibility during rolling upgrades. Assign
CHUNK_STATUS_UNSPECIFIED to a numeric value that does not conflict with the
original assignments of the existing constants, preserving the numeric values
that old nodes expect to send and receive when communicating with new nodes.
In `@d-engine-server/src/membership/raft_membership.rs`:
- Around line 762-763: The documentation comments around lines 762-763 and
823-824 incorrectly refer to the "inbound event loop" when describing where
zombie_rx should be passed, but the actual implementation forwards
InternalEvent::ZombieDetected through internal_event_tx which belongs to the
internal event loop. Update both comment blocks to use "internal event loop"
terminology instead of "inbound event loop" to accurately reflect where the
ZombieDetected signals are actually routed and to prevent confusion during
future maintenance and wiring changes.
In `@d-engine-server/src/network/grpc/grpc_raft_service_test.rs`:
- Around line 325-333: The test is silently ignoring the results of both send
operations on test_internal_event_tx (the LogFlushed event send and the
ApplyCompleted event send), which can cause the test to fail
nondeterministically if the receiver is dropped. Replace the let _ = pattern
with explicit assertions on both test_internal_event_tx.send() calls using
.expect() or .unwrap() to ensure they succeed and fail fast with a clear error
message if the raft loop has dropped the receiver.
---
Outside diff comments:
In `@d-engine-core/src/raft_role/follower_state.rs`:
- Around line 188-258: The VoteResponse being sent on lines 233 and 250 uses the
stale my_term value captured at the beginning, which does not reflect any term
update that may have occurred after the handle_vote_request call on line 218.
After updating the current term via self.update_current_term(new_term) on line
225, refresh the my_term variable or directly obtain the current term again
before constructing the VoteResponse objects to ensure they reply with the
updated term value.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 1339-1400: The InboundEvent::FatalError handler drains and
notifies pending_write_apply, linearizable_read_buffer, pending_reads,
lease_read_queue, and eventual_read_queue, but it fails to drain and notify
pending_client_writes and pending_commit_actions. Since the handler returns
Err(Fatal(...)) immediately after, those waiting senders are left without a
terminal response. Add similar notification logic for pending_client_writes and
pending_commit_actions before the final return statement, following the same
pattern as the other queues by draining each collection with appropriate warn
logs and sending the fatal_status() error to each waiting sender.
---
Nitpick comments:
In `@d-engine-core/src/raft_role/candidate_state_test.rs`:
- Around line 180-190: The test for CandidateState::tick() in
candidate_state_test.rs verifies the tick succeeds but doesn't actually assert
that the InternalEvent::BecomeLeader event is dispatched as part of the
contract. Remove the underscore prefix from _internal_event_rx to make it an
active receiver, and after the tick call, add an assertion that verifies the
InternalEvent::BecomeLeader event was received through the channel. This ensures
the test properly validates the event dispatch contract and will fail if the
event sending is accidentally removed from the CandidateState::tick()
implementation.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8230f229-904e-411a-84ec-26432aa4d004
⛔ Files ignored due to path filters (1)
d-engine-proto/src/generated/d_engine.server.storage.rsis excluded by!**/generated/**
📒 Files selected for processing (67)
CHANGELOG.mdREADME.mdd-engine-core/benches/leader_state_bench.rsd-engine-core/src/commit_handler/default_commit_handler.rsd-engine-core/src/commit_handler/default_commit_handler_test.rsd-engine-core/src/commit_handler/mod.rsd-engine-core/src/config/raft.rsd-engine-core/src/event.rsd-engine-core/src/membership.rsd-engine-core/src/network/mod.rsd-engine-core/src/raft.rsd-engine-core/src/raft_role/candidate_state.rsd-engine-core/src/raft_role/candidate_state_test.rsd-engine-core/src/raft_role/follower_state.rsd-engine-core/src/raft_role/follower_state_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/become_follower_test.rsd-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rsd-engine-core/src/raft_role/leader_state_test/client_read_test.rsd-engine-core/src/raft_role/leader_state_test/client_write_test.rsd-engine-core/src/raft_role/leader_state_test/commit_index_test.rsd-engine-core/src/raft_role/leader_state_test/deadline_test.rsd-engine-core/src/raft_role/leader_state_test/event_handling_test.rsd-engine-core/src/raft_role/leader_state_test/fatal_error_test.rsd-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rsd-engine-core/src/raft_role/leader_state_test/membership_change_test.rsd-engine-core/src/raft_role/leader_state_test/pending_commit_actions_test.rsd-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rsd-engine-core/src/raft_role/leader_state_test/pending_reads_test.rsd-engine-core/src/raft_role/leader_state_test/replication_test.rsd-engine-core/src/raft_role/leader_state_test/single_voter_commit_test.rsd-engine-core/src/raft_role/leader_state_test/snapshot_test.rsd-engine-core/src/raft_role/leader_state_test/snapshot_worker_test.rsd-engine-core/src/raft_role/leader_state_test/stale_learner_deadline_test.rsd-engine-core/src/raft_role/leader_state_test/state_management_test.rsd-engine-core/src/raft_role/leader_state_test/worker_lifecycle_test.rsd-engine-core/src/raft_role/learner_state.rsd-engine-core/src/raft_role/learner_state_test.rsd-engine-core/src/raft_role/mod.rsd-engine-core/src/raft_role/raft_role_test.rsd-engine-core/src/raft_role/role_state.rsd-engine-core/src/raft_test/drain_based_batch_architecture_tests.rsd-engine-core/src/raft_test/leader_discovered_tests.rsd-engine-core/src/raft_test/merge_append_entries_tests.rsd-engine-core/src/raft_test/mod.rsd-engine-core/src/raft_test/process_inbound_events_tests.rsd-engine-core/src/raft_test/raft_comprehensive_tests.rsd-engine-core/src/replication/mod.rsd-engine-core/src/state_machine_handler/worker.rsd-engine-core/src/state_machine_handler/worker_test.rsd-engine-core/src/storage/buffered_raft_log.rsd-engine-core/src/storage/buffered_raft_log_test/term_index_test.rsd-engine-core/src/test_utils/mock/mock_raft_builder.rsd-engine-proto/proto/server/storage.protod-engine-server/src/api/embedded_client.rsd-engine-server/src/membership/raft_membership.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/network/grpc/grpc_raft_service_test.rsd-engine-server/src/network/health_monitor.rsd-engine-server/src/network/health_monitor_test.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/mod.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rsd-engine-server/src/test_utils/mock/mock_node_builder.rsd-engine-server/tests/drain_batching/select_fairness_embedded.rsd-engine-server/tests/embedded_client/embedded_client_operations.rsd-engine/src/docs/server_guide/snapshot-guarantees.md
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
What Does This PR Do?
Migrates 6 internal lifecycle events (
CreateSnapshotEvent,SnapshotCreated,LogPurgeCompleted,StepDownSelfRemoved,MembershipApplied,PromoteReadyLearners) from the boundedevent_tx(P4) channel to the unboundedrole_tx(P2) channel, and renamesRaftEvent→InboundEvent/RoleEvent→InternalEventfor clarity.Type:
Why Is This Needed?
Fixes #413. The 6 lifecycle events were sent over the bounded
event_txchannel. Under high inbound RPC load,event_txfills up and background tasks block onsend().awaitwhile the main loop is blocked waiting to process those same RPCs — deadlock.Fix: move these internal-only events (no response channel, safe to drop on wrong role) to the unbounded
role_tx. Follower/Learner now silently ignore stale Leader-only events instead of returningRoleViolation, which is the correct behavior given thatBecomeFollowerand these events now share the same channel and ordering is a race.Checklist
Required:
make testpassesIf changing APIs:
Testing
How tested:
follower_state_test::test_follower_ignores_stale_leader_internal_events,learner_state_test::test_learner_ignores_stale_leader_internal_eventsInternalEventvariants inraft.rsFor bug fixes:
Does This Follow d-engine's Principles?
Reviewer Notes
Channel priority recap:
Because
BecomeFollowerand these events now share P2, ordering is a race. Follower/Learner receivingLogPurgeCompleted/PromoteReadyLearners/MembershipAppliedafter step-down is normal in-flight behavior.CandidateretainsRoleViolation—role_txis fully drained betweenLeader→FollowerandFollower→Candidate, so a Candidate receiving these events is a true protocol violation.is_fatal()guard added toStepDownSelfRemovedandLogPurgeCompleteddispatch arms for symmetry with the other 4 arms.See
LIFECYCLE_EVENT_MIGRATION_ANALYSIS.mdfor full protocol correctness analysis including the 4 open questions (Q1–Q4).Estimated review complexity:
Summary by CodeRabbit
Refactor
Protocol Changes
ChunkStatusin the storage protocol: addedCHUNK_STATUS_UNSPECIFIED, renamed existing values with aCHUNK_STATUS_prefix, and shifted the on-wire numeric codes.Documentation