[Detail Bug] HTTP/2: GOAWAY with last_stream_id does not fail reserved (pending) streams

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_67afbe46-604a-4bd5-b3ad-96a6c990cb3e

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

Summary

• Context: The Connection class manages HTTP/2 connections and handles protocol events including GOAWAY frames from the server.
• Bug: When a GOAWAY frame is received with a valid last_stream_id, streams in _pending_streams (reserved but not yet sent) are not failed, violating the code's own stated intent and creating an inconsistency with _fail_all_streams.
• Actual vs. expected: Actual: only _streams with stream_id > last_stream_id are failed; _pending_streams are left alive and later error out indirectly. Expected: pending streams should be failed with the same GOAWAY ProtocolError, since they were never processed by the server.
• Impact: Violates the principle of least surprise. Code that follows the pattern established by _fail_all_streams will expect pending streams to be handled. Future maintainers may introduce bugs by assuming consistency.

Code with Bug

# src/s2_sdk/_client.py
elif isinstance(event, h2.events.ConnectionTerminated):
    self._goaway_received = True
    err = ProtocolError(
        f"GOAWAY received: error_code={event.error_code}, "
        f"last_stream_id={event.last_stream_id}",
        error_code=event.error_code,
    )
    # Only fail streams the server never processed.
    if event.last_stream_id is not None:
        for stream_id, state in self._streams.items():
            if stream_id > event.last_stream_id:
                self._fail_stream(state, err)
        # <-- BUG 🔴 pending streams are never failed in this branch
    else:
        self._fail_all_streams(err)

Explanation

The comment states the handler should "Only fail streams the server never processed." _pending_streams contains reserved streams that were never sent (reserve_stream() called but not send_headers()), so the server definitively never processed them. However, the last_stream_id is not None branch only fails _streams with IDs above last_stream_id and forgets to fail _pending_streams, leaving them to fail later with unrelated h2 connection-state errors.

Codebase Inconsistency

_fail_all_streams fails both active and pending streams, but the GOAWAY handler with a non-None last_stream_id does not:

def _fail_all_streams(self, error: BaseException) -> None:
    for state in self._streams.values():
        self._fail_stream(state, error)
    for state in self._pending_streams.values():
        self._fail_stream(state, error)

Recommended Fix

Fail _pending_streams in the last_stream_id is not None branch, matching _fail_all_streams and the comment’s intent:

if event.last_stream_id is not None:
    for stream_id, state in self._streams.items():
        if stream_id > event.last_stream_id:
            self._fail_stream(state, err)
    for state in self._pending_streams.values():
        self._fail_stream(state, err)

History

This bug was introduced in commit 3dc9795. The initial implementation included _pending_streams handling in _fail_all_streams but omitted it in the GOAWAY handler’s last_stream_id branch.