fix(codex): persist account deletion before cleanup

src/codex/account-lifecycle.ts

@@ -1,3 +1,4 @@
+import { saveConfigPreservingClaudeCode, withConfigMutationLockSync } from "../config";
 import { removeCodexAccountCredential } from "./account-store";
 import { clearAccountNeedsReauth } from "./account-runtime-state";
 import { getMainChatgptAccountId } from "./auth-collision";
@@ -13,6 +14,13 @@ 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 function purgeCodexAccountRuntimeState(accountId: string): void {
   clearAccountNeedsReauth(accountId);
   clearAccountQuota(accountId);
@@ -71,25 +79,61 @@ 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);
+}
+
 /**
  * Delete a stored account while retaining its selector binding.
+ *
+ * Config deletion is committed before credentials or runtime state are destroyed. This prevents a
+ * failed config write from leaving a still-configured account with a tombstoned credential. The
+ * whole sequence shares the config mutation coordinator so a cooperating writer cannot re-add the
+ * 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 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;
+
+    try {
+      // Persist first. Destructive cleanup below must never run for a deletion that did not become
+      // durable. The auth API's existing follow-up save is intentionally idempotent.
+      saveConfigPreservingClaudeCode(runtimeConfig);
+    } catch (error) {
+      restoreRuntimeConfig(runtimeConfig, previousConfig);
+      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,150 @@
+import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
+import { existsSync, mkdirSync, 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 { 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("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 });
+  });
+});