Repository navigation
Conversation
cb29345 to
ef9faa0
Compare
NetConn always called SetReadLimit(-1), so a limit set before NetConn was silently dropped while one set after it applied. NetConn now disables the limit only if SetReadLimit was never called on the Conn.
The Wasm Conn tracks SetReadLimit calls separately from the native Conn, so the same four cases run in the browser against the echo server.
The fix-close-race-net-errclosed branch adds a test at the same place in ws_js_test.go, and the two insertions conflict on merge.
ef9faa0 to
125d60d
Compare
This comment has been minimized.
This comment has been minimized.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 2 | Last posted: Round 2, 10 findings (2 P3, 7 P4, 1 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedRound logRound 1Round type: round 1, full panel. Netero first pass: 1 P4, 1 Note, 1 OOS; no P0, panel proceeded. Law not run (effective additions 141). Panel: ging-go, ryosuke, takumi (specialists), pariston, mafuuu (core), bisky (tests changed), gon, leorio (floor), razor (wildcard, random). Cross-check: race finding merged from Bisky, Gon, Takumi at P3 (higher severity wins; decision log accepts the gap without a ticket). Gon P2 on doc wording merged into Leorio P4 doc finding: keep-argument was that redundant doc text costs readers, but the consequence is wording only and Leorio's suggested text already adopts Gon's one-sentence rule. Gon ws_js.go:49 P3 and Razor Nit merged into CRF-6 at P3. Gon netconn_js_test.go:27 P3 downgraded to P4 and merged with Gon conn_test.go:324 P4: keep-argument was that a default change makes two cases vacuous; the 32768 default is documented public behavior, so a change to it is a reviewed API change. CRF-9 dropped: keep-argument was that a reader looks for a test that does not exist; the claim is about coverage and the existing native test covers it. Reviewed against e1a21e5..125d60d. Round 2Churn guard PROCEED: CRF-1, 4, 5, 6, 7, 8 author fixed. Round type: fix round (no files added or renamed, no exported signature change, effective additions +63; native tests were moved into TestNetConnReadLimit in response to CRF-8). Netero: no findings, two notes. Panel: bisky, gon, takumi, leorio, razor (raised findings whose code changed), ging-go, ryosuke (Go specialists), meruem (wildcard, random). Fixes verified by Razor, Ryosuke, Leorio, Gon, Netero. CRF-14: Bisky and Takumi both P4; Meruem's note that a race test cannot reliably trigger is contradicted by Bisky and Takumi, who saw -race report DATA RACE after deleting the lock; ci/test.sh runs go test -race. CRF-15 and CRF-16 interact: moving the lock protocol into a per-platform method keeps every setReadLimit caller in the file that declares the lock. CRF-17 kept at P4: keep-argument is that a maintainer comparing the platform tables concludes native never-set has no test; drop-argument (author replied on CRF-8) is not visible in the code. CRF-18 OOS: orchestrator reproduced 2/60 isolated -race runs of TestNetConnReadLimit and 4/60 of base TestConn/ReadLimitExceededReturnsErrMessageTooBig failing with goroutine leak, so the root cause predates the PR. Full-suite -race runs under load also hung in TestWriteSingleFrameCompressed (compress_test.go:121) and failed TestConn/pingReceivedPongNotReceived; both unrelated to the diff, not recorded as findings. Reviewed against e1a21e5..bc04f4b. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR makes NetConn keep a read limit set with SetReadLimit before it is called, instead of always disabling it. Findings: 2 P3, 3 P4, 1 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
ws_js.go:149: on Wasm, exceeding the read limit callsClose(StatusMessageTooBig, ...), which the browser rejects for code 1009, so the connection stays open, contrary to theSetReadLimitdoc.read.go:532: the native error and close reason report the internal limit plus one (read limited at 1025 bytesafterSetReadLimit(1024)), while Wasm reports 1024, so the two platforms name different limits.ws_js.go:87: the Wasm default limit is the literal32768, duplicatingdefaultReadLimitinread.go, so the two defaults can diverge without a compile error.- coder/coder
agent/x/agentdesktop/api.go:122: callsSetReadLimit(-1)with the comment "No read limit", thencodersdk.WebsocketNetConn, which sets1 << 22beforeNetConn. With this PR the desktop connection is limited to 4 MiB per message and the comment becomes false.
Notes:
- coder/coder (on v1.8.14) calls
SetReadLimit(1 << 22)beforeNetConnincodersdk/websocket.go:46(WebsocketNetConn, about 19 callers) andcodersdk/agentsdk/agentsdk.go:413. Those 4 MiB limits have never been enforced and will be after the upgrade. No current path was found that sends a single message over 4 MiB, but the text-mode JSON paths were not checked, so the release notes should name this change. - The Wasm
BeforeNetConnandAfterNetConncases pass only because the browser rejectsclose(1009)(first out-of-scope item), so the deferredClose(StatusNormalClosure, "")still reaches the echo server. If that close starts succeeding,TestWasmfails with no change to the code under test.
🤖 This review was automatically generated with Coder Agents.
A net.Conn caller reads a byte stream and cannot tell that a kept read limit counts each WebSocket message separately. Before the read limit was kept, NetConn disabled it, so the doc did not need to say this.
The standalone test repeated netConn/readLimit with one added SetReadLimit(-1) call. As a table case with an expected result, it sits next to the cases it contrasts with, matching the Wasm test.
Each message size must exceed a limit defined elsewhere, and a bare literal hides which one. Naming the sizes by the limit they exceed shows which cases stop testing anything if a default or limit changes.
The native closure was named setLimit although it also creates the net.Conn, and the native cases ran as TestConn subtests under different names than the Wasm cases. A top-level TestNetConnReadLimit with a netConn field matches TestWasmNetConnReadLimit case for case.
The cases that expect NetConn to read without error only test something while their message exceeds the default read limit. A control case that reads the same size without NetConn fails if the default is raised to that size, instead of letting those cases pass without a limit to disable.
NetConn checked readLimitSet and then disabled the limit, so a SetReadLimit that ran between the two steps was overwritten and left the connection without a read limit. A mutex around both operations makes the check and the update atomic.
The field comment described only readLimitMu. A reader also needs to know that only SetReadLimit sets readLimitSet, because NetConn disables a limit set through setReadLimit alone.
|
/coder-agents-review
|
There was a problem hiding this comment.
This PR makes NetConn keep a read limit set with SetReadLimit before it is called. Findings this round: 4 P4.
All six round 1 findings are fixed and verified at bc04f4b: the readLimitMu lock closes the concurrent SetReadLimit/NetConn race, the field and NetConn docs are in place, and the test tables, constants, and names match on both platforms. The round 1 out-of-scope items are unchanged.
Out of scope (needs a ticket or explicit acceptance by a human):
main_test.go:23:TestMaincounts goroutines right afterm.Runwith no retry, butCloseNowcan return before theCloseReadgoroutine (read.go:77-84) exits, so a run can fail with "goroutine leak detected". Under-race, isolated runs failed 4 of 60 times for the existingTestConn/ReadLimitExceededReturnsErrMessageTooBigon base and 2 of 60 times for the newTestNetConnReadLimit, which uses the sameCloseReadpattern.
🤖 This review was automatically generated with Coder Agents.
| // Reading a message that exceeds it returns an error wrapping ErrMessageTooBig. | ||
| func NetConn(ctx context.Context, c *Conn, msgType MessageType) net.Conn { | ||
| c.SetReadLimit(-1) | ||
| c.readLimitMu.Lock() |
There was a problem hiding this comment.
P4 [CRF-14] No test calls SetReadLimit and NetConn concurrently, so deleting the readLimitMu lock passes every test in the PR. (Bisky, Takumi)
The reply on the concurrent-overwrite thread says such a test would fail too rarely to catch a regression, but ci/test.sh runs go test -race, and the race detector reports the unsynchronized readLimitSet access whenever the two calls run without ordering, not only when the limit is lost. A test that runs c1.SetReadLimit(readLimit) in an xsync.Go goroutine while the test goroutine calls NetConn on a fresh Conn passed 5 of 5 -race runs at HEAD and reported WARNING: DATA RACE in 3 of 3 runs with the Lock/Unlock lines removed. Add it as a Concurrent case in TestNetConnReadLimit.
🤖
| // Reading a message that exceeds it returns an error wrapping ErrMessageTooBig. | ||
| func NetConn(ctx context.Context, c *Conn, msgType MessageType) net.Conn { | ||
| c.SetReadLimit(-1) | ||
| c.readLimitMu.Lock() |
There was a problem hiding this comment.
P4 [CRF-15] NetConn now locks readLimitMu, reads readLimitSet, and calls setReadLimit, which are private fields and methods that conn.go and ws_js.go must each declare with the same names. (Meruem)
Before this PR, netconn.go (built on both platforms) used only Conn methods. Now the lock protocol is split across netconn.go, read.go, and ws_js.go, so a change to the limit state on one platform must also change netconn.go, and a native build does not show whether the Wasm side still compiles. Move these lines into a method such as func (c *Conn) disableReadLimitIfUnset() next to SetReadLimit in read.go and in ws_js.go, and call it from NetConn.
🤖
| c.setReadLimit(n) | ||
| } | ||
|
|
||
| func (c *Conn) setReadLimit(n int64) { |
There was a problem hiding this comment.
P4 [CRF-16] setReadLimit has no comment saying that callers must hold readLimitMu and that it does not set readLimitSet. (Gon)
In this package lowercase twins such as ping/Ping and write/Write do the same work as the exported method, so a reader expects setReadLimit(n) to behave like SetReadLimit(n). A future internal caller, such as the ReadLimit option discussed on #244, that calls setReadLimit(n) would have n replaced with -1 by the next NetConn, which is the bug this PR fixes. Add // setReadLimit stores n without setting readLimitSet. Callers must hold readLimitMu. here and on the Wasm setReadLimit (ws_js.go:429); if the lock protocol moves into a per-platform method as suggested on the NetConn lock, the comment still applies.
🤖
| <-writeDone | ||
| }) | ||
|
|
||
| for _, tc := range []struct { |
There was a problem hiding this comment.
P4 [CRF-17] The native TestNetConnReadLimit table has no Unset case, and nothing in the test says that TestConn/netConn/readLimit (conn_test.go:288) covers it. (Leorio)
TestWasmNetConnReadLimit runs Unset, DisabledBeforeNetConn, BeforeNetConn, and AfterNetConn, so a maintainer comparing the two tests will conclude that the native never-set case is untested. Add one line above the table, for example // TestConn/netConn/readLimit covers the case where SetReadLimit is never called.
🤖
NetConncalledSetReadLimit(-1)every time, so the outcome depended on call order: a limit set afterNetConnapplied, and a limit set before it was dropped without an error. Code that set a limit and then calledNetConnhad no limit at all.NetConnnow disables the limit only ifSetReadLimitwas never called on theConn. A limit set before or afterNetConnis kept, and so is an explicitSetReadLimit(-1). Callers that never callSetReadLimitsee no change:NetConnstill disables the 32768 byte default.Behavior change: if you call
SetReadLimit(n)beforeNetConn, that limit is now enforced. Reading a message larger thannbytes fails withErrMessageTooBig, asSetReadLimitdocuments; before this change the message was read in full. To keep the old behavior, remove theSetReadLimitcall or pass-1.New tests cover a limit set before
NetConn, set after it, never set, and set to-1before it, on native and on Wasm in the browser. The before-NetConntest fails on master on both platforms../ci/lint.sh,go test -race ./...and the Wasm browser test pass.Refs #244 (this PR does not add the
ReadLimitoption field).Decision log
SetReadLimitmust followNetConn: documentation alone leaves the call-order problem for every caller.ReadLimitfield inAcceptOptions/DialOptionshere: that is a separate API decision on Add ReadLimit to Option structs #244, and the field would need this rule anyway, becauseNetConnwould otherwise discard it.sync.Mutexon bothConntypes closes a race in whichNetConndisabled a limit thatSetReadLimitset at the same time. It is never held during I/O. A compare-and-swap on the limit without a sentinel value still lost the limit in a stress test, and a sentinel value would need handling inlimitReader.resetand the Wasm read path.