Skip to content

fix(iouring): do not mark a conn recv-paused when the cancel was never submitted - #511

Merged
FumingPower3925 merged 2 commits into
mainfrom
fix/iouring-recv-pause-sqe-full
Sep 7, 2026
Merged

fix(iouring): do not mark a conn recv-paused when the cancel was never submitted#511
FumingPower3925 merged 2 commits into
mainfrom
fix/iouring-recv-pause-sqe-full

Conversation

@FumingPower3925

Copy link
Copy Markdown
Contributor

The defect

drainDetachQueue takes a bare w.ring.GetSQE() for the WebSocket recv-pause cancel, then assigns cs.recvPaused = desired unconditionally, outside the if sqe != nil block:

if desired := cs.recvPauseDesired.Load(); desired != cs.recvPaused {
    if desired {
        if sqe := w.ring.GetSQE(); sqe != nil {   // ← no retry
            prepCancelUserDataSkipSuccess(...)
        }                                         // ← nil: nothing submitted
    } else {
        ... prepareRecv ...
    }
    cs.recvPaused = desired                       // ← set anyway
}

When the submission ring is full the cancel is never submitted, but the connection is recorded as paused while its multishot recv is still kernel-armed. The matching resume then sees desired=false != recvPaused=true, takes the re-arm branch, and calls prepareRecv — arming a second multishot recv on the same socket. Two armed recvs deliver interleaved data.

Nothing retried either. PauseRecv enqueues only on the false→true transition of recvPauseDesired, and that flag is already true by the time the drain runs, so a dropped pause was lost silently rather than reattempted.

The fix

Use getCancelSQE() — which submits and retries once when the ring is full — the form the close path already uses in the same file. Record the paused state only when the cancel actually went in, and re-enqueue the connection so the next drain retries.

The pause path is reachable only from WebSocket backpressure, so this is io_uring-only and WS-only. Epoll applies the same pause as a synchronous EPOLL_CTL_MOD that cannot be dropped and has no op to double-arm.

Regression guard

middleware/websocket/detach_residual_heap_linux_test.go — engine-parameterized, asserts residual heap across detached-connection teardown on both HeapAlloc and HeapInuse, so retention and span slack are distinguished.

It drives the validator's own RFC-6455 abuse modes (invalid UTF-8, unmasked client, continuation-without-start, reserved bits, ping flood) so teardown is server-initiated, which is the shape real traffic takes. An earlier draft closed from the client instead and passed trivially on a path that leaks nothing — worth knowing if this test is ever revised.

Measured on msa2-server (amd64, kernel 7.0.0-30), 3000 detaches per mode, HeapAlloc bytes per detach:

mode epoll io_uring
invalid-utf8 28.1 2.5
unmasked-client 12.9 −3.5
continuation-no-start 1.1 −1.3
fragmented-reserved 2.2 4.5
ping-flood −0.8 −2.6

Scope — please read before merging

This was written to reproduce the v1.5.11 24h soak's I-MEM-1 failure (2.4–2.7 KB/s heap trough slope on auth_session_ratelimit × io_uring, both architectures, ~156 B/detach). It does not reproduce it, as the table above shows.

So this PR fixes a real, independently verified defect on its own correctness merits. It does not claim to fix the soak failure, whose cause remains unestablished. Do not treat merging this as clearing the gate.

Verification

  • go build ./..., go vet ./engine/iouring/ clean.
  • engine/iouring full suite: PASS on msa2-server amd64.
  • middleware/websocket suite (incl. the celeris#482 regression guard TestBackpressurePauseDoesNotCancelInflightSend): PASS.
  • New guard: 10/10 sub-tests PASS across both engines.

…r submitted

drainDetachQueue took a bare w.ring.GetSQE() for the WebSocket recv-pause
cancel and then assigned cs.recvPaused = desired UNCONDITIONALLY, outside the
if sqe != nil block. When the SQ ring is full the cancel is never submitted,
yet the conn is recorded as paused while its multishot recv is still
kernel-armed. The matching resume sees desired=false != recvPaused=true, takes
the re-arm branch, and calls prepareRecv -- arming a SECOND multishot recv on
the same socket. Two armed recvs deliver interleaved data.

Nothing retried, either: PauseRecv enqueues only on the false->true transition
of recvPauseDesired, and that flag is already true by then, so the dropped
pause was lost silently rather than reattempted.

Fix: use getCancelSQE(), which submits and retries once when the ring is full
-- the form the close path already uses, in the same file. Record the paused
state only when the cancel actually went in, and re-enqueue the conn so the
next drain retries. The pause path is reachable only from WebSocket
backpressure, so this is io_uring-only and WS-only; epoll applies the same
pause as a synchronous EPOLL_CTL_MOD that cannot be dropped and has no op to
double-arm.

Also adds middleware/websocket/detach_residual_heap_linux_test.go, an
engine-parameterized guard on residual heap across detached-connection
teardown. It drives the validator's own RFC-6455 abuse modes so teardown is
server-initiated, which is the shape production traffic takes; an earlier
draft closed from the client and passed trivially on a path that leaks
nothing. Measured on amd64, 3000 detaches per mode, io_uring reads at or below
epoll on every mode and never above 4.5 B/detach.

The test was written to reproduce the v1.5.11 24h soak's I-MEM-1 failure
(2.4-2.7 KB/s heap trough slope, ~156 B/detach, io_uring only). It does not
reproduce it. That failure's cause is NOT established and this commit does not
claim to fix it.
@FumingPower3925
FumingPower3925 merged commit 476ca4e into main Sep 7, 2026
10 checks passed
@FumingPower3925
FumingPower3925 deleted the fix/iouring-recv-pause-sqe-full branch September 7, 2026 17:33
FumingPower3925 added a commit that referenced this pull request Sep 7, 2026
My #511 stopped marking a conn paused when the cancel SQE could not be
obtained. With no retry that means the middleware's requested pause NEVER
takes effect: recvPauseDesired stays true, recvPaused stays false, and
PauseRecv only enqueues on the false->true transition, so nothing tries again.
Under sustained backpressure the peer keeps filling a buffer nobody drains and
the conn hangs -- which regressed the celeris#482 guard with close-timeouts on
kernel 6.17 CI runners, where io_uring probes at tier=optional and the ring
fills far more readily than on the cluster at tier=high.

The first attempt at a fix re-enqueued the conn instead. That was worse: the
worker loop drains the queue every iteration, so it spun on that conn rather
than reaping the completions that would free the ring, and each pass appended
the same conn again.

Restore the shipped semantics -- mark the conn paused unconditionally -- and
keep the one genuine improvement from #511: getCancelSQE submits and retries
once, so the cancel actually lands far more often than with a bare GetSQE.

The residual risk is the one #482 documented: on a full ring the recv is still
kernel-armed while we record it as paused, so a later resume can arm a second
multishot recv. That is pre-existing, rare, and strictly less harmful than a
pause that never happens at all.
FumingPower3925 added a commit that referenced this pull request Sep 7, 2026
* fix(iouring): do not re-enqueue a dropped recv-pause, it spins the worker

#511 added a re-enqueue when getCancelSQE could not obtain an SQE, so the
pause would be retried. That was wrong in two ways, and it regressed main.

The worker loop calls drainDetachQueue on every iteration. Re-enqueueing the
conn and re-arming detachQPending therefore makes the worker drain the queue
again immediately instead of reaping the completions that would free the ring
-- and because the append happens while the drain iterates its swapped-out
spare slice, each pass appends the SAME conn again, so the queue grows without
bound for as long as the ring stays full.

The symptom is the celeris#482 regression guard failing on io_uring with
close-timeouts: TestBackpressurePauseDoesNotCancelInflightSend reported
conns that never completed their Close handshake, on kernel 6.17 CI runners
where io_uring probes at tier=optional and the ring fills more readily than on
the cluster. It failed on two PRs that touch neither io_uring nor WebSockets
(#513 ratelimit, #515 h1), which is what identified #511 rather than either of
them as the cause.

Drop the re-enqueue and keep the correctness property: still do NOT record the
conn as paused when the cancel was not submitted, because its multishot recv
is still kernel-armed and a resume would arm a second one. The pause is simply
deferred -- recvPauseDesired stays true and recvPaused stays false, so the next
drain retries. A conn under backpressure is by definition still moving data, so
another drain is imminent. Until then recv stays armed, which costs throughput
on one conn but is always safe, unlike a phantom pause.

* fix(iouring): restore the unconditional recv-pause state assignment

My #511 stopped marking a conn paused when the cancel SQE could not be
obtained. With no retry that means the middleware's requested pause NEVER
takes effect: recvPauseDesired stays true, recvPaused stays false, and
PauseRecv only enqueues on the false->true transition, so nothing tries again.
Under sustained backpressure the peer keeps filling a buffer nobody drains and
the conn hangs -- which regressed the celeris#482 guard with close-timeouts on
kernel 6.17 CI runners, where io_uring probes at tier=optional and the ring
fills far more readily than on the cluster at tier=high.

The first attempt at a fix re-enqueued the conn instead. That was worse: the
worker loop drains the queue every iteration, so it spun on that conn rather
than reaping the completions that would free the ring, and each pass appended
the same conn again.

Restore the shipped semantics -- mark the conn paused unconditionally -- and
keep the one genuine improvement from #511: getCancelSQE submits and retries
once, so the cancel actually lands far more often than with a bare GetSQE.

The residual risk is the one #482 documented: on a full ring the recv is still
kernel-armed while we record it as paused, so a later resume can arm a second
multishot recv. That is pre-existing, rare, and strictly less harmful than a
pause that never happens at all.

* fix(iouring): invert the SQE branch so it carries no empty block

revive's empty-block fired on the sqe == nil arm, which after restoring the
unconditional pause assignment held only the explanation. Invert to
`if sqe := w.getCancelSQE(); sqe != nil` and keep the reasoning above the
branch, where it explains the unconditional assignment rather than an empty
arm. No behaviour change.

* revert(iouring): restore the pre-#511 recv-pause branch verbatim

Diagnostic revert. #511 changed two things in drainDetachQueue's pause branch:
the bare w.ring.GetSQE() became w.getCancelSQE() (which submits and retries),
and the cs.recvPaused assignment became conditional. Restoring the conditional
assignment alone did NOT fix the celeris#482 guard on CI, so this reverts the
hunk to ed3d0af verbatim to establish whether #511 is the cause at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant