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

[pallavithegod]

Main Changes (????)

Close #3222.

• Root cause: queued leader/follower callbacks could overwrite the terminal error announced by RaftStateMachine.onError() after a failed snapshot. StoreAPI.checkHealthy() always returned HTTP 200, leaving an unrecoverable PD alive indefinitely.
• Fix details: retain terminal error and shutdown states across step-down, leader-start and start-following callbacks 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. Add queued follower, queued leader-start and shutdown transition regressions. All 9 tests pass.

Verifying these changes (????)

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

Validation used Java 17 and Maven 3.9.11:

• mvn -B -q apache-rat:check
• mvn -B -q editorconfig:format
• mvn -B -q clean compile -Dmaven.javadoc.skip=true -pl hugegraph-pd/hg-pd-service -am
• mvn -B -q test-compile checkstyle:check -pl hugegraph-pd/hg-pd-service -am -Dcheckstyle.config.location="$PWD/style/checkstyle.xml"
• mvn -B -q 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.

Follow-up validation: updated RaftEngineReadinessTest to keep terminal peer counts unknown after a queued leader start. The full pd-core-test profile passes: 150 cases, 0 failures/errors, 2 existing skips; all 9 focused StoreAPIReadyTest cases also pass.

Command: mvn -B -q test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test. MAVEN_OPTS=-Xmx2048m -XX:MaxMetaspaceSize=512m; no parallel Maven processes.

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

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

Fresh independent cross/adversarial review of this revision: 9/10; waiting for current-head CI.

The earlier queued-callback blocker is addressed. The leader/follower callbacks now preserve both terminal ERROR and SHUTDOWN without restarting leader notifications. The two previously inconsistent operator guides are also corrected, and the legacy-image Helm workaround is retained.

Independent bounded Java21 verification ran all nine checked-in test methods against unchanged extracted callback/status/endpoint bodies with dependency stubs: 9/9 pass. Replacing only the FSM bodies with the previous revision makes exactly the three new queued-transition regressions fail; the other six still pass. A separate extracted-callback harness checked all 335,923 serial sequences of up to seven events; terminal-state invariants passed on this version while 260,476 old-version sequences violated them. Pinned JRaft 1.3.14 dispatches these callbacks serially. This is deterministic bounded model evidence, not native fault injection. No new blocker was confirmed.

Evidence boundary: this is not a native Maven/PD, real HTTP, disk-full/restart or multi-PD fault run, and it does not establish performance or same-RC acceptance. Current-head CI is awaiting approval with no executed checks at review time. Please complete that CI before treating the change as merge-ready; the old score and earlier runs are historical only.

[bitflicker64]

Blocking: yes. Summary: The latest commit closes both earlier threads: onLeaderStart and onStartFollowing now leave STATE_ERROR and STATE_SHUTDOWN in place, and hugegraph-pd/README.md and helm/hugegraph/docs/operations.md now describe the new /v1/health contract. The new onLeaderStart guard breaks an existing PD core test, RaftEngineReadinessTest.testAlivePeerCountStopsAfterALeaderErrorsOrShutsDown, which still expects a leader start after onError to bring leadership back. That test runs in the pd-core-test CI step. Evidence: read the full diff against merge base d505f6f, plus RaftStateMachine, RaftEngine.getRaftStatus()/refreshAlivePeerCount()/init()/shutDown(), StoreAPI.checkReady(), PDCoreSuiteTest and the CI workflow. On JDK 17: mvn test -pl hugegraph-pd/hg-pd-service -am -Dtest=StoreAPIReadyTest passes (9 tests). mvn test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test fails at 5c30279 (150 run, 1 failure, expected:<2> but was:<-1> at RaftEngineReadinessTest.java:221). The same test passes at merge base d505f6f (12 run, 0 failures). No PD CI has run on this head: the only workflow run, HugeGraph-Server CI, is action_required.

[imbajin]

Follow-up independent review of the current revision: 9/10; waiting for current-head CI.

The PD-core test failure reported after my previous review is addressed in the updated regression. A queued leader-start cannot recover a terminal node, so its peer count correctly remains unknown and only the initial leader phase reads the native peer list.

I verified that the parent-to-current delta is test-only and the production code is unchanged. Fresh bounded Java21 execution passes the nine health test methods and the changed peer-count sequence with a counting Node stub; restoring the old expected count reproduces the assertion failure. My previous nine-case harness did not cover the native PD-core suite.

The author's reported 150-case PD-core run remains author-provided evidence; I have not independently run that full Maven suite. Current-head CI still awaits approval, with no executed checks. Real fault/restart, multi-PD and same-RC acceptance remain unverified.

[bitflicker64]

Blocking: no. Summary: The fix does what #3222 asks at this head: onLeaderStop, onLeaderStart and onStartFollowing keep STATE_ERROR and STATE_SHUTDOWN, and /v1/health answers 503 for both while staying 200 through elections and quorum loss; the updated RaftEngineReadinessTest expectation matches the new contract. Two Minor items remain: the terminal guard also survives RaftEngine.shutDown() plus init() on the same singleton, which PDCoreSuiteTest does for every PDCoreTestBase class, and the removed TODO drops the only code pointer to the still-open #3226. Evidence: read the full diff against merge base d505f6f and, at e785881, RaftStateMachine, RaftEngine (constructor, init(), shutDown(), getRaftStatus(), refreshAlivePeerCount()), StoreAPI.checkHealthy()/checkReady(), StoreAPIReadyTest, RaftEngineReadinessTest, PDCoreTestBase, PDCoreSuiteTest, the onRaftLeaderChanged listeners, and every /v1/health consumer (Dockerfile, compose files, Helm templates and tests). Static review only: both heavy-test hosts had the shared test lock held, and neither finding needs a run to confirm. No CI has run on this head: the only workflow run, HugeGraph-Server CI, is action_required.

[bitflicker64]

Minor: isTerminal() makes STATE_SHUTDOWN permanent for this RaftStateMachine object, but the object outlives the raft node. RaftEngine creates it once in its constructor (RaftEngine.java:88), and init() hands that same object to every new node (nodeOptions.setFsm(stateMachine), line 122). shutDown() sets raftNode to null, so a later init() starts a new node (the guard at line 96 passes), but probeView still holds STATE_SHUTDOWN from the old one. On the new node onLeaderStart and onStartFollowing return early. leaderTerm stays -1, /v1/health and /v1/ready keep answering 503, and no state listener runs.

PDCoreSuiteTest hits this today. PDCoreTestBase calls init() on the singleton in @BeforeClass and shutDown() in @AfterClass, so from IdServiceTest onward a leader start no longer runs ConfigService.onRaftLeaderChanged (loadConfig()) or PartitionService.onRaftLeaderChanged (partitionMeta.reload()). Those suites pass on a PD that reports itself shut down. A production PD does not restart the engine inside one JVM, so this is a test-fidelity problem and a latent trap, not a live bug.

Requested change: scope the terminal guard to the node that reported it. Reset the probe view to STATE_UNINITIALIZED and leaderTerm to -1 in RaftEngine.init() before raftGroupService.start(false), for example through a package-private RaftStateMachine reset method. Add a RaftEngineReadinessTest case: after onShutdown() and the reset, onLeaderStart(7) reports STATE_LEADER.

[bitflicker64]

Minor: This hunk deletes the whole TODO, including the half for #3226, and that issue is still open. #3226 covers a PD whose metadata RocksDB never opened because the previous instance still holds its LOCK. That failure never reaches a raft callback, and checkHealthy() reads only the raft probe view, so such a PD still answers 200 here indefinitely. With the TODO gone, nothing in the code points at #3226. hugegraph-pd/docs/api-reference.md now describes /v1/health as "the REST listener is up and Raft has no terminal error", which reads as a complete liveness check.

Requested change: keep a TODO above checkHealthy() for the store-open case, with the #3226 link. In the api-reference row, or the paragraph after it, say that a PD whose metadata store failed to open still answers 200 until #3226 is fixed.

[imbajin]

A current-revision clarification on the metadata-store note: HgKVStoreImpl.init() already throws on open failure and retains the explicit #3226 TODO for bounded LOCK retry. MetadataFactory closes/rethrows failed initialization, and PDService's startup hook cleans up and propagates the failure. Therefore the historical 'failed-open PD keeps serving health=200 indefinitely' description should not be carried forward as established behavior at this revision.

This is an independent static verification of the startup path, not a held-LOCK JVM reproduction or same-RC acceptance; #3226 remains open, including its bounded-retry work. I also corroborated the separately reported same-JVM FSM reinitialization caveat with bounded callback execution; it remains a nonblocking test-fidelity gap, not evidence of a normal process-restart failure. My independent score remains 9/10, waiting for current-head CI.