From 89200975f38964d0a91c8b73e1d6964cd8d0d550 Mon Sep 17 00:00:00 2001 From: BB-fat <45072480+BB-fat@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:45:34 +0800 Subject: [PATCH] fix(update): run periodic update check in daemon so hints actually fire (#76) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(update): run periodic update check in daemon so hints actually fire The CLI spawned a detached thread to refresh ~/.bsk/update-check.json, but the process exits as soon as the command finishes, killing the thread before the fetch could complete — so the cache was never written and the new-version hint never showed. Move the periodic check into the daemon: a tokio task ticks every 30 minutes (down from 24h), refreshes the cache when missing/stale, and logs failures at warn/info into the daemon log. The CLI path is now read-only: it prints the hint only from a fresh cache and never spawns threads or touches the network. * fix(update): decouple daemon refresh window so checks truly run every 30min The refresh decision reused the 30min tick interval as its freshness window, and with is_fresh()'s inclusive bound a cache written just after tick N still counted as fresh at tick N+1 — so in steady state the daemon only refetched every other tick (~60min). Judge staleness against DAEMON_REFRESH_WINDOW (25min = 5/6 of the tick) instead: every 30min tick now finds the cache stale and really refreshes, while a daemon restarted with a younger-than-25min cache still skips its first tick. CLI hint freshness semantics (UPDATE_CHECK_INTERVAL = 30min) are unchanged. --- crates/bsk-cli/src/cli/update.rs | 183 +++++++++++++++++++++++++---- crates/bsk-cli/src/daemon/start.rs | 65 ++++++++++ crates/bsk-cli/src/main.rs | 2 +- 3 files changed, 228 insertions(+), 22 deletions(-) diff --git a/crates/bsk-cli/src/cli/update.rs b/crates/bsk-cli/src/cli/update.rs index 679e0d2..fc93ba6 100644 --- a/crates/bsk-cli/src/cli/update.rs +++ b/crates/bsk-cli/src/cli/update.rs @@ -19,7 +19,21 @@ pub const DEFAULT_MANIFEST_URL: &str = "https://github.com/Tencent/BrowserSkill/releases/latest/download/version.json"; const FETCH_TIMEOUT: Duration = Duration::from_secs(10); const ARCHIVE_FETCH_TIMEOUT: Duration = Duration::from_secs(60); -const UPDATE_CHECK_INTERVAL: Duration = Duration::from_secs(24 * 60 * 60); +/// How often the daemon ticks the update check, and how long a cache +/// entry counts as fresh for the CLI hint. The daemon is the only +/// writer; CLI commands only ever read the cache. +pub(crate) const UPDATE_CHECK_INTERVAL: Duration = Duration::from_secs(30 * 60); + +/// Freshness window the daemon uses to decide whether a tick actually +/// refreshes the cache. Deliberately shorter than the tick cadence +/// ([`UPDATE_CHECK_INTERVAL`]): with an equal window, a cache refreshed +/// just after tick N would still count as fresh at tick N+1 (age just +/// under 30min), so steady state would only refetch every *other* tick. +/// At 5/6 of the interval (25min) every 30-minute tick finds the cache +/// stale and really refreshes it, while a daemon restarted with a +/// younger-than-25min cache still skips its first tick. +pub(crate) const DAEMON_REFRESH_WINDOW: Duration = + Duration::from_secs(UPDATE_CHECK_INTERVAL.as_secs() * 5 / 6); #[derive(Debug, Clone)] pub struct UpdateManifest { @@ -423,14 +437,30 @@ pub fn write_update_cache(path: &Path, cache: &UpdateCheckCache) -> Result<()> { Ok(()) } -fn now_epoch_secs() -> u64 { +pub(crate) fn now_epoch_secs() -> u64 { SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .map(|duration| duration.as_secs()) .unwrap_or(0) } -fn refresh_update_cache(cache_path: &Path) -> Result> { +/// Decide whether the update cache needs a network refresh: a missing +/// cache always does, a present one only once it is no longer fresh. +pub(crate) fn cache_needs_refresh( + cache: Option<&UpdateCheckCache>, + now_epoch_secs: u64, + interval: Duration, +) -> bool { + match cache { + Some(cache) => !cache.is_fresh(now_epoch_secs, interval), + None => true, + } +} + +/// Fetch the update manifest and rewrite the cache. Returns the update +/// hint when a newer version exists. Blocking: call from a blocking +/// context (the daemon wraps it in `spawn_blocking`). +pub(crate) fn refresh_update_cache(cache_path: &Path) -> Result> { let manifest = fetch_manifest(&manifest_url())?; let platform = current_platform_key()?; let hint = update_hint_for_manifest(&manifest, env!("CARGO_PKG_VERSION"), platform)?; @@ -444,7 +474,15 @@ fn refresh_update_cache(cache_path: &Path) -> Result> { Ok(hint) } -pub fn maybe_spawn_background_check(flags: &super::GlobalFlags, command: &super::Command) { +/// Print the cached "new version available" hint, if there is one. +/// +/// Read-only by design: the daemon's periodic task owns refreshing +/// `~/.bsk/update-check.json`, so a missing or stale cache just means +/// "stay quiet" — this function never spawns threads or touches the +/// network. (The old implementation spawned a detached refresh thread +/// that the exiting CLI process almost always killed before it could +/// write the cache, so the hint never fired.) +pub fn print_update_hint_from_cache(flags: &super::GlobalFlags, command: &super::Command) { if flags.quiet || flags.json || matches!( @@ -458,29 +496,29 @@ pub fn maybe_spawn_background_check(flags: &super::GlobalFlags, command: &super: let Ok(cache_path) = crate::daemon::paths::update_check_path() else { return; }; - let now = now_epoch_secs(); - match read_update_cache(&cache_path) { - Ok(Some(cache)) => { - if let Some(hint) = update_hint_for_cache(&cache, env!("CARGO_PKG_VERSION")) { - eprintln!("{hint}"); - } - if cache.is_fresh(now, UPDATE_CHECK_INTERVAL) { - return; - } - } + match cached_update_hint(&cache_path, env!("CARGO_PKG_VERSION"), now_epoch_secs()) { + Ok(Some(hint)) => eprintln!("{hint}"), Ok(None) => {} Err(err) => { tracing::debug!(error = %err, "update cache read failed"); } } +} - let _ = std::thread::Builder::new() - .name("bsk-update-check".to_string()) - .spawn(move || match refresh_update_cache(&cache_path) { - Ok(Some(hint)) => eprintln!("{hint}"), - Ok(None) => {} - Err(err) => tracing::debug!(error = %err, "background update check failed"), - }); +/// The hint to show for a cache file: only when the cache is present, +/// fresh, and names a version newer than `current_version`. +fn cached_update_hint( + cache_path: &Path, + current_version: &str, + now_epoch_secs: u64, +) -> Result> { + let Some(cache) = read_update_cache(cache_path)? else { + return Ok(None); + }; + if !cache.is_fresh(now_epoch_secs, UPDATE_CHECK_INTERVAL) { + return Ok(None); + } + Ok(update_hint_for_cache(&cache, current_version)) } fn confirm_update(candidate: &UpdateCandidate) -> Result { @@ -851,6 +889,109 @@ mod tests { assert_eq!(read_update_cache(&path).unwrap(), Some(cache)); } + #[test] + fn update_check_interval_is_thirty_minutes() { + assert_eq!(UPDATE_CHECK_INTERVAL, Duration::from_secs(1800)); + } + + #[test] + fn cache_needs_refresh_only_when_missing_or_stale() { + let fresh = UpdateCheckCache { + checked_at_epoch_secs: 1000, + latest_version: "0.2.0".to_string(), + }; + let stale = UpdateCheckCache { + checked_at_epoch_secs: 100, + latest_version: "0.2.0".to_string(), + }; + let now = 1000 + UPDATE_CHECK_INTERVAL.as_secs(); + + assert!(cache_needs_refresh(None, now, UPDATE_CHECK_INTERVAL)); + assert!(!cache_needs_refresh( + Some(&fresh), + now, + UPDATE_CHECK_INTERVAL + )); + assert!(cache_needs_refresh( + Some(&stale), + now, + UPDATE_CHECK_INTERVAL + )); + } + + #[test] + fn daemon_refresh_window_is_shorter_than_tick() { + // 25 minutes: 5/6 of the 30-minute tick. + assert_eq!(DAEMON_REFRESH_WINDOW, Duration::from_secs(1500)); + assert!(DAEMON_REFRESH_WINDOW < UPDATE_CHECK_INTERVAL); + } + + #[test] + fn daemon_refresh_window_refreshes_every_tick_in_steady_state() { + // A cache written just after tick N must count as stale at tick + // N+1 (30 minutes later), so every tick really refetches. + let cache = UpdateCheckCache { + checked_at_epoch_secs: 10_000, + latest_version: "0.2.0".to_string(), + }; + let next_tick = 10_000 + UPDATE_CHECK_INTERVAL.as_secs(); + assert!(cache_needs_refresh( + Some(&cache), + next_tick, + DAEMON_REFRESH_WINDOW + )); + + // ... while a daemon restarted with a cache younger than the + // refresh window still skips the fetch. + let just_checked = 10_000 + Duration::from_secs(10 * 60).as_secs(); + assert!(!cache_needs_refresh( + Some(&cache), + just_checked, + DAEMON_REFRESH_WINDOW + )); + } + + #[test] + fn cached_update_hint_only_for_fresh_newer_cache() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("update-check.json"); + let now = now_epoch_secs(); + + // Missing cache -> no hint. + assert_eq!(cached_update_hint(&path, "0.1.7", now).unwrap(), None); + + // Fresh cache with a newer version -> hint. + let fresh_newer = UpdateCheckCache { + checked_at_epoch_secs: now, + latest_version: "0.2.0".to_string(), + }; + write_update_cache(&path, &fresh_newer).unwrap(); + assert_eq!( + cached_update_hint(&path, "0.1.7", now).unwrap(), + Some("A new bsk version is available: 0.1.7 -> 0.2.0. Run `bsk update`.".to_string()) + ); + + // Fresh cache without a newer version -> no hint. + let fresh_current = UpdateCheckCache { + checked_at_epoch_secs: now, + latest_version: "0.1.7".to_string(), + }; + write_update_cache(&path, &fresh_current).unwrap(); + assert_eq!(cached_update_hint(&path, "0.1.7", now).unwrap(), None); + + // Stale cache, even with a newer version -> no hint. + let stale_newer = UpdateCheckCache { + checked_at_epoch_secs: now - UPDATE_CHECK_INTERVAL.as_secs() - 1, + latest_version: "0.2.0".to_string(), + }; + write_update_cache(&path, &stale_newer).unwrap(); + assert_eq!(cached_update_hint(&path, "0.1.7", now).unwrap(), None); + + // Corrupt cache file -> error surfaced to the caller, no panic. + std::fs::write(&path, b"not json").unwrap(); + assert!(cached_update_hint(&path, "0.1.7", now).is_err()); + } + fn tar_gz_with_bsk(binary: &[u8]) -> Vec { let mut tar_bytes = Vec::new(); { diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index 33327a1..f0c2a3c 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -220,6 +220,7 @@ pub fn run_foreground(cfg: DaemonConfig) -> Result<()> { let state = Arc::new(DaemonState::new(cfg.clone())); let session_idle_task = spawn_session_idle_reaper(Arc::clone(&state)); let browser_liveness_task = spawn_browser_liveness_reaper(Arc::clone(&state)); + let update_check_task = spawn_update_check_task(); let ws_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), cfg.ws_port); let ws_handle = ws::WsServer::new(Arc::clone(&state)) .bind(ws_addr) @@ -383,6 +384,8 @@ pub fn run_foreground(cfg: DaemonConfig) -> Result<()> { let _ = session_idle_task.await; browser_liveness_task.abort(); let _ = browser_liveness_task.await; + update_check_task.abort(); + let _ = update_check_task.await; ws_handle.shutdown.notify_waiters(); let _ = ws_handle.task.await; @@ -491,6 +494,68 @@ pub(crate) fn spawn_session_idle_reaper(state: Arc) -> tokio::task: }) } +/// Spawn the periodic update check owned by the production daemon. +/// +/// The daemon is the only writer of `~/.bsk/update-check.json`; CLI +/// commands only read it to print the "new version available" hint +/// (see [`crate::cli::update::print_update_hint_from_cache`]). The first +/// tick of `tokio::time::interval` is immediate, so the check runs right +/// after startup and then every +/// [`crate::cli::update::UPDATE_CHECK_INTERVAL`]. Each tick re-reads the +/// cache and skips the network fetch while it is still fresh within +/// [`crate::cli::update::DAEMON_REFRESH_WINDOW`] (25min — shorter than +/// the 30min tick, so steady state really refreshes on every tick +/// instead of every other one). The task +/// loops forever; shutdown aborts it like the other background tasks, so +/// it never delays daemon exit (an in-flight fetch is bounded by the +/// update client's own timeout and detached on abort). +pub(crate) fn spawn_update_check_task() -> tokio::task::JoinHandle<()> { + use crate::cli::update; + + tokio::spawn(async move { + let cache_path = match paths::update_check_path() { + Ok(path) => path, + Err(err) => { + warn!(error = %err, "periodic update check disabled: cannot resolve cache path"); + return; + } + }; + + let mut ticker = tokio::time::interval(update::UPDATE_CHECK_INTERVAL); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + loop { + ticker.tick().await; + + let needs_refresh = match update::read_update_cache(&cache_path) { + Ok(cache) => update::cache_needs_refresh( + cache.as_ref(), + update::now_epoch_secs(), + update::DAEMON_REFRESH_WINDOW, + ), + Err(err) => { + warn!(error = %err, "update cache unreadable; will refresh it"); + true + } + }; + if !needs_refresh { + debug!("update cache still fresh; skipping update check"); + continue; + } + + let result = { + let cache_path = cache_path.clone(); + tokio::task::spawn_blocking(move || update::refresh_update_cache(&cache_path)).await + }; + match result { + Ok(Ok(Some(hint))) => info!(%hint, "periodic update check found a new version"), + Ok(Ok(None)) => info!("periodic update check refreshed cache (already up to date)"), + Ok(Err(err)) => warn!(error = %err, "periodic update check failed"), + Err(err) => warn!(error = %err, "periodic update check task panicked"), + } + } + }) +} + fn record_activity(activity: &Arc>) { if let Ok(mut a) = activity.lock() { *a = Instant::now(); diff --git a/crates/bsk-cli/src/main.rs b/crates/bsk-cli/src/main.rs index 3e0c2e1..86c1451 100644 --- a/crates/bsk-cli/src/main.rs +++ b/crates/bsk-cli/src/main.rs @@ -25,7 +25,7 @@ fn main() -> ExitCode { if !matches!(cli.command, Command::Daemon(_)) { init_cli_tracing(&cli.flags); } - cli::update::maybe_spawn_background_check(&cli.flags, &cli.command); + cli::update::print_update_hint_from_cache(&cli.flags, &cli.command); let format = if cli.flags.json { Format::Json