fix(eventloop): a Write or RegisterConn racing Loop.Close no longer uses the worker's eventfd or epoll fd number after shutdown closed it (celeris#862) - #932
Merged
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (4)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
FumingPower3925
added a commit
that referenced
this pull request
Oct 3, 2026
…d a RegisterConn racing UnregisterConn is tested (celeris#862) Review round 1 of #932. RegisterConn held w.mu's read lock across its EPOLL_CTL_ADD. A queued writer of w.mu (a forget, a registration, shutdown) then made the worker's lookups wait for the syscall, because a sync.RWMutex holds new readers back once a writer waits. The read lock is not needed: c is in the map, a conn leaves the map only once it is marked closed, and shutdown marks every conn in the map closed, under its c.mu, before it closes the epoll fd. So c.mu and the closed check alone keep the ADD off a closed or reused epoll fd number, the rule flushLocked already relies on. TestRegisterConnRacingUnregisterConnLeavesNoEpollEntry862 covers the other half of that check: an UnregisterConn of the same fd that runs in RegisterConn's window must leave fd out of the epoll set. A check of the epoll fd instead of c.closed passes both earlier tests and fails this one. The register-vs-Close test now fails, rather than skipping its main check, when no epoll instance takes the closed epoll fd's number, and the wake test's comment says that its coverage rests on -race.
FumingPower3925
force-pushed
the
fix/celeris-862-eventloop-wake-close-race
branch
from
October 3, 2026 13:43
ec5095c to
dce5a3c
Compare
This was referenced Oct 3, 2026
…ses the worker's eventfd or epoll fd number after shutdown closed it (celeris#862) The standalone driver loop's worker published its wakeup eventfd's number in a plain field. A Write that left bytes pending read it with no lock (wake, via enqueueFlush, after c.mu is released) while shutdown closed the eventfd and stored -1, so a Write running alongside Loop.Close was a data race, and could write 8 bytes to the number after the close, into whatever had taken it. The worker now holds the eventfd through internal/wakefd.WakeFD, the handle the engines adopted for the same defect (celeris#655, #666): Signal and Close share a lock, so a wake either completes before the close or writes nothing. RegisterConn had the same shape for the other descriptor a caller reaches: it read epollFD under w.mu, released w.mu, and issued EPOLL_CTL_ADD after. A shutdown in between closed the epoll fd, so the ADD went to the epoll instance that had taken the number, or to a closed number (RegisterConn then returned an epoll_ctl error for a conn whose onClose had already fired). The ADD is now issued under w.mu's read lock and c.mu, after a check that the conn has not been torn down, like every other epoll_ctl of a conn; shutdown marks every conn closed and closes the epoll fd under the write lock. A read lock, so a registration does not hold up the worker's lookups. A conn whose ADD fails is marked closed before it leaves the map. BenchmarkWritePending862 (the wake path) and BenchmarkRegisterChurn862 (a conn's round trip while other goroutines register and unregister on its worker) measure the cost; both run on the base too.
…d a RegisterConn racing UnregisterConn is tested (celeris#862) Review round 1 of #932. RegisterConn held w.mu's read lock across its EPOLL_CTL_ADD. A queued writer of w.mu (a forget, a registration, shutdown) then made the worker's lookups wait for the syscall, because a sync.RWMutex holds new readers back once a writer waits. The read lock is not needed: c is in the map, a conn leaves the map only once it is marked closed, and shutdown marks every conn in the map closed, under its c.mu, before it closes the epoll fd. So c.mu and the closed check alone keep the ADD off a closed or reused epoll fd number, the rule flushLocked already relies on. TestRegisterConnRacingUnregisterConnLeavesNoEpollEntry862 covers the other half of that check: an UnregisterConn of the same fd that runs in RegisterConn's window must leave fd out of the epoll set. A check of the epoll fd instead of c.closed passes both earlier tests and fails this one. The register-vs-Close test now fails, rather than skipping its main check, when no epoll instance takes the closed epoll fd's number, and the wake test's comment says that its coverage rests on -race.
FumingPower3925
force-pushed
the
fix/celeris-862-eventloop-wake-close-race
branch
from
October 5, 2026 02:07
dce5a3c to
92f2913
Compare
FumingPower3925
marked this pull request as ready for review
October 5, 2026 02:22
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.
Lane D1, PR 1 of 3. The three PRs all edit
driver/internal/eventloop/loop_linux.goand are stacked in this order: this one (#862), then #933 (#842, which carries this commit), then #934 (#881, which carries both). Each targetsmainso that CI runs on it. In the later two, review only the last commit.Rebased onto main 8476e8b (2026-10-05)
This PR's head is now 92f2913 (commits 1807309 and 92f2913), rebased with
git rebase --onto 8476e8b df31adcfrom dce5a3c (commits 6c25e65 and dce5a3c). 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 (it touches onlydriver/internal/eventloop). The controls below ran at dce5a3c on df31adc. They were all re-run at 92f2913 on 8476e8b (linux/arm64 golang:1.27 container,-race, one process per observation), and every arm's tally matches round 1's: ff-main 0/60/0 (PASS/FAIL/SKIP over the three tests × 20 processes; 20 race reports), ff-main-hooked 0/60/0 (20), fix 60/0/0 (0), nc 0/60/0 (20), mut-MREG 20/40/0, mut-MREG2 20/40/0, mut-MWAKE 40/20/0 (20), mut-EPFDCHECK 40/20/0. Whole suites at 92f2913: the eventloop package 29/0/0, and./driver/... ./internal/wakefd/471/0/0, both with 0 race reports (464 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 862 20261005T0208Z. Logs:evidence/lanes-20261003/D1/fix-r4/logs/862-20261005T0208Z/. The timing A/B under Cost ran at dce5a3c and was not re-run, because the rebase changed no file this PR touches.Defect
The standalone driver event loop (
driver/internal/eventloop) is what the redis, memcached and postgres drivers use when they run without an engine. Its worker owns two descriptors that callers on other goroutines use: the wakeup eventfd and the epoll fd.Loop.Closecloses both. Two sites used one of those numbers outside the lock that shutdown closes it under:wake(the defect filed in eventloop: a Write racing Loop.Close can write the closed eventfd's number in wake(), which another socket may hold by then (data race) #862).wakereadw.eventFDwith no lock, then wrote 8 bytes to it. Meanwhileshutdownclosed the eventfd and stored -1 underw.mu. AWritethat leaves bytes pending wakes the worker after it has releasedc.mu, and it checksw.closedonly on entry. So aWriterunning alongsideLoop.Closewas a data race. It could also write01 00 00 00 00 00 00 00to the number after the close, into whatever had taken the number by then: a socket, a driver conn, or another eventfd.RegisterConn(same family, found in the audit). It readepollFDunderw.mu, releasedw.mu, and only then issuedEPOLL_CTL_ADD. A shutdown in between closed the epoll fd, and the ADD then had two possible outcomes:RegisterConnreturned anepoll_ctlerror for a conn whoseonClose(ErrLoopClosed)had already fired.Mechanism (base df31adc)
Line numbers are at 56a6c1e, where round 0's controls ran.
driver/internal/eventloopandinternal/wakefdare byte-identical at 93bd88f, where the triage probe failed, at df31adc, where round 1's controls below ran, and at 28383e8 (current main).wake,driver/internal/eventloop/loop_linux.go:276-283: the unlocked check is at:277and the write at:282.shutdown,:235-273: it takesw.mu.Lockat:245, closes the eventfd at:255and stores -1 at:258.enqueueFlushcallswakeat:476. It is reached afterc.muis released from these callers:Writeat:417. Itsw.closedcheck runs only on entry, at:378.WriteAndPollat:574.WriteAndPollBusyat:764.WriteAndPollMultiat:960.Loop.Close(loop.go:74-95) wakes the workers at:83and joins only the worker goroutines at:86(l.wg.Wait). It then runsshutdownat:90.RegisterConn,:290-321:epfd := w.epollFDis read underw.muat:308.w.muis released at:309, and the ADD onepfdruns at:311.Fix
internal/wakefd.WakeFD. This is the handle fix(engine): own the wakeup eventfd so no producer can write it after shutdown closed it (celeris#655) #666 introduced for the engines' twin of this defect (epoll/io_uring: a transplant can write the wakeup eventfd after shutdown closed it, into whatever reused that descriptor number (found by reading, not reproduced) #655).SignalandClosetake the same lock, so a wake either completes before the close or writes nothing.wakeis noww.wakeFD.Signal(), andshutdowncallsw.wakeFD.Close()underw.muas before. The worker goroutine reads the number once, at the top ofrun, with the lock-freeFD(). That is safe becauseshutdowncloses the eventfd only afterLoop.Closehas joinedrun. One behaviour changes with it (review round 2):WakeFD.Closediscards the eventfd'sclose(2)error, soLoop.Closenow returns only the epoll fd's close error, whereshutdownused to return the eventfd's too. The package is internal, and its only caller ofLoop.Closediscards the error (_ = l.Close()inRelease,registry.go:113).RegisterConnissues the ADD underc.mu, after a check ofc.closed. Every otherepoll_ctlof a conn already follows this rule (flushLocked,setEvents). The conn is in the map by then, and a conn leaves the map only once it is marked closed.shutdownmarks every conn in the map closed, each under itsc.mu, before it closes the epoll fd. So the ADD either completes before the close or is not issued. AnUnregisterConnof the same fd marks the conn closed before itsEPOLL_CTL_DEL, so the ADD comes before that DEL or not at all.UnregisterConnracing the registration) has already had itsonClose. No ADD is issued, and the registration is reported as made, the same as for a conn torn down right after it.onClosefor a registration that was refused.w.muis not held across the ADD (review round 1). Round 0 held its read lock there. Async.RWMutexmakes new readers wait once a writer is queued, so aforget, a registration or a shutdown queued behind the ADD made the worker's lookups wait for the syscall as well. v1.6.0 polish: drivers #887 already tracks the measured cost of anepoll_ctlheld under the exclusivew.mu(forget's DEL, for a reader on the same worker under register/unregister churn). This ADD adds none.testHookBeforeAdd, a nil-by-default test hook, sits next to fix(eventloop): never read a driver conn's descriptor number after UnregisterConn has returned (celeris#784) #843'stestHookBeforeRead, in the unlocked window between the map and the ADD.Deadlock check (RULE 10).
wake's callers hold no lock, or hold the conn'srecvMuwhen they run inside anonRecvcallback.recvMucomes first in the documented order (recvMu,w.mu,c.mu,c.rmu).RegisterConntakesw.mu(write) to insert the conn and releases it, then takesc.mu, thenc.rmu; when the ADD fails it takesw.muagain with no other lock held.shutdowntakesw.mu(write), thenc.mu/c.rmuthroughmarkClosed. No path takesw.muwhile it holdsc.mu.Tests and controls
Three tests are added in
wake_close_862_linux_test.go:TestWriteRacingCloseNeverTouchesTheClosedEventfd862runs 64 rounds. In each, 4 goroutinesWrite1 byte to a conn whose send buffer is full, so everyWritetakes the pending path and wakes the worker, whileLoop.Closeruns. It has two oracles:-race, the report of the unlocked eventfd read. The test's coverage rests on this report; CI's root step runs this package with-race.Closereturns take the numbers Close freed, and neither may receive a write. A wake has to land in the few microseconds between the close and the reuse for it to fire, so it seldom does, with or without-race(the "wake into a freed number" column below).TestRegisterConnRacingCloseNeverAddsToAClosedEpoll862is deterministic. The hook runsLoop.Closeto completion in the window, then the test opens epoll instances until one takes the closed epoll fd's number. The ADD must not land there and must not fail on the closed number.onCloseand the returned error must agree. If no epoll instance takes the number, the test fails rather than skipping that check (review round 1).TestRegisterConnRacingUnregisterConnLeavesNoEpollEntry862(review round 1) is deterministic. The hook runsUnregisterConnof the same fd to completion in the window. Once both calls have returned, the fd must not be in the worker's epoll set: the test probes the set with anEPOLL_CTL_ADDof its own, which fails withEEXISTif it is.RegisterConnmust return nil, andonClosemust fire once. Mutant EPFDCHECK (the ADD skipped only once shutdown has retired the epoll fd, instead of whenever the conn was torn down) passes the other two tests and fails this one.Each arm runs the tests as 20 separate processes (
-race, linux/arm64, golang:1.27, kernel 7.0.12-linuxkit; one process is one observation). The race detector reports a given race once per process, which is why-countinside one process would understate the rate.fix-r1/logs/862-r1b/ff-main.logfix-r1/logs/862-r1b/ff-main-hooked.logfix-r1/logs/862-r1b/fix.logfix-r1/logs/862-r1b/nc.logfix-r1/logs/862-r1b/mut-MREG.logfix-r1/logs/862-r1b/mut-MREG2.logfix-r1/logs/862-r1b/mut-MWAKE.logw.epollFD >= 0instead of!c.closed)fix-r1/logs/862-r1b/mut-EPFDCHECK.logfix-r1/logs/862-r1b/main-pkg.log)fix-r1/logs/862-r1b/fix-pkg.log)fix-r1/logs/862-r1b/fix-drivers.log)Failure rate on main: under
-race, the race test fails in 20 of 20 processes, and both register tests fail on the defect itself whenever the hook is at the base's ADD (ff-main-hooked). The race test's opportunistic oracle (a wake into a freed number) fired only in the processes counted in that column; its detection rests on-race. The second controls show that each test catches its own defect and only that one.This PR's head is unchanged in round 2 (dce5a3c), so these are round 1's controls, which ran at this head. Scripts:
bash evidence/lanes-20261003/D1/fix-r1/scripts/trees.sh df31adc dce5a3c 399072a a509a99 53994be(the export trees, built from round 1's later heads; the git worktree is never mutated), thenbash evidence/lanes-20261003/D1/fix-r1/scripts/controls.sh 862 r1b. Table:python3 evidence/lanes-20261003/D1/fix-r1/scripts/table862.py evidence/lanes-20261003/D1/fix-r1/logs/862-r1b. Paths are relative to the maintainer's evidence root (~/.claude/projects/-Users-fuming-Documents-github-celeris-probatorium/).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).#932 against df31adc. Each cell is the median ± CI. Each delta 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=4No row differs from df31adc at α=0.05 in sec/op or pairs/s, and allocs/op does not change on the paths this PR touches.
BenchmarkWritePending862(theSignalread lock): 369.8 ns against 370.2 ns (p=0.579).BenchmarkRegisterChurn862(the ADD underc.mu): no shift at churn 0, 1 or 4 (p=0.48 to 0.80). The one significant row in any metric ischunk=524288allocs/op, 5.5 down to 4.0 (p=0.023). That row's B/op is ±1004% in df31adc, so the shift is noise and not a saving.What changes on the measured paths, without numbers: a
Writethat leaves bytes pending now takesWakeFD.Signal's read lock around the eventfdwrite(2)it already made (#666 measured that shape for the engines ininternal/wakefd'sBenchmarkSignal).RegisterConntakesw.muonce, as on main, and now issues its ADD under the new conn'sc.mu, which is uncontended unless a call on the same fd or a teardown races the registration. The benchmarks committed for these paths areBenchmarkWritePending862andBenchmarkRegisterChurn862; both run on main too.Family audit
Every site where a caller on another goroutine uses one of the worker's own descriptors:
wake(enqueueFlush,Loop.Close)RegisterConnADDc.mu, closed check)flushLockedMODs (Write,WriteAndPoll*step 1,drainOne)c.muafter the closed check; shutdown marks every conn closed before it closes the epoll fdsetEvents(theWriteAndPoll*mask and re-arm)w.mu)forgetDELw.muwith anepollFD >= 0checkrun(EpollWait, the eventfd drain)shutdownloop_other.go(the non-Linux fallback; it builds on darwin, and the drivers do not build on windows at all: #937)#655 is the engines' twin; #666 took the same approach there (the wakeup eventfd, and
ctlMufor the epoll fd). The drivers do not own either descriptor.Review round 1
RegisterConnheldw.mu's read lock across the ADD, and its comment said that did not hold up the worker's lookups. That is not so once a writer is queued. The lock is dropped (see Fix); the controls, including mutants MREG and MREG2, were re-run at dce5a3c.TestRegisterConnRacingUnregisterConnLeavesNoEpollEntry862is added, with mutant EPFDCHECK as its second control.-raceand that the second oracle is opportunistic. A stronger non-race test is item "fix(eventloop): a Write or RegisterConn racing Loop.Close no longer uses the worker's eventfd or epoll fd number after shutdown closed it (celeris#862) #932 wake test" on v1.6.0 polish: CI and tests #890.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). This PR's code is unchanged in round 2.
RegisterConntakesc.mu, so neither pins that thec.closedcheck and the ADD are one critical section (the review's mutant CHECKOUTSIDE passes all three eventloop: a Write racing Loop.Close can write the closed eventfd's number in wake(), which another socket may hold by then (data race) #862 tests). A deterministic pin is item "fix(eventloop): a Write or RegisterConn racing Loop.Close no longer uses the worker's eventfd or epoll fd number after shutdown closed it (celeris#862) #932 register tests" on v1.6.0 polish: CI and tests #890.Loop.Closeno longer reports the eventfd's close error. Stated under Fix.loop_other.gorow said "darwin/windows", but./driver/...does not build on windows, at the base or here. 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.internal/wakefdimport sits next to theengineimport that Define the supported public API surface #443 rewrites next week, so whichever lands second has a one-line textual conflict. Nothing to change.Fixes #862