fix(proxy): fail over stuck HTTP bridge owner requests to a fresh account

[softkleenex]

Summary (English)

Intent: operators run long, autonomous Codex turns unattended (overnight, "leave it running and go to sleep"). Today, if the upstream OpenAI/ChatGPT side — not codex-lb, not the local machine — simply stops responding to one request (a hung connection, a quota/rate edge case, transient server flakiness), the client sees:

stream disconnected before completion: codex-lb is temporarily
overloaded during http_bridge_response_create_gate

...and the turn just sits there. Nothing crashes, nothing retries — it is silently dead until a human notices and manually intervenes. This PR's whole purpose is to make that class of interruption self-heal on the proxy side, so a hung upstream connection to one account doesn't kill an unattended run.

Root cause (traced end-to-end)

1. An HTTP bridge "owner" request is accepted upstream but the account never sends back a single response event (no response.created, no telemetry, nothing).
2. The owner's own keepalive loop had no failover of its own — it just emitted SSE keepalives until stream_idle_timeout_seconds (default 7200s, two hours) before giving up with a terminal error.
3. Separately, a different request waiting on that same session's gate times out after the much shorter http_responses_session_bridge_stuck_gate_retire_after_seconds (default 300s) and — only for a narrow set of requests — transparently retries on a different bridge. But if that waiter had already been reconnected once by its client (replay_count > 0), it was excluded from that retry even though replay count says nothing about whether this bridge attempt made upstream progress.

Net effect: a single flaky account can freeze a client-visible turn for anywhere from 5 minutes up to 2 hours, with no attempt to just try a different account.

What changes

def _http_bridge_owner_is_stuck(
    request_state: _WebSocketRequestState,
    *,
    yielded_any: bool,
    age_seconds: float,
    threshold_seconds: float,
) -> bool:
    # Mirrors the waiter-side stuck-gate predicate, but evaluated by the
    # owner request itself from inside its own keepalive loop. A previous-
    # response account pin makes moving accounts unsafe (continuation must
    # stay on the account that owns the prior response id), so those turns
    # are excluded here and left to the existing idle-timeout behavior.
    return (
        not yielded_any
        and request_state.response_id is None
        and request_state.response_event_count == 0
        and not request_state.downstream_visible
        and request_state.previous_response_id is None
        and age_seconds >= threshold_seconds
    )

• The owner's keepalive loop now runs this check on every tick. Once it fires, the proxy retires the session, records the failure against the account (so it isn't immediately reselected), picks a fresh eligible account, and resubmits the exact same request on a new bridge before yielding a single byte to the client. In the common case the client never even sees a hiccup — the turn just quietly continues on a healthier account.
• Continuity turns (previous_response_id set — i.e. this is a continuation of a prior response) are explicitly left alone. Moving those to a different account would be unsafe, so they keep the existing wait/idle-timeout behavior.
• The narrower waiter-side replacement predicate is broadened: a non-zero client-visible replay_count no longer disqualifies an otherwise definitively-unsubmitted waiter, since replay count reflects the client's reconnect history, not upstream progress on the current bridge attempt.
• Every other existing safeguard is untouched: any request that already has a response id, a response event, a downstream sequence number, or visible output is never silently retried.

Validation

$ uv run pytest tests/unit/test_proxy_http_bridge.py -k "owner_is_stuck or owner_stuck_gate or replacement_ignores_replay_count" -v
...
tests/unit/test_proxy_http_bridge.py::test_http_bridge_retired_gate_replacement_ignores_replay_count PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate[None-None-True] PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate[response_id-resp-owner-progressed-False] PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate[response_event_count-1-False] PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate[downstream_visible-True-False] PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate[previous_response_id-resp-prior-turn-False] PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_is_stuck_predicate_requires_age_and_no_output_yielded PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_stuck_gate_raises_after_threshold_with_no_pin PASSED
tests/unit/test_proxy_http_bridge.py::test_http_bridge_owner_stuck_gate_does_not_fire_with_continuity_pin PASSED

9 passed in 0.47s

• uv run ruff check app tests — clean
• uv run ruff format --check app tests — clean
• python3 scripts/check_proxy_architecture.py — passes (no capped file exceeded)
• uv run ty check — clean
• uv run pytest tests/unit — 4127 passed, 55 skipped
• uv run pytest tests/integration — 1554 passed, 8 skipped; the one unrelated failure (test_quota_planner_api.py::test_quota_planner_warm_now_keeps_bootstrap_for_metadata_less_primary_rows) was confirmed to reproduce identically on main with this diff stashed out — it is pre-existing and untouched by this PR.
• OpenSpec change proposal included at openspec/changes/failover-stuck-http-bridge-owner/ with proposal.md, tasks.md, and a MODIFIED Requirements delta against the proxy-admission-control capability spec.

---

요약 (한국어)

작업 의도: 사용자들은 장시간 자율 작업(밤새 돌려놓고 자는 식)을 codex-lb를 통해 실행합니다. 그런데 지금은 codex-lb나 로컬 환경이 아니라 OpenAI/ChatGPT 업스트림 서버 쪽에서 특정 요청 하나에 응답을 아예 안 주는 상황(연결 멈춤, 할당량/레이트리밋 경계 케이스, 일시적 서버 불안정 등)이 발생하면, 클라이언트에는 이렇게 뜹니다:

stream disconnected before completion: codex-lb is temporarily
overloaded during http_bridge_response_create_gate

...그리고 그 작업은 그냥 멈춰버립니다. 크래시도 안 나고, 재시도도 안 되고 — 사람이 알아채고 직접 개입할 때까지 조용히 죽어있는 상태가 됩니다. 이 PR의 목적은 정확히 이 유형의 중단을 프록시 단에서 스스로 회복시켜서, 계정 하나의 업스트림 연결이 멈췄다고 밤새 돌려놓은 작업 전체가 멎어버리는 일이 없게 하는 것입니다.

원인 (끝까지 추적함)

1. HTTP 브릿지의 "owner" 요청이 업스트림에 정상적으로 접수는 됐는데, 계정 쪽에서 응답 이벤트를 단 하나도 안 보내는 경우가 있습니다 (response.created도, 텔레메트리도, 아무것도 없음).
2. 이 owner 요청 자체는 자체적인 failover가 전혀 없었어요 — stream_idle_timeout_seconds(기본 7200초, 2시간)까지 그냥 keepalive만 보내다가 결국 터미널 에러로 끝났습니다.
3. 별개로, 같은 세션의 게이트를 "기다리던" 다른 요청은 훨씬 짧은 http_responses_session_bridge_stuck_gate_retire_after_seconds(기본 300초) 이후 타임아웃되고, 좁은 조건에서만 다른 브릿지로 투명하게 재시도합니다. 근데 그 대기 요청이 클라이언트 쪽에서 한 번이라도 재연결(replay_count > 0)됐던 적이 있으면, 실제로는 이 브릿지 시도 자체가 업스트림에 아무 진전도 없었는데도 그 재시도 대상에서 제외되고 있었습니다.

결과적으로: 계정 하나가 불안정하면 클라이언트가 보는 작업 턴이 짧으면 5분, 길면 2시간까지 그냥 멈춰버리고, 다른 계정으로 한번 시도해보려는 노력조차 없었습니다.

무엇을 바꿨나

• owner 요청의 keepalive 루프가 매 tick마다 위 코드의 판정을 돌립니다. 조건이 맞으면(응답 이벤트 0개, 화면에 아무것도 안 보임, 이전 응답 연속성 고정이 없음, 임계시간 초과) 세션을 정리하고, 해당 계정에 실패 기록을 남기고(바로 재선택 안 되게), 다른 정상 계정을 골라서 클라이언트에게 단 한 바이트도 보여주기 전에 완전히 동일한 요청을 새 브릿지에서 재제출합니다. 대부분의 경우 클라이언트는 끊긴 것조차 눈치 못 채고, 그냥 더 건강한 계정으로 조용히 이어집니다.
• previous_response_id가 있는 연속성(continuation) 턴은 명시적으로 건드리지 않습니다 — 그런 턴을 다른 계정으로 옮기는 건 안전하지 않아서, 기존의 대기/idle-timeout 동작을 그대로 유지합니다.
• 기존의 "대기자 재시도" 조건도 넓혔습니다: 클라이언트 쪽에서 재연결했던 횟수(replay_count)가 0이 아니라는 이유만으로 재시도 대상에서 제외되던 걸 없앴습니다 — 이 값은 클라이언트의 재연결 이력일 뿐, 현재 브릿지 시도의 업스트림 진행 상태와는 무관하기 때문입니다.
• 그 외 기존 안전장치는 전부 그대로입니다: 이미 response id, response 이벤트, downstream sequence number, 화면에 보여진 출력이 있는 요청은 절대 조용히 재시도되지 않습니다.

검증

• uv run ruff check app tests — 통과
• uv run ruff format --check app tests — 통과
• python3 scripts/check_proxy_architecture.py — 통과 (파일별 줄수 상한 안 넘음)
• uv run ty check — 통과
• uv run pytest tests/unit — 4127개 통과, 55개 skip
• uv run pytest tests/integration — 1554개 통과, 8개 skip. 무관한 실패 1개(test_quota_planner_api.py::test_quota_planner_warm_now_keeps_bootstrap_for_metadata_less_primary_rows)는 이 변경사항을 stash로 빼고 main에서 그대로 돌려도 동일하게 재현되는 걸 확인했습니다 — 이 PR과 무관한 기존 문제입니다.
• openspec/changes/failover-stuck-http-bridge-owner/에 제안서(proposal.md, tasks.md) 및 proxy-admission-control 캐퍼빌리티 스펙에 대한 MODIFIED Requirements 델타를 포함했습니다.

Test plan

• Unit: predicate coverage for the new owner-stuck check (positive/negative for every disqualifying field)
• Unit: owner-stuck keepalive loop raises and retires when no continuity pin is present
• Unit: owner-stuck check does not fire when a previous-response account pin is present
• Unit: waiter replacement predicate accepts a replayed-but-otherwise-unsubmitted waiter
• Existing regression suite (unit + integration) unaffected

[softkleenex]

Rebased onto latest `main` (`9b40f74`).

The `Lint (ruff)` job's failure (`proxy architecture check failed: service.py has 2604 lines; limit is 2600`) is a pre-existing issue on `main` itself, unrelated to this PR — this branch never touches `service.py`. Verified by checking out bare `origin/main` in a separate worktree and running `python3 scripts/check_proxy_architecture.py` directly against it with none of this PR's changes applied:

$ git worktree add /tmp/main-check origin/main
$ cd /tmp/main-check && python3 scripts/check_proxy_architecture.py
proxy architecture check failed: service.py has 2604 lines; limit is 2600

`app/modules/proxy/_service/http_bridge/streaming.py` (the only file this PR modifies with a line-count-relevant check) has no cap in `scripts/check_proxy_architecture.py`, so this PR's diff doesn't interact with the failing check at all. Flagging in case a separate fix for `service.py`'s line count is already in flight — happy to rebase again once that lands.

[softkleenex]

Heads up on the failing `Lint (ruff)` / `CI Required` checks: this is a pre-existing architecture-check failure, unrelated to this PR's diff — `service.py` is already 4 lines over the `check_proxy_architecture.py` line-count gate on current `main` itself (confirmed by checking out unmodified `main` directly). Opened #1416 as a small, standalone fix for it.

[Soju06]

Reviewed the current head (f7a51cb). Two gaps between the spec text and the implementation:

1. The proposal/spec says the proxy "retires the session, excludes that account for this attempt, selects a fresh eligible account" — but the failover call to _get_or_create_http_bridge_session in streaming.py never passes exclude_account_ids (the parameter already exists on main, app/modules/proxy/_service/http_bridge/mixin.py:362). Only the _handle_stream_error penalty is recorded, so the load balancer can legally re-select the exact account that just wedged for the "replacement" bridge.
2. request_state.started_at is never reset for the replacement attempt. _http_bridge_owner_is_stuck computes age from started_at inside the keepalive loop, and the replacement stream reuses the same request_state — so on the replacement session's first keepalive tick the predicate is already true again (age >= threshold, response_event_count == 0), the fresh session gets retired, and the re-raised response_create_gate_owner_stuck escapes the failover block as a terminal client error. The failover only succeeds if the replacement yields an event before its first keepalive interval.

Direction-wise we've consolidated on the #1410 watchdog track for this failure family (see the #1394 discussion). Once that lands, please rebase this on top and rework the owner-side failover against that baseline — the replay_count relaxation in _http_bridge_can_replace_retired_gate_session looks independently useful.

[github-actions]

This pull request has been automatically marked as stale because it has had no activity for 7 days.

It will be closed in 23 more days unless there is new activity.

If this is still relevant, please:

• Rebase or push an update if the branch drifted
• Address pending review feedback if there is any
• Leave a short comment confirming it is still being worked on

Thanks for the contribution 🙏

[softkleenex]

Still tracked — no action needed on my end right now. Per the 07-24 review, this is consolidating onto the #1410/#1394 watchdog track for the silent-upstream failure family; once that lands I'll rebase this PR on top and rework the owner-side failover against that baseline (keeping the replay_count relaxation in _http_bridge_can_replace_retired_gate_session that was called out as independently useful). Not stale, just waiting on the upstream dependency.

[Soju06]

@softkleenex the dependency you were parked on has landed: #1394 merged to main on 08-04 (a66f793) as the canonical fix for the silent-upstream family, and #1410 was closed as superseded — its watchdog refinements (anchored 2x retire cap, upstream-activity silence clock, eventless missing-response.created deadline) are in #1394's final form. http_bridge/streaming.py was substantially reworked there, so plan on a rebase onto current main rather than a conflict fix when you rework the owner-side failover. The replay_count relaxation in _http_bridge_can_replace_retired_gate_session remains independently wanted.

[softkleenex]

Rebased onto current main (`51fdddc2`) — and re-scoped the change substantially, so flagging clearly rather than presenting it as a quiet rebase.

Why the scope changed: this PR's original core feature — an owner-side stuck-gate detector that retires the session, excludes the account, and resubmits fresh — is now covered by `recover-fresh-hard-bridge-timeouts` (landed as part of #1394): its "Fresh hard bridge requests may recover across accounts" requirement does the same thing via a bounded eventless `response.created` watchdog, anchored to when the create request was actually sent (not `started_at`, which was one of the two gaps you found in the 07-24 review) and already excluding the failed account on retry. Re-implementing a second, parallel owner-side detector on top of that would just duplicate it. I also did not carry forward this PR's original account-penalization step (`_handle_stream_error` on the stuck account) — #1394's mechanism deliberately treats "no `response.created`" as upstream-ambiguous rather than proof the account is bad, and every one of its failure paths passes `penalize_account=False`; reintroducing penalization would contradict that design decision rather than build on it, so I left it alone.

What's left and still genuinely open turned out to be on a different, older code path — the waiter-side gate replacement (`_http_bridge_can_replace_retired_gate_session`), which #1394 didn't touch:

1. The `replay_count` relaxation you confirmed is still wanted — done, verbatim as discussed.
2. While rebasing I found this predicate's call site never added the retired session's account to `exclude_account_ids` before building the replacement — the same class of gap as your first 07-24 finding, just in this sibling function. Fixed: a waiter's replacement now excludes the account whose gate it was just waiting behind (continuity-pinned waiters are unaffected, they stay pinned as before).

Updated `openspec/changes/failover-stuck-http-bridge-owner` to describe this narrower scope and the supersession explicitly. Full suite (7118 passed, 3 pre-existing-and-unrelated deselected — the quota-planner flake noted on #1417 and the two `test_proxy_websocket_responses.py` failures that reproduce on a clean `upstream/main` checkout), ty check, ruff check/format, and the architecture-check script all green.

If you intended something broader by "rework the owner-side failover" than what #1394 already covers, let me know and I'll take another pass — this is my best read of what's actually still missing after reading through #1394's mechanism in detail.

[Komzpa]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Soju06]

Reviewed the re-scoped head (51fdddc). The re-scope is the right call — #1394's eventless response.created watchdog does supersede the owner-side detector, and both remaining waiter-side gaps are real: _http_bridge_can_replace_retired_gate_session on main still carries the replay_count == 0 disqualifier, and the replacement call site already passes exclude_account_ids=request_state.excluded_account_ids or None (streaming.py:2270) without anything populating it on this path. The replay_count relaxation is safe as argued via the remaining definitively-unsubmitted markers.

One correctness gap in the exclusion branch, though — streaming.py:2240-2246:

if request_state.previous_response_id is not None and replacement_preferred_account_id is None:
    replacement_preferred_account_id = session.account.id
else:
    request_state.excluded_account_ids.add(session.account.id)

The else also fires for account-pinned waiters: (a) a continuity waiter whose preferred_account_id was already resolved to the owner — which is the normal case, since streaming.py:1285-1289 resolves it from the durable bridge / live bridge / previous-response index before session creation, and a hard-strength session is created on that required account, so session.account.id == preferred; (b) a file-pinned waiter (rewritten_file_account_id set, previous_response_id None). In both cases the replacement is built with preferred_account_id=owner, exclude_account_ids={owner}, and fallback_on_preferred_account_unavailable=False (streaming.py:2263-2265). Exclusion filters the candidate pool before selection (load_balancer.py:447), so mixin.py:1802-1812 raises previous_response_owner_unavailable / preferred_account_unavailable — a terminal 503 where main would retry the pinned account on a fresh session. This contradicts the PR's own spec delta ("An anchored waiter MUST remain pinned to the previous-response owner account") and the inline comment, which only holds when preferred_account_id was still None. Since the exclusion persists on request_state.excluded_account_ids, it also poisons later recovery calls (streaming.py:1805, 2270) for the rest of the request.

Fix is one line — exclude only when the replacement is genuinely unpinned:

elif replacement_preferred_account_id is None:
    request_state.excluded_account_ids.add(session.account.id)

plus a regression test for the pinned case (continuity waiter with resolved preferred_account_id hitting the replacement path; assert the owner is not excluded and the replacement stays pinned).

Minor: the comment/spec rationale that replay_count "reflects client-side reconnect attempts" is imprecise — it is incremented at proxy-side upstream resubmission points too (request_submit.py:1838, request_submit.py:2376, websocket/helpers.py:364). The relaxation is justified by the definitively-unsubmitted markers, not the counter's provenance; please reword so the wrong justification doesn't get relied on later.

Everything else checks out: no duplication of #1394's mechanism, no penalization reintroduced, CI green, and the durable-full-resend test update (test_proxy_http_bridge.py:18971) matches the new exclusion semantics. Happy to merge once the pinned-waiter guard lands.

[softkleenex]

Good catch — fixed in `586ca050`.

Changed the guard from `else:` to `elif replacement_preferred_account_id is None:` exactly as you suggested, so a waiter whose replacement is already required to land on a specific account (previous-response owner already resolved, or a file-pinned account) keeps that account and is never added to `excluded_account_ids`. Added `test_stream_via_http_bridge_replaces_retired_hard_gate_keeps_pinned_account_unexcluded` covering that case directly (pre-set `preferred_account_id` matching the retired session's account, no `previous_response_id`) — asserts the replacement call gets `preferred_account_id` unchanged and `exclude_account_ids is None`.

Also reworded the `replay_count` justification in the code comment, proposal, and spec — dropped "reflects client-side reconnect attempts" and pointed at the predicate's other definitively-unsubmitted markers instead, per your note that the counter is incremented at proxy-side resubmission points too.

Rebased onto current main in the process (unrelated upstream drift, no conflicts). Full suite (7585 passed, same 3 pre-existing/unrelated deselections as before), ty check, ruff check/format, and the architecture-check script all green.

[softkleenex]

Heads up: Type check (ty) is now failing (CI Required too) — tests/integration/test_db_commit_durability.py:178 calls ApiKeysRepository.update_last_used, which no longer exists (likely removed/renamed by #1627, "coalesce last_used_at writes behind a periodic flush"). Confirmed this is pre-existing on a clean, unmodified upstream/main checkout (currently 410bfaea) — reproduces identically in a separate worktree, nothing in this branch touches that file or ApiKeysRepository. Not something I can fix from here without guessing at the intended replacement API; flagging since it'll block CI Required on any PR rebased past #1627 until it's fixed on main.

[softkleenex]

Same root cause also fails Tests (pytest, PostgreSQL) (test_db_commit_durability.py::test_usage_reservation_creation_and_settlement_relax_commit_durability, same AttributeError: 'ApiKeysRepository' object has no attribute 'update_last_used') — both failures trace to the one pre-existing gap noted above, not two separate issues.