MoonshotAI / MoonshotAI/kimi-cli
tracking: finish the OAuth race-prevention architecture (follow-up to #1996)
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 11.4k
- Forks
- 1.3k
- Avg merge
- 9h 47m
- Merged PRs (30d)
- 2
Description
What feature would you like to see?
Body
PR #1996 fixed the most destructive symptom — delete_tokens(ref) in the OAuthUnauthorized handler would wipe a concurrent instance's freshly-rotated token — and added a process-local rejection tombstone (_REJECTED_REFRESH_TOKENS) to preserve the OAuth + static api_key fallback path.
But the race itself still happens; it just no longer produces permanent damage. The tombstone is a Band-Aid, not a root-cause fix. This issue tracks the three remaining pieces of architectural work. Each item is independently shippable — check off as PRs land.
Background
Enough context to understand the three items:
- PR #1996 — the current Band-Aid; also where the rejection tombstone lives
- PR #1822 — introduced
_CrossProcessLock(central to item 2) - PR #1819 — confirmed via a 16-minute real-token experiment that
access_tokenlives only 15 min - PR #1658 — unintentionally introduced the "new
OAuthManagerper session-title-gen" anti-pattern (central to item 1) - #1940 — the "red alert keeps popping up, Retry eventually works" symptom in the VSCode extension, caused by item 3 not being fixed
Todo · 3 items
[ ] 1. Multiple OAuthManager instances per process — make it a singleton 【High】
Problem. Four places in the codebase instantiate their own OAuthManager:
src/kimi_cli/app.py:199src/kimi_cli/web/api/sessions.py:817(per session-title-gen — worst offender)src/kimi_cli/cli/plugin.py:212src/kimi_cli/plugin/tool.py:33
Each has its own _refresh_lock scoped to the instance, so managers in the same process coordinate nothing — they all read/write the same credentials file directly. With 15-min access tokens and a 7.5-min refresh threshold, each manager fires a refresh cycle frequently, and collision probability accumulates over normal daily usage.
Proposed direction. Make OAuthManager a per-process singleton (lazy or DI). Web / FastAPI paths route it through dependency injection. Once done, in-process race disappears at the root and the tombstone from #1996 can be simplified.
Acceptance.
- Exactly one
OAuthManagerinstance per process, accessible via a well-defined getter - All current creation sites routed through the singleton
- New test: two components hitting the refresh threshold simultaneously issue exactly one refresh
- Existing auth tests still pass
[ ] 2. _CrossProcessLock silently degrades to unlocked refresh — fail-closed 【Medium】
Problem. acquire_with_retry() (oauth.py:307) returns False after exhausting retries. The caller in _refresh_tokens (oauth.py:946) logs a warning and proceeds to refresh anyway, unlocked:
else:
logger.warning("Could not acquire cross-process lock for token refresh")
# ... unconditionally falls through to refresh_token(...)
This is the exact entry point for the race: two processes both send a refresh with R1, server consumes R1 for the winner, the loser gets 401. The architecturally correct behaviour is not to race in the first place.
Proposed direction (pick one; option 3 recommended):
- Fail closed — on acquire failure, return without attempting refresh. Next
ensure_freshcycle (60 s later) retries. - Block longer — extend total retry budget to ~30 s with exponential backoff. Most refreshes complete in < 5 s, so 30 s absorbs realistic contention.
- Hybrid — after timeout, re-read the file; if
refresh_tokenhas changed, treat the operation as completed by a peer. Only fall back to unlocked refresh when disk is unchanged andforce=True.
Also address the related Codex P1 from PR #1822's review: _acquire at oauth.py:288 conflates all OSError cases, so environments without flock support (NFS, some FUSE mounts) silently degrade instead of failing clearly.
Acceptance.
- Lock contention never leads to multiple processes issuing refresh requests simultaneously
tests/auth/test_oauth_cross_process.pystill passes- New test: 3+ processes racing to refresh result in exactly one successful rotation
- FS environments without
flockfail clearly rather than silently degrading
[ ] 3. ACP and wire-server 401 paths bypass auto refresh+retry 【Medium】
Problem. Three 401 surfaces behave differently today:
| Surface | Location | Behaviour on 401 |
|---|---|---|
| Interactive shell | soul/kimisoul.py:1178 (_run_with_connection_recovery) |
✅ ensure_fresh(force=True) then retry the operation once — user sees nothing if refresh picks up a rotated token |
| ACP session (VSCode / Cursor / Zed) | acp/session.py:221 |
❌ raise auth_required() immediately — no refresh attempt |
| Wire server (web UI) | wire/server.py:668 |
❌ Returns AUTH_EXPIRED immediately — no refresh attempt |
Because ACP and wire bypass _run_with_connection_recovery, users see "please /login" prompts even when a valid rotated token is sitting on disk. When the client retries (e.g. the VSCode "Retry" button in #1940), the second request happens to pick up the new token — producing the classic "red alert keeps popping up, Retry eventually works" loop.
Proposed direction. Extract a shared 401-recovery helper from _run_with_connection_recovery and call it from all three handlers, so the refresh-and-retry path is consistent across shell / ACP / wire.
Acceptance.
- Concurrent-rotation scenarios no longer emit
auth_requiredfrom ACP or web UI when a valid rotated token is on disk - New integration tests: simulate a 401 followed by a valid refresh; verify all three entry points recover transparently
- The specific loop in #1940 (VSCode extension repeated auth prompts) no longer happens
Overall completion criteria
When all three items land:
- The race goes from "happens regularly but no longer fatal" to "does not happen"
- The rejection tombstone from #1996 becomes redundant defense-in-depth (can be removed, or kept as a backstop)
- Shell, ACP, and web UI share identical OAuth refresh behaviour
- The OAuth + static
api_keyfallback chain works uniformly across all entry points
Each item is independently shippable. Comment below to claim one.
Additional information
No response
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Choose one independently shippable item and start by reading the named entry points: OAuthManager creation sites, oauth.py, or the 401 handlers in soul/kimisoul.py, acp/session.py, and wire/server.py. Run the relevant auth tests, including tests/auth/test_oauth_cross_process.py. Done means the selected acceptance criteria pass without regressing existing authentication or fallback behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, authentication, cli
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100