Repository navigation
fix(eventloop): dispatch each epoll event to the registration it was collected for, so a closed conn's stale event never reaches the conn that took its number (celeris#842) - #933
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
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 Linux event loop now tags connection registrations with nonzero generations and checks those generations when dispatching epoll events. Pending flushes retain connection pointers. Linux tests cover descriptor reuse, generation wrap, and event delivery after epoll updates. ChangesLinux eventloop registration and dispatch
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The Linux event-loop change is mergeable after normal checks; no unresolved defect in the changed dispatch or pending-flush paths was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens isolation between reused connections without adding external access or privileges. Remaining uncertainty concerns extreme counter reuse and deployment conditions outside the inspected paths. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
beede94 to
399072a
Compare
…mment says what a wrap would take to misdeliver (celeris#842) Review round 2 of #933. worker.gen's comment said only that a generation is never 0. It now says why the 32-bit per-worker count can wrap without misdelivering: an event reaches the wrong conn only if the conn now registered on its number has the event's generation, which takes a multiple of 2^32-1 registrations on the worker between the two registrations while the event still waits to be dispatched. TestGenerationsWrapPastZero842 starts the worker's count just below the wrap and registers three conns: their generations are MaxUint32, 1 and 2, and each is served. No behaviour changes.
…mment says what a wrap would take to misdeliver (celeris#842) Review round 2 of #933. worker.gen's comment said only that a generation is never 0. It now says why the 32-bit per-worker count can wrap without misdelivering: an event reaches the wrong conn only if the conn now registered on its number has the event's generation, which takes a multiple of 2^32-1 registrations on the worker between the two registrations while the event still waits to be dispatched. TestGenerationsWrapPastZero842 starts the worker's count just below the wrap and registers three conns: their generations are MaxUint32, 1 and 2, and each is served. No behaviour changes.
aeeccf7 to
2feac79
Compare
…collected for, so a closed conn's stale event never reaches the conn that took its number (celeris#842) The standalone driver loop dispatched every event of an epoll_wait batch by the descriptor number it carried. An event collected for conn A, still in the batch when A was unregistered and closed and a new conn B registered A's number on the same worker, was applied to B: A's EPOLLRDHUP (A's server had hung up) tore B down, its onClose fired and its requests failed. Each registration now takes a per-worker generation (never 0) in RegisterConn, under w.mu, and every epoll_event of the registration carries it in Pad: the EPOLL_CTL_ADD, the two EPOLL_CTL_MODs in flushLocked and the WriteAndPoll* mask and re-arm (setEvents), all built by one helper so no MOD can drop it. The worker looks the conn up by number and drops the event unless the conn's generation is the event's. This is the shape #771 asks of the epoll engine's driver dispatch. The rest of the worker's number-keyed paths act on conns too: EPOLLOUT goes to the conn the event names, and the pending-flush list holds conns, not numbers, so a flush queued for a conn that has gone cannot reach a conn that took its number.
…mment says what a wrap would take to misdeliver (celeris#842) Review round 2 of #933. worker.gen's comment said only that a generation is never 0. It now says why the 32-bit per-worker count can wrap without misdelivering: an event reaches the wrong conn only if the conn now registered on its number has the event's generation, which takes a multiple of 2^32-1 registrations on the worker between the two registrations while the event still waits to be dispatched. TestGenerationsWrapPastZero842 starts the worker's count just below the wrap and registers three conns: their generations are MaxUint32, 1 and 2, and each is served. No behaviour changes.
2feac79 to
08e14ab
Compare
Lane D1, PR 2 of 3, stacked on #932 (#862). It targets
mainso that CI runs, and it carries #862's commit. Review only this PR's two commits: 8195b6b (was 399072a; rounds 0 and 1, reviewed) and 2feac79 (was aeeccf7; review round 2: a comment and a test, no behaviour change). #934 (#881) is stacked on this one.Rebased onto #932's new head 92f2913 (2026-10-05)
This PR's head is now 2feac79 (commits 8195b6b and 2feac79), rebased with
git rebase --onto 92f2913 dce5a3cfrom aeeccf7 (commits 399072a and aeeccf7). #932 is now on main 8476e8b. The rebase applied with no conflicts, and each commit'sgit patch-id --stableis unchanged. None of main's 14 commits since df31adc touchesdriver/internal/eventlooporinternal/wakefd, and this PR changes no exported API. The controls below ran at aeeccf7. They were all re-run at 2feac79 (linux/arm64 golang:1.27 container,-race, one process per observation), and every arm's tally matches round 2's: ff-main 10/10/0 (PASS/FAIL/SKIP), ff-parent 10/10/0, fix 30/0/0, nc 10/10/0, mut-NOGEN 20/10/0, mut-MADD 0/30/0, mut-MARM 20/10/0, mut-MDISARM 20/10/0, mut-MSETEV 20/10/0, mut-NOZEROSKIP 20/10/0. All have 0 race reports. Whole suites at 2feac79: the eventloop package 32/0/0, and./driver/... ./internal/wakefd/474/0/0, both with 0 race reports (467 before; the 7 added are main's #859 tests). Scripts:bash evidence/lanes-20261003/D1/fix-r4/scripts/trees.sh 8476e8b 92f2913 2feac79, thenbash evidence/lanes-20261003/D1/fix-r4/scripts/controls.sh 842 20261005T0208Z. Logs:evidence/lanes-20261003/D1/fix-r4/logs/842-20261005T0208Z/. The timing A/B under Cost ran at aeeccf7 and was not re-run, because the rebase changed no file this PR touches. #934 has not been rebased yet. It still carries the old commits (399072a, aeeccf7).Defect
The standalone driver event loop (
driver/internal/eventloop) dispatched every event of anepoll_waitbatch by the descriptor number the event carried. An event collected for conn A could still be waiting in the batch when A was unregistered and closed, and a new conn B registered A's number on the same worker. The event was then applied to B.When A's event carried
EPOLLRDHUP,EPOLLHUPorEPOLLERR(A's server had hung up), it tore B down: B'sonClosefired, B left the map and the epoll set, andWrite(B)returnedfile descriptor not registered. B's socket was healthy. #843 (#784) stopped a reader that is inside A's read loop when A is unregistered. It did not cover an event dispatched after the unregister, which is this case.Mechanism (line numbers at 56a6c1e;
driver/internal/eventloopis byte-identical at 93bd88f, at df31adc, where the controls ran, and at 28383e8, current main)worker.run(loop_linux.go:1088-1121) takesfd := int(ev.Fd)at:1102and callshandleReadable(fd, ev.Events)at:1113andhandleWritable(fd)at:1116.handleReadable(:1125-1170) looks the conn up by that number at:1127. By then the number can be B's. It reads B, then applies A's flags:errorClose(c, nil)at:1167-1168.:311-314), the two MODs influshLocked(:438-441,:461-464) andsetEvents's MOD (:165-168) all carry onlyFd.pending []int,:55;enqueueFlush:474;drainOnelooks it up by number at:1194).Fix
RegisterConn, underw.mu, never 0, before the conn is published. Everyepoll_eventof the registration carries it inPad: theEPOLL_CTL_ADD, both MODs influshLocked, andsetEvents's MOD (theWriteAndPoll*mask and re-arm). All of them go through one helper,epollEvent, so no MOD can drop the generation; a MOD replaces the whole event data.Padexists on every linux GOARCH in x/sys v0.48.0.dispatch(ev)looks the conn up by number and drops the event unless the conn's generation is the event's. This is the shape epoll: a driver event harvested before UnregisterConn is dispatched by number after it, and closes the conn registered on the reused number (the epoll twin of #707) #771 asks of the epoll engine's driver dispatch.drainOne(c)). The pending-flush list holds conns, so a flush queued for a conn that has gone cannot reach the conn that took its number. That one was benign (it flushed B's own bytes), but it was a number-keyed table.worker.gen's comment says so (review round 2).testHookDroppedEvent, runs on the drop path. It is a witness for the test, and costs nothing on the delivery path.handleReadable(fd, …)now go throughdispatchwith the conn's event (read_critical_section_784_linux_test.go, andread_cost_784_bench_linux_test.go'sBenchmarkHandleReadable784).Tests and controls
Two tests are added in
stale_event_842_linux_test.go:TestStaleEventSparesTheConnThatTakesTheNumber842is deterministic and drives the real worker.onRecvwhile P and then A become ready; A's peer shuts its write side, so A's event carries EPOLLRDHUP.epoll_waitreturns P's event and then A's, and P parks the worker again.dup3s a new socket onto A's number and registers it as B on the same worker, then releases P.TestEventsReachTheConnAfterEveryEpollCtlMod842drives each MOD and then needs the conn's next event: the EPOLLOUT arm (the rest of a 1 MiB flush is sent only on EPOLLOUT), the disarm (next EPOLLIN), and eachWriteAndPoll*mask and re-arm (next EPOLLIN).Review round 2 adds
TestGenerationsWrapPastZero842, ingeneration_wrap_842_linux_test.go. It starts the worker's count just below the wrap and registers three conns, which must get generations MaxUint32, 1 and 2, and each must be served. It needs the generation, so the failing-first arms do not run it, and the NC moves it out. Mutant NOZEROSKIP (the count wraps to 0) is its second control.Each arm runs 10 separate processes (
-race, linux/arm64, golang:1.27).fix-r2/logs/842-r2/ff-main.logfix-r2/logs/842-r2/ff-parent.logfix-r2/logs/842-r2/fix.logfix-r2/logs/842-r2/nc.logfix-r2/logs/842-r2/mut-NOGEN.logfix-r2/logs/842-r2/mut-MADD.logfix-r2/logs/842-r2/mut-MARM.logfix-r2/logs/842-r2/mut-MDISARM.logfix-r2/logs/842-r2/mut-MSETEV.logfix-r2/logs/842-r2/mut-NOZEROSKIP.logfix-r2/logs/842-r2/parent-pkg.log)fix-r2/logs/842-r2/fix-pkg.log)fix-r2/logs/842-r2/fix-drivers.log)Scripts:
bash evidence/lanes-20261003/D1/fix-r2/scripts/trees.sh df31adc dce5a3c aeeccf7 d03bc69 a21d0ff(the export trees), thenbash evidence/lanes-20261003/D1/fix-r2/scripts/controls.sh 842 r2. Table:python3 evidence/lanes-20261003/D1/fix-r2/scripts/table.py 842 evidence/lanes-20261003/D1/fix-r2/logs/842-r2.Main moved during round 2. #935 (#859) merged into main as 28383e8 while this round ran. It changes
driver/postgresand adds close-after-teardown tests to the three drivers; none of the commits since the base touchesdriver/internal/eventlooporinternal/wakefd. The stack is behind main, and it merges with it cleanly (git merge-tree --write-tree 28383e8 a21d0ff). The merged tree passes with-race: the eventloop package 39/0/0 (PASS/FAIL/SKIP) with 0 race reports, and./driver/... ./internal/wakefd/481/0/0 with 0 race reports, including #935's 7 #859 tests (evidence/lanes-20261003/D1/fix-r2/logs/merged-28383e8-a21d0ff/; scriptbash evidence/lanes-20261003/D1/fix-r2/scripts/merged-suite.sh 28383e8 a21d0ff).Cost
Measured: no cost resolved. Measured 2026-10-05, 01:32Z to 01:57Z, with
bash evidence/lanes-20261003/D1/fix-r1/scripts/bench.sh 10 10 df31adc dce5a3c aeeccf7 a21d0ff(the fix-r1 copy of the command, which differs fromfix-r2/scripts/bench.shonly in its output directory; dce5a3c, aeeccf7 and a21d0ff are the current heads of #932, #933 and #934). It ran under the laptop TIMING lock, in one linux/arm64 golang:1.27 container (--cpus 4): 10 rounds, one process per arm per round, the arm order rotated each round,-benchtime 1s, and an A/A arm (df31adc's test binary run a second time as its own arm). benchstat: median ± 95% CI, n=10 per arm,~= not significant at α=0.05. Host: no game client ran. 12 of the 25 one-minute samples during the run were NOT-QUIET by the script's rule, each because of one desktop process (Orca Helper, 49% to 58% of one of the 8 cores); the other 13 were quiet. The A/A floor: on the micro benchmarks the A/A arm differs from df31adc by 1.2% to 1.4% in 2 of 5 rows at p=0.023 and p=0.029, so a shift under about 1.5% there is not resolved.BenchmarkReadWhileAnotherConnFlushes784's sec/op CIs are ±13% to ±384% per arm, and its gapped rows are bimodal in every arm, the A/A arm included (round trips cluster near 20 µs and near 26 µs), so only large shifts there are resolved. Output:evidence/lanes-20261003/D1/fix-r1/bench/20261005T013145Z/(the per-arm*.txt,run.logwith the test binaries' sha256,quiet-during.log,benchstat/). Analysis:evidence/lanes-20261003/D1/fix-r3/scripts/analyse.sh(benchstat) andfix-r3/scripts/tables.py(these tables).#933 against df31adc, and against #932, its parent. Each cell is the median ± CI. Each delta in the first three tables is against df31adc.
HandleReadable784WriteAndPoll784/WriteAndPollWriteAndPoll784/WriteAndPollBusyWriteAndPoll784/WriteAndPollMultiWritePending862RegisterChurn862/churn=0RegisterChurn862/churn=1RegisterChurn862/churn=4ReadWhileAnotherConnFlushes784/chunk=0/gap=0sReadWhileAnotherConnFlushes784/chunk=4096/gap=0sReadWhileAnotherConnFlushes784/chunk=65536/gap=0sReadWhileAnotherConnFlushes784/chunk=524288/gap=0sReadWhileAnotherConnFlushes784/chunk=262144/gap=1msReadWhileAnotherConnFlushes784/chunk=1048576/gap=5msHandleReadable784WriteAndPoll784/WriteAndPollWriteAndPoll784/WriteAndPollBusyWriteAndPoll784/WriteAndPollMultiWritePending862RegisterChurn862/churn=1RegisterChurn862/churn=4RegisterChurn862/churn=1RegisterChurn862/churn=4ReadWhileAnotherConnFlushes784/chunk=0/gap=0sReadWhileAnotherConnFlushes784/chunk=4096/gap=0sReadWhileAnotherConnFlushes784/chunk=65536/gap=0sReadWhileAnotherConnFlushes784/chunk=524288/gap=0sReadWhileAnotherConnFlushes784/chunk=262144/gap=1msReadWhileAnotherConnFlushes784/chunk=1048576/gap=5msReadWhileAnotherConnFlushes784/chunk=262144/gap=1msReadWhileAnotherConnFlushes784/chunk=1048576/gap=5msReadWhileAnotherConnFlushes784/chunk=262144/gap=1msReadWhileAnotherConnFlushes784/chunk=1048576/gap=5msThe first three tables are against df31adc. On the micro rows, which hold the generation store on each ADD/MOD and the generation compare on each dispatch, the one significant row is
WriteAndPollBusy, −1.25% (p=0.045). That is inside the A/A floor and is not a saving. The fourth table is against the parent, #932. In the 10-round run, the two gapped rows were +12.5% (p=0.029) and +22.1% (p=0.011) against #932, though not against df31adc (p=0.796 and p=0.481). In that run #933 drew the slow (≈26 µs) mode in 7 of 10 rounds and #932 in 2 of 10. A follow-up re-ran the two gapped rows for 20 more rounds with the same binaries, under the TIMING lock (01:58Z to 02:02Z; 3 of 7 30-second samples NOT-QUIET, the same Orca Helper process):bash evidence/lanes-20261003/D1/fix-r3/scripts/gap-followup.sh 20, outputevidence/lanes-20261003/D1/fix-r3/bench/gap-20261005T015850Z/. The last two tables are the follow-up. With n=20, #933 is −11% and −6% against #932 (p=0.815, p=0.588), and the A/A arm is as far from df31adc as #933 is. So the run-1 shift was the bimodality and not this PR.What changes on the measured paths, without numbers: each ADD and MOD fills one more 4-byte field of a stack
EpollEvent, and each dispatched event compares one moreuint32. The pending-flush list stores pointers instead of ints and drops one map lookup per entry.Family audit
These are the fd-number-keyed tables and dispatches in
driver/internaland the three drivers:run→handleReadable(fd), by numberdispatch, generation-checkedrun→handleWritable(fd)→drainOne(fd), by numberpending []int→drainOne(fd), by numberWriteAndPoll*'spoll(2)of the numberUnregisterConn's doc)loop_other.go(the non-Linux fallback; it builds on darwin, and the drivers do not build on windows at all: #937)net.FileConndup; the map entry is removed only if it is still the reader's conndriver/redis,driver/memcachedClose, afterUnregisterConnWrite,WriteAndPoll*,UnregisterConn, by numberPubSub.Closegoes by number to the conn that took the closed descriptor), and #859 tracked it for postgres (next row; fixed by #935)driver/postgresCloseacted on the number afteronClosehad closed the fd. Fixed on main by #935 (28383e8), which merged during this round (see Tests and controls)engine/epoll/loop.go:629)Review round 1
Both reviews found no defect in this commit. It is unchanged except for its rebase onto dce5a3c (#862's round-1 commit removed
w.mu's read lock fromRegisterConn, next to the ADD this commit changes;git range-diffshows only those context lines changed). Every control was re-run at 399072a. The cost (MINOR, perf/API/security) is under Cost.Review round 2
The correctness review approved. The perf/API/security review's MAJOR is #934's measurement, and its MINOR here is this PR's cost (see Cost).
worker.gen's comment now says what a wrap would take (see Fix).TestGenerationsWrapPastZero842pins the wrap, with mutant NOZEROSKIP as its second control. Every control above was re-run at aeeccf7.loop_other.gorow said "darwin/windows", but./driver/...does not build on windows. Filed as drivers: redis, postgres, memcached and the nine middleware stores built on them do not compile for GOOS=windows, which the README offers (unix.Dup in the fallback loop, int descriptors in each conn.go) #937 (pre-existing since v1.4.0), and the row now says so.Fixes #842