fix: WAL version gap on follower after HA cluster restart

[arcadedb-support]

Fix for a customer-reported issue: WAL version gap on follower after HA cluster restart.

I traced the root cause, reproduced it with a test, and verified the fix. Internal tracking: https://github.com/ArcadeData/arcadedb-operations/issues/438

Please review before merging.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

View in Codacy

🟢 Coverage 100.00% diff coverage · -8.13% coverage variation

Metric	Results
Coverage variation	✅ -8.13% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (9313170)	129758	96976	74.74%
Head commit (398f997)	161572 (+31814)	107613 (+10637)	66.60% (-8.13%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4609)	13	13	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review - PR #4609: WAL version gap on follower after HA cluster restart

Overview

This PR fixes a bug where ArcadeGraph.close() was unconditionally calling database.close() on server-managed databases. Since ServerDatabase.close() throws UnsupportedOperationException by design, this caused a WARN log flood and a race condition during shutdown where the Gremlin stop thread could interact with the WAL after the HA state machine had stopped. The fix is clean and well-reasoned.

---

What Changed

File	Change
ArcadeGraph.java	Adds sharedDatabase flag; new openShared() factory; close() now rolls back (not commits+closes) for shared databases
ArcadeGraphManager.java	Switches from ArcadeGraph.open() to ArcadeGraph.openShared()
ArcadeGraphSharedDatabaseIT.java	New integration test covering the two key scenarios
LocalGremlinFactoryIT.java	Fixes a silent no-op in @AfterAll cleanup + corrects the database path

---

Positive Observations

• Root cause is well understood: The commit message, Javadoc on openShared(), and test class comment all explain the exact failure mode clearly. Future maintainers will understand why this flag exists.
• Minimal blast radius: The change is confined to the Gremlin layer and does not touch HA or storage internals.
• Factory method pattern is correct: Adding openShared() as a distinct factory method rather than leaking the boolean parameter into the public API is good design.
• Rollback instead of commit on shared close: Choosing rollback over commit when closing a shared graph is the safe default - any in-flight transaction on a server graph should have been committed by the request lifecycle already, and silently committing in close() would be surprising and potentially unsafe.
• LocalGremlinFactoryIT fix is important: The original stream().map() without a terminal operation was a silent no-op - the database drop never actually executed, leaving stale state between test runs. The fix is correct.

---

Concerns and Suggestions

1. Behavior change in the non-shared path (low risk, worth noting)

Before this PR, ArcadeGraph.close() called database.commit() if a transaction was active, then database.close(). This means any uncommitted work was silently committed on close. This is the existing (unchanged) behavior for the non-shared path. It may be worth a comment acknowledging that this silent-commit behavior is intentional for the owned-database case, so a future reader doesn't unify the two paths and accidentally remove it.

2. Test uses BasicDatabase but getServerDatabase returns Database

In ArcadeGraphSharedDatabaseIT:

final BasicDatabase serverDb = getServerDatabase(0, getDatabaseName());

getServerDatabase() returns Database (which extends BasicDatabase), so this compiles and works. However, isOpen() is declared on Database, not BasicDatabase. If BasicDatabase doesn't expose isOpen(), this would fail to compile. Worth double-checking that isOpen() is accessible from the BasicDatabase reference used in the test (the assert assertThat(serverDb.isOpen()).isTrue() depends on it).

3. No test for the actual error path (pre-fix behavior)

The new tests verify the fixed behavior but do not verify that the old path (ArcadeGraph.open(serverDb)) threw or caused issues. Consider adding a negative test that calls ArcadeGraph.open(serverDb).close() and asserts that it does close the database - making the contract difference explicit. This would act as a canary if someone accidentally reverts ArcadeGraphManager to use open() again.

4. ArcadeGraphSharedDatabaseIT extends BaseGraphServerTest - check server startup cost

BaseGraphServerTest starts a full server cluster. If this test only needs a single server (and it does - it only uses getServerDatabase(0, ...)), consider whether there's a lighter base class available. That said, if BaseGraphServerTest already handles single-node setup efficiently, this is fine.

5. LocalGremlinFactoryIT database path change

The path changes from "local-database-factory" to "./target/databases/local-database-factory". This is fine as long as the ArcadeGraphFactory.withLocal(DATABASE_NAME) call creates the database at a path consistent with this. Since both beginTest and endTest now use the same constant, they are consistent. The ./target/databases/ prefix follows the Maven convention for test output, which is correct.

---

Minor Style Notes

• The inline comment // Database lifecycle is managed externally; do not close it. in close() is useful and appropriate per the project's guideline (explaining a non-obvious constraint).
• The Javadoc on openShared() is clear and follows the existing style.
• No System.out debug statements were introduced.
• Test assertions correctly use assertThat(...).as(...) for descriptive failure messages - good practice.

---

Summary

The fix is correct, well-tested, and well-documented. The LocalGremlinFactoryIT cleanup fix is a welcome bonus. The main suggestion is to verify that BasicDatabase.isOpen() is accessible (concern #2) and optionally add a negative test (concern #3) to guard against regression. Approving pending confirmation on point #2.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.93%. Comparing base (9313170) to head (398f997).
⚠️ Report is 50 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4609      +/-   ##
============================================
- Coverage     65.62%   64.93%   -0.69%     
- Complexity        0      526     +526     
============================================
  Files          1661     1661              
  Lines        129758   129768      +10     
  Branches      27826    27828       +2     
============================================
- Hits          85148    84262     -886     
- Misses        32813    33785     +972     
+ Partials      11797    11721      -76     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.