diff --git a/src/codex/account-lifecycle.ts b/src/codex/account-lifecycle.ts index 75f748a805..703e08f247 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, @@ -107,13 +106,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); } /** @@ -133,7 +129,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 @@ -162,7 +158,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 fccaa8abe5..3e49033acc 100644 --- a/tests/codex-integration/codex-account-delete-atomicity.test.ts +++ b/tests/codex-integration/codex-account-delete-atomicity.test.ts @@ -1,13 +1,23 @@ 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 fsModule from "node:fs"; import * as accountStoreModule from "../../src/codex/account-store"; +import * as websocketRegistryModule from "../../src/codex/websocket-registry"; +import * as quotaAutoRefreshStateModule from "../../src/codex/quota-auto-refresh-state"; import { getCodexAccountCredential, saveCodexAccountCredential, } from "../../src/codex/account-store"; import { CodexAccountDeleteCleanupError, + CodexAccountDeleteRollbackError, deleteCodexAccount, } from "../../src/codex/account-lifecycle"; import { @@ -85,7 +95,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 +126,72 @@ 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); + let replacementBytes: Buffer | undefined; + const realSave = configModule.saveConfigPreservingClaudeCode; + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(candidate => { + realSave(candidate); + const external = loadConfig(); + external.port = 12345; + replacementBytes = Buffer.from(JSON.stringify(external, null, 2) + "\n", "utf8"); + writeFileSync(getConfigPath(), replacementBytes); + throw new Error("forced concurrent failure"); + }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow(CodexAccountDeleteRollbackError); + expect(replacementBytes).toBeDefined(); + expect(readFileSync(getConfigPath())).toEqual(replacementBytes); + 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(); @@ -110,6 +200,111 @@ describe("Codex account delete persistence ordering", () => { } }); + 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(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("an unreadable config after uncertain failure preserves state and sanitizes errors", () => { + const config = seededConfig(); + const before = structuredClone(config); + const configPath = getConfigPath(); + const beforeBytes = readFileSync(configPath); + const readSpy = spyOn(fsModule, "readFileSync"); + const removeSpy = spyOn(accountStoreModule, "removeCodexAccountCredential"); + const invalidateSpy = spyOn(websocketRegistryModule, "invalidateCodexWebSocketsForAccount"); + const forgetSpy = spyOn(quotaAutoRefreshStateModule, "forgetCodexQuotaAutoRefreshAccount"); + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(() => { + readSpy.mockImplementationOnce(() => { + throw new Error("EACCES /private/config.json Bearer read-secret-token"); + }); + throw new Error("write failed /private/config.json Bearer write-secret-token"); + }); + + try { + let thrown: unknown; + try { + deleteCodexAccount(config, ACCOUNT_ID); + } catch (error) { + thrown = error; + } + expect(readSpy).toHaveBeenLastCalledWith(configPath); + expect(thrown).toBeInstanceOf(CodexAccountDeleteRollbackError); + expect((thrown as Error).message).toBe( + "Account deletion failed and the previous config could not be restored. Restart before retrying.", + ); + expect(String(thrown)).not.toContain("/private/config.json"); + expect(String(thrown)).not.toContain("secret-token"); + expect((thrown as Error).cause).toBeUndefined(); + expect(config).toEqual(before); + expect(readFileSync(configPath)).toEqual(beforeBytes); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + expect(removeSpy).not.toHaveBeenCalled(); + expect(invalidateSpy).not.toHaveBeenCalled(); + expect(forgetSpy).not.toHaveBeenCalled(); + } finally { + readSpy.mockRestore(); + saveSpy.mockRestore(); + removeSpy.mockRestore(); + invalidateSpy.mockRestore(); + forgetSpy.mockRestore(); + } + }); + + test("a transient config skips persistence but still removes credentials and runtime state", () => { + const config = seededConfig(); + const configPath = getConfigPath(); + unlinkSync(configPath); + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode"); + const removeSpy = spyOn(accountStoreModule, "removeCodexAccountCredential"); + const invalidateSpy = spyOn(websocketRegistryModule, "invalidateCodexWebSocketsForAccount"); + const forgetSpy = spyOn(quotaAutoRefreshStateModule, "forgetCodexQuotaAutoRefreshAccount"); + + try { + expect(deleteCodexAccount(config, ACCOUNT_ID)).toBe(true); + expect(saveSpy).not.toHaveBeenCalled(); + expect(existsSync(configPath)).toBe(false); + expect(config.codexAccounts).toEqual([]); + expect(config.codexAccountNamespaces).toEqual({ stable: ACCOUNT_ID }); + expect(config.pausedCodexAccountIds).toBeUndefined(); + expect(config.codexAccountPriorities).toBeUndefined(); + expect(config.activeCodexAccountPinned).toBeUndefined(); + expect(config.activeCodexAccountId).toBeUndefined(); + expect(getCodexAccountCredential(ACCOUNT_ID)).toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(false); + expect(getAccountQuota(ACCOUNT_ID)).toBeNull(); + expect(removeSpy).toHaveBeenCalledWith(ACCOUNT_ID); + expect(invalidateSpy).toHaveBeenCalledWith(ACCOUNT_ID); + expect(forgetSpy).toHaveBeenCalledWith(ACCOUNT_ID); + } finally { + saveSpy.mockRestore(); + removeSpy.mockRestore(); + invalidateSpy.mockRestore(); + forgetSpy.mockRestore(); + } + }); + test("the durable config deletion happens before credential and runtime cleanup", () => { const config = seededConfig(); const realSave = configModule.saveConfigPreservingClaudeCode;