From 419d33cae496a5cf77ca49b2b99cedddab7142ac Mon Sep 17 00:00:00 2001 From: ch-z-hc <45302000+ch-z-hc@users.noreply.github.com> Date: Thu, 15 Oct 2026 08:29:39 +0800 Subject: [PATCH] fix: coalesce concurrent automatic OAuth refreshes --- src/auth/credentials.ts | 7 +- src/auth/refresh.ts | 39 +++-- src/config/loader.ts | 8 +- src/config/paths.ts | 3 +- test/auth/refresh-concurrency.test.ts | 207 ++++++++++++++++++++++++++ 5 files changed, 244 insertions(+), 20 deletions(-) create mode 100644 test/auth/refresh-concurrency.test.ts diff --git a/src/auth/credentials.ts b/src/auth/credentials.ts index 02a22b51..9e253fcf 100644 --- a/src/auth/credentials.ts +++ b/src/auth/credentials.ts @@ -1,4 +1,5 @@ import { readConfigFile, writeConfigFile } from '../config/loader'; +import { getConfigPath } from '../config/paths'; import type { CredentialFile } from './types'; /** @@ -12,10 +13,10 @@ export async function loadCredentials(): Promise { return cfg.oauth ?? null; } -export async function saveCredentials(creds: CredentialFile): Promise { - const existing = readConfigFile() as Record; +export async function saveCredentials(creds: CredentialFile, path = getConfigPath()): Promise { + const existing = readConfigFile(path) as Record; existing.oauth = creds; - await writeConfigFile(existing); + await writeConfigFile(existing, undefined, path); } export async function clearCredentials(): Promise { diff --git a/src/auth/refresh.ts b/src/auth/refresh.ts index 84d2148c..a0569ff4 100644 --- a/src/auth/refresh.ts +++ b/src/auth/refresh.ts @@ -1,6 +1,8 @@ +import { resolve } from 'path'; import type { CredentialFile } from './types'; import type { Region } from '../config/schema'; import { OAUTH_HOSTS } from '../config/schema'; +import { getConfigPath } from '../config/paths'; import { saveCredentials } from './credentials'; import { CLIError } from '../errors/base'; import { ExitCode } from '../errors/codes'; @@ -10,6 +12,7 @@ const MAX_RETRIES = 2; const RETRY_DELAY_MS = 500; const REQUEST_TIMEOUT_MS = 10_000; const REFRESH_BUFFER_MS = 5 * 60 * 1000; +const pendingRefreshes = new Map>(); interface RefreshResponse { base_resp?: { status_code: number; status_msg?: string }; @@ -100,15 +103,29 @@ export async function ensureFreshToken(creds: CredentialFile): Promise { const expiresAt = new Date(creds.expires_at).getTime(); if (Date.now() < expiresAt - REFRESH_BUFFER_MS) return creds.access_token; - const fresh = await refreshAccessToken(creds.refresh_token, creds.region ?? 'global'); - const updated: CredentialFile = { - access_token: fresh.access_token, - refresh_token: fresh.refresh_token, - expires_at: fresh.expires_at, - region: creds.region, - resource_url: fresh.resource_url ?? creds.resource_url, - account: creds.account, - }; - await saveCredentials(updated); - return updated.access_token; + const path = resolve(getConfigPath()); + const key = JSON.stringify([path, creds.region ?? 'global', creds.refresh_token]); + const existing = pendingRefreshes.get(key); + if (existing) return existing; + + // Share persistence as well as the request: waiters need the rotated token on disk. + const pending = (async () => { + const fresh = await refreshAccessToken(creds.refresh_token, creds.region ?? 'global'); + const updated: CredentialFile = { + access_token: fresh.access_token, + refresh_token: fresh.refresh_token, + expires_at: fresh.expires_at, + region: creds.region, + resource_url: fresh.resource_url ?? creds.resource_url, + account: creds.account, + }; + await saveCredentials(updated, path); + return updated.access_token; + })(); + pendingRefreshes.set(key, pending); + try { + return await pending; + } finally { + pendingRefreshes.delete(key); + } } diff --git a/src/config/loader.ts b/src/config/loader.ts index af9a7a7a..5de80da0 100644 --- a/src/config/loader.ts +++ b/src/config/loader.ts @@ -1,4 +1,5 @@ import { copyFileSync, existsSync, readFileSync, renameSync, unlinkSync, writeFileSync } from 'fs'; +import { dirname } from 'path'; import { parseConfigFile, REGIONS, type Config, type ConfigFile, type Region } from './schema'; import { ensureConfigDir, getConfigPath } from './paths'; import { detectOutputFormat, type OutputFormat } from '../output/formatter'; @@ -36,8 +37,7 @@ export function renameWithCrossDeviceFallback( } } -export function readConfigFile(): ConfigFile { - const path = getConfigPath(); +export function readConfigFile(path = getConfigPath()): ConfigFile { if (!existsSync(path)) return {}; try { return parseConfigFile(JSON.parse(readFileSync(path, 'utf-8'))); @@ -53,9 +53,9 @@ export function readConfigFile(): ConfigFile { export async function writeConfigFile( data: Record, renameOps?: RenameWithFallbackOps, + path = getConfigPath(), ): Promise { - await ensureConfigDir(); - const path = getConfigPath(); + await ensureConfigDir(dirname(path)); const tmp = path + '.tmp'; writeFileSync(tmp, JSON.stringify(data, null, 2) + '\n', { mode: 0o600 }); renameWithCrossDeviceFallback(tmp, path, renameOps); diff --git a/src/config/paths.ts b/src/config/paths.ts index de3e6a14..731c33a8 100644 --- a/src/config/paths.ts +++ b/src/config/paths.ts @@ -12,8 +12,7 @@ export function getConfigPath(): string { return join(getConfigDir(), 'config.json'); } -export async function ensureConfigDir(): Promise { - const dir = getConfigDir(); +export async function ensureConfigDir(dir = getConfigDir()): Promise { const fs = await import('fs/promises'); await fs.mkdir(dir, { recursive: true, mode: 0o700 }); } diff --git a/test/auth/refresh-concurrency.test.ts b/test/auth/refresh-concurrency.test.ts new file mode 100644 index 00000000..fea096ca --- /dev/null +++ b/test/auth/refresh-concurrency.test.ts @@ -0,0 +1,207 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { mkdirSync, mkdtempSync, readFileSync, rmdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, relative } from 'node:path'; +import { loadCredentials, saveCredentials } from '../../src/auth/credentials'; +import { ensureFreshToken } from '../../src/auth/refresh'; +import type { CredentialFile } from '../../src/auth/types'; +import { readConfigFile, writeConfigFile } from '../../src/config/loader'; +import { OAUTH_HOSTS } from '../../src/config/schema'; +import { ExitCode } from '../../src/errors/codes'; +import { MiniMaxSDK } from '../../src/sdk'; +import { createMockServer, jsonResponse, type MockServer } from '../helpers/mock-server'; + +describe('concurrent automatic OAuth refresh', () => { + let directory: string; + let originalConfigDir: string | undefined; + let originalFetch: typeof fetch; + let server: MockServer; + let creds: CredentialFile; + + beforeEach(() => { + directory = mkdtempSync(join(tmpdir(), 'mmx-refresh-concurrency-')); + originalConfigDir = process.env.MMX_CONFIG_DIR; + process.env.MMX_CONFIG_DIR = directory; + originalFetch = globalThis.fetch; + creds = { + access_token: 'expired-access', + refresh_token: 'single-use-refresh', + expires_at: new Date(Date.now() - 1000).toISOString(), + region: 'global', + account: 'fixture-account', + }; + // Only the fixed OAuth host is redirected; SDK API requests use loopback. + globalThis.fetch = ((input, init) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + if (Object.values(OAUTH_HOSTS).includes(url.origin) + && url.pathname === '/oauth2/token') { + return originalFetch(`${server.url}/oauth2/token`, init); + } + if (url.origin === server.url) return originalFetch(input, init); + throw new Error(`Unexpected fixture request: ${url.origin}${url.pathname}`); + }) as typeof fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + server?.close(); + if (originalConfigDir === undefined) delete process.env.MMX_CONFIG_DIR; + else process.env.MMX_CONFIG_DIR = originalConfigDir; + rmSync(directory, { recursive: true, force: true }); + }); + + it('shares one rotating refresh and persists it before concurrent SDK requests continue', async () => { + let refreshes = 0; + const apiTokens: string[] = []; + server = createMockServer({ + routes: { + '/oauth2/token': async req => { + const form = new URLSearchParams(await req.text()); + expect(form.get('refresh_token')).toBe(creds.refresh_token); + refreshes++; + if (refreshes > 1) return jsonResponse({ error: 'invalid_grant' }, 400); + return jsonResponse({ + status: 'success', access_token: 'fresh-access', refresh_token: 'rotated-refresh', + expired_in: Date.now() + 3600000, resource_url: server.url, + }); + }, + '/v1/': async req => { + apiTokens.push(req.headers.get('Authorization') ?? ''); + // Every waiter must resume only after the rotated credentials are saved. + expect((await loadCredentials())?.refresh_token).toBe('rotated-refresh'); + return jsonResponse({ base_resp: { status_code: 0 }, system_voice: [], data: { image_urls: [] } }); + }, + }, + }); + await writeConfigFile({ oauth: creds, default_text_model: 'preserved-model' }); + const first = new MiniMaxSDK({ region: 'global', baseUrl: server.url }); + const second = new MiniMaxSDK({ region: 'global', baseUrl: server.url }); + const results = await Promise.allSettled([ + first.speech.voices(), first.image.generate({ prompt: 'Fixture' }), + second.speech.voices(), second.image.generate({ prompt: 'Fixture' }), + ]); + expect(results.map(result => result.status)).toEqual(Array(4).fill('fulfilled')); + expect(refreshes).toBe(1); + expect(apiTokens).toEqual(Array(4).fill('Bearer fresh-access')); + expect(await loadCredentials()).toMatchObject({ + access_token: 'fresh-access', refresh_token: 'rotated-refresh', + account: creds.account, region: 'global', resource_url: server.url, + }); + expect(readConfigFile().default_text_model).toBe('preserved-model'); + await first.speech.voices(); + expect(refreshes).toBe(1); + }); + + it('shares a failed refresh without changing credentials and allows a later attempt', async () => { + let refreshes = 0; + let fail = true; + server = createMockServer({ + routes: { + '/oauth2/token': () => { + refreshes++; + return fail ? jsonResponse({ error: 'invalid_grant' }, 400) : jsonResponse({ + status: 'success', access_token: 'recovered-access', expired_in: Date.now() + 3600000, + }); + }, + }, + }); + await saveCredentials(creds); + const results = await Promise.allSettled(Array.from({ length: 4 }, () => ensureFreshToken({ ...creds }))); + expect(results.every(result => result.status === 'rejected')).toBe(true); + for (const result of results) { + if (result.status === 'rejected') expect(result.reason).toMatchObject({ exitCode: ExitCode.AUTH }); + } + expect(refreshes).toBe(1); + expect(await loadCredentials()).toEqual(creds); + fail = false; + expect(await ensureFreshToken(creds)).toBe('recovered-access'); + expect(refreshes).toBe(2); + // A completed refresh must not become a permanent cached result. + expect(await ensureFreshToken(creds)).toBe('recovered-access'); + expect(refreshes).toBe(3); + }); + + it.each(['region', 'refresh_token'] as const)('does not share refreshes with a different %s', async field => { + let refreshes = 0; + server = createMockServer({ + routes: { '/oauth2/token': () => { + refreshes++; + return jsonResponse({ error: 'invalid_grant' }, 400); + } }, + }); + const other = { ...creds, [field]: field === 'region' ? 'cn' as const : 'different-refresh' }; + const results = await Promise.allSettled([ensureFreshToken(creds), ensureFreshToken(other)]); + expect(results.every(result => result.status === 'rejected')).toBe(true); + expect(refreshes).toBe(2); + }); + + it('shares refreshes when absolute and relative paths select the same store', async () => { + let refreshes = 0; + server = createMockServer({ + routes: { '/oauth2/token': () => { + refreshes++; + return refreshes > 1 ? jsonResponse({ error: 'invalid_grant' }, 400) : jsonResponse({ + status: 'success', access_token: 'fresh-access', expired_in: Date.now() + 3600000, + }); + } }, + }); + await saveCredentials(creds); + const first = ensureFreshToken(creds); + process.env.MMX_CONFIG_DIR = relative(process.cwd(), directory); + const second = ensureFreshToken(creds); + expect(await Promise.all([first, second])).toEqual(['fresh-access', 'fresh-access']); + expect(refreshes).toBe(1); + expect((await loadCredentials())?.access_token).toBe('fresh-access'); + }); + + it('does not share refreshes between different credential stores', async () => { + let refreshes = 0; + server = createMockServer({ + routes: { '/oauth2/token': () => { + refreshes++; + return jsonResponse({ + status: 'success', access_token: `access-${refreshes}`, refresh_token: `refresh-${refreshes}`, + expired_in: Date.now() + 3600000, + }); + } }, + }); + const otherDirectory = join(directory, 'other-store'); + const otherCreds = { ...creds, account: 'other-account' }; + await writeConfigFile({ oauth: creds, default_text_model: 'first-model' }); + process.env.MMX_CONFIG_DIR = otherDirectory; + await writeConfigFile({ oauth: otherCreds, default_text_model: 'second-model' }); + process.env.MMX_CONFIG_DIR = directory; + const first = ensureFreshToken(creds); + process.env.MMX_CONFIG_DIR = otherDirectory; + const second = ensureFreshToken(otherCreds); + const [firstToken, secondToken] = await Promise.all([first, second]); + expect(refreshes).toBe(2); + expect(firstToken).not.toBe(secondToken); + expect(JSON.parse(readFileSync(join(directory, 'config.json'), 'utf8'))).toMatchObject({ + oauth: { access_token: firstToken, account: creds.account }, default_text_model: 'first-model', + }); + expect(JSON.parse(readFileSync(join(otherDirectory, 'config.json'), 'utf8'))).toMatchObject({ + oauth: { access_token: secondToken, account: otherCreds.account }, default_text_model: 'second-model', + }); + }); + + it('propagates persistence failures to every waiter and clears the pending refresh', async () => { + let refreshes = 0; + server = createMockServer({ + routes: { '/oauth2/token': () => { + refreshes++; + return jsonResponse({ status: 'success', access_token: 'fresh-access', expired_in: Date.now() + 3600000 }); + } }, + }); + await saveCredentials(creds); + const blockedPath = join(directory, 'config.json.tmp'); + mkdirSync(blockedPath); + const results = await Promise.allSettled(Array.from({ length: 4 }, () => ensureFreshToken({ ...creds }))); + expect(results.every(result => result.status === 'rejected')).toBe(true); + expect(refreshes).toBe(1); + expect(await loadCredentials()).toEqual(creds); + rmdirSync(blockedPath); + expect(await ensureFreshToken(creds)).toBe('fresh-access'); + expect(refreshes).toBe(2); + }); +});