You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When a worker receives multimodal embeddings over NIXL READ, the receiver takes a buffer from its pre-registered pool before it checks the request. A request that passes schema validation but is not a usable READ then fails after the borrow. One example is metadata with a WRITE operation kind. The buffer never returns to the pool and is never tracked as in use, so it is lost. Its recorded size also keeps the request's size instead of the full buffer size. A valid request larger than the pooled buffer drains the pool the same way.
The defect is in Dynamo. Today the vLLM and common multimodal receivers are built with an empty pool, so every request takes the dynamic path and no deployment is known to have hit this.
Reproduction
Without the fix
With the fix
Note
Exact failure on the reported setup
Not run
—
Production receivers use max_items=0, so they have no pool to drain; the failure needs a pooled receiver, and the ticket reports no deployment that showed it.
Exact failure on a similar setup
Not run
—
Needs a GPU and native NIXL for a live encode-to-prefill embedding transfer; the coding run had neither.
Exact error in a test
❌ Exact error
✅ Passed
A real NixlReadEmbeddingReceiver with a one-buffer pool and a mocked NIXL agent, on CPU. Without the fix, a WRITE request raised in begin_read and left the pool empty. Malformed READ requests and an oversized request did the same. With the fix, the pool kept its buffer and original size.
Proposed fix
The receiver now checks the READ contract before it borrows a buffer, with the same checks the multimodal kwargs receiver already runs: READ kind, exactly one remote descriptor, a non-zero pointer, a matching size, a notification key, and agent metadata that decodes to non-empty bytes. These checks also run on the dynamic path. If building the buffer view fails after the borrow, the buffer goes back to the pool with its original size. Once a read has started, nothing changes: a failed read still keeps the buffer out of the pool, because NIXL may still write into it.
Case
Before
After
WRITE or malformed READ metadata, pooled receiver
Error after the borrow; pool loses a buffer
Error before the borrow; pool unchanged
Request larger than the pooled buffer
Error; pool loses a buffer
Error; buffer returned with its original size
Malformed metadata, dynamic path
Error after allocating a buffer
Error before allocating
Valid request
Read succeeds
Unchanged
Read fails after it started
Buffer kept out of the pool
Unchanged
Core change: about 26 lines (+25/−1). The logic is in 1 file, in reading order:
about 26 lines in embedding_transfer.py: Checks the READ metadata before a pooled buffer is borrowed. Returns the buffer to the pool when the view setup fails before the read starts.
The rest of the +165/−8 diff is tests, comments, and re-indentation.
Validation
This PR adds 13 unit tests in one new class, TestNixlReadEmbeddingReceiverPreparation, to components/src/dynamo/common/tests/multimodal/test_embedding_transfer.py. They run
without a GPU, against a mocked native NIXL agent with real CPU buffers.
Behavior checked
Checked by
Without the fix
With the fix
A WRITE or kind-less request is rejected before a buffer is borrowed, and the pool keeps it.
Two cases of a new parametrized unit test; also the reproduction above
❌ Failed: begin_read raised after the borrow, and the pool was left empty
✅ Passed
A request with no, or more than one, remote descriptor, a zero pointer, a size mismatch, or an empty notification key is rejected before the borrow.
Five cases of a new parametrized unit test
❌ Failed: the pool was left empty, or the connector raised or started a transfer after the borrow
✅ Passed
Empty, non-hex, or non-zlib agent metadata is rejected before the borrow.
Three cases of a new parametrized unit test
❌ Failed: pool left empty
✅ Passed
Agent metadata that decompresses to empty bytes is rejected before the borrow.
One case of a new parametrized unit test
❌ Failed: the transfer started after the borrow and the pool was left empty
✅ Passed
A valid request larger than the pooled buffer puts the buffer back with its original size.
New unit test
❌ Failed: pool left empty
✅ Passed
A buffer whose read has begun is kept out of the pool when the read fails.
…ng buffers
NixlReadEmbeddingReceiver.receive_embeddings borrowed a pooled descriptor
before checking that the request described a usable NIXL READ. A malformed
request (for example WRITE metadata, which passes RdmaMetadata validation
but is rejected by begin_read) or a request larger than the pooled buffer
removed the descriptor from the pool and left its _data_size changed.
Check the READ contract (kind, exactly one remote descriptor, non-zero
pointer, matching size, non-empty notification key, decodable agent
metadata) before borrowing, as MmKwargsNixlReceiver does since ai-dynamo#15482.
Return the descriptor with its original size if building the view fails.
Behavior once begin_read is called is unchanged.
Refs DYN-5224
Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com>
👋 Hi glamr-agent! Thank you for contributing to ai-dynamo/dynamo.
Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.
Runs the relevant Python unit tests without requiring a GPU.
Passed (exit 0)
TestNixlReadEmbeddingReceiverPreparation on CPU with a mocked NIXL agent: 12 passed with the fix. Without the fix, the same tests gave 11 failed, 1 passed. Kept here because CI does not repeat the without-fix comparison.
The real NIXL read on a GPU (TestNixlReadEmbeddingTransfer, marked gpu_1) was not run, because the run had no GPU or native NIXL.
Detailed evidence — validation complete
Left out of the table because CI repeats them: builds the changed Dynamo source and imports its compiled extension (Passed, exit 0); lint and formatting of the changed files (Passed, exit 0).
Evidence tally: [3/3 validated]
Advisory AI assessment: sound
Validation result: pass — the new tests fail on main (11 of 12, with the pool drained) and pass with the fix (12 of 12); the gpu_0 file run (20 passed), lint, and the build/import check pass; only the real-NIXL gpu_1 read is left to GPU CI.
Evidence audit: complete [3/3 validated] — the command report comes from recorded runs.
Commands and results [3/3 validated]
Generated from the commands recorded during this run.
Check 1
Builds the changed Dynamo source and confirms that Python can import its compiled extension.
Result: Passed (exit 0)
Command:
Not shown because the exact command contained private run data.
Check 2
Checks the changed files with the repository's fast lint and formatting commands.
The NIXL read receiver now validates READ metadata before buffer allocation. If tensor-view preparation fails, it restores the borrowed descriptor to the warmed pool. New tests cover invalid metadata, oversized requests, and failed reads.
The receiver checks the operation kind, descriptor count and values, notification key, and compressed NIXL metadata before allocating a buffer. Tests verify invalid metadata does not start a read or modify the pooled descriptor.
Descriptor preparation and failure handling components/src/dynamo/common/multimodal/embedding_transfer.py, components/src/dynamo/common/tests/multimodal/test_embedding_transfer.py
If tensor-view preparation fails, the receiver restores the descriptor’s original size, returns it to the warmed pool, and re-raises the exception. Tests cover an oversized request and a failed read.
Empty decoded metadata may still strand a warmed buffer if NIXL rejects it. This pre-existing edge case does not block merging.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: validating NIXL READ metadata before borrowing embedding buffers.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description follows the repository's template in substance. It has the Overview content (failure behavior and proposed fix), Key Changes (the before/after table and core change summary), and Valid…
Full details: Description check
Explanation
The description follows the repository's template in substance. It has the Overview content (failure behavior and proposed fix), Key Changes (the before/after table and core change summary), and Validation (a detailed table of 13 unit tests with results with and without the fix). The Issues and Ongoing Work section is covered by the closing references: it says the PR relates to #15482 and closes DYN-5224. Gaps: the template's 'Closes #XXXX' line uses a Linear ticket rather than a GitHub issue number, and the Pull Request Checklist item ('I used Dynamo's /pr-checklist skill') is not shown as checked. The description also has a count mismatch with the PR objectives: the description says 13 unit tests, while the objectives say 12 tests and 11 of 12 failing without the fix. The description's own table also lists 13 tests but the validation text does not reconcile the number. The description includes a 'Core change: about 26 lines (+25/-1)' note while the raw summary reports +32/-8 for the source file and +128/-0 for tests, which is a different accounting, so the line counts should be reconciled. Overall the description is mostly complete, and these are non-critical issues.
Full details: Docstring Coverage
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Inactive
github-actions
Bot
added
external-contribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Failure behavior
When a worker receives multimodal embeddings over NIXL READ, the receiver takes a buffer from its pre-registered pool before it checks the request. A request that passes schema validation but is not a usable READ then fails after the borrow. One example is metadata with a WRITE operation kind. The buffer never returns to the pool and is never tracked as in use, so it is lost. Its recorded size also keeps the request's size instead of the full buffer size. A valid request larger than the pooled buffer drains the pool the same way.
The defect is in Dynamo. Today the vLLM and common multimodal receivers are built with an empty pool, so every request takes the dynamic path and no deployment is known to have hit this.
max_items=0, so they have no pool to drain; the failure needs a pooled receiver, and the ticket reports no deployment that showed it.NixlReadEmbeddingReceiverwith a one-buffer pool and a mocked NIXL agent, on CPU. Without the fix, a WRITE request raised inbegin_readand left the pool empty. Malformed READ requests and an oversized request did the same. With the fix, the pool kept its buffer and original size.Proposed fix
The receiver now checks the READ contract before it borrows a buffer, with the same checks the multimodal kwargs receiver already runs: READ kind, exactly one remote descriptor, a non-zero pointer, a matching size, a notification key, and agent metadata that decodes to non-empty bytes. These checks also run on the dynamic path. If building the buffer view fails after the borrow, the buffer goes back to the pool with its original size. Once a read has started, nothing changes: a failed read still keeps the buffer out of the pool, because NIXL may still write into it.
Validation
This PR adds 13 unit tests in one new class,
TestNixlReadEmbeddingReceiverPreparation, tocomponents/src/dynamo/common/tests/multimodal/test_embedding_transfer.py. They runwithout a GPU, against a mocked native NIXL agent with real CPU buffers.
begin_readraised after the borrow, and the pool was left emptyRelates to #15482
Closes DYN-5224.
Summary by CodeRabbit