fix: update current_boot_patch to always return currently running patch (#202)

* fix: don't return true from isNewPatchAvailableForDownload if the new patch will not be installed

* fix: update current_boot_patch to always return currently running patch

* add C api tests

* add extra test check

Author: bryanoltman

library/src/c_api/mod.rs

@@ -503,6 +503,73 @@ mod test {
         assert_eq!(new, expected_new);
     }
 
+    #[serial]
+    #[test]
+    fn current_boot_patch_set_after_reporting_launch_start() {
+        testing_reset_config();
+        let tmp_dir = TempDir::new("example").unwrap();
+
+        // Generated by `string_patch "hello world" "hello tests"`
+        let base = "hello world";
+        let apk_path = tmp_dir.path().join("base.apk");
+        write_fake_apk(apk_path.to_str().unwrap(), 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());
+        // app_id is required or shorebird_init will fail.
+        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);
+
+        // set up the network hooks to return a patch.
+        testing_set_network_hooks(
+            |_url, _request| {
+                // Generated by `string_patch "hello world" "hello tests"`
+                let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45";
+                Ok(PatchCheckResponse {
+                    patch_available: true,
+                    patch: Some(crate::Patch {
+                        number: 1,
+                        hash: hash.to_owned(),
+                        download_url: "ignored".to_owned(),
+                        hash_signature: None,
+                    }),
+                    rolled_back_patch_numbers: None,
+                })
+            },
+            |_url| {
+                // Generated by `string_patch "hello world" "hello tests"`
+                let patch_bytes: Vec<u8> = vec![
+                    40, 181, 47, 253, 0, 128, 177, 0, 0, 223, 177, 0, 0, 0, 16, 0, 0, 6, 0, 0, 0,
+                    0, 0, 0, 5, 116, 101, 115, 116, 115, 0,
+                ];
+                Ok(patch_bytes)
+            },
+            |_url, _event| Ok(()),
+        );
+
+        // Ensure we start with no current patch
+        assert_eq!(shorebird_current_boot_patch_number(), 0);
+
+        // There is an update available.
+        assert!(shorebird_check_for_update());
+        // Go ahead and do the update.
+        shorebird_update();
+
+        // Ensure we have not yet updated the current patch.
+        assert_eq!(shorebird_current_boot_patch_number(), 0);
+
+        shorebird_report_launch_start();
+
+        // After reporting a launch start, the next boot patch should be the current patch.
+        assert_eq!(shorebird_current_boot_patch_number(), 1);
+
+        shorebird_report_launch_success();
+
+        // After reporting a launch success, the current patch number should not have changed.
+        assert_eq!(shorebird_current_boot_patch_number(), 1);
+    }
+
     #[serial]
     #[test]
     fn forgot_init() {

library/src/cache/updater_state.rs

@@ -171,14 +171,22 @@ impl UpdaterState {
         self.patch_manager.currently_booting_patch()
     }
 
-    /// This is the current patch that is running.
+    /// The last patch that was successfully booted (e.g., for which we record_boot_success was
+    /// called).
     /// Will be None if:
     /// - There was no good patch at time of boot.
     /// - The updater has been initialized but no boot recorded yet.
-    pub fn current_boot_patch(&self) -> Option<PatchInfo> {
+    pub fn last_successfully_booted_patch(&self) -> Option<PatchInfo> {
         self.patch_manager.last_successfully_booted_patch()
     }
 
+    /// This is the current patch that is running.
+    pub fn current_boot_patch(&self) -> Option<PatchInfo> {
+        self.patch_manager
+            .currently_booting_patch()
+            .or(self.patch_manager.last_successfully_booted_patch())
+    }
+
     /// This is the patch that will be used for the next boot.
     /// Will be None if:
     /// - There has never been a patch selected.
@@ -346,14 +354,45 @@ mod tests {
     }
 
     #[test]
-    fn current_boot_patch_forwards_from_patch_manager() {
+    fn last_successfully_booted_patch_forwards_from_patch_manager() {
         let tmp_dir = TempDir::new("example").unwrap();
         let patch = fake_patch(&tmp_dir, 1);
         let mut mock_manage_patches = MockManagePatches::new();
         mock_manage_patches
             .expect_last_successfully_booted_patch()
             .return_const(Some(patch.clone()));
         let state = test_state(&tmp_dir, mock_manage_patches);
+        assert_eq!(state.last_successfully_booted_patch(), Some(patch));
+    }
+
+    #[test]
+    fn current_boot_patch_returns_currently_booting_patch_if_present() {
+        let tmp_dir = TempDir::new("example").unwrap();
+        let patch1 = fake_patch(&tmp_dir, 1);
+        let patch2 = fake_patch(&tmp_dir, 2);
+        let mut mock_manage_patches = MockManagePatches::new();
+        mock_manage_patches
+            .expect_last_successfully_booted_patch()
+            .return_const(Some(patch1.clone()));
+        mock_manage_patches
+            .expect_currently_booting_patch()
+            .return_const(Some(patch2.clone()));
+        let state = test_state(&tmp_dir, mock_manage_patches);
+        assert_eq!(state.current_boot_patch(), Some(patch2));
+    }
+
+    #[test]
+    fn current_boot_patch_returns_last_successfully_booted_patch_if_no_patch_is_booting() {
+        let tmp_dir = TempDir::new("example").unwrap();
+        let patch = fake_patch(&tmp_dir, 1);
+        let mut mock_manage_patches = MockManagePatches::new();
+        mock_manage_patches
+            .expect_last_successfully_booted_patch()
+            .return_const(Some(patch.clone()));
+        mock_manage_patches
+            .expect_currently_booting_patch()
+            .return_const(None);
+        let state = test_state(&tmp_dir, mock_manage_patches);
         assert_eq!(state.current_boot_patch(), Some(patch));
     }
 

library/src/updater.rs

@@ -521,11 +521,6 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
 /// The patch that was last successfully booted. If we're booting a patch for the first time, this
 /// will be the previous patch (or None, if there was no previous patch) until the boot is
 /// reported as successful.
-///
-/// TODO: This should always return the currently running patch, even if it has not been marked as
-///   good or bad. Presently, users of the shorebird_code_push package will never get the wrong
-///   patch number from this function because a launch will have been reported to be either a
-///   success or a failure before they can call this function.
 pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_state(|state| Ok(state.current_boot_patch()))
 }
@@ -591,13 +586,17 @@ pub fn report_launch_success() -> anyhow::Result<()> {
             None => return Ok(()),
         };
 
-        let maybe_previous_boot_patch = state.current_boot_patch();
+        // Get the last successfully booted patch before we record the boot success.
+        let maybe_previous_boot_patch = state.last_successfully_booted_patch();
 
         state.record_boot_success()?;
 
-        if let (Some(previous_boot_patch), Some(current_boot_patch)) =
-            (maybe_previous_boot_patch, state.current_boot_patch())
-        {
+        // Check whether last_successfully_booted_patch has changed. If so, we should report a
+        // PatchInstallSuccess event.
+        if let (Some(previous_boot_patch), Some(current_boot_patch)) = (
+            maybe_previous_boot_patch,
+            state.last_successfully_booted_patch(),
+        ) {
             // If we had previously booted from a patch and it has the same number as the
             // patch we just booted from, then we shouldn't report a patch install.
             if previous_boot_patch.number == current_boot_patch.number {
@@ -1050,7 +1049,10 @@ mod tests {
                 &config.release_version,
                 config.patch_public_key.as_deref(),
             );
-            assert_eq!(state.current_boot_patch().unwrap().number, patch_number);
+            assert_eq!(
+                state.last_successfully_booted_patch().unwrap().number,
+                patch_number
+            );
             Ok(())
         })
         .unwrap();
@@ -1331,7 +1333,10 @@ mod rollback_tests {
         report_launch_success()?;
 
         with_mut_state(|state| {
-            assert_eq!(state.current_boot_patch().map(|p| p.number), Some(1));
+            assert_eq!(
+                state.last_successfully_booted_patch().map(|p| p.number),
+                Some(1)
+            );
             assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1));
             Ok(())
         })?;
@@ -1432,7 +1437,10 @@ mod rollback_tests {
         report_launch_success()?;
 
         with_mut_state(|state| {
-            assert_eq!(state.current_boot_patch().map(|p| p.number), Some(2));
+            assert_eq!(
+                state.last_successfully_booted_patch().map(|p| p.number),
+                Some(2)
+            );
             assert_eq!(state.next_boot_patch().map(|p| p.number), Some(2));
             Ok(())
         })?;