Make sure to query app version every 24 hours

[hulthe]

---

This change is 

[linear]

DES-811 Version checks are too infrequent

[dlon]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @hulthe)

---

mullvad-daemon/src/version_check.rs line 360 at r1 (raw file):

            .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 - time_since_last_update)

I think that this needs to be a checked subtraction, or it might panic.

---

mullvad-daemon/src/version_check.rs line 413 at r1 (raw file):

                            }
                        }
                        Some(VersionUpdaterCommand::RunVersionCheck(done_tx)) => {

Perhaps this function should only run version check if the current cache is actually stale (either None or too old), and otherwise just return the last known version. Then I believe we could get rid of app_version_info from Daemon.

---

mullvad-daemon/src/version_check.rs line 417 at r1 (raw file):

                                return;
                            }
                            // TODO: should we support multiple concurrent RunVersionCheck calls?

Yes. :)

---

mullvad-daemon/src/version_check.rs line 452 at r1 (raw file):

                        Err(err) => {
                            log::error!("Failed to fetch version info: {err:#}");
                            self.run_version_check_responder = None;

Should we handle responding to self.run_version_check_responder in the Ok case instead of in self.update_version_info? It feels too spaghetti.

---

mullvad-daemon/src/version_check.rs line 475 at r1 (raw file):

    let mtime = meta
        .modified()
        .expect("Platforms without file modification times aren't supported");

Are there no circumstances under which this can fail except that one?

[hulthe]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @dlon)

---

mullvad-daemon/src/version_check.rs line 360 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think that this needs to be a checked subtraction, or it might panic.

Right, forgot std durations can't be negative. Made it a saturating_sub instead 👍

---

mullvad-daemon/src/version_check.rs line 413 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Perhaps this function should only run version check if the current cache is actually stale (either None or too old), and otherwise just return the last known version. Then I believe we could get rid of app_version_info from Daemon.

Good thought, I'll look into it

---

mullvad-daemon/src/version_check.rs line 417 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Yes. :)

💯

---

mullvad-daemon/src/version_check.rs line 452 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we handle responding to self.run_version_check_responder in the Ok case instead of in self.update_version_info? It feels too spaghetti.

Agreed

---

mullvad-daemon/src/version_check.rs line 475 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Are there no circumstances under which this can fail except that one?

Yes, which is a bit weird tbh. I would expect that method to not be available at all on those platforms. 🤷

[hulthe]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @dlon)

---

mullvad-daemon/src/version_check.rs line 413 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Good thought, I'll look into it

there. @dlon whaddya think?

[dlon]

Reviewed 1 of 2 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

mullvad-daemon/src/version_check.rs line 413 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

there. @dlon whaddya think?

I think it turned out better. Nice!

---

mullvad-daemon/src/version_check.rs line 445 at r3 (raw file):

                                    version_check = self.do_version_check().fuse();
                                }
                                self.get_version_info_responders.push(done_tx);

Should we also retain only senders that aren't closed here? Maybe this is just me being paranoid.

[hulthe]

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @dlon)

---

mullvad-daemon/src/version_check.rs line 445 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we also retain only senders that aren't closed here? Maybe this is just me being paranoid.

Paranoid or not, it's only a single line so I went ahead and added it :)

[dlon]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: complete! all files reviewed, all discussions resolved