Let a Proxy finish deleting when its instance can no longer be reached

reconcileDelete asked Get first and dropped the finalizer only on
not-found, so a permanent Get retried forever: a KVM host removed from
the config, or a providerID the provider cannot resolve, left the Proxy
in Terminating with no way out. It now tries Delete anyway, which is
idempotent and can still finish where Get cannot converge, and releases
the object when that succeeds.

crawl.example.com/skip-cleanup abandons the instance for the cases that
stay stuck, such as revoked credentials. It is checked before the
provider lookup, so it also frees a Proxy whose provider is gone from
the config.

In the KVM provider, Delete over an unconfigured host is now a no-op
success, mirroring removeVolumes over a missing pool. Get keeps failing
permanently there on purpose: the live path reads not-found as "rebuild
it", so a typo in hosts must not look like a missing VM.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

README.md

@@ -164,6 +164,12 @@ Cloud-init from a Secret: the Secret **must** carry the label
 labelled Secrets, so an unlabelled one is invisible (the Proxy reports
 `CloudInitError`). Rotating the Secret's content triggers VM replacement.
 
+Stuck deletion: a Proxy stays `Terminating` while the operator keeps failing
+to clean its instance up — credentials revoked, a provider or a KVM host
+dropped from the config. Annotate it with
+`crawl.example.com/skip-cleanup: "true"` to abandon the instance and release
+the object. Whatever is still running then has to be removed by hand.
+
 ## Providers
 
 `Managed` proxies are provisioned by a provider — a small compute backend

api/v1alpha1/proxy_types.go

@@ -62,6 +62,15 @@ const (
 	// must be replaced.
 	AnnotationSpecHash = "crawl.example.com/spec-hash"
 
+	// AnnotationSkipCleanup abandons the provider-side instance: set it to
+	// "true" and deletion removes the finalizer without asking the provider
+	// to clean anything up. It is the escape hatch for a cleanup that cannot
+	// succeed — a provider dropped from the config, credentials revoked, a
+	// backend that answers permanently — where the object would otherwise
+	// stay in Terminating forever. The instance is left running and becomes
+	// the administrator's to remove.
+	AnnotationSkipCleanup = "crawl.example.com/skip-cleanup"
+
 	// LabelCloudInit must be set (to "true") on every Secret referenced by
 	// spec.cloudInit.secretRef: the manager's cache only holds Secrets
 	// carrying this label, so an unlabelled Secret is invisible to the

docs/architecture.md

@@ -489,6 +489,15 @@ they read as choices, not accidents. Chronological by build step.
   cluster-wide Secret read RBAC — an unrestricted cache would hold every
   Secret in scope). Unlabelled referenced Secrets are invisible by
   construction, surfacing as `CloudInitError`.
+- **Deletion never gives up on its own, but can be released by hand:** a
+  permanent cleanup failure keeps retrying with the finalizer on, because
+  dropping it automatically would leak a running instance on any provider
+  that answers "permission denied". Where `Get` fails permanently the
+  reconciler still calls `Delete`, since it is idempotent and may finish
+  where `Get` cannot converge. `crawl.example.com/skip-cleanup: "true"`
+  abandons the instance and releases the object; it is checked before the
+  provider lookup, so it also frees a Proxy whose provider is gone from the
+  config.
 - **Events RBAC from the plan is omitted:** nothing wires an
   EventRecorder in the prototype, and granting verbs nothing uses would
   be RBAC lint noise. Add the marker together with the recorder if events

docs/kvm-provider.md

@@ -264,6 +264,14 @@ The provider logs through controller-runtime's logger, named `kvm`, with
   cleaned when the same Proxy reconciles again (Delete removes volumes by
   name even with no domain), but a Proxy CR that is gone for good leaves
   them for `virsh vol-list` to find.
+- **A host removed from the config takes its VMs with it.** `Delete` for a
+  providerID naming an unconfigured host is a no-op success, so the Proxy
+  finishes deleting instead of pinning its finalizer, but the domain and its
+  volumes stay on the hypervisor and orphan GC cannot see them either (it
+  only sweeps configured hosts). Delete the Proxies before retiring a host,
+  or clean up afterwards with `virsh undefine --nvram` plus `virsh vol-delete`.
+  `Get` still fails permanently for such a providerID on purpose: a typo in
+  `hosts` must not look like a missing VM and trigger a rebuild elsewhere.
 - **The first image fetch is synchronous.** It holds a reconcile worker
   for the download (LAN speed, a few hundred MB, once per image per
   host). Other Proxies keep reconciling on the remaining workers.

docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md

@@ -0,0 +1,57 @@
+# Execution: fix the five code-review findings on `feat/kvm-provider`
+Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222-kvm-review-fixes.md)
+
+- [x] Step 0 — Save plan into the repo
+- [x] Step 1 — Unwedge deletion (finding 1)
+- [ ] Step 2 — Validate names that become identifiers (finding 2)
+- [ ] Step 3 — Read the SSH material lazily (finding 3)
+- [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5)
+- [ ] Step 5 — Validate, record, push
+
+## Step 0 — plan committed
+
+Copied out of `~/.claude/plans/` and committed alone before touching code, as
+the repo's plan rules require.
+
+## Step 1 — Unwedge deletion
+
+Three changes, in the order the bug unfolds.
+
+`reconcileDelete` calls `Get` first and only drops the finalizer when it
+answers not-found, so any permanent error there retried forever. It now
+switches on the class: a permanent `Get` still calls `Delete`, which is
+idempotent, and releases the object when that succeeds. Polling a `Get`
+that cannot converge was the wedge; `Delete` is the only call that can
+still make progress.
+
+`crawl.example.com/skip-cleanup: "true"` abandons the instance and removes
+the finalizer. It is checked **before** the provider lookup, which also
+frees a Proxy whose provider was deleted from the config — a second wedge
+sitting three lines above the reported one, with the same shape. No
+`EventRecorder` exists on the reconciler and none was added, so the
+abandonment is an Info log; `docs/architecture.md` already records that
+decision for the whole controller.
+
+In the KVM provider, `Delete` for a providerID naming an unconfigured host
+now logs and returns success. The precedent is two functions further down:
+`removeVolumes` treats a missing pool the same way, with a comment saying
+outright that refusing "would pin the Proxy's finalizer".
+
+**`Get` deliberately still fails permanently for that case.** Making it
+answer not-found would have been a one-line fix for the wedge, but the live
+reconcile path reads not-found as "the VM is gone" and rebuilds it
+elsewhere. A typo in `hosts` would then quietly duplicate every Proxy on
+that host, with the originals still running where GC cannot see them, since
+`ListByTag` only sweeps configured hosts. The asymmetry is now pinned by a
+test that asserts both halves at once.
+
+Worth noting: the review filed this as a KVM finding, but the fix is mostly
+in the controller, because KVM is the only provider that resolves a
+providerID segment against operator config at all. GCP and Kubernetes hand
+the lookup to the backend, which answers 404 for a retired zone or a deleted
+namespace, so they reach not-found and release on their own. Anything else
+that classifies permanently on the delete path — revoked RBAC on the
+Kubernetes provider is the realistic one — was equally stuck before this
+step and is what the annotation is really for. The KVM table case
+`"host not configured"` moved out of `TestDelete_errors` into its own named
+test, since it is no longer an error.

internal/controller/proxy_controller.go

@@ -273,17 +273,38 @@ func (r *ProxyReconciler) reconcileDelete(ctx context.Context, p *crawlv1alpha1.
 		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
 		return ctrl.Result{}, r.Update(ctx, p)
 	}
+	if p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] == "true" {
+		// Checked before the provider lookup so it also releases a Proxy whose
+		// provider is gone from the config. The instance is left running.
+		logf.FromContext(ctx).Info("skip-cleanup annotation set; abandoning the instance",
+			"providerID", p.Status.ProviderID, "provider", p.Spec.Provider)
+		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
+		return ctrl.Result{}, r.Update(ctx, p)
+	}
 	prov, ok := r.Providers[p.Spec.Provider]
 	if !ok {
 		return ctrl.Result{}, fmt.Errorf(
 			"provider %q is not configured; cannot clean up instance %s", p.Spec.Provider, p.Status.ProviderID)
 	}
 	_, err := prov.Get(ctx, p.Status.ProviderID)
-	if provider.Class(err) == provider.ErrNotFound {
+	switch {
+	case err == nil:
+	case provider.Class(err) == provider.ErrNotFound:
 		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
 		return ctrl.Result{}, r.Update(ctx, p)
-	}
+	case provider.Class(err) == provider.ErrPermanent:
-	if err != nil {
+		// Get will never answer for this instance — a host retired from the
+		// config, a providerID the provider can no longer resolve. Delete is
+		// idempotent and may still finish, so try it instead of polling a Get
+		// that cannot converge, and release the object when it does.
+		if delErr := prov.Delete(ctx, p.Status.ProviderID); delErr != nil {
+			return r.deletionFailure(errors.Join(err, delErr))
+		}
+		logf.FromContext(ctx).Info("instance unreachable but cleanup succeeded; removing the finalizer",
+			"providerID", p.Status.ProviderID, "error", err.Error())
+		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
+		return ctrl.Result{}, r.Update(ctx, p)
+	default:
 		return r.deletionFailure(err)
 	}
 	if err := prov.Delete(ctx, p.Status.ProviderID); err != nil {

internal/controller/reconcile_test.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"slices"
 	"testing"
 	"time"
 
@@ -78,6 +79,10 @@ func notFoundErr() error {
 	return provider.Wrap(provider.ErrNotFound, "get", "stub", "some-id", nil)
 }
 
+func permanentErr() error {
+	return provider.Wrap(provider.ErrPermanent, "get", "stub", "some-id", nil)
+}
+
 func testScheme(t *testing.T) *runtime.Scheme {
 	t.Helper()
 	s := runtime.NewScheme()
@@ -125,6 +130,15 @@ func withSpecHashAnnotation(hash string) func(*crawlv1alpha1.Proxy) {
 	}
 }
 
+func withSkipCleanup() func(*crawlv1alpha1.Proxy) {
+	return func(p *crawlv1alpha1.Proxy) {
+		if p.Annotations == nil {
+			p.Annotations = map[string]string{}
+		}
+		p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] = "true"
+	}
+}
+
 func deleting() func(*crawlv1alpha1.Proxy) {
 	return func(p *crawlv1alpha1.Proxy) {
 		now := metav1.Now()
@@ -375,6 +389,59 @@ func TestReconcile_actionTable(t *testing.T) {
 				assertProxyGone(t, r)
 			},
 		},
+		{
+			// A permanent Get (a retired KVM host, a providerID the provider
+			// cannot resolve) used to retry forever with the finalizer on.
+			name:       "deletion with an unreachable instance still deletes and releases",
+			proxy:      managedProxy(deleting(), withProviderID("stub-id-1")),
+			stub:       &stubProvider{getErr: permanentErr()},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) {
+				assertProxyGone(t, r)
+				if stub.deleteCalls != 1 {
+					t.Errorf("deleteCalls = %d, want 1 — Delete must be tried when Get cannot answer", stub.deleteCalls)
+				}
+			},
+		},
+		{
+			name:  "deletion keeps the finalizer when cleanup itself fails",
+			proxy: managedProxy(deleting(), withProviderID("stub-id-1")),
+			stub: &stubProvider{
+				getErr:    permanentErr(),
+				deleteErr: provider.Wrap(provider.ErrPermanent, "delete", "stub", "stub-id-1", nil),
+			},
+			wantResult: ctrl.Result{},
+			wantErr:    true,
+			verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) {
+				// getProxy fails the test if the object is gone, which is what
+				// finalizer removal would have caused.
+				if p := getProxy(t, r); !slices.Contains(p.Finalizers, crawlv1alpha1.FinalizerName) {
+					t.Error("finalizer removed although cleanup failed; the instance would leak silently")
+				}
+			},
+		},
+		{
+			name:       "skip-cleanup annotation abandons the instance",
+			proxy:      managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup()),
+			stub:       &stubProvider{getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateRunning}},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) {
+				assertProxyGone(t, r)
+				if stub.getCalls != 0 || stub.deleteCalls != 0 {
+					t.Errorf("getCalls/deleteCalls = %d/%d, want 0/0 — the provider must not be asked", stub.getCalls, stub.deleteCalls)
+				}
+			},
+		},
+		{
+			name: "skip-cleanup annotation releases a proxy whose provider is gone",
+			proxy: managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup(),
+				func(p *crawlv1alpha1.Proxy) { p.Spec.Provider = "retired" }),
+			stub:       &stubProvider{},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) {
+				assertProxyGone(t, r)
+			},
+		},
 		{
 			name:  "deletion deletes the instance and polls",
 			proxy: managedProxy(deleting(), withProviderID("stub-id-1")),

internal/provider/kvm/kvm.go

@@ -451,7 +451,8 @@ func mapState(s libvirt.DomainState, ip string) provider.InstanceState {
 // domain is not destroyed again, a missing domain is not undefined, and
 // the per-VM volumes are removed by name even when the domain is already
 // gone, so a crash between volume creation and define is cleaned up on the
-// next reconcile.
+// next reconcile. A providerID naming a host that is no longer configured
+// is a no-op success rather than an error.
 func (p *Provider) Delete(ctx context.Context, providerID string) error {
 	hostName, name, err := parseProviderID(providerID)
 	if err != nil {
@@ -459,7 +460,14 @@ func (p *Provider) Delete(ctx context.Context, providerID string) error {
 	}
 	host, err := p.host(hostName)
 	if err != nil {
-		return p.wrapErr("delete", providerID, err)
+		// The host is no longer configured, so this operator has no way to
+		// reach the domain and nothing left to clean. Same reasoning as
+		// removeVolumes over a missing pool: refusing would pin the Proxy's
+		// finalizer. Whatever is still running there is the admin's to remove,
+		// and orphan GC cannot see it either — see docs/kvm-provider.md.
+		p.logger(ctx).Info("host is not configured; delete has nothing to clean up",
+			"host", hostName, "providerID", providerID)
+		return nil
 	}
 	log := p.logger(ctx).WithValues("host", host.Name, "name", name)
 	err = p.hosts.call(ctx, log, host, func(api libvirtAPI) error {

internal/provider/kvm/kvm_test.go

@@ -726,6 +726,29 @@ func TestDelete_missingPoolIsNotAnError(t *testing.T) {
 	}
 }
 
+// A host dropped from the config leaves the operator no way to reach the
+// domain, so Delete has nothing left to clean. Failing instead would pin
+// the Proxy's finalizer forever, since the reconciler's delete path cannot
+// make progress without a Delete that can succeed.
+func TestDelete_unconfiguredHostIsNotAnError(t *testing.T) {
+	t.Parallel()
+	api := readyHost()
+	managedDomain(t, api, testName, testUID, true, nil)
+	p := newTestProvider(hostFakes{"nas1": api}, "nas1")
+
+	if err := p.Delete(context.Background(), "hosts/nas9/domains/"+testName); err != nil {
+		t.Fatalf("Delete over an unconfigured host: %v", err)
+	}
+	if api.domain(testName) == nil {
+		t.Error("domain on the configured host was touched; only the named host's state may change")
+	}
+	// Get still refuses, so the live path cannot mistake a config typo for a
+	// missing VM and recreate it somewhere else.
+	if _, err := p.Get(context.Background(), "hosts/nas9/domains/"+testName); provider.Class(err) != provider.ErrPermanent {
+		t.Errorf("Get class = %v, want ErrPermanent", provider.Class(err))
+	}
+}
+
 func TestDelete_errors(t *testing.T) {
 	t.Parallel()
 	cases := []struct {
@@ -735,7 +758,6 @@ func TestDelete_errors(t *testing.T) {
 		class error
 	}{
 		{"malformed id", "nope", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent},
-		{"host not configured", "hosts/nas9/domains/x", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent},
 		{"host unreachable", "hosts/nas1/domains/x", func(*testing.T) hostFakes { return hostFakes{} }, provider.ErrTransient},
 		{
 			"undefine fails", "hosts/nas1/domains/" + testName,