[WRONG BRANCH] fix(codex): bound entitlement version cache misses - #392
[WRONG BRANCH] fix(codex): bound entitlement version cache misses#392luvs01 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01d1186544
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const recent = (accountModelsMisses.get(missKey) ?? []) | ||
| .filter(startedAt => startedAt > now - MODEL_ROSTER_TTL_MS); | ||
| if (recent.length >= MODEL_ROSTER_VERSION_MISSES_PER_ACCOUNT_MAX) { | ||
| accountModelsMisses.set(missKey, recent); | ||
| return { | ||
| credentialIdentity: credential.credentialIdentity, | ||
| clientVersion, | ||
| expiresAt: now, | ||
| models: new Set(), | ||
| confirmed: false, | ||
| }; | ||
| } | ||
| recent.push(now); | ||
| accountModelsMisses.set(missKey, recent); |
There was a problem hiding this comment.
When one legitimate non-runtime client_version is retried during an upstream outage, every retry after the 15-second failure-cache expiry appends another timestamp because the budget does not record which version caused the miss. After four failed attempts, even that same version is refused until the five-minute roster window expires, so an upstream recovery after roughly one minute can leave gated models unavailable for several additional minutes. Track distinct versions as documented, or allow a previously counted version to continue retrying on the existing failure TTL.
Useful? React with 👍 / 👎.
| const missKey = `${credential.accountId}\u0000${credential.credentialIdentity}`; | ||
| const recent = (accountModelsMisses.get(missKey) ?? []) | ||
| .filter(startedAt => startedAt > now - MODEL_ROSTER_TTL_MS); |
There was a problem hiding this comment.
For Pool accounts, a normal access-token refresh increments the credential generation, so this identity-qualified key changes even when the account ID stays the same. Expired entries are pruned only when that exact old key is queried again, while the invalidation function is called only on model-400 retry paths, not routine credential refreshes; consequently, an active caller can leave one permanent map entry per account generation for the lifetime of the process. Replace the prior identity's entry when a generation changes or opportunistically remove expired keys so this new protection does not introduce an unbounded memory cache.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Motivation
client_versionquery onGET /v1/modelscould be cycled to create renewable cache misses that spawned authenticated upstream/backend-api/codex/modelsrequests under every stored account token, allowing sustained request amplification and causing legitimate gated-model checks to fail-closed.Description
MODEL_ROSTER_VERSION_MISSES_PER_ACCOUNT_MAX) and anaccountModelsMissesmap to limit how many caller-selected versions may trigger upstream requests in one roster TTL.MODEL_ROSTER_FLIGHTS_PER_ACCOUNT_MAX).Testing
bun test tests/codex-model-entitlements.test.ts, which passed (21 tests, 0 failures).bun run typecheckand the privacy check withbun run privacy:scan, both of which succeeded.bun run test) was executed; the entitlement-focused changes pass, while unrelated environment/timing tests in the full suite reported known external failures that are not caused by this change (these were observed but are outside the modified subsystem).Codex Task