fix: separate validation checks from next_boot_patch getter (#297)

* fix: separate validation checks from next_boot_patch getter

* coverage

* coverage

* coverage

library/include/updater.h

@@ -133,6 +133,14 @@ SHOREBIRD_EXPORT uintptr_t shorebird_current_boot_patch_number(void);
  */
 SHOREBIRD_EXPORT uintptr_t shorebird_next_boot_patch_number(void);
 
+/**
+ * Performs integrity checks on the next boot patch. If the patch fails these checks, the patch
+ * will be deleted and the next boot patch will be set to the last successfully booted patch or
+ * the base release if there is no last successfully booted patch.
+ */
+SHOREBIRD_EXPORT
+void shorebird_validate_next_boot_patch(void);
+
 /**
  * The path to the patch that will boot on the next run of the app, or NULL if
  * there is no next patch.

library/src/c_api/mod.rs

@@ -231,6 +231,18 @@ fn to_update_result(status: anyhow::Result<UpdateStatus>) -> UpdateResult {
     return result;
 }
 
+/// Performs integrity checks on the next boot patch. If the patch fails these checks, the patch
+/// will be deleted and the next boot patch will be set to the last successfully booted patch or
+/// the base release if there is no last successfully booted patch.
+#[no_mangle]
+pub extern "C" fn shorebird_validate_next_boot_patch() {
+    log_on_error(
+        || updater::validate_next_boot_patch(),
+        "validating next_boot_patch",
+        (),
+    );
+}
+
 /// The path to the patch that will boot on the next run of the app, or NULL if
 /// there is no next patch.
 #[no_mangle]
@@ -665,6 +677,7 @@ mod test {
         assert_eq!(shorebird_current_boot_patch_number(), 0);
         assert_eq!(shorebird_next_boot_patch_number(), 1);
 
+        shorebird_validate_next_boot_patch();
         // Read path contents into memory and check against expected.
         let c_path = shorebird_next_boot_patch_path();
         let path = to_rust(c_path).unwrap();
@@ -882,6 +895,7 @@ mod test {
     #[test]
     fn forgot_init() {
         testing_reset_config();
+        shorebird_validate_next_boot_patch();
         assert_eq!(shorebird_next_boot_patch_number(), 0);
         assert_eq!(shorebird_next_boot_patch_path(), null_mut());
     }

library/src/cache/patch_manager.rs

@@ -92,6 +92,10 @@ pub trait ManagePatches {
     /// - we cannot boot from the patch(es) on disk
     fn next_boot_patch(&mut self) -> Option<PatchInfo>;
 
+    /// Performs integrity checks on the next boot patch and updates the state accordingly. Returns
+    /// an error if the patch exists but is not bootable.
+    fn validate_next_boot_patch(&mut self) -> anyhow::Result<()>;
+
     /// Record that we're booting. If we have a next path, updates the last
     /// attempted patch to be the next boot patch.
     fn record_boot_start_for_patch(&mut self, patch_number: usize) -> Result<()>;
@@ -392,12 +396,14 @@ impl ManagePatches for PatchManager {
             .map(|patch| self.patch_info_for_number(patch.number))
     }
 
-    fn next_boot_patch(&mut self) -> Option<PatchInfo> {
+    fn validate_next_boot_patch(&mut self) -> anyhow::Result<()> {
         let next_boot_patch = match self.patches_state.next_boot_patch.clone() {
             Some(patch) => patch,
-            None => return None,
+            None => return anyhow::Ok(()),
         };
 
+        shorebird_info!("Validating patch {}", next_boot_patch.number);
+
         if let Err(e) = self.validate_patch_is_bootable(&next_boot_patch) {
             shorebird_error!("Patch {} is not bootable: {}", next_boot_patch.number, e);
 
@@ -408,8 +414,14 @@ impl ManagePatches for PatchManager {
                     e
                 );
             }
+
+            return Err(e);
         }
 
+        anyhow::Ok(())
+    }
+
+    fn next_boot_patch(&mut self) -> Option<PatchInfo> {
         self.patches_state
             .next_boot_patch
             .as_ref()
@@ -621,7 +633,6 @@ mod last_successfully_booted_patch_tests {
 #[cfg(test)]
 mod next_boot_patch_tests {
     use super::*;
-    use anyhow::Result;
     use tempdir::TempDir;
 
     #[test]
@@ -632,49 +643,18 @@ mod next_boot_patch_tests {
     }
 
     #[test]
-    fn returns_none_if_next_boot_patch_is_not_bootable() -> Result<()> {
+    fn returns_none_patch_if_first_patch_failed_to_boot() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
         let mut manager = PatchManager::manager_for_test(&temp_dir);
+
+        // Add a first patch and pretend it failed to boot.
         manager.add_patch_for_test(&temp_dir, 1)?;
+        manager.record_boot_start_for_patch(1)?;
+        manager.record_boot_failure_for_patch(1)?;
 
-        // Write junk to the artifact, this should render the patch unbootable in the eyes
-        // of the PatchManager.
-        let artifact_path = manager.patch_artifact_path(1);
-        std::fs::write(&artifact_path, "junk")?;
-
+        // Because there is no previous patch, we should not attempt to boot any patch.
         assert!(manager.next_boot_patch().is_none());
-
-        // Ensure the internal state is cleared.
-        assert!(manager.patches_state.next_boot_patch.is_none());
-
-        // The artifact should have been deleted.
-        assert!(!&artifact_path.exists());
-
-        Ok(())
-    }
-
-    #[test]
-    fn clears_current_and_next_on_boot_failure_if_they_are_the_same() -> Result<()> {
-        let patch_file_contents = "patch contents";
-        let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::manager_for_test(&temp_dir);
-        let file_path = &temp_dir.path().join("patch1.vmcode");
-        std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
-
-        // Write junk to the artifact, this should render the patch unbootable in the eyes
-        // of the PatchManager.
-        let artifact_path = manager.patch_artifact_path(1);
-        std::fs::write(&artifact_path, "junk")?;
-
-        assert!(manager.next_boot_patch().is_none());
-
-        // Ensure the internal state is cleared.
-        assert!(manager.patches_state.next_boot_patch.is_none());
-        assert!(manager.patches_state.last_booted_patch.is_none());
-
-        // The artifact should have been deleted.
-        assert!(!&artifact_path.exists());
+        assert!(manager.is_known_bad_patch(1));
 
         Ok(())
     }
@@ -707,6 +687,96 @@ mod next_boot_patch_tests {
         Ok(())
     }
 
+    #[test]
+    fn returns_last_booted_patch_if_next_patch_failed_to_boot() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
+
+        // Add a first patch and pretend it booted successfully.
+        manager.add_patch_for_test(&temp_dir, 1)?;
+        manager.record_boot_start_for_patch(1)?;
+        manager.record_boot_success()?;
+
+        // Add a second patch and pretend it failed to boot.
+        manager.add_patch_for_test(&temp_dir, 2)?;
+        manager.record_boot_start_for_patch(2)?;
+        manager.record_boot_failure_for_patch(2)?;
+
+        // Verify that we will next attempt to boot from patch 1.
+        assert_eq!(manager.next_boot_patch().unwrap().number, 1);
+        assert!(!manager.is_known_bad_patch(1));
+        assert!(manager.is_known_bad_patch(2));
+
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod validate_next_boot_patch_tests {
+    use super::*;
+    use anyhow::Result;
+    use tempdir::TempDir;
+
+    #[test]
+    fn does_nothing_if_no_next_boot_patch() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
+        assert!(manager.validate_next_boot_patch().is_ok());
+        Ok(())
+    }
+
+    #[test]
+    fn clears_next_boot_patch_if_it_is_not_bootable() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
+        manager.add_patch_for_test(&temp_dir, 1)?;
+
+        // Write junk to the artifact, this should render the patch unbootable in the eyes
+        // of the PatchManager.
+        let artifact_path = manager.patch_artifact_path(1);
+        std::fs::write(&artifact_path, "junk")?;
+
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
+        assert!(manager.next_boot_patch().is_none());
+
+        // Ensure the internal state is cleared.
+        assert!(manager.patches_state.next_boot_patch.is_none());
+
+        // The artifact should have been deleted.
+        assert!(!&artifact_path.exists());
+
+        Ok(())
+    }
+
+    #[test]
+    fn clears_current_and_next_on_boot_failure_if_they_are_the_same() -> Result<()> {
+        let patch_file_contents = "patch contents";
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
+        let file_path = &temp_dir.path().join("patch1.vmcode");
+        std::fs::write(file_path, patch_file_contents)?;
+        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
+
+        // Write junk to the artifact, this should render the patch unbootable in the eyes
+        // of the PatchManager.
+        let artifact_path = manager.patch_artifact_path(1);
+        std::fs::write(&artifact_path, "junk")?;
+
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
+        assert!(manager.next_boot_patch().is_none());
+
+        // Ensure the internal state is cleared.
+        assert!(manager.patches_state.next_boot_patch.is_none());
+        assert!(manager.patches_state.last_booted_patch.is_none());
+
+        // The artifact should have been deleted.
+        assert!(!&artifact_path.exists());
+
+        Ok(())
+    }
+
     #[test]
     fn does_not_fall_back_to_last_booted_patch_if_corrupted() -> Result<()> {
         let patch_file_contents = "patch contents";
@@ -731,6 +801,9 @@ mod next_boot_patch_tests {
         let patch_1_artifact_path = manager.patch_artifact_path(1);
         std::fs::write(patch_1_artifact_path, "junk")?;
 
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
+
         // Verify that we will not attempt to boot from either patch.
         assert!(manager.next_boot_patch().is_none());
 
@@ -744,46 +817,6 @@ mod next_boot_patch_tests {
         Ok(())
     }
 
-    #[test]
-    fn returns_none_patch_if_first_patch_failed_to_boot() -> Result<()> {
-        let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::manager_for_test(&temp_dir);
-
-        // Add a first patch and pretend it failed to boot.
-        manager.add_patch_for_test(&temp_dir, 1)?;
-        manager.record_boot_start_for_patch(1)?;
-        manager.record_boot_failure_for_patch(1)?;
-
-        // Because there is no previous patch, we should not attempt to boot any patch.
-        assert!(manager.next_boot_patch().is_none());
-        assert!(manager.is_known_bad_patch(1));
-
-        Ok(())
-    }
-
-    #[test]
-    fn returns_last_booted_patch_if_next_patch_failed_to_boot() -> Result<()> {
-        let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::manager_for_test(&temp_dir);
-
-        // Add a first patch and pretend it booted successfully.
-        manager.add_patch_for_test(&temp_dir, 1)?;
-        manager.record_boot_start_for_patch(1)?;
-        manager.record_boot_success()?;
-
-        // Add a second patch and pretend it failed to boot.
-        manager.add_patch_for_test(&temp_dir, 2)?;
-        manager.record_boot_start_for_patch(2)?;
-        manager.record_boot_failure_for_patch(2)?;
-
-        // Verify that we will next attempt to boot from patch 1.
-        assert_eq!(manager.next_boot_patch().unwrap().number, 1);
-        assert!(!manager.is_known_bad_patch(1));
-        assert!(manager.is_known_bad_patch(2));
-
-        Ok(())
-    }
-
     // The constant values below were generated by taking an arbitrary sha256 hash (INFLATED_PATCH_HASH)
     // and using openssl to sign it with the private key corresponding to `PUBLIC_KEY`.
 
@@ -806,6 +839,8 @@ mod next_boot_patch_tests {
 
         manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?;
 
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
         assert!(manager.next_boot_patch().is_none());
 
         Ok(())
@@ -818,6 +853,8 @@ mod next_boot_patch_tests {
 
         manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, None)?;
 
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
         assert!(manager.next_boot_patch().is_none());
 
         Ok(())
@@ -836,6 +873,8 @@ mod next_boot_patch_tests {
             Some(INFLATED_PATCH_HASH),
         )?;
 
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_err());
         assert!(manager.next_boot_patch().is_none());
 
         Ok(())
@@ -849,6 +888,9 @@ mod next_boot_patch_tests {
         manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?;
 
         assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_ok());
+        assert!(manager.next_boot_patch().is_some());
+
         let patch = manager.next_boot_patch().unwrap();
         assert_eq!(patch.number, 1);
 
@@ -868,6 +910,8 @@ mod next_boot_patch_tests {
             Some("not a valid signature"),
         )?;
 
+        assert!(manager.next_boot_patch().is_some());
+        assert!(manager.validate_next_boot_patch().is_ok());
         assert!(manager.next_boot_patch().is_some());
         let patch = manager.next_boot_patch().unwrap();
         assert_eq!(patch.number, 1);

library/src/cache/updater_state.rs

@@ -193,6 +193,15 @@ impl UpdaterState {
         self.patch_manager.next_boot_patch()
     }
 
+    /// Performs integrity checks on the next boot patch. If the patch fails these checks, the patch
+    /// will be deleted and the next boot patch will be set to the last successfully booted patch or
+    /// the base release if there is no last successfully booted patch.
+    ///
+    /// Returns an error if the patch fails integrity checks.
+    pub fn validate_next_boot_patch(&mut self) -> anyhow::Result<()> {
+        self.patch_manager.validate_next_boot_patch()
+    }
+
     /// Copies the patch file at file_path to the manager's directory structure sets
     /// this patch as the next patch to boot.
     pub fn install_patch(
@@ -407,6 +416,17 @@ mod tests {
         assert_eq!(state.next_boot_patch(), Some(patch));
     }
 
+    #[test]
+    fn validate_next_boot_patch_forwards_to_patch_manager() {
+        let tmp_dir = TempDir::new("example").unwrap();
+        let mut mock_manage_patches = MockManagePatches::new();
+        mock_manage_patches
+            .expect_validate_next_boot_patch()
+            .returning(|| Ok(()));
+        let mut state = test_state(&tmp_dir, mock_manage_patches);
+        assert!(state.validate_next_boot_patch().is_ok());
+    }
+
     #[test]
     fn install_patch_forwards_to_patch_manager() {
         let patch_number = 1;

library/src/updater.rs

@@ -538,6 +538,15 @@ where
     Ok(())
 }
 
+/// Performs integrity checks on the next boot patch. If the patch fails these checks, the patch
+/// will be deleted and the next boot patch will be set to the last successfully booted patch or
+/// the base release if there is no last successfully booted patch.
+///
+/// Returns an error if the patch fails integrity checks.
+pub fn validate_next_boot_patch() -> anyhow::Result<()> {
+    with_mut_state(|state| state.validate_next_boot_patch())
+}
+
 /// The patch which will be run on next boot (which may still be the same
 /// as the current boot).
 /// This may be changed any time by: