Repository navigation
Conversation
A body handler that failed inside a CONNECT tunnel closed the tunnel's TLS wrapper and left the verdict to the wrapper's close callback. That callback also reports that the origin ended the connection, which completes a chunked body at its last chunk and a close-delimited body. So the request resolved with what the decoder had written before it failed. The seven error sites in the tunnel's callbacks now fail the request themselves, as a direct connection and the tunnel's header stage do. The close callback only means that the inner TLS connection ended, and `shutdown_err` is gone.
…ugh a CONNECT tunnel
|
Reproduced with a loopback https origin behind a CONNECT proxy. The origin sends the response head, then a damaged compressed body in a later read. The body is gzip, deflate, br or zstd, and it is cut in half, has a wrong checksum, or is no stream. The HTTP framing around it is whole (chunked or close-delimited). Without the fix, PR: #44534 |
…el-local-close-is-a-failure
|
Updated 7:47 AM PT - Oct 10th, 2026
✅ @robobun, your commit ea7b3b3b29e79848c8f8ed68870483c75df879d3 passed in 🧪 To try this PR locally: bunx bun-pr 44534That installs a local version of the PR into your bun-44534 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Each callback already holds a ref on the tunnel. It now hands that ref to `fail_request`, which releases it on the next loop tick, so the failure path takes no second ref and `on_close` uses its guard on both paths. Comments are cut to one line each.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughProxyTunnel now routes callback and close errors through ChangesProxy tunnel error handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The tunnel failure change is mergeable after normal checks; the inspected close sequence does not expose a second request failure or invalid tunnel access. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the primary change: tunnelled HTTP requests now fail with errors found by their callbacks. |
| Description check | |
The description provides a detailed problem statement, fix summary, verification results, background, tradeoffs, and known test limitations. It does not use the template headings exactly, but it inclu… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewires the raw-pointer failure path inside the tunnel's SSLWrapper callbacks (where fail can free the HTTPClient mid-callback), a human look at src/http/ProxyTunnel.rs would still be worthwhile.
What was reviewed:
- Traced
fail_request->close_and_fail->fail->close_proxy_tunnel(true): the client's handle is taken beforewrapper.shutdown(true), which setsclosed_notifiedbefore firingon_close, so the reentranton_closereturns at the detached check and no lateron_data/on_handshakecan re-enter the freedctx. - Refcounts: the caller's guard moves into
fail_requestand is released viaschedule_proxy_deref, matching the oldon_closekeepalive;receive/on_writablestill hold their own guard across the wrapper call. - The
Socket::Nonearm now fails unconditionally; every attach/detach site (start,adopt,close_proxy_tunnel,detach_owner, pool close) pairs clearing the socket withproxy_tunnel.take(), so an attached tunnel with no socket was not found reachable, and failing there is the safer outcome anyway. - Tests: the new matrix holds the body until
fetch()resolves so the decoder error lands in a lateron_data, which is the path that previously resolved; the install test's env knob and--caPEM usage follow existing precedent.
Extended reasoning...
The diff removes shutdown_err, close_from_callback and close_raw from src/http/ProxyTunnel.rs and replaces the seven tunnel-callback error sites plus on_close's failure tail with a single fail_request that calls HTTPClient::close_and_fail/fail directly, so a locally detected error (decoder failure, unexpected data, TLS/cert failure) is no longer routed through the wrapper close and misread as a completed close-delimited or chunked body. It touches no auth or input-parsing surface, but it is unsafe raw-pointer lifecycle code in SSLWrapper callbacks where the client can be freed before the callback returns; the wrapper's closed_notified ordering and the paired socket/handle detach sites are what make the new path safe, and I verified those from the source. Two tests cover the fix (a 30-case codec x damage x framing matrix and a bun install manifest case). Five earlier bot inline threads were resolved by the author after a follow-up commit, and no CODEOWNER covers the changed files. The memory-safety sensitivity of the surface, not any defect found, is why a human look is still recommended.
This branch has not been deployed
2 participants
Problem
fetch()resolves with a damaged compressed body. A gzip stream cut inside a chunked body gives 50,080 of 108,000 bytes. Without the proxy it rejects withZlibError.bun installbehind a proxy uses a manifest whose gzip checksum failed.on_data(src/http/ProxyTunnel.rs) passed the error toclose_raw, which shut the TLS wrapper down. That runson_close, which reports success when a close completes the body. ProxyTunnel: close-delimited responses via proxy cause ECONNRESET #23719 (first in 1.3.2) put that check ahead of the stored error.Fix
HTTPClient::close_and_failthroughfail_request, as a direct connection does.fail()detaches the tunnel before it shuts the wrapper down. Soon_closeonly reports that the inner connection ended, andshutdown_erris removed.test/js/bun/http/proxy-stress-errors.test.ts(30 cases, 18 fail without the fix) andtest/cli/install/bun-install.test.ts(1 case).Background
SSLWrapperdecrypts it and callson_dataandon_close.Content-Lengthnor chunked encoding ends when the connection closes, soon_closecompletes it.on_closereads the stored error first. That keeps one callback with two meanings.Downsides
on_dataandon_closeare unchanged.Notes
Origin. #23719 (commit 24d9d64, first released in bun-v1.3.2, for the ECONNRESET report #23717) added the branch that completes a body when the tunnel's connection closes, and put it ahead of the read of the stored error. Before it,
onClosealways failed the request with the stored error. I checked this in that diff and withgit describe --contains. I did not run 1.3.1. I found no GitHub issue that reports this bug (20 searches, 111 distinct results, titles read).How a related change was classified. #34922 (merged, no linked issue) made truncated compressed bodies on close-delimited responses reject, and reworked this same block of
on_close. #28792 lists it as a breaking change for 1.4. This PR makes the same kind of change for an error found inside the tunnel, after 1.4.0. Whether it needs a release note is a maintainer decision.Where it showed. The origin sends the response head, and the body in a later read. Through an HTTP proxy to an https origin, before this change (a release build without the fix):
Each cell now rejects with
ZlibError,BrotliDecompressionErrororZstdDecompressionError, which is what the same fetch gives without the proxy.Content-Lengthbodies and connections that drop in the middle already rejected.Why the old path resolved. Debug log of gzip, chunked, cut:
ProxyTunnel onData body_chunk,Decompression error: ZlibError,ProxyTunnel onClose tunnel exists,progressUpdate true.on_closeaskedis_body_complete_on_close()(true for a chunked decoder in its trailers, and for a body with no framing in stageBody), thenfinalize_body_on_eof(), which returnedOkbecause the three streaming decoders accept input once they are in their error state. When the head and the body share one read, the error is found inhandle_on_data_headers, which already calledclose_and_fail. That is why a small response in one packet never showed the bug.Not only decoders. Any
Errfromhandle_response_bodyorhandle_response_body_chunked_encodingtook the same route, for example a failed allocation on a close-delimited body.bun install. A registry manifest withContent-Encoding: gzip, chunked, with one bit of its CRC-32 flipped: through the tunnel the manifest was used and the tarball was requested. It now stops aterror: ZlibError downloading package manifest bar, as it does without the proxy. The manifest in the test is 300 KB of incompressible data so that its last chunk arrives in a later read than the head.The
Socket::Nonearm. The old failure tail ofon_closefailed a tunnel with no socket only whenfinalize_body_on_eofhad failed (#34922 added that condition).fail_requestalways fails it. A tunnel that is attached to a client always has its socket (startandadoptset it, and it is cleared only after the client gave the tunnel up), so I found no way to reach this arm by reading. I also counted it: a local build with a counter ranfail_request46 times in the proxy suites listed below, and each time the tunnel had its socket. That does not prove that the arm cannot be reached.Other designs.
InternalState, or decoders that return their error again: both stopfinalize_body_on_eoffrom succeeding after a failure. Once the error site fails the request,stageisFailandon_closeis not reached with the tunnel attached, so nothing calls it after a failure.on_datarepeats the stage dispatch ofHTTPClient::on_data): a larger change in code that HTTP client: remove unsafe from src/http (h1/h2/h3 client, proxy tunnel, HTTP thread, keep-alive pool) #40385 rewrites.Related pull requests. #43169 changes one call in
on_close(finalize_body_on_eoftofinish_body_on_close). This diff leaves that line and the lines around it as they are. #40385 keepsshutdown_errand the same decision inon_close. The description of #42261 names the pathProxyTunnel::on_data -> close_from_callback -> fail. That path is nowon_data -> fail_request -> close_and_fail -> fail, and it still ends indispatch_result_and_reset. #34342 adds a call toclose_from_callbackand edits the parked-data site that this diff also edits. The one that lands second must callfail_requestthere.Not changed. A fatal TLS error on the inner connection (no
close_notify) also runson_close, and still completes a close-delimited body. I did not test how a direct connection treats that case.Suites run on a debug build with ASAN, on this branch merged with main
71d0d43997.proxy-stress-errors(83 pass), the 30 new cases 12 times (all pass),proxy-stress-lifecycle(93),proxy-stress-adversarial(151),proxy-stress-matrix(335),proxy-stress-headers(76),proxy-stress-protocol(102),proxy.test.js(16),fetch-proxy-connect-tunnel-split-envelope(5),fetch-proxy-tls-intern-race(1), the proxy cases ofbun-install.test.ts(3).proxy.test.ts(97 cases) andproxy-stress-concurrent(43 cases) pass except for the three cases below.bun checkintest/reports no error in the two test files of this PR.Three cases that fail with and without this change. An earlier version of these notes said that they hit a 5 s limit on a slow machine. I had not checked that, and for the first case it was wrong. I then ran each file several times and alternated two builds of the same tree: this branch, and this branch with main's
ProxyTunnel.rs.proxy-stress-concurrent,32× parallel http-proxy → https-origin(both keepalive values): one of the 32 requests rejects withECONNRESET. Without the change: 1 of 3 full-file runs. With it: 2 of 3.proxy-stress-concurrent, memory probemode=redirect ×300: it exceeds its 120 s limit. Without the change: 3 of 3. With it: 2 of 3.proxy.test.ts, the bad-record 101 case: it exceeds its 5 s limit. Without the change: 2 of 2. With it: 1 of 2. Its fixture alone takes 4.9 to 5.7 s on both builds.Over the 22 cases of
proxy-stress-concurrentthat take more than 1.5 s, the ratio of mean durations (with the change over without it) has a median of 0.99, from 0.81 to 1.13. In the redirect case the tunnel's debug log is the same on both builds, and the changed code does not run. Three runs per side do not show that the two rates are equal. I did not find why the request is reset.Binary size. CI's size check reports between -64.0 KB and +0.0 KB on the 12 targets. The comparison is against a canary build of main, not the merge base. The diff removes two functions and one field of
ProxyTunneland adds one function.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts