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 3 | Last posted: Round 3, 13 findings (2 P3, 8 P4, 3 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedCRF-16 (P4, read.go:104) - setReadLimit preconditions undocumented
Round 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. Round 3Churn guard PROCEED: CRF-14, 15, 17 author fixed; CRF-16 partly contested. Round type: fix round (no files added or renamed, no exported change, effective additions +22; commits respond to R2 findings plus two self-initiated doc rewrites). Netero: no findings, one note (ConcurrentWithNetConn fails only under -race). Dispute check-in: Gon reused via its R2 session (gon PR586 R2), accepted CRF-16. Panel: gon, bisky, takumi, meruem, leorio (raised R2 findings whose code changed; Leorio also for doc comment changes), ging-go, ryosuke (Go specialists), knov (wildcard, random). Verified fixes: CRF-14 (Bisky, Takumi), CRF-15 (Meruem, Ryosuke), CRF-17 (Leorio). CRF-5 doc rewrite in 9c7b5c8 checked by Leorio and Gon: kept, per-message unit and error are in the NetConn line 20 and SetReadLimit docs. CRF-19: Gon P2 on two locations, combined and downgraded to Nit. Keep-argument: a comment that spells out a function body drifts when the function changes, and this comment has been rewritten twice with more words each time. Downgrade reason: the comment is accurate today, the consequence is reading cost, and the finding concerns style. CRF-20 kept as Nit although Leorio considers CRF-17 resolved: the two findings differ (location vs name), and the platforms name the same case differently. CRF-21 interacts with CRF-16 and CRF-19: inlining removes the unlocked entry point CRF-16 documented. Reviewed against e1a21e5..b520823. 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.
This comment has been minimized.
This comment has been minimized.
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.
A separate TestNetConnReadLimit split the NetConn read limit tests across two functions, because the case without a limit stays in TestConn as netConn/readLimit. The cases now run under that name.
The readLimitSet comment started a sentence with readLimitMu and said NetConn keeps the limit. The Wasm test said the echo server expects a status instead of saying what fails.
Three of the four sentences repeated the NetConn and SetReadLimit docs. The doc now states the rule once and the reason for it.
Without readLimitMu, the race detector reports the unsynchronized readLimitSet access on every run, so this case catches a removed lock under go test -race.
NetConn is shared by both platforms but locked readLimitMu and read readLimitSet, private fields that conn.go and ws_js.go each declare. The check now lives next to SetReadLimit on each platform.
A caller that expects it to behave like SetReadLimit would have its limit disabled by the next NetConn.
|
/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: 1 P4, 2 Nit.
The round 2 findings are resolved at b520823. ConcurrentWithNetConn fails under -race without the lock, disableReadLimitUnlessSet keeps the lock and flag in each platform's files, and the native cases sit next to the never-set test. The defense for leaving the lock precondition out of the setReadLimit comment is accepted. The shortened NetConn doc from 9c7b5c8 still covers the per-message limit, because the existing "Every Write to the net.Conn" paragraph and the SetReadLimit doc already state it. The round 1 and round 2 out-of-scope items are unchanged.
🤖 This review was automatically generated with Coder Agents.
|
|
||
| // setReadLimit does not set readLimitSet, so NetConn can still disable the | ||
| // limit. | ||
| func (c *Conn) setReadLimit(n int64) { |
There was a problem hiding this comment.
P4 [CRF-21] setReadLimit shares no logic between its two callers, and its only lasting effect is an unlocked way to change the limit without setting readLimitSet. (Meruem)
disableReadLimitUnlessSet always passes -1, so on native it skips the n++ branch and runs only c.msgReader.limitReader.limit.Store(-1); on Wasm (ws_js.go:440) the helper is the single line c.msgReadLimit.Store(n). Inline the stores: SetReadLimit keeps its original body under the lock, and disableReadLimitUnlessSet stores -1 directly. Then readLimitSet and the limit change only inside the two locked methods, and the comment added in b520823 is no longer needed.
🤖
| readControlBuf [maxControlPayload]byte | ||
| msgReader *msgReader | ||
|
|
||
| // readLimitSet is true once SetReadLimit has been called, and NetConn |
There was a problem hiding this comment.
Nit [CRF-19] The readLimitMu/readLimitSet comment, the same on both Conn types (here and ws_js.go:50), describes the body of disableReadLimitUnlessSet instead of naming it. (Gon)
After edbbce1 the check and update live in that three-line function, and a43a2eb lengthened the comment to spell them out. Suggested text for both types: // readLimitSet is true once SetReadLimit has been called, and NetConn then leaves the limit unchanged. readLimitMu makes disableReadLimitUnlessSet atomic with SetReadLimit and is never held during I/O.
🤖
| return nc | ||
| }, overReadLimit, websocket.ErrMessageTooBig}, | ||
| } { | ||
| t.Run("netConn/readLimit/"+tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
Nit [CRF-20] The new subtests are named netConn/readLimit/<case>, so the existing never-set subtest netConn/readLimit (conn_test.go:288) reads as their parent rather than as one of the cases. (Gon)
go test -run 'TestConn/netConn/readLimit' -v lists netConn/readLimit next to netConn/readLimit/BeforeNetConn and the others, and the Wasm table calls the same case Unset. Rename the subtest at line 288 to netConn/readLimit/Unset.
🤖
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. A native test also callsSetReadLimitwhileNetConnruns, and under-raceit reports a data race if the lock is removed../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.