[Detail Bug] HTTP/2: Connections that already received GOAWAY are added to the pool, causing opaque h2 errors on first request

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_e32f4ffd-6a1b-4d57-8ecf-03fddc2a3e23

Introduced in #1 by @quettabit on Apr 7, 2026

Summary

• Context: The checkout() method creates new HTTP/2 connections and adds them to the connection pool. It waits for the server's SETTINGS frame before making the connection available.
• Bug: After waiting for SETTINGS, checkout() only checks _recv_dead to detect connection failures, but doesn't check _goaway_received. If the server sends a GOAWAY frame immediately after SETTINGS (e.g., during graceful shutdown), the connection is added to the pool in an unusable state.
• Actual vs. expected: Connection with _goaway_received=True is added to pool and a stream is reserved, but the pending stream's error attribute is NOT set by the GOAWAY handler (timing issue). When send_headers is called, the h2 library raises an internal error because the connection is in an invalid state.
• Impact: Requests fail with unclear errors from the h2 library, not a clean ProtocolError about GOAWAY.

Code with Bug

File: src/s2_sdk/_client.py

try:
    await asyncio.wait_for(
        conn._settings_received.wait(),
        timeout=self._connect_timeout,
    )
except asyncio.TimeoutError:
    pass  # Proceed with h2 defaults
if conn._recv_dead:  # <-- BUG 🔴 does not reject connections that already received GOAWAY
    await conn.close()
    raise ConnectError(
        f"Connection to {host}:{port} closed before HTTP/2 SETTINGS"
    )

pc = _PooledConnection(conn)
conns = self._hosts.get(base_url)
if conns is None:
    conns = [pc]
    self._hosts[base_url] = conns
else:
    conns.append(pc)
# ...
state = pc._conn.reserve_stream()
return _Checkout(pc, state)

Explanation

• During checkout(), the recv loop can process a GOAWAY while the main coroutine is blocked waiting for SETTINGS.
• GOAWAY handling fails only the streams that exist at the time it is processed by iterating _pending_streams.
• checkout() calls reserve_stream() only after the SETTINGS wait completes, so _pending_streams can still be empty when GOAWAY is processed. The newly reserved stream then has state.error is None and the SDK proceeds to call h2.send_headers(...), which fails inside the h2 library because the connection has already received GOAWAY (no new streams allowed).
• Since _recv_dead is not set by GOAWAY (recv loop keeps running), the current post-SETTINGS check does not catch this condition and the unusable connection is pooled.

Recommended Fix

Check _goaway_received in addition to _recv_dead before pooling/returning the connection:

if conn._recv_dead or conn._goaway_received:
    await conn.close()
    raise ConnectError(
        f"Connection to {host}:{port} closed before HTTP/2 SETTINGS"
    )

History

This bug was introduced in commit 3dc9795. Commit 74baa61 attempted to fix GOAWAY by iterating _pending_streams, but the timing race remains because _pending_streams can still be empty when GOAWAY arrives during the SETTINGS wait.