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

[colonelpanik]

Goal

Export a NotFound-classifying helper from internal/libvirt (generalizing checkpoint.go's existing unexported isNotFound, which already matches typed golibvirt.Error codes ErrNoDomainCheckpoint/ErrNoDomain/ErrNoDomainSnapshot before falling back to substring matching) and use it in internal/grpcapi/snapshot.go:395 instead of the current strings.Contains(delErr.Error(), "not found") / "no domain snapshot" checks.

Plan

Plan: export a NotFound-classifying helper from internal/libvirt

Goal

Promote internal/libvirt/checkpoint.go's unexported isNotFound to an exported
libvirt.IsNotFound, and use it at internal/grpcapi/snapshot.go:395 in place of the
hand-rolled

strings.Contains(delErr.Error(), "not found") || strings.Contains(delErr.Error(), "no domain snapshot")

so the "libvirt snapshot metadata is already gone — still clean up the corrosion record"
path is decided by one shared classifier that checks typed go-libvirt error codes
first and only then falls back to substring matching.

---

What exists today (read this before judging the diff)

1. The helper — internal/libvirt/checkpoint.go:160-178

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")
}

One call site: DeleteCheckpoint at checkpoint.go:91, where a not-found lookup returns
nil (idempotent delete).

golibvirt.Error is a value type (Code uint32, Message string) whose Error()
returns Message verbatim — go-libvirt's own checkError uses the same err.(Error)
assertion shape, so the direct type assertion here matches upstream idiom.

2. The call site — internal/grpcapi/snapshot.go:381-400

DeleteSnapshot calls FlattenSnapshot (running VM, last snapshot) or
DeleteSnapshot, falling back from the former to the latter. If the resulting delErr
looks like "already gone", it logs and continues so the corrosion record + vmstate file +
firmware sidecar are still cleaned up; otherwise it returns codes.Internal. The comment
at :391-393 is explicit that a prior revert consumes the libvirt snapshot metadata, so
this is a normal, expected state — not an error path.

3. Three facts that decide whether this change is a regression

(a) The helper's substring list does not contain "no domain snapshot".
This is the single most important detail. A naive swap to isNotFound would drop the
no domain snapshot arm that grpcapi added, which is exactly libvirt's message for the
case the :391-393 comment describes. The exported helper must gain
"no domain snapshot" in its fallback list. Without it this change is a silent
regression on its own motivating case.

(b) Today's grpcapi check usually matches on litevirt's own wrapper text, not
libvirt's. internal/libvirt/snapshot.go:293 wraps the snapshot lookup as
fmt.Errorf("snapshot %q not found: %w", snapshotName, err) — so every
DomainSnapshotLookupByName failure, whatever its cause, already contains "not found"
and is already classified as "already gone" today. The exported helper does not change
that (its "not found" arm matches the same string). Reviewers evaluating "is the new
classifier too permissive?" should note the existing one is permissive in precisely this
way; nothing here widens that.

(c) The typed path is genuinely reachable at this call site.
internal/libvirt/snapshot.go:296 returns c.virt.DomainSnapshotDelete(...)'s error
unwrapped — a raw golibvirt.Error. If a snapshot vanishes between lookup and delete
(a revert racing a delete), the error arrives with Code == ErrNoDomainSnapshot and an
arbitrary message. Today's substring check would classify that as a hard failure and
return Internal, leaking the corrosion record. The exported helper fixes it. This is the
one real behavioural win of the change, and the test below pins it.

---

Files to change

1. internal/libvirt/errors.go — new file

Holds the exported helper. A shared, cross-package classifier does not belong in
checkpoint.go (whose package doc is about dirty-bitmap backup); a dedicated file makes
it discoverable.

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") || // NEW — see (a) above
		strings.Contains(msg, "cannot find")
}

The body is isNotFound verbatim plus the one new "no domain snapshot" arm.

2. internal/libvirt/checkpoint.go — delete isNotFound, update its caller

• Remove lines 160-178.
• :91 becomes if IsNotFound(err) {.
• Drop "strings" from the import block. strings is used only inside
  isNotFound in this file (verified: grep -n 'strings\.' internal/libvirt/checkpoint.go
  hits 174-177 and nothing else). golibvirt stays — it is still used at :96 and :98.
  Missing this is a build break.

3. internal/grpcapi/snapshot.go — use the helper

• :395 becomes:

  if lv.IsNotFound(delErr) {

  The package is already imported as lv (internal/libvirt, used at :413 for
  lv.SnapshotFirmwareBundlePath). No new import.
• Drop "strings" from the import block. Line 395 is the file's only use of
  strings (verified). Missing this is a build break.
• Leave the surrounding comment at :391-393 as-is; it still describes exactly why the
  branch exists.

---

Exact behavioural delta

Call site	Before	After	Net
checkpoint.go:91 (DeleteCheckpoint)	typed codes + not found / no domain checkpoint / cannot find	same + no domain snapshot	A checkpoint lookup failing with a snapshot-shaped message is now idempotent. ErrNoDomainSnapshot is already in the typed list, so this is internally consistent; DomainCheckpointLookupByName cannot realistically produce that text anyway.
grpcapi/snapshot.go:395	case-sensitive not found / no domain snapshot	typed codes first, then case-insensitive not found / no domain snapshot / no domain checkpoint / cannot find	Gains the typed path (fact (c) — the actual fix). Gains cannot find, no domain checkpoint, and case-insensitivity.

The one risk worth naming. At the grpcapi site the accepted set grows by
"cannot find". A genuine DomainSnapshotDelete failure whose message happened to
contain that phrase would now be swallowed: the corrosion record is tombstoned while the
libvirt snapshot metadata survives, leaving orphaned metadata (no data loss — the disk
overlay and backing chain are untouched, and virsh snapshot-delete clears it).

Assessment: acceptable. libvirt's cannot find messages are emulator/chardev/statistics
lookups, not snapshot-delete failures; and per fact (b) the pre-existing check already
classifies every lookup failure as not-found via litevirt's own wrapper, so this is not a
new class of over-broad classification. The alternative — a second, narrower classifier —
defeats the point of having one shared helper.

Deliberately not changed

• err.(golibvirt.Error) stays a direct type assertion, not errors.As. errors.As
  would be strictly broader (it would also catch a typed error wrapped by fmt.Errorf)
  and would not regress the fallback, so it is safe — but it is a behaviour change,
  whereas everything else here except the no domain snapshot arm is a pure move. Keeping
  it lets a reviewer verify "same logic, new home" by inspection. Worth a follow-up commit;
  not worth entangling with this one. (It is also not needed for correctness today: the
  raw typed error at internal/libvirt/snapshot.go:296 is unwrapped, and the wrapped
  lookup error at :293 is caught by the "not found" arm.)
• internal/libvirt/snapshot.go:194, 540, 558 — the "lock" / "already" retry
  heuristics. These are QEMU/virtlockd write-lock retry predicates with no corresponding
  typed go-libvirt code; classifying "should I retry this start/restore/resume?" is a
  different question from "does this object exist?". Out of scope by constraint, and the
  unit test below pins that IsNotFound("Failed to get write lock") == false so a future
  refactor cannot quietly merge the two.
• internal/storage/iscsi.go:37, internal/lb/demote.go:430,
  internal/grpcapi/containers.go — none of these classify a libvirt error
  (iscsiadm stdout, load-balancer state, container runtime). Not candidates.

---

Tests

A. internal/libvirt/errors_test.go — new, package libvirt

There is currently no unit test for isNotFound anywhere. Table test for
IsNotFound:

Input	Want	Pins
nil	false	nil guard
golibvirt.Error{Code: uint32(golibvirt.ErrNoDomain), Message: "opaque"}	true	typed branch — message deliberately matches no substring
golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "opaque"}	true	typed branch
golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainCheckpoint), Message: "opaque"}	true	typed branch
golibvirt.Error{Code: uint32(golibvirt.ErrInternalError), Message: "opaque"}	false	typed branch is a whitelist, not "any typed error"
errors.New("no domain snapshot with matching name 'snap1'")	true	the new arm — libvirt's real message, and the string grpcapi:395 matched today
fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom"))	true	litevirt's own wrapper (internal/libvirt/snapshot.go:293)
errors.New("Domain Not Found")	true	case-insensitivity
errors.New("internal error: qemu unexpectedly closed the monitor")	false	negative control
errors.New("Failed to get write lock")	false	guards the constraint — lock errors are never not-found

Message-only cases must use messages that do not also satisfy the typed branch, and
typed cases must use messages that do not also satisfy the substring branch, so each
row exercises exactly one arm.

B. internal/grpcapi/snapshot_test.go — extend the existing file

This file already exists (179 lines, 9 tests, including TestDeleteSnapshot_VMNotFound
and TestDeleteSnapshot_WrongHost). Append the stub and the three tests below; do not
rewrite the file. The existing imports need errors, fmt, libvirtfake, and
golibvirt added; codes/status/corrosion/pb are already there.

Test doubles: s.virt is an unexported LibvirtBackend field, settable directly from a
same-package test (internal/grpcapi/vtpm_lifecycle_test.go:29 does s.virt = fake), and
the repo already has the embed-and-override stub idiom (recordingVirt in
dualrun_test.go:19). Add:

// 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 }

Setup for each case:

• s := testServer(t); s.dataDir = t.TempDir()
• ctx := adminCtx() — and pass it to every handler call. DeleteSnapshot opens with
  requirePermPrecheck(ctx, "operator") (snapshot.go:340), which returns
  codes.Unauthenticated outright when callerUsername(ctx) is empty
  (internal/grpcapi/auth.go:711-714), and then RequirePerm(…, "snapshot.delete", "operator") at :347. A bare context.Background() would fail at the first line and
  the fake backend — and therefore the classifier under test — would never be reached,
  making all three tests vacuous. testServer(t) leaves authEngine nil, so the
  precheck falls through to RequireRole(ctx, "operator") and adminCtx()'s admin role
  satisfies it; this is the same setup vtpm_lifecycle_test.go:117 already uses to call
  DeleteSnapshot successfully.
• insert the VM with state "stopped" (so flatten at snapshot.go:379 is false and
  only DeleteSnapshot is called — one code path, no flatten fallback to reason about)
• insert a matching corrosion.SnapshotRecord
• s.virt = &deleteErrVirt{Fake: libvirtfake.New(), delErr: …}

Because an auth failure and a "record survived" assertion can look alike, each test should
assert the specific expected code (codes.Internal for case 3) rather than just
err != nil — an Unauthenticated regression must not be able to pass as a failure case.

1. TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone — the headline test.
   delErr = golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "gone"}.
   Assert the RPC returns no error and corrosion.GetSnapshot returns nil (record
   tombstoned). "gone" contains neither "not found" nor "no domain snapshot", so
   this test fails on main with codes.Internal — it is the one that proves the
   change did something. (It costs the grpcapi test binary a direct go-libvirt import;
   the package already depends on it transitively through internal/libvirt, so this
   does not contradict the LibvirtBackend doc comment's intent — no libvirt socket
   client is constructed.)

2. TestDeleteSnapshot_MessageNotFoundTreatedAsAlreadyGone — sub-tests over
   errors.New("no domain snapshot with matching name 'snap1'") and
   fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom")). Same
   assertions. These pass before and after; they are the no-regression guard for the two
   arms the old check covered.

3. TestDeleteSnapshot_RealFailureStillPropagates —
   delErr = errors.New("internal error: qemu unexpectedly closed the monitor").
   Assert status.Code(err) == codes.Internal and that the corrosion record is still
   present. The second assertion is what makes this test non-vacuous: without it, a
   classifier that returns true for everything still passes.

Mutation verification (required by CLAUDE.md — run each, confirm red, restore)

Mutation	Must go red
Delete the typed switch from IsNotFound	libvirt typed rows; TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone
Remove the "no domain snapshot" arm	libvirt raw-message row; TestDeleteSnapshot_MessageNotFoundTreatedAsAlreadyGone
IsNotFound returns true unconditionally	TestDeleteSnapshot_RealFailureStillPropagates; libvirt negative rows
Revert grpcapi/snapshot.go:395 to the old strings.Contains pair	TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone — proves the call site really routes through the helper

No fleet-tier test is warranted: this is package-local error classification with no
multi-node failure mode, and tests/fleet/ has no snapshot-delete coverage to extend.
internal/libvirt/checkpoint_integration_test.go is behind the libvirt_integration
build tag and never runs in CI — do not put anything load-bearing there.

---

Verification

go build ./... && go vet ./...          # catches both dropped-import footguns
go test ./internal/libvirt/ ./internal/grpcapi/
go test ./...
make ci-guards

make ci-guards should be a clean no-op for this change: no schema change
(check-schema-bump.sh), no SQL builder (stmtshapecheck), no state write
(writecheck). The docs triangulation guard
(cmd/litevirt/docs_triangulation_test.go) is also unaffected — IsNotFound is an
internal Go symbol, not a command, config key, or litevirt_* identifier, so no
documentation is owed.

Suggested commit: refactor(libvirt): export IsNotFound and use it for snapshot delete.

---

Reviewer checklist

• "strings" removed from both internal/libvirt/checkpoint.go and
  internal/grpcapi/snapshot.go — each file's only use of it is being deleted.
• "no domain snapshot" is in the exported helper's fallback list. Without it the
  change regresses the exact case grpcapi/snapshot.go:391-393 exists for.
• The typed-code switch still runs before the substring fallback, and the fallback
  is still reached when the assertion fails.
• internal/libvirt/snapshot.go:194, 540, 558 are untouched.
• TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone fails when checked out
  against main's snapshot.go:395 — if it passes on both, it is not testing the
  change.
• TestDeleteSnapshot_RealFailureStillPropagates asserts the corrosion record
  survives, not just the gRPC code.
• Every new handler call passes adminCtx(). A context.Background() call returns
  Unauthenticated before s.virt is touched — the test would pass for the wrong
  reason (or fail as if the classifier were broken).
• The new tests were appended to internal/grpcapi/snapshot_test.go; the nine
  pre-existing tests in it are still present.

---

Deviations from this plan, as implemented

The plan was accurate on every concrete claim — line numbers, golibvirt.Error being a
value type, the snapshot %q not found: %w wrapper at internal/libvirt/snapshot.go:293,
the unwrapped DomainSnapshotDelete return at :296, strings being each file's only
remaining use, and testServer(t) + adminCtx() reaching the backend. Two cosmetic
departures:

1. The three grpcapi tests share a deleteSnapshotServer(t, delErr) helper rather
   than repeating the plan's five setup bullets three times. It does exactly what the plan
   listed (temp dataDir, adminCtx(), a stopped dvm on the local host, a matching
   SnapshotRecord, deleteErrVirt as s.virt). Because it returns the context, the
   test file's import list gained context on top of the errors / fmt /
   libvirtfake / golibvirt the plan enumerated.
2. The // NEW — see (a) above marker on the "no domain snapshot" arm was not
   shipped. It refers to this document, which the committed code cannot; the arm reads
   as a peer of the other three.

All four mutations were run and each went red as predicted — including
TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone failing under mutation 4 (call site
reverted to the strings.Contains pair with the helper left intact), which is the one that
proves the call site routes through the helper. Full go build ./..., go vet ./...,
go test ./... and make ci-guards are clean; ci-guards was a no-op as predicted.

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.