[FumingPower3925]

Flakes lane, package G: the eleven test/server starters that still picked a port, closed it and called Start(). Refs #1104

Method: commit 1 forces each starter onto one picked port and fails; commits 2 to 4 move seven starters onto startPicked1104 (#1135), commit 5 corrects what the PR claimed about native engines, commits 6, 7 and 8 move four more. One commit per family so each can be dropped, with these dependencies: commit 2 (family 1) defines startPickedPing1104 and ping1104, which commits 3 and 4 use, so drop 3 or 4 alone, or 2 together with them; and commit 6 adds startPickedCtx1104 and its two rows to the commit-1 test and needs commit 5 for the time import of that test file (drop 5 and 6 together, or neither). Tests only.

What the hand-off guarantees: the std engine only. startPicked1104 hands the picked listener to the server. Only std serves on that listener, so only on std is the port never free between the pick and the bind. epoll, io_uring and adaptive close the supplied listener inside Listen and rebind its address with SO_REUSEPORT; there the hand-off shortens the window to the close and the rebind, and two native servers that both win the port both bind it, with no error, and share it (so the address-in-use retry never fires). I measured it with the forced-port test on epoll: two servers ended on one port in 2 of 630 subtests in one run and in some of another run of 630 (not counted), and in none of three other runs (105, 630, 630); io_uring: 1 of 630 in two of three runs of 630; the review of this PR saw epoll 3 of 105, io_uring 2 of 210, adaptive 0 of 210. The remaining window is a few microseconds inside Listen instead of the milliseconds of a server construction, and I did not close it (an in-process lock around pick-to-ready would have to be taken by every picker of the package, net.Listen(":0") calls included, which is not a few lines). The forced-port test is therefore a std test, and its comment and startPicked1104's say so.

The hole

#1135 closed it for the SSE 855, h2upgrade-matrix and start-shutdown starters: a starter that picks a free port, closes it and has Start bind it can be given a port another test of the process picked at the same moment; one server binds it, the other's Start fails with address already in use, and the loser's readiness probe (GET /ping) is answered by the WINNER, so the starter reports success and the failure surfaces only later (the CI log of TestStalledSSEEndsWithShutdown855/std/h2c/budget has the two servers starting on 127.0.0.1:38797 0.5 ms apart). Eleven more starters had the same shape and the same probe; none has failed on it so far.

Starters moved

family	starter	file
1	startServerConfig761 (used by most of the package's tests)	large_response_linux_test.go
2	startServerConfig421	auto_head_options_engines_421_linux_test.go
2	startGlobalChainServer836	h2_stream_context_836_linux_test.go
3	bodyServer889	bodylimit_default_889_linux_test.go
3	start889	chunked_promotion_889_linux_test.go
3	startServerSrv870 (behind startServer870)	detect_split_first_segment_870_linux_test.go
3	startServer964	h2upgrade_disabled_964_linux_test.go
4	startServer760 (behind startDrainServer806; the 760 and 806 drain tests, adaptive_iouring_busy_poll_811)	shutdown_send_drain_linux_test.go
4	startH2PoolServer759 (the h2 pool and 884 gate tests)	shutdown_h2_pool_linux_test.go
5	runStdCancelBudgetCase	std_cancel_shutdown_timeout_test.go
6	TestServerAdaptiveStartsOnEpollWhenIOUringRefused886	iouring_async_cancel_refusal_886_linux_test.go

Families 1 to 3 go through startPickedPing1104 (new, a thin wrapper of startPicked1104 for the /ping-readiness starters, with the Shutdown-and-wait cleanup they all had) and ping1104 (registers /ping; each starter keeps its own route order, Use before routes where it needs it). The listener is handed to the server (StartWithListener; see the std-only note above). The ring-ENOMEM retry is startRetrying1104's; it also retries a taken port, ten times, which the old loops of these starters did not (they retried a ring ENOMEM only).

Family 4 stops its server by cancelling the context it starts under, so it goes through startPickedCtx1104, which calls StartWithListenerAndContext: that and StartWithContext both end in listenUntilCancelled (server.go), the same shutdown path. runStdCancelBudgetCase (std) keeps its listener and calls StartWithListenerAndContext directly.

Found in the review of this PR (picking starters with a /ping probe); the 886 test is moved, the drain-order starter is not:

• the drain-order starter (shutdown_drain_order_linux_test.go, runDrainOrderCase): its three modes are the entry points under test (StartWithContext with a direct Shutdown, StartWithContext with a cancel, StartWithListener), and its per-try goroutine records the order Start returns in; moving the two StartWithContext modes onto a listener entry point would change what they test. The StartWithListener mode already keeps its listener.
• (moved after all) TestServerAdaptiveStartsOnEpollWhenIOUringRefused886: in commit 8 (adaptive hands the supplied listener to the epoll fallback when the io_uring start engine is refused before Listen, so the test still tests the fallback; -race -count=10: 10 of 10 pass, no skip). Like every native arm it stays exposed to the window inside Listen (see above).
• server_sync_goexit_848's two Start() sites, below.
  These are the picking starters I found by grep and by the review; a starter that picks in a way neither finds may still exist.

Not moved: server_sync_goexit_848's two Start() sites (the brief listed :491). They are deliberate: setup-failure starts a server on a port a plain listener HOLDS (the held listener is the premise, so there is no pick-close-bind race), and lost-listener picks a port, closes it and takes it back with a plain listener while the server is paused, on a fixed Addr. Neither probes /ping. Converting them would remove what the tests are about.

Evidence

TestStartersForcedOntoOnePortEndOnPortsOfTheirOwn1104 (commit 1; commit 6 adds the rows of startServer760 and startH2PoolServer759; runStdCancelBudgetCase is a whole test of its own and has none) starts two of each starter at once on the same picked port (the hook listenForStart1104: the first picker is given the port, a picker that finds it held gets a fresh one), three rounds per starter, std engine. The two starts run as parallel subtests (the starters call t.Fatal, which must end only their own subtest), each held up until the other is up. golang:1.27.3, --cpus 2, 8 MiB memlock, -race -count=5:

	round subtests that end with both starters on one port
commit 1 with this round's test file (the seven starters pick through the hook but still close the port and call Start)	81 to 100 of 105 FAIL, depending on the run (94 and 100 in my two, 81 in the review's; the passes are the interleavings where the loser's Start error arrives before its probe succeeds)
this PR (nine starters in the test)	0 of 135 fail (an earlier run of the seven, before the subtests were held up for each other, had one failure that was the test's own artifact: the first server stopped with its subtest and the second was legitimately given its port; the barrier fixed it)

Family 4 has no failing-first. The startServer760 and startH2PoolServer759 rows pass on the old code too: those starters picked with net.Listen directly and never consulted the forced-port hook (commit 1 wired the hook into the seven starters only), so the two rows are green before and after. For these two the PR is the conversion alone, covered by the existing tests of those files (below); the rows run through listenForStart1104 now, but a later revert of the starters to a plain pick would leave them green, so they do not guard it. runStdCancelBudgetCase has no row at all.

The pair needs two parallel test slots (-parallel of at least 2): with -parallel 1 each round waits 30 s for the other subtest and then fails with a clear message. No CI step uses -parallel.

Touched files, -race -count=2, same container shape (8 MiB memlock): all 16 top-level tests of shutdown_send_drain, shutdown_h2_pool, std_cancel_shutdown_timeout, adaptive_iouring_busy_poll_811, shutdown_iouring_gate_884 and the forced-port test pass, no skip, 210 s.

Whole package once, go test -race -count=1 -timeout 50m ./test/server, same container shape: ok in 691 s; the only skips are TestZonedPeerAddressIsStdsOnNativeEngines890 and TestZonedListenAddressServesOnEveryEngine885 (no link-local address in the container; both are in the CI skip allow-list and untouched). No test was added that skips. That whole-package run was on the seven-starter head (commit 4); this round's commits were run as listed above. go vet is clean; golangci-lint --new-from-rev=origin/main is clean under the CI pin (v2.14); a local v2.15 also flags case <-done: / case <-srv.startDone: on a channel of error (errcheck) at the cleanups, the pattern main already has in h2upgrade_matrix_889.

Differences to know about: chunked_promotion's starter retried a ring ENOMEM with strings.Contains(..., "cannot allocate memory"); it now uses isRingENOMEM, which also takes io_uring not available on this system (the probe's own ring met the same ENOMEM), so on a host with no io_uring its io_uring arms wait the 30 s window before failing instead of failing at once (the other starters already behaved that way).

Release note

None (tests only).

Findings, not fixed here

• waitEngineBusy1104 (tests: test/server start retry takes a fresh port; the shutdown gate test waits for the engine (Refs #1104) #1135) stays: it is still needed for conns in the kernel accept queue when the listener is closed at cancel (the kernel resets them), whatever the gate fix does.
• The native engines (epoll, io_uring, adaptive) close a supplied listener inside Listen and rebind with SO_REUSEPORT, with no owner check beyond the kernel's same-UID rule, so two native servers can end on one port without an error. A fix would be in the engines or in a per-server token in /ping (as start_shutdown_return has); not done here.
• test/integration has its own starters (tests: test/integration starters retry the io_uring ring-setup ENOMEM (Refs #1104) #1132) with a different shape.

[coderabbitai]

Caution

Review failed

The pull request is closed.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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!