bindings: free the dlist handle wrapper in release_xfer_dlist_handle by EylonKrause · Pull Request #1829 · ai-dynamo/nixl

EylonKrause · 2026-06-24T15:24:51Z

What?

nixl_capi_prep_xfer_dlist (src/bindings/rust/wrapper.cpp) allocates the handle
wrapper with new nixl_capi_xfer_dlist_handle_s, but
nixl_capi_release_xfer_dlist_handle only released the inner handle and never
freed the wrapper:

     try {
         nixl_status_t ret = agent->inner->releasedDlistH(dlist_handle->handle);
+        delete dlist_handle;
         return ret == NIXL_SUCCESS ? NIXL_CAPI_SUCCESS : NIXL_CAPI_ERROR_BACKEND;
     }

Why?

releasedDlistH() deletes the inner nixlDlistH; the nixl_capi_xfer_dlist_handle_s
wrapper that prep new'd is a separate heap allocation that was never freed, so
each prep/release cycle leaks one wrapper. Rust's XferDlistHandle::drop calls release
exactly once and never retries, so an unconditional delete here is the sole, correct
teardown (no double-free).

Reproduction

Extracted LeakSanitizer reproducer of the new wrapper / release-frees-only-inner
shape (-fsanitize=address,leak):

buggy:  SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s)
fixed:  no leak

How (verification)

Confirmed the before/after with the reproducer above.
Compiled the modified wrapper.cpp in-tree with -Dsanitizer=address,undefined
(exit 0).

Related Issues

None.

Summary by CodeRabbit

Bug Fixes
- Fixed a handle cleanup issue so released transfer handles are now properly freed after use, helping prevent memory leaks.

nixl_capi_prep_xfer_dlist allocates the wrapper with `new nixl_capi_xfer_dlist_handle_s`, but nixl_capi_release_xfer_dlist_handle only released the inner handle via agent->inner->releasedDlistH() (which deletes the nixlDlistH) and never deleted the wrapper struct itself. The inner handle and the wrapper are distinct heap allocations, so each prep/release cycle leaked one wrapper. Delete the wrapper after releasing the inner handle. Rust's XferDlistHandle::drop calls release exactly once and never retries, so an unconditional delete here is the sole, correct teardown (no double-free). Signed-off-by: Eylon Krause <eylon1909@gmail.com>

copy-pr-bot · 2026-06-24T15:24:55Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

github-actions · 2026-06-24T15:25:00Z

👋 Hi EylonKrause! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

coderabbitai · 2026-06-24T15:27:08Z

📝 Walkthrough

Walkthrough

In nixl_capi_release_xfer_dlist_handle within the Rust C API wrapper, a single delete dlist_handle statement is added after the call to agent->inner->releasedDlistH(dlist_handle->handle), fixing a memory leak where the nixl_capi_xfer_dlist_handle_s wrapper struct was never freed.

Changes

Wrapper Handle Memory Fix

Layer / File(s)	Summary
Free wrapper handle after `releasedDlistH` `src/bindings/rust/wrapper.cpp`	Adds `delete dlist_handle` so the `nixl_capi_xfer_dlist_handle_s` wrapper object is deallocated after the underlying `releasedDlistH` call in `nixl_capi_release_xfer_dlist_handle`.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

A handle was freed from its duty one day,
But the wrapper still lingered — a ghost in the way.
One delete was missing, a leak in the code,
Now memory travels on down a clean road.
🐇✨ No more stray bytes left to roam!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main fix: freeing the dlist handle wrapper during release.
Description check	✅ Passed	The description matches the template and includes What, Why, and verification details, with only an optional How section.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bindings/rust/wrapper.cpp (1)

1430-1436: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Free the wrapper on the exception path too.

At Line 1431, if releasedDlistH(...) throws, control jumps to catch (...) and dlist_handle is never deleted, so this still leaks on exceptions.

Proposed fix

     try {
         nixl_status_t ret = agent->inner->releasedDlistH(dlist_handle->handle);
         delete dlist_handle;
         return ret == NIXL_SUCCESS ? NIXL_CAPI_SUCCESS : NIXL_CAPI_ERROR_BACKEND;
     }
     catch (...) {
+        delete dlist_handle;
         return NIXL_CAPI_ERROR_BACKEND;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bindings/rust/wrapper.cpp` around lines 1430 - 1436, The exception path
in releasedDlistH handling leaks the dlist wrapper because dlist_handle is only
deleted after the successful call. Update the try/catch around
agent->inner->releasedDlistH so the wrapper is freed even when an exception is
thrown, keeping the cleanup consistent with the success path and ensuring
NIXL_CAPI_ERROR_BACKEND is still returned on failure.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/bindings/rust/wrapper.cpp`:
- Around line 1430-1436: The exception path in releasedDlistH handling leaks the
dlist wrapper because dlist_handle is only deleted after the successful call.
Update the try/catch around agent->inner->releasedDlistH so the wrapper is freed
even when an exception is thrown, keeping the cleanup consistent with the
success path and ensuring NIXL_CAPI_ERROR_BACKEND is still returned on failure.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 2997fe6c-1c44-44bd-a18a-d01169f1a9ff

📥 Commits

Reviewing files that changed from the base of the PR and between a9f456b and f1ef0a3.

📒 Files selected for processing (1)

src/bindings/rust/wrapper.cpp

tomerg-nvidia · 2026-06-24T18:20:43Z

/build

tomerg-nvidia · 2026-06-24T18:22:30Z

/ok-to-test f1ef0a3

iyastreb · 2026-06-26T06:52:41Z

/build

EylonKrause requested review from gleon99, mkhazraee, roiedanino and tomerg-nvidia as code owners June 24, 2026 15:24

pull-request-size Bot added the size/XS label Jun 24, 2026

github-actions Bot added the external-contribution label Jun 24, 2026

coderabbitai Bot reviewed Jun 24, 2026

View reviewed changes

copy-pr-bot Bot temporarily deployed to SWX_AWS June 24, 2026 18:22 Inactive

iyastreb approved these changes Jun 26, 2026

View reviewed changes

Merge branch 'main' into fix/capi-dlist-handle-leak

9ba609f

tomerg-nvidia approved these changes Jun 26, 2026

View reviewed changes

brminich approved these changes Jun 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bindings: free the dlist handle wrapper in release_xfer_dlist_handle#1829

bindings: free the dlist handle wrapper in release_xfer_dlist_handle#1829
EylonKrause wants to merge 2 commits into
ai-dynamo:mainfrom
EylonKrause:fix/capi-dlist-handle-leak

EylonKrause commented Jun 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

copy-pr-bot Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

coderabbitai Bot commented Jun 24, 2026

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

tomerg-nvidia commented Jun 24, 2026

Uh oh!

tomerg-nvidia commented Jun 24, 2026

Uh oh!

iyastreb commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

EylonKrause commented Jun 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

Reproduction

How (verification)

Related Issues

Summary by CodeRabbit

Uh oh!

copy-pr-bot Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

coderabbitai Bot commented Jun 24, 2026

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

tomerg-nvidia commented Jun 24, 2026

Uh oh!

tomerg-nvidia commented Jun 24, 2026

Uh oh!

iyastreb commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EylonKrause commented Jun 24, 2026 •

edited by coderabbitai Bot

Loading