fix: handle streamed provider errors - #1901
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds detection and handling of inline upstream SSE errors and truncated upstream streams in the gateway streaming handler, emits content-filter SSE sequences or structured Changes
Sequence DiagramsequenceDiagram
actor Client
participant Gateway as Gateway<br/>(Streaming Handler)
participant Upstream as Upstream<br/>(OpenAI-like)
Client->>Gateway: POST /v1/chat/completions (stream: true)
Gateway->>Upstream: Forward streaming request
Upstream-->>Gateway: SSE stream (role/content chunks...)
rect rgba(255,200,100,0.5)
note right of Upstream: Upstream may send inline SSE error\n(payload with data.error) or truncate (no final chunk/[DONE])
Upstream-->>Gateway: SSE `data.error` OR abrupt close
end
rect rgba(255,100,100,0.5)
Gateway->>Gateway: Parse SSE/error, infer status/code and finishReason\nmark streamingError / sawUpstreamDoneSentinel
alt finishReason == "content_filter"
Gateway->>Client: Emit content-filter SSE sequence (chunk with finish_reason:"content_filter", usage) then `[DONE]`
else
Gateway->>Client: Emit SSE `event: error` (upstream_error) then `[DONE]`
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves streaming robustness by detecting upstream SSE streams that terminate without a terminal finish reason or [DONE], and surfacing those cases as a structured upstream_error with code stream_truncated, along with a regression test that reproduces a truncated upstream stream.
Changes:
- Add truncated-stream detection in the chat streaming finalization path and emit an
errorSSE event withcode: stream_truncated. - Extend the mock OpenAI server to support streaming chat completions and intentionally truncate the stream when triggered.
- Add a Vitest regression test validating client-visible SSE error shape and log output for truncated upstream streams.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/gateway/src/test-utils/mock-openai-server.ts | Adds SSE streaming behavior for /v1/chat/completions and a trigger to end the stream early (truncate). |
| apps/gateway/src/chat/chat.ts | Detects “ended without terminal event” streams and converts them into upstream_error / stream_truncated, emitting error + [DONE]. |
| apps/gateway/src/api.spec.ts | Adds a regression test asserting truncated upstream streams produce the expected SSE error and logged finishReason. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const responseText = | ||
| buffer.trim().length > 0 | ||
| ? buffer.substring(0, 5000) | ||
| : "Stream ended before a terminal finish reason or [DONE] event"; |
There was a problem hiding this comment.
buffer.trim() creates a new string and can be unnecessarily expensive with large buffers (MAX_STREAMING_BUFFER_MB defaults to 50MB). Consider avoiding trim() and instead checking for non-whitespace without copying (e.g., /\S/.test(buffer)), then using buffer.slice(0, 5000) when present.
| const responseText = | |
| buffer.trim().length > 0 | |
| ? buffer.substring(0, 5000) | |
| : "Stream ended before a terminal finish reason or [DONE] event"; | |
| const hasNonWhitespaceContent = /\S/.test(buffer); | |
| const responseText = hasNonWhitespaceContent | |
| ? buffer.slice(0, 5000) | |
| : "Stream ended before a terminal finish reason or [DONE] event"; |
![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: 936bd2a622
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
| } else { | ||
| } else if (!streamingError) { |
There was a problem hiding this comment.
Changing this branch to else if (!streamingError) drops the fallback finalization path for streams that set streamingError earlier without emitting an error SSE (for example the JSON-parse path at apps/gateway/src/chat/chat.ts:4434). In buffering/healing mode, [DONE] is intentionally deferred from the read loop, so these requests can now exit without any terminal event, which causes OpenAI-style streaming clients to treat the stream as broken/hanging.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/test-utils/mock-openai-server.ts`:
- Around line 541-544: The filter callback for computing shouldTruncateStream
currently uses the any type; change it to a properly typed parameter (e.g.,
define or reuse an appropriate Message type or use an inline type like { role:
string; content: string } for msg) and update the predicate to use that type so
TypeScript linting passes; keep the check logic (msg.role === "user" &&
msg.content.includes("TRIGGER_TRUNCATED_STREAM")) and reference the same
shouldTruncateStream and body.messages variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6bec9602-a34e-4bd9-92ba-5f9de7d45e99
📒 Files selected for processing (3)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/test-utils/mock-openai-server.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4503-4540: The inline content_filter branch currently writes
zero-token SSE events but leaves streamingError as null so the later finally
block treats the stream as successful; after the two writeSSEAndCache calls in
the errorType === "content_filter" branch, set a non-null sentinel on
streamingError (e.g., assign a short Error or object like new
Error('content_filter') or {type:'content_filter'}) or flip a dedicated flag
(e.g., skipFinalizer = true) so the finally block can detect this and skip
re-estimating prompt usage, cost calculation, and caching for this terminated
stream; update the finally block checks to honor streamingError or skipFinalizer
to avoid treating inline content_filter as a normal successful response.
- Around line 5270-5272: The current guard using streamEndedWithoutTerminalEvent
(which checks only finishReason === null) misses cases where the upstream sent a
terminal chunk but never the upstream “[DONE]” sentinel, because doneSent only
tracks what we emitted downstream; add a new boolean flag (e.g.,
sawUpstreamDoneSentinel) that is set true when the code sees the upstream
“[DONE]” sentinel, update the streamEndedWithoutTerminalEvent condition to also
require sawUpstreamDoneSentinel === false (so it triggers if we never observed
upstream done), and mirror the same change at the other occurrence around the
5421 area; ensure you set sawUpstreamDoneSentinel where the upstream DONE is
parsed and use it in the downstream synthesis logic to avoid synthesizing a
clean completion when upstream didn’t send DONE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 698eb87a-ca6e-46b6-a90b-42bce4cf95ea
📒 Files selected for processing (5)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.spec.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/test-utils/mock-openai-server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/api.spec.ts
| if (errorType === "content_filter") { | ||
| await writeSSEAndCache({ | ||
| data: JSON.stringify({ | ||
| id: data.id ?? `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: data.created ?? Math.floor(Date.now() / 1000), | ||
| model: data.model ?? usedModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: {}, | ||
| finish_reason: "content_filter", | ||
| }, | ||
| ], | ||
| }), | ||
| id: String(eventId++), | ||
| }); | ||
| await writeSSEAndCache({ | ||
| data: JSON.stringify({ | ||
| id: `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: Math.floor(Date.now() / 1000), | ||
| model: data.model ?? usedModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: {}, | ||
| finish_reason: null, | ||
| }, | ||
| ], | ||
| usage: { | ||
| prompt_tokens: 0, | ||
| completion_tokens: 0, | ||
| total_tokens: 0, | ||
| }, | ||
| }), | ||
| id: String(eventId++), | ||
| }); |
There was a problem hiding this comment.
Skip the generic finalizer after inline content_filter termination.
This branch emits a zero-token content_filter stream, but it leaves streamingError as null. The finally block later still treats the request as a normal successful stream, so it can re-estimate prompt usage, calculate non-zero costs, and cache the response even though the client just received 0 tokens. That makes the inline SSE path disagree with the existing HTTP content_filter handling above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 4503 - 4540, The inline
content_filter branch currently writes zero-token SSE events but leaves
streamingError as null so the later finally block treats the stream as
successful; after the two writeSSEAndCache calls in the errorType ===
"content_filter" branch, set a non-null sentinel on streamingError (e.g., assign
a short Error or object like new Error('content_filter') or
{type:'content_filter'}) or flip a dedicated flag (e.g., skipFinalizer = true)
so the finally block can detect this and skip re-estimating prompt usage, cost
calculation, and caching for this terminated stream; update the finally block
checks to honor streamingError or skipFinalizer to avoid treating inline
content_filter as a normal successful response.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9a05a29d7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| typeof openAiCompatibleStreamError.status_code === "number" | ||
| ? openAiCompatibleStreamError.status_code | ||
| : typeof openAiCompatibleStreamError.status === "number" | ||
| ? openAiCompatibleStreamError.status | ||
| : 400; |
There was a problem hiding this comment.
When an inline SSE error payload lacks a numeric status/status_code, this code hard-defaults inferredStatusCode to 400, which drives getFinishReasonFromError(...) to client_error for non-filter cases. Providers commonly emit stream errors without HTTP status metadata (for example overload/internal stream errors), so these get mislabeled as client faults instead of upstream failures, skewing logs/metrics and downstream error handling that depends on upstream_error classification.
Useful? React with 👍 / 👎.
steebchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b3828275b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| data: "[DONE]", | ||
| id: String(eventId++), | ||
| }); | ||
| doneSent = true; |
There was a problem hiding this comment.
When the provider returns a non-2xx content-filter response, the finishReason === "content_filter" branch calls writeStreamingContentFilterResponse, and that helper assigns to doneSent at this line before let doneSent = false is initialized later in the function. That is a temporal-dead-zone access in JavaScript, so this path can throw ReferenceError and break streaming requests that should return a normal content_filter terminal chunk.
Useful? React with 👍 / 👎.
| await writeSSEAndCache({ | ||
| event: "done", | ||
| data: "[DONE]", | ||
| id: String(eventId++), | ||
| }); |
There was a problem hiding this comment.
In the OpenAI-compatible streamed error branch, the content_filter case calls writeStreamingContentFilterResponse, which already emits a done event and marks completion, but this unconditional block sends another done event immediately afterward. For streamed moderation blocks this produces two terminal events in one response, which can confuse strict SSE/OpenAI stream consumers and violates the expected single-terminator sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/gateway/src/chat/chat.ts (1)
5327-5329:⚠️ Potential issue | 🟠 MajorA missing upstream
[DONE]still slips through as success.This guard only checks
finishReason === null. If upstream sends the terminal chunk and then drops the connection before[DONE],finishReasonis already set, this branch is skipped, and the finalizer synthesizes a clean completion instead ofstream_truncated.Suggested fix
- const streamEndedWithoutTerminalEvent = - !streamingError && !canceled && finishReason === null; + const streamEndedWithoutTerminalEvent = + !streamingError && + !canceled && + (finishReason === null || !sawUpstreamDone);Set
sawUpstreamDone = truein the upstream[DONE]branch when parsing events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 5327 - 5329, The current terminal-check uses finishReason === null and thus treats a connection that sent the terminal chunk but never the upstream "[DONE]" as a success; update the upstream "[DONE]" parsing branch to set sawUpstreamDone = true when you detect the "[DONE]" event so later logic (which uses streamingError, canceled, finishReason and sawUpstreamDone) can correctly detect a truncated stream and synthesize stream_truncated instead of a clean completion; locate the event-parsing code that handles the upstream "[DONE]" token and add setting of sawUpstreamDone there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4592-4636: The code unconditionally sends a final "[DONE]" via
writeSSEAndCache even when writeStreamingContentFilterResponse already emitted
the terminal frame and set doneSent; update the control flow in the error
handling block (inside the branch that checks errorType === "content_filter") so
that after calling writeStreamingContentFilterResponse (and any awaiting), you
do not proceed to the unconditional writeSSEAndCache that emits "[DONE]" — i.e.,
only call the trailing writeSSEAndCache({event: "done", data: "[DONE]" ...})
when doneSent is still false (or skip it inside the content_filter branch); keep
references to writeStreamingContentFilterResponse, writeSSEAndCache, doneSent,
and eventId when implementing the guard.
- Around line 2995-3090: The helper writeStreamingContentFilterResponse closes
the stream by setting doneSent but doneSent is declared later, causing a
temporal dead zone when writeStreamingContentFilterResponse is invoked; move the
doneSent declaration to a point above the writeStreamingContentFilterResponse
function (or hoist its declaration) so doneSent exists before any calls to
writeStreamingContentFilterResponse, ensuring the function can assign to
doneSent without a runtime ReferenceError.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 5327-5329: The current terminal-check uses finishReason === null
and thus treats a connection that sent the terminal chunk but never the upstream
"[DONE]" as a success; update the upstream "[DONE]" parsing branch to set
sawUpstreamDone = true when you detect the "[DONE]" event so later logic (which
uses streamingError, canceled, finishReason and sawUpstreamDone) can correctly
detect a truncated stream and synthesize stream_truncated instead of a clean
completion; locate the event-parsing code that handles the upstream "[DONE]"
token and add setting of sawUpstreamDone there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71e7ff9d-dc57-4f6b-8c15-bf7d01fa2626
📒 Files selected for processing (2)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abde997896
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !streamingError && | ||
| !canceled && | ||
| !handledTerminalProviderEvent && | ||
| (!sawUpstreamDoneSentinel || finishReason === null); |
There was a problem hiding this comment.
This truncation predicate now requires an upstream "[DONE]" sentinel, so streams that end cleanly with a terminal finish_reason but no literal done marker are misclassified as upstream_error. That affects providers with non-OpenAI SSE termination semantics (for example, Anthropic terminal events that are mapped to finish_reason in transformStreamingToOpenai), causing successful responses to be followed by an error event and incorrect failure logging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/gateway/src/chat/chat.ts (2)
2995-3090:⚠️ Potential issue | 🔴 CriticalMove
doneSentabove the helper.Line 3855 can invoke
writeStreamingContentFilterResponse()on the HTTPcontent_filterpath before execution reaches thedoneSentinitialization near Line 4024. ThedoneSent = truewrite at Line 3089 is still in the TDZ then, so this branch will throw instead of returning the syntheticcontent_filterstream.🛠️ Suggested fix
+ let doneSent = false; // Track if [DONE] has been sent + const writeStreamingContentFilterResponse = async ({ billingModel, billingProvider, @@ - let doneSent = false; // Track if [DONE] has been sent let sawUpstreamDoneSentinel = false; let handledTerminalProviderEvent = false;#!/bin/bash sed -n '2995,3092p' apps/gateway/src/chat/chat.ts printf '\n--- content_filter call site ---\n' sed -n '3848,3868p' apps/gateway/src/chat/chat.ts printf '\n--- doneSent declaration ---\n' sed -n '4018,4028p' apps/gateway/src/chat/chat.tsAlso applies to: 3855-3858, 4025-4026
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 2995 - 3090, The helper writeStreamingContentFilterResponse closes the SSE stream and sets doneSent = true but doneSent is declared later, causing a TDZ when content_filter path calls that helper; move the doneSent variable declaration (and any initialization like let doneSent = false) above the writeStreamingContentFilterResponse function so doneSent exists before any potential invocation, then ensure any references to eventId or other shared variables used by writeStreamingContentFilterResponse remain in scope and initialized as well.
4595-4640:⚠️ Potential issue | 🟠 MajorDon't emit
[DONE]twice after inlinecontent_filtererrors.
writeStreamingContentFilterResponse()already writes the terminal event and flipsdoneSent. The unconditional[DONE]at Line 4635 sends a second terminator and will also cache two terminal frames for the same response.🛠️ Suggested fix
- await writeSSEAndCache({ - event: "done", - data: "[DONE]", - id: String(eventId++), - }); - doneSent = true; + if (!doneSent) { + await writeSSEAndCache({ + event: "done", + data: "[DONE]", + id: String(eventId++), + }); + doneSent = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 4595 - 4640, The code currently always calls writeSSEAndCache(... "done" "[DONE]") after handling errors, which causes a duplicate terminal event when writeStreamingContentFilterResponse() has already emitted a terminal event and set doneSent/handledTerminalProviderEvent; update the logic in the error handling branch around writeStreamingContentFilterResponse and the unconditional writeSSEAndCache call so that you only send the final done event if neither handledTerminalProviderEvent nor doneSent is true (i.e., guard the final writeSSEAndCache("done", "[DONE]") with if (!handledTerminalProviderEvent && !doneSent) to avoid emitting and caching duplicate terminal frames).
🧹 Nitpick comments (2)
apps/gateway/src/api.spec.ts (1)
989-1099: Considertest.eachfor the two truncation cases.These tests only vary by trigger text while the setup and assertions stay the same. Parameterizing them would make future stream-termination cases cheaper to add and less likely to drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/api.spec.ts` around lines 989 - 1099, Replace the two nearly identical tests "streaming request surfaces truncated upstream streams" and "streaming request surfaces missing upstream done sentinel" with a single parameterized test using test.each over the trigger strings ("TRIGGER_TRUNCATED_STREAM", "TRIGGER_FINISH_WITHOUT_DONE"); keep the shared setup (db inserts for apiKey and providerKey), the request to /v1/chat/completions (model "llmgateway/custom", stream: true) and the common assertions against readAll(res.body) and waitForLogs(1). In the new test.each callback use a parameter like trigger to fill the message content, retain checks for streamResult.hasContent/hasError, errorEvents[0].error.type/code, and logs[0] assertions (finishReason, unifiedFinishReason, hasError, errorDetails.statusCode/statusText) so behavior is identical while removing duplicated code across the two original tests.apps/gateway/src/test-utils/mock-openai-server.ts (1)
675-679: Remove the non-standardeventfield from terminal sentinel.OpenAI-compatible streams terminate with a plain SSE
data: [DONE]message without aneventfield. The currentevent: "done"reduces mock fidelity and can mask parser differences with the actual upstream API.♻️ Proposed change
await stream.writeSSE({ - event: "done", data: "[DONE]", id: String(eventId++), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/test-utils/mock-openai-server.ts` around lines 675 - 679, The terminal sentinel sent by stream.writeSSE in the mock server currently includes a non-standard event field; modify the call in the mock-openai code path that constructs the sentinel (the stream.writeSSE invocation that uses eventId) to omit the event property and send only the standard SSE message with data: "[DONE]" (and optional id) so the mock matches OpenAI's plain "data: [DONE]" termination format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4547-4615: The SSE error handling currently sets streamingError
immediately which causes createLogEntry() to prefer that synthesized object over
the original provider chunk; instead introduce and populate a separate
raw-payload variable (e.g., streamingSseErrorPayload or
rawProviderSseEventPayload) with errorResponseText (or the truncated
rawProviderSseEvent) when you build errorResponseText, leave streamingError null
so the final logging still uses streamingSseErrorPayload ??
streamingRawResponseData/rawUpstreamData, and only construct/set streamingError
after logging if needed; update references around getFinishReasonFromError,
openAiCompatibleStreamError, and the content_filter path
(writeStreamingContentFilterResponse) to use the new raw-payload variable rather
than overloading streamingError.
- Around line 3006-3079: The code uses estimateTokens to set promptTokenCount
but the canonical prompt count comes from calculateCosts (used elsewhere as
costs.promptTokens); change the SSE payload to use the prompt-token value
returned by calculateCosts (e.g., streamingCosts.promptTokens or
streamingCosts.promptTokenCount) instead of the local promptTokenCount derived
from estimateTokens, applying the same Math.max/round fallback if the
calculateCosts field is missing; update this block (where promptTokenCount and
streamingCosts are used before writeSSEAndCache) and the similar block at the
other location (lines noted in the review) so usage.total_tokens and related
fields report the same canonical prompt-token value used for billing.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2995-3090: The helper writeStreamingContentFilterResponse closes
the SSE stream and sets doneSent = true but doneSent is declared later, causing
a TDZ when content_filter path calls that helper; move the doneSent variable
declaration (and any initialization like let doneSent = false) above the
writeStreamingContentFilterResponse function so doneSent exists before any
potential invocation, then ensure any references to eventId or other shared
variables used by writeStreamingContentFilterResponse remain in scope and
initialized as well.
- Around line 4595-4640: The code currently always calls writeSSEAndCache(...
"done" "[DONE]") after handling errors, which causes a duplicate terminal event
when writeStreamingContentFilterResponse() has already emitted a terminal event
and set doneSent/handledTerminalProviderEvent; update the logic in the error
handling branch around writeStreamingContentFilterResponse and the unconditional
writeSSEAndCache call so that you only send the final done event if neither
handledTerminalProviderEvent nor doneSent is true (i.e., guard the final
writeSSEAndCache("done", "[DONE]") with if (!handledTerminalProviderEvent &&
!doneSent) to avoid emitting and caching duplicate terminal frames).
---
Nitpick comments:
In `@apps/gateway/src/api.spec.ts`:
- Around line 989-1099: Replace the two nearly identical tests "streaming
request surfaces truncated upstream streams" and "streaming request surfaces
missing upstream done sentinel" with a single parameterized test using test.each
over the trigger strings ("TRIGGER_TRUNCATED_STREAM",
"TRIGGER_FINISH_WITHOUT_DONE"); keep the shared setup (db inserts for apiKey and
providerKey), the request to /v1/chat/completions (model "llmgateway/custom",
stream: true) and the common assertions against readAll(res.body) and
waitForLogs(1). In the new test.each callback use a parameter like trigger to
fill the message content, retain checks for streamResult.hasContent/hasError,
errorEvents[0].error.type/code, and logs[0] assertions (finishReason,
unifiedFinishReason, hasError, errorDetails.statusCode/statusText) so behavior
is identical while removing duplicated code across the two original tests.
In `@apps/gateway/src/test-utils/mock-openai-server.ts`:
- Around line 675-679: The terminal sentinel sent by stream.writeSSE in the mock
server currently includes a non-standard event field; modify the call in the
mock-openai code path that constructs the sentinel (the stream.writeSSE
invocation that uses eventId) to omit the event property and send only the
standard SSE message with data: "[DONE]" (and optional id) so the mock matches
OpenAI's plain "data: [DONE]" termination format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf83947d-5036-47e6-a148-67573362fa20
📒 Files selected for processing (3)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/test-utils/mock-openai-server.ts
| const { calculatedPromptTokens } = estimateTokens( | ||
| billingProvider, | ||
| messages, | ||
| null, | ||
| null, | ||
| 0, | ||
| ); | ||
| const promptTokenCount = Math.max( | ||
| 1, | ||
| Math.round(calculatedPromptTokens ?? 1), | ||
| ); | ||
| const streamingCosts = await calculateCosts( | ||
| billingModel, | ||
| billingProvider, | ||
| promptTokenCount, | ||
| 0, | ||
| null, | ||
| { | ||
| prompt: messages | ||
| .map((m) => messageContentToString(m.content)) | ||
| .join("\n"), | ||
| completion: "", | ||
| }, | ||
| null, | ||
| 0, | ||
| image_config?.image_size, | ||
| inputImageCount, | ||
| 0, | ||
| project.organizationId, | ||
| ); | ||
|
|
||
| await writeSSEAndCache({ | ||
| data: JSON.stringify({ | ||
| id: `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: Math.floor(Date.now() / 1000), | ||
| model: responseModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: {}, | ||
| finish_reason: "content_filter", | ||
| }, | ||
| ], | ||
| ...(metadata && { metadata }), | ||
| }), | ||
| id: String(eventId++), | ||
| }); | ||
|
|
||
| await writeSSEAndCache({ | ||
| data: JSON.stringify({ | ||
| id: `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: Math.floor(Date.now() / 1000), | ||
| model: responseModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: {}, | ||
| finish_reason: null, | ||
| }, | ||
| ], | ||
| usage: { | ||
| prompt_tokens: promptTokenCount, | ||
| completion_tokens: 0, | ||
| total_tokens: promptTokenCount, | ||
| cost_usd_total: streamingCosts.totalCost, | ||
| cost_usd_input: streamingCosts.inputCost, | ||
| cost_usd_output: streamingCosts.outputCost, | ||
| cost_usd_cached_input: streamingCosts.cachedInputCost, | ||
| cost_usd_request: streamingCosts.requestCost, | ||
| cost_usd_image_input: streamingCosts.imageInputCost, | ||
| cost_usd_image_output: streamingCosts.imageOutputCost, | ||
| }, |
There was a problem hiding this comment.
Blocked multimodal content_filter paths still under-report usage.
Line 3069 and Lines 3785-3799 use the raw estimateTokens() result, but later Line 5777 treats costs.promptTokens as the canonical prompt count because it folds in image-input adjustments. A content-filtered image request can therefore return/log lower prompt_tokens and total_tokens than the billed cost. Reuse the canonical prompt-token value from calculateCosts() here as well.
Also applies to: 3785-3799
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 3006 - 3079, The code uses
estimateTokens to set promptTokenCount but the canonical prompt count comes from
calculateCosts (used elsewhere as costs.promptTokens); change the SSE payload to
use the prompt-token value returned by calculateCosts (e.g.,
streamingCosts.promptTokens or streamingCosts.promptTokenCount) instead of the
local promptTokenCount derived from estimateTokens, applying the same
Math.max/round fallback if the calculateCosts field is missing; update this
block (where promptTokenCount and streamingCosts are used before
writeSSEAndCache) and the similar block at the other location (lines noted in
the review) so usage.total_tokens and related fields report the same canonical
prompt-token value used for billing.
| const errorResponseText = JSON.stringify(data); | ||
| if ( | ||
| debugMode && | ||
| streamingRawResponseData.length < MAX_RAW_DATA_SIZE | ||
| ) { | ||
| const rawProviderSseEvent = `data: ${errorResponseText}\n\n`; | ||
| streamingRawResponseData += rawProviderSseEvent.substring( | ||
| 0, | ||
| Math.max( | ||
| 0, | ||
| MAX_RAW_DATA_SIZE - streamingRawResponseData.length, | ||
| ), | ||
| ); | ||
| } | ||
| const inferredStatusCode = | ||
| typeof openAiCompatibleStreamError.status_code === "number" | ||
| ? openAiCompatibleStreamError.status_code | ||
| : typeof openAiCompatibleStreamError.status === "number" | ||
| ? openAiCompatibleStreamError.status | ||
| : 400; | ||
| const errorType = getFinishReasonFromError( | ||
| inferredStatusCode, | ||
| errorResponseText, | ||
| ); | ||
| const errorMessage = | ||
| typeof openAiCompatibleStreamError.message === "string" | ||
| ? openAiCompatibleStreamError.message | ||
| : "Upstream provider returned a streaming error"; | ||
| const errorCode = | ||
| typeof openAiCompatibleStreamError.code === "string" | ||
| ? openAiCompatibleStreamError.code | ||
| : typeof openAiCompatibleStreamError.type === "string" | ||
| ? openAiCompatibleStreamError.type | ||
| : errorType; | ||
|
|
||
| logger.info("[streaming] Provider SSE error received", { | ||
| requestId, | ||
| provider: usedProvider, | ||
| model: usedModel, | ||
| errorType, | ||
| errorCode, | ||
| inferredStatusCode, | ||
| errorMessage, | ||
| errorPayload: errorResponseText.substring(0, 5000), | ||
| }); | ||
|
|
||
| finishReason = errorType; | ||
|
|
||
| if (errorType === "content_filter") { | ||
| await writeStreamingContentFilterResponse({ | ||
| billingModel: usedModel, | ||
| billingProvider: usedProvider, | ||
| responseModel: data.model ?? usedModel, | ||
| }); | ||
| handledTerminalProviderEvent = true; | ||
| } else { | ||
| streamingError = { | ||
| message: errorMessage, | ||
| type: errorType, | ||
| code: errorCode, | ||
| details: { | ||
| statusCode: inferredStatusCode, | ||
| statusText: | ||
| typeof openAiCompatibleStreamError.type === "string" | ||
| ? openAiCompatibleStreamError.type | ||
| : "stream_error", | ||
| responseText: errorResponseText, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The new SSE error path still drops the original provider chunk from debug fields.
Lines 4547-4559 buffer the raw provider payload, but Line 4603 immediately makes streamingError non-null. The final createLogEntry() later prefers streamingError ?? streamingRawResponseData / streamingError ?? rawUpstreamData, so non-content_filter inline errors will log the synthesized error object instead of the original SSE frame this PR is trying to preserve. Keep separate raw-payload variables for logging rather than overloading streamingError.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 4547 - 4615, The SSE error
handling currently sets streamingError immediately which causes createLogEntry()
to prefer that synthesized object over the original provider chunk; instead
introduce and populate a separate raw-payload variable (e.g.,
streamingSseErrorPayload or rawProviderSseEventPayload) with errorResponseText
(or the truncated rawProviderSseEvent) when you build errorResponseText, leave
streamingError null so the final logging still uses streamingSseErrorPayload ??
streamingRawResponseData/rawUpstreamData, and only construct/set streamingError
after logging if needed; update references around getFinishReasonFromError,
openAiCompatibleStreamError, and the content_filter path
(writeStreamingContentFilterResponse) to use the new raw-payload variable rather
than overloading streamingError.
Summary
upstream_errorwithstream_truncateddata_inspection_failedcontent_filterinstead of falling through tounknownor truncation handlingrawResponseandupstreamResponseValidation
pnpm exec vitest run apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts --no-file-parallelismpnpm exec vitest run apps/gateway/src/api.spec.ts --no-file-parallelismpnpm buildpnpm formatSummary by CodeRabbit
New Features
Bug Fixes
Tests