[robfrank]

Closes #9553

Summary

When every voter of the cluster quarantined the same database (the #9547 soak: database chaos on all three nodes in the same second), nothing said so. Each node logged its own SEVERE line and showed its own local-resync-in-progress alert, whose advice (wait, or force a resync) can never work in this state. The followers kept retrying a resync against a leader that answers 503 for a quarantined database. The one override that lifts a quarantine without a resync, accept-diverged (#9449), was refused with 409 because these nodes are not sole voters.

Each node now publishes the databases it holds quarantined on the capability poll that every node already runs against every peer (POST /api/v1/cluster/capabilities, new optional quarantined array, sent only when non-empty). With that:

• Detect: RaftHAServer.getDatabasesQuarantinedOnEveryVoter() lists the databases this node holds quarantined that every other voter, in its last fresh answer, also reports quarantined. It is computed from memory only, so it is cheap enough for every status poll.
• Alert: GET /api/v1/cluster raises a new critical alert, no-healthy-copy-on-any-voter, with the database names (scoped to what the caller may see). The recommendation names the recovery: accept one copy with POST /api/v1/cluster/accept-diverged/{database}, make that node the leader, or restore from a backup.
• Make the recovery possible: accept-diverged (HTTP and the gRPC twin, both through RaftHAPlugin.acceptDivergedDatabase) is now also allowed when every voter reports the database quarantined. Before lifting, the node also asks every other voter live whether it serves the database (the copyOf bootstrap-state question, in parallel, one shared 5 s deadline), and refuses if one does or cannot be asked. The capability registry can be one poll behind, so after one node accepts, a second accept elsewhere is refused. Two accepts sent at the same moment can still both pass, and the alert says to accept on one node only.
• Stop the pointless retry (the optional point): retryUnfilledSnapshotGap leaves out databases that the leader reports holding quarantined. It checks this before taking its throttle slot, so a leader whose quarantine lifts gets retried on the next tick.

A peer that did not answer, has a stale answer, or runs a build without the field is never counted as quarantined. "Unknown" never turns into "no usable copy" here, because the alert tells the operator to force-accept a copy.

Note on the issue's premise: the issue says "the cluster view already carries each peer's diverged databases". It did not. GET /api/v1/cluster showed only the local localResync.divergedDatabases, and the capability advertisement only carried a serviceGap boolean (#8665), which does not include quarantines. This PR adds the per-peer information, using the same carrier #8665 used.

Test plan

• Issue9553AllVotersQuarantinedTest (20 tests, including the live voter check against stub peers that serve, do not serve, or cannot be dialled): wire shape and parsing (including an older peer without the field), registry freshness and staleness, every-voter decision (one healthy voter, unanswered voter, older-build voter, sole voter, healthy local node), alert content and scoping. Caller-level tests drive ClusterAlerts.scan, RaftHAPlugin.acceptDivergedDatabase(String, String) and retryUnfilledSnapshotGap through FakeRaftHAServer and a real state machine
• Mutation check: disabling each of the three changes (alert wiring, accept gate, retry filter) makes its caller test fail
• Related ha-raft unit tests (capability, quarantine, resync, alerts, hand-off, accept): 432 tests, 0 failures
• Issue9449AcceptDivergedDatabaseIT (sole voter over HTTP and the remote client): green
• PluginApiSpecTest, HardcodedTestServerPortsTest (server): green; grpc/grpcw compile

Coverage

Invariant: when every voter holds a database quarantined, the cluster reports it in one alert with a recovery that can actually be executed, and no follower keeps retrying a resync whose only source is quarantined.

Entry point	Status
Quarantine producers: handleUnexpectedApplyError (APPLY_ERROR, UNDECODABLE_LOG_ENTRY), WAL version gap, settleDivergedStateAfterInstall, quarantine restored from disk	Fixed here. All write divergedDatabases, which getQuarantinedDatabaseNames() reads, so detection does not depend on the cause
Status surface: GetClusterHandler -> ClusterAlerts.scan (all overloads go through the production one)	Fixed here
Recovery: POST /api/v1/cluster/accept-diverged/{db} and gRPC AcceptDivergedDatabase (both through RaftHAPlugin.acceptDivergedDatabase)	Fixed here, OpenAPI, proto and HAServerPlugin docs updated
Resync retry loop: retryUnfilledSnapshotGap (health tick)	Fixed here
One-shot triggerDatabaseResync at the mark	Argued: a single attempt per quarantine, not a loop. It fails with the leader's 503 and logs once
Operator POST /api/v1/cluster/resync/{db}	Argued: an explicit operator request. It should still run and report the leader's refusal
Quarantined-leader hand-off (handOffLeadershipToResync, #8483)	Argued: already bounded by QUARANTINE_HANDOFF_COOLDOWN_MS (one per node per 10 min), which #8483 designed for exactly "a database that EVERY peer quarantines". After an accept on a follower, a transfer is how leadership reaches the accepted copy, so suppressing it would get in the way of the recovery
Studio HA panel	Covered: studio-cluster.js renders alerts generically

Known gaps: None filed. Residual risk is below.

Residual risk

• Detection relies on the capability poll (5 s refresh, 20 s TTL). The registry is cleared on a leadership change, so for one round after a hand-off the alert can disappear and one follower retry can go to a quarantined leader and fail with 503, as it did before this PR.
• In a mixed-version cluster, a peer on a build without the field is never counted, so the alert and the extended override stay off until every voter runs this build. That is the safe direction.
• Two accept-diverged requests sent to two nodes at the same moment can both pass the live check, because neither node serves the database yet when the other asks. The live check narrows this to two concurrent requests but does not serialize them. Routing the accept through the leader or a Raft entry would close it, which is a larger change. The override is root only and logged with the user, and the alert says to accept on one node only.

Adversarial pass

The orchestrator could not spawn the independent follow-up subagent (no subagent tool available in this run), so this pass was a self-review against the issue text:

• "Detect that no voter holds a non-quarantined copy": a voter that is down is treated as possibly holding a good copy, so a cluster with one node unreachable and two quarantined gets no alert. This is deliberate: the alert recommends a destructive override.
• "Names the recovery: force-accept (A quarantine that already stands on a sole voter (restored from disk, or raised before the cluster shrank) still can never lift: add an operator override #9449)": before this PR that recovery returned 409 off a sole voter, so an alert alone would have recommended something impossible. Fixed here, see above.
• "Refuse to keep retrying": done for the periodic retry; the one-shot and operator paths are argued in the table.

Review cycles

Cycle	Head	Changes	claude review
1	34ebafeef9	Initial implementation	6 points. Point 1 (accept could pass on a stale registry answer) was actionable; the rest were confirmations or nits
2	be91aab100	Live confirmation with every voter before accepting off a sole voter (RaftHAServer.refuseUnlessNoOtherVoterServesNow) plus tests against stub peers; corrected the retry comment	Asked to soften the "only one copy" claim (concurrent accepts) and to document the blocking call
3	c0011646f9	Alert text says accept on ONE node only; javadoc states the concurrency limit and the bounded blocking	"No blocking issues found"

Skipped review items (with rationale)

• "Log spam in retryUnfilledSnapshotGap": the skip line goes through HALog.log(..., HALog.BASIC, ...), which prints only when arcadedb.ha.logVerbose >= 1 (default 0). It is a debug trace, and the operator-facing signal is the alert.
• "getLivePeers() may include non-voters": it returns RaftConfiguration.getCurrentPeers(), the voting members. Listeners are a separate role. isSoleVoter() (A single-node Raft cluster that loses a database directory now quarantines it and never becomes ready again #8940, The sole-voter escape added for the forceSnapshot replay was not applied to the other leader-side quarantine, so one apply error on a single-voter cluster leaves the node permanently not-ready with a Raft log that can never be checkpointed #9308) already relies on the same accessor for the same meaning. When the configuration cannot be read it falls back to the declared list, which can only keep the alert off.
• "Floor-only quarantines are excluded": deliberate. A node holding only a read floor has not quarantined the database, so it is not advertised and counts as a possible source. That keeps the override closed, which is the safe direction.
• "Unbounded quarantined array; non-string element": the route is root-authenticated, peer to peer, and bound to its author. A malformed element throws inside PeerCapabilityQuery.parse, which the refresh loop catches per peer (catch (final Exception e) -> suspendPeer), so one bad answer does not fail the round.
• "Blocking on the gRPC path": the override is a rare, root-only call. It blocks one handler thread for at most one 5 s round.
• "(review on PR #...) comment references": kept for consistency with the module's existing convention (for example review on PR #7953).
• "Advertisement constructor overloads": the shorter constructors are still used by existing tests (Issue7256, Issue7301, Issue7331, Issue7332, Issue7549, Issue8655). Dropping them is unrelated churn.
• "No multi-node IT where all nodes quarantine": the three changes are each tested at their caller (alert scan, plugin override, health tick) with a real state machine, and mutation-checked. A three-node IT that deterministically quarantines the same database everywhere would need a fault-injection seam that does not exist yet.

Final state

clean-approval after 3 cycles.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: f4ede162-df9d-4fd0-9c1b-ec1c54fb1204

---

📥 Commits

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

---

📒 Files selected for processing (13)

• grpc/src/main/proto/arcadedb-server.proto
• grpcw/src/main/java/com/arcadedb/server/grpc/ArcadeDbGrpcAdminService.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/ArcadeStateMachine.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/ClusterAlerts.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/PeerCapabilityQuery.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/PeerCapabilityRegistry.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/PostAcceptDivergedHandler.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/PostCapabilitiesHandler.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAPlugin.java
• ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAServer.java
• ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue9553AllVotersQuarantinedTest.java
• server/src/main/java/com/arcadedb/server/HAServerPlugin.java
• server/src/main/java/com/arcadedb/server/http/handler/openapi/PluginApiSpec.java

---

---

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

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

[claude]

Review

Overall this is a well-scoped, well-documented change. It reuses the existing capability poll, keeps the new field optional so mixed-version clusters stay on the safe side, and computes the check from memory only. Tests cover the wire format, the pure helpers, the alert and the accept path.

Points worth a look

1. Staleness window on the accept-diverged override. isQuarantinedOnEveryVoter relies on each peer's last fresh capability answer. If an operator accepts on node A, node B can still see A as quarantined until the next poll (up to the poll interval or freshness TTL). An accept on B in that window passes the guard, which defeats the 'only one copy can be chosen' guarantee, and the entries skipped on both nodes are not replayed. Options: require a fresh poll of every peer before accepting, or at least document the window. A test for the second accept inside the window would be useful.
2. Voter membership. Please confirm getLivePeers() returns only voters (no learners or non-voting members) and reflects the current Raft configuration. A learner that never reports quarantine would suppress the alert. The reverse is safe, but it is worth a test.
3. Mixed or partial answers are treated as 'not quarantined'. This is the right safe side for the alert. A down peer will also silence the alert, even if the surviving voters are all quarantined. That is acceptable, but the alert's docs could mention it so operators are not confused when one node is offline.
4. Resync skip is based on the leader's advertisement alone. If the leader's quarantine lifts between polls, the skip lasts until the next fresh answer. The comment says it is retried on the next tick, which holds only after the registry refreshes. Minor.
5. Style nit. The Advertisement records now have several overloaded constructors in two places (PeerCapabilityQuery and PeerCapabilityRegistry). They are fine, but check whether the legacy ones are still used outside tests so they can be dropped.
6. Test hygiene. The new test class uses reflection (java.lang.reflect.Field). Prefer package-private accessors where possible. There is no end-to-end HA test with three real nodes quarantining the same DB, so the soak scenario in HA: two committed schema changes give the same file id to different compacted index files, quarantining the database on every node #9547 is covered only at the unit level.

No security problems spotted: accept-diverged is still root-only, and the alert is scoped to visible databases. The only new data on the capability endpoint is database names, which that endpoint already treats as cluster-internal.

Nice work. I'd resolve point 1 (or document it) before merge.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 13 complexity

Metric	Results
Complexity	13

View in Codacy

🟢 Coverage 87.41% diff coverage · -5.62% coverage variation

Metric	Results
Coverage variation	✅ -5.62% coverage variation
Diff coverage	✅ 87.41% 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 (c001164)	254582 (+32749)	203568 (+13716)	79.96% (-5.62%)

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 (#9567)	143	125	87.41%

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]

Review of PR #9567 (all voters quarantined, #9553)

The design is sound. "Unknown is never counted as quarantined" is the right bias for an alert that tells an operator to force-accept a copy. The registry check is a cheap in-memory read, and the live confirmation before an accept closes the stale-poll window. The notes below are what I'd look at before merge.

Potential issues

1. Two concurrent accepts can still both pass. refuseUnlessNoOtherVoterServesNow asks the other voters whether they serve the database now. If accept-diverged is called on node A and node B at nearly the same time, both still read the other as quarantined, both lift, and two copies are chosen. The window is much narrower than with the registry alone, but the PR text says "Only one copy can be chosen", which is stronger than what the code guarantees. Either soften that wording, or serialize the accept through the leader or a Raft entry.
2. Log spam in retryUnfilledSnapshotGap. The early-return path logs Not retrying the targeted resync... on every health tick for as long as the state lasts. It runs before the throttle slot, so it isn't throttled. Log on a transition only, or rate-limit it.
3. Floor-only quarantines are excluded. acceptDivergedDatabase(sm, ...) accepts getDatabaseAppliedFloor(db) >= 0 with no quarantine cause, but isQuarantinedOnEveryVoter requires quarantineCause(db) != null. Probably intended, since getQuarantinedDatabaseNames() reads divergedDatabases. Please confirm.
4. getLivePeers() may include non-voters. The names say "voter", but this returns the group's peers. If learners or listeners can be present, they would have to report quarantined before the alert fires or the accept is allowed. That fails safe but could leave the cluster stuck. Please check that only voters are considered.
5. The live confirmation blocks an HTTP worker thread for up to ROUND_TIMEOUT_MS. Acceptable for a rare root-only operation. A short comment saying so would help.

Minor / style

• Both Advertisement records now have a compact constructor plus several convenience overloads, and each new field adds another. A static factory or dropping unused overloads would keep this manageable.
• PeerCapabilityQuery.parse takes the quarantined array from a peer with no cap on size or entry length. The route is authenticated, so this is low risk, but a cap is cheap.
• The alert recommendation hard-codes /api/v1/cluster/leader. A constant, like PostAcceptDivergedHandler.ROUTE, would avoid drift.
• The (review on PR #9567) comments describe history that git already records, so I'd drop them.

Tests

Issue9553AllVotersQuarantinedTest (18 tests) plus the mutation checks on the three wiring points cover the pure logic well. I didn't see direct tests of refuseUnlessNoOtherVoterServesNow with a serving voter, a timeout, or an undialable voter. I also didn't see a multi-node IT where all nodes quarantine, one accepts, and the others resync. The live-confirm path carries the safety argument for the accept, so it deserves direct tests, including the concurrent-accept case from (1). Please also confirm no test uses a hard-coded port (HardcodedTestServerPortsTest).

Security

The accept path stays root-only. The new field travels over the existing authenticated route. The alert is scoped to the caller's visible databases. I found no new exposure.

[claude]

Review of PR 9567

Well-scoped change. Conservative in the right places: an unknown or stale peer is never counted as quarantined, the new JSON field is optional and wire-compatible with older builds, and getDatabasesQuarantinedOnEveryVoter reads memory only. Test coverage is thorough (wire format, registry, decision logic, alert scoping, override, retry filter, live confirmation).

Points worth a look

1. Two simultaneous accepts can both pass. The alert text and the acceptDivergedDatabase Javadoc both admit this. The live refuseUnlessNoOtherVoterServesNow check narrows the window but cannot close it, since neither node is serving yet when both ask. The result would be two different accepted copies, the silent divergence the 409 exists to prevent. It is root-only and the operator is told to accept on one node, so this may be an acceptable trade-off. Please confirm it is deliberate; routing the accept through the leader would close it.

2. Quarantine set vs. the accept precondition. getQuarantinedDatabaseNames() advertises only divergedDatabases, while acceptDivergedDatabase also treats getDatabaseAppliedFloor(db) >= 0 as 'something stands'. A node holding only a stale applied floor never advertises it, so peers read it as a usable copy and the override stays closed. That is the safe direction, but worth a comment or a pinning test.

3. Unbounded peer-supplied array. PeerCapabilityQuery.parse copies every element of 'quarantined' into a LinkedHashSet with no cap. The route is authenticated so risk is low, but a cap is cheap. getString(i) on a non-string element throws; please check that this is handled per peer and does not fail the whole poll.

4. Blocking I/O on the request thread. refuseUnlessNoOtherVoterServesNow blocks the HTTP/gRPC handler for up to ROUND_TIMEOUT_MS. Fine for a rare root operation, but make sure the gRPC path does not hold a shared executor thread meanwhile.

5. Log noise. In retryUnfilledSnapshotGap the 'Not retrying ... (issue 9553)' message is logged at HALog.BASIC on every tick while the state persists. Consider logging on transition only.

6. Minor style. Constructor/overload growth on both Advertisement records, record(...) and advertisement(...) is getting heavy; drop the older overloads once callers migrate. The alert recommendation string has a very long line, so please wrap it. '(review on PR 9567)' references in comments will rot; state the reason instead.

No blocking issues found. I did not build or run the tests locally.

[codecov]

Codecov Report

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

Files with missing lines	Patch %	Lines
...java/com/arcadedb/server/ha/raft/RaftHAServer.java	69.23%	12 Missing and 8 partials ⚠️
...om/arcadedb/server/ha/raft/ArcadeStateMachine.java	72.72%	2 Missing and 4 partials ⚠️
...m/arcadedb/server/ha/raft/PeerCapabilityQuery.java	73.33%	2 Missing and 2 partials ⚠️
...rcadedb/server/ha/raft/PeerCapabilityRegistry.java	66.66%	2 Missing and 2 partials ⚠️
...cadedb/server/ha/raft/PostCapabilitiesHandler.java	62.50%	0 Missing and 3 partials ⚠️
...ava/com/arcadedb/server/ha/raft/ClusterAlerts.java	92.30%	0 Missing and 1 partial ⚠️
...java/com/arcadedb/server/ha/raft/RaftHAPlugin.java	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9567      +/-   ##
============================================
- Coverage     77.36%   77.36%   -0.01%     
- Complexity     4165     4228      +63     
============================================
  Files          2157     2157              
  Lines        221833   221965     +132     
  Branches      47521    47565      +44     
============================================
+ Hits         171611   171713     +102     
  Misses        32020    32020              
- Partials      18202    18232      +30     

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