Skip to content

proxy_h2: acknowledge downstream RST_STREAM while the upstream request-body write is blocked#911

Open
molocule wants to merge 6 commits into
cloudflare:mainfrom
modal-labs:rst-stream-fix
Open

proxy_h2: acknowledge downstream RST_STREAM while the upstream request-body write is blocked#911
molocule wants to merge 6 commits into
cloudflare:mainfrom
modal-labs:rst-stream-fix

Conversation

@molocule

Copy link
Copy Markdown

This fixes issue #910

Problem

In bidirection_down_to_up, the downstream-read select arm awaits send_body_to2() inside the arm body. While that write is parked on upstream flow control, the downstream RecvStream is not polled, so a downstream RST_STREAM is never observed.

The task keeps holding the downstream stream handles, and since the h2 crate only returns a reset stream's connection-window credit once all handles drop (release_closed_capacity at ref_count == 0), the cancelled stream pins its share of the downstream connection window for as long as the upstream write stays blocked (potentially indefinitely, because there is no default write timeout).

This is dangerous because cancelling a stream immediately frees its stream slot (max_concurrent_streams) on both ends, so the connection appears to have plenty of capacity and the client (e.g. Envoy or any gRPC client) keeps multiplexing new requests onto it. But the shared connection send window does not come back: credit the client spent can only be restored by the receiver's connection-level WINDOW_UPDATEs, and pingora never sends them because the cancelled streams' unconsumed credit stays pinned to their zombie streams. Flow-control accounting is correct, but clients will not be able to send data upstream.

Changes

  • pingora-core: add Session::watch_h2_stream_reset() to the HTTP server session enum. For H2 it resolves when the client resets the stream, reusing the existing Idle/poll_reset future; for H1/other protocols it is pending forever (there is no out-of-band abort signal — detecting an H1 close would require destructively reading the socket).
  • proxy_h2::send_body_to2: race write_body against watch_h2_stream_reset(). A reset surfaces as an H2Error with ErrorSource::Downstream.
  • bidirection_down_to_up: on a downstream-sourced error from send_body_to2, fail so the stream handles drop and the window credit is reclaimed immediately, except when a cache fill is in progress, in which case the downstream error is swallowed and the upstream response continues to be admitted to cache, mirroring the existing policy for downstream read errors.
  • proxy_down_to_up: on downstream-sourced errors, send RST_STREAM CANCEL on the upstream stream so the upstream peer also releases its stream resources promptly (previously only done for upstream read timeouts).

Notes

The race is placed around the raw write_body (after the request-body filters) rather than around all of send_body_to2 or at the call site. This is because it is the only spot where the borrows are disjoint (&mut SendStream vs. &mut Session), and it means a reset only ever cancels an h2 frame write on a stream that is being torn down, it never cancels user-defined body filters mid-await, which were never required to be cancel-safe.

Testing

Two integration tests (plus an h2c listener for the cache test service on :6154, since raw h2 frame control is needed):

  • test_h2_downstream_rst_while_upstream_write_blocked: upstream never grants window updates; client fills the upstream stream window until the proxy parks, then sends RST_STREAM. Asserts the upstream stream is cancelled (RST_STREAM CANCEL received) within a bounded time. Hangs without this fix.
  • test_h2_downstream_rst_during_cache_fill: same blocked-write reset, but with a cacheable response mid-admission; the upstream withholds the response tail until after the reset. Asserts the fill still completes: a follow-up request is a cache hit with the full body.

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.

1 participant