fix: detached-stream lifecycle follow-ups on all four engines - #517
Merged
Conversation
FumingPower3925
force-pushed
the
fix/detached-lifecycle-498
branch
from
September 8, 2026 09:01
3ed3554 to
d82804b
Compare
Five defects from the #496 review, each confirmed real and fixed with a fail-first test. They share a shape: a detached stream (SSE, or any handler that took the connection over) is not told when its peer goes away, or is not cleaned up when it does. 1. H2/h2c SSE was not covered at all. Manager.Close is the only teardown an H2 stream gets when its connection dies -- both native engines reach it from closeConn via CloseH2 -- and it skipped Cancel() for streams whose handler was still running on the H2 worker pool, which is exactly the SSE ones. So nothing ever cancelled them. This is worse than the H1 leak it mirrors: the H2 worker pool is process-global, so parked handlers go on to starve every later H2 connection. 2. epoll skipped EPOLLRDHUP for every detached conn. That is right when the FIN arrives on its own readable edge -- drainRead's Read returns 0 and the existing EOF path fires OnError(errPeerClosed) -- but wrong when it does not, so an SSE conn whose handler never writes never learns the peer left. Now fires OnError under detachMu for detached conns with no WS sink; WebSocket conns keep their current behaviour, since their middleware genuinely does own the close lifecycle. 3. std Shutdown cancelled nothing. http.Server.Shutdown only waits for connections to go idle, so a context-only SSE handler ran straight through it. Wired a BaseContext that is cancelled on shutdown: handlers wake cooperatively and in-flight non-detached requests still drain normally, rather than yanking connections with server.Close(). 4. io_uring never reaped a closing detached conn with undrained sends. closeConn defers the fd close while a SEND is outstanding and sets cs.closing; checkTimeouts skipped cs.closing conns, so the only exit was the SEND's own CQE. A peer that stops reading held the connection until it disconnected. Added a bounded closing timeout in checkTimeouts. 5. epoll could drop the terminal chunk. After the idle-deadline reap, closeConn took SHUT_WR without a final flush, so a remainder stuck on EAGAIN was lost. Now attempts one flushWrites under detachMu for truly detached conns first. Verified on msa2-server (amd64, kernel 7.0.0-30): engine/epoll, engine/iouring and middleware/sse suites all PASS.
…ad deadline The new h2c cells set an absolute SetReadDeadline on the raw conn handed to http2.Transport. That transport keeps a persistent read loop over the socket for the life of the stream, so the deadline killed the connection 10s after dial regardless of progress -- and an SSE stream is deliberately held open far longer than that. It passed on the cluster (kernel 7.0.0-30, io_uring tier=high) and failed on the CI runner (kernel 6.17 azure, tier=optional) with the round trip erroring at 10.7s, i.e. exactly on the deadline. It failed on epoll as well as io_uring, which is what showed it was the test harness rather than an engine. Bound the open with a request context instead, cancelled at test cleanup rather than on return -- cancelling on return would tear down the very stream the assertion needs. A client-side reset would also have made the server cancel the handler through handleRSTStream, a different path from the one under test, so the old shape risked a false pass as much as a flake.
FumingPower3925
force-pushed
the
fix/detached-lifecycle-498
branch
from
September 8, 2026 11:59
d82804b to
99d9e03
Compare
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
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.
Closes #498.
Five defects from the #496 review, each confirmed real and fixed with a fail-first test. They share a shape: a detached stream is not told when its peer goes away, or is not cleaned up when it does.
1. H2/h2c SSE was not covered at all
Manager.Closeis the only teardown an H2 stream gets when its connection dies — both native engines reach it fromcloseConnviaCloseH2— and it skippedCancel()for streams whose handler was still running on the H2 worker pool, which is exactly the SSE ones. Nothing ever cancelled them.This is worse than the H1 leak it mirrors: the H2 worker pool is process-global, so parked handlers go on to starve every later H2 connection.
Cancel()signals;Releasestays deferred to the goroutine that owns the stream.2. epoll skipped EPOLLRDHUP for every detached conn
That is right when the FIN arrives on its own readable edge —
drainRead'sReadreturns 0 and the existing EOF path firesOnError(errPeerClosed). It is wrong when it does not, so an SSE conn whose handler never writes never learns the peer left.Now fires
OnErrorunderdetachMufor detached conns with no WebSocket sink. WebSocket conns keep their current behaviour — their middleware genuinely does own the close lifecycle.3. std
Shutdowncancelled nothinghttp.Server.Shutdownonly waits for connections to go idle. A context-only SSE handler ran straight through it.Wired a
BaseContextcancelled on shutdown, so handlers wake cooperatively and in-flight non-detached requests still drain normally — rather than yanking connections withserver.Close().4. io_uring never reaped a closing detached conn with undrained sends
closeConndefers the fd close while a SEND is outstanding and setscs.closing;checkTimeoutsskippedcs.closingconns, so the only exit was the SEND's own CQE. A peer that stops reading held the connection until it disconnected. Added a bounded closing timeout incheckTimeouts.5. epoll could drop the terminal chunk
After the idle-deadline reap,
closeConntookSHUT_WRwithout a final flush, so a remainder stuck onEAGAINwas lost. Now attempts oneflushWritesunderdetachMufor truly-detached conns first.Verification
Each item was developed in an isolated worktree with a fail-first test, then integrated here. On msa2-server (amd64, kernel 7.0.0-30):
Plus root,
protocol/h2/streamandengine/stdgreen on darwin;gofmt,go vetand cross-compiled linux build all clean.Note
Touches
engine/iouring/worker.goincloseConn/checkTimeouts, which is a different region from thedrainDetachQueuechange in #516 — they should not conflict, but #516 is the more urgent of the two since it fixes a regression currently on main.