fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) - #3164
fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162)#3164vaijosh wants to merge 8 commits into
Conversation
… 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
left a comment
imbajin
left a comment
There was a problem hiding this comment.
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
|
Thanks @imbajin for review. I have addressed the review comments. |
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
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
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
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
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
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
left a comment
imbajin
left a comment
There was a problem hiding this comment.
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
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
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.
3 participants
Purpose of the PR
__raft_snapshot_metabut nodata/#3162This 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
data/on load: Added validation during the snapshot load process to verify the presence of thedata/directory, preventing the system from loading incomplete snapshots.test-snapshot-corruption.shto deterministically reproduce the bug and validate the fix across different storage states.Verifying these changes
test-snapshot-corruption.shscript to verify the corrupted snapshot detection and prevention.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need