diff --git a/docs/manual-checks.md b/docs/manual-checks.md index ea206ea..79da807 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -2423,7 +2423,9 @@ script warns rather than failing when that is legitimate, which is the first rel ### 16.7 The update arrives, and the restart lands in it · **the whole point of the work** With v0.1.0 installed and running, a vault unlocked, a host change made, and **a terminal open**, publish -v0.1.1 (`-Upload`). Then press CHECK NOW on Settings → General rather than waiting six hours. +v0.1.1 (`-Upload`). Then press CHECK NOW on Settings → General rather than waiting six hours. Closing and +reopening the application does the same thing without the button: the first pass of the loop runs at launch, +so a client started after a release finds it without anybody asking. **Pass:** the progress bar moves, the banner appears above the status bar, and — the part to actually watch — the terminal **reflows cleanly rather than being sliced**, with the remote seeing the smaller row count. diff --git a/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs index 880924f..6df44ae 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs @@ -74,16 +74,6 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab /// private static readonly TimeSpan CheckInterval = TimeSpan.FromHours(6); - /// How long to wait before the first pass. - /// - /// A delay, where VaultViewModel's sync loop runs a pass immediately. The difference is what the - /// user is waiting for: a vault edited on another machine should be current by the time they have - /// finished reading the list, whereas nothing anybody does in their first two minutes depends on an - /// update. Launch is already contending for the network and the CPU with a schema migration, a resumed - /// sign-in and a first sync, at the one moment somebody is watching the window. - /// - private static readonly TimeSpan FirstCheckDelay = TimeSpan.FromMinutes(2); - private readonly IUpdateChannel updates; private readonly ClientSettingsStore settings; private readonly TimeProvider clock; @@ -242,16 +232,40 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab loop = RunCheckLoopAsync(lifetime.Token); } + /// + /// + /// The first pass runs at launch, with no delay in front of it. It used to wait two minutes, on + /// the argument that nothing anybody does in their first two minutes depends on an update and launch is + /// already contending for the network with a schema migration, a resumed sign-in and a first sync. What + /// that argument leaves out is the run that is over before the two minutes are: a client opened to reach + /// one host and closed again never checks at all, and a machine used that way is exactly the one ADR + /// 0011 warns about — quietly a year behind, with the mechanism to fix it switched on and never reached. + /// Every start now asks. + /// + /// + /// The yield is what keeps that off the launch path. is called from + /// MainWindowViewModel.StartAsync before the migration, so running the pass inline would put + /// whatever the channel does before its own first await — Velopack reads the install layout from disk — + /// between the user and their window. Yielding hands the rest of the launch back and lets the check run + /// in a later turn, which is the same moment in every sense that matters and none of the cost. + /// + /// private async Task RunCheckLoopAsync(CancellationToken cancellationToken) { try { - await Task.Delay(FirstCheckDelay, clock, cancellationToken).ConfigureAwait(true); + await Task.Yield(); using var timer = new PeriodicTimer(CheckInterval, clock); do { + // Task.Yield takes no token, unlike the delay it replaced, so a shutdown that lands while + // the loop is waiting to be handed back the thread has to be observed here rather than + // only at the next tick. Otherwise an application closed during launch spends its last + // moment asking a release channel about a build it is not going to run. + cancellationToken.ThrowIfCancellationRequested(); + await CheckOnceAsync(cancellationToken).ConfigureAwait(true); } while (await timer.WaitForNextTickAsync(cancellationToken).ConfigureAwait(true)); diff --git a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs index d47bb73..cee898d 100644 --- a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs @@ -403,6 +403,46 @@ public sealed class UpdateFlowTests : IDisposable channel.Checks.ShouldBe(1); } + /// + /// + /// The loop rather than CheckOnceAsync, which is the one thing the rest of this file avoids + /// driving — and here it is the whole point, because the claim is about when the first pass happens + /// rather than about what it does. The first pass used to wait two minutes, which meant a client opened + /// to reach one host and closed again never asked at all. + /// + /// + /// It waits on the pass and not on a clock, so there is nothing here to be flaky about: a regression + /// that puts a delay back in front of the loop does not fail on a margin, it spins until the suite's own + /// cancellation ends it. + /// + /// + [Fact] + public async Task TheFirstPassRunsAtStart_RatherThanOnADelay() + { + channel.Available = new AvailableUpdate("1.3.0"); + + var updates = Build(); + await using var _ = updates.ConfigureAwait(false); + + updates.Start(); + + while (updates.State is not UpdateState.Ready) + { + Token.ThrowIfCancellationRequested(); + + await Task.Yield(); + } + + channel.Checks.ShouldBe(1); + updates.ReadyVersion.ShouldBe("1.3.0"); + } + + /// + /// Started and disposed with nothing in between, which since the first pass stopped waiting two minutes + /// is a race rather than a formality: the loop may be anywhere between its yield and a finished check + /// when the cancellation lands. What is asserted is what matters either way — that disposing returns, + /// rather than waiting on a pass that will never be allowed to finish. + /// [Fact] public async Task DisposingStopsTheLoop() {