refactor: move patch management functionality out of UpdaterState (#93)

* non-compiling WIP

* update

* add cfg test

* address clippy issues

* minor consistency

* introduce patch manager, delegate patch management functionality from updater state

* cleanup

* Restructure patch state on disk, adds patch validation

* Simplify patches state, move patches to subdirectories of patches/ instead of having the artifacts as immediate children

* Tests

* delete unused enum

* disk manager tests

* Tests, docs

* rename

* delete unused file

* fix todo

* error logging

* cleanup

* cleanup

* comments and cleanup

* cleanup

* debug log

* boot from previous patch if next patch is bad

* fallback logic and test

* fix log

* context -> with_context

* remove get from get_next_boot_patch

* Log patch state load error

* renaming

* coverage

* capitalization

* coverage

* Replace unwrap with ? in disk_io

* Fix record boot success

* eq none -> .is_none

* Move delete_patch_artifacts failure logging into function

* rename patch_install_success_fn to report_event_fn

* Fix patch install reporting

* update comment

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"

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,
+}

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<S, P>(serializable: &S, path: &P) -> anyhow::Result<()>
+where
+    S: ?Sized + Serialize,
+    P: AsRef<Path>,
+{
+    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<D, P>(path: &P) -> anyhow::Result<D>
+where
+    D: DeserializeOwned,
+    P: AsRef<Path>,
+{
+    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::<TestStruct, _>(&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::<TestStruct, _>(&path).is_err());
+
+        Ok(())
+    }
+}

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<PatchMetadata>,
+
+    /// 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<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>,
+}
+
+/// 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<PatchInfo>;
+
+    /// 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<PatchInfo>;
+
+    /// 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<usize>;
+
+    /// 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/
+    ///    <patch_number>/
+    ///      dlc.vmcode
+    ///    <patch_number>/
+    ///      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<PatchesState> {
+        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 <patch_number>.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<PatchInfo> {
+        self.patches_state
+            .last_booted_patch
+            .map(|patch| self.patch_info_for_number(patch.number))
+    }
+
+    fn next_boot_patch(&mut self) -> Option<PatchInfo> {
+        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<usize> {
+        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<dyn super::ManagePatches> = 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(())
+    }
+}

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<dyn ManagePatches>,
+
+    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<String>,
 
@@ -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<usize>,
-    /// List of patches that successfully booted. We will never rollback past
-    /// one of these for this device.
-    successful_patches: Vec<usize>,
-    /// Slot that the app is currently booted from.
-    current_boot_slot_index: Option<usize>,
-    /// Slot that will be used for next boot.
-    next_boot_slot_index: Option<usize>,
-    /// List of slots.
-    slots: Vec<Slot>,
     /// Events that have not yet been sent to the server.
     /// Format could change between releases, so this is per-release state.
     queued_events: Vec<PatchEvent>,
@@ -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<String>) -> 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<PatchEvent> {
-        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<Self> {
-        // 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<PatchInfo> {
-        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<PatchInfo> {
-        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<PatchInfo> {
-        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<usize> {
-        // 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<PatchInfo> {
+        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<usize>) {
-        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<usize> {
-        // 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<usize> {
+        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<PatchEvent> {
+        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<MP>(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));
     }
 }

library/src/network.rs

@@ -26,7 +26,7 @@ fn patches_events_url(base_url: &str) -> String {
 
 pub type PatchCheckRequestFn = fn(&str, PatchCheckRequest) -> anyhow::Result<PatchCheckResponse>;
 pub type DownloadFileFn = fn(&str) -> anyhow::Result<Vec<u8>>;
-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", &"<fn>")
             .field("download_file_fn", &"<fn>")
-            .field("patch_install_success_fn", &"<fn>")
+            .field("report_event_fn", &"<fn>")
             .finish()
     }
 }
@@ -64,11 +64,8 @@ fn download_file_throws(_url: &str) -> anyhow::Result<Vec<u8>> {
 }
 
 #[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<Vec<u8>> {
     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<PatchCheckResponse> {
-    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"),

library/src/updater.rs

@@ -269,11 +269,13 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     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<Option<PatchInfo>> {
     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<Option<PatchInfo>> {
 }
 
 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();