Skip to content

[Bugfix][NIXL] Roll back incomplete remote-agent handshakes - #59524

Open
xijiaat wants to merge 1 commit into
vllm-project:mainfrom
xijiaat:codex/nixl-handshake-rollback
Open

xijiaat wants to merge 1 commit into
vllm-project:mainfrom
xijiaat:codex/nixl-handshake-rollback

Conversation

@xijiaat

@xijiaat xijiaat commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Roll back resources acquired by a NIXL handshake that fails before publishing
the peer. Today, failed setup can leave native metadata and descriptor state
outside the published-peer cleanup path.

Failure workflow

  1. A background handshake registers remote agents with NIXL and prepares
    descriptors before publishing the peer in _remote_agents.
  2. Validation, descriptor preparation, or setup of a later rank raises after
    some resources have already been acquired.
  3. The handshake reports failure without publishing the peer. Cleanup based on
    _remote_agents therefore misses the incomplete registration.

The ownership gap is between native registration and Python-side publication.
The fix tracks registrations from acquisition until publication or successful
rollback, so an unsuccessful handshake does not leave them unaccounted for.

The native/API reproducer confirmed leftover metadata, but subsequent baseline
requests also succeeded. This fixes incomplete cleanup, not a demonstrated
permanent outage or inevitable deadlock.

This complements #50047's published-peer replacement cleanup and is distinct
from #55471's recovery after NIXL invalidates an already-published peer.

AI assistance was used. The human submitter has reviewed every changed line and
personally run the relevant tests for this change.

Claims

  • Own native registrations immediately after successful registration, including
    notification-only peers and earlier ranks in a partially completed handshake.
  • Reuse existing cleanup for failed setup and shutdown; retain unreleased
    resources if cleanup itself raises so a retry can finish cleanup.
  • Preserve unrelated healthy peers and the original handshake exception.

Validation

Base: 8fb16ea5a66f0ef362e728f0ff025f2845927fd5.

Validation Before After
Four added regression cases 4 fail 4 pass
Focused peer-replacement suite Not measured 14 pass
Connector/HMA/geometry/bidirectional/push CPU suite Not measured 365 pass, 238 deselected
Native CPU and CUDA buffers, 1 MiB Native metadata survives failed validation Cleaned; three retry/read byte comparisons pass per memory type
OPT-125m P/D API, one injected validation failure HTTP 500 and leaked metadata; next two requests succeed HTTP 500 without leaked metadata; next two requests succeed
.venv/bin/python -m pytest tests/v1/kv_connector/unit/test_nixl_connector.py \
  -m cpu_test -k TestPeerReplacement -v

.venv/bin/python -m pytest \
  tests/v1/kv_connector/unit/test_nixl_connector.py \
  tests/v1/kv_connector/unit/test_nixl_connector_hma.py \
  tests/v1/kv_connector/unit/test_nixl_desc_geometry.py \
  tests/v1/kv_connector/unit/test_bidirectional_kv_transfer.py \
  tests/v1/kv_connector/unit/test_nixl_push_connector.py -m cpu_test -v

.venv/bin/pre-commit run --files \
  vllm/distributed/kv_transfer/kv_connector/v1/nixl/base_worker.py \
  tests/v1/kv_connector/unit/test_nixl_connector.py

The serving test ran on two H20 GPUs, TP1 + TP1, eager, short token-ID prompts,
256 MiB KV per engine and dummy weights. It injects one exception after native
registration and real handshake validation, then checks native metadata and
subsequent API requests. Main Python sources used v0.30.0 native extensions and
NIXL 1.4.1. This is not a fresh CUDA build of main. The native transport tests use
UCX TCP/CUDA-copy/CUDA-IPC, not cross-node RDMA fault injection. Dummy weights are
not a quality evaluation. The baseline also recovers on retry in this setup;
the demonstrated defect is incomplete resource cleanup, not inevitable deadlock.
All applicable pre-commit checks passed, including mypy.

Details

A pending registration journal records native agents immediately after
registration, including notification-only peers and earlier ranks of a partial
handshake. It is discarded after successful publication, when the existing
published-peer cleanup takes ownership. Failed setup reuses the existing
per-engine cleanup to release descriptors and registrations.

Cleanup removes each resource's bookkeeping only after the corresponding native
operation succeeds. If rollback itself raises, the remaining entries stay
tracked; subsequent setup first retries the unfinished rollback. The original
handshake failure remains visible.

The change is limited to connector resource ownership; it does not retry failed
requests, change scheduling, change model output, or promise recovery of a broken
UCX context. The original failure remains visible. Tests extend existing peer
replacement fixtures. No steady-state GPU synchronization is added. Concurrent
integration with #55471 has not been validated.

Duplicate-work checks also covered #56852 (checkpoint/reinitialization hooks),
#56341 (explicit router-driven peer retirement) and #54757 (listener hardening).
They do not cover rollback of an incomplete worker-side registration.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).
  • AI assistance was used during the creation of this PR.
  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

Track native registrations until handshake publication, clean partial descriptor and peer state on setup failure, and retain failed cleanup for retry.

Co-authored-by: OpenAI Codex <codex@openai.com>
Signed-off-by: xijiade.aihemaiti <3146335281@qq.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify

mergify Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @xijiaat.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working kv-connector needs-rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant