Repository navigation
GH-29: bound response buffering with pass-through overflow - #36
Merged
Merged
Conversation
…errors
classifyProxyError treated net/http's response-header timeoutError as a
client abort because that type declares
Is(err) { return err == context.DeadlineExceeded } without exposing an
Unwrap chain. The client got an empty 200 after a 60s hang and the attempt
was never retried, contradicting the documented contract. Walk the unwrap
chain for a literal context.DeadlineExceeded instead of trusting errors.Is.
writeThrough dropped client write errors that happened after commit, so a
truncated pass-through was counted as a success with no log. Capture them
in attemptRecorder.err alongside commit()'s buffered flush.
TestProxyToVLLMOverflowPassesThrough now asserts the passthrough reason is size_limit, so dropping the Flush() that forces chunked framing fails the test instead of silently falling through to the Content-Length fast-path. Add the missing size_limit-then-truncate combination at the handler level, using chunk-framed output that is never terminated so the read ends in io.ErrUnexpectedEOF rather than the clean EOF of a close-delimited body.
The Prometheus table omitted gateway_proxy_attempts_total, gateway_response_passthrough_total and health_check_panics_total. Document all eight metrics and the label sets for the labeled counters.
commitGate.Write returned the client write error but never recorded it, so a stream whose client stopped reading was reported as a clean completion. Capture it in writeErr rather than err, which already carries upstream transport and body-read failures and drives the retry path — a failing client must not be classified as an upstream problem or leave the node in the offline set. Brings the streaming path in line with attemptRecorder.writeThrough.
…Recorder attemptRecorder.err was filled by three different sources: ReverseProxy's ErrorHandler and errCaptureBody, which report upstream failures, and a.w.Write, which reports a client that stopped reading. The handler could not tell them apart, so a dead client was logged as an upstream truncation and left the node in the offline set. Route client write failures to writeErr, matching commitGate.writeErr on the streaming path, and clear the node instead of reporting it. An upstream failure still takes precedence when both are set.
The streaming and pass-through paths logged the same root cause under two different strings, which makes it harder to grep or alert on a client that stopped reading. Collapse both onto "client write failed after commit". The two upstream messages are untouched so existing alerts keep matching: "stream terminated by upstream failure" and "response truncated after commit".
commitResponse discarded the error from writing a fully buffered response to the client, so outcomeOK logged nothing at all. A client that stopped reading was therefore visible only when the response happened to exceed the buffer limit and take the pass-through path, or when it was streaming. Return the write error and log it. The node answered correctly, so the offline set and the metrics are unchanged: this is a missing log line, not a behavior change.
The three paths that log "client write failed after commit" were the only
signal that a client stopped reading, and logs do not alert well.
gateway_response_client_write_failures_total{model} makes the rate visible.
gateway_requests_total keeps counting these as 200: it records what the
gateway did, and the gateway did commit a response. Delivery to the client is
a separate concern, so it gets a separate counter rather than overloading the
status label and breaking existing series.
The request body cap was a hardcoded package variable in the handler, so operators could not raise it for large prompts or lower it to protect the gateway. Expose it as max_request_body_bytes alongside max_buffered_response_bytes. A request body cap is a resource control, so unlike the response buffer there is no unlimited setting: negative values are rejected and 0 keeps the previous 32 MiB default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces unbounded
bufferedRecorderresponse buffering with a bounded buffer plus pass-through overflow, so a concurrent burst of large completions can no longer grow gateway memory without limit.max_buffered_response_bytesin[gateway](default8388608= 8 MiB;-1= unlimited;0/omitted = default).attemptRecorderbuffers up to the limit. Over the limit, or when upstreamContent-Lengthexceeds it, the buffered prefix is committed to the client and the remainder streams through.attemptResult.committedis true the handler never retries and never writes a 502 — an upstream failure yields a truncated body (same contract as the existing streamingcommitGate).gateway_response_passthrough_total{model,reason}with reasonssize_limit/content_length.stream: trueand request-sidemaxBodyBytesare unchanged.Design
Spec:
tmp/docs/sdd/2026-09-24-gh29-response-buffer-design.mdPlan:
tmp/docs/sdd/2026-09-24-gh29-response-buffer-plan.mdKey invariant:
committedmeans "bytes may have reached the client", not "commit fully succeeded".attemptRecorder.commit()sets the flag immediately afterWriteHeader, matchingcommitGateininternal/api/stream.go, so a failed body write can never fall through to a second status/headers write.Commits
dfdb73eaddmax_buffered_response_bytesconfig key841bb20bound response buffering with pass-through overflowf066d6crefuse retry and 502 after committed response7c0342fdocument bounded response buffering8b5bfb3document passthrough metric and commit-once wordingc62f789set committed before body write and gate passthrough metricTesting
go test ./...— all packages passgo test -race ./internal/api/ ./internal/config/ ./internal/metrics/go test -race -count=5 ./internal/api/— stability on go1.26.3 and go1.22.12GOTOOLCHAIN=go1.22.12 go test ./... && GOTOOLCHAIN=go1.22.12 go vet ./...gofmt -l .cleancommit()ordering, or the metric gate each fail their testsFollow-ups (not in this branch)
maxBodyBytesconfigurabilityclassifyProxyErrormaps response-header timeout tooutcomeClientAborted, contradicting the README retry wording — pre-existing, needs its own issuedocs/api.mdPrometheus table is stale (missing this metric and two pre-existing ones)size_limit-then-truncate untested at handler level;writeThroughtail write errors not captured toerrcloses #29