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 71.95122% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.45%. Comparing base (98477f0) to head (1c6bc23).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...ache/hugegraph/store/snapshot/SnapshotHandler.java	60.00%	8 Missing and 2 partials ⚠️
.../hugegraph/store/business/BusinessHandlerImpl.java	65.21%	6 Missing and 2 partials ⚠️
...he/hugegraph/store/raft/PartitionStateMachine.java	0.00%	3 Missing ⚠️
...he/hugegraph/store/options/RaftRocksdbOptions.java	93.54%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3164      +/-   ##
============================================
+ Coverage     37.78%   40.45%   +2.66%     
- Complexity     6556     7068     +512     
============================================
  Files           800      800              
  Lines         68929    69018      +89     
  Branches       9157     9178      +21     
============================================
+ Hits          26046    27921    +1875     
+ Misses        39824    37868    -1956     
- Partials       3059     3229     +170     

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

Blocking: no. Summary: The range lock closes the check-then-act race and the EBUSY mapping keeps a busy save from restarting the raft node; one regression remains: a full compaction that collides with a snapshot save is now silently dropped with no retry. Evidence: exact head 468e234 static review of BusinessHandlerImpl.java:1413-1481, SnapshotHandler.java:92-132, PartitionStateMachine.java:192-212 and every dbCompaction caller (PartitionEngine.java:1017,1263; HgStoreEngine.java:505; PartitionAPI.java:207; TTLCleaner.java:207,264); gh -R apache/hugegraph pr checks 3164 all 24 checks pass, store job runs CoreSuiteTest (7 tests) and RaftSuiteTest (6 tests).

[SebastianGruza]

The race reproduces deterministically

On a 3-VM lab (1 PD + 3 Store + 1 Server, replication 1, 25 M edges, partitions of 60–275 MB) the window is not narrow: a full compaction of a partition holding ~250 MB of unflushed data takes 7 s, and a snapshot requested 300 ms after it lands in state == doing every time. The compaction is started blocking through POST :8520/v1/compat?id=, the snapshot through GET :8520/test/snapshot. Script: cluster/race_snapshot_compaction.sh, store and jraft logs in results/issue-3162/.

The only case where the window really disappears is a freshly compacted partition: compactRange on 62 MB with nothing to do takes 0.1 s.

What master does, step by step

step	master 60c8803	PR head 1c6bc23
snapshot during the compaction	onSnapshotSave success in the log, directory snapshot_50210 with only __raft_snapshot_meta, no data/, no should_not_load; jraft immediately deletes the previous good snapshot and truncates the log prefix to 50209	snapshot save failed: compaction in progress, status EBUSY, no directory at all; the compaction finishes normally and the snapshot after it is valid
SIGKILL the store 1.3 s after the snapshot, restart without wiping	onSnapshotLoad failed → initSnapshotStorage failed → Raft 15 is restarting !!! once, then silence; the partition stays dead	same, only the exception text changes (snapshot is corrupt, data dir …)
what clients see meanwhile	g.E().count() = 24 045 646 of 25 000 000, g.V().count() = 97 185 of 100 000, HTTP 200, no error on the server side; store REST /v1/partition/15 returns 500	same
rm -rf snapshot_50210, restart	Missing logs in (0, 50209): the log prefix is gone, the partition stays dead	same
rm -rf raft/00015 (log, meta, snapshot), keep db/00015, restart	the partition comes back (100 000 vertices) but 257 001 edges are missing: HStore runs RocksDB with setDisableWAL(true), the raft log is the WAL, so the unflushed memtable of the killed process is lost together with the log

Two things this says about the PR

• The save side does exactly what is needed. With the fix no directory without data/ is created, and more importantly the previous good snapshot is not deleted and the log is not truncated. That is the fix that matters, because once an empty snapshot is committed there is no way back: jraft has removed the previous snapshot and the log prefix, and the local RocksDB has no WAL.
• The load side detects but does not rescue. The data/ validation only changes the message; the partition still does not come up and the server keeps answering 200 with partial results. That is not a complaint about the PR, rather a hint that "what to do with a snapshot written before the fix" deserves its own issue: for replication 1 I know no procedure that does not lose data; for replication ≥ 2 a re-install from the leader is probably the natural one.

Numbers for the 10 s discussion

onSnapshotSave on a 274 MB partition completes within the same log second (the checkpoint is hard links, CRC64 over the first and last 4 KB of each file), so the "well under a second" assumption holds. The compaction with 250 MB of unflushed data took 7 s, so with snapshotInterval 1800 a collision is rare, but with a compaction requested after a bulk load it will certainly happen; EBUSY instead of EIO matters there.

If it helps, I can repeat the same runs on the next PR head or at replication 3 (where the interesting part is what a follower installing a snapshot in that window receives).

[vaijosh]

The race reproduces deterministically

On a 3-VM lab (1 PD + 3 Store + 1 Server, replication 1, 25 M edges, partitions of 60–275 MB) the window is not narrow: a full compaction of a partition holding ~250 MB of unflushed data takes 7 s, and a snapshot requested 300 ms after it lands in state == doing every time. The compaction is started blocking through POST :8520/v1/compat?id=, the snapshot through GET :8520/test/snapshot. Script: cluster/race_snapshot_compaction.sh, store and jraft logs in results/issue-3162/.

The only case where the window really disappears is a freshly compacted partition: compactRange on 62 MB with nothing to do takes 0.1 s.

What master does, step by step

step master 60c8803 PR head 1c6bc23
snapshot during the compaction onSnapshotSave success in the log, directory snapshot_50210 with only __raft_snapshot_meta, no data/, no should_not_load; jraft immediately deletes the previous good snapshot and truncates the log prefix to 50209 snapshot save failed: compaction in progress, status EBUSY, no directory at all; the compaction finishes normally and the snapshot after it is valid
SIGKILL the store 1.3 s after the snapshot, restart without wiping onSnapshotLoad failed → initSnapshotStorage failed → Raft 15 is restarting !!! once, then silence; the partition stays dead same, only the exception text changes (snapshot is corrupt, data dir …)
what clients see meanwhile g.E().count() = 24 045 646 of 25 000 000, g.V().count() = 97 185 of 100 000, HTTP 200, no error on the server side; store REST /v1/partition/15 returns 500 same
rm -rf snapshot_50210, restart Missing logs in (0, 50209): the log prefix is gone, the partition stays dead same
rm -rf raft/00015 (log, meta, snapshot), keep db/00015, restart the partition comes back (100 000 vertices) but 257 001 edges are missing: HStore runs RocksDB with setDisableWAL(true), the raft log is the WAL, so the unflushed memtable of the killed process is lost together with the log

Two things this says about the PR

• The save side does exactly what is needed. With the fix no directory without data/ is created, and more importantly the previous good snapshot is not deleted and the log is not truncated. That is the fix that matters, because once an empty snapshot is committed there is no way back: jraft has removed the previous snapshot and the log prefix, and the local RocksDB has no WAL.
• The load side detects but does not rescue. The data/ validation only changes the message; the partition still does not come up and the server keeps answering 200 with partial results. That is not a complaint about the PR, rather a hint that "what to do with a snapshot written before the fix" deserves its own issue: for replication 1 I know no procedure that does not lose data; for replication ≥ 2 a re-install from the leader is probably the natural one.

Numbers for the 10 s discussion

onSnapshotSave on a 274 MB partition completes within the same log second (the checkpoint is hard links, CRC64 over the first and last 4 KB of each file), so the "well under a second" assumption holds. The compaction with 250 MB of unflushed data took 7 s, so with snapshotInterval 1800 a collision is rare, but with a compaction requested after a bulk load it will certainly happen; EBUSY instead of EIO matters there.

If it helps, I can repeat the same runs on the next PR head or at replication 3 (where the interesting part is what a follower installing a snapshot in that window receives).

Thanks, @SebastianGruza. Yes, this will really help!

[bitflicker64]

Blocking: no. Summary: The previous rounds' findings are resolved at this head. Snapshot save and compactRange() now share a per-partition ReentrantLock, a busy save reports RaftError.EBUSY instead of restarting the raft node, a compaction that meets a running save waits up to 10s before skipping with a WARN, and the data/ check sits after the should_not_load return. Evidence: exact-head diff and the incremental diff 468e2343..1c6bc23a (BusinessHandlerImpl.java:1440-1463, SnapshotHandler.java, PartitionStateMachine.java); all 24 checks on 1c6bc23a pass, and the store job log shows CoreSuiteTest (8 tests, including the skip path: Partition 4 skip dbCompaction ... after 200ms wait), BatchGraphIsolationTest (6) and RaftSuiteTest (6) passing. Optional nit: the comment at BusinessHandlerImpl.java:1446-1449 still says the next compaction pass will succeed, but the event-driven triggers are one-shot.