Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions app/modules/proxy/_service/http_bridge/streaming.py
Original file line number Diff line number Diff line change
Expand Up @@ -509,8 +509,13 @@ def _http_bridge_can_replace_retired_gate_session(
) -> bool:
# A gate timeout happens before this waiter is appended or sent. Once the
# stale owner has retired the session, only that fully cleaned pre-submit
# state is safe to carry to a replacement; any response/replay/downstream
# marker makes the upstream acceptance boundary ambiguous.
# state is safe to carry to a replacement; any response/downstream marker
# makes the upstream acceptance boundary ambiguous. replay_count is
# incremented at proxy-side resubmission points too, not only on client
# reconnects, so it says nothing about upstream progress on *this* bridge
# attempt either way; it's the other, definitively-unsubmitted markers
# below that establish the boundary is unambiguous, so replay_count must
# not disqualify replacement on its own.
code, _message = _proxy_error_code_message(exc)
return (
code == "response_create_gate_timeout"
Expand All @@ -521,7 +526,6 @@ def _http_bridge_can_replace_retired_gate_session(
and request_state.event_queue is not None
and request_state.response_id is None
and request_state.response_event_count == 0
and request_state.replay_count == 0
and request_state.last_downstream_sequence_number is None
and not request_state.downstream_visible
and not request_state.awaiting_response_created
Expand Down Expand Up @@ -2677,6 +2681,19 @@ async def rollback_pre_dispatch_recovery_claim() -> None:
replacement_preferred_account_id = request_state.preferred_account_id
if request_state.previous_response_id is not None and replacement_preferred_account_id is None:
replacement_preferred_account_id = session.account.id
elif replacement_preferred_account_id is None:
# The retired owner already proved stuck; a "replacement"
# that could legally reselect that same account isn't a
# replacement at all. An already-pinned waiter (continuity
# owner resolved above, or a file-pinned account) must
# stay pinned instead — excluding its required account
# here would make that account's own replacement
# impossible (fallback_on_preferred_account_unavailable is
# False for exactly this pinned case below) and would
# keep poisoning every later recovery call on this
# request, since excluded_account_ids persists on
# request_state.
request_state.excluded_account_ids.add(session.account.id)
while True:
try:
replacement_session = await self._get_or_create_http_bridge_session(
Expand Down
86 changes: 86 additions & 0 deletions openspec/changes/failover-stuck-http-bridge-owner/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
## Why

This change originally proposed giving the HTTP bridge *owner* request itself
a stuck-gate failover: when the owner produced zero response events for too
long, retire its session, exclude that account, select a fresh eligible
account, and resubmit — mirroring what the existing gate-timeout *waiter*
path already did for a second request stuck behind the same gate.

Since this was first proposed, `recover-fresh-hard-bridge-timeouts` (shipped
as part of #1394, "stabilize silent and clean-close recovery") landed a
bounded eventless `response.created` watchdog that does almost exactly this
for the owner request: when a hard, pre-response request with no
previous-response id, continuity anchor, proxy-injected anchor, or
account-scoped file ownership reaches that watchdog with zero response
events, pre-response recovery excludes the failed account and may resubmit
on a fresh one — see that capability's "Fresh hard bridge requests may
recover across accounts" requirement. That supersedes the core of what this
change originally asked for. Two differences worth naming rather than
silently re-implementing over: the watchdog's deadline is anchored to when
the create request was actually sent upstream (not the request's overall
`started_at`) and capped tighter than the flat stuck-gate threshold, and it
deliberately does not penalize the account's health — every failure path in
that mechanism treats "no `response.created`" as upstream-ambiguous, not
proof the account itself is bad. This proposal does not reopen either
design decision.

What's left, and still genuinely unaddressed, is on the *waiter* side of the
picture — a different, older code path
(`_http_bridge_can_replace_retired_gate_session`) that decides whether a
second request, timing out behind a session another request already wedged,
may be transparently resubmitted on a replacement bridge once that session
is retired:

1. A waiter whose `replay_count` is non-zero is disqualified from that
replacement today. `replay_count` isn't a clean "client reconnected"
signal — it's also incremented at proxy-side upstream resubmission
points, so it says nothing about upstream progress on the *current*
bridge attempt either way. The predicate's other markers (no response id,
no response event, no downstream sequence number, not downstream-visible)
already establish that the waiter is definitively unsubmitted; an
otherwise fully unsubmitted waiter is exactly as safe to move regardless
of its replay count.
2. The replacement session this path creates does not exclude the account
whose gate just proved stuck, so the load balancer can legally reselect
the exact same wedged account for the "replacement" — the same class of
gap this change originally raised, just in a sibling function the
eventless-watchdog rework didn't touch.

## What Changes

- Drop `request_state.replay_count == 0` from
`_http_bridge_can_replace_retired_gate_session`'s guard. A waiter is
disqualified by any response id, response event, downstream sequence
marker, or visible output — never by replay count alone.
- When that predicate accepts a waiter for replacement and the replacement
is not already pinned to a required account (no previous-response owner
resolved, no file-pinned account), add the retired session's account to
`request_state.excluded_account_ids` before building the replacement
session, so the fresh bridge cannot legally reselect the account that
just proved stuck. A waiter whose replacement *is* pinned must not have
its required account excluded — that account's own unavailability is a
separate, pre-existing failure mode this change does not touch, and
excluding it here would make its own required-account replacement
impossible and poison every later recovery call on the request, since
`excluded_account_ids` persists on `request_state`.
- No changes to the owner-side eventless watchdog, its threshold, its
account-neutral (no-penalization) treatment, or continuity-pinned
recovery — all of that is `recover-fresh-hard-bridge-timeouts`'s territory
and is left exactly as it is.

## Impact

- Affected capability: `proxy-admission-control`.
- A waiter with a non-zero `replay_count` now gets the same transparent
gate-replacement path as one with `replay_count == 0`, provided its
current bridge attempt is still definitively unsubmitted.
- An unpinned waiter's replacement bridge can no longer land back on the
account whose gate it was just waiting behind.
- A pinned waiter's (continuity or file-owned) replacement stays pinned to
its required account exactly as before — this change does not exclude it.
- No behavior change for continuity (previous-response-owner) waiters, which
remain pinned to their required account.
- No behavior change for any request that has already produced a response
id, response event, downstream sequence number, or visible output.
- No behavior change to the owner-side eventless watchdog added by
`recover-fresh-hard-bridge-timeouts`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
## MODIFIED Requirements

### Requirement: Stuck HTTP bridge response-create gate sessions are retired

When a visible HTTP bridge request times out waiting for a per-session
response-create gate, the proxy MUST retire the bridge session only if a
pending visible request still owns the gate, is still awaiting
`response.created`, has not produced downstream-visible output, and its age
meets or exceeds the configured stuck-gate retirement threshold. Receiving a
non-visible upstream event before `response.created`, including
`codex.rate_limits`, MUST NOT by itself suppress retirement because such an
event neither assigns the response nor releases the gate. The retirement MUST
emit a structured low-cardinality log and a Prometheus counter without raw keys
or prompt content. Pre-created `response.*` lifecycle activity MUST count as
response progress and suppress stuck-gate retirement even when it has not yet
produced downstream-visible text. If the timing-out waiter has hard affinity
and remains definitively unsubmitted, with no upstream response or downstream
sequence markers, the proxy MUST acquire a fresh bridge and submit that
waiter once within its original request deadline; a non-zero client-visible
replay counter MUST NOT by itself disqualify a waiter from this replacement,
since the other definitively-unsubmitted markers already establish the
upstream acceptance boundary is unambiguous regardless of replay count. When
the replacement is not already required to land on a specific account (no
previous-response owner resolved, no file-pinned account), the replacement
bridge MUST exclude the account whose gate just proved stuck. A waiter whose
replacement is pinned to a required account (a previous-response owner or a
file-pinned account) MUST remain pinned to that account, and that required
account MUST NOT be excluded on its behalf. The proxy MUST NOT reuse the
retired session object or transparently retry an ambiguously submitted
request.

#### Scenario: Leading rate-limit telemetry does not mask a stuck pre-created request

- **GIVEN** a visible HTTP bridge request owns the response-create gate
- **AND** upstream emits `codex.rate_limits` but never emits `response.created`
- **AND** the pending request becomes older than the configured stuck-gate retirement threshold
- **WHEN** another visible request times out waiting for that gate
- **THEN** the proxy retires the stuck bridge session
- **AND** if the waiter has hard affinity and is still definitively unsubmitted, the proxy submits it once on a fresh bridge
- **AND** the waiter keeps its original deadline and any previous-response account pin

#### Scenario: A reconnected waiter is not disqualified from replacement by its own replay count

- **GIVEN** a gate waiter has already reconnected once (`replay_count` is non-zero)
- **AND** the waiter otherwise has no response id, response event, downstream sequence number, or visible output
- **WHEN** its bridge is retired during gate contention
- **THEN** the proxy still submits that waiter once on a fresh bridge

#### Scenario: Ambiguous waiter is not moved to a replacement bridge

- **GIVEN** a gate waiter has a response event, downstream sequence, visible output, or pending-queue membership
- **WHEN** its bridge is retired during gate contention
- **THEN** the proxy does not transparently submit that waiter on another bridge

#### Scenario: Replacement bridge excludes the account that just proved stuck

- **GIVEN** an unpinned gate waiter (no previous-response owner, no file-pinned account) is accepted for replacement after its session is retired
- **WHEN** the proxy builds the replacement bridge session
- **THEN** account selection for that replacement excludes the retired session's account

#### Scenario: A pinned waiter's replacement keeps its required account, unexcluded

- **GIVEN** a gate waiter's replacement is required to land on a previous-response owner or a file-pinned account
- **AND** that required account is the same account whose gate just proved stuck
- **WHEN** the proxy builds the replacement bridge session
- **THEN** the replacement remains pinned to that required account
- **AND** that account is not added to the request's excluded-account set

#### Scenario: Healthy active stream is not retired during a normal wait

- **GIVEN** a pending HTTP bridge request has received `response.created` or produced downstream-visible output
- **WHEN** another visible request times out waiting for the gate
- **THEN** the proxy does not classify the active stream as a stuck pre-created gate owner

#### Scenario: Pre-created response lifecycle activity is not retired

- **GIVEN** a pending HTTP bridge request has not received `response.created`
- **BUT** upstream is emitting `response.*` lifecycle events for that request
- **WHEN** another visible request times out waiting for the gate
- **THEN** the proxy does not retire the actively progressing request
43 changes: 43 additions & 0 deletions openspec/changes/failover-stuck-http-bridge-owner/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Tasks

- [x] Investigate whether `recover-fresh-hard-bridge-timeouts` (#1394) already
covers this change's original owner-side stuck-gate failover — confirm
it does (its "Fresh hard bridge requests may recover across accounts"
requirement), and re-scope this change to what's still open instead of
duplicating that mechanism.
- [x] Drop `request_state.replay_count == 0` from
`_http_bridge_can_replace_retired_gate_session`'s guard.
- [x] When that predicate accepts a waiter with no previous-response account
pin, add the retired session's account to
`request_state.excluded_account_ids` before building the replacement
session.
- [x] Update `test_http_bridge_retired_gate_replacement_requires_unsubmitted_waiter`
to drop its now-stale `replay_count` case, and add
`test_http_bridge_retired_gate_replacement_ignores_replay_count`
asserting a waiter with `replay_count=1` is still accepted.
- [x] Add `test_stream_via_http_bridge_replaces_retired_hard_gate_excludes_stuck_account`
covering the account-exclusion fix end to end.
- [x] Update `test_stream_via_http_bridge_projects_plaintext_durable_full_resend_when_owner_is_unavailable`'s
`replace_retired_gate=True` assertion, which previously locked in the
gap this change fixes (a second stuck account was not excluded from a
third replacement attempt).
- [x] Run focused and full test suites, ruff check/format, `ty check`, and
the proxy architecture-check script.
- [x] Fix a correctness gap found in review (08-06): the exclusion branch
also fired for a waiter whose replacement is already required to land
on a specific account (a resolved previous-response owner, or a
file-pinned account) — excluding that required account made its own
replacement impossible and poisoned later recovery calls on the
request, since `excluded_account_ids` persists on `request_state`.
Only exclude when the replacement is genuinely unpinned
(`replacement_preferred_account_id is None`).
- [x] Add `test_stream_via_http_bridge_replaces_retired_hard_gate_keeps_pinned_account_unexcluded`
covering the pinned-waiter case.
- [x] Reword the `replay_count` relaxation's justification (in code comments
and this spec) away from "reflects client-side reconnect attempts" —
it's also incremented at proxy-side resubmission points, so that
framing is imprecise. The relaxation is justified by the predicate's
other definitively-unsubmitted markers, not by what increments the
counter.
- [x] Re-run focused and full test suites, ruff check/format, `ty check`,
and the architecture-check script after the fix.
Loading
Loading