diff --git a/cspell.config.yaml b/cspell.config.yaml index 52a5a629..d9ed884d 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -67,8 +67,10 @@ words: - pubspec - repr - reqwest + - resends - rlib - rsplit + - rollforward - rollouts - RTLD - rustflags diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index 0c052431..507cdc4f 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -876,6 +876,114 @@ mod test { assert_eq!(shorebird_current_boot_patch_number(), 0); } + /// Regression: rolling a patch forward after the server rolled it back + /// must allow the device to install it again. + /// + /// The dashboard's rollback and rollforward operations toggle + /// `is_rolled_back` on the same `app_release_patch_` row, so the server + /// resends the same `Patch` (same number, same hash) on rollforward. + /// `remove_patch` previously inserted the patch number into + /// `known_bad_patches` to defend against an orthogonal + /// resurrection-via-failed-delete race; that insert also banned the + /// number for any later legitimate reuse, so `should_install_patch` + /// returned `PatchKnownBad` for the rolled-forward patch and the + /// device silently refused it. + /// + /// Customer-visible symptom: `checkForUpdate` returns `upToDate` while + /// the server still says a patch is available. Combined with FRC-driven + /// notification logic that compares `currentPatch` against a target + /// patch number, the app loops on a force-restart prompt that nothing + /// can satisfy. + #[serial] + #[test] + fn rollforward_after_rollback_can_install_same_patch() { + testing_reset_config(); + let tmp_dir = TempDir::new().unwrap(); + + let apk_path = tmp_dir.path().join("base.apk"); + write_fake_apk( + apk_path.to_str().unwrap(), + HELLO_TESTS_PATCH.base.as_bytes(), + ); + let fake_libapp_path = tmp_dir.path().join("lib/arch/ignored.so"); + let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap()); + let c_yaml = c_string("app_id: foo"); + assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml)); + free_c_string(c_yaml); + free_parameters(c_params); + + // Phase 1: install patch 1 and report a successful launch. + testing_set_network_hooks( + |_url, _request| { + Ok(PatchCheckResponse { + patch_available: true, + patch: Some(crate::Patch { + number: 1, + hash: HELLO_TESTS_PATCH.hash.to_owned(), + download_url: "ignored".to_owned(), + hash_signature: None, + }), + rolled_back_patch_numbers: None, + }) + }, + |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest), + |_url, _event| Ok(()), + ); + assert!(shorebird_check_for_downloadable_update(std::ptr::null())); + run_update_expecting(std::ptr::null(), SHOREBIRD_UPDATE_INSTALLED); + shorebird_report_launch_start(); + shorebird_report_launch_success(); + assert_eq!(shorebird_current_boot_patch_number(), 1); + + // Phase 2: server rolls patch 1 back with no replacement. Phase-1 + // spawned threads (PatchDownload, PatchInstallSuccess) hold a clone + // of the config from when they were spawned, so they hit the old + // report hook above. Nothing in phase 2 should report or download — + // only a patch-check happens. + testing_set_network_hooks( + |_url, _request| { + Ok(PatchCheckResponse { + patch_available: false, + patch: None, + rolled_back_patch_numbers: Some(vec![1]), + }) + }, + UNEXPECTED_DOWNLOAD, + UNEXPECTED_REPORT, + ); + assert!(!shorebird_check_for_downloadable_update(std::ptr::null())); + assert_eq!(shorebird_next_boot_patch_number(), 0); + + // Phase 3: server rolls patch 1 forward — same number, same hash, + // empty rolled_back list (the row's `is_rolled_back` flipped back + // to false on the server). The device must accept this as a normal + // "patch available" response. + testing_set_network_hooks( + |_url, _request| { + Ok(PatchCheckResponse { + patch_available: true, + patch: Some(crate::Patch { + number: 1, + hash: HELLO_TESTS_PATCH.hash.to_owned(), + download_url: "ignored".to_owned(), + hash_signature: None, + }), + rolled_back_patch_numbers: Some(vec![]), + }) + }, + |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest), + |_url, _event| Ok(()), + ); + + // Pre-fix: returns false because patch 1 is in `known_bad_patches` + // (inserted by `remove_patch` in phase 2). Post-fix: returns true. + assert!(shorebird_check_for_downloadable_update(std::ptr::null())); + run_update_expecting(std::ptr::null(), SHOREBIRD_UPDATE_INSTALLED); + + // The rolled-forward patch is queued for the next boot. + assert_eq!(shorebird_next_boot_patch_number(), 1); + } + /// Patch-to-patch rollback: device on patch 2, server rolls back to /// patch 1 (sends rollback signal AND a downloadable replacement). /// `check_for_downloadable_update` returns true (replacement available), diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs index 6fc8b2e7..61f47b81 100644 --- a/library/src/cache/patch_manager.rs +++ b/library/src/cache/patch_manager.rs @@ -593,11 +593,30 @@ impl ManagePatches for PatchManager { } fn remove_patch(&mut self, patch_number: usize) -> Result<()> { - // Server-driven rollback: mark known-bad so a later fallback path - // (e.g. record_boot_failure_for_patch on a *different* patch) can't - // promote `last_booted_patch` back to `next_boot_patch` if its - // artifact deletion in try_fall_back_from_patch silently fails. - self.patches_state.known_bad_patches.insert(patch_number); + // Server-driven rollback: drop the patch as a fallback target so a + // later boot failure on a different patch can't promote it back via + // `try_fall_back_from_patch`'s else-if branch (e.g. if + // `delete_patch_artifacts` silently fails and the artifact still + // passes `validate_patch_is_bootable`). + // + // Do *not* add `patch_number` to `known_bad_patches`: that set is a + // permanent ban for the current `release_version`, so it would + // poison the number for any later rollforward of the same patch + // (the dashboard's rollforward flips `is_rolled_back` on the same + // row — same number, same hash) or for a numerically-equal patch + // in a freshly-recreated release at the same version. FFI's + // "what's running" signal is the session-scoped `running_patch`, + // so dropping `last_booted_patch` here doesn't regress the + // patch-to-release rollback fix from #348. + if self + .patches_state + .last_booted_patch + .as_ref() + .map(|p| p.number) + == Some(patch_number) + { + self.patches_state.last_booted_patch = None; + } self.try_fall_back_from_patch(patch_number) } @@ -1342,9 +1361,12 @@ mod fall_back_tests { /// Server rolls back patch 1, then patch 2 arrives and fails to boot. /// The else-if fallback path must not promote patch 1 back into - /// `next_boot_patch` even though `last_booted_patch` still points at - /// patch 1 — the server told us not to use it. `remove_patch` records - /// patch 1 as known-bad so this can't happen. + /// `next_boot_patch`: the server told us not to use it, even if a + /// silent `delete_patch_artifacts` failure leaves the artifact + /// intact and `validate_patch_is_bootable` would accept it. + /// `remove_patch` defends against this by clearing + /// `last_booted_patch` when its number matches the rolled-back + /// patch, so there's no fallback target to promote. #[test] fn rollback_then_failed_replacement_does_not_resurrect_rolled_back_patch() -> Result<()> { let temp_dir = TempDir::new()?; @@ -1355,21 +1377,14 @@ mod fall_back_tests { manager.record_boot_start_for_patch(1)?; manager.record_boot_success()?; - // Server rolls back patch 1. + // Server rolls back patch 1. last_booted_patch is dropped because + // it points at the rolled-back patch. manager.remove_patch(1)?; - assert!(manager.is_known_bad_patch(1)); assert!(manager.patches_state.next_boot_patch.is_none()); - // last_booted_patch is intentionally preserved; the running - // process is still using patch 1 until it restarts. - assert_eq!( - manager - .patches_state - .last_booted_patch - .as_ref() - .unwrap() - .number, - 1 - ); + assert!(manager.patches_state.last_booted_patch.is_none()); + // The number is *not* added to `known_bad_patches`, so a + // subsequent rollforward of the same patch can install it. + assert!(!manager.is_known_bad_patch(1)); // Re-create patch 1's artifact to simulate a silent // delete_patch_artifacts failure (e.g. transient FS issue). @@ -1382,9 +1397,10 @@ mod fall_back_tests { manager.record_boot_start_for_patch(2)?; manager.record_boot_failure_for_patch(2)?; - // Patch 1 must NOT be promoted back to next_boot_patch. + // Patch 1 must NOT be promoted back to next_boot_patch — the + // property the test guards. With `last_booted_patch` cleared in + // `remove_patch`, the else-if branch has no fallback target. assert!(manager.patches_state.next_boot_patch.is_none()); - assert!(manager.is_known_bad_patch(1)); assert!(manager.is_known_bad_patch(2)); Ok(())