Skip to content

[Bugfix][KV Offload] Preserve stored event prefix order - #63249

Merged
orozery merged 7 commits into
vllm-project:mainfrom
Alex-ai-future:codex/fix-offload-store-event-order
Oct 30, 2026
Merged

orozery merged 7 commits into
vllm-project:mainfrom
Alex-ai-future:codex/fix-offload-store-event-order

Conversation

@Alex-ai-future

@Alex-ai-future Alex-ai-future commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Preserve offload store key order so self-describing BlockStored events remain
parent-before-child within a transfer job. This prevents KV-aware routers from
dropping child blocks when a parent mapping has not been indexed yet.

Claims

  • Keep TransferJobStatus.keys in prefix order and deduplicate keys while
    retaining their first occurrence.
  • Do not change the KV-event wire format or guarantee ordering across separate
    secondary-tier jobs, tiers, or promotion events; router-side pending/replay
    still covers those cases.

Validation

  • pytest tests/v1/kv_connector/unit/offloading_connector/test_events.py -q:
    51 passed.
  • pytest tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py -q
    with the affected cases selected: 100 passed.
  • pre-commit run --files <changed-files>: passed.
  • Model evaluation is not applicable because this changes event ordering only.

Details

The missing prefix order resulted from three assumptions rather than a decision
that order was unnecessary:

  • #39186 introduced
    TransferJobStatus.keys as set-backed job bookkeeping before offload events
    existed.
  • #43468 made stored events
    self-describing and explicitly treated key order as irrelevant because each
    payload contains its parent hash. The order-independent test and
    “key order is free” comment codified that assumption.
  • A consumer test in
    #52103
    later showed that llm-d still needs the parent event indexed first. The author
    confirmed the gap in
    this reply.

prepare_store() returns keys in prefix order, but the scheduler stored them in
a set. This can emit a child first even when transfer completion is FIFO. The
scheduler now preserves job order without removing set-style deduplication.

Secondary tiers are intentionally outside the ordering guarantee. FS/OBJ jobs
complete asynchronously and may become distinct raw events; tier aggregation
emits primary and secondary events by tier, not by parent topology. Therefore
this change fixes intra-job ordering but does not reorder across secondary jobs,
tiers, or promotions. Those cases remain a consumer concern.

Duplicate-work check: no open PR directly fixes this ordering path. #63129
addresses reused-key metadata lifecycle, and #52103 refactors event provenance
and removals while still retaining set-backed job keys.

AI assistance was used for investigation, implementation, and tests. The human
submitter must review every changed line and rerun the validation above.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill.

  • AI assistance was used during the creation of this PR.

  • Design Fit: Small bug fix isolated to offloading event ordering.

  • Testing and Validation: Repro and related suites pass locally.

  • Code Quality and Style: Existing tests extended; pre-commit passes.

  • Pull Request Contents: Includes root cause, scope, and duplicate check.

Co-authored-by: OpenAI Codex <codex@openai.com>

Signed-off-by: Alex <alex.tech.lab@outlook.com>
@mergify mergify Bot added bug Something isn't working kv-connector labels Oct 23, 2026
@Alex-ai-future
Alex-ai-future marked this pull request as ready for review October 23, 2026 02:42

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

@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

I want to clarify the old key order is free assumption and why this PR intentionally narrows it.

That statement is correct at the payload level: each self-describing BlockStored contains its own tokens, parent hash, and block size, so the tracker can construct the payload without seeing the previous event.

It does not imply that a consumer can update its index in arbitrary order. For a child block, llm-d needs the parent engine hash to resolve an existing engine-hash -> request-key mapping before it can compute the child request key:

child request key = hash(parent request key, child tokens)

Co-authored-by: OpenAI Codex <codex@openai.com>

Signed-off-by: Alex <alex.tech.lab@outlook.com>
@orozery

orozery commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

@Change72 @karya0 @vMaroon PTAL

@Change72

Copy link
Copy Markdown
Contributor

Thanks for working on this. From my point of view, chunk_idx is only used internally to sort stored events parent-first. No downstream consumer reads it directly; llm-d only relies on the resulting order, while Dynamo does not.

Since ordering is still not guaranteed across jobs or tiers, llm-d must handle missing parents anyway. Preserving TransferJobStatus.keys order makes sense, but the extra chunk_idx sorting seems unnecessary unless vLLM wants to define a clear ordering guarantee.

Co-authored-by: OpenAI Codex <codex@openai.com>

Signed-off-by: Alex <alex.tech.lab@outlook.com>
@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

Thank you all. Are there any remaining issues or areas for improvement with this proposal?

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

orozery commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

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

Copy link
Copy Markdown

✅ Triggered Buildkite CI #98335 for commit b12b00700cd0.

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

@orozery

orozery commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #98571 for commit 6986ca0667f2.

@orozery

orozery commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #98677 for commit 9dff17fe0759.

@orozery
orozery merged commit dfe4d99 into vllm-project:main Oct 30, 2026
46 checks passed
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 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.

5 participants