From a18b90f01c51acd2d1b531b789314e58cbbbe2c5 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Tue, 16 Jul 2024 15:23:02 -0400 Subject: [PATCH] fix: track patches while they are booting (#185) * fix: track patches while they are booting * update comment * update comment * add test * only init UpdaterState if set_config succeeds * pr feedback * cleanup * pr feedback * add todo referencing github bug re: patch install failure events * document lifetime of currently_booting_patch * more details about on_init lifetime * add todo re: validation * update test comments * update test to regenerate patch manager when simulating a fresh start * Introduce InitError for better testing * fix test * add extra assert in PatchManager on_init_tests --- library/src/android.rs | 10 +-- library/src/c_api/mod.rs | 5 +- library/src/cache/patch_manager.rs | 101 +++++++++++++++++++++++++-- library/src/cache/updater_state.rs | 4 ++ library/src/config.rs | 35 ++++++---- library/src/updater.rs | 105 ++++++++++++++++++++++++++--- 6 files changed, 226 insertions(+), 34 deletions(-) diff --git a/library/src/android.rs b/library/src/android.rs index 9a5dea4..823a561 100644 --- a/library/src/android.rs +++ b/library/src/android.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; #[cfg(test)] use std::println as debug; // Workaround to use println! for logs. -use crate::UpdateError; +use crate::InitError; /// This function is a hack for Android. Android passes an array of paths, the /// first of which is `libapp.so` the second of which is a long (virtual) path @@ -24,9 +24,9 @@ use crate::UpdateError; /// "/data/app/~~7LtReIkm5snW_oXeDoJ5TQ==/com.example.shorebird_test-rpkDZSLBRv2jWcc1gQpwdg==/lib/x86_64/libapp.so" /// Will return: /// "/data/app/~~7LtReIkm5snW_oXeDoJ5TQ==/com.example.shorebird_test-rpkDZSLBRv2jWcc1gQpwdg==" -fn app_data_dir_from_libapp_path(libapp_path: &str) -> Result { +fn app_data_dir_from_libapp_path(libapp_path: &str) -> Result { let path = PathBuf::from(libapp_path); - let root = path.ancestors().nth(3).ok_or(UpdateError::InvalidArgument( + let root = path.ancestors().nth(3).ok_or(InitError::InvalidArgument( "original_libapp_paths".to_string(), format!("Invalid path: {}", libapp_path), ))?; @@ -184,7 +184,7 @@ pub(crate) fn open_base_lib(apks_dir: &Path, lib_name: &str) -> anyhow::Result Result { +pub fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result { // FIXME: This makes the assumption that the last path provided is the full // path to the libapp.so file. This is true for the current engine, but // may not be true in the future. Better would be for the engine to @@ -197,7 +197,7 @@ pub fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result, + /// The patch that is currently booting, if any. If the system initializes and + /// this has a value, we will consider this patch to have failed to boot. + /// This is given a value when we start booting a patch (record_boot_start_for_patch) and is + /// cleared when: + /// - the patch boots successfully (record_boot_success) + /// - the patch fails to boot (record_boot_failure_for_patch) + /// - the system initializes (on_init, we take this to mean the patch failed to boot) + currently_booting_patch: Option, + /// The highest patch number we have seen. This may be higher than the last booted /// patch or next patch if we downloaded a patch that failed to boot. highest_seen_patch_number: Option, @@ -54,6 +63,10 @@ struct PatchesState { /// Abstracts the process of managing patches. #[cfg_attr(test, automock)] pub trait ManagePatches { + /// Triggers any initialization logic needed by the patch manager. This is intended + /// to be called when Shorebird is initialized by the Flutter engine (shorebird_init). + fn on_init(&mut self) -> Result<()>; + /// Copies the patch file at file_path to the manager's directory structure sets /// this patch as the next patch to boot. /// @@ -213,7 +226,12 @@ impl PatchManager { } // If the last boot we tried was this patch, make sure we succeeded or the patch is bad. - if self.is_patch_last_attempted_patch(patch.number) { + // If we are currently in the process of booting this patch for the first time, this check + // will always fail, so skip it. + // TODO: this validation only needs to happen once. Move to on_init. + if !self.is_currently_booting_patch(patch.number) + && self.is_patch_last_attempted_patch(patch.number) + { // We are trying to boot from the same patch that we tried to boot from last time. match self.last_successful_boot_patch_number() { @@ -261,6 +279,14 @@ impl PatchManager { .unwrap_or(false) } + /// Whether this patch is in the process of booting. + fn is_currently_booting_patch(&self, patch_number: usize) -> bool { + match &self.patches_state.currently_booting_patch { + Some(currently_booting_patch) => currently_booting_patch.number == patch_number, + None => false, + } + } + /// The number of the patch we last successfully booted, if any. fn last_successful_boot_patch_number(&self) -> Option { self.patches_state @@ -346,6 +372,19 @@ impl PatchManager { } impl ManagePatches for PatchManager { + fn on_init(&mut self) -> Result<()> { + // If we were booting a patch but never recorded a successful boot, we assume that + // the patch failed to boot. Attempt to fall back. + // TODO: this should record a PatchInstallFailure event. https://github.com/shorebirdtech/updater/issues/188 + if let Some(failed_boot_patch) = self.patches_state.currently_booting_patch.clone() { + self.try_fall_back_from_patch(failed_boot_patch.number); + self.patches_state.currently_booting_patch = None; + self.save_patches_state()?; + } + + Ok(()) + } + // The explicit lifetime is required for automock to work with Options. // See https://github.com/asomers/mockall/issues/61. #[allow(clippy::needless_lifetimes)] @@ -445,7 +484,8 @@ impl ManagePatches for PatchManager { ); } - self.patches_state.last_attempted_patch = Some(next_boot_patch); + self.patches_state.last_attempted_patch = Some(next_boot_patch.clone()); + self.patches_state.currently_booting_patch = Some(next_boot_patch.clone()); self.save_patches_state() } @@ -456,6 +496,7 @@ impl ManagePatches for PatchManager { .clone() .context("No last_attempted_patch")?; + self.patches_state.currently_booting_patch = None; self.patches_state.last_booted_patch = Some(boot_patch.clone()); if let Err(e) = self.delete_patch_artifacts_older_than(boot_patch.number) { error!( @@ -467,6 +508,7 @@ impl ManagePatches for PatchManager { } fn record_boot_failure_for_patch(&mut self, patch_number: usize) -> Result<()> { + self.patches_state.currently_booting_patch = None; self.try_fall_back_from_patch(patch_number); self.save_patches_state() } @@ -538,13 +580,55 @@ mod debug_tests { let temp_dir = TempDir::new("patch_manager").unwrap(); let patch_manager = PatchManager::new(temp_dir.path().to_owned(), Some("public_key")); let expected_str = format!( - "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}", + "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, currently_booting_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}", temp_dir.path().display() ); assert_eq!(format!("{:?}", patch_manager), expected_str); } } +#[cfg(test)] +mod on_init_tests { + use super::*; + + #[test] + fn clears_currently_booting_patch() -> Result<()> { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::manager_for_test(&temp_dir); + + // Add a patch and start to boot from it. + manager.add_patch_for_test(&temp_dir, 1)?; + manager.record_boot_start_for_patch(1)?; + + assert_eq!( + manager + .patches_state + .currently_booting_patch + .as_ref() + .map(|p| p.number), + Some(1) + ); + assert_eq!( + manager.next_boot_patch().as_ref().map(|p| p.number), + Some(1) + ); + + // Simulate that the app is being started fresh (e.g. from a crash) + manager = PatchManager::manager_for_test(&temp_dir); + // Ensure that we didn't somehow lose next_boot_patch when recreating the manager. + assert_eq!( + manager.next_boot_patch().as_ref().map(|p| p.number), + Some(1) + ); + manager.on_init()?; + + // Verify that we are no longer booting from patch 1. + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } +} + #[cfg(test)] mod add_patch_tests { use super::*; @@ -817,10 +901,14 @@ mod next_boot_patch_tests { 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. + // Add a first patch and record that we started booting it, but not that it succeeded. manager.add_patch_for_test(&temp_dir, 1)?; manager.record_boot_start_for_patch(1)?; + // Simulate that the app is being started fresh (e.g. from a crash) + manager = PatchManager::manager_for_test(&temp_dir); + manager.on_init()?; + assert!(manager.next_boot_patch().is_none()); Ok(()) @@ -836,9 +924,14 @@ mod next_boot_patch_tests { manager.record_boot_start_for_patch(1)?; manager.record_boot_success()?; + // Add a second patch and record that we started booting it, but not that it succeeded. manager.add_patch_for_test(&temp_dir, 2)?; manager.record_boot_start_for_patch(2)?; + // Simulate that the app is being started fresh (e.g. from a crash) + manager = PatchManager::manager_for_test(&temp_dir); + manager.on_init()?; + assert!(manager .next_boot_patch() .is_some_and(|patch| patch.number == 1)); diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 6d92018..da522fc 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -149,6 +149,10 @@ impl UpdaterState { /// Patch management. All patch management is done via the patch manager. impl UpdaterState { + pub fn on_init(&mut self) -> Result<()> { + self.patch_manager.on_init() + } + /// Records that we are attempting to boot the patch with patch_number. pub fn record_boot_start_for_patch(&mut self, patch_number: usize) -> Result<()> { self.patch_manager.record_boot_start_for_patch(patch_number) diff --git a/library/src/config.rs b/library/src/config.rs index c25d9c5..df073bf 100644 --- a/library/src/config.rs +++ b/library/src/config.rs @@ -6,12 +6,13 @@ use crate::yaml::YamlConfig; use crate::{ExternalFileProvider, UpdateError}; use std::path::PathBuf; +use anyhow::{bail, Result}; use once_cell::sync::OnceCell; use std::sync::Mutex; // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests #[cfg(test)] -use std::{println as warn, println as debug}; // Workaround to use println! for logs. +use std::println as debug; // Workaround to use println! for logs. // cbindgen looks for const, ignore these so it doesn't warn about them. @@ -90,20 +91,20 @@ pub struct UpdateConfig { pub patch_public_key: Option, } +/// Returns Ok if the config was set successfully, Err if it was already set. pub fn set_config( app_config: AppConfig, file_provider: Box, libapp_path: PathBuf, yaml: &YamlConfig, network_hooks: NetworkHooks, -) { +) -> Result<()> { with_config_mut(|config: &mut Option| { if config.is_some() { // This previously returned an error, but this happens regularly // with apps that use Firebase Messaging, and logging it as an error // has caused confusion. - warn!("Updater already initialized, ignoring second shorebird_init call."); - return; + bail!("Updater already initialized, ignoring second shorebird_init call."); } let mut code_cache_path = std::path::PathBuf::from(&app_config.code_cache_dir); @@ -133,7 +134,9 @@ pub fn set_config( }; debug!("Updater configured with: {:?}", new_config); *config = Some(new_config); - }); + + Ok(()) + }) } // Arch/Platform names need to be kept in sync with the shorebird cli. @@ -167,6 +170,7 @@ pub fn current_platform() -> &'static str { mod tests { use super::set_config; use crate::{network::NetworkHooks, testing_reset_config, AppConfig, ExternalFileProvider}; + use anyhow::Result; use serial_test::serial; #[derive(Debug, Clone)] @@ -199,7 +203,7 @@ mod tests { // These tests are serial because they modify global state. #[serial] #[test] - fn set_config_correctly_sets_values() { + fn set_config_correctly_sets_values() -> Result<()> { testing_reset_config(); set_config( @@ -219,7 +223,7 @@ mod tests { patch_public_key: Some("patch_public_key".to_string()), }, NetworkHooks::default(), - ); + )?; let config = super::with_config(|config| Ok(config.clone())).unwrap(); assert_eq!(config.storage_dir.to_str(), Some("/app_storage")); @@ -235,30 +239,37 @@ mod tests { config.patch_public_key, Some("patch_public_key".to_string()) ); + + Ok(()) } // These tests are serial because they modify global state. #[serial] #[test] - fn set_config_is_noop_on_subsequent_calls() { + fn set_config_returns_err_on_subsequent_calls() -> Result<()> { testing_reset_config(); - set_config( + assert!(set_config( fake_app_config(), Box::new(FakeExternalFileProvider {}), "first_path".into(), &fake_yaml(), NetworkHooks::default(), - ); - set_config( + ) + .is_ok()); + + assert!(set_config( fake_app_config(), Box::new(FakeExternalFileProvider {}), "second_path".into(), &fake_yaml(), NetworkHooks::default(), - ); + ) + .is_err()); let config = super::with_config(|config| Ok(config.clone())).unwrap(); assert_eq!(config.libapp_path.to_str(), Some("first_path")); + + Ok(()) } } diff --git a/library/src/updater.rs b/library/src/updater.rs index 7e71c3b..3bb141b 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -46,9 +46,29 @@ impl Display for UpdateStatus { } } +/// Returned when a call to `init` is not successful. +#[derive(Debug, PartialEq)] +pub enum InitError { + InvalidArgument(String, String), + AlreadyInitialized, +} + +impl std::error::Error for InitError {} + +impl Display for InitError { + fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { + match self { + InitError::InvalidArgument(name, value) => { + write!(f, "Invalid Argument: {name} -> {value}") + } + InitError::AlreadyInitialized => write!(f, "Shorebird has already been initialized."), + } + } +} + +/// Returned when a function that is part of the update lifecycle fails. #[derive(Debug, PartialEq)] pub enum UpdateError { - InvalidArgument(String, String), InvalidState(String), BadServerResponse, FailedToSaveState, @@ -61,9 +81,6 @@ impl std::error::Error for UpdateError {} impl Display for UpdateError { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { match self { - UpdateError::InvalidArgument(name, value) => { - write!(f, "Invalid Argument: {name} -> {value}") - } UpdateError::InvalidState(msg) => write!(f, "Invalid State: {msg}"), UpdateError::FailedToSaveState => write!(f, "Failed to save state"), UpdateError::BadServerResponse => write!(f, "Bad server response"), @@ -101,10 +118,10 @@ dyn_clone::clone_trait_object!(ExternalFileProvider); // and a hard-coded name for the libapp file which we look up in the // split APKs in that datadir. On other platforms we just use a path. #[cfg(not(any(target_os = "android", test)))] -fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result { +fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result { let first = original_libapp_paths .first() - .ok_or(UpdateError::InvalidArgument( + .ok_or(InitError::InvalidArgument( "original_libapp_paths".to_string(), "empty".to_string(), )); @@ -120,17 +137,17 @@ pub fn init( app_config: AppConfig, file_provider: Box, yaml: &str, -) -> Result<(), UpdateError> { +) -> Result<(), InitError> { #[cfg(any(target_os = "android", test))] use crate::android::libapp_path_from_settings; init_logging(); let config = YamlConfig::from_yaml(yaml) - .map_err(|err| UpdateError::InvalidArgument("yaml".to_string(), err.to_string()))?; + .map_err(|err| InitError::InvalidArgument("yaml".to_string(), err.to_string()))?; let libapp_path = libapp_path_from_settings(&app_config.original_libapp_paths)?; debug!("libapp_path: {:?}", libapp_path); - set_config( + let set_config_result = set_config( app_config, file_provider, libapp_path, @@ -138,6 +155,21 @@ pub fn init( NetworkHooks::default(), ); + // set_config will return an error if the config is already initialized. This should not cause + // init to fail. + if set_config_result.is_err() { + return Err(InitError::AlreadyInitialized); + } + + let _ = with_config(|config| { + UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ) + .on_init() + }); + Ok(()) } @@ -597,6 +629,59 @@ mod tests { .unwrap(); } + #[serial] + #[test] + fn subsequent_init_calls_do_not_update_config() { + let tmp_dir = TempDir::new("example").unwrap(); + + testing_reset_config(); + let cache_dir = tmp_dir.path().to_str().unwrap().to_string(); + let mut yaml = "app_id: 1234".to_string(); + + assert_eq!( + crate::init( + crate::AppConfig { + app_storage_dir: cache_dir.clone(), + code_cache_dir: cache_dir.clone(), + release_version: "1.0.0+1".to_string(), + original_libapp_paths: vec!["/dir/lib/arch/libapp.so".to_string()], + }, + Box::new(FakeExternalFileProvider {}), + &yaml, + ), + Ok(()) + ); + + with_config(|config| { + assert_eq!(config.app_id, "1234"); + Ok(()) + }) + .unwrap(); + + // Attempt to init a second time with a different app_id. + yaml = "app_id: 5678".to_string(); + assert_eq!( + crate::init( + crate::AppConfig { + app_storage_dir: cache_dir.clone(), + code_cache_dir: cache_dir.clone(), + release_version: "1.0.0+1".to_string(), + original_libapp_paths: vec!["/dir/lib/arch/libapp.so".to_string()], + }, + Box::new(FakeExternalFileProvider {}), + &yaml, + ), + Err(crate::InitError::AlreadyInitialized) + ); + + // Verify that the app_id is still the original value. + with_config(|config| { + assert_eq!(config.app_id, "1234"); + Ok(()) + }) + .unwrap(); + } + #[serial] #[test] fn ignore_version_after_marked_bad() { @@ -698,7 +783,7 @@ mod tests { Box::new(FakeExternalFileProvider {}), "", ), - Err(crate::UpdateError::InvalidArgument( + Err(crate::InitError::InvalidArgument( "yaml".to_string(), "missing field `app_id`".to_string() ))