Skip to content

fix(proxy): fail over dead account proxy routes before upstream dispatch - #1542

Merged
Soju06 merged 4 commits into
mainfrom
fix/dead-account-proxy-route-failover
Aug 6, 2026
Merged

fix(proxy): fail over dead account proxy routes before upstream dispatch#1542
Soju06 merged 4 commits into
mainfrom
fix/dead-account-proxy-route-failover

Conversation

@Soju06

@Soju06 Soju06 commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

Fix account-bound proxy connection failures that occur before upstream dispatch so movable Responses requests fail over transparently instead of repeatedly selecting the dead route. Ambiguous POST failures and hard account ownership stay fail-closed.

Supersedes #1322 — original implementation by @luawl, taken over per the maintainer notice on that PR. The original commit is preserved with @luawl's authorship, rebuilt onto current main: since #1322 was opened, main independently adopted the sanitized dispatch-provenance layer (retryable_same_contract + failure_phase == "connect" via is_pre_dispatch_connection_failure), so this rework reuses that single source of truth instead of the PR's original dispatch_state enum, and reimplements the failover semantics against the current selection/bridge architecture (#1395/#1476/#1492/#1499-era).

Fixes #1314

Changes

  • Add is_confirmed_pre_dispatch_transport_error as the single predicate authorizing cross-account replay: proven pre-dispatch connect provenance only, explicitly excluding host-wide network loss (proxy_network_unavailable, which keeps its account-neutral process recovery path) and TLS verification failures.
  • Allow a confirmed pre-dispatch connect failure to try the next endpoint in the same resolved proxy pool even for a non-idempotent POST (no request bytes reached upstream).
  • Fail over movable requests to another eligible account across raw HTTP/SSE streaming, native Responses WebSocket connects, and HTTP bridge session startup (_HTTPBridgePreDispatchFailover), bounded by the existing attempt limits, request deadlines, and a monotonic exclusion set.
  • Release the failed account's stream lease before recording health, then jump straight to the existing bounded transient backoff floor (record_error_backoff, shared ERROR_BACKOFF_THRESHOLD: 30s floor, 300s cap) so independent requests stop rediscovering the dead route one generic error at a time. No pause/deactivation/rate-limit/quota penalty.
  • Preserve the original sanitized 502 when no replacement exists (instead of a generated no_accounts), and fail closed on it for previous-response, turn-state, uploaded-file, and required-account contracts. The request-scoped API-key reservation stays singular across internal failover and settles exactly once.
  • Carry pre-dispatch provenance across the routed WebSocket sanitizing conversion in connect_responses_websocket (previously collapsed to the process-network case only).
  • Ambiguous dispatch outcomes and idle disconnects never authorize replay or the backoff floor.

Diff composition

  • production: 283 additions / 26 deletions across 10 files
  • regression tests: 1,063 additions across 9 files
  • OpenSpec: openspec/changes/retry-account-proxy-connect-failures/ (174 additions), strict validation passes

Test plan

pytest tests/unit/test_proxy_utils.py tests/unit/test_proxy_http_bridge.py -q                       → 1301 passed
pytest tests/unit/test_codex_client.py tests/unit/test_codex_upstream_paths.py \
  tests/unit/test_load_balancer_concurrency.py tests/unit/test_proxy_websocket_client.py -q         → 189 passed
pytest tests/integration/test_proxy_responses.py tests/integration/test_proxy_websocket_responses.py \
  tests/integration/test_http_responses_bridge.py -q                                                → 250 passed
pytest tests/integration/test_token_refresh_claims.py -q                                            → 42 passed
ruff check / ruff format --check / ty check / scripts/check_proxy_architecture.py                   → pass
openspec validate retry-account-proxy-connect-failures --strict                                     → valid

The new streaming (3) and bridge (3) regressions were verified to fail against unpatched main (dead route surfaced terminally / bridge startup raised to the client) and pass with this change.

🤖 Generated with Claude Code

@Soju06

Soju06 commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44bc1cc738

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/proxy/_service/streaming/retry.py Outdated
Comment thread app/modules/proxy/_service/streaming/retry.py Outdated
@mereyabdenbekuly-ctrl

mereyabdenbekuly-ctrl commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

I picked up the remaining review fixes. Follow-up PR #1546 targets this PR's feature branch and addresses both unresolved P1s:

  • settlement/release is confirmed before keyed dead-route health writes across raw SSE, native WebSocket, and HTTP bridge
  • post-refresh confirmed proxy failures now respect hard ownership boundaries and preserve the replacement attempt's terminal outcome
  • HTTP-bridge cleanup is reservation-lifecycle-owned, including submit ownership transfer and cross-generation/concurrent-drain races

Commit: 5615948e
Validation: 5096 unit tests passed; 1510 affected unit/integration tests passed; Ruff, ty, and all 48 OpenSpec specs passed.

Please merge #1546 into fix/dead-account-proxy-route-failover; after that these review threads should be ready to resolve.

Update: GitHub CI is now fully green (CI Required and every applicable job passed). #1546 is MERGEABLE/CLEAN with no review findings or unresolved threads, so it is ready to merge into this feature branch.

@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Jul 30, 2026
@Komzpa

Komzpa commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@mereyabdenbekuly-ctrl

Copy link
Copy Markdown
Contributor

?

@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 1, 2026
@mereyabdenbekuly-ctrl

Copy link
Copy Markdown
Contributor

I rebased the full three-commit PR series onto current upstream/main (c539a200) and resolved the single additive conflict by preserving the new capability-routing helpers while retaining the websocket proxy failover regression test.

Because my account does not have write permission to Soju06/codex-lb, GitHub rejected a direct force-with-lease update of this PR head with HTTP 403. The tested rebased series is available at:

  • fork branch: mereyabdenbekuly-ctrl:codex/pr-1542-review-fixes
  • HEAD: c067e603
  • commits: 0e52433f, a1226d0c, c067e603

Maintainer handoff:

git fetch https://github.com/mereyabdenbekuly-ctrl/codex-lb.git codex/pr-1542-review-fixes
git switch fix/dead-account-proxy-route-failover
git reset --hard FETCH_HEAD
git push --force-with-lease origin fix/dead-account-proxy-route-failover

Validation on the rebased HEAD: 1,795 affected unit/integration tests passed; Ruff check/format, proxy architecture checks, ty, all 49 OpenSpec specs, and strict validation of retry-account-proxy-connect-failures passed. PR #1546 has been closed as superseded because its original fix was already cherry-picked here as 5a410f5f.

@mereyabdenbekuly-ctrl

Copy link
Copy Markdown
Contributor

Both remaining P1 review threads are already marked outdated and are addressed by the settlement/ownership changes in the rebased c067e603 series. I attempted to resolve them through the GitHub GraphQL API, but GitHub returned FORBIDDEN because this contributor account lacks ResolveReviewThread permission. A maintainer will need to resolve those two threads after updating the PR head.

@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Adopted @mereyabdenbekuly-ctrl's fork rebase (c067e603, branch codex/pr-1542-review-fixes) — thank you for carrying the series over the #1562 conflict and posting the handoff commands. Authorship of all three commits is preserved exactly (luawl / Soju06 / SSY), now rebased and force-pushed as 542905c6.

Topped up over today's merges on main:

The two outdated review threads on streaming/retry.py (deferred health writes until reservation settlement; ownership-gated post-refresh failover) are addressed by the adopted series and re-verified after the rebase.

Tests: test_proxy_http_bridge, test_proxy_utils, test_codex_client, test_proxy_websocket_client, test_codex_upstream_paths, test_load_balancer_concurrency (1602 passed) plus integration test_http_responses_bridge, test_proxy_websocket_responses, test_proxy_responses (282 passed); ruff check/format clean. PR is MERGEABLE again.

@Soju06
Soju06 force-pushed the fix/dead-account-proxy-route-failover branch from 542905c to 4750bd7 Compare August 4, 2026 08:50
@Komzpa

Komzpa commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@Komzpa Komzpa removed the needs rebase Needs rebase or conflict repair against current main label Aug 4, 2026
@jaekwonhong

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4750bd7190

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1968 to +1972
can_try_other_account = (
not require_preferred_account
and account.id != file_preferred_account_id
and attempt < max_attempts - 1
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow verified fresh replays past dead owner routes

When the first HTTP/SSE stream attempt for a locally verified full-resend continuation hits a confirmed pre-dispatch proxy connect failure, this check runs while require_preferred_account is still true, so the branch raises and returns the dead owner's sanitized 502 instead of using _move_verified_fresh_replay_from_owner to strip the owner anchor and select a healthy account. The same verified replay is already allowed for refresh/post-refresh failures, and without it a safe full-resend previous_response_id request remains pinned to an account whose proxy never dispatched any bytes; run the verified-owner replay step (with the same file/turn-state/single-account guards used below) before deciding this is hard ownership.

AGENTS.md reference: AGENTS.md:L105-L108

Useful? React with 👍 / 👎.

@Soju06

Soju06 commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Confirmed — the confirmed pre-dispatch branch in app/modules/proxy/_service/streaming/retry.py raises under require_preferred_account without attempting _move_verified_fresh_replay_from_owner, while the post-visible failover path at ~L2023 does. Since a confirmed pre-dispatch connect failure guarantees zero upstream bytes, a locally verified full-resend replay is safe to move off the dead owner. Will run the verified-owner replay step (same file/turn-state/single-account guards) ahead of the hard-ownership raise before merging.

@Soju06
Soju06 force-pushed the fix/dead-account-proxy-route-failover branch from 4750bd7 to a265419 Compare August 6, 2026 05:07
luawl and others added 3 commits August 6, 2026 07:48
An account-bound upstream proxy can stop accepting connections while the
account stays administratively active. Movable Responses requests selected
onto that account then failed with a terminal sanitized 502 (or a bridge
startup error) instead of failing over, and client retries could select the
same dead route again (#1314).

Rebuilt from PR #1322 onto current main, which already carries the sanitized
pre-dispatch provenance (`retryable_same_contract` + `failure_phase ==
"connect"`) this change originally introduced as a dispatch-state enum:

- `is_confirmed_pre_dispatch_transport_error` is the single predicate that
  authorizes cross-account replay; host-wide network loss keeps its
  account-neutral process recovery path and TLS verification failures stay
  non-replayable.
- A confirmed pre-dispatch connect failure may try the next endpoint in the
  same resolved proxy pool even for a non-idempotent POST.
- Raw HTTP/SSE streaming, native Responses WebSocket connects, and HTTP
  bridge session startup release the failed account's stream lease, record
  the bounded transient backoff floor (`record_error_backoff`, shared
  `ERROR_BACKOFF_THRESHOLD`), exclude the account, and retry another
  eligible account within the existing attempt and deadline budgets.
- Hard previous-response/turn-state/file/single-account ownership fails
  closed on the original sanitized failure without crossing accounts, and
  selection exhaustion preserves that failure instead of generating
  `no_accounts`.
- The API-key reservation stays request-scoped and singular across internal
  failover; ambiguous POST dispatch outcomes are surfaced without replay.

Fixes #1314
Supersedes #1322

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lover

Maintainer follow-up to the #1322 takeover: pin the negative boundaries of
the confirmed pre-dispatch predicate at the externally observable surfaces.

- TLS verification connect failures do not authorize same-pool POST
  fallback or cross-account websocket replay.
- Host-wide network loss (proxy_network_unavailable) and post-dispatch
  body-read failures never classify as confirmed pre-dispatch.
- An idle bridge disconnect does not exclude the account or record the
  transient backoff floor, so healthy accounts stay healthy.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re-dispatch hard-ownership raise

A confirmed pre-dispatch connect failure guarantees zero upstream bytes,
so the confirmed dead-route branch can safely run the verified-owner
replay step (same file/turn-state/single-account guards as the
post-refresh path) ahead of the require_preferred_account fail-closed
raise, moving a locally verified full-resend to a fresh account instead
of erroring.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Soju06
Soju06 force-pushed the fix/dead-account-proxy-route-failover branch from a265419 to 5bf458d Compare August 6, 2026 07:55
@Soju06
Soju06 merged commit e9a9e8a into main Aug 6, 2026
36 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.

bug: dead account-bound proxy does not fail over before visible output

5 participants