Export a NotFound-classifying helper from internal/libvirt (generaliz...

internal/grpcapi/snapshot.go

@@ -6,7 +6,6 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -392,7 +391,7 @@ func (s *Server) DeleteSnapshot(ctx context.Context, req *pb.DeleteSnapshotReque
 	// here — that must NOT block cleanup of the corrosion record + vmstate file
 	// (otherwise they leak). Treat a missing libvirt snapshot as already-deleted.
 	if delErr != nil {
-		if strings.Contains(delErr.Error(), "not found") || strings.Contains(delErr.Error(), "no domain snapshot") {
+		if lv.IsNotFound(delErr) {
 			slog.Info("delete snapshot: libvirt metadata already gone — cleaning up record", "vm", req.VmName, "snap", req.SnapshotName)
 		} else {
 			return nil, status.Errorf(codes.Internal, "delete snapshot: %v", delErr)

internal/grpcapi/snapshot_test.go

@@ -1,13 +1,18 @@
 package grpcapi
 
 import (
+	"context"
+	"errors"
+	"fmt"
 	"testing"
 
+	golibvirt "github.com/digitalocean/go-libvirt"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
 	pb "github.com/litevirt/litevirt/gen/litevirt/v1"
 	"github.com/litevirt/litevirt/internal/corrosion"
+	"github.com/litevirt/litevirt/internal/libvirtfake"
 )
 
 func TestCreateSnapshot_VMNotFound(t *testing.T) {
@@ -177,3 +182,98 @@ func TestDeleteSnapshot_WrongHost(t *testing.T) {
 		t.Errorf("code = %v, want Unavailable or FailedPrecondition", c)
 	}
 }
+
+// deleteErrVirt is a libvirtfake whose DeleteSnapshot returns a chosen error, so the
+// "libvirt metadata already gone" classification can be driven directly.
+type deleteErrVirt struct {
+	*libvirtfake.Fake
+	delErr error
+}
+
+func (v *deleteErrVirt) DeleteSnapshot(string, string) error { return v.delErr }
+
+// deleteSnapshotServer stages a stopped local VM with one snapshot record and a
+// backend whose DeleteSnapshot fails with delErr. Stopped means flatten is false at
+// snapshot.go's `flatten` decision, so only DeleteSnapshot runs — one path into the
+// classifier, no flatten fallback to reason about.
+func deleteSnapshotServer(t *testing.T, delErr error) (*Server, context.Context) {
+	t.Helper()
+	s := testServer(t)
+	s.dataDir = t.TempDir()
+	s.virt = &deleteErrVirt{Fake: libvirtfake.New(), delErr: delErr}
+	ctx := adminCtx()
+	insertTestVM(t, ctx, s.db, "dvm", s.hostName, "stopped")
+	if err := corrosion.InsertSnapshot(ctx, s.db, corrosion.SnapshotRecord{
+		VMName: "dvm", HostName: s.hostName, Name: "snap1", State: "ok", Type: "disk",
+	}); err != nil {
+		t.Fatalf("InsertSnapshot: %v", err)
+	}
+	return s, ctx
+}
+
+// A snapshot that vanishes between libvirt's lookup and its delete (a revert racing a
+// delete) surfaces a RAW golibvirt.Error — code ErrNoDomainSnapshot, arbitrary message.
+// The old substring check called that a hard failure and returned Internal, leaking the
+// corrosion record; the typed arm of lv.IsNotFound is what fixes it. "gone" contains
+// neither "not found" nor "no domain snapshot", so this test fails without that arm.
+func TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone(t *testing.T) {
+	s, ctx := deleteSnapshotServer(t, golibvirt.Error{
+		Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "gone",
+	})
+
+	if _, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{
+		VmName: "dvm", SnapshotName: "snap1",
+	}); err != nil {
+		t.Fatalf("DeleteSnapshot: want success for a typed not-found libvirt error, got %v", err)
+	}
+	snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1")
+	if snap != nil {
+		t.Errorf("corrosion record must be tombstoned, still present: %+v", snap)
+	}
+}
+
+// The two message shapes the hand-rolled check covered before the helper existed:
+// libvirt's own wording, and litevirt's `snapshot %q not found: %w` wrapper. These
+// passed before the change too — they are the no-regression guard.
+func TestDeleteSnapshot_MessageNotFoundTreatedAsAlreadyGone(t *testing.T) {
+	cases := []struct {
+		name   string
+		delErr error
+	}{
+		{"libvirt wording", errors.New("no domain snapshot with matching name 'snap1'")},
+		{"litevirt wrapper", fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom"))},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s, ctx := deleteSnapshotServer(t, tc.delErr)
+
+			if _, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{
+				VmName: "dvm", SnapshotName: "snap1",
+			}); err != nil {
+				t.Fatalf("DeleteSnapshot: want success for %v, got %v", tc.delErr, err)
+			}
+			snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1")
+			if snap != nil {
+				t.Errorf("corrosion record must be tombstoned, still present: %+v", snap)
+			}
+		})
+	}
+}
+
+// A libvirt failure that is NOT an absence must still fail the RPC and LEAVE the record
+// in place. Both assertions matter: the code pins that a real fault is not swallowed, and
+// the surviving record pins that the handler did not run the cleanup path anyway.
+func TestDeleteSnapshot_RealFailureStillPropagates(t *testing.T) {
+	s, ctx := deleteSnapshotServer(t, errors.New("internal error: qemu unexpectedly closed the monitor"))
+
+	_, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{
+		VmName: "dvm", SnapshotName: "snap1",
+	})
+	if c := status.Code(err); c != codes.Internal {
+		t.Fatalf("code = %v (err=%v), want Internal", c, err)
+	}
+	snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1")
+	if snap == nil {
+		t.Error("corrosion record must survive a real libvirt failure, it was tombstoned")
+	}
+}

internal/libvirt/checkpoint.go

@@ -13,7 +13,6 @@ package libvirt
 import (
 	"encoding/xml"
 	"fmt"
-	"strings"
 
 	golibvirt "github.com/digitalocean/go-libvirt"
 )
@@ -88,7 +87,7 @@ func (c *Client) DeleteCheckpoint(domain, checkpointName string, withChildren bo
 	}
 	cp, err := c.virt.DomainCheckpointLookupByName(dom, checkpointName, 0)
 	if err != nil {
-		if isNotFound(err) {
+		if IsNotFound(err) {
 			return nil // already gone — idempotent
 		}
 		return fmt.Errorf("lookup checkpoint %s: %w", checkpointName, err)
@@ -156,23 +155,3 @@ func buildBackupXML(diskTarget, parentCheckpoint, socket, scratch string) (strin
 	}
 	return string(b), nil
 }
-
-// isNotFound classifies a libvirt error as "object does not exist" so
-// existence probes don't conflate a missing checkpoint with a real fault.
-func isNotFound(err error) bool {
-	if err == nil {
-		return false
-	}
-	if e, ok := err.(golibvirt.Error); ok {
-		switch e.Code {
-		case uint32(golibvirt.ErrNoDomainCheckpoint),
-			uint32(golibvirt.ErrNoDomain),
-			uint32(golibvirt.ErrNoDomainSnapshot):
-			return true
-		}
-	}
-	msg := strings.ToLower(err.Error())
-	return strings.Contains(msg, "not found") ||
-		strings.Contains(msg, "no domain checkpoint") ||
-		strings.Contains(msg, "cannot find")
-}

internal/libvirt/errors.go

@@ -0,0 +1,35 @@
+package libvirt
+
+import (
+	"strings"
+
+	golibvirt "github.com/digitalocean/go-libvirt"
+)
+
+// IsNotFound classifies a libvirt error as "the object does not exist", so callers
+// can treat a delete/lookup of an already-gone domain, snapshot, or checkpoint as
+// success instead of conflating it with a real fault.
+//
+// Typed go-libvirt error codes are authoritative and checked first. The substring
+// fallback exists because litevirt wraps several libvirt calls with its own text
+// (e.g. snapshot.go's `snapshot %q not found: %w`), and because some paths surface
+// a message-only error with no code attached — a typed-only check would regress
+// those callers.
+func IsNotFound(err error) bool {
+	if err == nil {
+		return false
+	}
+	if e, ok := err.(golibvirt.Error); ok {
+		switch e.Code {
+		case uint32(golibvirt.ErrNoDomainCheckpoint),
+			uint32(golibvirt.ErrNoDomain),
+			uint32(golibvirt.ErrNoDomainSnapshot):
+			return true
+		}
+	}
+	msg := strings.ToLower(err.Error())
+	return strings.Contains(msg, "not found") ||
+		strings.Contains(msg, "no domain checkpoint") ||
+		strings.Contains(msg, "no domain snapshot") ||
+		strings.Contains(msg, "cannot find")
+}

internal/libvirt/errors_test.go

@@ -0,0 +1,78 @@
+package libvirt
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	golibvirt "github.com/digitalocean/go-libvirt"
+)
+
+// Each row exercises exactly ONE arm of IsNotFound: the typed rows carry an
+// "opaque" message that matches no substring, and the message rows carry no
+// libvirt error code. A row that satisfied both would not tell us which arm
+// answered, so it could not catch that arm being deleted.
+func TestIsNotFound(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil", nil, false},
+		{
+			"typed ErrNoDomain",
+			golibvirt.Error{Code: uint32(golibvirt.ErrNoDomain), Message: "opaque"},
+			true,
+		},
+		{
+			"typed ErrNoDomainSnapshot",
+			golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "opaque"},
+			true,
+		},
+		{
+			"typed ErrNoDomainCheckpoint",
+			golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainCheckpoint), Message: "opaque"},
+			true,
+		},
+		{
+			// The typed switch is a whitelist of absence codes, not "any typed error".
+			"typed ErrInternalError",
+			golibvirt.Error{Code: uint32(golibvirt.ErrInternalError), Message: "opaque"},
+			false,
+		},
+		{
+			// libvirt's real wording for a vanished snapshot, and the string
+			// grpcapi's DeleteSnapshot matched by hand before this helper existed.
+			"message no domain snapshot",
+			errors.New("no domain snapshot with matching name 'snap1'"),
+			true,
+		},
+		{
+			// litevirt's own wrapper text (snapshot.go's DeleteSnapshot lookup).
+			"message litevirt snapshot wrapper",
+			fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom")),
+			true,
+		},
+		{"message mixed case", errors.New("Domain Not Found"), true},
+		{
+			"message unrelated failure",
+			errors.New("internal error: qemu unexpectedly closed the monitor"),
+			false,
+		},
+		{
+			// Write-lock contention is a RETRY predicate, a different question from
+			// "does this object exist?" — snapshot.go keeps its own heuristics for it.
+			// This row exists so a future refactor cannot quietly merge the two.
+			"message write lock",
+			errors.New("Failed to get write lock"),
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := IsNotFound(tt.err); got != tt.want {
+				t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}