Skip to content

fix(rocksdb): synchronize session close and reset - #3298

Merged
imbajin merged 2 commits into
apache:masterfrom
contrueCT:fix/rocksdb-session-reset
Oct 10, 2026
Merged

imbajin merged 2 commits into
apache:masterfrom
contrueCT:fix/rocksdb-session-reset

Conversation

@contrueCT

Copy link
Copy Markdown
Contributor

Purpose of the PR

A request can dispose its native WriteBatch while its session is still registered in the pool. Concurrent graph truncation can retain that session and call reset() on the released native owner. Serialize close/reset and skip reset once the session is closed.

Addresses the native close/reset item already tracked in #3242 and the original review. The release checklist stays open. Existing #3293 handles snapshot recovery and #3294 handles general lease/scan/RPC cleanup; neither currently fixes this batch disposal/reset interleaving.

Main Changes

  • Use the same session monitor for native disposal and reset.
  • Ignore a closed session retained by an in-progress pool iterator.
  • Add a deterministic native regression that pauses after disposal but before close completes, then checks reset waiting and safe reuse of a retained closed reference.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • RocksDBSessionsTest: 20 tests pass on Java 17 with the standard JNI, with no failures/errors/skips.
  • The same new test fails against the original master production class with Reset touched a disposed native batch. The fixture traps the invalid access before JNI; it does not intentionally crash the JVM.
  • Affected Server reactor clean compilation, editorconfig formatting and git diff --check pass.

Real Topling JNI execution was not repeated on this macOS arm64 host; the shared session implementation is exercised with the standard runtime.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects: native session lifecycle synchronization
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need: internal synchronization repair; no configuration or API contract changes.

Serialize native batch disposal with cross-thread reset.
Skip reset after the session has closed.
Cover the disposed-but-registered window with real JNI.
Keep retained closed references safe after pool removal.
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 41.72%. Comparing base (fd64f74) to head (ec4d68d).

Files with missing lines Patch % Lines
...raph/backend/store/rocksdb/RocksDBStdSessions.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3298      +/-   ##
============================================
+ Coverage     41.71%   41.72%   +0.01%     
- Complexity     7036     7038       +2     
============================================
  Files           762      762              
  Lines         67056    67057       +1     
  Branches       9020     9021       +1     
============================================
+ Hits          27970    27978       +8     
+ Misses        35876    35870       -6     
+ Partials       3210     3209       -1     

☔ 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 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: no. Summary: Session close and reset are serialized, and reset safely skips a retained session after its native batch is closed. Evidence: exact-head source review and successful latest-head CI workflows. Score: 8.6/10.

@imbajin
imbajin merged commit 6df8dd9 into apache:master Oct 10, 2026
32 checks passed
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.

2 participants