diff --git a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs index 79a6dc9..098b6b4 100644 --- a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs +++ b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs @@ -101,6 +101,13 @@ public sealed class TerminalWorkspace : IAsyncDisposable private readonly Lock sessionGate = new(); private readonly CancellationTokenSource lifetime = new(); + /// + /// The payload that marks a frame as a replay rather + /// than a fresh open. A one-byte non-empty payload, so terminal.js's existing length check (empty + /// payload for a real open) tells the two apart without a second opcode. + /// + private static readonly byte[] ReplayMarker = [1]; + private uint nextSessionId = 1; private Task? server; private int disposed; @@ -125,6 +132,12 @@ public sealed class TerminalWorkspace : IAsyncDisposable // came from. Nothing here decides anything about the size: the shell owns it, because the shell is // what remembers it between launches. dataPlane.FontSizeStepRequested += (_, e) => FontSizeStepRequested?.Invoke(this, e); + + // Fire-and-forget: this fires on the socket-accept thread, in the middle of the data plane's own + // handshake handling, and has no business making that wait on however long a replay takes. See + // ReplayAfterAttachAsync for what "replay" means and why racing the fresh page's own first frames + // is harmless. + dataPlane.SocketAttached += (_, _) => _ = ReplayAfterAttachAsync(); } /// @@ -223,6 +236,26 @@ public sealed class TerminalWorkspace : IAsyncDisposable } } + /// + /// A live session's flow-control window, or null when the id names no session this workspace still has + /// open. + /// + /// + /// A test seam rather than something the shell has ever needed: nothing outside this assembly has a + /// reason to see a pump's credit window rather than what the transport does with it, but + /// 's reset of that window on reattach is exactly the kind of thing + /// that is easy to get backwards, and worth asserting directly rather than only through its side + /// effects. Internal rather than public, reachable from the test assembly through the + /// InternalsVisibleTo this project already declares for it. + /// + internal CreditWindow? CreditsFor(uint sessionId) + { + lock (sessionGate) + { + return sessions.TryGetValue(sessionId, out var session) ? session.Pump.Credits : null; + } + } + /// /// Raised with the session id when a shell ends on its own. /// @@ -254,6 +287,24 @@ public sealed class TerminalWorkspace : IAsyncDisposable /// public event EventHandler? FontSizeStepRequested; + /// + /// Raised once a (re)attached renderer has been sent everything this workspace owns for it. + /// + /// + /// + /// The workspace's own share of "put the page back the way it was" is the sessions — each live one gets + /// its SessionOpened frame again, done by the time this fires. What is left is what the workspace + /// has no business owning: the font size and which tab is selected are both remembered by the shell, not + /// by a terminal, so this is the seam the shell uses to re-push them. See + /// MainWindowViewModel's subscription for the other half. + /// + /// + /// Raised on the socket-accept thread, same as that + /// triggers it — a handler that touches a view model has to marshal. + /// + /// + public event EventHandler? RendererReattached; + /// Starts the loopback listener. public void Start() => server = dataPlane.RunAsync(lifetime.Token); @@ -563,6 +614,71 @@ public sealed class TerminalWorkspace : IAsyncDisposable lifetime.Dispose(); } + /// + /// Rebuilds a freshly (re)attached page's idea of what is running, then tells the shell to rebuild its + /// own. + /// + /// + /// + /// Runs on the socket-accept thread that raised — the + /// constructor wires it up fire-and-forget for exactly that reason, so this method owns its own error + /// handling rather than leaving an unobserved exception for nobody to see. + /// + /// + /// Every live session — one whose Run has not completed — gets two things. Its credit window is + /// reset, because whatever was outstanding was reserved against bytes sent to a page that is now gone; + /// the acknowledgement that would return that credit died with it, and without this reset the session + /// would stall the moment 256 KiB of history had accumulated. And it gets its SessionOpened frame + /// again, marked with so the page can tell a reattach from a session that is + /// genuinely new — the same frame a page that survived the socket drop already has a pane for, and one a + /// reloaded page does not. + /// + /// + /// A session whose shell has already ended gets nothing here. Its scrollback lived only in the page that + /// is gone, and sending a frame that implied otherwise would be exactly the kind of dishonesty this + /// fix is supposed to remove, not add. The tab strip still shows that session ended; nothing about this + /// method changes what or report. + /// + /// + /// This can race the fresh page's own first frames — an early resize, an acknowledgement for output it + /// already had. That is harmless: every frame in both directions names its session, delivery order + /// within a session is preserved by both xterm and the socket, and a frame for a pane the page has not + /// created yet is simply dropped, the same as any frame for a session it does not know — see + /// terminal.js's handleFrame. + /// + /// + private async Task ReplayAfterAttachAsync() + { + KeyValuePair[] live; + + lock (sessionGate) + { + live = [.. sessions.Where(entry => !entry.Value.Run.IsCompleted)]; + } + + try + { + foreach (var (sessionId, session) in live) + { + session.Pump.Credits.Reset(); + + await dataPlane + .SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.SessionOpened, sessionId, ReplayMarker), + CancellationToken.None) + .ConfigureAwait(false); + } + + RendererReattached?.Invoke(this, EventArgs.Empty); + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + // Best-effort, same as every other fire-and-forget path here: a page that dies again mid-replay + // leaves nothing worse than the problem this method exists to fix, and there is no caller on + // this thread left to hand a failure to. + } + } + private async Task RunSessionAsync(uint sessionId, TerminalSessionPump pump) { try diff --git a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs index abbbbf0..ee33752 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs @@ -8,6 +8,8 @@ internal sealed class FakeShellSession : ISshShellSession private readonly List 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. /// - internal FakeShellSession(long bytesToProduce = 0) => remaining = bytesToProduce; + /// + /// 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 + /// covers. then blocks until cancelled, which is what a real idle SSH channel's + /// read does. Exists for tests that need a session whose Run stays live without a background + /// read loop racing the test for control of the pump's credit window — see the reattach tests in + /// TerminalWorkspaceTests. + /// + internal FakeShellSession(long bytesToProduce = 0, bool blockReads = false) + { + remaining = bytesToProduce; + this.blockReads = blockReads; + } /// 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. /// -internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) : ISshConnectionFactory +internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue, bool blockShellReads = false) + : ISshConnectionFactory { /// Connections handed out, in order. internal List 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(connection); @@ -127,7 +149,8 @@ internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) } /// A connection that opens fake shells and records its own disposal. -internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell) : ISshConnection +internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell, bool blockShellReads = false) + : ISshConnection { /// public bool IsConnected { get; private set; } = true; @@ -148,7 +171,7 @@ internal sealed class FakeConnection(SshConnectionRequest request, long bytesPer /// public Task OpenShellAsync(TerminalSize size, CancellationToken cancellationToken) { - Shell = new FakeShellSession(bytesPerShell); + Shell = new FakeShellSession(bytesPerShell, blockShellReads); return Task.FromResult(Shell); } diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs index feac62b..52dca61 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs +++ b/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 ---- + + /// + /// + /// 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. + /// + /// + /// 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 Run live without a background read loop + /// competing with this test over the credit window's exact value. + /// + /// + /// 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 — calls + /// Credits.Reset() 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. + /// + /// + [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"); + } + + /// + /// 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. MainWindowViewModel's subscription is what actually + /// does that; this only asserts that the workspace hands it the chance to. + /// + [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 ---- /// @@ -362,12 +444,76 @@ public sealed class TerminalWorkspaceTests private static InMemoryTerminalAssetProvider StubAssets() => new(new Dictionary(StringComparer.Ordinal) { - [TerminalDataPlane.PagePath] = new("text/html; charset=utf-8", ""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( + $"")), }); private static SshConnectionRequest Request() => new("host.invalid", 22, "dodo", new SshPasswordCredential("irrelevant")); + /// + /// 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. + /// + private static async Task 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()); + } + /// /// 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