Skip to content

fix(proxy): bound upstream streams that never send response headers - #1611

Merged
Soju06 merged 2 commits into
Soju06:mainfrom
joschi655:fix/bound-stalled-upstream-streams
Aug 6, 2026
Merged

fix(proxy): bound upstream streams that never send response headers#1611
Soju06 merged 2 commits into
Soju06:mainfrom
joschi655:fix/bound-stalled-upstream-streams

Conversation

@joschi655

Copy link
Copy Markdown
Contributor

Why

An established upstream connection that never sends response headers has no short bound.
The shared session is built with ClientTimeout(total=None), streaming requests pass
sock_read=None, and upstream_connect_timeout_seconds covers only the handshake, so the
only applicable limit is http_responses_stream_request_budget_seconds (7200s by default).
stream_idle_timeout_seconds does not apply, because it guards _iter_sse_events, which
only runs once headers exist.

That matters because the waiting request holds its session's response_create_gate
(asyncio.Semaphore(1)) for the entire window.

Observed on a single-tenant deployment (older build) on 2026-08-05:

http_bridge_startup_wait_timeout stage=response_create_gate
  bridge_key=sha256:… available=0 pending_count=1 queued_count=5
  pending_request_ids=d27c4984-… pending_request_ages_seconds=1548.9

The holder produced no upstream event and no request_logs row; five later turns on the
same session retried gate acquisition every 10s for 26 minutes, and the Codex client gave
up with stream disconnected before completion: idle timeout waiting for SSE. Other
traffic on the same account completed normally throughout, so quota, account health, and
the client-side network path were not involved.

What changes

  • Pre-first-byte bound. sock_read carries the effective stream idle timeout on both
    streaming request paths. aiohttp.SocketTimeoutError is mapped to
    StreamIdleTimeoutError at the stream boundary so the existing stream_idle_timeout
    reporting, retry, and failover paths apply unchanged — the generic aiohttp.ClientError
    handler runs first and would otherwise report a silent established connection as an
    unavailable upstream. The idle-vs-budget tie-break in the timeout classifier is
    untouched.
  • TCP keepalive probes on upstream sockets via the connector socket_factory, so a
    connection dropped by an intermediary (NAT rebind, tunnel reconnect, route change)
    surfaces as a transport error instead of an indefinite wait. keepalive_timeout=90 and
    ttl_dns_cache=300 are unchanged; probe tuning is best-effort because TCP_KEEPIDLE is
    Linux-only and macOS spells it TCP_KEEPALIVE.
  • Unroutable-event logging. A bridge session multiplexes one upstream connection
    across its pending requests; an event matching none of them is dropped without a trace
    today, which makes "upstream went quiet" and "the event was not routed" look identical.
    The log is low-cardinality and carries no response ids, cache keys, or payload content,
    and stays silent when nothing is pending so drain and retirement paths do not add noise.

No new settings, no migration, no dashboard surface.

Scope note

The retirement side of this failure is already handled on main by
_http_bridge_pending_state_is_stale, which uses last_upstream_activity_at as the
silence clock. That is cleanup of the symptom; this PR removes the condition that
produces it. Kept as separate concerns.

Tests

  • tests/unit/test_http_client.py — keepalive enabled on constructed sockets, connector
    wiring, and tolerated unsupported probe options
  • tests/unit/test_proxy_utils.py — silence before response headers reports
    stream_idle_timeout with sock_read set to the idle budget; unroutable event is
    logged without leaking identifiers

uv run pytest tests/unit/test_http_client.py tests/unit/test_proxy_utils.py → 962 passed.
uv run ruff check / ruff format --check clean.

OpenSpec

openspec/changes/bound-stalled-upstream-streams/ covers outbound-http-clients (added
pre-first-byte bound, modified connector requirement) and proxy-runtime-observability
(added unroutable-event logging). The OpenSpec CLI was not available in my environment, so
openspec validate --strict still needs a run here — task 4.3 is left unchecked for that
reason.

Johannes Breitfeld and others added 2 commits August 5, 2026 19:38
An established upstream connection that never answers is bounded only by
the 7200s request budget: the session uses ClientTimeout(total=None),
streaming requests pass sock_read=None, and the connect timeout covers
only the handshake. The stream idle timeout does not apply because it
guards the SSE reader, which starts at the first byte.

The waiting request holds its session's response_create_gate for that
whole window, so later turns on the same session queue behind a request
that will never produce anything.

- Carry the effective idle timeout into sock_read on both streaming
  paths, and map aiohttp.SocketTimeoutError to StreamIdleTimeoutError so
  the existing stream_idle_timeout reporting and failover apply. The
  generic ClientError handler runs first, hence the mapping at the
  stream boundary; the idle-vs-budget tie-break is unchanged.
- Enable SO_KEEPALIVE on upstream sockets so a connection dropped by an
  intermediary surfaces as a transport error instead of an indefinite
  wait. Probe tuning is best-effort per platform.
- Log upstream bridge events that match no pending request, so a routing
  drop is visible instead of being inferred from a missing response.

No new settings and no migration.
@Soju06
Soju06 force-pushed the fix/bound-stalled-upstream-streams branch from 98b0988 to 407738b Compare August 6, 2026 03:51
@Soju06

Soju06 commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Triage review passed (details: the pre-response-header transport gap is real — sock_read=None on both streaming paths with ClientTimeout(total=None) on the shared session — and the SocketTimeoutError→StreamIdleTimeoutError mapping is required to keep it out of the generic upstream_unavailable classification; two adversarial passes found no blocking issue). CI flagged one ty diagnostic in the new test fixture, so I pushed the trivial annotation fix to your branch to keep this moving: self.timeouts now admits None and the assertion narrows before touching sock_read (407738b).

Three non-blocking notes for the record, no action needed pre-merge: (1) the incident narrative overstates coverage — the HTTP-bridge upstream is a websockets-library connection, which neither sock_read nor the aiohttp keepalive factory bounds; that path stays with the #1394 watchdog family (and #1580). (2) socket_factory is a silent no-op under SOCKS (aiohttp_socks delegates socket creation to python_socks). (3) The unroutable-event warning can fire once per delta for a late-streaming retired response — log volume only.

@Soju06
Soju06 merged commit af5051f into Soju06:main Aug 6, 2026
29 of 31 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.

3 participants