-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(oauth): choose the account with known headroom before dispatch #2878
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f43cb41
feat(oauth): open a turn on the account with known headroom
lidge-jun db21198
fix(oauth): close three defects in pre-dispatch account selection
lidge-jun 68e6bdb
fix(oauth): fall back instead of failing when a preferred account has…
lidge-jun 209bb11
fix(oauth): degrade to the active account when a preferred one vanishes
lidge-jun f121bcc
fix(oauth): re-check the chosen account against the live store before…
lidge-jun 134efaf
fix(oauth): reject an unusable preferred account inside the resolver'…
lidge-jun 420665c
docs(devlog): record the pre-dispatch selection phase and its five re…
lidge-jun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
87 changes: 87 additions & 0 deletions
87
devlog/_plan/260829_kiro_quota_pool/090_predispatch_selection.md
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 |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # 090 — Work-phase 3: pre-dispatch account selection | ||
|
|
||
| Doc `080` recorded kiro-lb as ahead on one axis that matters directly to the user's ask: | ||
| it picks an account *before* dispatch, while we only reordered the 429 recovery path. This | ||
| phase closes that gap. Branch `codex/kiro-pool-predispatch`, off merged `dev` `d82b3049d`. | ||
|
|
||
| ## What changed | ||
|
|
||
| `preferredInitialAccount(config, provider)` answers "which account should open this turn". | ||
| The initial OAuth resolution in `src/server/responses/core.ts` consults it and, when it | ||
| names an account, resolves that account's snapshot instead of the active one. | ||
|
|
||
| It is a **preference, not a gate**. A null answer means "use the active account", and null | ||
| is returned for: rotation disabled, fewer than two accounts, no quota evidence anywhere on | ||
| the roster, every candidate cooled, or the ranking simply agreeing with the active account. | ||
| A provider with no per-account quota therefore behaves exactly as before. | ||
|
|
||
| ## Five review rounds | ||
|
|
||
| An independent reviewer failed this four times before passing. Each finding was real, and | ||
| three of them were defects I would not have found by testing the happy path. | ||
|
|
||
| ### Round 1 — three blockers | ||
|
|
||
| 1. **Antigravity could pair B's bearer with A's project.** The ordinary path fills the CCA | ||
| project only when it is *empty* (`!route.provider.project`), so a preferred account | ||
| installed its own bearer beside the configured account's project — #2841 in its | ||
| original shape, at a site nobody had reason to look at. | ||
| 2. **A quota-less provider could still be redirected.** Cooling the active account collapses | ||
| the eligible list to one candidate, and ranking a single candidate returns it unchanged. | ||
| That *looks* like a ranked answer while nothing was ever measured. Evidence is now | ||
| checked across the whole roster, before eligibility narrows anything. | ||
| 3. **Two uncached credential-file reads per request.** `loadAuthStore` chmods the config | ||
| dir, chmods the secret, and re-parses the whole file on every call — the exact cost the | ||
| neighbouring `PRESENCE_CACHE_TTL_MS` comment exists to warn about. | ||
|
|
||
| ### Round 2 — the fail-closed 401 was worse than the bug | ||
|
|
||
| My first Antigravity fix returned 401 when a preferred account had no project. But | ||
| Antigravity tolerates project discovery failing, so a project-less account is an ordinary | ||
| stored state: a *preference* had been given the power to break a request that would | ||
| otherwise have worked. It now falls back to the active account. | ||
|
|
||
| ### Round 3 — a removed account became a 401 | ||
|
|
||
| The roster is cached for two seconds, so an account can be deleted after being chosen. | ||
| Resolving it throws, and that throw reached the client as 401 while a healthy active | ||
| account sat unused. The reviewer reproduced it exactly. Resolution failures now drop the | ||
| stale roster and retry on the active account. | ||
|
|
||
| ### Round 4 — the one a catch could not catch | ||
|
|
||
| The sharpest finding. An account newly flagged `needsReauth` **does not throw**: its | ||
| credential is still readable, so resolution succeeds and no error path fires. The request | ||
| would dispatch on an account already known to need a fresh login. | ||
|
|
||
| My first fix re-read the store to validate the winner — and reopened blocker 3, because the | ||
| steady state of this feature is a pool where one account consistently ranks higher, so | ||
| "validate only on redirect" is "validate on every request". | ||
|
|
||
| ### Round 5 — atomic validation, then PASS | ||
|
|
||
| The check belongs where the store row is *already* being read. | ||
| `getAccountCredentialWithStatus` returns credential and `needsReauth` from one read, and | ||
| `requireUsableAccount` makes account-scoped resolution reject an unusable account from | ||
| inside it. Selection now performs no store read at all; the caller's existing fallback | ||
| handles the rejection. Zero added I/O on the redirect path, both stale classes closed. | ||
|
|
||
| ## Verification | ||
|
|
||
| ```text | ||
| bun x tsc --noEmit -> exit 0 | ||
| bun run privacy:scan -> Privacy scan passed | ||
| bun test (11 files) -> 181 pass / 0 fail / 656 expect() calls | ||
| core-lab-boundary -> pass, no new src/lab/ reach | ||
| ``` | ||
|
|
||
| Tests worth naming, because each encodes a defect above: a redirecting selection with | ||
| `auth.json` deleted still answers (proves the cache); a reauth-flagged account resolves | ||
| plainly but rejects under `requireUsableAccount` (proves why a catch was insufficient); and | ||
| cooling the *active* account of a quota-less provider still returns null. | ||
|
|
||
| ## Result | ||
|
|
||
| The "pre-request selection" row moves out of doc `080`'s "they are ahead" column. Two rows | ||
| remain there honestly: kiro-lb persists quota across restart, and it has a real operations | ||
| dashboard. Neither is in scope here. |
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
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.
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.
When a Kiro quota probe ages past
ACCOUNT_QUOTA_TTL_MS,getKiroAccountExhaustiondeliberately returnsnull, butgetCachedProviderAccountQuotacontinues returning the percentage row becausesweepExpiredProviderAccountQuotaRowsis not registered inSTATE_STORE_REGISTRATIONS. This check therefore keeps accepting stale evidence; for example, a previously exhausted 100%-used account becomes “measured healthy” once its exhaustion verdict expires and can beat an unknown active account, proactively sending later requests back to the exhausted account. Make the evidence/ranking path enforce the quota TTL before redirecting.Useful? React with 👍 / 👎.