The review of #801 (celeris#750) left these minor findings and nits. Its blocking finding is fixed on the branch at f8f51fd: a held completion kept its conn on the dirty list, so the worker waited with a zero timeout, a spin, for as long as the handler ran (about 375 ms CPU per second, measured). The items below were left for later, and each says what to do. Line numbers are at f8f51fd.
1. The end-to-end test has no witness that a completion was held (minor)
TestIouringSendCompletionDuringASlowAsyncHandlerDoesNotStallItsWorker checks only the fast conn's error (send_completion_stall_linux_test.go:530), its worst latency against stallBudget704 (:533) and the /big read error (:538). It does not check that a ring SEND of /big's tail existed and completed while /slow's handler ran. If the server's send buffer takes the whole direct write, which can happen with a larger tcp_wmem, for example on bare metal or on a tree with #800, no ring SEND is made. Nothing is then held, and max_ms < 300 passes without exercising the fix. #811 lists this as an open candidate for its own single-request arm. On the laptop and in CI the window is entered: main fails 3 of 3 at 742 to 758 ms.
Cluster row 52 (evidence/_queue/cluster.tsv) judges this test. Until it has a witness, the row requires big_ok=false on every celeris750 SENDSTALL line. On this branch, which does not have #800, that shows /big's tail was still queued when /slow's handler wrote. This interim witness stops working once #800 is merged, because /big then arrives intact.
To do:
- Read
EngineMetrics.RingBytes (engine/engine.go:359, filled at engine/iouring/engine.go:507) before and after /slow, and fail the arm's apparatus check unless it grew. Alternatively, add a test hook with a high-water count of held completions and assert it is greater than 0.
- Make cluster row 52's PASS criterion require that witness.
2. Nothing pins the replay at the top of every hand-back entry (minor)
The replay sits at the top of drainDetachQueue's entry (worker.go:4886-4888), before the asyncClosed, transplantPending (:4909) and asyncH2Promoted branches. It must stay there. finishAsyncTransplant refuses a conn with cs.sending set (transplant_source.go:345), and a held completion keeps cs.sending set. endDispatch has already cleared relinkOwed. So a replay moved below the claim branch would leave the held completion (for example a partial SEND's remainder) unapplied until the conn's next request or close, and the client would wait for the rest of its response. handBack750 exercises only the loop-top hand-back, and the round-1 mutant M2 deletes the replay outright.
To do: add an arm that holds a completion and lets the goroutine exit through the park claim (with a drain set) and through the h2c exit. Assert that the entry applies the completion.
3. Two defensive parts have no test (minor)
Two changes survive every test when removed:
releaseConnState clearing heldSends (conn.go:550). Mutant Mrelease removes it.replayHeldSends dropping completions for a conn that left its slot or changed generation (worker.go:3461). Mutant Mgen keeps only the bounds check.
The review ran both with -race in the m8 shape, and both passed.
To do: add a test that holds a completion, then releases and reuses the connState (or the fd number), and asserts that nothing is applied to the new owner.
4. The end-to-end test waits 15 s per run on a tree without #800 (nit)
Without #800, /slow's response is interleaved into /big's, and the /slow read waits for its 15 s socket deadline (send_completion_stall_linux_test.go:486). CI Unit job 108811856764 logs the test at 15.09 s, which adds about 17 s to the engine/iouring step, and that step runs with -timeout=300s. With #800 merged the wait goes away.
To do: set a short read deadline before the /slow read, so the test is cheap in either merge order.
5. For the record: fixed in round 2
- The blocking finding.
flushDirty now gives up a conn with held completions, and the hand-back lists it again. The check is in the pass, not only where the completion is held. The reason: when the goroutine has already entered its next handler, the hand-back's drain entry holds the completion again and then lists the conn. The review's one-line control (unlink in holdOrLockSend) misses that case. The new held_again arm fails on it 3 of 3 in both shapes. In a CPU probe with a second /slow sent in its own recv, that control spun at about 375 ms per second in 5 of 6 runs, and this head used 5.2 to 7.6 ms in every run.
- The cost wording (nit). The PR body no longer credits the removed
defer with pass 2's conn=sync "-11.63%": that comparison is against a main column with a level shift. The body now says only "no regression detected".
The review of #801 (celeris#750) left these minor findings and nits. Its blocking finding is fixed on the branch at f8f51fd: a held completion kept its conn on the dirty list, so the worker waited with a zero timeout, a spin, for as long as the handler ran (about 375 ms CPU per second, measured). The items below were left for later, and each says what to do. Line numbers are at f8f51fd.
1. The end-to-end test has no witness that a completion was held (minor)
TestIouringSendCompletionDuringASlowAsyncHandlerDoesNotStallItsWorkerchecks only the fast conn's error (send_completion_stall_linux_test.go:530), its worst latency againststallBudget704(:533) and the/bigread error (:538). It does not check that a ring SEND of/big's tail existed and completed while/slow's handler ran. If the server's send buffer takes the whole direct write, which can happen with a largertcp_wmem, for example on bare metal or on a tree with #800, no ring SEND is made. Nothing is then held, andmax_ms < 300passes without exercising the fix. #811 lists this as an open candidate for its own single-request arm. On the laptop and in CI the window is entered: main fails 3 of 3 at 742 to 758 ms.Cluster row 52 (
evidence/_queue/cluster.tsv) judges this test. Until it has a witness, the row requiresbig_ok=falseon everyceleris750 SENDSTALLline. On this branch, which does not have #800, that shows/big's tail was still queued when/slow's handler wrote. This interim witness stops working once #800 is merged, because/bigthen arrives intact.To do:
EngineMetrics.RingBytes(engine/engine.go:359, filled atengine/iouring/engine.go:507) before and after/slow, and fail the arm's apparatus check unless it grew. Alternatively, add a test hook with a high-water count of held completions and assert it is greater than 0.2. Nothing pins the replay at the top of every hand-back entry (minor)
The replay sits at the top of
drainDetachQueue's entry (worker.go:4886-4888), before theasyncClosed,transplantPending(:4909) andasyncH2Promotedbranches. It must stay there.finishAsyncTransplantrefuses a conn withcs.sendingset (transplant_source.go:345), and a held completion keepscs.sendingset.endDispatchhas already clearedrelinkOwed. So a replay moved below the claim branch would leave the held completion (for example a partial SEND's remainder) unapplied until the conn's next request or close, and the client would wait for the rest of its response.handBack750exercises only the loop-top hand-back, and the round-1 mutant M2 deletes the replay outright.To do: add an arm that holds a completion and lets the goroutine exit through the park claim (with a drain set) and through the h2c exit. Assert that the entry applies the completion.
3. Two defensive parts have no test (minor)
Two changes survive every test when removed:
releaseConnStateclearingheldSends(conn.go:550). Mutant Mrelease removes it.replayHeldSendsdropping completions for a conn that left its slot or changed generation (worker.go:3461). Mutant Mgen keeps only the bounds check.The review ran both with
-racein the m8 shape, and both passed.To do: add a test that holds a completion, then releases and reuses the
connState(or the fd number), and asserts that nothing is applied to the new owner.4. The end-to-end test waits 15 s per run on a tree without #800 (nit)
Without #800,
/slow's response is interleaved into/big's, and the/slowread waits for its 15 s socket deadline (send_completion_stall_linux_test.go:486). CI Unit job 108811856764 logs the test at 15.09 s, which adds about 17 s to theengine/iouringstep, and that step runs with-timeout=300s. With #800 merged the wait goes away.To do: set a short read deadline before the
/slowread, so the test is cheap in either merge order.5. For the record: fixed in round 2
flushDirtynow gives up a conn with held completions, and the hand-back lists it again. The check is in the pass, not only where the completion is held. The reason: when the goroutine has already entered its next handler, the hand-back's drain entry holds the completion again and then lists the conn. The review's one-line control (unlink inholdOrLockSend) misses that case. The newheld_againarm fails on it 3 of 3 in both shapes. In a CPU probe with a second/slowsent in its own recv, that control spun at about 375 ms per second in 5 of 6 runs, and this head used 5.2 to 7.6 ms in every run.deferwith pass 2'sconn=sync"-11.63%": that comparison is against a main column with a level shift. The body now says only "no regression detected".