fix: improve streaming error diagnostics and logger serialization - #1866
Conversation
WalkthroughAdds a streaming error normalization utility and integrates it into gateway chat streaming/error flows, computes and attaches unified finish reasons across upstream failure paths, and updates logger methods to accept Error extras for structured logging. Tests added for normalization and logger behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as chat.ts
participant Upstream as Provider
participant Normalizer as normalize-streaming-error
participant Logger
participant Responder as SSE Response
Client->>Gateway: request stream
Gateway->>Upstream: open/read upstream stream
Upstream-->>Gateway: stream chunk / error
Gateway->>Normalizer: normalizeStreamingError(error, phase, provider, model, bufferSnapshot...)
Normalizer-->>Gateway: NormalizedStreamingError { client, log, statusCode, details }
Gateway->>Logger: logger.error("Error reading upstream stream", NormalizedStreamingError.log)
Gateway->>Responder: send SSE event:"error" with normalized.client (+ unified_finish_reason)
Responder-->>Client: SSE error event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 observability and client-facing diagnostics for streaming failures in the gateway by (1) normalizing mid-stream upstream/provider read errors into a structured error payload and (2) enhancing logger behavior so Error instances passed to non-error log levels retain stack/context.
Changes:
- Add
normalizeStreamingError()utility + tests to classify upstream stream terminations and produce consistent client/log shapes. - Expand gateway streaming read failure logging with upstream response/header context and normalized diagnostics, and forward structured SSE
errorevents to clients. - Update the shared logger to map
Errorextras passed totrace/debug/info/warnintoerrfor proper serialization; add unit test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/logger/src/index.ts | Adds mergeOptionalArg() and extends trace/debug/info/warn to accept Error and serialize via err. |
| packages/logger/src/logger.spec.ts | Adds coverage ensuring warn() maps an Error extra into { err }. |
| apps/gateway/src/chat/tools/normalize-streaming-error.ts | New normalization helper producing structured client SSE error payload + log diagnostics. |
| apps/gateway/src/chat/tools/normalize-streaming-error.spec.ts | Tests terminated-stream classification vs generic streaming errors. |
| apps/gateway/src/chat/chat.ts | Uses normalization helper for stream read failures; improves error logging context and SSE error payload structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ? "Upstream Stream Terminated" | ||
| : "Streaming Read Error"; | ||
| const message = terminated | ||
| ? "Upstream stream terminated unexpectedly before completion" | ||
| : `Streaming error: ${rawMessage}`; |
There was a problem hiding this comment.
phase is part of the options/details but it isn’t used to shape statusText/message (e.g. an upstream_connect failure that matches the termination heuristics would still be labeled "Upstream Stream Terminated"). Consider either (a) restricting the termination classification to phase === "upstream_read", or (b) branching statusText/message on phase so connect vs read failures are described accurately.
| ? "Upstream Stream Terminated" | |
| : "Streaming Read Error"; | |
| const message = terminated | |
| ? "Upstream stream terminated unexpectedly before completion" | |
| : `Streaming error: ${rawMessage}`; | |
| ? phase === "upstream_connect" | |
| ? "Upstream Connection Terminated" | |
| : "Upstream Stream Terminated" | |
| : phase === "upstream_connect" | |
| ? "Upstream Connection Error" | |
| : "Streaming Read Error"; | |
| const message = terminated | |
| ? phase === "upstream_connect" | |
| ? "Upstream connection terminated unexpectedly before completion" | |
| : "Upstream stream terminated unexpectedly before completion" | |
| : phase === "upstream_connect" | |
| ? `Streaming connection error: ${rawMessage}` | |
| : `Streaming error: ${rawMessage}`; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/gateway/src/chat/tools/normalize-streaming-error.ts (1)
52-79: Consider extracting error code retrieval intoextractErrorCauseor a shared utility.Both
getErrorCodehere andextractErrorCausein the sibling module traverse the error cause chain up to 5 levels deep. This creates a minor duplication of the traversal logic.♻️ Optional: Unify cause chain traversal
You could potentially extend
extractErrorCauseto also return error codes, or create a shared helper that both functions use. However, keeping them separate is also reasonable given their different purposes (code extraction vs. human-readable cause string).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/normalize-streaming-error.ts` around lines 52 - 79, The getErrorCode function duplicates cause-chain traversal logic found in extractErrorCause; refactor by extracting the traversal into a shared helper (e.g., traverseErrorCauses) or extend extractErrorCause to optionally return a code, then have getErrorCode call that helper to locate a string-valued code up to the same depth (5) instead of reimplementing the loop; update getErrorCode to delegate to the shared utility and remove the inline traversal so both modules reuse the same cause-chain logic.packages/logger/src/logger.spec.ts (1)
50-69: Test correctly validates Error serialization, but usesas anycast.The test properly verifies that
Errorinstances passed towarn()are serialized under anerrfield. Theas anycast on line 56 is used to access the privateloggerproperty for stubbing, which is a common pattern in tests. Consider exposing a test hook or using dependency injection if this pattern becomes prevalent.As per coding guidelines: "Never use
anyoras anyin TypeScript unless absolutely necessary" - in test code for accessing private members, this is a reasonable exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/logger/src/logger.spec.ts` around lines 50 - 69, The test uses an `as any` cast to access the private `logger` property on the object returned by createLogger to stub `customLogger.logger`; replace the cast by adding a small test-only access point or injection: either add an optional parameter to createLogger to inject a backing logger for tests or expose a test-only getter (e.g., getInternalLogger(instance)) that returns the private `logger`, then update the spec to call that accessor instead of using `as any`; keep references to createLogger, customLogger.logger and the warn call so the test still asserts that warn(serializes Error under err, message) correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/gateway/src/chat/tools/normalize-streaming-error.ts`:
- Around line 52-79: The getErrorCode function duplicates cause-chain traversal
logic found in extractErrorCause; refactor by extracting the traversal into a
shared helper (e.g., traverseErrorCauses) or extend extractErrorCause to
optionally return a code, then have getErrorCode call that helper to locate a
string-valued code up to the same depth (5) instead of reimplementing the loop;
update getErrorCode to delegate to the shared utility and remove the inline
traversal so both modules reuse the same cause-chain logic.
In `@packages/logger/src/logger.spec.ts`:
- Around line 50-69: The test uses an `as any` cast to access the private
`logger` property on the object returned by createLogger to stub
`customLogger.logger`; replace the cast by adding a small test-only access point
or injection: either add an optional parameter to createLogger to inject a
backing logger for tests or expose a test-only getter (e.g.,
getInternalLogger(instance)) that returns the private `logger`, then update the
spec to call that accessor instead of using `as any`; keep references to
createLogger, customLogger.logger and the warn call so the test still asserts
that warn(serializes Error under err, message) correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aab63360-1236-4f54-b5d5-cd7d6cf11de6
📒 Files selected for processing (5)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/normalize-streaming-error.spec.tsapps/gateway/src/chat/tools/normalize-streaming-error.tspackages/logger/src/index.tspackages/logger/src/logger.spec.ts
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/chat/chat.ts`:
- Around line 127-164: In getUnifiedFinishReasonForError, remove the early
return when errorType is falsy so the function will fall through to the existing
getUnifiedFinishReason and HTTP status mapping logic; keep the existing
special-case branches (errorType === "invalid_request_error",
"upstream_timeout", "request_canceled"/"canceled") intact, call
getUnifiedFinishReason(errorType, undefined) next, and only if that still yields
UNKNOWN use the status checks (401/403 => GATEWAY_ERROR, 4xx => CLIENT_ERROR,
5xx => UPSTREAM_ERROR) before returning UNKNOWN—this ensures missing error.type
will allow the status-based fallback to run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ba3bd408-b8e0-47fd-9e7c-55feaa80c28d
📒 Files selected for processing (1)
apps/gateway/src/chat/chat.ts
| function getUnifiedFinishReasonForError( | ||
| errorType: string | undefined, | ||
| status?: number, | ||
| ) { | ||
| if (!errorType) { | ||
| return UnifiedFinishReason.UNKNOWN; | ||
| } | ||
|
|
||
| if (errorType === "invalid_request_error") { | ||
| return UnifiedFinishReason.CLIENT_ERROR; | ||
| } | ||
|
|
||
| if (errorType === "upstream_timeout") { | ||
| return UnifiedFinishReason.UPSTREAM_ERROR; | ||
| } | ||
|
|
||
| if (errorType === "request_canceled" || errorType === "canceled") { | ||
| return UnifiedFinishReason.CANCELED; | ||
| } | ||
|
|
||
| const unifiedFinishReason = getUnifiedFinishReason(errorType, undefined); | ||
| if (unifiedFinishReason !== UnifiedFinishReason.UNKNOWN) { | ||
| return unifiedFinishReason; | ||
| } | ||
|
|
||
| if (status !== undefined) { | ||
| if (status === 401 || status === 403) { | ||
| return UnifiedFinishReason.GATEWAY_ERROR; | ||
| } | ||
| if (status >= 400 && status < 500) { | ||
| return UnifiedFinishReason.CLIENT_ERROR; | ||
| } | ||
| if (status >= 500) { | ||
| return UnifiedFinishReason.UPSTREAM_ERROR; | ||
| } | ||
| } | ||
|
|
||
| return UnifiedFinishReason.UNKNOWN; |
There was a problem hiding this comment.
Let the status fallback run when error.type is missing.
The early return on Line 131 makes the status mapping on Line 152 unreachable. Any passthrough payload that has an error object but omits error.type will therefore still get unified_finish_reason: "unknown" in the branches at Line 3232 and Line 5937, even though the HTTP status is already available.
🛠️ Suggested fix
function getUnifiedFinishReasonForError(
errorType: string | undefined,
status?: number,
) {
- if (!errorType) {
- return UnifiedFinishReason.UNKNOWN;
- }
-
if (errorType === "invalid_request_error") {
return UnifiedFinishReason.CLIENT_ERROR;
}
@@
- const unifiedFinishReason = getUnifiedFinishReason(errorType, undefined);
- if (unifiedFinishReason !== UnifiedFinishReason.UNKNOWN) {
- return unifiedFinishReason;
+ if (errorType) {
+ const unifiedFinishReason = getUnifiedFinishReason(errorType, undefined);
+ if (unifiedFinishReason !== UnifiedFinishReason.UNKNOWN) {
+ return unifiedFinishReason;
+ }
}🤖 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 127 - 164, In
getUnifiedFinishReasonForError, remove the early return when errorType is falsy
so the function will fall through to the existing getUnifiedFinishReason and
HTTP status mapping logic; keep the existing special-case branches (errorType
=== "invalid_request_error", "upstream_timeout", "request_canceled"/"canceled")
intact, call getUnifiedFinishReason(errorType, undefined) next, and only if that
still yields UNKNOWN use the status checks (401/403 => GATEWAY_ERROR, 4xx =>
CLIENT_ERROR, 5xx => UPSTREAM_ERROR) before returning UNKNOWN—this ensures
missing error.type will allow the status-based fallback to run.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/chat.ts (1)
4348-4416:⚠️ Potential issue | 🔴 CriticalRemove unreachable ternary operator on
client.type.The condition at line 4384–4386 is always true and produces dead code:
normalizedStreamingError.client.type === "gateway_error" ? "gateway_error" : "upstream_error"The
NormalizedStreamingError.client.typeis a literal type"gateway_error"(per the interface definition), and the function always returns that value. The ternary operator should be replaced with the literal string"gateway_error":unifiedFinishReason: getUnifiedFinishReason("gateway_error", usedProvider)If the intent was to pass different finish reasons based on error classification, the
normalizeStreamingErrorfunction should be modified to return a discriminated union or the error type detection should happen separately.🤖 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 4348 - 4416, The ternary checking normalizedStreamingError.client.type is unreachable because client.type is always the literal "gateway_error"; replace the expression in the getUnifiedFinishReason call inside the error handling block so it passes the literal "gateway_error" (i.e., unifiedFinishReason: getUnifiedFinishReason("gateway_error", usedProvider)), or if you actually need different finish reasons, change normalizeStreamingError to return a discriminated union (e.g., client.type = "gateway_error" | "upstream_error") and update call-sites accordingly; locate the call in the error logging/streaming block where unifiedFinishReason is computed and make the corresponding change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4348-4416: The ternary checking
normalizedStreamingError.client.type is unreachable because client.type is
always the literal "gateway_error"; replace the expression in the
getUnifiedFinishReason call inside the error handling block so it passes the
literal "gateway_error" (i.e., unifiedFinishReason:
getUnifiedFinishReason("gateway_error", usedProvider)), or if you actually need
different finish reasons, change normalizeStreamingError to return a
discriminated union (e.g., client.type = "gateway_error" | "upstream_error") and
update call-sites accordingly; locate the call in the error logging/streaming
block where unifiedFinishReason is computed and make the corresponding change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed2e8d88-6fcd-4d93-bc09-66d48efdeae4
📒 Files selected for processing (1)
apps/gateway/src/chat/chat.ts
Summary
Errorinstances passed towarn/info/debug/trace, so warning-level logs keep stack tracesTesting
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests