refactor(server): coordinate graph rpc lifecycle

[imbajin]

Purpose of the PR

Graphs added after startup could miss cache RPC registration. Cleanup failures could retain stale owners, and a same-name ADD could be discarded while an older owner was still healthy or closing. Graphs now become visible after RPC registration, unfinished cleanup remains retryable, and received ADDs reload current metadata after the old owner is released.

Main Changes

• Coordinate graph publication, RPC startup and removal by exact owner; retry failed peer cleanup on a subsequent same-name ADD.
• Retain failed consumer cleanup and share close-error aggregation while preserving fatal errors and replacement ownership.
• Complete task deletion when local execution ownership ends, without exposing deleted tasks or losing drain accounting.

Verifying these changes

• Trivial rework / code cleanup without any test coverage. (No Need)
• Already covered by existing tests.
• Need tests and can be verified as follows:

Java 17 formatting and clean reactor compilation passed, together with the affected graph lifecycle, loading-mode, local Gremlin binding and service-name tests. Conflict resolution preserves upstream loading ownership and local Gremlin binding alongside the existing RPC publication and rollback behavior. Unchanged task and Commons RPC code retain their prior complete-suite validation. Independent final reviews passed.

Live PD/etcd networking, physical HStore replacement and cross-Server shared-backend DML remain deployment checks. New-head CI is verified separately.

Does this PR potentially affect the following parts?

• Dependencies
• Modify configurations
• The public API
• Other affects (graph publication, RPC cleanup and task lifecycle)
• Nope

Documentation Status

• Doc - TODO
• Doc - Done
• Doc - No Need

Runtime and retry boundaries: server-runtime.md.

Conceptual PD-enabled lifecycle; the runtime guide defines failure and retry boundaries.

Paired source PR: hugegraph#303

[codecov]

Codecov Report

❌ Patch coverage is 87.98521% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.59%. Comparing base (92af2f8) to head (22ec7bb).
⚠️ Report is 22 commits behind head on master.

Files with missing lines	Patch %	Lines
...n/java/org/apache/hugegraph/core/GraphManager.java	86.29%	15 Missing and 19 partials ⚠️
...n/java/org/apache/hugegraph/StandardHugeGraph.java	83.15%	9 Missing and 7 partials ⚠️
...va/org/apache/hugegraph/rpc/RpcConsumerConfig.java	93.33%	4 Missing and 1 partial ⚠️
...ache/hugegraph/backend/store/raft/RaftContext.java	93.02%	1 Missing and 2 partials ⚠️
...pache/hugegraph/task/DistributedTaskScheduler.java	40.00%	2 Missing and 1 partial ⚠️
...g/apache/hugegraph/task/StandardTaskScheduler.java	84.61%	0 Missing and 2 partials ⚠️
...h/backend/store/raft/RaftBackendStoreProvider.java	94.11%	0 Missing and 1 partial ⚠️
...n/java/org/apache/hugegraph/util/CloseFailure.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3329      +/-   ##
============================================
+ Coverage     54.31%   54.59%   +0.28%     
- Complexity     2250     2280      +30     
============================================
  Files           770      774       +4     
  Lines         67952    68490     +538     
  Branches       9213     9342     +129     
============================================
+ Hits          36905    37392     +487     
- Misses        27470    27566      +96     
+ Partials       3577     3532      -45     

Flag	Coverage Δ
commons	96.37% <95.14%> (?)
hstore	35.48% <18.03%> (?)
pd	49.24% <ø> (?)
server_memory	54.18% <86.30%> (?)
server_rocksdb	42.13% <12.10%> (?)
store	31.98% <ø> (?)

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

[bitflicker64]

Blocking: yes. Summary: Late and lazily loaded graphs now register and export their cache notifier services before they are published, and the create, rollback and restore paths hold up against the code at this head. The rewritten close handling in dropGraph regresses two cases master tolerated: a close failure of another type aborts the rest of a removal batch on peers, and a graph that closed with an error is left registered with no working retry. Evidence: full diff at 11a3cfc (15 files, +1532/-112) read against master 0b7c0de; GraphManager.dropGraph, graphRemoveHandler, graphAddHandler, publishGraph, rollbackGraph, startRpcServer, StandardHugeGraph.close, registerRpcServices, taskScheduler, clearBackend, TaskManager.closeScheduler, RpcConsumerConfig, RpcProviderConfig, RpcServer; sofa-rpc-all 5.7.6 sources for ProviderConfig.setServer, DefaultProviderBootstrap.doExport/unExport, DefaultConsumerBootstrap.unRefer and BoltServer; GraphManagerConfigTest is in UnitTestSuite; all required CI jobs passed at this head (codecov/project is the only failure). I did not run the server unit suite. Cross-server invalidation against a real PD and shared backend was not exercised.

[imbajin]

P2 follow-up — Preserve the local deletion retry path when resource cleanup fails

Reviewed commit: cf26235.

The new cleanup queue in StandardHugeGraph.close() (around lines 1215–1240) retains failed operations, including RPC unregistration, and correctly allows a later close() to retry. The local non-PD DELETE path still calls drop(), whose existing catch (around lines 1283–1292) suppresses close failures once closed() is true, then deletes the configuration. GraphManager.dropGraphLocal() subsequently removes the factory binding.

If RPC unregistration keeps failing, the lenient GRAPH_DROP callback can leave only a closed manager entry behind, while the configuration and factory identity have already been removed. Retrying DELETE enters drop() and fails checkGraphNotClosed() rather than draining the retained cleanup queue. For a provider/native cleanup failure, the remaining manager entry may also be removed after successful unpublication.

The swallowing catch and generic cleanup limitation already exist on the base, so this is a remaining local lifecycle gap, not a claim that the PR newly introduced generic close-error suppression. The new RPC cleanup/retry contract makes it worth covering explicitly. Could we retain the local deletion identity until cleanup completes and add a non-PD persistent-unregister-failure → retry regression, or document that this local error path requires recovery outside DELETE? The new fault-injection cases currently exercise the PD path; the local deletion case covers successful cleanup.

Evidence boundary: independently cross-checked the exact-head source, caller chain, tests, and base implementation. No live failure injection was executed in this review.

[bitflicker64]

Blocking: no. Score: 9/10 (10 - 0.5 Minor duplicated close-failure helper - 0.5 Minor already raised local DELETE retry gap = 9). Summary: Late graphs now register and export their cache notifier services before they become visible, cleanup retries stay on the same owner, and the earlier review points (busy-graph batch abort, closed-owner retry, duplicate server bindings on exportAll, local publish ordering) are addressed at this head. Evidence: read the full diff at cf26235 and the callers in GraphManager, StandardHugeGraph, HugeFactory, TaskManager, RaftContext/RaftNode and the sofa-rpc 5.7.6 / jraft 1.3.9 bytecode for unRefer, AbstractCluster.destroy and RaftGroupService.shutdown; ran ServerClientTest and RpcConfigTest (31 tests, pass) and GraphManagerConfigTest with -P unit-test (42 tests, pass); a probe of RpcConsumerConfig.serviceProxy against an unreachable peer returned in about 1 s, so refer under the GraphManager monitor does not wait for the 20 s connect timeout; all required CI checks pass at this head. Cross-server invalidation against a real PD and shared backend was not exercised.

Already raised: @imbajin (review at cf26235) on the local non-PD DELETE path, where a persistent RPC unregistration failure leaves a closed manager entry that a later DELETE cannot drain.

[bitflicker64]

Blocking: yes. Summary: Late, lazily loaded and PD-added graphs now register and export their cache notifiers before they become visible, and the originating server's clearing DELETE can retry a failed close through the clear certificate. On a PD peer, a close that fails after closed = true now leaves the closed graph published with no retry path. A later graph created under the same name is then never loaded on that peer until a full reload or a restart. Evidence: read the full diff at cf26235 (18 files, +3074/-178) against merge base 92af2f8. Traced GraphManager.dropGraph, graphRemoveHandler, graphAddHandler, reload, graph(String, String), publishGraph, unpublishGraph, rollbackGraph, startRpcServer, GraphsAPI.drop, StandardHugeGraph.close/registerRpcServices/unregisterRpcServices, HugeFactory.open/remove, RaftContext.close, RaftNode.shutdown, RpcConsumerConfig and RpcProviderConfig. Compared master's dropGraph, which logged a close failure on a closed graph and removed the binding. Checked the retry fixtures in GraphManagerConfigTest (around lines 870-944): they retry by calling dropGraph(..., false) directly. All required CI checks pass at this head. I did not run tests: library was unreachable, and the finding follows from static control flow. The collectCloseFailure duplication (open thread) and the non-PD local DELETE retry gap (imbajin's review) are already raised and not repeated here.

[imbajin]

[P2] Keep a stable proxy-to-bootstrap identity across failed cleanup retries.

At this head, removeServiceProxy matches the original proxy against bootstrap.getProxyIns(). In SOFA RPC 5.7.6, DefaultConsumerBootstrap.unRefer() catches an Exception from cluster.destroy() and then clears proxyIns. If the explicit cluster.destroy() in cleanup() also fails, this code retains the bootstrap for retry, but the next removeServiceProxy call with the original proxy no longer matches it and returns false. StandardHugeGraph's cleanup Runnable ignores that boolean and is then removed, so graph-scoped cleanup can report completion while the failed bootstrap is still retained until global destruction.

Please retain a stable original-proxy/bootstrap association until cleanup fully succeeds, and add a regression that retries the same removeServiceProxy after unRefer has cleared proxyIns and cluster destruction initially fails. Existing failure tests retry owner.destroy(), which does not exercise this path. This finding is based on current-head source tracing, not an independently executed native failure test.

Upstream behavior: https://github.com/sofastack/sofa-rpc/blob/v5.7.6/bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java#L228-L256

[imbajin]

Thanks. The SOFA 5.7.6 source path is confirmed, but the native destroy-failure sequence has not been reproduced. Existing serviceId/ConsumerConfig references are shared by multiple proxies, so they cannot safely replace a per-registration identity. Following the agreed scope, this conflict refresh will not introduce a new cleanup registry or a null-proxy matching fallback. I have recorded a separate follow-up to reproduce this sequence with a second live proxy of the same service before choosing a narrow fix. Global provider destruction still retains the bootstrap for retry. Leaving this thread open because the reported failure path is not fixed.

[bitflicker64]

Blocking: yes. Score: 6/10 (10 - 2 Important reload replay - 0.5 Minor already raised consumer proxy identity = 7.5, capped at 6 for a blocking finding). Summary: The delta since cf26235 fixes the earlier peer retry gap and the duplicated close-failure helper, but the new ADD replay keeps a stale pending entry on the server that created a graph, which makes the first graph reload on that server fail. Evidence: read the PR-only delta (cf26235..9c4725d, 969bd33..f403170; the master merges, including b074481, leave GraphManager unchanged) for GraphManager, StandardHugeGraph, RpcConsumerConfig, the Raft close paths, CloseFailure and both task schedulers; skimmed the new test classes. Ran GraphManagerConfigTest (69), StandardTaskAdmissionTest (23), DistributedTaskAdmissionTest (15) with -P unit-test, and RpcConfigTest plus ServerClientTest (38): all pass. A test that creates a graph, delivers its own ADD and calls reload() fails as described inline; the same test without the ADD passes. CI at b074481: several jobs still running, codecov/project fails; no checks are required. Cross-server PD behaviour was not exercised.

Earlier points from this account:

• Closed PD peer owner with no retry path (review at cf26235): fixed in f403170, a failed peer removal is retried on the next same-name ADD.
• collectCloseFailure copied into three classes: fixed in f403170 by org.apache.hugegraph.util.CloseFailure.
• The APPROVE at cf26235 was superseded by the blocking review at the same commit; it should not be read as approval of the current head.

Already raised: @imbajin on RpcConsumerConfig.removeServiceProxy losing the proxy-to-bootstrap match after unRefer clears proxyIns (still open at this head); @imbajin on the local non-PD DELETE retry gap, now fixed by 9c4725d and f403170.

[imbajin]

Addressed the self-ADD/reload finding from this review. The inline reply records the native regression coverage and the fix, including preservation of the existing failed-peer cleanup retry.

[bitflicker64]

Blocking: no. Score: 9.0/10 (10 - 0.5 for the open Minor from @imbajin on RpcConsumerConfig.removeServiceProxy - 0.5 for the Minor below).

The earlier blocking finding from b074481 (the creating server's own PD ADD stayed in pendingGraphAdds, so action=reload republished the graph and timed out) is fixed in 4b2ca10 and 1549a1f. graphAddHandler now consumes the creation marker first, the marker is set before notifyGraphAdd, and notifyGraphAdd has no other caller, so every own ADD is consumed. A failed peer cleanup still takes priority over the marker. ff5926e clears removingGraphs when removeGraphConfig throws, so a later peer REMOVE is still handled. f520505 only simplifies CloseFailure and keeps the identity check.

Tested at f520505: GraphManagerConfigTest (memory backend) 72/72 pass, including testReloadAfterRestCreateAndSelfAdd, testLateSelfAddRetriesFailedPeerRemoval and testMetadataDeleteFailureDoesNotSkipPeerRemove. CI is still queued at this head; master has no required checks.