fix(auth): do not cooldown credentials on transport-level failures - #4996
fix(auth): do not cooldown credentials on transport-level failures#4996windedge wants to merge 12 commits into
Conversation
|
This pull request targeted The base branch has been automatically changed to |
|
This pull request targeted The base branch has been automatically changed to |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e7692284
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "connection refused", | ||
| "connection reset", | ||
| "broken pipe", | ||
| "no such host", | ||
| "network is unreachable", | ||
| "proxyconnect tcp", |
There was a problem hiding this comment.
When a credential has its own Auth.ProxyURL or Attributes["base_url"], a dead or misconfigured endpoint can produce exactly these proxyconnect tcp, no such host, or connection refused errors. Classifying them unconditionally as lifecycle failures makes MarkResult skip all cooldown state, so that broken auth remains selectable and round-robin routing repeatedly revisits it, causing recurring failed attempts—or failed requests when credential retries are limited—even while another credential is healthy. Preserve a transient cooldown when the selected auth carries connection-specific overrides, or otherwise distinguish shared transport outages from auth-specific routing failures.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Addressed: transport-classified failures now keep the transient cooldown when the selected auth carries its own Auth.ProxyURL, since a dead per-auth proxy is an auth-specific routing fault. Rotation moves off that credential while shared-network failures (global proxy or direct) still skip cooldown. Client cancellation and stream EOF lifecycle failures also still skip cooldown in all cases. Note on base_url: deliberately not treated as a connection-specific override - openai-compatible credentials always carry Attributes["base_url"] (it is mandatory, shared by every credential of the provider), so including it would reintroduce single-credential lockout for the most common deployment while providing no rotation benefit when the shared endpoint is down. Tests added for both the model-level and auth-level cooldown paths, plus a case confirming client cancellation still skips cooldown with a per-auth proxy. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dda15a6159
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isTransportFailureResultError(err) && authHasProxyOverride(auth) { | ||
| return false |
There was a problem hiding this comment.
When an auth has ProxyURL and a request-scoped error contains one of the transport substrings, this branch restores cooldown even though RequestScopedError explicitly means credential availability must not change. This occurs, for example, when a Claude Fast request wraps a status-less TLS handshake timeout in claudeFastRequestError: resultErrorFromError sets Code to request_scoped, but this message-only check still marks the auth unavailable. Check isRequestScopedResultError(err) before applying the per-auth transport override.
Useful? React with 👍 / 👎.
|
Addressed the review feedback: isTransportFailureResultError now requires the connection_lifecycle code explicitly before the per-auth proxy transport override applies, so request-scoped errors (Code request_scoped, e.g. a Claude Fast error wrapping a status-less TLS handshake timeout) always skip credential cooldown. Added a test covering request_scoped code + transport message + per-auth proxy (TestManager_MarkResult_RequestScopedTransportTextWithAuthProxySkipsCooldown); the earlier TestManager_MarkResult_TransportFailureWithAuthProxyStillCooldowns still passes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbd170815c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Transport failures through a per-auth proxy are auth-specific routing faults, | ||
| // not shared network outages, so they should keep credential cooldown. | ||
| func authHasProxyOverride(auth *Auth) bool { | ||
| return auth != nil && strings.TrimSpace(auth.ProxyURL) != "" |
There was a problem hiding this comment.
When a credential routes through Attributes["base_url"] without setting ProxyURL, a dead custom endpoint can produce the same status-less DNS, connection-refused, or TLS errors, but this helper returns false and shouldSkipCredentialCooldownForAuth leaves that credential immediately selectable. OpenAICompatExecutor.resolveCredentials and codexCreds both use this attribute as the credential's upstream endpoint, so round-robin routing can repeatedly select the broken credential. The fresh evidence in this revision is that the new override check handles only Auth.ProxyURL; include a non-empty credential base_url as an auth-specific routing override as well.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Declining this suggestion; base_url is not treatable as an auth-specific override in general. For openai-compatible providers the base URL is provider-level shared configuration: OpenAICompatibilityAPIKey carries no per-key BaseURL, and the synthesizer copies the provider baseURL into Attributes["base_url"] of every credential under that provider (internal/watcher/synthesizer/config.go). A non-empty base_url is therefore the norm for every openai-compatible credential, not a per-credential override signal. Including base_url in the override check would mean that any openai-compatible credential hitting a transient transport blip (for example the TLS handshake timeout in the issue that motivated this PR) gets cooled again - reintroducing exactly the single-credential lockout this PR fixes, now for every openai-compatible deployment. And when a shared endpoint is genuinely down, all credentials fail alike, so cooldown provides no rotation benefit, only lockout harm. The scenario worth protecting - a Claude/Codex/Gemini key with a private BaseURL override while siblings use the default endpoint - is real, but the auth record alone cannot distinguish a per-key custom endpoint from a provider-shared value. Doing that correctly needs an explicit marker at synthesis time (or comparison against sibling credentials), which is a separate follow-up feature rather than a tweak to this predicate. |
Transport failures such as "net/http: TLS handshake timeout", dial connection refused/reset, broken pipe, DNS failures, network unreachable, proxy connect errors and i/o timeouts carry no credential health signal, but were classified as generic failures and put the only credential into a 60s transient cooldown. With a single credential this made every subsequent request fail with a misleading "auth_unavailable: no auth available" 503 until the cooldown expired. Extend the existing connection_lifecycle classification with transport failure message patterns so these errors skip credential cooldown, matching the existing behavior for client cancellation and WebSocket EOF disconnects. Status-bearing upstream errors (401/429/5xx response bodies) still cool down credentials as before.
Address review feedback: a credential with its own Auth.ProxyURL that is dead or misconfigured produces the same transport errors (proxyconnect tcp, connection refused, no such host) that we now classify as connection lifecycle failures. Skipping cooldown unconditionally kept such a broken credential selectable, so round-robin repeatedly revisited it while another healthy credential was available. Restore the transient cooldown when a transport-classified failure hits an auth that carries its own proxy override. Client cancellation and stream EOF lifecycle failures still skip cooldown in all cases. Shared transport failures (global proxy or direct connection) also still skip cooldown, and base_url is deliberately not treated as a connection override because openai-compatible credentials always carry it.
Address review feedback: a request-scoped error whose message carries a transport substring (e.g. a Claude Fast error wrapping a status-less "TLS handshake timeout") still had cooldown restored when the auth carried a per-auth proxy, breaking the request_scoped contract that credential availability must not change. Require the connection_lifecycle classification explicitly before the per-auth proxy override applies, so request-scoped errors always skip credential cooldown.
cbd1708 to
1af47f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af47f714e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Transport failures through a per-auth proxy are auth-specific routing faults, | ||
| // not shared network outages, so they should keep credential cooldown. | ||
| func authHasProxyOverride(auth *Auth) bool { | ||
| return auth != nil && strings.TrimSpace(auth.ProxyURL) != "" |
There was a problem hiding this comment.
When Gemini Vertex service-account auths use different Metadata["location"] values, vertexBaseURL(location) constructs a credential-specific regional hostname (internal/runtime/executor/gemini_vertex_executor.go:1037-1057,1094-1102). If one location is invalid or its regional endpoint has a DNS/connection failure, this helper returns false unless ProxyURL is also set, so MarkResult skips cooldown and round-robin routing continues selecting that failing auth on later requests despite healthy auths in other locations. Include location-derived Vertex endpoints when deciding whether a transport failure is auth-specific.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Addressed in fdae3e5: Vertex location is genuinely per-credential config (written at credential import time in internal/api/handlers/management/vertex_import.go), unlike the openai-compatible base_url case, so this point is valid and different from the base_url suggestion. However, restoring cooldown for every Vertex service-account auth would regress the original fix, because location always defaults to us-central1 and single-credential Vertex users would go back to 60s lockouts on TLS timeouts. The implemented check is therefore pool-aware: a transport failure keeps cooldown only when other Vertex service-account auths exist in the pool and none shares the failing auth's location (i.e. its regional endpoint is unique, so rotation can only reach a healthy endpoint if this auth cools down). Shared-location pools and single-credential setups keep skipping cooldown, preserving the original transport-failure fix. Added coverage:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdae3e5c4e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isTransportFailureResultError(err) && m.vertexTransportFailureIsAuthSpecificLocked(auth) { | ||
| return false |
There was a problem hiding this comment.
When two Vertex service-account credentials use different locations behind the same global proxy, a proxyconnect tcp failure is classified as auth-specific solely because their locations differ, even though neither regional endpoint was reached. As each credential fails, this path cools it, eventually leaving the round-robin pool unavailable during a shared proxy outage. Only restore cooldown when the failure can actually be attributed to the location-derived endpoint, rather than for every transport pattern.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| for _, pattern := range transportFailureMessagePatterns { | ||
| if strings.Contains(lower, pattern) { | ||
| return true |
There was a problem hiding this comment.
Status-less transport errors whose platform text is not in this small substring list still enter the ordinary cooldown path. For example, net.DNSError can report lookup ...: server misbehaving, and socket connection timeouts can be formatted as connection timed out or operation timed out; none match here, so a shared DNS or network outage can cool every credential. Inspect typed net.Error/net.OpError and syscall causes, or cover these common transport forms.
Useful? React with 👍 / 👎.
…den transport patterns
|
Both points addressed in 6b7ef6a. Proxy dial failures are no longer attributed to the location-derived endpoint: shouldSkipCredentialCooldownPoolAware now ignores proxy-connect-layer failures (message containing "proxyconnect tcp") when deciding vertex attribution, so a shared global proxy outage skips cooldown instead of draining the pool. Per-auth ProxyURL handling via authHasProxyOverride is unchanged and still restores cooldown. Covered by TestManager_MarkResult_TransportFailureWithProxyDialSkipsVertexAttribution, including a regression guard that direct endpoint failures (no such host) with a unique location still cool down. transportFailureMessagePatterns now also recognizes "connection timed out", "operation timed out", and "server misbehaving" (net.DNSError), so these shared transport forms classify as connection_lifecycle and skip cooldown. Verified both layers that consume the list: isConnectionLifecycleMessage (classification) and isTransportFailureResultError (auth-specific restore gate); status-bearing errors remain coolable at both layers. Covered by TestManager_MarkResult_TransportFailureNewPatternsSkipCooldown. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b7ef6a25b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isTransportFailureResultError(err) && | ||
| !isProxyConnectFailureMessage(err.Message) && | ||
| m.vertexTransportFailureIsAuthSpecificLocked(auth) { | ||
| return false |
There was a problem hiding this comment.
When multiple Vertex service accounts use distinct locations during a shared host-network failure such as network is unreachable or DNS server misbehaving, this condition attributes every failure to its credential merely because each location is unique. Round-robin then cools each credential in succession and can leave the entire pool unavailable, even though these newly recognized errors provide no evidence that a regional endpoint is faulty; restrict regional attribution to errors that identify the failing endpoint rather than all non-proxy transport patterns.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| for _, peer := range m.auths { | ||
| if peer == nil || peer.ID == auth.ID { | ||
| continue | ||
| } | ||
| peerLoc := vertexSALocation(peer) |
There was a problem hiding this comment.
When the only other Vertex service-account credential is operator-disabled and has a different location, this loop still sets hasPeer, so a transport failure cools the sole selectable credential even though rotation cannot use the purported alternative. The scheduler explicitly removes auths with Disabled set, so filter disabled/status-disabled peers here before deciding that an alternative regional endpoint exists.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Both points addressed in e0278df. Regional attribution is now restricted to endpoint-attributable failures via a dedicated allowlist (vertexEndpointFailureMessagePatterns: no such host, connection refused, connection reset, broken pipe, i/o timeout, connection timed out, tls handshake timeout). Shared host-level forms such as "network is unreachable" and DNS "server misbehaving" no longer trigger vertex attribution and keep skipping cooldown, as does any "proxyconnect tcp" dial failure (excluded explicitly, since proxy errors often embed endpoint-style substrings like "connection refused"). isTransportFailureResultError and transportFailureMessagePatterns are untouched; they answer "is this a transport error" for the per-auth proxy path, which is a different question. Covered by TestManager_MarkResult_VertexSharedHostNetworkFailureSkipsCooldown, TestManager_MarkResult_VertexEndpointConnectionRefusedStillCooldowns, and the round-5 proxy test which still passes. Disabled peers are now filtered in vertexTransportFailureIsAuthSpecificLocked using the same criterion the scheduler applies (peer.Disabled || peer.Status == StatusDisabled, per selector.go isAuthBlockedForModel and scheduler.go upsertAuthLocked), so a sole selectable credential is never cooled because of a disabled different-location peer. Covered by TestManager_MarkResult_VertexDisabledPeerSkipsCooldown. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0278df930
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if peer.Disabled || peer.Status == StatusDisabled { | ||
| continue | ||
| } | ||
| peerLoc := vertexSALocation(peer) |
There was a problem hiding this comment.
When a same-location Vertex peer is temporarily unavailable due to an active credential or model cooldown, this loop still treats it as pollable and returns false after matching its location. For example, with a failing EU auth, a cooling EU peer, and a healthy US peer, the failing auth remains ready and round-robin routing repeatedly revisits it instead of failing over to the US credential, even though isAuthBlockedForModel excludes the supposed same-location peer. Filter peers that are currently blocked for the requested model and time, which requires passing the model into this helper.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Addressed in 0513e94. vertexTransportFailureIsAuthSpecificLocked now receives the model key and the failure timestamp, and filters peers with the exact criterion the scheduler uses for candidate selection (isAuthBlockedForModel in selector.go, which also handles model=="" by degrading to the auth-level availability check). A same-location peer that is currently cooling therefore no longer counts as a pollable alternative, so the failing credential restores cooldown and rotation fails over to the healthy different-location credential. Expired cooldowns still count as pollable, matching scheduler promotion behavior. Covered by TestManager_MarkResult_VertexCoolingPeerStillCooldowns (failing EU auth + cooling EU peer + healthy US peer, model-level and auth-level) and TestManager_MarkResult_VertexExpiredCooldownPeerSkipsCooldown. The existing disabled-peer and unique-location tests were adjusted where the new filtering changes shared-fixture expectations. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0513e9406d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if peer.Disabled || peer.Status == StatusDisabled { | ||
| continue | ||
| } | ||
| if blocked, _, _ := isAuthBlockedForModel(peer, modelKey, now); blocked { |
There was a problem hiding this comment.
When a same-location Vertex service-account peer is unblocked but is not registered for modelKey, this check treats it as pollable, and the later location match makes the helper skip cooldown. However, request selection separately removes that peer with authSupportsRouteModel in conductor_selection.go:1040; with a failing EU auth, a model-ineligible EU peer, and a healthy US peer, round-robin requests can therefore keep retrying the failing EU auth before falling back to the US auth. Apply the same model-support predicate when inspecting peers.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Addressed in a7e6cc3. Peer attribution now applies the same model-support predicate request selection uses: peers are additionally filtered with authSupportsRouteModel, so a same-location peer that is not registered for the failing model no longer counts as a pollable alternative and the failing credential restores cooldown, letting rotation fall back to the healthy different-location credential. For the auth-level path (empty model key) the predicate is a no-op by design (authSupportsRouteModel returns true for empty route model), so behavior there is unchanged. Covered by TestManager_MarkResult_VertexModelIneligiblePeerStillCooldowns (failing EU auth + ready model-ineligible EU peer + healthy US peer). The existing model-level Vertex tests were updated to register models explicitly, matching how the service layer registers credentials before selection (Manager.Register alone does not), so those tests now exercise the intended branches under production registration semantics. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e6cc323c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Transport failures through a per-auth proxy are auth-specific routing faults, | ||
| // not shared network outages, so they should keep credential cooldown. | ||
| func authHasProxyOverride(auth *Auth) bool { | ||
| return auth != nil && strings.TrimSpace(auth.ProxyURL) != "" |
There was a problem hiding this comment.
When an auth uses the supported proxy-url: direct or none mode—especially for a sole credential or every entry in a pool—this returns true even though proxyutil.Parse resolves those values to a direct transport with no proxy endpoint. Any status-less TLS, DNS, or socket failure then restores the credential cooldown and causes subsequent requests to report auth_unavailable until it expires, recreating the shared-transport failure this change is meant to avoid; check the parsed proxy mode rather than mere non-emptiness.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| if auth == nil || auth.Provider != "vertex" || auth.Metadata == nil { | ||
| return "" |
There was a problem hiding this comment.
When an SDK consumer registers a valid Vertex auth with casing or whitespace such as Provider: "Vertex", normal selection still accepts it because executorKeyFromAuth lowercases and trims providers, but this exact comparison returns no location. A transport failure on a unique regional endpoint therefore skips cooldown and leaves the failing credential selectable instead of rotating to a healthy credential in another region; use the same trimmed, case-insensitive provider comparison as selection.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
Did a self-review pass over the cumulative diff and fixed the findings in 189157b:
Verified no other cooldown-writing path bypasses the pool-aware check (refresh, ephemeral/home results, availability-neutral records are intentionally unaffected), no lock-ordering issues (m.mu -> registry mutex, leaf lock), and no stale callers of the applyAuthFailureState signature. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189157b9f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !m.authSupportsRouteModel(registryRef, peer, modelKey) { | ||
| continue |
There was a problem hiding this comment.
Under WeightedRoundRobinSelector, a same-location Vertex peer with weight 0 is never selectable (positiveWeightAuths and the scheduler predicate both exclude it), but this loop still treats it as pollable. With a failing EU credential, a zero-weight EU peer, and a healthy US credential, the location match makes the failure skip cooldown, so weighted routing can repeatedly revisit the failing credential instead of keeping it out of rotation. Fresh evidence in this revision is that peer filtering now mirrors model and cooldown eligibility but still omits the weighted selector's positive-weight requirement.
Useful? React with 👍 / 👎.
|
All three points verified against the code and addressed in 3782c01. Proxy mode: authHasProxyOverride now parses ProxyURL with proxyutil.Parse and only treats a real proxy endpoint (ModeProxy) as an override. "direct"/"none" (ModeDirect, documented in config.example.yaml) and empty values no longer restore cooldown, so a sole credential with proxy-url: direct keeps the transport-failure skip. Invalid/unparseable values still count as an override since a broken per-credential proxy config is an auth-specific fault. Covered by TestManager_MarkResult_TransportFailureWithDirectOrNoneProxySkipsCooldown (direct and none, model-level and auth-level). Provider normalization: vertexSALocation now compares the provider trimmed and case-insensitively (EqualFold), matching executorKeyFromAuth's ToLower(TrimSpace(...)) used by selection and executor routing. Covered by TestManager_MarkResult_VertexProviderCaseNormalizedStillCooldowns. Zero-weight peers: the attribution loop now consults the active selector. Only under WeightedRoundRobinSelector (where positiveWeightAuths and the scheduler's requirePositiveWeight both exclude weight<=0) do zero-weight peers no longer count as pollable; the default RoundRobinSelector ignores weight, so attribution correctly keeps counting them there. Covered by TestManager_MarkResult_VertexWeightedPeerZeroWeightStillCooldowns and TestManager_MarkResult_VertexRoundRobinIgnoresWeightSkipsCooldown. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3782c01c0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isTransportFailureResultError(err) && authHasProxyOverride(auth) { | ||
| return false |
There was a problem hiding this comment.
When multiple credentials specify the same real ProxyURL, an outage of that common proxy produces proxyconnect tcp or similar errors for every credential, but this unconditional override restores cooldown for each one. Round-robin retries can therefore cool the entire pool and make subsequent requests return auth_unavailable until the transient cooldown expires, recreating the original shared-transport failure; compare the effective proxy with selectable peers and restore cooldown only when the proxy route is credential-specific.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| if _, weighted := m.selector.(*WeightedRoundRobinSelector); weighted && authWeight(peer) <= 0 { | ||
| continue |
There was a problem hiding this comment.
When weighted round-robin is combined with session affinity, newRoutingSelector wraps the weighted selector in SessionAffinitySelector, so this type assertion is false even though the wrapper's selection path still excludes non-positive weights. A zero-weight same-location Vertex peer is consequently treated as pollable and can suppress cooldown for a failing credential, causing later requests to revisit it instead of rotating regions. Fresh evidence in this revision is that the zero-weight fix checks only a directly installed WeightedRoundRobinSelector; inspect the affinity fallback as well.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| hasPeer = true | ||
| if peerLoc == loc { | ||
| return false |
There was a problem hiding this comment.
When a failing Vertex credential and a healthy different-location credential are in the highest priority tier, a lower-priority same-location peer is counted here even though ordinary round-robin selection excludes that tier while any higher-priority credential remains available. The location match then skips cooldown for the failing high-priority credential, so each new request can select and fail on it again; apply the same highest-available-priority filtering used by availableAuthsForRouteModel.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
…peer eligibility - MarkResult cooldown attribution now treats a transport failure through a per-auth proxy as a shared infrastructure fault when another pollable credential uses the same ProxyURL, instead of cooling every credential behind the same dead proxy endpoint and draining the pool - Vertex endpoint-uniqueness attribution now mirrors the selector candidate set: peers are narrowed to the current highest available priority tier (availableAuthsForSelector, allPriorities=false semantics) and to positive weights when the strategy is weighted, including a SessionAffinitySelector whose fallback is a WeightedRoundRobinSelector - add tests for shared/dedicated proxy cooldown, session-affinity weighted zero-weight peers, and low-priority same-location peers
|
All three points verified and addressed in 401ddab. Shared proxy: a real per-auth ProxyURL still restores cooldown when it is credential-specific (sole credential, or no other selectable peer shares that URL), preserving TestManager_MarkResult_TransportFailureWithAuthProxyStillCooldowns. When another selectable peer (unblocked, and for the model path also registered for the model) has the same ProxyURL, the outage is treated as shared transport and cooldown is skipped, so a common company proxy dying no longer drains the whole pool. Covered by TestManager_MarkResult_TransportFailureWithSharedProxySkipsCooldown and TestManager_MarkResult_TransportFailureWithDedicatedProxyStillCooldowns. Vertex peer eligibility now reuses the selector helpers instead of hand-mirroring more criteria. vertexPeerCandidatesLocked feeds candidates through availableAuthsForSelector (highest available priority tier, same as availableAuthsForRouteModel with allPriorities=false) and applies positiveWeightAuths when the active selector is weighted, including the SessionAffinitySelector.fallback wrapping that newRoutingSelector installs for weighted+affinity. Those helpers do not take m.mu, so calling them from MarkResult's write lock is safe. Zero-weight peers behind an affinity-wrapped weighted selector and lower-priority same-location peers therefore no longer suppress cooldown. Covered by TestManager_MarkResult_VertexSessionAffinityWeightedZeroWeightStillCooldowns (both SessionAffinity constructors) and TestManager_MarkResult_VertexLowPrioritySameLocationPeerStillCooldowns. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 401ddabb35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if modelKey != "" && !m.authSupportsRouteModel(registryRef, peer, modelKey) { | ||
| continue | ||
| } | ||
| return true |
There was a problem hiding this comment.
Under WeightedRoundRobinSelector, if a failing positive-weight auth and a zero-weight auth share ProxyURL while another positive-weight auth uses a healthy route, this loop counts the zero-weight peer even though positiveWeightAuths removes it from selection. MarkResult consequently skips the failing auth's cooldown, so weighted routing continues selecting it instead of consistently failing over to the healthy credential; apply the same weight eligibility filtering used by the active selector before returning true.
Useful? React with 👍 / 👎.
|
Addressed in 67e0589. authHasSharedProxyPeerLocked now applies the same selector-eligibility pipeline as the vertex path: candidates (non-self, same provider via executorKeyFromAuth, unblocked per isAuthBlockedForModel, model-registered for the model path) pass through availableAuthsForSelector (highest available priority tier), and positiveWeightAuths is applied when the active selector is weighted, including the SessionAffinitySelector.fallback wrapping. A zero-weight peer sharing the proxy therefore no longer suppresses cooldown under weighted routing. One deliberate behavior note: shared-proxy peers are now limited to the same provider (executorKeyFromAuth match), matching how request selection builds per-provider candidate sets. A cross-provider credential sharing the URL can never serve this credential's requests, so counting it would wrongly skip cooldown and keep rotation pinned to the dead proxy. Covered by TestManager_MarkResult_TransportFailureWithSharedProxyWeightedZeroWeightPeerStillCooldowns (weighted, model and auth level) and TestManager_MarkResult_TransportFailureWithSharedProxyRoundRobinIgnoresWeightSkipsCooldown (default round-robin keeps skipping, since plain RR ignores weight). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e0589507
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !m.authSupportsRouteModel(registryRef, peer, modelKey) { | ||
| continue | ||
| } | ||
| candidates = append(candidates, peer) |
There was a problem hiding this comment.
When the failed request uses a credential policy, required auth kind, or disallowFreeAuth, this candidate list can include a same-proxy peer that selection explicitly removes with authSelectionEligibilityForRequest. For example, an ordinary Codex API key sharing the proxy with a failing alpha-search key makes this helper skip cooldown even though it cannot serve alpha-search requests; those requests then keep revisiting the failing key despite an eligible key on another proxy. Pass the request context/Result.Options into this judgment and apply the same eligibility predicate before counting peers.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| if len(candidates) == 0 { | ||
| return false | ||
| } | ||
| priorityTier, _, errAvailable := m.availableAuthsForSelector(m.selector, candidates, provider, modelKey, now) |
There was a problem hiding this comment.
When the failing auth is the sole credential in the highest priority tier, this call computes the tier only from peers because the failing auth was omitted above. A lower-priority peer sharing its proxy is therefore treated as active and suppresses cooldown; the still-ready high-priority auth then keeps being selected, so routing never reaches a healthy lower-priority credential on another proxy. The fresh evidence beyond the earlier priority-tier comment is that the new selector-alignment call is passed the peer-only slice rather than the full candidate set.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
1 participant
Problem
A transport-level failure on a provider (for example open.bigmodel.cn via the openai-compatible provider returning "net/http: TLS handshake timeout") put the only credential into a 60s transient cooldown. Subsequent requests then failed first with a 500 "TLS handshake timeout" and afterwards with 503 "auth_unavailable: no auth available" until the cooldown expired, because non-quota cooldowns surface as blockReasonOther and produce a misleading auth_unavailable error.
Fix
Classify transport-level failures as connection_lifecycle so they skip credential cooldown:
This is consistent with the existing design for client cancellation and WebSocket EOF disconnects, which already skip cooldown since they say nothing about credential health.
Safety
The classification only applies to errors without an HTTP status. Status-bearing upstream errors (401/429/5xx response bodies) still cool down credentials exactly as before. A test covers this case.
Tests
go test ./sdk/cliproxy/auth/ -count=1passes.Note
This change does not add in-request retries for these errors; it prevents credential penalization so the next request retries naturally.