fix: current_boot_patch survives server-driven rollback (#348)
* fix: current_boot_patch survives server-driven rollback Customer report (shorebirdtech/shorebird#3728): when the device's running patch is rolled back to the base release (no replacement patch), the running session sees `checkForUpdate` return `upToDate` even though a restart is needed. Patch-to-patch rollback works because the server's replacement patch makes `check_for_downloadable_update` return true, so Dart short-circuits to `outdated` before the comparison runs. Root cause: `UpdaterState::current_boot_patch()` derived its return value from `currently_booting_patch.or(last_successfully_booted_patch)`. After boot success, only `last_booted_patch` reflected the running patch. When the server rolled back that patch, `try_fall_back_from_patch` cleared `last_booted_patch` (correctly — it's no longer a valid fallback), and the FFI `shorebird_current_boot_patch_number` then reported 0 even though the process was still running the rolled-back patch. The conflation: `last_booted_patch` was doing two unrelated jobs — "fallback target" (its real role) and "what's running" (the proxy via `.or()` that broke under rollback). The earlier Dart-only fix in shorebirdtech/updater#312 assumed the FFI would still report the running patch number; that assumption only held in the mock. Fix: introduce a dedicated `current_boot_patch: Option<usize>` field on `PatchesState`. Set by `report_launch_start` from `next_boot_patch` (or `None` for a release boot). Read directly by `UpdaterState::current_boot_patch()` — no derivation, no fallback. Each field now has exactly one job: - `last_booted_patch`: fallback target for `try_fall_back_from_patch`. Doc updated to remove the "(usually the currently running patch)" parenthetical that perpetuated the conflation. - `current_boot_patch` (new): what this process is using. Survives rollbacks of that patch (the process is still using it). Reset on the next `report_launch_start` — including `None` on a release boot, so it doesn't go stale. - `currently_booting_patch`: unchanged. Still the boot-in-progress flag for crash detection on the next init. C API surface unchanged. `shorebird_current_boot_patch_number` still returns the same `usize` it always has — it just gets the right answer under rollback now. Verification (testing at the C API level, since that's the contract): - New regression test `rollback_to_release_keeps_current_boot_patch` reproduces the customer's bug. Fails on the parent commit (`current_boot_patch_number` returns 0); passes after this fix (returns 1). - New `rollback_to_release_then_restart_clears_current_boot_patch` proves the post-restart cleanup: the on-disk `current_boot_patch` is `Some(1)` from the previous run, but the next launch's `report_launch_start` resets it to `None` since `next_boot_patch` is `None`. No false-positive `restartRequired` on the release boot. - New `rollback_patch_to_patch_reports_current_and_next_distinctly` proves we didn't break the patch-to-patch case. Running on patch 2, server rolls back to patch 1: after `update()`, `current=2, next=1`. - All 225 existing tests pass without modification, including every C API test. Refs: shorebirdtech/shorebird#3728, shorebirdtech/updater#312, #270 * docs: TODOs for follow-up cleanup of patch state model Two cleanups deferred from #348 to keep the rollback fix focused: 1. Rename `last_booted_patch` → `fallback_patch`. Single mechanical rename, but touches ~30 test names that read in terms of the current field name. 2. Remove `currently_booting_patch` entirely. With `current_boot_patch` now tracking what's running, the boot-in-progress signal collapses to `boot_started_at.is_some()`, and the crashed-patch-on-init identification falls out of the previous run's `current_boot_patch`. This is the larger of the two — touches crash-detection logic and the boot-record helpers. Both should land as their own commits so the diff for each is easy to read and the rollback fix stays minimal. * test: assert rollback-only phases never report events In `rollback_to_release_keeps_current_boot_patch` and `rollback_to_release_then_restart_clears_current_boot_patch`, the phase that performs only the server-driven rollback never calls `shorebird_update` or `shorebird_report_launch_*`, so no event should ever be reported during it. Replace the no-op report hook with `UNEXPECTED_REPORT` to make that an asserted property of the test rather than a silent assumption — if a future change starts queueing or sending events from `check_for_downloadable_update`, these tests will surface it immediately. Phase-1 spawned threads (PatchDownload, PatchInstallSuccess) are unaffected: they hold a clone of the config from when they were spawned, so they hit the phase-1 hooks and never reach phase-2's panicking handler. The patch-to-patch test keeps the no-op hook because phase 2 there calls `shorebird_update`, which legitimately spawns a PatchDownload event using the new hooks. * docs: flag the last_booted_patch conflation as the underlying bug Replace the rename TODO with one that names the actual unfixed bug: `last_booted_patch` gets cleared in `try_fall_back_from_patch` while the running process is still using the patch. That's the deeper incoherence — the field's name and `record_boot_success` say it's a historical record, but the rollback path treats it as an operational fallback target. Those two roles only diverge under server rollback, which is the customer's case. This PR sidesteps the conflation by adding `current_boot_patch` for the "what's running" semantic. The TODOs now flag both: - The conflation itself, on the field declaration. - The specific line in try_fall_back_from_patch that does the historically-incorrect clearing. A sibling PR will prototype the alternative — keep last_booted_patch historical, express "don't fall back to this patch" via a separate signal — so we can compare the two approaches. * fix: stop clearing last_booted_patch when its patch is rolled back Roll #349 into this PR. Both fixes together — they address different real bugs and combining eliminates each PR's loose ends. Underlying data-model bug: `last_booted_patch` was conflated. Its field name and `record_boot_success` say it's a *historical* record (\"the patch that last successfully booted, ever\"). But `try_fall_back_from_patch`'s \"both bad\" branch clears it whenever the patch becomes invalid as a fallback — including when the server rolls it back, while the running process is still using it. Fix: in the \"both bad\" branch, only clear `next_boot_patch`. Leave `last_booted_patch` alone — that history shouldn't change because the server told us not to use the patch next time. The \"don't fall back to this patch\" intent is already covered by: - `delete_patch_artifacts(bad_patch_number)` at the top of the function, which removes the on-disk artifacts. - `validate_patch_is_bootable` in the else-if branch, which refuses to fall back to a patch with missing artifacts. - `is_known_bad_patch`, which records boot failures explicitly. `record_boot_failure_for_patch` flows through the same branch and benefits from the same correction — boot history is preserved across boot failures. Updated the corresponding test (`clears_last_booted_patch_if_it_is_the_failed_patch` → `preserves_last_booted_patch_on_failure_but_marks_bad`) to assert the new behavior: history preserved, known-bad recorded, artifacts deleted. Removes the two TODOs added in the previous commit: - The conflation TODO on `last_booted_patch` (now fixed). - The TODO on the offending line (line is being changed). This pairs with the `current_boot_patch` field added earlier in the same PR. The two fixes are orthogonal: - `current_boot_patch` gives us a session-scoped \"what's running\" signal, reset on `report_launch_start`. It's what the FFI reads. - The data-model fix here keeps `last_booted_patch` historically accurate, so the field's name finally matches what it stores. With both, `current_boot_patch()` no longer needs the `.or()` fallback that was the original source of the customer's bug.
This commit is contained in:
Eric Seidel
@@ -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);
|
||||
|
||||
|
||||
+347
-78
@@ -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 -- "<base>" "<new>"
|
||||
/// Then paste the four pieces into a `PatchFixture` constant:
|
||||
/// - `base`: the `<base>` argument (must match the bytes
|
||||
/// `write_fake_apk` writes for the test's fake APK)
|
||||
/// - `new`: the `<new>` 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<DownloadResult> {
|
||||
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<u8> = 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<u8> = 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<u8> = 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() {
|
||||
|
||||
Vendored
+126
-10
@@ -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<PatchMetadata>,
|
||||
|
||||
/// 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<PatchInfo>;
|
||||
|
||||
/// 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<PatchInfo>;
|
||||
|
||||
/// 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<usize>);
|
||||
|
||||
/// 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<PatchInfo> {
|
||||
crate::config::running_patch_number().map(|number| self.patch_info_for_number(number))
|
||||
}
|
||||
|
||||
fn set_running_patch(&mut self, patch_number: Option<usize>) {
|
||||
crate::config::set_running_patch_number(patch_number);
|
||||
}
|
||||
|
||||
fn currently_booting_patch(&self) -> Option<PatchInfo> {
|
||||
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());
|
||||
|
||||
Vendored
+13
-36
@@ -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<PatchInfo> {
|
||||
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<PatchInfo> {
|
||||
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<usize>) {
|
||||
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;
|
||||
|
||||
@@ -28,12 +28,40 @@ fn global_config() -> &'static Mutex<Option<UpdateConfig>> {
|
||||
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<Option<usize>> {
|
||||
static INSTANCE: OnceCell<Mutex<Option<usize>>> = OnceCell::new();
|
||||
INSTANCE.get_or_init(|| Mutex::new(None))
|
||||
}
|
||||
|
||||
pub fn running_patch_number() -> Option<usize> {
|
||||
*global_running_patch()
|
||||
.lock()
|
||||
.expect("Failed to acquire running_patch lock.")
|
||||
}
|
||||
|
||||
pub fn set_running_patch_number(patch_number: Option<usize>) {
|
||||
*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<F, R>(
|
||||
|
||||
+17
-15
@@ -802,26 +802,28 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
|
||||
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<Option<PatchInfo>> {
|
||||
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<Option<PatchInfo>> {
|
||||
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(());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user