fix(sockopts): drain what the receive queue actually holds before close, not a 32 KiB constant - #605
Merged
Merged
Conversation
Contributor
Author
|
CI found a real hole in this fix, and the fix's own truth table is what caught it. In that repetition the drain stopped at 72 KiB with 56 KiB still queued, and The cause is the 1 ms wall-clock budget the last review asked for, now checked inside the round: 128 KiB at a 4 KiB chunk is 32 Direction for the next revision, to be measured not argued: keep the byte budget as the primary bound, raise the time backstop far above any healthy drain (10 ms is still four orders of magnitude below the close deadline), and cut the syscall count by reading into a larger buffer so 128 KiB costs a handful of reads rather than 32. Then re-run the Tier 0 table with reps well above 20 on a deliberately loaded box, because a 1-in-20 event needs the sample size to be visible, and the Tier 1 flood cells on both engines. Held until that reads clean. The finding is good news for the rig: the truth table discriminates on a real kernel under real load, which is exactly what it was built for. |
…w clamped shut and two fixed bounds, so the backpressure close sends FIN (#569) The close-time drain exists so close(2) sends FIN instead of RST, because an RST also destroys what is staged in the SEND buffer. It only has that effect if it leaves the queue EMPTY: close(2) resets on one unread byte. Two measured reasons it did not (celeris#583 rig, both loop engines): - The 32 KiB cap from #572 (8 x 4 KiB) is below the autotuned receive buffer, so on the close this exists for — a recv-paused connection whose peer filled the window, 127-131 KiB queued — the drain stopped ~96 KiB short and close(2) reset on 2304 of 2304 closes, indistinguishable from no drain at all: the peer received neither the 19-26 KiB echo backlog nor the WebSocket Close frame. - Draining the SIOCINQ snapshot instead of a constant does not fix it either. Every read reopens the receive window and the peer's already-committed backlog follows the drain in: measured on the flood cell, the drain consumed its whole 128-129 KiB snapshot and close(2) still found 64-126 KiB queued, still resetting 32 of 32. Emptying that queue by reading alone took 20-22 snapshots and 2.6 MB, the peer's whole send buffer, at 250-550 us per close. So the drain now clamps the receive window shut first (TCP_WINDOW_CLAMP below one MSS keeps the kernel advertising a zero window), then drains the entry snapshot. The peer stays window-blocked where it already is, nothing follows the drain in, and that one snapshot empties the queue. Nothing changes on the per-request path; an empty queue at close costs one ioctl and no read. TWO bounds hold one close, and both are fixed before the first read: - Bytes: min(SIOCINQ-at-entry, SO_RCVBUF), computed ONCE by drainRecvBudget. - Time: drainRecvTimeBudget (1 ms), checked BETWEEN THE READS inside drainRecvRound, so the worst case is the budget plus one 4 KiB read. Neither can be raised by anything the peer does after the call starts. Chasing further could not help anyway: bytes that arrive after close(2) are answered by the kernel with an RST no matter what userspace did first. ## What the review asked for 1. "The only bound is a 1 ms wall clock checked BETWEEN rounds, so the worst case is 1 ms plus one whole SO_RCVBUF round." Accepted. The per-round re-snapshotted budget is gone: the byte bound is computed once from the entry SIOCINQ and the deadline is now consulted between the reads. This is the bound celeris#569's own fix candidate asked for ("drain up to the socket's SO_RCVBUF; one SIOCINQ read bounds the work"). 2. "TestDrainRecvBufferStopsAtTheTimeBudget asserts against a 1 s bound, which would pass even if the budget regressed a thousandfold." Accepted, and it was worse than that: measured, that test never stresses either bound. The drain outruns a single writing goroutine on a socketpair and ends on EAGAIN at 65 536 bytes in 10.9-17.2 us, so a 20 ms assertion would still have been vacuous. It now asserts 20 x drainRecvTimeBudget AND drained <= SO_RCVBUF, and the two bounds get deterministic discriminators of their own, each with its own negative control: - TestDrainRecvBufferByteBoundIsComputedOnce uses a SOCK_DGRAM socketpair, where Linux's unix_inq_len() reports only the FIRST datagram (it sums the queue only for SOCK_STREAM/SOCK_SEQPACKET). Three 4 KiB datagrams therefore sit behind a SIOCINQ of 4096. A once-computed budget drains 4096 and leaves 8192; a re-snapshotted one drains all 12 288. Measured both ways, below. - TestDrainRecvRoundStopsAtItsDeadline gives a round a budget of 1 GiB and a deadline already in the past: checked between the reads it consumes 0, checked only around the round it consumes the whole 180 224-byte backlog. Measured both ways, below. 3. Remaining claims, addressed rather than restated: - "The clamp is best-effort — not load-bearing for correctness, only for cost." That was the previous comment's claim and it is wrong now that a fixed byte bound sits behind it, so it is corrected, not defended: on a kernel that ignores TCP_WINDOW_CLAMP the reads reopen the window, the budget is spent on what follows them in, and the queue can still be non-empty at close(2). That close then resets exactly as it did before this fix — the floor this replaces, not a regression below it. The dependency does not run the other way: clamping can never make a close worse than not clamping. - Shrinking a live connection's receive window. It is never a live connection. All three call sites are shutdown(SHUT_WR) -> drain -> close(2) with nothing in between: engine/epoll/loop.go:2606, engine/iouring/worker.go:3166 and :3275. The socket is already half-closed and destroyed microseconds later. - 1 ms of event-loop thread per close. It is a ceiling, not a cost. Measured per close over 3072 joined closes, drain-on: flood cell p50 17.8 / 23.7 us, p99 69.8 / 112.8 us (epoll / io_uring); small cell p50 3.8 us on both. The 9 closes above 500 us all drained their full snapshot with inq_after=0, and one of them had inq_before=0 and drained=0 — i.e. the tail is container scheduling, not the deadline truncating a drain. No close in any arm was cut short by either bound: drained == inq_before on 3072 of 3072. - The drain discarding inbound data, #569 as originally filed. Still refuted and unchanged by this patch: tcp_close() frees the receive queue on both paths, so the inbound loss is identical with and without the drain. Re-running the rigs also caught a wedge in the branch's own harness, which is fixed here: the "peer that never stops" goroutine wrote in BLOCKING mode, so once the drain stopped reading it parked inside write(2) and never looked at its stop channel again, leaving the WaitGroup the test waits on unreleased. It hung a full -race run of ./internal/sockopts for the whole 20-minute timeout, with the writer's stack in syscall.Syscall(write) and the test's in sync.WaitGroup.Wait. It is now floodPeer, which writes non-blocking and treats EAGAIN as "try again", so the stop check is reachable on every iteration. The race is narrow and the counts are correspondingly weak: with the blocking writer it reproduced once in 80 runs under docker --cpus 1 (plus the original 20-minute hang); with floodPeer, 60 runs under --cpus 1, 20 under --cpus 4 and three full -race suites, none hung. ## Measured docker --cpus 4, kernel 7.0.12-linuxkit, celeris_closeprobe rig with CELERIS_DEBUG_CLOSE_PROBE=1, 32 conns per cell, all closes joined by raddr==laddr, P(inq_before>0)=1.000 in every cell. Tier 1, drain-on, 24 runs per engine (768 joined closes per cell): - flood cell, inq_before 128 000-131 568 B: drained == inq_before on 768/768 (drainedSum == inqBeforeSum, 98 811 424 epoll / 98 914 624 io_uring), inq_after>0 on 0, EOF 768/768, RST 0, closeFrameRx 768/768, fullRx 768/768, truncTail 0, TcpExt TCPAbortOnClose delta 0, outq at close 23 174 B. Per close: epoll p50 17.8 p95 42.0 p99 69.8 max 999.5 us; io_uring p50 23.7 p95 72.5 p99 112.8 max 985.2 us. - small cell, inq_before 16 380-17 388 B: drained == inq_before on 768/768, EOF 768/768, closeFrameRx and fullRx 768/768, abortOnClose 0. Per close: p50 3.8 us both engines, max 73.7 / 797.8 us. Tier 1 A/B arm, CELERIS_DEBUG_SKIP_CLOSE_DRAIN=1, 3 runs per engine: 96/96 RST in both cells on both engines, drained 0, inq_after>0 96/96, closeFrameRx 0, fullRx 0, truncTail 96, abortOnClose 96. The control still shows what the drain is preventing. Tier 1 negative control, the same tree with only DrainRecvBuffer reverted to the pre-#569 fixed 32 KiB cap, 3 runs per engine: flood 96/96 RST, drained exactly 32 768 against inq_before 128 000-130 560, closeFrameRx 0, fullRx 0, abortOnClose 96, inq_after>0 96; small cell unchanged at 96/96 EOF with drained == inq_before. The rig fails (exit 1) on all 6 flood runs, so it asserts the outcome rather than only reporting it. Tier 0 (internal/sockopts), 11 tests / 14 truth-table cells, run plain, with -race and with tcp_fin_timeout=120: all pass in all three. - drain_on/unread=131072, staged 0 and 65 536: drained 131 072/131 072, inq_after>0 on 0/20, abortOnClose 0, peer receives 65 536 of 65 536 staged bytes (the drain-off RST path delivers 24 576). - drain_off/unread=131072: drained 0, inq_after>0 20/20, abortOnClose 20. - window-blocked peer (2 750 604 B sent, 458 699 B queued, SO_RCVBUF 524 288): drained 458 699, inq_after 0, abortOnClose 0, peer EOF, and the close held the thread 48.6 us (plain) / 60.4 us (-race) / 104.7 us (fin120). - dgram byte bound: entry SIOCINQ 4096, 12 288 queued, drained 4096, inq_after 4096. - time budget end to end: 65 536 B in 10.9 us (plain), 65 536 B in 10.9 us (-race), 180 224 B in 159.4 us (fin120) — always within SO_RCVBUF 212 992. Tier 0 negative controls, each the same tree with one thing changed: - 32 KiB cap restored: fails TestDrainRecvBufferDrainsPastTheOldFixedCap (32 768 of 131 072), both drain_on/unread=131072 truth-table cells (drainedMin 32 768, inq_after>0 20/20) and TestDrainRecvBufferEmptiesAWindowBlockedPeer (drained 32 768, 425 931 left, TCPAbortOnClose +1). - bounds as they stood before this review (per-round re-snapshotted budget, deadline only between rounds): fails TestDrainRecvBufferByteBoundIsComputedOnce and nothing else — drained 12 288 against an entry SIOCINQ of 4096. - deadline check taken back out of drainRecvRound: fails TestDrainRecvRoundStopsAtItsDeadline and nothing else — the round consumed 180 224 of 180 224 queued bytes with its deadline already past. - floodPeer's writer put back in blocking mode: hangs TestDrainRecvBufferStopsAtTheTimeBudget, 1 of 80 runs under --cpus 1. Repo gates, all in the container: go vet ./... and GOOS=linux go vet ./... and go vet -tags celeris_closeprobe over the two touched packages all 0; golangci-lint v2.13.2 (built with go1.27.0, the version CI pins) 0 issues for both GOOS; ./internal/sockopts, ./middleware/websocket, ./engine/epoll and ./engine/iouring all ok. The alternative to the clamp was measured and rejected in the previous round, and those figures are carried over rather than re-run here: consuming the peer's backlog instead (20-22 snapshots, 2.6 MB, 250-550 us per close) keeps the Close frame for a peer that reads LATE — probe-off arm, 4 runs per engine, flood cell closeFrameRx 128/128 epoll and 127/128 io_uring, against 0/128 and 28/128 with the clamp — but at 20-40x the cost, and it is itself unreliable at the bound: 1 of 768 closes per engine overran the 1 ms budget and reset (io_uring 127/128 above is the same failure), which loses the frame for prompt readers too. The trade-off and its numbers are in the comment.
FumingPower3925
force-pushed
the
fix/569-drain-to-rcvbuf
branch
from
September 14, 2026 06:51
25a147b to
9f23af5
Compare
…cating it, and make the celeris#311 guard reach a read (#569) CI failed this branch's own truth table on the GitHub runner (kernel 6.17.0-1022-azure), 1 repetition in 20 of TestDrainRecvBufferTCPTruthTable/drain_on/unread=131072/staged=0: inqBeforeMin=131072 inqBeforeMax=131072 drainedMin=73728 drainedMax=131072 inqAfterMin=0 inqAfterMax=57344 inqAfterPos=1 abortOnClose=1 soErrEPIPE=1 The drain stopped at 72 KiB with 56 KiB still queued and close(2) reset: celeris#569's defect, reproduced by this fix's own bound. ## What was wrong, and it was the syscall count The byte budget — min(SIOCINQ-at-entry, SO_RCVBUF), computed once — is what bounds the work. The 1 ms wall clock is only a backstop against reads that are pathologically slow for a reason the byte count cannot see. At 4 KiB a read, the 128 KiB a real flood close queues costs 32 recv syscalls, and 32 syscalls on a loaded runner under -race do not reliably fit in 1 ms. So the backstop was reachable in normal operation, and when it fired it turned a FIN back into an RST — the thing the drain exists to prevent. ## The change - drainRecvBufSize 4 KiB -> 64 KiB, so the same 128 KiB close is two reads and the loop's exit rather than 32 reads. The buffer was a stack array, not pooled; at 64 KiB a frame that size would make the event-loop worker grow and copy its stack on a close path that used to touch neither, so it now comes from a sync.Pool (drainRecvBufPool). Nothing reads the bytes, so buffers go back unzeroed. - drainRecvTimeBudget 1 ms -> 10 ms, and its doc says what it is: a backstop, not the bound. Kept exactly as the last review left them: the byte budget computed ONCE before the first read, the deadline checked BETWEEN THE READS inside drainRecvRound, and the budget test asserting against drainRecvTimeBudget itself rather than a round number. ## Also fixed: the celeris#311 guard was vacuous, and so was its sibling DrainRecvBuffer returns early when SIOCINQ == 0, so TestDrainRecvBufferNonBlocking's empty blocking socketpair never reached a recv and an implementation that dropped MSG_DONTWAIT passed it. TestDrainRecvBufferDrainsThenReturns was vacuous the other way: 35 bytes queued against a 35-byte budget ends on drained == budget without ever reaching an EAGAIN read. #311 is the bug where a blocking read wedged an io_uring worker and stopped the engine, so both now call drainRecvRound directly — the first on an empty blocking socket, the second with a budget four times what is queued so the loop must take the read that finds the queue empty. MEASURED both ways, below. Two stale comments the same review flagged are corrected: close_drain_linux.go claimed a release binary carries "no ioctl on the close path" (the drain's own SIOCINQ is in both builds), and the Tier 1 rig still described its cells against a "32 KiB drain cap" that no longer exists. TestDrainRecvBufferByteBoundIsComputedOnce no longer sizes its datagrams from drainRecvBufSize, and TestDrainRecvRoundStopsAtItsDeadline's skip floor is no longer drainRecvBufSize either: both were coupling a socket-buffer quantity to the read size, which now moves independently and is comparable to a whole default AF_UNIX buffer. What those two discriminate is the budget and the placement of the deadline check, neither of which is about how big one read is. ## Measured docker --cpus 4, kernel 7.0.12-linuxkit, seccomp=unconfined. "Loaded" below means eight `yes > /dev/null` plus a `go build ./...` loop inside the same cgroup, sampled at 399-401% of its 4-CPU quota while the tests ran, with the test itself under -race. Tier 0, all 14 cells, 250 repetitions each (3500 connections), -race, loaded: PASS, 103.7 s, no cell degraded. - drain_on/unread=131072/staged=0 (the cell CI failed): EOF 250/250, RST 0, soErr0 250, inqBefore 131072/131072, drainedMin = drainedMax = 131072, inqAfterPos 0, abortOnClose 0. - drain_on/unread=131072/staged=65536: the same, plus peerBytes 65536 = serverAccepted 65536 on every rep. - drain_on/unread=16384, both staged arms: drained 16384/16384, inqAfterPos 0, abortOnClose 0. - drain_on/unread=0 and drain_off/unread=0: EOF 250/250, abortOnClose 0 — the reset detector does not fire spuriously. - drain_off/unread=16384 and 131072: abortOnClose 250/250, inqAfterPos 250/250, drained 0; staged=0 gives EOF+EPIPE 250/250 and staged=65536 gives ECONNRESET 250/250 with peerBytes 24576 < serverAccepted 65536. - post_close_write, both arms, tcp_fin_timeout=60: abortOnClose 0, abortOnData 0, secondWriteErr 250/250. Tier 0 focused on the CI cell, 1000 repetitions, -race, loaded: both unread=131072 cells 1000/1000 EOF, drainedMin = drainedMax = 131072, inqAfterPos 0, abortOnClose 0. Tier 0 diagnosis control — the same tree with ONLY the two constants put back (4096 and 1 ms), same 1000 repetitions, same load: FAILS both cells, which is the CI signature reproduced locally rather than argued. - staged=0: inqAfterPos 3/1000, abortOnClose 3, drainedMin 45056, inqAfterMax 86016, soErrEPIPE 3. - staged=65536: inqAfterPos 4/1000, abortOnClose 4, RST 4, drainedMin 4096, inqAfterMax 126976. - the seven truncated drains: 4096, 32768, 40960, 45056, 49152, 61440 and 122880 bytes consumed of 131072. Tier 1 (the celeris#583 WebSocket rig, -tags celeris_closeprobe, CELERIS_DEBUG_CLOSE_PROBE=1, 32 conns a run, workers=4 on every engine so the memlock 1-worker confound is not in play), drain-on, 24 runs per engine = 768 joined closes per cell, all six cells informative (P(inq_before>0) 1.000): - flood cell (inq_before 128 000-131 568 B): drainedSum == inqBeforeSum exactly on all three engines (98 779 360 epoll / 98 794 624 io_uring / 98 779 888 io_uring multishot), inq_after>0 on 0 of 768, EOF 768/768, RST 0, closeFrameRx 768/768, fullRx 768/768, truncTail 0, TCPAbortOnClose delta 0, writeBlocked 768/768. - small cell (inq_before 16 380-18 396 B): the same — drainedSum == inqBeforeSum, inq_after>0 on 0, EOF 768/768, closeFrameRx and fullRx 768/768, abortOnClose 0. - cost, over all 4950 probed closes measured on this tree: p50 10.0 us, p95 141 us, p99 285 us, max 1.949 ms. ONE close in 4950 exceeded the old 1 ms budget even with 64 KiB reads; none came within half of the new one. Tier 1 A/B arm, CELERIS_DEBUG_SKIP_CLOSE_DRAIN=1, 3 runs per engine: all six cells 96/96 ECONNRESET, drained 0, inq_after>0 96/96, abortOnClose 96, closeFrameRx 0, fullRx 0, truncTail 96. The control still shows what the drain is preventing, in the small cell as well as the flood one. Tier 1 negative control, the same tree with the byte budget forced back to the fixed 32 KiB constant, 3 runs per engine: the rig FAILS (exit 1) on all nine flood runs — drained exactly 32 768 of 128 000-131 568, up to 98 800 B left queued, 96/96 RST, closeFrameRx 0, fullRx 0, abortOnClose 96 — while the small cell is untouched at 96/96 EOF with drained == inq_before. The rig asserts the outcome rather than only reporting it, and it discriminates on exactly the cell the cap breaks. celeris#311 guard control — MSG_DONTWAIT removed from drainRecvRound and nothing else: - TestDrainRecvBufferNonBlocking FAILS (blocks 3.01 s on the empty blocking socket) and TestDrainRecvBufferDrainsThenReturns FAILS (blocks on the read after the queue is empty). - the two guards AS THEY STOOD BEFORE this commit, run against that same broken implementation, both PASS. That is the vacuity, measured rather than asserted. Suites, all in the same container, -race, -count=1: ./internal/sockopts ok 6.0 s, ./engine/epoll ok 15.9 s, ./engine/iouring ok 102.1 s. ./middleware/websocket FAILED, on TestBackpressureInboundSequenceIntegrity/io_uring/multishot_recv, which is flaky on main too and which this commit neither causes nor cures. See "Still open". CI does not run that package under -race as a whole; it runs ^TestBackpressure at 16 conns x 2 x 1000 frames, a far lighter shape than the default this failure appears at. Repo gates: gofmt -l clean on both touched directories; go vet ./... 0 and GOOS=darwin go vet ./... 0; GOOS=linux GOARCH=arm64 and GOARCH=amd64 go build ./... both clean; golangci-lint v2.13.2 (built with go1.27.1) run ./... 0 issues, and 0 issues again under GOOS=darwin. ## Still open TestBackpressureInboundSequenceIntegrity/io_uring/multishot_recv is flaky at its heavy default parameters, and the first measurement of it here was wrong in a way worth recording: five -race runs on origin/main came back 0 of 5, which read as a clean branch regression. Fifteen more runs on that same tree came back 7 of 20 failing. It is flaky on main. Same container, --cpus 4, 96 conns x 4 x 16 000 frames: - origin/main (8d8f2a5, the pre-#569 32 KiB drain): 7 of 20 failed. - this branch WITHOUT this commit (the rebased #569 fix alone): 3 of 5. - this branch WITH it: 10 of 15. Indistinguishable from the line above; nothing in this commit moves it either way. - the same tree with the window clamp removed: 2 of 5. - the same tree with the byte budget forced back to 32 KiB: 3 of 5. Pooled that is 13 of 20 with the #569 drain in any form against 7 of 20 without it: suggestive at p ~ 0.06, NOT established, and no single mechanism in the change accounts for it — removing the clamp does not clear it and nor does restoring the 32 KiB budget. At the shape CI actually gates it is not there at all. CI runs `-run ^TestBackpressure ./middleware/websocket/...` with WS484_CONNS=16, WS484_BURSTS=2, WS484_BURST_FRAMES=1000. Twenty -race runs of exactly that, per tree: this tree 0 failures of 20; origin/main 4 of 20, every one of them TestBackpressurePauseDoesNotCancelInflightSend, which is celeris#607 and not this test. TestBackpressureInboundSequenceIntegrity did not fail once in the 40 gated-shape runs across both trees. The signature is worth recording because it is celeris#569's own subject: a TAIL truncation rather than corruption — seqGaps 0, parseErr 0, overflowErr 0, but framesIn < framesSent per connection (58 626 of 64 000 on one) with protocol errors on clients the harness did not otherwise fail. Whether the drain makes that worse needs a sample larger than this round could afford, and the flake is unfiled either way. Other things still open: - The backstop is sized from what was measured, not proved: the worst single close over 4950 is 1.949 ms, so 10 ms is 5x that and 35x the p99, but a host slow enough for a two-read drain to exceed 10 ms would truncate again. The truth table is what would catch it, as it did at 1 ms. - celeris#608 (post_close_write/drain=false being intermittently red on main on per-namespace netstat counters and tcp_fin_timeout) did not reproduce in any run here: both post_close_write cells passed at 250 repetitions. This change neither helps nor hurts it — it touches neither the counters nor that path. - Five golangci-lint findings exist in the celeris_closeprobe build (goimports on the rig's import grouping, three misspell hits on a `strat` variable, one revive comment form on CloseProbeHook). They are pre-existing, the tagged build is not linted in CI, and none is in code this commit adds; left alone rather than mixed into this diff. - Carried over and still true: no kernel that ignores TCP_WINDOW_CLAMP was available, so "degrades to pre-fix behaviour" remains reasoning; the rejected-alternative figures are from an earlier round; the entry snapshot still cannot absorb a drip that lands between the ioctl and the last read.
FumingPower3925
force-pushed
the
fix/569-drain-to-rcvbuf
branch
from
September 14, 2026 07:41
9f23af5 to
026d6a6
Compare
Contributor
Author
|
Revised and re-measured. The hole CI found is closed, and the cause was the syscall count, not the byte budget. Change (commit on top, so the delta is reviewable on its own): the read buffer goes from 4 KiB to 64 KiB, so a 128 KiB close costs two reads instead of thirty-two, and since the buffer was a stack array rather than a pooled one it now comes from a Tier 0, 250 reps of all 14 cells, under deliberate load (eight Tier 1, 24 runs per engine, 4608 joined closes: Four negative controls, all fired. The 32 KiB cap restored fails all nine flood runs at exactly 32768 drained with 96/96 resets. The old constants at 1000 reps under load reproduce the CI signature locally: The 10 ms number is sized, not proved: five times the worst of 4950 closes and thirty-five times p99. A host slow enough for a two-read drain to exceed it would truncate again, and the truth table is what would catch that. Separately: celeris#608 did not reproduce here (both |
This was referenced Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the reopened #569 (measured under #583). The rig that found the defect judges the fix, the negative control was run, and an adversarial reviewer re-read the pushed diff rather than the report.
Reviewer verdict
Finding closed: True. Mergeable: True.
All named findings are closed in the pushed code (I read the diff, not the report).
(1) BYTE BOUND, COMPUTED ONCE — CLOSED.
internal/sockopts/sockopts_linux.gonow hasdrainRecvBudget(fd, inq, inqErr) = min(SIOCINQ-at-entry, SO_RCVBUF)with a 32 KiB fallback (drainRecvFallbackBudget) when the fd answers neither. It is evaluated exactly once, as an argument to the singledrainRecvRoundcall; there is no per-round re-snapshot and no outer loop —DrainRecvBufferis one ioctl, one setsockopt, one bounded pass. The discriminator test is real, and its kernel premise checks out: Linuxunix_inq_len()sums the receive queue only for SOCK_STREAM/SOCK_SEQPACKET and returnsskb_peek()->lenotherwise, so three 4 KiB datagrams do sit behind a SIOCINQ of 4096, and a once-computed budget provably stops at 4096. Control (B) failing only that test is consistent with the code.(2) DEADLINE INSIDE THE ROUND — CLOSED.
drainRecvRoundchecks!time.Now().Before(deadline)at the TOP of every read iteration, and clamps the chunk to the remaining budget, so the worst case isdrainRecvTimeBudget+ one 4 KiB read, not + one SO_RCVBUF.TestDrainRecvRoundStopsAtItsDeadline's first half (1 GiB budget, already-expired deadline, expect drained==0) is a genuine discriminator for check placement.(3) TIME-BUDGET TEST — CLOSED. The 1 s assertion is gone; it is now
elapsed > 20*drainRecvTimeBudget(20 ms) plusdrained <= SO_RCVBUF, and the comment states honestly that the test does not discriminate which bound ends the call.(4) DOC CLAIMS — CLOSED, and the call-site claim is true: I read all three sites on the branch (engine/epoll/loop.go:2604-2607, engine/iouring/worker.go:3161-3167 and 3273-3276) and each is
shutdown(SHUT_WR)->sockopts.CloseDrain->unix.Close(fd)with nothing between. The "clamp is not load-bearing" over-claim is retracted rather than defended, and the rejected-alternative numbers are now stated as 128/128 vs 127/128.(5) HARNESS WEDGE — CLOSED.
floodPeersets non-blocking, treats EAGAIN/EWOULDBLOCK as "try again", checksstopevery iteration, and its stop func issync.Once-guarded withwg.Wait(). The blocking-writer deadlock it describes is mechanically real.INDEPENDENT GATES I RAN (branch tree exported with
git archiveinto the scratchpad; no repo mutation, no docker):GOOS=linux GOARCH=amd64 go vet ./internal/sockopts/= 0; same with-tags celeris_closeprobeover./internal/sockopts/ ./middleware/websocket/= 0;GOOS=linux GOARCH=arm64 go build ./...= 0 (so TCP_WINDOW_CLAMP is present on both arches);golangci-lint run(v2.13.2, built with go1.27.0, GOOS=linux) on ./internal/sockopts/... = 0 issues;gofmt -lclean on all five touched files.git diff --name-statusshows 5 modified files and zero additions — no committed scratch files, no debug prints in non-probe code (the onlyFprintfis inclose_drain_probe_linux.go, which is//go:build linux && celeris_closeprobe).HOT PATH: the common empty-queue close is now one SIOCINQ ioctl and zero reads (previously one recvfrom) — cheaper. A non-empty close adds one ioctl + one setsockopt, plus a
time.Now()per 4 KiB read (32 calls at 128 KiB, vDSO). No new allocation;bufis still a stack array.RE-VERIFICATION SUPPORT: the numbers are internally consistent with the code (drained == inq_before on 3072/3072 is exactly what a min(SIOCINQ, SO_RCVBUF) budget plus a clamp predicts), and the four negative controls each fail the one test that the reverted mechanism should break. I could not re-run any of them (no docker, Linux-only tests), so those remain the agent's measurements, not mine. One discrepancy: the commit message's "Measured" block carries an EARLIER round's figures than the report quotes (max 999.5/985.2 us vs 145.8/356.0 us; window-blocked 48.6 us vs 84.96 us; drainedSum 98 811 424 vs 98 847 248). Same tree, different runs — not an error, but the commit is not quoting the run the report describes.
Residuals the reviewer recorded
Three things the agent did not report; none is a production-behaviour defect, the first two are one-line fixes I would want before merge.
THE celeris#311 REGRESSION GUARD IS NOW VACUOUS (new, introduced by this change, unreported).
DrainRecvBufferreturns early whenSIOCINQ == 0, before any recv.TestDrainRecvBufferNonBlocking(/Users/fuming/Documents/github/celeris/celeris/internal/sockopts/drain_recv_linux_test.go) puts an EMPTY blocking-mode socketpair fd in front of the drain and asserts it returns — that now takes the early-return path and never issues a read, so an implementation that dropped MSG_DONTWAIT would still pass.TestDrainRecvBufferDrainsThenReturnsis vacuous for the same reason in the other direction: 35 bytes queued, budget = 35, the loop ends ondrained == budgetwithout ever reaching an EAGAIN read. iouring: drainRecvBuffer blocking read wedges the event-loop worker under connection churn #311 is the bug where a blocking read wedged an io_uring worker and stopped the whole engine, so this is the most expensive guard in the file to lose. Cheap fix: calldrainRecvRound(fd, buf, drainRecvBufSize, time.Time{})directly on an empty blocking socket, which does reach the recv.STALE INVARIANT IN A NEARBY COMMENT. /Users/fuming/Documents/github/celeris/celeris/internal/sockopts/close_drain_linux.go (untouched by this branch) still says of the release build: "Keeping that variant behind a build tag means a release binary carries no environment read and no ioctl on the close path."
DrainRecvBuffernow performs a SIOCINQ ioctl on every close in release builds, so that sentence is false as written. Also stale: the Measure #569: does the pre-close recv drain change what the peer sees (FIN vs RST, lost outbound bytes) on the workload that produced it #583 rig comment in middleware/websocket/server_close_drain_linux_test.go still describes its cells as "below the 32 KiB drain cap" / "far above the cap", a cap that no longer exists.RESIDUAL, NOT A BLOCKER — the entry snapshot cannot absorb a late drip. The budget is exactly
min(SIOCINQ-at-entry, SO_RCVBUF), so bytes that land between the ioctl and the last read are left queued and close(2) resets; the old 8x4 KiB loop ran to EAGAIN and would have consumed them whenever the total stayed under 32 KiB. The epoll call site's own comment names this failure mode ("a multi-us window in which a fresh peer drip queues — then Close sees it and emits RST anyway"). The window is single-digit microseconds, the clamp suppresses new arrivals, and the measured small cell is 768/768 EOF, so this is strictly better than the pre-fix floor — but it is a narrowing in the sub-32-KiB regime that no test covers and the report does not mention. Related minor fragility:TestDrainRecvBufferDrainsPastTheOldFixedCapassertsdrained == queuedwith no guard forqueued > SO_RCVBUF, so a host with a smallnet.core.rmem_defaultwould fail it with the misleading message "the fixed cap is back".Carried over from the agent's own list and still true: the 1 ms wall-clock bound never binds in any measured cell (only the expired-deadline unit test and control C exercise it); no kernel that ignores TCP_WINDOW_CLAMP was available, so the "degrades to pre-fix behaviour" claim is reasoning, not measurement; the rejected-alternative and multishot-recv figures are from the previous round; the blocking-writer hang reproduces ~1 in 80 under --cpus 1, so the wedge fix rests on mechanism plus 80 clean runs. Process: no PR exists for fix/569-drain-to-rcvbuf (
gh pr list --headreturns empty) and issue #569 is still OPEN, milestone v1.6.0, label area/engine.