fix(lifecycle): don't crash process on benign broken-pipe from streaming disconnect (#1234)#1239
Conversation
When a downstream client disconnects during a streaming response (e.g. /v1/responses), the socket write can fail with EPIPE, ECONNRESET, or ERR_STREAM_PREMATURE_CLOSE. Previously these errors escaped to the process-level uncaughtException/unhandledRejection handlers and caused a process.exit(1), restarting the container. Now isBenignBrokenPipeError recognizes these disconnect codes (including through error.cause chains) so the crash handlers can safely log and return, treating the event as a per-request transport close rather than a fatal crash. EPIPE is also added to isTransportError for consistent classification. Closes #1234
📝 WalkthroughWalkthrough该 PR 新增良性断管错误检测能力，防止客户端流断连时 Node.js 的 EPIPE 异常导致整个 CCH 进程重启。实现包括一个新的错误检测库（仅识别 EPIPE），与进程级崩溃处理器的集成（区分客户端断连与真正错误），以及完整的单元测试覆盖。 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 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 |
![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 introduces mechanism to handle benign broken-pipe and client-disconnect errors (such as EPIPE, ECONNRESET, and ERR_STREAM_PREMATURE_CLOSE) gracefully without crashing the process. It adds a utility to detect these errors by traversing the error cause chain, integrates this check into the process-level crash handlers in instrumentation.ts, and adds comprehensive unit tests. The review feedback suggests extracting the matched error code from the cause chain using a new helper function (getBenignBrokenPipeCode) to ensure the correct nested error code is logged instead of resulting in undefined.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function isBenignBrokenPipeError(err: unknown): boolean { | ||
| let current: unknown = err; | ||
| for (let depth = 0; depth <= MAX_CAUSE_DEPTH && current != null; depth++) { | ||
| if (typeof current === "object") { | ||
| const code = (current as { code?: unknown }).code; | ||
| if (typeof code === "string" && BENIGN_BROKEN_PIPE_CODES.has(code)) { | ||
| return true; | ||
| } | ||
| current = (current as { cause?: unknown }).cause; | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If a benign error is wrapped inside a cause chain (e.g., a FetchError wrapping an EPIPE error), isBenignBrokenPipeError will correctly return true, but logging (err as NodeJS.ErrnoException).code in instrumentation.ts will result in undefined because the code is nested.
By extracting the actual matched code via a new helper getBenignBrokenPipeCode, we can log the correct error code that triggered the suppression, making troubleshooting much easier.
| export function isBenignBrokenPipeError(err: unknown): boolean { | |
| let current: unknown = err; | |
| for (let depth = 0; depth <= MAX_CAUSE_DEPTH && current != null; depth++) { | |
| if (typeof current === "object") { | |
| const code = (current as { code?: unknown }).code; | |
| if (typeof code === "string" && BENIGN_BROKEN_PIPE_CODES.has(code)) { | |
| return true; | |
| } | |
| current = (current as { cause?: unknown }).cause; | |
| continue; | |
| } | |
| break; | |
| } | |
| return false; | |
| } | |
| export function getBenignBrokenPipeCode(err: unknown): string | undefined { | |
| let current: unknown = err; | |
| for (let depth = 0; depth <= MAX_CAUSE_DEPTH && current != null; depth++) { | |
| if (typeof current === "object") { | |
| const code = (current as { code?: unknown }).code; | |
| if (typeof code === "string" && BENIGN_BROKEN_PIPE_CODES.has(code)) { | |
| return code; | |
| } | |
| current = (current as { cause?: unknown }).cause; | |
| continue; | |
| } | |
| break; | |
| } | |
| return undefined; | |
| } | |
| export function isBenignBrokenPipeError(err: unknown): boolean { | |
| return getBenignBrokenPipeCode(err) !== undefined; | |
| } |
| */ | ||
|
|
||
| import { startCacheCleanup } from "@/lib/cache/session-cache"; | ||
| import { isBenignBrokenPipeError } from "@/lib/lifecycle/benign-errors"; |
There was a problem hiding this comment.
Import getBenignBrokenPipeCode to extract the actual matched error code from the cause chain for logging.
| import { isBenignBrokenPipeError } from "@/lib/lifecycle/benign-errors"; | |
| import { getBenignBrokenPipeCode, isBenignBrokenPipeError } from "@/lib/lifecycle/benign-errors"; |
| if (isBenignBrokenPipeError(err)) { | ||
| logger.warn("[Lifecycle] ignored uncaught client disconnect", { | ||
| error: err.message, | ||
| errorName: err.name, | ||
| errorCode: (err as NodeJS.ErrnoException).code, | ||
| stack: err.stack, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Use getBenignBrokenPipeCode to extract and log the actual error code that triggered the suppression, even if it is nested deep in the cause chain.
| if (isBenignBrokenPipeError(err)) { | |
| logger.warn("[Lifecycle] ignored uncaught client disconnect", { | |
| error: err.message, | |
| errorName: err.name, | |
| errorCode: (err as NodeJS.ErrnoException).code, | |
| stack: err.stack, | |
| }); | |
| return; | |
| } | |
| const benignCode = getBenignBrokenPipeCode(err); | |
| if (benignCode) { | |
| logger.warn("[Lifecycle] ignored uncaught client disconnect", { | |
| error: err.message, | |
| errorName: err.name, | |
| errorCode: benignCode, | |
| stack: err.stack, | |
| }); | |
| return; | |
| } |
| if (isBenignBrokenPipeError(reason)) { | ||
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | ||
| error: err.message, | ||
| errorName: err.name, | ||
| errorCode: (reason as NodeJS.ErrnoException)?.code ?? (err as NodeJS.ErrnoException).code, | ||
| stack: err.stack, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Use getBenignBrokenPipeCode to extract and log the actual error code that triggered the suppression, avoiding potential undefined values when the code is nested in the cause chain of the rejection reason.
| if (isBenignBrokenPipeError(reason)) { | |
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | |
| error: err.message, | |
| errorName: err.name, | |
| errorCode: (reason as NodeJS.ErrnoException)?.code ?? (err as NodeJS.ErrnoException).code, | |
| stack: err.stack, | |
| }); | |
| return; | |
| } | |
| const benignCode = getBenignBrokenPipeCode(reason); | |
| if (benignCode) { | |
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | |
| error: err.message, | |
| errorName: err.name, | |
| errorCode: benignCode, | |
| stack: err.stack, | |
| }); | |
| return; | |
| } |
![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: 2f128bca44
ℹ️ 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".
| "ECONNRESET", | ||
| "ERR_STREAM_PREMATURE_CLOSE", |
There was a problem hiding this comment.
When any uncaught exception or unhandled rejection anywhere in the process carries ECONNRESET or ERR_STREAM_PREMATURE_CLOSE, registerCrashDiagnostics now returns before writing a report or exiting. These codes are not specific to downstream Response writes; upstream provider, Redis, DB, or other stream/socket failures can use the same codes, and the process-level handler has no request/socket context to prove they are client disconnects. In those scenarios a real unhandled infrastructure bug is reduced to a warning and the process continues in an unknown state instead of preserving the previous fail-fast behavior; please scope the suppression to the actual broken-pipe/write path or add context checks before swallowing these broader codes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "ECONNRESET", | ||
| "ERR_STREAM_PREMATURE_CLOSE", |
There was a problem hiding this comment.
ECONNRESET may also originate from upstream connections
ECONNRESET is not exclusively a client-disconnect signal — it can also be raised when an upstream TCP peer (e.g., a database, Redis, or the AI-provider API) resets the connection. If any such socket emits an 'error' event without a listener, or a promise carrying that error goes unhandled, it will be classified as benign here and swallowed with only a warn log instead of triggering a container restart. Depending on how thoroughly upstream clients (pg, ioredis, undici) guard their sockets, a transient infrastructure reset that should cause a restart could be silently absorbed.
Intentional? If so, adding a comment noting this tradeoff (or narrowing the set to EPIPE + ERR_STREAM_PREMATURE_CLOSE only, since both are write-side / close-side signals) would make the design decision explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/lifecycle/benign-errors.ts
Line: 26-27
Comment:
**`ECONNRESET` may also originate from upstream connections**
`ECONNRESET` is not exclusively a client-disconnect signal — it can also be raised when an *upstream* TCP peer (e.g., a database, Redis, or the AI-provider API) resets the connection. If any such socket emits an `'error'` event without a listener, or a promise carrying that error goes unhandled, it will be classified as benign here and swallowed with only a `warn` log instead of triggering a container restart. Depending on how thoroughly upstream clients (pg, ioredis, undici) guard their sockets, a transient infrastructure reset that should cause a restart could be silently absorbed.
Intentional? If so, adding a comment noting this tradeoff (or narrowing the set to `EPIPE` + `ERR_STREAM_PREMATURE_CLOSE` only, since both are write-side / close-side signals) would make the design decision explicit.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| process.on("unhandledRejection", (reason: unknown) => { | ||
| const err = reason instanceof Error ? reason : new Error(String(reason)); | ||
| // 同 uncaughtException：良性断连以 rejection 形式逃逸时同样不应使进程退出。 | ||
| // 注意：对原始 reason 判定（而非 wrap 后的 err），否则 { code: "EPIPE" } 之类 | ||
| // 非 Error 拒因在 new Error(String(reason)) 后会丢失 code。 | ||
| if (isBenignBrokenPipeError(reason)) { | ||
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | ||
| error: err.message, | ||
| errorName: err.name, | ||
| errorCode: (reason as NodeJS.ErrnoException)?.code ?? (err as NodeJS.ErrnoException).code, | ||
| stack: err.stack, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const reportPath = writeReport("unhandledRejection", err); |
There was a problem hiding this comment.
error field shows "[object Object]" for plain-object rejections
new Error(String(reason)) is constructed unconditionally before the benign guard, so for a non-Error rejection like { code: "EPIPE" }, String(reason) produces "[object Object]" and that becomes err.message. The error field in the logger.warn call will therefore be "[object Object]" rather than anything meaningful. Moving the err construction to after the benign check (or deriving the message directly from reason) fixes this for the warn path without affecting the fatal path.
| process.on("unhandledRejection", (reason: unknown) => { | |
| const err = reason instanceof Error ? reason : new Error(String(reason)); | |
| // 同 uncaughtException：良性断连以 rejection 形式逃逸时同样不应使进程退出。 | |
| // 注意：对原始 reason 判定（而非 wrap 后的 err），否则 { code: "EPIPE" } 之类 | |
| // 非 Error 拒因在 new Error(String(reason)) 后会丢失 code。 | |
| if (isBenignBrokenPipeError(reason)) { | |
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | |
| error: err.message, | |
| errorName: err.name, | |
| errorCode: (reason as NodeJS.ErrnoException)?.code ?? (err as NodeJS.ErrnoException).code, | |
| stack: err.stack, | |
| }); | |
| return; | |
| } | |
| const reportPath = writeReport("unhandledRejection", err); | |
| process.on("unhandledRejection", (reason: unknown) => { | |
| // 同 uncaughtException：良性断连以 rejection 形式逃逸时同样不应使进程退出。 | |
| // 注意：对原始 reason 判定（而非 wrap 后的 err），否则 { code: "EPIPE" } 之类 | |
| // 非 Error 拒因在 new Error(String(reason)) 后会丢失 code。 | |
| if (isBenignBrokenPipeError(reason)) { | |
| const code = (reason as NodeJS.ErrnoException)?.code; | |
| logger.warn("[Lifecycle] ignored unhandled client disconnect", { | |
| error: reason instanceof Error ? reason.message : String(reason), | |
| errorName: reason instanceof Error ? reason.name : typeof reason, | |
| errorCode: code, | |
| stack: reason instanceof Error ? reason.stack : undefined, | |
| }); | |
| return; | |
| } | |
| const err = reason instanceof Error ? reason : new Error(String(reason)); | |
| const reportPath = writeReport("unhandledRejection", err); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/instrumentation.ts
Line: 113-128
Comment:
**Warn-log `error` field shows `"[object Object]"` for plain-object rejections**
`new Error(String(reason))` is constructed unconditionally before the benign guard, so for a non-`Error` rejection like `{ code: "EPIPE" }`, `String(reason)` produces `"[object Object]"` and that becomes `err.message`. The `error` field in the `logger.warn` call will therefore be `"[object Object]"` rather than anything meaningful. Moving the `err` construction to after the benign check (or deriving the message directly from `reason`) fixes this for the warn path without affecting the fatal path.
```suggestion
process.on("unhandledRejection", (reason: unknown) => {
// 同 uncaughtException：良性断连以 rejection 形式逃逸时同样不应使进程退出。
// 注意：对原始 reason 判定（而非 wrap 后的 err），否则 { code: "EPIPE" } 之类
// 非 Error 拒因在 new Error(String(reason)) 后会丢失 code。
if (isBenignBrokenPipeError(reason)) {
const code = (reason as NodeJS.ErrnoException)?.code;
logger.warn("[Lifecycle] ignored unhandled client disconnect", {
error: reason instanceof Error ? reason.message : String(reason),
errorName: reason instanceof Error ? reason.name : typeof reason,
errorCode: code,
stack: reason instanceof Error ? reason.stack : undefined,
});
return;
}
const err = reason instanceof Error ? reason : new Error(String(reason));
const reportPath = writeReport("unhandledRejection", err);
```
How can I resolve this? If you propose a fix, please make it concise.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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
No significant issues identified in this PR. The fix is well-scoped, thoroughly tested, and correctly addresses the EPIPE crash issue without compromising the fail-fast behavior for genuine errors.
PR Size: M
- Lines changed: 335 (334 additions, 1 deletion)
- Files changed: 6
Review Coverage
- Logic and correctness - Clean. The
isBenignBrokenPipeErrorpredicate is code-only (no message matching), depth-limited, and cycle-safe. TheuncaughtException/unhandledRejectionhandlers correctly early-return for benign codes while preservingprocess.exit(1)for genuine errors. - Security (OWASP Top 10) - Clean. No new attack surface; the whitelist is narrow (3 codes only) and no user input is evaluated.
- Error handling - Clean. Errors are logged at
warnlevel witherrorCodefor benign cases; fatal path remains unchanged for non-benign errors. Thecausechain traversal is bounded. - Type safety - Clean. No
anyusage; theunknownparameter is safely narrowed withtypeofchecks. - Documentation accuracy - Clean. Comments accurately describe the behavior and the trade-offs (e.g., explicit note about avoiding message matching to prevent false suppression).
- Test coverage - Adequate. Three new test files cover the predicate (including cause chains, cycles, plain objects, and negative cases), the handler wiring (benign vs. genuine via spy), and the transport error classification regression.
- Code clarity - Good. The
benign-errors.tsmodule is intentionally zero-dependency and self-contained. TheregisterCrashDiagnosticsexport is justified for testability.
Minor Note (Author's Discretion)
The unhandledRejection handler constructs err before the benign check, so for a plain-object rejection like { code: "EPIPE" }, the logger.warn error field receives "[object Object]" rather than a meaningful message. The errorCode field is correct ("EPIPE"), so the log remains functional. This is a low-priority logging quality improvement.
Automated review by Claude AI
Restrict process-level crash-handler suppression to EPIPE (write-side socket pipe) only. ECONNRESET and ERR_STREAM_PREMATURE_CLOSE can originate upstream (providers, DB, Redis) where suppressing exit hides real infrastructure failures; they now trigger fail-fast. - Narrow BENIGN_BROKEN_PIPE_CODES to ["EPIPE"] in benign-errors.ts. - Add getBenignBrokenPipeCode to extract the matched code from nested causes for accurate logging. - Fix unhandledRejection logging to avoid "[object Object]" for non-Error rejections with EPIPE. - Remove EPIPE from isTransportError (proxy layer handles communication errors; crash handler covers uncaught EPIPE separately). - Update unit tests to verify EPIPE-only suppression, fail-fast for ambiguous codes, and correct logging. Refs #1234
Review feedback addressed (pushed in 97d162d)Thanks @gemini-code-assist, @chatgpt-codex-connector, and @greptile-apps. All points are now resolved: 1. P1/P2 —
|
|
To use Codex here, create a Codex account and connect to github. |
|
Thanks for the update, @ding113. The refinements to the benign error set and the handling of non-Error rejections are excellent. By restricting the benign set to |
|
All changes look excellent — this is exactly the right resolution. Narrowing the benign set to The Reverting the LGTM. ✓ |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Fixes #1234 — Codex
/v1/responses(and any other streaming) traffic could crash the entire CCH process via an uncaughtwrite EPIPEwhen a downstream client disconnects mid-stream.The final streaming
Responseis written to the socket by Next.js, outside any localtry/catch. When the client disconnects, Node throwswrite EPIPEwhich escapes to the process-leveluncaughtExceptionhandler insrc/instrumentation.ts— which calledprocess.exit(1)for every error, turning a per-request broken pipe into a full container restart (RestartCountkept climbing).Changes
src/lib/lifecycle/benign-errors.ts— zero-dependencyisBenignBrokenPipeError()that recognizesEPIPE/ECONNRESET/ERR_STREAM_PREMATURE_CLOSEon the error and itscausechain (depth-limited, cycle-safe). Detection is code-only (no loose message matching) so an upstream error body that merely mentions "EPIPE" can never falsely suppress a real crash.src/instrumentation.ts— bothuncaughtExceptionandunhandledRejectionhandlers now early-return withlogger.warn(noprocess.exit) for benign broken-pipe errors; genuine errors still fail-fast withexit(1)exactly as before.registerCrashDiagnosticsis exported for testability. The rejection handler checks the originalreason(before it is wrapped into anError) so a non-Errorrejection like{ code: "EPIPE" }keeps its code.src/app/v1/_lib/proxy/errors.ts— addedEPIPEtoTRANSPORT_ERROR_CODESsoisTransportError()(and thereforecategorizeErrorAsync→SYSTEM_ERROR) classifies it consistently, matching the forwarder's existing disconnect handling.Why this is safe
The whitelist is narrow (3 socket/stream disconnect codes only). Every other uncaught exception / unhandled rejection still triggers the diagnostic report +
process.exit(1)fail-fast, so genuine logic bugs are unaffected. This aligns the global handler with the per-stream disconnect handling already present inforwarder.ts.Tests
tests/unit/benign-broken-pipe-error.test.ts— predicate coverage incl. cause chains, cyclic-cause safety, plain-object carriers, and the critical negative case (message-only "EPIPE" with nocodemust not be benign).tests/unit/instrumentation-crash-handler.test.ts— drives the actual registered handlers: benign codes logwarnand do not exit; generic errors and non-benign codes stillexit(1).tests/unit/transport-error-detection.test.ts— EPIPE regression forisTransportError.Verification (local, full)
bun run typecheck✓bun run lint✓bun run build✓ (production / Turbopack standalone)bun run test✓ — 6191 passed, 13 skipped (691 files)🤖 Generated with Claude Code
Greptile Summary
This PR fixes a container-restart regression (#1234) where streaming client disconnects could crash the entire CCH process via an uncaught
write EPIPEescaping to the process-leveluncaughtExceptionhandler, which previously calledprocess.exit(1)for every error.src/lib/lifecycle/benign-errors.ts— new zero-dependency predicate (getBenignBrokenPipeCode) that detectsEPIPE(and onlyEPIPE) on the error or itscausechain (depth-limited, cycle-safe); deliberately excludesECONNRESET/ERR_STREAM_PREMATURE_CLOSEat the process level to preserve fail-fast semantics for ambiguous infrastructure errors.src/instrumentation.ts— bothuncaughtExceptionandunhandledRejectionhandlers now early-return withlogger.warnfor benign broken-pipe codes; the rejection path evaluates the rawreasonbefore wrapping it intonew Error(String(reason)), preserving thecodefield for plain-object rejections like{ code: \"EPIPE\" }.EPIPEwas added toTRANSPORT_ERROR_CODESinsrc/app/v1/_lib/proxy/errors.tswith a companion regression test, but neither change is present in the diff — the set in the current file still lacks\"EPIPE\".Confidence Score: 5/5
Core process-stability fix is correct and well-tested; the only gap is a stated change to errors.ts that was not included in the diff.
The process-level EPIPE suppression logic is narrow, correctly implemented, and thoroughly tested. The benign-error predicate is code-only (no loose message matching), restricted to the write-side EPIPE code, and cycle-safe. The main omission — adding EPIPE to TRANSPORT_ERROR_CODES — is an incomplete part of the stated scope but does not introduce new breakage; the existing catch-all SYSTEM_ERROR path in categorizeErrorAsync still handles it correctly in the common case.
src/app/v1/_lib/proxy/errors.ts — the stated EPIPE addition to TRANSPORT_ERROR_CODES and its companion test in tests/unit/transport-error-detection.test.ts were described in the PR but not applied.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Node.js emits uncaughtException / unhandledRejection] --> B[registerCrashDiagnostics handler] B --> C{getBenignBrokenPipeCode check EPIPE on error or cause chain up to 5 deep} C -- benign code found EPIPE only --> D[logger.warn no process.exit] C -- no match --> E{writeReport writeFatalStderr} E --> F[logger.fatal] F --> G[process.exit 1]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(crash-handler): suppress exit only f..." | Re-trigger Greptile