fix(auth): fail closed on unbounded credential unavailability - #92
Open
sususu98 wants to merge 16 commits into
Open
fix(auth): fail closed on unbounded credential unavailability#92sususu98 wants to merge 16 commits into
sususu98 wants to merge 16 commits into
Conversation
A model state marked unavailable or quota exceeded without any recovery deadline was reported as dispatchable. Home therefore kept handing a broken credential to CPA nodes, which refused to execute it and returned 503 "no execution models available" while healthy lower-priority credentials were never considered. Route the model-level decision through a shared availabilityBlock helper that mirrors CPA: an unavailable or quota-exceeded snapshot with no recovery time is unbounded and now blocks, elapsed deadlines restore availability on their own, and the reported deadline is the later of the retry and quota windows so Retry-After can no longer under-report. Credential-wide aggregates stay advisory: legacy Home versions recorded quota state with a "credential" scope, and honouring it here would block every model instead of the one that failed.
Failure transitions always marked the model state unavailable, but several branches then left no recovery deadline at all. A quota failure on a credential carrying disable_cooling produced unavailable and quota-exceeded with an empty recovery time, and unmapped status codes did the same for every credential. Now that dispatch fails closed on unbounded state, those credentials would be parked with nothing able to expire them. Treat unmapped status codes like the existing 408/5xx transient branch: cool the model for one minute so dispatch moves to another credential and the model returns on its own. Derive the unavailable flag from the deadline instead of asserting it unconditionally, and clear the unavailable and quota flags when disabled cooling leaves no deadline, since disable_cooling means keep retrying rather than park the credential forever. Failure detail is untouched: status, last error, and status message still record why the attempt failed, and real cooldowns such as the 12h model-not-supported window keep their deadline.
Session affinity validated an established binding against the highest available priority tier only. With mixed priorities a session bound to a lower-priority credential lost its binding the moment a higher-priority credential recovered, so a conversation silently moved to another credential mid-flight and dropped its provider-side reasoning and prompt cache context. Validate the bound credential against every tier while still handing the fallback selector the highest tier only. Credential priority keeps deciding cold bindings, sessionless requests, and genuine bound-credential failover; an established binding now outranks priority for as long as it stays available. Split the availability pass into a shared priority-mode helper so both views come from one evaluation, and narrow the fallback candidates with highestPriorityAuths.
Execution results carrying a revoked grant or a Cloudflare interstitial had no dedicated branch, so both landed on the transient path and were retried once a minute forever. A revoked refresh token cannot be fixed by retrying the same token, and a challenge page is not a provider response at all. Add both branches ahead of the status-code switch, mirroring CPA: invalid_grant on 400 or 401 suspends the model for 30 minutes, and a Cloudflare challenge reuses the quota backoff ladder with a ten second floor and records a "cloudflare challenge" quota reason so operators can tell it apart from a real provider quota. Other statuses keep their existing handling, so a 5xx that merely mentions invalid_grant stays transient. Cooling stays configurable: with disable_cooling both branches leave no deadline and the shared cleanup keeps the credential dispatchable.
Failing closed on a snapshot with no recovery deadline solved the wrong half of the problem. Home is the sole credential scheduler: downstream CPA nodes run with cooling disabled and execute whatever Home hands them, so a state nothing is able to expire does not protect anyone. It only removes a credential permanently. Such snapshots are real. Older Home versions parked a model on an unmapped status with no deadline at all, cluster merges can reconstruct the same shape, and both land in the database. Blocking them would strand those credentials for good: the scheduler only revisits a blocked entry once its retry deadline elapses, so an entry with no deadline is never reconsidered, and the Management API quota reset only clears state that carries the quota flag. Default availability to healthy instead. A snapshot blocks only while a recovery deadline is still in the future, which keeps the parts that were right - a quota flag alone blocks, an elapsed deadline restores availability, and the reported deadline is the later of the retry and quota windows so Retry-After cannot under-report. Bounding every failure transition, not failing closed, is what stops a broken credential from being handed out forever. Route the credential-wide aggregate through the same helper so an operator can never see an availability verdict that disagrees with the scheduler, and let that pass clear an unbounded flag it finds, so a legacy row heals on contact.
Home forces the global disable-cooling flag to false because it owns scheduling, so the per-credential override is the only way an operator can ask for a credential that never cools down and is scheduled purely by priority and session affinity. Every failure branch honoured that override except 401, which always parked the model for a minute waiting for the central refresh. Honour it there too, so the override means what it says on every path a caller can reach. The 12h model-not-supported window deliberately stays: a model the credential cannot serve is a capability verdict rather than a cooldown, and retrying it sooner would only rediscover the same rejection. Say so in a comment next to the branch.
The challenge branch borrowed the quota backoff ladder but not the rule that protects it. Home aggregates execution results from every CPA node, so one Cloudflare interstitial is reported once per node and per in-flight request, and each report restarted the window from the current time and advanced the ladder. A single incident could therefore walk a credential up to the 30 minute ceiling in seconds, punishing it once per observer. The quota path already solved this: a failure landing inside an open window reuses that window instead of escalating. Give the challenge branch the same behaviour by passing the whole quota state rather than only the backoff level. Sharing the window is only meaningful if the nodes share the row, so route the challenge through the store's StateMutator like a 429, and skip the round-trip when the local copy already shows an open window. Clear QuotaResetAt while writing the new state so an earlier operator reset cannot keep overriding it during a cluster merge.
A caller that hangs up mid-request produces a failed usage record like any other, so Home counted the attempt against the credential, wrote an error state, and cooled the model down. None of that is warranted: the provider never rejected anything, and a user who cancels repeatedly - closing a tab, aborting a stream, stopping an agent run - could walk a perfectly healthy credential through the failure path over and over. Drop cancelled results before they reach the state machine, so the attempt is neither counted nor cooled down and any cooldown an earlier real failure established stays exactly as it was. Cancellations are matched by the 499 status and by the cancellation vocabulary Go, net/http, and the CPA executors produce, since a disconnect reaches Home as a generic failure once the status is lost. Timeouts stay on the transient path even when their text says canceled: a deadline means the upstream stopped answering, which is precisely what the one minute cooldown is for.
Recording a revoked grant suspends the model in the registry, but the reconcile that rebuilds registry state from scheduler state did not recognise it: the suspend rule matches model-support errors and 402/403/404, and invalid_grant arrives as 400 or 401. Any credential apply or cluster event during the 30 minute block therefore resumed the model, republishing it while dispatch kept refusing it - the same mismatch between advertised and dispatchable state this branch set out to remove. Treat invalid_grant like the other verdicts the reconcile already understands, so the suspension lasts exactly as long as the dispatch block.
A request with no session signal handed the fallback selector the raw candidate slice while every session path handed it an availability-filtered one. Both ended up equivalent because the built-in selectors filter again internally, but the two paths reached that answer differently, and a fallback selector that trusted its input would have seen blocked credentials on one path only. Run the same availability pass before both branches so the sessionless path is a narrower case of the session path rather than a separate one, and correct two doc comments that described the merged view as carrying no ordering when it is in fact ID-sorted.
A malformed, oversized, or policy-rejected request fails identically on every credential, yet Home cooled down the credential/model pair that happened to serve it. Replaying one bad request was therefore enough to walk the whole pool into cooldown, and the caller still received the same rejection at the end. Mirror CPA's internal/clienterror classification so both sides agree on what counts as a request fault: 400, 409, 413 and 422, plus the structured error codes and types providers use to report bad input regardless of the status code. Classification order is the delicate part and is locked by tests. Several provider verdicts arrive on the very status codes a request fault uses - invalid_grant on 400, an unsupported model on 400 or 422, a Cloudflare challenge on anything - so each of those is settled first. Treating the status code as decisive would let a revoked grant masquerade as a bad request and never cool down, which is the failure this branch exists to prevent. The unmapped-status tests moved from 409 to 501, which is genuinely unmapped now that 409 is a request fault.
A challenge page was recorded as quota state. The backoff ladder it borrowed was the right shape, but the quota flag came with it, so dispatch answered with a quota cooldown error and the credential appeared rate limited by the provider. Anyone reading that reached for provider quota dashboards while the real cause sat at the edge. Keep the ladder, drop the disguise. The challenge now parks the credential/model pair through NextRetryAfter with its own RetryBackoffLevel, so quota state stays free for actual quota and the block is reported as an ordinary unavailability. Both scheduler states funnel into the same blocked index and the deadline is still bounded by the 30 minute ceiling, so recovery is unchanged. Window reuse moves with it: an in-flight window is now detected through the retry deadline rather than the quota window, which keeps one incident from advancing the ladder once per reporting node.
Reusing an established binding needs an answer about exactly one credential, yet every session-affinity pick first evaluated availability for the entire candidate pool, sorted it, and then walked the result to find the bound ID. That made the most common outcome the most expensive one, and it duplicated work the dispatch loop had already done to build the candidate slice. Resolve the binding directly and fall through to the pool-wide evaluation only when there is no usable binding, which is what a cold start or a failover actually needs. The fallback session key follows the same shape. Behaviour is unchanged: a bound credential is still validated across every priority tier, a blocked binding still reselects from the highest available tier and rebinds, and a sessionless request still goes straight to the fallback selector.
Home carries a full websocket preference: credentials are indexed by websocket capability, and the ready buckets expose a websocket-only view so a websocket request can prefer a capable credential over a higher priority HTTP-only one. None of it ever ran. CPA derives the signal from its own request context, the dispatch request has no field for it, and Home hardcoded the flag to false, so the ws index, the three functions threading the parameter, and the cross-tier rule were all dead. Carry the signal instead. The dispatch request gains an optional downstream_websocket field, and the forwarded upgrade header serves as a fallback so nodes that predate the field keep working. Both feed one metadata key that the scheduler reads where the flag used to be pinned. Scope matches CPA: only providers that can carry a websocket upstream, and only the single-provider path, since a mixed-provider pick has no single transport to preserve. Requires the matching CPA change to populate the field; until then the header fallback covers it.
Three leftovers that each claimed to do something they did not: A recorded LastError counted as a current fault forever, so one transient failure left the credential reported unhealthy long after every model had recovered and dispatch was already using it. Health now asks the same question dispatch asks - is anything still blocked - so the two can no longer disagree. Result.RetryAfter was parsed on every 429 and then discarded: the quota path takes its deadline from the local backoff ladder and ignored the argument entirely. Reviving it would need a real decision about how a provider hint interacts with window reuse across nodes, so the honest state is to delete the parsing rather than keep a field that looks load-bearing. SessionCache.InvalidateAuth claimed to run when a credential becomes unavailable; it only runs on removal, which is correct, because a binding is revalidated on every pick and should recover with the credential. Also documents why a bare 404 stays waivable by disable_cooling while an explicit model-not-supported verdict does not, and why a cancelled request is kept out of the recent-request counters entirely.
Carrying the downstream websocket signal to the scheduler was not enough. Enabling session affinity replaces the scheduler fast path entirely - the selector is no longer built-in, so dispatch runs the manager loop and hands the choice to the fallback selector, which has no websocket preference at all. The preference therefore still did nothing in the configuration that needs it most. It needs it most because of how a websocket session actually behaves: CPA opens one connection that lives for minutes and serves many requests, and it reaches Home with a session ID, so the very first pick binds the credential for the whole connection. Getting that first pick wrong runs the entire session on a credential that cannot hold the transport, and the binding is deliberately sticky, so nothing later corrects it. Apply the same rule the scheduler uses when a session has no binding yet: a websocket-capable credential outranks credential priority, falling back to priority when the pool has none. An established binding is untouched, since preserving the transport is the whole point of keeping it.
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
1 participant
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.
Problem
Home reported a credential as dispatchable when its model state was marked
unavailable or quota exceeded but carried no recovery deadline at all.
That state is unbounded: nothing can expire it. Home kept selecting the affected
credential, CPA nodes then refused to execute it, and requests failed with
503 no execution models availablewhile healthy lower-priority credentials werenever considered. The failing selection also produced no cooldown, so the loop
repeated for every request.
The write side actively produced that state. Failure transitions always set the
model state unavailable, but several branches left no deadline behind:
disable_coolingwas in effectdisable_coolingproducedunavailableandquota.exceededwith an empty recovery timeTwo more scheduling defects were found in the same paths:
priority tier only, so a session bound to a lower-priority credential lost its
binding the moment a higher-priority credential recovered
dedicated branch and were retried once a minute forever
Changes
Four independent commits, each buildable and green on its own.
1.
fail closed on unbounded credential unavailabilityRoute the model-level availability decision through a shared
availabilityBlockhelper mirroring CPA:
quota.exceededalone blocks, instead of being ignored whenunavailableis falsenext_retry_afterno longer releases a credential whose quota windowis still open
Retry-Aftercan no longer under-reportCredential-wide aggregates stay advisory. Legacy Home versions recorded quota
state with a
credentialscope, and honouring it here would block every modelinstead of the one that failed.
TestLegacyCredentialWideStateDoesNotBlockDispatchstill passes unchanged.
2.
clear unbounded availability flags when cooling is disabledminute so dispatch moves to another credential, then let it return on its own
since
disable_coolingmeans keep retrying rather than park the credentialFailure detail is untouched: status, last error, and status message still record why
the attempt failed. Real cooldowns such as the 12h model-not-supported window keep
their deadline.
3.
keep session bindings across credential priority tiersValidate the bound credential against every priority tier while still handing the
fallback selector the highest tier only. Credential priority keeps deciding cold
bindings, sessionless requests, and genuine bound-credential failover; an
established binding now outranks priority for as long as it stays available.
4.
cool down invalid_grant and cloudflare challenge failuresinvalid_granton 400 or 401 suspends the model for 30 minutesrecords a
cloudflare challengequota reason so it is distinguishable from a realprovider quota
Other statuses keep their existing handling, so a 5xx that merely mentions
invalid_grantstays transient.Scope
Two production files, +225/-38:
internal/cliproxy/auth/selector.goandinternal/cliproxy/auth/result.go. All three dispatch paths (incremental schedulerfast path, Manager dispatch, selector) already funnel through
isAuthBlockedForModel, so the availability fix covers every path.Deliberately unchanged:
Retry-Afteron 429 stays ignored in favour of the existing exponentialbackoff ladder
model, because Home runs its own refresh loop
MarkResultstill ignores results without a modelTesting
gofmtclean,go build ./cmd/homeOK,go test ./...green. No existing testwas modified.
28 new tests across four files. Each stage was verified by temporarily reverting
only its implementation and confirming the new tests fail:
TestSessionBindingSurvivesHigherPriorityRecovery)internal/cliproxy/auth/selector_availability_test.goincludesTestHighPriorityBadCredentialFallsBackToHealthy, the scheduling-level regressiontest for the reported 503.
Compatibility
No Management API route, request field, or response field changed, so
docs/management/api.mdneeds no update. Thestatus,unavailable, andnext_retry_afterfields keep their meaning; they simply stop being mutuallycontradictory.