fix(desktop): bound backend shutdown wait during quit - #7599
Conversation
The quit-time finalizer stopped every backend instance with no timeout, so a backend that didn't exit cleanly hung the wait forever. Since a recent change now destroys the window before this wait, the hang was invisible: holding Cmd+Q looked like it did nothing, requiring a force quit. Apply the same 5s timeout already used for this identical operation in DesktopUpdates.ts, so quit always makes progress.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — Straightforward bug fix that adds a 5-second timeout to backend shutdown during quit, preventing indefinite hangs. The change extracts existing logic into a named function, adds a timeout parameter, and includes a comprehensive test. Limited scope with clear defensive purpose. You can add or adjust custom eligibility rules. Learn more. |
Problem
The quit-time finalizer stopped every backend instance in the pool with no timeout. If any one backend didn't exit cleanly, the wait never ended, and the app never called
app.quit(). Since PR #6562 now destroys the window before this wait, the hang became invisible: holding Cmd+Q looked like it did nothing, and the only way out was a force quit.Fixes #7598
Fix
Bound the wait to 5 seconds, matching the identical pattern already used for this same operation in
apps/desktop/src/updates/DesktopUpdates.ts. On timeout, the close keeps running in the background instead of blocking quit, so a slow-but-healthy shutdown still finishes cleanly, and a genuinely hung one no longer blocks the app from exiting.Extracted the finalizer's logic into an exported
stopAllPoolInstancesfunction so it has a real seam to test against.Testing
DesktopBackendManager.test.tscalls the realstopAllPoolInstancesexport with a mocked pool whose instances hang past the timeout, and asserts the call still completes on time. Verified by hand that removing the timeout argument makes this test hang, confirming it actually catches a regression.vp test run apps/desktop/src/backend/DesktopBackendManager.test.ts: 26 passedvp run --filter @t3tools/desktop typecheck: cleanModel: Claude Sonnet 5. Harness: Claude Code.
Note
Medium Risk
Changes desktop quit/shutdown behavior for all pooled backends; bounded wait reduces hang risk but timed-out teardown may still run in the background.
Overview
Fixes quit hangs when a backend pool instance never finishes teardown (e.g. after the window is destroyed first), so Cmd+Q could appear to do nothing until force quit.
Quit finalizer now goes through exported
stopAllPoolInstances, which stops every pooled backend concurrently with a 5s timeout perinstance.stop—same pattern asDesktopUpdates.ts. After the budget, quit keeps moving instead of waiting forever; slow but healthy shutdown can still finish in the background.Adds a regression test that calls the real
stopAllPoolInstanceswith a mocked pool whose instances hang past the timeout and asserts the call completes on time.Reviewed by Cursor Bugbot for commit 6187110. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Bound backend shutdown wait to 5 seconds per instance during desktop app quit
stopAllPoolInstancesfunction in DesktopApp.ts that stops each instance fromDesktopBackendPoolwith a 5-second timeout, running stops concurrently.scopedProgram's finalizer with a call tostopAllPoolInstances, preventing the app from hanging indefinitely on quit if a backend fails to stop.Macroscope summarized 6187110.