Repository navigation
Review only: HOUSE-79 our fork's changes on upstream v8.0.44 - #26
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. |
🛑 10 blocking findings🟡 Medium · Newer quota observations can be overwritten by stale refreshes 🟡 Medium · In-flight polls can overwrite newer quota evidence 🟡 Medium · Bind usage results to the credential registration 🟡 Medium · Do not resurrect explicitly inactive quota windows 🟡 Medium · Idle Claude 5h window permanently trips quota reserve 🟡 Medium · Version persisted quota observations by observation time 🟡 Medium · Decode compressed Codex usage responses 🟡 Medium · Normalize provider names before scheduling usage reads 🟡 Medium · Preserve resolved-model availability in soonest-reset 🟡 Medium · Keep a 100% reserve enforced after quota resets WalkthroughThis review-only PR isolates the fork's own changes on top of 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.
🛑 10 blocking findings
🟡 Medium · Newer quota observations can be overwritten by stale refreshes
🟡 Medium · In-flight polls can overwrite newer quota evidence
🟡 Medium · Bind usage results to the credential registration
🟡 Medium · Do not resurrect explicitly inactive quota windows
🟡 Medium · Idle Claude 5h window permanently trips quota reserve
🟡 Medium · Version persisted quota observations by observation time
🟡 Medium · Decode compressed Codex usage responses
🟡 Medium · Normalize provider names before scheduling usage reads
🟡 Medium · Preserve resolved-model availability in soonest-reset
🟡 Medium · Keep a 100% reserve enforced after quota resets
Check
| } else if auth.Quota.ObservedAt.IsZero() { | ||
| auth.Quota = mergeQuotaObservation(auth.Quota, existing.Quota) |
There was a problem hiding this comment.
🟡 Medium · Newer quota observations can be overwritten by stale refreshes
A refresh/update can carry an older non-zero Quota.ObservedAt while a concurrent usage read has already written a newer observation to the registered auth. Because this branch merges the existing quota only when the incoming timestamp is zero, the stale update replaces the newer Signals and ReserveWindows, causing soonest-reset or quota-reserve routing to use stale data until another observation arrives.
| } | ||
| m.mu.Lock() | ||
| auth := m.auths[authID] | ||
| observed := auth != nil && auth.Quota.observeHeaders(auth.Provider, headers, time.Now(), true) |
There was a problem hiding this comment.
🟡 Medium · In-flight polls can overwrite newer quota evidence
A usage read can capture 9% usage, then finish after an inference response records 95%. This line timestamps the older reading at completion, replacing the newer Signals and ReserveWindows. With a 20% reserve, the credential becomes selectable again despite the newer evidence exceeding the 80% threshold. Poll commits need a freshness check against observations received since the read started.
| log.Warnf("quota usage read failed for auth %s: %v", authID, errRead) | ||
| return | ||
| } | ||
| s.coreManager.ObserveQuotaHeaders(authID, observed) |
There was a problem hiding this comment.
🟡 Medium · Bind usage results to the credential registration
If a credential is removed and re-registered under the same ID for another account while ReadQuotaUsage is pending, this call attaches the old account's response to the replacement. A low reading can therefore admit a previously unobserved protected account and bypass its reserve. The commit does not validate the RegistrationEpoch or CredentialVersion of the credential used for the read. Also reported for this defect: - Reject usage reads from superseded credentials (sdk/cliproxy/service_quota.go:70): If an auth is removed and re-registered under the same ID while ReadQuotaUsage is in flight, this call applies the old account's response to the replacement credential. A delayed 9%-used response can overwrite the replacement's 95%-used observation and admit it despite a 20% reserve. Unlike MarkResult, this path does not check the captured registration epoch or credential version before committing.
| if current, exists := merged[name]; !exists || !window.ObservedAt.Before(current.ObservedAt) { | ||
| merged[name] = window |
There was a problem hiding this comment.
🟡 Medium · Do not resurrect explicitly inactive quota windows
A refresh can snapshot a secondary window at 95%, then a newer zero-minute placeholder deletes that window before the refresh completes. This merge unconditionally accepts the older source entry because the target key is absent. MergeRefreshedAuth thus resurrects the inactive window and incorrectly blocks the now-weekly-only credential until another explicit deletion or the old reset. Also reported for this defect: - Preserve inactive-window removals during quota merges (sdk/cliproxy/auth/quota_reserve_history.go:105-106): An explicit Codex Window-Minutes: 0 observation deletes the corresponding reserve window, but this merge treats its absence as permission to restore any older source value. If a token refresh started before that observation, MergeRefreshedAuth resurrects the deleted window from its snapshot. For example, an obsolete 95%-used secondary window again excludes an otherwise eligible weekly-only credential until the obsolete reset expires.
| } else if next.UsedPercent == 0 && !rejected { | ||
| next.UsageTrusted = false |
There was a problem hiding this comment.
🟡 Medium · Idle Claude 5h window permanently trips quota reserve
Scenario: a Claude OAuth credential is listed in routing.quota-reserves (any percent > 0) and has no prior reserve history. That happens after a restart with the default save-cooldown-status:false, or for a newly added credential. The account is idle, so the startup usage read returns five_hour {utilization: 0, resets_at: null}. What happens: ParseClaudeUsageHeaders emits only the 5h Utilization header. observeReserveWindows reaches this else-if with no previous entry and stores the 5h window with UsageTrusted=false and a zero ResetAt. quotaReserveReached (quota_reserve.go:33-37) does not skip zero-reset windows and returns true for any untrusted window. The credential is therefore dropped from every pickNextLegacy, pickNextMixedLegacy and pickViaBuiltinScheduler candidate list. Why it never clears: excluded credentials get no inference, so the proxy never starts their 5h window. Every later read still reports 0 with no reset, and line 59 (exists && !rejected && (... || next.UsedPercent == 0) → continue) discards it. The block lifts only if someone uses the account outside the proxy. Result: an account with its full quota is never routed to, and requests fail with auth_not_found if it is the only candidate. The same account with a persisted, past-reset history entry is allowed, so the outcome depends on persistence rather than actual usage.
| Model: model, | ||
| Status: cooldownStateStatusObserved, | ||
| Quota: QuotaState{ObservedAt: observation.ObservedAt, Signals: observation.Signals, ReserveWindows: observation.ReserveWindows}, | ||
| UpdatedAt: updatedAt, |
There was a problem hiding this comment.
🟡 Medium · Version persisted quota observations by observation time
Usage polls advance quota observations without advancing auth/model UpdatedAt, but observation-only records use that older timestamp. With two instances sharing PostgreSQL, a request on one instance can advance the stored record beyond the other instance's timestamp; subsequent fresh polls on the latter are silently rejected by the store's timestamp-guarded upsert. Restart then restores the older quota. The existing-record attachment branch at lines 741–744 likewise leaves the persistence version unchanged.
| body, errRead := io.ReadAll(resp.Body) | ||
| if errRead != nil { | ||
| return nil, fmt.Errorf("read codex usage response: %w", errRead) | ||
| } | ||
| return helps.ParseCodexUsageHeaders(body), nil |
There was a problem hiding this comment.
🟡 Medium · Decode compressed Codex usage responses
The identity helper forwards credential-specific Accept-Encoding headers. When an operator configures gzip or another supported encoding, net/http does not automatically decompress the explicitly requested encoding. Unlike the inference request path, this method reads and parses the compressed bytes directly. A compressed 200 response consequently returns nil observation headers without an error, leaving quota routing stale or a previously unobserved reserved credential permanently excluded.
| if !ok || auth.Disabled || auth.AuthKind() != coreauth.AuthKindOAuth || !auth.HasValidAccessToken(time.Now()) { | ||
| return | ||
| } | ||
| if auth.Provider != "codex" && auth.Provider != "claude" { |
There was a problem hiding this comment.
🟡 Medium · Normalize provider names before scheduling usage reads
Provider dispatch elsewhere accepts case-insensitive provider names, but this exact comparison skips an otherwise valid OAuth auth whose Provider is, for example, "Claude" or "Codex". Quota-reserve matching is case-insensitive, so such an auth with no prior windows is excluded while this reader permanently refuses to populate those windows, resulting in auth_not_found for requests using that credential.
| func (s *SoonestResetSelector) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*Auth) (*Auth, error) { | ||
| _ = opts | ||
| now := time.Now() | ||
| available, err := getSelectorAvailableAuths(ctx, auths, provider, model, now) |
There was a problem hiding this comment.
🟡 Medium · Preserve resolved-model availability in soonest-reset
Without session affinity, the manager checks each credential's resolved upstream model, but this call checks availability again using the client-visible alias: SoonestResetSelector is not given the prevalidated-candidates context. Retargeting alias A to healthy model B while A retains a cooldown consequently rejects otherwise eligible credentials with a model_cooldown 429.
| if !window.ResetAt.IsZero() && !window.ResetAt.After(now) { | ||
| continue |
There was a problem hiding this comment.
🟡 Medium · Keep a 100% reserve enforced after quota resets
Validation accepts a 100% reserve, which leaves no quota available to proxy requests. Once every recorded window's reset passes, however, this continue skips every threshold check and quotaReserveReached returns false. Both Claude and Codex credentials can then be selected and consume their newly reset quota despite the full reserve, until another usable observation arrives.
Review only. Do not merge. This is the review of our own changes in #13, the merge of upstream
v8.0.44into the fork. That PR's own diff (966 files, almost all upstream) is too large for one review round. Upstream's code is out of scope: our goal is to validate our changes.c88d701f). The base is that tree with only our changed production files restored to upstreamv8.0.44. So this diff is our fork's delta on the new base, and the surrounding code is upstreamv8.0.44.v8.0.44, inconductor_selection.go,conductor_lifecycle.go,config_v8.go,service_config.goandcodex_executor_request.go;conductor_lifecycle.go).go.sum, docs and workflows.v8.0.44code around them. Do not report defects in upstream code that our changes do not touch.Linear: HOUSE-79
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.