diff --git a/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs b/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs index 50bdd91..6e88232 100644 --- a/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs +++ b/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs @@ -66,7 +66,6 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable new(TaskCreationOptions.RunContinuationsAsynchronously); private WebSocket? socket; - private int accepted; private int disposed; /// Where the renderer's files come from. @@ -106,6 +105,26 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable /// public event EventHandler? FontSizeStepRequested; + /// + /// Raised after a socket attaches — the first one, and every later takeover. + /// + /// + /// + /// Raised after has been swapped in but before starts + /// consuming it, on the socket-accept thread — the same thread that is in the middle of + /// for this connection. is this event's one + /// subscriber, and it uses the ordering to replay session state before anything the fresh page sends + /// (a resize, an early acknowledgement) can be dispatched; see its remark for why the two racing is + /// harmless regardless. + /// + /// + /// Unlike , which resolves once and answers "has a renderer ever attached" + /// for , this fires every time — because a takeover + /// is exactly the case was never meant to describe again. + /// + /// + public event EventHandler? SocketAttached; + /// Registers a session so inbound frames can be routed to it. public void Register(uint sessionId, TerminalSessionPump pump) { @@ -148,8 +167,9 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable // Each connection on its own task, and deliberately not awaited. An upgraded WebSocket // lives for the whole session, so handling connections in sequence would leave the accept // loop parked inside the receive loop and every later request unanswered — the page's - // script and stylesheet among them. Concurrency needs no coordination here because the - // single-attach guard is an interlocked exchange. + // script and stylesheet among them. Two upgrades racing each other need no coordination + // here either, because the takeover in UpgradeAsync swaps the shared socket field with an + // interlocked exchange rather than assuming it is the only writer. _ = HandleConnectionAsync(client, linked.Token); } } @@ -192,6 +212,29 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable .SendAsync(frame, WebSocketMessageType.Binary, endOfMessage: true, cancellationToken) .ConfigureAwait(false); } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Not a cancellation despite the type: .NET's ManagedWebSocket wraps a send that fails because + // the underlying connection is already gone — which is exactly what a killed renderer's socket + // looks like — in an OperationCanceledException of its own manufacture, regardless of whether + // anyone actually cancelled anything. The filter is what tells the two apart: if the caller's + // own token were the cause, IsCancellationRequested would be true here and this catch does not + // apply, so a real cancellation still propagates. Everything below about why this must not + // fault the caller applies here exactly as it does to the exception types in the next catch. + } + catch (Exception exception) + when (exception is WebSocketException or ObjectDisposedException + or InvalidOperationException or IOException) + { + // The state check above is not atomic with the send, and a WebView renderer process killed by + // Android leaves its socket reporting Open long after nobody is reading from the other end. This + // has to read as "nobody listening" — the same as the no-socket case above — and never as a + // fault: SendAsync is called from TerminalSessionPump.SendOutputAsync inside the flush loop, and + // letting this exception escape would fault that loop. A faulted flush loop stops draining the + // credit window, the reader blocks once it fills, and the SSH session behind it freezes for good + // while LiveSessionCount still counts it as running. A dropped frame is recoverable — a frozen + // session is not. + } finally { sendGate.Release(); @@ -267,6 +310,25 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable .ConfigureAwait(false); } + /// + /// + /// Takeover, not rejection. A valid upgrade always wins the socket, even when one is already + /// attached — the old socket is aborted and the newcomer takes its place. Refusing a second attach used + /// to be the rule, on the theory that one renderer lives for the whole process. That is WebView2's + /// truth and not Android's: the platform kills the WebView's renderer process under memory pressure or + /// simply for being backgrounded, the page reloads, and the reload's socket is a second valid upgrade — + /// refusing it left the terminal permanently unreachable with no way back short of restarting the app. + /// + /// + /// Refusing protects nothing here anyway: only our own page knows the token (see the type-level remark + /// on what the token defends against), so a second valid upgrade is our page, reattaching. + /// Waiting for the old socket to notice it is dead and close on its own is not a safer alternative + /// either — a killed renderer process sends no TCP FIN, so the old receive loop can sit unaware for the + /// whole 30-second keepalive interval, and every reload landing in that window would still find the + /// door held shut by a socket nobody is on the other end of. Taking over immediately is what makes a + /// reload actually reattach. + /// + /// private async Task UpgradeAsync( Stream stream, HttpRequestLine request, @@ -280,16 +342,6 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable return; } - if (Interlocked.Exchange(ref accepted, 1) == 1) - { - // One renderer, one socket. A second attach would be either a bug or something else on the - // machine having found the port. - await WriteResponseAsync( - stream, "409 Conflict", "text/plain", "Already attached"u8.ToArray(), cancellationToken) - .ConfigureAwait(false); - return; - } - var key = request.Headers.GetValueOrDefault("sec-websocket-key")!; var accept = ComputeHandshakeAccept(key); @@ -314,10 +366,28 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable KeepAliveInterval = TimeSpan.FromSeconds(30), }); - socket = webSocket; - rendererAttached.TrySetResult(); + // Whatever was attached before is displaced, not merely overwritten: Exchange hands back the old + // reference so it can be aborted rather than left to linger as a socket nothing reads from again. + // Abort rather than a graceful close — a close frame would wait on a peer that, per the remark + // above, may never notice it should reply, and the newcomer already proved it is our page. + var previous = Interlocked.Exchange(ref socket, webSocket); + previous?.Abort(); - await ReceiveLoopAsync(webSocket, cancellationToken).ConfigureAwait(false); + rendererAttached.TrySetResult(); + SocketAttached?.Invoke(this, EventArgs.Empty); + + try + { + await ReceiveLoopAsync(webSocket, cancellationToken).ConfigureAwait(false); + } + finally + { + // Cleared only if the field still holds this connection's own socket. A takeover has already + // swapped in a newer one by the time an aborted receive loop unwinds to here, and clearing the + // field regardless would race the newcomer: whichever of the two finished last would win, and + // it must always be the newcomer, never this one going away. + Interlocked.CompareExchange(ref socket, null, webSocket); + } } /// diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs index 1c0df1f..6d3ac4d 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs @@ -142,15 +142,96 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable await ConnectAsync(origin: "https://evil.example")); } + /// + /// The truth this replaced: a second valid attach used to be a 409, on the theory that one renderer + /// lives for the whole process. Android's WebView does not honour that theory — its renderer process is + /// routinely killed and the page reloads with a fresh socket — so a second valid attach is now a + /// takeover. This asserts both halves: the newcomer gets the connection, and the displaced socket + /// actually goes rather than lingering as a phantom nothing is reading from. + /// [Fact] - public async Task ASecondRenderer_IsRejected() + public async Task ASecondRenderer_TakesOver_AndTheFirstSocketIsDropped() { Start(); using var first = await ConnectAsync(); first.State.ShouldBe(WebSocketState.Open); - await Should.ThrowAsync(async () => await ConnectAsync()); + using var second = await ConnectAsync(); + second.State.ShouldBe(WebSocketState.Open); + + // The first socket was aborted rather than closed gracefully — Abort skips the close handshake + // entirely, so there is no Close frame for this side to see coming. What a receive on it sees + // instead is the connection simply gone, which the client surfaces as an exception rather than as + // a state that quietly flips on its own; nothing here reads from the socket otherwise, so the + // state alone would not move. + var firstBuffer = new byte[16]; + await Should.ThrowAsync(async () => + await first.ReceiveAsync(firstBuffer.AsMemory(), TestContext.Current.CancellationToken)); + + await using var session = new FakeShellSession(bytesToProduce: 64); + await using var pump = CreatePump(session); + plane.Register(SessionId, pump); + + var run = pump.RunAsync(TestContext.Current.CancellationToken); + + var opened = await ReceiveAsync(second); + opened.Opcode.ShouldBe((byte)TerminalServerOpcode.SessionOpened); + + var output = await ReceiveAsync(second); + output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output); + output.Payload.Length.ShouldBe(64); + + await SendAsync( + second, + (byte)TerminalClientOpcode.Acknowledge, + TerminalFrame.CreateAcknowledgementPayload((uint)output.Payload.Length)); + + await run; + } + + /// + /// + /// The other half of the takeover: a renderer process that dies without a close handshake — which is + /// what a killed Android WebView actually does, no FIN, nothing — must not fault the send path. A + /// faulted send would propagate into 's flush loop and freeze a live + /// session; see 's remark for why. Disposing the client socket + /// abruptly, with no close handshake sent, is the closest this harness gets to that: the server-side + /// socket is left believing itself open until it actually tries to write to it. + /// + /// + /// Driven straight through rather than through a pump, because + /// a pump adds nothing here — the point is entirely about the transport's own contract, and a session + /// layered on top would only leave it unclear whether a passing test proved the transport never threw or + /// merely that the frames never happened to need a live socket. + /// + /// + [Fact] + public async Task SendAsync_DoesNotThrow_WhenTheAttachedRendererDiedWithoutClosing_AndAFreshAttachStillReceives() + { + Start(); + + var first = await ConnectAsync(); + first.State.ShouldBe(WebSocketState.Open); + first.Dispose(); + + // Whether this particular send lands on the OS's send buffer before the peer's absence is noticed, + // or fails immediately, is not the point — either way it must not throw. + await Should.NotThrowAsync(async () => + await plane.SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "before"u8.ToArray()), + TestContext.Current.CancellationToken)); + + using var second = await ConnectAsync(); + + await Should.NotThrowAsync(async () => + await plane.SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "after"u8.ToArray()), + TestContext.Current.CancellationToken)); + + var output = await ReceiveAsync(second); + output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output); + Encoding.UTF8.GetString(output.Payload).ShouldBe("after"); } // ---- Frames ----