Skip to content

Commit 6af711f

Browse files
ndycodendycode
committed
fix refresh persistence review followups
1 parent 9f2a9ad commit 6af711f

6 files changed

Lines changed: 267 additions & 79 deletions

File tree

‎lib/accounts.ts‎

Lines changed: 88 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {
1919
type HybridSelectionOptions,
2020
} from "./rotation.js";
2121
import { nowMs } from "./utils.js";
22-
import { ERROR_MESSAGES } from "./constants.js";
22+
import { ERROR_MESSAGES, HTTP_STATUS } from "./constants.js";
2323
import { CodexAuthError } from "./errors.js";
2424
import {
2525
loadCodexCliState,
@@ -159,6 +159,37 @@ function findAccountIndexByIdentity<
159159
return undefined;
160160
}
161161

162+
const RETRYABLE_AUTH_PERSISTENCE_CODES = new Set(["EAGAIN", "EBUSY", "EPERM"]);
163+
164+
function isRetryableAuthPersistenceError(error: unknown): boolean {
165+
if (!error || typeof error !== "object") {
166+
return false;
167+
}
168+
169+
const candidate = error as {
170+
code?: unknown;
171+
status?: unknown;
172+
cause?: unknown;
173+
};
174+
const code =
175+
typeof candidate.code === "string"
176+
? candidate.code.toUpperCase()
177+
: undefined;
178+
if (code && RETRYABLE_AUTH_PERSISTENCE_CODES.has(code)) {
179+
return true;
180+
}
181+
182+
if (candidate.status === HTTP_STATUS.TOO_MANY_REQUESTS) {
183+
return true;
184+
}
185+
186+
if (candidate.cause && candidate.cause !== error) {
187+
return isRetryableAuthPersistenceError(candidate.cause);
188+
}
189+
190+
return false;
191+
}
192+
162193
export interface Workspace {
163194
id: string;
164195
name?: string;
@@ -845,25 +876,68 @@ export class AccountManager {
845876
delete storedAccount.coolingDownUntil;
846877
delete storedAccount.cooldownReason;
847878

848-
await persist(nextStorage);
849-
850879
const liveAccount = this.getAccountByIdentity(source, auth);
851-
if (!liveAccount) {
852-
log.warn("Unable to resolve refreshed live account after persistence", {
853-
sourceIndex: source.index,
854-
});
855-
return null;
880+
if (liveAccount) {
881+
const previousLiveAccountState = {
882+
access: liveAccount.access,
883+
refreshToken: liveAccount.refreshToken,
884+
expires: liveAccount.expires,
885+
accountId: liveAccount.accountId,
886+
accountIdSource: liveAccount.accountIdSource,
887+
email: liveAccount.email,
888+
enabled: liveAccount.enabled,
889+
coolingDownUntil: liveAccount.coolingDownUntil,
890+
cooldownReason: liveAccount.cooldownReason,
891+
consecutiveAuthFailures: liveAccount.consecutiveAuthFailures,
892+
};
893+
894+
this.updateFromAuth(liveAccount, auth);
895+
liveAccount.enabled = true;
896+
this.clearAccountCooldown(liveAccount);
897+
this.clearAuthFailures(liveAccount);
898+
899+
try {
900+
await persist(nextStorage);
901+
} catch (error) {
902+
liveAccount.access = previousLiveAccountState.access;
903+
liveAccount.refreshToken = previousLiveAccountState.refreshToken;
904+
liveAccount.expires = previousLiveAccountState.expires;
905+
liveAccount.accountId = previousLiveAccountState.accountId;
906+
liveAccount.accountIdSource =
907+
previousLiveAccountState.accountIdSource;
908+
liveAccount.email = previousLiveAccountState.email;
909+
liveAccount.enabled = previousLiveAccountState.enabled;
910+
liveAccount.consecutiveAuthFailures =
911+
previousLiveAccountState.consecutiveAuthFailures;
912+
if (
913+
previousLiveAccountState.coolingDownUntil === undefined
914+
) {
915+
delete liveAccount.coolingDownUntil;
916+
} else {
917+
liveAccount.coolingDownUntil =
918+
previousLiveAccountState.coolingDownUntil;
919+
}
920+
if (previousLiveAccountState.cooldownReason === undefined) {
921+
delete liveAccount.cooldownReason;
922+
} else {
923+
liveAccount.cooldownReason =
924+
previousLiveAccountState.cooldownReason;
925+
}
926+
throw error;
927+
}
928+
929+
return liveAccount;
856930
}
857931

858-
this.updateFromAuth(liveAccount, auth);
859-
liveAccount.enabled = true;
860-
this.clearAccountCooldown(liveAccount);
861-
this.clearAuthFailures(liveAccount);
862-
return liveAccount;
932+
await persist(nextStorage);
933+
log.warn("Unable to resolve refreshed live account after persistence", {
934+
sourceIndex: source.index,
935+
});
936+
return null;
863937
});
864938
} catch (error) {
865939
throw new CodexAuthError(ERROR_MESSAGES.TOKEN_REFRESH_FAILED, {
866-
retryable: true,
940+
retryable: isRetryableAuthPersistenceError(error),
867941
cause: error,
868942
});
869943
}

‎lib/refresh-guardian.ts‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,6 @@ export class RefreshGuardian {
205205
}
206206

207207
let requiresSave = false;
208-
const processed = new Set<number>();
209208
const refreshResults = await refreshExpiringAccounts(
210209
eligibleSnapshot,
211210
this.bufferMs,
@@ -218,7 +217,6 @@ export class RefreshGuardian {
218217
if (saveNeeded) {
219218
requiresSave = true;
220219
}
221-
processed.add(sourceAccount.index);
222220
},
223221
);
224222
if (refreshResults.size === 0) {
@@ -227,27 +225,6 @@ export class RefreshGuardian {
227225
return;
228226
}
229227

230-
const snapshotByIndex = new Map<number, (typeof snapshot)[number]>();
231-
for (const candidate of eligibleSnapshot) {
232-
snapshotByIndex.set(candidate.index, candidate);
233-
}
234-
235-
for (const [accountIndex, result] of refreshResults.entries()) {
236-
if (processed.has(accountIndex)) {
237-
continue;
238-
}
239-
const sourceAccount = snapshotByIndex.get(accountIndex);
240-
if (!sourceAccount) continue;
241-
const saveNeeded = await this.applyRefreshOutcome(
242-
manager,
243-
sourceAccount,
244-
result,
245-
);
246-
if (saveNeeded) {
247-
requiresSave = true;
248-
}
249-
}
250-
251228
if (requiresSave) {
252229
manager.saveToDiskDebounced();
253230
}

‎lib/request/fetch-helpers.ts‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,35 @@ function isRetryableRefreshFailure(
449449
}
450450
}
451451

452+
function isRetryableAuthSetterError(error: unknown): boolean {
453+
if (!error || typeof error !== "object") {
454+
return false;
455+
}
456+
457+
const candidate = error as {
458+
code?: unknown;
459+
status?: unknown;
460+
cause?: unknown;
461+
};
462+
const code =
463+
typeof candidate.code === "string"
464+
? candidate.code.toUpperCase()
465+
: undefined;
466+
if (code === "EAGAIN" || code === "EBUSY" || code === "EPERM") {
467+
return true;
468+
}
469+
470+
if (candidate.status === HTTP_STATUS.TOO_MANY_REQUESTS) {
471+
return true;
472+
}
473+
474+
if (candidate.cause && candidate.cause !== error) {
475+
return isRetryableAuthSetterError(candidate.cause);
476+
}
477+
478+
return false;
479+
}
480+
452481
/**
453482
* Refreshes the OAuth token and updates stored credentials
454483
* @param currentAuth - Current auth state
@@ -492,7 +521,7 @@ export async function refreshAndUpdateToken(
492521
});
493522
} catch (error) {
494523
throw new CodexAuthError(ERROR_MESSAGES.TOKEN_REFRESH_FAILED, {
495-
retryable: true,
524+
retryable: isRetryableAuthSetterError(error),
496525
cause: error,
497526
});
498527
}

‎test/accounts.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,6 +1270,47 @@ describe("AccountManager", () => {
12701270
expect(account.refreshToken).toBe("old-refresh");
12711271
});
12721272

1273+
it("propagates non-transient storage write failure as terminal CodexAuthError", async () => {
1274+
const { withAccountStorageTransaction } = await import("../lib/storage.js");
1275+
const mockWithAccountStorageTransaction = vi.mocked(
1276+
withAccountStorageTransaction,
1277+
);
1278+
mockWithAccountStorageTransaction.mockRejectedValueOnce(
1279+
Object.assign(new Error("EACCES"), { code: "EACCES" }),
1280+
);
1281+
1282+
const now = Date.now();
1283+
const stored = {
1284+
version: 3 as const,
1285+
activeIndex: 0,
1286+
accounts: [
1287+
{
1288+
refreshToken: "old-refresh",
1289+
accessToken: "old-access",
1290+
expiresAt: now,
1291+
addedAt: now,
1292+
lastUsed: now,
1293+
},
1294+
],
1295+
};
1296+
const manager = new AccountManager(undefined, stored as any);
1297+
const account = manager.getAccountByIndex(0)!;
1298+
const refreshedAuth: OAuthAuthDetails = {
1299+
type: "oauth",
1300+
access: "header.payload.signature",
1301+
refresh: "new-refresh",
1302+
expires: now + 3_600_000,
1303+
};
1304+
1305+
const error = await manager.commitRefreshedAuth(account, refreshedAuth).catch(
1306+
(err) => err as CodexAuthError,
1307+
);
1308+
1309+
expect(error).toBeInstanceOf(CodexAuthError);
1310+
expect(error.retryable).toBe(false);
1311+
expect(account.refreshToken).toBe("old-refresh");
1312+
});
1313+
12731314
it("prevents debounced saves from writing stale auth during refresh persistence", async () => {
12741315
vi.useFakeTimers();
12751316
try {

‎test/fetch-helpers.test.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,9 @@ describe('Fetch Helpers Module', () => {
183183
const auth: Auth = { type: 'oauth', access: 'old', refresh: 'oldr', expires: 0 };
184184
const client = {
185185
auth: {
186-
set: vi.fn().mockRejectedValue(new Error('persist failed')),
186+
set: vi.fn().mockRejectedValue(
187+
Object.assign(new Error('persist failed'), { code: 'EBUSY' }),
188+
),
187189
},
188190
} as any;
189191
vi.spyOn(refreshQueueModule, 'queuedRefresh').mockResolvedValue({
@@ -199,6 +201,29 @@ describe('Fetch Helpers Module', () => {
199201
expect(error).toBeInstanceOf(CodexAuthError);
200202
expect(error.retryable).toBe(true);
201203
});
204+
205+
it('throws terminal auth errors when auth persistence fails permanently', async () => {
206+
const auth: Auth = { type: 'oauth', access: 'old', refresh: 'oldr', expires: 0 };
207+
const client = {
208+
auth: {
209+
set: vi.fn().mockRejectedValue(
210+
Object.assign(new Error('persist failed'), { code: 'EACCES' }),
211+
),
212+
},
213+
} as any;
214+
vi.spyOn(refreshQueueModule, 'queuedRefresh').mockResolvedValue({
215+
type: 'success',
216+
access: 'new',
217+
refresh: 'newr',
218+
expires: 123,
219+
} as any);
220+
221+
const error = await refreshAndUpdateToken(auth, client).catch(
222+
(err) => err as CodexAuthError,
223+
);
224+
expect(error).toBeInstanceOf(CodexAuthError);
225+
expect(error.retryable).toBe(false);
226+
});
202227
});
203228

204229
describe('extractRequestUrl', () => {

0 commit comments

Comments
 (0)