fix(backup): bound the on-delete finalizer so a failing snapshot can't make an instance undeletable (#94)

Fixes #93.

## Problem
`BackupReconciler.HandleDeletion` requeued **forever** when the final
backup Job failed (or hung): it recorded the failure and requeued every
30s but never released the `hermes.agent/backup-on-delete` finalizer. So
a `HermesInstance` with `spec.backup.onDelete` and bad/unreachable S3
creds became **permanently undeletable** (stuck `Terminating`, which
also wedges its namespace). This is what hung the conformance
`backup-enabled` cleanup.

## Fix
Bound the deletion block with a grace window (`finalBackupDeadline`,
**30 min** from `deletionTimestamp`). Once it elapses, the operator
gives up:
- records `status.backup.lastFailureReason =
FinalBackupDeadlineExceeded`,
- emits a `Warning FinalBackupAbandoned` event (surfacing the data-loss
risk),
- **releases the finalizer so deletion proceeds.**

The `hermes.agent/skip-final-backup=true` annotation remains the
explicit, immediate escape hatch. Within the window the behavior is
unchanged (attempt the snapshot, hold the finalizer).

## Tests
Fake-client unit tests: deadline exceeded → finalizer released (instance
GC'd); within the window → finalizer held + final backup Job created.
Full controller envtest suite + `golangci-lint` green. Docs updated.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: stubbi

docs/backup-restore.md

@@ -80,9 +80,13 @@ When `spec.backup.onDelete = true`, the operator adds the `hermes.agent/backup-o
 3. When the Job succeeds, the finalizer is removed via **`r.Patch`** (`client.MergeFrom`), not `r.Update`. This is critical: `r.Update` bumps `metadata.generation` and replaces the pod on the next reconcile. Lesson #437 from openclaw-operator.
 4. Kubernetes GC'es the CR + cascades to owned resources.
 
+### Bounded grace window
+
+A failing or stuck final backup must not make the instance **permanently undeletable** (#93). The operator blocks deletion for at most a grace window (`finalBackupDeadline`, **30 minutes** from `deletionTimestamp`); after that it gives up — records `status.backup.lastFailureReason = FinalBackupDeadlineExceeded`, emits a `Warning FinalBackupAbandoned` event, and **releases the finalizer so deletion proceeds**. The final snapshot will not have been taken, so PVC data may be lost — the event makes this visible for post-mortems.
+
 ### Skipping the final backup
 
-If the final backup is hanging or the bucket is unreachable:
+To delete immediately (without waiting out the grace window) when the final backup is hanging or the bucket is unreachable:
 
 ```bash
 kubectl annotate hermesinstance/<name> hermes.agent/skip-final-backup=true --overwrite
@@ -100,7 +104,7 @@ A second CronJob (`<name>-backup-prune`) runs daily at 04:17 UTC and runs `resti
 |---|---|---|
 | Final backup Job fails with `S3 credentials secret missing key`. | Secret missing `S3_ACCESS_KEY_ID` or `S3_SECRET_ACCESS_KEY`. | Patch the Secret. The CR stays in deletion until the next reconcile picks up the new Secret. |
 | Scheduled CronJob runs but no snapshot appears. | Likely a network policy blocking egress to S3 endpoint. | Add an egress rule under `spec.networking.egress`. |
-| `kubectl delete` hangs forever. | Final backup Job failing repeatedly. | `kubectl describe job <name>-backup-final` for logs; either fix or use the skip annotation. |
+| `kubectl delete` blocks (up to the 30-minute grace window, then auto-releases). | Final backup Job failing repeatedly. | `kubectl describe job <name>-backup-final` for logs; fix the cause, or use the skip annotation to release immediately. After the grace window the finalizer auto-releases (`FinalBackupAbandoned` event) and deletion proceeds without a final snapshot. |
 | `status.restoredFrom` stays empty after `init-restore` exited 0. | Pod restarted before the operator observed the terminated state. | Force reconcile: `kubectl annotate hermesinstance <name> poke=$(date +%s) --overwrite`. |
 
 ## API stability

internal/controller/backup.go

@@ -32,6 +32,15 @@ type BackupReconciler struct {
 	Recorder record.EventRecorder
 }
 
+// finalBackupDeadline bounds how long deletion may block on the on-delete final
+// backup. A failed or stuck snapshot must not make a HermesInstance permanently
+// undeletable (#93): once this grace window from .metadata.deletionTimestamp
+// elapses, HandleDeletion gives up — it records the failure, emits a loud
+// warning, and releases the finalizer so deletion proceeds. The
+// skip-final-backup annotation remains the explicit, immediate escape hatch.
+// A var (not const) so tests can shrink it.
+var finalBackupDeadline = 30 * time.Minute
+
 // EnsureFinalizer adds the backup-on-delete finalizer when spec.backup.onDelete is true.
 //
 // CRITICAL: lesson #437: finalizer mutation uses r.Patch(ctx, inst, client.MergeFrom(original)),
@@ -152,6 +161,24 @@ func (b *BackupReconciler) HandleDeletion(ctx context.Context, inst *hermesv1.He
 		return ctrl.Result{}, false, nil
 	}
 
+	// Bound the deletion block (#93): if the final backup hasn't succeeded within
+	// finalBackupDeadline of the deletion request, give up so the instance can be
+	// deleted instead of hanging forever on a failing/stuck snapshot Job.
+	if dt := inst.DeletionTimestamp; dt != nil && time.Since(dt.Time) > finalBackupDeadline {
+		b.Recorder.Eventf(inst, corev1.EventTypeWarning, "FinalBackupAbandoned",
+			"Final backup did not complete within %s of deletion; releasing the finalizer so the instance can be deleted. The final snapshot was NOT taken and PVC data may be lost. Inspect Job %q, or set annotation %q=true to skip explicitly next time.",
+			finalBackupDeadline, FinalBackupJobName(inst), hermesv1.AnnotationSkipFinalBackup)
+		now := metav1.Now()
+		inst.Status.Backup.LastFailureTime = &now
+		inst.Status.Backup.LastFailureReason = "FinalBackupDeadlineExceeded"
+		// Best-effort status; deletion must proceed even if this write races the GC.
+		_ = b.Status().Update(ctx, inst)
+		if err := b.RemoveFinalizer(ctx, inst); err != nil {
+			return ctrl.Result{}, true, err
+		}
+		return ctrl.Result{}, false, nil
+	}
+
 	jobName := FinalBackupJobName(inst)
 	job, err := GetJob(ctx, b.Client, jobName, inst.Namespace)
 	if err != nil {

internal/controller/backup_deadline_test.go

@@ -0,0 +1,116 @@
+package controller
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	batchv1 "k8s.io/api/batch/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	hermesv1 "github.com/paperclipinc/hermes-operator/api/v1"
+)
+
+// onDeleteInstance returns a HermesInstance that holds the backup-on-delete
+// finalizer and has spec.backup.onDelete + a (placeholder) S3 target.
+func onDeleteInstance() *hermesv1.HermesInstance {
+	return &hermesv1.HermesInstance{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "dl",
+			Namespace:  "default",
+			Finalizers: []string{hermesv1.FinalizerBackupOnDelete},
+		},
+		Spec: hermesv1.HermesInstanceSpec{
+			Backup: hermesv1.BackupSpec{
+				OnDelete: true,
+				S3: &hermesv1.BackupS3Spec{
+					Bucket:               "b",
+					Endpoint:             "https://s3.example.com",
+					CredentialsSecretRef: hermesv1.LocalObjectReference{Name: "creds"},
+				},
+			},
+		},
+	}
+}
+
+func backupTestScheme(t *testing.T) *runtime.Scheme {
+	t.Helper()
+	sch := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(sch))
+	require.NoError(t, hermesv1.AddToScheme(sch))
+	return sch
+}
+
+// deletingClient builds a fake client holding inst, then Deletes it so it carries
+// a deletionTimestamp (the finalizer keeps it around), and returns the live copy.
+func deletingClient(t *testing.T, sch *runtime.Scheme, inst *hermesv1.HermesInstance) (client.Client, *hermesv1.HermesInstance) {
+	t.Helper()
+	cl := fake.NewClientBuilder().
+		WithScheme(sch).
+		WithObjects(inst).
+		WithStatusSubresource(&hermesv1.HermesInstance{}).
+		Build()
+	ctx := context.Background()
+	require.NoError(t, cl.Delete(ctx, inst))
+	got := &hermesv1.HermesInstance{}
+	require.NoError(t, cl.Get(ctx, client.ObjectKeyFromObject(inst), got))
+	require.NotNil(t, got.DeletionTimestamp, "expected deletionTimestamp after Delete")
+	return cl, got
+}
+
+// #93: once the grace window elapses, a failing/stuck final backup must not keep
+// the instance undeletable — HandleDeletion releases the finalizer.
+func TestHandleDeletion_DeadlineReleasesFinalizer(t *testing.T) {
+	orig := finalBackupDeadline
+	finalBackupDeadline = 0 // grace window already exceeded
+	defer func() { finalBackupDeadline = orig }()
+
+	sch := backupTestScheme(t)
+	cl, inst := deletingClient(t, sch, onDeleteInstance())
+
+	b := &BackupReconciler{Client: cl, Scheme: sch, Recorder: record.NewFakeRecorder(10)}
+	_, held, err := b.HandleDeletion(context.Background(), inst)
+	require.NoError(t, err)
+	assert.False(t, held, "deadline exceeded: finalizer must be released so deletion proceeds")
+
+	// Finalizer gone -> the fake client garbage-collects the terminating object.
+	after := &hermesv1.HermesInstance{}
+	if err := cl.Get(context.Background(), client.ObjectKeyFromObject(inst), after); err == nil {
+		assert.False(t, controllerutil.ContainsFinalizer(after, hermesv1.FinalizerBackupOnDelete),
+			"finalizer should be removed after the deadline give-up")
+	}
+}
+
+// Within the grace window the finalizer is still held and the final backup Job is
+// started — deletion is intentionally blocked while the snapshot is attempted.
+func TestHandleDeletion_WithinDeadlineHoldsFinalizer(t *testing.T) {
+	orig := finalBackupDeadline
+	finalBackupDeadline = time.Hour // plenty of grace
+	defer func() { finalBackupDeadline = orig }()
+
+	sch := backupTestScheme(t)
+	cl, inst := deletingClient(t, sch, onDeleteInstance())
+
+	b := &BackupReconciler{Client: cl, Scheme: sch, Recorder: record.NewFakeRecorder(10)}
+	_, held, err := b.HandleDeletion(context.Background(), inst)
+	require.NoError(t, err)
+	assert.True(t, held, "within the grace window the finalizer must still be held")
+
+	// The final backup Job should have been created, and the finalizer kept.
+	job := &batchv1.Job{}
+	require.NoError(t, cl.Get(context.Background(),
+		client.ObjectKey{Namespace: inst.Namespace, Name: FinalBackupJobName(inst)}, job))
+
+	after := &hermesv1.HermesInstance{}
+	require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(inst), after))
+	assert.True(t, controllerutil.ContainsFinalizer(after, hermesv1.FinalizerBackupOnDelete),
+		"finalizer must remain while the backup is in flight")
+}