feat(proxy): opt-in stream aggregation behind aggregate: true

rcsheets commented

2026-07-22 23:57:39 +00:00

Owner

A non-streaming completion sends no bytes -- headers included -- until
the last token is generated, so on a slow model the whole generation
happens inside the route's response-header timeout. Sized for the
worst-case generation, that timeout no longer detects a hung backend;
sized as a liveness check, it kills legitimate requests.

aggregate: true on a route converts the client's non-streaming
completion into an upstream streaming request and reassembles the SSE
chunks into the plain JSON response the client asked for. Upstream
headers arrive at time-to-first-token, so the timeout is a liveness
check again and generation may take as long as it takes.

This is SLP's one deliberate exception to never touching response
bytes, which is why it is opt-in per route and handled conservatively
at the edges: clients that asked to stream pass through untouched, as
do logprobs-family requests (gllm rejects logprobs with streaming) and
non-SSE upstream responses. In-band stream error events come back as a
real 502 with the upstream's error object -- something a pass-through
stream cannot offer -- and a stream that dies before [DONE] is a 502,
never a silently truncated 200.

Editions declare the flag themselves, never inheriting it from the
backend: an edition's upstream is a different program with its own
streaming behavior.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

A non-streaming completion sends no bytes -- headers included -- until the last token is generated, so on a slow model the whole generation happens inside the route's response-header timeout. Sized for the worst-case generation, that timeout no longer detects a hung backend; sized as a liveness check, it kills legitimate requests. aggregate: true on a route converts the client's non-streaming completion into an upstream streaming request and reassembles the SSE chunks into the plain JSON response the client asked for. Upstream headers arrive at time-to-first-token, so the timeout is a liveness check again and generation may take as long as it takes. This is SLP's one deliberate exception to never touching response bytes, which is why it is opt-in per route and handled conservatively at the edges: clients that asked to stream pass through untouched, as do logprobs-family requests (gllm rejects logprobs with streaming) and non-SSE upstream responses. In-band stream error events come back as a real 502 with the upstream's error object -- something a pass-through stream cannot offer -- and a stream that dies before [DONE] is a 502, never a silently truncated 200. Editions declare the flag themselves, never inheriting it from the backend: an edition's upstream is a different program with its own streaming behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rcsheets added 1 commit

2026-07-22 23:57:39 +00:00

feat(proxy): opt-in stream aggregation behind aggregate: true

ci / check (pull_request) Has been cancelled

Details

8f40366ba1

A non-streaming completion sends no bytes -- headers included -- until
the last token is generated, so on a slow model the whole generation
happens inside the route's response-header timeout. Sized for the
worst-case generation, that timeout no longer detects a hung backend;
sized as a liveness check, it kills legitimate requests.

aggregate: true on a route converts the client's non-streaming
completion into an upstream streaming request and reassembles the SSE
chunks into the plain JSON response the client asked for. Upstream
headers arrive at time-to-first-token, so the timeout is a liveness
check again and generation may take as long as it takes.

This is SLP's one deliberate exception to never touching response
bytes, which is why it is opt-in per route and handled conservatively
at the edges: clients that asked to stream pass through untouched, as
do logprobs-family requests (gllm rejects logprobs with streaming) and
non-SSE upstream responses. In-band stream error events come back as a
real 502 with the upstream's error object -- something a pass-through
stream cannot offer -- and a stream that dies before [DONE] is a 502,
never a silently truncated 200.

Editions declare the flag themselves, never inheriting it from the
backend: an edition's upstream is a different program with its own
streaming behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

pr-reviewer-bot commented

2026-07-22 23:57:41 +00:00

Collaborator

Automated review by pr-reviewer v0.37.0 | Safety Check | Claude Sonnet 5 | tracking id r-6158f5-e1a3ff
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

⚠️ The primary review (Mistral Small) failed because the diff is too large for that model. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — The "secret" flagged by automation is just fake test request-ID strings, and the aggregation logic is well-guarded and tested with no signs of committed credentials or breaking changes.

Reviewed the diff carefully.

The automated "possible secret" flag on aggregate.go is a false positive — it's just the gdqz1/gdqz000000000000000000000000 style fake request-ID strings used in tests/comments (mirroring gllm's real ID prefix), not real credentials or tokens. No actual secrets, keys, or connection strings are present anywhere in the diff.
No committed binaries or unrelated files; changes are scoped to proxy/config code, tests, and README.
Logic looks sound: aggregation is opt-in per route, guards against streaming clients, logprobs-family requests, non-SSE upstream responses, in-band error events, and truncated streams (no [DONE]) — all covered with corresponding tests (aggregate_test.go, config_test.go).
Edition Aggregate flag is explicitly not inherited from backend, matching stated intent, and is tested (TestCompileCarriesAggregate).
maxAggregateBytes cap bounds memory growth from a misbehaving upstream stream.
No breaking changes to existing non-aggregate routes; default behavior (aggregate: false) is unchanged pass-through.

Nothing here looks like it shouldn't have been pushed.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.37.0 | Safety Check | Claude Sonnet 5 | tracking id `r-6158f5-e1a3ff`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed because the diff is too large for that model. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — The "secret" flagged by automation is just fake test request-ID strings, and the aggregation logic is well-guarded and tested with no signs of committed credentials or breaking changes. Reviewed the diff carefully. - The automated "possible secret" flag on `aggregate.go` is a false positive — it's just the `gdqz1`/`gdqz000000000000000000000000` style fake request-ID strings used in tests/comments (mirroring gllm's real ID prefix), not real credentials or tokens. No actual secrets, keys, or connection strings are present anywhere in the diff. - No committed binaries or unrelated files; changes are scoped to proxy/config code, tests, and README. - Logic looks sound: aggregation is opt-in per route, guards against streaming clients, logprobs-family requests, non-SSE upstream responses, in-band error events, and truncated streams (no `[DONE]`) — all covered with corresponding tests (`aggregate_test.go`, `config_test.go`). - Edition `Aggregate` flag is explicitly not inherited from backend, matching stated intent, and is tested (`TestCompileCarriesAggregate`). - `maxAggregateBytes` cap bounds memory growth from a misbehaving upstream stream. - No breaking changes to existing non-aggregate routes; default behavior (aggregate: false) is unchanged pass-through. Nothing here looks like it shouldn't have been pushed.

rcsheets force-pushed feat/aggregate from 8f40366ba1

ci / check (pull_request) Has been cancelled

Details

to a3c97a2a68

ci / check (pull_request) Successful in 46s

Details

2026-07-22 23:57:55 +00:00

Compare

rcsheets merged commit 2888494230 into main

2026-07-23 00:07:09 +00:00

rcsheets deleted branch feat/aggregate

2026-07-23 00:07:09 +00:00