diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs index 2ab4aa3..19dcb38 100644 --- a/library/src/cache/patch_manager.rs +++ b/library/src/cache/patch_manager.rs @@ -268,31 +268,49 @@ impl PatchManager { /// successfully booted patch. If the last successfully booted patch is not bootable or has the same number /// as the patch we're falling back from, we clear it as well. fn try_fall_back_from_patch(&mut self, bad_patch_number: usize) -> Result<()> { - shorebird_info!("Falling back from patch {}", bad_patch_number); - // Continue even if we fail to delete the patch artifacts. It's more important to not try to // boot from a bad patch than to delete its artifacts. // No need to log failure – delete_patch_artifacts logs for us. let _ = self.delete_patch_artifacts(bad_patch_number); - if let Some(ref next_boot_patch) = self.patches_state.next_boot_patch { - // If our next boot patch is bad_patch_number, clear it. - if next_boot_patch.number == bad_patch_number { - self.patches_state.next_boot_patch = None; - } - } + let is_bad_patch_last_booted_patch = self + .patches_state + .last_booted_patch + .clone() + .map(|patch| patch.number == bad_patch_number) + .unwrap_or(false); + let is_bad_patch_next_boot_patch = self + .patches_state + .next_boot_patch + .clone() + .map(|patch| patch.number == bad_patch_number) + .unwrap_or(false); - // If we think we can still boot from the last booted patch, set it as the next_boot_patch. - // If something happened to render the last boot patch unbootable, clear it and delete its artifacts. - if let Some(last_boot_patch) = self.patches_state.last_booted_patch.clone() { - if last_boot_patch.number != bad_patch_number - && self.validate_patch_is_bootable(&last_boot_patch).is_ok() - { - self.patches_state.next_boot_patch = Some(last_boot_patch); - } else { - self.patches_state.last_booted_patch = None; - // No need to log failure – delete_patch_artifacts logs for us. - let _ = self.delete_patch_artifacts(last_boot_patch.number); + if is_bad_patch_last_booted_patch && is_bad_patch_next_boot_patch { + // If both patches are bad, delete them both and boot from the base release. + shorebird_info!("Clearing last booted patch and next boot patch"); + self.patches_state.last_booted_patch = None; + self.patches_state.next_boot_patch = None; + } else if is_bad_patch_next_boot_patch { + shorebird_info!("Clearing next boot patch"); + self.patches_state.next_boot_patch = None; + + if let Some(last_boot_patch) = self.patches_state.last_booted_patch.clone() { + if self.validate_patch_is_bootable(&last_boot_patch).is_ok() { + shorebird_info!( + "Setting last booted patch {} as next boot patch", + last_boot_patch.number + ); + self.patches_state.next_boot_patch = Some(last_boot_patch); + } else { + shorebird_info!( + "Last booted patch {} is not bootable, deleting artifacts", + last_boot_patch.number + ); + self.patches_state.last_booted_patch = None; + // No need to log failure – delete_patch_artifacts logs for us. + let _ = self.delete_patch_artifacts(last_boot_patch.number); + } } } @@ -940,29 +958,6 @@ mod fall_back_tests { Ok(()) } - #[test] - fn sets_next_patch_to_latest_patch_if_no_next_patch_exists() -> Result<()> { - let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::manager_for_test(&temp_dir); - - assert!(manager.patches_state.next_boot_patch.is_none()); - - manager.patches_state.last_booted_patch = Some(PatchMetadata { - number: 1, - size: 1, - hash: "hash".to_string(), - signature: Some("signature".to_owned()), - }); - manager.try_fall_back_from_patch(1)?; - - assert_eq!( - manager.patches_state.next_boot_patch, - manager.patches_state.last_booted_patch - ); - - Ok(()) - } - #[test] fn sets_next_patch_to_latest_patch_if_both_are_present() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; diff --git a/library/src/updater.rs b/library/src/updater.rs index f96e69d..48c4030 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -1379,6 +1379,89 @@ mod rollback_tests { report_launch_start, report_launch_success, tests::init_for_testing, with_mut_state, Patch, }; + #[serial] + #[test] + fn check_for_update_does_not_overwrite_good_next_patch() -> Result<()> { + // Reported in https://discord.com/channels/1030243211995791380/1446453671414992977 + + // Scenario: + // - Patch 1 is installed + // - Patches 2 and 3 are rolled back + // - Patch 4 is live and available to download + + // Checking for a downloadable update sets the next boot patch to the latest booted, even + // though the next boot patch is not rolled back. + + let mut server = mockito::Server::new(); + let download_url = format!("{}/patch/4", server.url()); + let check_response = PatchCheckResponse { + patch_available: true, + patch: Some(Patch { + number: 4, + hash: "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45" + .to_string(), + download_url: download_url.to_string(), + hash_signature: None, + }), + rolled_back_patch_numbers: Some(vec![3, 2]), + }; + let check_response_body = serde_json::to_string(&check_response).unwrap(); + let _ = server + .mock("POST", "/api/v1/patches/check") + .with_status(200) + .with_body(check_response_body) + .create(); + let _ = server + .mock("POST", "/api/v1/patches/events") + .with_status(201) + .create(); + let _ = server + .mock("GET", "/patch/4") + .with_status(200) + .with_body( + // Generated by `string_patch "hello world" "hello tests"` + [ + 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, + ], + ) + .create(); + let tmp_dir = TempDir::new("example").unwrap(); + init_for_testing(&tmp_dir, Some(&server.url())); + + let base = "hello world"; + let apk_path = tmp_dir.path().join("base.apk"); + write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes()); + + install_fake_patch(1)?; + + report_launch_start()?; + report_launch_success()?; + + let mut staging_update_check_result = crate::check_for_downloadable_update(None)?; + assert!(staging_update_check_result); + + let stable_update_result: crate::UpdateStatus = crate::update(None)?; + assert_eq!(stable_update_result, crate::UpdateStatus::UpdateInstalled); + + with_mut_state(|state| { + let next_boot_patch = state.next_boot_patch(); + println!("next_boot_patch after update: {:?}", next_boot_patch); + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(4)); + Ok(()) + })?; + + staging_update_check_result = crate::check_for_downloadable_update(None)?; + assert!(!staging_update_check_result); + + with_mut_state(|state| { + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(4)); + Ok(()) + })?; + + Ok(()) + } + #[serial] #[test] fn does_not_roll_back_when_rolled_back_patches_is_empty() -> Result<()> {