The review of #800 (celeris#751) had no blocking findings. It left these minor findings and nits, and each says what to do. Line numbers are at the PR's head b340cbe. The review's probes and logs are copied to evidence/lanes-20260927/EP-3/review-r1/ (correctness/ and evidence/).
1. The regression test covers one clause of ringSendOutstanding (minor)
ringSendOutstanding (worker.go:4597-4598) is cs.sending || cs.zcNotifPending || len(cs.sendBuf) > 0 || len(cs.bodyBuf) > 0. TestAsyncResponseWaitsForAnInFlightRingSend reaches only the cs.sending clause. The review's mutants M3 (return cs.sending) and M4 (return len(cs.sendBuf) > 0) each pass the test 4 of 4 times (review-r1/evidence/logs/800-mutants-m8.log).
The state that only the sendBuf clause catches is real. completeSend keeps a partial SEND's remainder in sendBuf. When the SQ ring is full, flushSend returns true and leaves the conn with sending=false (worker.go:5166-5171). SQ-full is the common case at CI's one-worker memlock. The review's probe review-r1/evidence/ov/800probe/zz_review_probe_751_sqfull_linux_test.go builds this state: the kernel sends 4096 bytes, the 64-entry SQ ring is filled, the SEND completes partially, and then a pipelined /small arrives. Results:
- head b340cbe: PASS 2 of 2,
early=0 sends=2.
- head with M3: FAIL 2 of 2,
early=103 sends=1, "response 1 corrupted at byte 223253".
- main dfd044f: fails the same way, 2 of 2.
To do: commit the probe as a third arm of the test. Add a SEND_ZC arm (first completion with F_MORE, then the notification) to cover zcNotifPending.
2. The guarded writeFn keeps its own copy of the condition (nit)
worker.go:2376-2377 tests !cs.fixedFile && !cs.sending && !cs.zcNotifPending && len(cs.sendBuf) == 0 && len(cs.bodyBuf) == 0 && .... That is the same four clauses as the new helper. No test goes beyond cs.sending for either copy, so the two can drift apart without any test noticing.
To do: use !ringSendOutstanding(cs) there, which keeps one source for the condition. Item 1's arms then cover both uses.
3. ringSendOutstanding's comment overstates the locking (nit)
The comment says "The worker mutates all four under cs.detachMu ... so no SEND can start while it does" (worker.go:4594-4595). Two worker paths call flushSend without detachMu:
closeConn's deferred-close flush (worker.go:3771);
- the
h2Conns write-queue drain (worker.go:1347).
Neither can overlap the goroutine's direct write:
closeConn sets asyncClosed first, and the goroutine either re-checks it under detachMu or has already gone.
h2Conns holds only conns whose goroutine has exited.
So the check is correct, but the comment should state this narrower invariant.
To do: reword the comment.
4. Over the send cap, the async path drops whole responses without closing (minor, pre-existing; covered by #805)
makeWriteFn silently drops a write made while writeBuf + sendBuf + bodyBuf exceeds sendCap() (4 MiB, worker.go:4608). The inline path closes the conn in that case (respondAndArm's cap check). runAsyncHandler never checks.
With #800, pipelined responses queue in writeBuf behind an outstanding SEND, including across separate goroutine iterations, where main would have written them directly. So a pipelining client behind a large response can lose whole responses without the conn closing. A later response can then be read as the answer to an earlier request.
The review's probe review-r1/correctness/probes/zz_review_overcap_linux_test.go sends /big (1 MiB) with its tail SEND outstanding, then 6 pipelined 1 MiB responses (-race, unlimited memlock, 2 of 2 runs):
So this is not a regression, but #800's ordering guarantee holds only below the cap.
Open PR #805 (celeris#761) makes a refused write set writeRefused, and it closes the conn on the dispatch goroutine's path too.
To do: once #800 and #805 are both merged, run the probe on main and confirm that the conn is closed and no response is lost silently. If #805 does not cover this, close over the cap on the async path, or stop dispatching new requests while the backlog is over the cap.
The review of #800 (celeris#751) had no blocking findings. It left these minor findings and nits, and each says what to do. Line numbers are at the PR's head b340cbe. The review's probes and logs are copied to
evidence/lanes-20260927/EP-3/review-r1/(correctness/andevidence/).1. The regression test covers one clause of
ringSendOutstanding(minor)ringSendOutstanding(worker.go:4597-4598) iscs.sending || cs.zcNotifPending || len(cs.sendBuf) > 0 || len(cs.bodyBuf) > 0.TestAsyncResponseWaitsForAnInFlightRingSendreaches only thecs.sendingclause. The review's mutants M3 (return cs.sending) and M4 (return len(cs.sendBuf) > 0) each pass the test 4 of 4 times (review-r1/evidence/logs/800-mutants-m8.log).The state that only the
sendBufclause catches is real.completeSendkeeps a partial SEND's remainder insendBuf. When the SQ ring is full,flushSendreturnstrueand leaves the conn withsending=false(worker.go:5166-5171). SQ-full is the common case at CI's one-worker memlock. The review's probereview-r1/evidence/ov/800probe/zz_review_probe_751_sqfull_linux_test.gobuilds this state: the kernel sends 4096 bytes, the 64-entry SQ ring is filled, the SEND completes partially, and then a pipelined/smallarrives. Results:early=0 sends=2.early=103 sends=1, "response 1 corrupted at byte 223253".To do: commit the probe as a third arm of the test. Add a SEND_ZC arm (first completion with F_MORE, then the notification) to cover
zcNotifPending.2. The guarded
writeFnkeeps its own copy of the condition (nit)worker.go:2376-2377tests!cs.fixedFile && !cs.sending && !cs.zcNotifPending && len(cs.sendBuf) == 0 && len(cs.bodyBuf) == 0 && .... That is the same four clauses as the new helper. No test goes beyondcs.sendingfor either copy, so the two can drift apart without any test noticing.To do: use
!ringSendOutstanding(cs)there, which keeps one source for the condition. Item 1's arms then cover both uses.3.
ringSendOutstanding's comment overstates the locking (nit)The comment says "The worker mutates all four under cs.detachMu ... so no SEND can start while it does" (
worker.go:4594-4595). Two worker paths callflushSendwithoutdetachMu:closeConn's deferred-close flush (worker.go:3771);h2Connswrite-queue drain (worker.go:1347).Neither can overlap the goroutine's direct write:
closeConnsetsasyncClosedfirst, and the goroutine either re-checks it underdetachMuor has already gone.h2Connsholds only conns whose goroutine has exited.So the check is correct, but the comment should state this narrower invariant.
To do: reword the comment.
4. Over the send cap, the async path drops whole responses without closing (minor, pre-existing; covered by #805)
makeWriteFnsilently drops a write made whilewriteBuf + sendBuf + bodyBufexceedssendCap()(4 MiB,worker.go:4608). The inline path closes the conn in that case (respondAndArm's cap check).runAsyncHandlernever checks.With #800, pipelined responses queue in
writeBufbehind an outstanding SEND, including across separate goroutine iterations, where main would have written them directly. So a pipelining client behind a large response can lose whole responses without the conn closing. A later response can then be read as the answer to an earlier request.The review's probe
review-r1/correctness/probes/zz_review_overcap_linux_test.gosends/big(1 MiB) with its tail SEND outstanding, then 6 pipelined 1 MiB responses (-race, unlimited memlock, 2 of 2 runs):responses_received=5 (want 7), with the conn still open.malformed HTTP version, which is the io_uring: an async handler's direct write goes out while a ring SEND of the previous response is still in flight, interleaving pipelined responses #751 corruption.So this is not a regression, but #800's ordering guarantee holds only below the cap.
Open PR #805 (celeris#761) makes a refused write set
writeRefused, and it closes the conn on the dispatch goroutine's path too.To do: once #800 and #805 are both merged, run the probe on main and confirm that the conn is closed and no response is lost silently. If #805 does not cover this, close over the cap on the async path, or stop dispatching new requests while the backlog is over the cap.