diff --git a/src/codex/account-lifecycle.ts b/src/codex/account-lifecycle.ts index 7404089db..b9757a988 100644 --- a/src/codex/account-lifecycle.ts +++ b/src/codex/account-lifecycle.ts @@ -1,3 +1,10 @@ +import { existsSync, readFileSync } from "node:fs"; +import { + atomicWriteFile, + getConfigPath, + saveConfigPreservingClaudeCode, + withConfigMutationLockSync, +} from "../config"; import { removeCodexAccountCredential } from "./account-store"; import { clearAccountNeedsReauth } from "./account-runtime-state"; import { getMainChatgptAccountId } from "./auth-collision"; @@ -13,6 +20,20 @@ import type { OcxConfig } from "../types"; let observedMainChatgptAccountId: string | undefined; +export class CodexAccountDeleteCleanupError extends Error { + constructor() { + super("Account deletion was saved, but local credential cleanup did not complete. Retry removal."); + this.name = "CodexAccountDeleteCleanupError"; + } +} + +export class CodexAccountDeleteRollbackError extends Error { + constructor() { + super("Account deletion failed and the previous config could not be restored. Restart before retrying."); + this.name = "CodexAccountDeleteRollbackError"; + } +} + export function purgeCodexAccountRuntimeState(accountId: string): void { clearAccountNeedsReauth(accountId); clearAccountQuota(accountId); @@ -71,25 +92,81 @@ export function resetMainCodexAccountIdentityTrackingForTests(): void { clearMainAccountCredentialPresence(); } +function restoreRuntimeConfig(target: OcxConfig, snapshot: OcxConfig): void { + for (const key of Object.keys(target) as Array) delete target[key]; + 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; + } + atomicWriteFile(configPath, previousBytes); +} + /** * Delete a stored account while retaining its selector binding. + * + * When the runtime config is backed by an existing config.json, commit the config deletion before + * credentials or runtime state are destroyed. Transient callers intentionally skip durable config + * persistence because they have no durable account row to protect. The whole sequence shares the + * config mutation coordinator so a cooperating writer cannot re-add a persisted account between + * the durable config commit and credential cleanup. + * * Returns true when a picker-visible row disappeared and the catalog must converge. */ export function deleteCodexAccount(runtimeConfig: OcxConfig, accountId: string): boolean { - const hadStoredAccount = (runtimeConfig.codexAccounts ?? []) - .some(account => !account.isMain && account.id === accountId); - const hadVisiblePickerBinding = hadStoredAccount - && codexAccountPickerEnabled(runtimeConfig) - && codexAccountNamespaceEntries(runtimeConfig) - .some(([, boundAccountId]) => boundAccountId === accountId); - removeCodexAccountCredential(accountId); - runtimeConfig.codexAccounts = (runtimeConfig.codexAccounts ?? []) - .filter(account => account.isMain || account.id !== accountId); - forgetCodexAccountPause(runtimeConfig, accountId); - forgetCodexAccountPriority(runtimeConfig, accountId); - clearCodexAccountPin(runtimeConfig, accountId); - if (runtimeConfig.activeCodexAccountId === accountId) runtimeConfig.activeCodexAccountId = undefined; - purgeCodexAccountRuntimeState(accountId); - invalidateCodexWebSocketsForAccount(accountId); - return hadVisiblePickerBinding; + let cleanupFailed = false; + const pickerVisibilityChanged = withConfigMutationLockSync(() => { + const previousConfig = structuredClone(runtimeConfig); + const configPath = getConfigPath(); + const hasPersistedConfig = existsSync(configPath); + const previousPersistedConfig = hasPersistedConfig ? readFileSync(configPath, "utf8") : undefined; + const hadStoredAccount = (runtimeConfig.codexAccounts ?? []) + .some(account => !account.isMain && account.id === accountId); + const hadVisiblePickerBinding = hadStoredAccount + && codexAccountPickerEnabled(runtimeConfig) + && codexAccountNamespaceEntries(runtimeConfig) + .some(([, boundAccountId]) => boundAccountId === accountId); + + runtimeConfig.codexAccounts = (runtimeConfig.codexAccounts ?? []) + .filter(account => account.isMain || account.id !== accountId); + forgetCodexAccountPause(runtimeConfig, accountId); + forgetCodexAccountPriority(runtimeConfig, accountId); + clearCodexAccountPin(runtimeConfig, accountId); + if (runtimeConfig.activeCodexAccountId === accountId) runtimeConfig.activeCodexAccountId = undefined; + + if (previousPersistedConfig !== undefined) { + try { + // Persist first for durable configs. Destructive cleanup below must never run for a + // deletion that failed to commit. Transient configs intentionally skip this write. + saveConfigPreservingClaudeCode(runtimeConfig); + } catch (error) { + restoreRuntimeConfig(runtimeConfig, previousConfig); + try { + restorePersistedConfig(configPath, previousPersistedConfig); + } catch { + throw new CodexAccountDeleteRollbackError(); + } + throw error; + } + } + + try { + removeCodexAccountCredential(accountId); + purgeCodexAccountRuntimeState(accountId); + invalidateCodexWebSocketsForAccount(accountId); + } catch { + // Do not throw through the mutation coordinator after config.json committed: that would roll + // back only the SQLite generation transaction, not the already-atomic file replacement. + cleanupFailed = true; + } + + return hadVisiblePickerBinding; + }); + + if (cleanupFailed) throw new CodexAccountDeleteCleanupError(); + return pickerVisibilityChanged; } diff --git a/tests/codex-account-delete-atomicity.test.ts b/tests/codex-account-delete-atomicity.test.ts new file mode 100644 index 000000000..8f71e730f --- /dev/null +++ b/tests/codex-account-delete-atomicity.test.ts @@ -0,0 +1,175 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { existsSync, mkdirSync, readFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import * as accountStoreModule from "../src/codex/account-store"; +import { + getCodexAccountCredential, + saveCodexAccountCredential, +} from "../src/codex/account-store"; +import { + CodexAccountDeleteCleanupError, + deleteCodexAccount, +} from "../src/codex/account-lifecycle"; +import { + isAccountNeedsReauth, + markAccountNeedsReauth, +} from "../src/codex/account-runtime-state"; +import { + getAccountQuota, + updateAccountQuota, +} from "../src/codex/quota"; +import { getConfigPath, loadConfig, saveConfig } from "../src/config"; +import * as configModule from "../src/config"; +import type { OcxConfig } from "../src/types"; + +const TEST_DIR = join(import.meta.dir, ".tmp-codex-account-delete-atomicity"); +const ACCOUNT_ID = "delete-atomicity"; +let previousHome: string | undefined; + +function seededConfig(): OcxConfig { + const config = loadConfig(); + config.codexAccounts = [{ + id: ACCOUNT_ID, + email: "delete-atomicity@example.test", + isMain: false, + }]; + config.codexAccountNamespaces = { stable: ACCOUNT_ID }; + config.codexAccountPickerEnabled = true; + config.pausedCodexAccountIds = [ACCOUNT_ID]; + config.codexAccountPriorities = { [ACCOUNT_ID]: 7 }; + config.activeCodexAccountPinned = ACCOUNT_ID; + config.activeCodexAccountId = ACCOUNT_ID; + saveConfig(config); + saveCodexAccountCredential(ACCOUNT_ID, { + accessToken: "delete-access", + refreshToken: "delete-refresh", + expiresAt: Date.now() + 60_000, + chatgptAccountId: "delete-chatgpt-id", + }); + markAccountNeedsReauth(ACCOUNT_ID); + updateAccountQuota(ACCOUNT_ID, 42); + return config; +} + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; +}); + +afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); +}); + +describe("Codex account delete persistence ordering", () => { + test("a config persistence failure leaves the account and destructive state intact", () => { + const config = seededConfig(); + const before = structuredClone(config); + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(() => { throw new Error("forced config write failure"); }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow("forced config write failure"); + + expect(config).toEqual(before); + expect(loadConfig().codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(true); + 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 restores the prior config", () => { + const config = seededConfig(); + const before = structuredClone(config); + const beforeBytes = readFileSync(getConfigPath(), "utf8"); + const realSave = configModule.saveConfigPreservingClaudeCode; + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(candidate => { + realSave(candidate); + throw new Error("forced post-write failure"); + }); + + try { + expect(() => deleteCodexAccount(config, ACCOUNT_ID)).toThrow("forced post-write failure"); + + expect(config).toEqual(before); + expect(readFileSync(getConfigPath(), "utf8")).toBe(beforeBytes); + expect(loadConfig().codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(true); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + } finally { + saveSpy.mockRestore(); + } + }); + + test("the durable config deletion happens before credential and runtime cleanup", () => { + const config = seededConfig(); + const realSave = configModule.saveConfigPreservingClaudeCode; + const saveSpy = spyOn(configModule, "saveConfigPreservingClaudeCode") + .mockImplementation(candidate => { + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(true); + expect(getAccountQuota(ACCOUNT_ID)).not.toBeNull(); + realSave(candidate); + }); + + try { + expect(deleteCodexAccount(config, ACCOUNT_ID)).toBe(true); + } finally { + saveSpy.mockRestore(); + } + + const persisted = loadConfig(); + expect(persisted.codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + expect(persisted.codexAccountNamespaces).toEqual({ stable: ACCOUNT_ID }); + expect(config.codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + 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(); + }); + + test("a cleanup failure keeps the deletion durable and exposes only a fixed recovery error", () => { + const config = seededConfig(); + const removeSpy = spyOn(accountStoreModule, "removeCodexAccountCredential") + .mockImplementation(() => { + throw new Error("private cleanup detail /private/codex-accounts.json Bearer secret-token"); + }); + + try { + let thrown: unknown; + try { + deleteCodexAccount(config, ACCOUNT_ID); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(CodexAccountDeleteCleanupError); + expect(String((thrown as Error).message)).toBe( + "Account deletion was saved, but local credential cleanup did not complete. Retry removal.", + ); + expect(String((thrown as Error).message)).not.toContain("private"); + expect(String((thrown as Error).message)).not.toContain("secret-token"); + expect(loadConfig().codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + expect(config.codexAccounts?.some(account => account.id === ACCOUNT_ID)).toBe(false); + expect(getCodexAccountCredential(ACCOUNT_ID)).not.toBeNull(); + } finally { + removeSpy.mockRestore(); + } + + // The route is retry-safe even after the durable row is gone: a second delete can finish the + // tombstone/runtime cleanup without recreating the account or selector mapping. + expect(deleteCodexAccount(config, ACCOUNT_ID)).toBe(false); + expect(getCodexAccountCredential(ACCOUNT_ID)).toBeNull(); + expect(loadConfig().codexAccountNamespaces).toEqual({ stable: ACCOUNT_ID }); + }); +});