fix(codex-accounts): stop account mutations blocking on the quota refresh (#10225)
Switching, adding, re-authing, or removing a Codex account awaited refreshForCodexAccountChange before resolving the IPC call. Since the per-account CODEX_HOME rollout (#9501) that probe runs against a cold home (10s RPC + 15s PTY fallback, 25s WSL) and can queue behind an in-flight global usage fetch, so the switcher sat unresponsive for tens of seconds and a fresh login looked stuck on the loading screen. Worse, in addAccount the awaited refresh sat inside the login cleanup try/catch: a refresh rejection after the account was durably committed deleted the just-created managed home, leaving a registered account with no home ("account never connects"). Run the refresh as best-effort background work instead. Its synchronous prefix still flips usage to "fetching" before the first await, so the switcher updates instantly and usage fills in via the normal rate-limit state pushes; a probe failure is logged and can never trigger managed-home cleanup. Fixes #10141
This commit is contained in:
parent
94d3db4a24
commit
babf1ff9eb
|
|
@ -2586,4 +2586,188 @@ describe('CodexAccountService config sync', () => {
|
|||
expect(state.systemDefault?.email).toBe('real@home.dev')
|
||||
})
|
||||
})
|
||||
|
||||
// Why: quota probes against a cold per-account CODEX_HOME can take 10–25s
|
||||
// (RPC + PTY fallback) and queue behind an in-flight global usage refresh;
|
||||
// account mutations must never block on — or fail because of — that probe.
|
||||
describe('quota refresh decoupling', () => {
|
||||
function createAccountOneSettings(): GlobalSettings {
|
||||
const managedHomePath = createManagedHome(
|
||||
testState.userDataDir,
|
||||
'account-1',
|
||||
'',
|
||||
'{"account":"managed"}\n'
|
||||
)
|
||||
return createSettings({
|
||||
codexManagedAccounts: [
|
||||
{
|
||||
id: 'account-1',
|
||||
email: 'user@example.com',
|
||||
managedHomePath,
|
||||
providerAccountId: null,
|
||||
workspaceLabel: null,
|
||||
workspaceAccountId: null,
|
||||
createdAt: 1,
|
||||
updatedAt: 1,
|
||||
lastAuthenticatedAt: 1
|
||||
}
|
||||
]
|
||||
})
|
||||
}
|
||||
|
||||
async function expectResolvesPromptly<T>(promise: Promise<T>, label: string): Promise<T> {
|
||||
let timer: NodeJS.Timeout | undefined
|
||||
try {
|
||||
return await Promise.race([
|
||||
promise,
|
||||
new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(
|
||||
() => reject(new Error(`${label} blocked on the quota refresh`)),
|
||||
2_000
|
||||
)
|
||||
})
|
||||
])
|
||||
} finally {
|
||||
clearTimeout(timer)
|
||||
}
|
||||
}
|
||||
|
||||
function createLoginSpawnMock() {
|
||||
return vi.fn((_command: string, _args: string[], options: { env: NodeJS.ProcessEnv }) => {
|
||||
const child = new EventEmitter() as EventEmitter & {
|
||||
stdout: PassThrough
|
||||
stderr: PassThrough
|
||||
kill: () => void
|
||||
}
|
||||
child.stdout = new PassThrough()
|
||||
child.stderr = new PassThrough()
|
||||
child.kill = vi.fn()
|
||||
writeFileSync(
|
||||
join(options.env.CODEX_HOME!, 'auth.json'),
|
||||
createCodexAuthJson('user@example.com', 'provider-account-1', 'refresh-token'),
|
||||
'utf-8'
|
||||
)
|
||||
queueMicrotask(() => child.emit('close', 0))
|
||||
return child
|
||||
})
|
||||
}
|
||||
|
||||
it('resolves selectAccount while the quota refresh never settles', async () => {
|
||||
const store = createStore(createAccountOneSettings())
|
||||
const rateLimits = {
|
||||
refreshForCodexAccountChange: vi.fn(() => new Promise<never>(() => {})),
|
||||
evictInactiveCodexCache: vi.fn()
|
||||
}
|
||||
const runtimeHome = createRuntimeHome()
|
||||
|
||||
const { CodexAccountService } = await import('./service')
|
||||
const service = new CodexAccountService(
|
||||
store as never,
|
||||
rateLimits as never,
|
||||
runtimeHome as never
|
||||
)
|
||||
|
||||
const state = await expectResolvesPromptly(
|
||||
service.selectAccount('account-1'),
|
||||
'selectAccount'
|
||||
)
|
||||
|
||||
expect(state.activeAccountId).toBe('account-1')
|
||||
expect(rateLimits.refreshForCodexAccountChange).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('resolves selectAccount when the quota refresh rejects', async () => {
|
||||
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const store = createStore(createAccountOneSettings())
|
||||
const rateLimits = {
|
||||
refreshForCodexAccountChange: vi.fn().mockRejectedValue(new Error('cold probe failed')),
|
||||
evictInactiveCodexCache: vi.fn()
|
||||
}
|
||||
const runtimeHome = createRuntimeHome()
|
||||
|
||||
const { CodexAccountService } = await import('./service')
|
||||
const service = new CodexAccountService(
|
||||
store as never,
|
||||
rateLimits as never,
|
||||
runtimeHome as never
|
||||
)
|
||||
|
||||
const state = await service.selectAccount('account-1')
|
||||
|
||||
expect(state.activeAccountId).toBe('account-1')
|
||||
await vi.waitFor(() => expect(errorSpy).toHaveBeenCalled())
|
||||
errorSpy.mockRestore()
|
||||
})
|
||||
|
||||
it('resolves addAccount while the post-login quota refresh never settles', async () => {
|
||||
vi.resetModules()
|
||||
writeFileSync(
|
||||
join(testState.fakeHomeDir, '.codex', 'config.toml'),
|
||||
'approval_policy = "never"\n',
|
||||
'utf-8'
|
||||
)
|
||||
const spawnMock = createLoginSpawnMock()
|
||||
vi.doMock('node:child_process', () => ({ execFileSync: vi.fn(), spawn: spawnMock }))
|
||||
vi.doMock('../codex-cli/command', () => ({ resolveCodexCommand: () => 'codex' }))
|
||||
|
||||
const store = createStore(createSettings())
|
||||
const rateLimits = {
|
||||
refreshForCodexAccountChange: vi.fn(() => new Promise<never>(() => {})),
|
||||
evictInactiveCodexCache: vi.fn()
|
||||
}
|
||||
const runtimeHome = createRuntimeHome()
|
||||
|
||||
const { CodexAccountService } = await import('./service')
|
||||
const service = new CodexAccountService(
|
||||
store as never,
|
||||
rateLimits as never,
|
||||
runtimeHome as never
|
||||
)
|
||||
|
||||
const state = await expectResolvesPromptly(service.addAccount(), 'addAccount')
|
||||
|
||||
expect(state.accounts).toHaveLength(1)
|
||||
expect(state.accounts[0].email).toBe('user@example.com')
|
||||
expect(rateLimits.refreshForCodexAccountChange).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('keeps the new account and its managed home when the post-login quota refresh rejects', async () => {
|
||||
vi.resetModules()
|
||||
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
writeFileSync(
|
||||
join(testState.fakeHomeDir, '.codex', 'config.toml'),
|
||||
'approval_policy = "never"\n',
|
||||
'utf-8'
|
||||
)
|
||||
const spawnMock = createLoginSpawnMock()
|
||||
vi.doMock('node:child_process', () => ({ execFileSync: vi.fn(), spawn: spawnMock }))
|
||||
vi.doMock('../codex-cli/command', () => ({ resolveCodexCommand: () => 'codex' }))
|
||||
|
||||
const store = createStore(createSettings())
|
||||
const rateLimits = {
|
||||
refreshForCodexAccountChange: vi.fn().mockRejectedValue(new Error('cold probe failed')),
|
||||
evictInactiveCodexCache: vi.fn()
|
||||
}
|
||||
const runtimeHome = createRuntimeHome()
|
||||
|
||||
const { CodexAccountService } = await import('./service')
|
||||
const service = new CodexAccountService(
|
||||
store as never,
|
||||
rateLimits as never,
|
||||
runtimeHome as never
|
||||
)
|
||||
|
||||
const state = await service.addAccount()
|
||||
|
||||
expect(state.accounts).toHaveLength(1)
|
||||
const account = store.getSettings().codexManagedAccounts[0]
|
||||
expect(account.email).toBe('user@example.com')
|
||||
// The durable mutation must survive a failed usage probe — previously the
|
||||
// rejection fell into login cleanup and deleted the just-created home.
|
||||
expect(existsSync(account.managedHomePath)).toBe(true)
|
||||
expect(existsSync(join(account.managedHomePath, 'auth.json'))).toBe(true)
|
||||
await vi.waitFor(() => expect(errorSpy).toHaveBeenCalled())
|
||||
errorSpy.mockRestore()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -204,6 +204,20 @@ export class CodexAccountService {
|
|||
return this.serializeMutation(() => this.doSelectAccount(accountId, target))
|
||||
}
|
||||
|
||||
// Why: quota probes against a cold per-account CODEX_HOME can take 10–25s
|
||||
// (RPC + PTY fallback) and queue behind an in-flight global usage refresh.
|
||||
// The refresh synchronously flips usage to "fetching" before its first await,
|
||||
// so the switcher updates immediately; the probe itself must never block or
|
||||
// fail the already-durable account mutation.
|
||||
private startQuotaRefreshInBackground(
|
||||
outgoingAccountId: string | null | undefined,
|
||||
target: CodexAccountSelectionTarget | undefined
|
||||
): void {
|
||||
void this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, target).catch((error) => {
|
||||
console.error('[codex-accounts] Quota refresh after account change failed:', error)
|
||||
})
|
||||
}
|
||||
|
||||
private async doAddAccount(target?: CodexAccountAddTarget): Promise<CodexRateLimitAccountsState> {
|
||||
const accountId = randomUUID()
|
||||
const managedHome = this.createManagedHome(accountId, target)
|
||||
|
|
@ -254,7 +268,7 @@ export class CodexAccountService {
|
|||
|
||||
// Why: switching activates the new account, so cache the outgoing account's usage for the switcher.
|
||||
const outgoingAccountId = getSelectedCodexAccountIdForTarget(settings, targetSelection)
|
||||
await this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, targetSelection)
|
||||
this.startQuotaRefreshInBackground(outgoingAccountId, targetSelection)
|
||||
return this.getSnapshot()
|
||||
} catch (error) {
|
||||
this.safeRemoveManagedHome(managedHomePath, accountId)
|
||||
|
|
@ -310,7 +324,7 @@ export class CodexAccountService {
|
|||
this.runtimeHome.syncForCurrentSelection(accountTarget)
|
||||
|
||||
// Why: re-auth can change the underlying Codex identity, so force a fresh read to avoid showing stale quota.
|
||||
await this.rateLimits.refreshForCodexAccountChange(undefined, accountTarget)
|
||||
this.startQuotaRefreshInBackground(undefined, accountTarget)
|
||||
return this.getSnapshot()
|
||||
}
|
||||
|
||||
|
|
@ -339,7 +353,7 @@ export class CodexAccountService {
|
|||
// Why: a removed account can no longer appear in the switcher dropdown,
|
||||
// so purge its cached usage to avoid stale entries.
|
||||
this.rateLimits.evictInactiveCodexCache(accountId)
|
||||
await this.rateLimits.refreshForCodexAccountChange(
|
||||
this.startQuotaRefreshInBackground(
|
||||
getSelectedCodexAccountIdForTarget(settings, getCodexSelectionTargetForAccount(account)) ===
|
||||
accountId
|
||||
? accountId
|
||||
|
|
@ -388,7 +402,7 @@ export class CodexAccountService {
|
|||
this.lifecycle.onHostSystemDefaultSelected?.()
|
||||
}
|
||||
|
||||
await this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, effectiveTarget)
|
||||
this.startQuotaRefreshInBackground(outgoingAccountId, effectiveTarget)
|
||||
return this.getSnapshot()
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue