[Detail Bug] HTTP/2 unary requests can hang indefinitely during body upload after stream reset (lost wake-up)

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_3c932e82-3e28-49af-92e5-a367fbec32c6

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

Summary

• Context: Connection.send_data in src/s2_sdk/_client.py streams a request body over HTTP/2, blocking on the per-stream flow-control window (state.window_updated) when the server hasn't granted more credit.
• Bug: When the receive loop fails the stream (sets state.error and state.window_updated) while send_data is parked waiting for _write_lock, send_data resumes and unconditionally calls state.window_updated.clear() without re-checking state.error. This erases the wake signal that the (now-dead) receive loop already delivered; the subsequent await state.window_updated.wait() hangs forever because nothing will ever set the event again.
• Actual vs. expected: send_data should raise the connection/stream error already recorded on state.error; instead it blocks indefinitely.
• Impact: In HttpClient.unary_request the body upload is await conn.send_data(stream_id, body, end_stream=True) with no asyncio.wait_for, so a hung send_data is not bounded by request_timeout and the request hangs permanently.

Code with Bug

# src/s2_sdk/_client.py — Connection.send_data
        offset = 0
        while offset < len(data):
            state = self._streams.get(stream_id)
            if state and state.error:                       # (A) only place state.error is checked
                raise state.error

            sent = False
            async with self._write_lock:                   # (B) send_data parks here when the recv
                window = self._h2.local_flow_control_window(stream_id)   #     loop holds this lock
                if window > 0:
                    ...
                    sent = True

            if sent:
                ...
                continue

            # Window exhausted — wait for update (lock released).
            state = self._streams.get(stream_id)
            if state:
                state.window_updated.clear()                # <-- BUG 🔴 clears failure wake-up without re-checking state.error
                # Re-check under lock to avoid missing an update.
                async with self._write_lock:
                    window = self._h2.local_flow_control_window(stream_id)
                if window <= 0:
                    await state.window_updated.wait()       # (hangs forever)

# src/s2_sdk/_client.py — HttpClient.unary_request
            if body is not None:
                assert stream_id is not None
                await conn.send_data(stream_id, body, end_stream=True)   # <-- BUG 🔴 unbounded wait; request_timeout does not apply

            resp_headers = await asyncio.wait_for(
                state.response_headers,
                timeout=self._request_timeout,
            )

Explanation

• send_data only checks state.error at the top of the loop. If it is blocked on _write_lock while the recv loop processes inbound frames under the same lock, the recv loop can fail the stream via _fail_stream(...) (e.g., on StreamReset / GOAWAY), setting state.error and calling state.window_updated.set().
• Once send_data acquires _write_lock and releases it, it takes the flow-control wait branch and calls state.window_updated.clear(), erasing the already-delivered wake-up. It then waits on the event, but no code will ever set it again because the stream is already failed.
• Verified behavior: after a RST_STREAM, h2’s local_flow_control_window(stream_id) remains 0 (does not raise and does not change), so the window re-check cannot break the wait.

Codebase Inconsistency

• There is an in-source claim that “Caller-level timeouts (unary or streaming) will cancel this if it takes too long”, but unary_request does not wrap send_data in asyncio.wait_for, so the unary path does not actually enforce request_timeout during upload.

Recommended Fix

Hold _write_lock across clear() + window re-check and re-check state.error (and state is None) before waiting, so a wake signal from a failed stream cannot be erased:

            state = self._streams.get(stream_id)
            if state is None:
                raise ConnectionClosedError("Stream closed during send")
            if state.error is not None:
                raise state.error
            async with self._write_lock:              # clear + re-check under the same lock
                state.window_updated.clear()
                window = self._h2.local_flow_control_window(stream_id)
                if window > 0:
                    continue
            if window <= 0:
                await state.window_updated.wait()

History

This bug was introduced in commit 3dc9795. The commit "feat: add initial version of s2-sdk (#1)" authored the entire src/s2_sdk/_client.py from scratch, including the send_data flow-control wait loop that placed state.window_updated.clear() outside _write_lock without re-checking state.error, and the accompanying unary_request call to await conn.send_data(...) with no asyncio.wait_for.