fix(std): a cancel of StartWithContext's context keeps Config.ShutdownTimeout (celeris#753) - #803
Merged
FumingPower3925 merged 2 commits intoSep 29, 2026
Conversation
FumingPower3925
added
bug
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe standard engine now uses one shared drain context for listener cancellation and shutdown calls. Shutdown deadlines bound the drain, and an expired caller context is returned as an error. Documentation and regression tests describe and check this behavior. ChangesBounded shutdown
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The shutdown change is mergeable after normal checks; no concrete outstanding issue was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The fix makes shutdown respect its timeout when listener cancellation starts the drain. Overlapping shutdown calls can also cause a shorter deadline to end a longer caller’s drain and advance cleanup while requests are still active. No new network or authorization entrypoint was identified. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Sep 28, 2026
FumingPower3925
added a commit
that referenced
this pull request
Sep 28, 2026
…ams on the shared worker pool, and std for its h2c streams (celeris#759) A stream on an async route (Route.Async, or a route AsyncHandlers has made async) runs its handler on the shared HTTP/2 worker pool, off the event loop, and its response comes back through the connection's write queue, which only the loop drains. At shutdown epoll and io_uring cancelled such streams (CloseH2) and closed their connections under the handlers: the client got unexpected EOF, and the OnShutdown hooks ran before the handlers had finished. On std, net/http hands an h2c connection over (hijack) and stops tracking it, so http.Server.Shutdown waited for no h2c stream: the hooks ran, and a direct Shutdown returned, while the handler was still running. epoll, io_uring (and so adaptive): the Processor counts its pool handlers (poolRunning: incremented before Submit, decremented after executeHandler has returned). Once its context is cancelled, a loop or worker sends every HTTP/2 connection GOAWAY(NO_ERROR, last client stream), so its client opens no new stream, and keeps turning, reading and writing as usual, until no HTTP/2 connection has a pool handler running or a response in its write queue, then shuts down as before (on io_uring the 250 ms send drain starts after). The wait ends at the deadline of the budget the last Engine.Shutdown handed over, never before 250 ms: io_uring's Engine.Shutdown now records its ctx, as epoll's does since celeris#760. std: Bridge counts the HTTP/2 requests in their handler, and the drain, after http.Server.Shutdown, waits for that count to reach zero, bounded by the drain's context (celeris#753). Tests: TestShutdownHooksRunAfterTheDrain gains the h2c cases on std and the h2c-async-route cases on every engine; TestShutdownSendsH2GoAwayThenFinishesStreams reads the frames of a raw h2c connection: GOAWAY first, then the held stream's response, then the close; TestShutdownH2PoolWaitIsBounded: a pool handler that does not return holds the shutdown for its budget, no longer. BenchmarkPoolDispatch and BenchmarkBridgeServeHTTP measure the two new counters. Stacked on #803 (celeris#753: std's drainCtx) and #807 (celeris#760: epoll's drain budget); merge after them. Fixes #759
FumingPower3925
added a commit
that referenced
this pull request
Sep 28, 2026
…ams on the shared worker pool, and std for its h2c streams (celeris#759) A stream on an async route (Route.Async, or a route AsyncHandlers has made async) runs its handler on the shared HTTP/2 worker pool, off the event loop, and its response comes back through the connection's write queue, which only the loop drains. At shutdown epoll and io_uring cancelled such streams (CloseH2) and closed their connections under the handlers: the client got unexpected EOF, and the OnShutdown hooks ran before the handlers had finished. On std, net/http hands an h2c connection over (hijack) and stops tracking it, so http.Server.Shutdown waited for no h2c stream: the hooks ran, and a direct Shutdown returned, while the handler was still running. epoll, io_uring (and so adaptive): the Processor counts its pool handlers (poolRunning: incremented before Submit, decremented after executeHandler has returned). Once its context is cancelled, a loop or worker sends every HTTP/2 connection GOAWAY(NO_ERROR, last client stream), so its client opens no new stream, and keeps turning, reading and writing as usual, until no HTTP/2 connection has a pool handler running or a response in its write queue, then shuts down as before (on io_uring the 250 ms send drain starts after). The wait ends at the deadline of the budget the last Engine.Shutdown handed over, never before 250 ms: io_uring's Engine.Shutdown now records its ctx, as epoll's does since celeris#760. std: Bridge counts the HTTP/2 requests in their handler, and the drain, after http.Server.Shutdown, waits for that count to reach zero, bounded by the drain's context (celeris#753). Tests: TestShutdownHooksRunAfterTheDrain gains the h2c cases on std and the h2c-async-route cases on every engine; TestShutdownSendsH2GoAwayThenFinishesStreams reads the frames of a raw h2c connection: GOAWAY first, then the held stream's response, then the close; TestShutdownH2PoolWaitIsBounded: a pool handler that does not return holds the shutdown for its budget, no longer. BenchmarkPoolDispatch and BenchmarkBridgeServeHTTP measure the two new counters. Stacked on #803 (celeris#753: std's drainCtx) and #807 (celeris#760: epoll's drain budget); merge after them. Fixes #759
…nTimeout (celeris#753) On std, StartWithContext derives Listen's context from the caller's, so a cancel reaches Listen before the watcher's Server.Shutdown, which carries Config.ShutdownTimeout, reaches Engine.Shutdown. Listen's cancel branch started the drain itself with context.Background(), and the drain is a sync.Once: the Shutdown with the budget waited in once.Do for that unbounded drain, and so did the OnShutdown hooks and the StartWithContext call, until the last handler returned. It then reported no error. The drain now runs under an engine-owned context, whichever call starts it, and every Shutdown(ctx) cancels that context when its ctx expires (with the #498 escalation, as before). A drain Listen started therefore ends at the budget of the Shutdown that follows, every Shutdown caller gets the drain's result, and a caller whose own ctx ran out gets that ctx's error (DeadlineExceeded), as a direct Shutdown did. The engine.Engine interface doc said an expired Shutdown ctx closes the remaining connections; no engine does, and it now says what they do. Tests: TestListenCancelDrainKeepsTheShutdownBudget (engine/std) forces the order (Listen's cancel first, Shutdown once the listener is closed) and TestStartWithContextCancelKeepsShutdownTimeoutOnStd (root) checks the hook and the Start call against the budget with the handler held. Fixes #753
FumingPower3925
force-pushed
the
fix/celeris-753-std-cancel-shutdown-timeout
branch
from
September 28, 2026 07:11
3d601ec to
325d83a
Compare
FumingPower3925
added a commit
that referenced
this pull request
Sep 28, 2026
FumingPower3925
added a commit
that referenced
this pull request
Sep 28, 2026
…ams on the shared worker pool, and std for its h2c streams (celeris#759) A stream on an async route (Route.Async, or a route AsyncHandlers has made async) runs its handler on the shared HTTP/2 worker pool, off the event loop, and its response comes back through the connection's write queue, which only the loop drains. At shutdown epoll and io_uring cancelled such streams (CloseH2) and closed their connections under the handlers: the client got unexpected EOF, and the OnShutdown hooks ran before the handlers had finished. On std, net/http hands an h2c connection over (hijack) and stops tracking it, so http.Server.Shutdown waited for no h2c stream: the hooks ran, and a direct Shutdown returned, while the handler was still running. epoll, io_uring (and so adaptive): the Processor counts its pool handlers (poolRunning: incremented before Submit, decremented after executeHandler has returned). Once its context is cancelled, a loop or worker sends every HTTP/2 connection GOAWAY(NO_ERROR, last client stream), so its client opens no new stream, and keeps turning, reading and writing as usual, until no HTTP/2 connection has a pool handler running or a response in its write queue, then shuts down as before (on io_uring the 250 ms send drain starts after). The wait ends at the deadline of the budget the last Engine.Shutdown handed over, never before 250 ms: io_uring's Engine.Shutdown now records its ctx, as epoll's does since celeris#760. std: Bridge counts the HTTP/2 requests in their handler, and the drain, after http.Server.Shutdown, waits for that count to reach zero, bounded by the drain's context (celeris#753). Tests: TestShutdownHooksRunAfterTheDrain gains the h2c cases on std and the h2c-async-route cases on every engine; TestShutdownSendsH2GoAwayThenFinishesStreams reads the frames of a raw h2c connection: GOAWAY first, then the held stream's response, then the close; TestShutdownH2PoolWaitIsBounded: a pool handler that does not return holds the shutdown for its budget, no longer. BenchmarkPoolDispatch and BenchmarkBridgeServeHTTP measure the two new counters. Stacked on #803 (celeris#753: std's drainCtx) and #807 (celeris#760: epoll's drain budget); merge after them. Fixes #759
FumingPower3925
added a commit
that referenced
this pull request
Sep 28, 2026
…ams on the shared worker pool, and std for its h2c streams (celeris#759) A stream on an async route (Route.Async, or a route AsyncHandlers has made async) runs its handler on the shared HTTP/2 worker pool, off the event loop, and its response comes back through the connection's write queue, which only the loop drains. At shutdown epoll and io_uring cancelled such streams (CloseH2) and closed their connections under the handlers: the client got unexpected EOF, and the OnShutdown hooks ran before the handlers had finished. On std, net/http hands an h2c connection over (hijack) and stops tracking it, so http.Server.Shutdown waited for no h2c stream: the hooks ran, and a direct Shutdown returned, while the handler was still running. epoll, io_uring (and so adaptive): the Processor counts its pool handlers (poolRunning: incremented before Submit, decremented after executeHandler has returned). Once its context is cancelled, a loop or worker sends every HTTP/2 connection GOAWAY(NO_ERROR, last client stream), so its client opens no new stream, and keeps turning, reading and writing as usual, until no HTTP/2 connection has a pool handler running or a response in its write queue, then shuts down as before (on io_uring the 250 ms send drain starts after). The wait ends at the deadline of the budget the last Engine.Shutdown handed over, never before 250 ms: io_uring's Engine.Shutdown now records its ctx, as epoll's does since celeris#760. std: Bridge counts the HTTP/2 requests in their handler, and the drain, after http.Server.Shutdown, waits for that count to reach zero, bounded by the drain's context (celeris#753). Tests: TestShutdownHooksRunAfterTheDrain gains the h2c cases on std and the h2c-async-route cases on every engine; TestShutdownSendsH2GoAwayThenFinishesStreams reads the frames of a raw h2c connection: GOAWAY first, then the held stream's response, then the close; TestShutdownH2PoolWaitIsBounded: a pool handler that does not return holds the shutdown for its budget, no longer. BenchmarkPoolDispatch and BenchmarkBridgeServeHTTP measure the two new counters. Stacked on #803 (celeris#753: std's drainCtx) and #807 (celeris#760: epoll's drain budget); merge after them. Fixes #759
Contributor
Author
FumingPower3925
marked this pull request as ready for review
September 29, 2026 09:25
Merging this PR will not alter performance
Comparing 
Footnotes
|
FumingPower3925
deleted the
fix/celeris-753-std-cancel-shutdown-timeout
branch
September 29, 2026 10:07
1 task
FumingPower3925
added a commit
to goceleris/docs
that referenced
this pull request
Sep 29, 2026
…nTimeout (celeris#753) (#80) graceful-shutdown page: on std, a cancel of StartWithContext's context now keeps Config.ShutdownTimeout (celeris v1.6.0, goceleris/celeris#803, fixes celeris#753): the hooks run, and StartWithContext returns, at the deadline while a running handler keeps running. The three places that said a std cancel waits for the handler (the StartWithContext paragraph, the early-exit pitfall, the FAQ's std bullet) now describe v1.6.0 and keep the old behaviour as a pre-v1.6.0 note; the measured line gains the new 501 ms figure. Follow-ups: goceleris/celeris#824.
FumingPower3925
added a commit
that referenced
this pull request
Sep 29, 2026
…ams on the shared worker pool, and std for its h2c streams (celeris#759) A stream on an async route (Route.Async, or a route AsyncHandlers has made async) runs its handler on the shared HTTP/2 worker pool, off the event loop, and its response comes back through the connection's write queue, which only the loop drains. At shutdown epoll and io_uring cancelled such streams (CloseH2) and closed their connections under the handlers: the client got unexpected EOF, and the OnShutdown hooks ran before the handlers had finished. On std, net/http hands an h2c connection over (hijack) and stops tracking it, so http.Server.Shutdown waited for no h2c stream: the hooks ran, and a direct Shutdown returned, while the handler was still running. epoll, io_uring (and so adaptive): the Processor counts its pool handlers (poolRunning: incremented before Submit, decremented after executeHandler has returned). Once its context is cancelled, a loop or worker sends every HTTP/2 connection GOAWAY(NO_ERROR, last client stream), so its client opens no new stream, and keeps turning, reading and writing as usual, until no HTTP/2 connection has a pool handler running or a response in its write queue, then shuts down as before (on io_uring the 250 ms send drain starts after). The wait ends at the deadline of the budget the last Engine.Shutdown handed over, never before 250 ms: io_uring's Engine.Shutdown now records its ctx, as epoll's does since celeris#760. std: Bridge counts the HTTP/2 requests in their handler, and the drain, after http.Server.Shutdown, waits for that count to reach zero, bounded by the drain's context (celeris#753). Tests: TestShutdownHooksRunAfterTheDrain gains the h2c cases on std and the h2c-async-route cases on every engine; TestShutdownSendsH2GoAwayThenFinishesStreams reads the frames of a raw h2c connection: GOAWAY first, then the held stream's response, then the close; TestShutdownH2PoolWaitIsBounded: a pool handler that does not return holds the shutdown for its budget, no longer. BenchmarkPoolDispatch and BenchmarkBridgeServeHTTP measure the two new counters. Stacked on #803 (celeris#753: std's drainCtx) and #807 (celeris#760: epoll's drain budget); merge after them. Fixes #759
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.
Defect
On
std, a cancel ofStartWithContext's (orStartWithListenerAndContext's) context ignoredConfig.ShutdownTimeout. The context handed toListenis derived from the caller's, so the cancel reachesstd.Engine.Listenfirst, and itsctx.Donebranch started the drain itself withcontext.Background(). The drain is async.Oncearoundhttp.Server.Shutdown, so the watcher'sServer.Shutdown, which carries the budget, waited inonce.Dofor that unbounded drain, and so did theOnShutdownhooks and theStartWithContextcall, until the last handler returned. The watcher'sEngine.Shutdowndid not run theOncebody and returned nil. The #498 escalation armed on the budget (baseCancelat the deadline) cancelsr.Context(), which a celeris handler does not see on HTTP/1.1, so nothing ended the wait. A directServer.Shutdownwas not affected:Engine.Shutdownruns beforeListen's context is cancelled, so its ctx won theOnce.Fix
engine/std: the drain runs under an engine-owned context (drainCtx), whichever call starts it, and everyShutdown(ctx)cancels that context when its ctx expires, together with the #498 escalation (onecontext.AfterFunc). So a drain thatListenstarted ends at the budget of theShutdownthat follows it. Every caller now gets the drain's result: a caller whose own ctx ran out gets that ctx's error (context.DeadlineExceeded, as a directShutdowndid), andListenstill returns nil when aShutdowncut its drain short (thatShutdownreports it). A drain that ends cleanly disarms the caller'sAfterFuncas before, so a hijacked WebSocket's request context is left alone.server.go: theServer.Shutdowngodoc explained the cancel ordering by the old race ("Cancelling first would let Listen win Engine.Shutdown's sync.Once and strip the deadline"); it now says the drain keeps everyShutdown's deadline whichever call starts it.engine/engine.go: theEngine.Shutdowndoc said that when ctx expires "remaining connections are closed". No engine does that; it now says what they do (the issue's related doc gap).No other engine changes: epoll, io_uring and adaptive already run the hooks at the deadline (#703).
Head 325d83a. The evidence below ran on 3d601ec; 325d83a changes only the
Server.Shutdowngodoc in server.go.Failing first
Script:
evidence/lanes-20260927/WRITE/753/run-controls.sh <ref> 5(one container, CI shape: linux/arm64, 4 CPUs, 8 MiB memlock). Each variant ofengine/std/engine.gois copied over the PR tree withcp; both tests run-count=5 -v; only--- PASS/FAIL/SKIPlines are counted.engine/std/engine.goTestListenCancelDrainKeepsTheShutdownBudgetTestStartWithContextCancelKeepsShutdownTimeoutOnStd(2 subtests each run)drainCtxLog:
753/logs/controls-3d601ec.log. The engine test forces the order (Listen's context is cancelled first, andShutdownis called only once the listener refuses connections, which is the first thinghttp.Server.Shutdowndoes), and both tests hold the handler until every assertion has run, so a drain that waits for it fails every time, not late. m1 and m2 are the second control: each breaks one of the two halves of the fix, and each is caught.The issue's measurement, re-run with lane LIFECYCLE's #738 deadline probe (unchanged copy;
753/probe/run-probe.sh <ref> m8): 500 ms deadline, a request held 2 s in its handler, times from the moment the shutdown began.StartWithContextreturnedc.Context()/ ignores itc.Context()/ ignores itShutdown(500 ms)Shutdown:DeadlineExceeded)Logs:
753/probe/probe-dfd044f-m8.log,753/probe/probe-3d601ec-m8.log(24/24 probe cases ran; the epoll, io_uring and adaptive rows are unchanged). The request itself is left to finish, ashttp.Server.Shutdownleaves it;c.Context()is still not cancelled at the deadline on std's HTTP/1.1 path.Deadlock check
No new lock.
drainis the samesync.Once; the only new wait isonce.Doin a caller that did not start the drain, which is where every caller already waited. The one new edge, aShutdowncaller'sAfterFunccancellingdrainCtx, runs on its own goroutine and takes nothing. The root test is the hang check: its handler is held past every bound, so a wait that depended on the handler returning fails the case at 3.3 s instead of passing late.Suites
scripts/suites.sh 753 <shape> './engine/std/ ./engine/ .' base:dfd044f head:3d601ec:go test -race -count=1 -v, base and head in one container per shape, linux/arm64. Counts are--- PASS/FAIL/SKIPlines (top-level | subtests).The head adds 2 top-level tests and 2 subtests. The skips are the same in both arms:
TestRouteAdaptive_SettleReopenCost(every shape) and threeTestAdaptiveSettledRouteRetime592/iouring/*subtests (one-worker ring only). Logs:753/suites/m8-…log,753/suites/unl-…log.linux/amd64 (emulated, no
-race, the shutdown tests of./engine/stdand the root package): both new tests PASS; the only FAIL isTestShutdownHooksRunAfterTheDrain's io_uring cases, "io_uring not available on this system" under emulation (753/suites/amd64-engine_std-head3d601ec.log).go vetand golangci-lint (the repo config) clean for GOOS=linux GOARCH=amd64 and arm64.CI: 17/17 checks passed on 3d601ec; the run on 325d83a is below.
Docs
goceleris/docs: goceleris/docs#80 (graceful-shutdown page: the std cancel caveat and the FAQ row are no longer true).Review follow-ups
The round-1 review found no blocking defect here; its minor points are filed as #821 (overlapping
Shutdowncalls share the shortest budget, and a caller whose own ctx is live getscontext.Canceled; theengine.Engine.Shutdownsentence "No engine closes a connection whose handler is still running when ctx expires", false for an HTTP/2 stream on the shared worker pool, which #808 rewrites; a test-margin nit). This head, 325d83a, is unchanged in round 2, and so are the numbers above.Fixes #753