From 4ff2839cdb5ff300e9cf5751794e7705dd6692b7 Mon Sep 17 00:00:00 2001 From: Eric Seidel Date: Mon, 30 Mar 2026 16:01:50 -0700 Subject: [PATCH] feat: add tests and docs for boot state machine (#309) * test: test api calls * chore: add docs * chore: fix cspell * fix: use no-op network hooks in multi_engine tests Set no-op network hooks after init_for_testing so the fire-and-forget thread spawned by report_launch_success completes instantly without network I/O, preventing leaked threads from interfering with subsequent serial tests that use mock servers. --- cspell.config.yaml | 2 + docs/boot_state_machine.md | 199 +++++++++++++++++++++++++++++++++++++ library/src/updater.rs | 191 +++++++++++++++++++++++++++++++++++ 3 files changed, 392 insertions(+) create mode 100644 docs/boot_state_machine.md diff --git a/cspell.config.yaml b/cspell.config.yaml index 724ec21..f3f0d05 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -36,6 +36,7 @@ words: - dllexport - dlopen - EDQUOT + - embedders - EOCD - endtemplate - ENOSPC @@ -70,6 +71,7 @@ words: - symbolication - unbootable - unbooted + - usize - Verdana - vmcode - withf diff --git a/docs/boot_state_machine.md b/docs/boot_state_machine.md new file mode 100644 index 0000000..22d2344 --- /dev/null +++ b/docs/boot_state_machine.md @@ -0,0 +1,199 @@ +# Boot State Machine + +## Overview + +The Shorebird updater manages over-the-air code updates for Flutter +applications. A critical part of this is the **boot state machine**, which +tracks: +- Which patch should be loaded on app start +- Whether a patch booted successfully +- Automatic rollback if a patch crashes during boot + +## State Variables + +### Persisted to Disk (`patches_state.json`) + +| Variable | Type | Description | +|----------|------|-------------| +| `next_boot_patch` | `Option` | The patch to boot on next app start. Set when a patch is downloaded/installed. | +| `last_booted_patch` | `Option` | The patch that last completed a successful boot cycle. Our "known good" state. | +| `currently_booting_patch` | `Option` | Transient flag: set when boot starts, cleared on success/failure. If set on init, indicates crash. | +| `known_bad_patches` | `HashSet` | Patches that have failed to boot. Never attempt these again for this release. | + +### In-Memory (Config) + +| Variable | Description | +|----------|-------------| +| `UpdateConfig` | Global config set once via `init()`. Contains app ID, paths, network hooks, etc. | + +## Boot Lifecycle + +### Happy Path + +``` +[Process Start] + │ + ▼ + init() + │ + ├─► Check currently_booting_patch + │ └─► If set: previous boot crashed → mark patch as bad, fall back + │ + ▼ + Engine gets next_boot_patch path + │ + ▼ + TryLoadFromPatch() loads patch snapshot + │ + └─► report_launch_start() [called once via std::once_flag] + │ └─► currently_booting_patch = next_boot_patch + │ + ▼ + Shell::Shell() constructor completes + │ + └─► report_launch_success() + │ ├─► last_booted_patch = currently_booting_patch + │ └─► currently_booting_patch = None + │ + ▼ + [App Running - Dart code executing] +``` + +### Crash Recovery + +If the app crashes between `report_launch_start()` and +`report_launch_success()`: + +1. Process dies with `currently_booting_patch` still set on disk +2. New process starts, calls `init()` +3. `handle_prior_boot_failure_if_necessary()` sees `currently_booting_patch` is + set +4. Marks that patch as failed (adds to `known_bad_patches`) +5. Falls back to `last_booted_patch` or base release + +## Implementation Details + +### Where Boot Lifecycle Calls Are Made + +**`report_launch_start()`** is called from `TryLoadFromPatch()` in +`runtime/shorebird/patch_cache.cc`: + +```cpp +std::shared_ptr TryLoadFromPatch(...) { + // ... validation and patch loading ... + + // Only report launch_start when we're actually about to use a patch, + // and only once per process (for the first symbol, isolate data) + static std::once_flag launch_start_flag; + if (symbol == kIsolateDataSymbol) { + std::call_once(launch_start_flag, []() { + shorebird_report_launch_start(); + }); + } + + // Return the mapping + // ... +} +``` + +**`report_launch_success()`** is called from `Shell::Shell()` constructor in +`shell/common/shorebird/shorebird.cc`, after the Dart VM is created +successfully. + +### Why This Placement Matters + +The boot lifecycle calls are placed at specific points for good reason: + +1. **`report_launch_start()` in `TryLoadFromPatch()`**: Called right before the + patched Dart snapshot is actually loaded. This ensures: + - FlutterEngineGroup warmup (which doesn't load patches) doesn't trigger + false crash detection + - The call only happens when we're actually about to use a patch + - `std::once_flag` ensures exactly one boot cycle per process + +2. **`report_launch_success()` in Shell constructor**: Called after the Dart VM + is created, indicating the patch loaded successfully. + +3. **Crash on patch load failure**: If `TryLoadFromPatch()` fails to load the + patch, it calls `FML_LOG(FATAL)` which crashes the process. On the next + launch, crash recovery naturally handles it. + +### Invariants + +1. **Config is set once per process**: `set_config()` returns error if already + set +2. **Crash recovery runs once per process**: Only on first successful `init()` +3. **One boot cycle per process**: `std::once_flag` ensures + `report_launch_start()` is called at most once +4. **State is persisted atomically**: Each state change writes to disk + immediately +5. **Bad patches are permanent**: Once in `known_bad_patches`, never tried again + for this release + +## API Behavior + +### `init()` +- Sets global config (once per process, subsequent calls return + `AlreadyInitialized`) +- Calls `handle_prior_boot_failure_if_necessary()` only on first init +- Does NOT run crash recovery if config already initialized + +### `report_launch_start()` +- If `next_boot_patch` exists, sets `currently_booting_patch` +- In production, called only once per process due to `std::once_flag` in C++ + +### `report_launch_success()` +- Clears `currently_booting_patch` +- Sets `last_booted_patch` +- Subsequent calls are no-ops if `currently_booting_patch` is None + +### `report_launch_failure()` +- Marks `currently_booting_patch` as bad +- Falls back to previous good state +- Queues failure event for server + +## Historical Context: FlutterEngineGroup Bug + +Prior to the current implementation, `report_launch_start()` was called from +`ConfigureShorebird()` during `FlutterMain::Init()`. This caused a bug: + +**Problem**: `FlutterEngineGroup`'s constructor calls +`ensureInitializationComplete()` which triggered `report_launch_start()`, but +does NOT create a Shell (no `report_launch_success()`). If the app was killed +before `createAndRunEngine()` was called, crash recovery incorrectly marked the +patch as bad. + +**Solution**: Move `report_launch_start()` to `TryLoadFromPatch()`, which is +only called when actually loading a patch. FlutterEngineGroup never calls +`TryLoadFromPatch()` (no Shell created), so no false positives occur. + +## Multiple Processes (Android) + +If multiple processes access the same state file: +- Each process has its own Java `initCalled` flag +- Each process has its own Rust global config +- BUT they share the same on-disk state files +- No cross-process locking exists + +This could cause issues if: +1. Process A sets `currently_booting_patch` +2. Process B also sets `currently_booting_patch` (it's the first engine in THAT + process) +3. Process A completes, clears flag +4. Process B is killed before completing +5. On restart: crash recovery sees flag set (by Process B) and marks patch as + bad + +## Testing + +Unit tests in `library/src/updater.rs` (`multi_engine_tests` module) verify the +boot state machine behavior: + +- `multi_engine_false_positive_rollback`: Demonstrates the historical bug where + multiple `report_launch_start()` calls caused false positive rollbacks +- `interleaved_boot_calls_success_clears_flag`: Verifies that + `report_launch_success()` properly clears the booting flag + +Note: In production, the C++ `std::once_flag` prevents multiple +`report_launch_start()` calls, so the Rust-level tests demonstrate behavior that +can only occur if the C++ guard is bypassed. diff --git a/library/src/updater.rs b/library/src/updater.rs index b55d8ac..995ebaf 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -2008,3 +2008,194 @@ mod check_for_downloadable_update_tests { Ok(()) } } + +#[cfg(test)] +mod multi_engine_tests { + use anyhow::Result; + use serial_test::serial; + use tempdir::TempDir; + + use crate::{ + network::{testing_set_network_hooks, PatchCheckResponse}, + report_launch_start, report_launch_success, test_utils::install_fake_patch, + updater::tests::init_for_testing, with_mut_state, with_state, + }; + + /// Sets up no-op network hooks so that the fire-and-forget thread spawned + /// by `report_launch_success` completes instantly without network I/O. + /// This prevents leaked threads from interfering with subsequent serial + /// tests that use mock servers. + fn set_noop_network_hooks() { + testing_set_network_hooks( + |_url, _request| { + Ok(PatchCheckResponse { + patch_available: false, + patch: None, + rolled_back_patch_numbers: None, + }) + }, + |_url| Ok(vec![]), + |_url, _event| Ok(()), + ); + } + + /// This test demonstrates what would happen if report_launch_start() is called multiple times. + /// + /// IMPORTANT: In production, this scenario does NOT occur because: + /// - The C++ TryLoadFromPatch() in runtime/shorebird/patch_cache.cc uses std::once_flag + /// - This ensures shorebird_report_launch_start() is only called once per process + /// - The call happens right before the patched snapshot is actually loaded + /// + /// This test documents the Rust API behavior in isolation: + /// If report_launch_start() IS called multiple times (e.g., from tests or custom embedders): + /// + /// Scenario: + /// 1. Engine A: starts boot (sets currently_booting_patch) + /// 2. Engine A: completes boot (clears currently_booting_patch) + /// 3. Engine B: starts boot (RE-SETS currently_booting_patch) + /// 4. Process is killed before Engine B completes + /// 5. On restart, crash recovery sees currently_booting_patch set and marks patch as bad + /// + /// This would be a FALSE POSITIVE - the patch didn't actually fail. + #[serial] + #[test] + fn multi_engine_false_positive_rollback() -> Result<()> { + let tmp_dir = TempDir::new("multi_engine_test").unwrap(); + init_for_testing(&tmp_dir, None); + set_noop_network_hooks(); + + // Install a patch + install_fake_patch(1)?; + + // Verify patch is installed and ready to boot + with_mut_state(|state| { + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); + assert!(state.currently_booting_patch().is_none()); + assert!(!state.is_known_bad_patch(1)); + Ok(()) + })?; + + // ======================================== + // Engine A: Full successful boot cycle + // ======================================== + report_launch_start()?; + + with_state(|state| { + // currently_booting_patch should be set + assert_eq!(state.currently_booting_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + report_launch_success()?; + + with_state(|state| { + // After success, currently_booting_patch should be cleared + assert!(state.currently_booting_patch().is_none()); + // And last_successfully_booted_patch should be set + assert_eq!( + state.last_successfully_booted_patch().map(|p| p.number), + Some(1) + ); + Ok(()) + })?; + + // ======================================== + // Engine B: Starts boot but process is killed + // ======================================== + // In real scenario, this is another FlutterEngine in the same process + // calling report_launch_start() after Engine A already finished. + report_launch_start()?; + + with_state(|state| { + // BUG: currently_booting_patch is now set AGAIN! + // This is the root cause - Engine B re-set the flag. + assert_eq!(state.currently_booting_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + // Process is killed here (Engine B never calls report_launch_success) + // We simulate this by reinitializing without completing Engine B's boot. + + // ======================================== + // New process: Crash recovery kicks in + // ======================================== + init_for_testing(&tmp_dir, None); + set_noop_network_hooks(); + + // The patch should NOT be marked as bad (Engine A booted successfully!) + // But when report_launch_start() is called multiple times at the Rust level, + // the second call re-sets currently_booting_patch, causing a false positive. + // + // NOTE: The C++ fix in TryLoadFromPatch() uses std::once_flag to prevent + // multiple calls, so this scenario cannot happen in production. This test + // documents the Rust API behavior when called directly without that guard. + with_mut_state(|state| { + // When report_launch_start() is called multiple times without a guard, + // the patch is incorrectly marked as known bad. + assert!( + state.is_known_bad_patch(1), + "Expected patch to be marked bad when report_launch_start() called twice" + ); + + // The next_boot_patch is None because patch was marked as bad + assert!( + state.next_boot_patch().is_none(), + "Expected next_boot_patch to be None after patch marked bad" + ); + + Ok(()) + })?; + + Ok(()) + } + + /// Test that interleaved start/success calls are handled correctly when + /// successes come after all starts. + /// + /// Scenario: start_A, start_B, success_A, [crash] + /// This works correctly because success_A clears the flag before the crash. + /// + /// NOTE: In production, the C++ std::once_flag in TryLoadFromPatch() prevents + /// multiple report_launch_start() calls. This test documents Rust API behavior. + #[serial] + #[test] + fn interleaved_boot_calls_success_clears_flag() -> Result<()> { + let tmp_dir = TempDir::new("interleaved_test").unwrap(); + init_for_testing(&tmp_dir, None); + set_noop_network_hooks(); + + install_fake_patch(1)?; + + // start_A + report_launch_start()?; + // start_B (simulated - same call, re-sets the same flag) + report_launch_start()?; + + with_state(|state| { + assert_eq!(state.currently_booting_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + // success_A + report_launch_success()?; + + with_state(|state| { + // Flag is cleared by success_A + assert!(state.currently_booting_patch().is_none()); + Ok(()) + })?; + + // Process killed here - success_B never called + init_for_testing(&tmp_dir, None); + set_noop_network_hooks(); + + with_mut_state(|state| { + // This should work correctly because success_A already cleared the flag + assert!(!state.is_known_bad_patch(1)); + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + Ok(()) + } +}