fix(store): guard legacy GraphID upgrades by imbajin · Pull Request #3399 · apache/hugegraph

imbajin · 2026-10-23T16:26:05Z

Before ➡️ After

Before: Legacy reserved GraphID rows can become invisible during upgrade allocation or Raft replay; replacing the JAR cannot undo persisted mappings.

After: Refuse affected storage before normal startup or snapshot activation, provide a read-only offline checker, and document verified logical recovery into an empty cluster.

Why

Preserve evidence and fail safely when row ownership cannot be proven instead of attempting automatic physical conversion.

Validation boundary

The author reports regression and isolated recovery checks, but original experiment logs/archives are unavailable. Recovery is limited to independently verified readable business graphs; ambiguous ownership, TTL/system state and rolling upgrades need separate validation. Production-scale and real-cluster legacy-snapshot rejection remain open. Paired website documentation is still required.

Source proposal: 🔗 hugegraph/hugegraph#325.

- reject legacy prefixes and invalid mappings before Store registration - validate incoming snapshots before replacing databases or advancing the index - ship a read-only checker using the existing Store runtime - document and test logical recovery through existing HugeGraph Tools

codecov · 2026-10-23T16:58:07Z

Codecov Report

❌ Patch coverage is 85.10638% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.82%. Comparing base (608cb1d) to head (827e6c1).
⚠️ Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
.../hugegraph/store/upgrade/LegacyGraphIdChecker.java	85.80%	10 Missing and 13 partials ⚠️
...pache/hugegraph/rocksdb/access/RocksDBSession.java	66.66%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3399      +/-   ##
============================================
+ Coverage     57.27%   57.82%   +0.55%     
+ Complexity     6577     3290    -3287     
============================================
  Files           776      778       +2     
  Lines         68881    72078    +3197     
  Branches       9431    10956    +1525     
============================================
+ Hits          39449    41680    +2231     
- Misses        25804    26639     +835     
- Partials       3628     3759     +131

Flag	Coverage Δ		*Carryforward flag
commons	`96.37% <ø> (ø)`
hstore	`36.08% <ø> (-0.01%)`	⬇️
pd	`55.80% <ø> (+3.64%)`	⬆️
server_hbase	`42.02% <ø> (-0.16%)`	⬇️	Carriedforward from 2753df9
server_memory	`56.02% <ø> (+0.03%)`	⬆️	Carriedforward from 2753df9
server_rocksdb	`42.76% <ø> (+0.10%)`	⬆️
store	`36.01% <85.10%> (+2.86%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

☔ 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: yes. Summary: The offline checker cannot inspect a supported Topling deployment with its effective configuration because its launcher omits the selected runtime. Evidence: Compared hugegraph-store-check.sh with start-hugegraph-store.sh and traced GraphIdCheckCommand.run into RocksDBRuntime.verify; validation was static, with bash -n passing for the new launcher.

imbajin · 2026-10-23T17:13:26Z

+cd -- "$TOP"
+exec "$JAVA" "${MODULE_OPTIONS[@]}" \
+    -Dloader.main=org.apache.hugegraph.store.node.GraphIdCheckCommand \
+    -cp "${JARS[0]}" org.springframework.boot.loader.PropertiesLauncher "$@"


⚠️ Important: Preserve the selected RocksDB provider in the checker launcher. With the supported rocksdb.provider=topling configuration and a prepared Topling runtime, this command still loads only the bundled standard RocksDB classes: unlike start-hugegraph-store.sh, it neither sources preload-topling.sh nor prepends TOPLING_RUNTIME_CLASSPATH. GraphIdCheckCommand.run() then calls RocksDBRuntime.verify(), which rejects that provider/runtime mismatch before inspecting any database, so the documented offline pre-upgrade check cannot run for these deployments. Please reuse the existing verified runtime selection and prepend its selected JAR to the PropertiesLauncher classpath, with launcher coverage for both providers.

bitflicker64

Blocking: yes. Summary: The GraphID guard itself reads correctly and its 25 new tests pass when run directly, but this head breaks two CI jobs on its own: the Store Docker images no longer build because the packaged guide is excluded from the Docker context, and the new exit-on-startup-failure path fails the Store launcher test, which also stops CI from ever running the new test classes. The earlier Topling comment on hugegraph-store-check.sh line 41 still applies at f1a5030; the launcher is unchanged. Evidence: read the full diff plus RocksDBSession, GraphIdManager, InnerKeyCreator, SnapshotHandler, PartitionManager path layout and the dist scripts; compared the failing docker / docker-build and pd_store / store logs with passing runs of the same jobs on other PRs at this base; built the Store dist from this head and started it with no PD (exit 1 after PD unreachable); ran GraphIdSnapshotGuardTest, GraphIdSnapshotLevelsTest, LegacyGraphIdCheckerTest and GraphIdCheckCommandTest with -P store-core-test (25 run, 0 failures).

bitflicker64 · 2026-10-24T20:30:52Z


    <files>
+        <file>
+            <source>${project.basedir}/../../docs/graphid-upgrade.md</source>


Important: This entry breaks every Docker build that packages the Store. The root .dockerignore excludes **/*.md and docs/ (lines 60-61), so docs/graphid-upgrade.md is missing from the build context and hg-store-dist fails with Failed to create assembly: Error adding file to archive: /pkg/hugegraph-store/hg-store-dist/./../../docs/graphid-upgrade.md. That is the docker / docker-build failure on this head (target server-hstore). The same job passes on other PRs at this base, and hugegraph-store/Dockerfile uses the same COPY . . context, so the published Store image cannot be built either. Please re-include the file in .dockerignore after those two patterns (for example !docs/graphid-upgrade.md) or keep the guide out of the assembly, then confirm docker / docker-build passes.

bitflicker64 · 2026-10-24T20:30:52Z

+            LoggerFactory.getLogger(StoreNodeApplication.class)
+                         .error("HugeGraph Store startup failed; exiting with status 1", e);
+            // An incomplete Spring bootstrap can leave non-daemon native/Raft executors alive.
+            System.exit(1);


Important: This exit fires for every startup failure, not only the GraphID preflight. A Store started without a reachable PD used to leave the JVM running; now it exits with status 1, and test-start-hugegraph-store.sh still expects the JVM to stay up. That makes pd_store / store fail on this head: Test 2 reports script exited early — foreground mode is not blocking and no running JVM available to verify explicit -g g1, Test 3 reports Java process not found, and Tests 3 and 4 see exit 1 instead of 137 and 143. The same job passes on other PRs at this base. I built the dist from this head and started it with no PD: hgStoreNodeService init throws RuntimeException: PDException: PD unreachable, pd.peers=localhost:8686, Spring logs Application run failed, and this catch exits 1 within seconds. Because that launcher step fails, the later Run core test step (-P store-core-test, which holds the four new GraphID test classes) and the lifecycle tests never run in CI. If failing fast without PD is the intended behaviour, update Tests 2-4 to run against a reachable or fake PD so they still have a live JVM to check. Otherwise, limit the exit to the GraphID preflight failure. Either way, pd_store / store needs to pass.

…review/server325-20261025

…erver325-20261025

bitflicker64

Blocking: no. Score: 9/10. Summary: The Store startup guard, incoming-snapshot guard, offline checker and its launcher read correctly, and the three earlier findings (Topling runtime in hugegraph-store-check.sh, .dockerignore for the packaged guide, exit only on the GraphID preflight) are fixed at this head. docker / docker-build, pd_store / store and the four new GraphID test classes pass on this head.

bitflicker64

Blocking: yes. Summary: Startup now locks every discovered DB version, and DatabaseLocks.add fails on a healthy DB that has CURRENT but no LOCK, which is the state loadSnapshot leaves until the partition reopens. The snapshot guard, offline checker, launcher and the earlier review fixes read correctly, and CI passes at this head.

bitflicker64 · 2026-10-25T18:45:03Z

+            }
+            for (Path database : databases) {
+                requireCurrent(database);
+                locks.add(database);


Important: loadSnapshot moves a checkpoint (no LOCK file) into db/<partition>_<v1>, and only a later read-write open creates LOCK, so a Store stopped or crashed in between now fails startup here with a bare NoSuchFileException from toRealPath() on healthy data. In DatabaseLocks.add, skip locking when LOCK is absent, since no RocksDB writer can hold it, and add a test that starts against a checkpoint-made DB directory.

bitflicker64 · 2026-10-28T11:03:16Z

The earlier review comments below still apply at 989d532; this head does not fix them.

Important: loadSnapshot moves a checkpoint (no LOCK file) into db/<partition>_<v1>, and only a later read-write open creates LOCK, so a Store stopped or crashed in between now fails startup here with a bare NoSuchFileException from toRealPath() on healthy data. In DatabaseLocks.add, skip locking when LOCK is absent, since no RocksDB writer can hold it, and add a test that starts against a checkpoint-made DB directory. (fix(store): guard legacy GraphID upgrades #3399 (comment))

bitflicker64 · 2026-10-28T12:06:29Z

Review summary

Prior assessment: startup compatibility issue unresolved; current-head full re-review pending.

Full review covered a8fac1bc..989d5320. The PR is now at 9c1a05bf; the linked locations were narrowly rechecked, without repeating the full review. Previously reported score: 7/10 at 989d5320, retained as historical, not recomputed as a reviewer mean by this formatting update.

Findings

Important: healthy checkpoint directories without LOCK can refuse startup (existing discussion).
Minor: trailing-comma data paths now refuse startup.
Uncertain / Minor: a zero-inspection check reports success. The behavior is verified; whether it should fail remains a contract question.

Notes

Uncertain, unscored: startup cost at large partition counts.
Unscored: Bash requirement in the test fixture.

Previously verified fixes: Docker build context and startup-exit scope.

Checks/limits: prior reviewers reported targeted GraphID, CoreSuite and lifecycle checks passing; no multi-node upgrade/recovery or Topling runtime validation. Incomplete-DB refusal is documented; companion docs linkage needs confirmation.

imbajin

Inline details for the existing review summary. Items marked Uncertain are retained questions, not confirmed defects.

imbajin · 2026-10-28T12:54:21Z

+            RocksDBRuntime.verify(String.valueOf(options.getOrDefault("rocksdb.provider", "rocksdb")));
+            LegacyGraphIdChecker.checkOffline(data, LegacyGraphIdChecker.configuration(options));
+            System.out.println("No legacy GraphID prefix or invalid GraphID mapping found in persisted " +
+                               "databases and latest snapshots: " + data);


Uncertain / Minor: report zero inspection coverage explicitly. An existing empty directory, or an existing parent directory supplied by mistake, passes checkOffline() without inspecting any database or snapshot and then reaches this success message/exit 0. The explicit-empty-root test currently expects that result, so I cannot establish that accepting an empty Store is unintended. For an upgrade preflight, however, zero coverage is indistinguishable from a populated Store checked clean. Could the command print the database/snapshot counts and distinguish a zero-inspection result (or document that it is intentional)? The behavior is verified; whether zero coverage should fail remains an open contract question.

imbajin · 2026-10-28T12:54:21Z

+    private static void inspect(String dataPaths, HugeConfig config, boolean offline)
+            throws IOException, RocksDBException {
+        List<Path> roots = new ArrayList<>();
+        for (String value : dataPaths.split(",", -1)) {


Minor: preserve the existing app.data-path delimiter semantics. For a previously accepted value such as /a,/b,, split(",", -1) produces a final empty item and this branch throws Empty Store data path. checkStore() wraps it as StartupPreflightException, so an otherwise healthy Store now refuses startup. The existing StoreMetadata constructor and PartitionManager still use split(","), which discards trailing empty items. Please preserve their trailing-delimiter behavior, or explicitly document/migrate this stricter startup requirement, and add a regression test for a trailing comma.

imbajin · 2026-10-28T12:54:21Z

+            for (Path database : databases) {
+                requireCurrent(database);
+                locks.add(database);
+                checkDatabase(database, config);


Uncertain / unscored: startup cost at large partition counts. This startup preflight opens each discovered database and snapshot for inspection. The code establishes that work is added to every boot, but no representative large-Store startup measurement was reported, so I cannot conclude that the cost is material. A measurement across realistic partition/version/snapshot counts would clarify the operational trade-off; this is not a confirmed performance defect or an additional scored finding.

imbajin · 2026-10-28T12:54:21Z

+        expected="$TOP/topling/rocksdbjni.jar:$expected"
+    fi
+    run_checker "$provider" || { cat "$TEST_ROOT/output"; exit 1; }
+    mapfile -d '' -t args < "$TEST_ROOT/capture.args"


Note / unscored: this test fixture needs a newer Bash. mapfile is unavailable in the Bash 3.2 bundled with macOS, so this fixture cannot run there as-is. Linux CI and the shipped checker launcher are unaffected. Please document the fixture's Bash requirement or use a portable reader if local macOS execution is intended.

- Reject dangling metadata paths before inspecting Store databases. - Extend native regressions for stored locations, missing CURRENT/LOCK and fresh roots. - Run the checker launcher regression in the Store CI job. - Clarify incomplete metadata recovery in the upgrade guide.

bitflicker64 · 2026-10-28T17:45:33Z

The earlier review comments below still apply at 827e6c1; this head does not fix them.

Important: loadSnapshot moves a checkpoint (no LOCK file) into db/<partition>_<v1>, and only a later read-write open creates LOCK, so a Store stopped or crashed in between now fails startup here with a bare NoSuchFileException from toRealPath() on healthy data. In DatabaseLocks.add, skip locking when LOCK is absent, since no RocksDB writer can hold it, and add a test that starts against a checkpoint-made DB directory. (fix(store): guard legacy GraphID upgrades #3399 (comment))

MrJs133 previously approved these changes Oct 23, 2026

View reviewed changes

imbajin mentioned this pull request Oct 23, 2026

fix(store): guard legacy GraphID upgrades hugegraph/hugegraph#325

Open

4 of 10 tasks

imbajin commented Oct 23, 2026

View reviewed changes

fix(store): fail GraphID check without explicit mode

e9a4392

imbajin dismissed MrJs133’s stale review via e9a4392 October 23, 2026 17:39

imbajin and others added 2 commits October 24, 2026 01:39

test(store): require explicit GraphID check mode

0659e28

docs(store): remove one-off GraphID validation receipts

f1a5030

imbajin mentioned this pull request Oct 24, 2026

[1.8.0] Release TODOs and cross-repository integration checklist #3242

Open

bitflicker64 reviewed Oct 24, 2026

View reviewed changes

orion added 2 commits October 25, 2026 06:26

Merge remote-tracking branch 'refs/remotes/organization/master' into …

2e85419

…review/server325-20261025

fix(store): preserve runtime and startup boundaries in upgrade checks

59f41ba

MrJs133 previously approved these changes Oct 25, 2026

View reviewed changes

imbajin mentioned this pull request Oct 25, 2026

docs: explain safe GraphID upgrade and migration hugegraph/hugegraph-doc#47

Open

docs(store): explain recovery from incomplete database remnants

9bc3296

imbajin dismissed MrJs133’s stale review via 9bc3296 October 25, 2026 07:40

Merge commit 'a8fac1bc88a67d6cf85ca40c9885d7daadaaa0f0' into review/s…

705c0b0

…erver325-20261025

imbajin mentioned this pull request Oct 25, 2026

fix(store): preflight legacy graph data before registration hugegraph/hugegraph#301

Closed

3 of 11 tasks

MrJs133 previously approved these changes Oct 25, 2026

View reviewed changes

bitflicker64 approved these changes Oct 25, 2026

View reviewed changes

fix(store): lock startup GraphID preflight databases

ff7df0e

imbajin dismissed MrJs133’s stale review via ff7df0e October 25, 2026 17:07

fix(store): preserve incomplete database diagnostics

db3f643

bitflicker64 reviewed Oct 25, 2026

View reviewed changes

MrJs133 previously approved these changes Oct 28, 2026

View reviewed changes

docs(store): remove review coordination links

989d532

imbajin dismissed MrJs133’s stale review via 989d532 October 28, 2026 07:37

MrJs133 previously approved these changes Oct 28, 2026

View reviewed changes

fix(store): reject incomplete metadata preflight

9c1a05b

imbajin dismissed MrJs133’s stale review via 9c1a05b October 28, 2026 12:10

imbajin commented Oct 28, 2026

View reviewed changes

imbajin added 2 commits October 28, 2026 22:45

Merge branch 'master' into fix/graphid-upgrade-guard

2753df9

Conversation

imbajin commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before ➡️ After

Why

Validation boundary

Uh oh!

codecov Bot commented Oct 23, 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!

Choose a reason for hiding this comment

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitflicker64 commented Oct 28, 2026

Uh oh!

bitflicker64 commented Oct 28, 2026 • edited by imbajin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review summary

Findings

Notes

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitflicker64 commented Oct 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imbajin commented Oct 23, 2026 •

edited

Loading

codecov Bot commented Oct 23, 2026 •

edited

Loading

bitflicker64 commented Oct 28, 2026 •

edited by imbajin

Loading