From 9b159ab5391acd483e1e131df698397ba95475cc Mon Sep 17 00:00:00 2001 From: softkleenex Date: Tue, 4 Aug 2026 18:32:35 +0900 Subject: [PATCH 1/2] fix(proxy): stop disqualifying replayed waiters, exclude stuck account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto main after #1394 ("stabilize silent and clean-close recovery") landed its own bounded eventless response.created watchdog, which already covers this change's original owner-side stuck-gate failover (see recover-fresh-hard-bridge-timeouts's "Fresh hard bridge requests may recover across accounts"). Re-scoped to what's still open on the separate waiter-side gate-replacement path (_http_bridge_can_replace_retired_gate_session): - A waiter whose client already reconnected once (replay_count > 0) is no longer disqualified from transparent replacement on its own — replay count reflects client reconnects, not upstream progress on the current bridge attempt. - A waiter's replacement bridge now excludes the account whose gate it was just waiting behind, so it can't legally land back on the exact account that just proved stuck. --- .../proxy/_service/http_bridge/streaming.py | 15 +- .../proposal.md | 76 +++++++++ .../specs/proxy-admission-control/spec.md | 69 ++++++++ .../failover-stuck-http-bridge-owner/tasks.md | 25 +++ tests/unit/test_proxy_http_bridge.py | 157 +++++++++++++++++- 5 files changed, 337 insertions(+), 5 deletions(-) create mode 100644 openspec/changes/failover-stuck-http-bridge-owner/proposal.md create mode 100644 openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md create mode 100644 openspec/changes/failover-stuck-http-bridge-owner/tasks.md diff --git a/app/modules/proxy/_service/http_bridge/streaming.py b/app/modules/proxy/_service/http_bridge/streaming.py index ff3054e3c0..200842de4a 100644 --- a/app/modules/proxy/_service/http_bridge/streaming.py +++ b/app/modules/proxy/_service/http_bridge/streaming.py @@ -509,8 +509,11 @@ 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. A non-zero replay + # count reflects client-side reconnect attempts, not upstream progress on + # *this* bridge attempt, so it does not by itself make the boundary + # ambiguous and must not disqualify replacement on its own. code, _message = _proxy_error_code_message(exc) return ( code == "response_create_gate_timeout" @@ -521,7 +524,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 @@ -2677,6 +2679,13 @@ 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 + else: + # The retired owner already proved stuck; a "replacement" + # that could legally reselect that same account isn't a + # replacement at all. Continuity turns are exempted above + # because they're pinned to this account by + # replacement_preferred_account_id instead. + request_state.excluded_account_ids.add(session.account.id) while True: try: replacement_session = await self._get_or_create_http_bridge_session( diff --git a/openspec/changes/failover-stuck-http-bridge-owner/proposal.md b/openspec/changes/failover-stuck-http-bridge-owner/proposal.md new file mode 100644 index 0000000000..eefc7fb556 --- /dev/null +++ b/openspec/changes/failover-stuck-http-bridge-owner/proposal.md @@ -0,0 +1,76 @@ +## 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 client already reconnected once (`replay_count > 0`) is + disqualified from that replacement today, even though replay count + reflects the client's own reconnect behavior, not upstream progress on + the *current* bridge attempt — 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 waiter has + no previous-response account pin — a continuity turn keeps recovering + onto its required account exactly as before), 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. +- 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 client whose prior connection dropped and reconnected once + (`replay_count > 0`) now gets the same transparent gate-replacement path + as a first-attempt waiter, provided its current bridge attempt is still + definitively unsubmitted. +- A waiter's replacement bridge can no longer land back on the account whose + gate it was just waiting behind. +- 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`. diff --git a/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md b/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md new file mode 100644 index 0000000000..fe06875d64 --- /dev/null +++ b/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md @@ -0,0 +1,69 @@ +## 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 it reflects client-side reconnect attempts rather than upstream +progress on the current bridge attempt. When the waiter has no +previous-response account pin, the replacement bridge MUST exclude the +account whose gate just proved stuck. An anchored waiter MUST remain pinned +to the previous-response owner account. 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** a gate waiter with no previous-response account pin 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 +- **AND** a continuity-pinned waiter's replacement remains pinned to its required account instead + +#### 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 diff --git a/openspec/changes/failover-stuck-http-bridge-owner/tasks.md b/openspec/changes/failover-stuck-http-bridge-owner/tasks.md new file mode 100644 index 0000000000..84fb82cec0 --- /dev/null +++ b/openspec/changes/failover-stuck-http-bridge-owner/tasks.md @@ -0,0 +1,25 @@ +# 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. diff --git a/tests/unit/test_proxy_http_bridge.py b/tests/unit/test_proxy_http_bridge.py index c69c140764..78b51ae0fc 100644 --- a/tests/unit/test_proxy_http_bridge.py +++ b/tests/unit/test_proxy_http_bridge.py @@ -2667,7 +2667,6 @@ async def test_http_bridge_gate_contention_does_not_retry_retired_session( [ ("response_id", "resp-already-created"), ("response_event_count", 1), - ("replay_count", 1), ("last_downstream_sequence_number", 0), ("downstream_visible", True), ("awaiting_response_created", True), @@ -2745,6 +2744,37 @@ def test_http_bridge_retired_gate_replacement_accepts_cleaned_hard_affinity_wait ) +def test_http_bridge_retired_gate_replacement_ignores_replay_count() -> None: + """A non-zero replay_count reflects the client's own reconnect attempts, + not upstream progress on this bridge attempt, so an otherwise fully + unsubmitted waiter must still be replaceable.""" + session = _make_bridge_session(key_value="sid-gate-replacement-replayed") + session.closed = True + request_state = proxy_service._WebSocketRequestState( + request_id="req-gate-replacement-replayed", + model="gpt-5.4", + service_tier=None, + reasoning_effort=None, + api_key_reservation=None, + started_at=time.monotonic(), + transport="http", + request_text='{"type":"response.create"}', + event_queue=asyncio.Queue(), + replay_count=1, + ) + gate_timeout_error = http_bridge_helpers_module._http_bridge_startup_wait_timeout_error( + "http_bridge_response_create_gate", + code="response_create_gate_timeout", + ) + + assert http_bridge_streaming_module._http_bridge_can_replace_retired_gate_session( + gate_timeout_error, + session=session, + request_state=request_state, + request_was_enqueued=False, + ) + + @pytest.mark.asyncio async def test_http_bridge_submit_gate_contention_still_reroutes_soft_sessions( monkeypatch: pytest.MonkeyPatch, @@ -6107,6 +6137,123 @@ async def fake_submit( assert "event=replace_retired_gate" in caplog.text +@pytest.mark.asyncio +async def test_stream_via_http_bridge_replaces_retired_hard_gate_excludes_stuck_account( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Unlike a continuity (previous-response-owner) turn — which is + intentionally re-pinned to the same account via preferred_account_id — + a plain waiter's replacement session must exclude the account whose gate + session just proved stuck, or the load balancer could legally reselect + the exact same wedged account for the "replacement".""" + service = proxy_service.ProxyService(cast(Any, nullcontext())) + payload = proxy_service.ResponsesRequest.model_validate( + { + "model": "gpt-5.6-sol", + "instructions": "hi", + "input": "continue", + } + ) + retired_session = _make_bridge_session(key_value="sid-retired-gate-exclude") + replacement_session = _make_bridge_session(key_value="sid-retired-gate-exclude") + get_or_create = AsyncMock(side_effect=[retired_session, replacement_session]) + request_state = proxy_service._WebSocketRequestState( + request_id="req-retired-gate-exclude", + model="gpt-5.6-sol", + service_tier=None, + reasoning_effort=None, + api_key_reservation=None, + started_at=time.monotonic(), + transport="http", + request_text='{"type":"response.create","model":"gpt-5.6-sol"}', + event_queue=asyncio.Queue(), + ) + gate_timeout_error = http_bridge_helpers_module._http_bridge_startup_wait_timeout_error( + "http_bridge_response_create_gate", + code="response_create_gate_timeout", + ) + + def fake_prepare( + _prepared_payload: proxy_service.ResponsesRequest, + _headers: dict[str, str] | Any, + **_kwargs: object, + ) -> tuple[proxy_service._WebSocketRequestState, str]: + return request_state, request_state.request_text or "{}" + + async def fake_submit( + session: proxy_service._HTTPBridgeSession, + *, + request_state: proxy_service._WebSocketRequestState, + text_data: str, + queue_limit: int, + ) -> None: + del text_data, queue_limit + if session is retired_session: + retired_session.closed = True + request_state.awaiting_response_created = False + request_state.response_create_gate = None + request_state.response_create_gate_acquired = False + raise gate_timeout_error + assert session is replacement_session + assert request_state.event_queue is not None + request_state.event_queue.put_nowait( + 'data: {"type":"response.completed","response":{"id":"resp-replaced-gate-exclude"}}\n\n' + ) + request_state.event_queue.put_nowait(None) + + monkeypatch.setattr( + proxy_service, + "get_settings_cache", + lambda: cast( + Any, + SimpleNamespace( + get=AsyncMock( + return_value=SimpleNamespace( + sticky_threads_enabled=False, + openai_cache_affinity_max_age_seconds=1800, + http_responses_session_bridge_prompt_cache_idle_ttl_seconds=3600, + http_responses_session_bridge_gateway_safe_mode=False, + ) + ) + ), + ), + ) + monkeypatch.setattr(proxy_service, "get_settings", lambda: _make_app_settings()) + monkeypatch.setattr(service._durable_bridge, "lookup_request_targets", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_resolve_websocket_previous_response_owner", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_resolve_file_account_for_responses", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_prepare_http_bridge_request", fake_prepare) + monkeypatch.setattr(service, "_get_or_create_http_bridge_session", get_or_create) + submit = AsyncMock(side_effect=fake_submit) + detach = AsyncMock() + monkeypatch.setattr(service, "_submit_http_bridge_request", submit) + monkeypatch.setattr(service, "_detach_http_bridge_request", detach) + + chunks = [ + chunk + async for chunk in service._stream_via_http_bridge( + payload, + headers={"session_id": "sid-retired-gate-exclude"}, + codex_session_affinity=True, + propagate_http_errors=True, + openai_cache_affinity=True, + api_key=None, + api_key_reservation=None, + suppress_text_done_events=False, + idle_ttl_seconds=120.0, + codex_idle_ttl_seconds=1800.0, + max_sessions=8, + queue_limit=4, + ) + ] + + assert chunks == ['data: {"type":"response.completed","response":{"id":"resp-replaced-gate-exclude"}}\n\n'] + assert get_or_create.await_count == 2 + _initial_call, replacement_call = get_or_create.await_args_list + assert replacement_call.kwargs["preferred_account_id"] is None + assert replacement_call.kwargs["exclude_account_ids"] == {retired_session.account.id} + + @pytest.mark.asyncio async def test_stream_via_http_bridge_soft_prompt_cache_queue_full_reroutes( monkeypatch: pytest.MonkeyPatch, @@ -20045,7 +20192,13 @@ async def fake_stream_events( assert third_call.kwargs["previous_response_id"] is None assert third_call.kwargs["preferred_account_id"] is None assert third_call.kwargs["durable_lookup"] is None - assert third_call.kwargs["exclude_account_ids"] == {"acc-owner"} + # When the fresh-replay session's own gate later times out (session.account + # is "acc-fallback"), the next replacement must also exclude it — a + # "replacement" that could legally reselect the account that just proved + # stuck isn't a replacement at all. + assert third_call.kwargs["exclude_account_ids"] == ( + {"acc-owner", "acc-fallback"} if replace_retired_gate else {"acc-owner"} + ) assert third_call.kwargs["allow_forward_to_owner"] is False assert captured_request_states[0].previous_response_id is None assert captured_request_states[0].enforce_openai_sdk_contract is False From 4b91e997fc8e47d8715ad6a0d0affb39e7b773a6 Mon Sep 17 00:00:00 2001 From: softkleenex Date: Fri, 7 Aug 2026 10:29:13 +0900 Subject: [PATCH 2/2] fix(proxy): don't exclude an already-pinned waiter's required account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a correctness gap: 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), not only for genuinely unpinned waiters. Excluding that required account made its own replacement impossible (fallback_on_preferred_account_unavailable is False for exactly this pinned case) and poisoned every later recovery call on the request, since excluded_account_ids persists on request_state. Only exclude when the replacement is genuinely unpinned. Also reworded the replay_count relaxation's justification away from "reflects client-side reconnect attempts" — it's also incremented at proxy-side resubmission points, so that framing was imprecise. The relaxation is justified by the predicate's other definitively-unsubmitted markers, not by what increments the counter. --- .../proxy/_service/http_bridge/streaming.py | 24 ++-- .../proposal.md | 44 ++++--- .../specs/proxy-admission-control/spec.md | 27 ++-- .../failover-stuck-http-bridge-owner/tasks.md | 18 +++ tests/unit/test_proxy_http_bridge.py | 120 ++++++++++++++++++ 5 files changed, 200 insertions(+), 33 deletions(-) diff --git a/app/modules/proxy/_service/http_bridge/streaming.py b/app/modules/proxy/_service/http_bridge/streaming.py index 200842de4a..22397b2d49 100644 --- a/app/modules/proxy/_service/http_bridge/streaming.py +++ b/app/modules/proxy/_service/http_bridge/streaming.py @@ -510,10 +510,12 @@ def _http_bridge_can_replace_retired_gate_session( # 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/downstream marker - # makes the upstream acceptance boundary ambiguous. A non-zero replay - # count reflects client-side reconnect attempts, not upstream progress on - # *this* bridge attempt, so it does not by itself make the boundary - # ambiguous and must not disqualify replacement on its own. + # 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" @@ -2679,12 +2681,18 @@ 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 - else: + 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. Continuity turns are exempted above - # because they're pinned to this account by - # replacement_preferred_account_id instead. + # 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: diff --git a/openspec/changes/failover-stuck-http-bridge-owner/proposal.md b/openspec/changes/failover-stuck-http-bridge-owner/proposal.md index eefc7fb556..749597a655 100644 --- a/openspec/changes/failover-stuck-http-bridge-owner/proposal.md +++ b/openspec/changes/failover-stuck-http-bridge-owner/proposal.md @@ -31,11 +31,15 @@ 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 client already reconnected once (`replay_count > 0`) is - disqualified from that replacement today, even though replay count - reflects the client's own reconnect behavior, not upstream progress on - the *current* bridge attempt — an otherwise fully unsubmitted waiter is - exactly as safe to move regardless of its replay count. +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 @@ -48,12 +52,17 @@ is retired: `_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 waiter has - no previous-response account pin — a continuity turn keeps recovering - onto its required account exactly as before), 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. +- 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 @@ -62,12 +71,13 @@ is retired: ## Impact - Affected capability: `proxy-admission-control`. -- A client whose prior connection dropped and reconnected once - (`replay_count > 0`) now gets the same transparent gate-replacement path - as a first-attempt waiter, provided its current bridge attempt is still - definitively unsubmitted. -- A waiter's replacement bridge can no longer land back on the account whose - gate it was just waiting behind. +- 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 diff --git a/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md b/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md index fe06875d64..f6a98cf249 100644 --- a/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md +++ b/openspec/changes/failover-stuck-http-bridge-owner/specs/proxy-admission-control/spec.md @@ -18,12 +18,16 @@ 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 it reflects client-side reconnect attempts rather than upstream -progress on the current bridge attempt. When the waiter has no -previous-response account pin, the replacement bridge MUST exclude the -account whose gate just proved stuck. An anchored waiter MUST remain pinned -to the previous-response owner account. The proxy MUST NOT reuse the retired -session object or transparently retry an ambiguously submitted request. +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 @@ -50,10 +54,17 @@ session object or transparently retry an ambiguously submitted request. #### Scenario: Replacement bridge excludes the account that just proved stuck -- **GIVEN** a gate waiter with no previous-response account pin is accepted for replacement after its session is retired +- **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 -- **AND** a continuity-pinned waiter's replacement remains pinned to its required account instead + +#### 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 diff --git a/openspec/changes/failover-stuck-http-bridge-owner/tasks.md b/openspec/changes/failover-stuck-http-bridge-owner/tasks.md index 84fb82cec0..9b39683af1 100644 --- a/openspec/changes/failover-stuck-http-bridge-owner/tasks.md +++ b/openspec/changes/failover-stuck-http-bridge-owner/tasks.md @@ -23,3 +23,21 @@ 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. diff --git a/tests/unit/test_proxy_http_bridge.py b/tests/unit/test_proxy_http_bridge.py index 78b51ae0fc..ccac94302e 100644 --- a/tests/unit/test_proxy_http_bridge.py +++ b/tests/unit/test_proxy_http_bridge.py @@ -6254,6 +6254,126 @@ async def fake_submit( assert replacement_call.kwargs["exclude_account_ids"] == {retired_session.account.id} +@pytest.mark.asyncio +async def test_stream_via_http_bridge_replaces_retired_hard_gate_keeps_pinned_account_unexcluded( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A waiter whose replacement is already required to land on a specific + account (a resolved previous-response owner, or a file-pinned account — + simulated here directly via a pre-set preferred_account_id with no + previous_response_id) must keep that account, unexcluded, even though it + is the same account whose gate just proved stuck. Excluding a waiter's + own required account would make its required-account replacement + impossible and poison every later recovery call on the request.""" + service = proxy_service.ProxyService(cast(Any, nullcontext())) + payload = proxy_service.ResponsesRequest.model_validate( + { + "model": "gpt-5.6-sol", + "instructions": "hi", + "input": "continue", + } + ) + retired_session = _make_bridge_session(key_value="sid-retired-gate-pinned") + replacement_session = _make_bridge_session(key_value="sid-retired-gate-pinned") + get_or_create = AsyncMock(side_effect=[retired_session, replacement_session]) + request_state = proxy_service._WebSocketRequestState( + request_id="req-retired-gate-pinned", + model="gpt-5.6-sol", + service_tier=None, + reasoning_effort=None, + api_key_reservation=None, + started_at=time.monotonic(), + transport="http", + request_text='{"type":"response.create","model":"gpt-5.6-sol"}', + event_queue=asyncio.Queue(), + preferred_account_id=retired_session.account.id, + ) + gate_timeout_error = http_bridge_helpers_module._http_bridge_startup_wait_timeout_error( + "http_bridge_response_create_gate", + code="response_create_gate_timeout", + ) + + def fake_prepare( + _prepared_payload: proxy_service.ResponsesRequest, + _headers: dict[str, str] | Any, + **_kwargs: object, + ) -> tuple[proxy_service._WebSocketRequestState, str]: + return request_state, request_state.request_text or "{}" + + async def fake_submit( + session: proxy_service._HTTPBridgeSession, + *, + request_state: proxy_service._WebSocketRequestState, + text_data: str, + queue_limit: int, + ) -> None: + del text_data, queue_limit + if session is retired_session: + retired_session.closed = True + request_state.awaiting_response_created = False + request_state.response_create_gate = None + request_state.response_create_gate_acquired = False + raise gate_timeout_error + assert session is replacement_session + assert request_state.event_queue is not None + request_state.event_queue.put_nowait( + 'data: {"type":"response.completed","response":{"id":"resp-replaced-gate-pinned"}}\n\n' + ) + request_state.event_queue.put_nowait(None) + + monkeypatch.setattr( + proxy_service, + "get_settings_cache", + lambda: cast( + Any, + SimpleNamespace( + get=AsyncMock( + return_value=SimpleNamespace( + sticky_threads_enabled=False, + openai_cache_affinity_max_age_seconds=1800, + http_responses_session_bridge_prompt_cache_idle_ttl_seconds=3600, + http_responses_session_bridge_gateway_safe_mode=False, + ) + ) + ), + ), + ) + monkeypatch.setattr(proxy_service, "get_settings", lambda: _make_app_settings()) + monkeypatch.setattr(service._durable_bridge, "lookup_request_targets", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_resolve_websocket_previous_response_owner", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_resolve_file_account_for_responses", AsyncMock(return_value=None)) + monkeypatch.setattr(service, "_prepare_http_bridge_request", fake_prepare) + monkeypatch.setattr(service, "_get_or_create_http_bridge_session", get_or_create) + submit = AsyncMock(side_effect=fake_submit) + detach = AsyncMock() + monkeypatch.setattr(service, "_submit_http_bridge_request", submit) + monkeypatch.setattr(service, "_detach_http_bridge_request", detach) + + chunks = [ + chunk + async for chunk in service._stream_via_http_bridge( + payload, + headers={"session_id": "sid-retired-gate-pinned"}, + codex_session_affinity=True, + propagate_http_errors=True, + openai_cache_affinity=True, + api_key=None, + api_key_reservation=None, + suppress_text_done_events=False, + idle_ttl_seconds=120.0, + codex_idle_ttl_seconds=1800.0, + max_sessions=8, + queue_limit=4, + ) + ] + + assert chunks == ['data: {"type":"response.completed","response":{"id":"resp-replaced-gate-pinned"}}\n\n'] + assert get_or_create.await_count == 2 + _initial_call, replacement_call = get_or_create.await_args_list + assert replacement_call.kwargs["preferred_account_id"] == retired_session.account.id + assert replacement_call.kwargs["exclude_account_ids"] is None + + @pytest.mark.asyncio async def test_stream_via_http_bridge_soft_prompt_cache_queue_full_reroutes( monkeypatch: pytest.MonkeyPatch,