Reconnect the terminal view when somebody comes back to it

Going back to a terminal left alone for a while found it stuck on
"Reconnecting the terminal view…", and stuck is the right word: the banner
stayed and nothing behind it was reconnecting.

The page's whole recovery story was a setTimeout chain, and a chain is exactly
what a WebView is entitled to stop running. Chromium throttles timers in a page
nobody is looking at — down to once a minute once it has been hidden five
minutes — and a renderer that is frozen, or reclaimed and not yet reloaded,
runs none of them. So the socket drops while nobody is watching, the banner
goes up, the retry is scheduled, and the retry is then the one thing not
running.

Three defects, each of which leaves that banner up for the rest of the page's
life.

◆ NOTHING LISTENED FOR THE PAGE COMING BACK. The only thing that could clear
the banner was a timer that may never fire. terminal.js now reconnects on
visibilitychange, focus and online — the events that mean somebody is looking
again, and the ones that cannot be throttled — cancelling the pending timer and
resetting the backoff. Over a healthy socket all three do nothing, which is
what makes them safe to fire as often as clicking a window does.

◆ A HANDSHAKE THAT NEVER FINISHED WAS INVISIBLE. Every retry was scheduled by a
close or an error, so an attempt parked in CONNECTING — which is what a
suspended renderer leaves behind — scheduled nothing at all, ever. There is now
a five-second watchdog on the handshake.

◆ STALE SOCKETS SCHEDULED RETRIES, AND THAT ONE IS A LOOP RATHER THAN A STALL.
connect() never detached the old socket's handlers, and the host aborts the
displaced socket on takeover — TerminalDataPlane.UpgradeAsync, doing exactly
what it should. That close read as a fresh failure and scheduled a retry
against the socket that had just succeeded, whose own close scheduled the next:
no fixed point, reconnecting every second forever with the banner up for most
of it. Every handler now asks whether it is still the page's own attempt, and
connect() closes what it abandons.

◆ WHICH OF THE PLATFORM BEHAVIOURS ACTUALLY BIT IS NOT ESTABLISHED, and the fix
does not depend on knowing. Throttled timers, a frozen renderer and a reclaimed
one all end at the same dead timer; guessing between them would have produced a
narrower fix for one of the three.

THE TEST RUNS terminal.js ITSELF, in a fake browser, inside dotnet test.
RendererPage loads the file the shell project ships — not a transcription of its
logic into C#, which would be a copy that stays correct while the page rots —
into a Jint engine, one per test, over a harness that fakes a WebSocket and a
clock and nothing else. Jint rather than a node script because CI would run the
node one and nobody's inner loop would; the cost is that Jint is not Chromium,
so this proves the page's logic and nothing about how a WebView behaves. That
line is drawn in RendererPage's remark and picked up by two new manual checks,
1.10 for the desktop and 11.12a for the phone, which own the platform half.

Four of the nine tests fail against the page as it stood — the stale close, the
parked handshake, and the two wake-ups. Two more assert that a wake-up over a
healthy socket does nothing, and pass against either version on purpose: they
are what stops the cure being worse.

Left alone deliberately: a socket that is open and dead shows no banner at all,
because readyState still reads OPEN. That looks like a terminal that swallows
what is typed, needs a liveness probe rather than a faster retry, and is written
down at the end of 11.12a rather than quietly bundled in here.

tests/DodoSSH.Client.Terminal.Tests/DodoSSH.Client.Terminal.Tests.csproj

@@ -4,10 +4,44 @@
     The throughput and backpressure harness the plan requires before any UI exists. Nothing
     here needs a WebView: the flow control is what is most likely to be wrong, and it is pure
     logic once ITerminalTransport is a seam.
+
+    And, since RendererReconnectionTests, the renderer's own half of the same protocol — the page
+    that reads these frames, run as a script rather than described in C#. It lives here rather than
+    in a test project of the shell's own because this is where the other end of the socket is
+    tested, and the two halves of "the renderer stays attached" are one mechanism: the host takes
+    a second upgrade over the first, and the page is what makes that second upgrade happen. A
+    project for the shell would still need a reference to this one's subject to say anything.
   -->
 
   <ItemGroup>
     <ProjectReference Include="../../src/DodoSSH.Client.Terminal/DodoSSH.Client.Terminal.csproj" />
   </ItemGroup>
 
+  <ItemGroup>
+    <!--
+      A JavaScript engine, so terminal.js can be run as it ships instead of transcribed into C#.
+      See RendererPage for the trade against a node script, and for what an engine that is not
+      Chromium can and cannot prove.
+    -->
+    <PackageReference Include="Jint" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <!--
+      ◆ THE PAGE IS COPIED OUT OF THE SHELL PROJECT, WHICH IS THE ONE ODD THING IN THIS FILE.
+
+      Reaching across for a source file is not something else here does, and the alternatives are
+      worse: referencing DodoSSH.Client.Shell would drag Avalonia into a suite that draws nothing,
+      and a copy of the page checked in beside the tests would be a copy — the thing that quietly
+      stops matching what ships, which is precisely the failure this test exists to catch.
+
+      PreserveNewest rather than Always so that editing the page is what rebuilds it.
+    -->
+    <None Include="../../src/DodoSSH.Client.Shell/WebAssets/terminal.js"
+          Link="Renderer/terminal.js"
+          CopyToOutputDirectory="PreserveNewest" />
+
+    <None Include="Renderer/renderer-harness.js" CopyToOutputDirectory="PreserveNewest" />
+  </ItemGroup>
+
 </Project>

tests/DodoSSH.Client.Terminal.Tests/Renderer/renderer-harness.js

@@ -0,0 +1,222 @@
+'use strict';
+
+/*
+  The world terminal.js is loaded into by RendererReconnectionTests.
+
+  It fakes exactly what the page's transport touches and nothing else: a WebSocket whose every step is
+  driven from the test, a clock that only moves when a test moves it, and the two DOM objects the page
+  reads at load. There is no Terminal, no fit addon and no WebGL here, because nothing on the reconnection
+  path builds a pane — a test that opens a session will have to add them, and should, rather than this
+  file guessing now at what such a test would want.
+
+  ◆ NOTHING HERE MAY DECLARE A NAME terminal.js ALSO DECLARES.
+
+  Both files are evaluated as scripts into the same global scope, and the page's own `const root` against
+  a `var root` here is a SyntaxError before a line of either runs. That is why the page's two elements are
+  `pageRoot` and `pageBanner` below and reached through getElementById, which is how the page reaches them
+  anyway.
+
+  ◆ AND THE FAKE SOCKET'S BEHAVIOUR IS THE PART TO GET RIGHT.
+
+  close() is the one method with a rule that is not obvious and that the tests lean on: a socket closed
+  while it is still CONNECTING never fires 'close' at all, per the WebSocket specification, while one
+  closed after it opened does. The page relies on exactly that — connect() closes the attempt it abandons
+  and expects to hear nothing back from it — so a fake that fired 'close' either way would report the loop
+  the page's guards exist to prevent, and one that fired it neither way would hide it.
+*/
+
+var clock = { now: 0, next: 1, timers: {} };
+
+/** Every socket the page has opened, in order, live or dead. */
+var sockets = [];
+
+function listeners(target) {
+  target.handlers = {};
+
+  target.addEventListener = function (name, handler) {
+    if (!target.handlers[name]) {
+      target.handlers[name] = [];
+    }
+
+    target.handlers[name].push(handler);
+  };
+
+  target.fire = function (name) {
+    var handlers = target.handlers[name] || [];
+
+    for (var i = 0; i < handlers.length; i++) {
+      handlers[i]({});
+    }
+  };
+
+  return target;
+}
+
+function FakeSocket(url, protocols) {
+  this.url = url;
+  this.protocols = protocols;
+  this.readyState = FakeSocket.CONNECTING;
+  this.binaryType = '';
+
+  listeners(this);
+  sockets.push(this);
+}
+
+FakeSocket.CONNECTING = 0;
+FakeSocket.OPEN = 1;
+FakeSocket.CLOSING = 2;
+FakeSocket.CLOSED = 3;
+
+FakeSocket.prototype.send = function () {};
+
+/** What the page calls. See the note above on why a connecting socket goes quietly. */
+FakeSocket.prototype.close = function () {
+  if (this.readyState === FakeSocket.CLOSED) {
+    return;
+  }
+
+  var wasConnecting = this.readyState === FakeSocket.CONNECTING;
+  this.readyState = FakeSocket.CLOSED;
+
+  if (!wasConnecting) {
+    this.fire('close');
+  }
+};
+
+var pageBanner = { textContent: 'Connecting…' };
+
+var pageRoot = listeners({
+  dataset: { token: 'test-token', socket: 'ws://127.0.0.1:1/socket' },
+  appendChild: function () {},
+});
+
+var document = listeners({
+  hidden: false,
+  getElementById: function (id) { return id === 'root' ? pageRoot : pageBanner; },
+  createElement: function () { return { dataset: {}, style: {}, remove: function () {} }; },
+});
+
+var window = listeners({});
+
+// The page warns through this on paths no reconnection test reaches. Present so that a test which does
+// reach one fails on its own assertion rather than on a missing global.
+var console = { warn: function () {}, log: function () {} };
+
+function setTimeout(callback, delay) {
+  var id = clock.next++;
+
+  clock.timers[id] = { at: clock.now + (delay || 0), callback: callback };
+
+  return id;
+}
+
+function clearTimeout(id) {
+  delete clock.timers[id];
+}
+
+function ResizeObserver() {
+  this.observe = function () {};
+}
+
+// ── What the tests drive the page with ──────────────────────────────────────────────────────────────
+
+/**
+ * Runs every timer due within the next `ms`, in the order they fall due.
+ *
+ * One at a time and re-scanned each round rather than collected up front, because a timer's callback
+ * routinely schedules the next one — which is the whole shape of the page's retry — and a snapshot taken
+ * before the first callback ran would miss it.
+ */
+function advance(ms) {
+  var target = clock.now + ms;
+
+  for (;;) {
+    var dueId = null;
+    var due = null;
+
+    for (var id in clock.timers) {
+      var timer = clock.timers[id];
+
+      if (timer.at <= target && (due === null || timer.at < due.at)) {
+        due = timer;
+        dueId = id;
+      }
+    }
+
+    if (due === null) {
+      break;
+    }
+
+    delete clock.timers[dueId];
+    clock.now = due.at;
+    due.callback();
+  }
+
+  clock.now = target;
+}
+
+/** How many sockets the page has opened since it loaded. */
+function attempts() {
+  return sockets.length;
+}
+
+/** What the one status element says — the banner the user sees. */
+function banner() {
+  return pageBanner.textContent;
+}
+
+/** Whether that attempt has been closed, by the page or by the far end. */
+function isClosed(index) {
+  return sockets[index].readyState === FakeSocket.CLOSED;
+}
+
+/** The host accepted the upgrade. */
+function accept(index) {
+  sockets[index].readyState = FakeSocket.OPEN;
+  sockets[index].fire('open');
+}
+
+/** An established socket goes away and the page is told. */
+function drop(index) {
+  sockets[index].readyState = FakeSocket.CLOSED;
+  sockets[index].fire('close');
+}
+
+/** An attempt that never connects: 'error' then 'close', as a refused connection reports itself. */
+function fail(index) {
+  sockets[index].readyState = FakeSocket.CLOSED;
+  sockets[index].fire('error');
+  sockets[index].fire('close');
+}
+
+/**
+ * A close arriving for a socket that died earlier — the host's takeover abort, landing late.
+ *
+ * Separate from drop() because the point of it is the delay: the socket is already dead by the time the
+ * event is delivered, which is what a suspended renderer's queued events look like on the way back.
+ */
+function deliverLateClose(index) {
+  sockets[index].readyState = FakeSocket.CLOSED;
+  sockets[index].fire('close');
+}
+
+/** The screen the page is on goes away, and comes back. */
+function becomeHidden() {
+  document.hidden = true;
+  document.fire('visibilitychange');
+}
+
+function becomeVisible() {
+  document.hidden = false;
+  document.fire('visibilitychange');
+}
+
+function takeFocus() {
+  window.fire('focus');
+}
+
+function comeOnline() {
+  window.fire('online');
+}
+
+var WebSocket = FakeSocket;

tests/DodoSSH.Client.Terminal.Tests/RendererPage.cs

@@ -0,0 +1,79 @@
+using System.Globalization;
+
+using Jint;
+
+namespace DodoSSH.Client.Terminal.Tests;
+
+/// <summary>
+/// One load of <c>terminal.js</c>, in a fake browser, for one test.
+/// </summary>
+/// <remarks>
+/// <para>
+/// <b>The page's own source is what runs.</b> Not a transcription of its logic into C# — that would test a
+/// copy, and the copy would be the thing that stayed correct. The file is read from the shell project and
+/// evaluated as it ships, so a change to the page that breaks the reconnection rules fails here.
+/// </para>
+/// <para>
+/// <b>Jint rather than node, and that is a deliberate trade.</b> A node script would be the obvious way to
+/// run JavaScript and would need node on every machine and in every CI job that runs the suite — so it
+/// would be a second test command, run separately, and the first thing to be forgotten. This runs inside
+/// <c>dotnet test</c> with everything else. What it costs is that the engine is not the engine the page
+/// actually runs in: Jint is not Chromium, so this can prove the page's own logic and can prove nothing
+/// about how WebView2 or Android's WebView behave. That boundary is exactly where
+/// <c>docs/manual-checks.md</c> picks up — see 1.10 and 11.12a.
+/// </para>
+/// <para>
+/// A fresh engine per test, because the page is a script with module-level state and there is no unloading
+/// it: two tests sharing one engine would share a socket list, a clock and a backoff.
+/// </para>
+/// </remarks>
+internal sealed class RendererPage
+{
+    private readonly Engine engine;
+
+    private RendererPage(Engine engine) => this.engine = engine;
+
+    /// <summary>How many sockets the page has opened since it loaded.</summary>
+    /// <remarks>
+    /// The measurement nearly every test here turns on, and it is deliberately a count of *attempts*
+    /// rather than of live sockets: the failures being guarded against are a page that stops trying and a
+    /// page that never stops, and both are counted rather than observed.
+    /// </remarks>
+    public int Attempts => (int)engine.Evaluate("attempts()").AsNumber();
+
+    /// <summary>What the status element says — the banner a user would be looking at.</summary>
+    public string Banner => engine.Evaluate("banner()").AsString();
+
+    /// <summary>
+    /// Loads the harness and then the page, leaving the page exactly as it is a moment after the WebView
+    /// navigated to it: one socket opened and still connecting.
+    /// </summary>
+    public static RendererPage Load()
+    {
+        var browser = new Engine();
+
+        // Order matters and is not incidental: the page connects on its last line, so every global it
+        // touches — the socket constructor above all — has to be in place before it is evaluated.
+        browser.Execute(Read("Renderer/renderer-harness.js"));
+        browser.Execute(Read("Renderer/terminal.js"));
+
+        return new RendererPage(browser);
+    }
+
+    /// <summary>Runs a line of the harness's own vocabulary — <c>accept(0)</c>, <c>advance(1000)</c>.</summary>
+    public void Do(string script) => engine.Execute(script);
+
+    /// <summary>Whether that attempt has been closed, by the page or by the far end.</summary>
+    public bool IsClosed(int attempt) =>
+        engine.Evaluate(
+                string.Create(CultureInfo.InvariantCulture, $"isClosed({attempt})"))
+            .AsBoolean();
+
+    /// <remarks>
+    /// Both files are copied beside the test assembly by the project file, the page out of the shell
+    /// project it belongs to. Read from disk rather than embedded so that the copy which runs here is the
+    /// same bytes the host serves, with nothing in between that could go stale.
+    /// </remarks>
+    private static string Read(string relativePath) =>
+        File.ReadAllText(Path.Combine(AppContext.BaseDirectory, relativePath));
+}

tests/DodoSSH.Client.Terminal.Tests/RendererReconnectionTests.cs

@@ -0,0 +1,192 @@
+namespace DodoSSH.Client.Terminal.Tests;
+
+/// <summary>
+/// The renderer page's half of staying attached — <c>terminal.js</c>'s <c>connect()</c> and what drives it.
+/// </summary>
+/// <remarks>
+/// <para>
+/// The host's half is <see cref="TerminalDataPlaneTests"/>, and the two are one mechanism: a socket that
+/// drops is ordinary here, and the page coming back for another is what makes it ordinary. What these
+/// tests protect is the property that failure of this mechanism has no other symptom — a terminal whose
+/// page has given up looks exactly like a terminal whose remote has gone quiet, except for a line of text
+/// nobody reads twice.
+/// </para>
+/// <para>
+/// Four of these were written against a page that failed them — the stale close, the handshake that never
+/// finishes, and the two wake-ups — and the rest describe behaviour that was already right and is easy to
+/// break while fixing those. The two that assert a wake-up does *nothing* pass against either version,
+/// which is the point of them: they are what stops the cure being worse, and they can only ever fail
+/// against a future change. See <see cref="RendererPage"/> for how the real file is loaded and for what
+/// this cannot reach.
+/// </para>
+/// </remarks>
+public sealed class RendererReconnectionTests
+{
+    [Fact]
+    public void ThePage_ConnectsWhenItLoads()
+    {
+        var page = RendererPage.Load();
+
+        page.Attempts.ShouldBe(1);
+    }
+
+    [Fact]
+    public void ADroppedSocket_IsRetriedAndTheBannerClears()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("accept(0)");
+        page.Banner.ShouldBe("");
+
+        page.Do("drop(0)");
+        page.Banner.ShouldStartWith("Reconnecting");
+
+        page.Do("advance(1000)");
+        page.Attempts.ShouldBe(2);
+
+        page.Do("accept(1)");
+        page.Banner.ShouldBe("");
+    }
+
+    /// <remarks>
+    /// The wait grows within one outage and goes back to a second once a socket has actually opened, so
+    /// that the next outage is not paid for at the previous one's rate.
+    /// </remarks>
+    [Fact]
+    public void TheWait_GrowsWithinAnOutageAndResetsAfterIt()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("fail(0); advance(1000)");
+        page.Attempts.ShouldBe(2);
+
+        page.Do("fail(1); advance(1999)");
+        page.Attempts.ShouldBe(2);
+
+        page.Do("advance(1)");
+        page.Attempts.ShouldBe(3);
+
+        page.Do("accept(2); drop(2); advance(1000)");
+        page.Attempts.ShouldBe(4);
+    }
+
+    /// <summary>
+    /// A close for a socket the page has already replaced must not start a reconnect.
+    /// </summary>
+    /// <remarks>
+    /// The loop this forbids costs nothing to enter and never leaves: the host aborts the displaced socket
+    /// on every takeover — see <c>TerminalDataPlane.UpgradeAsync</c> — so a stale close that schedules a
+    /// retry displaces the socket that has just succeeded, whose own close schedules the next. The visible
+    /// end of it is a terminal that reconnects every second forever with the banner up for most of it.
+    /// </remarks>
+    [Fact]
+    public void AStaleClose_DoesNotDisplaceTheSocketThatSucceeded()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("accept(0); drop(0); advance(1000)");
+        page.Do("accept(1)");
+        page.Attempts.ShouldBe(2);
+
+        // The first socket's end, arriving after the page has moved on.
+        page.Do("deliverLateClose(0)");
+        page.Do("advance(60000)");
+
+        page.Attempts.ShouldBe(2);
+        page.Banner.ShouldBe("");
+    }
+
+    /// <summary>
+    /// An attempt that never finishes its handshake is given up on rather than waited on forever.
+    /// </summary>
+    /// <remarks>
+    /// Every other retry in the page is scheduled by a close or an error, so a socket that reports neither
+    /// — which is what a renderer suspended mid-handshake leaves behind — used to schedule nothing at all.
+    /// The page then held a banner saying it was reconnecting with no timer pending and no socket coming,
+    /// for the rest of its life.
+    /// </remarks>
+    [Fact]
+    public void AHandshakeThatNeverFinishes_IsAbandonedAndRetried()
+    {
+        var page = RendererPage.Load();
+
+        // Nothing whatever from the first attempt: no open, no error, no close.
+        page.Do("advance(5000)");
+        page.Attempts.ShouldBe(1);
+
+        page.Do("advance(1000)");
+        page.Attempts.ShouldBe(2);
+        page.IsClosed(0).ShouldBeTrue();
+
+        page.Do("accept(1)");
+        page.Banner.ShouldBe("");
+    }
+
+    /// <summary>
+    /// Coming back to the page reconnects it, without waiting for a timer that may not be running.
+    /// </summary>
+    /// <remarks>
+    /// The case the whole wake-up path exists for, and the one a test can only approximate: the harness's
+    /// clock stands still here because a real hidden page's clock is throttled rather than stopped, and
+    /// standing still is the honest worst case of that. What is being asserted is that the page does not
+    /// need the clock at all to notice it is back.
+    /// </remarks>
+    [Fact]
+    public void BecomingVisibleAgain_ReconnectsWithoutTheTimer()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("accept(0); becomeHidden(); drop(0)");
+        page.Attempts.ShouldBe(1);
+
+        page.Do("becomeVisible()");
+        page.Attempts.ShouldBe(2);
+
+        // And the timer that was pending when the page woke must not open a third socket on top of the
+        // one that just succeeded — which would be the takeover loop, entered from the other side.
+        page.Do("accept(1); advance(60000)");
+        page.Attempts.ShouldBe(2);
+        page.Banner.ShouldBe("");
+    }
+
+    [Fact]
+    public void TakingTheKeyboardBack_AlsoReconnects()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("accept(0); drop(0); takeFocus()");
+
+        page.Attempts.ShouldBe(2);
+    }
+
+    /// <remarks>
+    /// The wake-ups fire on gestures as ordinary as clicking the window, so the check they make has to be
+    /// the thing that keeps them cheap rather than the frequency. A page whose socket is up must treat all
+    /// of them as nothing at all — anything else would be the takeover loop with a person's mouse driving it.
+    /// </remarks>
+    [Fact]
+    public void WakingUpOverAHealthySocket_DoesNothing()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("accept(0)");
+        page.Do("takeFocus(); becomeVisible(); comeOnline(); becomeHidden(); becomeVisible()");
+
+        page.Attempts.ShouldBe(1);
+        page.Banner.ShouldBe("");
+    }
+
+    /// <remarks>
+    /// An attempt already in flight is left to finish or to time out. Restarting it on every wake-up would
+    /// mean a page being clicked during a slow handshake never completing one.
+    /// </remarks>
+    [Fact]
+    public void WakingUpWhileConnecting_LeavesTheAttemptAlone()
+    {
+        var page = RendererPage.Load();
+
+        page.Do("takeFocus(); becomeVisible(); comeOnline()");
+
+        page.Attempts.ShouldBe(1);
+    }
+}

tests/DodoSSH.Client.Terminal.Tests/packages.lock.json

@@ -2,6 +2,15 @@
   "version": 2,
   "dependencies": {
     "net10.0": {
+      "Jint": {
+        "type": "Direct",
+        "requested": "[4.16.0, )",
+        "resolved": "4.16.0",
+        "contentHash": "YHofgoVtjWzqmG2GsGsp6eYMmcBfGgJOcH+Ki2UdZXNsYgKGnWrK2hSqRSPG/6HqeiipT5YPhzEH+bwxFO+YAQ==",
+        "dependencies": {
+          "Acornima": "1.7.0"
+        }
+      },
       "Meziantou.Analyzer": {
         "type": "Direct",
         "requested": "[3.0.137, )",
@@ -48,6 +57,11 @@
           "xunit.v3.mtp-v1": "[3.2.2]"
         }
       },
+      "Acornima": {
+        "type": "Transitive",
+        "resolved": "1.7.0",
+        "contentHash": "a2I4O4qkuAdB0oSaGz6/k0n/bXxMbGcLBra5dNTTVSLmTwM7uORA8ebpOrNMmDfFWOMwvuaRL6gPprRV4HTV0w=="
+      },
       "Castle.Core": {
         "type": "Transitive",
         "resolved": "5.1.1",