DodoTech-Public/DodoSSH
Public Access
2
0
Fork 0
Code Issues Pull Requests 2 Actions Packages Projects Releases 2 Wiki Activity

Take a returning renderer's socket over instead of refusing it

Browse Source 
One attach per process was WebView2's truth, not Android's: the phone
kills the WebView's renderer independently of the app process, the page
reloads, and its fresh socket was answered 409 by a guard that never
reset — with no way back short of restarting the app. Only our own page
knows the token, so a second valid upgrade is that page returning; it
now displaces the old socket, which may never notice it is dead on its
own, since a killed renderer sends no FIN.

A send into the dead socket also no longer escapes as a fault. It used
to unwind the pump's flush loop, after which nothing drained the credit
window and the still-live shell froze behind it for good — including
the BCL quirk where such a send surfaces as an OperationCanceledException
nobody's token asked for.
This commit is contained in:
jaap-jan
2026-08-09 10:54:20 +02:00
parent 3977f68870
commit 095774c498
2 changed files with 169 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/DodoSSH.Client.Terminal/TerminalDataPlane.cs
+85 -15
View File
@@ -66,7 +66,6 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable
new(TaskCreationOptions.RunContinuationsAsynchronously);
private WebSocket? socket;
private int accepted;
private int disposed;
/// <param name="assets">Where the renderer's files come from.</param>
@@ -106,6 +105,26 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable
/// </remarks>
public event EventHandler<TerminalFontSizeStepEventArgs>? FontSizeStepRequested;
/// <summary>
/// Raised after a socket attaches — the first one, and every later takeover.
/// </summary>
/// <remarks>
/// <para>
/// Raised after <see cref="socket"/> has been swapped in but before <see cref="ReceiveLoopAsync"/> starts
/// consuming it, on the socket-accept thread — the same thread that is in the middle of
/// <see cref="UpgradeAsync"/> for this connection. <see cref="TerminalWorkspace"/> 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.
/// </para>
/// <para>
/// Unlike <see cref="RendererAttached"/>, which resolves once and answers "has a renderer ever attached"
/// for <see cref="TerminalWorkspace.WaitForRendererAsync"/>, this fires every time — because a takeover
/// is exactly the case <see cref="RendererAttached"/> was never meant to describe again.
/// </para>
/// </remarks>
public event EventHandler? SocketAttached;
/// <summary>Registers a session so inbound frames can be routed to it.</summary>
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);
}
/// <remarks>
/// <para>
/// <b>Takeover, not rejection.</b> 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.
/// </para>
/// <para>
/// 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 <em>is</em> 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.
/// </para>
/// </remarks>
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,11 +366,29 @@ 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();
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);
}
}
/// <remarks>
/// The origin is checked because a page in the user's browser can attempt a WebSocket connection to
tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs
+83 -2
View File
@@ -142,15 +142,96 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable
await ConnectAsync(origin: "https://evil.example"));
}
/// <remarks>
/// 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.
/// </remarks>
[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<WebSocketException>(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<Exception>(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;
}
/// <remarks>
/// <para>
/// 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 <see cref="TerminalSessionPump"/>'s flush loop and freeze a live
/// session; see <see cref="TerminalDataPlane.SendAsync"/>'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.
/// </para>
/// <para>
/// Driven straight through <see cref="TerminalDataPlane.SendAsync"/> 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.
/// </para>
/// </remarks>
[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 ----