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

[vaijosh]

Purpose of the PR

• close [Bug] A race between RocksDB compaction and periodic Raft snapshot save can commit an invalid snapshot directory that contains __raft_snapshot_meta but no data/ #3162

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

[imbajin]

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.

[vaijosh]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.25%. Comparing base (431f6e6) to head (f2b7219).

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3164       +/-   ##
=============================================
+ Coverage     41.06%   93.25%   +52.19%     
+ Complexity      519       65      -454     
=============================================
  Files           771        9      -762     
  Lines         65962      267    -65695     
  Branches       8766       22     -8744     
=============================================
- Hits          27088      249    -26839     
+ Misses        36008        8    -36000     
+ Partials       2866       10     -2856     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[imbajin]

Blocking: no. Summary: The snapshot handling change is covered by green exact-head checks, but the new reproducer still cannot validate the compaction-busy save fix. Evidence: exact-head CI and Codecov checks completed successfully; see the inline finding.

[imbajin]

⚠️ Blocking: no. The default mode does not exercise the compaction-busy save path: it first creates a completed snapshot, then deletes data/ and should_not_load at lines 211-214 and labels the resulting load error as BUG REPRODUCED. The fixed implementation would hit the same injected corruption, while --fixed only checks the load-side warning. Please add a run that triggers snapshot while compaction is actually active and asserts the save failure/retry, or narrow the script's claim. Evidence: lines 168-178 and 211-214; /test/snapshot only invokes PartitionEngine.snapshot() and the script never invokes /test/compact.

[vaijosh]

Hi @imbajin
You're correct. The save-side throw (SnapshotHandler.java:98–101) only fires if compaction is actually in flight when snapshot() is called, and we have no reliable way to hold that state long enough. Because /test/compact submits a background task and returns immediately, the race window is too narrow to hit deterministically. This would make the test flaky.

Currently, the script simulates the outcome of that race (a missing data/ dir) by removing it manually, which lets us at least verify the load-side behavior. I have added a comment in the script to make this clear.

If there is a way to deterministically reproduce the save-side throw, I would be happy to incorporate it into the script.