From ef12e8cc99151b6f7ade9f654a654420cce1b073 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 14 Aug 2026 15:44:03 +0200 Subject: [PATCH] Stop the data plane's tests racing the socket they just connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI went red on the run that added the renderer tests, and not on anything they assert: TerminalDataPlaneTests.Output_ReachesTheRenderer read the output frame where it expected the session's opening one, having lost a race that has been in the helper since it was written. Connected and attached are two different moments. ClientWebSocket.ConnectAsync completes on the 101, which UpgradeAsync writes before it has a WebSocket to attach — it builds one from the stream and swaps it in a few instructions later, on the accept thread. SendAsync drops anything sent in between, which is the transport's documented contract rather than a bug: there is 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 helper that returned on the handshake and let its caller send immediately was betting on thread scheduling, every run, on every test in the file. It only started losing now because that assembly grew nine tests and a JavaScript engine to run them in, which is more work in parallel with a window measured in instructions. The race is older than the branch that exposed it. ConnectAsync now waits for the plane's own SocketAttached, subscribed before the connection because the event can be over before ConnectAsync returns, and bounded so a socket that never attaches fails the helper instead of hanging the suite in a later receive. TerminalWorkspaceTests.ConnectRendererAsync has the same exposure through OpenSessionAsync's opening frame and now waits on WaitForRendererAsync, with a note on why that is enough for the reattach tests and what would stop being enough. ◆ NOTHING IN src CHANGED, AND THAT IS THE CONCLUSION RATHER THAN THE SHORTCUT. Production waits for exactly this moment already — every path that opens a session goes through TerminalWorkspace.WaitForRendererAsync, which resolves from the same few lines that raise the event this helper now waits on. Only the tests skipped the gate the application does not. Verified by widening the window rather than by hunting the flake: a 100ms delay inserted between the 101 and the attach, in a throwaway tree, hangs the old helper outright — both frames dropped, the test blocked in receive — and passes 89/89 with this one. Green three times over on the CI platform besides (Alpine, musl, Release, dotnet/sdk:10.0-alpine). --- .../TerminalDataPlaneTests.cs | 44 +++++++++++++++++++ .../TerminalWorkspaceTests.cs | 18 ++++++++ 2 files changed, 62 insertions(+) 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 {