fix(update): run periodic update check in daemon so hints actually fire (#76)

* 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

@@ -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<Option<String>> {
+/// 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<Option<String>> {
     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<Option<String>> {
     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<Option<String>> {
+    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<bool> {
@@ -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<u8> {
         let mut tar_bytes = Vec::new();
         {

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<DaemonState>) -> 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<Mutex<Instant>>) {
     if let Ok(mut a) = activity.lock() {
         *a = Instant::now();

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