diff --git a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs index 3cebb3f..655c5df 100644 --- a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs @@ -332,6 +332,11 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp State = ShellState.Unlocked; await Vault.LoadAsync(cancellationToken).ConfigureAwait(true); + + // After the first load, so the list is on screen before anything talks to a server. The + // loop is started from the UI thread deliberately: every pass resumes here, which is what + // keeps the observable collections single-threaded. + Vault.StartAutoSync(); }).ConfigureAwait(true); } diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index c3f5591..35aee2c 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -2,6 +2,7 @@ using System.Collections.ObjectModel; using System.Globalization; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; +using DodoSSH.Client.Api; using DodoSSH.Client.Domain; using DodoSSH.Client.Session; using DodoSSH.Client.Ssh; @@ -58,12 +59,12 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// This is the whole justification for resolving a conflict automatically. If these were not shown, /// the merge would be last-writer-wins with a longer explanation. /// + // The lambda parameter is 'entry' rather than the obvious 'field': C# 14 made that a contextual + // keyword inside a property accessor, and this whole expression is one. internal string Detail => notice.Fields.Count == 0 ? string.Empty : string.Join( Environment.NewLine, - // Not named 'field': C# 14 made that a contextual keyword inside a property accessor, and - // this whole expression is one. notice.Fields.Select(entry => entry.DiscardedWasRemoval ? $"{entry.Field}: a removal was overridden; '{entry.Kept}' was kept" : $"{entry.Field}: kept '{entry.Kept}', discarded '{entry.Discarded}'")); @@ -77,8 +78,14 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// /// /// The list is the local mirror with unpushed changes laid over it, so an edit appears immediately and a -/// delete disappears immediately whether or not the network is there. Syncing is a separate, explicit -/// action; nothing here blocks on a server. +/// delete disappears immediately whether or not the network is there. Nothing here waits on a server to +/// show a change. +/// +/// +/// Syncing then happens on its own: once when the vault opens, straight after any local change, and on a +/// timer while it stays open. The Sync button remains, because a person who has just been handed a +/// credential wants to know now rather than within the minute — but nothing depends on it being pressed. +/// A background pass is deliberately quieter than the button: see . /// /// /// Credentials are not in the vault yet. SyncEntityType.Credential exists in the contract @@ -93,6 +100,19 @@ internal sealed partial class VaultViewModel( IKnownHostStore knownHosts, Func connection) : ObservableObject, IAsyncDisposable { + /// + /// A minute. The pull is a delta keyed on a cursor, so an idle pass is one small request and costs the + /// server almost nothing; the number that matters is how stale a teammate's change may look, and a + /// minute is short enough not to be noticed. Anything much shorter would be polling for its own sake, + /// and a change made on this machine does not wait for the timer anyway — saving pushes immediately. + /// + private static readonly TimeSpan AutoSyncInterval = TimeSpan.FromMinutes(1); + + /// Serialises every synchronisation pass, whether a button pressed it or a timer did. + private readonly SemaphoreSlim syncGate = new(1, 1); + + private CancellationTokenSource? autoSync; + private Task? autoSyncLoop; private bool disposed; /// The hosts to show, unpushed local state included. @@ -165,8 +185,26 @@ internal sealed partial class VaultViewModel( internal bool HasConflicts => Conflicts.Count > 0; - /// Reads the vault into the list. + /// Reads the vault into the list and says what is in it. internal async Task LoadAsync(CancellationToken cancellationToken) + { + await ReloadAsync(cancellationToken).ConfigureAwait(true); + + Status = Hosts.Count == 0 + ? "No hosts yet. Add one." + : $"{Hosts.Count} host(s) in {VaultName}."; + } + + /// + /// Reads the vault into the list, silently. + /// + /// + /// Separate from because every caller except the first load has something + /// better to say afterwards — a save, a deletion, or a sync report — and a background pass has nothing + /// to say at all. Rebuilding the list used to repaint the status line unconditionally, which made + /// "the background pass is quiet" false on the one path that mattered. + /// + private async Task ReloadAsync(CancellationToken cancellationToken) { var listing = await session.Hosts .ListAsync(session.ActiveVaultId, cancellationToken) @@ -189,10 +227,6 @@ internal sealed partial class VaultViewModel( PendingChanges = await session.PendingChangeCountAsync(cancellationToken).ConfigureAwait(true); await LoadConflictsAsync(cancellationToken).ConfigureAwait(true); - - Status = Hosts.Count == 0 - ? "No hosts yet. Add one." - : $"{Hosts.Count} host(s) in {VaultName}."; } /// Runs a synchronisation pass, if there is a server to talk to. @@ -209,14 +243,133 @@ internal sealed partial class VaultViewModel( "Synchronising…", async () => { - var report = await session.SyncAsync(server.Sync, cancellationToken).ConfigureAwait(true); + var report = await SyncOnceAsync(server.Sync, cancellationToken).ConfigureAwait(true); - await LoadAsync(cancellationToken).ConfigureAwait(true); - - Status = Describe(report); + // Null means a background pass held the gate. Saying so beats reporting a sync that this + // press did not perform. + Status = report is null + ? "A synchronisation is already running." + : Describe(report); }).ConfigureAwait(true); } + /// + /// Starts syncing in the background until the vault is disposed. + /// + /// + /// Explicit rather than started from the constructor, so that a test can drive + /// a pass at a time instead of racing a timer. + /// + internal void StartAutoSync() + { + if (autoSync is not null) + { + return; + } + + autoSync = new CancellationTokenSource(); + autoSyncLoop = RunAutoSyncLoopAsync(autoSync.Token); + } + + /// + /// One background synchronisation pass, which stays out of the way. + /// + /// + /// + /// Deliberately not routed through . That would raise the busy flag every + /// interval — disabling Connect and Save for the duration — and repaint the status line with + /// "Synchronising…" while the user was reading something else. A background pass that makes the + /// application feel intermittently broken is worse than a Sync button. + /// + /// + /// So it is silent unless it has something to say: the status line changes only when the pass actually + /// moved an item or produced something needing attention. It also yields to the user — a pass is + /// skipped outright while a command is running, rather than queueing behind it. + /// + /// + internal async Task AutoSyncAsync(CancellationToken cancellationToken) + { + if (IsBusy || connection() is not { } server) + { + return; + } + + try + { + var report = await SyncOnceAsync(server.Sync, cancellationToken).ConfigureAwait(true); + + if (report is not null && (report.Pulled > 0 || report.Pushed > 0 || report.NeedsAttention)) + { + Status = Describe(report); + } + } + catch (OperationCanceledException) + { + // Locking, or closing. + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + // Swallowed on purpose, and this is the one place in the view model where that is right: a + // laptop that has been closed all afternoon would otherwise replace whatever the user was + // reading with a socket error once a minute. The failure is not hidden — the account bar + // already shows when there is no connection, and pressing Sync reports the real reason. + } + } + + /// + /// The gate is shared with the manual command, so a press and a tick can never overlap. Taken with a + /// zero timeout rather than awaited: a pass that arrives while another is running has nothing to add by + /// waiting for it, and queueing them would turn a slow server into a backlog of identical work. + /// + private async Task SyncOnceAsync(ISyncApi api, CancellationToken cancellationToken) + { + if (!await syncGate.WaitAsync(0, cancellationToken).ConfigureAwait(true)) + { + return null; + } + + try + { + var report = await session.SyncAsync(api, cancellationToken).ConfigureAwait(true); + + await ReloadAsync(cancellationToken).ConfigureAwait(true); + + return report; + } + finally + { + syncGate.Release(); + } + } + + /// + /// ConfigureAwait(true) throughout, and that is load-bearing rather than habit: the loop is + /// started from the UI thread, so every continuation returns to it and the observable collections + /// rebuilds are still only ever touched from one thread. A + /// ConfigureAwait(false) here would mutate them from a timer thread, which Avalonia will + /// eventually notice in a way that looks like a rendering bug. + /// + private async Task RunAutoSyncLoopAsync(CancellationToken cancellationToken) + { + using var timer = new PeriodicTimer(AutoSyncInterval); + + try + { + // A pass on open, before the first tick. A vault edited on another machine should be current by + // the time the user has finished reading the list, not a minute afterwards. + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + + while (await timer.WaitForNextTickAsync(cancellationToken).ConfigureAwait(true)) + { + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + } + catch (OperationCanceledException) + { + // Locking, or closing. + } + } + /// Starts a new host. [RelayCommand] private void NewHost() @@ -299,13 +452,19 @@ internal sealed partial class VaultViewModel( } IsEditing = false; - await LoadAsync(cancellationToken).ConfigureAwait(true); + await ReloadAsync(cancellationToken).ConfigureAwait(true); SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == editingEntityId); editingEntityId = null; - Status = $"Saved '{host.Label}'. It will sync when you are online."; + Status = connection() is null + ? $"Saved '{host.Label}'. It will sync when you are online." + : $"Saved '{host.Label}'."; }).ConfigureAwait(true); + + // Pushed now rather than at the next tick. A change the user just made is the one they are most + // likely to be about to look for on another machine. + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } /// Queues a tombstone for the selected host. @@ -325,9 +484,13 @@ internal sealed partial class VaultViewModel( .DeleteAsync(session.ActiveVaultId, row.EntityId, cancellationToken) .ConfigureAwait(true); - await LoadAsync(cancellationToken).ConfigureAwait(true); + await ReloadAsync(cancellationToken).ConfigureAwait(true); Status = $"Deleted '{row.Label}'."; }).ConfigureAwait(true); + + // As with saving: a tombstone is worth pushing straight away, so the item does not reappear on + // another machine that syncs before the next tick. + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } /// Opens a terminal on the selected host. @@ -406,6 +569,23 @@ internal sealed partial class VaultViewModel( } disposed = true; + + // The loop is stopped and awaited before the session goes, not merely signalled. A pass in flight + // holds the vault keys and the cache; letting it run on into a disposed session is how locking + // turns into an ObjectDisposedException on a background thread that nobody sees. + if (autoSync is not null) + { + await autoSync.CancelAsync().ConfigureAwait(false); + } + + if (autoSyncLoop is not null) + { + await autoSyncLoop.ConfigureAwait(false); + } + + autoSync?.Dispose(); + syncGate.Dispose(); + await session.DisposeAsync().ConfigureAwait(false); } diff --git a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs index 395289f..7ddb598 100644 --- a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs +++ b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs @@ -40,6 +40,16 @@ internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKe /// When set, the next sign-in throws — how an unreachable server is exercised. internal Exception? SignInFailure { get; set; } + /// + /// When set, every synchronisation throws. + /// + /// + /// A server that answers but fails, as distinct from no server at all. The two are handled quite + /// differently by a background pass: one is expected and silent, the other has to not overwrite + /// whatever the user was reading. + /// + internal Exception? SyncFailure { get; set; } + /// public Uri ServerUrl { get; } = new("https://dodossh.example"); @@ -122,6 +132,11 @@ internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKe SyncPullRequest request, CancellationToken cancellationToken) { + if (SyncFailure is { } failure) + { + return Task.FromException(failure); + } + var after = request.Cursor is null ? 0 : long.Parse(request.Cursor.AsSpan("app-v1:".Length), provider: null); diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 5324310..64f2bcf 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -266,8 +266,14 @@ public sealed class ShellFlowTests : IAsyncLifetime offline.Vault.ShouldNotBeNull(); } + /// + /// This used to assert the opposite of its last two lines — that a save queued the change and pushed + /// nothing until Sync was pressed. Saving now pushes, so the assertion had to move rather than be + /// deleted: the local-first guarantee it was really protecting is that the list updates without a + /// server, and that is still covered by the offline test below. + /// [Fact] - public async Task AddingAHost_ShowsItImmediatelyAndQueuesIt() + public async Task AddingAHost_ShowsItImmediatelyAndPushesIt() { await UnlockedAsync(); var vault = shell.Vault!; @@ -287,11 +293,83 @@ public sealed class ShellFlowTests : IAsyncLifetime var row = vault.Hosts.ShouldHaveSingleItem(); row.Label.ShouldBe("prod-db"); row.Address.ShouldBe("deploy@db.internal:2222"); - row.HasUnsyncedChanges.ShouldBeTrue(); - row.Badge.ShouldBe("not synced"); - vault.PendingChanges.ShouldBe(1); - server.LiveRowCount.ShouldBe(0, "nothing should have been pushed yet"); + row.HasUnsyncedChanges.ShouldBeFalse("saving pushes, so nothing should still be pending"); + row.Badge.ShouldBeEmpty(); + + vault.PendingChanges.ShouldBe(0); + server.LiveRowCount.ShouldBe(1, "a save should reach the server without pressing Sync"); + } + + [Fact] + public async Task AnAutomaticPass_SaysNothingWhenThereIsNothingToDo() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + vault.Status = "Reading something the user cares about."; + + await vault.AutoSyncAsync(Token); + + vault.Status.ShouldBe( + "Reading something the user cares about.", + "a background pass with no changes must not repaint the status line"); + + vault.IsBusy.ShouldBeFalse("a background pass must never raise the busy flag"); + } + + /// + /// The queue has to be arranged by failing the automatic push first. Written the obvious way — add a + /// host, then call the pass — this test proved nothing at all: saving pushes, so there was no pending + /// change left and the count was unchanged whether the guard existed or not. It passed with the guard + /// deleted, which is the only reason it was noticed. + /// + [Fact] + public async Task AnAutomaticPass_YieldsWhileACommandIsRunning() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + server.SyncFailure = new HttpRequestException("The server is having a bad day."); + await AddHostAsync(vault, "prod-db"); + server.SyncFailure = null; + + vault.PendingChanges.ShouldBe(1, "there must be something to push for this to mean anything"); + + var pushesBefore = server.PushCount; + + // Standing in for a command in flight. A pass that pushed here would be competing with whatever + // the user is doing for the same session and the same cache. + vault.IsBusy = true; + + await vault.AutoSyncAsync(Token); + + server.PushCount.ShouldBe(pushesBefore, "the pass should have been skipped, not queued"); + } + + /// + /// The behaviour a background loop lives or dies by. A pass runs every minute; one that reported a + /// transient server error would replace whatever the user was reading, once a minute, indefinitely. + /// + [Fact] + public async Task AnAutomaticPassThatFails_LeavesTheStatusAlone() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddHostAsync(vault, "prod-db"); + + vault.Status = "Reading something the user cares about."; + server.SyncFailure = new HttpRequestException("The server is having a bad day."); + + await vault.AutoSyncAsync(Token); + + vault.Status.ShouldBe("Reading something the user cares about."); + vault.IsBusy.ShouldBeFalse(); + + // And pressing Sync still reports the real reason, so the failure is quiet rather than hidden. + await vault.SyncCommand.ExecuteAsync(null); + vault.Status.ShouldContain("bad day"); } [Fact] @@ -311,13 +389,26 @@ public sealed class ShellFlowTests : IAsyncLifetime vault.Status.ShouldContain("needs a name"); } + /// + /// This is what earns the right to let a background pass fail silently. The automatic push after a save + /// is best-effort; the outbox is the durable part. If a failed pass dropped the change, "quiet" would + /// mean "lost". + /// [Fact] - public async Task SyncingSendsTheQueueAndClearsIt() + public async Task AQueueLeftByAFailedPass_IsStillSentByTheNextSync() { await UnlockedAsync(); var vault = shell.Vault!; + server.SyncFailure = new HttpRequestException("The server is having a bad day."); + await AddHostAsync(vault, "prod-db"); + + server.LiveRowCount.ShouldBe(0, "the automatic push should have failed"); + vault.PendingChanges.ShouldBe(1, "and the change should still be queued"); + vault.Hosts.ShouldHaveSingleItem().HasUnsyncedChanges.ShouldBeTrue(); + + server.SyncFailure = null; await vault.SyncCommand.ExecuteAsync(null); server.LiveRowCount.ShouldBe(1); @@ -349,22 +440,22 @@ public sealed class ShellFlowTests : IAsyncLifetime } [Fact] - public async Task DeletingAHostRemovesItLocallyBeforeTheServerAgrees() + public async Task DeletingAHost_RemovesItLocallyAndPushesTheTombstone() { await UnlockedAsync(); var vault = shell.Vault!; await AddHostAsync(vault, "prod-db"); - await vault.SyncCommand.ExecuteAsync(null); vault.SelectedHost = vault.Hosts[0]; await vault.DeleteHostCommand.ExecuteAsync(null); vault.Hosts.ShouldBeEmpty(); - server.LiveRowCount.ShouldBe(1, "the tombstone has not been pushed yet"); - await vault.SyncCommand.ExecuteAsync(null); + // Pushed without a second action. A tombstone that sat in the outbox would let the item come back + // on a machine that synced in the meantime. server.LiveRowCount.ShouldBe(0); + vault.PendingChanges.ShouldBe(0); } [Fact]