fix(codex): recover incomplete account deletion cleanup - #1324
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex account deletion now supports deferred credential cleanup and cleanup-only retries. The API, CLI, and GUI report pending cleanup, prevent unsafe retries when an account exists, and provide retry feedback. SQLite mutation locking now uses exclusive transactions. ChangesCodex account cleanup lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant ManagementAPI
participant AccountLifecycle
participant CredentialStore
User->>GUI: Remove Codex account
GUI->>ManagementAPI: DELETE account
ManagementAPI->>AccountLifecycle: Delete ownership and invalidate runtime state
AccountLifecycle->>CredentialStore: Remove local credentials
CredentialStore-->>ManagementAPI: Return cleanup result
ManagementAPI-->>GUI: Return completion or accountCleanupPending
GUI->>ManagementAPI: Retry with cleanupOnly=1
ManagementAPI->>AccountLifecycle: Validate persisted account absence
AccountLifecycle->>CredentialStore: Remove remaining credentials
CredentialStore-->>GUI: Return cleanup completion
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs-site/src/content/docs/reference/cli/providers-accounts.md`:
- Around line 236-260: Update the provider-specific deletion help excerpts to
distinguish OAuth/API-key existence validation from Codex DELETE behavior for
absent accounts, and include --cleanup-only where the excerpt is complete. Apply
the same correction to
docs-site/src/content/docs/reference/cli/providers-accounts.md lines 236-260,
docs-site/src/content/docs/ja/reference/cli/providers-accounts.md lines 185-200,
docs-site/src/content/docs/ko/reference/cli/providers-accounts.md lines 184-199,
docs-site/src/content/docs/ru/reference/cli/providers-accounts.md lines 233-258,
and docs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.md lines
209-230, keeping translated pages consistent with the English behavior.
In `@src/cli/account-extended.ts`:
- Around line 289-292: Update the cleanup-only output branch around cleanupOnly
and accountCleanupPending so it does not print “account cleanup completed” when
cleanup remains pending; emit wording that accurately indicates incomplete
cleanup while preserving the existing removal message for non-cleanup-only
requests. Do not change exit-code behavior unless explicitly required elsewhere.
- Around line 282-283: Make the Codex-specific fields in the account removal
JSON output symmetric: update the accountCleanupPending spread beside
catalogRefreshPending so it is always emitted for Codex accounts, including
false, while remaining absent for non-Codex accounts. Preserve the existing
boolean value and avoid changing unrelated removal behavior.
In `@src/codex/auth-api.ts`:
- Around line 1413-1426: Update the DELETE account flow around hasStoredAccount,
storedCredential, and deleteCodexAccount so credential-only legacy deletions do
not persist runtimeConfig when config.json is absent. Set persistMissingConfig
only when the operation is not cleanupOnly and a persisted config already
exists, while preserving persistence for stored-account deletions and allowing
credential removal to complete.
- Around line 1427-1443: Update the DELETE account route around
deleteCodexAccount so it handles SQLite busy-lock failures from
withConfigMutationLockSync and CodexAccountDeleteRollbackError instead of
rethrowing them. Map each case to the route’s documented lock-timeout or safe
rollback response, while preserving the existing
CodexAccountCleanupRetryConflictError and CodexAccountDeleteCleanupError
handling.
In `@tests/codex-auth-api.test.ts`:
- Around line 3682-3693: Add a regression test beside the existing DELETE
unknown-account test that exercises handleCodexAuthAPI with cleanupOnly values
such as "0", "true", "yes", and an empty string, asserting each returns status
400 and the exact "cleanupOnly must be 1 when provided" error; keep the request
otherwise shaped like the existing DELETE case.
🪄 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: aec47c90-b9a1-4084-a3ba-97ee3385f995
⛔ Files ignored due to path filters (1)
assets/pr-account-cleanup-retry.pngis excluded by!**/*.png
📒 Files selected for processing (36)
docs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdgui/src/codex-account-mutation.tsgui/src/components/CodexAccountPool.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/hooks/useCodexAccountPool.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-pinned-badge.test.tsxgui/tests/codex-account-pool-toast-tone.test.tsxsrc/cli/account-catalog-refresh.tssrc/cli/account-extended.tssrc/cli/account.tssrc/codex/account-lifecycle.tssrc/codex/auth-api.tssrc/config.tsstructure/05_gui-and-management-api.mdtests/cli-account.test.tstests/codex-account-delete-atomicity.test.tstests/codex-admission-primitives.test.tstests/codex-auth-api.test.tstests/config-mutation-lock.test.tstests/native-claude-code-toggle.test.ts
bd15b9a to
177d918
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/cli-account.test.ts`:
- Around line 898-907: Add a regression test in the account CLI test suite for
removing an orphaned main credential with `remove openai main --yes
--cleanup-only`. Mock or inspect the cleanup request to verify it uses
`?id=__main__&cleanupOnly=1`, and assert the CLI reports the cleanup result
while preserving the expected exit status.
🪄 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: 3047f202-112a-4079-8992-84147b09d11d
📒 Files selected for processing (17)
docs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdsrc/cli/account-extended.tssrc/cli/help.tssrc/codex/auth-api.tsstructure/05_gui-and-management-api.mdtests/cli-account.test.tstests/codex-auth-api.test.tstests/config-mutation-lock.test.ts
177d918 to
eca2d09
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/cli-account.test.ts`:
- Around line 911-927: Add a focused test next to the existing Codex
cleanup-only removal test for plain `remove openai main --yes` without
`--cleanup-only`. Assert the command is rejected with the main Codex App login
error, no DELETE request is sent, and the `__main__` account remains present.
🪄 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: 8de666ce-e142-4798-bf3f-5131229747f4
📒 Files selected for processing (1)
tests/cli-account.test.ts
eca2d09 to
e483e21
Compare
github-actions
Bot
added
the
intake: hygiene-blocked
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
|
|
|
|
|
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/cli-account.test.ts`:
- Around line 143-166: Gate the cleanup-pending branch in the mock around the
codexOrphanAccountIds state so it only returns accountCleanupPending for
cleanup-only retries of recorded orphan IDs. Ensure unknown cleanup-only IDs
complete without pending state and are handled consistently with the server
contract, while preserving normal deletion behavior and failure-count updates.
🪄 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: 23f43f4b-f566-4fd9-9ff9-6ad9c8e84690
📒 Files selected for processing (1)
tests/cli-account.test.ts
e483e21 to
5319826
Compare
|
A maintainer must complete the required security review and apply ✅ Action performedReview finished.
|
|
|
|
|
|
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
The recovery design is directionally strong, and the earlier CodeRabbit findings are addressed. I still found three merge blockers on the current head:
- Data integrity: rollback can delete a dangling/unresolvable
config.jsonsymlink after the guarded config writer correctly refuses to replace it. - Recovery durability: the dashboard's only cleanup retry identifier lives in component-local React state, so reload/unmount/restart loses the recovery action after the account row is gone.
- Automation contract:
--cleanup-onlycan report that cleanup is still pending but still exit 0, so scripts see success for the one operation they explicitly asked to complete.
Security/privacy review is otherwise good: cleanup errors are projected to fixed public state instead of leaking storage/token details; stale cleanup retries fail closed with 409 when absence cannot be proven; live runtime ownership is invalidated after the durable row deletion; and acquiring the SQLite commit-required lock before external writes closes the reader/COMMIT window.
Review/process state also is not merge-ready yet: I found no submitted human maintainer review on this PR; the prior CodeRabbit threads are resolved, but its latest review is paused/pending. The current-head React Doctor and Cross-platform CI workflow runs are action_required with no jobs, so hosted CI has not actually validated this head. The local test results in the PR description are useful evidence, but they do not replace the blocked hosted runs.
Do not merge until the three blockers below are fixed and hosted CI actually runs on the resulting head.
| function restorePersistedConfig(configPath: string, previousBytes: string | undefined): void { | ||
| if (previousBytes === undefined) { | ||
| try { | ||
| unlinkSync(configPath); |
There was a problem hiding this comment.
🔴 Blocking — preserve an existing dangling config symlink during rollback. existsSync(configPath) is false for a dangling symlink, so this path records previousPersistedConfig === undefined. The config writer deliberately refuses an unresolvable symlink (resolveWriteTarget), but when that save throws this rollback executes unlinkSync(configPath) and deletes the symlink itself. That turns a safely refused account deletion into destructive mutation of the user's dotfiles-managed config entry. Track whether there was truly no directory entry (for example with lstat) and only remove a file that this operation actually created. Add a regression test with a dangling config.json symlink.
| const [reauthId, setReauthId] = useState<string | null>(null); | ||
| const [actionFeedback, setActionFeedback] = useState<string | null>(null); | ||
| const [actionFeedbackTone, setActionFeedbackTone] = useState<NoticeTone | null>(null); | ||
| const [cleanupRetryId, setCleanupRetryId] = useState<string | null>(null); |
There was a problem hiding this comment.
🔴 Blocking — the dashboard recovery state is not durable. cleanupRetryId exists only in component-local React state. After the DELETE succeeds, the account row is gone; if this component unmounts, the page reloads, or the app restarts before cleanup succeeds, this ID and the Retry cleanup action disappear. The UI also intentionally does not expose the opaque ID, so the dashboard has no way to reconstruct the retry. This can leave the credential orphaned indefinitely. Make pending cleanup rediscoverable/durable (or provide another server-backed recovery handle) and add a remount/reload recovery test.
| else if (classified.type === "codex" && removedActive && after.activeId === null) console.log(`openai: ${AUTO_NOTE}`); | ||
| else if (classified.type === "oauth") console.log(after.rows.length ? `${name}: active account is now ${after.activeId}` : `${name}: no accounts remaining`); | ||
| else if (classified.type === "api-key") console.log(after.rows.length ? `${name}: active key is now ${after.activeId}` : `${name}: no keys remaining`); | ||
| else if (cleanupOnly) console.log(accountCleanupPending |
There was a problem hiding this comment.
🟠 Blocking automation contract — --cleanup-only still succeeds when cleanup did not complete. This branch can print account cleanup is still pending, but cmdRemove still returns 0 below. Exit 0 is defensible for the initial delete because the account row was durably removed; for an explicit --cleanup-only retry, cleanup is the requested operation. Returning success when it remains pending makes shell automation silently accept a failed retry unless it parses human stderr/JSON. Return a non-zero result when cleanupOnly && accountCleanupPending (with stable machine-readable state preserved) and update the test that currently codifies exit 0.
|
The incomplete-cleanup bug is real and the recovery direction is valuable, but the current diff still carries a destructive rollback edge. In The retry contract also does not survive its most important boundary. We would merge a focused recovery PR that preserves pre-existing filesystem entries during rollback, makes the pending cleanup target recoverable from server-owned state after a dashboard reload, and returns a nonzero CLI status while cleanup is incomplete — with targeted rollback, reload-recovery, and exit-code tests, and the shared config transaction mode left alone. Any lock-mode change deserves its own PR with its own justification. Closing this version because the genuine deletion-recovery fix is bundled with unresolved data-safety, persistence, CLI-contract, and global-locking concerns. The fail-closed design instinct here is right; it is the blast radius that blocks it. |
Stop the PR quality gate from marking review-ready when hygiene still fails (as on lidge-jun#1324), and re-run the gate when sponsorship or exception labels change.
3 participants
Summary
This is the focused recovery follow-up to the non-blocking account-deletion note in Wibias's review of #1303. #1319 made account removal durable before destructive cleanup; this PR makes the remaining post-commit cleanup failure explicit and safely recoverable.
accountCleanupPending: truewhen the account row is gone but local credential cleanup did not finish.DELETE ...?cleanupOnly=1,ocx account remove ... --cleanup-only, and the dashboard.COMMITafter those writes complete.Dashboard recovery state
The screenshot uses synthetic
.invalidaccount data. The opaque account ID needed for retry remains component-owned and is never displayed in the recovery notice.Verification
bun run prepush— 10,153 pass / 10 skip / 0 fail; typecheck, changed-dashboard lint, privacy scan, and React Doctor passed.bun test tests/codex-account-delete-atomicity.test.ts tests/codex-auth-api.test.ts tests/cli-account.test.ts— passed.cd gui && bun test tests/codex-account-pool-behaviour.test.tsx tests/codex-account-pool-toast-tone.test.tsx tests/codex-account-pool-pinned-badge.test.tsx— passed.git diff --check upstream/dev...HEAD— clean.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
--cleanup-onlyto retry incomplete Codex account credential cleanup.