From 810bc48d3f11fea6c59db903ea02d5980111d62c Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:14:10 +0200 Subject: [PATCH] Tell the keep-alive wire when the Files session opens and closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HasLiveFileSession answers the phone's foreground-service question — is there a connection here that dying with the process would sever — and a bucket answers no, because HTTP holds nothing open. ActivityChanged now also fires at the end of MarkHostConnected and CloseSessionAsync, where both facts it reads are finally true together. Also makes the bucket pins test actually open a bucket: it never set Remote, so CONNECT dialled the auto-selected host, and its assertions passed only because that host had no pins either. --- .../ViewModels/TransfersViewModel.cs | 40 +++++++- .../ShellFlowTests.cs | 92 +++++++++++++++++++ 2 files changed, 127 insertions(+), 5 deletions(-) diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index c63bfd8..4af48e5 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -620,6 +620,19 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo [ObservableProperty] private string? connectedCipher; + /// + /// Whether there is a live SFTP connection this session would lose by dying — the phone's foreground- + /// service question, not the desktop's. + /// + /// + /// is already the fact that tells a host apart from a bucket, because only + /// a host set it — a bucket is HTTP, per-request, and closes nothing a dying process would have kept + /// open, so it answers false here even while is true. Android reads this to + /// decide whether an idle Files screen with no transfer moving still needs the process kept alive; the + /// desktop has no such question because nothing stops its process for having gone quiet. + /// + internal bool HasLiveFileSession => IsConnected && ConnectedCipher is not null; + /// The accepted host key's algorithm, e.g. ssh-ed25519. See . [ObservableProperty] private string? connectedHostKeyAlgorithm; @@ -745,13 +758,18 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo internal ObservableCollection Transfers { get; } = []; - /// Raised on the UI thread whenever a transfer appears or changes state. + /// + /// Raised on the UI thread whenever a transfer appears or changes state, or a host or bucket connects or + /// disconnects. + /// /// /// For a head that has to tell the operating system what this process is doing — Android's foreground - /// service, which must be up for as long as bytes are moving and down afterwards. An event rather than - /// letting that head watch itself: the collection announces rows arriving and - /// leaving, and the transition that matters most is neither of those but a row going from RUNNING to - /// DONE without moving. + /// service, which must be up for as long as bytes are moving, or a host session sits open, and down + /// afterwards. An event rather than letting that head watch itself: the + /// collection announces rows arriving and leaving, and the transition that matters most is neither of + /// those but a row going from RUNNING to DONE without moving. Connecting and disconnecting are the other + /// two transitions the service cares about — see — and neither touches + /// at all, so they need this same announcement made by hand. /// internal event EventHandler? ActivityChanged; @@ -1075,6 +1093,13 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo // far as the remote's own auth.log is concerned, so a log of ours that omitted it would disagree with // the host's — and anybody comparing the two would be right to believe the host. connected = (ConnectedTo, row.Label, row.EntityId, TimeProvider.System.GetUtcNow()); + + // Raised here rather than from OnIsConnectedChanged, on purpose: IsConnected is set first, above, + // and ConnectedCipher second — a partial method firing off the first assignment would read + // HasLiveFileSession against a ConnectedCipher still holding whatever the previous session left + // there. Only at the end of this method are both facts actually true together. + OnPropertyChanged(nameof(HasLiveFileSession)); + ActivityChanged?.Invoke(this, EventArgs.Empty); } /// @@ -1856,6 +1881,11 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo RemoteTrail.Clear(); SelectedRemoteEntry = null; + // Same ordering reason as the raise at the end of MarkHostConnected: both properties this reads are + // already null above, so the raise belongs after them rather than in OnIsConnectedChanged. This also + // covers OpenBucketAsync, which calls this method first and never itself turns HasLiveFileSession on. + OnPropertyChanged(nameof(HasLiveFileSession)); + ActivityChanged?.Invoke(this, EventArgs.Empty); } /// diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 6f24e3a..9cb60be 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -8114,6 +8114,57 @@ public sealed class ShellFlowTests : IAsyncLifetime shell.Transfers.HasConnectedPins.ShouldBeFalse(); } + /// + /// The phone's foreground-service question, proven at the view model rather than through Android: a + /// connect that opens an SFTP session is exactly the transition SessionKeepAlive needs to hear + /// about even when no transfer ever moves — see 's own + /// remark for why the queue's own raise, in OnTransferChanged, cannot cover a connect that never + /// touches Transfers at all. + /// + [Fact] + public async Task ConnectingATransfersHost_RaisesActivityChangedAndTurnsOnHasLiveFileSession() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts); + shell.Transfers.SelectedHost = shell.Transfers.Hosts[0]; + + var raised = 0; + shell.Transfers.ActivityChanged += (_, _) => raised++; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + + shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); + shell.Transfers.HasLiveFileSession.ShouldBeTrue(); + raised.ShouldBeGreaterThan(0); + } + + /// + /// The other half: a disconnect is as much a transition the service must hear about as a connect is, + /// because it is the moment the connection promised + /// was open stops being true — and the foreground service would otherwise keep the process alive over a + /// session that has already closed. + /// + [Fact] + public async Task DisconnectingTheTransfersScreen_RaisesActivityChangedAndTurnsOffHasLiveFileSession() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts); + shell.Transfers.SelectedHost = shell.Transfers.Hosts[0]; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + shell.Transfers.HasLiveFileSession.ShouldBeTrue(); + + var raised = 0; + shell.Transfers.ActivityChanged += (_, _) => raised++; + + await shell.Transfers.DisconnectCommand.ExecuteAsync(null); + + shell.Transfers.HasLiveFileSession.ShouldBeFalse(); + raised.ShouldBeGreaterThan(0); + } + /// /// A bucket is an IRemoteFileStore with no HostSecret underneath it, so there is no /// PinnedPaths to read at all — see 's own remark. @@ -8137,15 +8188,56 @@ public sealed class ShellFlowTests : IAsyncLifetime vault.BucketEditorRegion = "eu-west-1"; await vault.SaveObjectStoreCommand.ExecuteAsync(null); + // Remote is what ConnectAsync branches on, and Attach's RefreshHosts has already auto-selected the + // host ReadyToConnectAsync left in the picker — without this line the command below dialled that + // host, and every assertion here passed only because that host happens to have no pins either. The + // ConnectedTo check is the proof the bucket path was actually taken. + shell.Transfers.Remote = RemoteKind.Bucket; shell.Transfers.SelectedBucket = shell.Transfers.Buckets[0]; await shell.Transfers.ConnectCommand.ExecuteAsync(null); + shell.Transfers.ConnectedTo.ShouldBe("s3://backups"); shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); shell.Transfers.ConnectedPinnedPaths.ShouldBeEmpty(); shell.Transfers.HasConnectedPins.ShouldBeFalse(); } + /// + /// A bucket is HTTP, per-request, with nothing open that a dying process would lose — see + /// 's own remark. IsConnected alone would have + /// answered this wrongly, which is exactly why the flag reads ConnectedCipher as well: nothing + /// underneath a bucket ever sets it. + /// + [Fact] + public async Task ConnectingABucket_LeavesHasLiveFileSessionOff() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts, buckets: new FakeObjectStoreFactory()); + + vault.NewObjectStoreCommand.Execute(null); + vault.BucketEditorLabel = "Backups"; + vault.BucketEditorBucket = "backups"; + vault.BucketEditorAccessKeyId = "AKIAEXAMPLE"; + vault.BucketEditorSecretAccessKey = "a-secret-access-key"; + vault.BucketEditorRegion = "eu-west-1"; + await vault.SaveObjectStoreCommand.ExecuteAsync(null); + + // ReadyToConnectAsync already left a host in the picker, and Attach's own RefreshHosts auto-selects + // it — so without this the CONNECT command below would dial that host rather than open the bucket, + // and a host with no pins would make ConnectedPinnedPathsEmpty-style assertions pass for the wrong + // reason. Remote is what ConnectAsync actually branches on. + shell.Transfers.Remote = RemoteKind.Bucket; + shell.Transfers.SelectedBucket = shell.Transfers.Buckets[0]; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + + shell.Transfers.ConnectedTo.ShouldBe("s3://backups", "proof this opened the bucket rather than the host"); + shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); + shell.Transfers.HasLiveFileSession.ShouldBeFalse(); + } + /// A bucket that opens and lists as empty, so a bucket connect can be proven with no network. private sealed class FakeObjectStoreFactory : IObjectStoreFactory {