DodoTech-Public/DodoSSH
Public Access
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Tell the keep-alive wire when the Files session opens and closes

Browse Source 
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.
This commit is contained in:
jaap-jan
2026-08-09 10:14:10 +02:00
parent dbf6ce1bcf
commit 810bc48d3f
2 changed files with 127 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs
+35 -5
View File
@@ -620,6 +620,19 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
[ObservableProperty]
private string? connectedCipher;
/// <summary>
/// Whether there is a live SFTP connection this session would lose by dying — the phone's foreground-
/// service question, not the desktop's.
/// </summary>
/// <remarks>
/// <see cref="ConnectedCipher"/> 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 <see cref="IsConnected"/> 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.
/// </remarks>
internal bool HasLiveFileSession => IsConnected && ConnectedCipher is not null;
/// <summary>The accepted host key's algorithm, e.g. <c>ssh-ed25519</c>. See <see cref="ConnectedCipher"/>.</summary>
[ObservableProperty]
private string? connectedHostKeyAlgorithm;
@@ -745,13 +758,18 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
internal ObservableCollection<TransferRowViewModel> Transfers { get; } = [];
/// <summary>Raised on the UI thread whenever a transfer appears or changes state.</summary>
/// <summary>
/// Raised on the UI thread whenever a transfer appears or changes state, or a host or bucket connects or
/// disconnects.
/// </summary>
/// <remarks>
/// 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 <see cref="Transfers"/> 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 <see cref="Transfers"/> 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 <see cref="HasLiveFileSession"/> — and neither touches
/// <see cref="Transfers"/> at all, so they need this same announcement made by hand.
/// </remarks>
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);
}
/// <summary>
@@ -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);
}
/// <remarks>
tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs
+92
View File
@@ -8114,6 +8114,57 @@ public sealed class ShellFlowTests : IAsyncLifetime
shell.Transfers.HasConnectedPins.ShouldBeFalse();
}
/// <remarks>
/// 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 <c>SessionKeepAlive</c> needs to hear
/// about even when no transfer ever moves — see <see cref="TransfersViewModel.ActivityChanged"/>'s own
/// remark for why the queue's own raise, in <c>OnTransferChanged</c>, cannot cover a connect that never
/// touches <c>Transfers</c> at all.
/// </remarks>
[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);
}
/// <remarks>
/// 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 <see cref="TransfersViewModel.HasLiveFileSession"/> promised
/// was open stops being true — and the foreground service would otherwise keep the process alive over a
/// session that has already closed.
/// </remarks>
[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);
}
/// <remarks>
/// A bucket is an <c>IRemoteFileStore</c> with no <c>HostSecret</c> underneath it, so there is no
/// <c>PinnedPaths</c> to read at all — see <see cref="TransfersViewModel.OpenBucketAsync"/>'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();
}
/// <remarks>
/// A bucket is HTTP, per-request, with nothing open that a dying process would lose — see
/// <see cref="TransfersViewModel.HasLiveFileSession"/>'s own remark. <c>IsConnected</c> alone would have
/// answered this wrongly, which is exactly why the flag reads <c>ConnectedCipher</c> as well: nothing
/// underneath a bucket ever sets it.
/// </remarks>
[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();
}
/// <summary>A bucket that opens and lists as empty, so a bucket connect can be proven with no network.</summary>
private sealed class FakeObjectStoreFactory : IObjectStoreFactory
{