fix: Rare bug if rollback happens during second update call

The scenario is:

1. User is running patch 2 (booted successfully, so last_booted_patch = 2)
2. While the app is still running, they call the check-for-update API
3. Patch 3 is downloaded and installed (next_boot_patch = 3)
4. Before the app restarts, they check again and patch 4 is available
5. The buggy code is supposed to delete patch 3 (never booted), but instead deletes patch 2 (the last known-good patch)
6. Patch 4 is set as next_boot_patch

If patch 4 boots fine, nobody notices. But if patch 4 fails to boot and the system tries to roll back to patch 2, those artifacts are gone.
This commit is contained in:
Eric Seidel
2026-01-29 10:18:14 -08:00
parent 8691c8f60e
commit 08fb9df932
+34 -1
View File
@@ -407,8 +407,8 @@ impl ManagePatches for PatchManager {
// 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.clone(),
self.patches_state.last_booted_patch.clone(),
self.patches_state.next_boot_patch.clone(),
) {
if last_boot_patch.number != next_boot_patch.number {
shorebird_info!(
@@ -847,6 +847,39 @@ mod next_boot_patch_tests {
Ok(())
}
#[test]
fn adding_patch_deletes_unbooted_patch_not_last_booted() -> Result<()> {
let temp_dir = TempDir::new("patch_manager")?;
let mut manager = PatchManager::manager_for_test(&temp_dir);
// Add patch 1 and boot it successfully.
manager.add_patch_for_test(&temp_dir, 1)?;
manager.record_boot_start_for_patch(1)?;
manager.record_boot_success()?;
// Add patch 2 (not booted yet).
manager.add_patch_for_test(&temp_dir, 2)?;
let patch_1_artifact = manager.patch_artifact_path(1);
let patch_2_artifact = manager.patch_artifact_path(2);
assert!(patch_1_artifact.exists());
assert!(patch_2_artifact.exists());
// Add patch 3 — should delete patch 2 (unbooted), NOT patch 1 (last booted).
manager.add_patch_for_test(&temp_dir, 3)?;
assert!(
patch_1_artifact.exists(),
"Last booted patch 1 artifacts should NOT be deleted"
);
assert!(
!patch_2_artifact.exists(),
"Unbooted patch 2 artifacts should be deleted"
);
Ok(())
}
#[test]
fn returns_last_booted_patch_if_next_patch_failed_to_boot() -> Result<()> {
let temp_dir = TempDir::new("patch_manager")?;