[jiuyueQ]

Overview

Finishing a request with a load in flight can permanently put its KV request ID back into _failed_req_ids after on_request_finished() has removed it. Repeated mid-load cancellations accumulate stale IDs for the lifetime of the process. Reusing an ID on a retry is a secondary symptom: the stale entry suppresses that request's remote lookup.

Extracts C from #61667 as requested in review. Refs #61532.

Claims

• Finished requests do not permanently repopulate the failed-ID set through immediate cancellation failures, already-buffered outcomes or session close.
• Late failures still complete transfer-job cleanup without restoring finished IDs to the failure set.
• Close cleanup and request-ID reuse preserve this distinction; unfinished failures continue to trigger recomputation.

Validation

Rebased and tested on main 34091a07c7d7, including merged #64256 and #64250. Used cu130 wheel artifacts from fa3b9d137721 (identical C++/CUDA sources to this base), Python 3.12, PyTorch 2.14.0+cu130, an RTX 4060 Ti, and NIXL 1.4.1 over UCX TCP on localhost.

Check	Result
P2P and tiering component suites	436 passed
New real-weight LLM regression on the base, with only the new tests overlaid	1 failed for the assertion below
Same LLM regression with this fix	1 passed
Repository pre-commit hooks, including mypy and CI tethering	Passed

A manager regression finishes a request while its load is in flight, drains get_finished_jobs(), and asserts that the failed-ID set stays empty. It fails on the base with {'req-finish'} != set() and passes with the fix.

The dedicated real LLM e2e test enqueues a remote load, cancels its internal engine request ID after the peer observes the fetch, and drains the failed load using real engine steps. AbortAck separately confirms release of its blocks. A different prompt then reuses the same KV request ID; the peer must observe its new fetch rather than an unconditional local miss.

Reproduction commands from a configured checkout/venv:

PYTHONHASHSEED=0 UCX_TLS=tcp,self .venv/bin/python -m pytest -q \
  --confcutdir=tests/v1/kv_offload \
  tests/v1/kv_offload/tiering/p2p \
  tests/v1/kv_offload/tiering/test_tiering_offloading.py \
  -k 'not test_inference'

PYTHONHASHSEED=0 UCX_TLS=tcp,self VLLM_USE_FLASHINFER_SAMPLER=0 \
  .venv/bin/python -m pytest -v -s \
  tests/v1/kv_offload/tiering/test_p2p_lifecycle_e2e.py::test_inference_reuses_a_finished_kv_request_id

Base failure excerpt:

E AssertionError: late failure poisoned the reused KV request id
E assert not True
E + where True = empty()

With the fix: 1 passed.

Model: real HuggingFaceTB/SmolLM2-135M weights at 93efa2f097d58c2a74874c7e644dbc9b0cee75a2, FP16, eager, greedy sampling, and VLLM_BATCH_INVARIANT=1. Model validation covers deterministic generation and fault handling; no task-accuracy or performance claim is made. The existing v1-kv-offload Buildkite step selects these tests and its dependency list covers the changed P2P sources. All applicable Python and documentation hooks ran, including mypy, CI test tethering, and commit sign-off checks.

Details

Carry a default-false request_finished flag on the load state and LoadResult. Finish marks both in-flight loads and already-buffered results. Polling and close omit finished IDs from failure bookkeeping while still returning failed jobs for cleanup. This is independent of deferred slot release.

Duplicate-work check: this is the explicitly requested split from the original PR. Merged #64256 implements the core timed-out-load/deferred-release work using #63779's API; this PR adds the separate C fix on top of that implementation. The other split fixes can land independently. Relevant open-PR searches were refreshed before submission.

This is internal bookkeeping; the orchestration-layer documentation change was removed as requested in review.

AI assistance: OpenAI Codex assisted implementation, tests, and PR preparation.

---

Pull Request Checklist

• I used vLLM's /pr-checklist skill.
• AI assistance was used during the creation of this PR.
• Design Fit: Small P2P-local fix using the existing lifecycle.
• Testing and Validation: Component regressions, real-weight LLM regression, before/after evidence, and existing Buildkite coverage.
• Code Quality and Style: Repository hooks pass; internal bookkeeping requires no orchestration-layer documentation.
• Pull Request Contents: Scope, root cause, duplicate-work check, validation, and limitations documented.

Rebase validation (2026-10-29)

Adapted to #64256's immediate-failure/deferred-release lifecycle. All three split PRs remain independent. The same real-weight regression was rerun on the latest base with only the new tests overlaid, and on this fix; results are shown above. The original #61667 branch remains unchanged.

Review follow-up (2026-10-30)

Reframed the bug around failed-ID leakage, added the requested manager-level regression, moved the real-weight test and its private peer fixture into a dedicated e2e module, removed the internal-behavior docs paragraph, and replaced defensive getattr checks with direct field access.

[claude]

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.

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-28T15:34:05.018318Z We couldn’t complete this review. Comment @codex review to try again.	487ff83	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[mergify]

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

[mergify]

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

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

[liranschour]

Thanks for splitting this out. I traced the path on current main and the fix is needed, though I think the PR undersells why.

The bug is real, and it's mainly a leak. In on_request_finished, _failed_req_ids.discard(kv_request_id) runs before session.finish_request(). client.finish() then calls _abort_load() for every in-flight load, which immediately buffers a LoadResult(success=False). On the next _poll_once, if not lr.success: self._failed_req_ids.add(...) puts the id straight back. close() does the same for finished requests whose AbortAck hasn't arrived (they still have st.loads). That discard is the only place entries ever leave the set, so every request aborted mid-load (client disconnect, peer death with loads pending) leaves an entry in _failed_req_ids for the life of the process. Id reuse is the secondary symptom; the orchestrator contract says kv_request_id is unique, although an orchestrator that reuses the client's X-Request-Id will hit it on retries.

Approach looks right. A per-load / per-LoadResult request_finished flag is the right granularity. A manager-side "live ids" set would wrongly poison a reused id whose old failure arrives late, and test_finish_marks_buffered_result_before_request_id_reuse covers exactly that case.

Requests:

1. Reframe the description around the leak: "every mid-load abort permanently adds an entry to _failed_req_ids", with reuse as the secondary effect. Otherwise the natural reply is "ids must be unique".
2. Please drop the docs/features/kv_offloading_usage.md change. It describes internal bookkeeping, not anything the orchestration layer has to do, and it sits above the intro of the "Orchestration-Layer Protocol" section.
3. The model-based e2e test is heavy for test_sessions.py. It loads SmolLM2 on a GPU and needs a 78-line p2p_control_peer fixture used only by this test, in a file that is otherwise unit tests. The parametrized test_late_failure_preserves_request_finish plus the manager test already cover the logic. I'd drop it, or move it alongside the other model-based P2P tests.
4. Add a manager-level regression test: on_request_finished with a load in flight, then get_finished_jobs(), then assert _failed_req_ids is empty. That locks in the leak fix directly.
5. Nits:
   • getattr(loads[0], "request_finished", False) in the tests was only needed to run against the base branch; access the field directly.
   • The test_failed_load_records_kv_request_id docstring still says the id "must" be added; that's now only true for unfinished requests.

[liranschour]

All 5 comments were addressed.
LGTM. Thanks

[orozery]

/ci run

[github-actions]

✅ @jiuyueQ, CI is now available for this PR.

• /ci run starts upstream CI; /amd-ci run starts AMD CI only.
• Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
• /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
• /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
• /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

[github-actions]

✅ Triggered Buildkite CI #99190 for commit ee432385d5a2.

ℹ️ This PR is 260 commits behind upstream main. Merged upstream main into this branch as ee432385d5a2 before starting CI. Pull the branch before pushing more changes.