[robfrank]

Closes #9556

Apache Ratis 3.3.1 SegmentedRaftLog.close() takes the log's write lock and then joins the cache-eviction thread. If that thread was woken just before (by a cache miss or a segment roll), it may be parked in writeLock() inside checkAndEvictCache(), and ReentrantReadWriteLock.lock() ignores the interrupt. The close then hangs forever while holding the write lock.

RaftHAServer now calls the new RaftLogCacheEviction.stopBeforeClose(server) before each of its three Ratis server closes. The helper reflectively closes each division log's cacheEviction AwaitToRun while no lock is held, so the eviction thread can finish its pass and exit. AwaitToRun.close() is idempotent, so the close Ratis runs later has nothing left to join. This is the same reordering as the upstream fix, applied from outside. The join runs on its own daemon thread with a 10 s bound (STOP_WAIT_MS), so a write lock held elsewhere cannot hang the caller as well.

The issue's to-do items

Item	Finding
Is this fixed in a newer Ratis release?	Fixed upstream as RATIS-2719 (apache/ratis@68d21674cc, merged 2026-10-01) with the same reordering. Not released yet: 3.3.1 is the latest on Maven Central. Bump and removal of this workaround tracked in #9560.
Does the division close / in-place restart path have a timeout?	No. stop() calls raftServer.close() unbounded. The in-place restart bounds only the wait for a close already running elsewhere (awaitCloseInProgress), not its own oldServer.close(). This PR removes the deadlock #9556 reported, and the eviction stop it adds is bounded. A general bounded close needs a design decision (a timed-out close may still hold the storage lock and gRPC ports), tracked in #9561.

Completeness

Invariant: no ArcadeDB-initiated Ratis server close runs while that server's segmented log still has a live cache-eviction thread.

$ grep -n "oldServer.close();\|this.raftServer.close();\|        raftServer.close();" ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java
2548:          oldServer.close();
2612:          this.raftServer.close();
2825:        raftServer.close();
$ grep -rn "\.close();" ha-raft/src/main/java | grep -i raftserver   # no other RaftServer closes; RaftClusterManager:884 closes a RaftClient

Entry point	Status
RaftHAServer.stop() (raftServer.close())	fixed here
In-place restart, oldServer.close()	fixed here
Restart aborted by shutdown, this.raftServer.close()	fixed here
Closes Ratis starts itself (JVM-pause close, off by default since #8898; close after a failed start)	not reachable from ArcadeDB, covered only by the upstream fix, #9560
Native image: the reflective field read	fixed here (field registered in reachability-metadata.json, guarded by a test)

Reachability: the three call sites are on the live stop and restart paths. stopBeforeCloseStopsTheEvictionThreadOfARunningServer drives the helper against a real running single-node RaftServer.

Known gaps

• HA: bump Apache Ratis to the release carrying RATIS-2719 and drop the #9556 close workaround #9560: Ratis-initiated closes. These go away with the Ratis bump, and so does this workaround.
• HA: RaftHAServer.stop() and the in-place restart close the Ratis server with no timeout #9561: the Ratis server close itself is still unbounded.

Test plan

• Issue9556RaftLogCloseEvictionDeadlockTest (new, 6 tests):
  • ratisCloseHangsWhenTheEvictionThreadWaitsForTheWriteLock: control case. It reproduces the hang deterministically on 3.3.1 (the closer takes the write lock, cache misses park the eviction thread on it, then close) and asserts the closer is stuck in AwaitToRun.close. It goes red when a Ratis upgrade carries RATIS-2719, and that is the signal to delete the workaround.
  • closeCompletesWhenTheEvictionThreadWasStoppedFirst: the same sequence after stop(), and close returns.
  • stopGivesUpWhileAnotherThreadHoldsTheWriteLock: the bounded wait.
  • stopBeforeCloseStopsTheEvictionThreadOfARunningServer: real RaftServer, the entry point RaftHAServer calls.
  • nativeImageRegistersTheEvictionField, stopIgnoresALogThatIsNotSegmented.
• Proved the tests can fail: with stop() sabotaged into a no-op, 3 of the 4 fix-side tests fail.
• Issue9549RaftLogCacheBoundTest passes.
• Full ha-raft unit suite: 2906 tests, 0 failures.
• Restart ITs: Issue4839RecoveryRewiresStateMachineIT and Issue5271ClosedDivisionRecoveryIT pass. Issue8900ClosedServerInPlaceRestartIT fails in teardown (DatabaseAreNotIdentical Types 8 <> 7): 1 of 3 methods on this branch, 2 of 3 with main's RaftHAServer. It already fails on main and is filed as Issue8900ClosedServerInPlaceRestartIT fails in teardown on main: DatabaseAreNotIdentical Types 8 <> 7 #9562.

Adversarial pass

Not run: the subagent tool was not available in this session. In its place, a self-review of the corner cases:

• Another thread is already inside a deadlocked Ratis close (restart path, beforeClose == CLOSING): the stopper's join also blocks. The caller gives up after 10 s, logs at WARNING and proceeds exactly as before, leaving one parked daemon thread behind. This does not make the existing behaviour worse.
• A division that cannot be read (already closed or removed) is skipped. Ratis will not close a log for it either.
• Eviction stops a few milliseconds before the close, so the cache is not trimmed in that window. Nothing appends during a close.

Review cycles

Cycle	Head	Outcome
1	a8b40db	claude-review: "No blocking concerns. LGTM once CI is green." 5 optional suggestions, none applied (see below)

Deferred / skipped review items

1. "a short comment saying it is deliberately not pooled (rare path) would help" - skipped as optional. One short-lived thread per division, on server close only.
2. "On timeout, also log the group id" - already logged. The AwaitToRun name is <peer>@group-<id>-cacheEviction-AwaitToRun (built from RaftGroupMemberId), so the WARNING already names the division.
3. "System.getProperty("basedir", ".") would be more robust" - skipped to stay consistent with NativeImageReflectionMetadataTest, which resolves the same file the same way. Surefire runs from the module directory.
4. "consider @Tag("slow")" - not needed: the class runs in about 8 s.
5. Ratis thread naming - acknowledged by the reviewer as failing loudly. No action.

Final state: clean-approval

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability during high-availability server restarts and shutdowns. Server closure is less likely to hang while background log cleanup is in progress, helping the server complete its lifecycle transitions more predictably. This reduces the risk of prolonged or stuck shutdowns without changing the available server features or user-facing controls.

[mergify]

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
Security	1 high

View in Codacy

🟢 Metrics 16 complexity

Metric	Results
Complexity	16

View in Codacy

🟢 Coverage 65.96% diff coverage · -5.63% coverage variation

Metric	Results
Coverage variation	✅ -5.63% coverage variation
Diff coverage	✅ 65.96% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (169e31e)	221833	189852	85.58%
Head commit (a8b40db)	254497 (+32664)	203482 (+13630)	79.95% (-5.63%)

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 (#9565)	47	31	65.96%

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: c6ed8a3f-f5d8-4e80-9d3e-f490c4f82f04

---

📥 Commits

Reviewing files that changed from the base of the PR and between 169e31e and a8b40db.

---

📒 Files selected for processing (4)

• ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftLogCacheEviction.java
• ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue9556RaftLogCloseEvictionDeadlockTest.java
• native/src/main/resources/META-INF/native-image/com.arcadedb/arcadedb-native/reachability-metadata.json

---

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

---

📝 Walkthrough

Walkthrough

The change adds a bounded cache-eviction stop utility and calls it before Ratis server close during restart and shutdown. Tests cover the close behavior, and native-image metadata registers the reflected field.

Changes

Raft server close handling

Layer / File(s)	Summary
Bounded cache-eviction stop ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftLogCacheEviction.java	The utility reads divisions and logs, locates segmented-log eviction tasks through reflection, and closes each task on a daemon thread. It waits up to the configured timeout and logs when reflection is unavailable or a stopper remains alive.
Close-path integration and validation ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java, ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue9556RaftLogCloseEvictionDeadlockTest.java, native/src/main/resources/META-INF/native-image/com.arcadedb/arcadedb-native/reachability-metadata.json	Restart and shutdown paths stop cache eviction before closing the Ratis server. Tests cover the close deadlock, bounded stopping, server close, null logs, and reflection metadata. Native-image metadata registers the cacheEviction field.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant RaftHAServer
  participant RaftLogCacheEviction
  participant RatisDivisions
  participant EvictionThread
  participant RatisServer
  RaftHAServer->>RaftLogCacheEviction: stopBeforeClose(server)
  RaftLogCacheEviction->>RatisDivisions: Read divisions and logs
  RaftLogCacheEviction->>EvictionThread: Close cache eviction and wait with timeout
  RaftHAServer->>RatisServer: Close server

Loading

---

Merge Risk: ⚪ Minimal · up to a8b40

Before closing the Raft server during restart or shutdown, the server now stops the log cache-eviction thread, waiting at most 10 seconds. This avoids a known shutdown deadlock without adding a new blocking path, and no concrete problem was found in the change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		The PR addresses the coding requirement in directly linked issue #9556. RaftLogCacheEviction.stopBeforeClose(server) stops each segmented log's cacheEviction thread before the three RaftHAServer…
Out of Scope Changes check		The changed production code, tests, and native-image metadata support issue #9556. The three modified close paths are the ArcadeDB server stop and restart paths identified by the issue. Ratis-initiate…
Title check		The title clearly identifies the main fix: stopping the Raft log cache-eviction thread before closing the Ratis server. It is concise and directly related to the changes.
Description check		The description is detailed and covers the change, motivation, related issues, known gaps, implementation scope, tests, and validation results. It does not use the template headings exactly, and it do…

---

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. (1 skipped: 1 unsupported.)

---

---

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

Well-scoped, well-documented workaround. Closing the eviction AwaitToRun before Ratis takes the write lock matches the upstream RATIS-2719 fix, and removal is tracked in #9560.

Strengths

• stopBeforeClose never throws and degrades to the old behaviour if the field is missing (one-time WARNING).
• The join is bounded on a daemon thread, so a lock held elsewhere cannot hang the caller.
• All three RaftHAServer close sites are covered, and the native-image reflection entry is added and test-guarded.
• The control test proves the hang on the Ratis build in use, so the workaround cannot silently outlive its purpose.
• No raw wall-clock assertions, and ports come from allocateFreePorts.

Minor suggestions

1. stop spawns a thread per division on each close. Negligible, but a short comment saying it is deliberately not pooled (rare path) would help.
2. On timeout, also log the group id: eviction.toString() may not identify the division.
3. nativeImageRegistersTheEvictionField uses a cwd-relative path (../native/...). System.getProperty("basedir", ".") would be more robust outside Maven.
4. The class writes 10 x 1MB segments and has polling loops up to 30s. If it is slow in CI, consider @Tag("slow").
5. evictionThreads depends on Ratis thread naming. It fails loudly, so that is acceptable.

Risk

• Security: reflection is limited to one private Ratis field, with no user input.
• Performance: cost is on shutdown and restart paths only.
• Correctness: AwaitToRun.close() is idempotent, so the later Ratis close has nothing to join.

No blocking concerns. LGTM once CI is green.

[codecov]

Codecov Report

❌ Patch coverage is 59.57447% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.36%. Comparing base (169e31e) to head (a8b40db).

Files with missing lines	Patch %	Lines
.../arcadedb/server/ha/raft/RaftLogCacheEviction.java	61.36%	14 Missing and 3 partials ⚠️
...java/com/arcadedb/server/ha/raft/RaftHAServer.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9565   +/-   ##
=========================================
  Coverage     77.36%   77.36%           
- Complexity     4165     4173    +8     
=========================================
  Files          2157     2158    +1     
  Lines        221833   221880   +47     
  Branches      47521    47526    +5     
=========================================
+ Hits         171611   171649   +38     
- Misses        32020    32021    +1     
- Partials      18202    18210    +8     

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