Skip to content

fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) - #3164

Open
vaijosh wants to merge 2 commits into
apache:masterfrom
vaijosh:SnapshotIssue
Open

fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162)#3164
vaijosh wants to merge 2 commits into
apache:masterfrom
vaijosh:SnapshotIssue

Conversation

@vaijosh

@vaijosh vaijosh commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

This PR addresses a race condition that occurs during snapshot saves when compaction is busy, which could previously lead to corrupted snapshots or stuck partitions.

Main Changes

  • Throw on compaction-busy: Modified the snapshot save behavior to throw an exception rather than returning early/silently failing when compaction is busy.
  • Validate data/ on load: Added validation during the snapshot load process to verify the presence of the data/ directory, preventing the system from loading incomplete snapshots.
  • Reproduction Script: Added test-snapshot-corruption.sh to deterministically reproduce the bug and validate the fix across different storage states.
  • Unit Tests: Added UTs to cover the new validation logic and race condition handling.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • Execute the newly added unit tests.
    • Run the test-snapshot-corruption.sh script to verify the corrupted snapshot detection and prevention.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects (typed here)
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

… load (apache#3162)

- Throw HgStoreException in onSnapshotSave when RocksDB compaction is in
  progress so JRaft retries rather than committing an empty snapshot dir.
- In onSnapshotLoad, fall through to the real load path when should_not_load
  is present but data/ is missing (JVM-killed mid-checkpoint), so JRaft can
  signal the error and request a fresh snapshot from the leader.
- Add unit tests covering both fix paths in HgSnapshotHandlerTest.
- Add docker/test/test-snapshot-corruption.sh, a deterministic Docker
  reproducer that confirms the bug and validates the fix (--fixed mode).

Fixes apache#3162

Co-Authored-By: Claude <noreply@anthropic.com>
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working store Store module tests Add or improve test cases labels Aug 18, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Snapshot loading still silently accepts a non-directory data path, and the added reproducer cannot resolve its compose/root paths on a clean checkout; fixed mode also references a missing Dockerfile. Evidence: exact head 7ee5d42; all 17 exact-head check runs completed successfully.

Comment thread docker/test/test-snapshot-corruption.sh Outdated
Comment thread docker/test/test-snapshot-corruption.sh Outdated
Comment thread docker/test/test-snapshot-corruption.sh
@vaijosh

vaijosh commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @imbajin for review. I have addressed the review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files. store Store module tests Add or improve test cases

Projects

Status: In progress

2 participants