refactor(cli): replace _run_login's polling loop with threading.Event.wait - #298
Conversation
….wait _run_login bridged a threading.Event (set by either the Playwright browser-close callback or the background stdin-reader thread) into async code with a hand-rolled `while not done.is_set(): await asyncio.sleep(0.2)` loop — a busy-poll with up to 200ms of added latency before the login flow notices either signal. Replaced it with `await asyncio.to_thread(done.wait)`, which blocks a worker thread on the same Event.wait() until either signal fires, removing the polling entirely and reacting immediately. Behavior is unchanged: same Event, same two setters, same eventual outcome. Dropped the now-unused `monkeypatch.setattr(cli.asyncio, "sleep", noop_sleep)` lines (and the now-unused `noop_sleep` import) from the two tests exercising this path in tests/test_cli.py, since asyncio.sleep is no longer called here.
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe login flow now waits for the completion event with ChangesLogin wait flow
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized login-wait refactor preserves the existing completion signals and has no actionable merge-blocking risk beyond normal checks and review. 🚥 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2907d6e. Configure here.
…run_login Cursor Bugbot flagged that asyncio.to_thread(done.wait) parks a default-executor thread on a threading.Event that cancellation cannot interrupt. If _run_login is ever cancelled before done is set, that thread keeps blocking forever, and asyncio.run()'s shutdown sequence joins the default executor before returning — hanging process exit. Switched to asyncio.Event, set via loop.call_soon_threadsafe from both signal sources (the background stdin-reader thread, and the browser's own "close" callback). This keeps the "react immediately, no polling" property from the previous commit without ever parking a thread: awaiting an asyncio.Event is natively cancellable, so a cancelled _run_login unwinds cleanly with no orphaned thread.
2 participants
What
_run_login's CLI login flow bridges athreading.Event(done, set by either the Playwright browser-closecallback_mark_browser_closed, or the background stdin-reader thread_read_stdin) into async code with a hand-rolled busy-poll loop:This is exactly what
asyncio.to_thread+threading.Event.wait()exists for — blocking a worker thread on the event until it's set, instead of pollingis_set()on a fixed 200ms cadence. Replaced with:Why this is an improvement
asyncioandthreadingwere already imported incli.py.Why this is safe
Event, same two setters (_mark_browser_closed,_read_stdin), same eventual outcome — the coroutine still only resumes oncedoneis set, from whichever source fires first.tests/test_cli.py::test_run_login_opens_headed_persistent_browser_and_saves_profileandtest_run_login_exits_cleanly_when_browser_window_closes_firstexercise this exact code path (both signal sources) and pass unchanged.monkeypatch.setattr(cli.asyncio, "sleep", noop_sleep)lines from those two tests (and the now-unusednoop_sleepimport) sinceasyncio.sleepis no longer called in_run_login.ruff check src/ tests/clean.ruff format --checkon the touched files reports the same pre-existing "would reformat" lines that exist onmaintoday (unrelated to this diff, confirmed viagit stashA/B) — perAGENTS.md, existing files are not reformatted unless asked.🤖 Generated with Claude Code
https://claude.ai/code/session_01UNp1qd4G67kz4mMxt5Z6u3
Generated by Claude Code
Note
Low Risk
Localized change to the interactive login CLI wait path; same completion signals and cleanup behavior, with no auth or verification logic touched.
Overview
The interactive
logincommand no longer polls every 200ms for completion._run_loginnow waits on anasyncio.Eventwithawait done.wait(), while the Playwright close handler and background stdin reader signal completion vialoop.call_soon_threadsafe(done.set).This should wake immediately when the user presses Enter or closes the browser, and avoids parking a worker thread on
threading.Event.wait()if the coroutine is cancelled. Login tests no longer monkeypatchasyncio.sleepor importnoop_sleep, since the poll loop is gone.Reviewed by Cursor Bugbot for commit 5e2ad2e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit