fix: handle TimeoutError and AbortError with proper severity and status codes - #1661
Conversation
…us codes TimeoutError from AbortSignal.timeout() was reaching the global app.onError handler and being logged at error level as "Unhandled error" with a 500 response. These are expected operational conditions (slow upstream providers, client disconnects), not application bugs. - Gateway/API app.onError: detect TimeoutError (warn, 504) and AbortError (info, 499) - Add onError callbacks to all streamSSE calls so errors route through the structured logger instead of being silently swallowed by console.error - Anthropic handler streamSSE gets same treatment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds explicit TimeoutError (504) and AbortError (499) handling in global error handlers (API and gateway). Reworks the Anthropic streaming path to a consolidated SSE reader with per-chunk JSON guards, structured streaming events (message_start, content_block_*, message_delta), tool_call tracking, and robust reader cleanup and error/abort logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as Gateway
participant Anthropic as Anthropic
participant StreamHandler as StreamHandler
Client->>Gateway: POST /chat (stream)
Gateway->>Anthropic: Proxy request (fetch stream)
Anthropic-->>Gateway: Response stream (readable)
Gateway->>StreamHandler: start streamSSE(reader, callbacks)
loop for each chunk
Anthropic-->>StreamHandler: chunk (bytes)
StreamHandler->>StreamHandler: parse chunk -> JSON (try/catch)
alt first chunk with id
StreamHandler->>Client: SSE message_start
end
alt text/content block
StreamHandler->>Client: SSE content_block_start
StreamHandler->>Client: SSE content_block_delta
else tool_use block
StreamHandler->>StreamHandler: track tool_call index/id
StreamHandler->>Client: SSE content_block_start (tool)
StreamHandler->>Client: SSE content_block_delta (tool updates)
end
end
alt stream finished
StreamHandler->>Client: SSE content_block_stop (all)
StreamHandler->>Client: SSE message_delta (stop_reason, usage)
else stream error
StreamHandler->>StreamHandler: log error
StreamHandler->>Client: HTTP 500 / error SSE
else client abort
StreamHandler->>StreamHandler: log AbortError
StreamHandler->>Client: treat as aborted (499)
end
StreamHandler->>StreamHandler: reader.releaseLock() / cleanup (finally)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![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
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/anthropic/anthropic.ts (1)
810-822:⚠️ Potential issue | 🟡 Minor
determineStopReasonreturns"end_turn"forundefinedinput — should returnnull.The
defaultcase catchesundefinedand any unrecognized finish reasons, returning"end_turn". This is incorrect when called from the non-streaming path (line 797) wherefinish_reasonmay beundefined(e.g., ifchoicesis empty or missing). According to the Anthropic schema (line 139-141),stop_reasonis nullable, sonullis the appropriate value for unknown/absent reasons.Proposed fix
function determineStopReason( finishReason: string | undefined, ): "end_turn" | "max_tokens" | "stop_sequence" | "tool_use" | null { switch (finishReason) { case "stop": return "end_turn"; case "length": return "max_tokens"; case "tool_calls": return "tool_use"; default: - return "end_turn"; + return finishReason ? "end_turn" : null; } }
🤖 Fix all issues with AI agents
In `@apps/gateway/src/anthropic/anthropic.ts`:
- Around line 536-716: The inner try/catch currently wraps JSON.parse(data) plus
all stream.writeSSE calls, which hides write errors; change it so only the parse
is guarded: wrap JSON.parse(data) in its own try/catch that on parse failure
continues the loop, but move all logic that reads from the parsed chunk (the
handling of chunk, choice, delta, contentBlocks, tool calls, finish_reason and
all stream.writeSSE calls) outside that parse-catch so writeSSE failures
propagate to the outer error handler; specifically adjust the block around
JSON.parse(data) and ensure symbols like JSON.parse(data), chunk, choice, delta,
contentBlocks, toolCallBlockIndex, and stream.writeSSE are kept in the same
scope after a successful parse.
🧹 Nitpick comments (2)
apps/api/src/index.ts (1)
109-124: Sending a JSON response to a disconnected client may be futile.When the client has already disconnected (AbortError),
c.json(...)will attempt to write to a closed connection. This is likely harmless (Hono/the runtime should swallow the write error), but it's worth being aware that the 499 response body will never reach anyone. The logging atinfolevel is the real value here.The
499 as anycast is acceptable given Hono'sStatusCodetype doesn't include non-standard codes.apps/gateway/src/app.ts (1)
130-164: Consider extracting shared error-handling logic into a reusable utility.This block is nearly identical to
apps/api/src/index.tslines 92–124. If these two services evolve together, a shared helper (e.g., in a@llmgateway/sharedor@llmgateway/errorspackage) returning{ status, body, logLevel }would keep them in sync.Not urgent — fine to defer given these are separate apps.
There was a problem hiding this comment.
Pull request overview
This PR improves error handling for timeout and client disconnection scenarios in the LLM Gateway. Previously, TimeoutError from AbortSignal.timeout() and AbortError from client disconnects were reaching the global error handler and being logged at error level as "Unhandled error" with 500 status codes, despite being expected operational conditions rather than application bugs.
Changes:
- Added
TimeoutErrorandAbortErrordetection in gateway and APIapp.onErrorhandlers with appropriate log levels (warn/info) and HTTP status codes (504/499) - Added
onErrorcallbacks to all threestreamSSEcall sites to route escaped streaming errors through the structured pino logger instead ofconsole.error() - Preserved existing timeout handling logic in the chat handler - no changes to the catch blocks that already handle timeouts correctly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/gateway/src/app.ts | Added TimeoutError (504) and AbortError (499) handlers before the generic 500 fallback in the global error handler |
| apps/api/src/index.ts | Added TimeoutError (504) and AbortError (499) handlers before the generic 500 fallback in the global error handler |
| apps/gateway/src/chat/chat.ts | Added onError callbacks to streamSSE for cached stream replay and main streaming handler to log errors with appropriate severity |
| apps/gateway/src/anthropic/anthropic.ts | Added onError callback to streamSSE for Anthropic handler to log client disconnections at info level |
Comments suppressed due to low confidence (1)
apps/gateway/src/chat/chat.ts:1218
- This use of variable 'usedProvider' always evaluates to true.
if (!routingMetadata && usedProvider && usedProvider !== "llmgateway") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data: "[DONE]", | ||
| id: String(eventId++), | ||
| }); | ||
| doneSent = true; |
There was a problem hiding this comment.
The value assigned to doneSent here is unused.
| doneSent = true; |
| // Send final usage chunk before [DONE] if we have any usage data | ||
| if ( | ||
| finalPromptTokens !== null || | ||
| finalCompletionTokens !== null || |
There was a problem hiding this comment.
Variable 'finalCompletionTokens' cannot be of type null, but it is compared to an expression of type null.
| if ( | ||
| finalPromptTokens !== null || | ||
| finalCompletionTokens !== null || | ||
| finalTotalTokens !== null |
There was a problem hiding this comment.
Variable 'finalTotalTokens' cannot be of type null, but it is compared to an expression of type null.
| ); | ||
| // For cancelled requests, determine if we should include token counts for billing | ||
| const shouldIncludeTokensForBilling = | ||
| !canceled || (canceled && billCancelledRequests); |
There was a problem hiding this comment.
This use of variable 'canceled' always evaluates to true.
| !canceled || (canceled && billCancelledRequests); | |
| !canceled || billCancelledRequests; |
The actual root cause: after a successful fetch() in the non-streaming path, res.json() and res.text() can still throw TimeoutError if the abort signal fires during body consumption. These calls were outside the fetch try-catch, so the error propagated uncaught to app.onError. Wrap both body-read sites (error path res.text and success path res.json) with try-catch that detects TimeoutError and returns a proper 504 response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The body-read timeout catch blocks were returning 504 without calling insertLog, so these requests vanished from the database. Now they log with finishReason: "upstream_error", hasError: true, and errorDetails containing statusCode (the actual HTTP status from headers), statusText: "TimeoutError", matching the existing fetch-level timeout logging pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…JSON.parse The inner try-catch was wrapping both JSON.parse(data) and all stream.writeSSE calls, silently swallowing write errors. Now only JSON.parse is guarded (with continue on parse failure), so writeSSE failures propagate to the outer error handler where they become HTTPException 500s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/gateway/src/anthropic/anthropic.ts`:
- Around line 720-737: The catch currently wraps every error into a new
HTTPException which hides original AbortError/TimeoutError from the onError
callback; update the catch in the streaming handler so that if the caught error
is an AbortError (and optionally a TimeoutError) you re-throw the original error
instead of throwing HTTPException, otherwise wrap/throw HTTPException as before;
adjust the block around reader.read() / stream.writeSSE() and the catch that
throws HTTPException (referencing reader.read(), stream.writeSSE(),
HTTPException, onError, AbortError, TimeoutError) so client disconnects reach
the onError branch and trigger the info-level logging.
- Around line 728-738: Update the onError handler passed to streamSSE to match
the expected signature (error: Error, stream: SSEStreamingApi) by changing the
async callback from (error) => { ... } to (error, stream) => { ... }; inside the
handler preserve existing logic (check error.name === "AbortError" and log via
logger.info with message/path, otherwise logger.error) and, if needed, use the
supplied stream parameter to perform any stream-specific cleanup or logging for
the Anthropic streaming request (reference the existing onError callback in
anthropic.ts).
🧹 Nitpick comments (1)
apps/gateway/src/anthropic/anthropic.ts (1)
536-536:let chunk: any— consider a minimal structural type.The coding guidelines state: "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary." Sincechunkis the result ofJSON.parseon an OpenAI-format SSE payload, a lightweight interface (even a partial one covering.id,.model,.choices,.usage) would improve safety without much overhead. Low priority given the rest of the file also usesanyin several places.
| } catch (error) { | ||
| throw new HTTPException(500, { | ||
| message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`, | ||
| }); | ||
| } finally { | ||
| reader.releaseLock(); | ||
| } | ||
| } catch (error) { | ||
| throw new HTTPException(500, { | ||
| message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`, | ||
| }); | ||
| } finally { | ||
| reader.releaseLock(); | ||
| } | ||
| }); | ||
| }, | ||
| async (error) => { | ||
| if (error.name === "AbortError") { | ||
| logger.info("Anthropic streaming request aborted by client", { | ||
| message: error.message, | ||
| path: c.req.path, | ||
| }); | ||
| } else { | ||
| logger.error("Anthropic streaming error (escaped handler)", error); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Bug: catch wraps all errors (including AbortError) in HTTPException, making the onError AbortError branch dead code.
Every reader.read() and stream.writeSSE() call lives inside the try block (lines 506–719). When a client disconnects, the resulting AbortError is caught at line 720 and re-thrown as HTTPException(500). The onError callback then sees error.name === "HTTPException", never "AbortError", so the info-level logging path on line 729 is unreachable. This defeats the PR's stated goal of logging client disconnects at info level for the Anthropic handler.
Re-throw AbortError (and optionally TimeoutError) so they reach onError with their original identity:
Proposed fix
} catch (error) {
+ if (
+ error instanceof Error &&
+ (error.name === "AbortError" || error.name === "TimeoutError")
+ ) {
+ throw error;
+ }
throw new HTTPException(500, {
message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`,
});
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| throw new HTTPException(500, { | |
| message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`, | |
| }); | |
| } finally { | |
| reader.releaseLock(); | |
| } | |
| } catch (error) { | |
| throw new HTTPException(500, { | |
| message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`, | |
| }); | |
| } finally { | |
| reader.releaseLock(); | |
| } | |
| }); | |
| }, | |
| async (error) => { | |
| if (error.name === "AbortError") { | |
| logger.info("Anthropic streaming request aborted by client", { | |
| message: error.message, | |
| path: c.req.path, | |
| }); | |
| } else { | |
| logger.error("Anthropic streaming error (escaped handler)", error); | |
| } | |
| }, | |
| } catch (error) { | |
| if ( | |
| error instanceof Error && | |
| (error.name === "AbortError" || error.name === "TimeoutError") | |
| ) { | |
| throw error; | |
| } | |
| throw new HTTPException(500, { | |
| message: `Streaming error: ${error instanceof Error ? error.message : String(error)}`, | |
| }); | |
| } finally { | |
| reader.releaseLock(); | |
| } | |
| }, | |
| async (error) => { | |
| if (error.name === "AbortError") { | |
| logger.info("Anthropic streaming request aborted by client", { | |
| message: error.message, | |
| path: c.req.path, | |
| }); | |
| } else { | |
| logger.error("Anthropic streaming error (escaped handler)", error); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In `@apps/gateway/src/anthropic/anthropic.ts` around lines 720 - 737, The catch
currently wraps every error into a new HTTPException which hides original
AbortError/TimeoutError from the onError callback; update the catch in the
streaming handler so that if the caught error is an AbortError (and optionally a
TimeoutError) you re-throw the original error instead of throwing HTTPException,
otherwise wrap/throw HTTPException as before; adjust the block around
reader.read() / stream.writeSSE() and the catch that throws HTTPException
(referencing reader.read(), stream.writeSSE(), HTTPException, onError,
AbortError, TimeoutError) so client disconnects reach the onError branch and
trigger the info-level logging.
| async (error) => { | ||
| if (error.name === "AbortError") { | ||
| logger.info("Anthropic streaming request aborted by client", { | ||
| message: error.message, | ||
| path: c.req.path, | ||
| }); | ||
| } else { | ||
| logger.error("Anthropic streaming error (escaped handler)", error); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Hono version in apps/gateway
cd apps/gateway
cat package.json | jq '.dependencies.hono // .devDependencies.hono'Repository: theopenco/llmgateway
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Find and examine streamSSE in Hono types
fd -e "\.d\.ts$" node_modules/hono | xargs grep -l "streamSSE" | head -5Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look for the streamSSE function signature
rg "streamSSE" node_modules/hono --type ts --type tsx -A 10 | head -50Repository: theopenco/llmgateway
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find streaming-related files in hono
find node_modules/hono -name "*stream*" -type f | head -20Repository: theopenco/llmgateway
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for streamSSE signature - try different approach
rg "streamSSE" node_modules/hono -A 15 | head -60Repository: theopenco/llmgateway
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check the actual helper index to understand exports
find node_modules/hono -path "*/helper/*" -name "*.js" -o -name "*.d.ts" | grep -i streamRepository: theopenco/llmgateway
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find where streamSSE is imported and used
rg "streamSSE" apps/gateway/src --type ts -B 5 -A 15Repository: theopenco/llmgateway
Length of output: 9327
🏁 Script executed:
#!/bin/bash
# Check the full context around the code in question
sed -n '715,745p' apps/gateway/src/anthropic/anthropic.tsRepository: theopenco/llmgateway
Length of output: 751
🌐 Web query:
Hono streamSSE onError callback signature version 4.11.7 API
💡 Result:
In Hono v4.11.7, streamSSE takes an optional third argument onError with this signature:
streamSSE(
c: Context,
cb: (stream: SSEStreamingApi) => Promise<void>,
onError?: (e: Error, stream: SSEStreamingApi) => Promise<void>
): Response[1]
Fix onError callback signature to accept both error and stream parameters.
Hono v4.11.7's streamSSE accepts a third positional onError callback with signature (error: Error, stream: SSEStreamingApi) => Promise<void>, but the current callback only accepts the error parameter. Update to async (error, stream) => { ... } to match the expected signature.
🤖 Prompt for AI Agents
In `@apps/gateway/src/anthropic/anthropic.ts` around lines 728 - 738, Update the
onError handler passed to streamSSE to match the expected signature (error:
Error, stream: SSEStreamingApi) by changing the async callback from (error) => {
... } to (error, stream) => { ... }; inside the handler preserve existing logic
(check error.name === "AbortError" and log via logger.info with message/path,
otherwise logger.error) and, if needed, use the supplied stream parameter to
perform any stream-specific cleanup or logging for the Anthropic streaming
request (reference the existing onError callback in anthropic.ts).
Summary
AbortSignal.timeout()was reaching the globalapp.onErrorhandler and being logged at error level as"Unhandled error"with a 500 response. These are expected operational conditions (slow upstream providers), not application bugs.streamSSEcalls lackedonErrorcallbacks, causing errors inside streaming callbacks to be silently swallowed byconsole.error()instead of routed through the structured pino logger.Changes
app.onError: DetectTimeoutError(log atwarn, return 504 Gateway Timeout) andAbortError(log atinfo, return 499 Client Closed Request) before the generic 500 fallbackstreamSSEonError callbacks: Added to all threestreamSSEcall sites (main streaming handler, cached stream replay, Anthropic handler) so escaped errors are logged through the structured logger with appropriate severityTest plan
tsc --noEmitclean,turbo buildsucceeds)warnlevel instead oferrorinfolevel and return 499🤖 Generated with Claude Code
Summary by CodeRabbit