Skip to content

fix(server): Shutdown runs the OnShutdown hooks, and returns, only after the drain on every engine (#703) - #746

Merged
FumingPower3925 merged 5 commits into
mainfrom
fix/celeris-703-shutdown-drain-order
Sep 28, 2026
Merged

FumingPower3925 merged 5 commits into
mainfrom
fix/celeris-703-shutdown-drain-order

Conversation

@FumingPower3925

@FumingPower3925 FumingPower3925 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #703

What was wrong

Server.Shutdown's godoc says the OnShutdown hooks fire after the engine "stops accepting new connections and drains in-flight requests". On epoll and io_uring they did not. Their Engine.Shutdown is a no-op, and their drain runs in Listen once its context is cancelled; Server.Shutdown cancelled that context and went straight on to the CPU monitor and the hooks, and returned. So on the two default Linux engines:

std and adaptive kept the order: their Engine.Shutdown is the drain.

The fix

One wait, in Server.shutdown, after cancelListen and before the CPU monitor and the hooks: until the engine's Listen has returned, bounded by ctx. On epoll and io_uring the drain runs in Listen: Loop.shutdown and Worker.shutdown close their connections and join async dispatch before Listen returns, so every handler the workers run has returned by then. On std and adaptive Engine.Shutdown has already drained (std's Listen returned when that drain began; adaptive's Engine.Shutdown waits for its own Listen). What the drain does not cover is in round 2 below: two kinds of HTTP/2 stream (#759), and epoll's unflushed close (#760).

  • Every Start* entry point now runs Listen through one helper, listen, which closes a listenDone channel when Listen returns. publishEngine makes that channel together with the engine, so a Shutdown that loads a non-nil engine always finds it (every published engine is followed by exactly one listen). It is written before engineRef.Store and read only after a non-nil engineRef.Load, so the atomic orders it.
  • fix(server): shut down on every context cancel, and return only after it (celeris#673) #692's Start*Context watcher already joined on a local listenDone. It now wakes on this same channel, as the issue asked, so there is no second signal.
  • If ctx is done first, the hooks still run, with that ctx, and Shutdown returns ctx's error, as std's http.Server.Shutdown does when its drain outlives the deadline.
  • Round 2: listen then waits for the first direct Shutdown, so the Start* call it stopped returns after its hooks (below).
  • Doc comments: Shutdown (the wait, the error, what the drain covers, why a handler must not call Shutdown and wait for it), Config.ShutdownTimeout (one deadline for the drain and then the hooks), the Start* and OnShutdown godocs (the Start* call returns after the hooks), and the epoll/io_uring Engine.Shutdown notes.

Not on the request path: only the start and shutdown paths change.

Why the wait for Listen cannot deadlock

The wait graph: shutdown waits for listenDone. listenDone is closed by the Start* goroutine right after eng.Listen returns. eng.Listen returns once its context is cancelled and its workers have drained, and cancelListen cancelled that context before the wait. Listen waits for nothing Shutdown holds or does later. No lock is held across the wait: lifecycleMu and cpuMonMu are released before it.

  • The cancel path runs shutdown on the Start*Context watcher. The watcher waits for Listen, never for the Start call. The Start call waits for the watcher only after Listen has returned (err := s.listen(...); <-watcherDone). Mutant M3 below makes the wait be on the Start call's return instead, and the tests catch it: every cancel and direct-Shutdown case on the context path fails at its 10 s cap, far inside the 30 s budget it would otherwise have waited out.
  • A handler that calls Shutdown and waits for it waits for its own request, until ctx is done. That is also true of std today, and of net/http. The godoc says to call it from another goroutine.

The round-2 wait (Start waits for the direct Shutdown) has its own deadlock check in the round-2 section.

Round 2 (head 3d2ab72)

The review of 76c205c found two blocking defects. Both are fixed here.

1. Start returned before the hooks of the Shutdown that stopped it. Once Shutdown waited for Listen, closing listenDone woke the Start call and Shutdown's wait at the same moment, and the hooks ran after Start had returned. So a main shaped like net/http's lost its hooks on every engine:

go func() { <-sig; s.Shutdown(ctx) }()
s.Start() // then exit

That includes the session write-behind flush the docs put in OnShutdown. Measured in a child process with that main, a 200 ms hook that writes a marker file and one request held 500 ms, 5 runs per engine (round2/738/run-mainexit.sh, one container):

engine main 5936dd8 76c205c (round 1) 3d2ab72 (this head)
std hook lost 5/5 5/5 0/5
epoll 0/5 (the hooks ran at t=0) 5/5 0/5
io_uring 0/5 (the hooks ran at t=0) 5/5 0/5
adaptive 5/5 5/5 0/5

Fix: the first direct Shutdown publishes a directShutdownDone channel under lifecycleMu before it does anything else, and closes it when it returns. listen (the one path of every Start* entry point) waits for that channel after Listen has returned and listenDone is closed. A Start* call stopped by a direct Shutdown therefore returns only after that Shutdown, hooks included, on every engine. This is the contract the Start*Context calls already had for a cancel (#692). The Start, StartWithListener, StartWithContext, StartWithListenerAndContext and OnShutdown godocs say so. The last of these says that a hook must not wait for the Start* call to return.

It also changes std: Start used to return as soon as Shutdown closed the listener (net/http's ListenAndServe does too). It now returns when that Shutdown returns.

Deadlock check for the new wait. Lock order: lifecycleMu is taken only for the read or write of the channel and is never held across a wait. The waits are:

  • Shutdown waits for listenDone, bounded by its ctx.
  • listen closes listenDone (a deferred close) before it waits for directShutdownDone (the earlier defer, so it runs later).
  • The owning Shutdown closes directShutdownDone when it returns.

The owning Shutdown never waits for anything that waits on listen, so there is no cycle. A Shutdown that finds the watcher's claim waits on watcherShutdown and owns no directShutdownDone. The watcher waits for Listen, never for the Start call. The one cycle left is user-made, a hook that waits for Start to return, and the godoc forbids it. That cycle has the same bound as before: the hook's ctx.

Mutant R2 below puts the wait before the close. The new test then fails at its 10 s bound instead of hanging: the direct Shutdown did not return within 10s of Listen's release: it and the Start call wait on each other.

2. The drain does not cover every HTTP/2 stream. The drain does not wait for two kinds of stream:

  • an HTTP/2 stream on an async route on epoll, io_uring and adaptive, which runs on the shared H2 worker pool;
  • any h2c stream on std, which net/http stops tracking once h2c.NewHandler hijacks the connection.

For the first kind, the hooks run while the handler is still running and the response is lost. Filed as #759, with a repro on main 698bed6 (a handler held 400 ms on a fixed timer, independent of the hook). Joining the pool handlers is the H2 half of graceful shutdown (GOAWAY, then keep the connection's write path running until the streams finish). A plain join would deadlock on a handler blocked on flow control, so that fix is not in this PR. Instead this PR:

The minor findings (the listenDone comment was false for std; epoll's drain is "the handlers returned") are fixed in the comments changed here. The rest are in #777. The out-of-scope 4 MiB response stall is filed as #761.

Behaviour change

breaking label, as on #692 (behaviour, not API):

  • epoll and io_uring: a direct Shutdown now returns after the drain, not at once, and returns ctx's error if the drain outlives ctx. adaptive now returns that error too; its Engine.Shutdown used to swallow it.
  • epoll and io_uring: the hooks run after the drain. A hook that flips a readiness probe to steer a load balancer during the drain (docs deployment.md suggested this on epoll/io_uring) now runs too late on every engine, as it already did on std and adaptive. docs#77 moves the flip to the signal handler.
  • Every engine: a Start* call stopped by a direct Shutdown returns only after that Shutdown has returned, hooks included. On std, Start used to return as soon as Shutdown closed the listener, as ListenAndServe does. A hook that waits for the Start* call to return now waits for itself until its ctx is done. OnShutdown's godoc says not to do that, as it already said for a cancel.
  • A handler that calls Shutdown synchronously now waits until ctx is done on every engine, as it already did on std.

No non-test code in this repository calls Server.Shutdown: besides server.go, git grep -n '\.Shutdown(' -- '*.go' ':!*_test.go' lists only engine-internal calls, socket shutdown(2) calls and validation/endpoint.go's own http.Server. The test suites that start and shut down servers pass unchanged (below).

Tests

  • TestShutdownHooksRunAfterTheDrain (Linux, shutdown_drain_order_linux_test.go): the issue's experiment as a real test. It runs real engines (std, epoll, epoll async, io_uring, io_uring async, adaptive; default worker count) in three modes: a direct Shutdown on a StartWithContext server, a cancel of that context, and a direct Shutdown on a StartWithListener server, which runs Listen outside the watcher. Round 2 adds the same modes over h2c (prior knowledge) on the five native configs, on a sync route: 33 cases. One request is held in its handler. Each case asks four things. Had the handler finished when the first hook started? Had the hook started before the call that shut the server down returned? Did the client get the whole response? And (round 2) did the Start call return only after the hook had returned?
    • The order is forced, not raced. The handler is released as soon as a hook starts or the call returns, or after 300 ms if neither does. The hook then holds until the Start call returns, or for 200 ms. So a Shutdown that does not wait always reaches its hooks with the handler held, and a Start that does not wait always returns while the hook is held.
    • Every budget is 30 s and each call must return within 10 s of the release, so a wait that deadlocks until its budget also fails.
    • It fails (never skips) if an engine cannot start.
  • TestStartReturnsOnlyAfterADirectShutdownReturns (all OSes, round 2): the Start-return property on a fake engine whose Listen outlives its cancel until released, for Start and for StartWithContext with a direct Shutdown. The order is forced the same way. It is also the deadlock check for the round-2 wait.
  • TestShutdownRunsHooksOnlyAfterListenReturns and TestShutdownWaitForListenIsBoundedByCtx (all OSes): the round-1 property on the same fake engine, for Start, StartWithContext + Shutdown and StartWithContext + cancel. They also check that the wait is bounded by ctx, that the hooks still run with it, and that Shutdown returns context.DeadlineExceeded.
  • Two fix(server): shut down on every context cancel, and return only after it (celeris#673) #692 tests are adapted, not weakened. Their direct Shutdown now waits for the fake engine's Listen, so it runs on its own goroutine. TestStartContextWatcherDoesNotRepeatADirectShutdown also asserts that the hook has not run while Listen is still tearing down. That assertion races under the negative control; the deterministic detector is TestShutdownRunsHooksOnlyAfterListenReturns (Follow-ups from #746: the adapted #692 assertion is not forced, the ctx-bound test's unbounded-wait failure mode, CI runs the drain-order test in one shape #777).
  • TestShutdownBeforeTheWatcherLoadsRunsHooksOnce: the Follow-up from #692: a Shutdown that races Start*Context before the callsBefore load can run OnShutdown hooks twice #728 guard (below).

Failing-first (linux/arm64 Docker, --cpus 4, -race -v; counts are --- PASS/FAIL/SKIP lines only)

TestShutdownHooksRunAfterTheDrain's final text, overlaid on main 698bed6 (it uses only the public API; round2/703/ff-main.sh): 30 of 33 cases FAIL, in both shapes: the CI shape (8 MiB memlock, one io_uring worker) and unconstrained memlock (4 io_uring workers).

No SKIP line.

Controls (round2/703/controls-docker.sh)

One container per shape, each arm a copy of the head's tree with one edit to server.go. The generators are round2/703/make-mutants.py (R) and round 1's 703/mutants/make-mutants.py (M), applied to this head; both exit if an anchor drifts. The fake-engine tests also ran natively on darwin (round2/703/darwin-unit.sh).

arm fake-engine tests (7) TestShutdownHooksRunAfterTheDrain (33 cases)
head 3d2ab72 CI shape -count=3: 21/21 PASS. darwin -count=10: 70/70 PASS CI shape -count=3: 99/99 PASS. unconstrained -count=2: 66/66 PASS
R1, the round-2 negative control: listen does not wait for the direct Shutdown (the round-1 behaviour) darwin 3/3 runs FAIL TestStartReturnsOnlyAfterADirectShutdownReturns (both subtests), all else PASS 22 FAIL in both shapes: every direct-Shutdown case on all 11 configs, the Start call returned ... while the OnShutdown hook was still running. The 11 cancel cases pass.
R2: listen waits for the Shutdown before it closes listenDone (the deadlock) darwin: 4 tests FAIL at their 10 s bounds (the direct Shutdown did not return within 10s of Listen's release: it and the Start call wait on each other) not run (the fake-engine test is the deterministic check)
R3: the done channel closed when Shutdown starts, not when it returns darwin 3/3 runs FAIL the same test 22 FAIL, same 22
M1, the round-1 negative control: the wait for Listen deleted 3 FAIL (order test, ctx-bound test, adapted #692 test) 24 FAIL: every epoll/io_uring case, HTTP/1.1 and h2c, all 3 modes. std and adaptive pass.
M2: the wait for Listen moved after the hooks 3 FAIL, the same three 24 FAIL, the same 24
M3: the cancel's Shutdown waits for the Start call 3 FAIL (the order test, TestStartReturnsOnlyAfterADirectShutdownReturns, #692's TestADirectShutdownAfterTheWatchersDoesNotRepeatIt) 22 FAIL: every Shutdown and cancel case on the context path, each at the 10 s cap. The 11 StartWithListener cases pass.

No SKIP line in any arm. With the fix, all 165 engine cases (99 CI shape + 66 unconstrained) ran in this order: handler returned, hook started, hook returned, then Shutdown and Start returned. The client got 200 "done" in every case. On epoll, io_uring and adaptive the hook started 0.0-1.5 ms after the handler returned, which it did at 300-310 ms. On std the gap was 227-281 ms, from net/http's idle-connection poll. No start needed the ENOMEM retry.

What the test does not cover, measured on this head by round2/repro/run-repro.sh (not committed), is in #759: on epoll, io_uring and adaptive, an h2c request on an .Async() route got unexpected EOF with the hook before the handler, in 6/6 cases; on std the hook came before the h2c handler in 4/4 cases. The same script on this head gives the sync-route h2c cases on the native engines handler-then-hook with 200 "done" (6/6).

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

Root package. The arms are main 9b670b8 (main moved during the run; #765 landed), this head 3d2ab72, and a local merge of this head, #747's head fb52752 and main 9b670b8. All three merge cleanly.

shape main 9b670b8 this PR merged with #747 and main
CI (8 MiB memlock, one io_uring worker) 372 PASS, 0 FAIL, 1 SKIP (+98/0/3 subtests) 375/0/1 (+126/0/3) 383/0/1 (+161/0/3)
unconstrained memlock (4 io_uring workers) 372/0/1 (+101/0/0) 375/0/1 (+129/0/0) 383/0/1 (+164/0/0)

No test goes from PASS to anything else (round2/scripts/suite-compare2.py). Every test new in an arm passes. The 12 main tests absent from this PR's arm are the two TestAdapt* tests #736 added after this branch's base; they pass in the merged arm. 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/... (CI shape), main 9b670b8 vs this head: 1779/0/5 vs 1774/0/5 top-level, and 748 vs 723 subtests PASS. No regressions. The difference is the five tests #734 and #736 added on main after this branch's base. The five skips are the same (allocation tests under -race, and TestMeasureWedgeRate). The suites that start and shut down servers while clients hold connections (websocket, sse, static, session) are unchanged. That includes Start* now waiting for the Shutdown that stopped it.

Also:

  • golangci-lint v2.13 (the repo's config): 0 issues for GOOS=linux. On GOOS=darwin it reports the same two internal/deferlinger unused fields as on main.
  • gofmt clean except test/benchcmp_ws/bench_test.go, which is the same on main.
  • GOOS=linux GOARCH=amd64/arm64 go vet and test-binary builds are clean (lint/r2-703-3d2ab72.log).
  • CI on 3d2ab72: run 36349652266, all 11 jobs green on the first attempt; the root package was ok in 71.9 s at 8 MiB memlock.

#728

#728 (a Shutdown racing Start*Context before the watcher's callsBefore load can run the hooks twice) does not hold on main. The snapshot it names existed at #692's review head 5a05c2e and was replaced before the merge by the directShutdown claim (a20af40). TestShutdownBeforeTheWatcherLoadsRunsHooksOnce forces the #728 order, and this PR adds it as a guard, since it changes the same path. Measured with -race -count=20 per tree (728/verify.sh): the hook ran twice in 20/20 runs at 5a05c2e and at f17c04a, and once in 20/20 at a20af40 and at main dccb839. At this head it passes 10/10 on darwin and 3/3 in the CI shape (the controls above).

Docs

goceleris/docs#77 (round 2 at 5b463a1) makes the graceful-shutdown pages state these rules and their HTTP/2 and epoll exceptions, and settles #738, which is to be closed by hand once both merge. It should merge after this PR. Measuring what the pages should say about the deadline found #753: on std, a cancel's shutdown does not keep to ShutdownTimeout. That is pre-existing, the same on main, and not changed here.

Evidence

Scripts and logs are in the maintainer's probatorium evidence root, evidence/lanes-20260927/LIFECYCLE/:

Every number above is printed by a script there. The cluster row (_queue/cluster.tsv, lane LIFECYCLE) is re-pinned to 3d2ab72: this test on bare metal with many io_uring workers, both arches.

…ter the drain on every engine (celeris#703)

Server.Shutdown ran every hook and returned right after cancelling the listen
context, so on epoll and io_uring, whose Engine.Shutdown is a no-op and whose
drain runs in Listen, the hooks ran while requests were still being handled,
and a direct Shutdown returned before the drain.

Every Start* entry point now runs Listen through one helper that closes a
listenDone channel, published with the engine, when Listen returns; Shutdown
waits for it, bounded by ctx, before it closes the CPU monitor and runs the
hooks, and returns ctx's error if ctx is done first. The Start*Context
watcher (#692) wakes on the same channel.

Also pins the window celeris#728 named (a Shutdown before the watcher's view,
then a cancel, must run the hooks once).
…e native engines' Shutdown notes the wait (celeris#703)

Comment-only.
…M in the celeris#703 drain-order test

At CI's 8 MiB memlock the kernel charges ring memory per UID, so a start
right after the previous case, or while another test binary holds rings,
can fail with nothing leaked; the first failing-first run on main lost two
io_uring cases that way. Same convention as startC714DetachServer: a new
server, for up to 10 s, then fail (never skip).
@FumingPower3925 FumingPower3925 added this to the v1.6.0 milestone Sep 27, 2026
@FumingPower3925 FumingPower3925 added bug Something isn't working area/engine Engine interface or implementation platform/linux Linux-specific (io_uring, epoll) engine/epoll Epoll engine specifics engine/iouring io_uring engine specifics breaking Breaking change (called out in release notes) labels Sep 27, 2026
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: 3fcefff7-be56-478a-9e8a-ecb59021f7c8

📥 Commits

Reviewing files that changed from the base of the PR and between 3d2ab72 and 859d843.

📒 Files selected for processing (1)
  • server.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

Server.Shutdown now waits for Listen to return before running shutdown hooks, subject to the shutdown context. Start calls wait for direct shutdown completion. Tests cover drain ordering, context deadlines, and shutdown interleavings.

Changes

Shutdown lifecycle

Layer / File(s) Summary
Listen completion coordination
server.go
A shared listen helper tracks Listen completion and direct shutdown completion. Start entry points use this helper.
Shutdown wait and hook ordering
server.go, config.go, engine/epoll/engine.go, engine/iouring/engine.go
Shutdown waits for Listen to return, bounded by its context, before running hooks. Documentation describes the ordering and timeout behavior.
Engine drain ordering tests
shutdown_drain_order_linux_test.go
Linux tests check handler completion, response delivery, hook ordering, and Start completion across engine and shutdown modes.
Shutdown interleaving tests
shutdown_waits_listen_test.go, shutdown_race_hooks_once_test.go, start_context_shutdown_test.go
Tests cover deadline-bounded waits, Start completion after direct shutdown, and direct-shutdown and cancellation-watcher races.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 859d8

The reviewed shutdown ordering has no identified issue that needs resolution before merge; normal checks still apply.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 859d8

The change improves the ordering of request draining and shutdown hooks without establishing a new request-controlled shutdown path. Lifecycle and engine coverage remains incomplete, so the assessment is not minimal risk.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected exposure is the application's server lifecycle: request handlers and resources released by shutdown hooks can overlap if draining has not completed. The new wait reduces that overlap while remaining context-bounded.

Trust Boundaries and Controls

  • inferred — StartWithListener receives an application-supplied listener, while Shutdown is a separate public lifecycle call. The inspected changed flow does not establish a request-to-shutdown authority transition.

Resilience and Maintainability Implications

  • observed — The wait for listener completion returns on context cancellation; shutdown then proceeds to cleanup and hooks. Consequently, the ordering guarantee is bounded by the caller's shutdown deadline.

Hardening Proposals

  • proposed — Consider making direct shutdown single-flight if applications rely on shutdown hooks running at most once under repeated or concurrent calls.
🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title uses valid Conventional Commit syntax and accurately describes the shutdown ordering change. It references issue #703, but it does not use the required repository-qualified format such as `(… Change the suffix from (#703) to (celeris#703). Ensure the title ends with the required issue reference format fix(server): Shutdown runs the OnShutdown hooks, and returns, only after the drain on every engine (celeris#703).
Out of Scope Changes check ⚠️ Warning The incremental change adds unrelated startup-failure cleanup in server.go. prepareWithListener now closes listeners on startup errors, doPrepare closes the CPU monitor after engine creation err… Remove the celeris#737 listener, CPU-monitor, and settle-reopener cleanup changes from this pull request, or move them to a separate pull request with the corresponding linked issue.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the shutdown-ordering defect, implementation, scope, tests, and linked issues. It is clearly related to the changeset.
Linked Issues check ✅ Passed Issue #703 requires Server.Shutdown to cancel listening, wait for the published Listen call with the shutdown context, then run hooks and return. server.go provides listenDone coordination and…
Full details: Title check

Explanation

The title uses valid Conventional Commit syntax and accurately describes the shutdown ordering change. It references issue #703, but it does not use the required repository-qualified format such as (celeris#703).

Full details: Out of Scope Changes check

Explanation

The incremental change adds unrelated startup-failure cleanup in server.go. prepareWithListener now closes listeners on startup errors, doPrepare closes the CPU monitor after engine creation errors, and listenContext releases the settle re-opener and CPU monitor. The code labels these changes celeris#737; Issue #703 concerns shutdown drain ordering and does not require them.

  • Fix all pre-merge checks with AI

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

@codecov

codecov Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…fter that Shutdown, hooks included; scope the drain's godoc (celeris#703)

Round 2 of the #746 review.

Once Shutdown waited for Listen, closing listenDone woke the Start call
and Shutdown's wait at the same moment, and the hooks ran after Start had
returned. A main that exits when Start returns lost its hooks on every
engine: epoll and io_uring had run them at t=0 before, and on std Start
returned when the drain began. listen now waits for the first direct
Shutdown after it closes listenDone, which is all Shutdown waits for, so
the two never wait on each other. That is the same contract the
Start*Context calls already had for a cancel.

The drain's godoc and comments now say what the drain covers. They no
longer claim every HTTP/2 stream: std's h2c streams and HTTP/2 streams
on async routes are not waited for (celeris#759). They also say that
epoll closes connections without flushing (celeris#760). The listenDone
comment no longer says std's Listen returns after the drain.

Tests: TestStartReturnsOnlyAfterADirectShutdownReturns (fake engine,
forced order, also the deadlock check). TestShutdownHooksRunAfterTheDrain
now also asserts that Start returns after the hook, and runs h2c cases
on the native engines' sync routes.
FumingPower3925 added a commit that referenced this pull request Sep 27, 2026
…, the CPU monitor and the settle re-opener (#737) (#747)

Bug: a Start that failed before its engine ran left the caller's listener bound (handshakes into a backlog nothing accepts) and, on a createEngine failure, the CPU monitor's /proc/stat fd open; a Start whose Listen failed or ran after Shutdown leaked the monitor fd and the settle re-opener goroutine (+20 fds per 20 starts on main dccb839).
Change (server.go): prepareWithListener closes the supplied listener on a failed start except ErrAlreadyStarted; doPrepare closes the CPU monitor when createEngine fails; the func listenContext returns (deferred by every Start*) also stops the settle re-opener and closes the monitor. Breaking: a failed StartWithListener now closes the listener, as net/http's Serve does.
Verification: six new tests fail on main (5 FAIL, 23 of 25 subtests) and pass at head (-count=3, 27/75 PASS); revert arm 5 FAIL, and each of four partial mutants is caught only by its own tests. Root suite merged with #746: 380 PASS / 0 FAIL / 1 SKIP (CI shape). CI green on 3d29a49 (run 36358042411); CodeRabbit: no actionable comments.
Follow-ups: #778.
Fixes #737
@FumingPower3925

Copy link
Copy Markdown
Contributor Author

Round 2 pushed as 3d2ab72 (fast-forward from 76c205c). CI run 36349652266 is green. Both blocking findings are fixed. (1) A Start* call stopped by a direct Shutdown now returns only after that Shutdown, hooks included, on every engine. The main-exit hook loss goes from 5/5 to 0/5 on std, epoll, io_uring and adaptive. (2) The drain's godoc and claims are scoped to what it covers, the h2c sync-route cases are added to the test, and the uncovered HTTP/2 streams are filed as #759. The minors: #760 (epoll unflushed close) and #761 (4 MiB body dropped) are filed, and the rest are in #777. Every number in the body is re-measured on the final test text: failing-first on main, controls R1-R3 and M1-M3, and suites in both shapes. Details are in the body's Round 2 section.

@FumingPower3925
FumingPower3925 marked this pull request as ready for review September 28, 2026 03:15
@codspeed

codspeed Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 11.47%

⚡ 1 improved benchmark
✅ 53 untouched benchmarks
⏩ 16 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ BenchmarkContextQueryFirstParse 826 ns 741 ns +11.47%

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-703-shutdown-drain-order (859d843) with main (0cf0c52)2

Open in CodSpeed

Footnotes

  1. 16 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 (f0886ca) during the generation of this report, so 0cf0c52 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@FumingPower3925
FumingPower3925 merged commit dfd044f into main Sep 28, 2026
29 of 30 checks passed
@FumingPower3925
FumingPower3925 deleted the fix/celeris-703-shutdown-drain-order branch September 28, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/engine Engine interface or implementation breaking Breaking change (called out in release notes) bug Something isn't working engine/epoll Epoll engine specifics engine/iouring io_uring engine specifics platform/linux Linux-specific (io_uring, epoll)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Server.Shutdown runs OnShutdown hooks and returns before epoll/io_uring drain in-flight requests, breaking its documented order

1 participant