fix(reauth): preserve polling through cancellation HTTP races - #4945
Conversation
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 73 / 80이 PR은 메인 계정 기기 재인증(reauth) 카드에서 취소(DELETE)와 상태 폴링(GET)이 동시에 실패/비-2xx로 겹칠 때 폴링이 멈춰 버려, 나중에 성공한 로그인을 못 보는 레이스를 고칩니다. 바로 앞 팁에 들어온 #4594가 “취소 소유권을 유지하고 retryable DELETE 실패 시 카드를 살린다”까지 했고, 이번 패치는 그 연장선에서 같은 flow를 취소 중일 때는 non-2xx GET이어도 폴링 주기를 유지해 이후 terminal(succeeded 등)을 관측하게 합니다. 변경 핵심은 라인 use-main-device-reauth.ts !res.ok + cancellationRequested - 의도는 명확합니다. 다만 cancellationRequested인데 DELETE가 아직 안 나갔거나, 취소 클릭 직후 아주 짧은 창에서만 GET non-2xx가 오면 “취소 소유권” 플래그와 실제 DELETE 진행이 어긋날 수 있는지 확인이 필요합니다. 플래그 set 시점과 DELETE 시작 순서가 테스트 하네스와 실사용에서 동일한지 한 줄 더 적어 두면 좋습니다. 라인 use-main-device-reauth.ts else 분기 succeeded/cancelled/failed - 들여쓰기 재배치로 로직은 이전과 같아야 합니다. diff상 terminal에서 flowRef=null + onCompleted는 succeeded에만 유지된 것으로 보입니다. #4594에서 문서화한 “successful DELETE + terminal failed DTO” 경로와 이번 “keep polling under cancel ownership”이 서로 덮어쓰지 않는지, cancelFailed 배지가 terminal까지 남는지도 기존 테스트가 여전히 통과하는지 CI에서 한 번 더 보면 됩니다. 라인 main-device-reauth-ownership.test.tsx raced polling HTTP error and cancellation - 좋은 회귀입니다. 다만 실제 버그는 “GET non-2xx가 폴링을 영구 중지”였고, 테스트는 양쪽 503 후 한 번의 추가 GET으로 succeeded를 줍니다. sleepers/wake 타이밍에 의존하므로 플레이키하면 고정 타이머나 명시적 poll 카운트 assert를 보강하세요. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Merging on the macOS exception now recorded in #4956, with the Windows evidence this change needed. At its exact head all nine Windows shards are green in the dispatched Holding this fix for that defect would delay a correctness fix for a problem it does not share and cannot influence. The release candidate remains held on #4956; this merge is not a promotion and makes no claim about macOS at this SHA. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd2f1cd03d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| flowRef.current = null; | ||
| setState({ phase: "failed", code: failureCode(dto.code) }); | ||
| return; | ||
| if (!cancellationRequested) return; |
There was a problem hiding this comment.
When cancellation has been requested, this now continues polling after every non-2xx response, including the status endpoint's definitive 404 unknown_flow. If the DELETE response is lost and the server's terminal receipt expires while the browser is suspended or disconnected, each later GET returns unknown_flow, but the hook keeps the stale pending card and issues another GET every two seconds indefinitely instead of allowing re-login. Handle unknown_flow here like the DELETE path by releasing the expired flow, while continuing only for genuinely retryable failures.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
1 participant
Summary
The 2.59.0 merged-tree audit found a race between reauthentication status polling and cancellation ownership. A non-2xx status read stopped the poll loop while a failed DELETE preserved the active device card, so a later successful login was never observed.
Keep the existing polling cadence while cancellation still owns that same flow. Successful terminal responses release ownership and notify completion once. The regression matrix covers pending and committing phases in both response orders, then observes successful completion without another login POST.
Verification
Presentation reference from #4594, using a synthetic device code. This patch changes polling lifetime behind that card; the image is not a new runtime-verification claim.
Checklist