Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 1 of 13: auth routing, affinity, quota, cooldown - #14
ericlitman wants to merge 1 commit into
Conversation
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
🛑 8 blocking findings🟡 Medium · Store-adopted credentials keep the old version 🟡 Medium · Require observations to postdate the cooldown write 🟡 Medium · Delayed records replace a newer independent cooldown 🟡 Medium · SIWC expiry blocks valid Codex-client credentials 🟡 Medium · Treat future credit-hint timestamps as stale 🟡 Medium · Quota restoration clears unrelated legacy model limits 🟡 Medium · Successful refresh retains the old 401 deadline 🟡 Medium · Do not clear failures for an unrefreshed token partition WalkthroughThis review-only slice of the upstream
Since the last review
Repair promptRead the
Details
First review of this pull request. @unfret review · @unfret status · @unfret help |
![unfret[bot]](https://avatars.githubusercontent.com/in/4933690?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛑 8 blocking findings
🟡 Medium · Store-adopted credentials keep the old version
🟡 Medium · Require observations to postdate the cooldown write
🟡 Medium · Delayed records replace a newer independent cooldown
🟡 Medium · SIWC expiry blocks valid Codex-client credentials
🟡 Medium · Treat future credit-hint timestamps as stale
🟡 Medium · Quota restoration clears unrelated legacy model limits
🟡 Medium · Successful refresh retains the old 401 deadline
🟡 Medium · Do not clear failures for an unrefreshed token partition
Check
| m.mu.Unlock() | ||
| err := m.persist(ctx, saved) | ||
| m.mu.Lock() | ||
| mergeAuthSaveDelta(auth, before, saved, true) |
There was a problem hiding this comment.
🟡 Medium · Store-adopted credentials keep the old version
PersistCodexAuthFile can adopt tokens rotated by another process during an ordinary metadata save. This merge publishes those tokens without advancing CredentialVersion, which updateInternalLocked calculated before saving. For example, a note-only Update adopts token B from disk but retains token A's version; a delayed 401 from A then passes isStaleExecutionResult and imposes a 30-minute model cooldown on healthy B. Credential changes introduced by persistence need the same version fencing as other replacements. Also reported for this defect: - Advance credential version when persistence adopts disk tokens (sdk/cliproxy/auth/conductor_persistence.go:230): PersistCodexAuthFile can adopt tokens rotated by another process during an ordinary save. This merge publishes those replacement tokens without advancing CredentialVersion. An in-flight request using the old token consequently retains the current version, passes isStaleExecutionResult, and can apply its old-token 401 to the replacement credential, imposing a 30-minute model cooldown. This is reproducible by rotating the file, recording another request's success to adopt it, then recording the old request's 401.
| if hasUnauthorizedAuthFailure(existing) { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Medium · Require observations to postdate the cooldown write
The restoration check is documented to require an observation strictly newer than the exhaustion write, but Before accepts ObservedAt == existing.UpdatedAt. With coarse or replayed timestamps that are newer than the previous quota snapshot but equal to the cooldown write, positive signals incorrectly clear the Devin credential and model cooldowns before there is post-failure evidence of recovery.
| if preferred == nil || (liveShadow != nil && !liveShadow.UpdatedAt.After(updatedAt)) { | ||
| preferred = recordShadow | ||
| } else if liveShadow != nil && liveShadow.UpdatedAt.After(updatedAt) { | ||
| preferred = liveShadow |
There was a problem hiding this comment.
🟡 Medium · Delayed records replace a newer independent cooldown
When an auth-level credential quota coexists with a live unwrapped model cooldown, liveShadow captures that newer model state above. However, state.credentialQuotaShadow is still nil, so this condition selects recordShadow without comparing timestamps. Restoring an older one-hour record over a newer 12-hour independent block consequently saves the one-hour shadow; subsequent Devin quota restoration reinstates it and releases the model 11 hours early. The captured live layer must participate in the newest-wins selection even when no shadow previously existed.
| if IsCodexUseSIWC(auth) { | ||
| return authSIWCTokenField(auth, "access_token") |
There was a problem hiding this comment.
🟡 Medium · SIWC expiry blocks valid Codex-client credentials
With use_siwc enabled, an expired or rejected SIWC token now makes AccessTokenExpirationTime report the credential as expired. Both legacy selection and the scheduler use that context-free check, so a Codex client receives auth_unavailable even when its outer OAuth token remains valid. This contradicts IsCodexUseSIWCForRequest and the executor, which explicitly use outer credentials for Codex clients. Reproduced with a codex_cli_rs User-Agent, a valid outer token, and expired SIWC credentials. Also reported for this defect: - Use the request's credential partition during selection (sdk/cliproxy/auth/conductor_refresh.go:409-410): With use_siwc enabled, a valid outer token, and an expired or rejected SIWC token, Codex-client requests fail before reaching the executor. Both selector paths call AccessTokenExpirationTime(), which now reads the SIWC token through this context-free helper, despite IsCodexUseSIWCForRequest explicitly requiring Codex clients to use outer credentials. A request with User-Agent codex-tui/0.154.0 therefore returns auth_unavailable even though its required outer token is valid. - Do not block Codex clients on SIWC expiration (sdk/cliproxy/auth/scheduler.go:897): With use_siwc=true, AccessTokenExpirationTime evaluates the nested SIWC token, but IsCodexUseSIWCForRequest explicitly routes Codex user agents through the outer token. This request-agnostic scheduler check therefore removes the credential when SIWC is expired or rejected even if the outer token is valid, causing the default fast path to return no auth for otherwise valid Codex-client requests.
Also affects:
|
|
||
| // IsAntigravityCreditsHintFresh reports whether a known credits hint is still | ||
| // within AntigravityCreditsHintRefreshInterval of now. | ||
| func IsAntigravityCreditsHintFresh(hint AntigravityCreditsHint, now time.Time) bool { |
There was a problem hiding this comment.
🟡 Medium · Treat future credit-hint timestamps as stale
If a hint is written by a node whose clock is ahead, now.Sub(hint.UpdatedAt) is negative and therefore always below the refresh interval. A known-unavailable hint can consequently exclude an Antigravity credential without revalidation until the local clock passes the future timestamp plus ten minutes, yielding a false no-fallback result when it is the only candidate.
| if !state.Quota.Exceeded || (reason != "quota" && !wrapped) { | ||
| continue |
There was a problem hiding this comment.
🟡 Medium · Quota restoration clears unrelated legacy model limits
RestoreCooldownStates loads model records before the auth-level record, so legacy records can leave a genuine model-scoped 429 as an unwrapped state with reason quota and no shadow. This condition admits every such state into restoration, regardless of whether its error describes daily/weekly exhaustion. A fresh positive Devin quota observation therefore resets an unrelated, still-live model rate limit. Reproduced with a 12-hour model rate-limit record and a separate credential_quota record.
| if state.Quota.Exceeded && (state.Quota.NextRecoverAt.IsZero() || state.Quota.NextRecoverAt.After(now)) { | ||
| state.LastError = nil | ||
| if strings.Contains(strings.ToLower(state.StatusMessage), "unauthorized") || isInvalidGrantErrorMessage(state.StatusMessage) { | ||
| state.StatusMessage = state.Quota.Reason | ||
| } | ||
| state.UpdatedAt = now | ||
| continue |
There was a problem hiding this comment.
🟡 Medium · Successful refresh retains the old 401 deadline
If a model receives a 10-second quota cooldown followed by an in-flight 401, MarkResult extends NextRetryAfter to 30 minutes while leaving Quota.NextRecoverAt at 10 seconds. After credential replacement, this branch clears the unauthorized error but retains its 30-minute retry deadline. The model consequently remains blocked long after its legitimate quota cooldown expires. Reproduced through MarkResult followed by UpdateRefreshedAuth.
| if updated.Status == StatusError || updated.Status == "" { | ||
| updated.Status = StatusActive | ||
| applyPreservedRejectedTokens(auth, updated, true) | ||
| if updated.RejectedAccessToken == "" || authAccessToken(updated) != updated.RejectedAccessToken { |
There was a problem hiding this comment.
🟡 Medium · Do not clear failures for an unrefreshed token partition
When the outer token is rejected and a refresh rotates only SIWC, applyPreservedRejectedTokens intentionally retains RejectedAccessToken; however this condition compares that outer token with authAccessToken(updated), which resolves to SIWC, and clears the auth error/unavailable state. Equivalent predicates in the merge, lifecycle, and SIWC-bind paths have the same result, so the scheduler can route Codex clients back to the unchanged rejected outer token and produce recurring 401s.
|
Closing: the review scope changed. Eric (2026-10-27): "i don't want to fix anything that's an issue in upstream - our only goal here is to validate our changes". Upstream code is out of scope. Our fork's own changes on v8.0.44 get one review PR instead. Findings from this part are recorded in the HOUSE-79 adjudication record. None was a defect in our code. |
Review only. Do not merge. This PR is one part of the review of #13, the merge of upstream
v8.0.44into the fork. That PR's diff (966 files) is too large for one review round.c88d701f7bc0894cde517ca7f931f5c05749cd74). The base is that tree with only this part's files restored tomain(c265779f), so the diff is this part's files alone and the surrounding code is the real final code.go.sumare in no part's diff.Linear: HOUSE-79
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.