fix(codex): persist account deletion before cleanup

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<keyof OcxConfig>) 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;
 }

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 });
+  });
+});