diff --git a/library/include/updater.h b/library/include/updater.h
index 857567c..9860712 100644
--- a/library/include/updater.h
+++ b/library/include/updater.h
@@ -130,7 +130,9 @@ SHOREBIRD_EXPORT bool shorebird_should_auto_update(void);
/**
* The currently running patch number, or 0 if the release has not been
- * patched.
+ * patched. The internal name for this concept is `running_patch`; the
+ * FFI symbol keeps the historical `current_boot_patch_number` spelling
+ * because Flutter Engine links against it.
*/
SHOREBIRD_EXPORT uintptr_t shorebird_current_boot_patch_number(void);
diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs
index 11b4971..e256923 100644
--- a/library/src/c_api/mod.rs
+++ b/library/src/c_api/mod.rs
@@ -190,12 +190,14 @@ pub extern "C" fn shorebird_should_auto_update() -> bool {
}
/// The currently running patch number, or 0 if the release has not been
-/// patched.
+/// patched. The internal name for this concept is `running_patch`; the
+/// FFI symbol keeps the historical `current_boot_patch_number` spelling
+/// because Flutter Engine links against it.
#[no_mangle]
pub extern "C" fn shorebird_current_boot_patch_number() -> usize {
log_on_error(
- || Ok(updater::current_boot_patch()?.map_or(0, |p| p.number)),
- "fetching next_boot_patch_number",
+ || Ok(updater::running_patch()?.map_or(0, |p| p.number)),
+ "fetching running_patch_number",
0,
)
}
@@ -402,6 +404,7 @@ mod test {
use crate::{
network::{
testing_set_network_hooks, DownloadResult, PatchCheckResponse, UNEXPECTED_DOWNLOAD,
+ UNEXPECTED_REPORT,
},
test_utils::write_fake_apk,
};
@@ -473,6 +476,66 @@ mod test {
)
}
+ /// A precomputed bidiff patch artifact along with the inputs that
+ /// produced it. Generate one with:
+ /// cargo run --bin string_patch -- "" ""
+ /// Then paste the four pieces into a `PatchFixture` constant:
+ /// - `base`: the `` argument (must match the bytes
+ /// `write_fake_apk` writes for the test's fake APK)
+ /// - `new`: the `` argument (the inflated content after
+ /// applying the patch — what tests assert against)
+ /// - `bytes`: the "Patch:" byte array
+ /// - `hash`: the "Hash (new):" sha256 hex of `new`
+ ///
+ /// All fixtures used together in a single test must share the same
+ /// `base`, since `write_fake_apk` only writes one set of bytes.
+ struct PatchFixture {
+ base: &'static str,
+ new: &'static str,
+ hash: &'static str,
+ bytes: &'static [u8],
+ }
+
+ impl PatchFixture {
+ /// Helper for `testing_set_network_hooks` download callbacks: writes
+ /// the fixture's patch bytes to `dest` and returns the matching
+ /// `DownloadResult`.
+ fn write_to(&self, dest: &Path) -> anyhow::Result {
+ let total_bytes = self.bytes.len() as u64;
+ std::fs::write(dest, self.bytes)?;
+ Ok(DownloadResult {
+ total_bytes,
+ content_length: Some(total_bytes),
+ })
+ }
+ }
+
+ /// `string_patch "hello world" "hello tests"` — the default fixture
+ /// used by tests that don't care which patch is which.
+ const HELLO_TESTS_PATCH: PatchFixture = PatchFixture {
+ base: "hello world",
+ new: "hello tests",
+ hash: "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45",
+ bytes: &[
+ 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,
+ ],
+ };
+
+ /// `string_patch "hello world" "hello patch 2"` — distinct from
+ /// `HELLO_TESTS_PATCH` for tests that need two non-equal artifacts
+ /// (e.g. patch-to-patch rollback). Shares the same `base` so both
+ /// fixtures can be used in the same test.
+ const HELLO_PATCH_2_PATCH: PatchFixture = PatchFixture {
+ base: "hello world",
+ new: "hello patch 2",
+ hash: "2bc806572d14496a1ddfbddf7ed7380fca87a80b739b2881544aba397d267c68",
+ bytes: &[
+ 40, 181, 47, 253, 0, 128, 193, 0, 0, 223, 177, 0, 0, 0, 16, 0, 0, 6, 0, 0, 0, 0, 0, 0,
+ 7, 112, 97, 116, 99, 104, 32, 50, 0,
+ ],
+ };
+
#[serial]
#[test]
fn init_with_nulls() {
@@ -583,11 +646,11 @@ mod test {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
- let expected_new: &str = "hello tests";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -602,32 +665,18 @@ mod test {
// We didn't specify a channel in either the shorebird_check_for_downloadable_update
// call or the shorebird.yaml, so we should default to "stable".
assert_eq!(request.channel, "stable");
- // Generated by `string_patch "hello world" "hello tests"`
- let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45";
Ok(PatchCheckResponse {
patch_available: true,
patch: Some(crate::Patch {
number: 1,
- hash: hash.to_owned(),
+ 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| {
- // Generated by `string_patch "hello world" "hello tests"`
- let patch_bytes: Vec = vec![
- 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,
- ];
- let total_bytes = patch_bytes.len() as u64;
- std::fs::write(dest, &patch_bytes)?;
- Ok(DownloadResult {
- total_bytes,
- content_length: Some(total_bytes),
- })
- },
+ |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
|_url, _event| Ok(()),
);
// There is an update available.
@@ -644,7 +693,7 @@ mod test {
let path = to_rust(c_path).unwrap();
unsafe { shorebird_free_string(c_path) };
let new = std::fs::read_to_string(path).unwrap();
- assert_eq!(new, expected_new);
+ assert_eq!(new, HELLO_TESTS_PATCH.new);
}
#[serial]
@@ -653,11 +702,11 @@ mod test {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
- let expected_new: &str = "hello tests";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -670,32 +719,18 @@ mod test {
testing_set_network_hooks(
|_url, request| {
assert_eq!(request.channel, "beta");
- // Generated by `string_patch "hello world" "hello tests"`
- let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45";
Ok(PatchCheckResponse {
patch_available: true,
patch: Some(crate::Patch {
number: 1,
- hash: hash.to_owned(),
+ 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| {
- // Generated by `string_patch "hello world" "hello tests"`
- let patch_bytes: Vec = vec![
- 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,
- ];
- let total_bytes = patch_bytes.len() as u64;
- std::fs::write(dest, &patch_bytes)?;
- Ok(DownloadResult {
- total_bytes,
- content_length: Some(total_bytes),
- })
- },
+ |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
|_url, _event| Ok(()),
);
// There is an update available.
@@ -719,7 +754,7 @@ mod test {
let path = to_rust(c_path).unwrap();
unsafe { shorebird_free_string(c_path) };
let new = std::fs::read_to_string(path).unwrap();
- assert_eq!(new, expected_new);
+ assert_eq!(new, HELLO_TESTS_PATCH.new);
Ok(())
}
@@ -730,10 +765,11 @@ mod test {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -770,10 +806,11 @@ mod test {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -804,10 +841,11 @@ mod test {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -822,14 +860,11 @@ mod test {
// shorebird_update_with_result was called with the beta channel, ensure that is
// piped through to the network request.
assert_eq!(request.channel, "beta");
-
- // Generated by `string_patch "hello world" "hello tests"`
- let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45";
Ok(PatchCheckResponse {
patch_available: true,
patch: Some(crate::Patch {
number: 1,
- hash: hash.to_owned(),
+ hash: HELLO_TESTS_PATCH.hash.to_owned(),
download_url: "ignored".to_owned(),
hash_signature: None,
}),
@@ -855,14 +890,15 @@ mod test {
#[serial]
#[test]
- fn current_boot_patch_set_after_reporting_launch_start() {
+ fn running_patch_set_after_reporting_launch_start() {
testing_reset_config();
let tmp_dir = TempDir::new().unwrap();
- // Generated by `string_patch "hello world" "hello tests"`
- let base = "hello world";
let apk_path = tmp_dir.path().join("base.apk");
- write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes());
+ 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());
// app_id is required or shorebird_init will fail.
@@ -874,32 +910,18 @@ mod test {
// set up the network hooks to return a patch.
testing_set_network_hooks(
|_url, _request| {
- // Generated by `string_patch "hello world" "hello tests"`
- let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45";
Ok(PatchCheckResponse {
patch_available: true,
patch: Some(crate::Patch {
number: 1,
- hash: hash.to_owned(),
+ 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| {
- // Generated by `string_patch "hello world" "hello tests"`
- let patch_bytes: Vec = vec![
- 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,
- ];
- let total_bytes = patch_bytes.len() as u64;
- std::fs::write(dest, &patch_bytes)?;
- Ok(DownloadResult {
- total_bytes,
- content_length: Some(total_bytes),
- })
- },
+ |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
|_url, _event| Ok(()),
);
@@ -925,6 +947,253 @@ mod test {
assert_eq!(shorebird_current_boot_patch_number(), 1);
}
+ /// Regression test for the patch-to-release rollback bug.
+ /// Customer scenario: device is running patch 1, server rolls patch 1
+ /// back to the base release (no replacement patch). After
+ /// shorebird_check_for_downloadable_update processes the rollback,
+ /// shorebird_current_boot_patch_number must still report 1 — the running
+ /// process is still using patch 1 and needs to restart. Pair that with
+ /// shorebird_next_boot_patch_number == 0 so callers can detect the
+ /// "current != next" condition that signals restart_required.
+ #[serial]
+ #[test]
+ fn rollback_to_release_keeps_running_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);
+
+ // First, 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()));
+ shorebird_update();
+ shorebird_report_launch_start();
+ shorebird_report_launch_success();
+
+ // Sanity: we are now running patch 1.
+ assert_eq!(shorebird_current_boot_patch_number(), 1);
+ assert_eq!(shorebird_next_boot_patch_number(), 1);
+
+ // Now the server rolls back patch 1 with no replacement. The device
+ // should fall back to the base release on the *next* boot, and the
+ // running session should be told a restart is required.
+ // 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 — only a
+ // patch-check request happens — so use UNEXPECTED_REPORT to assert
+ // that.
+ testing_set_network_hooks(
+ |_url, _request| {
+ Ok(PatchCheckResponse {
+ patch_available: false,
+ patch: None,
+ rolled_back_patch_numbers: Some(vec![1]),
+ })
+ },
+ UNEXPECTED_DOWNLOAD,
+ UNEXPECTED_REPORT,
+ );
+
+ // Server has no downloadable update — just the rollback signal.
+ assert!(!shorebird_check_for_downloadable_update(std::ptr::null()));
+
+ // The bug: pre-fix this returns 0. Post-fix it must return 1, because
+ // the running process is still on patch 1.
+ assert_eq!(shorebird_current_boot_patch_number(), 1);
+ // Next boot has been cleared — the device will boot the release.
+ assert_eq!(shorebird_next_boot_patch_number(), 0);
+ }
+
+ /// After a patch-to-release rollback, the next launch boots the base
+ /// release. `running_patch` must reflect that — it cannot keep
+ /// reporting the rolled-back patch from the previous run, or callers
+ /// would see a perpetual `restartRequired`. The contract: a fresh
+ /// process starts with `running_patch == None` (it's a session-scoped
+ /// global, not persisted) and `report_launch_start` keeps it `None`
+ /// because `next_boot_patch` is also `None`.
+ #[serial]
+ #[test]
+ fn rollback_to_release_then_restart_clears_running_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");
+
+ // Phase 1: install patch 1 and report a successful launch.
+ {
+ 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);
+ }
+
+ 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()));
+ shorebird_update();
+ shorebird_report_launch_start();
+ shorebird_report_launch_success();
+ assert_eq!(shorebird_current_boot_patch_number(), 1);
+
+ // Phase 2: server rolls back patch 1 with no replacement.
+ // Phase-1 spawned threads (PatchDownload, PatchInstallSuccess) hold a
+ // clone of the config from when they were spawned, so they hit the
+ // phase-1 hooks above. Phase 2 only does a patch check, so no report
+ // is expected here.
+ 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_current_boot_patch_number(), 1);
+ assert_eq!(shorebird_next_boot_patch_number(), 0);
+
+ // Phase 3: simulate app restart by resetting config and re-initializing
+ // against the same on-disk state (same tmp_dir).
+ testing_reset_config();
+ {
+ 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);
+ }
+
+ // The release boot has no next patch. running_patch is a
+ // session-scoped global, so a fresh process starts with it None.
+ // report_launch_start keeps it None because next_boot_patch is None.
+ assert_eq!(shorebird_next_boot_patch_number(), 0);
+ shorebird_report_launch_start();
+ assert_eq!(shorebird_current_boot_patch_number(), 0);
+ shorebird_report_launch_success();
+ assert_eq!(shorebird_current_boot_patch_number(), 0);
+ }
+
+ /// 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),
+ /// and after `update()` installs patch 1, the running session sees
+ /// `current=2, next=1` — the signal Dart needs for `restartRequired`.
+ #[serial]
+ #[test]
+ fn rollback_patch_to_patch_reports_current_and_next_distinctly() {
+ 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);
+
+ // Set up patch 2 (HELLO_PATCH_2_PATCH) as the running patch.
+ testing_set_network_hooks(
+ |_url, _request| {
+ Ok(PatchCheckResponse {
+ patch_available: true,
+ patch: Some(crate::Patch {
+ number: 2,
+ hash: HELLO_PATCH_2_PATCH.hash.to_owned(),
+ download_url: "ignored".to_owned(),
+ hash_signature: None,
+ }),
+ rolled_back_patch_numbers: None,
+ })
+ },
+ |_url, dest: &Path, _resume_from: u64| HELLO_PATCH_2_PATCH.write_to(dest),
+ |_url, _event| Ok(()),
+ );
+ assert!(shorebird_check_for_downloadable_update(std::ptr::null()));
+ shorebird_update();
+ shorebird_report_launch_start();
+ shorebird_report_launch_success();
+ assert_eq!(shorebird_current_boot_patch_number(), 2);
+
+ // Server rolls back patch 2 with patch 1 (HELLO_TESTS_PATCH) as
+ // replacement. The replacement is a distinct artifact from patch 2.
+ 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![2]),
+ })
+ },
+ |_url, dest: &Path, _resume_from: u64| HELLO_TESTS_PATCH.write_to(dest),
+ |_url, _event| Ok(()),
+ );
+ assert!(shorebird_check_for_downloadable_update(std::ptr::null()));
+ shorebird_update();
+
+ // Running process is still on patch 2; next boot will be patch 1.
+ assert_eq!(shorebird_current_boot_patch_number(), 2);
+ assert_eq!(shorebird_next_boot_patch_number(), 1);
+ }
+
#[serial]
#[test]
fn forgot_init() {
diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs
index 4c6ca7a..6fc8b2e 100644
--- a/library/src/cache/patch_manager.rs
+++ b/library/src/cache/patch_manager.rs
@@ -37,7 +37,13 @@ struct PatchMetadata {
/// What gets serialized to disk
#[derive(Debug, Default, Deserialize, Serialize)]
struct PatchesState {
- /// The patch we are currently running, if any.
+ /// Historical record of the patch that most recently completed
+ /// `record_boot_success` on any run. Used as a fallback target by
+ /// `try_fall_back_from_patch` when the next-boot patch is invalid.
+ /// Updated only by `record_boot_success` — boot failures and
+ /// server-driven rollbacks of the running patch don't erase the
+ /// historical fact that the patch booted. Not the same thing as the
+ /// patch this process is running; see `running_patch` for that.
last_booted_patch: Option,
/// The patch that will be run on the next app boot, if any. This may be the same
@@ -84,10 +90,30 @@ pub trait ManagePatches {
signature: Option<&'a str>,
) -> Result<()>;
- /// Returns the patch we most recently successfully booted from (usually the currently running patch),
- /// or None if no patch is installed.
+ /// The patch most recently known to have successfully booted on a prior
+ /// run, or None if no patch is installed. Used as a fallback target by
+ /// `try_fall_back_from_patch` when the next-boot patch becomes invalid.
+ /// Not the same thing as the patch this process is running; for that, see
+ /// [`running_patch`].
fn last_successfully_booted_patch(&self) -> Option;
+ /// The patch this process is using, set at `report_launch_start` from
+ /// whatever `next_boot_patch` was at that moment. `None` means the
+ /// process is running the base release. Survives server-driven
+ /// rollbacks of that patch (the process is still using it). Backed by
+ /// a session-scoped global, not by `PatchesState` on disk: a fresh
+ /// process starts with `None` until the next `report_launch_start`,
+ /// which flutter_engine calls before `dart:ffi` is available, so the
+ /// `None` window is not observable from Dart. Distinct from
+ /// `currently_booting_patch`, which is the persisted "boot in progress"
+ /// breadcrumb used for cross-restart crash detection.
+ fn running_patch(&self) -> Option;
+
+ /// Sets the patch this process is using. Called from
+ /// `report_launch_start` with `Some(n)` when launching a patch, or
+ /// `None` when launching the base release.
+ fn set_running_patch(&mut self, patch_number: Option);
+
/// 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
@@ -312,16 +338,35 @@ impl PatchManager {
.unwrap_or(false);
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;
+ // The bad patch is both the last successfully-booted patch and
+ // the queued next-boot patch. Clear `next_boot_patch` so we boot
+ // the base release on next launch, but leave `last_booted_patch`
+ // alone — the patch *did* successfully boot, and the running
+ // process is still using it. Erasing that historical record
+ // while the field was being read by FFI as "what's running"
+ // caused the patch-to-release rollback bug in
+ // shorebirdtech/shorebird#3728.
+ //
+ // The else-if fallback path consults `is_known_bad_patch`
+ // before promoting `last_booted_patch` back to
+ // `next_boot_patch`. For server-driven rollbacks, `remove_patch`
+ // adds the patch to `known_bad_patches` first, so a later
+ // fallback can't accidentally re-promote a rolled-back patch
+ // even if its artifact deletion above silently failed.
+ shorebird_info!(
+ "Clearing next boot patch (rollback target was both last booted and next boot)"
+ );
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() {
+ let is_known_bad = self
+ .patches_state
+ .known_bad_patches
+ .contains(&last_boot_patch.number);
+ if !is_known_bad && self.validate_patch_is_bootable(&last_boot_patch).is_ok() {
shorebird_info!(
"Setting last booted patch {} as next boot patch",
last_boot_patch.number
@@ -329,7 +374,7 @@ impl PatchManager {
self.patches_state.next_boot_patch = Some(last_boot_patch);
} else {
shorebird_info!(
- "Last booted patch {} is not bootable, deleting artifacts",
+ "Last booted patch {} is not a valid fallback, deleting artifacts",
last_boot_patch.number
);
self.patches_state.last_booted_patch = None;
@@ -445,6 +490,14 @@ impl ManagePatches for PatchManager {
.map(|patch| self.patch_info_for_number(patch.number))
}
+ fn running_patch(&self) -> Option {
+ crate::config::running_patch_number().map(|number| self.patch_info_for_number(number))
+ }
+
+ fn set_running_patch(&mut self, patch_number: Option) {
+ crate::config::set_running_patch_number(patch_number);
+ }
+
fn currently_booting_patch(&self) -> Option {
self.patches_state
.currently_booting_patch
@@ -540,6 +593,11 @@ 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);
self.try_fall_back_from_patch(patch_number)
}
@@ -1282,6 +1340,56 @@ mod fall_back_tests {
Ok(())
}
+ /// 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.
+ #[test]
+ fn rollback_then_failed_replacement_does_not_resurrect_rolled_back_patch() -> Result<()> {
+ let temp_dir = TempDir::new()?;
+ let mut manager = PatchManager::manager_for_test(&temp_dir);
+
+ // Patch 1: install + successful boot.
+ manager.add_patch_for_test(&temp_dir, 1)?;
+ manager.record_boot_start_for_patch(1)?;
+ manager.record_boot_success()?;
+
+ // Server rolls back patch 1.
+ 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
+ );
+
+ // Re-create patch 1's artifact to simulate a silent
+ // delete_patch_artifacts failure (e.g. transient FS issue).
+ let patch_1_path = manager.patch_artifact_path(1);
+ std::fs::create_dir_all(patch_1_path.parent().unwrap())?;
+ std::fs::write(&patch_1_path, "patch contents")?;
+
+ // Patch 2 arrives and then fails to boot.
+ manager.add_patch_for_test(&temp_dir, 2)?;
+ 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.
+ assert!(manager.patches_state.next_boot_patch.is_none());
+ assert!(manager.is_known_bad_patch(1));
+ assert!(manager.is_known_bad_patch(2));
+
+ Ok(())
+ }
+
#[test]
fn succeeds_if_deleting_artifacts_fails() -> Result<()> {
let temp_dir = TempDir::new()?;
@@ -1445,8 +1553,14 @@ mod record_boot_failure_for_patch_tests {
Ok(())
}
+ /// Patch 1 successfully booted on a prior run; on this run boot fails.
+ /// `last_successfully_booted_patch` keeps reporting patch 1 — it *did*
+ /// successfully boot once, that's a historical fact. The operational
+ /// "don't try this patch again" intent is captured by
+ /// `is_known_bad_patch` and by deleting the artifacts; nobody falls
+ /// back to a patch with missing artifacts.
#[test]
- fn clears_last_booted_patch_if_it_is_the_failed_patch() -> Result<()> {
+ fn preserves_last_booted_patch_on_failure_but_marks_bad() -> Result<()> {
let temp_dir = TempDir::new()?;
let mut manager = PatchManager::manager_for_test(&temp_dir);
manager.add_patch_for_test(&temp_dir, 1)?;
@@ -1463,7 +1577,9 @@ mod record_boot_failure_for_patch_tests {
// Now pretend it failed to boot
assert!(manager.record_boot_start_for_patch(1).is_ok());
assert!(manager.record_boot_failure_for_patch(1).is_ok());
- assert!(manager.last_successfully_booted_patch().is_none());
+ // Historical fact preserved.
+ assert_eq!(manager.last_successfully_booted_patch().unwrap().number, 1);
+ // Operational state: don't try this patch again.
assert!(manager.next_boot_patch().is_none());
assert!(manager.is_known_bad_patch(1));
assert!(!patch_artifact_path.exists());
diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs
index f5d0d47..e8ca3b0 100644
--- a/library/src/cache/updater_state.rs
+++ b/library/src/cache/updater_state.rs
@@ -233,11 +233,19 @@ impl UpdaterState {
self.patch_manager.last_successfully_booted_patch()
}
- /// This is the current patch that is running.
- pub fn current_boot_patch(&self) -> Option {
- self.patch_manager
- .currently_booting_patch()
- .or(self.patch_manager.last_successfully_booted_patch())
+ /// The patch this process is using, set at `report_launch_start` and
+ /// kept until the next launch. `None` means the process is running the
+ /// base release. Survives server-driven rollbacks of that patch — the
+ /// running process is still using it.
+ pub fn running_patch(&self) -> Option {
+ self.patch_manager.running_patch()
+ }
+
+ /// Records which patch this process is using. Called from
+ /// `report_launch_start` with `Some(n)` when launching a patch, or
+ /// `None` when launching the base release.
+ pub fn set_running_patch(&mut self, patch_number: Option) {
+ self.patch_manager.set_running_patch(patch_number);
}
/// This is the patch that will be used for the next boot.
@@ -486,37 +494,6 @@ mod tests {
assert_eq!(state.last_successfully_booted_patch(), Some(patch));
}
- #[test]
- fn current_boot_patch_returns_currently_booting_patch_if_present() {
- let tmp_dir = TempDir::new().unwrap();
- let patch1 = fake_patch(&tmp_dir, 1);
- let patch2 = fake_patch(&tmp_dir, 2);
- let mut mock_manage_patches = MockManagePatches::new();
- mock_manage_patches
- .expect_last_successfully_booted_patch()
- .return_const(Some(patch1.clone()));
- mock_manage_patches
- .expect_currently_booting_patch()
- .return_const(Some(patch2.clone()));
- let state = test_state(&tmp_dir, mock_manage_patches);
- assert_eq!(state.current_boot_patch(), Some(patch2));
- }
-
- #[test]
- fn current_boot_patch_returns_last_successfully_booted_patch_if_no_patch_is_booting() {
- let tmp_dir = TempDir::new().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()));
- mock_manage_patches
- .expect_currently_booting_patch()
- .return_const(None);
- 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;
diff --git a/library/src/config.rs b/library/src/config.rs
index 3a1b143..ac5fd9a 100644
--- a/library/src/config.rs
+++ b/library/src/config.rs
@@ -28,12 +28,40 @@ fn global_config() -> &'static Mutex> {
INSTANCE.get_or_init(|| Mutex::new(None))
}
+/// Session-scoped patch number this process is using. Set by
+/// `report_launch_start` from the next-boot patch at that moment, read
+/// by `updater::running_patch()` (surfaced to Dart as
+/// `shorebird_current_boot_patch_number`). Lives outside the on-disk
+/// `PatchesState` because it tracks running state, not bootable-patch
+/// metadata: it must survive a server-driven rollback of the running
+/// patch (the process is still using it) and must reset to `None` on
+/// every fresh process start. `report_launch_start` is called by
+/// flutter_engine before `dart:ffi` is available, so the `None` window
+/// before launch start is not observable from Dart.
+fn global_running_patch() -> &'static Mutex> {
+ static INSTANCE: OnceCell>> = OnceCell::new();
+ INSTANCE.get_or_init(|| Mutex::new(None))
+}
+
+pub fn running_patch_number() -> Option {
+ *global_running_patch()
+ .lock()
+ .expect("Failed to acquire running_patch lock.")
+}
+
+pub fn set_running_patch_number(patch_number: Option) {
+ *global_running_patch()
+ .lock()
+ .expect("Failed to acquire running_patch lock.") = patch_number;
+}
+
/// Unit tests should call this to reset the config between tests.
#[cfg(test)]
pub fn testing_reset_config() {
with_config_mut(|config| {
*config = None;
});
+ set_running_patch_number(None);
}
pub fn check_initialized_and_call(
diff --git a/library/src/updater.rs b/library/src/updater.rs
index cd3cd4d..b9cb45d 100644
--- a/library/src/updater.rs
+++ b/library/src/updater.rs
@@ -802,26 +802,28 @@ pub fn next_boot_patch() -> anyhow::Result> {
with_mut_state(|state| Ok(state.next_boot_patch()))
}
-/// The patch that was last successfully booted. If we're booting a patch for the first time, this
-/// will be the previous patch (or None, if there was no previous patch) until the boot is
-/// reported as successful.
-pub fn current_boot_patch() -> anyhow::Result> {
- with_state(|state| Ok(state.current_boot_patch()))
+/// The patch this process is using, set at `report_launch_start` and
+/// surfaced over FFI as `shorebird_current_boot_patch_number`. `None`
+/// means the process is running the base release. Survives server-driven
+/// rollbacks of the running patch — the process is still using it.
+pub fn running_patch() -> anyhow::Result> {
+ with_state(|state| Ok(state.running_patch()))
}
pub fn report_launch_start() -> anyhow::Result<()> {
- // 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"
shorebird_info!("Reporting launch start.");
with_mut_state(|state| {
- if let Some(next_boot_patch) = state.next_boot_patch() {
- state.record_boot_start_for_patch(next_boot_patch.number)
- } else {
- Ok(())
+ let next_boot_patch = state.next_boot_patch();
+ // Capture what this run is using. None means we're booting the base
+ // release. Backed by a session-scoped global, not by PatchesState
+ // on disk, so this is in-memory only — the record_boot_start_for_patch
+ // call below is the only disk write on this path.
+ state.set_running_patch(next_boot_patch.as_ref().map(|p| p.number));
+ if let Some(next_boot_patch) = next_boot_patch {
+ state.record_boot_start_for_patch(next_boot_patch.number)?;
}
+ Ok(())
})
}
@@ -889,13 +891,13 @@ pub fn report_launch_success() -> anyhow::Result<()> {
// Check whether last_successfully_booted_patch has changed. If so, we should report a
// PatchInstallSuccess event.
- if let (Some(previous_boot_patch), Some(current_boot_patch)) = (
+ if let (Some(previous_boot_patch), Some(latest_boot_patch)) = (
maybe_previous_boot_patch,
state.last_successfully_booted_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 {
+ if previous_boot_patch.number == latest_boot_patch.number {
return Ok(());
}
}