Skip to content

fix: pending streams not failed when GOAWAY is received #50

Merged
quettabit merged 1 commit into
mainfrom
qb/iss-44
May 11, 2026
Merged

fix: pending streams not failed when GOAWAY is received #50
quettabit merged 1 commit into
mainfrom
qb/iss-44

Conversation

@quettabit
Copy link
Copy Markdown
Member

closes #44

@quettabit
Copy link
Copy Markdown
Member Author

@greptileai review pls

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR fixes a bug where streams reserved via reserve_stream() but not yet promoted to active streams (i.e., send_headers not yet called) were left in a hanging state when a GOAWAY frame was received from the server with a non-null last_stream_id.

  • Root cause fixed: The ConnectionTerminated handler already iterated _streams to fail those with IDs above last_stream_id, but _pending_streams — entries that have no stream ID yet and are therefore guaranteed unprocessed by the server — were never touched in that branch.
  • Fix: Two lines mirror the existing _fail_all_streams pattern by iterating _pending_streams.values() and calling _fail_stream on each, which idempotently sets the error and unblocks any awaiting coroutines.

Confidence Score: 5/5

Safe to merge — the fix is small, targeted, and consistent with the existing stream-failure pattern.

The change adds two lines that mirror the already-tested _fail_all_streams pattern. Pending streams have no assigned stream ID, so they are by definition never processed by the server; failing them on GOAWAY is always correct. The iteration is safe under _write_lock, _fail_stream is idempotent, and send_headers already checks state.error before promoting a pending stream, so the error surfaces cleanly to callers.

No files require special attention.

Important Files Changed

Filename Overview
src/s2_sdk/_client.py Added two lines to the GOAWAY handler that fail all pending (pre-header-send) streams; correctly mirrors the existing _fail_all_streams pattern and is safe under the _write_lock.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Connection
    participant H2

    Caller->>Connection: reserve_stream()
    Connection-->>Caller: state (added to _pending_streams)

    note over Connection,H2: GOAWAY received before send_headers()

    H2-->>Connection: "ConnectionTerminated (last_stream_id=N)"
    Connection->>Connection: _handle_event()
    alt last_stream_id is not None
        loop "_streams where stream_id > N"
            Connection->>Connection: _fail_stream(state, err)
        end
        loop _pending_streams (NEW in this PR)
            Connection->>Connection: _fail_stream(state, err)
        end
    else last_stream_id is None
        Connection->>Connection: _fail_all_streams(err)
    end

    Caller->>Connection: send_headers(state, ...)
    Connection-->>Caller: raises state.error (ProtocolError)
Loading

Reviews (2): Last reviewed commit: "initial commit" | Re-trigger Greptile

@quettabit quettabit marked this pull request as ready for review May 11, 2026 23:27
@quettabit quettabit requested a review from a team as a code owner May 11, 2026 23:27
@quettabit quettabit merged commit 74baa61 into main May 11, 2026
9 checks passed
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.

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

1 participant