fix(lume): handle guest-initiated VM shutdown via VZVirtualMachineDelegate#1254
fix(lume): handle guest-initiated VM shutdown via VZVirtualMachineDelegate#1254NikkeTryHard wants to merge 2 commits intotrycua:mainfrom
Conversation
|
@NikkeTryHard is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change replaces polling-based guest shutdown detection with an event-driven approach using delegate callbacks. The virtualization service now awaits guest termination via a new Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VM.run()
participant Service as VMVirtualizationService
participant Delegate as VZVirtualMachine Delegate
participant Cleanup as Cleanup Handler
VM->>Service: waitForGuestStop()
activate Service
Note over Service: Awaits guest stop event
Delegate->>Service: guestDidStop() or didStopWithError()
Service->>Service: Resolve continuation with error status
deactivate Service
Service-->>VM: Returns Error? (nil or error)
VM->>Cleanup: Perform cleanup on return
Cleanup->>Cleanup: Stop clipboardWatcher
Cleanup->>Cleanup: Stop vncService
Cleanup->>Cleanup: Release file lock (flock LOCK_UN)
Cleanup->>Cleanup: Close file handle
Cleanup->>Cleanup: Unlock config file
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
NikkeTryHard
force-pushed
the
fix/guest-shutdown-cleanup
branch
from
2b5f297 to
4646b5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/lume/tests/Mocks/MockVMVirtualizationService.swift (1)
66-68: Keep the mock stop waiter stateful.Returning
nilimmediately makesVM.run()finish as soon asstart()succeeds, andcurrentStatenever transitions back to.stopped. That makes the new lifecycle hard to exercise in tests and leaves no way to cover guest-error handling. A test-controlled continuation/result here would keep the mock aligned with production behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/tests/Mocks/MockVMVirtualizationService.swift` around lines 66 - 68, The mock waitForGuestStop currently returns nil immediately which prevents VM.run() from observing a guest stop and blocks tests from exercising lifecycle and error paths; update MockVMVirtualizationService to make waitForGuestStop() await a test-controlled continuation/result (e.g., store a CheckedContinuation<Error?, Never> or similar in the mock) and provide helper methods on the mock to resume that continuation with nil (normal stop) or an Error (guest failure), and ensure start() and any stop helpers update currentState appropriately so tests can drive the state transitions and cover guest-error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/lume/src/VM/VM.swift`:
- Around line 328-347: The code logs guestError from service.waitForGuestStop()
but swallows it by returning normally; capture the error when waitForGuestStop()
returns a non-nil guestError (in the block surrounding
service.waitForGuestStop() in VM.swift), perform the existing shared cleanup
(stop clipboardWatcher, nil out virtualizationService, stop vncService, release
lock/close fileHandle, call unlockConfigFile()), and then rethrow the captured
guestError so callers see the unexpected stop; apply the same pattern to the
other occurrence mentioned (the block around lines 1071–1084) so both places
log, clean up, and then throw the original error instead of returning
successfully.
---
Nitpick comments:
In `@libs/lume/tests/Mocks/MockVMVirtualizationService.swift`:
- Around line 66-68: The mock waitForGuestStop currently returns nil immediately
which prevents VM.run() from observing a guest stop and blocks tests from
exercising lifecycle and error paths; update MockVMVirtualizationService to make
waitForGuestStop() await a test-controlled continuation/result (e.g., store a
CheckedContinuation<Error?, Never> or similar in the mock) and provide helper
methods on the mock to resume that continuation with nil (normal stop) or an
Error (guest failure), and ensure start() and any stop helpers update
currentState appropriately so tests can drive the state transitions and cover
guest-error handling.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f094a50a-a4c0-47a5-a7e0-8836da4af801
📒 Files selected for processing (3)
libs/lume/src/VM/VM.swiftlibs/lume/src/Virtualization/VMVirtualizationService.swiftlibs/lume/tests/Mocks/MockVMVirtualizationService.swift
1 participant
Problem
When shutting down macOS from inside the guest (Apple menu > Shut Down), the Lume host process does not exit.
lume lscontinues to show the VM as running, andlume stophangs because thewhile true { Task.sleep }loop inVM.run()never breaks. The only recovery iskill.The root cause is that Lume never sets a
VZVirtualMachineDelegateon itsVZVirtualMachineinstance, so theguestDidStopcallback -- which Virtualization.framework fires when the guest OS powers off -- is never received.Fix
Conform
BaseVirtualizationServicetoVZVirtualMachineDelegateand replace the infinite sleep loops withwaitForGuestStop(), a continuation-based suspend that returns whenguestDidStopordidStopWithErrorfires. On return, the existing cleanup path runs (release file lock, stop VNC, nil out service).A
pendingGuestStopbuffer handles the edge case where the delegate fires betweenservice.start()andwaitForGuestStop().Changes
VMVirtualizationService.swift-- addNSObject+VZVirtualMachineDelegateconformance toBaseVirtualizationService, set delegate in init, implementguestDidStop/didStopWithError, addwaitForGuestStop()to protocol and implementationVM.swift-- replacewhile trueloops inrun()andrunWithUSBStorage()withwaitForGuestStop()+ cleanup on returnMockVMVirtualizationService.swift-- addwaitForGuestStop()stubTest plan
lume run <vm>-- inside guest, Apple menu > Shut Down -- host process exits cleanly,lume lsshows stoppedlume run <vm>--lume stop <vm>from another terminal -- still works as beforelume serve-- run VM via API, guest shutdown -- VM cleaned up, server stays aliveswift buildpasses (verified on macOS 15.7.5 -- zero new errors, all failures are pre-existing ARM-only symbols)Closes #1184
Summary by CodeRabbit