-
Notifications
You must be signed in to change notification settings - Fork 602
stack 5/7: probe reset-derived cooldowns without waiting to be selected (#915) #955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6ebc81a
2b805f8
0cbd672
1ab19f3
493329d
8b69769
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,15 +67,48 @@ export function isCodexQuotaExhausted( | |
| plan?: string | null, | ||
| ): boolean { | ||
| if (!quota) return false; | ||
| const normalizedPlan = plan?.trim().toLowerCase(); | ||
| const values = normalizedPlan === "go" || normalizedPlan === "free" | ||
| const values = codexQuotaWindowForPlan(plan) === "monthly" | ||
| ? [quota.monthlyPercent] | ||
| : [quota.weeklyPercent, quota.monthlyPercent]; | ||
| return values.some(value => typeof value === "number" | ||
| && Number.isFinite(value) | ||
| && value >= CODEX_EXHAUSTED_USAGE_PERCENT); | ||
| } | ||
|
|
||
| /** | ||
| * Which usage window a plan reports in. This is the SINGLE rule shared by quota | ||
| * parsing, exhaustion, and recovery — they must not diverge. | ||
| * | ||
| * An allowlist of "known" plans was tried here and was wrong: the upstream model | ||
| * snapshot alone carries 21 distinct plan strings (`edu_plus`, `finserv`, `k12`, | ||
| * `quorum`, `self_serve_business_usage_based`, ...), and `CodexAccount.plan` is an | ||
| * unrestricted string, so any list is a list of the plans someone remembered. | ||
| * Twelve real plans would have been refused recovery and stayed cooled forever — | ||
| * the very defect this unit exists to fix, reintroduced as a typo-shaped hole. | ||
| * | ||
| * The honest rule is the parser's own: Go and Free report a 30-day window, | ||
| * everything else (including an absent plan) reports weekly. Recovery reads the | ||
| * window the parser actually wrote rather than second-guessing it. | ||
| */ | ||
| export function codexQuotaWindowForPlan(plan?: string | null): "monthly" | "weekly" { | ||
| const normalized = plan?.trim().toLowerCase(); | ||
| return normalized === "go" || normalized === "free" ? "monthly" : "weekly"; | ||
| } | ||
|
|
||
| export function isCompleteCodexQuotaRecoverySnapshot( | ||
| quota: Pick<StoredAccountQuota, "weeklyPercent" | "monthlyPercent"> | null, | ||
| plan?: string | null, | ||
| ): boolean { | ||
| if (!quota || isCodexQuotaExhausted(quota, plan)) return false; | ||
| // Recovery still fails closed on MISSING EVIDENCE — a credits-only or windowless payload | ||
| // carries no usage reading at all and must never clear a cooldown. What it does not do is | ||
| // fail closed on an unfamiliar plan NAME, which only ever meant "cooled forever". | ||
| const required = codexQuotaWindowForPlan(plan) === "monthly" | ||
| ? quota.monthlyPercent | ||
| : quota.weeklyPercent; | ||
|
Comment on lines
+106
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For Team and other non-Free/Go plans, WHAM can legitimately return only an explicitly monthly primary window; Useful? React with 👍 / 👎. |
||
| return typeof required === "number" && Number.isFinite(required); | ||
| } | ||
|
|
||
| export function normalizeUsagePercent(value: unknown): number | undefined { | ||
| const numeric = typeof value === "number" | ||
| ? value | ||
|
|
@@ -367,7 +400,7 @@ export function parseUsageQuota(data: WhamUsageResponse): Omit<StoredAccountQuot | |
| } | ||
|
|
||
| const quota: Omit<StoredAccountQuota, "updatedAt"> = {}; | ||
| const thirtyDayOnly = data.plan_type?.trim().toLowerCase() === "go" || data.plan_type?.trim().toLowerCase() === "free"; | ||
| const thirtyDayOnly = codexQuotaWindowForPlan(data.plan_type) === "monthly"; | ||
| const primaryWindow = data.rate_limit.primary_window; | ||
| const secondaryWindow = data.rate_limit.secondary_window; | ||
| const tertiaryWindow = data.rate_limit.tertiary_window; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,18 @@ export type CodexCooldownSource = "retry-after" | "reset-derived" | "default"; | |
| */ | ||
| export type CodexQuotaScope = "shared" | "spark"; | ||
|
|
||
| export type CodexQuotaRecoveryProbeClaim = { | ||
| accountId: string; | ||
| scope?: CodexQuotaScope; | ||
| leaseId: string; | ||
| cooldownGeneration: number; | ||
| credentialGeneration: number; | ||
| }; | ||
|
|
||
| export type CodexQuotaRecoveryProbeProof = { | ||
| credentialGeneration?: number; | ||
| }; | ||
|
|
||
| /** | ||
| * Requests without a resolved native model retain the historic one-account-per- | ||
| * thread behavior. Requests with a known quota scope get an independent | ||
|
|
@@ -417,6 +429,119 @@ function canAcquireQuotaProbeLease(health: CodexUpstreamHealth | undefined, now: | |
| return now - origin >= CODEX_QUOTA_PROBE_INTERVAL_MS; | ||
| } | ||
|
|
||
| /** | ||
| * Claim due reset-derived cooldown probes without consulting account selection. | ||
| * Pool credentials only: the main account has no quota-refresh single-flight. | ||
| */ | ||
| export function claimDueCodexQuotaRecoveryProbes( | ||
| config: OcxConfig, | ||
| limit: number, | ||
| now = Date.now(), | ||
| ): CodexQuotaRecoveryProbeClaim[] { | ||
| const boundedLimit = Math.max(0, Math.floor(limit)); | ||
| if (boundedLimit === 0) return []; | ||
| const candidates: Array<{ | ||
| accountId: string; | ||
| scope?: CodexQuotaScope; | ||
| health: CodexUpstreamHealth; | ||
| credentialGeneration: number; | ||
| order: number; | ||
| }> = []; | ||
| for (const [order, account] of (config.codexAccounts ?? []).entries()) { | ||
| if (!isSelectableCodexPoolAccount(account) | ||
| || isCodexAccountPaused(config, account.id) | ||
| || isAccountNeedsReauth(account.id)) continue; | ||
| const record = readCodexAccountRecord(account.id); | ||
| if (!record?.credential || record.deletedAt != null) continue; | ||
| const due = [ | ||
| { scope: undefined, health: upstreamHealth.get(account.id) }, | ||
| ...[...(quotaScopedHealth.get(account.id) ?? [])].map(([scope, health]) => ({ scope, health })), | ||
| ].filter((entry): entry is { scope?: CodexQuotaScope; health: CodexUpstreamHealth } => | ||
| // `spark` is deliberately never claimed. `GET /backend-api/wham/usage` takes no scope | ||
| // parameter and returns generic weekly/monthly windows, so its result can never prove a | ||
| // spark recovery — a claim here would spend an upstream call to settle `false` every | ||
| // time, and (with one claim per account per pass) delay the shared scope that CAN recover. | ||
| entry.scope !== "spark" | ||
| && entry.health?.cooldownSource === "reset-derived" | ||
| && canAcquireQuotaProbeLease(entry.health, now)) | ||
| .sort((a, b) => | ||
| (a.health.lastProbeAt ?? a.health.cooldownSince ?? 0) | ||
| - (b.health.lastProbeAt ?? b.health.cooldownSince ?? 0)); | ||
| const candidate = due[0]; | ||
| if (candidate) candidates.push({ | ||
| accountId: account.id, | ||
| ...(candidate.scope ? { scope: candidate.scope } : {}), | ||
| health: candidate.health, | ||
| credentialGeneration: record.generation, | ||
| order, | ||
| }); | ||
| } | ||
| candidates.sort((a, b) => { | ||
| const age = (a.health.lastProbeAt ?? a.health.cooldownSince ?? 0) | ||
| - (b.health.lastProbeAt ?? b.health.cooldownSince ?? 0); | ||
| return age || a.order - b.order; | ||
| }); | ||
| return candidates.slice(0, boundedLimit).map(candidate => { | ||
| const leaseId = randomUUID(); | ||
| const next = { | ||
| ...candidate.health, | ||
| probeLeaseId: leaseId, | ||
| probeLeaseGeneration: candidate.health.cooldownGeneration ?? 0, | ||
| lastProbeAt: now, | ||
| }; | ||
| if (candidate.scope) setScopedHealth(candidate.accountId, candidate.scope, next); | ||
| else upstreamHealth.set(candidate.accountId, next); | ||
| return { | ||
| accountId: candidate.accountId, | ||
| ...(candidate.scope ? { scope: candidate.scope } : {}), | ||
| leaseId, | ||
| cooldownGeneration: candidate.health.cooldownGeneration ?? 0, | ||
| credentialGeneration: candidate.credentialGeneration, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| /** Settle one background recovery claim without mutating account-wide outcome state. */ | ||
| export function settleCodexQuotaRecoveryProbe( | ||
| claim: CodexQuotaRecoveryProbeClaim, | ||
| recovered: boolean, | ||
| proof: CodexQuotaRecoveryProbeProof, | ||
| now = Date.now(), | ||
| ): boolean { | ||
| const health = claim.scope | ||
| ? scopedHealthFor(claim.accountId, claim.scope) | ||
| : upstreamHealth.get(claim.accountId); | ||
| if (!health || health.probeLeaseId !== claim.leaseId) return false; | ||
| const fenced = (health.cooldownGeneration ?? 0) === claim.cooldownGeneration | ||
| && (health.probeLeaseGeneration ?? 0) === claim.cooldownGeneration | ||
| && claim.credentialGeneration === proof.credentialGeneration | ||
| && isCodexAccountGenerationLive(claim.accountId, claim.credentialGeneration); | ||
|
Comment on lines
+517
to
+518
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a cooled account's access token is near expiry, Useful? React with 👍 / 👎. |
||
| if (!recovered || !fenced) { | ||
| const released = withProbeLeaseReleased(health, now); | ||
| if (claim.scope) setScopedHealth(claim.accountId, claim.scope, released); | ||
| else upstreamHealth.set(claim.accountId, released); | ||
| return false; | ||
| } | ||
| if (claim.scope) { | ||
| deleteScopedHealth(claim.accountId, claim.scope); | ||
| } else { | ||
| const { | ||
| cooldownUntil: _until, | ||
| cooldownSince: _since, | ||
| cooldownSource: _source, | ||
| probeLeaseId: _leaseId, | ||
| probeLeaseGeneration: _leaseGeneration, | ||
| ...rest | ||
| } = health; | ||
| upstreamHealth.set(claim.accountId, { | ||
| ...rest, | ||
| cooldownGeneration: claim.cooldownGeneration + 1, | ||
| lastProbeAt: now, | ||
| }); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** Acquire the recovery probe for one confirmed model-specific quota group. */ | ||
| export function tryAcquireCodexQuotaScopeProbeLease( | ||
| accountId: string, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a minimal failure signal to the recovery worker.
The worker swallows every failure. Line 849 catches per-claim errors, and line 853 catches worker-level errors. Neither path records anything. The worker runs on the 60-second sweep tick and issues upstream WHAM calls.
The failure mode is diagnostic, not functional. If an account stays cooled, an operator cannot distinguish these cases from outside:
retry-afteror default-derived.That is the exact question issue
#915raises, so the fix for it should be observable.settleCodexQuotaRecoveryProbealready returns a boolean, and the return value is currently discarded at lines 846 and 850.A counter or a debug-level line keyed on the settle outcome is enough. Do not include the account id, the plan, or any token material in the signal.
📈 Sketch: capture settle outcomes without logging credentials
cooldownRecoveryInFlight = (async () => { const claims = claimDueCodexQuotaRecoveryProbes(config, POOL_QUOTA_REFRESH_CONCURRENCY, now); + let cleared = 0; + let retained = 0; await mapWithConcurrency(claims, POOL_QUOTA_REFRESH_CONCURRENCY, async claim => { const account = configuredPoolAccount(config, claim.accountId); if (!account) { settleCodexQuotaRecoveryProbe(claim, false, {}, now); + retained += 1; return; } try { const result = await fetchPoolAccountQuota(claim.accountId, true, account.plan); const recovered = claim.scope !== "spark" && isCompleteCodexQuotaRecoverySnapshot(result.freshQuota ?? null, result.freshPlan ?? account.plan); - settleCodexQuotaRecoveryProbe(claim, recovered, { + const settled = settleCodexQuotaRecoveryProbe(claim, recovered, { credentialGeneration: result.freshCredentialGeneration, }, now); + if (settled) cleared += 1; else retained += 1; } catch { settleCodexQuotaRecoveryProbe(claim, false, {}, now); + retained += 1; } }); + recordCodexCooldownRecoveryPass({ claimed: claims.length, cleared, retained }); })().catch(() => {Do you want me to wire this into the existing metrics surface instead of a local counter?
🤖 Prompt for AI Agents