diff --git a/src/codex/account-lifecycle.ts b/src/codex/account-lifecycle.ts index 8f750a719f2..88df335dc80 100644 --- a/src/codex/account-lifecycle.ts +++ b/src/codex/account-lifecycle.ts @@ -1,6 +1,5 @@ import { existsSync, readFileSync } from "node:fs"; import { - atomicWriteFile, deleteConfigTopLevelKey, getConfigPath, saveConfigPreservingClaudeCode, @@ -99,13 +98,10 @@ function restoreRuntimeConfig(target: OcxConfig, snapshot: OcxConfig): void { Object.assign(target, snapshot); } -function restorePersistedConfig(configPath: string, previousBytes: string): void { - try { - if (readFileSync(configPath, "utf8") === previousBytes) return; - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; +function assertPersistedConfigUnchanged(configPath: string, previousBytes: Buffer): void { + if (!readFileSync(configPath).equals(previousBytes)) { + throw new CodexAccountDeleteRollbackError(); } - atomicWriteFile(configPath, previousBytes); } /** @@ -125,7 +121,7 @@ export function deleteCodexAccount(runtimeConfig: OcxConfig, accountId: string): const previousConfig = structuredClone(runtimeConfig); const configPath = getConfigPath(); const hasPersistedConfig = existsSync(configPath); - const previousPersistedConfig = hasPersistedConfig ? readFileSync(configPath, "utf8") : undefined; + const previousPersistedConfig = hasPersistedConfig ? readFileSync(configPath) : undefined; const hadStoredAccount = (runtimeConfig.codexAccounts ?? []) .some(account => !account.isMain && account.id === accountId); const hadVisiblePickerBinding = hadStoredAccount @@ -154,7 +150,7 @@ export function deleteCodexAccount(runtimeConfig: OcxConfig, accountId: string): } catch (error) { restoreRuntimeConfig(runtimeConfig, previousConfig); try { - restorePersistedConfig(configPath, previousPersistedConfig); + assertPersistedConfigUnchanged(configPath, previousPersistedConfig); } catch { throw new CodexAccountDeleteRollbackError(); } diff --git a/tests/codex-integration/codex-account-delete-atomicity.test.ts b/tests/codex-integration/codex-account-delete-atomicity.test.ts index fccaa8abe5c..25728bbda52 100644 --- a/tests/codex-integration/codex-account-delete-atomicity.test.ts +++ b/tests/codex-integration/codex-account-delete-atomicity.test.ts @@ -1,5 +1,11 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdirSync, readFileSync} from "node:fs"; +import { + existsSync, + mkdirSync, + readFileSync, + unlinkSync, + writeFileSync, +} from "node:fs"; import { join } from "node:path"; import * as accountStoreModule from "../../src/codex/account-store"; import { @@ -8,6 +14,7 @@ import { } from "../../src/codex/account-store"; import { CodexAccountDeleteCleanupError, + CodexAccountDeleteRollbackError, deleteCodexAccount, } from "../../src/codex/account-lifecycle"; import { @@ -85,7 +92,26 @@ describe("Codex account delete persistence ordering", () => { } }); - test("a failure after durable config replacement restores the prior config", () => { + test("a failure before durable config replacement rethrows while disk remains unchanged", () => { + const config = seededConfig(); + const before = structuredClone(config); + const beforeBytes = readFileSync(getConfigPath(), "utf8"); + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(() => { throw new Error("forced pre-write failure"); }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow("forced pre-write failure"); + expect(config).toEqual(before); + expect(readFileSync(getConfigPath(), "utf8")).toBe(beforeBytes); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("a failure after durable config replacement leaves changed disk untouched", () => { const config = seededConfig(); const before = structuredClone(config); const beforeBytes = readFileSync(getConfigPath(), "utf8"); @@ -97,11 +123,91 @@ describe("Codex account delete persistence ordering", () => { }); try { - expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow("forced post-write failure"); + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow(CodexAccountDeleteRollbackError); expect(config).toEqual(before); - expect(readFileSync(getConfigPath(), "utf8")).toBe(beforeBytes); - expect(loadConfig().codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(true); + expect(readFileSync(getConfigPath(), "utf8")).not.toBe(beforeBytes); + expect(loadConfig().codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("a concurrent external edit remains byte-identical after uncertain failure", () => { + const config = seededConfig(); + const before = structuredClone(config); + const realSave = configModule.saveConfigPreservingClaudeCode; + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(candidate => { + realSave(candidate); + const external = loadConfig(); + external.port = 12345; + writeFileSync(getConfigPath(), JSON.stringify(external, null, 2) + "\n"); + throw new Error("forced concurrent failure"); + }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow(CodexAccountDeleteRollbackError); + const persisted = loadConfig(); + expect(persisted.port).toBe(12345); + expect(persisted.codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + expect(config).toEqual(before); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("distinct bytes with the same decoded text are treated as changed", () => { + const config = seededConfig(); + const before = structuredClone(config); + const validBytes = Buffer.from('{"value":"\uFFFD"}\n', "utf8"); + const malformedBytes = Buffer.concat([ + Buffer.from('{"value":"', "utf8"), + Buffer.from([0x80]), + Buffer.from('"}\n', "utf8"), + ]); + expect(validBytes.equals(malformedBytes)).toBe(false); + expect(validBytes.toString("utf8")).toBe(malformedBytes.toString("utf8")); + writeFileSync(getConfigPath(), validBytes); + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(() => { + writeFileSync(getConfigPath(), malformedBytes); + throw new Error("forced byte-alias failure"); + }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow(CodexAccountDeleteRollbackError); + expect(readFileSync(getConfigPath()).equals(malformedBytes)).toBe(true); + expect(config).toEqual(before); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("a missing config after uncertain failure is not recreated", () => { + const config = seededConfig(); + const before = structuredClone(config); + const realSave = configModule.saveConfigPreservingClaudeCode; + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(candidate => { + realSave(candidate); + unlinkSync(getConfigPath()); + throw new Error("forced missing-file failure"); + }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow(CodexAccountDeleteRollbackError); + expect(existsSync(getConfigPath())).toBe(false); + expect(config).toEqual(before); expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull();