feat(#9399): expose the follower replication-path and unreachable-leader state in GET /api/v1/cluster - #9451

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant HealthMonitor
  participant RaftHAServer
  participant GetClusterHandler
  participant ClusterAlerts
  HealthMonitor->>RaftHAServer: Track leader reach after restart
  RaftHAServer-->>GetClusterHandler: Provide replication-path state
  GetClusterHandler->>ClusterAlerts: Scan using replication-path state
  ClusterAlerts-->>GetClusterHandler: Return selected alerts

Merge Risk: ⚪ Minimal · up to f6e95

The new cluster-status fields and alerts have no identified merge-blocking issue; proceed with normal checks.

• Fix all pre-merge checks with AI

• Autopilot · 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.

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

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.

Up to standards ✅

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

Review of PR 9451

I read the main-code diff: LeaderReachSinceRestartTracker, the RaftHAServer hook, the HealthMonitor placement, GetClusterHandler and ClusterAlerts. I skimmed the tests and the OpenAPI changes. I did not compile or run anything.

Overall: the design is clean. The predicate is pure and separate from the stateful tracker, and the hook is defensive (it never propagates, and it resets on any read failure). It sits before the early returns in tick() and has a sensible grace of 2x the election timeout max. The alert precedence is documented, and the new fields are marked required in the schema.

Points to check

1. Full-cluster restart or no-quorum case. When !leaderKnown and the path is unproven, the predicate returns true. A node that sits through a cluster-wide outage or a long election after an in-place restart will raise a critical alert after about 2x election max. The alert text admits it is "a symptom" of a missing quorum. A critical alert that fires on every lost-quorum event could be noisy for dashboards. Consider whether leaderKnown == false should need a longer grace, or a lower severity. Please also confirm that isReplicationPathUnprovenSinceRestart() is false for nodes that were never restarted in place, so ordinary startup cannot trigger it.
2. leaderReportedCommitIndex. The hook reads this field from the health thread. Please confirm it is volatile and that a stale or negative value cannot cause a false positive. The predicate guards >= 0, which is good. A short test on the boundary leaderCommitIndex == heldIndex would pin it down.
3. Clock sharing. The tracker uses followerStallClock, which is named for the stall feature. That is fine as a test seam, but a neutral name, or a comment at the field, would avoid surprise if one is changed independently.
4. Alert text. The recommendation literal in addLeaderUnreachableSinceRestartAlert has one very long line, which breaks the wrapping used elsewhere. It also hard-codes the defaults (60s, escalation). Those can drift from GlobalConfiguration, so consider building the text from the config values or dropping the numbers.
5. Flapping. observe resets on any single false tick, so the spell restarts after one good tick. Please make sure a test covers a single false tick resetting the spell.

Tests

Coverage looks reasonable: a unit test for the tracker and path state, ClusterAlertsTest, a schema-matches-handler test, and the #8901 IT. Please confirm there is a test for each of the following:

• alert precedence (stale-term suppresses unreachable, which suppresses stalled)
• the leader-masked -1 path in GetClusterHandler
• isResyncInProgress resetting the tracker

Per CLAUDE.md, check that none of the new tests assert on raw wall-clock time. They should use the injected clock or StallAwareStopwatch.

Minor

• sinceMs is non-volatile, which is fine given the single-writer comment.
• No new dependencies, no security concerns, and no hot-path cost: the work is one cheap call per health tick.

Nothing blocking from my side; point 1 is the one I would settle before merging.

Review of #9451

Overall this is a solid, well-documented change. It adds two node-level fields (localReplicationPathUnproven, localLeaderUnreachableSinceRestart) and one new alert, with clear alert precedence. I read the main-code diff: ClusterAlerts, GetClusterHandler, HealthMonitor, LeaderReachSinceRestartTracker and the RaftHAServer hook. I did not read the test diffs in detail and did not compile or run anything.

Good

• LeaderReachSinceRestartTracker keeps the predicate pure and the state small. The single volatile snapshot is a safe publication for the HTTP readers, and only the health thread writes.
• The hook is placed beside trackFollowerStall(), ahead of the early returns, as the description says. Every exception path resets the tracker, so a Ratis transient can't leave a stale "unreachable" report.
• Alert precedence (stale-term, then leader-unreachable, then stalled) avoids duplicate critical alerts for one condition. Dropping severity to warning when no leader is known avoids noise during a quorum loss.
• details.leaderKnown and replicationPathUnproven let consumers branch on fields instead of prose.

Suggestions (minor)

1. ClusterAlerts.scan now has three overloads that each delegate to the next, with an ever-longer positional parameter list (12 args, several of them booleans and nullable trackers). The next addition will make this worse. Consider a small parameter record, or an options object, for the node-local state (stuckAtStaleTerm, replicationPathUnproven, stalledBehindLeader, leaderUnreachable).
2. leaderReachTracker shares followerStallClock on purpose, and the comment says so. A dedicated field or a rename (e.g. followerTrackingClock) would be less surprising to the next reader, and wouldn't change tests that already set the clock.
3. In trackLeaderReachSinceRestart the catch is Exception, which also swallows unexpected bugs at FINE. That matches isReadyForTraffic(), but a narrower catch (IllegalStateException) would stop genuine defects from being hidden.
4. observe() and reset() assume a single writer, and sinceMs is non-volatile. That is fine today and documented. A @VisibleForTesting note, or an assertion in tests, would protect against a future second caller.
5. The no-leader-known arm is true for any unproven node on a quorum-less cluster. That is acknowledged as a warning. It would help to confirm in a test that the alert clears right away when a leader appears and the path is proven (the reset path), if that isn't already covered.
6. The ClusterStatus OpenAPI schema now requires both fields. Check that no other consumer (Studio, the other HA status clients, the docs) assumes the old shape.

Tests
Unit and IT coverage is present for the alerts, the handler and the schema parity. Please confirm that there is a tracker unit test for the grace boundary (elapsed == graceMs) and for flapping (hold, break, hold restarts the clock). Also confirm that the clock-based tests don't assert on raw wall time, per CLAUDE.md.

Nothing blocking from me.

Review of PR #9451 (diff read only, tests not run). Well-scoped and well-documented; nothing blocking. Items worth a look: (1) the PR description says the new alert is critical, but the code raises critical only when a leader is known and warning when none is, so update the description. (2) On the leaderless arm, localLeaderUnreachableSinceRestart is true on every in-place-restarted node during quorum loss while the alert is only a warning; consider keeping the boolean false there or logging at INFO. (3) leaderReachTracker shares followerStallClock; a neutral name would be less surprising. (4) Note in the tracker Javadoc that observe/reset are single-threaded (sinceMs is a plain long). (5) The two stale-term recommendation strings duplicate much prose; consider shared constants. (6) The IT uses deprecated new URL(String); prefer URI.create(...).toURL() or an existing fixture HTTP helper. (7) Confirm HardcodedTestServerPortsTest stays green with SERVER_LIST in Issue9399ReplicationPathStateTest. (8) The IT adds about grace+2s of waiting; consider @tag("slow") if the grace is large. Positives: thorough tests (predicate, tracker, both arms, grace, resync, closed division, precedence, OpenAPI, real-Ratis idle IT), negligible hot-path cost, no new deps, no new security surface. Items 1 and 2 are the ones I would address before merge.

🤖 Generated with Claude Code

Review

I read the diff only. I did not build the PR or run its tests. The design is clean: the tracker is a small pure class with an injectable clock, alert precedence is explicit, and the OpenAPI schema is updated. Findings:

1. PR description and code disagree on severity. The description calls the new alert "critical", but addLeaderUnreachableSinceRestartAlert raises critical only when leaderKnown is true, and warning otherwise. Please update the description, since anyone alerting on "critical" will miss the no-leader arm.

2. The no-leader arm can fire on healthy or idle nodes. leaderNotReaching returns true whenever the path is unproven and no leader is known, with no evidence about this node's own path. The warning severity and the quorum-loss hint soften this, but localLeaderUnreachableSinceRestart is a bare boolean that is true on both arms. Consumers reading only that field cannot tell direct evidence from a symptom of quorum loss. Consider exposing the leader-known flag at node level, or documenting that the alert details must be read.

3. Staleness of leaderReportedCommitIndex. Please confirm it is reset on a leader change or in-place restart. Otherwise a value from a previous leader or term could cause a false positive for up to the grace period (2x election timeout max). A unit test with a stale index would pin this down.

4. Thread-safety is fine but fragile. sinceMs is a plain field written only by the health-monitor thread, which is correct today. If reset() is later called from elsewhere, it becomes a race. Making it volatile costs nothing, or add a comment.

5. Test coverage. The tracker predicate and grace, alert precedence, schema required fields, and the idle/clearing IT are covered. Missing is an end-to-end test where the alert actually raises with localLeaderUnreachableSinceRestart true. The IT only proves the negative. The Awaitility.during(grace + 2s) adds fixed wall-clock time, so consider @Tag("slow") if the grace is large.

6. Minor. The WARNING log text is long. Consider a one-line log and moving the guidance into docs.

None of these look blocking. Item 1 is a doc fix, and items 3 and 5 are worth a quick check before merge.

Codecov Report

❌ Patch coverage is 83.51648% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.04%. Comparing base (3119a4c) to head (f6e95a9).
⚠️ Report is 14 commits behind head on main.

@@            Coverage Diff            @@
##               main    #9451   +/-   ##
=========================================
  Coverage     77.04%   77.04%           
- Complexity     4004     4046   +42     
=========================================
  Files          2131     2132    +1     
  Lines        215829   215914   +85     
  Branches      46026    46045   +19     
=========================================
+ Hits         166278   166349   +71     
- Misses        31795    31804    +9     
- Partials      17756    17761    +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.