fix(server): a Start that never serves releases the caller's listener, the CPU monitor and the settle re-opener (#737)

[FumingPower3925]

Fixes #737

What was wrong

StartWithListener and StartWithListenerAndContext tell the caller not to close the listener it hands over. Two kinds of Start then left things open that nothing would ever close:

1. A start that fails before any engine runs (in doPrepare: config validation, a bad TrustedProxies entry, createEngine). It returned the error with the caller's listener still bound and listening, so the kernel kept completing handshakes into a backlog nothing accepts (the middleware/websocket tests: the native-engine start helper never reads Start's error, so a failed start is reported as a 30 s "server not ready within timeout" #706 symptom). When createEngine failed, the CPU monitor's /proc/stat descriptor, opened just before, stayed open too: only Shutdown closed it, and a caller whose Start failed has no reason to call Shutdown.
2. A start that publishes its engine and runs Listen, but ends with no Shutdown to come. Found while measuring (1), same class: Listen fails (the address is taken), or Shutdown was called before Start, so Listen ran on a context that was already cancelled and returned at once (Server.Start / StartWithListener never return after Shutdown on io_uring and epoll: Listen blocks on a Background context and Engine.Shutdown is a no-op #595). doPrepare had opened the CPU monitor and started the settle re-opener (adaptive dispatch never re-times a settled route: a store-backed handler that turns slow runs inline on the engine worker forever (#493 item 4, measured) #592), and again only Shutdown released them: one descriptor and one goroutine per such Start, for the life of the process. The fix(server): shut down on every context cancel, and return only after it (celeris#673) #692 design makes the second case explicit: a Shutdown before Start sets directShutdown, so the Start*Context watcher leaves the shutdown to it, and that Shutdown found no engine to shut down.

Measured on main dccb839 (linux/arm64 Docker, CI shape), 20 Start calls per row, GC off so no finalizer closes a leaked descriptor, /proc/self/fd counted before and after; route_adaptive=true (so a published engine starts the re-opener):

scenario	fd delta after 20	settle re-openers left running
createEngine fails, StartWithListener	+40	0
createEngine fails, StartWithListenerAndContext	+40	0
createEngine fails, StartWithContext	+20	0
config validation fails, StartWithListener	+20	0
TrustedProxies fails, StartWithListener	+20	0
Listen fails (address taken), std, Start	+20	20
Listen fails (address taken), epoll, StartWithContext	+20	20
Shutdown before Start, std, StartWithContext	+20	20
Shutdown before Start, epoll, StartWithListener	+20	20

(probe-dccb839.log, from 737/probe/run-probe.sh; the probe test is evidence only, not committed.)

The fix (server.go only)

• prepareWithListener closes the supplied listener when the start fails, except on ErrAlreadyStarted: then a server is already running, perhaps on that very listener (std serves on it directly), so closing it would stop that server. The listener stays the caller's, and the StartWithListener doc now says both.
• doPrepare closes the CPU monitor when createEngine fails.
• The function listenContext returns, which every Start* entry point defers right after it (so it runs once Listen has returned, and the server never serves again: Start cannot be retried), now also stops the settle re-opener and closes the CPU monitor. Both are idempotent under their own locks, so a Shutdown before or after it changes nothing. Putting the release there rather than at each Listen call site keeps it in one place, and keeps this PR out of the lines fix(server): Shutdown runs the OnShutdown hooks, and returns, only after the drain on every engine (#703) #746 (Server.Shutdown runs OnShutdown hooks and returns before epoll/io_uring drain in-flight requests, breaking its documented order #703) changes: the two branches merge cleanly, and the suites below also ran the merge.

Not on the request path: all three run once per Start.

Tests

• TestFailedStartClosesTheSuppliedListener (all OSes): each doPrepare failure x both listener entry points, called twice on the same server (the second call returns the cached error with a new listener). Checks the listener is closed (a second Close reports net.ErrClosed) and a dial to its address is refused.
• TestErrAlreadyStartedLeavesTheListenerToTheCaller: the boundary.
• TestFailedStartClosesTheCPUMonitor: createEngine failure via Start, StartWithContext, StartWithListener.
• TestStartWithoutShutdownReleasesWhatItOpened (all OSes, std): Listen fails (3 entry points) and Shutdown first (3 entry points); after Start returns, the re-opener is stopped and the monitor closed.
• TestFailedStartsLeaveNoProcStatDescriptor and TestStartsWithoutShutdownLeaveNoProcStatDescriptor (Linux): the issue's measurement, 20 starts, GC off, counting only descriptors whose link is /proc/stat so other tests' descriptors cannot move it.

Failing-first and controls (linux/arm64 Docker, --cpus 4, CI shape: 8 MiB memlock; -race -v; counts are --- PASS/FAIL/SKIP lines only)

On main dccb839, the six new tests (overlaid, scripts/ff-main.sh, ff-main/final2/): 5 FAIL (23 of 25 subtests), in both shapes (CI shape and unconstrained memlock); only the ErrAlreadyStarted boundary passes, as it should. No SKIP line.

Controls, one container (737/controls.sh), head fb52752, each arm a copy of the head's tree with server.go replaced (never git checkout); the run covers the six new tests plus TestStartWithListenerDoubleStart, TestServerDoubleStart and #592's TestRouteAdaptive_NoReopenerWhenEngineCreationFails:

arm	top-level	subtests	what fails
head, -count=3	27 PASS	75 PASS	nothing
revert: main's server.go (the negative control)	4 PASS, 5 FAIL	2 PASS, 23 FAIL	all five new behaviour tests; the ErrAlreadyStarted boundary passes, as it does on main
N1: no listener close	8 PASS, 1 FAIL	6 FAIL	only TestFailedStartClosesTheSuppliedListener (all 6 cases)
N2: no monitor close on a createEngine failure	7 PASS, 2 FAIL	7 FAIL	TestFailedStartClosesTheCPUMonitor (3) and TestFailedStartsLeaveNoProcStatDescriptor (4, +20 /proc/stat descriptors each)
N3: listenContext returns the bare cancel	7 PASS, 2 FAIL	10 FAIL	TestStartWithoutShutdownReleasesWhatItOpened (6) and TestStartsWithoutShutdownLeaveNoProcStatDescriptor (4, +20 each)
N4: the listener closed on ErrAlreadyStarted too (over-closing)	8 PASS, 1 FAIL	2 FAIL	only the boundary test

Each partial mutant is caught by exactly the tests written for its part, and no SKIP line appears in any arm.

Suites (scripts/suites.sh, -race -count=1 -v, one container per shape, one go test process per arm)

Root package, arms: base main dccb839, #746's head 76c205c, this head fb52752, and a local merge of the two (they merge cleanly):

shape	base	this PR	merged with #746
CI (8 MiB memlock, one io_uring worker)	370 PASS, 0 FAIL, 1 SKIP (+88/0/3 subtests)	375 PASS, 1 FAIL, 1 SKIP (+111/2/3): see below	380 PASS, 0 FAIL, 1 SKIP (+134/0/3)
unconstrained memlock (several io_uring workers)	370/0/1 (+91/0/0)	376/0/1 (+116/0/0)	380/0/1 (+137/0/0)

Apart from the one failure below, no test goes from PASS to anything else, and every test new in an arm passes (scripts/suite-compare.py).

The failure is TestAdaptiveSettledRouteRetime592/epoll/settled in the CI shape, verdict NOT_FIXED. The failing run's own log is what attributes it. Its reference window, taken while /kv was settled, saw /ping fast (284 samples, median 0.107 ms), so the rig's premise (a pinned worker to compare against) did not hold in that run. The rig counts on every worker holding a /kv connection, and its own comment puts the miss at about 2^-7 per run. The same code passed in the merged arm of the same container and in the unconstrained shape. The release this PR adds runs only after Listen has returned, never during the run.

A re-run (scripts/rerun.sh, 05-reruns/retime592.log, one container, CI shape, -count=5) gave main 5/5 PASS and this head 5/5 PASS, every settled arm FIXED with a stalled reference window (2-7 samples, medians 0.59-2.10 s). That is consistent with the attribution, but at a 2^-7 miss rate five runs a side cannot tell the two sides apart, so the re-run is not the attribution. The skips are the same in every arm: TestRouteAdaptive_SettleReopenCost (env-gated) and, at 8 MiB only, the three io_uring subtests of TestAdaptiveSettledRouteRetime592 (#709).

./middleware/... ran for #746 (below the root package, it is where servers are started and shut down most); for this PR, CI's Unit job ran it.

Round 2 (this head unchanged, fb52752). #746 changed in its round 2 (3d2ab72: a Start* call stopped by a direct Shutdown now waits for it). A local merge of 3d2ab72, this head and main 9b670b8 merges cleanly and passes the root package in both shapes, 0 FAIL (round2/scripts/suites.sh, -race -count=1 -v):

shape	main 9b670b8	merged (#746 3d2ab72 + this PR + main)
CI (8 MiB memlock)	372/0/1 (+98/0/3)	383/0/1 (+161/0/3)
unconstrained memlock	372/0/1 (+101/0/0)	383/0/1 (+164/0/0)

No test goes from PASS to anything else (round2/scripts/suite-compare2.py), and the six #737 tests pass in both shapes. In the merge, this PR's release (in listenContext's returned function, deferred in listen) runs before listen closes listenDone and before it waits for a direct Shutdown. So a Shutdown that waits for Listen finds the monitor and the re-opener already released, and its own releases are no-ops.

Also: golangci-lint v2.13 (the repo's config) 0 issues for GOOS=linux; GOOS=linux GOARCH=amd64/arm64 go vet and test-binary builds clean (lint/); on GOOS=darwin the linter reports the same two internal/deferlinger unused fields as on main. CI on fb52752: run 36341288144, every job green on the first attempt. amd64 on this laptop is emulated (--platform linux/amd64, 04-amd64/): go vet clean and the six new tests pass (6/6, 25 subtests).

Behaviour change

breaking label (added in round 2, as on #692 and #746: behaviour, not API). A failed StartWithListener / StartWithListenerAndContext now closes the listener, as net/http's Server.Serve does ("Serve always returns a non-nil error and closes l"). The one pattern this changes: a caller that, after a failed start, hands the same listener to a second Server, for example falling back from IOUring (whose constructor fails with "io_uring not available on this system") to Epoll. That second start now fails too. Such a caller should let the default adaptive engine choose, or keep a duplicate of the listener ((*net.TCPListener).File) to retry with. No caller in this repository does this: of the call sites git grep -n StartWithListener -- '*.go' lists outside server.go, every one binds its listener just before the call, and on a start error the test skips, fails or returns.

Follow-ups

The review's minor and nit findings not fixed here are in #778: the /proc/stat test comments and the != vs > check, a dial assertion that cannot fail, and the uncommitted fd-delta probe.

Evidence

Scripts and logs: evidence/lanes-20260927/LIFECYCLE/ in the maintainer's probatorium evidence root (737/, ff-main/, 03-suites/, 04-amd64/, lint/, scripts/). Every number above is printed by a script there.

[coderabbitai]

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 configuration

Configuration used: Repository: goceleris/celeris/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: f63cb4b3-3b80-4abf-a178-a603023fe454

📥 Commits

Reviewing files that changed from the base of the PR and between a64f920 and fb52752.

📒 Files selected for processing (3)

• server.go
• start_failure_release_linux_test.go
• start_failure_release_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

---

📝 Walkthrough

Walkthrough

Startup failure paths now release supplied listeners and the CPU monitor. The cleanup returned by listenContext also stops the settle re-opener and closes the CPU monitor after Listen returns.

Changes

Startup resource cleanup

Layer / File(s)	Summary
Preparation failure ownership server.go, start_failure_release_test.go	Preparation failures close supplied listeners except when the error is ErrAlreadyStarted. Engine-creation failures close the CPU monitor. Tests cover both cleanup paths and listener ownership.
Listen return cleanup server.go, start_failure_release_test.go, start_failure_release_linux_test.go	The cleanup returned by listenContext cancels its context, stops the settle re-opener, and closes the CPU monitor. Tests cover Listen failures, shutdown before start, and Linux /proc/stat descriptor counts.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to fb527

The reviewed cleanup does not show an outstanding merge-blocking risk; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to fb527

Failed starts now release listeners that would otherwise remain reachable, and starts that end without Shutdown release server resources. The reviewed paths do not add a network entry point, but this is a public lifecycle change and security coverage is incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The directly affected exposure is the caller-supplied listener’s pending-connection backlog and per-server startup resources, rather than a newly added request route or accept path.

Trust Boundaries and Controls

• observed — The caller-to-server listener handoff distinguishes failed preparation from ErrAlreadyStarted. The former closes the listener; the latter leaves it with the caller, avoiding closure of a potentially active listener.

Resilience and Maintainability Implications

• observed — After Listen returns, cleanup stops the adaptive re-opener and closes the monitor even if Shutdown is not called, limiting resources left behind by a failed or interrupted start.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #737 requires cleanup on doPrepare failure. In server.go, prepareWithListener closes the supplied listener for all errors except ErrAlreadyStarted, and doPrepare closes the CPU monitor when crea…
Out of Scope Changes check	✅ Passed	The changed production code and tests address Start resource ownership and cleanup described by issue #737. The settle re-opener cleanup and post-Listen tests cover the same no-Shutdown resource-leak …
Title check	✅ Passed	The title uses the required Conventional Commit format, describes the resource-cleanup fix, and ends with issue reference #737.
Description check	✅ Passed	The description directly explains the listener, CPU monitor, and settle re-opener cleanup, including behavior changes and test coverage.

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[codspeed]

Merging this PR will improve performance by 13.01%

⚡ 13 improved benchmarks
✅ 52 untouched benchmarks
⏩ 5 skipped benchmarks¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	BenchmarkChainDeepParallel	2.6 µs	2 µs	+28%
⚡	BenchmarkChainMinimalAPI	3.1 µs	2.7 µs	+15.25%
⚡	BenchmarkChainPprofPassthrough	3.2 µs	2.8 µs	+14.29%
⚡	BenchmarkChainFullAPI	3.1 µs	2.7 µs	+12.72%
⚡	BenchmarkChainStaticPassthrough	3.2 µs	2.8 µs	+12.34%
⚡	BenchmarkChainBaseline	2.3 µs	2.1 µs	+12.21%
⚡	BenchmarkChainDeep	4.3 µs	3.8 µs	+11.95%
⚡	BenchmarkChainETagHit	1.7 µs	1.5 µs	+11.02%
⚡	BenchmarkChainStaticServe	2.9 µs	2.6 µs	+10.7%
⚡	BenchmarkChainFullAPI304	3 µs	2.7 µs	+10.62%
⚡	BenchmarkChainSwaggerPassthrough	3.1 µs	2.8 µs	+10.59%
⚡	BenchmarkChainRewritePassthrough	2.6 µs	2.4 µs	+10.55%
⚡	BenchmarkChainStaticFile304	3 µs	2.7 µs	+10.04%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing fix/celeris-737-failed-start-release (3d29a49) with main (698bed6)²}

Footnotes

1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

2. No successful run was found on main (5936dd8) during the generation of this report, so 698bed6 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[FumingPower3925]

Round 2: no code change. The review had no blocking finding, and the head stays fb52752. Added the breaking label. The body now says what attributes the retime592 failure, and has the root suites for a merge with #746's round-2 head 3d2ab72 and main 9b670b8 (383/0/1 in both shapes, no regressions). The minors and nits are in #778.