Repository navigation
fix(router): keep RawBody's fast path when an insert changes nothing - #2663
Conversation
RawBody::insert marked the body mutated even when the value it set was already there, so the prefill leg's unconditional stream:false on a non-streaming request re-serialized a body its parsed bytes could have served as they were, defeating the fast path #2641 introduced. An insert of the value already present now leaves the body unmutated. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Limit details: You’ve used all 6 included reviews currently available. Your 47 included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesRaw body insertion
Estimated code review effort: 2 (Simple) | ~8 minutes Merge Risk: ⚪ Minimal · up to Unchanged raw values retain the original request bytes, while changed values continue to be applied. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// The prefill leg always sets `stream: false`; a non-streaming request | ||
| /// already carries it, and marking the body mutated for that re-serialized | ||
| /// a body the parsed bytes could have served as they were. |
There was a problem hiding this comment.
🟡 Nit: This comment (and the PR description) misstates the mechanism it fixes. In pd_router.rs, the prefill leg's bytes are always produced by prefill_body.to_bytes(..) (pd_router.rs:557) — there is no parsed-bytes reuse gated on mutated() for that leg. The stream: false insert (pd_router.rs:1015) lands on the prefill_body clone, never on the body whose mutated() gates the decode leg's fast path (pd_router.rs:562), and sanitize_prefill_for_kv_handoff unconditionally caps max_tokens/max_output_tokens to 1, so the prefill leg is genuinely mutated for nearly every request regardless of stream.
The insert precision fix is worthwhile hardening of the mutated invariant, but as written no current call site regains a fast path from it — realizing the described win would also need pd_router to gate the prefill bytes on prefill_body.mutated(). Consider rewording this comment (and the description) so future readers don't go looking for a fast path that isn't wired up.
There was a problem hiding this comment.
Reviewed the RawBody::insert change and traced all call sites (serialize_request_body, inject_bootstrap, inject_dp_rank, sanitize_prefill_for_kv_handoff, the kv-handoff decode insert). The equality check is correct: raw-text comparison is conservative (a semantically-equal-but-differently-formatted value still marks the body, which only costs a re-serialization), the mutated flag stays monotonic, and byte output is unchanged in every case. The new test covers both the no-op and the changed-value branch.
Summary: 0 🔴 Important, 1 🟡 Nit (the test's doc comment and PR description claim a prefill-leg fast path that no current call site is wired to), 0 🟣 Pre-existing.
2 participants
Description
Problem
From the review of #2641:
RawBody::insertalways setsmutated = true, even when the value is unchanged. The PD prefill leg setsstream: falseunconditionally, so every non-streaming PD request, whose body already carriesstream: false, was marked mutated and re-serialized instead of going out as the bytes it was parsed from: the fast path #2641 introduced never applied to exactly the requests it was meant for.Solution
An insert of the value already present (compared as raw JSON text) returns without marking the body. Everything else about
insertis unchanged: an existing field keeps its position, a new one goes last.Not done from that review, on purpose: the duplicate-top-level-key dedupe and adversarial case table (the input is serde output of a typed struct, so a duplicate key is unreachable, as the review itself notes) and a friendlier non-object error text (serde's is clear).
Test Plan
insert_of_an_unchanged_value_does_not_mark_the_body:stream:falseinserted into a body carrying it leavesmutated()false; a different value marks it (watched fail first).cargo test -p smg --lib -- request_body pd_router: 46 passed.cargo +nightly fmt --all -- --checkclean;cargo clippy -p smg --all-targets -- -D warningsclean (without--all-features, no OpenCV here).Checklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses (without--all-features, see above)