fix: stop a disconnected client from crashing the response-write thread - #828
Conversation
do_POST wraps every route in a single try/except Exception that, on any failure, calls _send_error(500, ...) -> _send(...) to report it. If the original failure WAS _send() itself raising because the client already closed the socket (gave up waiting on a slow /v1/chat/completions "auto" run), the except handler's own _send() call hits the same closed socket and raises again -- this second BrokenPipeError is unhandled and crashes the request-handling thread (visible in server logs as two consecutive tracebacks per request). Route every response-writing method (_send, _send_text, _send_sse, _begin_sse, _write_sse) through one _write_response helper that swallows BrokenPipeError/ConnectionError/OSError: a client that disconnected mid- response is not a server error, there's nothing left to deliver, and letting the exception propagate only produces a second identical failure in the error-reporting path itself. Found while re-running LineageWeave's keyman-extraction backfill against a live orchestrator instance: a client-side timeout waiting on a slow "auto" response reliably reproduced the crash in this repo's server.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHTTP 응답 작성에 ChangesHTTP 응답 연결 종료 안전성
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The change prevents disconnected clients from crashing response threads, but failed SSE writes currently allow the backend stream to continue running and retain execution capacity, which can cause healthy requests to receive 503 responses after repeated disconnects. Merge readiness is blocked until the stream exits on write failure and this behavior is covered by a regression test. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
📝 Info: Disconnected streams logged as successful 200
When a client disconnects mid-stream, _stream_route_completion now returns cleanly, so do_POST still records the analytics event at server.py:5270-5280 with status_code: 200 and response_streamed: True. Aborted deliveries are counted as fully successful streamed responses.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): Deep diff review + fixes applied; all required checks green on current head except strix (org-wide NVIDIA NIM quota exhaustion — external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). Full local suite green on this head. |
# Conflicts: # contextual_orchestrator/server.py
| for delta in orchestrator.stream_route(messages, workflow_run_id=run_id): | ||
| self._write_sse(frame({"content": delta})) | ||
| self._write_sse(frame({}, finish="stop")) | ||
| if not self._write_sse(frame({"content": delta})): | ||
| return |
There was a problem hiding this comment.
📝 Info: Generator closed by refcount, not explicit close
On disconnect, the for loop returns without explicitly closing the stream_route generator. CPython refcounting closes it promptly so upstream work stops, but this relies on refcount semantics rather than an explicit close().
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-25): Integrated with the merged opaque-session auth design (#788) — purpose resolution now composes with session/bearer validation, denials and audit_replay access are audited, and state-changing admin routes keep the same-origin check. Full local suite green on this head. All required hosted checks green except strix (org-wide NVIDIA NIM quota exhaustion — external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). |
1 participant
Summary
do_POSTwraps every route in a singletry/except Exceptionthat reports any failure via_send_error(500, ...)->_send(...). If the original failure was_send()itself raising because the client already closed its socket (gave up waiting on a slow/v1/chat/completions"auto"run), theexcepthandler's own_send()call hits the same closed socket and raises again — unhandled this time, crashing the request-handling thread. Visible in server logs as two consecutive tracebacks per affected request._send,_send_text,_send_sse,_begin_sse,_write_sse) through one_write_responsehelper that swallowsBrokenPipeError/ConnectionError/OSError: a client that disconnected mid-response is not a server error, there is nothing left to deliver, and letting the exception propagate only produces a second identical failure in the error-reporting path itself. Unrelated exceptions (e.g. a real bug in payload construction) still propagate normally — only disconnect-shaped errors are swallowed."auto"-mode response reliably reproduced the crash.Test plan
tests/test_http_response_write_disconnect_safety.py— asserts_write_responseswallowsBrokenPipeError/ConnectionResetErrorbut still propagates an unrelatedTypeErrorpython -m pytest tests/test_http_response_write_disconnect_safety.py -v— 2 passedpython -m pytest tests/ -k "http_honesty or server or commercial" -q— 1082 passed, 0 failed🤖 Generated with Claude Code
Summary by CodeRabbit
버그 수정
테스트