fix(sglang): rebuild prefill/decode requests on every Proxy call#984
Conversation
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
2d93770 to
918496a
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the SGLangConnector to rebuild prefill and decode requests on every Proxy call instead of caching them. This change ensures that request bodies, which are one-shot buffers, are not drained during retries. Additionally, unit tests were added to verify that request bodies are not reused and that the input request map is not mutated. Feedback suggests improving retry isolation by generating unique room IDs per call, handling errors in test handlers, and strengthening the mutation test with deep comparisons.
| bootstrapRoom int64 | ||
| lastPrefillAddr string | ||
| lastDecodeAddr string | ||
| bootstrapRoom int64 |
There was a problem hiding this comment.
The bootstrapRoom ID is currently stored in the SGLangConnector struct and initialized once when the connector is created. Since the connector instance is reused across retries for the same request (as noted in the PR description), the same room ID is reused for every attempt. If a retry occurs against the same prefill pod, this could lead to collisions or stale state in the prefill pod's bootstrap server if it hasn't yet cleaned up the previous attempt. For better isolation between retries, consider generating a unique bootstrapRoom ID inside the Proxy method for each call, which would also make the connector struct stateless.
|
|
||
| prefillServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| idx := atomic.AddInt32(&prefillCalls, 1) - 1 | ||
| body, _ := io.ReadAll(r.Body) |
There was a problem hiding this comment.
The error from io.ReadAll(r.Body) is ignored here and in the decode server handler. It is better to check this error and fail the test explicitly (e.g., using t.Errorf) if the body cannot be read, ensuring the test doesn't proceed with an empty body and potentially mask a failure in request construction.
| keysBefore := make(map[string]struct{}) | ||
| for k := range reqBody { | ||
| keysBefore[k] = struct{}{} | ||
| } | ||
|
|
||
| _, _ = connector.Proxy(c, reqBody, "127.0.0.1:1", "127.0.0.1:2") | ||
|
|
||
| for k := range reqBody { | ||
| if _, existed := keysBefore[k]; !existed { | ||
| t.Errorf("Proxy() mutated caller reqBody by adding key %q", k) | ||
| } | ||
| } |
There was a problem hiding this comment.
This mutation test is currently insufficient as it only checks if new keys were added to the reqBody map. It does not verify if existing values were modified (e.g., max_tokens being capped by preparePrefillBody) or if keys were removed. To properly ensure the caller's map is not mutated across retries, the test should perform a deep comparison of the map before and after the Proxy call.
There was a problem hiding this comment.
Pull request overview
Fixes a retry-path bug in the SGLang PD connector where *http.Request objects (with one-shot bodies) were cached on the connector instance and incorrectly reused across Proxy() calls, causing drained/empty request bodies during router retries.
Changes:
- Remove prefill/decode request caching fields from
SGLangConnectorand rebuild both upstream requests on everyProxy()call. - Add regression tests to ensure request bodies are non-empty across repeated
Proxy()calls and that caller request maps are not mutated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/kthena-router/connectors/sglang.go |
Rebuild prefill/decode http.Requests each Proxy() call to avoid drained body reuse during retries. |
pkg/kthena-router/connectors/sglang_test.go |
Adds regression coverage for body-drain retry behavior and reqBody immutability expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -105,30 +101,28 @@ func (s *SGLangConnector) Proxy(c *gin.Context, reqBody map[string]interface{}, | |||
| // Build the decode request first (before preparePrefillBody mutates reqBody). | |||
| // TestSGLangConnectorReqBodyNotMutated checks that Proxy() does not mutate the | ||
| // caller's reqBody map. proxyToPDDisaggregated passes the same modelRequest | ||
| // across all retry iterations, so mutations would bleed between retries. | ||
| func TestSGLangConnectorReqBodyNotMutated(t *testing.T) { |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The SGLang connector was caching the prefill and decode
*http.Requestobjects on the struct betweenProxycalls, keyed bylastPrefillAddr/lastDecodeAddr. These requests wrap abytes.Bufferbody inio.NopCloser; once the HTTP transport reads it, it's gone. Can't reuse it.The router's retry loop in
proxyToPDDisaggregatedcallsconnector.Proxy(...)multiple times on the same connector instance. In a typical PD setup with 1 prefill pool shared across multiple decode pods, the scheduler scores all decode pods against the same prefill pod set and picks the same best prefill pod for each iteration. So you end up withPrefillPods = [P0, P0]andDecodePods = [D0, D1]. When D0 fails and the router retries with D1, the prefill addr is still P0 , cache hit , and the drainedrequest gets reused. Transport sends Content-Length N with 0 bytes. Prefill server rejects it, or the SGLang bootstrap handshake aborts with
KVTransferError: Aborted by AbortReq. Worse, the decode side already wrote headers to the response stream, so the client can get HTTP 200 with an empty/truncated SSE response ; totally silent failure.The fix is to rebuild both requests on every
Proxycall and drop the dead caching fields from the struct. Same approach used in 067dfb5 for NIXL and d9f656a for HTTP.Special notes for your reviewer:
This is the same body-reuse regression class that was just fixed for NIXL and HTTP connectors , the SGLang connector was added after those fixes landed and reintroduced the same pattern. The most common topology that triggers it is a single prefill pool with 2+ decode pods, which is pretty standard for SGLang PD deployments.
Added two regression tests in
sglang_test.gothat mirror the NIXL ones , one that callsProxytwice with the same prefill addr on the same connector instance and checks both backends got a non-empty body each time, and one that checks the caller'sreqBodymap isn't mutated across calls.Does this PR introduce a user-facing change?: