fix: GPT-5.6 SSE streams truncated through /v1/responses (chunk framing, raw identity passthrough, gzip window) - #988
Conversation
Spherrrical
left a comment
There was a problem hiding this comment.
Reviewed the code and ran the full local gate suite against this branch — cargo test --lib (176 tests across the 5 crates), cargo fmt --check, cargo clippy --locked --all-targets --all-features -D warnings, and the wasm32-wasip1 release build all pass, including the 7 new regression tests. Nice work; the every-64-byte-offset replay test is a great guard.
A few non-blocking hardening/clarity asks below. Two things I couldn't cover locally and am trusting the from-source verification for: the live gzip window_bits behavior (needs a running Envoy + real upstream) and the e2e streaming path.
Also, on the window_bits: 15 change — just flagging for ops awareness that this now affects gzip decoding for all providers, not only chatgpt. That's the correct scope given the truncation class, but it's not isolated to the reported model.
Nit: the full-stream test asserts out.len() == baseline_len + substring presence, but the PR description claims byte-identical output. Consider a strict assert_eq!(out, expected_bytes) if you want the test to actually enforce that claim.
| data.trim() == "[DONE]" || serde_json::from_str::<serde_json::Value>(data).is_ok() | ||
| } | ||
| // event-only line (e.g. `event: foo`) with no data payload | ||
| None => true, |
There was a problem hiding this comment.
A chunk boundary landing inside an event: line (e.g. event: respo) parses as a valid event-only SseEvent, so None => true makes it "complete" and it gets consumed now with a truncated name; the continuation on the next chunk is then dropped as unparseable. This is safe today only because every coupled path re-derives the event name from data.type and suppresses the raw event: line — an implicit invariant, not an enforced one.
Could you either hold back a trailing event:-only line too (return false here when the line isn't newline-terminated), or add a comment documenting why truncating an event: line is safe, so a future non-coupled consumer doesn't silently regress?
There was a problem hiding this comment.
Fixed in 59e5428. Trailing event-only lines without a newline are now always retained, so a split such as event: respo cannot be consumed as a complete event. Added test_mid_event_name_split_is_buffered_until_continuation, which asserts both buffering after the first fragment and exact recombination after its continuation.
| /// SSE chunk processor (to decide whether to buffer a line for cross-chunk | ||
| /// recombination) and the identity passthrough transform (to decide whether to | ||
| /// propagate the error so the processor can retry with the next chunk). | ||
| pub fn is_incomplete_json_error<E: std::fmt::Display + ?Sized>(err: &E) -> bool { |
There was a problem hiding this comment.
Could you add a unit test pinning the actual serde_json EOF messages this must catch (so a wording change fails loudly), or classify on serde_json::Error::is_eof() where the concrete type is still available rather than the stringified Box<dyn Error>?
There was a problem hiding this comment.
Fixed in 59e5428. Internal streaming retry decisions now downcast to serde_json::Error and use is_eof() rather than matching rendered error text. The public Display-based helper remains for API compatibility. Regression coverage confirms truncated JSON is retried, invalid JSON is not, and an unrelated UnexpectedEof is not misclassified.
…and buffer partial event lines Replace the fragile display-text-based incomplete JSON classifier with a robust serde::Error::is_eof check. Also fix a bug where event-only lines (e.g., "event: response") were incorrectly considered complete, causing mid-event-name splits across chunks to be emitted prematurely. Now such lines are buffered until the next chunk provides the full event name. Add tests for both changes.
|
Addressed the remaining review items in 59e5428 and pushed the branch. The identity-stream fixture now asserts byte-for-byte equality for the single-chunk baseline and every 64-byte boundary phase. The gzip |
|
thanks @Spherrrical 🙏 |
Problem
chatgpt/gpt-5.6-{sol,terra,luna}(ChatGPT-subscription Codex backend) return HTTP 200 through Plano's/v1/responses, but the client only receivesresponse.created+response.in_progress— no text, noresponse.completed. Fixes #980.Live debugging with a from-source gateway traced this to three stacked defects, each independently able to truncate or corrupt streams:
1. SSE chunk-boundary loss in
SseChunkProcessor(hermesllm)SseStreamItersplits the buffer withstr::lines()and parses each line independently. When a network chunk boundary lands inside adata:/event:line prefix (e.g.da+ta: {…}), the partial line fails with a non-JSON error, whichSseStreamIter::nextsilently skips — it never reaches the incomplete-JSON buffering. Both halves are unparseable, so the event is permanently lost with no log line. A boundary inside a multi-byte UTF-8 char could similarly kill the whole chunk viastr::from_utf8.Fix: byte-level SSE framing in
process_chunk— only bytes up to the last\nare parsed; the trailing partial line is held back and prepended to the next chunk (with a completeness check for a final unterminated line likedata: [DONE], since nothing flushes the buffer at end-of-stream, and a 1 MiB cap so a broken upstream can't grow the buffer unboundedly).2. Lossy parse-and-reconstruct on the identity Responses→Responses path
The "passthrough" path strictly deserialized every event into
ResponsesAPIStreamEventand re-serialized from the struct. Any unmodeled event type or field was dropped or mangled: via-Plano streams lostreasoning.mode/reasoning.context,text.verbosity,tool_usage, …, and a future unknown event type (e.g.response.reasoning_text.delta) would be dropped entirely. Additionally,llm_gateway's per-event loop aborted the whole remaining chunk (return Err(Action::Continue)) when one event failed to parse.Fix: on the identity Responses→Responses path, forward the original wire bytes verbatim (reconstructing the coupled
event: <type>\ndata: <json>\n\nblock from the raw payload — output is byte-identical to upstream). Parsing is still attempted for token counting; unknown events forward raw withprovider_stream_response = None, and incomplete-JSON errors still propagate for cross-chunk recombination (sharedis_incomplete_json_errorhelper, used by both call sites).stream_context.rsnow logs and falls through on unparsed events instead of aborting the chunk. Cross-API arms and Anthropic/ChatCompletions identity behavior are untouched.3. gzip decompressor
window_bits: 9truncates streams (envoy config)This turned out to be the dominant user-visible cause. Envoy's decompressor filter advertises
accept-encodingupstream, so the backend returns gzip. The decompressor was configured withwindow_bits: 9(512-byte inflate window) while the paired compressor useswindow_bits: 10and real upstreams use 15. Gzip headers don't carry the window size, so inflate doesn't fail upfront — it silently stops emitting data once a back-reference exceeds the window. Debug trace of a failing stream:response data decompressed from 1372 bytes to 2638 bytes, then307 → 2, then0for every remaining frame. This truncated any sufficiently large/redundant SSE stream traversing two listeners (gpt-5.4 included, when the client path involved compression).Fix: decompressor
window_bits: 15on all four gzip decompressor blocks (three were at 9; theegress_traffic_llmone had nowindow_bitsand silently used Envoy's default of 12). Note the blast radius: this config applies to all providers' gzip-encoded responses, not just chatgpt — which is intended, since the truncation class affects any of them. Raising the inflate window is strictly safe (a larger window decodes anything a smaller one can;max_inflate_ratiozip-bomb protection is retained).Also: added
gpt-5.6-{sol,terra,luna}to the chatgpt models inprovider_models.yaml(consumed viainclude_str!inProviderId), and a pre-existing one-line clippy fix (for_kv_map) needed to keep the workspace-D warningsgate green.Verification
Unit tests (all inline, ids obfuscated):
test_gpt56_responses_identity_passthrough_full_stream— replays a real captured GPT-5.6 stream (11 events incl. a reasoning output item) through the processor at every 64-byte split offset, asserting all event types emerge and output bytes == input bytes. Failed before the fix; passes now.OutputItem::Reasoningwithoutsummary/ withcontent/encrypted_content/status; omittedlogprobs).cargo test --lib(388 tests across brightstaff/common/hermesllm/llm_gateway/prompt_gateway),cargo fmt --check,cargo clippy --locked --all-targets --all-features -- -D warnings, wasm32-wasip1 release builds.Live through a from-source gateway (
planoai build+up):chatgpt/gpt-5.6-*tiers andchatgpt/gpt-5.4now stream the full ladder (created … output_text.delta … completed) with correct answer text.reasoning.mode,text.verbosity,tool_usage) are preserved byte-for-byte.Notes
ResponsesAPIStreamBuffercross-API path Stoff81/chatgpt5.5 #958 works in.🤖 Generated with Claude Code