Repository navigation
perf(epp): release the request body after it is forwarded - #3884
Conversation
The request context kept RawBody and the parsed request (typed API struct and payload map) until the stream ended. For a streamed response that is the whole generation, so each in-flight request held about 3 copies of its body. Release them once the body response is sent to the proxy. TokenizedRequest, Stream, MaxOutputTokens and Model stay, because response-phase plugins read them. Signed-off-by: Thomas Fahrner <thomas.fahrner@parasail.io>
ahg-g
left a comment
There was a problem hiding this comment.
Releasing at this point is right, and no in-tree response-phase reader uses the cleared fields. Main ask: swap Body for a smaller struct instead of clearing the shared one in place, to fix a race with timed-out data producers. As a follow-up, it would be worth an issue to give the response-phase interfaces a type that has no request content, so the compiler enforces this.
|
|
||
| // releaseRequestBody drops the request body once it has been forwarded, because | ||
| // a streamed response can last for the whole generation. | ||
| func (r *RequestContext) releaseRequestBody() { |
There was a problem hiding this comment.
Replace SchedulingRequest.Body with a new struct that holds only the kept fields, instead of clearing fields on the shared one.
func (r *RequestContext) releaseRequestBody() {
r.Request.RawBody = nil
if r.SchedulingRequest != nil && r.SchedulingRequest.Body != nil {
r.SchedulingRequest.Body = r.SchedulingRequest.Body.ResponsePhaseView()
}
}Why: dataProducerPluginsWithTimeout does not wait for the producer goroutine when it times out (plugin_executor.go:104-118), and the Director keeps going with the request (director.go:356, :370). A tokenizer still running when the body is forwarded shares this Body. For example, renderBackend.produce checks body.Messages != nil, makes a network call in useLegacy, then reads body.Messages again in renderLegacyMessages. If ReleaseContent runs in between, that second read is a nil dereference in a goroutine, which crashes the EPP process. CacheSaltFromBody and WirePayload have the same race. With the swap, response-phase plugins also get their request copies after it, so they can only reach the kept fields.
Why it's safe: a straggling goroutine keeps reading the old struct, which stays valid until it finishes. predictedlatency/plugin.go:428 stores a full copy of the request, which would keep the old Body alive until the request ends; it only reads .TargetModel, so store just that.
There was a problem hiding this comment.
Done. releaseRequestBody now sets Body to Body.ResponsePhaseView(). predictedlatency keeps only targetModel, not a copy of the request.
| // ReleaseContent drops the request content once the body has been forwarded, so a | ||
| // long response does not keep it in memory. It keeps TokenizedRequest and the | ||
| // derived fields that response-phase plugins read. | ||
| func (b *InferenceRequestBody) ReleaseContent() { |
There was a problem hiding this comment.
Build the released body from the fields to keep, instead of listing the fields to clear.
func (b *InferenceRequestBody) ResponsePhaseView() *InferenceRequestBody {
return &InferenceRequestBody{
TokenizedRequest: b.TokenizedRequest,
Stream: b.Stream,
MaxOutputTokens: b.MaxOutputTokens,
Model: b.Model,
}
}Why: a content field added to InferenceRequestBody later, such as a new API type, would silently be kept for the whole stream unless someone remembers to add it here. With a keep-list, a new field is released unless someone decides to keep it.
There was a problem hiding this comment.
Done. ResponsePhaseView builds the new body from the 4 kept fields, and ReleaseContent is removed.
| require.True(t, director.body.Stream) | ||
| require.Equal(t, "m", director.body.Model) | ||
| require.NotNil(t, director.body.MaxOutputTokens) | ||
| require.Equal(t, int64(16), *director.body.MaxOutputTokens) |
There was a problem hiding this comment.
Check that TokenizedRequest survives the release, alongside Stream, Model and MaxOutputTokens.
Why: latencyreporter reads Body.TokenizedRequest.TokenCount() in ResponseBody (plugin.go:220). If a change dropped it, prompt-token latency metrics would quietly turn to zero, and no test would catch it.
There was a problem hiding this comment.
Done. The test sets TokenizedRequest in HandleRequest and checks that the response phase has the same pointer.
| return reqCtx, nil | ||
| } | ||
|
|
||
| func TestProcessReleasesRequestContentAfterForwarding(t *testing.T) { |
There was a problem hiding this comment.
Add a test where a ResponseHeader and a ResponseBody plugin run after forwarding and check exactly what request.Body exposes, plus a -race test with a data producer that outlives its timeout.
Why: this test uses a test Director and never runs the response phase, so nothing tests what plugins see there, and the producer race above is untested.
There was a problem hiding this comment.
Done. TestProcessReleasesRequestContentAfterForwarding now sends response headers and a first body chunk after forwarding. It checks that request.Body in HandleResponseHeader and HandleResponseBody equals exactly {Model, Stream, MaxOutputTokens, TokenizedRequest}.
TestProcessReleaseKeepsBodyForStragglingProducer starts a goroutine that holds a copy of the request, as dataProducerPluginsWithTimeout gives each producer, and reads the body before and after the release. With the previous in-place clear, it fails with -race (DATA RACE) and without -race (nil content). It uses a test Director: requestcontrol imports handlers, so the handlers tests cannot run the real Director.
| r.requestRunning = true | ||
| // Dump the response so a new stream message can begin | ||
| r.reqBodyResp = nil | ||
| r.releaseRequestBody() |
There was a problem hiding this comment.
Add one sentence to the ResponseHeaderProcessor and ResponseBodyProcessor docs (plugins.go:80-105) saying that only Model, Stream, MaxOutputTokens and TokenizedRequest are set on request.Body.
Why: a plugin outside this repo that reads Payload or the typed request in the response phase gets nil after this PR, with no compile error and no documentation explaining it.
There was a problem hiding this comment.
Done. The ResponseHeaderProcessor and ResponseBodyProcessor docs now say that request.Body holds only Model, Stream, MaxOutputTokens and TokenizedRequest.
|
Follow-up for the compile-time version of the response-phase restriction: #3895. |
A DataProducer that outlives its timeout keeps running with a copy of the request that shares Body, so clearing Body in place could hand it nil fields mid-run. releaseRequestBody now replaces Body with ResponsePhaseView, a new body built from the fields to keep. A field added to InferenceRequestBody later is released unless it is added to that list. predictedlatency kept a full copy of the request until the request ended, which would keep the original body alive. It now keeps only TargetModel, the one field it reads. The ResponseHeaderProcessor and ResponseBodyProcessor docs list the body fields that remain. Tests check the exact body that response-phase hooks receive, that TokenizedRequest survives, and that a goroutine holding a pre-release copy of the request still reads the content (run with -race). Signed-off-by: Thomas Fahrner <thomas.fahrner@parasail.io>
|
Thanks for filing #3895. |
What type of PR is this?
/kind performance
What this PR does / why we need it:
The request context keeps
Request.RawBodyand the parsed request (typed API struct,Payload,InferenceRequestBody.RawBody) until the stream ends. For a streamed response, that is the full generation time.This PR releases that content after the body response is sent to the proxy.
TokenizedRequest,Stream,MaxOutputTokensandModelstay because response-phase plugins read them. TheResponseHeaderandResponseBodyplugins on main read only those fields or checkBody != nil.BenchmarkProcessInFlightRetainedHeap, retained bytes per stream,-count 3, Apple M-series:Test plan:
RawBody, the typed request and the payload map are unreachable while the stream waits for the responseStream,ModelandMaxOutputTokenskeep their valuesmake presubmitWhich issue(s) this PR fixes:
Fixes #3881
Release note (write
NONEif no user-facing change):