Skip to content

fix(server): register cache rpc for late graphs - #3329

Open
imbajin wants to merge 7 commits into
apache:masterfrom
hugegraph:fix/server-rpc-late-graph
Open

imbajin wants to merge 7 commits into
apache:masterfrom
hugegraph:fix/server-rpc-late-graph

Conversation

@imbajin

@imbajin imbajin commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Purpose of the PR

Graphs created after startup could miss RPC cache invalidation registration, leaving other Server instances with stale cached data. Export each graph before publishing it and retain its ownership until cleanup actually completes.

Main Changes

  • Serialize RPC export/removal and avoid exporting the same provider configuration twice, while retaining distinct aliases and administrative permission checks.
  • Retry only unfinished graph/native cleanup, drain context executors, and keep a shared Raft listener available to other graphs.
  • Remember successful backend clearing for the exact graph owner. A later clearing DELETE refuses an already-closed owner without that certificate instead of reporting success with old data retained.
  • Continue ordinary peer-removal failures, preserve fatal Error identity, and attempt the remaining resources before propagating cleanup errors. Failures after destructive clearing do not promise rollback.

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 full reactor compilation; full Commons/RPC unit suites and affected graph lifecycle, cache and task-admission tests.
    • Real RocksDB clear/retry, PD removal handler, native Raft listener/node/executor and concurrent RPC publication controls, followed by three independent reviews.

Current-head CI remains a separate gate on both repositories. The native shared-listener close fixture controls a separate duplicate-processor startup defect; production multi-graph startup, broader PD/shared-backend cross-Server DML and complete HTTP authentication remain release validation tasks.

Does this PR potentially affect the following parts?

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

Documentation Status

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

Runtime and retry boundaries: docs/server-runtime.md.

Cache RPC lifecycle

Conceptual PD-enabled lifecycle; the runtime document defines the local-deletion, failure and retry boundaries.

Paired source PR: hugegraph#303

@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.03448% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.09%. Comparing base (92af2f8) to head (cf26235).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...n/java/org/apache/hugegraph/StandardHugeGraph.java 78.00% 10 Missing and 12 partials ⚠️
...n/java/org/apache/hugegraph/core/GraphManager.java 83.06% 14 Missing and 7 partials ⚠️
...ache/hugegraph/backend/store/raft/RaftContext.java 78.78% 5 Missing and 9 partials ⚠️
...h/backend/store/raft/RaftBackendStoreProvider.java 72.50% 4 Missing and 7 partials ⚠️
...va/org/apache/hugegraph/rpc/RpcConsumerConfig.java 76.92% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3329      +/-   ##
============================================
+ Coverage     54.31%   57.09%   +2.78%     
+ Complexity     2250     1409     -841     
============================================
  Files           770      588     -182     
  Lines         67952    56420   -11532     
  Branches       9213     7930    -1283     
============================================
- Hits          36905    32215    -4690     
+ Misses        27470    21025    -6445     
+ Partials       3577     3180     -397     

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

@imbajin
imbajin marked this pull request as ready for review October 13, 2026 16:33
Pengzna
Pengzna previously approved these changes Oct 13, 2026
- register late graph services before publication
- serialize cold startup and graph publication
- remove exact graph-owned RPC resources
- preserve legacy cleanup and verify lifecycle
- enforce the existing ADMIN gate before unregistering
- preserve RPC owners after denied Gremlin cleanup
- isolate event and DB executors in graph tests
- cover null contexts and suite shutdown ordering
- scope trusted lazy graph RPC registration
- serialize graph export and cleanup by service
- retain graph owners when pre-delete cleanup fails
- document cleanup and retry boundaries
- wait for actual listener closure before fixture port reuse
- retain production RPC and graph cleanup behavior
- verify delayed close followed by same-port RPC calls
- retain callback transaction cleanup from original merge
- preserve failed graph rollback owner regression tests
- restore documented RPC deletion and cleanup guarantees
@imbajin
imbajin force-pushed the fix/server-rpc-late-graph branch from 1203616 to e7fb041 Compare October 13, 2026 20:23
register rpc before publishing local graph events
restore retained graph rpc after metadata failure
preserve primary errors and clean partial rpc resources
cover production listener order and deletion failure stages

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- retain graph ownership until native cleanup completes
- drain context executors without closing shared listeners
- reject uncleared closed owners and preserve fatal errors
- deduplicate RPC exports and verify native retry paths

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return this.tx;
}

private static Throwable collectCloseFailure(Throwable first, Throwable next) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: collectCloseFailure and referencesFailure are copied verbatim into three classes: here, RaftBackendStoreProvider (line 195) and RaftContext (line 220).

The three copies implement the same Error-priority and cycle-avoidance rules. A later fix to one copy (for example how a cycle is logged) will not reach the other two, and the close paths would then report failures differently.

Move the two methods into one public helper in hugegraph-core (for example next to LockUtil in org.apache.hugegraph.util) and call it from all three close paths.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

g.close();
} catch (Exception failure) {
// Logical closed does not certify native release; keep this owner until close succeeds.
throw new HugeException("Failed to close graph '%s', please retry later", failure, graphName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: on a PD peer, this rethrow leaves a closed graph published with no way to retry, and the peer then skips any graph created later under the same name.

The peer path is graphRemoveHandler -> dropGraph(gs, name, false) on a grpc thread. Suppose g.close() fails after closed = true, for example in the new unregisterRpcServices step, storeProvider.close() or the Raft executor drain. The HugeException thrown here is caught and only logged in graphRemoveHandler (line 2808). g stays in graphs, in HugeFactory and in the Gremlin bindings, and GRAPH_DROP is never fired.

Nothing on that peer retries the close:

  • the PD remove event has already been consumed;
  • a REST DELETE on the peer passes clear=true (GraphsAPI.drop), and line 2357 refuses a closed owner that has no clear certificate;
  • when the graph is created again under the same name, graphAddHandler sees this.graphs.containsKey(graphName) (line 2749) and skips it, so requests to this peer keep resolving to the closed instance.

Only PUT .../graphs/manage with action=reload (closes and reloads every graph on the server) or a restart recovers. On master the same failure on a closed graph was logged, and the binding and factory entry were removed, so a re-created graph loaded normally.

The new fault-injection tests (for example around lines 911-944) retry by calling manager.dropGraph("DEFAULT", name, false) again. No production caller does that for a peer.

Requested change: give the peer path a real retry trigger. One option: when graphAddHandler (or graph(graphSpace, name)) finds an existing entry that is closed(), run dropGraph(parts[0], parts[1], false) again before deciding to skip, and load the new config once that succeeds. Another option: queue failed peer removals for a background retry. Please add a test that drives graphRemoveHandler with a failing close, then graphAddHandler for the same name, and asserts that the replacement is loaded once cleanup succeeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants