[pallavithegod]

Main Changes (主要改动)

Close #3229.

• Root cause: HeartbeatService.registerStore() published Up before restoration and graceful shutdown retained stale Up metadata. StoreNodeService.allocShards() and reallocShards() selected renewed Offline leases, while PDService.reportStoreShutdown() lacked caller authorization under the legacy gRPC transport policy.
• Fix details: Restore on an owned worker before Up, renew the recovery lease independently of restore polling, and join producers/workers before shutdown reporting. Separate leased liveness from placement eligibility using the canonical Up state for allocation, replica additions, balancing and capacity checks; retain existing membership during a temporary recovery lease. Back off failed restore retries to the heartbeat interval and log full traces only for the first or changed failure set.
• Fix details: Add a dedicated best-effort ReportStoreShutdown RPC, preserving metadata and the remaining lease while retaining administrative Offline guards/removal. A scoped interceptor requires a privileged internal credential matching auth.secret-key, including before follower forwarding; it fails closed without a configured credential. This shared credential grants internal privileges across Store IDs, rather than authenticating an individual Store. Store reads HG_PD_AUTH_SECRET_KEY; Helm and Compose pass the PD credential. Refused, unreachable or older-PD reports fall back to expiry, never administrative SetStore.
• Fix details: Return authoritative registration state compatibly, suppress false Pending warnings and gate partition pulses on stream startup. Preserve the manual membership/Raft catch-up barrier: Up proves local restoration, not catch-up.
• Tests: StoreRestoreRegistrationTest covers repeated renewal beyond a controlled five-minute lease, no new placement while Offline, eligibility after Up and preservation of existing membership. StoreShutdownAuthenticationTest uses real local gRPC calls for accepted/refused credentials and legacy registration compatibility. StoreRegistrationTest covers backoff, renewal throttling, refused registration, restore completion/reconnect and worker ownership. StoreShutdownReportTest, HeartbeatShutdownTest and StoreHeartbeatAgeTest cover shutdown and diagnostics. The Helm credential contract checks the supplied Secret reference.

Verifying these changes (验证改动)

• Added/updated unit tests in StoreRegistrationTest, StoreRestoreRegistrationTest, StoreShutdownAuthenticationTest, StoreShutdownReportTest, HeartbeatShutdownTest and StoreHeartbeatAgeTest; registered PD coverage in PDCoreSuiteTest
• Verified repository mvn checkstyle:check and mvn apache-rat:check pass locally
• Ran editorconfig:format, clean compilation, 43 focused Store/PD cases and the full pd-core-test profile (169 cases, zero failures/errors, two existing skips) on Java 17
• Helm lint and 310 chart tests pass; both Compose topologies render and pass the credential to every Store
• Doc - Done: PD API reference, Docker and chart operations; paired website PR docs(dist): explain pd and store recovery signals hugegraph-doc#532.

Current-head upstream CI requires maintainer approval. Lease timeout checks use a controlled metadata clock; no live rolling cluster, real Store TTL deployment or matching-release acceptance is claimed.

Visual overview

Conceptual Store lifecycle: renew the live lease during local restoration, require both a live lease and canonical Up for new placement, and stop and join workers before a privileged authenticated best-effort shutdown report. Rejected or unavailable reports fall back to lease expiry. Up proves local restoration, not per-group Raft catch-up; existing leader-transfer target selection has separate limitations.

[bitflicker64]

Blocking: yes. Summary: Reporting Offline now removes the Store from PD's active set at graceful shutdown and again during restore. When pd.initial-store-count equals the Store count, which is what the Helm chart and the 3-store compose file configure, PD then answers every other Store's heartbeat with Cluster_Not_Ready and those Stores reject all writes until the restarted Store registers Up. Before this PR the keepalive entry (300 s by default) kept a quickly restarted Store active. The same Offline registration also runs on every heartbeat-stream reconnect, including after a PD leader change, and one failed partition now stops the remaining partitions from being restored. Evidence: static reading at a63be1c of HeartbeatService.registerStore/reportOffline, StoreNodeService.register/updateStore/heartBeat/checkStoreStatus, PDService.setStore, HgStoreNodeService.addRaftTask, HgStoreEngine.restoreLocalPartitionEngine/createPartitionEngine, PDConfig (initial-store-count, keepAlive-timeout) and helm/hugegraph/templates/pd-statefulset.yaml (HG_PD_INITIAL_STORE_COUNT = store.replicas). No tests or cluster runs were done; the PR has no CI checks reported.

[pallavithegod]

Thanks for tracing these paths. I will preserve the keepalive readiness window during temporary Offline reporting, restore only once per process across reconnects, and attempt every local partition before reporting aggregate restore failure. I will add regression coverage for all three cases.

[imbajin]

Non-blocking test determinism issue at

hugegraph/hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/StoreRegistrationTest.java

Lines 97 to 100 in 9c08bf6

	service.onStateChanged(Metapb.StoreState.Offline);
	service.registerStore();
	Assert.assertEquals(List.of("Up"), events);
	Assert.assertEquals(Metapb.StoreState.Up, service.getStoreInfo().getState());

: please wait until the Offline callback has completed before invoking registerStore(), and await the final Up callback before asserting state. onStateChanged uses Utils.runInThread, so the queued Offline callback can run after the second registration and before the Up assertion. The test can miss the intended reconnect transition or fail nondeterministically. Two independent static reviews agree that the three prior production findings are addressed in this revision; this comment concerns test synchronization, not a newly demonstrated production outage. No independent live-cluster or real-TTL execution was performed.

[imbajin]

Fresh independent cross/adversarial review of the reviewed revision: 7/10 overall. Two independent source reviews confirm that the three prior production findings are repaired. The new P2 detailed inline is a mismatch between graceful shutdown reporting and PD's administrative Offline gate, not a demonstrated new write outage. The separately reported asynchronous Offline-test race remains.

No independent Java reactor run, PD-service integration, live-cluster/real-TTL or rolling-restart execution was performed. Current CI is action_required, with no check runs. Those execution and RC boundaries remain open.

[bitflicker64]

Blocking: yes. Score: 4/10 (10 - 2 Important restore keepalive - 2 Important already raised by @imbajin, Offline report rejected when the cluster is not OK - 0.5 x 3 new Minor - 0.5 Minor already raised by @imbajin, StoreRegistrationTest callback race = 4.0; blocking cap 6 does not lower it further). Summary: The three earlier findings are fixed, but a Store that is back and restoring now never renews its keepalive entry. A failed or slow restore therefore makes every other Store reject writes after the old entry expires, which did not happen before this PR. Your reply on the earlier thread describes this expiry as intended, but for a Store that has already come back it turns one bad partition into a cluster-wide write stop. Evidence: ran mvn -o test -pl hugegraph-store/hg-store-test,hugegraph-pd/hg-pd-test,hugegraph-pd/hg-pd-service -am -Dtest=StoreRegistrationTest,StoreRestoreRegistrationTest,StoreHeartbeatAgeTest,HeartbeatShutdownTest at 9c08bf6: all pass (7, 5, 3 and 8 tests). Findings are from code traces of HeartbeatService.registerStore/reportOffline, StoreNodeService.register/updateStore/heartBeat/checkStoreStatus, PDService.setStore, DefaultPdProvider and HgStoreNodeService.addRaftTask. No cluster or real-TTL run. The PR has no CI checks reported.

Earlier findings from the previous review, checked at 9c08bf6:

• Offline report removing the Store from the active set: fixed in 9c08bf6 for the shutdown report and for restarts shorter than the remaining keepalive; see the new inline comment for the restore case.
• Offline handshake repeated on stream reconnect: fixed in 9c08bf6 (restored flag).
• Restore loop stopping at the first failed partition: fixed in 9c08bf6 (aggregate failure after trying every partition).

Already raised by @imbajin and still open at this head: PDService.setStore rejects the shutdown Offline report whenever the cluster is not Cluster_OK, and the onStateChanged race in StoreRegistrationTest.testReconnectRegistersUpWithoutRestoringAgain.

Shutdown when PD is unreachable does not hang: reportOffline makes at most two blocking calls, each bounded by the PD client deadline (60 s by default), and logs the failure.

[bitflicker64]

Blocking: no. Summary: Two new Minor findings at 9c08bf6: every normal restart now logs a false "Store is not activated" warning from the Offline registration step, and the production state-listener wiring was removed while the recovery shutdown fixture still drives restore through that wiring. The Important and P2 threads already open at this head (restore never renewing keepalive, the Cluster_OK gate in PDService.setStore, the StoreRegistrationTest callback race, and the Minor threads on pdPulse, mixed-version shutdown and administrative Offline) are not repeated here and still apply. Evidence: static reading at 9c08bf6 of DefaultPdProvider.registerStore, PDClient.getStore/ClientCache, HeartbeatService.registerStore/onStateChanged/shutdown, HgStoreEngine.init/stateChanged, StoreNodeService.register and HeartbeatShutdownTest.runInvalidShardRecovery; git grep addStateListener at the head finds no production caller. No tests or cluster runs were done for these two findings; CI for this head is action_required with no check runs.

[pallavithegod]

Thanks for the follow-up reviews and for checking the earlier fixes. I'll address the remaining blocking and minor points once #3229 is assigned to me.

[imbajin]

Fresh independent cross/adversarial review of the current revision: 8.5/10. No new blocking code finding from this pass; recommendation: wait for current-head CI and rolling-update acceptance.

The previously reported shutdown/readiness mismatch is repaired: the dedicated RPC bypasses the administrative readiness gate, while the node transition preserves metadata and the existing lease. Administrative Offline retains its safety check and active-entry removal.

The owned restore worker lets registration keep renewing the returned Store's lease while recovery is blocked or fails, and shutdown joins that worker before closing owners. The callback synchronization, production-path shutdown fixture, authoritative registration response and old-PD fallback also address the earlier review points.

Independent evidence: cloud Java21 method-level probes executed the exact extracted production bodies with dependency stubs: 520 lifecycle assertions across 20 repetitions, 45 shutdown endpoint/state assertions, and a separate 43-assertion client compatibility probe passed. A controlled substitution of the previous registerStore body reproduces its inability to return/renew while restoration is blocked; the current body passes. These are bounded dependency-isolated checks, not a native reactor/JUnit, live-cluster, real-TTL, JDK17 or performance run. The author's 33 focused tests and 161 PD-core cases are separate reported evidence.

Current-head CI remains action_required with zero check runs. A retained active lease means alive, and Up means local restoration; neither proves per-group Raft catch-up or a safe next rolling replacement. Keep the documented PD-first upgrade and manual membership/leader barrier until matching-RC rolling acceptance is demonstrated.

[bitflicker64]

Blocking: no. Summary: The open threads from earlier rounds are addressed at this head: the restoring Store renews its keepalive entry, shutdown uses the new ReportStoreShutdown RPC (no readiness gate, no fallback to SetStore(Offline)), administrative Offline removes the active entry again, partition pulses wait for the stream, the Pending-only warning reads the registration response, and the recovery fixture and reconnect test now go through the production path with proper synchronization. One minor issue remains: a restore that keeps failing is retried in full about once a second with no backoff, and every attempt logs the full aggregate stack trace. Evidence: read the full diff against merge base be68aa2 at 5511593, plus HeartbeatService (registration loop, shutdown, requestExit), HgStoreEngine.init/stateChanged/createPartitionEngine, DefaultPdProvider and FakePdServiceProvider, PDClient (stub deadline, ClientCache), StoreNodeService.register/updateStore/heartBeat/checkStoreStatus, TaskScheduleService.patrolStores, PDService.setStore/getAllStores and the status listeners, the CI profiles that pick up the new tests (store-core-test, pd-core-test), and every existing review thread. No tests were run. CI for this head is action_required with no check runs.

[imbajin]

Blocking: yes. Summary: Restoring Stores remain eligible for shard placement, and the shutdown RPC can change another Store's state without caller authorization. Evidence: static review of StoreNodeService.register, StoreInfoMeta.getActiveStores, StoreNodeService.allocShards, PDService.reportStoreShutdown, and GRpcServerConfig.configure.

[imbajin]

Independent follow-up review for head 5511593ea0508bf6599e907a8d710341c7b1a89f (2026-10-18): 7/10; needs a placement-eligibility fix before merge. This revises my earlier 8.5/10 for the same head based on new evidence.

I independently confirmed the issue already reported in this thread: Offline registration renews the active TTL entry, active-store lookup does not filter readiness, and allocation/reallocation selects directly from that list.

A bounded cloud Java21 probe executing the unchanged getActiveStores, allocShards and reallocShards method bodies passed 15 assertions: two Up Stores plus one restoring Offline Store satisfy a minimum of three and place shards on the Offline Store; an under-replicated group also selects it and emits an adjustment. With a single Offline candidate/minimum of one, it is assigned the leader role. Controls confirmed that existing-group allocation is unchanged and an Up-only placement candidate list excludes the recovering Store/rejects insufficient ready capacity.

Please retain the restart liveness lease, but separate it from readiness for new placement and audit related capacity/balancing consumers. A blanket change to every active-store lookup could alter liveness/expiry semantics. Add a regression that holds restoration incomplete beyond the lease timeout, verifies lease renewal without new placement, then verifies eligibility after restoration succeeds.

Evidence boundary: metadata/TTL retrieval, protobuf/configuration and notification collaborators were stubbed; this was not a live RocksDB/Raft cluster or same-RC test. The demonstrated defect is premature assignment during recovery when a new group or replica addition is needed, not automatic reassignment on every restart or proven data loss. Native CI and rolling-update acceptance remain separate requirements.

[imbajin]

Fresh independent cross/adversarial review of ebdc83863e384e2f0b899d0c64c766ec29ae66c4: 8.5/10; wait for current-head CI and rolling-update acceptance.

The previously confirmed new-placement issue is repaired for new group allocation and replica addition. getPlacementStores() requires a live lease plus canonical Up, including when the retained TTL snapshot still says Up. Replica additions reject insufficient ready candidates before mutation, while temporary restoration does not itself shrink an existing full group. The affected shard-balancing and capacity paths use the same eligibility set. Recovery retry backoff, independent lease renewal and repeated-trace suppression also address the earlier operational feedback.

Independent cloud Java21 checks execute exact extracted methods with lightweight collaborators: 672 lifecycle, 37 placement/capacity/offline-safety and 30 scoped-authentication assertions passed. These are bounded method-level checks, not native JUnit/reactor, real gRPC integration, live TTL/Raft, performance or rolling-restart acceptance. The author's reported suite results are separate evidence.

One scope caveat: the unchanged balancePartitionLeader() target construction and transfer selection still choose existing members without a canonical-readiness filter. This is inherited behavior, distinct from adding new replicas; target selection alone does not demonstrate a successful transfer to an unready Store or an outage. I would add a readiness-target regression before claiming every balancing path excludes restoring Stores.

Current-head CI is action_required with no check runs, not a test failure. A live lease and local Up still do not establish per-group Raft catch-up. Keep the documented manual membership/leader barrier and matching-RC rolling acceptance.

⚠️: CI failed ❕

[imbajin]

CI follow-up at ebdc83863e384e2f0b899d0c64c766ec29ae66c4: the native packaging build now has a confirmed blocker. This qualifies my earlier bounded-method assessment.

PD CI attempt 2 fails compiling the new StoreShutdownAuthenticationTest import and StoreShutdownReportTest field: the service interceptor package and PDService are unavailable on the compile classpath. CodeQL autobuild independently reaches the same errors.

These tests are under hg-pd-test/src/main/java, so -Dmaven.test.skip=true does not bypass their main-source compilation. Logs show hg-pd-service replacing its main artifact with the Spring Boot repackaged archive first; the existing repackage configuration has no classifier. This strongly points to the new service-facing tests exposing an inherited packaging limitation. The identical package command passes in the comparison CI. This is a source/log diagnosis, with no locally reproduced fix yet.

Please move the service-facing regressions into hg-pd-service/src/test/java, update their suite registration, and ensure CI explicitly executes them. Verify a clean full package, the moved authentication/shutdown tests and CodeQL again. No new production-behavior regression is demonstrated here, but this packaging blocker must be fixed before merge; the earlier method-level checks do not establish native reactor acceptance.

[bitflicker64]

Blocking: yes. Summary: The two new PD tests in hg-pd-test/src/main/java import classes from the repackaged hg-pd-service jar, so the repository package build fails at hg-pd-test and every PD/Store CI job at this head stops there. Separately, a shutdown report that reaches a follower PD is forwarded to the leader without its credential, so the new interceptor on the leader refuses it and the Store falls back to lease expiry. The threads open from earlier rounds (placement eligibility for restoring Stores, caller authorization for ReportStoreShutdown, restore retry backoff) are addressed at this head, so they are not repeated. Evidence: read the full diff against merge base be68aa2 plus StoreNodeService.register/reallocShards/heartBeat/checkStoreStatus, TaskScheduleService.patrolStores/balancePartitionShard, PDService.redirectToLeader, PDClient.newLeaderStub, the PD Authentication base class, HeartbeatService and HgStoreEngine, the hg-pd-test and hg-pd-service poms, the Helm _helpers.tpl env checks and pd-store-ci.yml. With JDK 17 at ebdc838, mvn -o test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test passed StoreRestoreRegistrationTest (11), StoreShutdownAuthenticationTest (5) and StoreShutdownReportTest (3), and mvn -o test -pl hugegraph-store/hg-store-test -am -P store-core-test passed StoreRegistrationTest (13) and HeartbeatShutdownTest (8). mvn -o clean package -pl hugegraph-pd/hg-pd-test -am -Dmaven.test.skip=true failed in hg-pd-test with the same two compile errors as CI run 39294815115 (pd_store / pd, pd_store / store, pd_store / hstore, cluster / cluster-test, codeql, dependency-check). A temporary probe test, not kept, ran two local gRPC servers with StoreShutdownAuthentication, the follower forwarding with the same call shape as PDService.redirectToLeader, and got UNAUTHENTICATED with zero leader calls for a client that sent the correct credential. No live multi-PD cluster was run.

[bitflicker64]

Critical: This import, and PDService in StoreShutdownReportTest (line 39), resolve against the hg-pd-service artifact because hg-pd-test compiles its tests as main sources. In a reactor package build that artifact is the Spring Boot repackaged jar, with the classes under BOOT-INF/classes/, so hg-pd-test fails with package org.apache.hugegraph.pd.service.interceptor does not exist and cannot find symbol: class PDService, and the build stops there.

This is why pd_store / pd, pd_store / store, pd_store / hstore, cluster / cluster-test, codeql and dependency-check all fail at this head in run 39294815115, each with exactly these two errors (StoreShutdownAuthenticationTest.java:[29,51], StoreShutdownReportTest.java:[39,13]). None of the new or existing PD and Store tests ran in CI. I reproduced it with mvn -o clean package -pl hugegraph-pd/hg-pd-test -am -Dmaven.test.skip=true on JDK 17: hg-pd-test FAILURE with the same two errors, and unzip -l on the hg-pd-service jar shows BOOT-INF/classes/org/.... The local runs in the PR description pass because mvn test resolves the reactor module to target/classes instead of the repackaged jar.

Please move StoreShutdownAuthenticationTest and StoreShutdownReportTest into hugegraph-pd/hg-pd-service/src/test/java (next to StoreHeartbeatAgeTest and StoreAPIReadyTest), drop them from PDCoreSuiteTest, and confirm the root mvn clean package -Dmaven.test.skip=true passes.

[bitflicker64]

Important: A report that reaches a follower is forwarded without its credential, so the leader refuses it and the Store's shutdown report never takes effect in that case.

StoreShutdownAuthentication validates the credential header on the follower, then this call forwards through PDService.redirectToLeader, which sends channel.newCall(method, CallOptions.DEFAULT) (line 1335) on a bare channel with no interceptor, so the forwarded call carries no credential header. The same interceptor is registered on the leader, sees an empty credential and closes the call with UNAUTHENTICATED. The follower relays that to the Store, HeartbeatService.reportOffline logs a warning, and PD keeps the Store Up until its lease expires, which is the pre-PR behavior this RPC was added to fix.

This is a normal path in multi-PD deployments such as the Helm cluster preset and docker-compose-3pd-3store-3server.yml: PDClient.newLeaderStub (lines 315 to 317) keeps the stub on the first reachable host even when getMembers names a different leader, and any call during a leader change also lands on a follower. I checked the call shape with a probe test (not kept): two local gRPC servers with StoreShutdownAuthentication and the same configured secret, the follower forwarding with ClientCalls.asyncUnaryCall(leaderChannel.newCall(PDGrpc.getReportStoreShutdownMethod(), CallOptions.DEFAULT), ...). A client sending the correct store:<secret> credential to the follower got UNAUTHENTICATED: Shutdown report requires an internal credential, and the leader handler ran zero times. StoreShutdownAuthenticationTest uses one server, so it does not cover this path.

Please carry the credential across the redirect, for example by having the interceptor put the validated header into a gRPC Context key and attaching it to the forwarded call with MetadataUtils.newAttachHeadersInterceptor, or by having the follower sign the forwarded call with its own auth.secret-key. Please add a test with a follower forwarding to a leader, both with the interceptor installed.

[bitflicker64]

Minor: The chart now owns HG_PD_AUTH_SECRET_KEY on the Store container, but the store entry of $reservedEnv in _helpers.tpl (line 948) does not list it, while the pd entry does. A store.extraEnv entry with that name therefore renders after this one and, as the comment above $reservedEnv says, the last duplicate wins, so it silently replaces the chart's Secret reference. A mismatched value only shows up as a warning at shutdown, because the report is refused and the Store falls back to expiry.

Please add HG_PD_AUTH_SECRET_KEY to the store list and a chart test that store.extraEnv with that name fails rendering, like the existing reserved-name tests.

[bitflicker64]

The earlier review comments below still apply at 343c77c; this head does not fix them.

• Critical: This import, and PDService in StoreShutdownReportTest (line 39), resolve against the hg-pd-service artifact because hg-pd-test compiles its tests as main sources. In a reactor package build that artifact is the Spring Boot repackaged jar, with the classes under BOOT-INF/classes/, so hg-pd-test fails with package org.apache.hugegraph.pd.service.interceptor does not exist and cannot find symbol: class PDService, and the build stops there. (fix(store): gate up registration on restore and report offline on shutdown #3362 (comment))
• Important: A report that reaches a follower is forwarded without its credential, so the leader refuses it and the Store's shutdown report never takes effect in that case. (fix(store): gate up registration on restore and report offline on shutdown #3362 (comment))
• Minor: The chart now owns HG_PD_AUTH_SECRET_KEY on the Store container, but the store entry of $reservedEnv in _helpers.tpl (line 948) does not list it, while the pd entry does. A store.extraEnv entry with that name therefore renders after this one and, as the comment above $reservedEnv says, the last duplicate wins, so it silently replaces the chart's Secret reference. A mismatched value only shows up as a warning at shutdown, because the report is refused and the Store falls back to expiry. (fix(store): gate up registration on restore and report offline on shutdown #3362 (comment))

[imbajin]

Blocking: yes. Summary: The exact head still fails compiling its service-facing test imports; this review adds separate shutdown-state, Secret-rotation and shutdown-timeout findings. Evidence: current-head CI annotations report unresolved service classes in hg-pd-test.

[imbajin]

⚠️ Important. Blocking: no. Summary: This updates only the canonical Store record and leaves the TTL active-store entry's serialized state as Up. Evidence: StoreInfoMeta.updateStore writes the canonical key, while getActiveStores reads the separate TTL key and getAllStores(excludeOfflineStores=true) returns it. Please filter active API results against canonical state while retaining the lease for liveness accounting.

[imbajin]

⚠️ Important. Blocking: no. Summary: Store Pods now read the PD auth Secret, but the pod template has no checksum for that Secret, so a Secret rotation leaves already-running Stores with the old value. Evidence: this Secret-backed env is added here; the Store template renders only configured podAnnotations. Please include the resolved PD auth checksum in the Store pod template so rotation rolls Stores and shutdown reports use the new credential.

[imbajin]

⚠️ Important. Blocking: no. Summary: This best-effort report blocks the shutdown hook for the PD client's default 60-second deadline before teardown can continue. Evidence: PDClient.getStub applies PDConfig.grpcTimeOut (60,000 ms) to this call; with an unresponsive PD, this can exceed a shorter termination grace window. Please use a shorter shutdown-specific deadline so lease expiry remains the prompt fallback.