Add a limit on number of pending HTTP/2 PING ACKs

[mrek-msft]

No description provided.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR adds a safety limit to prevent an HTTP/2 connection from accumulating an unbounded number of queued “fire-and-forget” control frames (PINGs and SETTINGS ACKs), aborting the connection once the queue exceeds an internal threshold, and adds functional tests to validate the behavior.

Changes:

• Add a queued-frame counter and queueing helpers in Http2Connection that abort the connection when the limit is exceeded.
• Route RTT-estimator PINGs and keep-alive PINGs through the new queueing path.
• Add a new localized error string and functional tests that exercise SETTINGS/PING queue exhaustion scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File	Description
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs	Adds functional tests for SETTINGS/PING queue limit behavior and introduces a test stream used to block client writes.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs	Switches RTT PING send path to use the new queueing helper.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs	Implements the queue limit, queueing helpers, and abort behavior when the limit is exceeded.
src/libraries/System.Net.Http/src/Resources/Strings.resx	Adds a new error string for the queue-limit abort.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "44842ac086fa16e916800cf1eb10fa234c6dabd3",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "12a9b3b909af57f3d88be0d0ec422aa336ecbd7e",
  "last_reviewed_commit": "44842ac086fa16e916800cf1eb10fa234c6dabd3",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "12a9b3b909af57f3d88be0d0ec422aa336ecbd7e",
  "last_recorded_worker_run_id": "29687168566",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "44842ac086fa16e916800cf1eb10fa234c6dabd3",
      "review_id": 4730772597
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Real and justified. A malicious or misbehaving HTTP/2 server can flood the client with SETTINGS/PING frames while stalling its own reads; because the client answers each with an ACK queued onto an unbounded write channel, the queue (and buffered memory) can grow without bound — a genuine DoS/OOM vector. Bounding the number of in-flight fire-and-forget frames is the right thing to protect against.

Approach: Reasonable and minimal. SendPingAsync/SendSettingsAckAsync are converted into QueuePing/QueueSettingsAck that increment a shared counter before enqueueing and decrement it when the frame is committed to the outgoing buffer; exceeding MaxQueuedFireAndForgetFrames (1000) aborts the connection with a clear HttpIOException. The counter therefore bounds the frames sitting in the write channel that have not yet been moved to the buffer — exactly the unbounded-growth path. Consolidating LogExceptions inside the new helpers keeps all call sites (ProcessSettingsFrame, ProcessPingFrame, VerifyKeepAlive, RTT ping) consistent.

Summary: ⚠️ Needs Human Review. The change is sound and well-scoped, and I found no correctness blockers. A human should confirm (1) the fixed 1000 limit — shared across SETTINGS ACKs, PING ACKs, and keep-alive/RTT PINGs — cannot be tripped by legitimate high-throughput traffic, and (2) the new tests are not flaky under CI given their reliance on Task.Delay polling and write-blocking, since this worker cannot build or run them.

---

Detailed Findings

✅ Correctness — Counter accounting is safe

The increment happens before PerformWriteAsync enqueues, and the decrement runs inside the write action when the frame is committed to _outgoingBuffer. If _writeChannel.Writer.TryWrite fails or the entry is drained via the _abortException/TryDisableCancellation paths, the write action never runs and the counter is not decremented — but that only occurs during shutdown/abort, where a stale counter is irrelevant. No leak of practical consequence.

✅ Consistency — Renamed helpers

All previous SendPingAsync/SendSettingsAckAsync call sites (including Http2StreamWindowManager.OnDataOrHeadersReceived) were updated, and no dangling references remain. LogExceptions is now applied uniformly inside the helpers.

⚠️ Design question — Single shared limit of 1000

MaxQueuedFireAndForgetFrames bounds SETTINGS ACKs, PING ACKs, keep-alive PINGs, and RTT PINGs with one counter. This is defensible (the outgoing loop normally drains these near-instantly, so the count stays low), but it is worth a maintainer sanity check that no legitimate scenario — e.g., a burst of server SETTINGS during a large multiplexed workload while the socket briefly back-pressures — could legitimately approach 1000 and abort a healthy connection. Not a blocker; flagging for visibility.

⚠️ Tests — Timing-based, unverifiable here

Http2_PingInFlightLimitExceeded relies on BlockedWriteNetworkStream plus Task.Delay(50) polling on WriteCallCount and a UseSsl=false/EnableTransparentPingResponse=false setup to deterministically preload exactly 1000 queued frames. The logic reads correctly, but this class of test is prone to CI flakiness across platforms. clientNetStream is used non-null in the server callback; it is in practice assigned by ConnectCallback before the server reads the client preface/settings, so the nullable deref is safe. A human should confirm the tests are green in CI rather than relying on static reading.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 102.4 AIC · ⌖ 10.2 AIC · ⊞ 10K