diff --git a/Directory.Packages.props b/Directory.Packages.props index 5354c42..84d631c 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -221,5 +221,20 @@ mistakes that cause real authorization holes. --> + + \ No newline at end of file diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 928aefa..877ddb2 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -171,6 +171,32 @@ bound to the same side of `IsImportOpen`. SettingsNav lighting a different row w over the importer would be a guard added to `OnKeyDown` for `IsSettingsMode` that the design never asked for and this application's own quick-connect card was built to reach past. +### 1.10 A terminal left alone for a long time is still a terminal · **needs an hour, or a debugger** + +Open a shell, leave the terminal for another screen — HOSTS, FILES, anything — and leave the application +alone for long enough that the window has been in the background for the better part of an hour. Locking the +machine or letting it sleep counts and is the easier way to get there. Come back and click the session's +tab. + +**Pass:** the pane is exactly where it was and takes input straight away. If anything is shown at all it is +`Reconnecting the terminal view…` for a moment, in the second or so before the socket is back — never a +status that is still there after that. + +**Both of the page's own rules here are covered by `RendererReconnectionTests`**, which runs `terminal.js` +itself in a fake browser — so a failure of this check is more likely to be the WebView behaving unlike that +fake than the page's logic being wrong. That is exactly the division: the test owns the logic, this owns the +platform. + +**Failure means:** a banner that stays up is the page's retry not running. It is a `setTimeout` chain, and a +chain is what a WebView is entitled to throttle or freeze while nobody is looking at the page; `terminal.js` +answers that with wake-ups on `visibilitychange`, `focus` and `online`, none of which can be throttled, +plus a watchdog for a handshake that never finishes. A banner that flickers on and off every second or two +instead is the opposite fault — a reconnect loop, in which each attempt displaces the socket before it +through `TerminalDataPlane.UpgradeAsync`'s takeover and the displaced socket's close schedules the next. +That is what the "is this still the page's socket" guard in `connect()` exists to stop. A pane that takes no +input while the banner is *clear* is neither: the socket is open and dead, which nothing on this page can +currently see — see the note at the end of 11.12a. + --- ## Phase 2 — Known Hosts as its own page @@ -1797,6 +1823,29 @@ worse than saying nothing: the banner exists so this is never silently wrong. If banner is there, the session's credit window was not reset on reattach and the shell is frozen behind it — see `TerminalWorkspace.ReplayAfterAttachAsync`. +### 11.12a Coming back to a terminal screen left alone for a long while + +The same shape as 11.12 and a different trigger: rather than backgrounding the app, stay in it. With a shell +open, leave the terminal for HOSTS, FILES or MORE — which collapses the renderer to GONE, so the page is +hidden by Chromium's reckoning — and leave the phone alone for at least ten minutes with the screen off. +Then come back to the app and to the terminal. + +**Pass:** the pane is there and takes input at once, or reconnects visibly within about a second of the +screen appearing. `Reconnecting the terminal view…` on the way in is fine; still being there once the +terminal has been on screen for a couple of seconds is not. + +**Failure means:** the page's retry did not survive being hidden. A hidden WebView has its timers throttled +— once a minute after five minutes hidden — and a renderer that was frozen or reclaimed runs none of them, +which is why `terminal.js` does not rely on the timer alone: `visibilitychange` is the event that says the +screen is back, and it reconnects immediately rather than waiting to be asked twice. Ten minutes is chosen +to clear the five-minute threshold with room to spare. + +**Not covered by either check, and worth knowing:** a socket that is *open and dead* — the connection gone +without either end noticing, which a suspended renderer can leave behind — shows no banner at all, because +`readyState` still reads OPEN and nothing on this page probes further. The symptom is a terminal that looks +connected and swallows what is typed. If that is ever seen, it is a different bug from this one and needs a +liveness probe rather than a faster retry. + --- ## Phase 12 — Shared vaults: the operations that span two accounts diff --git a/src/DodoSSH.Client.Shell/WebAssets/terminal.js b/src/DodoSSH.Client.Shell/WebAssets/terminal.js index b3779f9..20b3e85 100644 --- a/src/DodoSSH.Client.Shell/WebAssets/terminal.js +++ b/src/DodoSSH.Client.Shell/WebAssets/terminal.js @@ -16,7 +16,8 @@ happened to straddle a frame boundary, which shows up as occasional mojibake in exactly the conditions that are hardest to reproduce. - 3. The socket reconnects itself, forever, with backoff. This page's WebView is routinely killed + 3. The socket reconnects itself, forever, with backoff — and on being looked at again, which is not + the same thing and is the half a timer cannot cover. This page's WebView is routinely killed and reloaded by Android under memory pressure or simply for being backgrounded, so "the socket closed" is an ordinary event here, not the end of the terminal's life — see connect(). A reloaded page starts with an empty session map, so createSession is idempotent (a session @@ -52,6 +53,22 @@ const SCROLLBACK_LINES = 5000; const RECONNECT_INITIAL_DELAY_MS = 1000; const RECONNECT_MAX_DELAY_MS = 5000; +/* + How long a socket is given to finish its handshake before it is treated as a failure. + + A socket that cannot connect normally says so and says it quickly — 'error' then 'close', within a + millisecond or two of a loopback refusal. The case this covers is the one that says nothing: an attempt + parked in CONNECTING with no event ever coming, which is what a WebSocket opened by a renderer that is + then suspended, or one whose handshake the host never answers, leaves this page holding. + + Without the watchdog that state is terminal, and quietly so. Every retry in this file is scheduled by a + close or an error, so an attempt that produces neither schedules nothing: the banner says the view is + reconnecting for the rest of the page's life while nothing whatever is reconnecting. Five seconds is + generous against a handshake that ordinarily takes about a millisecond, and short against somebody + waiting on their terminal to come back. +*/ +const HANDSHAKE_TIMEOUT_MS = 5000; + /* Styled like the SESSION_CLOSED banner (matching \x1b[38;5;244, the same dim grey), but written by createSession's caller rather than by createSession itself: only a *replay* landing on a pane that @@ -583,8 +600,47 @@ function handleFrame(buffer) { } } +/* + ── THE SOCKET COMES BACK BY ITSELF, INCLUDING FOR A PAGE NOBODY HAS LOOKED AT IN AN HOUR ──────────── + + Retrying on a timer is the easy half and was the whole of this. The three rules below are what make the + retry actually reach a page that has been sitting collapsed — each of them a way this page was found + showing "Reconnecting the terminal view…" over a socket that nothing was reconnecting. + + 1. ONLY THE CURRENT ATTEMPT'S EVENTS COUNT. connect() abandons whatever socket was attached before it, + and an abandoned socket still reports its end: the host aborts it the moment the newcomer upgrades, + which is TerminalDataPlane.UpgradeAsync's takeover doing exactly what it is meant to. Counting that + close as a fresh failure schedules a retry against a socket that has just succeeded, and the + takeover then aborts *that* one, whose close schedules the next — a loop with no fixed point, in + which the terminal reconnects every second or so forever and the banner is up for most of it. Every + handler below asks whether it is still the page's socket before it does anything. + + 2. AN ATTEMPT THAT NEVER FINISHES IS A FAILURE TOO. See HANDSHAKE_TIMEOUT_MS. + + 3. COMING BACK IS A REASON TO TRY, NOT ONLY THE CLOCK. The retry is a setTimeout chain, and a chain is + precisely 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 at all. 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: coming back shows a terminal that says it is reconnecting and, for as long as that lasts, + is not. + + The phone is where this is easiest to reach, because leaving the terminal for another screen + collapses its WebView to GONE — see createSession's note on the GPU context — and a WebView with no + surface is a page the platform may treat as hidden. Which of the two mechanisms actually bit is not + established here, and does not need to be: both end with a timer that will not run, and the fix is + not to guess at either but to stop depending on the timer alone. + + Hence the wake-ups at the bottom of this file. What they add is not a faster retry; it is a retry + driven by the one thing that cannot be throttled, which is the user arriving. +*/ + /** @type {number | null} */ let reconnectTimer = null; + +/** @type {number | null} */ +let handshakeTimer = null; + let reconnectDelay = RECONNECT_INITIAL_DELAY_MS; /** @@ -608,16 +664,86 @@ function scheduleReconnect() { reconnectDelay = Math.min(reconnectDelay * 2, RECONNECT_MAX_DELAY_MS); } +/** + * Tries the socket again now, if it is down. + * + * The entry point for "somebody is looking at this page again", and it has to be safe to call as often + * as that happens — which on the desktop is every time the window is clicked. A socket that is up makes + * this nothing at all. + * + * An attempt still in CONNECTING is left alone rather than restarted: it may be about to succeed, and + * the one that is not is already the handshake watchdog's to give up on. + */ +function reconnectIfDown() { + if (socket !== null + && (socket.readyState === WebSocket.OPEN || socket.readyState === WebSocket.CONNECTING)) { + return; + } + + // Whatever the timer was going to do, this is doing now. Left pending it would land on top of the + // socket this call is about to open, and the takeover that followed is the loop rule 1 describes. + if (reconnectTimer !== null) { + clearTimeout(reconnectTimer); + reconnectTimer = null; + } + + // Back to the quick attempt. The wait grew to space out retries during an outage nobody was watching, + // and being called at all means somebody is watching now. + reconnectDelay = RECONNECT_INITIAL_DELAY_MS; + + connect(); +} + function connect() { const token = root.dataset.token; const url = root.dataset.socket; + /* + Whatever was attached is abandoned here, and closed rather than dropped: the host takes the socket + over regardless, but one left open is a connection it has to abort and an event this page then has to + ignore. Cleared out of `socket` before the close, so that every handler — including that close, + whenever it lands — can already tell the attempt is no longer the page's. + */ + const abandoned = socket; + socket = null; + abandoned?.close(); + // The token travels as a subprotocol rather than a query parameter, which keeps it out of // anything that logs URLs. - socket = new WebSocket(url, ['dodossh.terminal.v1', `token.${token}`]); - socket.binaryType = 'arraybuffer'; + const pending = new WebSocket(url, ['dodossh.terminal.v1', `token.${token}`]); + pending.binaryType = 'arraybuffer'; + socket = pending; - socket.addEventListener('open', () => { + /** Whether this attempt is still the page's, rather than one a later connect() has replaced. */ + const isCurrent = () => socket === pending; + + const forgetHandshakeTimer = () => { + if (handshakeTimer !== null) { + clearTimeout(handshakeTimer); + handshakeTimer = null; + } + }; + + forgetHandshakeTimer(); + + handshakeTimer = setTimeout(() => { + handshakeTimer = null; + + // Still CONNECTING with nothing on its way. Only the retry is arranged here; abandoning the attempt + // is left to the connect() that retry runs, which is the one place a socket is replaced. + if (isCurrent() && pending.readyState === WebSocket.CONNECTING) { + scheduleReconnect(); + } + }, HANDSHAKE_TIMEOUT_MS); + + pending.addEventListener('open', () => { + // A replaced attempt cannot reach here — connect() closes what it abandons, and a socket closed + // while connecting never opens — so this is a guard against the ordering rather than a live case. + if (!isCurrent()) { + return; + } + + forgetHandshakeTimer(); setTransportStatus(''); // Back to the quick attempt for whatever the next failure turns out to be. Kept slow between @@ -625,14 +751,25 @@ function connect() { reconnectDelay = RECONNECT_INITIAL_DELAY_MS; }); - socket.addEventListener('message', (event) => handleFrame(event.data)); + pending.addEventListener('message', (event) => { + if (isCurrent()) { + handleFrame(event.data); + } + }); // Both close and error retry. They are not the same event on every failure — a socket that never // opens can fire only 'error', one that opens and later drops fires only 'close' — and the host // side of this same problem (TerminalDataPlane.UpgradeAsync's takeover) is exactly why retrying is // safe: whichever attempt eventually reaches the host, a fresh valid upgrade always wins the socket. - socket.addEventListener('close', scheduleReconnect); - socket.addEventListener('error', scheduleReconnect); + const retry = () => { + if (isCurrent()) { + forgetHandshakeTimer(); + scheduleReconnect(); + } + }; + + pending.addEventListener('close', retry); + pending.addEventListener('error', retry); } // One observer for the whole root rather than one per pane: resizes arrive in bursts while a @@ -645,4 +782,30 @@ new ResizeObserver(() => { window.addEventListener('beforeunload', () => socket?.close()); +/* + The ways this page finds out somebody is looking at it again — rule 3 at the top of the transport + section. All three end in the same check, and that check is what makes them safe to be as noisy as they + are: with a healthy socket every one of them does nothing. + + 'visibilitychange' is the phone's. A WebView collapsed to GONE is a hidden page, so coming back to the + terminal screen is the event that says so, and it is the same event Chromium lifts its own throttling + on — this page simply does not wait to be asked twice. + + 'focus' is the desktop's, where the page is *not* marked hidden while the WebView is collapsed + (measured; see docs/platform-flags.md) but a window left in the background for hours has had its timers + throttled all the same. It fires when the terminal takes the keyboard back, which is the same gesture. + + 'online' is neither head's ordinary case, because the socket is loopback and has nothing to do with the + network — but the event does follow a machine coming back from sleep, and a page with a dead socket has + no reason to ignore any hint that the world has moved. +*/ +document.addEventListener('visibilitychange', () => { + if (!document.hidden) { + reconnectIfDown(); + } +}); + +window.addEventListener('focus', reconnectIfDown); +window.addEventListener('online', reconnectIfDown); + connect(); diff --git a/tests/DodoSSH.Client.Terminal.Tests/DodoSSH.Client.Terminal.Tests.csproj b/tests/DodoSSH.Client.Terminal.Tests/DodoSSH.Client.Terminal.Tests.csproj index 44b9525..d191df9 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/DodoSSH.Client.Terminal.Tests.csproj +++ b/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. --> + + + + + + + + + + + + diff --git a/tests/DodoSSH.Client.Terminal.Tests/Renderer/renderer-harness.js b/tests/DodoSSH.Client.Terminal.Tests/Renderer/renderer-harness.js new file mode 100644 index 0000000..878be07 --- /dev/null +++ b/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; diff --git a/tests/DodoSSH.Client.Terminal.Tests/RendererPage.cs b/tests/DodoSSH.Client.Terminal.Tests/RendererPage.cs new file mode 100644 index 0000000..dacd105 --- /dev/null +++ b/tests/DodoSSH.Client.Terminal.Tests/RendererPage.cs @@ -0,0 +1,79 @@ +using System.Globalization; + +using Jint; + +namespace DodoSSH.Client.Terminal.Tests; + +/// +/// One load of terminal.js, in a fake browser, for one test. +/// +/// +/// +/// The page's own source is what runs. 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. +/// +/// +/// Jint rather than node, and that is a deliberate trade. 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 +/// dotnet test 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 +/// docs/manual-checks.md picks up — see 1.10 and 11.12a. +/// +/// +/// 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. +/// +/// +internal sealed class RendererPage +{ + private readonly Engine engine; + + private RendererPage(Engine engine) => this.engine = engine; + + /// How many sockets the page has opened since it loaded. + /// + /// 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. + /// + public int Attempts => (int)engine.Evaluate("attempts()").AsNumber(); + + /// What the status element says — the banner a user would be looking at. + public string Banner => engine.Evaluate("banner()").AsString(); + + /// + /// 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. + /// + 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); + } + + /// Runs a line of the harness's own vocabulary — accept(0), advance(1000). + public void Do(string script) => engine.Execute(script); + + /// Whether that attempt has been closed, by the page or by the far end. + public bool IsClosed(int attempt) => + engine.Evaluate( + string.Create(CultureInfo.InvariantCulture, $"isClosed({attempt})")) + .AsBoolean(); + + /// + /// 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. + /// + private static string Read(string relativePath) => + File.ReadAllText(Path.Combine(AppContext.BaseDirectory, relativePath)); +} diff --git a/tests/DodoSSH.Client.Terminal.Tests/RendererReconnectionTests.cs b/tests/DodoSSH.Client.Terminal.Tests/RendererReconnectionTests.cs new file mode 100644 index 0000000..616524b --- /dev/null +++ b/tests/DodoSSH.Client.Terminal.Tests/RendererReconnectionTests.cs @@ -0,0 +1,192 @@ +namespace DodoSSH.Client.Terminal.Tests; + +/// +/// The renderer page's half of staying attached — terminal.js's connect() and what drives it. +/// +/// +/// +/// The host's half is , 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. +/// +/// +/// 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 for how the real file is loaded and for what +/// this cannot reach. +/// +/// +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(""); + } + + /// + /// 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. + /// + [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); + } + + /// + /// A close for a socket the page has already replaced must not start a reconnect. + /// + /// + /// The loop this forbids costs nothing to enter and never leaves: the host aborts the displaced socket + /// on every takeover — see TerminalDataPlane.UpgradeAsync — 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. + /// + [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(""); + } + + /// + /// An attempt that never finishes its handshake is given up on rather than waited on forever. + /// + /// + /// 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. + /// + [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(""); + } + + /// + /// Coming back to the page reconnects it, without waiting for a timer that may not be running. + /// + /// + /// 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. + /// + [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); + } + + /// + /// 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. + /// + [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(""); + } + + /// + /// 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. + /// + [Fact] + public void WakingUpWhileConnecting_LeavesTheAttemptAlone() + { + var page = RendererPage.Load(); + + page.Do("takeFocus(); becomeVisible(); comeOnline()"); + + page.Attempts.ShouldBe(1); + } +} diff --git a/tests/DodoSSH.Client.Terminal.Tests/packages.lock.json b/tests/DodoSSH.Client.Terminal.Tests/packages.lock.json index fa8d2fb..24c0db8 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/packages.lock.json +++ b/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",