fix: propagate upstream stream errors to prevent truncated responses marked as success#917
Conversation
…marked as success When a SocketError occurs during active streaming, the nodeStream error handler in forwarder.ts was calling controller.close() which signals a normal end-of-stream. This caused ResponseHandler to set streamEndedNormally=true, marking truncated responses as successful. Changes: - forwarder.ts: Use controller.error(err) instead of controller.close() in the nodeStream error handler, so downstream readers detect the error - errors.ts: Export isTransportError() for use by response-handler - response-handler.ts: Add isTransportError() check in the catch block to route SocketError/ECONNRESET to STREAM_UPSTREAM_ABORTED path with proper circuit breaker accounting and 502 status recording Fixes ding113#916
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where upstream streaming errors, such as socket closures, were being silently swallowed and responses were incorrectly marked as successful, leading to truncated data for clients. The changes ensure that these transport errors are properly propagated and handled throughout the proxy, enabling accurate error reporting, logging, and system behavior, preventing misleading success states for partial responses. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthrough该PR修复了流式响应期间发生传输层错误（如SocketError）时的处理问题。通过导出传输错误判断函数、修改上游错误信号机制（从关闭改为错误传播），以及在响应处理器中添加对应的错误检测和清理逻辑，确保截断的流不会被误标记为成功。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical bug where upstream stream errors caused truncated responses to be marked as successful. The fix correctly propagates the error from the forwarder to the response-handler by using controller.error(err) instead of controller.close(). The new error handling logic in response-handler.ts for transport errors is robust and ensures that such failures are properly logged and recorded.
I have one suggestion in response-handler.ts to refactor some duplicated error handling logic into a helper function to improve maintainability. Overall, this is a solid fix that significantly improves the reliability of streaming responses.
| } catch (finalizeError) { | ||
| logger.error("ResponseHandler: Failed to finalize transport-error stream", { | ||
| taskId, | ||
| messageId: messageContext.id, | ||
| finalizeError, | ||
| }); | ||
|
|
||
| await persistRequestFailure({ | ||
| session, | ||
| messageContext, | ||
| statusCode: 502, | ||
| error: err, | ||
| taskId, | ||
| phase: "stream", | ||
| }); | ||
| } |
There was a problem hiding this comment.
This catch block for handling finalizeStream errors is very similar to other error handling paths in this file (e.g., for isResponseTimeout and isIdleTimeout). To improve maintainability and reduce code duplication, consider extracting this error handling logic into a shared helper function. This would make the error handling more consistent and easier to manage across different stream failure scenarios.
github-actions
bot
added
bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR correctly fixes a critical bug where SocketError during active streaming caused truncated responses to be marked as successful. The fix properly propagates upstream stream errors to downstream readers using controller.error(err) instead of controller.close().
PR Size: XS
- Lines changed: 45 (39 additions, 6 deletions)
- Files changed: 3
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (this PR fixes a silent failure bug)
- Type safety - Clean
- Documentation accuracy - Comments properly updated
- Test coverage - Adequate (function already tested via categorizeError path)
- Code clarity - Good
Technical Assessment
forwarder.ts: The change from controller.close() to controller.error(err) correctly signals an abnormal stream termination to downstream readers, allowing proper error detection.
response-handler.ts: The new isTransportError(err) branch correctly routes transport errors (SocketError, ECONNRESET, etc.) to the STREAM_UPSTREAM_ABORTED path with:
- Proper error logging with error name/code/message
- Circuit breaker failure recording via finalizeStream
- 502 status in internal bookkeeping
errors.ts: Simple export addition to make isTransportError available for response-handler.
The error handling follows the existing pattern established for upstream abort handling, maintaining consistency with the codebase.
Automated review by Claude AI
![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)
src/app/v1/_lib/proxy/forwarder.ts (1)
3994-4033:⚠️ Potential issue | 🟠 Major不要把非
end的"close"当成成功结束。把
"error"改成controller.error(err)是对的，但这里的"close"处理仍然无条件controller.close()。对 NodeReadable来说，"close"不等于成功读到 EOF；如果底层流被提前销毁/中断而没有走到"end"，下游还是会拿到done: true，截断响应仍可能被当成成功。建议只在已见到"end"时关闭，其余"close"按异常终止处理；如果希望继续落到STREAM_UPSTREAM_ABORTED，这里合成的异常也要和isTransportError()的判定条件对齐。建议修改
private static nodeStreamToWebStreamSafe( nodeStream: Readable, providerId: number, providerName: string ): ReadableStream<Uint8Array> { let chunkCount = 0; let totalBytes = 0; + let ended = false; + let cancelled = false; return new ReadableStream<Uint8Array>({ start(controller) { ... nodeStream.on("end", () => { + ended = true; logger.debug("ProxyForwarder: Node stream ended normally", { providerId, providerName, chunkCount, totalBytes, }); try { controller.close(); } catch { // 如果已关闭，忽略 } }); nodeStream.on("close", () => { logger.debug("ProxyForwarder: Node stream closed", { providerId, providerName, chunkCount, totalBytes, }); - try { - controller.close(); - } catch { - // 如果已关闭，忽略 - } + if (!ended && !cancelled) { + const prematureCloseError = Object.assign( + new Error("Upstream stream closed before end"), + { code: "ECONNRESET" } + ); + try { + controller.error(prematureCloseError); + } catch { + // 如果已关闭或已出错，忽略 + } + } }); nodeStream.on("error", (err) => { ... }); }, cancel(reason) { + cancelled = true; try { nodeStream.destroy( reason instanceof Error ? reason : reason ? new Error(String(reason)) : undefined );In Node.js streams, can a Readable emit "close" without first emitting "end", and does "close" indicate normal EOF or premature termination? Please cite the Node.js stream documentation semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 3994 - 4033, The "close" handler currently calls controller.close() unconditionally which is wrong because Node Readable can emit "close" without "end"; change the nodeStream handlers so that only the "end" event calls controller.close(), while the "close" event should call controller.error(...) with a synthetic transport error that aligns with isTransportError() and the STREAM_UPSTREAM_ABORTED sentinel (so ResponseHandler treats it as an aborted upstream), and preserve the existing "error" handler behavior that calls controller.error(err); update references in the same block around nodeStream.on("end"), nodeStream.on("close"), nodeStream.on("error"), controller.close, controller.error, isTransportError, and STREAM_UPSTREAM_ABORTED accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Line 4022: Remove the emoji from the inline comment in
src/app/v1/_lib/proxy/forwarder.ts — locate the comment containing "⭐
关键：将上游流错误传播到下游 reader，确保 ResponseHandler 能检测到截断" (near the
ResponseHandler/reader logic in forwarder.ts) and edit it to remove the "⭐"
character so it reads e.g. "关键：将上游流错误传播到下游 reader，确保 ResponseHandler 能检测到截断" to
comply with the repo rule banning emoji in code/comments.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3994-4033: The "close" handler currently calls controller.close()
unconditionally which is wrong because Node Readable can emit "close" without
"end"; change the nodeStream handlers so that only the "end" event calls
controller.close(), while the "close" event should call controller.error(...)
with a synthetic transport error that aligns with isTransportError() and the
STREAM_UPSTREAM_ABORTED sentinel (so ResponseHandler treats it as an aborted
upstream), and preserve the existing "error" handler behavior that calls
controller.error(err); update references in the same block around
nodeStream.on("end"), nodeStream.on("close"), nodeStream.on("error"),
controller.close, controller.error, isTransportError, and
STREAM_UPSTREAM_ABORTED accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1d5af14-0530-4135-a2a3-051812f4047b
📒 Files selected for processing (3)
src/app/v1/_lib/proxy/errors.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-handler.ts
| }); | ||
|
|
||
| // ⭐ 关键：吞掉错误事件，避免 "terminated" 冒泡 | ||
| // ⭐ 关键：将上游流错误传播到下游 reader，确保 ResponseHandler 能检测到截断 |
There was a problem hiding this comment.
移除这条注释里的 emoji。
⭐ 违反了当前仓库对 TS/JS 注释的约束，建议直接删掉，避免后续同类注释继续扩散。
As per coding guidelines **/*.{js,ts,tsx,jsx}: Never use emoji characters in any code, comments, or string literals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/forwarder.ts` at line 4022, Remove the emoji from the
inline comment in src/app/v1/_lib/proxy/forwarder.ts — locate the comment
containing "⭐ 关键：将上游流错误传播到下游 reader，确保 ResponseHandler 能检测到截断" (near the
ResponseHandler/reader logic in forwarder.ts) and edit it to remove the "⭐"
character so it reads e.g. "关键：将上游流错误传播到下游 reader，确保 ResponseHandler 能检测到截断" to
comply with the repo rule banning emoji in code/comments.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Problem
When a SocketError (
other side closed/UND_ERR_SOCKET) occurs during an active streaming response, the stream is silently truncated and marked as success. The client (e.g. Claude Code) receives a partial response with no error signal, causing it to stop mid-operation.Log evidence:
Root Cause
In
forwarder.ts,nodeStreamToWebStreamSafe()handles upstream stream errors by callingcontroller.close()— which signals a normal end-of-stream. The downstreamResponseHandlerreader gets{ done: true }, setsstreamEndedNormally = true, and finalizes as success.Fix
1.
forwarder.ts— Signal error instead of normal closeThis causes
reader.read()inResponseHandlerto reject with the original error.2.
errors.ts— ExportisTransportError()The function was already implemented but not exported. Now available for
response-handler.ts.3.
response-handler.ts— Route transport errors correctlyAdded
isTransportError()check in the catch block so SocketError/ECONNRESET errors are routed to theSTREAM_UPSTREAM_ABORTEDpath with proper:Relationship to #911
These are independent bugs in different code paths.
Changes
src/app/v1/_lib/proxy/forwarder.ts:controller.close()→controller.error(err)in nodeStream error handler (+4/-4)src/app/v1/_lib/proxy/errors.ts: ExportisTransportError()(+1/-1)src/app/v1/_lib/proxy/response-handler.ts: AddisTransportErrorimport and catch branch (+34/-1)Testing
tsc --noEmit: ✅ Passbiome check: ✅ PassSTREAM_UPSTREAM_ABORTEDinstead of being silently swallowedFixes #916
Greptile Summary
This PR fixes a bug where upstream transport errors (SocketError, ECONNRESET) during active streaming were silently swallowed, causing truncated responses to be marked as successful. The fix has three parts: (1)
forwarder.tsnow propagates stream errors viacontroller.error(err)instead ofcontroller.close(), (2)errors.tsexports the existingisTransportError()utility, and (3)response-handler.tsadds a new catch branch that routes transport errors toSTREAM_UPSTREAM_ABORTEDfinalization with proper circuit breaker failure recording and 502 status codes.isTransportErrorbranch correctly mirrors the existing upstream abort handling logic, including fallback topersistRequestFailureif finalization itself failsnodeStreamToWebStreamSafeis now outdated and should be updated to reflect the new error-propagation behaviorConfidence Score: 4/5
src/app/v1/_lib/proxy/forwarder.ts— thecloseevent fires aftererrorin Node.js, and while the try/catch handles this safely, the outdated JSDoc may confuse future maintainers.Important Files Changed
controller.close()tocontroller.error(err)in the Node stream error handler, correctly propagating upstream errors to the downstream reader instead of silently swallowing them. Thecloseevent handler's try/catch safely handles the subsequent call. Outdated JSDoc on the method.exportkeyword to the existingisTransportError()function to make it available forresponse-handler.ts. No logic changes.isTransportErrorcatch branch that routes transport errors (SocketError, ECONNRESET) toSTREAM_UPSTREAM_ABORTEDfinalization with proper circuit breaker accounting, mirroring the existing upstream abort handling path.Sequence Diagram
sequenceDiagram participant US as Upstream Provider participant NS as Node Stream participant WS as Web ReadableStream (forwarder.ts) participant RH as ResponseHandler (reader) US->>NS: data chunks... NS->>WS: controller.enqueue(chunk) WS->>RH: reader.read() → {value, done:false} Note over US,NS: SocketError / ECONNRESET occurs rect rgb(255, 230, 230) Note right of NS: Before this PR NS->>WS: error event → controller.close() WS->>RH: reader.read() → {done:true} RH->>RH: streamEndedNormally = true Note over RH: Finalized as SUCCESS ❌ end rect rgb(230, 255, 230) Note right of NS: After this PR NS->>WS: error event → controller.error(err) WS->>RH: reader.read() rejects with err RH->>RH: catch → isTransportError(err) = true RH->>RH: finalizeStream(STREAM_UPSTREAM_ABORTED) Note over RH: Finalized as FAILURE ✅ endComments Outside Diff (1)
src/app/v1/_lib/proxy/forwarder.ts, line 3966 (link)Outdated JSDoc after behavior change
The comment says "吞掉上游流的错误事件，避免 'terminated' 错误冒泡到调用者" (swallow upstream stream errors), but this PR intentionally changes the behavior to propagate errors via
controller.error(err). The JSDoc should be updated to reflect the new semantics.Prompt To Fix All With AI
Last reviewed commit: b968b1d
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!