fix(streaming): report OpenAI stream errors in-band instead of raising - #268
fix(streaming): report OpenAI stream errors in-band instead of raising#268zhangw wants to merge 1 commit into
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: vincent.
|
When streaming fails on /v1/chat/completions, stream_wrapper() re-raised the
exception. StreamingResponse has already sent its headers at that point, so
Starlette cannot turn it into an error response and aborts with:
RuntimeError: Caught handled exception, but response already started.
The client receives "data: [DONE]" with no content and no reason, and the
server logs a full ASGI traceback for what is an upstream condition. This is
reproducible with any streaming failure, e.g. a 504 from the first-token
retry loop or a mid-stream connection drop.
/v1/messages already handles this correctly by emitting an SSE error event,
so this brings the OpenAI path in line: the error is now yielded as an
OpenAI-format error chunk followed by [DONE], and the exception is no longer
re-raised. Applied to both the account-system and legacy branches via a
shared build_sse_error_chunk() helper.
Tests: 3 unit tests for build_sse_error_chunk (HTTPException status/detail
preserved, generic exception reported as 500, empty message stays valid
JSON), 3 route tests on /v1/chat/completions (first-token timeout, mid-stream
failure after chunks were already sent, happy path unaffected), and a
matching parity test on /v1/messages.
b791cfa to
82182b8
Compare
|
Thanks for the PR! 馃帀 Before merge, we need a one-time CLA confirmation. Full CLA text: Please reply once with: You need to write once, all further messages from me can be ignored. |
|
I have read the CLA and I accept its terms |
|
@zhangw could you give a try to this gateway: https://github.com/ankitcharolia/kiro-gateway It works quite well with All AI harness and actively being developed. The most important thing is that it is ACP compliant |
What
stream_wrapper()inchat_completions()re-raised the exception afterStreamingResponsehad already sent its headers, so Starlette cannot turn it into an error response and aborts the connection instead:Two consequences:
data: [DONE]with no content and no reason - it looks like the gateway silently returned an empty answer. The 504 detail never reaches it./v1/messagesalready handles this correctly (event: error, then a graceful end of stream), so only the OpenAI path was affected.Why it happens in practice
Any streaming failure after the first byte of the response reaches this path. The easiest way to hit it is the first-token retry loop, because not every model streams reasoning deltas: the non-Claude models exposed by Kiro send nothing at all while they think, so the first byte only arrives together with the answer.
Measured with one heavy request (~270 KB payload, 20 tools, non-Claude model), first-token timeout temporarily raised to 180 s to observe the real latency:
A Claude model on the same gateway starts emitting
reasoning_contentafter ~2-4 s, which is why the defaultFIRST_TOKEN_TIMEOUT=15has been fine so far. With a model that stays silent while reasoning, all three attempts time out and the resulting 504 lands on the broken error path above.Whether the default timeout should change is a separate discussion - this PR only fixes the error reporting.
Related: #129 (mid-stream
RemoteProtocolError) hits the same code path on/v1/chat/completions, and improvement 2 requested there - telling the client it was a connection drop - now works for the OpenAI API too.Change
build_sse_error_chunk(exc)renders an exception as an OpenAI-format error chunk.HTTPException.status_code/.detailare preserved, anything else is reported as 500.stream_wrapper()variants (account-system branch and legacy branch) now yield the error chunk followed by[DONE]and no longer re-raise.The client now receives the reason:
Server-side logging and debug-log flushing are unchanged -
streaming_erroris still set, so the existingHTTP 500 - POST /v1/chat/completions (streaming)access log line andflush_on_error()still fire.Tests
tests/unit/test_routes_openai.pyTestBuildSseErrorChunk- HTTPException status/detail preserved; generic exception reported as 500; exception with an empty message still produces valid JSON.TestStreamingErrorAfterResponseStart- first-token timeout producescode: 504+[DONE]; mid-stream failure keeps the chunks already sent and appendscode: 500; happy path unaffected (no error chunk, exactly one[DONE]).tests/unit/test_routes_anthropic.pyTestStreamingErrorAfterResponseStart- parity test asserting/v1/messagesreports the failure asevent: errorwithout raising. It passes onmainas well; it is there to keep the two APIs from drifting apart again.Verification: 5 of the 7 new tests fail on
main(the 3 helper tests plus the 2 OpenAI streaming-failure tests, which abort with theRuntimeErrorabove) and all 7 pass with this change. Full unit suite: same pass/fail set before and after this change, no new failures.