Skip to content

[Bugfix][KV Offload] Ignore stale async lookup results - #54872

Merged
orozery merged 2 commits into
vllm-project:mainfrom
Alex-ai-future:bugfix/kv-offload-async-lookup-stale-result
Sep 2, 2026
Merged

[Bugfix][KV Offload] Ignore stale async lookup results#54872
orozery merged 2 commits into
vllm-project:mainfrom
Alex-ai-future:bugfix/kv-offload-async-lookup-stale-result

Conversation

@Alex-ai-future

@Alex-ai-future Alex-ai-future commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Purpose

Ignore stale secondary-tier lookup results after request cleanup.

User case

  1. Request A starts an async lookup for KV key K.
  2. A is cancelled or finishes while a slow NFS/PVC/object-store lookup is
    still running; cleanup removes A's state.
  3. Request B reuses K and starts a new lookup.
  4. A's late result is applied to B because the old result carries only K.
    B can then use a stale verdict or hit the duplicate-result assertion when
    its own result arrives.

The fix adds a generation to each lookup state and ignores results from older
generations. Active requests sharing one pending lookup are unchanged.

This is distinct from #49176/#49328, which fixes failed-load verdict
invalidation. No open issue or PR was found for this request-cleanup race.

Test Plan

.venv/bin/python -m pytest \
  tests/v1/kv_offload/tiering/test_async_lookup.py -q
pre-commit run --files \
  vllm/v1/kv_offload/tiering/async_lookup.py \
  tests/v1/kv_offload/tiering/test_async_lookup.py
git diff --check

The regression test covers cleanup and key reuse, stale/current generation
result delivery, shared entries, and same-generation duplicate results.

Test Result

14 passed in 1.66s
pre-commit: passed
git diff --check: passed

Model evaluation is not applicable; this is a scheduler lifecycle fix.

AI assistance was used for code and test development. The human submitter
must review every changed line before submission.


Checklist
  • Purpose, user case, test plan, and results included.
  • Duplicate-work check documented.
  • AI assistance disclosed.

@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 added the bug Something isn't working label Sep 2, 2026
Alex-ai-future and others added 2 commits September 2, 2026 10:47
Co-authored-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
Co-authored-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Alex <jihui.huang@daocloud.io>
@Alex-ai-future
Alex-ai-future force-pushed the bugfix/kv-offload-async-lookup-stale-result branch from 213cc1a to a23d6a2 Compare September 2, 2026 02:47
@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

@orozery this is ready and hope you can take a look

@bojiang3

bojiang3 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The generation approach looks right to me. cleanup() deletes the state once
the last request drops it, so the next lookup() allocates a fresh generation
from a monotonic counter, and a late result can only ever carry an older one —
which drain_results now discards instead of applying to whoever inherited the
key. I traced the batch path too: a key deleted between lookup() and flush()
still gets probed, but its result lands on state is None and is dropped, so
that's just a wasted probe rather than a wrong verdict.

One thing I could not settle by reading, so posing it as a question rather than
a change request: should mark_miss participate in the generation scheme?

It sets state.result = False directly, without invalidating the generation
in flight. That seems fine in the normal path — a load only follows a True
verdict, so that generation's result has already arrived and no second one is
coming. But after a cleanup/recreate cycle it looks less obvious:

  1. gen 0 for key K resolves True; a load starts and later fails.
  2. The requests holding K finish; cleanup() deletes the state.
  3. A new request looks K up again → gen 1, in flight, result is None.
  4. The failed load from (1) calls mark_miss([K]), which sets the gen 1
    state to False.
  5. Gen 1's own result arrives, generation matches, and
    assert state.result is None fires.

I haven't been able to prove step 4 can actually be reached after step 3 — it
depends on how far the failed-load handler can lag on_request_finished() — so
this may well be impossible in practice. If it is reachable, having mark_miss
bump the generation (or having drain_results tolerate an already-decided
verdict on a matching generation) would close it with the machinery this PR
already adds.

@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 2, 2026
@orozery

orozery commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

/ci run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

@Alex-ai-future, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • /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

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #86833 for commit a23d6a288abf.

@orozery

orozery commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

/ci retry

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Queued 1 failed job(s) for retry in Buildkite CI #86833.

@orozery
orozery merged commit 35faf95 into vllm-project:main Sep 2, 2026
91 checks passed
mylibrar pushed a commit to tanyuqian/vllm that referenced this pull request Sep 3, 2026
…54872)

Signed-off-by: Alex <jihui.huang@daocloud.io>
Co-authored-by: OpenAI Codex <noreply@openai.com>
sheralskumar pushed a commit to sheralskumar/vllm that referenced this pull request Sep 8, 2026
…54872)

Signed-off-by: Alex <jihui.huang@daocloud.io>
Co-authored-by: OpenAI Codex <noreply@openai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants