Skip to content

LinuxPod: stop the VM when setup fails after start - #847

Open
shoemoney wants to merge 1 commit into
apple:mainfrom
shoemoney:fix/linuxpod-vm-leak-on-start
Open

LinuxPod: stop the VM when setup fails after start#847
shoemoney wants to merge 1 commit into
apple:mainfrom
shoemoney:fix/linuxpod-vm-leak-on-start

Conversation

@shoemoney

Copy link
Copy Markdown

Summary

LinuxPod.create() calls vm.start() outside the do/catch that cleans up on failure, so a
throw from start() leaves the VZVirtualMachine — and its
com.apple.Virtualization.VirtualMachine.xpc helper process — running with nothing able to stop it.
vm is local to create(), so once the error propagates there is no handle left to call stop() on.

This is the same defect fixed for LinuxContainer.create() in #836, at the sibling call site.
Issue #804 describes the LinuxContainer case and is still open.

The change

Move try await vm.start() inside the existing do block.

 let relayManager = UnixSocketRelayManager(vm: vm)
-try await vm.start()
 
 do {
+    try await vm.start()

That is the whole diff — 1 insertion, 1 deletion. No new error handling is introduced: the catch
that already closes this block (LinuxPod.swift:833) does exactly the right thing and is identical
in shape to the one in LinuxContainer:

} catch {
    try? await relayManager.stopAll()
    try? await vm.stop()
    state.phase.setErrored(error: error)
    throw error
}

So after this change a start() failure takes the same stopAll()stop()setErrored()
rethrow path that every other setup failure in this function already takes.

I verified by brace depth that the catch at line 833 is the one closing the do opened at line 632,
rather than assuming it from proximity.

Why it matters

The reproduction in #804 is a vmnet allocation failure surfaced through network device attachment.
Any start() failure has the same effect: an orphaned VM plus a leaked XPC helper process per
attempt, which accumulates across retries.

Testing

  • swift build --target Containerization — clean (Swift 6.4, arm64-apple-macosx, Build complete!).
  • No new tests added. The change moves one statement into an existing, already-tested error path and
    adds no new branches; exercising it requires inducing a real VZVirtualMachine.start() failure,
    which needs host virtualization state (e.g. vmnet exhaustion) rather than a unit-test seam.
    LinuxContainer: stop the VM when setup fails after start #836 landed the identical change on LinuxContainer without new tests for the same reason.

Notes

LinuxContainer and LinuxPod still keep two separate copies of this create-and-start-then-clean-up
sequence, which is why the fix had to be applied twice. Consolidating them is a larger refactor and
deliberately out of scope here — happy to open a follow-up issue if that is wanted.

LinuxPod.create() called vm.start() outside the do/catch that otherwise
cleans up on failure. If vm.start() threw -- for example a vmnet allocation
failure surfaced through network device attachment -- the VZVirtualMachine
and its com.apple.Virtualization.VirtualMachine.xpc helper process were
never stopped. vm is local to create(), so nothing outside could reach it
to stop it afterwards.

Move vm.start() inside the existing do block so its failure runs the same
relayManager.stopAll() / vm.stop() / setErrored path every other setup
failure already takes.

This is the same fix applied to LinuxContainer.create() in apple#836, for the
sibling call site in LinuxPod.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant