Carry the deployment id on AlreadyLatest instead of asserting it

release-notes/unreleased/174-no-reconcile-panic.md

@@ -0,0 +1,43 @@
+# Release Notes for #174 follow-up: no panic on the Knative reconcile path
+
+## Bug Fix
+
+### What Changed
+
+`RegistrationAction::AlreadyLatest` now carries the deployment id it was decided from, so the
+Knative reconciler reads it out of the verdict instead of unwrapping the `Option` it had just
+passed in.
+
+The planner reaches `AlreadyLatest` only after matching a recorded id against Restate's usage
+map, so the id is necessarily present — but that reasoning lived in an `.expect()` at the call
+site rather than in the type:
+
+```rust
+let recorded_id = recorded_id.cloned().expect("AlreadyLatest implies an id");
+```
+
+The invariant held, so this was not reachable in practice. It was still a panic sited on the
+reconcile path, where the cost of being wrong is not an error condition but the controller
+task unwinding, and it was only reachable there because the id had to be recovered separately
+from the decision that depended on it. Moving the id into the variant makes the bad state
+unrepresentable and removes the assertion entirely.
+
+The ReplicaSet path compared against the variant rather than destructuring it, so it changes
+to `matches!` and is otherwise untouched.
+
+### Why This Matters
+
+A `RestateDeployment` in the steady state — already latest, nothing to tell Restate — is the
+most frequently taken branch in the Knative reconciler. An assertion there is the one least
+likely to be exercised by anything but production.
+
+### Impact on Users
+
+None. No behaviour change, no CRD change, and no new status field or annotation: the panic was
+unreachable, and this removes the possibility rather than a symptom.
+
+### Related Issues
+
+- Issue #174: Support rolling RestateDeployments back to a previously registered revision
+- Review feedback on #178 (`reconcilers/knative.rs`): "i think we shouldn't panic here, it's on
+  the reconcile path"

src/controllers/restatedeployment/controller.rs

@@ -496,7 +496,7 @@ impl RestateDeployment {
             });
         }
 
-        if action != RegistrationAction::AlreadyLatest {
+        if !matches!(action, RegistrationAction::AlreadyLatest { .. }) {
             let valid = async {
                 if let Some(cluster_name) = &self.spec.restate.register.cluster {
                     // wait for the cluster to be ready before registering to it

src/controllers/restatedeployment/reconcilers/knative.rs

@@ -671,14 +671,13 @@ async fn register_or_promote_deployment(
 
     match &action {
         // Restate already sends new invocations here; nothing to say to it.
-        registration::RegistrationAction::AlreadyLatest => {
-            let recorded_id = recorded_id.cloned().expect("AlreadyLatest implies an id");
+        registration::RegistrationAction::AlreadyLatest { deployment_id } => {
             trace!(
-                deployment_id = %recorded_id,
+                deployment_id = %deployment_id,
                 "Configuration's deployment is already latest"
             );
-            annotate_configuration(ctx, namespace, config, &recorded_id).await?;
-            return Ok(recorded_id);
+            annotate_configuration(ctx, namespace, config, deployment_id).await?;
+            return Ok(deployment_id.clone());
         }
         registration::RegistrationAction::Conflict => {
             return Err(Error::DeploymentNotLatest {

src/controllers/restatedeployment/registration.rs

@@ -55,8 +55,10 @@ pub(super) enum RegistrationAction {
     /// it, intact. Carries the id that currently holds latest, for the event the operator
     /// emits afterwards.
     Promote { superseded_by: String },
-    /// Already serving new invocations. Leave it alone.
-    AlreadyLatest,
+    /// Already serving new invocations. Leave it alone. Carries the recorded id, which this
+    /// variant can only be reached with — holding it here is what saves the caller from
+    /// re-deriving it from an `Option` it has already proven to be `Some`.
+    AlreadyLatest { deployment_id: String },
     /// Our deployment is registered but superseded, and no version of this RestateDeployment
     /// is serving these services either — so something outside it is. Forcing here would
     /// start a promotion war between two controllers, each bumping revisions to take the
@@ -103,7 +105,9 @@ pub(super) fn plan_registration(
     };
 
     if usage.latest_for_service {
-        return RegistrationAction::AlreadyLatest;
+        return RegistrationAction::AlreadyLatest {
+            deployment_id: recorded_id.to_owned(),
+        };
     }
 
     // Our deployment exists but is superseded. It can only have been superseded by whoever
@@ -341,6 +345,12 @@ mod tests {
         }
     }
 
+    fn already_latest(deployment_id: &str) -> RegistrationAction {
+        RegistrationAction::AlreadyLatest {
+            deployment_id: deployment_id.into(),
+        }
+    }
+
     #[test]
     fn nothing_recorded_registers() {
         assert_eq!(
@@ -364,10 +374,24 @@ mod tests {
         let deployments = DeploymentUsageMap::from([("dp_v1".into(), usage(true, 0))]);
         assert_eq!(
             plan_registration(Some("dp_v1"), &deployments, || owned(&["dp_v1"])),
-            RegistrationAction::AlreadyLatest
+            already_latest("dp_v1")
         );
     }
 
+    /// The id travels with the verdict so the caller never has to assert an invariant the
+    /// planner already established. Previously the Knative path recovered it by unwrapping
+    /// the same `Option` it had passed in, which put a panic on the reconcile path.
+    #[test]
+    fn already_latest_carries_the_recorded_id() {
+        let deployments = DeploymentUsageMap::from([("dp_v1".into(), usage(true, 3))]);
+        let RegistrationAction::AlreadyLatest { deployment_id } =
+            plan_registration(Some("dp_v1"), &deployments, || owned(&["dp_v1"]))
+        else {
+            panic!("expected AlreadyLatest");
+        };
+        assert_eq!(deployment_id, "dp_v1");
+    }
+
     /// The steady state must not pay for the cluster-wide reflector walk that only a
     /// rollback needs.
     #[test]
@@ -473,7 +497,7 @@ mod tests {
     fn only_promotion_overwrites() {
         assert_eq!(RegistrationAction::Register.overwrite(), Overwrite::No);
         assert_eq!(promote("dp_v2").overwrite(), Overwrite::Yes);
-        assert_eq!(RegistrationAction::AlreadyLatest.overwrite(), Overwrite::No);
+        assert_eq!(already_latest("dp_v1").overwrite(), Overwrite::No);
         assert_eq!(RegistrationAction::Conflict.overwrite(), Overwrite::No);
         assert!(!Overwrite::No.force());
         assert!(Overwrite::Yes.force());