fix(pd): report fatal raft errors through health probes

[pallavithegod]

Main Changes (主要改动)

Close #3222.

• Root cause: RaftStateMachine.onLeaderStop() overwrote the terminal error announced by onError() after a failed snapshot. StoreAPI.checkHealthy() always returned HTTP 200, leaving an unrecoverable PD alive indefinitely.
• Fix details: retain STATE_ERROR during step-down and return HTTP 503 from /v1/health for error or shutdown states. Preserve the empty text body and HTTP 200 during ordinary elections or quorum loss. Probes continue to use the cached state without acquiring the Raft node lock. Document restart requirements and retain the chart workaround for older images.
• Tests: extend StoreAPIReadyTest with fatal-error/step-down, quorum-loss liveness, and shutdown cases. All 6 tests pass.

Verifying these changes (验证改动)

• Added/updated unit tests in StoreAPIReadyTest
• Verified mvn checkstyle:check and mvn apache-rat:check pass locally

Validation used Java 17 and Maven 3.9.11:

• mvn apache-rat:check
• mvn editorconfig:format
• mvn clean compile -Dmaven.javadoc.skip=true -pl hugegraph-pd/hg-pd-service -am
• mvn test-compile checkstyle:check -pl hugegraph-pd/hg-pd-service -am -Dcheckstyle.config.location=/pd-health/style/checkstyle.xml
• mvn test -pl hugegraph-pd/hg-pd-service -am -Dtest=StoreAPIReadyTest -Dsurefire.failIfNoSpecifiedTests=false

The explicit Checkstyle configuration selects the repository's style/checkstyle.xml; otherwise the PD modules use the plugin's default rules when the goal is invoked directly.

Website documentation: apache/hugegraph-doc#532. Coordinate these merges.

[bitflicker64]

Blocking: no. Summary: The fix does what #3222 asks: onLeaderStop no longer overwrites the STATE_ERROR that onError set, and /v1/health now answers 503 for STATE_ERROR and STATE_SHUTDOWN while staying 200 through elections and quorum loss. One minor problem: the chart's own description of the liveness derivation still says /v1/health never consults raft. Evidence: read the full diff at 2aebabd and the surrounding RaftStateMachine, RaftEngine.getRaftStatus(), StoreAPI, the chart helper and the chart tests. onStopFollowing keeps the previous state and an errored node is no longer active, so no later callback brings back STATE_FOLLOWER. RaftEngine.shutDown() is only called from ShutdownHook, so the new 503 for STATE_SHUTDOWN only shows while the process is exiting. Health still reads the cached probe view and never takes the node lock. Ran mvn test -pl hugegraph-pd/hg-pd-service -am -Dtest=StoreAPIReadyTest on JDK 17 at this head: 6 tests, 0 failures. No CI has run on this head yet: the 'HugeGraph-Server CI' run is action_required, waiting for a maintainer to approve it.

[pallavithegod]

Thanks for the review. I will align the chart helper, values, and test comments with the new health contract and make the older-image qualification explicit.

[imbajin]

Fresh independent cross/adversarial review of the reviewed revision: 7/10 overall. Two independent full source reviews identify the new P2 detailed inline: already-queued follower callbacks can overwrite terminal probe state, leaving the fatal-health contract incomplete. The previous chart-comment repair is retained.

Evidence includes bounded Java 21 execution of the unchanged extracted callback bodies and a source-verified legal queue ordering in pinned JRaft 1.3.14. The harness uses dependency stubs; this is not a full PD/JRaft race reproduction. Current CI is action_required, with no check runs. Full runtime/rollout and RC acceptance remain open.

[imbajin]

[P2] Keep the terminal error sticky across already-queued callbacks

This guard only protects onLeaderStop; onStartFollowing still unconditionally replaces the probe with STATE_FOLLOWER. In the pinned JRaft 1.3.14 dependency, SnapshotExecutorImpl.reportError queues the FSM error before the node becomes terminal. A concurrent higher-term AppendEntries can therefore queue LEADER_STOP and START_FOLLOWING behind it while the node is still active. After ERROR calls this state machine's onError and Node.onError, the queued START_FOLLOWING is dispatched without an error-status guard. It overwrites STATE_ERROR; the subsequent onStopFollowing retains STATE_FOLLOWER, so /v1/health returns 200 for the terminally failed node.

Please preserve terminal state across the follower/leader transition callbacks, and add a regression for ERROR → LEADER_STOP → START_FOLLOWING → STOP_FOLLOWING. Two independent source reviews confirmed this ordering. A bounded Java 21 harness using the unchanged extracted callback bodies produced STATE_ERROR/503 for the existing sequence and STATE_FOLLOWER/200 for the queued-following sequence; dependencies were stubbed, so this is not a full JRaft/PD race test.

[bitflicker64]

Score: 6/10. Blocking: yes.

The main path works at ae49a99: after onError, onLeaderStop keeps STATE_ERROR, and /v1/health answers 503 for STATE_ERROR and STATE_SHUTDOWN while staying 200 through elections and quorum loss. The score is 10, minus 2 for the open Important below, capped at 6 because that item blocks.

Earlier threads:

• My Minor on helm/hugegraph/values.yaml:180 (chart comments still said /v1/health never consults raft): fixed in ae49a99. _helpers.tpl, values.yaml and the chart test comment now limit that wording to older images and use the same TODO condition.
• @imbajin's Important on RaftStateMachine.java:218 (a queued START_FOLLOWING overwrites STATE_ERROR): still open. I traced jraft 1.3.14 and agree. FSMCallerImpl dispatches LEADER_START and START_FOLLOWING without checking the error status. The same window opens when onApply fails mid-batch, because setError runs on the FSM thread while earlier transition tasks are still queued. A queued LEADER_START from a timed-out transferLeadershipTo (PDService.updatePdRaft) also overwrites STATE_ERROR. The LEADER_STOP that Node.onError queues behind it then leaves STATE_FOLLOWER with no leader, so /v1/health stays 200 and /v1/ready returns 503. A normal election cannot do this because its leader start runs inside doCommitted, which returns early once an error is set. Making STATE_ERROR sticky in onLeaderStart and onStartFollowing covers both cases.

Checks: StoreAPIReadyTest passes at this head on JDK 17 (6 tests, 0 failures), and helm unittest on tests/pd_readiness_path_test.yaml passes (8 tests).

Evidence gaps: the only CI run on this head is HugeGraph-Server CI, which is waiting for approval, so no PD CI has run. There was no live fault run, such as a snapshot save failing on a full disk.

Note on deterministic failures: if the same log entry fails on every replica, each multi-replica PD now returns 503, is restarted, replays the entry and fails again, ending in CrashLoopBackOff. Before this PR that cluster was just as unusable but reported healthy, so this is not a regression.

[pallavithegod]

Thanks for tracing the queued-callback case and confirming the earlier comment corrections. I'll follow up on the terminal-state guard once issue #3222 is assigned to me.

[imbajin]

Blocking: yes. Summary: A queued follower transition can still overwrite STATE_ERROR, so a terminally failed PD may answer 200 from /v1/health; two operator guides also remain inconsistent with this endpoint contract. Evidence: existing exact-head finding at #3358 (comment); exact-head docs at hugegraph-pd/README.md:311 and helm/hugegraph/docs/operations.md:127.

[imbajin]

🧹 Minor. Blocking: no. Summary: This table now documents /v1/health as returning 503 for a terminal Raft error or shutdown, but hugegraph-pd/README.md:311 and helm/hugegraph/docs/operations.md:127 still describe it as confirming only a live listener. Please update both descriptions to clarify that elections and temporary quorum loss remain healthy while terminal error or shutdown returns 503. Evidence: those exact-head operator docs contradict the API contract added here.