MoonshotAI / MoonshotAI/kimi-cli

tracking: finish the OAuth race-prevention architecture (follow-up to #1996)

Open
#2,006 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
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_token lives only 15 min
  • PR #1658 — unintentionally introduced the "new OAuthManager per 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:199
  • src/kimi_cli/web/api/sessions.py:817 (per session-title-gen — worst offender)
  • src/kimi_cli/cli/plugin.py:212
  • src/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 OAuthManager instance 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):

  1. Fail closed — on acquire failure, return without attempting refresh. Next ensure_fresh cycle (60 s later) retries.
  2. Block longer — extend total retry budget to ~30 s with exponential backoff. Most refreshes complete in < 5 s, so 30 s absorbs realistic contention.
  3. Hybrid — after timeout, re-read the file; if refresh_token has changed, treat the operation as completed by a peer. Only fall back to unlocked refresh when disk is unchanged and force=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.py still passes
  • New test: 3+ processes racing to refresh result in exactly one successful rotation
  • FS environments without flock fail 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_required from 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:

  1. The race goes from "happens regularly but no longer fatal" to "does not happen"
  2. The rejection tombstone from #1996 becomes redundant defense-in-depth (can be removed, or kept as a backstop)
  3. Shell, ACP, and web UI share identical OAuth refresh behaviour
  4. The OAuth + static api_key fallback chain works uniformly across all entry points

Each item is independently shippable. Comment below to claim one.

Additional information

No response

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.