fix stuck finalizer on delete

CLAUDE.md

@@ -206,24 +206,24 @@ Both RestateCluster and RestateDeployment use finalizers for cleanup:
 
 ### Restate Invocation Lifecycle and Deployment Status
 
-**Important**: The operator aligns with Restate's invocation retention model when determining deployment activity:
+**Important**: `list_deployments` (`src/controllers/restatedeployment/controller.rs`) queries the admin API for two independent signals per registered deployment, returning them as a `DeploymentState`:
 
-- **Invocation retention**: Completed invocations remain in `sys_invocation_status` for 24 hours (default) before automatic purging
-- **Deployment status**: A deployment is considered "active" if it has ANY invocation in `sys_invocation_status`, including completed ones
-- **Cleanup timing**: Configurations tied to "active" deployments are retained until Restate purges the invocations
-- **Manual override**: Use `restate invocations purge <id>` to immediately purge completed invocations for testing
+- **`latest_endpoint`**: the deployment serves the latest revision of at least one service (`sys_service`)
+- **`has_pinned_invocations`**: at least one *non-completed* invocation is pinned to it (`sys_invocation_status WHERE status != 'completed'`)
 
-**Deployment states** (`restate deployments list`):
-- `Active`: Has latest service revision
-- `Draining`: Superseded but has pinned invocations (including completed)
-- `Drained`: Superseded and all invocations purged (active_inv == 0)
+Only one of them is a reason to wait, and which one depends on whether the RestateDeployment is being deleted:
+
+- **Normal reconcile**: either signal makes a version "active" — it is kept, and any pending removal timer is reset.
+- **Being deleted** (`deletion_timestamp` set): only `has_pinned_invocations` blocks. Nothing can supersede the latest endpoint once the object is going away, so waiting on it would wedge the finalizer forever (issue #172). A version with live pinned invocations is held for `spec.restate.drainDelaySeconds` and then force-deregistered; `spec.revisionHistoryLimit` is bypassed so every version is actually deregistered before the finalizer is released.
+
+Completed invocations do **not** keep a deployment alive — the operator does not wait for Restate's 24h retention purge (changed in #71). Note this differs from what `restate deployments list` reports: it shows a superseded deployment as `Draining` while completed invocations are still retained, and `Drained` only once they are purged.
 
 **SQL tables**:
-- `sys_invocation_status`: ALL invocations including completed (used by operator's `list_deployments` query)
-- `sys_invocation`: Same content as sys_invocation_status
-- Both tables include a `status` column to filter by invocation state
+- `sys_service`: current service revisions, one row per service
+- `sys_invocation_status`: ALL invocations including completed; the `status` column is what the operator filters on
+- `sys_invocation`: same content as `sys_invocation_status`
 
-The operator's cleanup logic intentionally waits for Restate's invocation purge before considering a deployment truly inactive, ensuring Configuration lifecycle aligns with Restate's internal state management.
+For testing, `restate invocations purge <id>` immediately purges a completed invocation.
 
 ## Helm Chart
 

release-notes/unreleased/172-fix-stuck-finalizer-on-delete.md

@@ -0,0 +1,53 @@
+# Release Notes for Issue #172: Fix RestateDeployment finalizer stuck in CleanupFailed loop on delete
+
+## Bug Fix
+
+### What Changed
+
+When deleting a `RestateDeployment`, the finalizer no longer gets permanently
+stuck with `CleanupFailed(DeploymentInUse)`. Previously, cleanup treated the
+latest version's Restate deployment as an unconditional blocker because it is
+always "active" — it is the latest entry in `sys_service`. Since no new version
+ever registers during deletion, the `active_count > 0` check caused an infinite
+retry loop with no way out.
+
+The operator now tracks the two liveness signals separately instead of one
+`active` flag:
+
+- `latest_endpoint` — serves the latest revision of a service (`sys_service`)
+- `has_pinned_invocations` — has a non-completed invocation pinned to it
+  (`sys_invocation_status`)
+
+Outside deletion nothing changes: either signal keeps a version alive. During
+deletion only pinned invocations are worth waiting for — a version that is
+merely the latest endpoint has nothing to drain, so it is deregistered and
+removed immediately. A version with live pinned invocations is held for
+`spec.restate.drainDelaySeconds` first, then force-deregistered as before.
+
+Cleanup during deletion also bypasses `spec.revisionHistoryLimit`. Retaining
+versions for rollback made no sense once the object is going away, and it left
+the Restate deployments registered forever with nothing to deregister them
+after the finalizer was released.
+
+Both deployment modes are covered — ReplicaSet and Knative (Configurations).
+
+### Why This Matters
+
+This is the common case in **ephemeral PR/preview environments**: a service is
+deployed, registered with Restate, but no workflows are ever invoked before the
+environment is torn down. Without this fix, deleting the `RestateDeployment`
+would stall forever and require manual intervention to remove the finalizer.
+
+### Impact on Users
+
+- **Existing deployments being deleted:** Stuck `RestateDeployment` objects will
+  make progress on the next reconcile after upgrading.
+- **New deletions with no live invocations:** Complete immediately, without
+  waiting out the drain delay.
+- **New deletions with live pinned invocations:** Drain delay is respected, then
+  the Restate deployment is force-deleted and the finalizer released.
+- **No migration required.**
+
+### Related Issues
+
+- Issue #172: RestateDeployment finalizer stuck in CleanupFailed(DeploymentInUse) loop when no invocations ran

src/controllers/restatedeployment/controller.rs

@@ -211,6 +211,36 @@ fn error_policy<K, C>(_rs: Arc<K>, _: &Error, _ctx: C) -> Action {
     Action::requeue(Duration::from_secs(30))
 }
 
+/// Why Restate still considers a registered deployment live. The two signals are
+/// kept apart because only one of them is a reason to wait: a version can be the
+/// latest endpoint of a service and have never served an invocation.
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
+pub(super) struct DeploymentState {
+    /// Serves the latest revision of at least one service (`sys_service`).
+    pub latest_endpoint: bool,
+    /// At least one non-completed invocation is pinned to it (`sys_invocation_status`).
+    pub has_pinned_invocations: bool,
+}
+
+impl DeploymentState {
+    pub fn active(&self) -> bool {
+        self.latest_endpoint || self.has_pinned_invocations
+    }
+
+    /// Whether this version must be left alone this reconcile.
+    ///
+    /// While the RestateDeployment is being deleted only live pinned invocations
+    /// count: nothing will ever supersede the latest endpoint now, so waiting on
+    /// it wedges the finalizer forever. Otherwise either signal keeps it.
+    pub fn blocks_removal(&self, is_deleting: bool) -> bool {
+        if is_deleting {
+            self.has_pinned_invocations
+        } else {
+            self.active()
+        }
+    }
+}
+
 impl RestateDeployment {
     /// Resolve the RestateCloudEnvironment values a `tunnelMode: in-process`
     /// deployment derives its identity from (None for every other mode). They feed
@@ -442,8 +472,7 @@ impl RestateDeployment {
         if existing_deployment_id.is_none_or(|existing_deployment_id| {
             !deployments
                 .get(existing_deployment_id)
-                .cloned()
-                .unwrap_or_default()
+                .is_some_and(DeploymentState::active)
         }) {
             let valid = async {
                 if let Some(cluster_name) = &self.spec.restate.register.cluster {
@@ -494,9 +523,15 @@ impl RestateDeployment {
                     self.spec.restate.use_http11.as_ref().cloned(),
                 )
                 .await?;
-            // if registration succeeded, treat this as an active endpoint
+            // if registration succeeded, treat this as the latest endpoint
             // if we fail after this point we will re-register and should get the same deployment id
-            deployments.insert(deployment_id.clone(), true);
+            deployments.insert(
+                deployment_id.clone(),
+                DeploymentState {
+                    latest_endpoint: true,
+                    has_pinned_invocations: false,
+                },
+            );
 
             debug!(
                 "Updating deployment-id annotation of ReplicaSet/Service {versioned_name} in namespace {namespace}"
@@ -940,22 +975,29 @@ impl RestateDeployment {
         Ok(resp.id)
     }
 
-    pub(super) async fn list_deployments(&self, ctx: &Context) -> Result<HashMap<String, bool>> {
-        // This query finds deployments, noting those that are the latest for a particular service, or have an active invocation
+    pub(super) async fn list_deployments(
+        &self,
+        ctx: &Context,
+    ) -> Result<HashMap<String, DeploymentState>> {
+        // This query finds deployments, noting separately those that are the latest for a
+        // particular service, and those that still have a live invocation pinned to them
         let sql_query = r#"
-            WITH active_deployments AS (
+            WITH latest_deployments AS (
                 SELECT DISTINCT deployment_id as id
                 FROM sys_service
                 WHERE deployment_id IS NOT NULL
-                UNION
+            ),
+            pinned_deployments AS (
                 SELECT DISTINCT pinned_deployment_id as id
                 FROM sys_invocation_status
                 WHERE pinned_deployment_id IS NOT NULL AND status != 'completed'
             )
             SELECT d.id as deployment_id,
-                   a.id IS NOT NULL as active
+                   l.id IS NOT NULL as latest_endpoint,
+                   p.id IS NOT NULL as has_pinned_invocations
             FROM sys_deployment d
-            LEFT JOIN active_deployments a ON d.id = a.id;
+            LEFT JOIN latest_deployments l ON d.id = l.id
+            LEFT JOIN pinned_deployments p ON d.id = p.id;
         "#;
 
         #[derive(Deserialize)]
@@ -966,7 +1008,8 @@ impl RestateDeployment {
         #[derive(Deserialize)]
         struct DeploymentQueryResultRow {
             deployment_id: String,
-            active: bool,
+            latest_endpoint: bool,
+            has_pinned_invocations: bool,
         }
 
         let resp = ctx
@@ -984,21 +1027,15 @@ impl RestateDeployment {
             .await
             .map_err(Error::AdminCallFailed)?;
 
-        let mut endpoints = HashMap::with_capacity(response.rows.len());
+        let mut endpoints: HashMap<String, DeploymentState> =
+            HashMap::with_capacity(response.rows.len());
 
         for row in response.rows {
-            match endpoints.entry(row.deployment_id) {
-                std::collections::hash_map::Entry::Occupied(mut entry) => {
-                    // two rows with same deployment id shouldnt happen...
-                    // we treat the deployment as active if any row is active
-                    if !entry.get() {
-                        entry.insert(row.active);
-                    }
-                }
-                std::collections::hash_map::Entry::Vacant(entry) => {
-                    entry.insert(row.active);
-                }
-            }
+            // two rows with same deployment id shouldnt happen, but if they do,
+            // any row asserting a signal wins
+            let entry = endpoints.entry(row.deployment_id).or_default();
+            entry.latest_endpoint |= row.latest_endpoint;
+            entry.has_pinned_invocations |= row.has_pinned_invocations;
         }
 
         Ok(endpoints)
@@ -1557,4 +1594,30 @@ mod tests {
         let s1_again = latest_version_label_selector(&v1, None).expect("v1 selector again");
         assert_eq!(s1, s1_again, "selector should be deterministic");
     }
+
+    #[test]
+    fn deletion_only_waits_for_pinned_invocations() {
+        let latest_only = DeploymentState {
+            latest_endpoint: true,
+            has_pinned_invocations: false,
+        };
+        // outside deletion, the latest endpoint is the live version and is kept
+        assert!(latest_only.active());
+        assert!(latest_only.blocks_removal(false));
+        // ...but nothing can supersede it once the RestateDeployment is going away,
+        // and with no invocations there is nothing to drain (issue #172)
+        assert!(!latest_only.blocks_removal(true));
+
+        let pinned = DeploymentState {
+            latest_endpoint: false,
+            has_pinned_invocations: true,
+        };
+        assert!(pinned.blocks_removal(false));
+        assert!(pinned.blocks_removal(true));
+
+        let drained = DeploymentState::default();
+        assert!(!drained.active());
+        assert!(!drained.blocks_removal(false));
+        assert!(!drained.blocks_removal(true));
+    }
 }