Skip to content

fix(server): close shortest path edge iterators - #3167

Open
contrueCT wants to merge 2 commits into
apache:masterfrom
contrueCT:task/fix-shortest-path-scanner-close
Open

fix(server): close shortest path edge iterators#3167
contrueCT wants to merge 2 commits into
apache:masterfrom
contrueCT:task/fix-shortest-path-scanner-close

Conversation

@contrueCT

Copy link
Copy Markdown
Contributor

Purpose of the PR

ShortestPathTraverser could return after finding a path without closing the current backend edge iterator. With HStore, the abandoned iterator could leave its remote scanner active and Store threads waiting to deliver pages that the client no longer consumed.

The same ownership gap also existed in the backward search and in the iterator used by the super-node probe.

Main Changes

  • Retain the original iterator returned by edgesOfVertex() in both forward() and backward().
  • Close that source iterator in finally, including early-return and exception paths. The source iterator is closed directly because skipSuperNodeIfNeeded() may consume and replace it.
  • Close the iterator used by superNode() after counting it.
  • Add focused regression coverage for forward match, backward match, and super-node probe cleanup, and register it in UnitTestSuite.

The change does not modify traversal semantics, degree/capacity limits, request timeouts, HStore code, configuration, or public APIs.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • On unpatched apache/master, the new test preserves the expected path results and fails all three iterator-close assertions: Tests run: 3, Failures: 3.
    • On this branch, ShortestPathTraverserTest passes: OK (3 tests).
    • MAVEN_OPTS='-Xms64m -Xmx256m' mvn clean compile -pl hugegraph-server/hugegraph-test -am -Dmaven.javadoc.skip=true completes with BUILD SUCCESS for all 21 affected reactor modules.
    • mvn editorconfig:format completes with BUILD SUCCESS, and git diff --check reports no errors.

Additional HStore runtime validation of the same production patch used one PD, one Store, and one Server with the full SNAP Twitter-2010 graph (41,652,230 vertices and 1,468,365,182 directed edges). The bounded shortest-path gate completed 1,516/1,516 warmup and 5,791/5,791 measured requests with zero errors, zero swap, stable service PIDs, and zero blocked HgChannel.send frames at all sampled points through 600 seconds.

Does this PR potentially affect the following parts?

Documentation Status

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

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working tests Add or improve test cases labels Aug 23, 2026
Close the original backend iterator on forward, backward, and super-node traversal paths so early returns cannot leave HStore scanners active. Add focused iterator-lifecycle regression coverage.

Fixes apache#3166
@contrueCT
contrueCT force-pushed the task/fix-shortest-path-scanner-close branch from 274433e to 1a08017 Compare August 23, 2026 17:45

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The new cleanup covers iterators after acquisition, but multi-label acquisition can still leak already-opened scanners, and the regression tests do not exercise the threshold, wrapper, or exception paths. Evidence: exact-head ShortestPathTraverser/HugeTraverser control flow, TinkerPop CloseableIterator behavior, and the added ShortestPathTraverserTest.

@contrueCT contrueCT changed the title fix(core): close shortest path edge iterators fix(server): close shortest path edge iterators Aug 24, 2026
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 24, 2026
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.60606% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.15%. Comparing base (431f6e6) to head (74e8701).

Files with missing lines Patch % Lines
...aph/traversal/algorithm/ShortestPathTraverser.java 65.90% 12 Missing and 3 partials ⚠️
...e/hugegraph/traversal/algorithm/HugeTraverser.java 50.00% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3167      +/-   ##
============================================
- Coverage     41.06%   36.15%   -4.92%     
+ Complexity      519      426      -93     
============================================
  Files           771      771              
  Lines         65962    65987      +25     
  Branches       8766     8767       +1     
============================================
- Hits          27088    23857    -3231     
- Misses        36008    39466    +3458     
+ Partials       2866     2664     -202     

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

@contrueCT

Copy link
Copy Markdown
Contributor Author

Codecov status clarification

The reported 4.92 percentage-point project coverage drop is caused by incomplete coverage aggregation for the PR head, rather than by this PR removing coverage from thousands of existing lines.

The Codecov steps for Commons, PD, Store, HStore, Server RocksDB, and Server HBase all found coverage files but then logged an empty token and HTTP 429 rate-limit errors. The uploader exits with status 0 by default, so the corresponding GitHub Actions jobs still appear green. Only the Server memory upload was accepted.

Relevant logs:

Accordingly, codecov/patch passes at 60.60% against a 41.06% target, while codecov/project fails because it compares the single-session head report with a five-session base report. The 26 missing lines shown in the bot comment are therefore based on an incomplete coverage set and should not be treated as the complete patch-coverage result.

This does not remove the need to review exception-path coverage independently. The new ShortestPathTraverserTest is included in UnitTestSuite, and the CI unit suite completed 629 tests with zero failures. The reviewer threads covering multi-label acquisition cleanup, primary/suppressed exceptions, wrapper cleanup, backward traversal, and super-node traversal are resolved.

Related repository-level JaCoCo aggregation work is tracked in #3160 and #3161. The anonymous-upload 429 failures across the other workflows are additional CI infrastructure evidence, so I am not changing the functional implementation solely in response to this incomplete Codecov report.

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

Labels

bug Something isn't working size:XL This PR changes 500-999 lines, ignoring generated files. tests Add or improve test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ShortestPathTraverser does not close edge iterators on early return

2 participants