Sync the vault automatically instead of only on a button press

Three triggers: once when the vault opens, straight after any local change,
and every minute while it stays open. The Sync button stays, because someone
just handed a credential wants to know now rather than within the minute, but
nothing depends on it being pressed any more.

A background pass is deliberately not the button's code path. Routing it
through RunAsync would raise the busy flag every minute — disabling Connect
and Save for the duration — and repaint the status line over whatever the user
was reading. So it is quiet: the status changes only when a pass actually
moved an item or produced something needing attention, and a pass is skipped
outright while a command is running rather than queueing behind it. Both
guards are covered; removing either fails a test.

A shared semaphore serialises every pass, taken with a zero timeout rather
than awaited — a pass arriving while another runs has nothing to add by
waiting, and queueing them would turn a slow server into a backlog of
identical work.

Failures are swallowed, which is right in exactly this one place: a laptop
closed all afternoon would otherwise replace the status line with a socket
error once a minute. It is quiet rather than hidden — the account bar already
shows when there is no connection, and pressing Sync reports the real reason.
What earns that is the outbox: a test proves a change left queued by a failed
pass is still sent by the next sync, so quiet never means lost.

Two existing tests asserted the opposite behaviour — that a save queued and
pushed nothing until Sync was pressed — and were rewritten rather than
deleted; the local-first guarantee they were really protecting is that the
list updates with no server, which the offline test still covers.

Two things the tests caught in my own work. ReloadAsync had to be split out
of LoadAsync because rebuilding the list repainted the status line
unconditionally, which made "the background pass is quiet" false on the one
path that mattered. And the yields-to-a-command test was vacuous as first
written: saving pushes, so there was no pending change left and the assertion
held with the guard deleted. It now fails the automatic push first to arrange
a real queue.

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);
     }
 

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.
     /// </remarks>
+    // 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)
 /// <remarks>
 /// <para>
 /// 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.
+/// </para>
+/// <para>
+/// 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 <see cref="AutoSyncAsync" />.
 /// </para>
 /// <para>
 /// <b>Credentials are not in the vault yet.</b> <c>SyncEntityType.Credential</c> exists in the contract
@@ -93,6 +100,19 @@ internal sealed partial class VaultViewModel(
     IKnownHostStore knownHosts,
     Func<IVaultServer?> connection) : ObservableObject, IAsyncDisposable
 {
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    private static readonly TimeSpan AutoSyncInterval = TimeSpan.FromMinutes(1);
+
+    /// <summary>Serialises every synchronisation pass, whether a button pressed it or a timer did.</summary>
+    private readonly SemaphoreSlim syncGate = new(1, 1);
+
+    private CancellationTokenSource? autoSync;
+    private Task? autoSyncLoop;
     private bool disposed;
 
     /// <summary>The hosts to show, unpushed local state included.</summary>
@@ -165,8 +185,26 @@ internal sealed partial class VaultViewModel(
 
     internal bool HasConflicts => Conflicts.Count > 0;
 
-    /// <summary>Reads the vault into the list.</summary>
+    /// <summary>Reads the vault into the list and says what is in it.</summary>
     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}.";
+    }
+
+    /// <summary>
+    /// Reads the vault into the list, silently.
+    /// </summary>
+    /// <remarks>
+    /// Separate from <see cref="LoadAsync" /> 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.
+    /// </remarks>
+    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}.";
     }
 
     /// <summary>Runs a synchronisation pass, if there is a server to talk to.</summary>
@@ -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);
     }
 
+    /// <summary>
+    /// Starts syncing in the background until the vault is disposed.
+    /// </summary>
+    /// <remarks>
+    /// Explicit rather than started from the constructor, so that a test can drive
+    /// <see cref="AutoSyncAsync" /> a pass at a time instead of racing a timer.
+    /// </remarks>
+    internal void StartAutoSync()
+    {
+        if (autoSync is not null)
+        {
+            return;
+        }
+
+        autoSync = new CancellationTokenSource();
+        autoSyncLoop = RunAutoSyncLoopAsync(autoSync.Token);
+    }
+
+    /// <summary>
+    /// One background synchronisation pass, which stays out of the way.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Deliberately not routed through <see cref="RunAsync" />. 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.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// </remarks>
+    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.
+        }
+    }
+
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    private async Task<SyncReport?> 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();
+        }
+    }
+
+    /// <remarks>
+    /// <c>ConfigureAwait(true)</c> 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
+    /// <see cref="LoadAsync" /> rebuilds are still only ever touched from one thread. A
+    /// <c>ConfigureAwait(false)</c> here would mutate them from a timer thread, which Avalonia will
+    /// eventually notice in a way that looks like a rendering bug.
+    /// </remarks>
+    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.
+        }
+    }
+
     /// <summary>Starts a new host.</summary>
     [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);
     }
 
     /// <summary>Queues a tombstone for the selected host.</summary>
@@ -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);
     }
 
     /// <summary>Opens a terminal on the selected host.</summary>
@@ -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);
     }
 

tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs

@@ -40,6 +40,16 @@ internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKe
     /// <summary>When set, the next sign-in throws — how an unreachable server is exercised.</summary>
     internal Exception? SignInFailure { get; set; }
 
+    /// <summary>
+    /// When set, every synchronisation throws.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    internal Exception? SyncFailure { get; set; }
+
     /// <inheritdoc />
     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<SyncPullResponse>(failure);
+        }
+
         var after = request.Cursor is null
             ? 0
             : long.Parse(request.Cursor.AsSpan("app-v1:".Length), provider: null);

tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs

@@ -266,8 +266,14 @@ public sealed class ShellFlowTests : IAsyncLifetime
         offline.Vault.ShouldNotBeNull();
     }
 
+    /// <remarks>
+    /// 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.
+    /// </remarks>
     [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");
+    }
+
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    [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");
+    }
+
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    [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");
     }
 
+    /// <remarks>
+    /// 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".
+    /// </remarks>
     [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]