Repository navigation
fix(engine): serve the connections already queued when accept pauses, and stop re-arming accept on the closing listen socket (celeris#662) - #663
Merged
Conversation
… and stop re-arming accept on the closing listen socket (celeris#662) A pause lost the connections already waiting in the kernel accept queue, and every adaptive switch pauses the old engine. The clients had completed the handshake, and many had already sent a request. epoll accept4()ed each one, then shut it down and closed it, so the client read EOF. io_uring cancelled its multishot accept, reaped the completions already posted and closed the listen fd, so the kernel reset whatever was still queued. Neither engine counted any of it. epoll: the pause branch runs acceptAll until the queue is empty, then removes and closes the listen fd. acceptAll is the registration every accept uses: EPOLL_CTL_ADD with EPOLLRDHUP, connState, the live set, the counters, OnConnect and sockopts. A descriptor it cannot register is closed and counted. acceptAll now returns why it stopped, so the drain continues past the per-call cap and stops on EMFILE/ENFILE. The drain is bounded at connTableSize. The shutdown+close loop and its FIN justification are gone. io_uring: the pause saves the listen fd and clears w.listenFD before it cancels and handles completions. It then drains the non-blocking socket with accept4 into onAcceptedFD, the path an accept completion takes, and closes the fd. If the SQ ring is full, the first recv is left on the dirty list and re-armed after the next submit. Errors go through AcceptFailed. Clearing w.listenFD first removes a stale re-arm. handleAccept re-arms on any completion without F_MORE while listenFD >= 0, which includes the cancelled accept's ECANCELED, so every pause queued an accept for the descriptor it then closed. On main that accept failed with EBADF on every pause. With the drain in place, it failed with EINVAL whenever the sibling worker's drain had already reused the number. The full package run caught this as `bucket AcceptOther moved by 1`. An instrumented copy ran the queued test 50 times: 9 runs failed. Accept completions were ECANCELED 100, EBADF 91 and EINVAL 9, and every EINVAL's fd was one the other worker's drain had just accepted. A pause now costs one failed accept per worker instead of two. Comments that described EBADF as what a pause leaves behind are corrected; EBADF's classification is unchanged. In both engines the drained connections hold connCount above zero. The paused engine serves them, and parks once they close or are transplanted. Measured in Docker on linux/arm64 with -race and --cpus 4. New tests on unmodified main: - epoll TestPauseAcceptServesQueuedConnections fails 5/5 at 8 MiB (EOF 8 of 8, AcceptCount 2). - io_uring TestPauseAcceptServesQueuedConnections fails 5/5 at 128 MiB and 1/1 at 8 MiB (reset 8 of 8). - io_uring TestPauseAcceptDoesNotRearmTheListenSocketItCloses fails 5/5 at 128 MiB (4 failed accepts on 2 workers) and 3/3 at 8 MiB. - TestPauseAcceptQueuedControl (no pause) passes on both trees. With the fix: - Every new test passes 5/5 on each engine, and 3/3 on io_uring at 8 MiB. - The io_uring queued test passes 50/50. Negative controls, each sha256-restored: - Restoring epoll's shutdown+close fails the epoll queued test 3/3. - Removing io_uring's drain call fails its queued test 3/3. - Clearing w.listenFD after the close again fails the re-arm test 3/3, and the queued test 1/3. Packages: - ./engine/epoll/... at 8 MiB: ok. - ./engine/iouring/... at 128 MiB: ok, twice. - ./adaptive/... at 128 MiB: fails only TestBidirectionalFlapAsync (celeris#657). Not fixed here: the adaptive ramp still loses connections at a promotion. At --cpus 2, TestRampH1Sync/Async lose 0 to 381 connections per run on the fix, against 0 to 2134 on main. That is no demonstrated improvement. With an uncommitted accounting probe, every lost connection was one that neither engine ever accepted. Over 8 runs per tree, main's 139 `unexpected EOF` (the old epoll drain) are gone on the fix. The resets remain: 1429 on main, 1128 on the fix. They drop to 0 in 4 runs with net.ipv4.tcp_migrate_req=1. Both listen sockets set TCP_DEFER_ACCEPT. The kernel holds a handshaken connection with no data yet outside the accept queue, so no drain can reach it. A pause resets 8 of 8 such connections on each engine. With TCP_DEFER_ACCEPT off they are accepted before the pause and served. Refs #662
This was referenced Sep 16, 2026
FumingPower3925
added a commit
that referenced
this pull request
Sep 26, 2026
…se drain (celeris#662) #663 taught PauseAccept to serve what its accept queues hold, which fixed the queued case. It cannot reach a connection whose handshake completed but which has sent no data: createListenSocket sets TCP_DEFER_ACCEPT=1, so the kernel holds that connection as a TCP_NEW_SYN_RECV request socket and keeps it out of the accept queue entirely. The drain never sees it, the listen-socket close orphans it, and the client's first request is answered with a reset. Three failing-first tests, in both engines and on the adaptive switch: - TestPauseAcceptKeepsHandshakedIdleConnections dials 8 connections that write nothing, asserts the accept count BEFORE the pause -- 0 on main, which is the defect -- pauses, and only then sends the requests. RESET 8 of 8 on main, on both engines and at both memlock shapes. - TestListenSocketDoesNotDeferAccept asserts the property on the socket itself. It calls createListenSocket directly rather than reading a loop-owned listenFD from the test goroutine, which -race would flag as a fault in the test instead of the engine. - TestSwitchKeepsHandshakedIdleConnections promotes epoll -> io_uring with 16 such connections established: RESET 16 of 16. It is guarded by requireUpSwitch, so the adaptive CI job's CELERIS_REQUIRE_UPSWITCH=1 turns its skip into a failure. TestPauseAcceptIdleControl is the same rig without the pause. It passes on main, which attributes the paused arm's failure to the pause rather than to the rig's unusual write-nothing clients. Each engine rig also logs the kernel's own witness, TcpExtTCPDeferAcceptDrop, whose delta over the dials is 8 while the option is set and 0 once it is gone. The 1 s deferral timer is these rigs' only clock, and it is a vacuity hazard rather than a flake hazard: past about a second the kernel's SYN-ACK retransmit creates the children itself and main would pass for the wrong reason. The measured phase is bounded two orders of magnitude below that, and the elapsed time is asserted, so an overrun fails the test loudly instead of hiding inside it. The setsockopt is deliberately left in place. This commit only proves the loss class.
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
1 participant
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.
Summary
When an engine's accept was paused, connections already waiting in the kernel accept queue were dropped. They got no response, and the engine counted nothing. Every adaptive switch pauses the old engine. The connections had completed the handshake, and many had already sent a request.
accept4()ed each queued connection, then calledshutdown(SHUT_RDWR)andclose. The comment said this sends a FIN rather than an RST, but a client that has sent its request loses it either way.Measured on unmodified
c40d0cbwith the rig from #662, now committed as a test for both engines. A handler blocks until released and holds every loop inside it. 8 more connections each writeGET /and wait in the accept queues. ThenPauseAccept()is called and the handler released:AcceptCountAcceptCancelled(ECANCELED + EBADF per worker, see below)AcceptCancelled"want" is every blocker plus the 8 queued connections.
io_uring also re-armed accept on the socket it was closing
With only the drain in place, the new test passed in isolation. The full
./engine/iouring/...run failed it once withbucket AcceptOther moved by 1, want 0: errors +4, of whichAcceptCancelled3 andAcceptOther1, while all 8 requests still got 200.An uncommitted instrumented copy logged the errno at both
AcceptFailedcall sites and ran the test at-count=50(128 MiB):ECANCELED100,EBADF91,EINVAL9. All arrived while paused, all withoutF_MORE. The drain returned no error.ECANCELEDwhilew.listenFDstill named the socket.handleAcceptre-arms wheneverlistenFD >= 0 && !F_MORE, so it queued an accept SQE for the fd the branch then closed. That SQE was submitted after the close:EBADFon every pause, on main too. That is the second error per worker in the table above, and the reason several comments called EBADF a pause cost.EINVALwhen the sibling worker's drain had already been handed the closed number. That was the case in 9 of 9: each stale fd was one the other worker's drain had just accepted.w.listenFDbefore handling completions, so the existing guard stops the re-arm. A pause now costs one failed accept per worker, itsECANCELED, instead of two.TestPauseAcceptDoesNotRearmTheListenSocketItClosespins this deterministically.What this does not fix: the ramp loss is a second drop class,
TCP_DEFER_ACCEPT#662 attributes a GitHub runner's ramp loss to the queued-connection drop. Measured locally, this PR removes that drop but not the ramp loss. The loss that remains is connections the kernel holds outside the accept queue. Both engines' listen sockets set
TCP_DEFER_ACCEPT, so a handshaken connection whose first bytes have not arrived is not queued yet. Noaccept4can reach it, and the listen close resets it. Details and numbers are under the ramp section below. Hence Refs #662, not Closes.Refs #662
Changes
engine/epoll/loop.go):acceptQueuedOnPause, which runsacceptAlluntil EAGAIN, then removes and closes the listen fd.acceptAllis the registration every accept uses:EPOLL_CTL_ADDwithEPOLLRDHUP, connState, the live set,AcceptCount/ActiveConnections,OnConnect,sockopts.ApplyFD. A descriptor it cannot register (conn-table cap,EPOLL_CTL_ADDfailure) is closed and counted.acceptAllreturns why it stopped, so the drain continues past the per-call cap. It stops on EMFILE/ENFILE (counted inAcceptFDLimit) and is bounded atconnTableSize.PauseAcceptdoc's promise of them.engine/iouring/worker.go):w.listenFDbefore cancelling and handling completions.acceptQueuedOnPause(ctx, lfd)then drains the non-blocking socket withaccept4until EAGAIN, and the fd is closed.onAcceptedFD, the path an accept completion takes. If the SQ ring is full, the first recv is left on the dirty list withneedsRecvand re-armed after the next submit.AcceptFailed. The drain is bounded at the conn-table size.connCount > 0, so the paused engine serves them and parks in DRAINING→SUSPENDED once they close or are transplanted. The tests assert that the park still happens.ResumeAcceptracing the drain loses nothing: the loop closes the listener it read as paused and re-creates it next iteration, as before.engine/engine.go,errclass/errclass.go,errclass/accept_linux.go, io_uring'shandleAcceptanderrorclass_linux_test.go, and theTestRevertChargesItsAcceptTeardownToTheStandbydoc. EBADF's classification is unchanged.engine/{epoll,iouring}/pause_accept_queued_linux_test.go.TestPauseAcceptServesQueuedConnectionsasserts:AcceptCountequals every connection taken off a queue, andOnConnectmatches it;CloseCount == AcceptCount,OnDisconnectmatches, and active is 0;ErrorCount == 0, and io_uring moves no bucket exceptAcceptCancelled;TestPauseAcceptQueuedControlruns the same rig without the pause. Both block as many loops as the engine runs; io_uring caps its workers under a low memlock.TestPauseAcceptDoesNotRearmTheListenSocketItCloses: a pause costs at most one failed accept per worker.Test Plan
Docker, linux/arm64,
-race,--cpus 4unless stated. Counts come only from--- PASS/FAILlines.New tests, main vs fix:
c40d0cbTestPauseAcceptServesQueuedConnectionsTestPauseAcceptServesQueuedConnections-count=50with 0AcceptOtherTestPauseAcceptServesQueuedConnectionsTestPauseAcceptDoesNotRearmTheListenSocketItClosesTestPauseAcceptDoesNotRearmTheListenSocketItClosesTestPauseAcceptQueuedControl(no pause)TestPauseAcceptQueuedControl(no pause)TestPauseAcceptQueuedControl(no pause)"+N" is the
ErrorCounta pause adds, all of it inAcceptCancelled.The drain-only tree, before the re-arm fix, failed the io_uring queued test 9/50 in the instrumented copy. At 8 MiB the io_uring tests do not skip:
capWorkersToMemlockstarts one worker, and the rig blocks as many workers as run.Negative controls on the fix tree. Each was applied, vetted, and run at
-count=3. The file was then restored withcpand its sha256 matched again:ServesQueuedConnectionsDoesNotRearm…QueuedControlc23c3e78…→bbdad5e1…c23c3e78…✓acceptQueuedOnPausecall removed297ef6f5…→74959da4…297ef6f5…✓w.listenFDcleared after the close again (re-arm restored)297ef6f5…→4f48d2f6…AcceptOther+1, the EINVAL race)297ef6f5…✓Packages on the fix (
-race -count=1):./engine/epoll/...Workers: 1config is rejected./engine/iouring/...run 1TestPauseAcceptChargesItsTeardownToAcceptCancellednow measures +2 on 2 workers (was +4)./engine/iouring/...run 2./adaptive/...TestBidirectionalFlapAsync, "transplant did not migrate to io_uring / standby did not drain" on flaps 1 and 3 (err=0). That is the known celeris#657, which fails on main too.TestRevertChargesItsAcceptTeardownToTheStandbypasses at +4AcceptCancelledon 4 workers, one per workerThe ramp (
TestRampH1Sync|TestRampH1Async,--cpus 2= 2 workers, memlock 128 MiB)The "probe" rows run uncommitted copies with the ramp client patched. The patch records whether each failed connection had been served, and aggregates error strings so the counts are complete. Client errors per run:
c40d0cbnet.ipv4.tcp_migrate_req=1TCP_DEFER_ACCEPToff in both enginesread after >0 ok: i/o timeoutin a revert phase, not accept-sideNo demonstrated improvement at this sample size. Per run, main ranges 0–2134 and the fix 0–692. Several fix runs lose exactly 256 connections, one
rampTodial step.The error mix did change. Over the 8 probe runs on each tree:
read after 0 ok: connection reset by peerread after 0 ok: unexpected EOF(the old epoll drain's signature, as in the rig)read after >0 ok: i/o timeout(see below)The connections lost were never accepted. An uncommitted accounting probe logs the client's successful dials and each sub-engine's
AcceptCount,CloseCount, error buckets and transplant ledger after every phase. In every promotion phase that lost connections, lost = dials − (epoll accepts + io_uring accepts), exactly:No transplant refusal, strand or drain-stop was counted.
tcp_migrate_req=1removes the loss entirely. On a listen close, that sysctl migrates the kernel's pending connections to another listener in theSO_REUSEPORTgroup. So does turningTCP_DEFER_ACCEPToff in both engines: 0 connections lost at a promotion in 4 of 4 runs.What the kernel is holding is
TCP_DEFER_ACCEPT. Both engines'createListenSocketset it. An uncommitted rig makes this deterministic: dial 8 connections without writing, wait 100 ms, pause (or not), then write a request on each:TCP_DEFER_ACCEPTA connection in that state has finished the handshake and is not in the accept queue. No drain, epoll's or io_uring's, can take it, and the listen close resets it once the client writes. On a starved host, the gap between a ramp client's connect and its first write is exactly that state.
Removing
TCP_DEFER_ACCEPTchanges a default the benchmark path was tuned with, so it needs its own performance measurement and is left for a follow-up. A pause that migrated these connections instead would needtcp_migrate_reqor aBPF_SK_REUSEPORT_SELECT_OR_MIGRATEprogram.One error class in these runs is not accept-side. One fix-tree Sync run (accounting probe) flapped within its promotion phase: 809 connections transplanted epoll→io_uring and 528 back, with an oscillation unlock. The following revert phase hit its 20 s settle limit with 396
read after >0 ok: i/o timeout: connections that had been served, then stalled. This change does not touch that path. It matches the keep-alive stranding tracked in celeris#657, and I have not attributed it further.mage checkpasses (not run locally;go vetis clean for linux/arm64 and linux/amd64, and so is gofmt)Tested on: [ ] std [x] epoll [x] io_uring — [ ] amd64 (vet only) [x] arm64
Release notes
breaking)bug)