fix: add separate storage dir for state (#84)

library/include/updater.h

@@ -36,9 +36,15 @@ typedef struct AppParameters {
    */
   int original_libapp_paths_size;
   /**
-   * Path to cache_dir where the updater will store downloaded artifacts.
+   * Path to app storage directory where the updater will store serialized
+   * state and other data that persists between releases.
    */
-  const char *cache_dir;
+  const char *app_storage_dir;
+  /**
+   * Path to cache directory where the updater will store downloaded
+   * artifacts and data that can be deleted when a new release is detected.
+   */
+  const char *code_cache_dir;
 } AppParameters;
 
 #ifdef __cplusplus

library/src/c_api.rs

@@ -37,8 +37,13 @@ pub struct AppParameters {
     /// Length of the original_libapp_paths array.
     pub original_libapp_paths_size: libc::c_int,
 
-    /// Path to cache_dir where the updater will store downloaded artifacts.
-    pub cache_dir: *const libc::c_char,
+    /// Path to app storage directory where the updater will store serialized
+    /// state and other data that persists between releases.
+    pub app_storage_dir: *const libc::c_char,
+
+    /// Path to cache directory where the updater will store downloaded
+    /// artifacts and data that can be deleted when a new release is detected.
+    pub code_cache_dir: *const libc::c_char,
 }
 
 /// Converts a C string to a Rust string, does not free the C string.
@@ -74,7 +79,8 @@ fn app_config_from_c(c_params: *const AppParameters) -> anyhow::Result<updater::
     let c_params_ref = unsafe { &*c_params };
 
     Ok(updater::AppConfig {
-        cache_dir: to_rust(c_params_ref.cache_dir)?,
+        app_storage_dir: to_rust(c_params_ref.app_storage_dir)?,
+        code_cache_dir: to_rust(c_params_ref.code_cache_dir)?,
         release_version: to_rust(c_params_ref.release_version)?,
         original_libapp_paths: to_rust_vector(
             c_params_ref.original_libapp_paths,
@@ -298,7 +304,8 @@ mod test {
         let app_paths = c_array(app_paths_vec);
 
         super::AppParameters {
-            cache_dir: c_string(&cache_dir),
+            app_storage_dir: c_string(&cache_dir),
+            code_cache_dir: c_string(&cache_dir),
             release_version: c_string("1.0.0"),
             original_libapp_paths: app_paths as *const *const libc::c_char,
             original_libapp_paths_size: app_paths_size,
@@ -306,7 +313,8 @@ mod test {
     }
 
     fn free_parameters(params: super::AppParameters) {
-        free_c_string(params.cache_dir as *mut libc::c_char);
+        free_c_string(params.app_storage_dir as *mut libc::c_char);
+        free_c_string(params.code_cache_dir as *mut libc::c_char);
         free_c_string(params.release_version as *mut libc::c_char);
         free_c_array(
             params.original_libapp_paths as *mut *mut libc::c_char,
@@ -331,7 +339,8 @@ mod test {
         testing_reset_config();
         // Should log but not crash.
         let c_params = AppParameters {
-            cache_dir: std::ptr::null(),
+            app_storage_dir: std::ptr::null(),
+            code_cache_dir: std::ptr::null(),
             release_version: std::ptr::null(),
             original_libapp_paths: std::ptr::null(),
             original_libapp_paths_size: 0,

library/src/cache.rs

@@ -162,17 +162,21 @@ impl UpdaterState {
     }
 
     fn create_new_and_save(
-        cache_dir: &Path,
+        storage_dir: &Path,
         release_version: &str,
         client_id: Option<String>,
     ) -> Self {
-        let state = Self::new(cache_dir.to_owned(), release_version.to_owned(), client_id);
+        let state = Self::new(
+            storage_dir.to_owned(),
+            release_version.to_owned(),
+            client_id,
+        );
         let _ = state.save();
         state
     }
 
-    pub fn load_or_new_on_error(cache_dir: &Path, release_version: &str) -> Self {
-        let load_result = Self::load(cache_dir);
+    pub fn load_or_new_on_error(storage_dir: &Path, release_version: &str) -> Self {
+        let load_result = Self::load(storage_dir);
         match load_result {
             Ok(mut loaded) => {
                 let maybe_client_id = loaded.client_id.clone();
@@ -181,12 +185,20 @@ impl UpdaterState {
                         "release_version changed {} -> {}, clearing updater state",
                         loaded.release_version, release_version
                     );
-                    return Self::create_new_and_save(cache_dir, release_version, maybe_client_id);
+                    return Self::create_new_and_save(
+                        storage_dir,
+                        release_version,
+                        maybe_client_id,
+                    );
                 }
                 let validate_result = loaded.validate();
                 if let Err(e) = validate_result {
                     warn!("Error while validating state: {:#}, clearing state.", e);
-                    return Self::create_new_and_save(cache_dir, release_version, maybe_client_id);
+                    return Self::create_new_and_save(
+                        storage_dir,
+                        release_version,
+                        maybe_client_id,
+                    );
                 }
                 loaded
             }
@@ -194,7 +206,7 @@ impl UpdaterState {
                 if !is_file_not_found(&e) {
                     warn!("Error loading state: {:#}, clearing state.", e);
                 }
-                Self::create_new_and_save(cache_dir, release_version, None)
+                Self::create_new_and_save(storage_dir, release_version, None)
             }
         }
     }

library/src/config.rs

@@ -71,7 +71,7 @@ where
 // The config passed into init.  This is immutable once set and copyable.
 #[derive(Debug, Clone)]
 pub struct UpdateConfig {
-    pub cache_dir: PathBuf,
+    pub storage_dir: PathBuf,
     pub download_dir: PathBuf,
     pub auto_update: bool,
     pub channel: String,
@@ -91,12 +91,12 @@ pub fn set_config(
     with_config_mut(|config| {
         anyhow::ensure!(config.is_none(), "shorebird_init has already been called.");
 
-        let mut cache_path = std::path::PathBuf::from(&app_config.cache_dir);
-        cache_path.push("downloads");
-        let download_dir = cache_path;
+        let mut code_cache_path = std::path::PathBuf::from(&app_config.code_cache_dir);
+        code_cache_path.push("downloads");
+        let download_dir = code_cache_path;
 
         let new_config = UpdateConfig {
-            cache_dir: std::path::PathBuf::from(app_config.cache_dir),
+            storage_dir: std::path::PathBuf::from(app_config.app_storage_dir),
             download_dir,
             channel: yaml
                 .channel

library/src/updater.rs

@@ -78,7 +78,8 @@ impl Display for UpdateError {
 // However rusty api would probably used `&str` instead of `String`,
 // but making `&str` from `CStr*` is a bit of a pain.
 pub struct AppConfig {
-    pub cache_dir: String,
+    pub app_storage_dir: String,
+    pub code_cache_dir: String,
     pub release_version: String,
     pub original_libapp_paths: Vec<String>,
 }
@@ -124,7 +125,8 @@ fn check_for_update_internal() -> anyhow::Result<PatchCheckResponse> {
     with_config(|config| {
         // Load UpdaterState from disk
         // If there is no state, make an empty state.
-        let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+        let state =
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         send_patch_check_request(config, &state)
     })
 }
@@ -224,7 +226,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     // racing with us, we should get a new state inside a lock if we want
     // to write.
     let read_only_state =
-        UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+        UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
 
     // We discard any events if we have more than 3 queued to make sure
     // we don't stall the client.
@@ -238,7 +240,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     // We're abusing the config lock as a UpdateState lock for now.
     let read_only_state = with_config(|_| {
         let mut state =
-            UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         // This will clear any events which got queued between the time we
         // loaded the state now, but that's OK for now.
         let result = state.clear_events();
@@ -282,7 +284,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
             number: patch.number,
         };
         let mut state =
-            UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         // Move/state update should be "atomic" (it isn't today).
         state.install_patch(&patch_info)?;
         info!("Patch {} successfully installed.", patch.number);
@@ -350,7 +352,8 @@ where
 /// This may be changed any time `update()` or `start_update_thread()` are called.
 pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
-        let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+        let state =
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         Ok(state.next_boot_patch())
     })
 }
@@ -360,7 +363,8 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
 /// `next_boot_patch`.
 pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
-        let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+        let state =
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         Ok(state.current_boot_patch())
     })
 }
@@ -368,7 +372,7 @@ pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
 pub fn report_launch_start() -> anyhow::Result<()> {
     with_config(|config| {
         let mut state =
-            UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
         // Validate that we have an installed patch.
         // Make that patch the "booted" patch.
         state.activate_current_patch()?;
@@ -382,7 +386,7 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
     info!("Reporting failed launch.");
     with_config(|config| {
         let mut state =
-            UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
 
         let patch =
             state
@@ -419,7 +423,7 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
 pub fn report_launch_success() -> anyhow::Result<()> {
     with_config(|config| {
         let mut state =
-            UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
 
         if let Some(patch) = state.current_boot_patch() {
             if !state.is_known_good_patch(patch.number) {
@@ -485,7 +489,8 @@ mod tests {
         let cache_dir = tmp_dir.path().to_str().unwrap().to_string();
         crate::init(
             crate::AppConfig {
-                cache_dir: cache_dir.clone(),
+                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()],
             },
@@ -511,7 +516,7 @@ mod tests {
             fs::write(&artifact_path, "hello").unwrap();
 
             let mut state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             state
                 .install_patch(&PatchInfo {
                     path: artifact_path,
@@ -580,7 +585,8 @@ mod tests {
         assert_eq!(
             crate::init(
                 crate::AppConfig {
-                    cache_dir: cache_dir.clone(),
+                    app_storage_dir: cache_dir.clone(),
+                    code_cache_dir: cache_dir.clone(),
                     release_version: "1.0.0+1".to_string(),
                     original_libapp_paths: vec!["original_libapp_path".to_string()],
                 },
@@ -624,7 +630,7 @@ mod tests {
             fs::write(&artifact_path, "hello").unwrap();
 
             let mut state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             state
                 .install_patch(&PatchInfo {
                     path: artifact_path,
@@ -642,7 +648,7 @@ mod tests {
         let next_boot_patch = crate::next_boot_patch().unwrap().unwrap();
         with_config(|config| {
             let state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             assert!(!state.is_known_good_patch(next_boot_patch.number));
             Ok(())
         })
@@ -652,7 +658,7 @@ mod tests {
 
         with_config(|config| {
             let state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             assert!(state.is_known_good_patch(next_boot_patch.number));
             Ok(())
         })
@@ -675,7 +681,7 @@ mod tests {
             fs::write(&artifact_path, "hello").unwrap();
 
             let mut state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             state
                 .install_patch(&PatchInfo {
                     path: artifact_path,
@@ -693,7 +699,7 @@ mod tests {
         let next_boot_patch = crate::next_boot_patch().unwrap().unwrap();
         with_config(|config| {
             let state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             // It's not bad yet.
             assert!(!state.is_known_bad_patch(next_boot_patch.number));
             Ok(())
@@ -704,7 +710,7 @@ mod tests {
 
         with_config(|config| {
             let state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             // It's now bad.
             assert!(state.is_known_bad_patch(next_boot_patch.number));
             // And we've queued an event.
@@ -731,7 +737,7 @@ mod tests {
 
         with_config(|config| {
             let mut state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             let fail_event = PatchEvent {
                 app_id: config.app_id.clone(),
                 arch: current_arch().to_string(),
@@ -779,7 +785,7 @@ mod tests {
 
         with_config(|config| {
             let state =
-                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
             // All 5 events should be cleared, even though only 3 were sent.
             assert_eq!(state.copy_events(10).len(), 0);
             Ok(())