diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs index 6d3ac4d..a9b40fa 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs @@ -416,6 +416,36 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable TimeProvider.System, new TerminalPumpOptions { FlushInterval = TimeSpan.FromMilliseconds(10) }); + /// + /// Opens a renderer's socket and returns once the plane is actually holding it. + /// + /// + /// + /// Connected and attached are two different moments, and the gap between them is where this used to + /// flake. ClientWebSocket.ConnectAsync completes on the 101, which + /// writes before it has a to + /// attach — it builds one from the stream and swaps it in a few instructions later, on the accept + /// thread. A frame sent in between is dropped, by design rather than by accident: the transport has + /// nowhere to put a frame for a renderer that is not there, and queueing it is the unbounded growth the + /// credit window exists to prevent. + /// + /// + /// So a test that connected and immediately expected a frame was racing that window on every run. It + /// lost one on CI — Output_ReachesTheRenderer read the output frame first and asked why it was + /// not the session's opening one, the opening one having been dropped a moment earlier — which is a + /// scheduling accident on a loaded machine and says nothing whatever about the transport. + /// + /// + /// Production does not race it and needs no change: everything that opens a session waits on + /// TerminalWorkspace.WaitForRendererAsync first, and that resolves from the same few lines this + /// event is raised from. + /// + /// + /// Subscribed before the connection rather than after it, because the event is raised on the accept + /// thread and can be over before ConnectAsync has returned here. Bounded, so that a socket that + /// never attaches fails this helper rather than hanging the suite in a later receive. + /// + /// private async Task ConnectAsync( string? token = "", string? origin = null) @@ -433,17 +463,31 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable "Origin", origin ?? string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{plane.Port}")); + var attached = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + void OnAttached(object? sender, EventArgs e) => attached.TrySetResult(); + + plane.SocketAttached += OnAttached; + try { await socket.ConnectAsync( new Uri($"ws://127.0.0.1:{plane.Port}{TerminalDataPlane.SocketPath}"), TestContext.Current.CancellationToken); + + await attached.Task.WaitAsync( + TimeSpan.FromSeconds(5), + TestContext.Current.CancellationToken); } catch { socket.Dispose(); throw; } + finally + { + plane.SocketAttached -= OnAttached; + } return socket; } diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs index 8c5736a..34c5e87 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs @@ -538,10 +538,26 @@ public sealed class TerminalWorkspaceTests 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. + /// + /// + /// And it waits for the attach rather than only for the handshake, for the reason + /// TerminalDataPlaneTests.ConnectAsync sets out at length: the 101 is written before the socket + /// is attachable, and a frame sent in between is dropped. A caller that opens a session on the socket + /// this returns and then reads its opening frame is exactly the shape that loses that race. + /// + /// + /// WaitForRendererAsync answers only for the first renderer ever to attach, so a second call + /// returns immediately without proving anything about the second socket. That is enough here and is + /// not luck: the only frames a second socket is given are the replay, and the replay is *caused* by the + /// attach — and the frames before it cannot be sent + /// early. The day a test sends something else down a reattached socket, this needs the data plane's own + /// SocketAttached, which the workspace does not forward today. + /// /// private static async Task ConnectRendererAsync(TerminalWorkspace workspace) { @@ -561,6 +577,8 @@ public sealed class TerminalWorkspaceTests try { await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken); + + await workspace.WaitForRendererAsync(TestContext.Current.CancellationToken); } catch {