Make sure to query app verion every 24 hours

even if the daemon was restarted.

2 files changed, 123 insertions, 59 deletions

diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs
index d0fad0493d..b8629ee32d 100644
--- a/mullvad-daemon/src/lib.rs
+++ b/mullvad-daemon/src/lib.rs
@@ -704,8 +704,6 @@ where
             settings_event_listener.notify_settings(settings.to_owned());
         });
 
-        let app_version_info = version_check::load_cache(&cache_dir).await;
-
         let initial_selector_config = new_selector_config(&settings);
         let relay_selector = RelaySelector::new(
             initial_selector_config,
@@ -868,9 +866,10 @@ where
             api_availability.clone(),
             cache_dir.clone(),
             internal_event_tx.to_specialized_sender(),
-            app_version_info.clone(),
             settings.show_beta_releases,
-        );
+        )
+        .await;
+        let app_version_info = version_updater.last_app_version_info().cloned();
         tokio::spawn(version_updater.run());
 
         // Attempt to download a fresh relay list
diff --git a/mullvad-daemon/src/version_check.rs b/mullvad-daemon/src/version_check.rs
index afb1d7bee8..b87da5db5c 100644
--- a/mullvad-daemon/src/version_check.rs
+++ b/mullvad-daemon/src/version_check.rs
@@ -1,6 +1,7 @@
 use crate::{version::is_beta_version, DaemonEventSender};
 use futures::{
     channel::{mpsc, oneshot},
+    future::FusedFuture,
     stream::FusedStream,
     FutureExt, SinkExt, StreamExt, TryFutureExt,
 };
@@ -9,16 +10,18 @@ use mullvad_types::version::{AppVersionInfo, ParsedAppVersion};
 use once_cell::sync::Lazy;
 use serde::{Deserialize, Serialize};
 use std::{
+    cmp::max,
     future::Future,
     io,
     path::{Path, PathBuf},
+    pin::Pin,
     str::FromStr,
-    time::Duration,
+    time::{Duration, SystemTime},
 };
 use talpid_core::mpsc::Sender;
 use talpid_future::retry::{retry_future, ConstantInterval};
 use talpid_types::ErrorExt;
-use tokio::fs::{self, File};
+use tokio::{fs::File, io::AsyncReadExt};
 
 const VERSION_INFO_FILENAME: &str = "version-info.json";
 
@@ -94,12 +97,15 @@ pub(crate) struct VersionUpdater {
     version_proxy: AppVersionProxy,
     cache_path: PathBuf,
     update_sender: DaemonEventSender<AppVersionInfo>,
-    last_app_version_info: Option<AppVersionInfo>,
+    /// The last known [AppVersionInfo], along with the time it was determined.
+    last_app_version_info: Option<(AppVersionInfo, SystemTime)>,
     platform_version: String,
     show_beta_releases: bool,
     rx: Option<mpsc::Receiver<VersionUpdaterCommand>>,
     availability_handle: ApiAvailabilityHandle,
-    internal_done_tx: Option<oneshot::Sender<AppVersionInfo>>,
+
+    /// Oneshot channels for responding to [VersionUpdaterCommand::RunVersionCheck].
+    run_version_check_responders: Vec<oneshot::Sender<AppVersionInfo>>,
 }
 
 #[derive(Clone)]
@@ -142,14 +148,16 @@ impl VersionUpdaterHandle {
 }
 
 impl VersionUpdater {
-    pub fn new(
+    pub async fn new(
         mut api_handle: MullvadRestHandle,
         availability_handle: ApiAvailabilityHandle,
         cache_dir: PathBuf,
         update_sender: DaemonEventSender<AppVersionInfo>,
-        last_app_version_info: Option<AppVersionInfo>,
         show_beta_releases: bool,
     ) -> (Self, VersionUpdaterHandle) {
+        // load the last known AppVersionInfo from cache
+        let last_app_version_info = load_cache(&cache_dir).await;
+
         api_handle.factory = api_handle.factory.default_timeout(DOWNLOAD_TIMEOUT);
         let version_proxy = AppVersionProxy::new(api_handle);
         let cache_path = cache_dir.join(VERSION_INFO_FILENAME);
@@ -166,20 +174,23 @@ impl VersionUpdater {
                 show_beta_releases,
                 rx: Some(rx),
                 availability_handle,
-                internal_done_tx: None,
+                run_version_check_responders: vec![],
             },
             VersionUpdaterHandle { tx },
         )
     }
 
-    fn create_update_future(
+    /// Get the last known [AppVersionInfo]. May be stale.
+    pub fn last_app_version_info(&self) -> Option<&AppVersionInfo> {
+        self.last_app_version_info.as_ref().map(|(info, _)| info)
+    }
+
+    /// Immediately query the API for the latest [AppVersionInfo].
+    fn query_app_version(
         &mut self,
-        done_tx: oneshot::Sender<AppVersionInfo>,
-    ) -> std::pin::Pin<
+    ) -> Pin<
         Box<dyn Future<Output = Result<mullvad_api::AppVersionResponse, Error>> + Send + 'static>,
     > {
-        self.internal_done_tx = Some(done_tx);
-
         let api_handle = self.availability_handle.clone();
         let version_proxy = self.version_proxy.clone();
         let platform_version = self.platform_version.clone();
@@ -193,28 +204,31 @@ impl VersionUpdater {
                 .map_err(Error::Download)
         };
 
+        // retry immediately on network errors (unless we're offline)
+        let should_retry_immediate = move |result: &Result<_, Error>| {
+            if let Err(Error::Download(error)) = result {
+                error.is_network_error() && !api_handle.get_state().is_offline()
+            } else {
+                false
+            }
+        };
+
         Box::pin(retry_future(
             download_future_factory,
-            move |result| Self::should_retry_immediate(result, &api_handle),
+            should_retry_immediate,
             IMMEDIATE_RETRY_STRATEGY,
         ))
     }
 
-    fn should_retry_immediate<T>(
-        result: &Result<T, Error>,
-        api_handle: &ApiAvailabilityHandle,
-    ) -> bool {
-        match result {
-            Err(Error::Download(error)) if error.is_network_error() => {
-                !api_handle.get_state().is_offline()
-            }
-            _ => false,
-        }
-    }
-
-    fn create_update_background_future(
+    /// Query the API for the latest [AppVersionInfo].
+    ///
+    /// This function waits until background calls are enabled in
+    /// [ApiAvailability](mullvad_api::availability::ApiAvailability).
+    ///
+    /// On any error, this function retries repeatedly every [UPDATE_INTERVAL_ERROR] until success.
+    fn query_app_version_in_background(
         &self,
-    ) -> std::pin::Pin<
+    ) -> Pin<
         Box<dyn Future<Output = Result<mullvad_api::AppVersionResponse, Error>> + Send + 'static>,
     > {
         let api_handle = self.availability_handle.clone();
@@ -240,8 +254,9 @@ impl VersionUpdater {
         ))
     }
 
+    /// Write [Self::last_app_version_info], if any, to the cache file ([VERSION_INFO_FILENAME]).
     async fn write_cache(&self) -> Result<(), Error> {
-        let last_app_version_info = match self.last_app_version_info.as_ref() {
+        let last_app_version_info = match self.last_app_version_info() {
             Some(version_info) => version_info,
             None => {
                 log::debug!("The version cache is empty -- not writing");
@@ -265,6 +280,7 @@ impl VersionUpdater {
         Ok(())
     }
 
+    /// Convert a [mullvad_api::AppVersionResponse] to an [AppVersionInfo].
     fn response_to_version_info(
         &mut self,
         response: mullvad_api::AppVersionResponse,
@@ -284,6 +300,7 @@ impl VersionUpdater {
         }
     }
 
+    /// If current_version is not the latest, return a string containing the latest version.
     fn suggested_upgrade(
         current_version: &ParsedAppVersion,
         latest_stable: &Option<String>,
@@ -300,35 +317,60 @@ impl VersionUpdater {
             None
         };
 
-        let latest_version = stable_version.iter().chain(beta_version.iter()).max()?;
+        let latest_version = max(stable_version, beta_version)?;
 
-        if current_version < latest_version {
+        if current_version < &latest_version {
             Some(latest_version.to_string())
         } else {
             None
         }
     }
 
+    /// Update [Self::last_app_version_info] and write it to disk cache.
+    ///
+    /// Also, if we are currently have a pending [RunVersionCheck][rvc] command, respond to it.
+    ///
+    /// [rvc]: VersionUpdaterCommand::RunVersionCheck
     async fn update_version_info(&mut self, new_version_info: AppVersionInfo) {
-        if let Some(done_tx) = self.internal_done_tx.take() {
-            let _ = done_tx.send(new_version_info.clone());
-        }
-
         // if daemon can't be reached, return immediately
         if self.update_sender.send(new_version_info.clone()).is_err() {
             return;
         }
 
-        self.last_app_version_info = Some(new_version_info);
+        self.last_app_version_info = Some((new_version_info, SystemTime::now()));
         if let Err(err) = self.write_cache().await {
             log::error!("Failed to save version cache to disk: {}", err);
         }
     }
 
+    /// Wait until [VersionUpdater::last_app_version_info] becomes stale and needs to be refreshed.
+    ///
+    /// This happens [UPDATE_INTERVAL] after the last version query.
+    fn wait_until_version_is_stale(&self) -> Pin<Box<impl FusedFuture<Output = ()>>> {
+        let now = SystemTime::now();
+        let time_until_stale = self
+            .last_app_version_info
+            .as_ref()
+            .map(|(_, last_update_time)| last_update_time)
+            .and_then(|&last_update_time| now.duration_since(last_update_time).ok())
+            .map(|time_since_last_update| UPDATE_INTERVAL.saturating_sub(time_since_last_update))
+            // if there is no last_app_version_info, or if clocks are being weird,
+            // assume that the version is stale
+            .unwrap_or(Duration::ZERO);
+
+        // Boxed, pinned, and fused.
+        // Alternate title: "We don't want to deal with the borrow checker."
+        Box::pin(talpid_time::sleep(time_until_stale).fuse())
+    }
+
+    /// Returns true if we are currently handling one or more `RunVersionCheck` commands.
+    fn is_running_version_check(&self) -> bool {
+        !self.run_version_check_responders.is_empty()
+    }
+
     pub async fn run(mut self) {
         let mut rx = self.rx.take().unwrap().fuse();
-        let next_delay = || Box::pin(talpid_time::sleep(UPDATE_INTERVAL)).fuse();
-        let mut check_delay = next_delay();
+        let mut version_is_stale = self.wait_until_version_is_stale();
         let mut version_check = futures::future::Fuse::terminated();
 
         // If this is a dev build, there's no need to pester the API for version checks.
@@ -351,8 +393,8 @@ impl VersionUpdater {
                             self.show_beta_releases = show_beta_releases;
 
                             if let Some(last_app_version_info) = self
-                                .last_app_version_info
-                                .clone()
+                                .last_app_version_info()
+                                .cloned()
                             {
                                 let suggested_upgrade = Self::suggested_upgrade(
                                     &APP_VERSION,
@@ -373,8 +415,12 @@ impl VersionUpdater {
                             if self.update_sender.is_closed() {
                                 return;
                             }
-                            let download_future = self.create_update_future(done_tx).fuse();
-                            version_check = download_future;
+
+                            if !self.is_running_version_check() {
+                                version_check = self.query_app_version().fuse();
+                            }
+
+                            self.run_version_check_responders.push(done_tx);
                         }
                         // time to shut down
                         None => {
@@ -383,15 +429,14 @@ impl VersionUpdater {
                     }
                 },
 
-                _sleep = check_delay => {
+                _ = version_is_stale => {
                     if rx.is_terminated() || self.update_sender.is_closed() {
                         return;
                     }
-                    if self.internal_done_tx.is_some() {
-                        // Sync check in progress
+                    if self.is_running_version_check() {
                         continue;
                     }
-                    version_check = self.create_update_background_future().fuse();
+                    version_check = self.query_app_version_in_background().fuse();
                 },
 
                 response = version_check => {
@@ -403,42 +448,62 @@ impl VersionUpdater {
                         Ok(version_info_response) => {
                             let new_version_info =
                                 self.response_to_version_info(version_info_response);
+
+                            // Respond to all pending RunVersionCheck commands
+                            for done_tx in self.run_version_check_responders.drain(..) {
+                                let _ = done_tx.send(new_version_info.clone());
+                            }
+
                             self.update_version_info(new_version_info).await;
-                        },
+
+                        }
                         Err(err) => {
-                            log::error!("Failed to fetch version info: {}", err);
-                            self.internal_done_tx = None;
-                        },
+                            log::error!("Failed to fetch version info: {err:#}");
+                            self.run_version_check_responders.clear();
+                        }
                     }
 
-                    check_delay = next_delay();
+                    version_is_stale = self.wait_until_version_is_stale();
                 },
             }
         }
     }
 }
 
-async fn try_load_cache(cache_dir: &Path) -> Result<AppVersionInfo, Error> {
+async fn try_load_cache(cache_dir: &Path) -> Result<(AppVersionInfo, SystemTime), Error> {
     if *IS_DEV_BUILD {
-        return Ok(dev_version_cache());
+        return Ok((dev_version_cache(), SystemTime::now()));
     }
 
     let path = cache_dir.join(VERSION_INFO_FILENAME);
     log::debug!("Loading version check cache from {}", path.display());
-    let content = fs::read_to_string(&path)
+
+    let mut file = File::open(&path).map_err(Error::ReadVersionCache).await?;
+    let meta = file.metadata().map_err(Error::ReadVersionCache).await?;
+    let mtime = meta
+        .modified()
+        .expect("Platforms without file modification times aren't supported");
+
+    let mut content = String::new();
+    file.read_to_string(&mut content)
         .map_err(Error::ReadVersionCache)
         .await?;
+
     let version_info: CachedAppVersionInfo =
         serde_json::from_str(&content).map_err(Error::Deserialize)?;
 
     if version_info.cached_from_version == mullvad_version::VERSION {
-        Ok(version_info.version_info)
+        Ok((version_info.version_info, mtime))
     } else {
         Err(Error::CacheVersionMismatch)
     }
 }
 
-pub async fn load_cache(cache_dir: &Path) -> Option<AppVersionInfo> {
+/// Read the app version cache from the provided directory.
+///
+/// Returns the [AppVersionInfo] along with the modification time of the cache file,
+/// or `None` on any error.
+async fn load_cache(cache_dir: &Path) -> Option<(AppVersionInfo, SystemTime)> {
     match try_load_cache(cache_dir).await {
         Ok(app_version_info) => Some(app_version_info),
         Err(error) => {