[mafredri]

When the server answers 101 Switching Protocols but the handshake still fails, for example on an invalid Sec-WebSocket-Accept or a subprotocol the client did not offer, Dial now closes the response body and returns at once.

Previously, every Dial that failed after receiving a response read up to 1024 bytes of the response body for debugging, with a 3 second timer. After a 101 the body is the upgraded connection, not an HTTP body, so when the server sent nothing more, each of these failures took 3 seconds.

For these failures the returned response keeps its status and headers, and resp.Body is empty (http.NoBody). Failures with any other status are unchanged: resp.Body holds up to the first 1024 bytes, read with the same 3 second limit.

The tradeoff: if a server answers 101 and then writes error text on the connection, resp.Body no longer contains that text. Bytes after a 101 belong to the upgraded protocol, not to an HTTP body.

TestDialFailedHandshakeBody covers a 101 with an invalid Sec-WebSocket-Accept from a server that then sends a few bytes and keeps the connection open (fails on master after 3.0 s with those bytes in the body; passes with this change in about 0 s against a 1 s limit) and a 403 whose body is still returned. Lint, race tests and the WASM test pass.

Decision log

• Skip the read only for status 101: other statuses have a real HTTP body, and its first bytes often explain the rejection.
• Return http.NoBody instead of a nil resp.Body: when Dial returns a response with an error, resp.Body has never been nil, so callers that read it need no new nil check.
• Rejected: a shorter timer for 101. Each such failure would still wait, and the body would still contain WebSocket bytes.
• The Dial doc now says the body is empty after a 101. Its old text ("you can only read the first 1024 bytes of the body") stayed literally true, but a caller debugging a failed 101 could not tell an empty body from a silent server.
• Test limit of 1 s: far from master's 3 s and from the fix's near 0 s, so a slow CI machine does not flip the result.

🤖 This PR was created with the help of Coder Agents.

[coder-agents-review]

Chat: Review posted | View chat
Requested: 2026-11-03 19:56 UTC by @mafredri
Spend: $8.58 / $100.00

Review history

• R1 (2026-11-03): 10 reviewers, 1 P3, 3 P4, COMMENT. Review
• R2 (2026-11-03), 1 P3, 3 P4, COMMENT. Review
• R3 (2026-11-03): 6 reviewers, 1 P3, 3 P4, APPROVE. Review

deep-review v0.13.0 | Round 3 | e1a21e5..1abc068

Last posted: Round 3, 4 findings (1 P3, 3 P4), APPROVE. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	Note	Note (body)	dial.go:152	101 branch comment is false for custom transports with a non-ReadWriteCloser body; their buffered body is now discarded	R1	Netero	No
CRF-2	P3	Author fixed (5b96150)	dial_test.go:436	Silent server lets a short-timer read of the upgraded connection pass the test	R1	Bisky	Yes
CRF-3	Note	Note (body)	dial.go:154	respBody.Close() is guarded only by the TestMain goroutine leak check, not by the subtest	R1	Bisky, Chopper	No
CRF-4	P4	Author fixed (d967226)	dial.go:152	Comment implies an unbounded wait; the read is bounded by the 3s timer that this branch avoids	R1	Gon P2	Yes
CRF-5	P4	Dropped by orchestrator (superseded by CRF-2 fix, which rewrites this comment)	dial_test.go:438	Test server comment narrates io.Copy instead of stating why the server stays silent	R1	Gon P2	No
CRF-6	P4	Author fixed (5b96150)	dial_test.go:454	1s limit has no comment tying it to the 3s debug-read timer	R1	Gon P3	Yes
CRF-7	P4	Author fixed (1abc068)	dial.go:113	Dial doc does not say resp.Body is empty after a failed 101 handshake	R1	Leorio	Yes
CRF-8	Note	Dropped by orchestrator (restates CRF-4 and CRF-5)	dial.go:152	Both added comments narrate rather than state the reason	R1	Gon	No
CRF-9	Note	Dropped by orchestrator (confirms unchanged behavior, asks nothing)	dial.go:151	101 without Upgrade headers reaches the branch with an empty body; no cost	R1	Mafuuu	No
CRF-10	Note	Dropped by orchestrator (confirms correct branching, asks nothing)	dial.go:151	Every post-defer error return matches its status branch	R1	Melody	No
CRF-11	Note	Dropped by orchestrator (confirms message quality, asks nothing)	dial_test.go:455	Failure message names measured value and limit	R1	Leorio	No
CRF-12	OOS	Out of scope (body)	main_test.go:23	TestMain leak check counts goroutines right after m.Run and flakes on base	R1	Mafuuu	No
CRF-13	OOS	Out of scope (body)	dial.go:162	3s timer callback for stalled non-101 bodies has no test	R1	Bisky	No
CRF-14	OOS	Out of scope (body)	dial.go:160	1024 is an unnamed literal repeated in the Dial doc	R1	Gon	No
CRF-15	OOS	Out of scope (body)	dial.go:162	time.Second*3 is an unnamed literal the new test threshold depends on	R1	Gon	No
CRF-16	OOS	Out of scope (body)	dial.go:159	Debug-read comment does not state the size or time limits	R1	Gon	No
CRF-17	P2	Dropped by orchestrator (wording written to fix CRF-4, author explained why; accurate, no reader misled)	dial.go:154	101 branch comment restates the read limits below and repeats 1024	R3	Gon	No
CRF-18	P2	Dropped by orchestrator ("would" marks a hypothetical read; no reader misled)	dial_test.go:439	"reading it would stall" could be read as referring to the server's io.Copy	R3	Gon	No
CRF-19	P2	Dropped by orchestrator (redundant word only)	dial.go:115	"of the body" is redundant in the Dial doc	R3	Gon	No
CRF-20	Note	Dropped by orchestrator (restates CRF-17 to CRF-19)	dial_test.go:455	Added comments use three names for the debug read	R3	Gon	No

Downgrade and drop reasoning (R1)

• CRF-4 P2 to P4. Keep-argument: a maintainer may read the comment as describing an unbounded hang and misjudge what the branch fixes. Downgrade reason: the sentence is literally true, the timer is visible a few lines below in the same function, and the consequence is a slower read of the code, not wrong behavior.
• CRF-5 P2 to P4, dropped. Keep-argument: if the author rejects the CRF-2 fix, the narrating comment stays. Drop reason: the comment does state the scenario ("Send nothing more"), and the CRF-2 fix makes the server send bytes, which forces a rewrite of this comment.
• CRF-6 P3 to P4. Keep-argument: raising the limit to 3s or more silently makes the check pass on the old code. Downgrade reason: requires a future edit, and with the CRF-2 fix the body assertion catches a reintroduced read independently of the timing check.
• CRF-8 to CRF-11 dropped. Keep-argument: none ask for a change; CRF-8 restates CRF-4 and CRF-5.

Downgrade and drop reasoning (R3)

• CRF-17 dropped. Keep-argument: the comment narrates the read limits and adds a third copy of 1024 that can drift. Drop reason: the author wrote this wording to fix CRF-4 and replied that the suggested wording covered only a silent server; the comment is accurate, and asking for another rewrite of the same lines is oscillation with no correctness or reader consequence.
• CRF-18 dropped. Keep-argument: a reader may think the server's io.Copy is the read that stalls. Drop reason: "Keep the connection open" names io.Copy's role, and "would stall" marks a hypothetical read; the test's assertions and failure messages identify Dial as the subject.
• CRF-19 dropped. Keep-argument: exported doc wording should be tight. Drop reason: a single redundant word misleads no caller.
• CRF-20 dropped. Keep-argument: none beyond CRF-17 to CRF-19.

Contested and acknowledged

None.

Round log

Round 1

Netero first pass: no findings, 1 Note (CRF-1). Law not run (effective +67). Full panel, round 1: ging-go, ryosuke, killua, melody (specialists: Go code, connection and HTTP body lifecycle), pariston, mafuuu (core), bisky (tests), gon, leorio (once-per-PR floor), chopper (wildcard). Size 10 exceeds the focused target of 6-8 because four specialists (ging-go, ryosuke, killua, melody) plus the Gon and Leorio floor were required. Orchestrator verified CRF-2: with the server writing "after upgrade" after the 101 headers, head passes and base fails. 1 P3, 3 P4 posted; 2 Notes and 5 out-of-scope items in body; 5 dropped. Reviewed against e1a21e5..e4d658a.

Round 2 update

BLOCKED. CRF-7 silent (no doc change, no reply). CRF-2, CRF-4, CRF-6 author fixed, unverified by panel. No review. Reviewed against e1a21e5..d967226.

Round 3

Churn guard PROCEED (CRF-7 addressed in 1abc068). Fix round (only comments, doc, and test data changed since R1 panel; no files added, no signature changes). Netero: no findings; verified base dial.go fails the new test on the body assertion. Panel: bisky, gon, leorio (raised CRF-2, CRF-4, CRF-6, CRF-7), ging-go, ryosuke (.go files), chopper (wildcard). CRF-2, CRF-4, CRF-6, CRF-7 fixes verified by panel. Gon raised 3 P2 wording items and 1 Note, all dropped (CRF-17 to CRF-20). No open findings. Reviewed against e1a21e5..1abc068.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Komugi	flake/determinism
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

[code-asher]

Looks good to me!