fix(responses): reject with OpenAIError when the stream emits an error event - #2021
fix(responses): reject with OpenAIError when the stream emits an error event#2021morgan-coded wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34bfea5d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case 'error': { | ||
| // The API reports failures with an `error` event instead of a non-2xx response, so | ||
| // convert it here. Throwing hands it to `EventStream`'s error path, which rejects | ||
| // `finalResponse()` and notifies `error` listeners with the converted error. | ||
| throw new APIError(undefined, event, event.message, undefined); |
There was a problem hiding this comment.
When the server sends an error frame before response.created, this branch is never reached because accumulateResponse() first throws its generic “expected 'response.created'” OpenAIError. Consequently finalResponse() and error listeners lose the server's message, code, and parameter and do not receive the promised APIError. Handle event.type === 'error' before calling the accumulator.
Useful? React with 👍 / 👎.
| case 'error': { | ||
| // The API reports failures with an `error` event instead of a non-2xx response, so | ||
| // convert it here. Throwing hands it to `EventStream`'s error path, which rejects | ||
| // `finalResponse()` and notifies `error` listeners with the converted error. | ||
| throw new APIError(undefined, event, event.message, undefined); |
There was a problem hiding this comment.
When a for await consumer is still processing the raw error frame before requesting its next item, this throw reaches the iterator's error listener while readQueue is empty. That listener marks the iterator done but does not retain the failure, so the subsequent next() returns cleanly with done: true instead of rejecting with this APIError; an await in the loop body makes this especially likely. Convert the frame before emitting it through event, or store the failure so the next iterator read rejects.
Useful? React with 👍 / 👎.
1 participant
Changes being requested
Fixes #2020
An API
errorframe reachingResponseStreamrejectsfinalResponse()with the raw frame as a plain object —instanceof OpenAIErroris false — on both the live path (the SSE bare-data arm only converts on adata.errorkey, which{"type":"error",...}doesn't carry) and thefromReadableStreamreplay path..on('error')listeners receive the same raw frame, and the declared event type asserted it (ResponseEventsmappederrorto the rawResponseErrorEvent).The fix is a real
case 'error':in#addEventthat throwsnew APIError(undefined, event, event.message, undefined)— the call shapecore/streaming.tsalready uses for its conversions.APIErrorextendsOpenAIErrorand readscode/paramstraight off the frame, keeping the whole frame on.error. Throwing routes throughEventStream's own error path (its_emit('error', …)contract allows only#handleErrorto call it), sofinalResponse()and.on('error')observe the same converted object — the regression test asserts identity, plus message and code. Red onmain(Received constructor: Object), green with the fix; full suite 1463 passing, snapshots untouched.One line beyond the runtime fix:
'error'joins theResponseEventsOmitlist (alongside the two delta events there for the same reason), so.on('error')is typedOpenAIError. It's a narrowing — only code relying on the raw-frame behavior would notice — and easy to drop if you'd rather keep the type surface untouched.Additional context & links
Two adjacent things deliberately left out; flag if you'd like either:
core/streaming.tscarries a TODO ("Is this where the error should be thrown?") in the arm these frames pass through — converting there would cover every consumer but widens the blast radius to Chat Completions.errorframe arriving beforeresponse.creatednever reaches this switch:ResponseAccumulatorthrows its own generic error first, losing the server's message and code. Happy to follow up separately.