Closing one side of a tunnel ends its copy loops, which close the other
side. closeConnectionsLocked sent each close message and closed that
connection before moving on, in map order, so when the client side came
first the upstream was often closed before its close message went out.
Send every close message in the batch first, then close the connections.
The second close then regularly finds the connection already closed.
Treat net.ErrClosed as expected instead of returning it, so reloads with
open websockets no longer log "use of closed network connection" at
ERROR level.
The upstream close message test goes back to running the real cleanup,
which it can now do reliably.
* reverseproxy: set a write deadline on websocket close messages
Cleanup sends a Close control message to every attached websocket while
holding the tunnel tracker lock, during a config reload. If the peer has
stopped reading, the tunnel's copy goroutine is blocked writing to it and
the close message waits behind that write forever. The reload never
finishes and holds the config lock, so every later /load and /config
request hangs until Caddy is restarted.
Setting a write deadline before the close message unblocks both writes.
The connection is closed right after, so the deadline is never cleared.
* Add bot disclosure
* reverseproxy: make close control timeout a const, harden stalled-peer test
* Delete BOT_DISCLOSURE.md
Signed-off-by: Francis Lavoie <lavofr@gmail.com>
* reverseproxy: bound each batch of websocket close messages by one deadline
The close loops are serial and hold the tunnel tracker lock, so a
per-connection write timeout let N stalled peers hold a reload for N
times the timeout. Compute one absolute deadline per batch instead,
and skip the best-effort close frame on connections that cannot set a
write deadline rather than risk the hang.
Extended CONNECT streams gain SetWriteDeadline through
http.ResponseController. For HTTP/1 upgrades, net/http returns the
upstream socket inside a body that hides its deadline methods, so the
upstream close message was never bounded; record the connection from
the request's ClientTrace and write the close message to that socket.
Restores the TODO about being the exclusive writer, per review.
* reverseproxy: make websocket close tests deterministic
Per review, the stalled-peer tests use net.Pipe instead of real TCP:
it is unbuffered and lets one write through at a time, so a close
message waits behind a stuck copy write without large allocations or
sleeps. The batch test checks that graceful closes share one deadline
instead of timing cleanup.
The HTTP/1 upstream close test now runs a real upgrade through
ServeHTTP and the transport, so it fails if the GotConn hook is
removed. The choice of upstream close writer moves into
upstreamCloseWriter, with a table test for the HTTP/2 and HTTP/3 case.
* reverseproxy: record the upstream conn with atomic.Pointer
Per review, store the connection from the GotConn hook in an
atomic.Pointer[net.Conn] instead of a mutex-guarded holder type.
Rename upstreamCloseWriter to upstreamWriteCloser. It returns
io.ReadWriteCloser, since both the body and the recorded socket are
one, so gracefulClose keeps its original io.ReadWriteCloser parameter.
* reverseproxy: share one close deadline across detached trackers
Per review, an upstream removal notifies every stopped tracker in
turn, and each close loop holds its own lock, so a deadline per
tracker let N trackers with stalled peers hold a reload for N times
the timeout. Compute one absolute deadline in
notifyDetachedTunnelTrackersOfUpstreamRemoval and pass it through to
closeConnectionsForUpstream.
Add a test with two stopped trackers holding stalled net.Pipe peers
that asserts both graceful closes receive the same deadline.
* reverseproxy: simplify the websocket close deadline change
Fold the two tunnel close loops into closeConnectionsLocked, so the
shared-deadline rule and its rationale live in one place. Replace the
per-tracker closeWriteTimeout field, which production never varied,
with a package variable that tests shorten.
Record the upstream socket only for websocket requests, since only
their graceful close reads it, and store a copy of the connection
rather than the address of the hook's parameter. Use the package's
caddy.CtxKey convention for the context key.
Share the upgrade tunnel fixture between the half-close test and the
upstream close message test.
* reverseproxy: close backend body and hijacked socket on upgrade setup failures
When an upstream response indicates an HTTP upgrade (101 Switching Protocols),
handleUpgradeResponse takes ownership of res.Body and subsequently hijacks
the client connection.
However, if any setup check fails prior to starting handleUpgradeTunnel
(such as mismatched upgrade headers, non-writable body assertion failure,
hijack unsupported, or handshake buffer flush errors), the function returned
early without closing res.Body or the hijacked client socket.
Because calling Hijack() permanently disables net/http's automatic connection
cleanup, and raw TCP sockets are not closed by the garbage collector, these
early failure exits leaked file descriptors into CLOSE_WAIT/ESTABLISHED.
Fix this by:
1. Adding a deferred cleanup guard that closes res.Body and any hijacked
front connection unless the tunnel successfully takes ownership.
2. Wrapping connections with onceCloseConn to ensure idempotent closure
while preserving optional CloseWrite (TCP half-close) support.
3. Adding TestUpgradeResponseCleanupOnFailure to verify cleanup.
* reverseproxy: test post-hijack cleanup and remove once-close wrappers
Cover handshake flush and buffered backend-write failures after successful hijacking, checking that both connections close. Keep setup-failure cleanup while using the original connections directly.
Code and tests were AI-assisted using an OpenAI coding assistant in Pi.
* reverseproxy: Optionally detach stream (websockets) from config lifecycle
* lint
* Improved logging facilities
* Adjustments from Weidi's review
* record bytes read and written for response writers unless detached
* simplify streaming handling
* correctly close detached streams
* make handleUpgradeTunnel a standalone func
* fix tests
* fix tests
* change the log level if hijacking without writing a status code first
* only clean up connections when stopped
* Move type and const down to the bottom
* Rename to tunnelTracker, reflow some comments
* Add note about capturing h
* Rename to stream_detached
* More comments
* reverseproxy: Preserve half-close behavior after rebase
Keep master half-close propagation and its unit coverage in the refactored tunnel lifecycle. Adapt response finalization tests to the selected upstream argument.
AI-assisted-by: Codex (GPT-6)
* reverseproxy: Fix detached stream cleanup and recorder chains
Snapshot the tracker registry before closing connections, and unregister empty trackers outside their mutex. Mark trackers stopped at cleanup entry so upstream removal closes detached streams even with stream_close_delay, while attached streams retain their delay.
Detach every supported response writer in the middleware chain so inner recorders do not retain state or count detached traffic. Add regressions for all four failures and an access-log/encode/intercept reload integration test.
AI-assisted-by: Codex (GPT-6)
---------
Co-authored-by: WeidiDeng <weidi_deng@icloud.com>
* reverseproxy: propagate TCP half-close on upgraded streams
handleUpgradeResponse starts one copy goroutine per direction but returns as
soon as the first one reports a result, and returning runs two unconditional
deferred closes (the client connection and the backend connection). A client
that ends its send direction with a TCP half-close therefore causes the whole
tunnel to be torn down: the backend connection is reset mid-conversation and
whatever it was about to send is never delivered.
Treat a clean EOF as the end of one direction rather than the end of the
tunnel. When a copy finishes without error, propagate the half-close to the
destination with CloseWrite where the connection supports it, and keep waiting
for the other direction so pending bytes can still drain. Real copy errors, the
stream timeout and request cancellation/shutdown still tear down immediately.
If the destination does not support CloseWrite, the peer cannot be told that
the direction ended, so the previous behavior is kept and the tunnel is torn
down rather than held open until the stream timeout.
This is the same class that was fixed upstream in net/http/httputil
(https://go.dev/issue/35892); Caddy has its own upgraded-stream copier, so the
standard library fix does not apply here.
Fixes#8026
* reverseproxy: follow net/http/httputil shape for the half-close
Replace the copyResult struct and the closeWriter/closeWrite helpers with
upstream's plain error channel and errCopyDone sentinel, propagating
CloseWrite inside the copier, as requested in review.
Behaviour is unchanged. The wait is repeated twice inside the existing
select because handleUpgradeResponse also selects on the stream timeout
and sizes the channel at 2 so both goroutines can exit when it fires
(#7418), which net/http/httputil has no equivalent of.
* reverseproxy: explain the repeated wait next to it
Move the reasoning for why the wait is repeated inside the select, rather
than written as upstream's two-line form, into a comment beside the loop
so a future reader does not have to find the review discussion.
* reverseproxy: add an upgraded-stream half-close integration test
Drive a real upgraded stream through the handler and check that closing one
direction is propagated to the other side instead of tearing the tunnel down.
Mirrors net/http/httputil's TestReverseProxyWebSocketHalfTCP, which covers the
same class upstream (https://go.dev/issue/35892): a backend that hijacks and
hands back a *net.TCPConn, the handler in front of it, and a client that dials
it directly, then the four close-read / close-write combinations.
Against the unfixed handler the two half-close cases fail - the surviving
direction never delivers its pending bytes - while the two close-read cases
pass, so the test isolates exactly the reported behaviour.
* reverseproxy: give the half-close test reads a deadline
Without one, a regression blocks in ReadFull until the package timeout
instead of failing the test, which is the flakiness an end-to-end test of
this kind is usually accused of. Ten seconds is far above any real latency
here and turns a hang into a clear failure.
* reverseproxy: hijack the test backend via http.NewResponseController
Matches how the rest of the tree reaches Hijack (responsewriter.go,
caddyauth.go, streaming.go itself) and keeps working if the writer is
wrapped, where a direct http.Hijacker assertion would not.
* reverseproxy: Add more debug logs
This makes debug logging very noisy when reverse proxying, but I guess
that's the point.
This has shown to be useful in troubleshooting infrastructure issues.
* Update modules/caddyhttp/reverseproxy/streaming.go
Co-authored-by: Francis Lavoie <lavofr@gmail.com>
* Update modules/caddyhttp/reverseproxy/streaming.go
Co-authored-by: Francis Lavoie <lavofr@gmail.com>
* Add opt-in `trace_logs` option
* Rename to VerboseLogs
---------
Co-authored-by: Francis Lavoie <lavofr@gmail.com>
* caddyhttp: Make use of http.ResponseController
Also syncs the reverseproxy implementation with stdlib's which now uses ResponseController as well https://github.com/golang/go/commit/2449bbb5e614954ce9e99c8a481ea2ee73d72d61
* Enable full-duplex for HTTP/1.1
* Appease linter
* Add warning for builds with Go 1.20, so it's less surprising to users
* Improved godoc for EnableFullDuplex, copied text from stdlib
* Only wrap in encode if not already wrapped
* ci: Use golangci's github action for linting
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix most of the staticcheck lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the prealloc lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the misspell lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the varcheck lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the errcheck lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the bodyclose lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the deadcode lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the unused lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the gosec lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the gosimple lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the ineffassign lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Fix the staticcheck lint errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Revert the misspell change, use a neutral English
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Remove broken golangci-lint CI job
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Re-add errantly-removed weakrand initialization
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* don't break the loop and return
* Removing extra handling for null rootKey
* unignore RegisterModule/RegisterAdapter
Co-authored-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* single-line log message
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
* Fix lint after a1808b0dbf209c615e438a496d257ce5e3acdce2 was merged
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Revert ticker change, ignore it instead
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Ignore some of the write errors
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Remove blank line
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Use lifetime
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* close immediately
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
* Preallocate configVals
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Update modules/caddytls/distributedstek/distributedstek.go
Co-authored-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
Chris Oliver
Zen Dodd
Rob Martin
Francis Lavoie
Kévin DunglasandZen Dodd
Turhan ACAR
Matt HoltandFrancis Lavoie
