Skip to content

Commit 55de5e8

Browse files
committed
fix: address remaining v1.2.4 review follow-ups
1 parent fb63443 commit 55de5e8

6 files changed

Lines changed: 316 additions & 10 deletions

File tree

docs/releases/v1.2.4.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ This patch release follows the already-published `v1.2.3` rebuild and lands the
2525
- `npm run typecheck`
2626
- `npm run build`
2727
- `npm test`
28-
- Full suite passed: `222/222` files, `3303/3303` tests
28+
- Full suite passed: `222/222` files, `3307/3307` tests
2929

3030
## Related
3131

lib/unified-settings.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,24 +98,34 @@ async function readSettingsRecordAsyncFromPath(
9898
* Best-effort backup reader for sync callers.
9999
*
100100
* Backup corruption is treated as an unavailable backup so callers can keep
101-
* their legacy null-on-unavailable behavior.
101+
* their legacy null-on-unavailable behavior, but unreadable or locked backups
102+
* still surface so writers do not rebuild from `{}` over a transient failure.
102103
*/
103104
function readSettingsBackupSync(): JsonRecord | null {
104105
try {
105106
return readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
106-
} catch {
107-
return null;
107+
} catch (error) {
108+
if (isInvalidSettingsRecordError(error)) {
109+
return null;
110+
}
111+
throw error;
108112
}
109113
}
110114

111115
/**
112116
* Best-effort backup reader for async callers.
117+
*
118+
* Like the sync variant, only corrupt backups are collapsed to `null`.
119+
* Unreadable or locked backups are rethrown so callers can fail closed.
113120
*/
114121
async function readSettingsBackupAsync(): Promise<JsonRecord | null> {
115122
try {
116123
return await readSettingsRecordAsyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
117-
} catch {
118-
return null;
124+
} catch (error) {
125+
if (isInvalidSettingsRecordError(error)) {
126+
return null;
127+
}
128+
throw error;
119129
}
120130
}
121131

scripts/codex.js

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,9 +233,25 @@ function maybeThrowSimulatedShadowHomeBusyError() {
233233
}
234234
}
235235

236-
function renameFileWithRetry(sourcePath, destinationPath) {
236+
function ensureShadowHomeDestinationMatchesSnapshot(destinationPath, expectedState) {
237+
if (!expectedState) {
238+
return;
239+
}
240+
const currentState = captureShadowHomeState(destinationPath);
241+
if (!shadowHomeStateMatches(currentState, expectedState)) {
242+
const error = new Error("shadow-home destination changed during sync-back retry");
243+
error.code = "EEXIST";
244+
throw error;
245+
}
246+
}
247+
248+
function renameFileWithRetry(sourcePath, destinationPath, expectedDestinationState) {
237249
for (let attempt = 0; attempt <= SHADOW_HOME_CLEANUP_BACKOFF_MS.length; attempt += 1) {
238250
try {
251+
ensureShadowHomeDestinationMatchesSnapshot(
252+
destinationPath,
253+
expectedDestinationState,
254+
);
239255
maybeThrowSimulatedShadowHomeBusyError();
240256
renameSync(sourcePath, destinationPath);
241257
return;
@@ -532,15 +548,19 @@ function shadowHomeStateMatches(left, right) {
532548
);
533549
}
534550

535-
function syncShadowHomeStateFile(sourcePath, destinationPath) {
551+
function syncShadowHomeStateFile(
552+
sourcePath,
553+
destinationPath,
554+
expectedDestinationState,
555+
) {
536556
const tempPath = join(
537557
dirname(destinationPath),
538558
`.${basename(destinationPath)}.codex-multi-auth-sync-${process.pid}.tmp`,
539559
);
540560
try {
541561
mkdirSync(dirname(destinationPath), { recursive: true });
542562
copyFileSync(sourcePath, tempPath);
543-
renameFileWithRetry(tempPath, destinationPath);
563+
renameFileWithRetry(tempPath, destinationPath, expectedDestinationState);
544564
} catch (error) {
545565
try {
546566
rmSync(tempPath, { force: true });
@@ -662,7 +682,7 @@ function createCompatibilityCodexHome(
662682
if (shadowHomeStateMatches(shadowState, originalSnapshot)) {
663683
continue;
664684
}
665-
syncShadowHomeStateFile(shadowPath, originalPath);
685+
syncShadowHomeStateFile(shadowPath, originalPath, originalSnapshot);
666686
tightenShadowHomePermissions(originalPath);
667687
} catch {
668688
// Best-effort only; runtime auth refreshes should not fail cleanup.

test/codex-bin-wrapper.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,70 @@ describe("codex bin wrapper", () => {
536536
expect(readFileSync(join(originalHome, ".codex-global-state.json"), "utf8").trim()).toBe('{"last":"shadow"}');
537537
});
538538

539+
it("does not clobber sync-back files that change during rename retry backoff", () => {
540+
const fixtureRoot = createWrapperFixture();
541+
const fakeBin = createCustomFakeCodexBin(fixtureRoot, [
542+
"#!/usr/bin/env node",
543+
'const { spawn } = require("node:child_process");',
544+
'const fs = require("node:fs");',
545+
'const path = require("node:path");',
546+
'const home = process.env.CODEX_HOME ?? "";',
547+
'const originalHome = process.env.CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME ?? "";',
548+
'fs.writeFileSync(path.join(home, "auth.json"), \'{"token":"shadow"}\\n\', "utf8");',
549+
'fs.writeFileSync(path.join(home, "accounts.json"), \'{"accounts":["shadow"]}\\n\', "utf8");',
550+
'fs.writeFileSync(path.join(home, ".codex-global-state.json"), \'{"last":"shadow"}\\n\', "utf8");',
551+
"if (originalHome) {",
552+
" const mutateScript = [",
553+
' \'const fs = require("node:fs");\',',
554+
' \'const path = require("node:path");\',',
555+
' \'const target = process.argv[1];\',',
556+
' \'setTimeout(() => {\',',
557+
' \' fs.writeFileSync(path.join(target, \"accounts.json\"), \"{\\\\\"accounts\\\\\":[\\\\\"external-during-retry\\\\\"]}\\\\n\", \"utf8\");\',',
558+
' \' fs.writeFileSync(path.join(target, \".codex-global-state.json\"), \"{\\\\\"last\\\\\":\\\\\"external-during-retry\\\\\"}\\\\n\", \"utf8\");\',',
559+
' \' process.exit(0);\',',
560+
' \'}, 40);\',',
561+
" ].join(\"\\n\");",
562+
" const mutator = spawn(process.execPath, [\"-e\", mutateScript, originalHome], {",
563+
" detached: true,",
564+
' stdio: "ignore",',
565+
" });",
566+
" mutator.unref();",
567+
"}",
568+
"process.exit(0);",
569+
]);
570+
const originalHome = join(fixtureRoot, "codex-home");
571+
const controlledTmp = join(fixtureRoot, "tmp");
572+
mkdirSync(originalHome, { recursive: true });
573+
mkdirSync(controlledTmp, { recursive: true });
574+
writeFileSync(join(originalHome, "auth.json"), '{"token":"original"}\n', "utf8");
575+
writeFileSync(join(originalHome, "accounts.json"), '{"accounts":["original"]}\n', "utf8");
576+
writeFileSync(join(originalHome, ".codex-global-state.json"), '{"last":"original"}\n', "utf8");
577+
writeFileSync(join(originalHome, "config.toml"), 'model_reasoning_effort = "xhigh"\n', "utf8");
578+
579+
const result = runWrapper(
580+
fixtureRoot,
581+
["exec", "status", "--model", "gpt-5.1"],
582+
{
583+
CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin,
584+
CODEX_HOME: originalHome,
585+
CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME: originalHome,
586+
TMP: controlledTmp,
587+
TEMP: controlledTmp,
588+
TMPDIR: controlledTmp,
589+
...injectShadowCleanupBusyFailures(3),
590+
},
591+
);
592+
593+
expect(result.status).toBe(0);
594+
expect(readFileSync(join(originalHome, "auth.json"), "utf8").trim()).toBe('{"token":"shadow"}');
595+
expect(readFileSync(join(originalHome, "accounts.json"), "utf8").trim()).toBe(
596+
'{"accounts":["external-during-retry"]}',
597+
);
598+
expect(
599+
readFileSync(join(originalHome, ".codex-global-state.json"), "utf8").trim(),
600+
).toBe('{"last":"external-during-retry"}');
601+
});
602+
539603
it("rewrites unquoted config reasoning effort values for mini compatibility models", () => {
540604
const fixtureRoot = createWrapperFixture();
541605
const fakeBin = createCustomFakeCodexBin(fixtureRoot, [

test/index.test.ts

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4643,6 +4643,141 @@ describe("OpenAIOAuthPlugin runtime toast forwarding", () => {
46434643
expect(fallbackCancelSpy).toHaveBeenCalledTimes(1);
46444644
});
46454645

4646+
it("records capability failure once for non-429 stream failover fallback errors", async () => {
4647+
const { AccountManager } = await import("../lib/accounts.js");
4648+
const { CapabilityPolicyStore } = await import("../lib/capability-policy.js");
4649+
const fetchHelpersModule = await import("../lib/request/fetch-helpers.js");
4650+
const streamFailoverModule = await import("../lib/request/stream-failover.js");
4651+
const currentAccount = {
4652+
index: 0,
4653+
accountId: "acc-1",
4654+
email: "alpha@example.com",
4655+
refreshToken: "refresh-1",
4656+
accessToken: "access-alpha",
4657+
};
4658+
const fallbackAccount = {
4659+
index: 1,
4660+
accountId: "acc-2",
4661+
email: "beta@example.com",
4662+
refreshToken: "refresh-2",
4663+
accessToken: "access-beta",
4664+
};
4665+
const recordFailure = vi.fn();
4666+
const saveToDiskDebounced = vi.fn();
4667+
const capabilityFailureSpy = vi.spyOn(
4668+
CapabilityPolicyStore.prototype,
4669+
"recordFailure",
4670+
);
4671+
const pendingFailovers: Array<Promise<unknown>> = [];
4672+
const fallbackErrorResponse = new Response(
4673+
new ReadableStream({
4674+
start(controller) {
4675+
controller.enqueue(new TextEncoder().encode("server error"));
4676+
controller.close();
4677+
},
4678+
}),
4679+
{ status: 500 },
4680+
);
4681+
const manager = {
4682+
getAccountCount: () => 2,
4683+
getCurrentOrNextForFamilyHybrid: () => currentAccount,
4684+
getCurrentOrNextForFamily: () => currentAccount,
4685+
getCurrentWorkspace: () => null,
4686+
getAccountByIndex: (index: number) =>
4687+
index === fallbackAccount.index ? fallbackAccount : currentAccount,
4688+
getAccountsSnapshot: () => [currentAccount, fallbackAccount],
4689+
isAccountAvailableForFamily: (index: number) => index === fallbackAccount.index,
4690+
toAuthDetails: (account: typeof currentAccount | typeof fallbackAccount) => ({
4691+
type: "oauth" as const,
4692+
access: account.accessToken,
4693+
refresh: account.refreshToken,
4694+
expires: Date.now() + 60_000,
4695+
}),
4696+
hasRefreshToken: () => true,
4697+
saveToDiskDebounced,
4698+
updateFromAuth: () => {},
4699+
clearAuthFailures: () => {},
4700+
incrementAuthFailures: () => 1,
4701+
saveToDisk: async () => {},
4702+
markAccountCoolingDown: () => {},
4703+
markRateLimited: () => {},
4704+
markRateLimitedWithReason: () => {},
4705+
consumeToken: () => true,
4706+
refundToken: () => {},
4707+
syncCodexCliActiveSelectionForIndex: async () => {},
4708+
markSwitched: () => {},
4709+
removeAccount: () => {},
4710+
recordFailure,
4711+
recordSuccess: () => {},
4712+
recordRateLimit: () => {},
4713+
getMinWaitTimeForFamily: () => 0,
4714+
shouldShowAccountToast: () => false,
4715+
markToastShown: () => {},
4716+
setActiveIndex: () => null,
4717+
};
4718+
vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValueOnce(manager as never);
4719+
vi.mocked(fetchHelpersModule.handleErrorResponse).mockImplementation(
4720+
async (response: Response) =>
4721+
({
4722+
response,
4723+
errorBody: "server error",
4724+
}) as never,
4725+
);
4726+
vi.mocked(fetchHelpersModule.transformRequestForCodex).mockImplementation(
4727+
async (init, _url, _userConfig, _codexMode, body) => ({
4728+
updatedInit: {
4729+
...(init as RequestInit),
4730+
body: JSON.stringify(body ?? {}),
4731+
},
4732+
body: (body ?? {
4733+
model: "gpt-5.1",
4734+
stream: true,
4735+
}) as {
4736+
model: string;
4737+
stream?: boolean;
4738+
},
4739+
}),
4740+
);
4741+
vi.spyOn(streamFailoverModule, "withStreamingFailover").mockImplementation(
4742+
(initialResponse, getFallbackResponse) => {
4743+
pendingFailovers.push(getFallbackResponse(1, 0));
4744+
return initialResponse;
4745+
},
4746+
);
4747+
let fetchCount = 0;
4748+
globalThis.fetch = vi.fn().mockImplementation(async () => {
4749+
fetchCount += 1;
4750+
if (fetchCount === 1) {
4751+
return new Response("data: ok\n\n", { status: 200 });
4752+
}
4753+
return fallbackErrorResponse;
4754+
});
4755+
4756+
const mockClient = createMockClient();
4757+
const { OpenAIOAuthPlugin } = await import("../index.js");
4758+
const plugin = await OpenAIOAuthPlugin({ client: mockClient } as never) as unknown as PluginType;
4759+
const sdk = await plugin.auth.loader(getOAuthAuth, { options: {}, models: {} });
4760+
const response = await sdk.fetch!("https://api.openai.com/v1/chat/completions", {
4761+
method: "POST",
4762+
body: JSON.stringify({ model: "gpt-5.1", stream: true }),
4763+
});
4764+
await Promise.all(pendingFailovers);
4765+
4766+
expect(response.status).toBe(200);
4767+
expect(recordFailure).toHaveBeenCalledTimes(1);
4768+
expect(recordFailure).toHaveBeenCalledWith(
4769+
fallbackAccount,
4770+
"gpt-5.1",
4771+
"gpt-5.1",
4772+
);
4773+
expect(capabilityFailureSpy).toHaveBeenCalledTimes(1);
4774+
expect(capabilityFailureSpy.mock.calls[0]?.[0]).toMatch(
4775+
/^account:acc-2::email:/,
4776+
);
4777+
expect(capabilityFailureSpy.mock.calls[0]?.[1]).toBe("gpt-5.1");
4778+
expect(saveToDiskDebounced).not.toHaveBeenCalled();
4779+
});
4780+
46464781
it("forwards persistence error toast arguments through manual OAuth flow", async () => {
46474782
const authModule = await import("../lib/auth/auth.js");
46484783
const storageModule = await import("../lib/storage.js");

0 commit comments

Comments
 (0)