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

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.77%. Comparing base (98477f0) to head (0e1c319).

Files with missing lines	Patch %	Lines
...ache/hugegraph/store/snapshot/SnapshotHandler.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3164      +/-   ##
============================================
- Coverage     37.78%   37.77%   -0.01%     
+ Complexity     6556     6555       -1     
============================================
  Files           800      800              
  Lines         68929    68934       +5     
  Branches       9157     9158       +1     
============================================
- Hits          26046    26042       -4     
- Misses        39824    39832       +8     
- Partials       3059     3060       +1     

☔ 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.

[vaijosh]

@imbajin
I could not found deterministic steps to repro this issue ( because its timing issue). I had encountered this while testing changes related to Cloud storage support feature.

But this PR addresses critical edge case. For users running distributed HugeGraph clusters, this enhancement prevents silent failures or data inconsistency during high-throughput operations when snapshots and compactions overlap.

[bitflicker64]

Blocking: no. Summary: The load-side validation is correct (isDirectory() rather than exists(), and deliberately not requiring a non-empty data/, which keeps empty partitions working) and the fix lands in the handler that PartitionEngine actually wires up; the save-side change is broader than the defect needs, and the new tests and reproducer have a few rough edges. Evidence: read of SnapshotHandler.java, HgSnapshotHandlerTest.java and docker/test/test-snapshot-corruption.sh at 8e121d4; PartitionEngine.java:176-177, PartitionStateMachine.java:192-206 and BusinessHandlerImpl.dbCompaction read for the surrounding lifecycle; gh -R apache/hugegraph pr checks 3164 (all 17 pass).

[bitflicker64]

Blocking: no. Summary: The save-side throw is the right shape and is handled by PartitionStateMachine as a raft EIO status, but none of the new tests runs in any build, so the change merges with 0% patch coverage. The load-side data/ guard sits inside the should_not_load branch, so it covers only the flag-present variant of the signature #3162 records. Evidence: exact-head diff against merge-base 98477f0f (4 files, +216/-3); surefire include lists at hugegraph-store/hg-store-test/pom.xml:225-302; CoreSuiteTest.java:22-44 with the suite annotations commented out; .github/workflows/pd-store-ci.yml:281-296 running common/client/rocksdb/raftcore only; codecov on this head reporting 0% patch coverage, 8 lines missing, all in SnapshotHandler.java; RocksDBSession.java:740-745 already throwing for a missing snapshot path; and git ls-tree -r --name-only 0e1c319 showing docker/test/test-snapshot-corruption.sh absent from this head, though the description still names it as the verification path.

[bitflicker64]

⚠️ None of the new tests runs in any build, so the fix ships without an executed guard.

hg-store-test runs its src/main/java tests through six surefire executions with fixed include lists (hugegraph-store/hg-store-test/pom.xml:225-302): ClientSuiteTest, CoreSuiteTest plus BatchGraphIsolationTest, CommonSuiteTest, RocksDbSuiteTest, ServerSuiteTest, RaftSuiteTest. SnapshotHandlerTest matches none. The two new methods in HgSnapshotHandlerTest would only run through CoreSuiteTest, whose @RunWith/@Suite.SuiteClasses block is commented out with HgSnapshotHandlerTest.class inside it (CoreSuiteTest.java:22-44), and the workflow never runs that profile (.github/workflows/pd-store-ci.yml:281-296). Codecov agrees: 0% patch coverage, 8 lines missing.

Requested change, both parts. This class is pure Mockito, so add it to RaftSuiteTest's @Suite.SuiteClasses, which store-raftcore-test executes. The HgSnapshotHandlerTest methods need a live engine via StoreEngineTestBase, so they additionally need CoreSuiteTest re-enabled and a -P store-core-test step in the workflow.

[bitflicker64]

⚠️ Nested inside the should_not_load branch, this covers only the flag-present variant of the signature #3162 records.

The issue describes the bad directory as __raft_snapshot_meta present, data/ missing, and should_not_load "often missing for early-return path". With the flag absent, shouldNotLoad(reader) is false (SnapshotHandler.java:217-220), this block is skipped, and control reaches businessHandler.loadSnapshot exactly as before. That call already fails: RocksDBSession.loadSnapshot throws Snapshot file %s not exists (RocksDBSession.java:740-745), wrapped by BusinessHandlerImpl.java:1128-1137. So the more common variant is unaffected, and it fails with a RocksDB path error rather than a corruption diagnosis.

Requested change: hoist the data/ check above the shouldNotLoad test and throw with a message naming the corrupt snapshot directory, so both variants are reported the same way.

[bitflicker64]

🧹 Three small things about this exception, assuming the throw survives the discussion on line 99.

The text says the save was skipped, but the save now fails and reaches the operator as a raft EIO status (PartitionStateMachine.java:199-201). "Skipped" describes the behaviour this PR removes; say what happened instead, for example Partition %d snapshot save failed: compaction in progress.

new HgStoreException(String) resolves to EC_FAIL (1000). The neighbouring save failure uses a specific code, EC_RKDB_EXPORT_SNAPSHOT_FAIL (BusinessHandlerImpl.java:1123); a dedicated code here would be easier to grep for in the field.

Line 182 embeds a non-ASCII em dash in the new log.warn. At this head the only Java files under hugegraph-store containing one are the three this PR touches (git grep -l '—' 0e1c319 -- 'hugegraph-store/*.java'); please keep log text ASCII.

[bitflicker64]

🧹 This block duplicates the four imports already present below it.

Lines 38-41 add RaftOutter, SnapshotReader, SnapshotWriter and Message; lines 47-50 import the same four types. Duplicate single-type imports compile, and the checkstyle plugin is not bound for hugegraph-store (only hugegraph-server and hugegraph-commons configure it), but style/checkstyle.xml:54 does flag RedundantImport. The new block also splits the org.junit imports out of their group.

Requested change: drop the newly added lines 38-41 rather than the pre-existing 47-50. That removes the duplication and restores the original import grouping in one edit.

[imbajin]

Blocking: yes. Summary: The snapshot guard still has a check-then-act race and does not guarantee that snapshots cannot overlap compaction. Evidence: exact-head static review of SnapshotHandler.java:97-105, BusinessHandlerImpl.java:1413-1421, and PartitionStateMachine.java:192-204; all non-Codecov exact-head checks are completed.

[imbajin]

⚠️ Blocking: yes. This guard is a non-atomic check followed by the save at line 105. dbCompaction runs asynchronously, initializes compactionState at BusinessHandlerImpl.java:1414, then sets doing at 1418 before compactRange() at 1420; it does not synchronize with this check. If compaction starts after getState() returns (including while the state entry is still null), this path still calls saveSnapshot during compaction, so the change does not guarantee that overlapping snapshots are rejected. Please coordinate the state transition and snapshot save with the same per-partition lock or another atomic protocol, or re-check the state under such a lock immediately before saving. Evidence: exact head SnapshotHandler.java:97-105 and BusinessHandlerImpl.java:1408-1421.