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.

[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]

⚠️ Throwing for the whole compaction window is broader than the defect in #3162 requires.

BusinessHandlerImpl.dbCompaction sets doing immediately before op.compactRange() and clears it to compactionDone immediately after (BusinessHandlerImpl.java:1418-1421) — a full RocksDB range compaction, which on a large partition runs for a long time. PartitionStateMachine.onSnapshotSave turns this exception into done.run(new Status(RaftError.EIO, e.toString())) (PartitionStateMachine.java:199-201), so with this change every jraft periodic snapshot that happens to overlap a compaction is reported as an IO failure and the raft log is not truncated for the duration of that compaction.

The same compaction path already arranges for a snapshot as soon as it finishes — it logs "dbCompaction end and start to do snapshot" and submits SYNC_BLANK_TASK (BusinessHandlerImpl.java:1422-1437) — which suggests the previous early return was a deliberate skip rather than an oversight. The defect reported in #3162 is that the skip let jraft commit the snapshot as valid, not that the skip existed.

Requested change: bound the impact instead of failing outright — wait a configurable interval for the state to leave doing (the setAndNotifyState path already exists) and only throw if it is still busy when that budget expires, so an ordinary compaction/snapshot overlap resolves itself rather than surfacing as a raft-level IO error.

[bitflicker64]

🧹 This test asserts nothing on its success path, and the new tests sit in the wrong test class.

handler.onSnapshotSave(stubWriter) is wrapped in try { ... } catch (HgStoreException e) with the only assertion inside the catch, so if no exception is thrown the test passes without checking anything — and it would still pass if the compaction guard at SnapshotHandler.java:98-102 were deleted outright, since the guard is not the thing being exercised on this path.

Separately, all four new tests construct SnapshotHandler, but the class is HgSnapshotHandlerTest and its setUp builds new HgSnapshotHandler(createPartitionEngine(0)) (line 65) for the pre-existing tests. With two near-identical handler classes in the same package — only SnapshotHandler is wired into production, at PartitionEngine.java:176 — mixing them in one file makes it hard to tell which one any given test covers.

Requested change: assert positively that the non-busy path proceeds (for example verify(mockBusinessHandler).saveSnapshot(anyString(), eq(""), eq(0))) rather than swallowing the exception, and move the four SnapshotHandler tests into their own SnapshotHandlerTest.

[bitflicker64]

🧹 The documented invocation points at a path that does not exist.

Both usage lines say bash docker/hbase/test/test-snapshot-corruption.sh, but the script is committed at docker/test/test-snapshot-corruption.sh (confirmed against the head tree at 8e121d4d). The code below resolves COMPOSE_FILE correctly from $SCRIPT_DIR, so only the documentation is wrong — but it is the first thing a reader copies, and the previous review round already flagged the stale docker/hbase/test prefix.

Requested change: update both lines to bash docker/test/test-snapshot-corruption.sh (and ... --fixed).

[bitflicker64]

🧹 This line can abort the whole run with no diagnostic.

The script sets set -euo pipefail (line 35). If $TARGET has no five-digit path segment, grep -oE '/[0-9]{5}/' exits 1, pipefail surfaces that as the pipeline status even though head and tr succeed, and the failing command substitution in a simple assignment trips set -e — the script exits silently right after reporting Target: partition ... , with the cluster left running and one snapshot already about to be corrupted.

Requested change: tolerate the miss and fail loudly instead, e.g.

PARTITION_ID=$(echo "$TARGET" | grep -oE '/[0-9]{5}/' | head -1 | tr -d '/' || true)
[ -n "$PARTITION_ID" ] || fail "Could not derive partition id from snapshot path: $TARGET"