Skip to content

Don't end an upgraded H1 tunnel on the request's own body end - #1016

Open
Shubham-Padkonde wants to merge 2 commits into
cloudflare:mainfrom
Shubham-Padkonde:fix-h1-upgrade-body-end-teardown
Open

Shubham-Padkonde wants to merge 2 commits into
cloudflare:mainfrom
Shubham-Padkonde:fix-h1-upgrade-body-end-teardown

Conversation

@Shubham-Padkonde

@Shubham-Padkonde Shubham-Padkonde commented Sep 20, 2026 •

Copy link
Copy Markdown

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 -- --check and cargo clippy -p pingora-proxy --lib --tests pass.

The current proxy library suite reports 168 passed, 23 failed. Unchanged upstream 623e163 reports 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 101 whenever the upstream's response is read before the proxy has processed the upgrade request's own (empty) body.

send_body_to_pipe marks a task UpgradedBody only when session.was_upgraded() is already true, so the end of the upgrade request's own body is always a plain HttpTask::Body(None, true). Two orderings exist in proxy_handle_upstream:

  • body task consumed first (slower upstream): request_done = true while was_upgraded() is still false, so response_done stays false; reading the 101 then resets request_done and the tunnel works;

  • 101 read first (fast upstream): the queued plain Body(None, true) arrives with was_upgraded() already true, so request_done && was_upgraded() also sets response_done. Both flags are set, while !request_done || !response_done exits and rx is dropped — the client→upstream direction is never wired, and proxy_handle_downstream then fails with try_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: an UpgradedBody end, an unexpected task, or the downstream sender being dropped. A plain Body end seen after the upgrade can only be the upgrade request's own body, so the tunnel stays open and request_done is 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 leftover Body end still goes through send_body_to1 as before (finish_body() on a body writer that the next UpgradedBody converts to close-delimited), and maybe_force_close_body_reader is unaffected because received_upgrade_req_body is only set by the UpgradedBody path.

Test

upgraded_request_survives_body_end_queued_before_the_101 drives proxy_handle_upstream directly over a tokio::io::duplex pair, which makes the race deterministic instead of load-dependent: the upstream's 101 is 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:


thread 'proxy_h1::tests::upgraded_request_survives_body_end_queued_before_the_101' panicked at

pingora-proxy/src/proxy_h1.rs:1297:

the tunnel should still forward the client's frames, upstream saw:

"GET /socket HTTP/1.1\r\nHost: example.com\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n"

test result: FAILED. 0 passed; 1 failed

With the change the frame is forwarded and the tunnel ends only when the client closes it.


$ cargo test -p pingora-proxy --lib

test result: ok. 59 passed; 0 failed



$ cargo fmt -p pingora-proxy -- --check     # clean

$ cargo clippy -p pingora-proxy --lib --tests   # clean

The pingora-proxy integration 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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP/1 upgrade torn down when the upstream's 101 is read before the request's empty body

2 participants