fix(iouring): retire recvCancelPending when the pause's cancel matches nothing - #602
Merged
Merged
Conversation
…ng (celeris#596) cs.recvCancelPending was never retired when the backpressure pause's ASYNC_CANCEL MISSED -- the recv completed with data before the cancel ran, or nothing was armed -- so the flag stopped describing "a cancel is in flight for this conn's recv". The resume branch reads it to witness the celeris#484 window, and counted every resume after the first missed cancel: RecvResumeWhileCancelPending 11179 / 10721 / 9769 at MaxBackpressureBuffer=8 and 5975 / 6045 / 5996 at 16, against a real RecvResumeWhileRecvInFlight of 0 or 1 (origin/main bef00fc, WS #484 oracle, io_uring single-shot, 3 runs per BP). The issue's premise -- "the cancel's -ENOENT CQE is tagged udProvide and dropped" -- is not what the kernel does. Probed on 7.0.12: with IORING_ASYNC_CANCEL_ALL a cancel that matched nothing completes as res == 0, a SUCCESS, so CQE_SKIP_SUCCESS suppressed the completion entirely and there was no CQE to drop. The miss was unobservable, not merely discarded. Mechanism: * the pause's cancel is now submitted REPORTED (prepCancelUserDataReported, no CQE_SKIP_SUCCESS) and tagged udRecvCancel -- a conn-bound tag, so it passes the generation gate -- instead of the udProvide "drop it" sentinel. One extra CQE per backpressure pause; nothing on the per-request path changes. * handleRecvCancel retires the cancel when it cancelled nothing (res <= 0, covering both res == 0 and -ENOENT) and leaves it outstanding when it did (res > 0, or -EALREADY), because then the recv's own -ECANCELED is coming and handleRecv's re-arm branch (the #484 fix #560 guards) still needs it. * recvCancelPending becomes a COUNT. A conn that pauses, resumes and pauses again before the ring drains has two cancels in flight and they resolve in either order; as a bool the one that MISSED cleared the state the one that HIT still needed, and that -ECANCELED then fell through handleRecv's generic negative-result path and closed a healthy connection mid-stream. That is not theoretical: the bool-valued intermediate version failed the oracle once in 12 runs at BP=8 with parseErr=1 (io.ErrUnexpectedEOF, conn 66). Exactly one retirement per cancel, either from its own completion or from the -ECANCELED it caused. Measured with the merged oracle (TestBackpressureInboundSequenceIntegrity, io_uring single-shot only, N=12 per BP): BP=8 RecvResumeWhileCancelPending 0 in all 12 runs (was ~10^4) BP=16 0 in 11 runs, 1 in the twelfth -- the run that also reported RecvResumeWhileRecvInFlight=1, i.e. the two witnesses now agree both RecvDoubleArmed=0, RecvCQEUnaccounted=0, parseErr=0, 24/24 PASS framesSent is unchanged within noise (control 5.69/6.04/5.61M vs fix 5.81/5.99/5.63/5.66/5.66M at BP=8). Rigs, both of which fail with the fix reverted: * TestMissedPauseCancelClearsCancelPending -- the miss, written without naming udRecvCancel so the body also runs on origin/main, where it fails on the state assertion. * TestMissedCancelDoesNotRetireASecondOutstandingCancel -- two cancels outstanding, miss resolving first; with retireRecvCancel reduced to bool semantics it reaches closeConn on a healthy conn. * TestResumeBeforeCancelLandsPlacesNoSecondRecv, extended to assert that a cancel which HIT does not retire the state its -ECANCELED needs.
Contributor
Author
|
The |
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
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 #596. Measured, not argued: the rig that found the defect is the rig that judges the fix, and the negative control was run.
Mechanism
Three coupled changes, each named in a code comment:
The pause's cancel is now submitted REPORTED instead of suppressed. New helper prepCancelUserDataReported (engine/iouring/sqe.go) = prepCancelUserDataSkipSuccess without CQE_SKIP_SUCCESS, so the cancel ALWAYS posts its own CQE and the engine learns what it did. With IORING_ASYNC_CANCEL_ALL, res > 0 is the number of ops cancelled and res <= 0 means none were. cancelConnOps (close path) keeps the suppressed udProvide form — those conns are being torn down and nothing reads the outcome.
New conn-bound user_data tag udRecvCancel = 0x05 << 56 (engine/iouring/cqe.go), replacing udProvide on the pause cancel only. Being conn-bound it carries the generation and passes the same staleConnCQE gate as the recv it targets; it is not a terminalOp, so kernelInflight accounting is unchanged. Routed in BOTH dispatch switches — the inlined hot-path switch in Worker.run and processCQE — to the new handleRecvCancel, which retires the cancel when it cancelled nothing (res <= 0, covering res == 0 and -ENOENT) and leaves it outstanding when it did (res > 0, or -EALREADY), because then the recv's own -ECANCELED is still coming and handleRecv's re-arm branch (the io_uring WS: backpressure pause/resume drops buffered inbound bytes, truncating frames #484 fix that fix(iouring): never arm a second recv on a connection that already has one (#484) #560 guards) needs it.
SECOND DEFECT, found by measurement, not in the issue: connState.recvCancelPending is now a uint16 COUNT, not a bool, with one retirement per cancel via retireRecvCancel(). A conn that pauses, resumes and pauses again before the ring drains has two ASYNC_CANCELs in flight and they resolve in either order; as a bool, the one that MISSED cleared the state the one that HIT still needed, and that -ECANCELED then fell through handleRecv's generic negative-result path and closed a healthy connection mid-stream — the celeris#484 failure, reintroduced. This was not theoretical: the bool-valued intermediate version failed the oracle once in 12 runs at BP=8 (parseErr=1, io.ErrUnexpectedEOF on conn 66, run log scratchpad/logs/fix/fix_bp8_run1.log).
No per-request hot-path cost: the only added work is one extra CQE per WebSocket backpressure pause, an event that exists only on a detached WS conn under backpressure. handleRecv's data path is untouched.
Verification
ORACLE (merged witnesses, TestBackpressureInboundSequenceIntegrity, io_uring SINGLE-SHOT variant alone via -run 'TestBackpressureInboundSequenceIntegrity/^io_uring$' -skip 'TestBackpressureInboundSequenceIntegrity/io_uring/multishot_recv', N=12 per BP, nothing else running):
BP=8, fix, 12/12 PASS: RecvResumeWhileCancelPending = 0 in all 12 runs; RecvResumeWhileRecvInFlight = 0 in all 12; RecvArmDeclined = 0,2,0,1,1,0,2,0,0,1,1,3; RecvDoubleArmed = 0 and RecvCQEUnaccounted = 0 and parseErr = 0 in all 12.
BP=16, fix, 12/12 PASS: RecvResumeWhileCancelPending = 0,1,0,0,0,0,0,0,0,0,0,0; the single 1 is in the SAME run that reported RecvResumeWhileRecvInFlight = 1 — the two witnesses now agree exactly, which is the acceptance criterion. RecvArmDeclined = 0,6,0,2,1,0,1,0,0,3,0,4. RecvDoubleArmed = 0, RecvCQEUnaccounted = 0, parseErr = 0 in all 12.
UNIT RIGS (engine/iouring, real ring + socketpair fixture), all pass, and all fail with the fix reverted (see negative_control):
Run -count=3 -race: ok github.com/goceleris/celeris/engine/iouring 1.050s
THROUGHPUT (no regression): framesSent at BP=8, origin/main 5694443 / 6041619 / 5607260 vs fix 5813545 / 5989302 / 5629086 / 5661513 / 5655266 — within run-to-run noise.
All container runs used: docker run --rm --cpus 4 --security-opt seccomp=unconfined --ulimit memlock=134217728:134217728 -v "$PWD":/src -w /src -v "$(go env GOMODCACHE)":/go/pkg/mod -v gocache484:/root/.cache/go-build golang:1.27 ... ; every io_uring run logged "workers=4" and "tier=high ... provided_buffers=true" (not a SKIP). Kernel 7.0.12-linuxkit. All tallies are from the RECVARM line and the --- PASS/FAIL lines, which the merged oracle prints AFTER settle().
Negative control
ORACLE ON origin/main (bef00fc), same container, same command, io_uring single-shot:
BP=8, 3 runs: RecvResumeWhileCancelPending = 11179, 10721, 9769 against RecvResumeWhileRecvInFlight = 0, 1, 0.
BP=16, 3 runs: RecvResumeWhileCancelPending = 5975, 6045, 5996 against RecvResumeWhileRecvInFlight = 1, 0, 0.
All 6 runs PASS with RecvDoubleArmed=0, RecvCQEUnaccounted=0, parseErr=0 — i.e. the over-count is the only thing wrong, exactly as the issue says. That is the ~10^4-vs-0/1 over-count the fix removes (0 vs 0 at BP=8, 1 vs 1 at BP=16).
KERNEL PROBE (scratch test, deleted before commit) establishing the real mechanism: the same missed cancel submitted WITH CQE_SKIP_SUCCESS produced 0 CQEs (2s wait, ETIME); submitted WITHOUT it, exactly 1 CQE with res=0 (not -ENOENT); a cancel that HIT produced res=1 plus the recv's -ECANCELED (-125).
SECOND-DEFECT CONTROL (fix partially reverted): with retireRecvCancel() reduced to
cs.recvCancelPending = 0(bool semantics) and everything else unchanged, TestMissedCancelDoesNotRetireASecondOutstandingCancel fails at "recvCancelPending=0 after the missed cancel's completion, want 1". With that intermediate assertion also disabled, the run proceeds into the actual harm and the stack shows handleRecv (worker.go:2044) -> closeConn -> finishCloseDetached on a healthy connection. Both patches were reverted afterwards and the tests re-run green.MEASURED HARM of the bool version at load: oracle BP=8 N=12 on the bool-valued intermediate build gave 11/12 PASS and 1 FAIL with parseErr=1 ("parse error on conn 66: unexpected EOF", clientCloseFail=28, serverRST=4). The counter version is 24/24 clean over the same BP=8 and BP=16 matrices.
Regression risk
Not verified
Adversarial review
Skeptic 1 — refuted: False, mergeable: True
I could not refute the branch. The named mechanism is addressed, and addressed at the right lines: the pause's ASYNC_CANCEL is submitted REPORTED (prepCancelUserDataReported, sqe.go) with its own conn-bound tag udRecvCancel, and handleRecvCancel retires the state when the cancel matched nothing. The issue's literal prescription (clear on the miss path in handleRecv's data branch) was deliberately not taken, with a sound reason recorded in code: it would not cover a pause placed while nothing was armed, and it would put a test on the per-request data path. The cancel's own completion covers every miss.
Invariant checks I ran myself rather than trusting the report:
(op == udRecv || op == udSend) && !cqeHasMore(...), so udRecvCancel cannot touch kernelInflight, closedOps/noteStaleTerminalOp, or recvArmed; the stale-path PushBuffer is gated on CQE_F_BUFFER, which a cancel CQE never carries. The "outside the accounting exactly as udProvide was" claim is true.git log -S"0x05 << 56"shows 0x05 was udPeek, deleted in 05429a7 well before v1.5.0; nothing live uses it, and udProvide's 0x06 is unchanged.Rig flipped with numbers: origin/main oracle gives 10-11k resumeWhileCancelPending against 0/1 resumeWhileRecvInFlight at BP=8 and ~6k at BP=16 over 6 runs; the fix gives 0/0 (BP=8, 12 runs) and 1/1 in the single run that reached the window (BP=16, 12 runs), with doubleArmed/cqeUnaccounted/parseErr zero throughout. The second defect has its own control (retireRecvCancel reduced to
= 0, everything else unchanged) plus a measured 1-in-12 oracle failure with io.ErrUnexpectedEOF on the bool-valued build.Claims consistent with the diff: I archived the branch commit to scratch and ran GOOS=linux go vet ./engine/... (clean, so it compiles with tests) and gofmt -l engine/ (empty). .golangci.yml enables errcheck/govet/staticcheck/revive/ineffassign/unused/misspell/unconvert/unparam/prealloc/nolintlint and NOT copyloopvar, so the
c := cin the new test does not contradict the 0-issues claim. Diff is 7 files, all under engine/; no fmt.Print/log/TODO, correct //go:build linux tag, no scratchpad or log files committed. No downstream consumer breaks: the loosened witness is only logged (never asserted) by the WS oracle, and probatorium references it nowhere.Hot-path cost: the per-CQE dispatch switch goes from 10 to 11 sparse cases (same binary-search depth) and the two changed tests are bool->
> 0on the -ECANCELED branch, not the data path. One extra CQE per WS backpressure pause only. Throughput measured within noise.Three defects found, all comment accuracy, none behavioural:
cs.recvCancelPending != 1against a bool), and with the type fixed it would fail earlier atn != 1(the miss produces zero CQEs there). The real control for that half was the oracle numbers.Residual risk the branch does not close, and the author says so: single host (Docker Desktop linuxkit 7.0.12, x86_64, 4 CPUs), no arm64, no cluster, no soak; -EALREADY reasoned from the kernel contract, never observed. Pre-existing and untouched: drainDetachQueue still sets cs.recvPaused = desired when GetSQE() returns nil, so an SQ-full pause is marked with no cancel submitted — the count simply stays 0 there, exactly as the bool stayed false, so this is not a regression.
Skeptic 2 — refuted: False, mergeable: True
I could not refute it. The rig flipped with numbers on both sides of the same apparatus: origin/main (bef00fc) RecvResumeWhileCancelPending = 11179/10721/9769 at BP=8 and 5975/6045/5996 at BP=16, against 0 in 12/12 and 0-or-1 in 12/12 on the branch, same container, same command, io_uring single-shot. The negative control is not a bare "it passes now": the fixed build still shows RecvArmDeclined 1-6 per run and framesSent within main's range, and every run logged workers=4 / tier=high, so the pause path is provably still being driven rather than silently skipped.
The unit rigs are real, not mocks. newRecvArmFixture (engine/iouring/recv_arming_window_test.go) builds a real Ring plus a socketpair and drives the actual drainDetachQueue pause/resume branches and staleConnCQE, and its submit() fails on a partial submit. The kernel claim that overturns the issue's stated mechanism is itself rig-backed inside the committed tests: TestResumeBeforeCancelLandsPlacesNoSecondRecv asserts a cancel that HIT reports res > 0 on a live ring, and TestMissedPauseCancelClearsCancelPending asserts the MISS posts exactly one CQE. So the issue's premise being wrong is measured, not argued.
Independent checks I ran read-only (git archive of the branch into scratchpad; no docker, nothing modified):
if sqe := w.ring.GetSQE(); sqe != nilblock, so an SQ-full drop cannot inflate the count (the known memlock/SQ-full hazard is neither introduced nor worsened).Invariant check: the fix explicitly preserves the one the surrounding comments state - a cancel that HIT must leave recvCancelPending set so handleRecv's -ECANCELED branch re-arms (the #484 fix that #560 guards) - and the merged test was extended to assert exactly that, so the obvious way to break #484 while "fixing" #596 is guarded. The bool-to-count change is the correct fix for the sibling-cancel case and is backed by a measured failure (1 parseErr in 12 at BP=8 on the bool intermediate) plus a deterministic rig.
Hot-path cost: verified by reading, not taken on trust. The only additions are one case in a dense tag switch, one retireRecvCancel() call replacing a field assignment in a branch already off the data path, and one extra CQE per WS backpressure pause. handleRecv's c.Res > 0 path is byte-for-byte untouched. connState.recvCancelPending going bool -> uint16 sits next to recvPaused bool and recvPauseDesired atomic.Bool, so alignment absorbs it.
Nothing is committed that must not be: the commit is 7 files, all under engine/, and a grep of the diff for fmt.Print / println / log.Print / scratchpad / /tmp/ / DEBUG / stray build tags / t.Skip returns nothing. The scratch kernel probe was deleted before the commit.
The deviation from the issue's prescribed fix ("clear it when the paused recv completes with data, and on the cancel's -ENOENT") is deliberate and better justified than the prescription: clearing in handleRecv's data path would be a per-request test and would still miss a pause placed while nothing was armed, and the -ENOENT half rests on a kernel behaviour the branch measured to be false on 7.0.12. That is a correction to the issue, not a failure to address it.
Residuals, none functional: (1) the comment at engine/iouring/recv_cancel_miss_test.go:25 claims the test body "runs against origin/main, where it fails on the recvCancelPending assertion" - it cannot, because main declares recvCancelPending as a bool and
cs.recvCancelPending != 1is a compile error there; the report's blanket "all fail with the fix reverted" is therefore true only for a partial revert that keeps the uint16, which is what negative_control item 3 actually did. The oracle is the real control for the primary defect and it is sound. (2) engine/iouring/worker.go:2959-2966 still says cancelConnOps' targeting "mirrors prepCancelUserDataSkipSuccess's WS-pause usage" (no longer true) and still says the cancel CQE is "tagged udProvide on failure (-ENOENT/-EALREADY)", which this branch's own probe falsifies for the ordinary miss - a comment the change invalidates and leaves standing in a file it edits. (3) retireRecvCancel retires exactly one per cancel while IORING_ASYNC_CANCEL_ALL can report res > 1; that is guarded only observationally by RecvDoubleArmed == 0, though it is strictly better than main. (4) the tight-witness POSITIVE case was reached on the fixed build in exactly 1 of 24 oracle runs, so the "the two witnesses agree exactly" criterion rests on n=1 at load - the deterministic unit rig compensates, which is the right compensating control.