fix: don't permanently ban patch numbers on server-driven rollback

cspell.config.yaml

@@ -67,8 +67,10 @@ words:
   - pubspec
   - repr
   - reqwest
+  - resends
   - rlib
   - rsplit
+  - rollforward
   - rollouts
   - RTLD
   - rustflags

library/src/c_api/mod.rs

@@ -876,6 +876,114 @@ mod test {
         assert_eq!(shorebird_current_boot_patch_number(), 0);
     }
 
+    /// Regression: rolling a patch forward after the server rolled it back
+    /// must allow the device to install it again.
+    ///
+    /// The dashboard's rollback and rollforward operations toggle
+    /// `is_rolled_back` on the same `app_release_patch_` row, so the server
+    /// resends the same `Patch` (same number, same hash) on rollforward.
+    /// `remove_patch` previously inserted the patch number into
+    /// `known_bad_patches` to defend against an orthogonal
+    /// resurrection-via-failed-delete race; that insert also banned the
+    /// number for any later legitimate reuse, so `should_install_patch`
+    /// returned `PatchKnownBad` for the rolled-forward patch and the
+    /// device silently refused it.
+    ///
+    /// Customer-visible symptom: `checkForUpdate` returns `upToDate` while
+    /// the server still says a patch is available. Combined with FRC-driven
+    /// notification logic that compares `currentPatch` against a target
+    /// patch number, the app loops on a force-restart prompt that nothing
+    /// can satisfy.
+    #[serial]
+    #[test]
+    fn rollforward_after_rollback_can_install_same_patch() {
+        testing_reset_config();
+        let tmp_dir = TempDir::new().unwrap();
+
+        let apk_path = tmp_dir.path().join("base.apk");
+        write_fake_apk(
+            apk_path.to_str().unwrap(),
+            HELLO_TESTS_PATCH.base.as_bytes(),
+        );
+        let fake_libapp_path = tmp_dir.path().join("lib/arch/ignored.so");
+        let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap());
+        let c_yaml = c_string("app_id: foo");
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
+        free_c_string(c_yaml);
+        free_parameters(c_params);
+
+        // Phase 1: install patch 1 and report a successful launch.
+        testing_set_network_hooks(
+            |_url, _request| {
+                Ok(PatchCheckResponse {
+                    patch_available: true,
+                    patch: Some(crate::Patch {
+                        number: 1,
+                        hash: HELLO_TESTS_PATCH.hash.to_owned(),
+                        download_url: "ignored".to_owned(),
+                        hash_signature: None,
+                    }),
+                    rolled_back_patch_numbers: None,
+                })
+            },
+            |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
+            |_url, _event| Ok(()),
+        );
+        assert!(shorebird_check_for_downloadable_update(std::ptr::null()));
+        run_update_expecting(std::ptr::null(), SHOREBIRD_UPDATE_INSTALLED);
+        shorebird_report_launch_start();
+        shorebird_report_launch_success();
+        assert_eq!(shorebird_current_boot_patch_number(), 1);
+
+        // Phase 2: server rolls patch 1 back with no replacement. Phase-1
+        // spawned threads (PatchDownload, PatchInstallSuccess) hold a clone
+        // of the config from when they were spawned, so they hit the old
+        // report hook above. Nothing in phase 2 should report or download —
+        // only a patch-check happens.
+        testing_set_network_hooks(
+            |_url, _request| {
+                Ok(PatchCheckResponse {
+                    patch_available: false,
+                    patch: None,
+                    rolled_back_patch_numbers: Some(vec![1]),
+                })
+            },
+            UNEXPECTED_DOWNLOAD,
+            UNEXPECTED_REPORT,
+        );
+        assert!(!shorebird_check_for_downloadable_update(std::ptr::null()));
+        assert_eq!(shorebird_next_boot_patch_number(), 0);
+
+        // Phase 3: server rolls patch 1 forward — same number, same hash,
+        // empty rolled_back list (the row's `is_rolled_back` flipped back
+        // to false on the server). The device must accept this as a normal
+        // "patch available" response.
+        testing_set_network_hooks(
+            |_url, _request| {
+                Ok(PatchCheckResponse {
+                    patch_available: true,
+                    patch: Some(crate::Patch {
+                        number: 1,
+                        hash: HELLO_TESTS_PATCH.hash.to_owned(),
+                        download_url: "ignored".to_owned(),
+                        hash_signature: None,
+                    }),
+                    rolled_back_patch_numbers: Some(vec![]),
+                })
+            },
+            |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
+            |_url, _event| Ok(()),
+        );
+
+        // Pre-fix: returns false because patch 1 is in `known_bad_patches`
+        // (inserted by `remove_patch` in phase 2). Post-fix: returns true.
+        assert!(shorebird_check_for_downloadable_update(std::ptr::null()));
+        run_update_expecting(std::ptr::null(), SHOREBIRD_UPDATE_INSTALLED);
+
+        // The rolled-forward patch is queued for the next boot.
+        assert_eq!(shorebird_next_boot_patch_number(), 1);
+    }
+
     /// Patch-to-patch rollback: device on patch 2, server rolls back to
     /// patch 1 (sends rollback signal AND a downloadable replacement).
     /// `check_for_downloadable_update` returns true (replacement available),

library/src/cache/patch_manager.rs

@@ -593,11 +593,30 @@ impl ManagePatches for PatchManager {
     }
 
     fn remove_patch(&mut self, patch_number: usize) -> Result<()> {
-        // Server-driven rollback: mark known-bad so a later fallback path
-        // (e.g. record_boot_failure_for_patch on a *different* patch) can't
-        // promote `last_booted_patch` back to `next_boot_patch` if its
-        // artifact deletion in try_fall_back_from_patch silently fails.
-        self.patches_state.known_bad_patches.insert(patch_number);
+        // Server-driven rollback: drop the patch as a fallback target so a
+        // later boot failure on a different patch can't promote it back via
+        // `try_fall_back_from_patch`'s else-if branch (e.g. if
+        // `delete_patch_artifacts` silently fails and the artifact still
+        // passes `validate_patch_is_bootable`).
+        //
+        // Do *not* add `patch_number` to `known_bad_patches`: that set is a
+        // permanent ban for the current `release_version`, so it would
+        // poison the number for any later rollforward of the same patch
+        // (the dashboard's rollforward flips `is_rolled_back` on the same
+        // row — same number, same hash) or for a numerically-equal patch
+        // in a freshly-recreated release at the same version. FFI's
+        // "what's running" signal is the session-scoped `running_patch`,
+        // so dropping `last_booted_patch` here doesn't regress the
+        // patch-to-release rollback fix from #348.
+        if self
+            .patches_state
+            .last_booted_patch
+            .as_ref()
+            .map(|p| p.number)
+            == Some(patch_number)
+        {
+            self.patches_state.last_booted_patch = None;
+        }
         self.try_fall_back_from_patch(patch_number)
     }
 
@@ -1342,9 +1361,12 @@ mod fall_back_tests {
 
     /// Server rolls back patch 1, then patch 2 arrives and fails to boot.
     /// The else-if fallback path must not promote patch 1 back into
-    /// `next_boot_patch` even though `last_booted_patch` still points at
-    /// patch 1 — the server told us not to use it. `remove_patch` records
-    /// patch 1 as known-bad so this can't happen.
+    /// `next_boot_patch`: the server told us not to use it, even if a
+    /// silent `delete_patch_artifacts` failure leaves the artifact
+    /// intact and `validate_patch_is_bootable` would accept it.
+    /// `remove_patch` defends against this by clearing
+    /// `last_booted_patch` when its number matches the rolled-back
+    /// patch, so there's no fallback target to promote.
     #[test]
     fn rollback_then_failed_replacement_does_not_resurrect_rolled_back_patch() -> Result<()> {
         let temp_dir = TempDir::new()?;
@@ -1355,21 +1377,14 @@ mod fall_back_tests {
         manager.record_boot_start_for_patch(1)?;
         manager.record_boot_success()?;
 
-        // Server rolls back patch 1.
+        // Server rolls back patch 1. last_booted_patch is dropped because
+        // it points at the rolled-back patch.
         manager.remove_patch(1)?;
-        assert!(manager.is_known_bad_patch(1));
         assert!(manager.patches_state.next_boot_patch.is_none());
-        // last_booted_patch is intentionally preserved; the running
-        // process is still using patch 1 until it restarts.
-        assert_eq!(
-            manager
-                .patches_state
-                .last_booted_patch
-                .as_ref()
-                .unwrap()
-                .number,
-            1
-        );
+        assert!(manager.patches_state.last_booted_patch.is_none());
+        // The number is *not* added to `known_bad_patches`, so a
+        // subsequent rollforward of the same patch can install it.
+        assert!(!manager.is_known_bad_patch(1));
 
         // Re-create patch 1's artifact to simulate a silent
         // delete_patch_artifacts failure (e.g. transient FS issue).
@@ -1382,9 +1397,10 @@ mod fall_back_tests {
         manager.record_boot_start_for_patch(2)?;
         manager.record_boot_failure_for_patch(2)?;
 
-        // Patch 1 must NOT be promoted back to next_boot_patch.
+        // Patch 1 must NOT be promoted back to next_boot_patch — the
+        // property the test guards. With `last_booted_patch` cleared in
+        // `remove_patch`, the else-if branch has no fallback target.
         assert!(manager.patches_state.next_boot_patch.is_none());
-        assert!(manager.is_known_bad_patch(1));
         assert!(manager.is_known_bad_patch(2));
 
         Ok(())