Skip to content

[Bugfix][NIXL] Fail pull reads whose producer lease has expired - #61549

Closed
sawsa307 wants to merge 3 commits into
vllm-project:mainfrom
sawsa307:nixl-pull-lease-check
Closed

sawsa307 wants to merge 3 commits into
vllm-project:mainfrom
sawsa307:nixl-pull-lease-check

Conversation

@sawsa307

@sawsa307 sawsa307 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Fixes #61229. In NIXL pull mode, D reads P's KV blocks without checking P's lease. Once P reaps an expired lease and reuses the blocks, D's READ still succeeds and D silently decodes another request's KV. With this PR, D fails such reads through the existing KV load failure path (kv_load_failure_policy).

Not a duplicate: #61229 has no fix PR. #59713 (closed, unmerged) is prior art for gating on the producer's lease; this PR differs as described in Details.

Claims

  • A pull read is failed instead of decoded when P's lease has (nearly) expired: checked before the READ is issued and again when it completes, so a lease that expires while the READ is in flight is also caught.
  • Healthy requests whose lease D kept alive with heartbeats are not failed, even when they are read after the lease's original expiry.
  • No new kv_transfer_params key, no handshake change, no NIXL_CONNECTOR_VERSION bump.

Validation

VLLM_TARGET_DEVICE=cpu HF_HUB_OFFLINE=1 pytest \
  tests/v1/kv_connector/unit/test_nixl_heartbeat.py \
  tests/v1/kv_connector/unit/test_bidirectional_kv_transfer.py \
  tests/v1/kv_connector/unit/test_kv_load_failure_recovery.py \
  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_nixl_push_connector.py

658 passed, 2 skipped, 3 failed (test_abort_timeout_on_prefiller[ray|None] and test_fewer_blocks_with_hma[gemma]; they need ray / an online model and fail the same way on main in this environment). pre-commit, including mypy, passes.

New worker tests on a fake clock cover:

  • a read after the original expiry whose lease D kept alive with heartbeats (must proceed);
  • failed heartbeat sends, a heartbeat gap longer than the extension, no heartbeats, a late first heartbeat (must be declined, with the KV-expired metric incremented);
  • a lease expiring while the READ is in flight (load failed, invalid blocks reported);
  • small leases (1 s, 4 s), running D against a stub P on the same clock, asserting D never treats a lease as alive after P has reaped it;
  • the heartbeat record being dropped when the load finishes or fails;
  • P nodes exporting remote_blocks_expiry_time.

With the previous NIXL sources, 10 of the new tests fail; the "kept alive by heartbeats" test passes both before and after, guarding against false failures.

GPU: the #61229 repro (1P1D, kv_lease_duration=5, SIGSTOP the decode engine for ~10 s while unrelated prompts reuse P's blocks, then SIGCONT) has not been run on this PR yet; results will be added here. Expected: HTTP 500 under fail, the correct text under recompute, and no failures in a control run with a 2 s stall. Keeping this as a draft until then.

Details

Cause. Only the bidirectional turn-2 readback was gated on a remote block expiry (_is_turn2_read_expired), and P exported remote_blocks_expiry_time only from D nodes.

Fix.

  • P also exports remote_blocks_expiry_time in request_finished.
  • A pull D worker records, per (engine, remote request), the first and last time it sent a heartbeat to every agent of the engine, and the largest gap between them.
  • P extends a lease by kv_lease_duration * 2 // 3 per heartbeat and stamped the expiry E0 before D's first heartbeat, so (2 * (E0 - first) - 2) / 3 is a lower bound on P's extension without D knowing P's config. If the heartbeats form an unbroken chain under that bound, the deadline is max(E0, last + bound) - margin; otherwise it is E0 - 5 s.
  • D checks the deadline before every pull READ (replacing the turn-2-only gate) and again when the READ completes. Both go through the existing receive-failure path and the KV-expired metric.

Difference from #59713. #59713 ignored heartbeats, so it also failed healthy requests that queued past the original expiry while D kept the lease alive; it checked only before the READ; and it added a new key and push-mode changes. This PR credits D's heartbeats, also checks at completion, adds no key, and covers pull mode only.

Limitations.

  • Conservative: a request whose first heartbeat leaves D within about 10 s of P's lease expiry gets no heartbeat credit and is failed if read after E0 - 5 s (recompute avoids user-visible errors).
  • Clock-offset error beyond the margin, or a heartbeat that was sent but lost, can still allow a stale read.
  • Push mode is unchanged: P drives the WRITEs, so it needs a check on the writer thread and a new wire message.
  • For HMA models no invalid blocks are reported (existing TODO), so recompute is not block-precise there.
  • Mixed versions: new D with old P behaves as today. An old D with bidirectional_kv_xfer enabled would now also gate turn-1 reads against E0 - 5 s without heartbeat credit, so upgrade D before P in that setup.

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. (GPU repro pending.)

  • 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.

AI assistance (Claude) was used to write and review this change.

🤖 Generated with Claude Code

Problem (vllm-project#61229): in pull mode D reads P's blocks without checking P's
lease. Once P reaps an expired lease and reuses the blocks, D's READ
still succeeds and D decodes another request's KV.

Cause: only the bidirectional turn-2 readback was gated on a remote
block expiry, and P exported remote_blocks_expiry_time only from D nodes.

Fix:
- P also exports remote_blocks_expiry_time in request_finished.
- A pull D worker records, per (engine, remote request), the first and
  last time it sent a heartbeat to every agent of the engine and the
  largest gap between them. A record is dropped when the request's load
  finishes or fails on this worker, when a heartbeat set no longer lists
  it, and when its engine is evicted.
- P extends a lease by kv_lease_duration * 2 // 3 per heartbeat and
  stamped the expiry E0 before D's first heartbeat, so
  (2 * (E0 - first) - 2) / 3 bounds P's extension from below, including
  the integer floor, without knowing P's config. If that bound is
  positive, the first heartbeat landed before E0 and no gap exceeds the
  bound, the deadline is max(E0, last + bound) - margin, with
  margin = min(5 s, bound / 4); otherwise it is E0 - 5 s.
- D checks the deadline before issuing every pull READ (replacing the
  turn-2-only gate) and again when the READ completes, failing reads
  whose deadline passed in flight. Both use the KV-expired metric and
  the existing receive-failure path, so kv_load_failure_policy applies.
  For HMA no invalid blocks are reported (existing TODO), so recompute
  is not block-precise there.
- A missing expiry skips the check.

The check is conservative: a request whose first heartbeat leaves D
within about 10 s of P's lease expiry gets no heartbeat credit and is
failed if read after E0 - 5 s (kv_load_failure_policy applies; recompute
avoids user-visible errors).

Not covered: clock-offset error beyond the margin and heartbeats that
were sent but lost still allow a stale read. Push mode is unchanged:
there P drives the WRITEs, so it needs a check on the writer thread and
a new wire message.

Compatibility: no new kv_transfer_params key, no handshake change and
no NIXL_CONNECTOR_VERSION bump.
- New D with old P: P exports no expiry, so reads proceed as today.
- Old D with new P: only an old D with bidirectional_kv_xfer enabled is
  affected. Its turn-2 gate now also gates turn-1 reads against E0 - 5 s
  without heartbeat credit, so upgrade D before P in that setup.

Tests: worker tests on a fake clock for reads after E0 kept alive by
heartbeats, failed heartbeat sends, a heartbeat gap longer than the
extension, no heartbeats, a late first heartbeat, a lease expiring while
the READ is in flight, small leases (1 s, 4 s) where the integer floor
matters, and the heartbeat record dropped once the load finishes or
fails; a P-node export test. The new expiry tests fail on the previous NIXL sources.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: David Cheung <d7cheung@gmail.com>
@sawsa307

Copy link
Copy Markdown
Contributor Author

62065 seems to be a better fix, closing this PR

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: NixlConnector: KV blocks released on lease expiry while remote READ in flight, decode silently consumes overwritten KV

1 participant