fix(proxy): close upstream session if response.prepare fails - #29279
Open
Jiahui-Gu wants to merge 1 commit into
Open
fix(proxy): close upstream session if response.prepare fails#29279Jiahui-Gu wants to merge 1 commit into
Jiahui-Gu wants to merge 1 commit into
Conversation
web.StreamResponse construction and resp.prepare(request) were outside the try/finally that closes the upstream aiohttp ClientSession and ClientResponse. If prepare() raises (e.g. the client disconnects before headers are flushed) the handler exits via exception and neither upstream_resp.release() nor session.close() is ever called, leaking sockets on every interrupted request. Move StreamResponse construction and prepare() inside the existing try block so the finally that releases the upstream response and closes the session runs in all paths. Add a regression test that subclasses web.StreamResponse to raise ClientConnectionError from prepare(), tracks ClientSession.__init__/close and ClientResponse.release, and asserts the proxy-opened session is closed and the upstream response is released before the test tears down the runners. Verified to fail on the pre-fix code. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
alt-glitch
added
type/bug
Contributor
|
Thanks for the focused regression fix. Current The change in Automated hermes-sweeper review. |
teknium1
added
sweeper:risk-message-delivery
2 tasks
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hermes_cli/proxy/server.py::_open_upstream(lines 204-244) opened an upstreamClientSessionandClientResponse, then constructed the localweb.StreamResponseand calledawait resp.prepare(request)outside the existingtry/finallycleanup block. If the client disconnected mid-prepare (ConnectionResetError,ClientConnectionError, browser cancel,curl --max-time), the exception bypassed cleanup and both the aiohttpClientSessionandClientResponseleaked. On a busy reverse-proxy deployment this gradually exhausts file descriptors / socket pool.Fix
Move
web.StreamResponse(...)andawait resp.prepare(request)inside thetryblock. The existingfinally(upstream_resp.release()+await session.close()) now runs on every exception path.Test plan
tests/hermes_cli/test_proxy.py::test_server_closes_session_when_prepare_fails— subclassesweb.StreamResponseto raiseClientConnectionErrorfromprepare(), patches it into the proxy module, and tracksClientSession.__init__/close+ClientResponse.release. Asserts the proxy's upstream session is closed before runner teardownserver.pyand passes on the fixtests/hermes_cli/test_proxy.py(37 tests) passes🤖 Generated with Claude Code