From 56c19b006dca05e1442e1b7ea746eb05a222261e Mon Sep 17 00:00:00 2001 From: Laurens Bezemer | DodoTech Date: Tue, 25 Aug 2026 16:14:09 +0200 Subject: [PATCH] Stop the updater offering the build it saw first, whatever has been published since MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A client two versions behind was offered the older of the two, installed it, restarted, and was told at once that another update was waiting. Two downloads, two installs and two restarts to move one step. The feed was not the problem and could not have been. The nightly release is deleted and remade on every push to main, so it holds exactly one build: a client can only ever be offered the newest thing there is. The wrong version came from this application's own state. ◆ THE PASS STOPPED ASKING, WHICH IS NOT WHAT IT WAS MEANT TO STOP DOING. if (!IsAutomatic || State is UpdateState.Downloading or UpdateState.Ready) Ready means something is downloaded and waiting. The intent behind excluding it is right — there is no point fetching what has already been fetched — and what it actually did was skip the check. So from the first thing any process downloaded, its banner was pinned to that build for the rest of its life, however many newer ones were published afterwards. On a rolling channel that is the normal case rather than a corner. Not re-fetching is still the rule and now lives in FetchAsync, which compares versions rather than refusing to look: the same build is left alone, a newer one replaces what was pending. Which is also why FetchAsync answers with what is *pending* rather than with what the check returned — a client on 1.0 holding 1.1 is told 1.1 by every later check, because 1.1 is still the newest thing on the feed, so "the check found something" and "there is something new" are different questions and only the second one is worth a download. A build already fetched is never discarded, including when a check answers null. Null means the feed's newest is no newer than the build that is *running*, which says nothing about the one on disk — that one is still newer, and withdrawing an offer somebody has already been shown would be the worse answer. ◆ AND A LAST LOOK BEFORE THE PROCESS IS REPLACED, WHICH IS THE HALF THE TIMER CANNOT REACH. The loop asks every six hours. The build that caused this was published minutes before RESTART NOW was pressed, so no interval anybody would accept would have caught it. The press now asks once more. It costs one small request in the ordinary case, because the feed answers with the build already on disk and nothing is fetched. It costs a download only when there genuinely is something newer, which is the case where the alternative was two downloads anyway. Bounded at ten seconds, and the wait rather than the request: VelopackUpdateChannel records that Velopack's check takes no token, so cancelling would be observed only after the call had already returned. Ten seconds bounds a small JSON document against a network that drops packets rather than refusing them; a refused connection fails in milliseconds and never reaches it. Nothing here refuses a restart. An unreachable forge, a look that ran out of time, a fetch of the newer build that failed halfway — all leave the build on disk untouched and still newer than the one running, so the restart goes ahead with it. Hence swallowed rather than reported: Failed is for something a person is waiting on an answer to, and this person is waiting on a restart they are about to get. IsRestarting keeps the banner up through it. Everything the strip draws keys off IsReady and the look moves the state off that, so without it pressing RESTART NOW made the banner vanish with nothing on screen saying why, for as long as a 120 MB package takes. It says what it is doing instead, percentage and all. ◆ THE CHANNEL HAD TO LEARN WHICH BUILD IT FETCHED, AS AGAINST WHICH IT LAST FOUND. Required by both halves above rather than tidied along with them. Matched() resolved an AvailableUpdate only against `found`, which a check overwrites — and both changes make this application check while a build is already downloaded. A check that lands a newer answer and then fails to fetch it, or is given up on halfway, leaves the shell restarting with the build it already has and Matched() throwing at it: the offer real, the package on disk real, and the only thing wrong being that the channel had forgotten which one it was. A slow forge became a refused restart. So `fetched` is recorded after a download returns, tried before `found`, and survives every later check. Settings → General claimed checks start "a couple of minutes after launch", which has been untrue since the first pass moved to launch. It now also says the loop keeps asking while an update is waiting, because that is the behaviour somebody reading that row would want to know about. UpdateFlowTests goes from 21 to 28: the pass asks again and fetches once, the pending build is replaced when the feed moves on, a superseded offer reappears through a dismissal, the press installs what the last look finds, and the three ways the last look can fail all still install what was already on disk. Check 16.7a is the manual half, needing two real releases. Co-Authored-By: Claude Opus 5 --- docs/manual-checks.md | 26 ++ .../Platform/VelopackUpdateChannel.cs | 71 ++++- .../Views/SettingsGeneralPage.axaml | 2 +- .../ViewModels/UpdateViewModel.cs | 288 ++++++++++++++++-- .../UpdateFlowTests.cs | 184 ++++++++++- 5 files changed, 536 insertions(+), 35 deletions(-) diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 6886b5f..97a9c47 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -2706,6 +2706,32 @@ having been broken, and the fallback is to move the offer into the titlebar inst the swap having been blocked, usually by a process still holding a file under `current\`. Being asked to enrol again means the profile directory did not survive, which is 16.1's failure arriving late. +### 16.7a Two releases published while it is running cost one update · **needs two releases** + +The nightly channel is the easier place to do this — every push to `main` publishes one — but it works the +same way on the release channel with two tags. With the client running and the banner already up for one +build, publish a second. Then press **RESTART NOW** on the banner. + +**Pass:** one download and one restart, landing on the **second** build. The banner says +*"A newer build appeared. Fetching it before restarting"* with a percentage while it does, and it stays on +screen throughout rather than vanishing. Starting up afterwards offers nothing, because there is nothing +left to offer. + +**And the timer's half, if you have six hours or a debugger.** Leave the client running past a check +interval with a build already fetched and a newer one published. The banner's version changes on its own, +and LATER having been pressed does not stop it coming back — a different version is a different question. + +**Failure means:** two downloads and two restarts is the regression this check exists for, and it was the +shipped behaviour: the background pass skipped itself entirely once anything was fetched, so a running +client was pinned to whichever build it first saw and installed that, then found the next one at the launch +straight afterwards. On a rolling channel — where the release is deleted and remade on every push, so the +feed holds exactly one build — that was the normal case rather than a corner. + +A banner that disappears when RESTART NOW is pressed is `IsRestarting` not reaching it; everything the strip +draws keys off `IsReady`, and the last look moves the state off that. A restart that does not happen at all +when the forge is unreachable is the last look having become a gate: it is a courtesy, and the build already +on disk is still newer than the one running. + ### 16.8 The first connect after an update is not a cold start Immediately after 16.7, connect to a host. diff --git a/src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs b/src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs index 346be69..aa48656 100644 --- a/src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs +++ b/src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs @@ -204,6 +204,25 @@ internal sealed class VelopackUpdateChannel : IUpdateChannel /// private UpdateInfo? found; + /// + /// The last thing actually fetched, kept separately from the last thing found. + /// + /// + /// + /// ◆ Because the two can disagree, and when they do it is an apply that pays for it. A check + /// overwrites , and the shell now checks while a build is already downloaded and + /// waiting — that is how it replaces a pending build when the feed has moved on, and how it takes one + /// last look on the way to a restart. Either of those can land a newer answer in found and then + /// fail to fetch it, or be given up on halfway; the shell's answer is to restart with the build it + /// already has, and resolving only against found would have thrown at it. + /// + /// + /// Which turned a slow forge into a refused restart: the offer on screen was real, the package on disk + /// was real, and the only thing wrong was that this class had forgotten which one it was. + /// + /// + private UpdateInfo? fetched; + internal VelopackUpdateChannel(UpdateManager manager) => this.manager = manager; /// @@ -321,7 +340,23 @@ internal sealed class VelopackUpdateChannel : IUpdateChannel // Velopack reports progress as an Action and the rest of this codebase speaks IProgress, // so the adaptation happens here rather than leaking the older shape into the view models. - return manager.DownloadUpdatesAsync(Matched(update), progress.Report, cancellationToken); + return DownloadAndRememberAsync(Matched(update), progress, cancellationToken); + } + + /// + /// Recorded after the await and not before, so that a download which threw leaves the previous fetch as + /// the one this channel can still apply. See . + /// + private async Task DownloadAndRememberAsync( + UpdateInfo info, + IProgress progress, + CancellationToken cancellationToken) + { + await manager + .DownloadUpdatesAsync(info, progress.Report, cancellationToken) + .ConfigureAwait(false); + + fetched = info; } /// @@ -335,21 +370,39 @@ internal sealed class VelopackUpdateChannel : IUpdateChannel } /// + /// /// The guard exists because the seam narrows UpdateInfo down to a version string, so nothing in /// the type system stops a caller inventing one. Every legitimate caller passes back exactly what - /// returned; a mismatch is a bug in this application rather than anything a - /// user did, which is why it throws rather than resolving to some safe-looking default. + /// returned, or what a download of that then fetched; a mismatch is a bug in + /// this application rather than anything a user did, which is why it throws rather than resolving to + /// some safe-looking default. + /// + /// + /// What was fetched is tried before what was found, because that is the one an apply names and the one a + /// later check cannot take away. See for how the two come to disagree. + /// /// private UpdateInfo Matched(AvailableUpdate update) { - if (found is not { } info - || !string.Equals(info.TargetFullRelease.Version.ToString(), update.Version, StringComparison.Ordinal)) + if (Names(fetched, update)) { - throw new InvalidOperationException( - $"No update matching {update.Version} has been found by this channel. " - + "Call CheckAsync and pass back what it returned."); + return fetched!; } - return info; + if (Names(found, update)) + { + return found!; + } + + throw new InvalidOperationException( + $"No update matching {update.Version} has been found or fetched by this channel. " + + "Call CheckAsync and pass back what it returned."); } + + private static bool Names(UpdateInfo? info, AvailableUpdate update) => + info is not null + && string.Equals( + info.TargetFullRelease.Version.ToString(), + update.Version, + StringComparison.Ordinal); } diff --git a/src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml b/src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml index d07c6ad..3c77313 100644 --- a/src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml +++ b/src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml @@ -47,7 +47,7 @@ + Text="Every six hours while DodoSSH is running, starting at launch. It keeps checking while the keychain is locked — where builds come from has nothing to do with your vault — and while an update is already waiting, so what is offered is the newest build rather than the first one this run happened to see." /> private static readonly TimeSpan CheckInterval = TimeSpan.FromHours(6); + /// How long the look taken on the way to a restart may take before it is given up on. + /// + /// + /// Ten seconds. It bounds one HTTPS request for a small JSON document, which ordinarily answers in well + /// under one — and the case it exists for is not a slow answer but no answer at all: a captive portal or + /// a firewall that drops packets rather than refusing them, where the underlying handler's own timeout is + /// a hundred seconds. A refused connection fails in milliseconds and never reaches this. + /// + /// + /// Bounded because somebody is waiting on it with a finger on RESTART NOW, which is the whole difference + /// from the timer's own pass. Giving up is not a failure here: the build already on disk is still newer + /// than the one running, so the restart goes ahead with it. + /// + /// + private static readonly TimeSpan LastLookTimeout = TimeSpan.FromSeconds(10); + private readonly IUpdateChannel updates; private readonly ClientSettingsStore settings; private readonly TimeProvider clock; @@ -143,6 +159,18 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab [ObservableProperty] private DateTimeOffset? lastChecked; + /// + /// Whether a restart is under way, including the look it takes before replacing anything. + /// + /// + /// It exists to keep the banner up. Everything the banner shows keys off , and the + /// last look moves the state to Checking and possibly Downloading — so without this, pressing RESTART NOW + /// makes the strip vanish and leaves nothing on screen saying why, for however long a package takes. See + /// , which is what it says instead. + /// + [ObservableProperty] + private bool isRestarting; + internal bool IsSupported => State is not UpdateState.Unsupported; internal bool IsUnsupported => State is UpdateState.Unsupported; @@ -155,12 +183,37 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab internal bool CanCheckNow => IsSupported && State is not (UpdateState.Checking or UpdateState.Downloading); - internal bool IsBannerShowing => IsReady && !IsBannerDismissed; + internal bool IsBannerShowing => (IsReady || IsRestarting) && !IsBannerDismissed; /// What the banner says. - internal string ReadyHeadline => ReadyVersion is { Length: > 0 } version - ? $"DodoSSH {version} is ready to install." - : "An update is ready to install."; + /// + /// Three sentences rather than one, because a restart no longer necessarily starts with a build in hand: + /// it takes one last look first, and may find something newer than what was fetched six hours ago. That + /// look is short and the fetch behind it is not, so both say what they are doing — a strip that read + /// "ready to install" while it downloaded would be describing the wrong build, and a blank one would be + /// describing nothing. + /// + internal string ReadyHeadline + { + get + { + if (IsRestarting && State is UpdateState.Checking) + { + return "Checking for a newer build before restarting…"; + } + + if (IsRestarting && State is UpdateState.Downloading) + { + return string.Create( + CultureInfo.CurrentCulture, + $"A newer build appeared. Fetching it before restarting — {DownloadPercent}%"); + } + + return ReadyVersion is { Length: > 0 } version + ? $"DodoSSH {version} is ready to install." + : "An update is ready to install."; + } + } /// What restarting costs, in the terms this application has already taught. /// @@ -201,10 +254,24 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab OnPropertyChanged(nameof(CanCheckNow)); OnPropertyChanged(nameof(IsBannerShowing)); OnPropertyChanged(nameof(RestartWarning)); + OnPropertyChanged(nameof(ReadyHeadline)); } partial void OnIsBannerDismissedChanged(bool value) => OnPropertyChanged(nameof(IsBannerShowing)); + partial void OnIsRestartingChanged(bool value) + { + OnPropertyChanged(nameof(IsBannerShowing)); + OnPropertyChanged(nameof(ReadyHeadline)); + } + + /// + /// The headline carries the percentage while a restart is fetching, so it has to be re-read as the bar + /// moves. Raised unconditionally rather than only while restarting, because a condition here would be a + /// second place that has to agree with the headline's own about when the percentage is in it. + /// + partial void OnDownloadPercentChanged(int value) => OnPropertyChanged(nameof(ReadyHeadline)); + partial void OnReadyVersionChanged(string? value) => OnPropertyChanged(nameof(ReadyHeadline)); partial void OnLastCheckedChanged(DateTimeOffset? value) => OnPropertyChanged(nameof(LastCheckedSummary)); @@ -278,14 +345,32 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab /// One pass of the loop. /// + /// /// Quiet by construction. A pass that finds nothing writes nothing, and a pass that cannot reach /// the forge writes nothing either — an unreachable release page is a laptop on a train, it is not news, /// and it heals itself in six hours. The same discipline as VaultViewModel.AutoSyncAsync: what /// nobody asked for may only speak when it has something to say. + /// + /// + /// ◆ A pass still runs when one is already fetched, and this used to be the one state that stopped it + /// for good. The guard read Downloading or Ready, on the reasonable-looking argument that + /// there is no point fetching what has already been fetched. What it actually did was stop asking + /// — so the moment anything was downloaded, the banner was pinned to that build for the rest of the + /// process's life, however many newer ones were published afterwards. On the nightly channel, where the + /// release is deleted and remade on every push to main, that is most of the time: the client offered the + /// build it happened to see first, installed it, restarted, and immediately offered the next one. Two + /// updates and two restarts to move one step. + /// + /// + /// Not fetching what is already fetched is still the rule, and is where it now + /// lives — by comparing versions rather than by refusing to look. Downloading is still excluded, because + /// a fetch in flight must not be restarted underneath itself; Checking is excluded for the same reason, + /// and it is a pressed CHECK NOW that a tick would otherwise land on top of. + /// /// internal async Task CheckOnceAsync(CancellationToken cancellationToken) { - if (!IsAutomatic || State is UpdateState.Downloading or UpdateState.Ready) + if (!IsAutomatic || State is UpdateState.Checking or UpdateState.Downloading) { return; } @@ -339,27 +424,91 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab } } - /// Checks, and fetches whatever it finds. - /// The update found, or null. - private async Task FetchAsync(CancellationToken cancellationToken) + /// Checks, and fetches whatever it finds that is not already fetched. + /// Cancellation. + /// + /// A bound on the check alone, or null for none. See . Deliberately not a + /// bound on the whole method: a package legitimately takes minutes, and a caller that wants a short + /// answer wants it about the question, not about the download the answer may lead to. + /// + /// What is waiting to be installed once this has run, or null if nothing is. + /// + /// + /// ◆ It answers with what is pending rather than with what the check returned, and the difference is + /// what makes a second pass safe. A client running 1.0 that has fetched 1.1 is told 1.1 by every + /// later check, because 1.1 is still the newest thing on the feed — so "the check found something" is + /// not the same question as "there is something new". Comparing the two is what lets the loop keep + /// asking without fetching the same package every six hours, and it is what replaces the pending build + /// when the feed has moved on underneath it. + /// + /// + /// A build already fetched is never thrown away, including when a check answers null. Null means the + /// feed's newest is no newer than the build that is running, which says nothing about the one + /// on disk; that one is still newer than what is running, and withdrawing an offer somebody has already + /// been shown would be the worse answer. + /// + /// + private async Task FetchAsync( + CancellationToken cancellationToken, + TimeSpan? checkWithin = null) { State = UpdateState.Checking; Status = string.Empty; - var found = await updates.CheckAsync(cancellationToken).ConfigureAwait(true); + var found = await AskAsync(checkWithin, cancellationToken).ConfigureAwait(true); LastChecked = clock.GetUtcNow(); if (found is null) { - State = UpdateState.Idle; + // Answered with what is still pending rather than with nothing, which is what keeps a pressed + // CHECK NOW honest: "you are on the latest build" is true of the build that is running and false + // of the machine, if there is one downloaded and waiting to be it. + State = ready is null ? UpdateState.Idle : UpdateState.Ready; - return null; + if (ready is { } waiting) + { + Status = Waiting(waiting); + } + + return ready; } + if (ready is { } already + && string.Equals(already.Version, found.Version, StringComparison.Ordinal)) + { + // The ordinary answer for every pass after the first one that found something. Not fetched + // again, which is the rule the old Ready guard was reaching for and got by not asking at all. + State = UpdateState.Ready; + Status = Waiting(already); + + return already; + } + + return await FetchIntoReadyAsync(found, cancellationToken).ConfigureAwait(true); + } + + /// Downloads one update and offers it. + /// + /// Split from for length rather than for reuse — it has one caller — and the + /// division is a real one: above is which build to fetch, below is fetching it. + /// + private async Task FetchIntoReadyAsync( + AvailableUpdate found, + CancellationToken cancellationToken) + { + var superseded = ready; + State = UpdateState.Downloading; DownloadPercent = 0; + // Said while it downloads, and only when this displaces something already fetched. Both places that + // shows are places somebody is looking: a banner whose version changes under them, and a RESTART NOW + // that starts a download instead of a restart. + Status = superseded is { } stale + ? $"DodoSSH {found.Version} appeared while {stale.Version} was waiting. Fetching it instead." + : string.Empty; + // Monotonic, and it has to be. Progress delivers its callbacks by posting them to the captured // synchronisation context rather than invoking them inline, so a report can arrive after the // download has already returned — and an unguarded assignment then puts a stale smaller number @@ -382,13 +531,43 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab DownloadPercent = 100; ready = found; ReadyVersion = found.Version; + + // Undismissed, because this is a different build from the one that was dismissed. LATER was an + // answer about a version, and there is a new question. IsBannerDismissed = false; State = UpdateState.Ready; - Status = $"DodoSSH {found.Version} is downloaded and will run after a restart."; + Status = Waiting(found); return found; } + /// Asks the channel, giving up after if one was given. + /// + /// + /// The wait is bounded rather than the request cancelled, and that is not the same thing: the desktop + /// channel's own remark records that Velopack's check takes no token, so cancelling would be observed + /// only once the call had already returned. Waiting on it with a timeout is what actually gives an + /// answer back in ten seconds. + /// + /// + /// Which leaves the request running, and it is the reason the channel had to learn to remember what it + /// downloaded rather than only what it last found: an orphaned check can land afterwards and + /// move the channel's idea of the newest build, and the restart that gave up on it is about to name the + /// older one. See VelopackUpdateChannel.Matched. + /// + /// + private async Task AskAsync(TimeSpan? within, CancellationToken cancellationToken) + { + var asking = updates.CheckAsync(cancellationToken); + + return within is { } bound + ? await asking.WaitAsync(bound, clock, cancellationToken).ConfigureAwait(true) + : await asking.ConfigureAwait(true); + } + + private static string Waiting(AvailableUpdate update) => + $"DodoSSH {update.Version} is downloaded and will run after a restart."; + /// /// /// Hands the update to whoever was given the job at composition rather than applying it here, and the @@ -406,26 +585,89 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab [RelayCommand] private async Task RestartNowAsync() { - if (ready is not { } update) + if (ready is null) { return; } - // ◆ Reported rather than allowed to escape, and the phone is why. Where applying ends the process - // a throw here is nearly unreachable; where it does not — see ApplyingEndsTheProcess — the whole - // point is that this returns and the application carries on, so an exception has somewhere to go: - // out of a command handler, past the dispatcher, and into the process. What that looks like to - // somebody holding the phone is pressing INSTALL and having the application close, with nothing - // installed and nothing said. Failed and the message is the same answer CheckNowAsync gives, and - // it is the difference between a bug that describes itself and one that presents as a crash. + IsRestarting = true; + try { - await restart(update).ConfigureAwait(true); + await TakeOneLastLookAsync().ConfigureAwait(true); + + if (ready is not { } update) + { + return; + } + + // ◆ Reported rather than allowed to escape, and the phone is why. Where applying ends the + // process a throw here is nearly unreachable; where it does not — see ApplyingEndsTheProcess — + // the whole point is that this returns and the application carries on, so an exception has + // somewhere to go: out of a command handler, past the dispatcher, and into the process. What + // that looks like to somebody holding the phone is pressing INSTALL and having the application + // close, with nothing installed and nothing said. Failed and the message is the same answer + // CheckNowAsync gives, and it is the difference between a bug that describes itself and one + // that presents as a crash. + try + { + await restart(update).ConfigureAwait(true); + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + State = UpdateState.Failed; + Status = exception.Message; + } + } + finally + { + // Never reached on a desktop, where restart() replaces the process from inside the try. It is + // for the phone, where INSTALL is a request the platform may refuse, and for every failure + // path: a flag that keeps the banner up has to come down again. + IsRestarting = false; + } + } + + /// + /// Asks the feed once more, on the way to replacing this process. + /// + /// + /// + /// ◆ Because what was fetched can be hours old, and a restart is the one moment being one build + /// behind is expensive. The loop asks every six hours; a build published between the last pass and + /// this press would otherwise be installed on the next restart, which is to say: update, + /// restart, be told at once that another update is waiting, update, restart again. Two of everything to + /// move one step, which is exactly what a rolling nightly channel produces. + /// + /// + /// It costs one small request in the ordinary case, because the feed answers with the build already on + /// disk and compares rather than downloads. It costs a download only when there + /// genuinely is something newer, which is the case where the alternative was two downloads anyway. + /// + /// + /// Nothing here refuses a restart. A forge that cannot be reached, a look that ran out of time, a + /// fetch of the newer build that failed halfway — all of them leave the build already on disk untouched + /// and still newer than the one running, so the restart goes ahead with it. That is why this swallows + /// rather than reporting: is for something a person is waiting on an + /// answer to, and this person is waiting on a restart they are about to get. + /// + /// + private async Task TakeOneLastLookAsync() + { + try + { + await FetchAsync(lifetime.Token, checkWithin: LastLookTimeout).ConfigureAwait(true); + } + catch (OperationCanceledException) when (lifetime.IsCancellationRequested) + { + // Shutting down, which is the one case that must not go on to restart: whatever is fetched is + // applied at the next ordinary launch anyway, and that is the policy rather than a fallback. + ready = null; } catch (Exception exception) when (exception is not OutOfMemoryException) { - State = UpdateState.Failed; - Status = exception.Message; + // Including the bound running out. Back to the offer that was already on screen. + State = UpdateState.Ready; } } diff --git a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs index cee898d..417a724 100644 --- a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs @@ -390,8 +390,14 @@ public sealed class UpdateFlowTests : IDisposable updates.State.ShouldBe(UpdateState.Ready); } + /// + /// Asks, and does not fetch again. The distinction is the whole of the fix for a client that installed + /// two updates to move one step: the old guard skipped the pass once anything was ready, so the + /// banner was pinned to whatever build the process happened to see first. Not re-fetching is still the + /// rule; it is enforced by comparing versions now rather than by refusing to look. + /// [Fact] - public async Task APassWhileOneIsAlreadyReady_DoesNotAskAgain() + public async Task APassWhileOneIsAlreadyReady_AsksAgainAndDoesNotFetchTwice() { channel.Available = new AvailableUpdate("1.3.0"); @@ -400,7 +406,181 @@ public sealed class UpdateFlowTests : IDisposable await updates.CheckOnceAsync(Token); await updates.CheckOnceAsync(Token); - channel.Checks.ShouldBe(1); + channel.Checks.ShouldBe(2); + channel.Downloads.ShouldBe(1, "the second pass found the build it had already fetched"); + updates.State.ShouldBe(UpdateState.Ready); + updates.ReadyVersion.ShouldBe("1.3.0"); + } + + /// + /// ◆ The regression this whole change is about, in the case the timer can reach. A rolling nightly + /// channel keeps exactly one build, so a client left running does not accumulate a queue of updates — it + /// is offered whichever one the feed holds, and the pending build has to be replaced when that moves on. + /// + [Fact] + public async Task APassWhileOneIsAlreadyReady_ReplacesItWhenTheFeedHasMovedOn() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + updates.ReadyVersion.ShouldBe("1.3.0"); + + channel.Available = new AvailableUpdate("1.4.0"); + + await updates.CheckOnceAsync(Token); + + channel.Downloads.ShouldBe(2); + updates.State.ShouldBe(UpdateState.Ready); + updates.ReadyVersion.ShouldBe("1.4.0"); + } + + /// + /// A build superseded after LATER was pressed brings the banner back, because LATER was an answer about + /// a version and this is a different one. + /// + [Fact] + public async Task ASupersededOffer_IsShownAgainEvenIfTheOldOneWasDismissed() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + updates.DismissBannerCommand.Execute(null); + updates.IsBannerShowing.ShouldBeFalse(); + + channel.Available = new AvailableUpdate("1.4.0"); + await updates.CheckOnceAsync(Token); + + updates.IsBannerShowing.ShouldBeTrue(); + updates.ReadyHeadline.ShouldContain("1.4.0"); + } + + /// + /// ◆ The half the timer cannot reach, and the one a person actually hit: a build published between the + /// last pass and the press. Without this the press installs a build that was already superseded, and the + /// next launch offers the newer one straight away — two downloads, two installs and two restarts to move + /// one step. + /// + [Fact] + public async Task Restarting_TakesOneLastLookAndInstallsWhatItFinds() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + + // Published while the banner was up, which on a rolling channel replaces the release rather than + // adding to it — so the feed now holds only this one. + channel.Available = new AvailableUpdate("1.4.0"); + + await updates.RestartNowCommand.ExecuteAsync(null); + + channel.Restarts.ShouldBe(1); + channel.RestartedWith!.Version.ShouldBe("1.4.0"); + restarts.ShouldBe(1, "one restart, not one per build"); + } + + /// + /// The ordinary case, and what it costs: one question and no download. A press that re-fetched what was + /// already on disk would have made the fix worse than the bug. + /// + [Fact] + public async Task Restarting_AsksOnceMoreAndFetchesNothingWhenNothingIsNewer() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + await updates.RestartNowCommand.ExecuteAsync(null); + + channel.Checks.ShouldBe(2); + channel.Downloads.ShouldBe(1); + channel.RestartedWith!.Version.ShouldBe("1.3.0"); + } + + /// + /// The look is a courtesy and never a gate. Somebody pressing RESTART NOW on a train has a build on disk + /// that is newer than the one running, and a forge that cannot be reached must not stand between them. + /// + [Fact] + public async Task RestartingWhenTheLastLookFails_StillInstallsWhatWasAlreadyFetched() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + + channel.CheckFailure = new HttpRequestException("no such host"); + + await updates.RestartNowCommand.ExecuteAsync(null); + + channel.Restarts.ShouldBe(1); + channel.RestartedWith!.Version.ShouldBe("1.3.0"); + updates.State.ShouldNotBe(UpdateState.Failed, "nothing the user asked for failed"); + } + + /// + /// + /// The same rule one step further in: the newer build was found and then could not be fetched. What is + /// on disk is untouched, so that is what gets installed. + /// + [Fact] + public async Task RestartingWhenTheNewerFetchFails_StillInstallsWhatWasAlreadyFetched() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + + channel.Available = new AvailableUpdate("1.4.0"); + channel.DownloadFailure = new HttpRequestException("the connection was reset"); + + await updates.RestartNowCommand.ExecuteAsync(null); + + channel.Restarts.ShouldBe(1); + channel.RestartedWith!.Version.ShouldBe("1.3.0"); + } + + /// + /// Everything the banner draws keys off IsReady, and the last look moves the state off it — so + /// without IsRestarting the strip vanished the moment RESTART NOW was pressed and left nothing on + /// screen saying why, for as long as a package takes to fetch. + /// + [Fact] + public async Task TheBannerStaysUpWhileARestartFetchesANewerBuild() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + + channel.Available = new AvailableUpdate("1.4.0"); + channel.HoldDownload = new TaskCompletionSource(); + + var pressed = updates.RestartNowCommand.ExecuteAsync(null); + + // The gate is what makes the intermediate state observable; without it the fetch is over before + // anything could look at it. + while (updates.State is not UpdateState.Downloading) + { + await Task.Yield(); + } + + updates.IsBannerShowing.ShouldBeTrue(); + updates.ReadyHeadline.ShouldContain("before restarting"); + + channel.HoldDownload.SetResult(); + await pressed; + + channel.RestartedWith!.Version.ShouldBe("1.4.0"); } ///