Skip to content

Refund HTTP/2 connection credit for buffered DATA - #1

Merged
hiroTamada merged 3 commits into
masterfrom
hypeship/refund-h2-conn-window
Aug 31, 2026
Merged

Refund HTTP/2 connection credit for buffered DATA#1
hiroTamada merged 3 commits into
masterfrom
hypeship/refund-h2-conn-window

Conversation

@hiroTamada

@hiroTamada hiroTamada commented Aug 29, 2026

Copy link
Copy Markdown

summary

  • return connection-level HTTP/2 flow-control credit after response DATA is buffered
  • keep stream-level credit tied to application body reads
  • avoid duplicate connection credit on response-body close
  • batch connection-level WINDOW_UPDATEs: refunds accumulate in unsentConnRefund and are announced once they reach half the connection window, instead of one frame + flush per DATA frame — avoids hot-path write amplification and a per-frame stream-0 WINDOW_UPDATE cadence that no real browser produces
  • add a regression test for paused response bodies exhausting a connection, plus assert exactly-once refunds (no double credit) in the unused-flow-control test

why this diverges from upstream x/net

upstream returns connection credit on body reads and never hits this starvation because its default connection window is 1 GB against 4 MB stream windows. fhttp advertises browser-realistic windows (ConnectionFlow 15663105 with 6 MB streams for Chrome), so three unread bodies can pin the whole connection window. refunding at buffer time is required here; the batching mirrors the accumulate-and-threshold approach upstream adopted in CL 448155 (inflow).

testing

  • go test -vet=off -run 'TestTransportPausedBodiesDoNotExhaustConnectionWindow' -count=10 ./http2
  • go test -race -vet=off -run 'TestTransportPausedBodiesDoNotExhaustConnectionWindow|TestTransportReturnsUnusedFlowControl|TestTransportReturnsDataPaddingFlowControl' -count=3 ./http2
  • go test -vet=off -run 'TestTransportReturnsUnusedFlowControl|TestTransportReturnsDataPaddingFlowControl|TestTransportAdjustsFlowControl' ./http2
  • full package tests not run as a gate: the repository has pre-existing failures under the current toolchain (including TestTransportFlowControl and a nil-pointer panic in legacy server tests). verified the failure set is byte-identical between this branch and its base.

telemetry accessors

  • (*ClientConn).BufferedUnreadBytes() and (*Transport).BufferedUnreadBytes(): buffered-but-unread response bytes, for callers to export as a gauge — the connection window no longer bounds this memory, so consumers should watch it during rollout (kernel/kernel#3674 wires it into metro-api).

release-lineage note

kernel releases are cut as master + 8ab32de (the stream over-credit fix, pending upstream as bogdanfinn#24) + the module rename. this PR is based on master, and both change the same stream-refresh line in transportResponseBody.Read (and both add transport_flow_test.go). the correct combined resolution is unsent := int(cc.streamFlow) - int(cs.inflow.n) - cs.bufPipe.Len() — subtract buffered bytes (8ab32de) and use the stream-only window (this PR, since available() is capped by the batched connection window). branch hypeship/refund-h2-conn-window-rc is that resolution already done: v0.6.8-kernel.1 + this PR merged, all flow tests (including TestTransportSlowReaderLargeResponse) passing — ready to tag as v0.6.8-kernel.2 once this merges.


Note

High Risk
Changes core HTTP/2 client flow-control accounting and WINDOW_UPDATE timing; bugs could starve connections or mis-announce windows, though behavior is covered by targeted regression tests.

Overview
Refunds HTTP/2 connection-level receive window credit when response DATA lands in the stream buffer, instead of when Response.Body is read. Stream-level credit still follows application reads. This prevents multiple paused/unread bodies from pinning the whole connection window under browser-sized ConnectionFlow (~15.6 MiB), which could block other streams.

Connection refunds go through new refundConnFlow: bytes accumulate in unsentConnRefund and are announced with stream-0 WINDOW_UPDATE only after they reach half the connection window, reducing per-frame write churn. transportResponseBody.Close no longer breaks the pipe early or sends connection WINDOW_UPDATE for unread buffered data (avoids double-refund with the buffer-time path).

Adds BufferedUnreadBytes() on ClientConn and Transport (plus pool aggregation) so callers can gauge buffered-but-unread response bytes now that the connection window no longer caps that memory.

Tests add TestTransportPausedBodiesDoNotExhaustConnectionWindow and tighten unused-flow-control expectations (full 5000-byte conn refund, no duplicate stream-0 updates).

Reviewed by Cursor Bugbot for commit 80ab7d2. Bugbot is set up for automated code reviews on this repo. Configure here.

@hiroTamada
hiroTamada marked this pull request as ready for review August 29, 2026 00:46
@hiroTamada
hiroTamada requested a review from rgarcia August 29, 2026 18:20

@rgarcia rgarcia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving — the fix is right and the mechanism checks out. verified the regression test fails on base without the fix, race detector is clean, and the full-suite failure set is byte-identical to master, so the pre-existing breakage claim holds. batching the stream-0 WINDOW_UPDATEs to a half-window cadence instead of per-frame is the right call for the fingerprint too.

one question before we tag a release and bump consumers: the connection window was incidentally the only cap on unread buffered body data per connection (~15.7MB). after this it's 6MB × concurrent streams — we've observed 100+ open unread bodies on a busy host, so worst case is potentially hundreds of MB per connection with nothing bounding the dataBuffer growth. that's what chrome does too, but chrome has its own memory accounting on top and we don't.

can we either:

  1. size it — what's the p99 concurrent unread bodies per pooled connection under real load? if it's <20 this is a non-issue, or
  2. add telemetry — a gauge for buffered-unread bytes per ClientConn (sum of bufPipe lengths) exposed so callers can export it, and we watch process RSS during rollout

either answers it. (2) is probably worth having regardless since this class of bug was invisible until we added body-level tracking.

@hiroTamada
hiroTamada force-pushed the hypeship/refund-h2-conn-window branch from c3db874 to 80ab7d2 Compare August 31, 2026 14:41
@hiroTamada
hiroTamada merged commit 9e1a615 into master Aug 31, 2026
1 check passed

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 80ab7d2. Configure here.

Comment thread http2/transport.go
if f.Length > 0 {
cc.mu.Lock()
ok := cc.inflow.take(f.Length)
connAdd := cc.inflow.add(int(f.Length))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close leaves response body hung

High Severity

transportResponseBody.Close no longer calls bufPipe.BreakWithError, so a Read already blocked on the body never wakes if the peer has not sent END_STREAM. After forgetStreamID, later DATA is dropped rather than written to the pipe, so that Read waits indefinitely. Aborting a slow download via Body.Close now hangs instead of returning errClosedResponseBody.

Fix in Cursor Fix in Web

Triggered by learned rule: HTTP/2 browser windows vs paused-stream starvation

Reviewed by Cursor Bugbot for commit 80ab7d2. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants