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

@@ -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<PathBuf, UpdateError> {
+fn app_data_dir_from_libapp_path(libapp_path: &str) -> Result<PathBuf, InitError> {
     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<C
     Ok(Cursor::new(buffer))
 }
 
-pub fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result<PathBuf, UpdateError> {
+pub fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result<PathBuf, InitError> {
     // 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<Pat
     // https://developer.android.com/reference/android/content/pm/ApplicationInfo#nativeLibraryDir
     let full_libapp_path = original_libapp_paths
         .last()
-        .ok_or(UpdateError::InvalidArgument(
+        .ok_or(InitError::InvalidArgument(
             "original_libapp_paths".to_string(),
             "empty".to_string(),
         ))?;

library/src/c_api/mod.rs

@@ -543,9 +543,8 @@ mod test {
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: bar");
 
-        // This will return true, but log a warning and not change the already-
-        // set config.
-        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
+        // This will return false because we have already initialized.
+        assert!(!shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
     }

library/src/cache/patch_manager.rs

@@ -46,6 +46,15 @@ struct PatchesState {
     /// as the last booted patch patch if no new patch has been downloaded.
     next_boot_patch: Option<PatchMetadata>,
 
+    /// 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<PatchMetadata>,
+
     /// 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<usize>,
@@ -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<usize> {
         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));

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)

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<String>,
 }
 
+/// Returns Ok if the config was set successfully, Err if it was already set.
 pub fn set_config(
     app_config: AppConfig,
     file_provider: Box<dyn ExternalFileProvider>,
     libapp_path: PathBuf,
     yaml: &YamlConfig,
     network_hooks: NetworkHooks,
-) {
+) -> Result<()> {
     with_config_mut(|config: &mut Option<UpdateConfig>| {
         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(())
     }
 }

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<PathBuf, UpdateError> {
+fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result<PathBuf, InitError> {
     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<dyn ExternalFileProvider>,
     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()
             ))