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

[pallavithegod]

Main Changes (主要改动)

Close #3222.

• Root cause: RaftStateMachine.onLeaderStop() replaced a terminal error with follower state, allowing /v1/health to report success after an unrecoverable Raft failure. Queued leader/follower callbacks could also overwrite terminal state.
• Fix details: Preserve error/shutdown for the lifetime of the node and return health 503 for those states, while elections and quorum loss remain live. Reset the probe view and leader term before starting a new node on the same engine, and re-enable listener callbacks. Keep the [Bug] PD reports "Started" and keeps serving after failing to open its RocksDB (lock held by the previous instance): every request fails, /v1/health stays 200, no retry, no exit #3226 pointer and distinguish Raft health from metadata initialization. Clarify the shipped Compose startup checks and the older-image probe workaround.
• Tests: StoreAPIReadyTest covers terminal states, queued callbacks and ordinary elections. RaftEngineReadinessTest.testNewNodeResetAllowsLeadershipAfterShutdown checks reinitialization and listener delivery; the peer-count regression follows the terminal-state contract.

Verifying these changes (验证改动)

• Added/updated unit tests in StoreAPIReadyTest and RaftEngineReadinessTest
• Verified repository mvn checkstyle:check and mvn apache-rat:check pass locally
• Ran editorconfig:format, clean compilation, the full pd-core-test profile (151 cases, zero failures/errors, two existing skips) and nine focused health tests on Java 17
• Doc - Done: PD API reference, Docker guide and chart operations; paired website PR docs(dist): explain pd and store recovery signals hugegraph-doc#532.

Current-head upstream CI still requires maintainer approval to run. Local tests do not establish disk-full/restart or multi-PD acceptance.

Visual overview

Conceptual overview: fatal Raft error and shutdown states remain terminal for the current node and produce health 503 despite late callbacks. A new node resets probe state before startup. Ordinary elections remain live; liveness is separate from readiness.

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

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

[imbajin]

Blocking: no. Summary: The new Docker guidance is not reflected by the shipped Compose readiness dependencies. Evidence: Both exact-head Store dependency examples still gate on the PD /v1/health check; tests were not run.

[imbajin]

Independent follow-up at e7e66661a: 9/10 overall; no new blocking finding.

The new-node reset clears the terminal probe view and leader term and re-enables listener dispatch. Its placement before node start, following the joined shutdown lifecycle, addresses the previously reported singleton-reuse issue without weakening terminal guards on the existing node. Callback draining remains a separate lifecycle step; this is not a claim that the whole PD service is restartable inside one JVM.

Fresh bounded cloud Java21 execution of the exact extracted reset/callback methods passed 13 assertions: terminal guards before reset, uninitialized/nonleader reset, renewed leader notification, callback completion and follower recovery after an error. The no-reset control fails the reset-state assertion. These are method probes with dependency stubs, not the native JUnit/Maven suite or a real Raft restart.

The Docker text now accurately requires an external readiness gate and states that shipped Compose dependencies still use liveness. The metadata note accurately separates startup failure propagation from the outstanding bounded LOCK retry work. These close the documentation follow-ups without implying either Compose quorum gating or full #3226 resolution.

The author's 151-case PD-core and nine focused health runs remain author evidence. Exact-head CI is still action_required with zero check runs. Wait for executed CI and real fault/restart acceptance; this is a COMMENT, not an approval or merge-ready claim.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.43%. Comparing base (92af2f8) to head (e7e6666).
⚠️ Report is 34 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3358       +/-   ##
=============================================
- Coverage     54.31%   37.43%   -16.88%     
- Complexity     2250     6332     +4082     
=============================================
  Files           770      741       -29     
  Lines         67952    64327     -3625     
  Branches       9213     8775      -438     
=============================================
- Hits          36905    24079    -12826     
- Misses        27470    37417     +9947     
+ Partials       3577     2831      -746     

Flag	Coverage Δ
hstore	35.84% <ø> (?)
pd	49.41% <ø> (?)
store	31.93% <ø> (?)

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