Skip to content

fix(router): use full conversation for PD chat cache-aware routing (#26263) - #27430

Merged
Kangyan-Zhou merged 2 commits into
sgl-project:mainfrom
pjdurden:fix/pd-router-chat-cache-aware-full-conversation
Jun 22, 2026
Merged

fix(router): use full conversation for PD chat cache-aware routing (#26263)#27430
Kangyan-Zhou merged 2 commits into
sgl-project:mainfrom
pjdurden:fix/pd-router-chat-cache-aware-full-conversation

Conversation

@pjdurden

@pjdurden pjdurden commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Motivation

Closes #26263.

The PD router's cache-aware routing builds its routing text for chat
completions from messages.first() only. For multi-turn conversations this
ignores the rest of the conversation (system prompt, prior turns, the current
message, tool context) — exactly the content that drives KV-cache reuse — so
prefix matching is dominated by system-prompt similarity rather than the real
cached prefix.

As reported in the issue, this materially hurts cache locality:

  • first-message-only: ~69% cache hits, ~678 output TPS
  • full-context: ~96% cache hits, ~1078–1085 output TPS

Modifications

  • route_chat in sgl-model-gateway/src/routers/http/pd_router.rs now builds
    routing text via ChatCompletionRequest::extract_text_for_routing() — the
    same method the regular (non-PD) HTTP router already uses
    (routers/http/router.rs) — so both routers derive routing text identically
    from the full conversation.
  • Extracted the logic into a small build_chat_request_text helper and removed
    the now-unused first-message ChatMessage/MessageContent matching.

Tests

  • Added regression test test_chat_request_text_uses_full_conversation,
    asserting later turns (not just the first message) are included in the
    routing text.
  • Full library test suite passes (cargo test --lib, 384 tests).
  • cargo fmt --check and cargo clippy are clean.

CI States

Latest PR Test (Base): ✅ Run #27057060542
Latest PR Test (Extra): ❌ Run #27930620068

…gl-project#26263)

The PD router built its cache-aware routing text for chat completions from
`messages.first()` only. For multi-turn conversations this ignores the
history (system prompt, prior turns, current message, tool context) that
actually drives KV-cache reuse, so routing was dominated by system-prompt
similarity rather than real cache prefixes.

Replace the hand-rolled first-message extraction with
`ChatCompletionRequest::extract_text_for_routing()` -- the same method the
regular (non-PD) HTTP router already uses -- so both routers build routing
text identically from the whole conversation. The logic is moved into a
small `build_chat_request_text` helper and covered by a regression test
that asserts later turns (not just the first message) are included.

Reported impact in the issue: ~69% -> ~96% cache hits and ~678 -> ~1080
output TPS for multi-turn chat under cache-aware PD routing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 6, 2026 07:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes cache-aware routing for PD chat requests by ensuring the routing text is derived from the full conversation (not just the first message), improving KV-cache prefix matching accuracy.

Changes:

  • Switch chat request routing text generation to reuse extract_text_for_routing() over the full conversation.
  • Add a dedicated helper (build_chat_request_text) for PD chat routing text creation.
  • Add a regression test covering multi-turn chat routing text behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sgl-model-gateway/src/routers/http/pd_router.rs

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the PD router to build routing text from the full conversation history rather than just the first message, ensuring consistent cache-aware routing in multi-turn chats. It also adds a regression test for this behavior. The review feedback points out an unused import of GenerationRequest and suggests returning None instead of Some("") when the extracted routing text is empty to prevent routing issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread sgl-model-gateway/src/routers/http/pd_router.rs
Comment thread sgl-model-gateway/src/routers/http/pd_router.rs
Address review feedback on sgl-project#27430: build_chat_request_text now returns
None when the conversation has no text content, instead of Some(""),
preserving the prior PD behavior of not feeding an empty key into
prefix matching. Add a regression test for the empty-conversation case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pjdurden

Copy link
Copy Markdown
Contributor Author

@slin1237 gentle nudge when you have time. The PD router cache routing fix is tested and lint clean, it just needs the run ci label to unblock the gate. Happy to adjust anything.

@petrpan26

petrpan26 commented Jun 18, 2026

Copy link
Copy Markdown

woulld love to have this merged in as well ! @slin1237

@pjdurden

Copy link
Copy Markdown
Contributor Author

@CatherineSue could you take a look when you get a chance? This is a small one-file fix in src/routers for PD chat cache-aware routing (#26263). Approved already and lint clean, just needs the run ci label to clear the gate. Thanks!

@slin1237

Copy link
Copy Markdown
Collaborator

i thought I fixed it a lifetime ago. Why is this broken again 😓

@Kangyan-Zhou
Kangyan-Zhou merged commit adc203d into sgl-project:main Jun 22, 2026
113 of 123 checks passed
@petrpan26

Copy link
Copy Markdown

wooo merged ! would love to see this push into new version

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PD router cache-aware routing for chat completions only uses the first message

5 participants