fix: [#4741] follower-side recovery for stuck Raft-log divergence (IN…

[lvca]

…CONSISTENCY loop)

On an idle cluster, a tiny (1-2 entry) Raft-log divergence at a low index makes the leader's Ratis GrpcLogAppender spin forever on received INCONSISTENCY reply with nextIndex {4,5}, errorCount=...: the follower replies snapshotIndex+1 for one index then fails the previous-entry term check for the next, oscillating. The leader's log is never compacted, so shouldNotifyToInstallSnapshot stays null and the proper reconcile path never fires; the divergence is too small for the lag-based recoveries (HA_STALE_FOLLOWER_LAG_THRESHOLD / HA_STALLED_REPLICA_RESYNC_DURATION_MS), which also only re-acquire the database, not the Raft log. Only a node restart cleared it.

Add a follower-side detector: when a running follower recognizes a leader at a newer term but cannot apply the leader's current-term entries (currentTerm > lastAppliedTerm and commit == applied), HealthMonitor waits HA_STALE_FOLLOWER_RECOVERY_DURATION_MS for the condition to persist, then reformats the follower's Raft storage and rejoins the group (RaftHAServer.recoverFromDivergence -> restartRatis(format=true)). The peer rejoins as a fresh member so the leader reconciles it via the snapshot-install path - the automated, scoped equivalent of the manual node restart. Gated by the new HA_DIVERGED_FOLLOWER_RECOVERY config (default true).

Tests: HealthMonitorTest streak unit tests (deterministic clock); RaftDivergedFollowerRecoveryIT (@slow) verifies reformat+rejoin with no data loss and no false positives on a healthy node.

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

• I have run the build using mvn clean package command
• My unit tests cover both failure and success scenarios

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Not up to standards ⛔

🔴 Issues 8 minor

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

Results:
8 new issues

Category	Results
Documentation	1 minor
CodeStyle	7 minor

View in Codacy

🟢 Metrics 44 complexity

Metric	Results
Complexity	44

View in Codacy

🟢 Coverage 70.45% diff coverage · -7.37% coverage variation

Metric	Results
Coverage variation	✅ -7.37% coverage variation
Diff coverage	✅ 70.45% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (4da1bb6)	131929	98114	74.37%
Head commit (1a62cba)	163848 (+31919)	109783 (+11669)	67.00% (-7.37%)

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 (#4744)	132	93	70.45%

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

[gemini-code-assist]

Code Review

This pull request introduces a self-healing mechanism to resolve issues where a Raft follower becomes stuck due to log divergence (issue #4741). It adds a new configuration option HA_DIVERGED_FOLLOWER_RECOVERY, implements stuck-divergence detection and recovery in HealthMonitor and RaftHAServer (which reformats the local Raft storage and rejoins the group), and includes comprehensive unit and integration tests. The feedback suggests adding a check and warning log in RaftHAServer if deleteRecursive fails to completely delete the Raft storage directory before formatting, which would improve diagnostics and serviceability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If deleteRecursive(storageDir) fails to completely delete the Raft storage directory (e.g., due to locked files or permission issues on Windows), the directory will still exist and may contain partial files. Since deleteRecursive silently ignores deletion failures, Ratis will subsequently attempt to start with RaftStorage.StartupOption.FORMAT on a non-empty directory, which can fail or cause unexpected behavior. Adding a check and a warning log if the directory still exists after deletion will greatly improve serviceability and diagnostics.

        if (formatStorage) {
          if (storageDir.exists()) {
            deleteRecursive(storageDir);
            if (storageDir.exists()) {
              LogManager.instance().log(this, Level.WARNING,
                  "Failed to completely delete Raft storage directory: %s. Ratis startup may fail.", storageDir.getAbsolutePath());
            }
          }
          startupOption = RaftStorage.StartupOption.FORMAT;
        } else

[claude]

Code Review - PR #4744: follower-side recovery for stuck Raft-log divergence (#4741)

Reviewed the full diff against the HA/Raft code and the existing stale-follower recovery it mirrors. Overall this is a well-scoped, well-documented fix that fits the existing self-healing architecture cleanly.

Strengths

• Mirrors the established pattern. checkStuckFollower() is a faithful sibling of checkStaleFollower() (first-observation arms the streak, any clearing tick resets, fires once per streak), and isFollowerStuckDiverged() matches the structure/guards of isFollowerLaggingBeyond(). Easy to reason about.
• Backward compatible. The new 5-arg HealthMonitor constructor chains through the old 3/4-arg ones with divergedFollowerRecoveryEnabled = false, and divergenceRecoveryDisabledByLegacyConstructor locks that in.
• Clean refactor. Folding the format/recover distinction into a single private restartRatis(boolean) and keeping restartRatisIfNeeded() as the false entry point avoids duplication; the recoveryLock + restartFailureCount machinery is reused correctly so the two restart paths stay serialized.
• Detection predicate is sound. currentTerm > appliedTerm && commit == appliedIndex is a good signal: a normally-lagging follower has commit > applied so it won't match, and the catching-up / snapshot-pending guards plus the 60s persistence window filter the brief post-election no-op gap. The Javadoc reasoning is excellent.
• Config defaults and docs are thorough and cross-reference the related knobs.

Concerns / questions

1. The positive detection path has no test exercising it. The unit tests drive a boolean stuckDiverged flag in the fake, and the IT calls recoverFromDivergence() directly on a healthy node. So isFollowerStuckDiverged() returning true from a genuinely diverged term/commit/applied state is never verified - only its false-positive case (IT assertion build support #1) is. Since the action is destructive (wipes Raft storage), the predicate is the highest-risk piece here. Consider a focused unit test that stubs getCurrentTerm()/getCommitIndex()/getLastAppliedTermIndex() (or a small seam) to confirm the predicate returns true only for the divergence case and false for the post-election window, leaderless, catching-up, and leader states. The PR rightly notes reproducing the real loop is hard, but the predicate itself is unit-testable in isolation.

2. DB-level divergence isn't covered. The IT reformats a healthy follower and confirms its DB survives the reformat+snapshot-install (count stays 50). But the real AppendLogResponseHandler: received INCONSISTENCY reply #4741 hazard is a follower that applied a diverged entry to its database. The test doesn't confirm the snapshot-install path overwrites/corrects an already-diverged DB. Worth a comment noting this is assumed, or a follow-up that injects a divergent local write before recovery.

3. Non-atomic state reads. isFollowerStuckDiverged() reads getLeaderId(), getLastAppliedTermIndex(), getCurrentTerm(), and getCommitIndex() as separate raftServer.getDivision(...) calls, so they can observe slightly different moments during an election. The 60s persistence window makes a one-tick inconsistency harmless, so this is acceptable - just flagging it's relied upon implicitly.

4. Minor: applied.getTerm() isn't range-guarded. The guard checks currentTerm < 0, appliedIndex < 0, commit < 0 but not applied.getTerm() < 0. If a TermIndex ever carried a negative term, currentTerm > negativeTerm would be true and could combine with commit == appliedIndex to fire. Very unlikely for a valid TermIndex, but cheap to add for symmetry.

5. Repeated false detection -> server stop. A failed reformat increments the shared restartFailureCount, so a hypothetical flapping false-positive could escalate to arcadeServer.stop() after HA_RATIS_RESTART_MAX_RETRIES. This is the intended cluster-level escalation and a successful reformat resets the counter to 0, so it's fine - just be aware the divergence path now feeds that same backstop.

Style / conventions

• Matches CLAUDE.md conventions: final on params/locals, no em dashes, single-statement if without braces consistent with surrounding code, @Tag("slow") correctly applied to the IT, Apache license header present, and the IT author tag credits a human (no Claude attribution).

Verdict

Solid, low-risk fix with good documentation and a sensible default-on gate. The main gap is test coverage of the positive detection predicate, given the destructive nature of the recovery action. Recommend adding a unit-level test for isFollowerStuckDiverged() before merge; the rest are minor/optional.

[claude]

Review: follower-side recovery for stuck Raft-log divergence (issue #4741)

Solid, well-scoped fix. The design (follower self-detects, waits for persistence, reformats + rejoins so the leader reconciles via snapshot-install) is the right automated analogue of the manual node restart, and the diff is clean and well-documented. Extracting the pure isStuckDivergedState(...) predicate so it can be unit-tested in isolation is exactly the right call given the recovery is destructive. Observations, ordered by significance:

CORRECTNESS / POTENTIAL ISSUES

1. Repeated-reformat circuit breaker gap. restartRatis(true) shares the restartFailureCount budget with the CLOSED/EXCEPTION path, but that counter resets to 0 on every successful restart (line 735). A reformat that restarts cleanly is a "success" even if the node immediately re-diverges and trips the detector again 60s later. In the happy path the snapshot-install makes the divergence go away for good, so this never bites - but if some underlying condition keeps reproducing the divergence, the node will reformat + full-snapshot-install every HA_STALE_FOLLOWER_RECOVERY_DURATION_MS indefinitely without ever hitting the max-retries breaker. Consider a separate counter (or simple backoff) for divergence reformats so a pathological case is bounded and visible, rather than a silent 60s reformat loop.

2. False-positive surface on a slow-but-not-diverged follower. The signature currentTerm > appliedTerm && commitIndex == appliedIndex is also momentarily true for a perfectly healthy follower on an idle cluster that simply has not yet received the new leader no-op entry (its commit/applied are still at the old term, currentTerm has advanced). The only thing distinguishing stuck-diverged from "no-op has not arrived yet" is the 60s persistence window. On a healthy cluster the no-op replicates in milliseconds so this is fine, and the code/comments acknowledge it - but it is worth confirming the default HA_STALE_FOLLOWER_RECOVERY_DURATION_MS (60s) is comfortably larger than any realistic worst-case no-op propagation delay under network degradation, since the cost of a false positive is a disruptive (if non-data-losing) full snapshot install. The default looks generous; just flagging the dependency.

3. Both recovery paths can fire in one tick. tick() runs checkStaleFollower() then checkStuckFollower(). The two signatures are largely mutually exclusive (stale = commit - applied > threshold; stuck = commit == applied), so in practice only one arms - but nothing structurally prevents both from triggering in the same tick. A one-line comment noting they are mutually exclusive by construction (or an else/early-return) would make the intent explicit.

TESTS

• isStuckDivergedState predicate coverage is excellent: positive signature, healthy-at-current-term, normal lag (commit > applied), leaderless, catching-up, snapshot-pending, unreadable (negative) inputs, and the same-term edge. Good adversarial set for the highest-risk piece.
• HealthMonitorTest streak coverage mirrors the stale-follower tests well (legacy-constructor-off, flag-off, first-observation, after-duration, reset-on-clear, reset-on-unhealthy, shutdown).
• The IT honestly documents its scope: it exercises the recovery action against a healthy follower, not a real on-disk divergence. Reasonable given the timing-dependent nature of the bug, and the predicate positive path is covered in the unit test. One gap: there is no end-to-end test where the follower local state actually diverges before recovery, so the "leader reconciles a genuinely diverged DB via snapshot-install" claim rests on the pre-existing SnapshotInstaller path rather than being proven here. Acceptable, but the assumption is load-bearing.

STYLE / MINOR

• Code adheres to project conventions (final params/locals, no FQNs, single-statement if without braces, @tag("slow") on the IT). Config doc on HA_DIVERGED_FOLLOWER_RECOVERY is thorough.
• The non-atomic multi-read of Raft state is explicitly documented and tolerated by the persistence window, so no change needed there.

Overall this is a careful, defensively-coded fix with strong unit coverage. My only substantive ask is bounding the reformat loop (point 1); points 2-3 are confirmations/comments.

[claude]

Review: follower-side recovery for stuck Raft-log divergence (#4741)

Reviewed the full diff plus the surrounding RaftHAServer / HealthMonitor context. This is a well-engineered, defensively-written fix. The decision logic is correctly factored out into a pure, unit-testable predicate (isStuckDivergedState), the destructive action is gated behind a persistence window + a circuit breaker, and the config is thoroughly documented. Nice work overall. A few things worth discussing before merge.

Potential issues

1. False positive on a slow/asymmetric link vs. genuine log divergence (main risk).
isStuckDivergedState cannot distinguish a follower whose log has genuinely diverged from one that simply cannot receive AppendEntries for a sustained period for non-divergence reasons (asymmetric network where heartbeats arrive so getLeaderId() != null and currentTerm advances, but log replication is failing/throttled; leader appender backpressure; etc.). In that case commit == applied at a stale term holds without any real divergence, and after HA_STALE_FOLLOWER_RECOVERY_DURATION_MS (default 60s) the follower would reformat. The reformat itself is "safe" (rejoin + snapshot-install), but on a large database it converts a transient, self-healing condition into a full snapshot transfer. Is 60s enough margin in practice, or is there an additional discriminator available (e.g. whether the appender is actually returning INCONSISTENCY vs. timing out) that could tighten the signature? At minimum worth calling out in the config doc that a long one-sided network outage can trigger a reformat.

2. Cost of a spurious reformat scales with DB size.
The circuit breaker caps the count (default 5) but each reformat triggers a whole-database snapshot install. On a multi-GB database, even a handful of false-positive reformats is expensive and adds replication load to the leader. Given the destructive+expensive nature, including the current applied/commit/term values in the WARNING log (so an operator can confirm it really was diverged, not partitioned) would help post-incident diagnosis.

3. Integration test does not exercise the positive detection path.
RaftDivergedFollowerRecoveryIT honestly documents that it calls recoverFromDivergence() directly rather than reproducing a real divergence, and only asserts the negative (isFollowerStuckDiverged() is false on a healthy node). So the actual end-to-end trigger - that a genuinely diverged Ratis follower produces currentTerm > appliedTerm && commit == applied from the real getters - is never verified; only the hand-fed predicate is. If Ratis's getDivision(...).getInfo() ever reports these differently in a true divergence, the whole mechanism silently never fires and the only signal would be the absence of recovery. This is hard to reproduce deterministically (acknowledged), but it's the one gap that matters most given this is the headline scenario. Even a best-effort, flaky-tolerant repro behind @Tag("slow") would materially de-risk it.

Minor / style

• Non-atomic multi-source read: getCommitIndex() reads raftLog.getLastCommittedIndex() while appliedIndex comes from stateMachine.getLastAppliedTermIndex() - two different subsystems read across separate getDivision calls. Correctly documented and the persistence window absorbs one-tick skew, so it's fine; flagging only for the record.
• Constructor telescoping: HealthMonitor now has four overloaded constructors for backward-compat (the legacy 2-arg/4-arg forms are covered by divergenceRecoveryDisabledByLegacyConstructor), which is reasonable, but a builder or a single config-object param would age better if more knobs get added.
• Field-init alignment in HealthMonitor is mostly clean; divergenceRecoveryExhausted is the one field not column-aligned with its neighbours (cosmetic).

Things done well

• Pure predicate split out specifically because "the destructive recovery makes the predicate the highest-risk piece" - exactly the right instinct, and RaftHAServerStuckDivergenceTest covers the signature plus every negative branch, including all four unreadable/negative inputs.
• Streak + persistence-window logic mirrors the existing checkStaleFollower, and the mutual-exclusivity reasoning (commit == applied vs commit - applied > threshold) is correct, so at most one path arms per tick.
• Circuit breaker with episode re-arm after 5x healthy duration is a genuinely thoughtful addition - it prevents the reformat -> snapshot loop that would otherwise be masked by restartFailureCount being reset on each clean restart. The re-arm is tested (divergenceBudgetReArmsAfterHealthyPeriod).
• deleteRecursive partial-failure detection before FORMAT (Windows locked-file case) is a nice defensive touch.
• Guarded against acting on the leader, during shutdown, while catching up, and during snapshot install.

Verdict

Solid, well-tested at the unit level, and conservative by construction. The one item I'd want resolved before merge is #1/#3: confidence that the signature is specific to divergence and not to a sustained network outage, ideally backed by (or at least an explicit note about) the partition case. The rest are non-blocking.

[claude]

Review: follower-side recovery for stuck Raft-log divergence (#4741)

Thanks for this - the change is well-reasoned and the documentation is excellent. The root-cause writeup (idle cluster, tiny divergence, leader's GrpcLogAppender spinning on INCONSISTENCY because the log is never compacted so shouldNotifyToInstallSnapshot stays null) makes the fix easy to follow, and splitting the destructive decision into a pure, unit-testable predicate (isStuckDivergedState) is exactly the right instinct given the stakes.

Strengths

• Pure predicate + deterministic tests. isStuckDivergedState(...) is isolated from Ratis plumbing, with positive/negative/unreadable-state coverage. HealthMonitorTest drives the streak logic with an injected clock. Good TDD hygiene.
• Bounded reformat budget. Nice catch that a clean reformat resets the shared restartFailureCount (confirmed at RaftHAServer.java:737), so the existing HA_RATIS_RESTART_MAX_RETRIES breaker can't catch a re-diverging node. The per-episode cap with a healthy-period re-arm closes that loop, and the SEVERE-once-then-stop behavior is the right operator experience.
• Backward compatibility. Legacy constructors preserved and explicitly tested (divergenceRecoveryDisabledByLegacyConstructor).
• Single-threaded safety. scheduleWithFixedDelay guarantees ticks never overlap, so the non-volatile streak/budget fields in HealthMonitor are safe without synchronization.

Concerns

1. Unrelated mongodbw SASL PLAIN change bundled in (#4746). The diff includes MongoDBBackend + MongoDBAuthenticationTest, which is issue #4746 and unrelated to #4741. That commit also already appears on main (31ad371), so it should drop out on a rebase - please rebase to confirm it disappears. If it doesn't, it shouldn't ship in a #4741 PR (harder to review/revert independently).

2. Destructive recovery is default-on. HA_DIVERGED_FOLLOWER_RECOVERY defaults to true, i.e. automatic deletion of Raft storage is enabled out of the box. The narrow predicate plus the 60s persistence gate make a false positive unlikely, but a single mis-fire that persists one window will reformat a node automatically with no operator in the loop. Worth a conscious sign-off on default-on vs. opt-in for the first release - the manual mitigation it replaces (node restart) was at least operator-initiated.

3. No cluster-level coordination across followers. If a systemic condition makes several followers simultaneously satisfy currentTerm > appliedTerm && commit == applied (e.g. a leader that recognizes the group but cannot commit its current-term no-op), every follower's persistence streak starts at roughly the same time and they could all reformat within the same window. The cluster is likely already broken in that scenario, but a simultaneous quorum-wide reformat is worth guarding against (stagger by peer, or only reformat when a healthy quorum is observed). At minimum, please confirm this can't turn a recoverable hiccup into a coordinated wipe.

4. Predicate matches non-divergence "can't receive entries" states. A follower that receives heartbeats (so it updates its term and sees the leader) but cannot receive AppendEntries (no-op never arrives) - e.g. asymmetric/partial connectivity - matches the divergence signature without actually being diverged. Reformatting won't fix connectivity and just churns the node. No data loss (the leader still holds everything), so low severity, but it means "stuck-diverged" is really "stuck at a stale term for any reason." Maybe reflect that in the naming/log message.

5. Partial-delete then proceed-anyway. In restartRatis(true), if deleteRecursive leaves the directory behind (locked file / Windows), the code logs a WARNING and still calls FORMAT on a non-empty dir, where Ratis behavior is undefined. Consider aborting the reformat and counting it as a failed attempt (so the retry/budget logic handles it) rather than proceeding into an undefined startup.

Test coverage

Strong at the unit level. The one gap: RaftDivergedFollowerRecoveryIT invokes recoverFromDivergence() directly on a healthy follower (acknowledged in the Javadoc), so the end-to-end path of detection actually firing on a genuinely diverged follower is never exercised - only the recovery action plus the unit-tested predicate. That's a reasonable limitation given how timing-dependent the real divergence is, but it leaves the predicate-to-recovery wiring unverified against a live cluster. If feasible, even crude fault injection (force a stale lastAppliedTermIndex on a real follower) would close it.

Minor

• isFollowerStuckDiverged() reads term/commit/applied via separate getDivision(...) calls (non-atomic) - already documented and absorbed by the persistence window, fine.
• Edge: a freshly-formatted/empty node (appliedTerm=0, applied==commit==0) against an idle leader at term>0 could match the signature in any window where catchingUp/snapshotPending aren't set. Low risk, but worth a sanity check that the bootstrap window always sets one of those guards.

Overall this is a solid, defensive fix with good tests. The main blocker is splitting out the unrelated mongodbw change (likely just a rebase); items 2-3 are worth an explicit decision before merge given the destructive, default-on nature of the recovery.

[claude]

PR Review: #4744

Reviewed the full diff. Overall this is high-quality, well-documented work: the Raft divergence detector is split into a pure, unit-testable predicate, the destructive recovery is gated, bounded, and opt-in, and every code path has a test. The MongoDB fixes are small, correct, and each ships a regression test. Points worth discussing, ordered by importance:

1. Scope: this PR bundles four unrelated issues

The diff mixes the HA/Raft diverged-follower recovery (#4741) with three MongoDB wire-protocol fixes (#4745, #4746 SASL, #4746 find limit). These touch disjoint modules and carry independent risk profiles. Splitting into two PRs would make review, bisection, and revert far cleaner - especially since the Raft change is destructive and the MongoDB changes are user-facing data correctness. Not a blocker, but worth noting.

2. recoverFromDivergence() runs restartRatis() synchronously on the health-monitor thread

`checkStuckFollower()` calls `target.recoverFromDivergence()` inline, which calls `restartRatis(true)` under `synchronized (recoveryLock)` - a full Ratis teardown + storage delete + rebuild. This blocks the single health-monitor scheduled thread for the whole reformat, so no other health checks (lag, CLOSED/EXCEPTION recovery, allowlist refresh) run until it returns. This matches the existing `restartRatisIfNeeded()` pattern so it's consistent, but the reformat path is heavier (disk delete + FORMAT + full snapshot install). Worth confirming the tick interval and any executor timeouts tolerate a multi-second block here.

3. MongoDBDatabaseWrapper.collections is now never invalidated

Previously `handleClose()` cleared the cache; now entries live for the wrapper's lifetime (correct fix for the reconnect bug). But `getCollection()` caches a `MongoDBCollectionWrapper` by name via `computeIfAbsent` and never removes it. If a type is dropped and recreated, the stale wrapper lingers. The `existsType()` guard correctly returns `null` for a dropped type, so behavior is safe, but the map only grows. Likely negligible; flagging in case collection churn is expected - worth a one-line note that drop/recreate relies on `MongoDBCollectionWrapper` re-resolving the type by name per op.

4. HA_DIVERGED_FOLLOWER_MAX_REFORMATS negative value silently means "unbounded"

The breaker check is `divergedFollowerMaxReformats > 0`, so `0` (documented as unbounded) and any negative value behave identically. The doc only mentions `0`. Either clamp negatives or note that `<= 0` disables the breaker, so an operator typo like `-1` doesn't silently remove the safety cap.

5. SASL PLAIN: confirm credentials are never logged

The implementation is sound and TLS guidance is documented. `parts[2]` is the cleartext password, and on the unsupported-command path `MongoDBDatabaseWrapper.handleCommand` logs the full `document` at SEVERE. Since `saslStart`/`saslContinue` are intercepted in `MongoDBBackend.handleCommand` before delegating to the wrapper, the password document never reaches that log line - good. Just worth a deliberate confirmation that no underlying bwaldvogel/Netty debug logging echoes the SASL payload.

6. Minor

• `isStuckDivergedState` reads several Ratis getters non-atomically; well-documented and the persistence window absorbs it. Good call extracting it to a pure function with the positive-case unit test - the right place to invest given the destructive action.
• `RaftDivergedFollowerRecoveryIT` honestly documents that it exercises the recovery action against a healthy follower, not a genuinely diverged log. Reasonable given the timing-dependence, and the real-getter invariant assertions (appliedTerm==currentTerm, commit==applied on a healthy node) nicely de-risk the predicate-vs-live-getters wiring.
• Defaulting `HA_DIVERGED_FOLLOWER_RECOVERY` to `false` for a destructive auto-action is the right conservative choice for a first release.
• The budget re-arm timer can begin accumulating "healthy" time while the node is still `catchingUp` (since `isStuckDivergedState` returns false during catch-up). Harmless, but the streak start is slightly earlier than "fully healthy".

Test coverage

Strong. `HealthMonitorTest` covers the streak state machine, budget cap, unbounded mode, and re-arm-after-healthy with a deterministic clock; `RaftHAServerStuckDivergenceTest` covers the predicate's positive and all negative branches including unreadable state; the MongoDB regressions each pin a specific reported bug.

Nothing here is a hard blocker - items 2-4 are the ones I'd most want addressed (or explicitly acknowledged) before merge.

Automated review by Claude. Reviewer guidance from CLAUDE.md applied.

[claude]

PR Review: #4744 - follower-side recovery for stuck Raft-log divergence

Reviewed the full diff. This is solid, carefully-reasoned work - the self-documenting config descriptions, the split between the pure predicate (isStuckDivergedState) and the Ratis plumbing, and the deterministic HealthMonitor unit tests are all excellent. Feedback below, grouped by theme.

1. PR scope (process)

The PR bundles two unrelated efforts: the HA Raft divergence recovery (#4741) and the MongoDB wire-protocol changes (SASL PLAIN auth #4746, find limit / lazy collection / per-channel cleanup #4745). They touch disjoint modules and have independent risk profiles. Splitting into two PRs would make review, bisect, and revert cleaner. Not blocking (maintainer's call), just flagging.

2. HA Raft - the highest-risk piece

Destructive recovery on a non-divergence signature (the core trade-off). isStuckDivergedState returns true for currentTerm > appliedTerm && commitIndex == appliedIndex. This same signature is produced by a follower that is merely behind on an idle cluster (it learns currentTerm from heartbeats while its local log hasn't committed the new-term entry, so commitIndex == appliedIndex at a stale term). The only thing separating "genuinely diverged" from "transiently behind / one-sided outage" is the persistence window (HA_STALE_FOLLOWER_RECOVERY_DURATION_MS, default 60s). That's a sound design and the code/docs acknowledge it - but it means the safety of an automatic, destructive action rests entirely on one timing threshold. Two suggestions to harden it:

• For a large but legitimate replication backlog, confirm isCatchingUp() reliably stays true for the whole catch-up (otherwise a slow-but-progressing follower over a congested link could trip the 60s window and reformat needlessly). Worth a sentence in the IT or a comment pinning that assumption.
• Consider keying off whether appliedIndex is actually stuck (unchanged across the streak) rather than only the term relationship - a follower whose applied index is still advancing is making progress and should never reformat, even if it momentarily satisfies the term predicate.

Operator visibility. The exhaustion path logs SEVERE exactly once and then goes silent. Per the concurrency/metrics conventions in CLAUDE.md (Micrometer PoolMetrics + Studio cards), a sustained-divergence / reformat-count metric (gauge or counter, tagged per server) would let operators running dashboards - not log scraping - notice a node stuck in the give-up state or a cluster reformatting repeatedly. The "no cross-follower coordination" risk (several followers reformatting in the same window and briefly costing quorum) is acknowledged and deferred; a metric makes that observable in the meantime.

Non-atomic state reads. getCurrentTerm() / getCommitIndex() / getLeaderId() / getLastAppliedTermIndex() are separate getDivision(...) calls and can observe different moments. This is documented and absorbed by the persistence window, and all of them return -1 on failure which the predicate rejects - good. No change needed, just confirming the negative-guard covers every getter (it does).

restartRatis(true) delete handling. The "abort if directory still exists after deleteRecursive" guard before FORMAT is exactly right - avoids FORMAT-on-non-empty undefined behaviour, and routing the throw through the existing retry/backoff machinery is clean.

3. MongoDB wire protocol

lastResults.clear() -> lastResults.remove(channel) is a genuine bug fix: the wrapper is cached per-database and shared across connections, so the old clear() wiped every client's state when any one disconnected. Removing only the closing channel is correct, and dataStillReturnedAfterClientReconnect covers it well.

SASL PLAIN auth - enforcement question (security). handleSaslStart validates credentials and fails closed with a generic message (good - no info leak). But I don't see per-channel authentication state being tracked: a client that simply never sends saslStart falls straight through to super.handleCommand(...). So unless AbstractMongoBackend independently gates commands on a prior successful auth, the SASL handshake is advisory - it validates credentials only for clients that opt in via MongoCredential. Please confirm whether unauthenticated connections are actually rejected on the command path; if not, this should be documented as a known limitation (or the channel marked authenticated and enforced). The tests only cover the happy/wrong-password paths with a credential, not the skip-auth path.

find default limit -1 -> 0. Correct fix for the single-document bug (#4745), and the inline comment about the -1 legacy single-batch sentinel is helpful. Covered by findWithoutFilterReturnsAllDocuments.

Lazy getCollection cache never evicted. The bounded-by-type-count reasoning and the "stale entry harmless because the wrapper re-resolves by name" note are reasonable. Fine as-is.

4. Tests

• HealthMonitorTest: excellent - deterministic clock, covers first-observation, persistence, reset-on-clear, reset-on-unhealthy, the bounded budget, unbounded (max=0), and budget re-arm. Thorough.
• RaftHAServerStuckDivergenceTest: good coverage of the predicate including the positive case and unreadable-state negatives.
• RaftDivergedFollowerRecoveryIT: the scope note is honest - it exercises the recovery action on a healthy follower, not an actual induced divergence, and the false-positive check against the real Ratis getters is a nice touch. The gap (no injected on-disk divergence before recovery) is acceptable given how hard the real race is to reproduce, but it does mean the end-to-end "diverged log actually gets reconciled" path is unverified by an automated test - worth calling out in the PR description.

Overall: well-engineered and well-tested. My main asks before merge are (a) confirm the MongoDB SASL auth is actually enforced (not just validated for opt-in clients), and (b) consider strengthening the divergence signal with an "applied index actually stuck" check and/or a metric, given the action is destructive and automatic.

[codecov]

Codecov Report

❌ Patch coverage is 64.39394% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.31%. Comparing base (4da1bb6) to head (1a62cba).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...java/com/arcadedb/server/ha/raft/RaftHAServer.java	21.62%	29 Missing ⚠️
...c/main/java/com/arcadedb/mongo/MongoDBBackend.java	65.51%	4 Missing and 6 partials ⚠️
...ava/com/arcadedb/server/ha/raft/HealthMonitor.java	90.56%	4 Missing and 1 partial ⚠️
...ava/com/arcadedb/mongo/MongoDBDatabaseWrapper.java	66.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4744      +/-   ##
============================================
- Coverage     65.32%   65.31%   -0.01%     
- Complexity      580      594      +14     
============================================
  Files          1673     1673              
  Lines        131929   132045     +116     
  Branches      28244    28267      +23     
============================================
+ Hits          86181    86246      +65     
- Misses        33851    33892      +41     
- Partials      11897    11907      +10     

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