feat(codex): record why a live binding was kept, moved, or released (#4546) #4592
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,9 +59,52 @@ type ThreadAffinityEntry = { | |
| }; | ||
|
|
||
| export type CodexThreadResolution = | ||
| | { status: "selected"; accountId: string; affinity?: CodexAffinityDecision } | ||
| | { status: "none"; affinity?: CodexAffinityDecision } | ||
| | { status: "expired"; accountId: string; affinity?: CodexAffinityDecision }; | ||
|
Comment on lines
+62
to
+64
There was a problem hiding this comment.
This adds a shared Codex routing result contract and threads it through authentication and request logging, but the commit updates none of the structure documents mapped to AGENTS.md reference: src/AGENTS.md:L11-L11 Useful? React with 👍 / 👎. |
||
|
|
||
| /** What happened to this thread's binding on this request (#4546). */ | ||
| export type CodexAffinityMove = | ||
| /** Served by its own bound account, which was healthy. */ | ||
| | "reused" | ||
| /** Served by its own bound account while something transient was wrong with it. */ | ||
| | "held" | ||
| /** Served by another account while the binding stayed put. */ | ||
| | "detour" | ||
| /** The binding was released and a different account took the thread. */ | ||
| | "rebound" | ||
| /** There was no live binding; this request established one. */ | ||
| | "new_bind" | ||
| /** The binding was released without a replacement on this request. */ | ||
| | "cleared"; | ||
|
|
||
| /** | ||
| * Why. A move is the expensive event -- it discards the prompt-cache prefix warmed on the old | ||
| * account -- so the operator should not have to infer it from account labels across log lines, | ||
| * which is how #4546 had to be diagnosed. | ||
| */ | ||
| export type CodexAffinityReason = | ||
| | "healthy" | ||
| | "quota_headroom" | ||
| | "quota_refusal" | ||
| | "transient" | ||
| | "transient_hold_expired" | ||
| | "unusable" | ||
| | "generation" | ||
| | "expired" | ||
| | "model_lane"; | ||
|
|
||
| export interface CodexAffinityDecision { | ||
| move: CodexAffinityMove; | ||
| reason: CodexAffinityReason; | ||
| } | ||
|
|
||
| /** The decision to report once a binding has been released and selection starts over. */ | ||
| function affinityAfterRelease(releaseReason: CodexAffinityReason | undefined): CodexAffinityDecision { | ||
| return releaseReason === undefined | ||
| ? { move: "new_bind", reason: "healthy" } | ||
| : { move: "rebound", reason: releaseReason }; | ||
| } | ||
|
|
||
| /** | ||
| * Process-local cursor for automatic RR/fill-first (and quota-429 when not | ||
|
|
@@ -360,17 +403,46 @@ export function clearThreadAccountMap(): void { | |
| threadAffinityEntryTotal = 0; | ||
| } | ||
|
|
||
| export function clearThreadAccountMapForAccount( | ||
| accountId: string, | ||
| reason: CodexAffinityReason = "unusable", | ||
| ): void { | ||
| for (const [threadId, affinities] of threadAccountMap) { | ||
| for (const [scope, entry] of affinities) { | ||
| if (entry.accountId === accountId && affinities.delete(scope)) { | ||
| threadAffinityEntryTotal = Math.max(0, threadAffinityEntryTotal - 1); | ||
| notePendingReleaseReason(threadId, reason); | ||
| } | ||
| } | ||
| if (affinities.size === 0) threadAccountMap.delete(threadId); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Why a binding was released, held until that thread's next resolve can report it (#4546). | ||
| * | ||
| * A release and the request that pays for it are two different moments: a 429 clears the pin | ||
| * inside the outcome recorder, and the next request arrives with nothing left to explain why it | ||
| * is starting cold. Bounded, because it is a diagnostic and must not become a leak. | ||
| */ | ||
| const pendingReleaseReasons = new Map<string, CodexAffinityReason>(); | ||
|
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Clear pending release reasons with the thread-affinity reset.
Clear 🤖 Prompt for AI Agents |
||
| const MAX_PENDING_RELEASE_REASONS = 4096; | ||
|
|
||
| function notePendingReleaseReason(threadId: string, reason: CodexAffinityReason): void { | ||
| if (!pendingReleaseReasons.has(threadId) && pendingReleaseReasons.size >= MAX_PENDING_RELEASE_REASONS) { | ||
| const oldest = pendingReleaseReasons.keys().next(); | ||
| if (!oldest.done) pendingReleaseReasons.delete(oldest.value); | ||
| } | ||
| pendingReleaseReasons.set(threadId, reason); | ||
| } | ||
|
|
||
| function consumePendingReleaseReason(threadId: string | null): CodexAffinityReason | undefined { | ||
| if (threadId === null) return undefined; | ||
| const reason = pendingReleaseReasons.get(threadId); | ||
| if (reason !== undefined) pendingReleaseReasons.delete(threadId); | ||
| return reason; | ||
| } | ||
|
|
||
| export function clearCodexUpstreamHealth(): void { | ||
| // Operator preferences are routing state, not health, but they live and die with the same | ||
| // reset points. Leaving them behind lets a selection from one context suppress the | ||
|
|
@@ -2701,9 +2773,9 @@ export function resolveCodexAccountForThreadDetailed( | |
| ); | ||
| if (cooler) { | ||
| bindModelDetourAffinity(threadId, cooler, now, modelId, quotaScope); | ||
| return { status: "selected", accountId: cooler, affinity: { move: "rebound", reason: "model_lane" } }; | ||
| } | ||
| return { status: "selected", accountId: detourEntry.accountId, affinity: { move: "reused", reason: "model_lane" } }; | ||
| } | ||
| // The model lane gets the same transient hold as the ordinary one. Without it a | ||
| // model-scoped request drops its detour pin on three 503s and falls back to an ordinary | ||
|
|
@@ -2717,25 +2789,28 @@ export function resolveCodexAccountForThreadDetailed( | |
| detourEntry.lastUsedAt = now; | ||
| if (lane !== null && lane !== detourEntry.accountId) { | ||
| detourEntry.transientDetourAccountId = lane; | ||
| return { status: "selected", accountId: lane, affinity: { move: "detour", reason: "transient" } }; | ||
| } | ||
| // A provider-wide outage soft-avoids every sibling, so there is nowhere to detour. | ||
| // That is a statement about where this request can go, not about who owns the | ||
| // conversation: dropping the pin here would rebuild the cold prefix elsewhere for | ||
| // exactly the failure mode the hold exists to survive. | ||
| return { status: "selected", accountId: detourEntry.accountId, affinity: { move: "held", reason: "transient" } }; | ||
| } | ||
| // Detour expiry or invalidation must not expire the ordinary task. Drop only | ||
| // this model lane and select from ordinary/shared state below. | ||
| deleteModelDetourAffinity(threadId, modelId, quotaScope); | ||
| } | ||
| } | ||
|
|
||
| // Why the binding went away, when it did. Carried to the selection below so the request that | ||
| // pays for a cold prefix can say what it paid for. | ||
| let releaseReason: CodexAffinityReason | undefined; | ||
| const entry = threadId ? getThreadAffinity(threadId, quotaScope) : undefined; | ||
| if (threadId && entry) { | ||
| if (isThreadAffinityExpired(entry, now)) { | ||
| deleteThreadAffinity(threadId, quotaScope); | ||
| return { status: "expired", accountId: entry.accountId, affinity: { move: "cleared", reason: "expired" } }; | ||
|
There was a problem hiding this comment.
When a thread binding expires, this new Useful? React with 👍 / 👎. |
||
| } | ||
| const generationLive = isThreadAffinityGenerationLive(entry); | ||
| const selectableForSharedState = generationLive | ||
|
|
@@ -2779,9 +2854,9 @@ export function resolveCodexAccountForThreadDetailed( | |
| promoteActiveCodexAccount(config, cooler); | ||
| } | ||
| bindThreadAffinity(threadId, cooler, now, quotaScope); // rebinds + resets clocks | ||
| return { status: "selected", accountId: cooler, affinity: { move: "rebound", reason: "quota_headroom" } }; | ||
| } | ||
| return { status: "selected", accountId: entry.accountId, affinity: { move: "reused", reason: "healthy" } }; | ||
| } | ||
| // Transient trouble on the bound account is a reason to send elsewhere, not a reason to | ||
| // give up the conversation. Detour this request and KEEP the binding, so recovery is free | ||
|
|
@@ -2798,21 +2873,33 @@ export function resolveCodexAccountForThreadDetailed( | |
| entry.transientDetourAccountId = detour; | ||
| // Deliberately no promoteActiveCodexAccount and no rebind: this is one request routing | ||
| // around a blip, not the pool deciding where the conversation now lives. | ||
| return { status: "selected", accountId: detour, affinity: { move: "detour", reason: "transient" } }; | ||
| } | ||
| // No sibling can take it either -- the usual shape of a provider-wide 503. The binding | ||
| // survives: "cannot send right now" and "forget which account owns this conversation" | ||
| // are different answers, and conflating them is what the hold was added to stop. | ||
| return { status: "selected", accountId: entry.accountId, affinity: { move: "held", reason: "transient" } }; | ||
| } | ||
| // A model-only exclusion does not invalidate the shared task binding. Health, | ||
| // generation, pause, cooldown, and failure evidence still retire it normally. | ||
| if (!modelScopedSelection || !healthyForSharedAffinity) { | ||
| releaseReason = !generationLive | ||
| ? "generation" | ||
| : quotaRefused | ||
| ? "quota_refusal" | ||
| : isTransientHoldExpired(entry, now) | ||
| ? "transient_hold_expired" | ||
| : !isCodexAccountUsable(config, entry.accountId, selectionOptions) | ||
| ? "unusable" | ||
| : "quota_headroom"; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| deleteThreadAffinity(threadId, quotaScope); | ||
| } else { | ||
| preserveExistingModelScopedAffinity = true; | ||
| } | ||
| } | ||
| // A release recorded by the outcome path (a 429 clears the pin before the next request even | ||
| // arrives) is the reason this request is starting cold, so it outranks having found nothing. | ||
| releaseReason ??= consumePendingReleaseReason(threadId); | ||
|
Comment on lines
+2900
to
+2902
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Preserve a release decision when no replacement exists. This consumes Return 🤖 Prompt for AI Agents |
||
|
|
||
| // A request-scoped roster may still contain unhealthy candidates. Non-quota strategies return | ||
| // before the quota/failover helpers below, so prefer only shared-healthy roster members here; | ||
|
|
@@ -2853,7 +2940,7 @@ export function resolveCodexAccountForThreadDetailed( | |
| // the thing the preference exists to protect. | ||
| promoteActiveCodexAccount(config, strategyPick); | ||
| } | ||
| return { status: "selected", accountId: strategyPick, affinity: affinityAfterRelease(releaseReason) }; | ||
|
There was a problem hiding this comment.
When Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| let active = getEffectiveActiveCodexAccountId(config); | ||
|
|
@@ -2864,7 +2951,7 @@ export function resolveCodexAccountForThreadDetailed( | |
| selectionOptions?.nativeMainSelectionOnly === true | ||
| && selectionOptions.modelEligibleAccountIds !== undefined | ||
| ) { | ||
| return { status: "selected", accountId: MAIN_CODEX_ACCOUNT_ID, affinity: affinityAfterRelease(releaseReason) }; | ||
| } | ||
| return { status: "none" }; | ||
| } | ||
|
|
@@ -2902,13 +2989,13 @@ export function resolveCodexAccountForThreadDetailed( | |
| // return main only as a non-mutating sentinel so the caller's atomic claim can | ||
| // classify maintenance. Do not fall through to the configured-but-ineligible | ||
| // active account or persist/bind this synthetic selection. | ||
| return { status: "selected", accountId: MAIN_CODEX_ACCOUNT_ID, affinity: affinityAfterRelease(releaseReason) }; | ||
| } else if ( | ||
| hasConfiguredPoolAccount(config, active, selectionOptions) | ||
| && !isCodexAccountPaused(config, active) | ||
| && !isCodexAccountPlanExcluded(config, active) | ||
| ) { | ||
| return { status: "selected", accountId: active, affinity: affinityAfterRelease(releaseReason) }; | ||
| } else { | ||
| return { status: "none" }; | ||
| } | ||
|
|
@@ -2950,13 +3037,13 @@ export function resolveCodexAccountForThreadDetailed( | |
| ); | ||
| if (!isCodexAccountUsable(config, active, selectionOptions)) { | ||
| return hasConfiguredPoolAccount(config, active, selectionOptions) | ||
| ? { status: "selected", accountId: active, affinity: affinityAfterRelease(releaseReason) } | ||
| : { status: "none" }; | ||
| } | ||
| if (isCodexAccountPaused(config, active)) return { status: "none" }; | ||
| if (getCodexQuotaHealthSnapshot(active, quotaScope, now)) { | ||
| return hasConfiguredPoolAccount(config, active, selectionOptions) | ||
| ? { status: "selected", accountId: active, affinity: affinityAfterRelease(releaseReason) } | ||
| : { status: "none" }; | ||
| } | ||
| if (threadId) { | ||
|
|
@@ -2966,7 +3053,7 @@ export function resolveCodexAccountForThreadDetailed( | |
| bindThreadAffinity(threadId, active, now, quotaScope); | ||
| } | ||
| } | ||
| return { status: "selected", accountId: active, affinity: affinityAfterRelease(releaseReason) }; | ||
| } | ||
|
|
||
| export function recordCodexUpstreamOutcome( | ||
|
|
@@ -3153,7 +3240,7 @@ export function recordCodexUpstreamOutcome( | |
| // The reauth flag carries the same provenance, so a replacement landing after this call cannot | ||
| // inherit a quarantine that was never about it. | ||
| markAccountNeedsReauth(accountId, writerGeneration, meta.credentialGeneration); | ||
| clearThreadAccountMapForAccount(accountId, "quota_refusal"); | ||
|
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Record the actual credential-release reason. A 401 or credential-classified 403 clears affinity because the account is unusable, not because upstream refused quota. The next resolution will incorrectly log Pass 🤖 Prompt for AI Agents |
||
| return; | ||
| } | ||
|
|
||
|
|
@@ -3188,7 +3275,7 @@ export function recordCodexUpstreamOutcome( | |
| // threads must leave it and new requests should prefer an eligible account. | ||
| // Reserve remains isolated so a same-account Terra/Luna combo fallback can run. | ||
| if (quotaScope === "shared" && !meta.fixedAccount) { | ||
| clearThreadAccountMapForAccount(accountId, "quota_refusal"); | ||
| notePoolRotationFailure(POOL_KEY_CODEX, accountId); | ||
| if (getEffectiveActiveCodexAccountId(config) === accountId) { | ||
| // Same-request 429 retry already picked via excludeAccountId — reuse it so | ||
|
|
@@ -3234,7 +3321,7 @@ export function recordCodexUpstreamOutcome( | |
| }), | ||
| }); | ||
| if (!meta.fixedAccount) { | ||
| clearThreadAccountMapForAccount(accountId, "quota_refusal"); | ||
| // An independent native quota request may discover an account-wide throttle, | ||
| // but it still must not advance the shared RR ring or active cursor. The next | ||
| // shared request observes the cooldown and chooses its own fallback. | ||
|
|
||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.