Repository navigation
fix #253: snapshot stale-file corruption on retry + configurable chunk timeout - #368
Merged
Merged
Conversation
…d config reference
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a per-chunk snapshot receive timeout (default 30s) to SnapshotConfig and validates it, uses that timeout during snapshot streaming, changes temp snapshot file open to truncate (avoid append), adds unit and integration tests validating stale-temp behavior and leader-change recovery, and adds "Snapshot Guarantees" docs. ChangesSnapshot Timeout Configurability & Stale File Handling
Sequence DiagramsequenceDiagram
participant Leader
participant Learner
participant SnapshotAssembler
participant Filesystem
Leader->>Learner: initiate snapshot transfer (chunks)
Learner->>SnapshotAssembler: stream incoming chunks (per-chunk timeout)
SnapshotAssembler->>Filesystem: open temp file (create, write, truncate)
SnapshotAssembler->>Filesystem: write chunk N
Note right of SnapshotAssembler: if chunk not received within timeout,\nstream errors/abort
SnapshotAssembler->>Filesystem: flush, close, atomic rename to final snapshot
Learner->>Leader: confirm catch-up / apply snapshot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. 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: 6
🤖 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/config/raft.rs`:
- Around line 513-518: SnapshotConfig::validate() must reject zero-valued
timeouts because Duration::from_secs(0) causes tokio::time::timeout to fire
immediately and silently break snapshot transfers; update
SnapshotConfig::validate() to check receive_chunk_timeout_in_sec (and the other
timeout fields referenced in the same config block) and return an Err or
validation failure if any are 0 (require >= 1 second or a sensible non-zero
minimum), referencing the receive_chunk_timeout_in_sec field name and
SnapshotConfig::validate to locate where to add the guard.
In `@d-engine-server/tests/snapshot_and_recovery/mod.rs`:
- Around line 16-17: The Test Coverage section in mod.rs is out of date: add
bullets for the newly registered modules snapshot_interrupted_transfer_embedded
and snapshot_leader_change_during_transfer_embedded to the "## Test Coverage"
list so the overview matches the actual registered suites; update the coverage
bullets to include those two module names alongside the existing entries and
ensure formatting matches the surrounding list style.
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs`:
- Around line 239-245: Run rustfmt on the test file to fix the formatting issue
flagged by CI: reformat the assert_eq! block and the following info! invocation
in snapshot_and_recovery::snapshot_interrupted_transfer_embedded.rs so the
multiline assert_eq!(actual, format!("value_{i}").into_bytes(), "...") and the
info!("Snapshot-only entries [0, {}, {}] verified on Learner",
snapshot_only_boundary / 2, snapshot_only_boundary - 1) call conform to cargo
fmt style; simply run `cargo fmt` (or `cargo fmt --all`) and commit the
resulting changes.
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs`:
- Around line 197-203: Replace the fixed
tokio::time::sleep(Duration::from_millis(400)).await with a polling loop that
observes the learner's snapshot-transfer progress and only kills the leader when
a mid-transfer condition is detected: repeatedly check the learner's working
directory for the presence of the temporary snapshot file (e.g.
"temp-snapshot.part.tar.gz") while also verifying that the replicated key
(referenced as last_key in the test) is still absent, and once that observable
condition is true call engines.remove(leader_idx) to drop the leader; target the
checks around the learner node object used in this test so you wait for an
actual in-progress snapshot instead of sleeping a fixed duration.
- Around line 247-264: The test currently verifies entries from
new_leader_engine/new_leader_client, but per the review we must verify recovered
data from the learner to catch partial/corrupt snapshot cases; replace the final
verification loop that iterates 0..BASELINE_ENTRIES to read from learner_engine
(or learner_client = learner_engine.client().clone()) instead of
new_leader_engine/new_leader_client and run the same get_linearizable and
assert_eq checks against format!("value_{i}") so the end-of-test integrity
asserts the learner (not the replacement leader) has all expected keys below the
purge boundary.
In `@d-engine/src/docs/server_guide/snapshot-guarantees.md`:
- Line 52: Update the documentation defaults to match the actual constants:
change the config reference table entries for max_log_entries_before_snapshot,
chunk_size, and max_bandwidth_mbps to reflect the code defaults
(default_max_log_entries_before_snapshot = 1000, default_chunk_size = 1024 bytes
(1 KB), default_max_bandwidth_mbps = 1), and revise the inline wording that
currently says "chunks larger than the default 1 MB" to "chunks larger than the
default 1 KB"; apply the same corrections to the repeated section around lines
68-76 so all mentions match the real defaults in d-engine-core's raft.rs.
🪄 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: 6228f2ea-20cf-415d-b519-3d6be7db5655
📒 Files selected for processing (13)
d-engine-core/src/config/raft.rsd-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/state_machine_handler/snapshot_assembler.rsd-engine-core/src/state_machine_handler/snapshot_assembler_test.rsd-engine-core/src/test_utils/common.rsd-engine-server/tests/snapshot_and_recovery/mod.rsd-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_replication_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_follower_generation_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rsd-engine/src/docs/overview.mdd-engine/src/docs/server_guide/mod.rsd-engine/src/docs/server_guide/snapshot-guarantees.md
💤 Files with no reviewable changes (2)
- d-engine-server/tests/snapshot_and_recovery/snapshot_follower_generation_embedded.rs
- d-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_replication_embedded.rs
… defaults, improve leader-change test
…er poll with wait_ready
…e after changed()
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-server/src/api/embedded.rs`:
- Around line 492-493: Update the doc bullet for Self::wait_ready to correctly
describe that it waits for leader election / no-op commit readiness (i.e.,
cluster consensus readiness), not for the local RPC server; explicitly
distinguish it from Self::watch_membership (membership changes) and
Self::leader_change_notifier (leader election notifications) so readers are
directed to the correct signal.
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs`:
- Line 251: The test currently returns Ok(()) while four EmbeddedEngine
instances are still running; before returning, explicitly shut down all
EmbeddedEngine instances created in this test by calling their
shutdown/stop/close method (and await it if async) or otherwise join their
background task(s) so they don't leak — e.g., invoke the shutdown/join method on
each EmbeddedEngine variable used in snapshot_interrupted_transfer_embedded.rs
and await completion before the final Ok(()).
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs`:
- Around line 195-203: The test currently removes the leader wrapper with
engines.remove(leader_idx) after calling learner_engine.wait_ready(), but that
doesn't guarantee the actual leader process stopped; update the test to
explicitly stop and await shutdown of the old leader so we exercise a real crash
during snapshot transfer: locate the leader wrapper referenced by leader_idx /
old_leader_id (the EmbeddedEngine instance) and call its proper
shutdown/stop/await method (e.g., EmbeddedEngine::shutdown(), stop(), or await
termination) instead of merely removing it, ensuring the leader's runtime is
killed and awaited before proceeding with the rest of the test.
- Line 272: The test returns without stopping the remaining EmbeddedEngine
instances (e.g., the learner and surviving voter nodes), leaving tasks running;
before the final Ok(()) call, call and await each node's stop() (or explicitly
invoke EmbeddedEngine::stop()) for all embedded node variables created in the
test (learner, surviving_voters, any other engines) so they are cleanly shut
down and don't outlive the test; ensure you handle any errors from stop()
(propagate or log) and only then return Ok(()) so EmbeddedEngine::drop won't log
skipped stops or leave background tasks running.
🪄 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: 1a63e552-9961-469f-8080-22db0705ea22
📒 Files selected for processing (9)
d-engine-core/src/config/raft.rsd-engine-core/src/event.rsd-engine-server/src/api/embedded.rsd-engine-server/src/membership/membership_snapshot.rsd-engine-server/tests/snapshot_and_recovery/mod.rsd-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rsd-engine-server/tests/watch_and_subscriptions/watch_membership_embedded.rsd-engine/src/docs/server_guide/snapshot-guarantees.md
🚧 Files skipped from review as they are similar to previous changes (1)
- d-engine/src/docs/server_guide/snapshot-guarantees.md
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs`:
- Around line 76-78: The learner's initial_cluster entry for node n3 has an
incorrect role value (role = 3) that conflicts with the primary cluster (where
n3 is role = 1); update the learner config's initial_cluster node entry for name
'n3' (the map with id = 3) to set role = 1 so it matches the primary cluster and
the NODE_ROLE_FOLLOWER enum.
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs`:
- Around line 150-155: The learner's initial_cluster entry incorrectly marks
node 3 as role = 3 (LEADER) causing a topology mismatch; update the learner's
initial_cluster array (the block named initial_cluster in
snapshot_leader_change_during_transfer_embedded.rs) so that the entry for id = 3
uses role = 1 (FOLLOWER) to match the original cluster configuration (ensure id
= 1,2,3 all have role = 1).
🪄 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: 76eb922e-2706-4d0f-8d3c-36bfe39bb14b
📒 Files selected for processing (4)
d-engine-core/src/config/raft_test.rsd-engine-server/src/api/embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs
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
`@d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs`:
- Around line 251-254: The loop calling engine.stop() can return early on the
first Err and leak remaining engines; change the shutdown to attempt stopping
all engines (e.g., iterate and call engine.stop().await for each, collecting any
Errs) and only then return an error if any stops failed; apply the same pattern
for learner_engine.stop().await so all stops are attempted; reference the
engines collection and learner_engine and the stop() calls when making the
change.
🪄 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: 77ab5ac3-b013-4c4c-86e4-766215d388d7
📒 Files selected for processing (2)
d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs
…vote livelock on slow CI
There was a problem hiding this comment.
🧹 Nitpick comments (2)
d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs (1)
208-225: 💤 Low valueRedundant
engine.node_id() != old_leader_idguard —old_leaderwas already removed.
engines.remove(leader_idx)at line 205 drops the old leader from the slice, so the inner loop at line 212 can no longer encounter it. The!= old_leader_idpredicate is always true in this loop. Not a correctness bug, just dead logic.🤖 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-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs` around lines 208 - 225, The loop checking for a new leader contains a redundant guard comparing engine.node_id() != old_leader_id because engines.remove(leader_idx) already removed the old leader; remove the dead predicate from the inner if so the condition is simply engine.is_leader() (leave new_leader_id assignment, break logic and timeout sleep intact), referencing engines.remove(leader_idx), old_leader_id, new_leader_id, engine.is_leader(), and engine.node_id() to locate and update the code.d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs (1)
82-88: ⚡ Quick winConsider adding
[raft.election]to the cluster config for consistent CI stability.The companion test (
snapshot_leader_change_during_transfer_embedded.rs) pinselection_timeout_min = 300/election_timeout_max = 3000to bound re-election latency. This test writes 150 entries before proceeding; a transient leader step-down with a large default election timeout could push total runtime past the 15-secondwait_for_snapshotbudget on slow CI runners.🔧 Suggested addition
[raft] general_raft_timeout_duration_in_ms = 5000 + +[raft.election] +election_timeout_min = 300 +election_timeout_max = 3000 + [raft.snapshot]🤖 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-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs` around lines 82 - 88, Add an explicit [raft.election] section to the cluster config used in snapshot_interrupted_transfer_embedded.rs to bound re-election latency: set election_timeout_min and election_timeout_max (e.g., 300 and 3000) so leader re-elections are predictable under CI; update the config string that currently contains [raft] and [raft.snapshot] (the same config used by wait_for_snapshot and the test that writes 150 entries) to include the new [raft.election] block with those timeout values.
🤖 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.
Nitpick comments:
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rs`:
- Around line 82-88: Add an explicit [raft.election] section to the cluster
config used in snapshot_interrupted_transfer_embedded.rs to bound re-election
latency: set election_timeout_min and election_timeout_max (e.g., 300 and 3000)
so leader re-elections are predictable under CI; update the config string that
currently contains [raft] and [raft.snapshot] (the same config used by
wait_for_snapshot and the test that writes 150 entries) to include the new
[raft.election] block with those timeout values.
In
`@d-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rs`:
- Around line 208-225: The loop checking for a new leader contains a redundant
guard comparing engine.node_id() != old_leader_id because
engines.remove(leader_idx) already removed the old leader; remove the dead
predicate from the inner if so the condition is simply engine.is_leader() (leave
new_leader_id assignment, break logic and timeout sleep intact), referencing
engines.remove(leader_idx), old_leader_id, new_leader_id, engine.is_leader(),
and engine.node_id() to locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 291533c2-65bf-4663-a959-6ea3daf9dc3b
📒 Files selected for processing (4)
d-engine-server/src/api/embedded_test.rsd-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rsd-engine-server/tests/watch_and_subscriptions/watch_membership_embedded.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- d-engine-server/tests/watch_and_subscriptions/watch_membership_embedded.rs
Merged
10 of 12 tasks
11 tasks done
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?
Fixes two production bugs in snapshot transfer: (1) a stale partial temp file from an interrupted transfer is now truncated instead of appended to on retry, preventing silent snapshot corruption; (2) the per-chunk receive timeout is now configurable via
SnapshotConfiginstead of hardcoded to 10 seconds. Adds fault-path integration tests and a newsnapshot-guarantees.mdoperator guide.Type:
Why Is This Needed?
Bug 1 — stale temp file corruption (production impact)
When a snapshot transfer fails mid-way (timeout, leader crash, network drop),
temp-snapshot.part.tar.gzremains on disk. On the next retry, the old code usedappend(true)— new chunks were appended after the stale data, producing a corrupted tar.gz. The leader retries indefinitely; the follower can never apply the snapshot and gets permanently stuck.Fix:
SnapshotAssembler::new()now opens the temp file withtruncate(true), clearing stale data before every new transfer.Bug 2 — hardcoded chunk timeout
receive_chunk_timeout_in_secwas hardcoded to 10 seconds inprocess_snapshot_stream(). Large snapshots over slow networks always timed out. Fix: moved toSnapshotConfigwith a default of 30 seconds.Checklist
Required:
make testpassesIf changing APIs:
snapshot-guarantees.md+overview.md)Testing
How tested:
test_assembler_on_stale_temp_file_does_not_append_old_data— red before fix, green after; confirmsappend(true)→truncate(true)changetest_snapshot_receiver_truncates_stale_temp_file— pre-injects stale file, verifies learner catches up and snapshot-only entries (purged from log) are readabletest_snapshot_transfer_resumes_after_leader_change— kills leader mid-transfer, verifies learner recovers from new leaderFor bug fixes:
Does This Follow d-engine's Principles?
Reviewer Notes
Bug 1: The key diff is one line in
snapshot_assembler.rs—.append(true)→.write(true).truncate(true). The rest is tests and docs.Bug 2:
SnapshotConfiggains one new fieldreceive_chunk_timeout_in_sec(default 30). Existing configs without this field get the default via#[serde(default)]— no breaking change.Estimated review complexity:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation