Bound the ambient wait when confirming a claude-swap switch - #3750
keepitmello wants to merge 1 commit into
Conversation
An unbounded await on refreshProvider(.claude) can never return when Claude Code's own credential read has no answer. The transient switch state then stays set, so the menu keeps the requested account pending and handleClaudeSwapAccountSelection refuses every later selection until relaunch.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 18, 2026, 11:16 PM ET / September 19, 2026, 03:16 UTC. ClawSweeper reviewWhat this changesAdds a five-second timeout race around the Claude usage refresh performed after switching a claude-swap account. Merge readiness⛔ Blocked before merge - 8 items remain The remaining stall is still actionable, but this patch does not enforce its timeout: the task group waits for the suspended ambient waiter before returning. The reported successful switches do not exercise the deadline. Priority: P2 Review scores
Verification
How this fits togetherCodexBar switches Claude accounts through an external adapter, then refreshes Claude usage and the adapter’s account list. The menu disables further switching until this reconciliation completes. flowchart TD
A[Account chip selected] --> B[External account switch]
B --> C[Ambient Claude refresh]
C --> D[Proposed timeout race]
D --> E[Await adapter account list]
E --> F[Clear pending state and enable chips]
Decision needed
Why: The recent owner-authored reconciliation fix and current documentation explicitly retain the guard until both refreshes finish; changing that lifetime requires intent beyond repairing the race. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: With owner agreement, use the existing single-completion timeout pattern to bound ambient waiting while preserving adapter reconciliation, truthful pending state, and stale-result protection. Do we have a high-confidence way to reproduce the issue? Yes, from source: hold ambient refresh pending after the adapter completes and the new task group still cannot exit when its timer wins. No runtime tests were executed during this read-only review. Is this the best way to solve the issue? No. The structured task-group race cannot enforce the claimed bound; the repository already has a continuation-based timeout pattern suitable for adaptation. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6e603f578647. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Maintainer review of There is a second reconciliation concern when repairing the timeout. Current provider refreshes wait for predecessor requests before starting, and the Claude adapter-list refresh is scheduled only after that start. A second switch while the first ambient refresh remains suspended can therefore reach reconciliation without obtaining its own fresh adapter list. Please add deterministic coverage for timeout expiry while ambient work is still held, a second switch before the first refresh finishes, fresh adapter/account identity, and rejection of late results. The fix needs to preserve the serialized account-reconciliation contract from #3736/#3740. Keeping this open for that repair; the existing two-second success path does not exercise timeout expiry. |
Bound the ambient refresh wait without cancelling the provider request, and refresh the adapter independently when a stalled predecessor delays that read. Keep credential transactions serialized through the current adapter list. Adopts #3750 with a bounded join instead of a task group that still waits for its suspended child. Refs #3736. Co-authored-by: keepitmello <keepitmello@users.noreply.github.com> (cherry picked from commit b73c145)
|
Adopted with a bounded-join rewrite in #3812. The timeout ends only the ambient wait, while adapter reconciliation and serialized credential transactions remain intact. Synthetic regressions cover a suspended ambient probe, a later switch queued behind it, retained switch errors, and late ambient results. Thanks @keepitmello! |
2 participants
Fixes the "stuck after switching" half of #3736.
What happens
UsageStore.switchClaudeSwapAccountholds the transient switch state untilawait refreshProvider(.claude)returns. That refresh reads Claude Code's ownambient credential, and when that read has no answer it does not come back at
all — so the switch never completes:
Details for B/Loading…while chip A stays highlighted,handleClaudeSwapAccountSelectionreturns early while
claudeSwapTransientState.task != nil.The segmented control is then inert until relaunch. The issue reported the first
symptom; the second is why waiting it out never helped.
Observed with two accounts, where the adapter had already answered and the menu
still never moved:
It is not the adapter: the same thing happens with
cswapand with a differentexecutable that speaks the same
--list/--switch-toschema and answers in0.02–0.04 s.
The change
The ambient wait is bounded instead of removed.
ClaudeSwapSwitchReconciliationTestspins the serialization contract ("ambient completion keeps switching serialized
until the real adapter list finishes"), and detaching the refresh entirely breaks
it — and would likely reopen the "previous account's data after switching" class
of bugs (#1785, #2731). Past the bound, reconciliation continues on the adapter
list and the ambient snapshot lands whenever it completes.
5 s is a first proposal; the value is easy to move if you would rather have it
shorter, configurable, or expressed as a switch-phase deadline.
Verified
swift buildswift test --filter ClaudeSwap— 182 tests in 21 suites, all passingmake check— 0 violations in 2417 filesaccounts, segmented layout: three consecutive chip switches confirmed, adapter
invocations logged for each.