fix: populate current_patch_number from running_patch, not the boot breadcrumb (#363)
* fix: send running_patch as current_patch_number on patch check Both patch-check construction sites read current_patch_number from state.currently_booting_patch(), a transient boot-handshake breadcrumb that record_boot_success() clears to None before any Dart-initiated patch check fires. The field was therefore omitted from the wire for essentially the entire fleet. Use state.running_patch() instead: the session-scoped accessor set at report_launch_start, kept for the whole session, surviving server-driven rollbacks. This is the same value the FFI symbol shorebird_current_boot_patch_number already exposes. * test: assert patch-check request carries running_patch as current_patch_number Covers both construction sites (check_for_downloadable_update and update). Reproduces the steady state where the bug manifested — patch booted, currently_booting_patch cleared by record_boot_success — and asserts the request still carries the running patch number. Verified to fail (field omitted, captured as -1) against the pre-fix currently_booting_patch() code. * test: cover all changed lines in patch-check regression test Codecov's patch gate flagged 13 uncovered lines in the new test (78.7% patch coverage), none of them the fix: the no-op download/report closures never fired, and multi-line assert failure messages are only reached when an assertion panics. Swap the closures for the existing UNEXPECTED_DOWNLOAD/UNEXPECTED_REPORT placeholders (also asserting that path is never hit), collapse the assertions to single lines with the sentinel legend moved to comments, and tighten the module doc. Changed lines now at 100% patch coverage; fmt, clippy, and the full suite stay green.
This commit is contained in:
+109
-2
@@ -282,7 +282,7 @@ pub fn check_for_downloadable_update(channel: Option<&str>) -> anyhow::Result<bo
|
||||
let (client_id, current_patch_number) = with_state(|state| {
|
||||
Ok((
|
||||
state.client_id().to_string(),
|
||||
state.currently_booting_patch().map(|p| p.number),
|
||||
state.running_patch().map(|p| p.number),
|
||||
))
|
||||
})?;
|
||||
|
||||
@@ -417,7 +417,7 @@ fn update_internal(_: &UpdaterLockState, channel: Option<&str>) -> anyhow::Resul
|
||||
Ok(PatchCheckRequest::new(
|
||||
&config,
|
||||
&state.client_id(),
|
||||
state.currently_booting_patch().map(|p| p.number),
|
||||
state.running_patch().map(|p| p.number),
|
||||
))
|
||||
})?;
|
||||
|
||||
@@ -4079,3 +4079,110 @@ mod multi_engine_tests {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression tests asserting the patch-check request's `current_patch_number`
|
||||
/// carries the running patch, not the boot breadcrumb (`currently_booting_patch`)
|
||||
/// which `report_launch_success` clears before any patch check fires. Covers
|
||||
/// both construction sites: `check_for_downloadable_update` and `update`.
|
||||
#[cfg(test)]
|
||||
mod patch_check_current_patch_number_tests {
|
||||
use anyhow::Result;
|
||||
use serial_test::serial;
|
||||
use std::sync::atomic::{AtomicI64, Ordering};
|
||||
use tempfile::TempDir;
|
||||
|
||||
use crate::{
|
||||
check_for_downloadable_update,
|
||||
network::{
|
||||
testing_set_network_hooks, PatchCheckResponse, UNEXPECTED_DOWNLOAD, UNEXPECTED_REPORT,
|
||||
},
|
||||
report_launch_start, report_launch_success,
|
||||
test_utils::install_fake_patch,
|
||||
update,
|
||||
updater::tests::init_for_testing,
|
||||
with_state,
|
||||
};
|
||||
|
||||
/// `current_patch_number` was `None` on the wire (the bug we're guarding).
|
||||
const FIELD_OMITTED: i64 = -1;
|
||||
/// The patch-check hook never ran.
|
||||
const HOOK_NOT_CALLED: i64 = i64::MIN;
|
||||
|
||||
/// Last `current_patch_number` seen by the patch-check hook. Shared by
|
||||
/// both tests; safe because they are `#[serial]` (only one runs at a
|
||||
/// time) and `arrange_capturing_hooks` resets it before each check.
|
||||
static CAPTURED: AtomicI64 = AtomicI64::new(HOOK_NOT_CALLED);
|
||||
|
||||
/// Installs patch 1 and completes a full boot, then asserts the steady
|
||||
/// state in which patch checks actually run: the boot breadcrumb is
|
||||
/// cleared, but the process is still running patch 1. Reading
|
||||
/// `currently_booting_patch` here yields `None` — that is precisely the
|
||||
/// regression this guards against.
|
||||
fn boot_patch_one(tmp_dir: &TempDir) -> Result<()> {
|
||||
init_for_testing(tmp_dir, None);
|
||||
install_fake_patch(1)?;
|
||||
report_launch_start()?;
|
||||
report_launch_success()?;
|
||||
with_state(|state| {
|
||||
// Steady state: the boot breadcrumb is cleared, yet patch 1 is
|
||||
// still the running patch.
|
||||
assert!(state.currently_booting_patch().is_none());
|
||||
assert_eq!(state.running_patch().map(|p| p.number), Some(1));
|
||||
Ok(())
|
||||
})
|
||||
}
|
||||
|
||||
/// Installs network hooks whose patch-check leg records the request's
|
||||
/// `current_patch_number` into `CAPTURED` (`None` -> `FIELD_OMITTED`) and
|
||||
/// reports no available update.
|
||||
fn arrange_capturing_hooks() {
|
||||
CAPTURED.store(HOOK_NOT_CALLED, Ordering::SeqCst);
|
||||
testing_set_network_hooks(
|
||||
|_url, request| {
|
||||
CAPTURED.store(
|
||||
request
|
||||
.current_patch_number
|
||||
.map(|n| n as i64)
|
||||
.unwrap_or(FIELD_OMITTED),
|
||||
Ordering::SeqCst,
|
||||
);
|
||||
Ok(PatchCheckResponse {
|
||||
patch_available: false,
|
||||
patch: None,
|
||||
rolled_back_patch_numbers: None,
|
||||
})
|
||||
},
|
||||
// No update is offered, so neither hook should ever fire.
|
||||
UNEXPECTED_DOWNLOAD,
|
||||
UNEXPECTED_REPORT,
|
||||
);
|
||||
}
|
||||
|
||||
#[serial]
|
||||
#[test]
|
||||
fn check_for_downloadable_update_sends_running_patch() -> Result<()> {
|
||||
let tmp_dir = TempDir::new().unwrap();
|
||||
boot_patch_one(&tmp_dir)?;
|
||||
arrange_capturing_hooks();
|
||||
|
||||
check_for_downloadable_update(None)?;
|
||||
|
||||
// 1 = running patch sent; -1 = field omitted (the bug); i64::MIN = hook never ran.
|
||||
assert_eq!(CAPTURED.load(Ordering::SeqCst), 1);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[serial]
|
||||
#[test]
|
||||
fn update_sends_running_patch() -> Result<()> {
|
||||
let tmp_dir = TempDir::new().unwrap();
|
||||
boot_patch_one(&tmp_dir)?;
|
||||
arrange_capturing_hooks();
|
||||
|
||||
update(None)?;
|
||||
|
||||
// 1 = running patch sent; -1 = field omitted (the bug); i64::MIN = hook never ran.
|
||||
assert_eq!(CAPTURED.load(Ordering::SeqCst), 1);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user