fix(codex): persist account deletion before cleanup - #1319
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAccount deletion now commits configuration changes before credential and runtime cleanup. Persistence failures restore runtime and persisted configuration. Post-commit cleanup failures raise dedicated errors. Tests cover rollback, ordering, sanitized errors, and retries. ChangesAccount deletion atomicity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant deleteCodexAccount
participant ConfigPersistence
participant CredentialRuntimeCleanup
deleteCodexAccount->>ConfigPersistence: Persist account removal
alt Persistence fails
ConfigPersistence-->>deleteCodexAccount: Return failure
deleteCodexAccount->>ConfigPersistence: Restore configuration snapshots
else Persistence succeeds
ConfigPersistence-->>deleteCodexAccount: Confirm durable deletion
deleteCodexAccount->>CredentialRuntimeCleanup: Remove credentials and runtime state
CredentialRuntimeCleanup-->>deleteCodexAccount: Return cleanup result
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/account-lifecycle.ts`:
- Around line 115-121: The save failure path around
saveConfigPreservingClaudeCode must distinguish failures before and after
durable persistence. Update the contract or rollback handling so config.json is
restored to its prior file state when post-write steps such as
bumpGenerationForCooperatingConfigWrite or adoptCustomModelCatalogMigration
fail, and only call restoreRuntimeConfig(runtimeConfig, previousConfig) when
persistence never completed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 099b82a8-f078-40be-853d-ced85a2f8abf
📒 Files selected for processing (2)
src/codex/account-lifecycle.tstests/codex-account-delete-atomicity.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/codex/account-lifecycle.ts (2)
139-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve picker visibility when credential cleanup fails.
src/codex/account-lifecycle.ts:99-143computespickerVisibilityChangedfrom the pre-delete account row, thensrc/codex/account-lifecycle.ts:142throwsCodexAccountDeleteCleanupError. If a caller retries throughsrc/codex/auth-api.ts:1399-1416, the first attempt has already removed the account fromruntimeConfig.codexAccounts, so the retry returnsfalseand/api/syncreceivescatalogRefreshPending: falsefor a row that only disappeared from the picker selection map.Carry the visibility-change flag on the cleanup error, have the cleanup path converge the deleted binding before throwing, or make the retry loop call
convergeAccountNamespaceCatalogwith the same flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/account-lifecycle.ts` around lines 139 - 143, Preserve the computed picker visibility state when account cleanup fails: update the cleanup error flow around pickerVisibilityChanged and CodexAccountDeleteCleanupError so the deleted binding is converged before throwing, or the same flag is propagated and reused by the retry loop in the auth API. Ensure retries through the account deletion path retain the original visibility-change result for catalogRefreshPending.
140-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle post-callback
COMMITfailures indeleteCodexAccount.
withConfigMutationLockSynccommits afterdeleteCodexAccount’s callback returns, andsaveConfigPreservingClaudeCodealready persistsconfig.jsoninside that callback. If the outerdatabase.exec("COMMIT")then fails,deleteCodexAccountexits before any rollback, whilecodex-accounts.jsonand generation/state can still be partially updated. Define an idempotent delete/retry recovery path for post-callback commit failures or constrain the mutation contract to make this state impossible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/account-lifecycle.ts` around lines 140 - 143, Update deleteCodexAccount and its withConfigMutationLockSync mutation flow to recover when the post-callback database COMMIT fails after saveConfigPreservingClaudeCode has persisted changes. Add an idempotent delete/retry recovery path that restores or completes codex-accounts.json and generation/state consistently, and ensure cleanupFailed handling remains correct across retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/account-lifecycle.ts`:
- Around line 118-126: Update the hasPersistedConfig error path around
saveConfigPreservingClaudeCode so it tracks whether the durable file replacement
completed, compensates by restoring the previous config when it did, and
restores runtimeConfig before rethrowing. Ensure runtimeConfig is restored on
every failure, while the durable config is restored only after a completed
write, and expose the completion status through the persistence API as needed.
- Around line 91-95: Update the documentation near the account lifecycle
sequence to state that transient callers with hasPersistedConfig false skip
durable config persistence, while still mutating runtimeConfig and performing
credential removal, runtime-state purge, and websocket invalidation. Keep the
existing description of persisted-config ordering and coordination intact.
---
Outside diff comments:
In `@src/codex/account-lifecycle.ts`:
- Around line 139-143: Preserve the computed picker visibility state when
account cleanup fails: update the cleanup error flow around
pickerVisibilityChanged and CodexAccountDeleteCleanupError so the deleted
binding is converged before throwing, or the same flag is propagated and reused
by the retry loop in the auth API. Ensure retries through the account deletion
path retain the original visibility-change result for catalogRefreshPending.
- Around line 140-143: Update deleteCodexAccount and its
withConfigMutationLockSync mutation flow to recover when the post-callback
database COMMIT fails after saveConfigPreservingClaudeCode has persisted
changes. Add an idempotent delete/retry recovery path that restores or completes
codex-accounts.json and generation/state consistently, and ensure cleanupFailed
handling remains correct across retries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 802246b5-6662-45b8-bcc6-a915eeb2a3bc
📒 Files selected for processing (1)
src/codex/account-lifecycle.ts
| if (hasPersistedConfig) { | ||
| 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); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Restore durable config state when persistence fails after the file write.
saveConfigPreservingClaudeCode can complete persistConfigUnlocked and then throw in bumpGenerationForCooperatingConfigWrite() or adoptCustomModelCatalogMigration(). The catch at Line [124] restores only runtimeConfig; it does not restore config.json. The caller can receive a persistence error while the account is already durably removed, leaving credentials without the corresponding account row.
Make the persistence API report whether the file replacement completed, or perform a compensating durable restore before rethrowing. Restore runtimeConfig only when the durable write did not complete. This is the same unresolved durability gap identified in the previous review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/account-lifecycle.ts` around lines 118 - 126, Update the
hasPersistedConfig error path around saveConfigPreservingClaudeCode so it tracks
whether the durable file replacement completed, compensates by restoring the
previous config when it did, and restores runtimeConfig before rethrowing.
Ensure runtimeConfig is restored on every failure, while the durable config is
restored only after a completed write, and expose the completion status through
the persistence API as needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/codex-account-delete-atomicity.test.ts`:
- Around line 87-104: Update the test around deleteCodexAccount to capture the
original config.json bytes before invoking the forced post-write failure, then
read the file after rollback and assert the bytes match exactly. Keep the
existing in-memory and account-state assertions, but add the exact
persisted-config restoration check so fields such as pins, pauses, priorities,
and selector mappings are covered.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 834269c8-3ace-42da-9097-bdb52d756730
📒 Files selected for processing (2)
src/codex/account-lifecycle.tstests/codex-account-delete-atomicity.test.ts
|
Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
1 participant
Summary
config.jsonbefore destructive credential/runtime cleanup.Root cause
deleteCodexAccount()tombstoned the credential and purged runtime state before the caller persisted the config mutation. IfsaveConfigPreservingClaudeCode()then failed, the durable config could still contain the account while its credential was already tombstoned and its live state had been cleared.Tests
Added
tests/codex-account-delete-atomicity.test.tscovering:Notes
This is the separate non-blocking hardening follow-up identified while reviewing #1303. It is intentionally scoped to account-delete persistence ordering and does not change the account-picker feature itself.
CI should run the repository validation for this branch; no local checkout was available in this session to run Bun tests before publishing.
Summary by CodeRabbit
Bug Fixes
Tests