[FumingPower3925]

Lane L19 H2-TIMEOUTS of v1.6.0. Refs #889, Refs #885, Refs #884.

Draft. Nothing in the trackers is ticked or edited. The policy is decided: ship A + A1 + A2 + A3 (the smallest combination that is no worse than main for any client, the recommendation of Opus r1), and build per-stream deadlines (option D below) as the next piece of work, before the tag. The branch is the proposal (A) and three refinements (A1, A2, A3), each a pair of commits (tests first, then code). Opus r2: approve-with-fixes, no blocker or major; its fixes are the last commits (F1 to F6 below).

Found by the 889.34 probe of PR #1131 and reproduced by its review. The second defect it uncovered, io_uring's write clock, is its own PR: #1136.

Method: the defect was reproduced on main f4ae3ad on every engine with a failing test (commit 1); every number below has a log behind it (docker golang:1.27.3, --cpus 2, --security-opt seccomp=unconfined --ulimit memlock=-1, uid 1000, linux/arm64, kernel 7.0.12-linuxkit; io_uring ran, tier=high; adaptive ran on epoll). Logs are in the lane's scratch (logs/), cited by name.

The defect and the rule it gets

The timeout sweep of epoll, io_uring and adaptive applied ReadTimeout and IdleTimeout, measured from the client's last read, to an h2 conn whatever streams it had open: an h2c SSE stream whose client sends no frame was closed at the timeout (1.003 to 1.007 s with 1 s on epoll and adaptive; io_uring when the gap between two server writes exceeds it). std keeps the stream.

The rule this branch builds, by steps:

step	what the sweep does for an h2 conn that is past ReadTimeout or IdleTimeout
main	closes it
A (the proposal)	skips it if a stream is open
A1	... only if every open stream has a whole request (HalfClosedRemote): a client that owes the rest of a request is the silence ReadTimeout is for
A2	... and no response is waiting for the client's window (buffered tail, or a pool handler waiting in AwaitSendWindow): the client owes a WINDOW_UPDATE
A3	when it has no such stream any more, keeps it until the timeouts are due from the sweep that last saw one (the idle period starts where the last stream ends, within a sweep): only for a stream that outlived the timeout, see Findings 7

Commits (base f4ae3ad)

	commit	red on the previous one
A	94dfe2e tests, 18f7a0d the change (H2State.ActiveStreams, h2StreamOpen in both sweeps), 055f5dc benchmark, 7d1368b unit tests	10 cells on main
A1	6e8047c tests, 09621bf code	6 cells (logs/red-a1-evidence.log)
A2	f01b84c tests, 997df26 code	12 cells (logs/red-a2-evidence.log)
A3	0e6b88e tests, 4a5e435 code	8 cells (logs/red-a3-evidence.log)
minors	cf9e09f
r2 fixes	merge of main (200cef0); f8b4789 race test, then the one-Swap fix in handleGoAway (F2); godoc and pinned-test comment (F1, F4); this body (F3, F5, F6)	the GOAWAY race test fails 5 of 5 on the old code

Dropping a refinement: drop its two commits and, for A1 and A2, change the one expression they edited (Manager.ServerOwes, one clause per line, and the call sites that A1 renamed from ActiveStreams() > 0; the renamed h2ServerOwes and the benchmark fixture are A1's).

What changed in each sweep

• internal/protocol/h2/stream/manager.go: requestOpen atomic.Uint32, moved with activeStreams in updateActiveCount (the one place a state change reaches them) and in TryOpenStream; Manager.ServerOwes() = activeStreams > 0 && requestOpen == 0 && outboundHeld == 0 && sendWindowWaiters == 0. outboundHeld (bytes buffered for the peer's window) and sendWindowWaiters already existed and are maintained where the buffer is charged and refunded; four atomic loads, no scan of the streams, no lock. internal/conn/h2.go: H2State.ActiveStreams, H2State.ServerOwes, and OpenStreamForTest (a seam for the engines' sweep tests, as EnqueueWrite is).
• epoll checkTimeouts and io_uring checkTimeouts: the Idle and Read checks become timeoutsDue(elapsed); only once due the sweep calls h2ServerOwes(cs): exempt (and noted, A3) if the server owes, skipped if the look could not be made, otherwise closed unless the timeouts are not due from the time a stream was last owed (A3). h2ServerOwes reads cs.h2State under detachMu (the h1-to-h2c upgrade writes it off the loop thread) with TryLock; with the lock held epoll reads the switch flag and the busy test in one asyncInMu section (F3 of the review) and, if switched, the pointer without the lock (ordered by the flag); io_uring skips the conn, as its snapshotH1Deadlines already does. The WriteTimeout arm is untouched.
• A3's field: epoll keeps the time in detectDeadline, which is free once the protocol is detected (connState is at its 448-byte size class, TestConnStateStaysInItsSizeClass; accessors h2OwedAt/noteH2Owed check detected); io_uring has its own h2OwedAt. Not lastActivity: it is the write clock of the sweep on both engines, and restamping it would loosen io_uring's WriteTimeout.
• config.go: the Config.ReadTimeout godoc states the final behaviour of the combination (the exemption, A3, what is not bounded); IdleTimeout cross-refers to it. It changes with the decision.

Failing on main: reproduction per engine

Raw h2c client (x/net/http2 framer): after its request it sends nothing. Async route, a 12-byte chunk every 200 ms ("steady") or every 2 s ("quiet"); the timeout under test is 1 s, the others off. Request to server close (logs/red-main-final2.log):

engine (main f4ae3ad)	defect	steady Read	steady Idle	quiet Read	quiet Idle
std	no	open 3 s	open 3 s	open 5 s	open 5 s
epoll	yes	1.003 s	1.004 s	1.006 s	1.007 s
adaptive	yes	1.005 s	1.005 s	1.005 s	1.005 s
io_uring	yes, quiet only	open 3 s	open 3 s	1.000 s	1.001 s

io_uring is spared on the steady stream because a completed send restamps its clock. On the branch every cell is open for the whole hold.

Tests

test/server, no helper of #1132/#1133, no skip (CI skip allow-list untouched), engines761 + startServerConfig761:

• h2_conn_timeouts_889_linux_test.go (A): steady and quiet streams not cut (read, idle, and the write guard on the native engines); a conn without a stream is still reaped by each timeout alone (56 cells: preface only, SETTINGS only, partial frames, HEADERS without END_HEADERS, a finished stream, a client RST_STREAM, 101 streams against MAX_CONCURRENT_STREAMS and a reset of all); a client that pings keeps the stream (option B's premise).
• h2_silent_streams_889_linux_test.go (A1, A2): the silent or dead clients, 18 cells = 3 native engines x sync and async routes x {POST without END_STREAM, a 1 MiB and an 8 MiB response the client grants no window for}, ReadTimeout 1 s, IdleTimeout off, WriteTimeout 2 s: closed at about the timeout. And a pinned test, 6 cells: a client that sends a PING every 500 ms keeps an unfinished POST and an unread 1 MiB response open past 6 s on all three native engines, as on main; it passes before and after, and changes when per-stream deadlines exist.
• h2_conn_after_stream_889_linux_test.go (A3): a 2.5 s stream ends; 400 ms later the client's next request on the same conn is served, and the conn is kept for about the timeout and then reaped (12 cells, read and idle).
• engine unit tests (h2_server_owes_889_linux_test.go in both): answers with a request open and whole, with and without detachMu; the held lock (handler running on HTTP/1, other holder, switched to h2); race tests: a writer of h2State under detachMu, and the real upgrade order (write h2State, set h2Switched under asyncInMu, keep detachMu) read lock-free. Stream package: the counts through every way a stream ends (state change, delete, reset, duplicate Closed, a buffered tail dropped with its stream, a waiting handler).

Local (branch head cf9e09f): new test/server cells 115 of 115 green, 0 skips (logs/final-new-cells.log); with -race the same set and then -run 'H2|Shutdown|Detach|Hijack|SSE' green (logs/race2-testserver.log, 124 s and 490 s); go test -race of ./internal/engine/epoll/..., ./internal/conn/..., ./internal/protocol/... ok (logs/race2-epoll-conn-protocol.log, epoll 245 s) and ./internal/engine/iouring/... ok (logs/race2-iouring.log, 485 s); mage apiCheck ok; GOOS=windows and darwin go vet of ./internal/conn/... ./internal/protocol/... . ok; golangci-lint clean on the new code (three errcheck hits in files this PR does not touch, a cross-GOOS artefact). Earlier, on A alone: whole test/server and test/... green (689 s).

Mutants (each applied, the named tests run, file restored)

mutant	red
M1: h2StreamOpen never exempts (= main)	the 10 cells of commit 1 (logs/mutant-m1-never-exempt.log)
M2: exempts every h2 conn (A only)	48 of 48 native reap cells (logs/mutant-m2-exempt-every-h2.log)
M3: h2State read before detachMu	DATA RACE in both packages under -race (logs/mutant-m3-read-outside-lock.log)
M4: updateActiveCount never lowers the count	18 reap cells (logs/mutant-m4-count-never-lowered.log)
MA1: ServerOwes without the requestOpen clause	the 6 request-open cells (logs/mutant-a1-no-request-clause.log)
MA2: without the outboundHeld and sendWindowWaiters clauses	the 12 response cells (logs/mutant-a2-no-outbound-clause.log)
MA3: the A3 restart disabled (epoll)	8 cells: reuse and kept-for-the-timeout on epoll and adaptive (logs/mutant-a3-no-restart.log)
M6: the lock-free read of h2State made before the flag is seen	the real-upgrade race test, DATA RACE (logs/mutant-m6-switched-read-unordered.log)

io_uring's A3 cannot be observed by an end-to-end test: a completed send restamps its clock, so a stream that ends with a send ends with a fresh clock (measured: 1.0 to 1.7 s after the end, the spread being the idle worker's sweep cadence); it is covered by reading and by sharing the code shape with epoll.

Decision pending: ReadTimeout and IdleTimeout on h2 connections with active streams

The maintainer has decided (A + A1 + A2 + A3, then D). What the measurements say, with the correction the review forced:

What main's read sweep actually bounds. Only a silent or dead peer. A client that sends any frame once per ReadTimeout restarts the conn clock. Measured on main f4ae3ad, ReadTimeout 1 s, WriteTimeout 2 s, a PING every 500 ms (logs/ping-attacker-main.log, TestZZRevPingAttacker889 of the reviewer, run again here on all four engines):

cell	std	main, silent client	main, client pings every 500 ms	A alone, silent client	A + A1 + A2, silent client	A + A1 + A2, pinging client
POST without END_STREAM	RST_STREAM NO_ERROR at 1.00 s, conn kept	conn closed 1.00 to 1.005 s	open past 6 s, epoll, adaptive, io_uring	open past 8 s	closed 1.000 to 1.08 s (sync and async routes)	open past 6 s
1 MiB response not taken	RST_STREAM INTERNAL_ERROR 2.00 to 2.01 s	closed 1.004 to 1.233 s	open past 6 s, all three	open past 8 s	closed 1.004 to 1.161 s	open past 6 s
8 MiB response not taken	RST_STREAM INTERNAL_ERROR 2.00 s	closed 1.004 to 1.207 s	(not run)	open past 8 s	closed 1.004 to 1.157 s	(not run)
h2c SSE, silent client (the defect)	open	cut at 1.00 s (epoll, adaptive; io_uring when quiet)	open	open	open	open

So "A opens a slowloris hole main does not have" was overstated: main has no per-stream bound either; an active attacker passes it for one frame per ReadTimeout (bounded by MAX_CONCURRENT_STREAMS x the connection count). What A alone loses is the reaping of silent or dead peers with a stream open: request-open and the response cells above go from closed at 1 s to open for good (checked with ReadHeaderTimeout 2 s as well: still open; logs/probe-silent-readheader2s-branch.log). A1 + A2 give that reaping back (the 18 cells above, logs/final-new-cells.log) while every stream the server is writing to stays open.

Options

• A alone: not acceptable (dead peers leak forever). It is the first commit pair only so that the progression can be measured.
• A + A1 + A2 + A3 (recommended by the reviewer): no worse than main for any client: silent and dead peers are reaped as on main, a pinging client is bounded as on main (not at all), SSE streams are no longer cut, a conn is not closed under a client that is about to reuse it after a stream that outlived the timeout (Findings 7). A3 is separable (without it, epoll and adaptive close a conn 4 to 20 ms after a long stream ends, no GOAWAY, losing the client's next request: logs/red-a3-evidence.log).
• B (keep ReadTimeout, measure it from the last frame of any kind): main's behaviour already; TestH2ReadTimeoutCountsAnyClientFrame889 passes on main on all four engines. The B that would change something is a server PING keepalive on conns with a stream open; it would reap dead peers only (an attacker ACKs a PING), not slowloris. Not built.
• C (leave as is, document -1): turns off the reaping of silent h2 conns server-wide; fixes nothing.
• D, the real fix, not built (std parity): per-stream deadlines. D1: ReadTimeout from HEADERS until END_STREAM, then RST_STREAM and the conn is kept (what std does: RST_STREAM NO_ERROR at 1.00 s above). D2: WriteTimeout as "no progress" on a stream with buffered or flow-blocked DATA, then RST_STREAM(INTERNAL_ERROR) and the buffer freed (std: 2.00 s above; Findings 2). Only D bounds an active attacker. With D in place A1 and A2 are redundant and A + A3 is enough.

Release-note wording if the combination is chosen: say plainly that a client that sends any frame once per ReadTimeout can hold streams open, as it can on main, bounded only by MaxConcurrentStreams x the connection count.

Benchmark note: the sweep, not a request path

ActiveStreams, ServerOwes and h2ServerOwes are reached only from checkTimeouts (epoll: at most every 25 ms; io_uring about 50 to 100 ms) and only for a conn past ReadTimeout or IdleTimeout. A1 does touch the h2 stream-state path: the counters are maintained in updateActiveCount, which the state changes already called: one more atomic add in TryOpenStream and one more atomic transition (a CAS loop) when a request completes (Open to HalfClosedRemote), per request. The in-memory h2 request benchmarks, 12 rounds interleaved main, branch (cf9e09f) and an A/A twin (logs/bench5/table.md), medians and minimums, branch vs main (the twin vs main in brackets): InlineDispatch893 +4.1%, +0.7% (twin -1.4%, -1.9%; about 4 ns on 102), AsyncRequest951 +2.2%, -6.3% (twin +6.7%, -5.7%), PoolDispatch -0.7%, +4.1% (twin -3.0%, +6.5%), ProcessH2Headers889/single -2.1%, +8.2% (twin +1.9%, -2.3%), /pipelined8 -3.9%, +10.6% (twin +1.7%, +12.3%), StreamedResponse889/closed -5.8%, +7.3% (twin -0.7%, +10.4%), /unclosed -3.7%, +3.5% (twin -2.4%, +15.6%); allocations unchanged. The InlineDispatch893 samples overlap completely (main 88.7 to 140.3 ns/op, branch 89.4 to 132.6, twin 87.0 to 138.3), so the +4.1% median is not resolvable from this data, in either direction, and "two atomics explain it" is not claimed. What holds is the bound by construction: one atomic add (TryOpenStream) and one compare-and-swap (Open to HalfClosedRemote) per request, uncontended, on a path of about 100 ns in this micro-benchmark and microseconds per real request. Measured on arm64 only (the Mac); a LOCK-prefixed operation costs more on amd64 and is unmeasured: native amd64 was not available (docker amd64 emulation is not a timing source), so the cluster P-run covers it. The minimums of this busy host move 10 to 15% between twin and main. Not claimed: no cost on the h2 request path.

Docker --cpus 2 (the Mac was busier than in the first round: main medians are 20 to 30% higher), 12 rounds, interleaved main (f4ae3ad), A (7d1368b), A+A1+A2+A3 (cf9e09f) and an A/A twin of main (a copy of main's binary), the order rotated, -benchtime 200ms; ns per whole sweep; medians and minimums; full table logs/bench3-table.md:

benchmark (4096 conns)	main med / min	A/A twin vs main (med, min)	A vs main	A1+A2+A3 vs main
epoll CheckTimeouts865/sync (existing)	22037 / 16863	-1.5%, +1.2%	+0.5%, -0.2%	-3.5%, -0.7%
epoll CheckTimeouts865/async	28636 / 22602	+2.8%, -2.0%	-1.3%, -4.0%	+2.3%, -0.7%
epoll H2889/h1-within/sync (defaults set, nothing due)	23028 / 16785	-4.7%, +1.0%	-4.9%, +0.1%	-7.6%, -0.3%
epoll H2889/h2-within/sync (h2 conns, a stream open)	15797 / 12592	+0.6%, -2.6%	-0.2%, -1.6%	-4.1%, -3.0%
iouring CheckTimeouts888/attached (existing)	44506 / 36562	+1.7%, +4.9%	+3.7%, -0.8%	+6.6%, +2.4%
iouring H2889/h1-within	42844 / 36499	+2.3%, -8.6%	+7.5%, -7.8%	+6.1%, -10.5%
iouring H2889/h2-within	33706 / 30308	+6.4%, +0.1%	+1.2%, -6.3%	-1.1%, -14.0%

The within rows never run the new code (the conn is not past a timeout); their differences are layout and noise, inside the twin's spread (the iouring medians move up to 8% between twin and main). Not claimed: a speedup; and a copy of main's binary is a floor for run-to-run noise, not for code-layout effects of a rebuild.

The cost that is new is per exempt conn per sweep (past the timeout with a stream owed: on every sweep for the stream's life), extra ns per conn over h2-within in the same runs, medians (minimums in brackets):

	A	A1+A2+A3	scratch variant of A2 that scans (OutboundPending)
epoll sync, 256 / 4096 conns	2.4 (2.0) / 5.4 (3.9)	4.5 (3.4) / 8.7 (5.8)	32 to 43
epoll async, 256 / 4096	4.5 (3.6) / 7.9 (5.8)	6.1 (5.2) / 12.0 (7.5)	32 to 43
io_uring, 256 / 4096	6.4 (4.6) / 12.5 (8.1)	9.1 (6.8) / 15.7 (10.6)	not measured

The counter version costs 1.6 to 4.1 ns more per exempt conn than A (the two extra atomic loads and the A3 store), against 32 to 43 for the scan. At 10 000 exempt conns on a loop and 40 sweeps a second: 0.5% of a core on epoll (12 ns x 10 000 x 40).

Findings, not fixed here

1. Per-stream deadlines (D above) are the real fix for an active attacker; they are std's model and the native engines have none. D2 is also the pre-existing gap this PR exposed: an h2 response tail buffered for the client's window (TryBufferOutbound buffers a body of any size when the conn holds nothing) has no WriteTimeout bound on the native engines; measured on main with ReadTimeout and IdleTimeout off and WriteTimeout 2 s: the 1 MiB and 8 MiB cells stay open past 8 s on epoll, adaptive and io_uring; std resets them at 2.01 s. The read sweep (60 s by default) was its only bound; A2 keeps it.
2. io_uring WriteTimeout cuts a quiet h2 stream at a chunk: its own PR, iouring: the write clock of a conn starts when its send starts, so a quiet h2 stream is not cut at a chunk (v1.6.0, Refs #889) #1136 (found by this PR's quiet-stream test failing in CI once, https://github.com/goceleris/celeris/actions/runs/40464118254; the write arm of that test was removed from this PR).
3. epoll: a server write does not restamp the conn's clock, io_uring's completed send does: the cause of the table's asymmetry; a consequence of the clocks, not changed.
4. std: ReadTimeout does not reap an idle h2 conn; IdleTimeout does; the reap test has no std/read cell for that reason. std's idle timer, measured, closes a conn 2.00 s after the end of a stream with IdleTimeout 1 s.
5. A real h2c upgrade end to end (a long stream 1 on an upgraded conn) is covered by the unit tests of the lock logic and by reading (the injected stream 1 is HalfClosedRemote), not by an end-to-end cell.
6. middleware: the ring keeps the queued SSE copy behind a wrapper, negotiate.ParseInto is bounded, h2c SSE probe and docs, static Root docs, websocket start retry, cache Store.Set timeout (default pending) (v1.6.0, Refs #888 #889 #884) #1131's native h2c cells of sse_detach_deadlines_855_linux_test.go (WriteTimeout arm only) can add the read and both arms back once the decision lands; this PR does not touch that file.
7. A3 covers only a stream that outlived the timeout (r2 F1). The restart is noted only when a sweep finds the conn past a timeout with a stream owed; a stream that ends before the conn's clock (the client's last frame) reaches the timeout is never noted, and the conn is closed at last-frame + timeout, which can be milliseconds after the stream ended, without GOAWAY. Measured by the reviewer (ReadTimeout or IdleTimeout 1 s, a 0.8 s stream, the next request 300 ms after it ended): epoll and adaptive close the conn, io_uring and std serve it; main does the same, so this is not a regression and the long-stream case A3 targets is fixed. The godoc now says so. Follow-up: count a completed write as activity on epoll as io_uring does (also moves epoll's HTTP/1 ReadTimeout, v1.6.0 polish: epoll engine #885 item 35, and weakens the dead-peer reaping A1 and A2 restored), or stamp when the last owed stream ends on the loop thread (costs on the stream path).
8. A3 can close up to one sweep early (r2 F5): measured 0.980 s after the end with a 1 s timeout; only ever keeps a conn longer than main, never shorter.
9. Pre-existing, io_uring: with WriteTimeout -1 a non-reading peer with a SEND in flight is never reaped (r2 F6, V12): the sweep's dirty/sending arm only applies WriteTimeout, so io_uring holds the conn (the whole 48 MiB delivered after 6 s of silence), where epoll and adaptive reap it at ReadTimeout. Identical on main; worth a release-note line or an issue.

For the reviewer to attack

• A3 at sweep granularity: a conn can be closed up to one sweep period (25 ms epoll, up to 100 ms io_uring) earlier than the timeout after the stream's end; and epoll keeps h2OwedAt in detectDeadline (guarded by detected): is there a path that sets detectDeadline after detection (adoption, the transplant, the H1 handoff of the adaptive engine)?
• Answered by r2 (V10, V11): a leak of outboundHeld, sendWindowWaiters or requestOpen makes ServerOwes() false, so the conn is simply not exempt (back to main); only an activeStreams leak with nothing else held would exempt a silent conn for good, and the paths probed (RST of a parked pool handler, of a buffered tail, GOAWAY during SSE, a handler panic after a first DATA) all behave as main.
• The pinned ping test: does it pin the right thing, and is it the right place to flip when D lands?
• A1's HalfClosedLocal counted as request-open: a server that answers before the request is whole (early 4xx) keeps the client owing; right?
• The Config godoc says what the combination does; it must change if the maintainer chooses otherwise.

[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

❌ Patch coverage is 77.77778% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/conn/h2.go	0.00%	8 Missing ⚠️
internal/engine/epoll/loop.go	85.71%	4 Missing ⚠️
internal/engine/iouring/worker.go	77.77%	4 Missing ⚠️
internal/engine/epoll/conn.go	75.00%	1 Missing ⚠️
internal/protocol/h2/stream/manager.go	95.23%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!