Merge pull request 'Tests: wait for a dropped session's forwards to stop, not just for the tab to go dead' (#23) from claude/elastic-mccarthy-6b4ae7 into main

Reviewed-on: #23

src/DodoSSH.Client.App/App.axaml

@@ -1223,14 +1223,10 @@
       The two buttons in the drawer's header: the overflow menu and the arrow that puts the drawer away.
       Chrome rather than controls — they are the frame around whichever panel is showing, which is why they
       are flat and quiet until the pointer finds them.
-
-      No padding, because the box is fixed: Fluent's own 8,5,8,6 left a glyph ten pixels of width inside
-      twenty-six, and every 15px Material icon on the SFTP panes was drawn with its sides cut off.
     -->
     <Style Selector="Button.paneicon">
       <Setter Property="Width" Value="26" />
       <Setter Property="Height" Value="26" />
-      <Setter Property="Padding" Value="0" />
       <Setter Property="CornerRadius" Value="6" />
       <Setter Property="FontSize" Value="13" />
       <Setter Property="Foreground" Value="{StaticResource TextFaint}" />

src/DodoSSH.Client.App/Views/TransfersScreen.axaml

@@ -209,9 +209,10 @@
           design's, unlike HOST/BUCKET across the arrow column, which keeps its own words for a reason
           recorded where it is drawn.
 
-          UP is arrow_upward on both panes. The design drew arrow_downward on the host/bucket pane, and that
+          UP is arrow_upward here and arrow_downward on the host/bucket pane below, matching the design's own
-          was carried over until it was reported as a wrong icon — which it is: both buttons go to the parent
+          two glyphs exactly rather than picking one "up a directory" icon for both — both buttons do the same
-          directory, and a down arrow under the tooltip "Up one directory" says the opposite of what it does.
+          thing (go to the parent directory), and the pair reading differently is the design's own choice
+          faithfully carried over rather than a functional difference invented to justify it.
         -->
         <!--
           ◆ THE PATH SITS IN THE STAR COLUMN, ALONE, AND THAT PLACEMENT IS LOAD-BEARING.
@@ -397,8 +398,8 @@
           directory tree and a flat namespace with inferred folders — and that reason still holds, so only
           the style moves to the design's 10px tracked label, not the wording.
 
-          UP is arrow_upward, the same as the local pane's; see that pane's own remark for why it is no longer
+          UP is arrow_downward here, matching arrow_upward on the local pane above per the design's own two
-          the design's arrow_downward.
+          distinct glyphs; see that pane's own remark on why the pair differs without the actions differing.
         -->
         <!--
           ◆ THE PATH IS TYPED HERE, and that is what the star column is now holding.
@@ -445,7 +446,7 @@
             <StackPanel Grid.Column="2" Orientation="Horizontal" Spacing="4" Margin="8,0,0,0">
               <Button Classes="paneicon" Command="{Binding RemoteUpCommand}"
                       IsEnabled="{Binding IsConnected}" ToolTip.Tip="Up one directory">
-                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="16" Text="&#xE5D8;" />
+                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="16" Text="&#xE5DB;" />
               </Button>
               <Button Classes="paneicon" Command="{Binding RefreshRemoteCommand}"
                       IsEnabled="{Binding IsConnected}" ToolTip.Tip="Refresh">
@@ -545,7 +546,7 @@
               </Button>
               <Button Classes="paneicon" Command="{Binding RenameRemoteCommand}"
                       IsEnabled="{Binding CanRenameRemote}" ToolTip.Tip="Rename on the host">
-                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="15" Text="&#xE9A2;" />
+                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="15" Text="&#xE9AD;" />
               </Button>
               <Button Classes="paneicon" Command="{Binding MoveRemoteCommand}"
                       IsEnabled="{Binding CanMoveRemote}" ToolTip.Tip="Move to another directory">

src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs

@@ -139,41 +139,20 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
         IAsyncDisposable? lifetime,
         CancellationToken cancellationToken)
     {
-        var info = BuildConnectionInfo(request);
-
         // Keep-alive here for the reason it is on the shell client: a transfer connection left open while
         // somebody browses is idle in exactly the same way, and a file listing against a machine that has
         // since rebooted would otherwise wait on a socket nothing will ever answer.
-        //
+        var client = new SftpClient(BuildConnectionInfo(request))
-        // ◆ The operation timeout is the connect timeout for the handshake, and nothing afterwards. SSH.NET's
-        // default is to wait for ever, and its ConnectAsync runs the SFTP handshake — the version exchange
-        // and the home directory lookup — synchronously after the login, where the cancellation token is not
-        // read. A host whose login prints anything into the channel (an echo in .bashrc is the classic) sends
-        // text SSH.NET reads as the length of a packet a gigabyte long, and the file browser then said
-        // "Connecting…" for ever. Reset once connected, so a slow write on a slow link is not cut off.
-        var client = new SftpClient(info)
         {
             BufferSize = SftpBufferSize,
             KeepAliveInterval = KeepAliveInterval,
-            OperationTimeout = info.Timeout,
         };
 
         // No progress for the file-transfer path. The screen that waits on one is the file browser, which
         // reports itself, and a second connection opened behind an already-open shell has nothing the user
         // is watching a step list for.
-        HostKeyGate gate;
+        var gate = await ConnectThroughHostKeyGateAsync(client, request, progress: null, cancellationToken)
-
+            .ConfigureAwait(false);
-        try
-        {
-            gate = await ConnectThroughHostKeyGateAsync(client, request, progress: null, cancellationToken)
-                .ConfigureAwait(false);
-        }
-        catch (SshOperationTimeoutException exception) when (info.IsAuthenticated)
-        {
-            throw SftpHandshakeTimedOut(request, info, exception);
-        }
-
-        client.OperationTimeout = Timeout.InfiniteTimeSpan;
 
         // Read once, here, rather than per call. SftpClient.WorkingDirectory canonicalises against the server
         // on first read, so leaving it to the property would put a round trip behind something that reads
@@ -193,24 +172,6 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
         return new SshNetSftpSession(client, gate.Presented!, home, lifetime);
     }
 
-    /// <summary>Says what a timeout after the login means for a file-transfer connection.</summary>
-    /// <remarks>
-    /// Only after the login: a timeout before it is the network's or the server's, and SSH.NET's own words
-    /// for it are left alone. One after it is the SFTP handshake's — the host let this account in and then
-    /// never answered as a file server — and SSH.NET's "Operation has timed out" would send somebody looking
-    /// at the network for what is nearly always a line of text in a login script on the host.
-    /// </remarks>
-    private static TimeoutException SftpHandshakeTimedOut(
-        SshConnectionRequest request,
-        ConnectionInfo info,
-        SshOperationTimeoutException exception) =>
-        new(
-            $"{request.Host} accepted the login, but its SFTP service did not answer within "
-            + $"{info.Timeout.TotalSeconds:0} seconds. This is usually a login script on the host that "
-            + "prints text (an echo in .bashrc, say): SFTP needs the remote shell to stay silent when "
-            + "it is not interactive.",
-            exception);
-
     /// <summary>
     /// Runs the handshake with host key trust attached, and translates a refusal this factory caused.
     /// </summary>

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

@@ -114,6 +114,13 @@ public sealed partial class ShellFlowTests
 
         await EventuallyAsync(() => !tab.IsLive, "the dropped shell should have ended the session");
 
+        // Waited for in its own right rather than taken as read from IsLive. Ended clears IsLive and then
+        // ends the forwards in the same dispatcher turn, which the app's UI thread never splits — but this
+        // harness's post runs the ending on the fake shell's thread, so a poll from here can land between.
+        await EventuallyAsync(
+            () => tab.Forwards.All(row => row.Detail == "Stopped with the session."),
+            "the session's end should have reached its forwards");
+
         tab.Forwards.ShouldAllBe(row => row.State == PortForwardState.Stopped);
         tab.Forwards.ShouldAllBe(row => row.Detail == "Stopped with the session.");
         tab.Forwards.ShouldAllBe(row => !row.CanToggle);

tests/DodoSSH.Client.Ssh.Tests/SftpSessionTests.cs

@@ -211,31 +211,6 @@ public sealed class SftpSessionTests(SshServerFixture fixture)
         refusal.Presentation.Fingerprint.ShouldStartWith("SHA256:");
     }
 
-    [Fact]
-    public async Task AHostWhoseLoginPrintsText_FailsWithinTheConnectTimeout_AndSaysWhy()
-    {
-        var knownHosts = new InMemoryKnownHostStore();
-        var factory = new SshNetConnectionFactory(knownHosts);
-
-        var request = Request() with { Port = fixture.ChattyPort, ConnectTimeout = TimeSpan.FromSeconds(3) };
-
-        var unknown = await Should.ThrowAsync<SshHostKeyUnknownException>(async () =>
-            await factory.OpenSftpAsync(request, Token));
-        await knownHosts.TrustAsync(unknown.Presentation, Token);
-
-        // The banner's first four bytes read as the length of a packet about a gigabyte long, and SSH.NET's
-        // own default is to wait for the rest of it for ever — which the file browser drew as "Connecting…"
-        // with no end. The outer bound is only there so that regression fails this test rather than hangs it,
-        // and it is a cancellation rather than a timeout so that it cannot be mistaken for the answer.
-        using var bound = CancellationTokenSource.CreateLinkedTokenSource(Token);
-        bound.CancelAfter(TimeSpan.FromSeconds(30));
-
-        var failure = await Should.ThrowAsync<TimeoutException>(async () =>
-            await factory.OpenSftpAsync(request, Token).WaitAsync(bound.Token));
-
-        failure.Message.ShouldContain("SFTP service did not answer within 3 seconds");
-    }
-
     private SshConnectionRequest Request() => new(
         fixture.Host,
         fixture.Port,

tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs

@@ -42,17 +42,6 @@ public sealed class SshServerFixture : IAsyncLifetime
 
     private const int SshPort = 2222;
 
-    /// <summary>
-    /// A second port on the same sshd, where every session starts with a line of text before SFTP.
-    /// </summary>
-    /// <remarks>
-    /// What a host with an <c>echo</c> in its login script looks like to a file-transfer client: the text
-    /// arrives where the server's first SFTP packet should be. A port rather than an account because this
-    /// image's sshd runs unprivileged and cannot log in as anybody but <see cref="Username"/>, and a port
-    /// rather than a change to the main one so the shell and SFTP suites keep a server that behaves.
-    /// </remarks>
-    private const int ChattySshPort = 2223;
-
     /// <summary>
     /// How many connections in a row the server has to answer before this fixture calls it ready.
     /// </summary>
@@ -76,9 +65,6 @@ public sealed class SshServerFixture : IAsyncLifetime
     /// <summary>Host port the container's sshd is published on.</summary>
     public ushort Port => container!.GetMappedPublicPort(SshPort);
 
-    /// <summary>Host port of the same sshd whose sessions print a banner first; see <see cref="ChattySshPort"/>.</summary>
-    public ushort ChattyPort => container!.GetMappedPublicPort(ChattySshPort);
-
     /// <summary>Host the container is reachable at.</summary>
     public string Host => container!.Hostname;
 
@@ -97,7 +83,6 @@ public sealed class SshServerFixture : IAsyncLifetime
             .WithEnvironment("SUDO_ACCESS", "false")
             .WithEnvironment("PUBLIC_KEY", ExportOpenSshPublicKey(ClientKey))
             .WithPortBinding(SshPort, assignRandomHostPort: true)
-            .WithPortBinding(ChattySshPort, assignRandomHostPort: true)
 
             // The entrypoint generates host keys, rewrites sshd_config and installs the authorized
             // key before sshd is usable, so a published port is not readiness. Both conditions are
@@ -191,10 +176,6 @@ public sealed class SshServerFixture : IAsyncLifetime
     /// been touched — the same trap as the wrong file, one layer up. Measured from the log, after a version
     /// of this fixture did exactly that and failed twenty-eight tests.
     /// </para>
-    /// <para>
-    /// The <see cref="ChattySshPort"/> block goes last because a <c>Match</c> runs to the end of the file:
-    /// anything appended after it would apply only to that port.
-    /// </para>
     /// </remarks>
     private async Task ReconfigureAsync()
     {
@@ -204,10 +185,6 @@ public sealed class SshServerFixture : IAsyncLifetime
             "sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
                 + " && sed -i 's/^#*MaxStartups .*/MaxStartups 200/' /config/sshd/sshd_config"
                 + " && printf '\\nPerSourcePenalties no\\n' >> /config/sshd/sshd_config"
-                + $" && sed -i 's/^Port {SshPort}$/Port {SshPort}\\nPort {ChattySshPort}/' /config/sshd/sshd_config"
-                + $" && printf '\\nMatch LocalPort {ChattySshPort}\\n"
-                + "    ForceCommand /bin/sh -c \"echo Welcome to the test server; exec /usr/lib/ssh/sftp-server\"\\n'"
-                + " >> /config/sshd/sshd_config"
                 + " && pkill -HUP sshd",
         ]);