fix(proxy): settle keyed failover health writes safely - #1546
Closed
mereyabdenbekuly-ctrl wants to merge 11 commits into
Closed
Conversation
…uest's API key (Soju06#1540) * fix(proxy): attribute bridge failure fan-out request logs to each request's API key HTTP-bridge callers of _fail_pending_websocket_requests pass a session-level api_key=None because a shared bridge session can hold pending requests from multiple API keys. The per-request log write used that parameter directly, so every bridge-path error row (stream_incomplete, upstream_request_timeout, upstream_unavailable, upstream_rejected_input, ...) landed in request_logs with api_key_id NULL and disappeared from key-filtered dashboard queries. Attribute each pending request's row to its own request_state.api_key, falling back to the caller-provided key — the idiom the terminal-event path already uses, which is why success rows attribute correctly. Fixes Soju06#1539 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(proxy): cover bridge failure key attribution at the /v1/responses route Codex review P1: the unit regression intercepts _write_request_log, so it cannot catch a key that never reaches the request state or the persisted row. Add a route-level regression that authenticates a bridge request, fails it through the session failure fan-out (upstream send failure on a reused session), and asserts the persisted RequestLog.api_key_id. Red on main, green with the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
18 tasks
* fix(cost): openai updates the terra and luna's cost * fix openspec
* feat(proxy): add durable capability lineage for reconnect * fix(proxy): enforce capability routing edge contracts
* fix(proxy): surface forwarded compact settlement failures * fix(review): P2 - cover compact settlement double failure * fix(review): P2 - distinguish compact settlement failure provenance
* feat(helm): allow Grafana dashboard title overrides * style: format Helm dashboard test * docs(helm): clarify optional Grafana hierarchy * docs: move Grafana hierarchy guidance * fix(helm): preserve default dashboard rendering
* feat(ui): separate service and usage health status * fix(ui): reserve wrapped status bar space * chore(i18n): remove unused status bar translations
* feat(ui): warn when request log retention is disabled * fix(ui): make retention presets informational
Soju06#1477) * feat(conv-list): add conversation list alongside with request_logs list * update frontend * add coverage * review * review: don't process all request log data, show back the day selector * ruff * feat(conv): add conversation list ui fields * review * review * Stabilize the rolling window across polling refetches * fix:address db review * update query * fix: ruff & conversation mode fix to update 5 stat box when changing the time * update: change the conversation id filter from first message to all message in window * review I * review II * review III * review IV * review V * review & change the lasted unit to smallest in second * ruff * review pagination * review update openspec * Update query * conversation should be hidden in guest mode * add conversation docs * review
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 (Soju06#1314). Rebuilt from PR Soju06#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 Soju06#1314 Supersedes Soju06#1322 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lover Maintainer follow-up to the Soju06#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>
mereyabdenbekuly-ctrl
force-pushed
the
codex/pr-1542-review-fixes
branch
from
August 3, 2026 04:57
5615948 to
c067e60
Compare
github-actions
Bot
added
the
db migration
Contributor
Author
|
Closing this follow-up as superseded/duplicate. The intended settlement and keyed failover health-write fix from the original head ( The fork branch has since been rebased onto current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context
Follow-up to #1542 that addresses the two unresolved P1 review threads. The base is intentionally
fix/dead-account-proxy-route-failoverso this PR contains only review fixes for the maintainer takeover, not the full parent diff againstmain.Validation
uv run pytest -q tests/unit— 5096 passed, 67 skippeduv run pytest -q tests/unit/test_proxy_utils.py tests/unit/test_proxy_http_bridge.py tests/integration/test_proxy_websocket_responses.py tests/integration/test_http_responses_bridge.py— 1510 passeduv run ruff check .uv run ruff format --check .uv run ty checknpx --yes @fission-ai/openspec validate --specs— 48 passed