fix(proxy): fail over stuck HTTP bridge owner requests to a fresh account - #1401
fix(proxy): fail over stuck HTTP bridge owner requests to a fresh account#1401softkleenex wants to merge 1 commit into
Conversation
38b1fd6 to
f7a51cb
Compare
|
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: `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. |
|
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. |
|
Reviewed the current head (
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 |
|
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:
Thanks for the contribution 🙏 |
|
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 |
|
@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- |
Rebased onto main after Soju06#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.
f7a51cb to
51fdddc
Compare
|
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:
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. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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:
...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)
response.created, no telemetry, nothing).stream_idle_timeout_seconds(default 7200s, two hours) before giving up with a terminal error.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
previous_response_idset — 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.replay_countno longer disqualifies an otherwise definitively-unsubmitted waiter, since replay count reflects the client's reconnect history, not upstream progress on the current bridge attempt.Validation
uv run ruff check app tests— cleanuv run ruff format --check app tests— cleanpython3 scripts/check_proxy_architecture.py— passes (no capped file exceeded)uv run ty check— cleanuv run pytest tests/unit— 4127 passed, 55 skippeduv 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 onmainwith this diff stashed out — it is pre-existing and untouched by this PR.openspec/changes/failover-stuck-http-bridge-owner/withproposal.md,tasks.md, and aMODIFIED Requirementsdelta against theproxy-admission-controlcapability spec.요약 (한국어)
작업 의도: 사용자들은 장시간 자율 작업(밤새 돌려놓고 자는 식)을 codex-lb를 통해 실행합니다. 그런데 지금은 codex-lb나 로컬 환경이 아니라 OpenAI/ChatGPT 업스트림 서버 쪽에서 특정 요청 하나에 응답을 아예 안 주는 상황(연결 멈춤, 할당량/레이트리밋 경계 케이스, 일시적 서버 불안정 등)이 발생하면, 클라이언트에는 이렇게 뜹니다:
...그리고 그 작업은 그냥 멈춰버립니다. 크래시도 안 나고, 재시도도 안 되고 — 사람이 알아채고 직접 개입할 때까지 조용히 죽어있는 상태가 됩니다. 이 PR의 목적은 정확히 이 유형의 중단을 프록시 단에서 스스로 회복시켜서, 계정 하나의 업스트림 연결이 멈췄다고 밤새 돌려놓은 작업 전체가 멎어버리는 일이 없게 하는 것입니다.
원인 (끝까지 추적함)
response.created도, 텔레메트리도, 아무것도 없음).stream_idle_timeout_seconds(기본 7200초, 2시간)까지 그냥 keepalive만 보내다가 결국 터미널 에러로 끝났습니다.http_responses_session_bridge_stuck_gate_retire_after_seconds(기본 300초) 이후 타임아웃되고, 좁은 조건에서만 다른 브릿지로 투명하게 재시도합니다. 근데 그 대기 요청이 클라이언트 쪽에서 한 번이라도 재연결(replay_count > 0)됐던 적이 있으면, 실제로는 이 브릿지 시도 자체가 업스트림에 아무 진전도 없었는데도 그 재시도 대상에서 제외되고 있었습니다.결과적으로: 계정 하나가 불안정하면 클라이언트가 보는 작업 턴이 짧으면 5분, 길면 2시간까지 그냥 멈춰버리고, 다른 계정으로 한번 시도해보려는 노력조차 없었습니다.
무엇을 바꿨나
previous_response_id가 있는 연속성(continuation) 턴은 명시적으로 건드리지 않습니다 — 그런 턴을 다른 계정으로 옮기는 건 안전하지 않아서, 기존의 대기/idle-timeout 동작을 그대로 유지합니다.replay_count)가 0이 아니라는 이유만으로 재시도 대상에서 제외되던 걸 없앴습니다 — 이 값은 클라이언트의 재연결 이력일 뿐, 현재 브릿지 시도의 업스트림 진행 상태와는 무관하기 때문입니다.검증
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개 skipuv 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