fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) by vaijosh · Pull Request #3164 · apache/hugegraph

vaijosh · 2026-08-18T10:37:08Z

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?

Documentation Status

Doc - TODO
Doc - Done
Doc - No Need

… load (apache#3162) - Throw HgStoreException in onSnapshotSave when RocksDB compaction is in progress so JRaft retries rather than committing an empty snapshot dir. - In onSnapshotLoad, fall through to the real load path when should_not_load is present but data/ is missing (JVM-killed mid-checkpoint), so JRaft can signal the error and request a fresh snapshot from the leader. - Add unit tests covering both fix paths in HgSnapshotHandlerTest. - Add docker/test/test-snapshot-corruption.sh, a deterministic Docker reproducer that confirms the bug and validates the fix (--fixed mode). Fixes apache#3162 Co-Authored-By: Claude <noreply@anthropic.com>

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.

… load (apache#3162) - Addressed Review comments

vaijosh · 2026-08-19T13:31:35Z

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

codecov · 2026-08-19T15:20:42Z

Codecov Report

❌ Patch coverage is 63.29114% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.39%. Comparing base (98477f0) to head (468e234).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
.../hugegraph/store/business/BusinessHandlerImpl.java	30.00%	13 Missing and 1 partial ⚠️
...ache/hugegraph/store/snapshot/SnapshotHandler.java	60.00%	8 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.39%   +2.61%     
- Complexity     6556     7059     +503     
============================================
  Files           800      800              
  Lines         68929    69015      +86     
  Branches       9157     9178      +21     
============================================
+ Hits          26046    27880    +1834     
+ Misses        39824    37909    -1915     
- Partials       3059     3226     +167

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

… load (apache#3162) - Added comment in test-snapshot-corruption.sh to make clear that its just load-path reproducer for the HStore snapshot corruption bug

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

… load (apache#3162) - Addressed review comments. Added few Unit test cases, -Removed test-snapshot-corruption.sh because scenario is already covered by UTs.

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.

…es (apache#3162) Addresses review comments: - onSnapshotSave/dbCompaction shared a non-atomic state check, letting saves race with compaction; coordinate both through a dedicated per-partition lock, checked non-blockingly on both sides - add EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL so the busy-save case has its own grep-able error code, and fix the exception text (compaction, not "skipped") and a stray non-ASCII em dash - onSnapshotLoad checks data/ before should_not_load, so a snapshot missing its flag is reported as corrupt instead of failing later with an unrelated RocksDB path error - drop the duplicate jraft/protobuf imports in HgSnapshotHandlerTest - register SnapshotHandlerTest in RaftSuiteTest and HgSnapshotHandlerTest in CoreSuiteTest, and run store-core-test in CI, so both actually execute instead of being skipped by every bound surefire profile

The pd-store-ci.yml store job gained a store-core-test profile and hg-store-core module in a prior commit, but test-check-jacoco-report.sh's hardcoded aggregation contract still asserted the old 4-profile set, breaking CI with an AssertionError on the set-equality checks.

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.

…he#3164) - report EBUSY instead of EIO when a snapshot save is skipped due to an in-progress compaction, so jRaft retries later instead of escalating to a full raft node restart (only EIO triggers that in SnapshotExecutorImpl#onSnapshotSaveDone) - check should_not_load before validating the data/ directory in onSnapshotLoad, so a locally-saved snapshot (which has no data/ by design) is skipped instead of reported as corrupt - keep the raftRocksdbConfigRegistered guard flag unset until registration actually completes, so a failure partway through can be retried instead of being silently swallowed forever - restore EC_RKDB_TRUNCATE_FAIL, EC_RKDB_TRANSFER_SNAPSHOT_FAIL, and EC_METRIC_FAIL, which were unintentionally dropped and would have broken binary compatibility for downstream consumers - stop CoreSuiteTest and BatchGraphIsolationTest from sharing a surefire fork: HgStoreEngine's `closing` flag is set by the former's teardown and never reset, so the latter failed with "store is closing" whenever both ran in the same JVM Updates HgSnapshotHandlerTest's should_not_load/data-missing case to expect a skip rather than a throw, matching the corrected check order.

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

bitflicker64 · 2026-09-11T15:42:52Z

+                            ReentrantLock rangeLock =
+                                    compactionRangeLock.computeIfAbsent(id,
+                                                                        k -> new ReentrantLock());
+                            if (!rangeLock.tryLock()) {


⚠️ A full compaction that starts while a snapshot save holds this lock is dropped, and the event-driven callers never retry it.

The comment says the next scheduled or triggered compaction will retry, but these requests are one-shot: handleCleanOp after a data clean (PartitionEngine.java:1017), destroyPartition when graphs remain (HgStoreEngine.java:505), the PD DB_COMPACTION instruction (PartitionEngine.java:1263), and the /compat REST call (PartitionAPI.java:207), which has already told the operator the compaction was submitted. The only periodic caller is the daily TTL cleaner, and it compacts only after it cleaned something. If a periodic snapshot on the same partition is between tryLockCompactionRange and unlockCompactionRange (SnapshotHandler.java:96-129: checkpoint plus the checksum pass over every file), the request leaves one INFO line. The cleaned range stays uncompacted and the post-compaction SYNC_BLANK_TASK snapshot never runs. Before this change a snapshot never cancelled a compaction.

Requested change: the snapshot side already fails fast, so this side can wait. Use rangeLock.tryLock(timeout, unit) with a bound (this thread already waits up to timeoutMillis in lock(path) above), and skip with a WARN only if the save is still running when it expires. That cannot deadlock: onSnapshotSave only calls non-blocking tryLock() and never takes pathLock. Please also fix the comment.

dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working store Store module tests Add or improve test cases labels Aug 18, 2026

github-project-automation Bot added this to HugeGraph PD-Store Tasks Aug 18, 2026

github-project-automation Bot moved this to In progress in HugeGraph PD-Store Tasks Aug 18, 2026

imbajin reviewed Aug 18, 2026

View reviewed changes

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

f2b7219

… load (apache#3162) - Addressed Review comments

imbajin reviewed Aug 19, 2026

View reviewed changes

Comment thread docker/test/test-snapshot-corruption.sh Outdated

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

8e121d4

… load (apache#3162) - Added comment in test-snapshot-corruption.sh to make clear that its just load-path reproducer for the HStore snapshot corruption bug

vaijosh requested a review from imbajin August 27, 2026 15:42

bitflicker64 reviewed Aug 29, 2026

View reviewed changes

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

46ce0c4

… load (apache#3162) - Addressed review comments. Added few Unit test cases, -Removed test-snapshot-corruption.sh because scenario is already covered by UTs.

bitflicker64 mentioned this pull request Aug 31, 2026

fix(pd): keep KvClient watches alive after reconnect failures #3157

Open

11 tasks

Merge branch 'apache:master' into SnapshotIssue

0e1c319

bitflicker64 reviewed Sep 1, 2026

View reviewed changes

imbajin reviewed Sep 1, 2026

View reviewed changes

Comment thread ...h-store/hg-store-core/src/main/java/org/apache/hugegraph/store/snapshot/SnapshotHandler.java Outdated

vaijosh added 2 commits September 5, 2026 14:03

bitflicker64 reviewed Sep 5, 2026

View reviewed changes

bitflicker64 reviewed Sep 11, 2026

View reviewed changes

Uh oh!

Conversation

vaijosh commented Aug 18, 2026

Purpose of the PR

Main Changes

Verifying these changes

Does this PR potentially affect the following parts?

Documentation Status

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaijosh commented Aug 19, 2026

Uh oh!

codecov Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

bitflicker64 Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 19, 2026 •

edited

Loading