fix: handling 0 block transfer #3613

oandreeva-nv · 2025-10-14T18:20:12Z

Overview:

fixes DIS-825

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

closes GitHub issue: #xxx

Summary by CodeRabbit

New Features
- Transfer operations now immediately no-op (and warn) when both source and target lists are empty, improving responsiveness.
Bug Fixes
- Transfer requests with mismatched source/target counts now return a clear error instead of proceeding unpredictably.
Improvements
- More robust input validation for transfers across logical and distributed paths, reducing unnecessary work and improving stability.

copy-pr-bot · 2025-10-14T18:20:26Z

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.

Signed-off-by: Olga Andreeva <[email protected]>

oandreeva-nv · 2025-10-14T18:21:42Z

/ok to test 062394f

coderabbitai · 2025-10-14T18:22:29Z

Walkthrough

Introduces early input validation for transfer operations: returning an immediate completion when both sources and targets are empty, and producing a CountMismatch error when their counts differ. Implemented across logical, distributed leader/worker, and local transfer layers without changing function signatures or downstream behavior for valid inputs.

Changes

Cohort / File(s)	Summary
Transfer validation guards `lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs`, `lib/llm/src/block_manager/block/locality.rs`, `lib/llm/src/block_manager/block/transfer.rs`	Added early checks: skip transfer with immediate oneshot completion when sources and targets are both empty; return TransferError::CountMismatch when counts differ; preserved existing flow for valid, non-empty inputs.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant C as Caller
    participant L as Logical::handle_transfer
    participant D as DistributedLeaderWorkerResources::handle_transfer
    participant T as handle_local_transfer

    C->>L: handle_transfer(sources, targets)
    alt Empty sources AND targets
        L-->>C: Immediate oneshot completion (skip)
    else Count mismatch
        L-->>C: Error(TransferError::CountMismatch)
    else Valid counts
        L->>D: handle_transfer(...)
        alt Empty sources AND targets
            D-->>L: Immediate oneshot completion (skip)
        else Count mismatch
            D-->>L: Error(CountMismatch)
        else Valid counts
            D->>T: handle_local_transfer(...)
            alt Empty sources AND targets
                T-->>D: Immediate oneshot completion (skip)
            else Count mismatch
                T-->>D: Error(CountMismatch)
            else Valid counts
                T-->>D: Transfer future/receiver
            end
        end
        D-->>L: Propagate result
        L-->>C: Propagate result
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at tidy bounds,
Empty baskets? I won’t hop rounds.
Mismatched carrots? Count them twice—
Burrow rules keep things precise.
When numbers match, I scurry through—
A swift, clean transfer, carrot-true. 🥕🐇

Pre-merge checks

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description largely follows the template structure but leaves critical sections empty, including Details and Where should the reviewer start, and retains a placeholder Related Issues entry without the actual issue number. The Overview only references the issue code without summarizing the changes made. As a result, the description lacks sufficient information to guide reviewers.	Please fill out the Details section with a summary of the changes, specify which files reviewers should focus on, replace the Related Issues placeholder with the actual DIS-825 issue reference, and expand the Overview to clearly describe the purpose and impact of this fix.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	Title 'fix: handling 0 block transfer' succinctly captures the primary change of introducing validation for zero-block transfers and directly relates to the modifications in handle_transfer methods.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a55fa9 and b5dc3e4.

📒 Files selected for processing (3)

lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs (1 hunks)
lib/llm/src/block_manager/block/locality.rs (1 hunks)
lib/llm/src/block_manager/block/transfer.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

lib/llm/src/block_manager/block/transfer.rs (2)

lib/bindings/python/rust/llm/block_manager/vllm/connector/leader.rs (1)

oneshot (113-113)

lib/llm/src/block_manager/distributed/worker.rs (3)

oneshot (287-287)

oneshot (346-346)

oneshot (349-349)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Build and Test - dynamo
GitHub Check: clippy (lib/runtime/examples)
GitHub Check: clippy (launch/dynamo-run)
GitHub Check: clippy (.)
GitHub Check: clippy (lib/bindings/python)

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.

❤️ Share

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

coderabbitai · 2025-10-14T18:22:55Z

Caution

Review failed

The head commit changed during the review from f6b3974 to 062394f.

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.

❤️ Share

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

coderabbitai · 2025-10-14T18:27:41Z

Walkthrough

Adds early-empty-input guards to transfer handlers. When sources or targets are empty, the code logs a warning, creates a oneshot channel, immediately signals readiness, and returns the receiver, skipping subsequent transfer logic. Non-empty inputs keep existing behavior.

Changes

Cohort / File(s)	Summary
Early empty-input guard in transfer handlers `lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs`, `lib/llm/src/block_manager/block/locality.rs`, `lib/llm/src/block_manager/block/transfer.rs`	Added pre-checks in handle_transfer/handle_local_transfer to short-circuit when `sources.is_empty()` or `targets.is_empty()` by logging a warning, creating a oneshot channel, sending readiness immediately, and returning its receiver; otherwise existing transfer flow unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant TransferHandler as Transfer Handler
  participant Guard as Empty-Input Guard
  participant Channel as oneshot::channel

  Caller->>TransferHandler: handle_transfer(sources, targets)
  TransferHandler->>Guard: check sources/targets empty?
  alt Empty inputs
    Guard-->>TransferHandler: true
    TransferHandler->>Channel: create (tx, rx)
    TransferHandler->>Channel: tx.send(())
    TransferHandler-->>Caller: Ok(rx) (immediately ready)
  else Non-empty inputs
    Guard-->>TransferHandler: false
    TransferHandler->>TransferHandler: proceed with existing transfer logic
    TransferHandler-->>Caller: Ok(rx) (completes per prior flow)
  end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitched my ears at empty bins,
Skipped the haul with gentle grins.
A ready ping, then off I hop—
No seeds to move, no need to stop.
When baskets fill, I’ll proudly ferry—
Until then, swift, concise, and merry! 🥕🐇

Pre-merge checks

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description follows the template structure but none of the required sections are populated meaningfully. The Overview section only references the internal ticket rather than summarizing the change, the Details and Where should the reviewer start sections are empty, and the Related Issues line still uses a placeholder #xxx. This makes it hard for reviewers to understand the intent and scope.	Please add a concise summary of the empty-transfer guard change to the Overview, describe the specific code adjustments in Details, specify which files should be reviewed under “Where should the reviewer start,” and replace the placeholder in Related Issues with the actual issue number.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title “fix: handling 0 block transfer” concisely reflects the main change of adding a guard for zero-block transfers and clearly indicates a bug fix without extraneous detail. It accurately summarizes the primary adjustment made in the code.

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.

❤️ Share

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

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a55fa9 and 062394f.

📒 Files selected for processing (3)

lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs (1 hunks)
lib/llm/src/block_manager/block/locality.rs (1 hunks)
lib/llm/src/block_manager/block/transfer.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

lib/llm/src/block_manager/block/transfer.rs (2)

lib/bindings/python/rust/llm/block_manager/vllm/connector/leader.rs (1)

oneshot (113-113)

lib/llm/src/block_manager/distributed/worker.rs (3)

oneshot (287-287)

oneshot (346-346)

oneshot (349-349)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)

GitHub Check: trtllm (amd64)
GitHub Check: sglang
GitHub Check: trtllm (arm64)
GitHub Check: vllm (arm64)
GitHub Check: vllm (amd64)
GitHub Check: tests (lib/bindings/python)
GitHub Check: clippy (lib/bindings/python)
GitHub Check: clippy (launch/dynamo-run)
GitHub Check: tests (launch/dynamo-run)
GitHub Check: clippy (lib/runtime/examples)
GitHub Check: tests (.)
GitHub Check: tests (lib/runtime/examples)
GitHub Check: clippy (.)
GitHub Check: Build and Test - dynamo

lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs

lib/llm/src/block_manager/block/locality.rs

lib/llm/src/block_manager/block/transfer.rs

Signed-off-by: Olga Andreeva <[email protected]>

oandreeva-nv · 2025-10-14T21:30:44Z

/ok to test 354fac8

oandreeva-nv requested a review from a team as a code owner October 14, 2025 18:20

pull-request-size bot added the size/M label Oct 14, 2025

Adding edge cases handling

062394f

Signed-off-by: Olga Andreeva <[email protected]>

oandreeva-nv force-pushed the oandreeva_transfer_edge_case branch from f6b3974 to 062394f Compare October 14, 2025 18:21

oandreeva-nv changed the title ~~bug: handling 0 block transfer~~ fix: handling 0 block transfer Oct 14, 2025

github-actions bot added the fix label Oct 14, 2025

coderabbitai bot reviewed Oct 14, 2025

View reviewed changes

lib/llm/src/block_manager/block/data/logical/distributed_leader_worker.rs Outdated Show resolved Hide resolved

lib/llm/src/block_manager/block/locality.rs Outdated Show resolved Hide resolved

lib/llm/src/block_manager/block/transfer.rs Outdated Show resolved Hide resolved

oandreeva-nv added 2 commits October 14, 2025 11:34

returning error on count missmatch

b5dc3e4

Signed-off-by: Olga Andreeva <[email protected]>

cargo fmt

354fac8

Signed-off-by: Olga Andreeva <[email protected]>

richardhuo-nv approved these changes Oct 14, 2025

View reviewed changes

copy-pr-bot bot temporarily deployed to GITLAB October 14, 2025 21:30 Inactive

copy-pr-bot bot temporarily deployed to GITLAB October 14, 2025 21:35 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: handling 0 block transfer #3613

fix: handling 0 block transfer #3613

oandreeva-nv commented Oct 14, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

copy-pr-bot bot commented Oct 14, 2025

Uh oh!

oandreeva-nv commented Oct 14, 2025

Uh oh!

coderabbitai bot commented Oct 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 14, 2025

Review failed

Uh oh!

coderabbitai bot commented Oct 14, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oandreeva-nv commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: handling 0 block transfer #3613

Are you sure you want to change the base?

fix: handling 0 block transfer #3613

Conversation

oandreeva-nv commented Oct 14, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview:

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

Summary by CodeRabbit

Uh oh!

copy-pr-bot bot commented Oct 14, 2025

Uh oh!

oandreeva-nv commented Oct 14, 2025

Uh oh!

coderabbitai bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks

Uh oh!

coderabbitai bot commented Oct 14, 2025

Review failed

Uh oh!

coderabbitai bot commented Oct 14, 2025

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oandreeva-nv commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oandreeva-nv commented Oct 14, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 14, 2025 •

edited

Loading