flutter/shorebird-updater
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: only check whether a patch failed to boot on initialization (#186)

Browse Source 
* fix: track patches while they are booting

* update comment

* update comment

* add test

* refactor: only check whether a patch failed to boot on initialization

* fix test
This commit is contained in:
Bryan Oltman
2024-07-16 16:39:53 -04:00
committed by GitHub
parent a18b90f01c
commit 3073a76dbc
3 changed files with 22 additions and 117 deletions
Download Patch File Download Diff File Expand all files Collapse all files
library/src/cache/patch_manager.rs
Vendored
+10 -105
View File
@@ -39,15 +39,10 @@ struct PatchesState {
/// The patch we are currently running, if any.
last_booted_patch: Option<PatchMetadata>,
/// The last patch we attempted to boot, if any.
last_attempted_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 patch that is currently booting, if any. If the system initializes and
/// this has a value, we will consider this patch to have failed to boot.
/// This is given a value when we start booting a patch (record_boot_start_for_patch) and is
/// cleared when:
/// - the patch boots successfully (record_boot_success)
@@ -85,9 +80,11 @@ pub trait ManagePatches {
/// or None if no patch is installed.
fn last_successfully_booted_patch(&self) -> Option<PatchInfo>;
/// The patch we most recently attempted to boot. Will be the same as
/// last_successfully_booted_patch if the last boot was successful.
fn last_attempted_boot_patch(&self) -> Option<PatchInfo>;
/// The patch we are currently booting, if any. This will only have a value:
/// 1. Between record_boot_start_for_patch and record_boot_success or record_boot_failure_for_patch
/// 2. On init if we attempted to boot a patch but never recorded a successful boot (e.g., because
/// the system crashed).
fn currently_booting_patch(&self) -> Option<PatchInfo>;
/// Returns the next patch to boot, or None if:
/// - no patches have been downloaded
@@ -225,33 +222,6 @@ impl PatchManager {
);
}
// If the last boot we tried was this patch, make sure we succeeded or the patch is bad.
// If we are currently in the process of booting this patch for the first time, this check
// will always fail, so skip it.
// TODO: this validation only needs to happen once. Move to on_init.
if !self.is_currently_booting_patch(patch.number)
&& self.is_patch_last_attempted_patch(patch.number)
{
// We are trying to boot from the same patch that we tried to boot from last time.
match self.last_successful_boot_patch_number() {
Some(last_successful_patch_number)
if last_successful_patch_number == patch.number =>
{
// Our last boot attempt was this patch, and we've successfully booted from this
// patch before. This patch is safe to boot from.
}
_ => {
// We've tried to boot from this patch before and didn't
// succeed. Don't try again.
bail!(
"Already attempted and failed to boot patch {}",
patch.number
)
}
}
}
if let Some(public_key) = &self.patch_public_key {
// If we have a public key, verify that the patch's hash has a signature.
let signature = patch
@@ -269,32 +239,6 @@ impl PatchManager {
Ok(())
}
/// Whether the given patch number is the last one we attempted to boot
/// (whether it was successful or not).
fn is_patch_last_attempted_patch(&self, patch_number: usize) -> bool {
self.patches_state
.last_attempted_patch
.as_ref()
.map(|patch| patch.number == patch_number)
.unwrap_or(false)
}
/// Whether this patch is in the process of booting.
fn is_currently_booting_patch(&self, patch_number: usize) -> bool {
match &self.patches_state.currently_booting_patch {
Some(currently_booting_patch) => currently_booting_patch.number == patch_number,
None => false,
}
}
/// The number of the patch we last successfully booted, if any.
fn last_successful_boot_patch_number(&self) -> Option<usize> {
self.patches_state
.last_booted_patch
.as_ref()
.map(|patch| patch.number)
}
fn delete_patch_artifacts(&mut self, patch_number: usize) -> Result<()> {
info!("Deleting patch artifacts for patch {}", patch_number);
@@ -440,9 +384,9 @@ impl ManagePatches for PatchManager {
.map(|patch| self.patch_info_for_number(patch.number))
}
fn last_attempted_boot_patch(&self) -> Option<PatchInfo> {
fn currently_booting_patch(&self) -> Option<PatchInfo> {
self.patches_state
.last_attempted_patch
.currently_booting_patch
.as_ref()
.map(|patch| self.patch_info_for_number(patch.number))
}
@@ -484,7 +428,6 @@ impl ManagePatches for PatchManager {
);
}
self.patches_state.last_attempted_patch = Some(next_boot_patch.clone());
self.patches_state.currently_booting_patch = Some(next_boot_patch.clone());
self.save_patches_state()
}
@@ -492,9 +435,9 @@ impl ManagePatches for PatchManager {
fn record_boot_success(&mut self) -> Result<()> {
let boot_patch = self
.patches_state
.last_attempted_patch
.currently_booting_patch
.clone()
.context("No last_attempted_patch")?;
.context("No currently_booting_patch")?;
self.patches_state.currently_booting_patch = None;
self.patches_state.last_booted_patch = Some(boot_patch.clone());
@@ -580,7 +523,7 @@ mod debug_tests {
let temp_dir = TempDir::new("patch_manager").unwrap();
let patch_manager = PatchManager::new(temp_dir.path().to_owned(), Some("public_key"));
let expected_str = format!(
"PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, currently_booting_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}",
"PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, next_boot_patch: None, currently_booting_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}",
temp_dir.path().display()
);
assert_eq!(format!("{:?}", patch_manager), expected_str);
@@ -1222,44 +1165,6 @@ mod record_boot_success_for_patch_tests {
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::manager_for_test(&temp_dir);
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, "hash", None)
.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.
manager.record_boot_start_for_patch(patch_number)?;
assert!(manager.record_boot_success().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().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(())
}
#[test]
fn deletes_other_patch_artifacts() -> Result<()> {
let temp_dir = TempDir::new("patch_manager")?;
library/src/cache/updater_state.rs
Vendored
+4 -3
View File
@@ -169,9 +169,10 @@ impl UpdaterState {
self.patch_manager.record_boot_success()
}
/// The patch we most recently attempted to boot.
pub fn last_attempted_boot_patch(&self) -> Option<PatchInfo> {
self.patch_manager.last_attempted_boot_patch()
/// The patch that is currently in the process of booting. That is, we've recorded a boot start
/// but not yet a boot success or failure.
pub fn currently_booting_patch(&self) -> Option<PatchInfo> {
self.patch_manager.currently_booting_patch()
}
/// This is the current patch that is running.
library/src/updater.rs
+8 -9
View File
@@ -493,11 +493,9 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
config.patch_public_key.as_deref(),
);
let patch = state
.last_attempted_boot_patch()
.ok_or(anyhow::Error::from(UpdateError::InvalidState(
"last_attempted_boot_patch is None".to_string(),
)))?;
let patch = state.currently_booting_patch().ok_or(anyhow::Error::from(
UpdateError::InvalidState("currently_booting_patch is None".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.record_boot_failure_for_patch(patch.number);
@@ -529,7 +527,7 @@ pub fn report_launch_success() -> anyhow::Result<()> {
config.patch_public_key.as_deref(),
);
let last_attempted_boot_patch = match state.last_attempted_boot_patch() {
let booting_patch = match state.currently_booting_patch() {
Some(patch) => patch,
// We didn't boot from a patch, so there's nothing to do.
@@ -555,7 +553,7 @@ pub fn report_launch_success() -> anyhow::Result<()> {
let event = PatchEvent {
app_id: config_copy.app_id.clone(),
arch: current_arch().to_string(),
patch_number: last_attempted_boot_patch.number,
patch_number: booting_patch.number,
platform: current_platform().to_string(),
release_version: config_copy.release_version.clone(),
identifier: EventType::PatchInstallSuccess,
@@ -722,7 +720,8 @@ mod tests {
crate::report_launch_start().unwrap();
crate::report_launch_success().unwrap();
assert!(crate::next_boot_patch().unwrap().is_some());
// mark it bad.
// boot again, this time failing
crate::report_launch_start().unwrap();
crate::report_launch_failure().unwrap();
// Technically might need to "reload"
// ask for current patch (should get none).
@@ -801,7 +800,7 @@ mod tests {
.unwrap_err()
.downcast::<crate::UpdateError>()
.unwrap(),
crate::UpdateError::InvalidState("last_attempted_boot_patch is None".to_string())
crate::UpdateError::InvalidState("currently_booting_patch is None".to_string())
);
assert!(crate::report_launch_success().is_ok());
}