Repository navigation
Don't end an upgraded H1 tunnel on the request's own body end - #1016
Open
Shubham-Padkonde wants to merge 2 commits into
Open
Shubham-Padkonde wants to merge 2 commits into
Shubham-Padkonde wants to merge 2 commits into
Conversation
The downstream half only marks body tasks `UpgradedBody` once it has observed the upgrade, so the end of the upgrade request's own (empty) body is sent as a plain `Body(None, true)`. When the upstream's 101 is read before that task is consumed, the task arrives at `proxy_handle_upstream` with `was_upgraded()` already true and is mistaken for the end of the tunnel: both `request_done` and `response_done` are set, the loop exits and the task pipe is dropped, so the client-to-upstream direction of the tunnel is never wired and both connections are closed right after a successful upgrade. Only treat the request side as terminating the tunnel when the task is not a plain `Body`, i.e. when it is an upgraded-body end, an unexpected task or the downstream sender being dropped. Fixes cloudflare#946 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Validation after merging main — 20 October 2026
The merge preserves upstream test helpers and both asynchronous I/O imports alongside the upgrade regression test. The regression passes;
cargo fmt -p pingora-proxy -- --checkandcargo clippy -p pingora-proxy --lib --testspass.The current proxy library suite reports 168 passed, 23 failed. Unchanged upstream
623e163reports 167 passed and the identical 23 failed tests in the same environment. These existing failures include zero-capacity test streams and disconnect tests; the merged branch adds one passing upgrade regression. The earlier 59-test result below describes the original submission, not the expanded current upstream suite. Integration tests requiring origin servers were not run.Conflict resolution and this validation update were prepared with Codex assistance.
Fixes #946
Problem
An HTTP/1 upgrade is torn down immediately after a successful
101whenever the upstream's response is read before the proxy has processed the upgrade request's own (empty) body.send_body_to_pipemarks a taskUpgradedBodyonly whensession.was_upgraded()is already true, so the end of the upgrade request's own body is always a plainHttpTask::Body(None, true). Two orderings exist inproxy_handle_upstream:body task consumed first (slower upstream):
request_done = truewhilewas_upgraded()is still false, soresponse_donestays false; reading the101then resetsrequest_doneand the tunnel works;101read first (fast upstream): the queued plainBody(None, true)arrives withwas_upgraded()already true, sorequest_done && was_upgraded()also setsresponse_done. Both flags are set,while !request_done || !response_doneexits andrxis dropped — the client→upstream direction is never wired, andproxy_handle_downstreamthen fails withtry_reserve() body pipe for upstream: Closed(..).The result is a client that receives the
101, writes its first frame and gets a clean EOF.Fix
In the request branch, only let the request side terminate the tunnel when the task is not a plain
Body: anUpgradedBodyend, an unexpected task, or the downstream sender being dropped. A plainBodyend seen after the upgrade can only be the upgrade request's own body, so the tunnel stays open andrequest_doneis reset to keep streaming.This is deliberately narrow: nothing changes for non-upgraded requests, or when the body task is consumed before the
101. The leftoverBodyend still goes throughsend_body_to1as before (finish_body()on a body writer that the nextUpgradedBodyconverts to close-delimited), andmaybe_force_close_body_readeris unaffected becausereceived_upgrade_req_bodyis only set by theUpgradedBodypath.Test
upgraded_request_survives_body_end_queued_before_the_101drivesproxy_handle_upstreamdirectly over atokio::io::duplexpair, which makes the race deterministic instead of load-dependent: the upstream's101is already on the wire when the loop starts, and the request-body end is delivered afterwards, followed by a tunnel frame and then the client closing its half.Without the change, the upstream only ever sees the request header:
With the change the frame is forwarded and the tunnel ends only when the client closes it.
The
pingora-proxyintegration tests were not run locally (they need the test origin servers); the change is confined to the H1 proxy loop and the new unit test covers both orderings.Disclosure: this change was written by Claude Code (Claude Opus 5) working as my agent, at my direction. The test, lint and reproduction output quoted above comes from real runs in my local environment; I am accountable for what is submitted here and will follow up on review feedback.
🤖 Generated with Claude Code