Repository navigation
Retry backup snapshot node lookup on compaction conflicts - #8652
Open
Amaury Chamayou (achamayou) wants to merge 4 commits into
Open
Amaury Chamayou (achamayou) wants to merge 4 commits into
Amaury Chamayou (achamayou) wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95d800181a6a4870bcde923bfb96d952
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block approval of the backup snapshot lookup retry change.
No open issues 🎉
What changed in this PR
Prevents compaction conflicts during backup snapshot node lookup from aborting a backup node, addressing #8650 while preserving existing download retries and unrelated exception handling.
Changes:
- Retry the primary-node lookup up to three times, using a fresh read-only transaction for each attempt.
- Log and skip the optional snapshot fetch if all lookup attempts encounter compaction conflicts.
- Add deterministic unit tests for transient conflicts, retry exhaustion, and unrelated exception propagation.
Custom instructions used
.github/copilot-instructions.md— repository guidance supplied in the review context..github/instructions/reviewing.instructions.md— security/safety and C++ guidance supplied in the review context.
| File | Description |
|---|---|
src/node/test/backup_snapshot_fetch.cpp |
Tests retry success, exhaustion, and exception propagation. |
src/node/node_state.h |
Applies bounded retries and skips fetching after persistent conflicts. |
src/node/backup_snapshot_fetch.h |
Adds the compaction-specific lookup retry helper. |
CMakeLists.txt |
Registers and links the new unit test. |
Review effort: Balanced
Take the retry callback by value since it is invoked repeatedly rather than forwarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95d800181a6a4870bcde923bfb96d952
Record the backup snapshot fetch fix under 7.0.24, bump the Python SDK version to match, and explain why the lookup retry has no backoff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95d800181a6a4870bcde923bfb96d952
Drop the unnecessary ccf_tasks link from the unit test (the logger is header-only) and link ccf_kv instead, so the test can drive a real kv::Store. Add a test case that triggers the genuine CompactedVersionConflict from BaseTx::get_handle_by_name by compacting between read-version capture and handle creation, guarding against the catch type drifting from the KV throw type. Mark the helper [[nodiscard]] and trim implementation detail from the changelog entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95d800181a6a4870bcde923bfb96d952
Amaury Chamayou (achamayou)
requested a review
from Andrea Piccione (andpiccione)
November 2, 2026 23:54
Andrea Piccione (andpiccione)
approved these changes
Nov 3, 2026
| return; | ||
| } | ||
|
|
||
| files::dump(latest_peer_snapshot->snapshot_data, dst_path); |
There was a problem hiding this comment.
I believe this is one more spot in this task that can still take a backup down. If files::dump fails to write the snapshot, it throws and the worker aborts. Since the fetch is best effort anyway, could we catch that, log it and skip, like the other failure paths here?
| } | ||
| catch (const ccf::kv::CompactedVersionConflict& e) | ||
| { | ||
| LOG_DEBUG_FMT( |
There was a problem hiding this comment.
Could this be LOG_INFO_FMT? That would be good to confirm the fix works. It's rare enough that it won't be noisy.
This branch has not been deployed
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
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.
Motivation
Compaction can race the Nodes-table lookup performed by backup snapshot fetching, allowing
CompactedVersionConflictto escape to the task worker and abort a backup node.Closes #8650
Implementation summary
Retry the primary-node lookup up to three times, creating a fresh read-only transaction for each attempt.
If compaction conflicts persist, log and skip this optional fetch; subsequent snapshot evidence can schedule another fetch.
Keep unrelated exceptions and existing snapshot download retries unchanged.
Unit tests cover successful retry after a transient conflict, exhaustion after three conflicts, propagation of unrelated exceptions, and recovery from a genuine
CompactedVersionConflictraised by a realkv::Storecompacted between read-version capture and handle creation.A
7.0.24changelog entry and matching Python SDK version bump are included.The focused unit test,
ccf_launcherbuild, andscripts/ci-checks.shpass.The tests exercise the retry policy deterministically; they do not reproduce the production race in a running network.
Safety and compatibility
Only the expected compaction conflict is handled locally, preserving task-worker handling of unexpected failures.
The existing scope-exit cleanup still clears the pending fetch task when retries are exhausted.
There are no public API, configuration, ledger, snapshot-format, or consensus-protocol changes.
Mixed-version and recovery compatibility tests were not run.