diff --git a/docs/manual-checks.md b/docs/manual-checks.md index e5dde21..4d096c5 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -202,6 +202,44 @@ That is what the "is this still the page's socket" guard in `connect()` exists t input while the banner is *clear* is neither: the socket is open and dead, which nothing on this page can currently see — see the note at the end of 11.12a. +### 1.10a A remote that floods the terminal does not grow the process · **watch Task Manager** + +Open a shell and run `yes dodossh`. Leave it running for a full minute with the tab in front of you, then +switch to another screen and leave it another minute, then come back. Watch the application's memory in +Task Manager (Details tab, *Memory (active private working set)*) the whole time. + +**Pass:** memory settles at a plateau and stays there — the number moves as the GC breathes, but there is no +staircase. Ctrl+C, and the prompt comes back straight away. Somewhere in the scrollback there may be a dim +grey line reading `── N MB of output was dropped; the terminal could not keep up ──`; that line is a pass, +not a failure. It is this client saying it discarded output it could not draw, which is what keeps the +plateau flat, and the number after it tells you how far behind the display got. + +**Failure means:** a working set that climbs steadily for the whole minute and does not come back down after +Ctrl+C is the defect this check exists for, and it will not be in this repository's own buffers — it is +SSH.NET's. `ShellStream` copies everything the channel delivers into a buffer that doubles and is never +given back, and the channel keeps delivering because `Channel.OnData` re-opens the SSH receive window as +bytes *arrive* rather than as they are consumed. Measured against the container in +`DodoSSH.Client.Ssh.Tests`, a reader that stops entirely leaves that buffer holding 44 MB after five +seconds. The only thing that keeps it empty is `TerminalSessionPump` reading continuously and bounding what +it holds in `OutputBacklog` instead — so a staircase here means something has put a wait back into that read +loop. `ARunawayRemote_KeepsBeingReadAndStaysInsideTheBacklog` is the test that fails first. + +### 1.10b A tab whose shell exited stops costing anything · **needs a debugger or `netstat`** + +Open a shell, type `exit`, and **leave the tab open**. Note the host and port. Then, from a terminal outside +the application, run `netstat -ano | findstr ` and look for the connection to that host. + +**Pass:** the connection is gone within a second or two of the shell exiting, even though the tab is still on +screen showing the session ended and still offering RECONNECT. Clicking that reconnect still works, and +closing the tab afterwards still works. + +**Failure means:** a connection that is still ESTABLISHED for as long as the tab is open is the session +holding an authenticated socket it has no further use for — with a keep-alive putting a packet on the wire +every thirty seconds and SSH.NET's read buffer still at whatever size the session's busiest moment grew it +to. It used to do exactly that, until whichever tab it was got closed. `TerminalWorkspace.ReleaseAsync` is +what gives it back now, and `ASessionWhoseRemoteHasExited_GivesBackItsConnectionWithoutWaitingForTheTab` is +the test that says so. + ### 1.11 The right button copies and pastes, and never opens a menu Open a shell and run something that prints a few lines — `ls -la /etc`. Drag a selection across part of the diff --git a/src/DodoSSH.Client.Shell/WebAssets/terminal.js b/src/DodoSSH.Client.Shell/WebAssets/terminal.js index 50c91b7..68aaf73 100644 --- a/src/DodoSSH.Client.Shell/WebAssets/terminal.js +++ b/src/DodoSSH.Client.Shell/WebAssets/terminal.js @@ -34,6 +34,7 @@ const SERVER_SESSION_REMOVED = 5; const SERVER_PASTE = 6; const SERVER_FONT_SIZE = 7; const SERVER_CLIPBOARD = 8; +const SERVER_OUTPUT_DROPPED = 9; const CLIENT_INPUT = 1; const CLIENT_ACKNOWLEDGE = 2; @@ -534,6 +535,24 @@ function handleFrame(buffer) { break; } + case SERVER_OUTPUT_DROPPED: { + const session = sessions.get(sessionId) ?? createSession(sessionId); + + /* + The host could not keep the whole of a flood, and this is the line saying where the gap is. It + goes in exactly here, between the bytes either side of it — the socket keeps order within a + session, so nothing has to be done to place it. + + Written without an acknowledgement, deliberately. Credit describes what the remote sent and what + this page has drawn of it; these bytes came from the host itself, and acknowledging them would + hand back credit for output that was never reserved against the window in the first place. The + payload arrives already styled and already terminated by a reset — see the host's own remark for + why the reset comes first as well. + */ + session.term.write(payload); + break; + } + case SERVER_SESSION_ACTIVATED: { const session = sessions.get(sessionId); diff --git a/src/DodoSSH.Client.Terminal/CreditWindow.cs b/src/DodoSSH.Client.Terminal/CreditWindow.cs index 70a2970..7b65fe1 100644 --- a/src/DodoSSH.Client.Terminal/CreditWindow.cs +++ b/src/DodoSSH.Client.Terminal/CreditWindow.cs @@ -1,27 +1,33 @@ namespace DodoSSH.Client.Terminal; /// -/// Credit-based flow control over one terminal session's output. +/// Credit-based flow control over what has been sent to the renderer and not yet drawn. /// /// /// -/// This is what makes yes survivable. A terminal emulator renders at 60 Hz at best, while a -/// remote process can produce output as fast as the network allows. Without a limit the difference -/// accumulates somewhere — an unbounded queue in the client, or an ever-growing scrollback — and the -/// application's memory grows until it dies. +/// A terminal emulator renders at 60 Hz at best, while a remote process can produce output as fast as +/// the network allows. The difference has to accumulate somewhere, and the one place it must never +/// accumulate is inside the WebView: xterm queues whatever it is written and nothing on this side can +/// see how deep that queue is, so a page told to draw faster than it can grows until the tab dies. /// /// /// The mechanism: the renderer is granted a window of bytes it is allowed to be behind by. Each byte /// sent to it consumes credit; each byte it reports having actually rendered returns credit. When -/// credit reaches zero the pump stops reading the SSH channel. That closes SSH's own receive -/// window, which makes the remote sshd block on write, which propagates the backpressure all -/// the way to the process producing the output. Nothing buffers without bound because nothing is read -/// that cannot be delivered. +/// credit reaches zero the pump sends nothing more until some comes back. /// /// -/// The window has to be large enough that a normal burst never stalls and small enough to bound -/// memory. 256 KiB is roughly a screenful of dense output many times over, and it caps a session's -/// in-flight cost at a quarter of a megabyte. +/// ◆ What this does not do is bound what the remote has sent, and it used to be written as +/// though it did — the claim was that a pump which stops reading closes SSH's own receive window and +/// blocks the remote sshd. It does not, because SSH.NET re-opens that window as bytes arrive +/// rather than as they are consumed. Holding output back therefore never reached the remote at all; +/// it only chose which buffer the flood landed in, and the one it landed in had no ceiling. The bound +/// that was missing now lives in , which is also where the evidence for +/// this paragraph is written down. +/// +/// +/// The window has to be large enough that a normal burst never stalls and small enough to bound what +/// the page is holding. 256 KiB is roughly a screenful of dense output many times over, and it caps a +/// session's in-flight cost at a quarter of a megabyte. /// /// public sealed class CreditWindow diff --git a/src/DodoSSH.Client.Terminal/OutputBacklog.cs b/src/DodoSSH.Client.Terminal/OutputBacklog.cs new file mode 100644 index 0000000..d9b7e8e --- /dev/null +++ b/src/DodoSSH.Client.Terminal/OutputBacklog.cs @@ -0,0 +1,264 @@ +namespace DodoSSH.Client.Terminal; + +/// +/// One session's bounded queue of output waiting for the renderer, oldest first. +/// +/// +/// +/// ◆ This is where a session's memory is actually bounded, and it exists because the obvious place +/// cannot do it. The design used to be that bounded everything: the pump +/// stopped reading the SSH channel when the renderer fell behind, SSH's own receive window closed, and the +/// remote blocked on write. The middle step is not true of the library this client uses. +/// Renci.SshNet.Channels.Channel.OnData calls its window adjustment before raising +/// DataReceived and re-opens the channel window as soon as it drops below one packet, and +/// ShellStream's own handler copies the bytes into a growable buffer without blocking and without a +/// ceiling. So a reader that stops reading does not slow the remote down by one byte; it only decides +/// whose buffer the flood accumulates in — and SSH.NET's grows by doubling and does not give the memory +/// back until the stream is disposed. A remote running yes took the process with it. +/// +/// +/// So the pump now reads continuously, which is what keeps SSH.NET's buffer empty, and the backlog it +/// reads into is this one, with a hard capacity. When the renderer cannot keep up the oldest +/// bytes go rather than the newest, because the newest are the ones somebody is waiting to see: the tail +/// of a flood is the prompt that comes back after it. Dropping is announced rather than silent — see +/// — and it is the honest end of the trade, since output arriving faster than the +/// terminal can draw it was going to scroll off a 5,000-line scrollback regardless. +/// +/// +/// The credit window is still here and still does its own job: it bounds what is in flight to the +/// renderer, which is the buffer inside the WebView that nothing on this side can see. What changed +/// is that it no longer pretends to bound what is in flight from the remote. +/// +/// +public sealed class OutputBacklog +{ + /// + /// Default capacity: 4 MiB. + /// + /// + /// Sixteen times the credit window, so nothing an ordinary burst produces is ever dropped — a + /// screenful of dense output is kilobytes, and a build that scrolls for a second is well under this. + /// It is a per-session cost, and it is the whole of what a runaway remote can take, which is the + /// number worth choosing against: twenty tabs all flooding at once is 80 MiB and survivable, where the + /// same twenty unbounded was whatever the machine had. + /// + public const int DefaultCapacityBytes = 4 * 1024 * 1024; + + private readonly Lock gate = new(); + private readonly Queue segments = new(); + private readonly int capacityBytes; + + /// Signalled when output arrives at an empty backlog, and when it is completed. + private TaskCompletionSource available = CreateSignal(); + + /// How much of the head segment has already been taken. + private int head; + + private int buffered; + private long droppedSinceTake; + private long droppedTotal; + private bool completed; + + /// How many unsent bytes to hold before dropping the oldest. + public OutputBacklog(int capacityBytes = DefaultCapacityBytes) + { + ArgumentOutOfRangeException.ThrowIfLessThan(capacityBytes, 1); + + this.capacityBytes = capacityBytes; + } + + /// How many unsent bytes are held. + public int Buffered + { + get + { + lock (gate) + { + return buffered; + } + } + } + + /// How many bytes this backlog has discarded over its whole life. + public long DroppedTotal + { + get + { + lock (gate) + { + return droppedTotal; + } + } + } + + /// Whether the producer has said there will be no more output. + public bool IsCompleted + { + get + { + lock (gate) + { + return completed; + } + } + } + + /// + /// Adds output, dropping the oldest already held if that puts the backlog over capacity. + /// + /// + /// Takes ownership of ; the caller must not write to it afterwards. + /// Appending to a completed backlog does nothing rather than throwing, because a producer finding out + /// it has been shut down is a race rather than a mistake. + /// + /// Bytes read from the remote, in order. + public void Append(byte[] segment) + { + ArgumentNullException.ThrowIfNull(segment); + + if (segment.Length == 0) + { + return; + } + + lock (gate) + { + if (completed) + { + return; + } + + var wasEmpty = buffered == 0; + + segments.Enqueue(segment); + buffered += segment.Length; + + // Never the segment just added, however large it is. A read bigger than the whole capacity is + // possible if somebody configures one, and a backlog that answered every take with nothing + // would stall the session rather than bound it. + while (buffered > capacityBytes && segments.Count > 1) + { + var oldest = segments.Dequeue(); + var discarded = oldest.Length - head; + + head = 0; + buffered -= discarded; + droppedSinceTake += discarded; + droppedTotal += discarded; + } + + // Only on the transition, because a waiter that found bytes here never captured the signal in + // the first place — see WaitForOutputAsync, which checks under this same lock. Signalling on + // every append would allocate a TaskCompletionSource per read of a session doing megabytes a + // second, for a waiter that is not there. + if (wasEmpty) + { + Signal(); + } + } + } + + /// + /// Moves the oldest output held into , as much of it as will fit. + /// + /// + /// + /// Into a caller's span rather than into an array of its own, because this is the hot path: the pump + /// writes straight into the frame it is about to send, so a terminal showing a gigabyte costs one copy + /// of it here rather than two. + /// + /// + /// is what makes a drop visible instead of silent. It counts the bytes + /// discarded since the previous take, which is exactly the gap sitting immediately before the bytes + /// being handed over now. The caller writes a line saying so, and it has to — the alternative is a + /// terminal quietly showing a different transcript from the one the remote sent. It is reported even + /// when nothing is taken, so a drop at the very end of a session is still announced. + /// + /// + /// Where the bytes go, oldest first. + /// Bytes discarded since the previous take, and zero on the ordinary path. + /// How many bytes were written, which is zero when nothing is held. + public int TakeInto(Span destination, out long dropped) + { + lock (gate) + { + dropped = droppedSinceTake; + droppedSinceTake = 0; + + var take = Math.Min(destination.Length, buffered); + var offset = 0; + + while (offset < take) + { + var oldest = segments.Peek(); + var fromThis = Math.Min(oldest.Length - head, take - offset); + + oldest.AsSpan(head, fromThis).CopyTo(destination[offset..]); + + offset += fromThis; + head += fromThis; + buffered -= fromThis; + + if (head == oldest.Length) + { + _ = segments.Dequeue(); + head = 0; + } + } + + return take; + } + } + + /// Says there will be no more output, releasing anything waiting for some. + public void Complete() + { + lock (gate) + { + completed = true; + Signal(); + } + } + + /// Waits until there is output to take, or until there will never be any. + /// Abandons the wait. + /// once the backlog is completed and empty. + public async ValueTask WaitForOutputAsync(CancellationToken cancellationToken) + { + while (true) + { + Task signal; + + lock (gate) + { + if (buffered > 0) + { + return true; + } + + if (completed) + { + return false; + } + + signal = available.Task; + } + + await signal.WaitAsync(cancellationToken).ConfigureAwait(false); + } + } + + /// + /// Completed inside the lock so a waiter cannot miss the transition, and replaced there too so the + /// next wait is on a signal that has not already fired. Continuations run asynchronously, so nothing + /// resumes while the lock is held. + /// + private void Signal() + { + _ = available.TrySetResult(); + available = CreateSignal(); + } + + private static TaskCompletionSource CreateSignal() => + new(TaskCreationOptions.RunContinuationsAsynchronously); +} diff --git a/src/DodoSSH.Client.Terminal/TerminalFrame.cs b/src/DodoSSH.Client.Terminal/TerminalFrame.cs index 5d318a0..0b04acf 100644 --- a/src/DodoSSH.Client.Terminal/TerminalFrame.cs +++ b/src/DodoSSH.Client.Terminal/TerminalFrame.cs @@ -120,6 +120,32 @@ public enum TerminalServerOpcode : byte /// /// Clipboard = 8, + + /// + /// A line for the pane saying that output was dropped. Payload is UTF-8, already styled. + /// + /// + /// + /// Written into the pane exactly where it arrives, which is where the gap is — the socket keeps order + /// within a session, so this frame lands between the bytes before the drop and the bytes after it. + /// + /// + /// Its own opcode rather than a few bytes spliced into an frame, which is + /// what it was first, and the reason is that the two are not the same kind of thing. Output is what the + /// remote said, and this is what this application says about it. Keeping them apart means the host's + /// own sentence is never mistaken for the transcript — by anything reading a frame, and in particular + /// by the arithmetic that has to hold if dropping is to stay honest: every byte read from the remote is + /// either in an payload or counted in TerminalSessionPump.BytesDropped, and + /// a notice inside the output stream would put this process's words on the left of that equation. + /// + /// + /// Not acknowledged, and so not part of the credit window: credit accounts for what the remote sent + /// and what the renderer has drawn of it. A notice is a few dozen bytes the host chose to add, and + /// billing them to a window that is meant to describe the remote would make the number mean two + /// things. + /// + /// + OutputDropped = 9, } /// Frames the renderer sends to the host. diff --git a/src/DodoSSH.Client.Terminal/TerminalSessionPump.cs b/src/DodoSSH.Client.Terminal/TerminalSessionPump.cs index ec32919..e843eb6 100644 --- a/src/DodoSSH.Client.Terminal/TerminalSessionPump.cs +++ b/src/DodoSSH.Client.Terminal/TerminalSessionPump.cs @@ -1,6 +1,6 @@ using System.Buffers; +using System.Globalization; using System.Text; -using System.Threading.Channels; using DodoSSH.Client.Ssh; namespace DodoSSH.Client.Terminal; @@ -49,6 +49,16 @@ public sealed class TerminalPumpOptions /// How far behind the renderer may fall, in bytes. public int WindowBytes { get; init; } = CreditWindow.DefaultWindowBytes; + + /// + /// How much unsent output to hold for a renderer that is behind, before dropping the oldest. + /// + /// + /// This is the number that bounds a session's memory, and says at length + /// why it rather than is the one that does. Larger drops less and shows a + /// flood's tail later; smaller drops sooner and keeps the terminal closer to live. + /// + public int BacklogBytes { get; init; } = OutputBacklog.DefaultCapacityBytes; } /// @@ -56,27 +66,45 @@ public sealed class TerminalPumpOptions /// /// /// -/// Credit is reserved before reading, never after. That ordering is the whole design: because the pump -/// cannot read more than the renderer has room for, the coalescing buffer is bounded by the credit -/// window rather than by how fast the remote can talk. Reserving after reading would leave an -/// unbounded queue between the socket and the screen, which is the failure mode this exists to -/// prevent. +/// ◆ The pump reads the shell channel continuously and never stops, which is deliberate rather than +/// careless. It used to stop as soon as the renderer fell behind, on the reasoning that a reader +/// which does not read closes SSH's receive window, blocks the remote sshd on write, and blocks +/// the process producing the output in turn — backpressure all the way to the source, with no custom +/// protocol. That reasoning is wrong for this client's SSH library, and wrong in the direction that costs +/// memory: SSH.NET re-opens the channel window when bytes arrive rather than when they are +/// consumed, and keeps everything it has been handed in a growable array it does not give back. Not +/// reading slowed nothing down; it only moved the flood into a buffer with no ceiling. +/// carries the full account. /// /// -/// When credit runs out the pump stops reading. SSH's own receive window then closes, the remote -/// sshd blocks on write, and the process producing output blocks in turn — backpressure all the -/// way to the source, with no custom protocol. +/// So there are two bounds now, each enforced where it can be. The backlog bounds what is waiting to be +/// sent, by dropping its oldest bytes and saying so. The credit window bounds what has been sent and not +/// yet drawn — the buffer inside the WebView that nothing on this side can measure — and the flush loop +/// reserves against it before every frame, holding off while there is none. Reading is no longer part of +/// either, because reading was never what the remote was waiting on. /// /// public sealed class TerminalSessionPump : IAsyncDisposable { + /// + /// The most output one frame carries. + /// + /// + /// Under the 85,000-byte threshold at which the runtime allocates on the large object heap, and that is + /// the whole of why the number is here: a frame is allocated per send, and a session doing megabytes a + /// second would otherwise put a quarter-megabyte array on the LOH sixty times a second, which is not + /// compacted and is collected only with a generation 2 pass. Below the threshold the same traffic is + /// generation 0 garbage that costs nothing to collect. It is a ceiling rather than a target — a quiet + /// terminal sends whatever it has, which is a few bytes. + /// + private const int MaximumFrameBytes = 64 * 1024; + private readonly uint sessionId; private readonly ISshShellSession session; private readonly ITerminalTransport transport; private readonly TimeProvider clock; private readonly TerminalPumpOptions options; - private readonly Channel pending = Channel.CreateUnbounded( - new UnboundedChannelOptions { SingleReader = true, SingleWriter = true }); + private readonly OutputBacklog backlog; private readonly CancellationTokenSource lifetime = new(); private int disposed; @@ -104,6 +132,7 @@ public sealed class TerminalSessionPump : IAsyncDisposable this.options = options ?? new TerminalPumpOptions(); Credits = new CreditWindow(this.options.WindowBytes); + backlog = new OutputBacklog(this.options.BacklogBytes); } /// This session's flow-control window. @@ -112,6 +141,19 @@ public sealed class TerminalSessionPump : IAsyncDisposable /// Total bytes read from the remote, for the throughput harness. public long BytesRead { get; private set; } + /// + /// Total bytes this session discarded because the renderer could not keep up with the remote. + /// + /// + /// Zero for every ordinary session, and the number to look at first when somebody reports missing + /// output. The user is told as it happens — a dropped run puts a line in the pane rather than a gap — + /// so this is for a test or a report rather than for the interface. + /// + public long BytesDropped => backlog.DroppedTotal; + + /// Bytes read from the remote and not yet sent to the renderer. + public int BytesPending => backlog.Buffered; + /// Total frames sent to the renderer, for the throughput harness. public long FramesSent { get; private set; } @@ -152,31 +194,7 @@ public sealed class TerminalSessionPump : IAsyncDisposable var reader = ReadLoopAsync(linked.Token); var flusher = FlushLoopAsync(linked.Token); - TerminalSessionOutcome outcome; - try - { - await reader.ConfigureAwait(false); - - // The read loop finished of its own accord, which means the shell is over. Which of the two ways - // it is over is not a question asked of the transport now — by now the server has very likely hung - // up behind a clean exit, and asking would call `exit` a dropped connection — but of what the shell - // latched at the moment it happened. See ISshShellSession.TransportFailure. - outcome = session.TransportFailure is { Length: > 0 } failure - ? new TerminalSessionOutcome(SshSessionEnd.ConnectionLost, failure) - : new TerminalSessionOutcome(SshSessionEnd.RemoteClosed, "The remote closed the session."); - } - catch (OperationCanceledException) - { - outcome = new TerminalSessionOutcome(SshSessionEnd.ClosedLocally, "The session was closed."); - } - catch (Exception exception) - { - // A read that threw rather than returning zero. Whatever it was, it was not this client asking — - // that arrives as the cancellation above — so it is the connection going out from under the read. - outcome = new TerminalSessionOutcome( - SshSessionEnd.ConnectionLost, - session.TransportFailure is { Length: > 0 } failure ? failure : exception.Message); - } + var outcome = await ReadUntilItEndsAsync(reader).ConfigureAwait(false); // Before the drain and the last frame, so that anything reading this off a completed run — the // workspace's own announcement — cannot see the default. @@ -185,7 +203,17 @@ public sealed class TerminalSessionPump : IAsyncDisposable // Stop the flusher, but only after draining what the reader already produced — the last thing // a remote writes is often the most important, and dropping it makes a clean exit look like a // crash. - pending.Writer.TryComplete(); + backlog.Complete(); + + // ◆ AND RELEASE THE FLUSHER IF IT IS PARKED ON CREDIT, WHICH IS NOT THE SAME SIGNAL. Completing + // the backlog wakes anything waiting for output; it says nothing to a flush loop that has output + // and is waiting for the renderer to make room for it. Against a renderer that has stopped + // acknowledging — the page's process killed, the window gone — that acknowledgement never comes, + // and without this the await below would never return: the shell would have ended, the tab would + // never be told, and the session would never be torn down. Resetting rather than waiting is + // right because the window describes a renderer's remaining room and there is nothing left to + // protect it from: what follows is one backlog's worth of tail and then the session is over. + Credits.Reset(); try { @@ -203,6 +231,42 @@ public sealed class TerminalSessionPump : IAsyncDisposable .ConfigureAwait(false); } + /// + /// Waits for the read loop to finish, and says which of the three ways it did. + /// + /// + /// Split out of for length rather than for reuse, and it is the half of that + /// method with no side effects: three branches, one answer, nothing touched. + /// + /// The read loop's task. + private async Task ReadUntilItEndsAsync(Task reader) + { + try + { + await reader.ConfigureAwait(false); + + // The read loop finished of its own accord, which means the shell is over. Which of the two ways + // it is over is not a question asked of the transport now — by now the server has very likely hung + // up behind a clean exit, and asking would call `exit` a dropped connection — but of what the shell + // latched at the moment it happened. See ISshShellSession.TransportFailure. + return session.TransportFailure is { Length: > 0 } failure + ? new TerminalSessionOutcome(SshSessionEnd.ConnectionLost, failure) + : new TerminalSessionOutcome(SshSessionEnd.RemoteClosed, "The remote closed the session."); + } + catch (OperationCanceledException) + { + return new TerminalSessionOutcome(SshSessionEnd.ClosedLocally, "The session was closed."); + } + catch (Exception exception) + { + // A read that threw rather than returning zero. Whatever it was, it was not this client asking — + // that arrives as the cancellation above — so it is the connection going out from under the read. + return new TerminalSessionOutcome( + SshSessionEnd.ConnectionLost, + session.TransportFailure is { Length: > 0 } failure ? failure : exception.Message); + } + } + /// Forwards keystrokes to the remote. public ValueTask WriteInputAsync(ReadOnlyMemory data, CancellationToken cancellationToken) => session.WriteAsync(data, cancellationToken); @@ -227,44 +291,37 @@ public sealed class TerminalSessionPump : IAsyncDisposable // Unblocks anything waiting on credit that will now never be acknowledged. Credits.Reset(); - pending.Writer.TryComplete(); + backlog.Complete(); lifetime.Dispose(); await session.DisposeAsync().ConfigureAwait(false); } + /// + /// No credit is consulted here, and the class remark says at length why: the remote is not waiting on + /// this loop, so pausing it buys nothing and costs a buffer inside SSH.NET that grows without bound. + /// What this loop owes is to keep that buffer empty, which means reading as fast as the channel + /// delivers and putting the result somewhere with a ceiling. + /// private async Task ReadLoopAsync(CancellationToken cancellationToken) { var buffer = ArrayPool.Shared.Rent(options.ReadBufferBytes); try { - while (!cancellationToken.IsCancellationRequested) + while (true) { - await Credits.WaitForCreditAsync(cancellationToken).ConfigureAwait(false); + // Thrown rather than tested in the loop condition, and the difference is the whole of how + // this session's ending is described. RunAsync reads a read loop that RETURNS as + // the remote having closed the channel, and one that throws OperationCanceledException as + // this client having ended it. A condition on the token would leave by the first door for + // a reason that belongs to the second, and a tab the user closed would report itself as a + // shell that exited on its own. + cancellationToken.ThrowIfCancellationRequested(); - var granted = Credits.TryReserve(options.ReadBufferBytes); - if (granted == 0) - { - continue; - } - - int read; - try - { - read = await session - .ReadAsync(buffer.AsMemory(0, granted), cancellationToken) - .ConfigureAwait(false); - } - catch - { - Credits.Return(granted); - throw; - } - - // Hand back what was reserved but not used, so a short read does not permanently - // shrink the window. - Credits.Return(granted - read); + var read = await session + .ReadAsync(buffer.AsMemory(0, options.ReadBufferBytes), cancellationToken) + .ConfigureAwait(false); if (read == 0) { @@ -272,7 +329,10 @@ public sealed class TerminalSessionPump : IAsyncDisposable } BytesRead += read; - await pending.Writer.WriteAsync(buffer[..read], cancellationToken).ConfigureAwait(false); + + // Copied out of the rented buffer, because the backlog holds it until the renderer has + // room and the next read is already about to overwrite this one. + backlog.Append(buffer[..read]); } } finally @@ -283,65 +343,160 @@ public sealed class TerminalSessionPump : IAsyncDisposable private async Task FlushLoopAsync(CancellationToken cancellationToken) { - var segments = new List(); - - while (await pending.Reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false)) + while (await backlog.WaitForOutputAsync(cancellationToken).ConfigureAwait(false)) { - if (!pending.Reader.TryRead(out var first)) + if (backlog.IsCompleted) { + // The reader has finished and what is left is the tail, which goes out regardless of + // credit. Leaving by this door rather than going round again is load-bearing: with the + // backlog complete, SendWhatCreditAllowsAsync declines to wait for an acknowledgement + // that may never come, so a renderer that has stopped would put this loop into a spin — + // output to send, no credit to send it with, and a wait for more output that returns at + // once because there is some. + break; + } + + // Coalesce for one frame's worth of time before the first send, so a burst arriving at an idle + // terminal becomes a single write. Under sustained output the loop below never comes back + // here, and does not need to: credit is what paces it then, and a frame is as large as the + // renderer has room for. + await Task.Delay(options.FlushInterval, clock, cancellationToken).ConfigureAwait(false); + + await SendWhatCreditAllowsAsync(cancellationToken).ConfigureAwait(false); + } + + await DrainAsync().ConfigureAwait(false); + } + + /// Sends until the backlog is empty or the renderer has no room left. + private async Task SendWhatCreditAllowsAsync(CancellationToken cancellationToken) + { + while (true) + { + var wanted = Math.Min(backlog.Buffered, MaximumFrameBytes); + if (wanted == 0) + { + return; + } + + var granted = Credits.TryReserve(wanted); + + if (granted == 0) + { + // The renderer is behind. Nothing more goes out until it acknowledges, and meanwhile the + // backlog holds what it can and drops its oldest for what it cannot — which is the whole + // of how this session's memory stays bounded. + if (backlog.IsCompleted) + { + // Except at the end, where waiting on an acknowledgement that may never come would + // hang the session's teardown behind a renderer that has already gone. + return; + } + + await Credits.WaitForCreditAsync(cancellationToken).ConfigureAwait(false); continue; } - segments.Clear(); - segments.Add(first); + var sent = await SendFromBacklogAsync(granted, cancellationToken).ConfigureAwait(false); - // Coalesce for one frame's worth of time, then send everything at once. Whatever the - // remote produced in that window becomes a single write to the terminal. - await Task.Delay(options.FlushInterval, clock, cancellationToken).ConfigureAwait(false); - - while (pending.Reader.TryRead(out var more)) + if (granted > sent) { - segments.Add(more); + // Whatever was reserved and not used, so a backlog that shrank between the reservation + // and the take does not permanently narrow the window. Guarded rather than called with a + // zero, because returning credit wakes every waiter and replaces the signal they were + // waiting on, and this is the hot path. + Credits.Return(granted - sent); } - - await SendOutputAsync(segments, cancellationToken).ConfigureAwait(false); - } - - // The channel completed. Anything the reader wrote before finishing still has to go out. - segments.Clear(); - while (pending.Reader.TryRead(out var trailing)) - { - segments.Add(trailing); - } - - if (segments.Count > 0) - { - await SendOutputAsync(segments, CancellationToken.None).ConfigureAwait(false); } } - private async ValueTask SendOutputAsync(List segments, CancellationToken cancellationToken) + /// + /// Puts the last of the output out, regardless of credit. + /// + /// + /// The session is over by the time this runs, so there is no renderer left to protect from a flood — + /// and what remains is bounded by the backlog's own capacity rather than by anything the remote can + /// still do. Waiting on credit here instead would mean a shell that exited against a renderer which + /// had gone never finished tearing down. + /// + private async Task DrainAsync() { - var total = 0; - foreach (var segment in segments) + while (await SendFromBacklogAsync(MaximumFrameBytes, CancellationToken.None) + .ConfigureAwait(false) > 0) { - total += segment.Length; + // Until it is empty. + } + } + + /// + /// Moves up to out of the backlog and onto the wire. + /// + /// How many bytes of output were sent, which is zero once the backlog is empty. + private async ValueTask SendFromBacklogAsync(int maxBytes, CancellationToken cancellationToken) + { + // Taken straight into the frame, header space and all. A second buffer here would be an + // allocation and a copy of every byte the terminal ever shows. + var frame = new byte[TerminalFrame.HeaderLength + maxBytes]; + var taken = backlog.TakeInto(frame.AsSpan(TerminalFrame.HeaderLength), out var dropped); + + if (dropped > 0) + { + // Sent immediately before the bytes it explains, and under its own opcode — see + // TerminalServerOpcode.OutputDropped for why this is not simply spliced into the output. + await SendAsync(TerminalServerOpcode.OutputDropped, DroppedNotice(dropped), cancellationToken) + .ConfigureAwait(false); + } + + if (taken == 0) + { + return 0; } - var frame = new byte[TerminalFrame.HeaderLength + total]; TerminalFrame.Write(frame, (byte)TerminalServerOpcode.Output, sessionId, default); - var offset = TerminalFrame.HeaderLength; - foreach (var segment in segments) - { - segment.CopyTo(frame, offset); - offset += segment.Length; - } - FramesSent++; - await transport.SendAsync(frame, cancellationToken).ConfigureAwait(false); + + await transport + .SendAsync(frame.AsMemory(0, TerminalFrame.HeaderLength + taken), cancellationToken) + .ConfigureAwait(false); + + return taken; } + /// + /// The line a user sees where output was dropped. + /// + /// + /// + /// It opens with a reset rather than the colour, and that is the load-bearing part: the bytes were cut + /// at an arbitrary offset, so whatever escape sequence the remote was in the middle of has been left + /// half-said. Resetting first means the notice reads as a notice and the output after it starts from a + /// known state, instead of inheriting a colour or a mode from a sequence nobody finished. + /// + /// + /// Dim grey, matching the renderer's own banners for a session that closed and a view that + /// reconnected — this is the same kind of thing, a sentence from the application rather than from the + /// remote, and it should not read like part of the transcript. + /// + /// + private static byte[] DroppedNotice(long dropped) => + Encoding.UTF8.GetBytes(string.Create( + CultureInfo.InvariantCulture, + $"\u001b[0m\r\n\u001b[38;5;244m── {Sized(dropped)} of output was dropped; " + + $"the terminal could not keep up ──\u001b[0m\r\n")); + + /// + /// One decimal place from a megabyte up, because the number is an order of magnitude rather than an + /// accounting: what a person needs from it is whether they lost a screenful or a session's worth. + /// + private static string Sized(long bytes) => bytes switch + { + < 1024 => string.Create(CultureInfo.CurrentCulture, $"{bytes} bytes"), + < 1024 * 1024 => string.Create(CultureInfo.CurrentCulture, $"{bytes / 1024.0:0.#} KB"), + < 1024L * 1024 * 1024 => string.Create(CultureInfo.CurrentCulture, $"{bytes / (1024.0 * 1024):0.#} MB"), + _ => string.Create(CultureInfo.CurrentCulture, $"{bytes / (1024.0 * 1024 * 1024):0.#} GB"), + }; + private async ValueTask SendAsync( TerminalServerOpcode opcode, ReadOnlyMemory payload, diff --git a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs index 72962d6..ff48e12 100644 --- a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs +++ b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs @@ -209,11 +209,12 @@ public sealed class TerminalWorkspace : IAsyncDisposable /// /// /// - /// Not sessions.Count, which over-reports. Nothing removes an entry when the remote closes - /// the channel on its own — only drops the renderer registration — so - /// a session whose shell exited half an hour ago is still in the dictionary. A completed - /// Run task is what "the shell is gone" actually looks like: the pump's loops have finished - /// and it has already sent SessionClosed to the renderer. + /// Not sessions.Count, which over-reports. Nothing removes an entry when the remote closes the + /// channel on its own — gives back what that session was holding but leaves + /// the entry, so the tab can go on saying it ended — and a session whose shell exited half an hour ago + /// is therefore still in the dictionary. A completed Run task is what "the shell is gone" + /// actually looks like: the pump's loops have finished and it has already sent SessionClosed to + /// the renderer. /// /// /// This exists because the shell shows the number on the unlock screen, and a lock screen that @@ -642,6 +643,13 @@ public sealed class TerminalWorkspace : IAsyncDisposable /// A session id this workspace does not know is ignored rather than throwing. The caller is a tab /// strip, and a tab that closed while a key was in flight is ordinary rather than exceptional. /// + /// + /// A session whose shell has ended is ignored the same way, and has to be: the tab is still on + /// screen and still selectable, so an accessory key pressed at one is as ordinary as a key pressed at a + /// tab that has closed. It is also the one of the two that would throw rather than fail quietly, since + /// disposes the shell channel the moment the remote hangs up. Liveness + /// rather than mere presence, which is the same question answers. + /// /// /// The terminal to type into. /// Raw bytes, already encoded as the remote expects them. @@ -655,7 +663,9 @@ public sealed class TerminalWorkspace : IAsyncDisposable lock (sessionGate) { - pump = sessions.TryGetValue(sessionId, out var session) ? session.Pump : null; + pump = sessions.TryGetValue(sessionId, out var session) && !session.Run.IsCompleted + ? session.Pump + : null; } return pump is null ? ValueTask.CompletedTask : pump.WriteInputAsync(data, cancellationToken); @@ -848,14 +858,79 @@ public sealed class TerminalWorkspace : IAsyncDisposable // tab, the remote hanging up, and the process shutting down. Every one of them arrives here as // the pump unwinding, which is why CloseSessionAsync needs no call of its own — and why this // must not do any work: it is running on a thread-pool thread inside DisposeAsync's loop when - // the application is closing. + // the application is closing, and everything inside a finally is time the run task has not + // completed yet, which is time LiveSessionCount still counts this session as running. // - // SessionEnded is deliberately NOT raised from here, and it used to be — see - // AnnounceEndedAsync for what was wrong with that. + // Neither SessionEnded nor the release of what the session held is done from here, and both + // used to be candidates for it — see AnnounceEndedAsync, which is where they ended up and why. ConnectionLog?.Closed(sessionId, clock.GetUtcNow()); } } + /// + /// Gives back everything a session that ended on its own was still holding. + /// + /// + /// + /// ◆ Nothing used to do this, and a shell that exited went on costing what a running one costs. + /// The entry stayed in until the user closed the tab, and with it an undisposed + /// — an authenticated socket to the remote, its cipher state, and a + /// keep-alive still putting a packet on the wire every thirty seconds — plus the pump and, underneath + /// it, SSH.NET's read buffer at whatever size the session's heaviest moment had grown it to. A tab + /// somebody typed exit into an hour ago and left open was indistinguishable from a live one in + /// everything except what it drew, and a working day of those is the memory a user actually notices. + /// + /// + /// The dictionary entry stays even so, and that is not an oversight. + /// , and + /// 's containment check are all written against it — a session that + /// ended still has to be able to say so, and a tab that shows ENDED and offers a reconnect is reading + /// exactly that. What goes is everything the entry was holding that costs anything; what is left is + /// three references and a completed task. + /// + /// + /// Skipped entirely when the entry has already been removed, which is precisely the deliberate path: + /// takes the entry out before it disposes anything, so the two can + /// never both be tearing the same session down. That is also what makes this a no-op during + /// , which closes every session by that route before anything here could run. + /// + /// + /// Every failure is swallowed. The caller is a fire-and-forget continuation with nobody to hand a fault + /// to, and the shell it would be about is already gone. + /// + /// + private async Task ReleaseAsync(uint sessionId) + { + LiveSession? session; + + lock (sessionGate) + { + if (!sessions.TryGetValue(sessionId, out session)) + { + return; + } + } + + try + { + // The connection goes even if the pump's disposal throws, for the reason CloseSessionAsync + // gives at the same pair of calls: an undisposed ISshConnection is a live authenticated + // channel this process has forgotten about. + try + { + await session.Pump.DisposeAsync().ConfigureAwait(false); + } + finally + { + await session.Connection.DisposeAsync().ConfigureAwait(false); + } + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + // Best-effort. The shell is already gone; nothing a caller could do with this. + } + } + /// Announces a session's end once its run task has actually completed. /// The session, as the renderer knows it. /// @@ -898,12 +973,25 @@ public sealed class TerminalWorkspace : IAsyncDisposable announce = sessions.ContainsKey(sessionId); } - if (announce) + if (!announce) + { + return; + } + + try { // Read after the await, which is the only point at which the pump has settled it — see // TerminalSessionPump.Outcome. SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId, pump.Outcome)); } + finally + { + // After the announcement rather than before it, and in a finally so a handler that throws + // cannot leave a connection open. Tearing down an SSH connection is a socket closing and a + // library shutting down its listener; doing that first would put it between the shell ending + // and the tab being told, which is latency on the one thing the user is watching for. + await ReleaseAsync(sessionId).ConfigureAwait(false); + } } /// The address as dialled, for the log. diff --git a/tests/DodoSSH.Client.Ssh.Tests/PumpOverRealSshTests.cs b/tests/DodoSSH.Client.Ssh.Tests/PumpOverRealSshTests.cs index ef1dd87..0a35473 100644 --- a/tests/DodoSSH.Client.Ssh.Tests/PumpOverRealSshTests.cs +++ b/tests/DodoSSH.Client.Ssh.Tests/PumpOverRealSshTests.cs @@ -162,6 +162,84 @@ public sealed class PumpOverRealSshTests(SshServerFixture fixture) "the strip under the terminal prints this sentence, so an empty one is a blank status line"); } + /// + /// A remote flooding a renderer that never draws anything is bounded, and the pump keeps reading. + /// + /// + /// + /// ◆ THIS IS THE TEST THE OLD DESIGN WOULD FAIL, AND IT NEEDS A REAL SSH.NET UNDERNEATH IT TO SAY SO. + /// The pump used to stop reading once a renderer had a credit window's worth of unrendered output, on + /// the reasoning that SSH's own receive window would then close and block the remote. It does not: + /// Renci.SshNet.Channels.Channel.OnData adjusts the window as bytes arrive, before it + /// hands them on, so the remote goes on sending at full speed and ShellStream copies every byte + /// into a buffer that doubles and is never given back. Against this test, yes would have been + /// read at BytesRead == WindowBytes exactly and then stopped for ever while the process grew. + /// A fake shell cannot show that, because a fake hands over bytes when it is asked and a real channel + /// hands them over when they arrive. + /// + /// + /// So the assertion is in two halves and needs both. Reading keeps going, which is what keeps SSH.NET's + /// buffer drained; and what this side holds stays inside the backlog, which is what keeps the drained + /// bytes from simply piling up here instead. Either alone is satisfied by a design that leaks. + /// + /// + [Fact] + public async Task ARunawayRemote_KeepsBeingReadAndStaysInsideTheBacklog() + { + const int Window = 64 * 1024; + const int Backlog = 512 * 1024; + + await using var connection = await ConnectTrustedAsync(); + + var shell = await connection.OpenShellAsync( + new TerminalSize(100, 30, 1000, 750), TestContext.Current.CancellationToken); + + // Deliberately never acknowledges: this is a renderer that has stopped drawing, which is the whole + // condition under examination. CountingTransport only acknowledges when it is given a pump. + var transport = new CountingTransport(); + + await using var pump = new TerminalSessionPump( + 1, + shell, + transport, + TimeProvider.System, + new TerminalPumpOptions + { + FlushInterval = TimeSpan.FromMilliseconds(20), + WindowBytes = Window, + BacklogBytes = Backlog, + }); + + var run = pump.RunAsync(TestContext.Current.CancellationToken); + + await pump.WriteInputAsync("yes dodossh\n"u8.ToArray(), TestContext.Current.CancellationToken); + + // Long enough that a pump which had stopped at the window would plainly have stopped, and that one + // which had not would have read many times the backlog's capacity. + var deadline = TimeProvider.System.GetUtcNow() + TimeSpan.FromSeconds(10); + + while (TimeProvider.System.GetUtcNow() < deadline && pump.BytesRead < Backlog * 4L) + { + await Task.Delay(100, TestContext.Current.CancellationToken); + + pump.BytesPending.ShouldBeLessThanOrEqualTo( + Backlog, + "output is accumulating on this side of the socket, which is the growth the backlog bounds"); + } + + pump.BytesRead.ShouldBeGreaterThan( + Backlog * 4L, + "the pump stopped reading, so the flood is piling up inside SSH.NET where nothing bounds it"); + + pump.BytesDropped.ShouldBeGreaterThan( + 0, + "nothing was dropped, so something between here and the screen is still holding all of it"); + + await pump.DisposeAsync(); + + await Should.NotThrowAsync(() => run.WaitAsync(TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken)); + } + /// Trusts the relay's identity, then connects through it. /// /// Pinned under the relay's own address, which is the honest thing here: what is under examination is a diff --git a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs index bd6661c..430ca84 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs @@ -9,6 +9,7 @@ internal sealed class FakeShellSession : ISshShellSession private readonly Lock gate = new(); private readonly bool blockReads; + private readonly TimeSpan? pacePerRead; private long remaining; private byte pattern; @@ -26,10 +27,26 @@ internal sealed class FakeShellSession : ISshShellSession /// 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) + /// + /// How long each read takes, or null for one that returns as fast as the CPU can fill a buffer. + /// + /// + /// + /// The default producer emits at memory bandwidth, which is faster than any network and faster than + /// the pump can frame and send — useful for the throughput harness, and useless for asking whether a + /// terminal that can keep up loses anything, because nothing can keep up with it. A pace makes + /// the fake a link instead of a memcpy: 8 KiB every millisecond is 8 MB/s, which is a fast remote + /// rather than an impossible one. + /// + /// + internal FakeShellSession( + long bytesToProduce = 0, + bool blockReads = false, + TimeSpan? pacePerRead = null) { remaining = bytesToProduce; this.blockReads = blockReads; + this.pacePerRead = pacePerRead; } /// @@ -81,6 +98,11 @@ internal sealed class FakeShellSession : ISshShellSession await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken).ConfigureAwait(false); } + if (pacePerRead is { } pace) + { + await Task.Delay(pace, cancellationToken).ConfigureAwait(false); + } + await Task.Yield(); cancellationToken.ThrowIfCancellationRequested(); @@ -212,6 +234,40 @@ internal sealed class FakeConnection(SshConnectionRequest request, long bytesPer } } +/// +/// Counts output without keeping it, and acknowledges it as a renderer that is keeping up would. +/// +/// +/// 's cheap sibling, and the throughput harness needs it to be the one +/// measuring rather than the one measured: recording copies every frame into a list that grows to the size +/// of the whole run, which at the rates that harness drives makes the test double the slowest thing +/// in the pipeline. A harness whose consumer cannot keep up is measuring itself. +/// +internal sealed class CountingTransport : ITerminalTransport +{ + private long outputBytes; + + /// Set to acknowledge every output frame immediately, as a keeping-up renderer would. + internal TerminalSessionPump? AutoAcknowledge { get; set; } + + /// Payload bytes of every output frame sent so far. + internal long OutputBytes => Interlocked.Read(ref outputBytes); + + /// + public ValueTask SendAsync(ReadOnlyMemory frame, CancellationToken cancellationToken) + { + if (TerminalFrame.TryRead(frame.Span, out var opcode, out _, out var payload) + && opcode == (byte)TerminalServerOpcode.Output) + { + _ = Interlocked.Add(ref outputBytes, payload.Length); + + AutoAcknowledge?.Acknowledge((uint)payload.Length); + } + + return ValueTask.CompletedTask; + } +} + /// Records frames, and can acknowledge them to keep credit flowing. internal sealed class RecordingTransport : ITerminalTransport { @@ -270,6 +326,23 @@ internal sealed class RecordingTransport : ITerminalTransport return [.. output]; } + /// Concatenated payloads of every frame of one opcode. + internal byte[] PayloadsOf(TerminalServerOpcode opcode) + { + var payloads = new List(); + + foreach (var frame in Frames) + { + if (TerminalFrame.TryRead(frame, out var actual, out _, out var payload) + && actual == (byte)opcode) + { + payloads.AddRange(payload); + } + } + + return [.. payloads]; + } + /// Counts frames of one opcode. internal int CountOf(TerminalServerOpcode opcode) => Frames.Count(frame => diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalSessionPumpTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalSessionPumpTests.cs index 0220fe0..8441e54 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalSessionPumpTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalSessionPumpTests.cs @@ -1,5 +1,6 @@ using System.Diagnostics; using System.Globalization; +using System.Text; using DodoSSH.Client.Ssh; namespace DodoSSH.Client.Terminal.Tests; @@ -84,13 +85,22 @@ public sealed class TerminalSessionPumpTests await run; } + /// + /// A renderer that has stopped acknowledging does not stop the pump reading, and does not grow it. + /// + /// + /// ◆ THIS TEST REPLACES ONE THAT ASSERTED THE OPPOSITE, and the reversal is the point rather than a + /// relaxation. The old contract was that the pump reads exactly one credit window and then stops, + /// which was believed to close SSH's receive window and block the remote. It does not — see + /// — so all the old behaviour bought was an unbounded buffer inside + /// SSH.NET instead of a bounded one here. What has to be true now is both halves at once: reading + /// continues, so nothing piles up underneath, and what is held stays inside the backlog. + /// [Fact] - public async Task WhenCreditRunsOut_ThePumpStopsReading() + public async Task WhenCreditRunsOut_ThePumpKeepsReadingAndTheBacklogHoldsTheLine() { - // The property the whole design rests on. With no acknowledgement the pump must read exactly - // the window and then stop, so nothing accumulates between the socket and the screen. Left - // unbounded, a remote running `yes` grows the client's memory until it dies. const int Window = 4 * 1024; + const int Backlog = 64 * 1024; await using var session = new FakeShellSession(bytesToProduce: long.MaxValue); var transport = new RecordingTransport(); @@ -104,19 +114,32 @@ public sealed class TerminalSessionPumpTests ReadBufferBytes = 1024, FlushInterval = TimeSpan.FromMilliseconds(5), WindowBytes = Window, + BacklogBytes = Backlog, }); var run = pump.RunAsync(TestContext.Current.CancellationToken); await Task.Delay(TimeSpan.FromMilliseconds(300), TestContext.Current.CancellationToken); - pump.BytesRead.ShouldBe( - Window, - "The pump read past its credit window, so output accumulates without bound."); + var readByNow = pump.BytesRead; - // Still stalled a moment later, rather than merely slow. + readByNow.ShouldBeGreaterThan( + Window, + "The pump stopped reading, which leaves the flood accumulating inside SSH.NET instead."); + + pump.BytesPending.ShouldBeLessThanOrEqualTo( + Backlog, + "The backlog grew past its capacity, which is the unbounded growth it exists to prevent."); + + pump.BytesDropped.ShouldBeGreaterThan( + 0, + "Nothing was dropped, so something is still holding what the renderer never took."); + + // Still reading a moment later, rather than having quietly stalled once the backlog filled. await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); - pump.BytesRead.ShouldBe(Window); + + pump.BytesRead.ShouldBeGreaterThan(readByNow); + pump.BytesPending.ShouldBeLessThanOrEqualTo(Backlog); pump.Credits.Available.ShouldBe(0); @@ -125,10 +148,13 @@ public sealed class TerminalSessionPumpTests } [Fact] - public async Task AcknowledgingCredit_LetsThePumpResume() + public async Task AcknowledgingCredit_LetsMoreOutputThrough() { const int Window = 4 * 1024; + // Endless, so the backlog never completes while this is being measured: once it does, the tail + // goes out regardless of credit and there is nothing for the window to hold back. Drops along the + // way change which bytes are sent, never how many — and the notice they raise is its own opcode. await using var session = new FakeShellSession(bytesToProduce: long.MaxValue); var transport = new RecordingTransport(); @@ -145,13 +171,16 @@ public sealed class TerminalSessionPumpTests var run = pump.RunAsync(TestContext.Current.CancellationToken); await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); - pump.BytesRead.ShouldBe(Window); + + // One window's worth has gone out and stopped there, because nothing has been rendered. + var sentBeforeAcknowledgement = transport.OutputBytes().Length; + sentBeforeAcknowledgement.ShouldBe(Window); pump.Acknowledge(2048); await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); - pump.BytesRead.ShouldBe(Window + 2048); + transport.OutputBytes().Length.ShouldBe(sentBeforeAcknowledgement + 2048); await pump.DisposeAsync(); await run; @@ -161,9 +190,10 @@ public sealed class TerminalSessionPumpTests public async Task AnOverLargeAcknowledgement_DoesNotWidenTheWindow() { // The acknowledgement comes from JavaScript. A page claiming to have rendered a gigabyte must - // not be able to talk the host into an unbounded read. + // not be able to talk the host into filling its own buffer without limit. const int Window = 4 * 1024; + // Endless for the reason the test above gives: a backlog that completes stops the window mattering. await using var session = new FakeShellSession(bytesToProduce: long.MaxValue); var transport = new RecordingTransport(); @@ -185,7 +215,8 @@ public sealed class TerminalSessionPumpTests await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); // One further window's worth at most, never everything the endless producer could offer. - pump.BytesRead.ShouldBeLessThanOrEqualTo(Window * 2); + transport.OutputBytes().Length.ShouldBeLessThanOrEqualTo(Window * 2); + pump.Credits.Outstanding.ShouldBeLessThanOrEqualTo(Window); await pump.DisposeAsync(); await run; @@ -344,7 +375,11 @@ public sealed class TerminalSessionPumpTests const int Window = CreditWindow.DefaultWindowBytes; await using var session = new FakeShellSession(bytesToProduce: Target); - var transport = new RecordingTransport(); + + // Counting rather than recording, for the reason CountingTransport carries: a consumer that + // copies every frame into a 32 MB list is the slowest thing in this pipeline, and a harness + // whose consumer cannot keep up measures the harness. + var transport = new CountingTransport(); await using var pump = CreatePump( session, @@ -369,7 +404,15 @@ public sealed class TerminalSessionPumpTests var peakOutstanding = await sampler; pump.BytesRead.ShouldBe(Target); - transport.OutputBytes().Length.ShouldBe((int)Target); + + // ◆ Every byte read either reached the renderer or was counted as dropped, and none of it went + // anywhere else. That is the invariant worth asserting here rather than "nothing was dropped": + // this producer emits at memory bandwidth with no network under it, which is faster than any + // real remote and faster than the pump can frame and send, so drops are the honest outcome and + // the harness would be lying to forbid them. What must never happen is a byte that is neither + // delivered nor accounted for — see NothingIsDropped_WhenTheRendererKeepsUpWithTheRemote for + // the case where the producer is one a network could actually deliver. + (transport.OutputBytes + pump.BytesDropped).ShouldBe(Target); var megabytesPerSecond = Target / 1024d / 1024d / stopwatch.Elapsed.TotalSeconds; @@ -387,6 +430,142 @@ public sealed class TerminalSessionPumpTests $"Peak in-flight was {peakOutstanding} bytes against a {Window}-byte window.")); } + /// + /// A renderer that keeps up loses nothing, however much the remote sends. + /// + /// + /// The other half of the trade the backlog makes, and the half a user actually lives in: dropping is + /// for a terminal that cannot keep up, and a terminal that can must show every byte. The producer here + /// is sized for a real link rather than for memory bandwidth — several times the backlog, delivered in + /// pieces a network could carry — so a drop would mean the pump losing output it had room for. + /// + [Fact] + public async Task NothingIsDropped_WhenTheRendererKeepsUpWithTheRemote() + { + const int Backlog = 256 * 1024; + const long Target = 512L * 1024; + + // 8 KiB a millisecond — 8 MB/s, a fast remote rather than an impossible one, and thirty-two + // times the backlog's capacity per second so the pump has no room to be merely lucky. + await using var session = new FakeShellSession( + bytesToProduce: Target, + pacePerRead: TimeSpan.FromMilliseconds(1)); + + var transport = new CountingTransport(); + + await using var pump = CreatePump( + session, + transport, + new TerminalPumpOptions + { + ReadBufferBytes = 8 * 1024, + FlushInterval = TimeSpan.FromMilliseconds(1), + WindowBytes = CreditWindow.DefaultWindowBytes, + BacklogBytes = Backlog, + }); + + transport.AutoAcknowledge = pump; + + await pump.RunAsync(TestContext.Current.CancellationToken); + + pump.BytesDropped.ShouldBe(0); + transport.OutputBytes.ShouldBe(Target); + } + + /// + /// A shell that ends against a renderer which stopped acknowledging still finishes. + /// + /// + /// ◆ THE HANG THIS DESIGN MAKES POSSIBLE, AND THE TEST THAT SAYS IT DOES NOT HAPPEN. The flush loop + /// waits on credit when the renderer is behind, and completing the backlog is a different signal from + /// returning credit — so a run whose reader finished while the flusher was parked on an + /// acknowledgement that will never come would wait for ever. Nothing above would notice: the shell has + /// ended, but the tab is told by SessionEnded, which fires off the run task completing, and the + /// connection is given back at the same moment. A session in that state is invisible and permanent. + /// + [Fact] + public async Task WhenTheRemoteCloses_TheRunEndsEvenIfTheRendererNeverAcknowledged() + { + const int Window = 4 * 1024; + + // More than the window, so the flusher has certainly run out of credit and parked by the time the + // producer reaches the end of what it has to give. + await using var session = new FakeShellSession(bytesToProduce: Window * 4); + + // No AutoAcknowledge, ever: this renderer's process died holding the socket. + var transport = new RecordingTransport(); + + await using var pump = CreatePump( + session, + transport, + new TerminalPumpOptions + { + ReadBufferBytes = 1024, + FlushInterval = TimeSpan.FromMilliseconds(5), + WindowBytes = Window, + }); + + var run = pump.RunAsync(TestContext.Current.CancellationToken); + + await Should.NotThrowAsync(() => run.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken)); + + pump.Outcome.End.ShouldBe(SshSessionEnd.RemoteClosed); + + // And the tail went out rather than being abandoned with the flusher: the last thing a remote + // writes is usually why it ended. + transport.OutputBytes().Length.ShouldBe(Window * 4); + transport.CountOf(TerminalServerOpcode.SessionClosed).ShouldBe(1); + } + + /// + /// A drop is announced in the pane rather than leaving a silent gap. + /// + /// + /// The thing that makes dropping defensible at all. A terminal quietly showing a different transcript + /// from the one the remote sent is worse than one that says it could not keep up, because the second + /// can be believed. Its own opcode, so the sentence is never counted as something the remote said — + /// see . + /// + [Fact] + public async Task WhenOutputIsDropped_TheUserIsToldInThePane() + { + const int Window = 4 * 1024; + const int Backlog = 16 * 1024; + + await using var session = new FakeShellSession(bytesToProduce: long.MaxValue); + var transport = new RecordingTransport(); + + await using var pump = CreatePump( + session, + transport, + new TerminalPumpOptions + { + ReadBufferBytes = 1024, + FlushInterval = TimeSpan.FromMilliseconds(5), + WindowBytes = Window, + BacklogBytes = Backlog, + }); + + var run = pump.RunAsync(TestContext.Current.CancellationToken); + + await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); + + // Enough credit for one more frame, which is what carries the notice out. + pump.Acknowledge((uint)Window); + + await Task.Delay(TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); + + transport.CountOf(TerminalServerOpcode.OutputDropped).ShouldBeGreaterThan(0); + + var notice = Encoding.UTF8.GetString(transport.PayloadsOf(TerminalServerOpcode.OutputDropped)); + + notice.ShouldContain("of output was dropped"); + notice.ShouldStartWith("\u001b[0m"); + + await pump.DisposeAsync(); + await run; + } + /// Watches in-flight bytes for the duration of a run, returning the highest seen. private static async Task SamplePeakOutstandingAsync( TerminalSessionPump pump, diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs index 0d069af..82ece61 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs @@ -83,9 +83,9 @@ public sealed class TerminalWorkspaceTests } /// - /// The case a naive sessions.Count gets wrong. Nothing removes the entry when the remote - /// closes the channel by itself — the session is still in the dictionary, holding a connection that - /// is finished — so counting entries would report a shell that exited as still running. A user + /// The case a naive sessions.Count gets wrong. Nothing removes the entry when the remote closes + /// the channel by itself — what it held is given back, but the entry stays so the tab can go on saying + /// the session ended — so counting entries would report a shell that exited as still running. A user /// deciding whether it is safe to walk away is the person that lie is told to. /// [Fact] @@ -645,6 +645,86 @@ public sealed class TerminalWorkspaceTests await workspace.SendInputAsync(sessionId, "x"u8.ToArray(), TestContext.Current.CancellationToken)); } + /// + /// A shell that ended on its own gives its connection back at once, without waiting for the tab. + /// + /// + /// ◆ THE HALF THAT WAS MISSING. Closing a tab disposed the connection and always had; a remote hanging + /// up did not, so a session that ended went on holding an authenticated socket, a keep-alive putting a + /// packet on the wire every thirty seconds, and SSH.NET's read buffer at whatever size the session's + /// busiest moment had grown it to — for as long as the user left the tab open, which for a tab showing + /// the last thing a job said is indefinitely. + /// + [Fact] + public async Task ASessionWhoseRemoteHasExited_GivesBackItsConnectionWithoutWaitingForTheTab() + { + // A shell with no output to give: its first read returns 0, which is the remote closing the + // channel, so the pump finishes with nobody having asked it to. + var connections = new FakeConnectionFactory(bytesPerShell: 0); + + await using var workspace = CreateWorkspace(connections); + + var sessionId = await workspace.OpenSessionAsync( + Request(), TerminalSize.Default, progress: null, TestContext.Current.CancellationToken); + + await WaitUntilAsync(() => connections.Connections.TrueForAll(connection => connection.IsDisposed)); + + var connection = connections.Connections.ShouldHaveSingleItem(); + + connection.IsConnected.ShouldBeFalse(); + connection.Shell.ShouldNotBeNull().IsOpen.ShouldBeFalse(); + + // The entry itself stays, and has to: the tab is still on screen, still says it ended, and still + // offers a reconnect — all of which read this workspace rather than the tab strip's own memory. + workspace.CreditsFor(sessionId).ShouldNotBeNull(); + workspace.IsSessionLive(sessionId).ShouldBeFalse(); + } + + /// + /// The consequence of the release above, and the reason SendInputAsync asks about liveness + /// rather than presence: the shell channel is disposed the moment the remote hangs up, so an accessory + /// key pressed at a tab that ended reaches a disposed stream. It has to be as ordinary as pressing one + /// at a tab that has closed, which is to say nothing happens. + /// + [Fact] + public async Task SendingInputToASessionWhoseRemoteHasExited_IsIgnored() + { + var connections = new FakeConnectionFactory(bytesPerShell: 0); + + await using var workspace = CreateWorkspace(connections); + + var sessionId = await workspace.OpenSessionAsync( + Request(), TerminalSize.Default, progress: null, TestContext.Current.CancellationToken); + + await WaitUntilAsync(() => !workspace.IsSessionLive(sessionId)); + + await Should.NotThrowAsync(async () => + await workspace.SendInputAsync(sessionId, "x"u8.ToArray(), TestContext.Current.CancellationToken)); + } + + /// + /// Closing a tab over a session that already released itself must still work, because that is the + /// ordinary way a dead tab goes away. Everything the close does is idempotent by construction — the + /// entry is removed under the lock, and both disposals are guarded — and this is the test that says so + /// rather than leaving it to be inferred from three separate classes. + /// + [Fact] + public async Task ClosingATabOverASessionThatAlreadyEnded_IsStillSafe() + { + var connections = new FakeConnectionFactory(bytesPerShell: 0); + + await using var workspace = CreateWorkspace(connections); + + var sessionId = await workspace.OpenSessionAsync( + Request(), TerminalSize.Default, progress: null, TestContext.Current.CancellationToken); + + await WaitUntilAsync(() => !workspace.IsSessionLive(sessionId)); + + await Should.NotThrowAsync(() => workspace.CloseSessionAsync(sessionId)); + + workspace.CreditsFor(sessionId).ShouldBeNull(); + } + /// /// How connections are made. The renderer-gate tests never reach it — they stop at the gate — but they /// take a fake anyway, because reaching a real host from here would make this a network test.