Merge branch 'claude/terminal-reattach'

Brings the Android keep-alive corrections and the terminal renderer
reattach: the foreground service now actually comes up for shells and
an idle Files session, survives refreshes from the background, and the
terminal's data plane lets a reloaded WebView page take its socket back
over instead of freezing every session behind a dead one.

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

@@ -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
     {

tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs

@@ -8,6 +8,8 @@ internal sealed class FakeShellSession : ISshShellSession
     private readonly List<byte> written = [];
     private readonly Lock gate = new();
 
+    private readonly bool blockReads;
+
     private long remaining;
     private byte pattern;
 
@@ -16,7 +18,19 @@ internal sealed class FakeShellSession : ISshShellSession
     /// endless producer, which is what a runaway remote process looks like — those sessions are ended
     /// by disposing the pump rather than by running out of data.
     /// </param>
-    internal FakeShellSession(long bytesToProduce = 0) => remaining = bytesToProduce;
+    /// <param name="blockReads">
+    /// True for a shell that is open and live but has nothing to say — an idle prompt, rather than either
+    /// end of the "produces bytes" and "hit end of stream" spectrum <paramref name="bytesToProduce"/>
+    /// covers. <see cref="ReadAsync"/> then blocks until cancelled, which is what a real idle SSH channel's
+    /// read does. Exists for tests that need a session whose <c>Run</c> stays live without a background
+    /// read loop racing the test for control of the pump's credit window — see the reattach tests in
+    /// <c>TerminalWorkspaceTests</c>.
+    /// </param>
+    internal FakeShellSession(long bytesToProduce = 0, bool blockReads = false)
+    {
+        remaining = bytesToProduce;
+        this.blockReads = blockReads;
+    }
 
     /// <inheritdoc />
     public bool IsOpen { get; private set; } = true;
@@ -52,6 +66,13 @@ internal sealed class FakeShellSession : ISshShellSession
     {
         ReadCount++;
 
+        if (blockReads)
+        {
+            // Never completes on its own. The only way out is the same way a real blocked read ends: the
+            // token being cancelled, which is what disposing the pump does.
+            await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken).ConfigureAwait(false);
+        }
+
         await Task.Yield();
         cancellationToken.ThrowIfCancellationRequested();
 
@@ -109,7 +130,8 @@ internal sealed class FakeShellSession : ISshShellSession
 /// workspace is the layer that decides when a session is over, and that decision is what needs a
 /// connection whose shell can be made to end on cue.
 /// </remarks>
-internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) : ISshConnectionFactory
+internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue, bool blockShellReads = false)
+    : ISshConnectionFactory
 {
     /// <summary>Connections handed out, in order.</summary>
     internal List<FakeConnection> Connections { get; } = [];
@@ -119,7 +141,7 @@ internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue)
         SshConnectionRequest request,
         CancellationToken cancellationToken)
     {
-        var connection = new FakeConnection(request, bytesPerShell);
+        var connection = new FakeConnection(request, bytesPerShell, blockShellReads);
         Connections.Add(connection);
 
         return Task.FromResult<ISshConnection>(connection);
@@ -127,7 +149,8 @@ internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue)
 }
 
 /// <summary>A connection that opens fake shells and records its own disposal.</summary>
-internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell) : ISshConnection
+internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell, bool blockShellReads = false)
+    : ISshConnection
 {
     /// <inheritdoc />
     public bool IsConnected { get; private set; } = true;
@@ -148,7 +171,7 @@ internal sealed class FakeConnection(SshConnectionRequest request, long bytesPer
     /// <inheritdoc />
     public Task<ISshShellSession> OpenShellAsync(TerminalSize size, CancellationToken cancellationToken)
     {
-        Shell = new FakeShellSession(bytesPerShell);
+        Shell = new FakeShellSession(bytesPerShell, blockShellReads);
 
         return Task.FromResult<ISshShellSession>(Shell);
     }

tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs

@@ -142,15 +142,96 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable
             await ConnectAsync(origin: "https://evil.example"));
     }
 
+    /// <remarks>
+    /// The truth this replaced: a second valid attach used to be a 409, on the theory that one renderer
+    /// lives for the whole process. Android's WebView does not honour that theory — its renderer process is
+    /// routinely killed and the page reloads with a fresh socket — so a second valid attach is now a
+    /// takeover. This asserts both halves: the newcomer gets the connection, and the displaced socket
+    /// actually goes rather than lingering as a phantom nothing is reading from.
+    /// </remarks>
     [Fact]
-    public async Task ASecondRenderer_IsRejected()
+    public async Task ASecondRenderer_TakesOver_AndTheFirstSocketIsDropped()
     {
         Start();
 
         using var first = await ConnectAsync();
         first.State.ShouldBe(WebSocketState.Open);
 
-        await Should.ThrowAsync<WebSocketException>(async () => await ConnectAsync());
+        using var second = await ConnectAsync();
+        second.State.ShouldBe(WebSocketState.Open);
+
+        // The first socket was aborted rather than closed gracefully — Abort skips the close handshake
+        // entirely, so there is no Close frame for this side to see coming. What a receive on it sees
+        // instead is the connection simply gone, which the client surfaces as an exception rather than as
+        // a state that quietly flips on its own; nothing here reads from the socket otherwise, so the
+        // state alone would not move.
+        var firstBuffer = new byte[16];
+        await Should.ThrowAsync<Exception>(async () =>
+            await first.ReceiveAsync(firstBuffer.AsMemory(), TestContext.Current.CancellationToken));
+
+        await using var session = new FakeShellSession(bytesToProduce: 64);
+        await using var pump = CreatePump(session);
+        plane.Register(SessionId, pump);
+
+        var run = pump.RunAsync(TestContext.Current.CancellationToken);
+
+        var opened = await ReceiveAsync(second);
+        opened.Opcode.ShouldBe((byte)TerminalServerOpcode.SessionOpened);
+
+        var output = await ReceiveAsync(second);
+        output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output);
+        output.Payload.Length.ShouldBe(64);
+
+        await SendAsync(
+            second,
+            (byte)TerminalClientOpcode.Acknowledge,
+            TerminalFrame.CreateAcknowledgementPayload((uint)output.Payload.Length));
+
+        await run;
+    }
+
+    /// <remarks>
+    /// <para>
+    /// The other half of the takeover: a renderer process that dies without a close handshake — which is
+    /// what a killed Android WebView actually does, no FIN, nothing — must not fault the send path. A
+    /// faulted send would propagate into <see cref="TerminalSessionPump"/>'s flush loop and freeze a live
+    /// session; see <see cref="TerminalDataPlane.SendAsync"/>'s remark for why. Disposing the client socket
+    /// abruptly, with no close handshake sent, is the closest this harness gets to that: the server-side
+    /// socket is left believing itself open until it actually tries to write to it.
+    /// </para>
+    /// <para>
+    /// Driven straight through <see cref="TerminalDataPlane.SendAsync"/> rather than through a pump, because
+    /// a pump adds nothing here — the point is entirely about the transport's own contract, and a session
+    /// layered on top would only leave it unclear whether a passing test proved the transport never threw or
+    /// merely that the frames never happened to need a live socket.
+    /// </para>
+    /// </remarks>
+    [Fact]
+    public async Task SendAsync_DoesNotThrow_WhenTheAttachedRendererDiedWithoutClosing_AndAFreshAttachStillReceives()
+    {
+        Start();
+
+        var first = await ConnectAsync();
+        first.State.ShouldBe(WebSocketState.Open);
+        first.Dispose();
+
+        // Whether this particular send lands on the OS's send buffer before the peer's absence is noticed,
+        // or fails immediately, is not the point — either way it must not throw.
+        await Should.NotThrowAsync(async () =>
+            await plane.SendAsync(
+                TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "before"u8.ToArray()),
+                TestContext.Current.CancellationToken));
+
+        using var second = await ConnectAsync();
+
+        await Should.NotThrowAsync(async () =>
+            await plane.SendAsync(
+                TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "after"u8.ToArray()),
+                TestContext.Current.CancellationToken));
+
+        var output = await ReceiveAsync(second);
+        output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output);
+        Encoding.UTF8.GetString(output.Payload).ShouldBe("after");
     }
 
     // ---- Frames ----

tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs

@@ -1,3 +1,6 @@
+using System.Globalization;
+using System.Net.WebSockets;
+using System.Text;
 using DodoSSH.Client.Ssh;
 
 namespace DodoSSH.Client.Terminal.Tests;
@@ -291,6 +294,85 @@ public sealed class TerminalWorkspaceTests
         connections.Connections.ShouldAllBe(connection => connection.IsDisposed);
     }
 
+    // ---- Reattach ----
+
+    /// <remarks>
+    /// <para>
+    /// The scenario the whole fix exists for: a page that lost its socket — killed WebView renderer, or
+    /// simply a reload — reattaches, and the session that was already running has to come back rather than
+    /// sit there forever with its output going nowhere.
+    /// </para>
+    /// <para>
+    /// The session's shell blocks on every read rather than producing output, which is what an idle prompt
+    /// looks like and — for this test — is what keeps its <c>Run</c> live without a background read loop
+    /// competing with this test over the credit window's exact value.
+    /// </para>
+    /// <para>
+    /// Neither assertion below polls, deliberately. The "before" one does not need to: reserving credit is
+    /// a synchronous call, so it is true the instant it returns. The "after" one does not need to either,
+    /// for a subtler reason — <see cref="TerminalWorkspace.ReplayAfterAttachAsync"/> calls
+    /// <c>Credits.Reset()</c> and only then awaits sending the replay frame for that same session, with no
+    /// suspension between the two, so by the time this test has received that frame the reset has
+    /// necessarily already happened. A poll here would only have hidden a real ordering bug behind a
+    /// generous timeout instead of catching it.
+    /// </para>
+    /// </remarks>
+    [Fact]
+    public async Task ANewRenderer_ReplaysTheLiveSessionAndResetsItsCredits()
+    {
+        var connections = new FakeConnectionFactory(blockShellReads: true);
+
+        await using var workspace = CreateWorkspace(connections);
+        workspace.Start();
+
+        using var first = await ConnectRendererAsync(workspace);
+
+        var sessionId = await workspace.OpenSessionAsync(
+            Request(), TerminalSize.Default, TestContext.Current.CancellationToken);
+
+        // The session's own opening frame, sent as soon as the pump starts running. Not a replay, and not
+        // what this test is about — read and discarded so it cannot be confused for one below.
+        await ReceiveFrameAsync(first);
+
+        var credits = workspace.CreditsFor(sessionId).ShouldNotBeNull();
+        credits.TryReserve(4096);
+        credits.Outstanding.ShouldBeGreaterThanOrEqualTo(
+            4096, "the pump's own read loop may have reserved a buffer's worth on top of this");
+
+        using var second = await ConnectRendererAsync(workspace);
+
+        var replay = await ReceiveFrameAsync(second);
+        replay.Opcode.ShouldBe((byte)TerminalServerOpcode.SessionOpened);
+        replay.SessionId.ShouldBe(sessionId);
+        replay.Payload.ShouldBe(new byte[] { 1 }, "a replay is flagged so the page can tell it apart from a fresh open");
+
+        credits.Outstanding.ShouldBe(0, "the replay frame above cannot have been sent before the reset that precedes it");
+    }
+
+    /// <remarks>
+    /// The other half of a reattach: the workspace has replayed what it owns, and this is the seam the
+    /// shell uses to replay what it owns instead — the font size and the selected tab, neither of which a
+    /// terminal session knows anything about. <c>MainWindowViewModel</c>'s subscription is what actually
+    /// does that; this only asserts that the workspace hands it the chance to.
+    /// </remarks>
+    [Fact]
+    public async Task ANewRenderer_RaisesRendererReattached()
+    {
+        var connections = new FakeConnectionFactory();
+
+        await using var workspace = CreateWorkspace(connections);
+        workspace.Start();
+
+        using var first = await ConnectRendererAsync(workspace);
+
+        var reattachedCount = 0;
+        workspace.RendererReattached += (_, _) => Interlocked.Increment(ref reattachedCount);
+
+        using var second = await ConnectRendererAsync(workspace);
+
+        await WaitUntilAsync(() => Volatile.Read(ref reattachedCount) > 0);
+    }
+
     // ---- Helpers ----
 
     /// <remarks>
@@ -362,12 +444,76 @@ public sealed class TerminalWorkspaceTests
     private static InMemoryTerminalAssetProvider StubAssets() =>
         new(new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)
         {
-            [TerminalDataPlane.PagePath] = new("text/html; charset=utf-8", "<!doctype html>"u8.ToArray()),
+            // The placeholders, not a token and URL already filled in — the reattach tests below have to
+            // connect a real renderer, and doing that by reading them back out of the served page is what
+            // proves the workspace serves a page a real renderer could actually attach with, rather than
+            // one that merely looks servable.
+            [TerminalDataPlane.PagePath] = new(
+                "text/html; charset=utf-8",
+                Encoding.UTF8.GetBytes(
+                    $"<html><body data-token=\"{TerminalDataPlane.TokenPlaceholder}\" "
+                    + $"data-socket=\"{TerminalDataPlane.SocketUrlPlaceholder}\"></body></html>")),
         });
 
     private static SshConnectionRequest Request() =>
         new("host.invalid", 22, "dodo", new SshPasswordCredential("irrelevant"));
 
+    /// <remarks>
+    /// Attaches the way the real page does: by fetching the served page, reading the token and socket URL
+    /// back out of it, and presenting them on the upgrade — rather than reaching into the workspace for a
+    /// token it does not expose. A shortcut here would prove only that a socket can be opened, not that the
+    /// workspace serves a page a renderer could actually attach with.
+    /// </remarks>
+    private static async Task<ClientWebSocket> ConnectRendererAsync(TerminalWorkspace workspace)
+    {
+        using var http = new HttpClient();
+        var page = await http.GetStringAsync(workspace.PageUrl, TestContext.Current.CancellationToken);
+
+        var token = ExtractAttribute(page, "data-token");
+        var socketUrl = ExtractAttribute(page, "data-socket");
+
+        var client = new ClientWebSocket();
+        client.Options.AddSubProtocol(TerminalDataPlane.SubProtocol);
+        client.Options.AddSubProtocol($"token.{token}");
+        client.Options.SetRequestHeader(
+            "Origin",
+            string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{workspace.PageUrl.Port}"));
+
+        try
+        {
+            await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken);
+        }
+        catch
+        {
+            client.Dispose();
+            throw;
+        }
+
+        return client;
+    }
+
+    private static string ExtractAttribute(string html, string name)
+    {
+        var marker = $"{name}=\"";
+        var start = html.IndexOf(marker, StringComparison.Ordinal) + marker.Length;
+        var end = html.IndexOf('"', start);
+
+        return html[start..end];
+    }
+
+    private static async Task<(byte Opcode, uint SessionId, byte[] Payload)> ReceiveFrameAsync(
+        ClientWebSocket socket)
+    {
+        var buffer = new byte[64 * 1024];
+
+        var result = await socket.ReceiveAsync(buffer.AsMemory(), TestContext.Current.CancellationToken);
+
+        TerminalFrame.TryRead(buffer.AsSpan(0, result.Count), out var opcode, out var sessionId, out var payload)
+            .ShouldBeTrue();
+
+        return (opcode, sessionId, payload.ToArray());
+    }
+
     /// <remarks>
     /// Polled rather than awaited on a task, because the point is what an observer of the property
     /// sees: the pump ends on a thread of its own, and the count has to catch up without anyone