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 43.33%. Comparing base (98477f0) to head (98fdaac).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3164      +/-   ##
============================================
+ Coverage     37.78%   43.33%   +5.54%     
+ Complexity     6556     6368     -188     
============================================
  Files           800      620     -180     
  Lines         68929    58155   -10774     
  Branches       9157     8002    -1155     
============================================
- Hits          26046    25201     -845     
+ Misses        39824    29969    -9855     
+ Partials       3059     2985      -74     

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

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

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

[bitflicker64]

Blocking: yes. Summary: On compaction-busy the new throw reaches jraft as RaftError.EIO, the one code SnapshotExecutorImpl escalates to reportError, which ends in PartitionEngine.restartRaftNode(); and the Run core test step this PR adds is red on this head. Two blocking comments (SnapshotHandler.java, CoreSuiteTest.java) and three nits (SnapshotHandler.java load check, RaftRocksdbOptions.java, HgStoreException.java). Evidence: failing check store at 98fdaac, https://github.com/apache/hugegraph/actions/runs/33957642990/job/101289530213 ; jraft 1.3.13 sources for the snapshot error path; line references in each comment.

[bitflicker64]

‼️ This does more than refuse one snapshot: it restarts the partition's raft node.

PartitionStateMachine.onSnapshotSave (line 199-201, unchanged by this PR) turns the exception into done.run(new Status(RaftError.EIO, ...)), and EIO is the one code jraft escalates. In jraft-core-1.3.13 SnapshotExecutorImpl.onSnapshotSaveDone, any other non-zero result only reaches writer.setError(...), but:

if (ret == RaftError.EIO.getNumber()) {
    reportError(RaftError.EIO.getNumber(), "Fail to save snapshot.");
}

reportError goes to FSMCallerImpl.setError, which calls fsm.onError and node.onError. On this side PartitionStateMachine.java:129-134 forwards to the state listeners and PartitionEngine.java:709-712 implements onError as restartRaftNode(), i.e. shutdown(); init(this.options);. NodeImpl.onError also steps down and sets State.STATE_ERROR.

So a scheduled snapshot landing inside a compactRange() now costs a teardown and re-init of that partition's raft node: leader step-down, re-election, log storage close/reopen, replay. Shipped defaults make that overlap realistic, snapshotInterval: 1800 in hg-store-node/src/main/resources/application.yml and hg-store-dist/src/assembly/static/conf/application.yml, 300 as the @Value fallback in AppConfig.java:183, against a full RocksDB range compaction on a large partition.

Requested change: report busy with a code jraft does not escalate, RaftError.EBUSY (1009) instead of EIO (1014). With any non-EIO code, onSnapshotSaveDone sets the writer error and LocalSnapshotStorage.close destroys the temp snapshot directory without reportError, so the incomplete snapshot is still refused (the #3162 fix holds) while the node keeps serving and jraft retries next interval. That needs PartitionStateMachine.onSnapshotSave to catch EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL separately from a real save failure; alternatively, wait a bounded interval for the lock here before giving up.

[bitflicker64]

⚠️ Re-enabling this suite makes the Run core test step added in this PR fail, and it is red on this head.

The store-core-test surefire execution runs **/CoreSuiteTest.java and **/BatchGraphIsolationTest.java in one fork (hugegraph-store/hg-store-test/pom.xml:245-248). With HgSnapshotHandlerTest back in the suite, CoreSuiteTest pulls in StoreEngineTestBase, whose @AfterClass shutDownEngine() calls HgStoreEngine.getInstance().shutdown(). That singleton's closing flag is never cleared, so the next class in the same JVM cannot open a session and BusinessHandlerImpl.getSession throws at BusinessHandlerImpl.java:1318.

From job 101289530213, step 16:

10:20:50.494 [INFO]  Tests run: 7 ... in org.apache.hugegraph.store.core.CoreSuiteTest
10:20:50.505 [ERROR] HgStoreException: store is closing
                       at BatchGraphIsolationTest.setup(BatchGraphIsolationTest.java:113)
[ERROR] Failed to execute goal ... maven-surefire-plugin:2.20:test (store-core-test)

This is what the deleted // TODO: uncomment it until all test can run free. was warning about.

Requested change: stop the two sharing a JVM. Move **/BatchGraphIsolationTest.java into its own surefire execution, or set <reuseForks>false</reuseForks> on store-core-test, so the engine CoreSuiteTest shuts down cannot leak into it.

[bitflicker64]

🧹 Placing this above the should_not_load early return widens it past the corruption it targets.

Line 188 returns early for a locally saved snapshot precisely because nothing is loaded from it. With the check above that return, such a snapshot missing data/ now throws, PartitionStateMachine.onSnapshotLoad (line 228-235) returns false, FSMCallerImpl.doSnapshotLoad calls setError(ESTATEMACHINE), and that reaches PartitionEngine.onError -> restartRaftNode(). Re-init reads the same on-disk snapshot, so it repeats.

The #3162 signature does not need the wider placement: the old early return in onSnapshotSave happened before markShouldNotLoad, so a snapshot corrupted that way has no flag, shouldNotLoad is false, and the check still fires from below the return.

Requested change: move the block after the shouldNotLoad(reader) early return at lines 187-191. testOnSnapshotLoadThrowsWhenShouldNotLoadPresentButDataMissing then needs its expectation flipped to "skips", which is the behaviour before this PR.

[bitflicker64]

🧹 The guard releases the monitor before the registration it is guarding.

raftRocksdbConfigRegistered flips inside synchronized (RaftRocksdbOptions.class) at line 66, but every StorageOptionsFactory.register* call sits outside the block (lines 68-103, the calls at 81, 90 and 102). Because the flag is set before the work, a throw anywhere in the body, new LRUCache(SizeUnit.GB) on line 68 for instance, permanently suppresses every later attempt in that JVM with no log line, and the process then runs on jraft defaults instead of the configured DBOptions/ColumnFamilyOptions. The same gap lets a concurrent second caller return early while the first is still registering, though today only test setups call this more than once.

Requested change: hold the monitor across the whole method body, or set raftRocksdbConfigRegistered = true only after the final registerRocksDBColumnFamilyOptions call.

[bitflicker64]

🧹 This hunk also deletes three public constants, which is unrelated to the snapshot fix.

Alongside adding EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL = 1217, the diff removes EC_RKDB_TRUNCATE_FAIL = 1212, EC_RKDB_TRANSFER_SNAPSHOT_FAIL = 1216 and EC_METRIC_FAIL = 1401. Grepping the head tree for the three names returns nothing, so nothing in-repo breaks, but they are public static final members of a type published in the hg-store-core artifact: anything downstream that recompiles against the new jar stops compiling. Nothing in this PR needs the removal.

Requested change: restore the three constants and keep this hunk to the single added code. If the cleanup is wanted, it belongs in its own PR.