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

fix: checking for update should not overwrite good next patch (#307)

Browse Source 
* chore: remove unnecessary mutability of self in next_boot_patch

* fix: checking for update should not overwrite good next patch
This commit is contained in:
Bryan Oltman
2025-12-19 13:18:08 -05:00
committed by GitHub
parent 58a5bcc0b0
commit 9db198a634
2 changed files with 120 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
library/src/cache/patch_manager.rs
Vendored
+37 -42
View File
@@ -268,31 +268,49 @@ impl PatchManager {
/// successfully booted 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 as well.
fn try_fall_back_from_patch(&mut self, bad_patch_number: usize) -> Result<()> {
shorebird_info!("Falling back from patch {}", bad_patch_number);
// Continue even if we fail to delete the patch artifacts. It's more important to not try to
// boot from a bad patch than to delete its artifacts.
// No need to log failure – delete_patch_artifacts logs for us.
let _ = self.delete_patch_artifacts(bad_patch_number);
if let Some(ref next_boot_patch) = self.patches_state.next_boot_patch {
// If our next boot patch is bad_patch_number, clear it.
if next_boot_patch.number == bad_patch_number {
self.patches_state.next_boot_patch = None;
}
}
let is_bad_patch_last_booted_patch = self
.patches_state
.last_booted_patch
.clone()
.map(|patch| patch.number == bad_patch_number)
.unwrap_or(false);
let is_bad_patch_next_boot_patch = self
.patches_state
.next_boot_patch
.clone()
.map(|patch| patch.number == bad_patch_number)
.unwrap_or(false);
// If we think we can still boot from the last booted patch, set it as the next_boot_patch.
// If something happened to render the last boot patch unbootable, clear it and delete its artifacts.
if let Some(last_boot_patch) = self.patches_state.last_booted_patch.clone() {
if last_boot_patch.number != bad_patch_number
&& self.validate_patch_is_bootable(&last_boot_patch).is_ok()
{
self.patches_state.next_boot_patch = Some(last_boot_patch);
} else {
self.patches_state.last_booted_patch = None;
// No need to log failure – delete_patch_artifacts logs for us.
let _ = self.delete_patch_artifacts(last_boot_patch.number);
if is_bad_patch_last_booted_patch && is_bad_patch_next_boot_patch {
// If both patches are bad, delete them both and boot from the base release.
shorebird_info!("Clearing last booted patch and next boot patch");
self.patches_state.last_booted_patch = None;
self.patches_state.next_boot_patch = None;
} else if is_bad_patch_next_boot_patch {
shorebird_info!("Clearing next boot patch");
self.patches_state.next_boot_patch = None;
if let Some(last_boot_patch) = self.patches_state.last_booted_patch.clone() {
if self.validate_patch_is_bootable(&last_boot_patch).is_ok() {
shorebird_info!(
"Setting last booted patch {} as next boot patch",
last_boot_patch.number
);
self.patches_state.next_boot_patch = Some(last_boot_patch);
} else {
shorebird_info!(
"Last booted patch {} is not bootable, deleting artifacts",
last_boot_patch.number
);
self.patches_state.last_booted_patch = None;
// No need to log failure – delete_patch_artifacts logs for us.
let _ = self.delete_patch_artifacts(last_boot_patch.number);
}
}
}
@@ -940,29 +958,6 @@ mod fall_back_tests {
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::manager_for_test(&temp_dir);
assert!(manager.patches_state.next_boot_patch.is_none());
manager.patches_state.last_booted_patch = Some(PatchMetadata {
number: 1,
size: 1,
hash: "hash".to_string(),
signature: Some("signature".to_owned()),
});
manager.try_fall_back_from_patch(1)?;
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")?;
library/src/updater.rs
+83
View File
@@ -1379,6 +1379,89 @@ mod rollback_tests {
report_launch_start, report_launch_success, tests::init_for_testing, with_mut_state, Patch,
};
#[serial]
#[test]
fn check_for_update_does_not_overwrite_good_next_patch() -> Result<()> {
// Reported in https://discord.com/channels/1030243211995791380/1446453671414992977
// Scenario:
// - Patch 1 is installed
// - Patches 2 and 3 are rolled back
// - Patch 4 is live and available to download
// Checking for a downloadable update sets the next boot patch to the latest booted, even
// though the next boot patch is not rolled back.
let mut server = mockito::Server::new();
let download_url = format!("{}/patch/4", server.url());
let check_response = PatchCheckResponse {
patch_available: true,
patch: Some(Patch {
number: 4,
hash: "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45"
.to_string(),
download_url: download_url.to_string(),
hash_signature: None,
}),
rolled_back_patch_numbers: Some(vec![3, 2]),
};
let check_response_body = serde_json::to_string(&check_response).unwrap();
let _ = server
.mock("POST", "/api/v1/patches/check")
.with_status(200)
.with_body(check_response_body)
.create();
let _ = server
.mock("POST", "/api/v1/patches/events")
.with_status(201)
.create();
let _ = server
.mock("GET", "/patch/4")
.with_status(200)
.with_body(
// Generated by `string_patch "hello world" "hello tests"`
[
40, 181, 47, 253, 0, 128, 177, 0, 0, 223, 177, 0, 0, 0, 16, 0, 0, 6, 0, 0, 0,
0, 0, 0, 5, 116, 101, 115, 116, 115, 0,
],
)
.create();
let tmp_dir = TempDir::new("example").unwrap();
init_for_testing(&tmp_dir, Some(&server.url()));
let base = "hello world";
let apk_path = tmp_dir.path().join("base.apk");
write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
install_fake_patch(1)?;
report_launch_start()?;
report_launch_success()?;
let mut staging_update_check_result = crate::check_for_downloadable_update(None)?;
assert!(staging_update_check_result);
let stable_update_result: crate::UpdateStatus = crate::update(None)?;
assert_eq!(stable_update_result, crate::UpdateStatus::UpdateInstalled);
with_mut_state(|state| {
let next_boot_patch = state.next_boot_patch();
println!("next_boot_patch after update: {:?}", next_boot_patch);
assert_eq!(state.next_boot_patch().map(|p| p.number), Some(4));
Ok(())
})?;
staging_update_check_result = crate::check_for_downloadable_update(None)?;
assert!(!staging_update_check_result);
with_mut_state(|state| {
assert_eq!(state.next_boot_patch().map(|p| p.number), Some(4));
Ok(())
})?;
Ok(())
}
#[serial]
#[test]
fn does_not_roll_back_when_rolled_back_patches_is_empty() -> Result<()> {