[Bugfix][KV Transfer] Use kv_transfer_params for P2pNcclConnector coordination

[eicherseiji]

After #27987, Prefill and Decode get different internal request_ids, breaking P/D coordination in the P2P NCCL connector. The connector currently encodes the remote address into the request_id string and parses it back out with a regex, which is also fragile.

Implements the design from https://gist.github.com/markmc/0c10179d49bb7fed8b737e1cfa56f912: switch to kv_transfer_params following the NIXL pattern. The proxy injects the decode instance's KV address before sending to prefill, prefill returns its own address and request ID on completion, and the proxy forwards those to decode.

Includes unit tests, updates to both proxy implementations, and design doc changes.

Repro steps

pip install quart aiohttp pyzmq msgpack

# Terminal 1: Proxy
python examples/online_serving/disaggregated_serving_p2p_nccl_xpyd/disagg_proxy_p2p_nccl_xpyd.py

# Terminal 2: Prefill (GPU 0)
CUDA_VISIBLE_DEVICES=0 vllm serve facebook/opt-125m \
    --enforce-eager --host 0.0.0.0 --port 20003 \
    --dtype float16 --max-model-len 2048 --gpu-memory-utilization 0.5 \
    --kv-transfer-config \
    '{"kv_connector":"P2pNcclConnector","kv_role":"kv_producer","kv_port":"21001","kv_connector_extra_config":{"proxy_ip":"0.0.0.0","proxy_port":"30001","http_port":"20003","send_type":"PUT_ASYNC"}}'

# Terminal 3: Decode (GPU 1)
CUDA_VISIBLE_DEVICES=1 vllm serve facebook/opt-125m \
    --enforce-eager --host 0.0.0.0 --port 20005 \
    --dtype float16 --max-model-len 2048 --gpu-memory-utilization 0.5 \
    --kv-transfer-config \
    '{"kv_connector":"P2pNcclConnector","kv_role":"kv_consumer","kv_buffer_size":"5e9","kv_port":"22001","kv_connector_extra_config":{"proxy_ip":"0.0.0.0","proxy_port":"30001","http_port":"20005","send_type":"PUT_ASYNC"}}'

# Terminal 4: Test
curl -s http://localhost:11001/v1/completions \
  -H "Content-Type: application/json" \
  -d '{"model":"facebook/opt-125m","prompt":"The capital of France is","max_tokens":20}' \
  | python -m json.tool

Without fix

With fix

[gemini-code-assist]

Code Review

This pull request adds CI coverage for P2pNcclConnector and LMCacheConnectorV1 by adding new integration test steps and parameterizing the test script. The changes are well-structured and address a gap in test coverage. My feedback focuses on improving the new CI steps for better consistency and robustness by using the shared requirements file for installing dependencies.

[eicherseiji]

Prioritizing bug fix, will follow up with CI test here: #34050

[eicherseiji]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a critical bug in the P2P NCCL KV transfer mechanism, which was causing hangs due to the use of a randomized internal request_id. The fix involves replacing this with the consistent external_req_id for coordination between prefill and decode instances. The changes correctly propagate the external_req_id through the Request and NewRequestData structures and apply it within the P2pNcclConnector. The implementation appears correct and effectively resolves the issue. I have included one suggestion to enhance code maintainability by refactoring an indexed tuple into a more robust NamedTuple or dataclass.

[gemini-code-assist]

The chunked_prefill dictionary stores a tuple with three elements, which are then accessed by index (e.g., [0], [1], [2]) in build_connector_meta. This practice is fragile and can lead to bugs if the tuple structure is modified in the future. To improve readability and maintainability, consider using a NamedTuple or a dataclass to provide meaningful names to the fields.

For example:

from typing import NamedTuple

class ChunkedPrefillData(NamedTuple):
    block_ids: list[int]
    prompt_token_ids: list[int] | None
    external_req_id: str

# In __init__
self.chunked_prefill: dict[str, ChunkedPrefillData] = {}

# Then in build_connector_meta, you can access fields by name:
# prompt_token_ids = self.chunked_prefill[req_id].prompt_token_ids
# kv_request_id = self.chunked_prefill[req_id].external_req_id

[NickLucche]

Hey thanks for the fix @eicherseiji !
Left a comment.
cc @markmc on whether this is the best approach to integrate the request_id.

[NickLucche]

I would personally prefer not to edit request.py unless deemed necessary more generally, as we're still trying to figure out how much the nccl one is actually used.
I think this mapping could be stored within the connector itself for the time being.
Let's hear @markmc thoughts on this global change

[eicherseiji]

Makes sense. I could also strip the trailing request ID characters, or add a method that does this on InputProcessor. TIA for taking a look @markmc!

[markmc]

Hi @eicherseiji

I'm not familiar at all with this connector, and don't really have time right now to dig in deeply. But I did spend some time with Claude, trying to formulate some feedback and a recommendation based on how the NIXL connector works and also #32937 for the moriio connector. Obviously I could be missing something, but I think this is pretty solid: https://gist.github.com/markmc/0c10179d49bb7fed8b737e1cfa56f912

[shwgao]

Hi, apologies — I didn't notice that #33947 by @eicherseiji already addresses the same bug before I opened this PR. Sorry for the duplicate!

That said, the two PRs take different approaches:

#33947: Propagates external_req_id through Request and NewRequestData (changes 3 files: request.py, output.py, p2p_nccl_connector.py)
#34278: Strips the assign_request_id() random suffix inside the connector itself via _strip_internal_id_suffix() (changes 1 file: p2p_nccl_connector.py only)
My approach avoids modifying request.py / output.py, which aligns with @NickLucche's comment on #33947 suggesting the mapping be kept within the connector for now.

Totally fine to close #34278. Just wanted to offer an alternative, happy to defer to whatever the maintainers decide.

[orozery]

Obviously I could be missing something, but I think this is pretty solid: https://gist.github.com/markmc/0c10179d49bb7fed8b737e1cfa56f912

I agree. I think Request.kv_transfer_params is a perfect fit for the solution.

[njhill]

Agree we should avoid changing request.py and output.py for this if possible.

[shwgao]

Obviously I could be missing something, but I think this is pretty solid: https://gist.github.com/markmc/0c10179d49bb7fed8b737e1cfa56f912

I agree. I think Request.kv_transfer_params is a perfect fit for the solution.

Agree, the proper long-term fix is to follow the NIXL pattern. I could also take a look at this factor after @eicherseiji updates the PR, since I am currently working close to the P2pncclconnector.
But, this design touches multiple components (connector + proxy + protocol), it goes beyond a simple bugfix. The P2P NCCL connector is currently completely broken on main, so it might be worth landing a minimal hotfix first to unblock users, then following up with the NIXL-pattern refactor as a separate PR.

[eicherseiji]

Thanks @markmc, all for feedback. Will proceed with the kv_transfer_params design here.

In the meantime, maybe we can merge @shwgao's #34278 to recover main? @NickLucche, thoughts?

[markmc]

I'd be more inclined to accept a CLI argument to disable the request ID randomization - this would be a temporary feature available to users of the broken connectors as a workaround

[mergify]

Documentation preview: https://vllm--33947.org.readthedocs.build/en/33947/

[eicherseiji]

@markmc, if it's temporary, thoughts on an environment variable for a more minimal change? #34415

This PR is ready to review.

[markmc]

@markmc, if it's temporary, thoughts on an environment variable for a more minimal change? #34415

This PR is ready to review.

Thanks - I think this is a pragmatic solution 👍

[eicherseiji]

@markmc bumping for your review when you have a chance. Thanks!