fix(vz): stop the VM even when CanRequestStop is false

pkg/driver/vz/vz_driver_darwin.go

@@ -500,43 +500,66 @@ func (l *LimaVzDriver) requestStopViaSSH(ctx context.Context) error {
 	return nil
 }
 
+// gracefulStopTimeout is how long to wait for the guest to shut down by itself.
+// Most Linux machines shutdown within 5 seconds, but macOS machines can take longer.
+const gracefulStopTimeout = 30 * time.Second
+
 func (l *LimaVzDriver) Stop(ctx context.Context) error {
 	logrus.Info("Shutting down VZ")
-	canStop := l.machine.CanRequestStop()
-
-	if canStop {
-		_, err := l.machine.RequestStop()
-		if err != nil {
-			return err
+	defer l.removePIDFile()
+
+	requested := false
+	if l.machine.CanRequestStop() {
+		// The returned boolean tells whether the request was made, not whether the guest
+		// has already stopped.
+		var err error
+		if requested, err = l.machine.RequestStop(); err != nil {
+			logrus.WithError(err).Warn("vz: failed to request the guest to stop")
 		}
+	} else {
+		logrus.Warn("vz: CanRequestStop is not supported")
+	}
 
-		if *l.Instance.Config.OS == limatype.DARWIN {
-			// macOS VM does not respond to l.machine.RequestStop(),
-			// so we need to run `shutdown -h now` in the VM via SSH for graceful shutdown.
-			if err := l.requestStopViaSSH(ctx); err != nil {
-				logrus.WithError(err).Warn("Failed to request shutdown via SSH")
-			}
+	// macOS VM does not respond to l.machine.RequestStop(), and neither does a guest that
+	// could not be asked at all, so we need to run `shutdown -h now` in the VM via SSH for
+	// graceful shutdown.
+	if !requested || *l.Instance.Config.OS == limatype.DARWIN {
+		if err := l.requestStopViaSSH(ctx); err != nil {
+			logrus.WithError(err).Warn("Failed to request shutdown via SSH")
 		}
+	}
 
-		// Most Linux machines shutdown within 5 seconds, but macOS machines can take longer.
-		timeout := time.After(30 * time.Second)
-		ticker := time.NewTicker(500 * time.Millisecond)
-		for {
-			select {
-			case <-timeout:
-				return errors.New("vz timeout while waiting for stop status")
-			case <-ticker.C:
-				l.machine.mu.Lock()
-				stopped := l.machine.stopped
-				l.machine.mu.Unlock()
-				if stopped {
-					return nil
-				}
+	// The VM is never stopped forcibly with l.machine.Stop(): that does not give the guest
+	// a chance to stop cleanly, and may corrupt the disk.
+	return l.waitForStopped(gracefulStopTimeout)
+}
+
+func (l *LimaVzDriver) waitForStopped(timeout time.Duration) error {
+	timeoutCh := time.After(timeout)
+	ticker := time.NewTicker(500 * time.Millisecond)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-timeoutCh:
+			return errors.New("vz timeout while waiting for stop status")
+		case <-ticker.C:
+			l.machine.mu.Lock()
+			stopped := l.machine.stopped
+			l.machine.mu.Unlock()
+			if stopped {
+				return nil
 			}
 		}
 	}
+}
 
-	return errors.New("vz: CanRequestStop is not supported")
+// removePIDFile removes the PID file of the driver, so that a PID file is never left
+// behind for a VM that is not running anymore.
+func (l *LimaVzDriver) removePIDFile() {
+	pidFile := filepath.Join(l.Instance.Dir, filenames.PIDFile(*l.Instance.Config.VMType))
+	if err := os.RemoveAll(pidFile); err != nil {
+		logrus.WithError(err).Warnf("Failed to remove %#q", pidFile)
+	}
 }
 
 func (l *LimaVzDriver) GuestAgentConn(_ context.Context) (net.Conn, string, error) {

pkg/driver/vz/vz_driver_darwin_test.go

@@ -0,0 +1,55 @@
+//go:build darwin && !no_vz
+
+// SPDX-FileCopyrightText: Copyright The Lima Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package vz
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"gotest.tools/v3/assert"
+
+	"github.com/lima-vm/lima/v2/pkg/limatype"
+	"github.com/lima-vm/lima/v2/pkg/limatype/filenames"
+)
+
+func TestWaitForStopped(t *testing.T) {
+	t.Run("returns nil after the VM has stopped", func(t *testing.T) {
+		l := &LimaVzDriver{machine: &virtualMachineWrapper{}}
+		go func() {
+			l.machine.mu.Lock()
+			l.machine.stopped = true
+			l.machine.mu.Unlock()
+		}()
+		assert.NilError(t, l.waitForStopped(10*time.Second))
+	})
+
+	t.Run("returns an error when the VM is still running", func(t *testing.T) {
+		l := &LimaVzDriver{machine: &virtualMachineWrapper{}}
+		assert.ErrorContains(t, l.waitForStopped(600*time.Millisecond), "timeout")
+	})
+}
+
+func TestRemovePIDFile(t *testing.T) {
+	dir := t.TempDir()
+	l := &LimaVzDriver{
+		Instance: &limatype.Instance{
+			Dir:    dir,
+			Config: &limatype.LimaYAML{VMType: new(limatype.VZ)},
+		},
+	}
+	pidFile := filepath.Join(dir, filenames.PIDFile(limatype.VZ))
+	assert.NilError(t, os.WriteFile(pidFile, []byte("42\n"), 0o644))
+
+	l.removePIDFile()
+	_, err := os.Stat(pidFile)
+	assert.Assert(t, errors.Is(err, os.ErrNotExist))
+
+	// Removing a PID file that does not exist is not an error.
+	l.removePIDFile()
+}