fix(tf): close C++ API sessions#5800
Conversation
Make TensorFlow session ownership failure-atomic across construction, public initialization retries, and destruction. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughTensorFlow C++ and JAX API wrappers now centralize resource cleanup, handle partial initialization, and validate retry and destruction lifecycles through expanded backend tests. ChangesTensorFlow session lifecycle
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant APIWrapper
participant SessionCleanupGuard
participant TensorFlowSession
APIWrapper->>SessionCleanupGuard: guard session during init
APIWrapper->>TensorFlowSession: create and initialize session
TensorFlowSession-->>APIWrapper: success or exception
alt initialization fails
SessionCleanupGuard->>TensorFlowSession: Close()
SessionCleanupGuard->>TensorFlowSession: delete and null pointer
else initialization succeeds
APIWrapper->>SessionCleanupGuard: release()
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Reviewed via the code-review skill. Correct and complete: all four tensorflow::Session* owners now close+delete the session; the SessionCleanupGuard binds session by reference (so it cleans the pointer NewSession later assigns), releases only after inited=true, and its member-declaration order matches the reordered init lists (no -Wreorder). No double-free on the throwing-constructor path (the in-init guard nulls session before the catch block runs), and Close().IgnoreError() is the right cleanup-path call. Nicely, unlike a pure-UB fix, this genuinely fails pre-fix under CI's -fsanitize=leak leg, which does run runUnitTests_cc, and all four wrappers create a session before the forced failure so the leak is LSan-visible. Long-overdue fix (PR #3062 fixed the analogous graph_def leak in 2023 but never the session).
Two non-blocking notes:
- Same leak family, still open elsewhere: DeepPotJAX (source/api_cc/src/DeepPotJAX.cc) has the identical leak-on-throwing-init pattern with TF C-API handles (TF_Session/TF_Graph/TFE_Context/TF_Function) -- its multi-arg constructor has no try/catch and ~DeepPotJAX only frees resources if inited==true, so a throw after TF_LoadSessionFromSavedModel but before inited=true leaks everything. Out of #5657's scope (different handle type), but worth a follow-up issue.
- The new lifecycle test covers only the failed-init/retry path; the successful-init-then-normal-destroy path (the headline #5657 scenario, which exercises the destructor's Close() on a LIVE session) is only covered implicitly by the pre-existing functional tests under the same LSan leg -- a direct case would make that explicit. Also worth calling out in the PR description that this is the first test to white-box-link deepmd_backend_tf into runUnitTests_cc rather than going through the dlopen plugin facade. LGTM.
Make DeepPotJAX initialization failure-atomic by releasing every partially created TensorFlow C API handle, and add direct failed and successful lifecycle coverage for the JAX and TensorFlow backends. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Thanks for the non-blocking follow-up notes. I addressed both of them in
I rebuilt Coding agent: Codex |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5800 +/- ##
==========================================
- Coverage 79.85% 77.73% -2.12%
==========================================
Files 1022 1052 +30
Lines 117351 120707 +3356
Branches 4313 4363 +50
==========================================
+ Hits 93706 93835 +129
- Misses 22101 25319 +3218
- Partials 1544 1553 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4a0b95d
Closes #5657.
Summary
Session*member tonullptrinDeepPotTF,DeepSpinTF,DeepTensorTF, andDipoleChargeModifierTF;Close()followed bydeleteand pointer reset;init()calls are failure-atomic and can be retried without overwriting a session left by an earlier failed attempt;DeepPotJAXinitialization failure-atomic by unconditionally releasing all partially created TensorFlow C API handles (TF_Session,TF_Graph,TF_SessionOptions,TF_Status,TFE_Context,TFE_ContextOptions, andTF_Functionreferences);Root cause
The TensorFlow wrappers owned raw sessions created by
NewSession, but their destructors and initializing-constructor catch blocks only deleted theGraphDef. Default construction followed by a failing publicinit()could also retain a partially initialized session, allowing a retry to overwrite the pointer.DeepPotJAXhad the same failure mode with TensorFlow C API handles: its destructor only released resources wheninited == true, so an exception before the final initialization commit leaked every handle created up to that point.The TensorFlow rollback guard now owns cleanup until initialization has completely committed.
DeepPotJAX::clear_tf_resources()is null-safe for partial initialization and is called both from the exception path and the destructor. Successful initialization commits only after all metadata has been loaded.Test coverage
The lifecycle test intentionally white-box-links
deepmd_backend_tfanddeepmd_backend_jaxintorunUnitTests_cc, rather than going through thedlopenplugin facade. This makes the concrete backend destructors directly visible to the LSan test process.The tests cover:
DeepPotJAXinitialization;DeepPotJAXinitialization followed by normal destruction when the generated SavedModel test artifact is available.Ordinary builds verify retry safety and destructor behavior; the CI leak-sanitizer leg provides the direct leak regression signal.
Validation
deepmd_backend_tf,deepmd_backend_jax,runUnitTests_cc, anddeepmd_opin a TensorFlow/JAX C++ test configuration with LAMMPS plugin mode disabled;TestTensorFlowSessionLifecycle.*: 3 passed, 1 skipped locally because the generated JAX SavedModel artifact was unavailable;ruff check .;ruff format .;git diff --check.Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh