fix: a stalled upload no longer wedges the bridge and kills idle-stop - #438
Merged
Merged
Conversation
Closes #435. When the engine answers before the request body has finished forwarding, the connection is written off after a 250ms grace and torn down: engine.Close() <-bodySent The writer is `bodySent <- req.Write(engine)`, and req.Write blocks on two different things: it READS req.Body, which reads the client, and it WRITES to the engine. That teardown only closes the engine, so it only covers one of them. The covered case -- engine answered early and stopped reading -- has a test already (TestEarlyErrorOnAbandonedUploadIsSalvaged). The other one did not. A client that stalls mid-upload parks req.Write in a Read that never returns, engine.Close() cannot touch a read, and `<-bodySent` waits forever. The cost is wildly out of proportion to the cause. rewriteBinds never returns, so Server.handle never returns, so s.clients.Add(-1) never runs: - ActiveConns stays above zero for the life of the process, so maybeIdleStop vetoes on "open client connections" forever. The idle-timeout feature -- a README headline, and the answer to the most common WSL2 complaint -- is silently dead, with nothing in the log to say so. - Serve's wg.Wait() never completes, so shutdown hangs while holding the single-instance lock, which the comment above Serve says must not happen. closeActive() would rescue it, and that runs only on shutdown, which is already blocked behind wg.Wait(). A laptop that sleeps during `docker build`, a dropped VPN, a killed CLI. No malice required. The fix, in abandonBody: cut BOTH sides and bound the wait. - engine.Close() for the write stall, as before - a read deadline in the past on the client conn, which is what actually reaches the read. The caller has already set resp.Close, so the connection is finished either way and making it unreadable costs nothing. - a bounded select, so the function returns even if the writer is wedged on something neither close reached. Leaking one goroutine costs a little memory; not returning disables idle-stop for every user of the process. Verified as a negative control: with the original two lines restored, TestStalledUploadDoesNotWedgeTheConnection hangs for its full 30s timeout and fails. With the fix it returns in 0.25s. The second test pins the other direction -- that the 5s failsafe is not being waited out on the ordinary path, which would turn this into a latency bug nobody attributed to it. Not fixed here: #436, the 101-upgrade path sharing clientR with the still-live body writer. Same 250ms branch, different failure, and the two are ordered -- joining the writer before relayBuffered is one of the candidate fixes there and would have reintroduced this hang if done first.
zcsizmadia
added a commit
that referenced
this pull request
Sep 19, 2026
…#439) Closes #436. When the engine answers a 101 before the request body has finished forwarding, rewriteBinds handed the connection to relayBuffered while the abandoned writer was still inside req.Body. relayBuffered does clientR.Buffered()/Peek()/Discard(). req.Write reads req.Body, which reads the SAME bufio.Reader. Two goroutines, one bufio.Reader, no internal synchronisation: corrupted buffer indices and a read past the slice. That is the heap-corruption class of #166, which this package already diagnosed and fixed once -- and the invariant that fix established ("exactly one goroutine reads clientR at any moment") is stated a dozen lines below the call that broke it. The same overlap also writes to the engine concurrently with req.Write, interleaving bytes on the hijacked stream. The teardown could not save it: the deferred abandonBody runs AFTER relayBuffered returns, so the overlap lasted the whole session -- an exec or an attach, so potentially hours. Refusing is the conservative answer and the asymmetry decides it: a failed `docker exec` is visible, local and retryable; a corrupted heap is none of those. It is also rare, because exec and attach carry little or no request body, so reaching the 250ms grace at all means something is already wrong. Ordering -------- This could not be fixed before #438. The other candidate fix -- join the body writer before relayBuffered instead of deferring it -- would have reintroduced the unbounded `<-bodySent` hang that #438 removed. Now that the join is bounded, that option exists; the refusal is still the safer of the two and is what ships. What the test proves, and what it does not ------------------------------------------ TestHijackIsRefusedWhileTheBodyWriterIsLive pins the REFUSAL: no 101 reaches the client and the relay returns an error naming the reason. It does NOT prove corruption is prevented. A data race on a bufio.Reader cannot be deterministically provoked, and the race detector will not see this path either -- which is precisely why it survived review twice. The argument for the guard is in the comment at the call site, not in the test. Negative control: with the guard disabled the test fails, though by hanging for its full 30s timeout rather than by the 101 assertion -- relayBuffered enters and relays for the life of the connection, so the relay simply never returns. Failing is the point; the mechanism of the failure is worth knowing. TestRewriteFallsBackToRawOn101 is the other half and is unchanged: a 101 whose body DID finish still upgrades normally, which is the case docker exec and docker attach actually take.
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.
Closes #435.
When the engine answers before the request body has finished forwarding, the connection is written off after a 250 ms grace:
The writer is
bodySent <- req.Write(engine), andreq.Writeblocks on two things: it readsreq.Body(which reads the client) and it writes to the engine. That teardown closes only the engine.The covered case — engine answered early and stopped reading — already has a test. The other one did not: a client that stalls mid-upload parks
req.Writein aReadthat never returns,engine.Close()cannot touch a read, and<-bodySentwaits forever.Why a stalled client was allowed to kill a feature
rewriteBindsnever returns →Server.handlenever returns →s.clients.Add(-1)never runs:ActiveConnsstays above zero for the life of the process.maybeIdleStopvetoes on "open client connections" forever — the idle-timeout feature is silently dead, with nothing in the log saying so. That is a README headline and the answer to the most common WSL2 complaint.Serve'swg.Wait()never completes, so shutdown hangs holding the single-instance lock — which the comment aboveServesays must not happen.closeActive()would rescue it, and it runs only on shutdown, which is already blocked behindwg.Wait().A laptop that sleeps during
docker build. A dropped VPN. A killed CLI. No malice required.The fix
abandonBodycuts both sides and bounds the wait:engine.Close()for the write stall, as beforeresp.Closeis already set, so the connection is finished either way and making it unreadable costs nothing.Verification
Negative control, run both ways:
A second test pins the other direction — that the 5 s failsafe is not being waited out on the ordinary path, which would turn a correctness fix into a latency bug nobody attributed to it.
Not fixed here
#436 — the 101-upgrade path sharing
clientRwith the still-live body writer. Same 250 ms branch, different failure, and the two are ordered: joining the writer beforerelayBufferedis one of the candidate fixes there, and doing it first would have reintroduced exactly this hang.