diff --git a/library/Cargo.toml b/library/Cargo.toml index cd77b5c..7c74b43 100644 --- a/library/Cargo.toml +++ b/library/Cargo.toml @@ -65,6 +65,7 @@ log-panics = { version = "2", features = ["with-backtrace"] } simple-logging = "2.0.2" [dev-dependencies] +mockall = "0.11.4" # Gives #[serial] attribute for locking all of our shorebird_init # tests to a single thread so they don't conflict with each other. serial_test = "2.0.0" diff --git a/library/src/cache.rs b/library/src/cache.rs index d74e094..c50ac7b 100644 --- a/library/src/cache.rs +++ b/library/src/cache.rs @@ -1,3 +1,12 @@ +mod disk_io; +mod patch_manager; pub mod updater_state; -pub use updater_state::{PatchInfo, UpdaterState}; +pub use updater_state::UpdaterState; + +/// The public interface for talking about patches to the Cache. +#[derive(PartialEq, Eq, Debug, Clone)] +pub struct PatchInfo { + pub path: std::path::PathBuf, + pub number: usize, +} diff --git a/library/src/cache/disk_io.rs b/library/src/cache/disk_io.rs new file mode 100644 index 0000000..e8cb427 --- /dev/null +++ b/library/src/cache/disk_io.rs @@ -0,0 +1,100 @@ +use anyhow::{bail, Context}; +use serde::{de::DeserializeOwned, Serialize}; +use std::{ + fs::File, + io::{BufReader, BufWriter}, + path::Path, +}; + +// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests +#[cfg(test)] +use std::println as debug; // Workaround to use println! for logs. + +pub fn write(serializable: &S, path: &P) -> anyhow::Result<()> +where + S: ?Sized + Serialize, + P: AsRef, +{ + debug!("Writing to {:?}", path.as_ref()); + + let path_as_ref = path.as_ref(); + let containing_dir = path_as_ref + .parent() + .with_context(|| format!("Failed to get parent dir for {:?}", path_as_ref))?; + + // Because File::create can sometimes fail if the full directory path doesn't exist, + // we create the directories in its path first. + std::fs::create_dir_all(containing_dir) + .with_context(|| format!("Failed to create dir {:?}", path_as_ref))?; + + let file = File::create(path).with_context(|| format!("File::create for {:?}", path_as_ref))?; + let writer = BufWriter::new(file); + serde_json::to_writer_pretty(writer, serializable) + .with_context(|| format!("failed to serialize to {:?}", path_as_ref)) +} + +pub fn read(path: &P) -> anyhow::Result +where + D: DeserializeOwned, + P: AsRef, +{ + debug!("Reading from {:?}", path.as_ref()); + + let path_as_ref = path.as_ref(); + if !path_as_ref.exists() { + bail!("File {} does not exist", path_as_ref.display()); + } + + let file = File::open(path_as_ref)?; + let reader = BufReader::new(file); + serde_json::from_reader(reader) + .with_context(|| format!("failed to deserialize from {:?}", &path_as_ref)) +} + +#[cfg(test)] +mod test { + use std::path::Path; + + use serde::{Deserialize, Serialize}; + use tempdir::TempDir; + + use anyhow::{Ok, Result}; + + #[derive(Serialize, Deserialize, PartialEq, Eq)] + struct TestStruct { + a: u32, + b: String, + } + + #[test] + fn writes_and_reads_serialized_object() -> Result<()> { + let test_struct = TestStruct { + a: 1, + b: "hello".to_string(), + }; + let temp_dir = TempDir::new("test")?; + let path = temp_dir.path().join("test.json"); + super::write(&test_struct, &path)?; + let read_struct: TestStruct = super::read(&path)?; + + assert!(test_struct == read_struct); + + Ok(()) + } + + #[test] + fn read_errs_if_file_doesnt_exist() { + assert!(super::read::(&Path::new("nonexistent.json")).is_err()); + } + + #[test] + fn read_errs_if_struct_cannot_be_deserialized() -> Result<()> { + let temp_dir = TempDir::new("test")?; + let path = &temp_dir.path().join("test.json"); + std::fs::write(path, "junk")?; + + assert!(super::read::(&path).is_err()); + + Ok(()) + } +} diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs new file mode 100644 index 0000000..da7dc47 --- /dev/null +++ b/library/src/cache/patch_manager.rs @@ -0,0 +1,908 @@ +use super::{disk_io, PatchInfo}; +use anyhow::{bail, Context, Result}; +use core::fmt::Debug; +use serde::{Deserialize, Serialize}; +use std::path::{Path, PathBuf}; + +#[cfg(test)] +use mockall::automock; +#[cfg(test)] +use tempdir::TempDir; + +// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests +#[cfg(test)] +use std::{println as info, println as error}; // Workaround to use println! for logs. + +const PATCHES_DIR_NAME: &str = "patches"; +const PATCHES_STATE_FILE_NAME: &str = "patches_state.json"; +const PATCH_ARTIFACT_FILENAME: &str = "dlc.vmcode"; + +#[derive(Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, Hash)] +struct PatchMetadata { + /// The number of the patch. + number: usize, + + /// The size of the patch artifact on disk. + size: u64, +} + +/// What gets serialized to disk +#[derive(Debug, Default, Deserialize, Serialize)] +struct PatchesState { + /// The patch we are currently running, if any. + last_booted_patch: Option, + + /// The patch that will be run on the next app boot, if any. This may be the same + /// as the last booted patch patch if no new patch has been downloaded. + next_boot_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, +} + +/// Abstracts the process of managing patches. +#[cfg_attr(test, automock)] +pub trait ManagePatches { + /// Copies the patch file at file_path to the manager's directory structure sets + /// this patch as the next patch to boot. + fn add_patch(&mut self, number: usize, file_path: &Path) -> Result<()>; + + /// Returns the patch we most recently successfully booted from (usually the currently running patch), + /// or None if no patch is installed. + fn last_successfully_booted_patch(&self) -> Option; + + /// Returns the next patch to boot, or None if: + /// - no patches have been downloaded + /// - the patch on disk is not bootable + fn next_boot_patch(&mut self) -> Option; + + /// Records that the patch with number patch_number booted successfully and is + /// safe to use for future boots. + fn record_boot_success_for_patch(&mut self, patch_number: usize) -> Result<()>; + + /// Records that the patch with number patch_number failed to boot, and ensures + /// that it will never be returned as the next boot or last booted patch. + fn record_boot_failure_for_patch(&mut self, patch_number: usize) -> Result<()>; + + /// The highest patch number that has been added. This may be higher than the + /// last booted or next boot patch if we downloaded a patch that failed to boot. + fn highest_seen_patch_number(&self) -> Option; + + /// Resets the patch manager to its initial state, removing all patches. This is + /// intended to be used when a new release version is installed. + fn reset(&mut self) -> Result<()>; +} + +// This allows us to use the Debug trait on dyn ManagePatches, which is +// required to have it as a property of UpdaterState. +impl Debug for dyn ManagePatches { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "ManagePatches") + } +} + +#[derive(Debug)] +pub struct PatchManager { + /// The base directory used to store patch artifacts and state. + /// The directory structure created within this directory is: + /// patches_state.json + /// patches/ + /// / + /// dlc.vmcode + /// / + /// dlc.vmcode + root_dir: PathBuf, + + /// Metadata about the patches we have downloaded that is persisted to disk. + patches_state: PatchesState, +} + +impl PatchManager { + /// Creates a new PatchManager with the given root directory. This directory is + /// assumed to exist. The PatchManager will use this directory to store its + /// state and patch binaries. + pub fn with_root_dir(root_dir: PathBuf) -> Self { + let patches_state = Self::load_patches_state(&root_dir).unwrap_or_default(); + + Self { + root_dir, + patches_state, + } + } + + fn load_patches_state(root_dir: &Path) -> Option { + let path = root_dir.join(PATCHES_STATE_FILE_NAME); + match disk_io::read(&path) { + Ok(maybe_state) => maybe_state, + Err(e) => { + error!( + "Failed to load patches state from {}: {}", + path.display(), + e + ); + None + } + } + } + + fn save_patches_state(&self) -> Result<()> { + let path = self.root_dir.join(PATCHES_STATE_FILE_NAME); + disk_io::write(&self.patches_state, &path) + } + + /// The directory where all patch artifacts are stored. + fn patches_dir(&self) -> PathBuf { + self.root_dir.join(PATCHES_DIR_NAME) + } + + /// The directory where artifacts for the patch with the given number are stored. + fn patch_dir(&self, patch_number: usize) -> PathBuf { + self.patches_dir().join(patch_number.to_string()) + } + + /// The path to the runnable patch artifact with the given number. Runnable patch artifact files are + /// named .vmcode + fn patch_artifact_path(&self, patch_number: usize) -> PathBuf { + self.patch_dir(patch_number).join(PATCH_ARTIFACT_FILENAME) + } + + fn patch_info_for_number(&self, patch_number: usize) -> PatchInfo { + PatchInfo { + path: self.patch_artifact_path(patch_number), + number: patch_number, + } + } + + /// Checks that the patch with the given number: + /// - Has an artifact on disk + /// - That artifact on disk is the same size it was when it was installed + /// + /// Returns Ok if the patch is bootable, or an error if it is not. + fn validate_patch_is_bootable(&self, patch: &PatchMetadata) -> Result<()> { + let artifact_path = self.patch_artifact_path(patch.number); + if !Path::exists(&artifact_path) { + bail!( + "Patch {} does not exist at {}", + patch.number, + artifact_path.display() + ); + } + + let artifact_size_on_disk = std::fs::metadata(&artifact_path)?.len(); + if artifact_size_on_disk != patch.size { + bail!( + "Patch {} has size {} on disk, but expected size {}", + patch.number, + artifact_size_on_disk, + patch.size + ); + } + + Ok(()) + } + + fn delete_patch_artifacts(&mut self, patch_number: usize) -> Result<()> { + info!("Deleting patch artifacts for patch {}", patch_number); + + let patch_dir = self.patch_dir(patch_number); + + std::fs::remove_dir_all(&patch_dir) + .map_err(|e| { + error!("Failed to delete patch dir {}: {}", patch_dir.display(), e); + e + }) + .with_context(|| format!("Failed to delete patch dir {}", &patch_dir.display())) + } + + /// Attempts to use the last successfully booted patch as the next boot 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. + fn try_fall_back_to_last_booted_patch(&mut self) { + if let Some(next_boot_patch) = self.patches_state.next_boot_patch { + // If we have a next_boot_patch that we're falling back from, delete its artifacts. + let _ = self.delete_patch_artifacts(next_boot_patch.number); + self.patches_state.next_boot_patch = None; + + if let Some(last_boot_patch) = self.patches_state.last_booted_patch { + if last_boot_patch.number == next_boot_patch.number { + // If the last booted patch is the same as the next boot patch, clear it. + self.patches_state.last_booted_patch = None; + } + } + } + + if let Some(last_boot_patch) = self.patches_state.last_booted_patch { + if self.validate_patch_is_bootable(&last_boot_patch).is_ok() { + // If we think we can still boot from the last booted patch, set it as the next_boot_patch. + self.patches_state.next_boot_patch = Some(last_boot_patch); + } else { + self.patches_state.last_booted_patch = None; + let _ = self.delete_patch_artifacts(last_boot_patch.number); + } + } + } +} + +impl ManagePatches for PatchManager { + fn add_patch(&mut self, patch_number: usize, file_path: &Path) -> Result<()> { + if !file_path.exists() { + bail!("Patch file {} does not exist", file_path.display()); + } + + let patch_path = self.patch_artifact_path(patch_number); + + std::fs::create_dir_all(self.patch_dir(patch_number)) + .with_context(|| format!("create_dir_all failed for {}", patch_path.display()))?; + + std::fs::rename(file_path, &patch_path)?; + + let new_patch = PatchMetadata { + number: patch_number, + size: std::fs::metadata(&patch_path)?.len(), + }; + + // If a patch was never booted (next_boot_patch != last_booted_patch), we should delete + // it here before setting next_boot_patch to the new patch. + if let (Some(last_boot_patch), Some(next_boot_patch)) = ( + self.patches_state.next_boot_patch, + self.patches_state.last_booted_patch, + ) { + if last_boot_patch.number != next_boot_patch.number { + let _ = self.delete_patch_artifacts(next_boot_patch.number); + } + } + + self.patches_state.next_boot_patch = Some(new_patch); + self.patches_state.highest_seen_patch_number = self + .patches_state + .highest_seen_patch_number + .map(|highest_patch_number: usize| highest_patch_number.max(patch_number)) + .or(Some(patch_number)); + self.save_patches_state() + } + + fn last_successfully_booted_patch(&self) -> Option { + self.patches_state + .last_booted_patch + .map(|patch| self.patch_info_for_number(patch.number)) + } + + fn next_boot_patch(&mut self) -> Option { + let next_boot_patch = match self.patches_state.next_boot_patch { + Some(patch) => patch, + None => return None, + }; + + if let Err(e) = self.validate_patch_is_bootable(&next_boot_patch) { + error!("Patch {} is not bootable: {}", next_boot_patch.number, e); + + self.try_fall_back_to_last_booted_patch(); + + if let Err(e) = self.save_patches_state() { + error!("Failed to save patches state: {}", e); + } + return None; + } + + self.patches_state + .next_boot_patch + .as_ref() + .map(|patch| self.patch_info_for_number(patch.number)) + } + + fn record_boot_success_for_patch(&mut self, patch_number: usize) -> Result<()> { + let next_boot_patch = self + .patches_state + .next_boot_patch + .context("No next_boot_patch")?; + + if next_boot_patch.number != patch_number { + bail!( + "Attempted to record boot success for patch {} but next_boot_patch is {}", + patch_number, + next_boot_patch.number + ); + } + + if let Some(current_patch) = self.patches_state.last_booted_patch { + if current_patch.number != patch_number { + // If we now have a new last_booted_patch, delete the old one's artifacts. + let _ = self.delete_patch_artifacts(current_patch.number); + } + } + + self.patches_state.last_booted_patch = Some(next_boot_patch); + self.save_patches_state() + } + + fn record_boot_failure_for_patch(&mut self, patch_number: usize) -> Result<()> { + let next_boot_patch = self + .patches_state + .next_boot_patch + .context("No next_boot_patch")?; + + if next_boot_patch.number != patch_number { + bail!( + "Attempted to record boot failure for patch {} but should have booted from {}", + patch_number, + next_boot_patch.number + ); + } + + self.try_fall_back_to_last_booted_patch(); + + self.save_patches_state() + } + + fn highest_seen_patch_number(&self) -> Option { + self.patches_state.highest_seen_patch_number + } + + fn reset(&mut self) -> Result<()> { + self.patches_state = PatchesState::default(); + self.save_patches_state()?; + std::fs::remove_dir_all(self.patches_dir()).with_context(|| { + format!( + "Failed to delete patches dir {}", + self.patches_dir().display() + ) + }) + } +} + +#[cfg(test)] +impl PatchManager { + pub fn manager_for_test(temp_dir: &TempDir) -> PatchManager { + PatchManager::with_root_dir(temp_dir.path().to_owned()) + } + + pub fn add_patch_for_test(&mut self, temp_dir: &TempDir, patch_number: usize) -> Result<()> { + let file_path = &temp_dir + .path() + .join(format!("patch{}.vmcode", patch_number)); + std::fs::write(file_path, patch_number.to_string().repeat(patch_number)).unwrap(); + self.add_patch(patch_number, file_path) + } +} + +#[cfg(test)] +mod debug_tests { + use tempdir::TempDir; + + use super::PatchManager; + + #[test] + fn manage_patches_is_debug() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let patch_manager: Box = Box::new( + super::PatchManager::with_root_dir(temp_dir.path().to_owned()), + ); + assert_eq!(format!("{:?}", patch_manager), "ManagePatches"); + } + + #[test] + fn patch_manager_is_debug() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let patch_manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let expected_str = format!( + "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }} }}", + temp_dir.path().display() + ); + assert_eq!(format!("{:?}", patch_manager), expected_str); + } +} + +#[cfg(test)] +mod add_patch_tests { + use super::*; + use std::path::Path; + use tempdir::TempDir; + + #[test] + fn errs_if_file_path_does_not_exist() { + let mut manager = PatchManager::manager_for_test(&TempDir::new("patch_manager").unwrap()); + assert!(manager + .add_patch(1, Path::new("/path/to/file/that/does/not/exist")) + .is_err()); + } + + #[test] + fn adds_patch_successfully() { + let patch_number = 1; + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents).unwrap(); + + assert!(manager + .add_patch(patch_number, Path::new(file_path)) + .is_ok()); + + assert_eq!( + manager.patches_state.next_boot_patch, + Some(PatchMetadata { + number: patch_number, + size: patch_file_contents.len() as u64 + }) + ); + assert!(!file_path.exists()); + assert_eq!(manager.highest_seen_patch_number(), Some(patch_number)); + } + + #[test] + fn does_not_set_higher_highest_seen_patch_number_if_added_patch_is_lower() -> Result<()> { + let patch_file_contents = "patch contents"; + + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + assert!(manager.highest_seen_patch_number().is_none()); + + // Add patch 1 + let file_path = &temp_dir.path().join("patch.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(1, file_path).is_ok()); + assert_eq!(manager.highest_seen_patch_number(), Some(1)); + + // Add patch 4, expect 4 to be the highest patch number we've seen + let file_path = &temp_dir.path().join("patch.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(4, file_path).is_ok()); + assert_eq!(manager.highest_seen_patch_number(), Some(4)); + + // Add patch 3, expect 4 to still be the highest patch number we've seen + let file_path = &temp_dir.path().join("patch.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(3, file_path).is_ok()); + assert_eq!(manager.highest_seen_patch_number(), Some(4)); + + Ok(()) + } +} + +#[cfg(test)] +mod last_successfully_booted_patch_tests { + use super::*; + use tempdir::TempDir; + + #[test] + fn returns_none_if_no_patch_has_been_booted() -> Result<()> { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + assert!(manager.last_successfully_booted_patch().is_none()); + + Ok(()) + } + + #[test] + fn returns_value_from_patches_state() -> Result<()> { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + + let expected = PatchInfo { + path: manager.patch_artifact_path(1), + number: 1, + }; + manager.patches_state.last_booted_patch = manager.patches_state.next_boot_patch; + assert_eq!(manager.last_successfully_booted_patch(), Some(expected)); + + Ok(()) + } +} + +#[cfg(test)] +mod get_next_boot_patch_tests { + use super::*; + use anyhow::Result; + use tempdir::TempDir; + + #[test] + fn returns_none_if_no_next_boot_patch() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + assert!(manager.next_boot_patch().is_none()); + } + + #[test] + fn returns_none_if_next_boot_patch_is_not_bootable() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + + // Write junk to the artifact, this should render the patch unbootable in the eyes + // of the PatchManager. + let artifact_path = manager.patch_artifact_path(1); + std::fs::write(&artifact_path, "junk")?; + + assert!(manager.next_boot_patch().is_none()); + + // Ensure the internal state is cleared. + assert!(manager.patches_state.next_boot_patch.is_none()); + + // The artifact should have been deleted. + assert!(!&artifact_path.exists()); + + Ok(()) + } + + #[test] + fn clears_current_and_next_on_boot_failure_if_they_are_the_same() -> Result<()> { + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(1, file_path).is_ok()); + + // Write junk to the artifact, this should render the patch unbootable in the eyes + // of the PatchManager. + let artifact_path = manager.patch_artifact_path(1); + std::fs::write(&artifact_path, "junk")?; + + assert!(manager.next_boot_patch().is_none()); + + // Ensure the internal state is cleared. + assert!(manager.patches_state.next_boot_patch.is_none()); + assert!(manager.patches_state.last_booted_patch.is_none()); + + // The artifact should have been deleted. + assert!(!&artifact_path.exists()); + + Ok(()) + } + + #[test] + fn falls_back_to_last_booted_patch_if_still_bootable() -> Result<()> { + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + + // Add patch 1, pretend it booted successfully. + assert!(manager.add_patch(1, file_path).is_ok()); + assert!(manager.record_boot_success_for_patch(1).is_ok()); + + // Add patch 2, pretend it failed to boot. + let file_path = &temp_dir.path().join("patch2.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(2, file_path).is_ok()); + assert!(manager.record_boot_failure_for_patch(2).is_ok()); + + // Verify that we will next attempt to boot from patch 1. + assert_eq!(manager.next_boot_patch().unwrap().number, 1); + + Ok(()) + } + + #[test] + fn does_not_fall_back_to_last_booted_patch_if_corrupted() -> Result<()> { + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + + // Add patch 1, pretend it booted successfully. + assert!(manager.add_patch(1, file_path).is_ok()); + assert!(manager.record_boot_success_for_patch(1).is_ok()); + + // Add patch 2, pretend it failed to boot. + let file_path = &temp_dir.path().join("patch2.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(2, file_path).is_ok()); + assert!(manager.record_boot_failure_for_patch(2).is_ok()); + + // Write junk to patch 1's artifact. This should prevent us from falling back to it. + let patch_1_artifact_path = manager.patch_artifact_path(1); + std::fs::write(patch_1_artifact_path, "junk")?; + + // Verify that we will not attempt to boot from either patch. + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } +} + +#[cfg(test)] +mod fall_back_tests { + use super::*; + + #[test] + fn does_nothing_if_no_patch_exists() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + assert!(manager.patches_state.last_booted_patch.is_none()); + assert!(manager.patches_state.next_boot_patch.is_none()); + + manager.try_fall_back_to_last_booted_patch(); + + assert!(manager.patches_state.last_booted_patch.is_none()); + assert!(manager.patches_state.next_boot_patch.is_none()); + + 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::with_root_dir(temp_dir.path().to_owned()); + + assert!(manager.patches_state.next_boot_patch.is_none()); + + manager.patches_state.last_booted_patch = Some(PatchMetadata { number: 1, size: 1 }); + manager.try_fall_back_to_last_booted_patch(); + + 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")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + manager.add_patch_for_test(&temp_dir, 1)?; + manager.record_boot_success_for_patch(1)?; + manager.add_patch_for_test(&temp_dir, 2)?; + + manager.try_fall_back_to_last_booted_patch(); + + assert_eq!(manager.patches_state.last_booted_patch.unwrap().number, 1); + assert_eq!(manager.patches_state.next_boot_patch.unwrap().number, 1); + + Ok(()) + } + + #[test] + fn clears_next_and_last_patches_if_both_fail_validation() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + manager.add_patch_for_test(&temp_dir, 1)?; + manager.record_boot_success_for_patch(1)?; + let patch_1_path = manager.patch_artifact_path(1); + std::fs::write(patch_1_path, "junkjunkjunk")?; + + manager.add_patch_for_test(&temp_dir, 2)?; + + manager.try_fall_back_to_last_booted_patch(); + + assert!(manager.patches_state.last_booted_patch.is_none()); + assert!(manager.patches_state.next_boot_patch.is_none()); + + Ok(()) + } + + #[test] + fn succeeds_if_deleting_artifacts_fails() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + manager.add_patch_for_test(&temp_dir, 1)?; + manager.record_boot_success_for_patch(1)?; + + manager.add_patch_for_test(&temp_dir, 2)?; + + let patch_dir = manager.patch_dir(1); + std::fs::remove_dir_all(patch_dir)?; + let patch_dir = manager.patch_dir(2); + std::fs::remove_dir_all(patch_dir)?; + + manager.try_fall_back_to_last_booted_patch(); + + assert!(manager.patches_state.last_booted_patch.is_none()); + assert!(manager.patches_state.next_boot_patch.is_none()); + + Ok(()) + } +} + +#[cfg(test)] +mod record_boot_success_for_patch_tests { + use super::*; + use anyhow::{Ok, Result}; + use tempdir::TempDir; + + #[test] + fn errs_if_no_next_boot_patch() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + + // This should fail because no patches have been added. + assert!(manager.record_boot_success_for_patch(1).is_err()); + + Ok(()) + } + + #[test] + fn errs_if_patch_number_does_not_match_next_patch() -> Result<()> { + let patch_number = 1; + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(patch_number, file_path).is_ok()); + assert!(manager + .record_boot_success_for_patch(patch_number + 1) + .is_err()); + + Ok(()) + } + + #[test] + fn succeeds_when_provided_next_boot_patch_number() -> Result<()> { + let patch_number = 1; + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + assert!(manager.add_patch(patch_number, file_path).is_ok()); + + assert!(manager.record_boot_success_for_patch(patch_number).is_ok()); + + Ok(()) + } + + #[test] + fn repeated_calls_to_record_success_succeed() -> Result<()> { + let patch_number = 1; + let patch_file_contents = "patch contents"; + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, patch_file_contents)?; + + // Add the patch, make sure it has an artifact. + assert!(manager.add_patch(patch_number, file_path).is_ok()); + let patch_artifact_path = manager.patch_artifact_path(patch_number); + assert!(patch_artifact_path.exists()); + + // Record success, make sure the artifact still exists. + assert!(manager.record_boot_success_for_patch(patch_number).is_ok()); + assert_eq!( + manager.last_successfully_booted_patch().unwrap().number, + patch_number + ); + assert_eq!(manager.next_boot_patch().unwrap().number, patch_number); + assert!(patch_artifact_path.exists()); + + // Record another success, make sure the artifact still exists. + assert!(manager.record_boot_success_for_patch(patch_number).is_ok()); + assert_eq!( + manager.last_successfully_booted_patch().unwrap().number, + patch_number + ); + assert_eq!(manager.next_boot_patch().unwrap().number, patch_number); + assert!(patch_artifact_path.exists()); + + Ok(()) + } +} + +#[cfg(test)] +mod record_boot_failure_for_patch_tests { + use super::*; + use anyhow::{Ok, Result}; + use tempdir::TempDir; + + #[test] + fn errs_if_no_next_boot_patch() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + assert!(manager.record_boot_failure_for_patch(1).is_err()); + + Ok(()) + } + + #[test] + fn errs_if_patch_number_does_not_match_next_boot_patch() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + + assert!(manager.record_boot_failure_for_patch(2).is_err()); + + Ok(()) + } + + #[test] + fn deletes_failed_patch_artifacts() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + assert!(manager.record_boot_success_for_patch(1).is_ok()); + let succeeded_patch_artifact_path = manager.patch_artifact_path(1); + + manager.add_patch_for_test(&temp_dir, 2)?; + let failed_patch_artifact_path = manager.patch_artifact_path(2); + + // Make sure patch artifacts exist + assert!(failed_patch_artifact_path.exists()); + assert!(succeeded_patch_artifact_path.exists()); + + assert!(manager.record_boot_failure_for_patch(2).is_ok()); + assert!(!failed_patch_artifact_path.exists()); + + Ok(()) + } + + #[test] + fn clears_last_booted_patch_if_it_is_the_failed_patch() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + let patch_artifact_path = manager.patch_artifact_path(1); + + // Pretend we booted from this patch + assert!(manager.record_boot_success_for_patch(1).is_ok()); + assert_eq!(manager.last_successfully_booted_patch().unwrap().number, 1); + assert_eq!(manager.next_boot_patch().unwrap().number, 1); + assert!(patch_artifact_path.exists()); + + // Now pretend it failed to boot + assert!(manager.record_boot_failure_for_patch(1).is_ok()); + assert!(manager.last_successfully_booted_patch().is_none()); + assert!(manager.next_boot_patch().is_none()); + assert!(!patch_artifact_path.exists()); + + Ok(()) + } +} + +#[cfg(test)] +mod highest_seen_patch_number_tests { + use super::*; + use anyhow::{Ok, Result}; + use tempdir::TempDir; + + #[test] + fn returns_value_from_internal_state() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + + assert!(manager.patches_state.highest_seen_patch_number.is_none()); + assert!(manager.highest_seen_patch_number().is_none()); + + manager.patches_state.highest_seen_patch_number = Some(1); + assert_eq!(manager.highest_seen_patch_number(), Some(1)); + + Ok(()) + } +} + +#[cfg(test)] +mod reset_tests { + use super::*; + use anyhow::{Ok, Result}; + use tempdir::TempDir; + + #[test] + fn deletes_patches_dir_and_resets_patches_state() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::manager_for_test(&temp_dir); + manager.add_patch_for_test(&temp_dir, 1)?; + let path_artifacts_dir = manager.patches_dir(); + + // Make sure the directory and artifact files were created + assert!(path_artifacts_dir.exists()); + assert_eq!(std::fs::read_dir(&path_artifacts_dir).unwrap().count(), 1); + + assert!(manager.reset().is_ok()); + + // Make sure the directory and artifact files were deleted + assert!(!path_artifacts_dir.exists()); + + Ok(()) + } +} diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 2a2bdc2..be185df 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -5,48 +5,49 @@ // consistent and use patch number everywhere. // PatchInfo can probably go away. -use std::fs::File; -use std::io::{BufReader, BufWriter}; use std::path::{Path, PathBuf}; -use anyhow::{bail, Context, Result}; +use anyhow::Result; use serde::{Deserialize, Serialize}; use crate::events::PatchEvent; -use crate::updater::UpdateError; // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests #[cfg(test)] -use std::{println as info, println as warn, println as debug}; // Workaround to use println! for logs. +use std::{println as info, println as warn}; // Workaround to use println! for logs. +use super::patch_manager::{ManagePatches, PatchManager}; +use super::{disk_io, PatchInfo}; + +/// Where the updater state is stored on disk. const STATE_FILE_NAME: &str = "state.json"; -/// The public interface for talking about patches to the Cache. -#[derive(PartialEq, Debug)] -pub struct PatchInfo { - pub path: PathBuf, - pub number: usize, -} - -/// The private interface onto slots/patches within the cache. -#[derive(Deserialize, Serialize, Default, Clone, Debug)] -struct Slot { - /// Patch number for the patch in this slot. - patch_number: usize, -} - +/// Records the updater's "state of the world" - which patches we know to be +/// good or bad, which patches we have downloaded, which patch we're currently +/// booted from, events that need to be reported to the server, etc. +/// // This struct is public, as callers can have a handle to it, but modifying // anything inside should be done via the functions below. // TODO(eseidel): Split the per-release state from the per-device state. // That way per-release state is reset when the release version changes. // but per-device state is not. -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug)] pub struct UpdaterState { // Per-device state: /// Where this writes to disk. Don't serialize this field, as it can change /// between runs of the app. - #[serde(skip)] cache_dir: PathBuf, + + patch_manager: Box, + + serialized_state: SerializedState, +} + +/// UpdaterState fields that are serialized to disk. +/// +/// Written out to disk as a json file at STATE_FILE_NAME. +#[derive(Debug, Deserialize, Serialize)] +struct SerializedState { /// The client ID for this device. pub client_id: Option, @@ -55,17 +56,6 @@ pub struct UpdaterState { /// If this does not match the release version we're booting from we will /// clear the cache. release_version: String, - /// List of patches that failed to boot. We will never attempt these again. - failed_patches: Vec, - /// List of patches that successfully booted. We will never rollback past - /// one of these for this device. - successful_patches: Vec, - /// Slot that the app is currently booted from. - current_boot_slot_index: Option, - /// Slot that will be used for next boot. - next_boot_slot_index: Option, - /// List of slots. - slots: Vec, /// Events that have not yet been sent to the server. /// Format could change between releases, so this is per-release state. queued_events: Vec, @@ -84,88 +74,39 @@ fn generate_client_id() -> String { uuid::Uuid::new_v4().to_string() } +/// Lifecycle methods for the updater state. impl UpdaterState { /// Creates a new `UpdaterState`. If `client_id` is None, a new one will be generated. fn new(cache_dir: PathBuf, release_version: String, client_id: Option) -> Self { Self { - cache_dir, - release_version, - client_id: client_id.or(Some(generate_client_id())), - current_boot_slot_index: None, - next_boot_slot_index: None, - queued_events: Vec::new(), - failed_patches: Vec::new(), - successful_patches: Vec::new(), - slots: Vec::new(), + cache_dir: cache_dir.clone(), + patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.clone())), + serialized_state: SerializedState { + client_id: client_id.or(Some(generate_client_id())), + release_version, + queued_events: Vec::new(), + }, } } - pub fn client_id_or_default(&self) -> String { - self.client_id.clone().unwrap_or(String::new()) - } - - pub fn is_known_good_patch(&self, patch_number: usize) -> bool { - self.successful_patches.iter().any(|v| v == &patch_number) - } - - pub fn is_known_bad_patch(&self, patch_number: usize) -> bool { - self.failed_patches.iter().any(|v| v == &patch_number) - } - - pub fn queue_event(&mut self, event: PatchEvent) { - self.queued_events.push(event); - } - - pub fn copy_events(&self, limit: usize) -> Vec { - self.queued_events.iter().take(limit).cloned().collect() - } - - pub fn clear_events(&mut self) -> Result<()> { - self.queued_events.clear(); - self.save() - } - - pub fn mark_patch_as_bad(&mut self, patch_number: usize) -> Result<()> { - if self.is_known_good_patch(patch_number) { - bail!("Tried to report failed launch for a known good patch. Ignoring."); - } - - if !self.is_known_bad_patch(patch_number) { - // This is at least info! since we're in a failure state and want to log. - info!("Marking patch {} as bad", patch_number); - self.failed_patches.push(patch_number); - } - Ok(()) - } - - pub fn mark_patch_as_good(&mut self, patch_number: usize) -> Result<()> { - if self.is_known_bad_patch(patch_number) { - bail!("Tried to report successful launch for a known bad patch. Ignoring."); - } - - if !self.is_known_good_patch(patch_number) { - self.successful_patches.push(patch_number); - } - Ok(()) - } - + /// Loads UpdaterState from disk fn load(cache_dir: &Path) -> anyhow::Result { - // Load UpdaterState from disk let path = cache_dir.join(STATE_FILE_NAME); - let file = File::open(path)?; - let reader = BufReader::new(file); - // TODO: Now that we depend on serde_yaml for shorebird.yaml - // we could use yaml here instead of json. - let mut state: UpdaterState = serde_json::from_reader(reader)?; - state.cache_dir = cache_dir.to_path_buf(); - if state.client_id.is_none() { + let serialized_state = disk_io::read(&path)?; + let mut state = UpdaterState { + cache_dir: cache_dir.to_path_buf(), + patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.to_path_buf())), + serialized_state, + }; + if state.serialized_state.client_id.is_none() { // Generate a client id if we don't already have one. - state.client_id = Some(generate_client_id()); + state.serialized_state.client_id = Some(generate_client_id()); let _ = state.save(); } Ok(state) } + /// Initializes a new UpdaterState and saves it to disk. fn create_new_and_save( storage_dir: &Path, release_version: &str, @@ -176,7 +117,9 @@ impl UpdaterState { release_version.to_owned(), client_id, ); - let _ = state.save(); + if let Err(e) = state.save() { + warn!("Error saving state {:?}, ignoring.", e); + } state } @@ -184,21 +127,13 @@ impl UpdaterState { let load_result = Self::load(storage_dir); match load_result { Ok(mut loaded) => { - let maybe_client_id = loaded.client_id.clone(); - if loaded.release_version != release_version { + let maybe_client_id = loaded.serialized_state.client_id.clone(); + if loaded.serialized_state.release_version != release_version { info!( "release_version changed {} -> {}, clearing updater state", - loaded.release_version, release_version + loaded.serialized_state.release_version, release_version ); - 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); + let _ = loaded.patch_manager.reset(); return Self::create_new_and_save( storage_dir, release_version, @@ -218,26 +153,33 @@ impl UpdaterState { /// Saves the updater state to disk. pub fn save(&self) -> anyhow::Result<()> { - std::fs::create_dir_all(&self.cache_dir) - .with_context(|| format!("create_dir_all failed for {}", self.cache_dir.display()))?; - let path = Path::new(&self.cache_dir).join("state.json"); - let file = File::create(path).context("File::create for state.json")?; - let writer = BufWriter::new(file); - serde_json::to_writer_pretty(writer, self)?; - Ok(()) + let path = Path::new(&self.cache_dir).join(STATE_FILE_NAME); + disk_io::write(&self.serialized_state, &path) + } +} + +/// Serialized updater state +impl UpdaterState { + pub fn client_id_or_default(&self) -> String { + self.serialized_state + .client_id + .clone() + .unwrap_or(String::new()) + } +} + +/// Patch management. All patch management is done via the patch manager. +impl UpdaterState { + /// Records that the patch with patch_number failed to boot, uninstalls the patch. + pub fn record_boot_failure_for_patch(&mut self, patch_number: usize) -> Result<()> { + self.patch_manager + .record_boot_failure_for_patch(patch_number) } - fn patch_info_at(&self, index: usize) -> Option { - if index >= self.slots.len() { - return None; - } - let slot = &self.slots[index]; - // to_str only ever fails if the path is invalid utf8, which should - // never happen, but this way we don't crash if it is. - Some(PatchInfo { - path: self.patch_path_for_index(index), - number: slot.patch_number, - }) + /// Records that the patch with patch_number was successfully booted, marks the patch as "good". + pub fn record_boot_success_for_patch(&mut self, patch_number: usize) -> Result<()> { + self.patch_manager + .record_boot_success_for_patch(patch_number) } /// This is the current patch that is running. @@ -245,209 +187,21 @@ impl UpdaterState { /// - 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 { - if let Some(slot_index) = self.current_boot_slot_index { - return self.patch_info_at(slot_index); - } - None + 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. /// - There was a patch selected but it was later marked as bad. - pub fn next_boot_patch(&self) -> Option { - if let Some(slot_index) = self.next_boot_slot_index { - return self.patch_info_at(slot_index); - } - None - } - - fn validate(&mut self) -> anyhow::Result<()> { - // iterate through all slots: - // Make sure they're still valid. - // If not, remove them. - let slot_count = self.slots.len(); - let mut needs_save = false; - // Iterate backwards so we can remove slots. - for i in (0..slot_count).rev() { - let slot = &self.slots[i]; - if !self.validate_slot(slot) { - warn!("Slot {} is invalid, clearing.", i); - self.clear_slot(i)?; - needs_save = true; - } - } - if needs_save { - self.save()?; - } - Ok(()) - } - - fn validate_slot(&self, slot: &Slot) -> bool { - // Check if the patch is known bad. - if self.is_known_bad_patch(slot.patch_number) { - debug!("Slot {:?} is known bad.", slot); - return false; - } - let index = self - .slots - .iter() - .position(|s| s.patch_number == slot.patch_number); - let patch_path = self.patch_path_for_index(index.unwrap()); - if !patch_path.exists() { - debug!("Slot {:?} {} does not exist.", slot, patch_path.display()); - return false; - } - // TODO: This should also check if the hash matches? - // let hash = compute_hash(&PathBuf::from(&slot.path)); - // if let Ok(hash) = hash { - // if hash == slot.hash { - // return true; - // } - // error!("Hash mismatch for slot: {:?}", slot); - // } - true - } - - fn latest_bootable_slot(&self) -> Option { - // Find the latest slot that has a patch that is not bad. - // Sort the slots by patch number, then return the highest - // patch number that is not bad. - let mut slots = self.slots.clone(); - slots.sort_by(|a, b| a.patch_number.cmp(&b.patch_number)); - slots.reverse(); - for slot in slots { - if self.validate_slot(&slot) { - return Some(slot.patch_number); - } - } - None - } - - pub fn activate_latest_bootable_patch(&mut self) -> Result<(), UpdateError> { - self.set_next_boot_patch_slot(self.latest_bootable_slot()); - self.save().map_err(|_| UpdateError::FailedToSaveState) - } - - fn available_slot(&self) -> usize { - // Assume we only use two slots and pick the one that's not current. - if self.slots.is_empty() { - return 0; - } - if let Some(slot_index) = self.current_boot_slot_index { - // This does not check next_boot_slot_index, we're assuming that - // whoever is calling this is OK with replacing the next boot - // patch. - if slot_index == 0 { - return 1; - } - } - 0 - } - - fn clear_slot(&mut self, index: usize) -> anyhow::Result<()> { - // Index is outside of the slots we have. - if index >= self.slots.len() { - // Ignore slots past the end for now? - return Ok(()); - } - self.slots[index] = Slot::default(); - let slot_dir_string = self.slot_dir_for_index(index); - if slot_dir_string.exists() { - std::fs::remove_dir_all(&slot_dir_string)?; - } - Ok(()) - } - - fn set_slot(&mut self, index: usize, slot: Slot) { - debug!("Setting slot {} to {:?}", index, slot); - if self.slots.len() < index + 1 { - // Make sure we're not filling with empty slots. - assert!(self.slots.len() == index); - self.slots.resize(index + 1, Slot::default()); - } - // Set the given slot to the given version. - self.slots[index] = slot; - } - - fn patch_path_for_index(&self, index: usize) -> PathBuf { - self.slot_dir_for_index(index).join("dlc.vmcode") - } - - fn slot_dir_for_index(&self, index: usize) -> PathBuf { - Path::new(&self.cache_dir).join(format!("slot_{index}")) + pub fn next_boot_patch(&mut self) -> Option { + self.patch_manager.next_boot_patch() } + /// Copies the patch file at file_path to the manager's directory structure sets + /// this patch as the next patch to boot. pub fn install_patch(&mut self, patch: &PatchInfo) -> anyhow::Result<()> { - let slot_index = self.available_slot(); - let slot_dir_string = self.slot_dir_for_index(slot_index); - let slot_dir = PathBuf::from(&slot_dir_string); - - // Clear the slot. - self.clear_slot(slot_index)?; // Invalidate the slot. - self.save()?; - std::fs::create_dir_all(&slot_dir) - .with_context(|| format!("create_dir_all failed for {}", slot_dir.display()))?; - - if self.is_known_bad_patch(patch.number) { - return Err(UpdateError::InvalidArgument( - "patch".to_owned(), - format!("Refusing to install known bad patch: {patch:?}"), - ) - .into()); - } - - // Move the artifact into the slot. - let artifact_path = slot_dir.join("dlc.vmcode"); - std::fs::rename(&patch.path, artifact_path)?; - - // Update the state to include the new slot. - self.set_slot( - slot_index, - Slot { - patch_number: patch.number, - }, - ); - self.set_next_boot_patch_slot(Some(slot_index)); - - if let Some(latest) = self.latest_patch_number() { - if patch.number < latest { - warn!( - "Installed patch {} but latest downloaded patch is {latest:?}", - patch.number - ); - } - } - self.save()?; - - let path = self.patch_path_for_index(slot_index); - if path.exists() { - debug!("Patch {} installed to {:?}", patch.number, path); - } else { - warn!( - "Patch {} installed but does not exist {:?}", - patch.number, path - ); - } - - Ok(()) - } - - /// Sets the `current_boot` slot to the `next_boot` slot. - pub fn activate_current_patch(&mut self) -> Result<(), UpdateError> { - if self.next_boot_slot_index.is_none() { - return Err(UpdateError::InvalidState( - "No patch to activate.".to_owned(), - )); - } - self.current_boot_slot_index = self.next_boot_slot_index; - assert!(self.current_boot_slot_index.is_some()); - Ok(()) - } - - /// Switches the next boot slot to the given slot or clears it if None. - pub fn set_next_boot_patch_slot(&mut self, maybe_index: Option) { - self.next_boot_slot_index = maybe_index; + self.patch_manager.add_patch(patch.number, &patch.path) } /// Returns highest patch number that has been installed for this release. @@ -457,18 +211,33 @@ impl UpdaterState { /// and the bad patch list (we don't need to keep bad patches on disk /// to know that they're bad). /// Used by the patch check logic. - pub fn latest_patch_number(&self) -> Option { - // Get the max of the patch numbers in the slots. - // We probably could do this with chain and max? - let installed_max = self.slots.iter().map(|s| s.patch_number).max(); - let failed_max = self.failed_patches.clone().into_iter().max(); - match installed_max { - None => failed_max, - Some(installed) => match failed_max { - None => installed_max, - Some(failed) => Some(std::cmp::max(installed, failed)), - }, - } + pub fn latest_seen_patch_number(&self) -> Option { + self.patch_manager.highest_seen_patch_number() + } +} + +/// PatchEvent management +impl UpdaterState { + /// Adds an event to the queue to be sent to the server. + pub fn queue_event(&mut self, event: PatchEvent) -> Result<()> { + self.serialized_state.queued_events.push(event); + self.save() + } + + /// Returns up to `limit` events from the reporting queue. + pub fn copy_events(&self, limit: usize) -> Vec { + self.serialized_state + .queued_events + .iter() + .take(limit) + .cloned() + .collect() + } + + /// Removes all events from the reporting queue. + pub fn clear_events(&mut self) -> Result<()> { + self.serialized_state.queued_events.clear(); + self.save() } } @@ -476,11 +245,25 @@ impl UpdaterState { mod tests { use tempdir::TempDir; - use super::{PatchInfo, UpdaterState, STATE_FILE_NAME}; + use crate::cache::patch_manager::MockManagePatches; - fn test_state(tmp_dir: &TempDir) -> UpdaterState { - let cache_dir = tmp_dir.path(); - UpdaterState::new(cache_dir.to_owned(), "1.0.0+1".to_string(), None) + use mockall::predicate::eq; + + use super::*; + + fn test_state(tmp_dir: &TempDir, patch_manager: MP) -> UpdaterState + where + MP: ManagePatches + 'static, + { + UpdaterState { + cache_dir: tmp_dir.path().to_path_buf(), + patch_manager: Box::new(patch_manager), + serialized_state: SerializedState { + release_version: "1.0.0+1".to_string(), + client_id: None, + queued_events: Vec::new(), + }, + } } fn fake_patch(tmp_dir: &TempDir, number: usize) -> super::PatchInfo { @@ -490,84 +273,24 @@ mod tests { } #[test] - fn next_boot_patch_does_not_crash() { + fn release_version_changed_resets_patches() { let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - assert_eq!(state.next_boot_patch(), None); - state.next_boot_slot_index = Some(3); - assert_eq!(state.next_boot_patch(), None); - state.slots.push(super::Slot::default()); - // This used to crash, where index was bad, but slots were not empty. - assert_eq!(state.next_boot_patch(), None); - } + let mut patch_manager = PatchManager::with_root_dir(tmp_dir.path().to_path_buf()); + let file_path = &tmp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch file contents").unwrap(); + assert!(patch_manager.add_patch(1, file_path).is_ok()); - #[test] - fn release_version_changed() { - let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - state.next_boot_slot_index = Some(1); - state.save().unwrap(); - let loaded = UpdaterState::load_or_new_on_error(&state.cache_dir, &state.release_version); - assert_eq!(loaded.next_boot_slot_index, Some(1)); + let state = test_state(&tmp_dir, patch_manager); + let release_version = state.serialized_state.release_version.clone(); + assert!(state.save().is_ok()); - let loaded_after_version_change = + let mut state = UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version); + assert_eq!(state.next_boot_patch().unwrap().number, 1); + + let mut next_version_state = UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2"); - assert_eq!(loaded_after_version_change.next_boot_slot_index, None); - } - - #[test] - fn latest_downloaded_patch() { - let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - assert_eq!(state.latest_patch_number(), None); - state.install_patch(&fake_patch(&tmp_dir, 1)).unwrap(); - assert_eq!(state.latest_patch_number(), Some(1)); - state.install_patch(&fake_patch(&tmp_dir, 2)).unwrap(); - assert_eq!(state.latest_patch_number(), Some(2)); - state.install_patch(&fake_patch(&tmp_dir, 1)).unwrap(); - // This probably should be Some(2) assuming we didn't write - // over the top of patch 2 when re-installing patch 1. - // I expect if we support rollbacks we might be more explicit - // that it's a rollback? - assert_eq!(state.latest_patch_number(), Some(1)); - } - - #[test] - fn do_not_install_known_bad_patch() { - let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - let bad_patch = fake_patch(&tmp_dir, 1); - state.mark_patch_as_bad(bad_patch.number).unwrap(); - let number = bad_patch.number; - assert!(state.install_patch(&bad_patch).is_err()); - - // Calling a second time should not error. - state.mark_patch_as_bad(number).unwrap(); - } - - #[test] - fn do_not_mark_bad_patch_good() { - let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - let bad_patch = fake_patch(&tmp_dir, 1); - assert!(state.mark_patch_as_bad(bad_patch.number).is_ok()); - assert!(state.mark_patch_as_good(bad_patch.number).is_err()); - assert!(state.is_known_bad_patch(bad_patch.number)); - assert!(!state.is_known_good_patch(bad_patch.number)); - } - - #[test] - fn mark_patch_as_good() { - let tmp_dir = TempDir::new("example").unwrap(); - let mut state = test_state(&tmp_dir); - let patch = fake_patch(&tmp_dir, 1); - state.mark_patch_as_good(patch.number).unwrap(); - assert!(state.is_known_good_patch(patch.number)); - assert!(!state.is_known_bad_patch(patch.number)); - // Marking it twice doesn't change anything. - state.mark_patch_as_good(patch.number).unwrap(); - assert!(state.is_known_good_patch(patch.number)); - assert!(!state.is_known_bad_patch(patch.number)); + assert!(next_version_state.next_boot_patch().is_none()); + assert!(next_version_state.latest_seen_patch_number().is_none()); } #[test] @@ -585,31 +308,27 @@ mod tests { fn creates_updater_state_with_client_id() { let tmp_dir = TempDir::new("example").unwrap(); let state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1"); - assert!(state.client_id.is_some()); + assert!(state.serialized_state.client_id.is_some()); let saved_state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1"); - assert_eq!(state.client_id, saved_state.client_id); + assert_eq!( + state.serialized_state.client_id, + saved_state.serialized_state.client_id + ); } #[test] fn adds_client_id_to_saved_state() { let tmp_dir = TempDir::new("example").unwrap(); - let state = UpdaterState { - cache_dir: tmp_dir.path().to_path_buf(), - release_version: "1.0.0+1".to_string(), - client_id: None, - queued_events: Vec::new(), - current_boot_slot_index: None, - next_boot_slot_index: None, - failed_patches: Vec::new(), - successful_patches: Vec::new(), - slots: Vec::new(), - }; + let mock_manage_patches = MockManagePatches::new(); + let state = test_state(&tmp_dir, mock_manage_patches); - state.save().unwrap(); + assert!(state.save().is_ok()); - let loaded_state = - UpdaterState::load_or_new_on_error(&state.cache_dir, &state.release_version); - assert!(loaded_state.client_id.is_some()); + let loaded_state = UpdaterState::load_or_new_on_error( + &state.cache_dir, + &state.serialized_state.release_version, + ); + assert!(loaded_state.serialized_state.client_id.is_some()); } // A new UpdaterState is created when the release version is changed, but @@ -618,27 +337,23 @@ mod tests { fn client_id_does_not_change_if_release_version_changes() { let tmp_dir = TempDir::new("example").unwrap(); - let original_state = UpdaterState { - cache_dir: tmp_dir.path().to_path_buf(), - release_version: "1.0.0+1".to_string(), - client_id: None, - queued_events: Vec::new(), - current_boot_slot_index: None, - next_boot_slot_index: None, - failed_patches: Vec::new(), - successful_patches: Vec::new(), - slots: Vec::new(), - }; + let state = test_state( + &tmp_dir, + PatchManager::with_root_dir(tmp_dir.path().to_path_buf()), + ); let original_loaded = UpdaterState::load_or_new_on_error( - &original_state.cache_dir, - &original_state.release_version, + &state.cache_dir, + &state.serialized_state.release_version, ); - let new_loaded = UpdaterState::load_or_new_on_error(&original_state.cache_dir, "1.0.0+2"); + let new_loaded = UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2"); - assert!(original_loaded.client_id.is_some()); - assert!(new_loaded.client_id.is_some()); - assert_eq!(original_loaded.client_id, new_loaded.client_id); + assert!(original_loaded.serialized_state.client_id.is_some()); + assert!(new_loaded.serialized_state.client_id.is_some()); + assert_eq!( + original_loaded.serialized_state.client_id, + new_loaded.serialized_state.client_id + ); } #[test] @@ -646,14 +361,14 @@ mod tests { let original_tmp_dir = TempDir::new("example").unwrap(); let original_state = UpdaterState { cache_dir: original_tmp_dir.path().to_path_buf(), - release_version: "1.0.0+1".to_string(), - client_id: None, - queued_events: Vec::new(), - current_boot_slot_index: None, - next_boot_slot_index: None, - failed_patches: Vec::new(), - successful_patches: Vec::new(), - slots: Vec::new(), + patch_manager: Box::new(PatchManager::with_root_dir( + original_tmp_dir.path().to_path_buf(), + )), + serialized_state: SerializedState { + release_version: "1.0.0+1".to_string(), + client_id: None, + queued_events: Vec::new(), + }, }; original_state.save().unwrap(); @@ -664,9 +379,84 @@ mod tests { let new_state = UpdaterState::load(new_tmp_dir.path()).unwrap(); assert_eq!(new_state.cache_dir, new_tmp_dir.path()); - assert_eq!( - new_state.slot_dir_for_index(1), - new_tmp_dir.path().join("slot_1").to_path_buf() - ); + } + + #[test] + fn record_boot_failure_for_patch_forwards_to_patch_manager() { + let patch_number = 1; + let tmp_dir = TempDir::new("example").unwrap(); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_record_boot_failure_for_patch() + .with(eq(patch_number)) + .returning(|_| Ok(())); + let mut state = test_state(&tmp_dir, mock_manage_patches); + assert!(state.record_boot_failure_for_patch(patch_number).is_ok()); + } + + #[test] + fn record_boot_success_for_patch_forwards_to_patch_manager() { + let patch_number = 1; + let tmp_dir = TempDir::new("example").unwrap(); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_record_boot_success_for_patch() + .with(eq(patch_number)) + .returning(|_| Ok(())); + let mut state = test_state(&tmp_dir, mock_manage_patches); + + assert!(state.record_boot_success_for_patch(patch_number).is_ok()); + } + + #[test] + fn current_boot_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.current_boot_patch(), Some(patch)); + } + + #[test] + fn next_boot_patch_forwards_from_patch_manager() { + let patch_number = 1; + let tmp_dir = TempDir::new("example").unwrap(); + let patch = fake_patch(&tmp_dir, patch_number); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_next_boot_patch() + .return_const(Some(patch.clone())); + let mut state = test_state(&tmp_dir, mock_manage_patches); + assert_eq!(state.next_boot_patch(), Some(patch)); + } + + #[test] + fn install_patch_forwards_to_patch_manager() { + let patch_number = 1; + let tmp_dir = TempDir::new("example").unwrap(); + let patch = fake_patch(&tmp_dir, patch_number); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_add_patch() + .with(eq(patch.number), eq(patch.path.clone())) + .returning(|_, __| Ok(())); + let mut state = test_state(&tmp_dir, mock_manage_patches); + + assert!(state.install_patch(&patch).is_ok()); + } + + #[test] + fn latest_patch_number_returns_value_from_patch_manager() { + let highest_patch_number = 1; + let tmp_dir = TempDir::new("example").unwrap(); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_highest_seen_patch_number() + .return_const(Some(highest_patch_number)); + let state = test_state(&tmp_dir, mock_manage_patches); + assert_eq!(state.latest_seen_patch_number(), Some(highest_patch_number)); } } diff --git a/library/src/network.rs b/library/src/network.rs index e926750..40855dd 100644 --- a/library/src/network.rs +++ b/library/src/network.rs @@ -26,7 +26,7 @@ fn patches_events_url(base_url: &str) -> String { pub type PatchCheckRequestFn = fn(&str, PatchCheckRequest) -> anyhow::Result; pub type DownloadFileFn = fn(&str) -> anyhow::Result>; -pub type PatchInstallSuccessFn = fn(&str, CreatePatchEventRequest) -> anyhow::Result<()>; +pub type ReportEventFn = fn(&str, CreatePatchEventRequest) -> anyhow::Result<()>; /// A container for network callbacks which can be mocked out for testing. #[derive(Clone)] @@ -36,7 +36,7 @@ pub struct NetworkHooks { /// The function to call to download a file. pub download_file_fn: DownloadFileFn, /// The function to call to report patch install success. - pub patch_install_success_fn: PatchInstallSuccessFn, + pub report_event_fn: ReportEventFn, } // We have to implement Debug by hand since fn types don't implement it. @@ -45,7 +45,7 @@ impl core::fmt::Debug for NetworkHooks { f.debug_struct("NetworkHooks") .field("patch_check_request_fn", &"") .field("download_file_fn", &"") - .field("patch_install_success_fn", &"") + .field("report_event_fn", &"") .finish() } } @@ -64,11 +64,8 @@ fn download_file_throws(_url: &str) -> anyhow::Result> { } #[cfg(test)] -pub fn patch_install_success_throws( - _url: &str, - _request: CreatePatchEventRequest, -) -> anyhow::Result<()> { - bail!("please set a patch_install_success_fn"); +pub fn report_event_throws(_url: &str, _request: CreatePatchEventRequest) -> anyhow::Result<()> { + bail!("please set a report_event_fn"); } impl Default for NetworkHooks { @@ -77,7 +74,7 @@ impl Default for NetworkHooks { Self { patch_check_request_fn: patch_check_request_default, download_file_fn: download_file_default, - patch_install_success_fn: patch_install_success_default, + report_event_fn: report_event_default, } } @@ -86,7 +83,7 @@ impl Default for NetworkHooks { Self { patch_check_request_fn: patch_check_request_throws, download_file_fn: download_file_throws, - patch_install_success_fn: patch_install_success_throws, + report_event_fn: report_event_throws, } } } @@ -112,10 +109,7 @@ pub fn download_file_default(url: &str) -> anyhow::Result> { Ok(bytes.to_vec()) } -pub fn patch_install_success_default( - url: &str, - request: CreatePatchEventRequest, -) -> anyhow::Result<()> { +pub fn report_event_default(url: &str, request: CreatePatchEventRequest) -> anyhow::Result<()> { let client = reqwest::blocking::Client::new(); let result = client.post(url).json(&request).send(); handle_network_result(result)?; @@ -156,14 +150,14 @@ fn handle_network_result( pub fn testing_set_network_hooks( patch_check_request_fn: PatchCheckRequestFn, download_file_fn: DownloadFileFn, - patch_install_success_fn: PatchInstallSuccessFn, + report_event_fn: ReportEventFn, ) { crate::config::with_config_mut(|maybe_config| match maybe_config { Some(config) => { config.network_hooks = NetworkHooks { patch_check_request_fn, download_file_fn, - patch_install_success_fn, + report_event_fn, }; } None => { @@ -235,7 +229,7 @@ pub fn send_patch_check_request( config: &UpdateConfig, state: &UpdaterState, ) -> anyhow::Result { - let latest_patch_number = state.latest_patch_number(); + let latest_patch_number = state.latest_seen_patch_number(); // Send the request to the server. let request = PatchCheckRequest { @@ -262,9 +256,9 @@ pub fn send_patch_check_request( pub fn send_patch_event(event: PatchEvent, config: &UpdateConfig) -> anyhow::Result<()> { let request = CreatePatchEventRequest { event }; - let patch_install_success_fn = config.network_hooks.patch_install_success_fn; + let report_event_fn = config.network_hooks.report_event_fn; let url = &patches_events_url(&config.base_url); - patch_install_success_fn(url, request) + report_event_fn(url, request) } pub fn download_to_path( @@ -298,15 +292,15 @@ mod tests { #[test] fn check_patch_request_response_deserialization() { - let data = r###" + let data = r#" { "patch_available": true, "patch": { "number": 1, "download_url": "https://storage.googleapis.com/patch_artifacts/17a28ec1-00cf-452d-bdf9-dbb9acb78600/dlc.vmcode", - "hash": "#" + "hash": "1234" } - }"###; + }"#; let response: PatchCheckResponse = serde_json::from_str(data).unwrap(); @@ -316,7 +310,7 @@ mod tests { let patch = response.patch.unwrap(); assert_eq!(patch.number, 1); assert_eq!(patch.download_url, "https://storage.googleapis.com/patch_artifacts/17a28ec1-00cf-452d-bdf9-dbb9acb78600/dlc.vmcode"); - assert_eq!(patch.hash, "#"); + assert_eq!(patch.hash, "1234"); } #[test] @@ -366,7 +360,7 @@ mod tests { let debug = format!("{:?}", network_hooks); assert!(debug.contains("patch_check_request_fn")); assert!(debug.contains("download_file_fn")); - assert!(debug.contains("patch_install_success_fn")); + assert!(debug.contains("report_event_fn")); } #[test] @@ -411,7 +405,7 @@ mod tests { release_version: "release_version".to_string(), identifier: EventType::PatchInstallSuccess, }; - let result = super::patch_install_success_default( + let result = super::report_event_default( // Make the request to a non-existent URL, which will trigger the // same error as a lack of internet connection. &patches_events_url("http://asdfasdfasdfasdfasdf.asdfasdf"), @@ -425,7 +419,7 @@ mod tests { #[test] fn handle_network_result_unknown_error() { - let result = super::patch_install_success_default( + let result = super::report_event_default( // Make the request to an incorrectly formatted URL, which will // trigger the same error as a lack of internet connection. &patches_events_url("asdfasdf"), diff --git a/library/src/updater.rs b/library/src/updater.rs index 8dc1b02..32f0c0f 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -269,11 +269,13 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result { prepare_for_install(&config, &download_path, &output_path)?; // Check the hash before moving into place. - check_hash(&output_path, &patch.hash).context(format!( - "This app reports version {}, but the binary is different from \ + check_hash(&output_path, &patch.hash).with_context(|| { + format!( + "This app reports version {}, but the binary is different from \ the version {} that was submitted to Shorebird.", - config.release_version, config.release_version - ))?; + config.release_version, config.release_version + ) + })?; // We're abusing the config lock as a UpdateState lock for now. // This makes it so we never try to write to the UpdateState file from @@ -352,7 +354,7 @@ where /// This may be changed any time `update()` or `start_update_thread()` are called. pub fn next_boot_patch() -> anyhow::Result> { with_config(|config| { - let state = + let mut state = UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); Ok(state.next_boot_patch()) }) @@ -370,33 +372,34 @@ pub fn current_boot_patch() -> anyhow::Result> { } pub fn report_launch_start() -> anyhow::Result<()> { - with_config(|config| { - let mut state = - 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()?; - state.save() - }) + // We previously set the "current" patch the value of the "next" patch, but no longer + // do so because the semantics have changed: + // current is now "last successfully booted patch" + // next is now "patch to boot next" + + Ok(()) } /// Report that the current active path failed to launch. /// This will mark the patch as bad and activate the next best patch. pub fn report_launch_failure() -> anyhow::Result<()> { info!("Reporting failed launch."); + with_config(|config| { let mut state = UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + // Attempting to get next_boot_patch might return None if the failure was due to + // the patch being altered on disk. let patch = state - .current_boot_patch() + .next_boot_patch() .ok_or(anyhow::Error::from(UpdateError::InvalidState( "No current patch".to_string(), )))?; // Ignore the error here, we'll try to activate the next best patch // even if we fail to mark this one as bad (because it was already bad). - let mark_result = state.mark_patch_as_bad(patch.number); + let mark_result = state.record_boot_failure_for_patch(patch.number); if mark_result.is_err() { error!("Failed to mark patch as bad: {:?}", mark_result); } @@ -411,51 +414,57 @@ pub fn report_launch_failure() -> anyhow::Result<()> { }; // Queue the failure event for later sending since right after this // function returns the Flutter engine is likely to abort(). - state.queue_event(event); - // TODO(eseidel): This does the actual save for the above mutations - // which is confusing. - state - .activate_latest_bootable_patch() - .map_err(anyhow::Error::from) + state.queue_event(event) }) } pub fn report_launch_success() -> anyhow::Result<()> { with_config(|config| { + // We can tell the UpdaterState that we have successfully booted from the "next" patch + // and make that the "current" patch. let mut state = 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) { - // Ignore the error here, we'll try to activate the next best patch - // even if we fail to mark this one as good. - if state.mark_patch_as_good(patch.number).is_ok() { - let config_copy = config.clone(); - let client_id = state.client_id_or_default(); - std::thread::spawn(move || { - let event = PatchEvent { - app_id: config_copy.app_id.clone(), - arch: current_arch().to_string(), - client_id, - patch_number: patch.number, - platform: current_platform().to_string(), - release_version: config_copy.release_version.clone(), - identifier: EventType::PatchInstallSuccess, - }; - let report_result = crate::network::send_patch_event(event, &config_copy); - if let Err(err) = report_result { - error!("Failed to report successful patch install: {:?}", err); - } - }); - } - } + let next_boot_patch = match state.next_boot_patch() { + Some(patch) => patch, - state - .save() - .map_err(|_| anyhow::Error::from(UpdateError::FailedToSaveState)) - } else { - Ok(()) + // We didn't boot from a patch, so there's nothing to do. + None => return Ok(()), + }; + + let maybe_previous_boot_patch = state.current_boot_patch(); + + state.record_boot_success_for_patch(next_boot_patch.number)?; + + if let (Some(previous_boot_patch), Some(current_boot_patch)) = + (maybe_previous_boot_patch, state.current_boot_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 { + return Ok(()); + } } + + let config_copy = config.clone(); + let client_id = state.client_id_or_default(); + std::thread::spawn(move || { + let event = PatchEvent { + app_id: config_copy.app_id.clone(), + arch: current_arch().to_string(), + client_id, + patch_number: next_boot_patch.number, + platform: current_platform().to_string(), + release_version: config_copy.release_version.clone(), + identifier: EventType::PatchInstallSuccess, + }; + let report_result = crate::network::send_patch_event(event, &config_copy); + if let Err(err) = report_result { + error!("Failed to report successful patch install: {:?}", err); + } + }); + + Ok(()) }) } @@ -647,9 +656,12 @@ mod tests { let next_boot_patch = crate::next_boot_patch().unwrap().unwrap(); with_config(|config| { - let state = + let mut state = UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); - assert!(!state.is_known_good_patch(next_boot_patch.number)); + assert_eq!( + state.next_boot_patch().unwrap().number, + next_boot_patch.number + ); Ok(()) }) .unwrap(); @@ -659,7 +671,10 @@ mod tests { with_config(|config| { let state = UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); - assert!(state.is_known_good_patch(next_boot_patch.number)); + assert_eq!( + state.current_boot_patch().unwrap().number, + next_boot_patch.number + ); Ok(()) }) .unwrap(); @@ -698,10 +713,13 @@ mod tests { let next_boot_patch = crate::next_boot_patch().unwrap().unwrap(); with_config(|config| { - let state = + let mut state = 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)); + assert_eq!( + state.next_boot_patch().unwrap().number, + next_boot_patch.number + ); Ok(()) }) .unwrap(); @@ -709,10 +727,10 @@ mod tests { super::report_launch_failure().unwrap(); with_config(|config| { - let state = + let mut state = 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)); + assert!(state.next_boot_patch().is_none()); // And we've queued an event. let events = state.copy_events(1); assert_eq!(events.len(), 1); @@ -748,11 +766,11 @@ mod tests { release_version: config.release_version.clone(), }; // Queue 5 events. - state.queue_event(fail_event.clone()); - state.queue_event(fail_event.clone()); - state.queue_event(fail_event.clone()); - state.queue_event(fail_event.clone()); - state.queue_event(fail_event.clone()); + assert!(state.queue_event(fail_event.clone()).is_ok()); + assert!(state.queue_event(fail_event.clone()).is_ok()); + assert!(state.queue_event(fail_event.clone()).is_ok()); + assert!(state.queue_event(fail_event.clone()).is_ok()); + assert!(state.queue_event(fail_event.clone()).is_ok()); Ok(()) }) .unwrap();