fix(auth): harden failover recovery - #175
Conversation
Quarantine failed affinity targets across requests and bound stalled streams by time to first meaningful chunk. Preserve protocol-specific terminal failures while retrying truly empty completions.
Pre-bind cache-miss selections so concurrent requests for one session stay sticky. Keep stream first-chunk failover explicit and disabled by default.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Generate deterministic IDs for missing Gemini tool calls and match function responses by name and FIFO order. Preserve explicit IDs and stable fallback IDs for unmatched responses.
Keep failed credentials excluded across outer attempts so dead keys are not retried. Carry affinity metadata through copied request options and append bounded, redacted route summaries to terminal errors.
Aggregate split and newline-less SSE and JSON frames through EOF before judging an upstream response empty.
Stop the first-chunk timer on the first successful upstream chunk, including metadata-only prefixes, while preserving bootstrap judgment. Do not retry after semantic stream commit.
Current-head evidence — follow-up commits & validation (head
|
| Commit | Scope |
|---|---|
32723082 fix(stream): stop TTFT on upstream activity |
sdk/cliproxy/auth/conductor_stream.go, stream_ttft_test.go |
3744665d fix(stream): classify split empty completions |
sdk/cliproxy/auth/empty_completion.go, empty_completion_export.go, empty_completion_test.go |
28668d01 fix(auth): preserve failover state across retries |
sdk/cliproxy/auth/conductor_execution.go, conductor_selection.go, selector.go, route_tracker.go + 8 test files (12 files, +1215/−52) |
65a7364e fix(translator): stabilize Gemini tool call IDs |
internal/translator/openai/gemini/openai_gemini_request.go (+53), openai_gemini_request_test.go (+303) |
- 65a7364 — Gemini→OpenAI translation now derives deterministic tool-call IDs (
sha256("call|msgIdx|partIdx|funcName|payload")[:24], prefixedcall_) for unmatchedfunctionCallparts and pairsfunctionResponsetool messages to those IDs, with stable standalone fallback IDs for unmatched responses. Preserves explicit IDs when present. Regression:TestConvertGeminiRequestToOpenAI_Deterministic100Invocationsruns the translation 100 times and asserts byte-identical output. - 28668d0 — failover state now survives the outer retry loop: excluded/
triedauth IDs are hoisted out ofopts.Metadataper attempt (pickOptscopy sites inexecuteMixedOnce/CountMixedOnce/StreamMixedOnce), so a dead credential is never re-invoked across outer retries; mixed-pool affinity namespace ("mixed" provider + requested model) stamped during selection survives the metadata copy into successResult.Options; route exhaustion preserves typed errors. New regression coverage:outer_retry_exclusions_test.go(255 lines),route_exhaustion_test.go(474 lines),selected_auth_metadata_test.go(153 lines). - 3744665 — empty-completion judgment now aggregates split / newline-less SSE and JSON frames through EOF (
streamBootstrapState.finish()+StreamBootstrapDetector) before declaring an upstream response empty, instead of judging per-fragment buffers. - 3272308 — the first-chunk (TTFT) timer stops on the first successful upstream chunk, including metadata-only prefixes (e.g.
: keepalive), while bootstrap judgment is preserved; post-commit failures are not retried.
Correctness semantics (regression-encoded)
- Dead credentials not re-invoked across outer retries —
TestOuterRetryExclusions_NonStream_DisableCooling_InvokedOnce/..._Stream_..._InvokedOnceuse a real retry window (SetRetryConfig(3, 100ms, 3)+ 429/Retry-After 5ms) and assert exactly one executor invocation across the whole outer retry span (pre-fix: 4). - Mixed affinity cache namespace survives copied metadata —
TestManagerExecute_MixedAffinityNamespaceRetainedThroughExclusions,TestManagerSelection_NilMetadataPreservesAffinityNamespace,TestPublishSelectedAuthMetadataIncludesStableIndexprove the stamped "mixed" provider+model namespace survives value-copy and a subsequent same-session request binds to the same auth. - Typed errors preserved — route exhaustion terminates with typed final errors after bounded candidate traversal (
route_exhaustion_test.go,errors.go). - Metadata-only first chunk stops TTFT; error-only does not —
TestManagerExecuteStream_MetadataFirstPrefixStopsTTFTWithoutFailoverasserts: keepaliveis buffered first, TTFT is stopped, no failover to auth-b, auth-a stays available;readStreamBootstrapinvokes the stop callback only on payload chunks (an error chunk returns before the callback). - Post-commit errors delivered once without retry —
TestManagerExecuteStream_PostCommitErrorNotRetriedasserts one terminalStreamChunk.Err, exactly one content chunk (no replay), and exactly one executor call.
Fresh validation (run for this comment)
- TTFT regressions:
go test -count=100 ./sdk/cliproxy/auth/ -run 'TestManagerExecuteStream_(TTFTTimeoutFailsOverToNextAuth|PostFirstChunkDelayNotCutOffByTTFT)$'→ ok (17.8s) - Full suite:
go test -count=1 ./...→ exit 0 (all packages ok) - Race:
go test -race -count=1 ./internal/translator/openai/gemini/→ ok;go test -race -count=1 ./sdk/cliproxy/auth/(TTFT/outer-retry/affinity selection) → ok go build ./...→ exit 0git diff --check→ exit 0- Current-head GitHub CI: build ✅ (pass, run 31653416694) and close-when-agents-md-changed ✅ (pass, run 31653415250) on head
32723082.
Independent local reviews
- 9 independent local review passes completed across projects (CPA, CPAPlus, Studio —
caveman-review/ponytail-review/jbcontext-review× each project, each pass additionally usingjbcontext); all real findings accepted and fixed, with regression coverage added. - CPAPlus final clean re-review found no actionable findings on the current diff.
Maintainer review requested
The Codex connector on this PR currently returns only a setup notice ("create a Codex account and connect to github") and produced no review. A maintainer review of the current head is invited.
No claim of SINGULARITY is made here; this comment reports current-head evidence only.
When route exhaustion fired, the returned routeExhaustionError wrapped only the cause's message, so any upstream non-*Error cause that carried passthrough metadata (Retry-After, X-Request-Id) lost those headers — handlers that collect headers from the final routed error got nothing. routeExhaustionError now exposes Headers() that resolves the wrapped cause via errors.As and returns a defensive clone via cloneHTTPHeader, so the caller's header map is never mutated. errors.As starts at the cause and stops at the first matching carrier, preserving outermost-wins disambiguation for nested carriers; nil receiver returns nil.
ttftScope now owns a per-attempt timeout() that returns a typed TTFT timeout error when the deadline wins, so the shared refresh path re-arms TTFT for the stale-token retry instead of inheriting an expired budget. A timed-out refresh no longer leaves the failover consumer waiting on a dead timer (the Execute/CountTokens/HttpRequest stubs keep the executor contract), and the retried attempt starts with a clean timer so a post-refresh stream is not truncated.
routeExhaustionClonedError no longer clones the cause Error; it wraps the cause and appends the sanitized route-exhaustion summary to Error() without mutating the cause stored Message. It gains SafeResponseHeaders(), forwarding the wrapped cause trusted response headers (e.g. the Home busy error Retry-After) through route exhaustion so handlers collecting passthrough headers do not lose them. Fresh copy never mutates caller headers.
…e quota/balance IsRequestFault now treats structured auth markers (authentication_error type, invalid/incorrect/expired_api_key codes, Gemini UNAUTHENTICATED status) as credential faults even when paired with a generic invalid_request_error code, so failover rotates the credential instead of misclassifying as a request fault. Quota/payment surfaces carrying a generic invalid_request_error code or type stay in quota/balance scope. The shared mixed-auth loop pins this contract via conductor_overrides_test.go.
Current-head evidence — three follow-up commits & validation (head
|
| Commit | Scope | Behavior |
|---|---|---|
286649ae fix(auth): classify upstream 401/403 as credential faults and preserve quota/balance |
internal/clienterror/client_error.go, internal/clienterror/client_error_test.go, sdk/cliproxy/auth/conductor_overrides_test.go |
IsRequestFault treats structured auth markers (authentication_error type, invalid/incorrect/expired_api_key codes, Gemini UNAUTHENTICATED status) as credential faults even when paired with a generic invalid_request_error code, so failover rotates the credential instead of misclassifying as a request fault. Quota/payment surfaces carrying a generic invalid_request_error code or type stay in quota/balance scope. |
e84dc793 fix(auth): forward safe response headers through route exhaustion |
sdk/cliproxy/auth/route_tracker.go, sdk/cliproxy/auth/route_exhaustion_test.go |
routeExhaustionClonedError stops cloning the cause Error; it now wraps the cause and appends the sanitized route-exhaustion summary to Error(), and gains SafeResponseHeaders() forwarding the wrapped cause's trusted response headers (e.g. Home busy Retry-After) through route exhaustion. Fresh copy never mutates caller headers. |
5548eaa0 fix(stream): restart a fresh TTFT timer and path on refresh retry |
sdk/cliproxy/auth/conductor_stream.go, sdk/cliproxy/auth/stream_ttft_test.go |
ttftScope now owns a per-attempt timeout() returning a typed TTFT timeout error when the deadline wins, so the shared refresh path re-arms TTFT for the stale-token retry instead of inheriting an expired budget and leaving the failover consumer waiting on a dead timer. |
Fresh validation (run for this comment, local working tree clean)
go test -count=1 ./internal/clienterror/→ ok (0.356s)go test -count=1 ./sdk/cliproxy/auth/ -run 'TestStream|TestRouteExhaustion|TestConductorOverrides|TestTTFT|TestOuterRetry'→ ok (1.569s)go test -race -count=1 ./internal/clienterror/ ./sdk/cliproxy/auth/ -run 'TestStream|TestRouteExhaustion|TestConductorOverrides|TestIsRequestFault|TestClientError'→ ok (clienterror 1.387s, auth 3.421s)go build ./...→ exit 0go vet ./internal/clienterror/ ./sdk/cliproxy/auth/→ exit 0go test -count=1 ./...(full suite) → all packages ok, no failures/panicsgit diff --check main...HEAD→ exit 0git status --short: no tracked-file modifications (untracked build/cache dirs only)
Environment note
This sandbox cannot write Go build dirs to /tmp (mkdir /tmp/go-build...: operation not permitted), so the run pinned TMPDIR, GOTMPDIR, and GOCACHE to repo-local dirs (.go-tmp, .go-cache) for the commands above. All exits above are with those pinned; /tmp was only redirected, never written.
PR checks / mergeability (GitHub, at evidence time)
- build ✅ SUCCESS (run 31712308339, job 94488132909)
- close-when-agents-md-changed ✅ SUCCESS (run 31712304856, job 94488120797)
mergeable: MERGEABLE,mergeStateStatus: CLEAN
No claim of SINGULARITY is made here; this comment reports current-head evidence only.
|
Soliciting a Codex review of the current head @codex review |
|
To use Codex here, create a Codex account and connect to github. |
Current-head evidence — delete-only ponytail refactor (head
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Current-head evidence — surfacing stream errors before [DONE] (head
|
|
Soliciting a Codex review of the current head @codex review |
|
To use Codex here, create a Codex account and connect to github. |
Current-head evidence — full OpenAI sink parity (head
|
Port of CPA 22d05b8b.
Port of CPA 40d63c47.
Port of CPA bc3afa22.
Port of CPA 4bb7807c.
Port of CPA 4b694431.
Port of CPA: bootstrap.hasMeaningfulOutput() gates error-chunk forwarding; scaffold-only buffer + upstream error fails over.
…g emptiness
A no-argument MCP call such as {"type":"mcp_tool_use","id":"tool_1","name":"lookup","input":{}}
with stop_reason:"tool_use" was misclassified as an empty completion because only tool_use and
server_tool_use received semantic validation, cooling the credential and discarding a valid tool
call. Treat mcp_tool_use with non-empty id and name (or a non-empty input payload) as meaningful,
mirroring how claude_input_tokens.go already handles the block type. Red-proof tests cover
non-stream and SSE variants plus missing-id/missing-name controls.
…xclusions resetRecoveredExclusions only honored DisableCoolingOverride, so when cooling was disabled through manager runtime config (globally or per OpenAI-compatible provider), MarkResult left the failed credential available while the reset dropped its request-scoped exclusion - letting an outer retry with a supplied delay re-select and hammer the same credential. Use the same manager-aware predicate (cooldownDisabledForAuth) in the reset path. Red-proof: global and provider-compat config-disabled cases failed pre-fix; control with cooling enabled still resets.
…06, known flake - local 30/30 green)
…sponses args, mixed JSON - Claude signature_delta frames now deserialize and validate delta.signature, so a signature-only thinking block followed by message_stop is meaningful instead of discarded. - Responses function_call_arguments events only set hasToolCalls when they carry non-empty delta/arguments or a call item was already validated, keeping scaffold-only frames retryable. - Multi-value JSON payloads mark the aggregate unknown/pass-through when any decoded value matches no supported shape, so unknown provider-specific output is never discarded as empty. Red-proof tests per case; race suite x10 green.
…thought signatures - conductor_stream: when a custom executor returns a StreamResult alongside an immediate unauthorized error, the refresh retry now drains the original chunk channel before replacing it, so a still-sending producer cannot block indefinitely (mirrors the other result-discard paths). - empty_completion: Gemini responses whose only payload is a non-empty thoughtSignature (both thoughtSignature and thought_signature spellings) on an otherwise empty part with zero/omitted candidatesTokenCount now count as meaningful output, preserving provider state for request round-tripping instead of cooling the credential. Red-proof tests for both; race suite x10 green.
…soning items in bootstrap - shouldForward now includes positive completion usage (sawUsage && completionTokens > 0), so a terminal frame with empty choices and positive usage is delivered immediately instead of hanging the client when the upstream holds the stream open. - Responses reasoning output items require actual payload (non-empty encrypted_content or summary) before counting as content; the empty in_progress reasoning scaffold emitted by our own translators no longer commits the bootstrap, preserving failover on early errors. Red-proof tests for both; full suites and race x10 green in CPA and CPAPlus.
…stream errors
The standalone-credential pattern required the token to contain a digit, uppercase letter,
or punctuation, so a lowercase-only value such as "Bearer abcdef" survived redaction in
downstream errors and API error logs. Match the full RFC 6750 b64token charset
([-A-Za-z0-9._~+/=]{3,}) without requiring any particular character class. Red-proof:
lowercase-only and mixed tokens redacted; plain prose ("bearer of bad news") untouched.
Race x10 green.
…short credential masking
- empty_completion: Claude partial_json fragments are parsed semantically - whitespace-only
objects/arrays/null ("{ }", "[]", "null ") without usable arguments no longer set
hasToolCalls, so a terminal stop_reason:"tool_use" after them still triggers failover.
- route_tracker: routeExhaustionClonedError.Error() returns the original structured error
body verbatim when it is valid JSON, keeping clienterror.IsRequestFault classification
intact so the Responses WebSocket path sends the actionable client error instead of
silently closing; summaries still append to plain-text causes.
- openai handlers: the standalone auth-scheme pattern drops the {3,} minimum (RFC 6750
allows a single character) and adds a prose deny-list ("bearer of bad news"), masking
short credentials without touching natural language.
Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
3b821a4 to
2f7ab80
Compare
- openai handlers: masking no longer exempts tokens by content (deny-list removed); instead
a lowercase scheme word followed by space and lowercase prose words is treated as natural
language, while standalone credentials of any length ("Bearer of", "Bearer ab") are masked.
- conductor_stream / empty_completion: an observed terminal [DONE] marker on a stream that
delivered only scaffolding now reports the empty completion immediately, so the conductor
rotates credentials instead of waiting forever on an open channel.
Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
… scaffolds
- openai handlers: prose/context exemption removed - any scheme-anchored token-shaped match
(bearer|basic + RFC 6750 charset, any length) is masked unconditionally, so credentials
followed by prose words ("bearer abc expired") can no longer leak. Collateral prose
masking is the accepted trade-off per reviewer.
- empty_completion: Responses function_call/custom_tool_call output items only set
hasToolCalls when they carry usable identity, name, or input, so the empty scaffolds
emitted by our own translators stay in bootstrap and preserve failover.
Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
…done call items - conductor_stream + config: stream-first-chunk-timeout-seconds promised first-chunk enforcement but the timer stopped once headers arrived, letting a headered-but-bodiless stream hang forever; repository policy (AGENTS.md L58) forbids extending timeouts past an established connection. The option is rescoped as stream-connect-timeout-seconds (stream_connect_timeout_ms), with the legacy keys kept as deprecated aliases. - empty_completion: response.output_item.done now routes through hasMeaningfulResponsesCallItem, so empty function_call/custom_tool_call done items stay in bootstrap and preserve failover, matching the added-branch behavior. Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
…id-key errors
- empty_completion: OpenAI tool/function call argument strings are parsed semantically, so
"{}" / "[]" / "null" without usable call identity no longer bypass empty-completion
failover (hasMeaningfulJSONArguments shared with the Responses call-item validator).
- conductor_cooldown: isCredentialScopedError now classifies Gemini 400 "API key not valid"
as credential-scoped, so multi-model credentials exit the model pool after the first
rejection instead of re-issuing the request with the same dead key. CPAPlus also gains the
CredentialScope result wiring in its conductor loops (parity with CPA).
Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
…ircuit, semantic arg events
- codex_websockets_stream: isTerminalEvent now includes response.incomplete and
response.failed, so a terminal payload ends the request (result channel closed, reqMu
released) instead of hanging the execution session while reading the persistent socket.
- empty_completion: Claude message_stop marks the whole-stream terminal state (like [DONE]),
so an empty Claude stream over an open channel is classified empty and rotates instead of
waiting indefinitely.
- empty_completion: Responses argument events ("{}", "[]", "null") route through
hasMeaningfulJSONArguments unless a usable call was already established.
Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
…nd Gemini STOP - pluginhost executor_route: the direct-plugin bootstrap loop now checks detector.IsTerminalEmpty() and emits the empty-completion error immediately when a stream delivers only a terminal marker ([DONE] / message_stop) over an open channel, instead of waiting indefinitely (AGENTS.md L58). - empty_completion: Gemini frames whose candidates all finish with STOP now mark the whole-stream terminal state, so an empty Gemini completion over an open channel is classified empty and fails over rather than hanging. Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
A Claude-compatible upstream that emits data: {"type":"message_stop"} without a separate
event: line (a shape exercised by the repo claude executor fixtures) never set the
whole-stream terminal flag, so conductor and direct-plugin bootstrap loops waited
indefinitely on an open channel instead of failing over. chunk.Type == "message_stop" now
marks terminal just like the SSE event field. Red-proof: TestClaudeDataOnlyMessageStopTerminalEmpty
plus the pluginhost open-channel case; race x10 green.
…in plugin stream on terminal-empty - conductor_cooldown: once a success proves a credential works again, every model suspended with the credential-wide invalid_api_key reason is resumed, not just the probed model - previously the other models stayed hidden from registry-backed model lists indefinitely. - pluginhost executor_route: the terminal-empty early return now drains the source channel, so unbuffered adapter producers (mapExecutorStreamChunks, RPC wrappers) cannot block and RPC stream cleanup always runs. Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
…stream Both terminal-empty exits now drain the upstream chunk channel: the empty-completion failover/return path and the closed=true substitution path, so unbuffered producers cannot block after readStreamBootstrap stops early. Pre-publish review (5-skill gate) PASS. Red-proof: TestConductor_ExecuteStreamDrainsSource* (single-model return and model-pool failover); race x10 green in CPA and CPAPlus.
Tests for the drain fix: TestConductor_ExecuteStreamDrainsSource* (single-model return and model-pool failover), verifying upstream producers never block when bootstrap exits early.
… for credential-wide suspensions - empty_completion: non-empty choices[].delta.images / choices[].message.images (shapes emitted by the antigravity and codex translators) count as meaningful output, so image-only completions are delivered instead of discarded and cooled. - conductor_cooldown + registry: sibling-model auto-resume after a successful probe now applies only to credential-wide suspensions (reason invalid_api_key, via new ModelRegistry.GetClientModelSuspensionReason); model-specific suspensions such as model_not_supported, quota, or 404 survive a sibling success, so registry-backed model lists never advertise an unroutable model. Red-proof tests per case (image-only, sibling-suspension-survives, credential-wide resume); full suites and race x10 green in CPA and CPAPlus. CI note: TestResponsesWebsocketReplays- ImmediatelyAfterPinnedAuthFailure failure was a CI TCP-teardown flake (local 20/20 + race x50 green in both repos).
…s as meaningful - openai handlers: compound-key masking now covers camelCase suffixes (refreshToken, clientSecret, apiKey, accessToken, KeyId...) via a case-sensitive suffix branch, so upstream errors carrying such assignments no longer leak credentials. - empty_completion: Claude citations_delta blocks with a non-empty citation object count as meaningful output (matching the translator that preserves them as annotations), so citations-only completions are delivered instead of discarded and cooled. Red-proof tests per case; full suites and race x10 green in CPA and CPAPlus.
- empty_completion: openAIResponseOutputItem deserializes result; image_generation_call items whose only payload is a non-empty result count as meaningful output, so bootstrap no longer withholds a completed image while upstream lingers. - openai_responses_websocket_forward: buildResponsesWebsocketErrorPayload copies the sanitized top-level error fields (type, code, message, param) into the rebuilt error object, so the supported flat error form keeps its actionable structure instead of dumping the whole document into error.message. Red-proof tests per case (reversion-verified); race x10 green in CPA and CPAPlus.
Summary
Hardens the auth failover and recovery pipeline in CPAPlus — the mirror of CPA #4881, adapted for Plus-specific routing paths and executor topology.
When a provider returns an empty completion (HTTP 200,
finish_reason: stop, zero content/tool-calls/tokens — observed on AI Studio and Antigravity paths), today the response passes through unchanged: no retry, no cooldown, session affinity stays pinned to the failing auth. Agent harnesses die with "empty response, finishReason=stop".This PR treats empty terminal completions as retriable failures and fixes a chain of related issues exposed during live failover testing.
Key changes (24 commits, 47 files)
Empty-completion detection and retry
empty_completion.go(new): conservative predicates for OpenAI SSE streams and non-stream JSON — empty iff terminal AND zero non-whitespace content AND zero tool calls AND zero completion tokens.reasoning_contentcounts as content; unrecognized formats → no behavior change.conductor_stream.go: when bootstrap read closes with an empty aggregate (before any byte reaches the client), synthesize retriableerrEmptyCompletion,MarkResult(failure), continue model/auth loop.conductor_execution.go,conductor_home.go: same judgment on non-stream and home paths.5420c121-equivalent): complete newline-lessdata:frames are evaluated immediately; truncated JSON and partial[DONE]remain buffered.Auth selection and session affinity
selector.gouses generation-token compare-and-swap to prevent ABA rebind races between concurrent requests for the same session.CompareAndReplaceGroupCAS prevents split-brain.Stream and error handling
Retry-Afterandx-request-idsurvive the route-summary wrapper viaHeaders()andSafeResponseHeaders().openAIStreamSanitizedErrorno longer exposesUnwrap()— raw upstream payloads stay sealed.SafeResponseHeaders()returns a cloned header copy at wrap time.TrustedDirectResponsedual-flag bypass preserves passthrough headers.[DONE]frame.Upstream credential handling
Provider-specific coverage
Test plan
go test -count=1 -timeout 300s ./...— all packages passgo test -race -count=1 -timeout 300s ./sdk/cliproxy/auth— race-freego build ./...— cleanMirror
CPA: router-for-me/CLIProxyAPI#4881 (equivalent changes adapted from the shared codebase)
Related issues