Skip to content

fix(engine): close the listen socket and ring a failed io_uring worker leaves behind, and stop the adaptive standby build retrying into it (celeris#656) - #664

Merged
FumingPower3925 merged 4 commits into
mainfrom
fix/celeris-656-df1269c
Sep 16, 2026
Merged

FumingPower3925 merged 4 commits into
mainfrom
fix/celeris-656-df1269c

Conversation

@FumingPower3925

@FumingPower3925 FumingPower3925 commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

The defect (celeris#656)

Worker.run (engine/iouring/worker.go) creates its SO_REUSEPORT listen socket, then sets up its ring and submits the first accept. If ring setup failed, or that first submit failed, run sent the error on ready and returned without closing anything it had already created:

  • Ring setup failure: the listen socket leaked.
  • Initial submit failure: the listen socket, the ring and the H2 eventfd leaked, plus the buffer ring when multishot recv is opted in.

shutdown is the only code that closes a worker's descriptors, and it runs from the event loop, which a worker that never became ready does not reach. Engine.Listen joins the failed worker and returns its error. The worker is never published in e.workers, so nothing can reach those descriptors later. The leaked socket stays LISTENing in the port's SO_REUSEPORT group for the life of the process, so the kernel keeps sending part of the new connections to a backlog that nobody accepts. Every worker whose own setup fails leaks its own socket, so one failed Listen can leak more than one.

The adaptive engine made this worse:

  • The bind wait ignored Listen's error. buildAndStartStandby logged the standby's Listen error from the goroutine and kept polling Addr() for the full 5 s, while holding e.mu.
  • The abort recorded nothing. performSwitch logged "aborting switch" and returned without recording anything. The controller's load had not changed, so it recommended the same build on the next tick. Each attempt put the io_uring engine's sockets on the serving port again, and on main each one leaked them.
  • Late binders were left running. A standby that bound only after the 5 s deadline kept running under the Listen context: in the group, accepting, and in no slot.

The fix

1. fix(iouring): close what a worker created when its init fails

  • releaseFailedInit() closes the buffer ring, then the ring, then the eventfd, then the listen socket, and resets each field so a second call does nothing. It also sets listenFDClosed, the flag the running loop maintains for that descriptor and PauseAccept polls, so the field never sits in a "fd closed, flag false" state its own doc does not describe.
  • Both failure sites call it before sending the error on ready, so the socket is already gone when Listen sees the error.
  • A defer guarded by initDone covers any early return added later. initDone is set just before ready <- nil, because from then on shutdown owns the descriptors.
  • Two package-level seams follow the epoll/io_uring: Listen publishes a nil Addr() and logs "engine listening" when boundAddr fails on loop 0 #639 listenAddrOf precedent: newWorkerRing (= NewRingCPU) and submitInitialAccept (= (*Ring).Submit). Tests use them to fail one worker's init. No other call site changes.

2. fix(adaptive): stop waiting on a standby whose Listen failed, stop one that misses the deadline, and back off a failed build

  • Bind wait: the standby runs under a child context. The wait is a select over the standby's Listen result, the context, the 5 s timer and a 5 ms tick, so a Listen that returns ends the wait at once. Any failure cancels the child context, so a late binder is stopped and the wait group joins it.
  • The standby's CancelFunc is kept. A standby that is handed back stores its cancel in e.standbyCancels (at most one per slot), and Shutdown calls them. A standby this engine started is never reachable only through a context someone else owns.
  • Backoff: a failed build calls controller.recordStandbyBuildFailure. evaluate then holds the load-driven switches off for 30 s, doubling with each consecutive failure up to 10 min, and the sustain tick counts restart. A successful build clears it. It does not touch recordSwitch, lastSwitch or switchTimes.
  • The error-rate safety revert is decided above the backoff, and the sampler runs on every tick (see the review follow-up below).
  • ForceSwitch calls performSwitch directly, so it is not held back by the backoff.

3. ci: the regression tests actually run

Review follow-ups

The adversarial review of f6b7937 raised two majors, four minors and two nits. All are fixed; none are disputed.

Finding What changed Evidence
MAJOR the three io_uring tests skip silently in CI — zero cover iouring CI job (memlock raised, skipping forbidden, PASS-count interlock) + skipOrFail656 on all three skip paths in the test file 3 PASS in the job's shape; 3 FAIL at 8 MiB with the env; 3 SKIP + ok/exit 0 without it
MAJOR the backoff suppressed the io_uring error-rate safety revert for up to 10 min evaluate samples, then decides the safety revert, and only then applies the backoff — which now gates the load-driven switches only TestErrorRateSafetyRevertIsNotGatedByTheStandbyBuildBackoff; NC-C fails it with the gate moved back
minor the gate returned before sampler.Sample, so the first post-backoff tick saw a stale delta baseline the sample is taken above every backoff return TestStandbyBuildBackoffKeepsSamplingEveryTick; NC-C reports "sampler was called 0 times over 5 backed-off ticks"
minor the cap assertion could not fail for the stated reason (two failures = 60 s, probed at 11 min) the 11-minute probe became a 61 s probe with the cap claim removed; the doubling and the cap branch get their own test against the constants TestStandbyBuildBackoffDoublesAndCapsAtTheMaximum walks 30 s → 60 s → 2 → 4 → 8 → 10 → 10 → 10 min and asserts the table matches standbyBuildBackoffBase/Max
minor the successful standby's CancelFunc was dropped stored in e.standbyCancels, cancelled by Shutdown ./adaptive and the integration adaptive subset re-run green
minor the comment over-claimed that the accept SQE never reached the kernel reworded: Submit does not report on its error path whether the SQE reached the kernel (io_uring_enter can consume part of a batch — hence retryPending), which is why the ring is closed first comment only
nit releaseFailedInit did not set listenFDClosed one line, with a comment on why nothing reads it yet comment + code
nit the main-FAIL provenance check was never captured this round writes r2-provenance.log: the hash of every tested byte, each control's reverted bytes, the git show df1269c hashes recomputed, and the restore check r2-provenance.log

Measured: FAIL on main

All runs: golang:1.27 container (go1.27.1 linux/arm64, kernel 7.0.12-linuxkit), --cpus 4, go test -race.

The io_uring tests need the two seams to fail one worker's init, so they ran on main plus the seam-only diff (no behaviour change). The adaptive tests ran on adaptive/{engine,controller}.go byte-identical to df1269c.

Test Result on main What it measured
TestListenClosesListenSocketsWhenEveryWorkerRingSetupFails FAIL 2 listen sockets survived on the port; plain bind still address already in use after 3 s
TestListenClosesListenSocketWhenOneWorkerRingSetupFails FAIL 1 listen socket survived (the failed worker's); bind refused
TestListenClosesListenSocketRingAndEventfdWhenInitialSubmitFails FAIL 2 listen sockets survived; io_uring descriptors 0 → 2; eventfd descriptors 1 → 3
TestLazyStandbyListenFailureAbortsPromptlyAndBacksOff FAIL performSwitch took 5.0057 s with e.mu held after Listen had already failed; the same build was recommended again at +6 s and +29 s
TestLazyStandbyThatMissesTheBindDeadlineIsStopped FAIL the abandoned standby was still running 2 s after the switch gave up on it

Measured: PASS on the fix

go test -race -count=5 -v, memlock 128 MiB, CELERIS_REQUIRE_IOURING_WORKERS=1 (so a skip would fail):

Test Runs Result
TestListenClosesListenSocketsWhenEveryWorkerRingSetupFails 5 PASS
TestListenClosesListenSocketWhenOneWorkerRingSetupFails 5 PASS
TestListenClosesListenSocketRingAndEventfdWhenInitialSubmitFails 5 PASS
TestLazyStandbyListenFailureAbortsPromptlyAndBacksOff 5 PASS
TestLazyStandbyThatMissesTheBindDeadlineIsStopped 5 PASS
TestErrorRateSafetyRevertIsNotGatedByTheStandbyBuildBackoff 5 PASS
TestStandbyBuildBackoffStillGatesTheLoadDrivenSwitch 5 PASS
TestStandbyBuildBackoffKeepsSamplingEveryTick 5 PASS
TestStandbyBuildBackoffDoublesAndCapsAtTheMaximum 5 PASS

45/45 PASS, 0 FAIL, 0 SKIP (ok engine/iouring 1.134s, ok adaptive 26.029s, exit 0). performSwitch after a failed standby Listen: 31–251 µs, against 5.0057 s on main.

Each io_uring test runs an oracle positive control first, with no io_uring involved: it creates a socket with this package's own createListenSocket, asserts the /proc/self/fd counter sees exactly 1 listener on the port and that a plain net.Listen is refused with EADDRINUSE, then closes it and asserts the count is 0. The tests also require Listen to fail with the injected error, so they cannot pass vacuously.

Measured: the CI gap is closed

Same container, running the new job's exact command and its PASS-count interlock:

Shape memlock CELERIS_REQUIRE_IOURING_WORKERS Result Interlock
the new iouring job unlimited 1 3 PASS, 0 FAIL, 0 SKIP, ok, exit 0 3 → step passes
control: env without the raised limit 8 MiB 1 3 FAIL, exit 1 — RLIMIT_MEMLOCK funds 1 io_uring workers, the test needs 2 … CELERIS_REQUIRE_IOURING_WORKERS=1 forbids skipping 0 → step fails
control: what the unit job does today 8 MiB unset 3 SKIP, ok, exit 0 0 → step fails

The third row is the review's point reproduced: at the runner default these tests skip and the job still goes green. The first row is the fix; the second proves the guard is live rather than decorative.

Confirmed on a real runner, not just in the shape: this PR's own CI run of the new job logs memlock (KiB): unlimited, then --- PASS for each of the three tests, then its interlock line celeris#656 tests passed: 3 (job io_uring init-failure regression (./engine/iouring)). All 12 checks on this head are green.

Measured: negative controls

Three controls, each reverting exactly one thing, hashes recorded before and after and restored with cp (r2-provenance.log).

Control Reverted Result
NC-A the leak fix only worker.go → main + seam-only (67647e2c…; 0 occurrences of releaseFailedInit, vs 5 in the fixed file) the 3 io_uring tests FAIL: 2 listeners on port 40487, 1 on 39291, 2 on 39579 with io_uring 0 → 2 and eventfd 1 → 3
NC-B the adaptive fix only adaptive/{engine,controller}.go → df1269c exactly (7f191e5a… / f0dba3a7…, recomputed from git show) the 2 adaptive tests FAIL: performSwitch took 5.003034919s, same build recommended at +6 s and +29 s, retry within 59 s, standby still running after 2 s
NC-C only the evaluate restructure the backoff gate moved back above the sample and the safety revert (94ebb105…), nothing else 7 PASS / 2 FAIL — exactly the two new claims fail: the revert is "suppressed 1s / 30s / 1m59s into a 2m0s backoff" and "still suppressed one minute before a 10m0s backoff expires", and "the sampler was called 0 times over 5 backed-off ticks"

NC-C is the one that matters for the safety-revert finding: with the restructure reverted and everything else intact, the two tests that judge it fail and the other seven still pass. NC-B moves the new white-box test file aside, because it calls an API that does not exist on main; the two behaviour tests it keeps compile against main unchanged.

Injection-removed control: with the seams left at the real NewRingCPU / Submit, Listen no longer fails and the tests t.Fatal rather than passing. Cross-engine: epoll already closes listenFD/epollFD on its early returns, so this defect is io_uring-only; not run.

Measured: affected packages

go test -race -count=1 -v:

Package memlock PASS FAIL SKIP Verdict
./engine/iouring unlimited 197 (126 + 71 subtests) 0 0 ok 109.6 s
./adaptive 128 MiB 91 (75 + 16 subtests) 1 0 only TestBidirectionalFlapAsync (celeris#657, pre-existing, quarantined by name in ci.yml)
./test/integration (TestAdaptive|TestEpollPauseResume) 128 MiB 7 0 0 ok 4.4 s, incl. TestAdaptiveSwitchUnderLoad and TestAdaptiveConstrainedRing, which drive a real successful lazy standby build through the rewritten bind wait

TestBidirectionalFlapAsync was measured on main in the same shape during this work (FAIL on main, TestBidirectionalFlap PASS on both). gofmt clean; go vet clean for GOOS=linux on arm64 and amd64; golangci-lint 0 issues; actionlint clean on the modified workflow.

Scope, and what is not claimed

  • The leak is fixed and pinned by tests that now run in CI. The rate at which worker init fails under real load is still unmeasured — the issue says so too.
  • The backoff no longer touches the safety revert. The earlier claim that gating it "costs nothing that was available anyway" was wrong, and the review was right: on the io_uring-start path (connSwitchEnabled and loadDownRevert both false) the error-rate revert is the only switch evaluate can ever recommend, so a backoff over it would leave an erroring engine serving for up to 10 minutes — and the storm itself would drive the backoff up. The revert is now decided above the gate. A revert whose epoll standby build keeps failing therefore keeps retrying every tick: that build is bounded work that leaves nothing behind (the leak this PR fixes was io_uring's; epoll closes its descriptors on every early return), and there is no other way back.
  • The unit job still runs ./engine/iouring at the runner's 8 MiB, unchanged, where these three tests skip; the new job is scoped to the three by -run so the rest of the package keeps the shape it has been green in.
  • Adaptive reachability, a correction to the issue text. At 8 MiB memlock with more than one wanted worker, ioUringViable is already false, so an epoll-start adaptive engine never attempts the io_uring build. The retry loop needs a per-worker ring failure on a host whose memlock pre-flight passes, and the retry cadence was ~5–6 s, not 1 s. The amplification is real but conditional; the leak itself is unconditional.
  • Seams: newWorkerRing and submitInitialAccept are test seams following the listenAddrOf precedent from epoll/io_uring: Listen publishes a nil Addr() and logs "engine listening" when boundAddr fails on loop 0 #639. They are written before Listen and restored after it returns, and Listen joins every worker before returning, so -race stays clean.

Closes #656

…ts ring setup or first submit fails (celeris#656)

Worker.run creates its SO_REUSEPORT listen socket, then sets up its ring
and submits the first accept. When the ring setup or that submit failed,
run sent the error on ready and returned. Nothing closed what it had
already created, because shutdown only runs from the event loop, which a
worker that never became ready does not reach. Listen joined the worker
and returned the error, and the worker was never published. The socket
stayed LISTENing in the port's reuseport group for the life of the
process, taking a share of new connections into a backlog nobody accepts.
One failed Listen leaks one socket per failed worker; the submit path also
leaks the ring and the H2 eventfd.

releaseFailedInit closes the buffer ring, ring, eventfd and listen socket
and resets each field. Both failure sites call it before reporting, and a
defer guarded by initDone covers any other early return. initDone is set
just before ready <- nil, because from then on shutdown owns the
descriptors and a second close could hit a reused descriptor number.

newWorkerRing and submitInitialAccept put the two calls behind vars, as
listenAddrOf did for #639, so the tests can fail one worker's init.
… one that misses the bind deadline, and back off a failed build (celeris#656)

When the lazy standby's Listen failed, as an io_uring engine with a failing
worker does, buildAndStartStandby logged the error and still polled Addr()
for the full 5s while holding e.mu. performSwitch then aborted without
recording anything, so the controller asked for the same build on the next
tick. Every attempt put the standby's sockets on the serving port again. A
standby that bound after the deadline kept running under the Listen
context, accepting in the reuseport group while sitting in no slot.

The standby now runs under a child context. The bind wait selects on the
standby's Listen result, so a failed Listen ends the wait immediately, and
any failure cancels the child context, which stops a late binder.

A failed build now records a backoff. evaluate holds off for 30s, doubling
with each consecutive failure up to 10min, and the sustain ticks restart. A
successful build clears it. The backoff is kept apart from recordSwitch, so
direction, cooldown and the oscillation lock do not move. ForceSwitch is not
held back. The backoff state is shown in the CELERIS_ADAPTIVE_DEBUG tick
trace and in the abort log.
@FumingPower3925 FumingPower3925 added bug Something isn't working engine/iouring io_uring engine specifics area/engine Engine interface or implementation labels Sep 16, 2026
@FumingPower3925 FumingPower3925 added this to the v1.6.0 milestone Sep 16, 2026
@FumingPower3925 FumingPower3925 added the area/ci CI/CD pipeline label Sep 16, 2026
…tenFDClosed gap (celeris#656)

The three tests that pin the failed-worker listen-socket leak all asked for
two io_uring workers on one port. RLIMIT_MEMLOCK funds one worker per 12 MiB
(minMemlockPerWorker), GitHub-hosted runners default to 8 MiB, and the only
job that runs ./engine/iouring is `unit`, which does not raise it and does not
pass -v. So all three skipped, invisibly, and the fix shipped with no CI
cover: the job went green precisely because the tests did not run.

Same two ingredients the `adaptive` job uses for the up-switch tests (#641):
a job that raises memlock with prlimit, and an environment variable --
CELERIS_REQUIRE_IOURING_WORKERS=1 -- that turns every environment skip in
init_failure_leak_linux_test.go into a failure. Because -run is a regex over
test names, a rename would select nothing and `go test` would still exit 0,
so the step counts the PASS lines and fails unless exactly three report.

Also on the engine side: releaseFailedInit now sets listenFDClosed when it
closes the socket, the flag the running loop maintains for that descriptor
and PauseAccept polls. Nothing reads it for a worker that failed init -- one
is never published in e.workers -- but "fd closed, flag false" is a state the
field's own doc does not describe. And the comment above it no longer claims
the accept SQE never reached the kernel: Submit does not report that on its
error path (io_uring_enter can consume part of a batch, which is why it calls
retryPending), and not knowing is exactly why the ring is closed first.
…build backoff (celeris#656)

The backoff a failed lazy standby build records sat at the top of evaluate,
above everything. That suppressed the io_uring error-rate safety revert --
the one path this file documents as ALWAYS active -- for as long as ten
minutes. It is not a theoretical loss: on the io_uring-start path New leaves
connSwitchEnabled and loadDownRevert false, so the error revert is the only
switch evaluate can ever recommend there. One transient epoll build failure
during an error storm parked the fallback for 30s, and each further attempt
doubled it, so the storm drove its own suppression up to the cap.

evaluate now samples, decides the safety revert, and only then applies the
backoff, which holds off the load-driven switches alone. Sampling above the
gate also fixes a second defect: liveSampler is delta-based, so a tick that
returned before Sample widened the next window to the whole hold-off and
smeared a fresh error burst across it -- and the first tick after the backoff
is exactly the one the revert would read.

A revert whose epoll standby build keeps failing now keeps retrying. That
build is bounded work that leaves nothing behind: the leaked listener this
issue is about was io_uring's, and epoll closes its descriptors on every
early return.

Also: a standby that is handed back stores its context's CancelFunc in
e.standbyCancels (at most one per slot) and Shutdown calls them, so a standby
this engine started is never reachable only through a context someone else
owns.

Tests: the cap assertion in the existing test claimed something it could not
judge -- after two failures the backoff is 60s, so probing at 11 minutes
passed for any backoff under 11 minutes, capped or not. It is now a 61s
probe, and the doubling and the cap branch get their own test that walks
30s -> 60s -> 2 -> 4 -> 8 -> 10 -> 10 -> 10 min against the constants. Two
further tests pin the boundary the restructure draws: the safety revert fires
inside a backoff (including one minute before a 10-minute one expires), and
the sampler is called on every backed-off tick.
@FumingPower3925
FumingPower3925 merged commit 6136554 into main Sep 16, 2026
12 checks passed
@FumingPower3925
FumingPower3925 deleted the fix/celeris-656-df1269c branch September 16, 2026 03:12
FumingPower3925 added a commit that referenced this pull request Sep 16, 2026
…(celeris#667)

The two celeris#667 regression tests ran in no CI job, and neither did the
other ~14 chanReader unit tests: the root race job excludes
./middleware/websocket, and the one websocket step selected
`-run '^TestBackpressure'`, which matches neither of them.

Widen that step to `^(TestBackpressure|TestChanReader)` and add the interlock
the `adaptive` (#660) and `iouring` (#664) jobs established. `-run` is a regex,
so a rename would select nothing while `go test` still exits 0; the tally
requires exactly the two celeris#667 tests to report PASS, and a skipped test
prints `--- SKIP`, never `--- PASS`, so a skip cannot satisfy it either.

Unlike those two jobs this needs no raised privileges and has no environment
skip to forbid: the tests are in-process over newChanReader, with no sockets,
no engine and no io_uring ring. `-v` is required for the tally, and the budget
goes to 300s because the step now runs two suites with per-test output; the
measured wall time of this exact shape is ~50s.

Proved in the CI shape (golang:1.27, -race, memlock 8 MiB, the step body
extracted verbatim from ci.yml and run as `bash --noprofile --norc -eo
pipefail`): fix rc=0 with passed=2; base rc=1 with both tests FAIL; both tests
renamed out of the namespace rc=1 on the interlock; both tests skipped rc=1 on
the interlock.
FumingPower3925 added a commit that referenced this pull request Sep 16, 2026
… shutdown closed it (#666)

Both engines published their wakeup eventfd's descriptor NUMBER in a loop field. Producers on
other goroutines -- a driver registering a connection, a transplant handing over a descriptor, a
detached WebSocket applying backpressure, an H2 handler queueing a response frame -- read that
number and wrote 8 bytes to it. Shutdown closed the descriptor without stopping them, so a
producer that ran afterwards wrote into whatever the process opened next: a spurious wakeup on
another eventfd, bytes injected into a client socket, or silent corruption of a file. Nothing
reported any of it.

`internal/wakefd.WakeFD` owns the descriptor instead of publishing it. `Signal` and `Close` take
the same lock, so a signal either happens entirely before the close or does not happen at all --
generalising to every producer the rule #658 applied to the two adopt queues.

The loop thread does not pay for it. `FD()`, which the epoll event loop calls for every event it
dispatches, is a relaxed atomic load of a field padded off the mutex's cache line; the number is
written exactly twice in a loop's life, both on the loop thread.

The mutex is a leaf: nothing is acquired while it is held, so `Signal` is safe to call under a
queue mutex. A nil handle is valid and permanently disabled, which is what a loop whose eventfd
could not be created needs.

One commit here is a rebase reconciliation rather than part of the fix: #664 landed
`releaseFailedInit` closing the raw `w.h2EventFD`, which this branch replaces with a `WakeFD`.
The two touched different lines, so git merged them cleanly into code that did not compile. It
now calls `w.wakeFD.Close()`, matching what `shutdown` already does on this branch.

Closes #655.
FumingPower3925 added a commit that referenced this pull request Sep 26, 2026
…(celeris#667)

The two celeris#667 regression tests ran in no CI job, and neither did the
other ~14 chanReader unit tests: the root race job excludes
./middleware/websocket, and the one websocket step selected
`-run '^TestBackpressure'`, which matches neither of them.

Widen that step to `^(TestBackpressure|TestChanReader)` and add the interlock
the `adaptive` (#660) and `iouring` (#664) jobs established. `-run` is a regex,
so a rename would select nothing while `go test` still exits 0; the tally
requires exactly the two celeris#667 tests to report PASS, and a skipped test
prints `--- SKIP`, never `--- PASS`, so a skip cannot satisfy it either.

Unlike those two jobs this needs no raised privileges and has no environment
skip to forbid: the tests are in-process over newChanReader, with no sockets,
no engine and no io_uring ring. `-v` is required for the tally, and the budget
goes to 300s because the step now runs two suites with per-test output; the
measured wall time of this exact shape is ~50s.

Proved in the CI shape (golang:1.27, -race, memlock 8 MiB, the step body
extracted verbatim from ci.yml and run as `bash --noprofile --norc -eo
pipefail`): fix rc=0 with passed=2; base rc=1 with both tests FAIL; both tests
renamed out of the namespace rc=1 on the interlock; both tests skipped rc=1 on
the interlock.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci CI/CD pipeline area/engine Engine interface or implementation bug Something isn't working engine/iouring io_uring engine specifics

Projects

None yet

1 participant