fix(iouring): leave a claimed async conn to its claim when a reap is retried or lands (celeris#758) - #765
Conversation
…nn to its claim (celeris#758) rerunHandOff (a reap's retry, or its landing) read only asyncRun, which a dispatch goroutine clears when it claims its own hand-off. Between the claim and its drain the worker handed the conn off, and the claim's drain then counted TransplantDoubleClaim for a conn moved once. Two orders: the retry running between the claim's unlock and its enqueue (measured on main 698bed6, CI run 36341302731), and a reap landing between the enqueue and the drain. Fails on this tree; the fix follows.
…retried or lands (celeris#758) rerunHandOff re-runs the hand-off for a promoted async conn whenever its dispatch goroutine is not running, and read only asyncRun for that. A goroutine that has claimed its own hand-off has cleared asyncRun too, and it enqueues the claim only after releasing asyncInMu. A reap retry owed from an earlier miss could run in between: its reap landed, the conn was handed off with its claim still set, and the claim's drain then found the slot empty and counted TransplantDoubleClaim, a must-stay-0 counter, for a conn moved once (main 698bed6, CI run 36341302731). rerunHandOff now applies the one-owner rule tryTransplant already applies (celeris#657 A6): a claimed conn is left to its claim, whose drain runs finishAsyncTransplant itself, and the deferral is counted as TransplantClaimDeferred, a rate. No lock is added: the claim is read under the asyncInMu hold that already reads asyncRun.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe io_uring hand-off retry path now defers when an async claim is pending. Tests cover reap retries and reap landings before claim drain, and verify one hand-off with no double claim. ChangesAsync hand-off claim deferral
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable issue remains from the supplied evidence; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The hand-off change defers to an existing connection claim rather than creating a new entrypoint or privilege path. The reviewed interleavings support a single hand-off, but they do not cover every interruption or deployment condition. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Round 2. The review found nothing blocking, so the head is unchanged at d030f16 and CI remains 17/17 green. The findings were handled as follows:
Reproduce materialThe injection patch and the three tally scripts are below. The drivers that ran the containers and applied these scripts to the logs ( The 2 ms claim-window injection (NOT for merge): a new file, plus one line in
|
…t, or upgraded it to h2c, to its queued exit (celeris#780) (#799) rerunHandOff (a reap retry or a reap landing) now also leaves an async conn alone when its dispatch goroutine has set asyncClosed or made it H2C, then cleared asyncRun, but has not yet enqueued its exit. tryTransplant's async branch also returns when asyncClosed is set; its protocol gate already refuses an H2C conn. Before this, the queued close could close the number the hand-off had given up, which the next accept can hold, and an H2C conn could reach the HTTP/1 epoll target. New tests: TestReapRerunLeavesAnExitingDispatchToItsExit and TestTryTransplantLeavesAClosingAsyncConnAlone. Each fails on the parent and passes with the fix. The TransplantClaimDeferred comment on TestSweepDoesNotReClaimAnAsyncHandoff now gives #765's meaning. Follow-ups: #816. Fixes #780
Fixes #758
What this fixes
TestHandoffHasNothingInFlight/asyncfailed once on main 698bed6 withTransplantDoubleClaim = 1(CI run 36341302731, Unit job 108681805572). That counter must stay 0: the adaptive flap test and probatorium's validation checker both treat a nonzero value as a defect. #758 has the mechanism and the evidence. In short:rerunHandOffre-runs the io_uring→epoll hand-off when a reap is retried (retryReaps) or lands (reapOutcome). For a promoted async conn it calledfinishAsyncTransplantwheneverasyncRunread false.asyncRunas well.runAsyncHandlersetstransplantPending=trueandasyncRun=falseunderasyncInMu, and callsenqueueDetachonly after unlocking.Nothing moved twice. The slot check refused the claim's attempt, so the conn was handed off once. In the failing CI iteration, all 128 conns were handed off and the hand-off count was 128 (
detached=128). The same holds in every iteration of the runs below that had no client errors. The defect is a must-stay-0 counter that counted an ordering.The pattern is from #681 (4770d07). It is not a regression from the merges on 2026-09-27.
The change
tryTransplantalready leaves a claimed conn to its claim (celeris#657 A6).rerunHandOffnow applies the same rule. When the claim is set, it countsTransplantClaimDeferred(a rate) and returns. The claim's drain then runsfinishAsyncTransplantitself: it places the reap, and the hand-off happens at that reap's-ECANCELED.Commits, on main 698bed6:
566b82fadds two subtests toTestOneOwnerPerHandoff, one per call site ofrerunHandOff. They use the existingfdlFixtureand are judged by outcome (exactly one hand-off, no double claim), so they run unchanged on both trees. The subtests:reap_retry_between_claim_and_enqueue: the order CI hit. A retry is owed, the claim is published but not queued, the retry drain runs, and the claim is enqueued and drained.reap_lands_between_claim_and_drain: a reap placed while the previous claim was being finished lands after the next claim is queued, before the drain. That needs a recv that outlives the request which respawned the goroutine: a multishot recv,CELERIS_IOURING_MULTISHOT_RECV=1.d030f16is the fix. It also updates the doc comments ofrerunHandOffandTransplantClaimDeferred, the latter inengine.EngineMetricsand inhandoffLossStats.No lock is added or reordered. The claim is read under the same
asyncInMuhold that already readsasyncRun, andnoteClaimDeferredis an atomic add after the unlock. The non-test diff adds or removes noLock/Unlock/Waitcall.This is not on the per-request path.
rerunHandOffis reached only fromreapOutcome, which runs only whencs.transplantReap > 0, and fromretryReaps, which runs only whenlen(w.reapRetry) > 0. A reap or a reap retry exists only while a drain is set. That is why the PR has no benchmark.TestNoDrainSQESequenceIsUnchangedstill pins the SQE sequence with no drain set.TransplantClaimDeferrednow also counts retries that land in a claim window. No gate reads it: probatorium only records it (engine_transplant_claim_deferred).Failing-first, fix, negative control
These are the deterministic subtests. Each row is one Docker container: linux/arm64, kernel 7.0.12-linuxkit, go1.27.1,
--cpuset-cpus 0-3, memlock 8 MiB (one io_uring worker, as on the CI runner),seccomp=unconfined,go test -race -v -run '^TestOneOwnerPerHandoff$'. Verdicts come from--- PASS/FAILlines only.reap_retry_between_claim_and_enqueuereap_lands_between_claim_and_drain566b82f: tests only (engine as on main)TransplantDoubleClaim = 1,ClaimDeferred = 0; the retry placed 1 reapTransplantDoubleClaim = 1,ClaimDeferred = 0d030f16: headClaimDeferred = 1ClaimDeferred = 2fd_lifetime.gorestored from main bycp566b82f566b82fIn the landing subtest,
ClaimDeferredis 2 because two attempts find the claim set. One is the landing's own re-run inreapOutcome. The other is thetryTransplantthat follows every recv completion while a drain is set. So the exact pin also depends on thattryTransplantcall (#780, item 3).Second control: the engine under load, with the window widened
The test is the unchanged
TestHandoffHasNothingInFlight/async: 128 keep-alive clients, then a drain. It ran against the real engine, with one line added to both trees. That line sleeps 2 ms between the claim'sasyncInMu.Unlock()and itsenqueueDetach(), and it is not part of this PR:cs.asyncInMu.Unlock() + dcClaimGap() // time.Sleep(CELERIS_DC_GAP_US), 2000 here w.enqueueDetach(cs)Each arm is one
-race -v -count=Nprocess, with memlock 8 MiB. Verdicts are the async arm's--- PASS/FAILlines. "Counter" is the number of iterations in whichTransplantDoubleClaimwas above 0:--cpuset-cpus 0-3(nproc 4), 50--cpus 4(nproc 8, the #758 triage's shape), 80--cpus 4, 150--cpus 4arms give 14/230 vs 3/230, p = 0.0056.worker.goline,--cpus 4, memlock 8 MiB) from prebuilt-racebinaries, with the two trees interleaved in 4 rounds of 40 iterations.io_uring_setuphit ENOMEM at memlock 8 MiB, so it has no load line.ClaimDeferredwas 188 on main and 5,230 on the head.TransplantClaimDeferredtotals 164 on main and 5,967 on the head.reapOutcomeand re-arms the recv, and the claim's drain reaps it again. That path counts 2 reaps for 1 hand-off, and a review probe that builds it readsreaps=2 misses=0, one hand-off. In general, reaps − misses ≥ hand-offs.Client errors in both trees. Both 150-iteration arms had client read timeouts:
In all three head failures,
claimdeferred=0anddoubleclaim=0. The new branch never ran in them, sorerunHandOffbehaved exactly as on main. Main's iteration 78 has the same signature: 89 timeouts and 39 of 128 conns handed off. The head's iterations 90 and 91 handed off 31 and 0 of 128.Each event happened during a run of about 5 consecutive iterations. Throughput fell to about a third, then recovered. Head iteration 91 served 1 request in total, so the engine was not serving even during the 300 ms warm-up. The warm-up runs before any drain, and the changed code cannot run then. Another lane's container was running on the same laptop VM. Host contention fits, but it is not proven.
None of these runs had client errors: the 50- and 80-iteration arms, the multishot arms, the review's interleaved re-run, and the 3,200 async iterations of the two GitHub runs below.
Misses. Reap misses are a rate, and the head's differ from main's: 9,552 vs 6,496 over the sleep arms, 1,624 vs 1,008 in the GitHub stress, but 1,045 vs 1,679 in the nproc-4 arm. One explanation, not measured: a deferred retry's reap is now placed at the claim's drain instead of inside the window, so the client's next request beats it more often. A miss means that request is served on io_uring and the hand-off is retried, as designed.
TransplantReapFailed,TransplantHoldRescuedandTransplantDoubleClaimstay 0 throughout.The other order: multishot recv, observed or not
CELERIS_IOURING_MULTISHOT_RECV=1, with no sleep, memlock unlimited, 30 iterations per tree. The worker printed that multishot recv was on in every engine. Results: 0/30 with a double claim on main and 0/30 on the head. Misses were 0 on both, since a multishot recv stays armed until the reap. So the landing order was not observed, andreap_lands_between_claim_and_drainbuilds it rather than reproducing a measurement.Whole suite, static checks, CI
./engine/iouring,-race -count=1 -v, on the head:--cpuset-cpus 0-3, memlock 8 MiB): 320 PASS, 0 FAIL, 5 SKIP, in 156.6 s. The 5 SKIPs are exactly the five the CI step allows.SynackRetriesZeropair, which is also in CI's list.DATA RACE.GOOS=linuxfor both amd64 and arm64:go build ./...,go test -cof./engine/iouring,./adaptiveand./engine/epoll, andgo vet ./...all pass.golangci-lintv2.13.2 (CI pins v2.13) reports 0 issues.gofmtis clean.driver/postgresTestStreamingLarge(heap grew 11.4 MB against a 10 MB budget). That test binary does not containengine/iouring(go list -test -deps), and the job passed on re-run. No earlier failure of that test turned up in 368 Unit/Coverage logs from 2026-09-13 to 2026-09-27.GitHub-hosted stress (celeris-stress, target=github)
The inputs are the #758 triage's main arm exactly:
^TestHandoffHasNothingInFlight$in./engine/iouring,-race, memlock 8m, 4 shards per arch ×-count=200, probatorium f1acc11.RULE 14: this comparison cannot discriminate. The head has 0 failures, and main also had 0.
-count=200run falls after its first iterations, as the triage showed.What the stress run does show:
ClaimDeferredis 478 on the head against 128 on main.The discriminating evidence is the deterministic subtests, with their negative control, and the injected engine runs above.
Not in this PR
The dispatch goroutine has other exits with the same unlock-then-enqueue window: the panic exit, the h2c-upgrade exit, and the
processErrexits.rerunHandOffcan still hand off a conn in them. After that, the exit's queued entry can close a reused fd number, or register an h2c conn that has already been handed off. This is pre-existing and identical on main 698bed6, and this PR does not change it. It is #780, with a probe and a suggested guard.Reproduce
Every number above comes from logs tallied by scripts, not by hand. The container is
golang:1.27:docker run --rm --cpuset-cpus 0-3 --ulimit memlock=8388608 --security-opt seccomp=unconfined -v <tree>:/src:ro -w /src golang:1.27 go test -race -v …. The other shapes use--cpus 4or memlock-1, and the sleep arms add-e CELERIS_DC_GAP_US=2000.This comment attaches:
c2_claimgap.goand theworker.goline)tally.pycounts theceleris657 loadcounters per arm, together with the--- PASS/FAIL/SKIPlines and the Fisher p.invariant.pychecks reaps − misses = hand-offs per iteration.verdicts.pygives each iteration's verdict, with the counters of every FAIL.Local only. These files hard-code laptop paths and are not attached:
run2.sh,seq-*.sh,tally-all.shandround2.shtotals.pysums the counters, andall128.shchecks that every error-free iteration handed off all 128 connsThe laptop logs are local too. The stress logs are the artifacts of probatorium runs 36344368270 and 36349377290.