diff --git a/docs/android-port.md b/docs/android-port.md index 6a137b4..7724b45 100644 --- a/docs/android-port.md +++ b/docs/android-port.md @@ -30,7 +30,10 @@ verified is that it compiles, links, packages, and carries the right natives. transfers protected by a **foreground service**. File transfer is not in the first scope; when it arrives it is **one remote pane** with Android's document picker for moving files in and out. *It has since arrived, both ways:* the pane, the queue, `ACTION_OPEN_DOCUMENT` going in and `ACTION_CREATE_DOCUMENT` coming out, -with the foreground service now counting transfers as well as shells. +with the foreground service now counting transfers as well as shells — and, since, an idle-but-connected +Files session as well, which a transfer count alone was blind to. *Corrected the same round:* the service's +other half — a shell's own opening — had never been wired to anything at all, so a shell survived only for +as long as the app stayed foreground; see [Sessions survive backgrounding](#sessions-survive-backgrounding-via-a-foreground-service). **What was actually checked**, so the rest can be read with the right amount of trust: @@ -235,6 +238,37 @@ The parts that are definitely different are the on-screen keyboard, and the fact needs Ctrl, Esc, Tab and arrows that the software keyboard does not offer — every Android SSH client ships an accessory key row for this. That is UI work, not porting. +> **⚠️ Corrected by the build. The data plane assumed a renderer that attaches once and lives forever, and +> that assumption is WebView2's truth, not Android's.** Desktop's WebView2 process starts with the window and +> dies with it; `TerminalDataPlane` was written to that reality — one socket, attached once, +> `Interlocked.Exchange`-guarded against a second attach ever happening at all. On a phone the WebView's own +> renderer process is a separate thing from the app process the foreground service above is keeping alive, +> and Android kills *that* independently — under memory pressure, or simply for being backgrounded — with no +> foreground service able to save it. The page then reloads with a fresh socket, and three things broke on +> that reload before this was found: the second attach was refused outright (`409 Conflict`), because a +> second valid upgrade could only mean a bug or a hostile second process, never our own page coming back; a +> send into the dead first socket threw, and that exception unwound `TerminalSessionPump`'s flush loop, +> freezing the still-live shell behind it — `LiveSessionCount` kept counting a session nothing would ever +> drain again; and every byte sent while no page was attached had already spent flow-control credit that no +> acknowledgement could ever return, so a session outliving 256 KiB of output into a dead page stalled for +> good regardless of the other two. Waiting for the old socket to notice it was dead and close on its own +> was never going to be enough either — a killed renderer sends no TCP FIN, so the old receive loop could sit +> unaware for the whole 30-second keepalive. +> +> Fixed as a takeover rather than a guard: a second valid upgrade — origin, token and subprotocol all +> checked exactly as before — now displaces whatever socket was attached instead of being refused, since +> only this app's own page ever knows the token, so a second valid attach *is* that page, back again. +> `TerminalDataPlane.SendAsync` no longer lets a dead-socket send escape as a fault; it reads as "nobody +> listening," same as no socket being attached at all. `TerminalWorkspace` resets each live session's credit +> window on every attach and resends its `SessionOpened` frame, flagged as a replay, so the fresh page +> rebuilds the pane and the pump stops waiting on an acknowledgement that was never coming. And +> `terminal.js`'s socket now retries itself, forever, with backoff, instead of reporting the connection +> failed and stopping — the page dies with the app anyway, so there is no case where retrying is the wrong +> call. What is **not** recovered, and says so rather than pretending otherwise: scrollback across a page +> reload. It lived in the page's own DOM, and a reloaded page is a new DOM. The replay banner — *"the view +> reconnected; earlier output stayed on the host"* — is that honesty put where the person looking at the +> terminal will actually read it, not buried in a log. + --- ## Decisions taken @@ -280,13 +314,40 @@ What is desktop-only is the *left* pane — `LocalDirectory`, the drive list, th ### Sessions survive backgrounding, via a foreground service -A persistent notification for as long as a shell or a transfer is live. +A persistent notification for as long as a shell, a transfer, or an idle-but-connected Files session is +live. It costs the user a notification and some battery. It buys the behaviour the desktop client already promises and documents — that a shell outlives a vault lock, and that a transfer finishes — and the alternative was to make `TerminalWorkspace`'s guarantee desktop-only, which is a worse thing to have to write down than a notification is to look at. +**Three corrections found after the first cut shipped, all in the wiring rather than the design:** + +- **A shell opening never started the service.** `SessionKeepAlive` heard `TerminalWorkspace.SessionEnded` + and refreshed on that, but nothing announced the opposite event — so a user who opened a shell and + backgrounded the app immediately had no foreground service at all, and Android was free to kill the + process holding it. `MainWindowViewModel.TerminalSessionOpened` is now wired the same way in + `App.axaml.cs`'s `ComposeKeepAlive`. +- **A connected-but-idle Files session counted as nothing.** A host open on the Files screen with no + transfer moving is a live SFTP connection a dying process would sever, and the old two-argument + `Reconcile(liveSessions, activeTransfers)` had no way to hear about it. `TransfersViewModel.HasLiveFileSession` + — `IsConnected` with a real `ConnectedCipher`, which a bucket never has — is the third fact `Reconcile` now + takes. +- **Refreshing the notification restarted the service, which throws when backgrounded.** `Reconcile` called + `StartForegroundService` on every refresh, including the common case of a service that was already + running. On API 31+ that throws `ForegroundServiceStartNotAllowedException` the instant the app is + backgrounded — a transfer finishing in the pocket, one of two shells dying — which crashed the process and + took every session with it. `SessionForegroundService` now tracks whether it is already running and, when + it is, posts the updated notification through `NotificationManager.Notify` instead of asking Android to + start anything. + +**The notification permission is requested, not just declared.** API 33+ requires `POST_NOTIFICATIONS` at +runtime or the receipt is silently invisible — the service still runs, but nothing on screen says so. +`SessionForegroundService.Reconcile` asks for it the first time in this process there is actually something +to show, at most once, with no result read back: a refusal costs the notification and nothing else, which is +what the manifest's own comment on the permission says. + ### Phone first About 360dp wide. The tablet route was cheaper — a landscape tablet is close to the existing 880×560 minimum @@ -523,7 +584,12 @@ go at 360dp: stopping it from a count rather than a lifecycle. `TerminalWorkspace.LiveSessionCount` is the source of truth deliberately: it already knows that a session whose shell exited is not live, which a counter incremented on open would not, and a phone showing "1 shell connected" over nothing would be exactly the - dishonesty the unlock screen's count exists to prevent. + dishonesty the unlock screen's count exists to prevent. *Corrected since:* the opened half of a shell's + lifecycle was never wired in, so the service could never come up for a shell at all; an idle-but-connected + Files session now counts as a third live fact rather than nothing; a refresh while backgrounded updates + the notification in place instead of restarting the service, which the API throws on; and + `POST_NOTIFICATIONS` is now actually requested rather than merely declared. See + [Sessions survive backgrounding](#sessions-survive-backgrounding-via-a-foreground-service) for all four. 7. ~~**The interface**, phone-first.~~ **Done for the decided scope** — all seven screens of the design, plus the two states the design does not draw because it starts at an enrolled phone (naming a server, and choosing a passphrase). diff --git a/docs/manual-checks.md b/docs/manual-checks.md index e824381..df9f537 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -1745,6 +1745,39 @@ is a terminal that answers the buttons and ignores the keyboard: it reads as the Worth doing on the software keyboard too, where the same fault shows as the keyboard closing on the first tap of an arrow key. +### 11.11 Closing a connection and opening a new one both take you somewhere real + +Open a shell, close its tab, then open a different one from HOSTS. + +**Pass:** the new terminal renders and takes input straight away — no stuck "Connecting…" status, no blank +pane that never receives the prompt. + +**Failure means:** `TerminalDataPlane` refused the page's reattach. The renderer's `WebSocket` does not +survive a tab going from one to zero and back to one on every device, and a host that answers a second valid +upgrade with `409 Conflict` instead of taking the socket over leaves every terminal after the first +permanently unreachable — see the correction in `docs/android-port.md`'s terminal section. + +### 11.12 A backgrounded shell survives its renderer being killed · **needs several minutes, or developer tooling** + +With a shell open and something worth reading in its scrollback, background the app (home button, not back) +for several minutes — long enough for Android to consider reclaiming it — then return. If the device exposes +it, forcing a stop of the WebView renderer process from Developer Options while backgrounded is the more +reliable way to trigger the same thing on demand rather than waiting on the OS's own judgement. Either way, +type something once you are back. + +**Pass:** one of two honest outcomes, both good. Either the pane is exactly as it was — the renderer process +survived, so nothing needed to happen — or the pane is empty but for a dim line reading `── the view +reconnected; earlier output stayed on the host ──`, meaning the page reloaded and reattached. In both cases +what is typed now reaches the shell, and the shell is still the same one — not a new tab, not a reconnect +sheet, no "Connecting…" status stuck on screen. + +**Failure means:** if the status stays stuck or nothing typed arrives, the renderer's socket did not retry +itself — see `terminal.js`'s `connect()` and its backoff. If the pane came back empty with **no** banner, a +session that survived a reload is being shown as though its scrollback had too, which is not true and is +worse than saying nothing: the banner exists so this is never silently wrong. If typing does nothing but the +banner is there, the session's credit window was not reset on reattach and the shell is frozen behind it — +see `TerminalWorkspace.ReplayAfterAttachAsync`. + --- ## Phase 12 — Shared vaults: the operations that span two accounts @@ -2075,9 +2108,57 @@ Queue several files in each direction, put the phone to sleep with the screen of notification goes away when the last one does — with no shell open. With a shell open it stays, because that is what it was already for. +Now, separately: open a shell to the host, press the home button (backgrounding rather than sleeping — the +distinction matters, because backgrounded is the state in which Android is free to kill a process no +foreground service is protecting), wait thirty seconds with the shell doing nothing, and return. + +**Pass:** the notification stayed up the whole time, and the shell is exactly where it was — same scrollback, +same prompt — with typing reaching the host immediately. Exit the shell. + +**Pass:** the notification goes with it, once nothing else is open. + **Failure means:** an upload that stalls with the screen off is the count not reaching `SessionForegroundService`, and Android has stopped the process mid-transfer. A notification left up -afterwards is `ActivityChanged` not being subscribed — the other end of the same wire. +afterwards is `ActivityChanged` not being subscribed — the other end of the same wire. A shell that has +disconnected on return is `MainWindowViewModel.TerminalSessionOpened` never reaching `SessionKeepAlive` — the +service only ever heard about a shell *ending*, so it never came up for one in the first place. + +### 14.6a A Files connection with nothing moving still survives backgrounding + +Connect to a host on the Files screen with no transfer queued — just browse to somewhere and stop. Note the +directory shown, then background the app, wait thirty seconds, and return. + +**Pass:** the notification stayed up the whole time (check the shade if the return is too quick to see it +directly), and the pane is exactly where it was — the same listing, the same breadcrumb — with no reconnect +needed. + +**Failure means:** `TransfersViewModel.HasLiveFileSession` not reaching `SessionKeepAlive`, so an idle but +still-open SFTP connection read as nothing running at all and the process was free to die under it. + +### 14.6b The notification permission is asked for once, at the first thing worth showing · **needs Android 13+** + +On a device running Android 13 or later, on a fresh install that has never connected to anything, open a +shell or the Files screen for the first time. + +**Pass:** a system dialogue asking to allow notifications appears at that moment — not at launch, and not +before this first connect. Answer it either way; the connection completes regardless, and background the app +afterwards to confirm nothing else changed about it. + +**Failure means:** the dialogue appearing at launch is asking before there is anything on screen to justify +it. Never appearing at all on API 33+ is the harder failure to notice, because nothing else surfaces it — +the service still starts and still holds the process open, only the receipt is invisible. See +`SessionForegroundService.RequestNotificationPermission`. + +### 14.6c Refusing the permission costs the notification and nothing else + +Continuing from 14.6b: choose **Don't allow** on the system dialogue. Queue a transfer, or open a shell, and +background the app. + +**Pass:** no notification appears anywhere, but the transfer still finishes, or the shell is still there on +return, exactly as in 14.1–14.6a. + +**Failure means:** anything disconnecting or failing here is the permission refusal being read as though it +had refused the service itself, rather than only the notification Android draws for it. ### 14.7 SAVE FILE writes where you pointed it, and the file opens diff --git a/src/DodoSSH.Client.Android/App.axaml.cs b/src/DodoSSH.Client.Android/App.axaml.cs index 6f49eed..91f4ce3 100644 --- a/src/DodoSSH.Client.Android/App.axaml.cs +++ b/src/DodoSSH.Client.Android/App.axaml.cs @@ -90,32 +90,61 @@ public sealed partial class DodoSshApp : Avalonia.Application shell.DataContext = viewModel; - // Difference 2: the foreground service, which is what makes TerminalWorkspace's promise — that a - // shell outlives a vault lock — true on a platform that stops backgrounded processes. - // - // The transfer count is real now that the document picker gives this head a way to start one, and - // it is the half that matters most here: a shell survives backgrounding because somebody is looking - // at it, and an upload has to survive precisely when nobody is — the screen is off and the phone is - // in a pocket. Queued counts as active, so putting five files in the queue and locking the phone - // moves five files. - // - // A local rather than a field, matching the desktop head: an Avalonia Application has no disposal - // hook, so a field holding a disposable would have nowhere honest to release it. It stays alive - // because it is subscribed to the workspace, which lives as long as the process. - var keepAlive = new SessionKeepAlive( - workspace, - activeTransfers: () => viewModel.Transfers.ActiveTransfers); - - // The other end of the same wire: the workspace announces its own sessions ending, and the queue - // announces transfers appearing and finishing. Without this the notification would come up when an - // upload started and stay up after it finished, which is the failure this class exists to prevent. - viewModel.Transfers.ActivityChanged += (_, _) => keepAlive.Refresh(); - - keepAlive.Refresh(); + // Difference 2, wired up in its own method purely for length — see ComposeKeepAlive for what it + // does and why. + ComposeKeepAlive(workspace, viewModel); return shell; } + /// + /// Wires up the foreground service that makes TerminalWorkspace's promise — that a shell outlives + /// a vault lock — true on a platform that stops backgrounded processes. + /// + /// + /// + /// Split out of for length rather than for reuse; there is exactly one caller. + /// + /// + /// The transfer count is real now that the document picker gives this head a way to start one, and it + /// matters exactly when nobody is looking: a shell survives backgrounding because somebody opened it, + /// and an upload has to survive precisely when nobody is — the screen is off and the phone is in a + /// pocket. Queued counts as active, so putting five files in the queue and locking the phone moves five + /// files. holdsFileSession covers the third case a count alone cannot: a host connected on the + /// Files screen with no transfer moving is still a live SFTP session that backgrounding would sever, and + /// TransfersViewModel.HasLiveFileSession is the existing fact — IsConnected with a real + /// cipher, which a bucket never has — that answers whether one is open. + /// + /// + /// keepAlive is a local rather than a field, matching the desktop head: an Avalonia Application + /// has no disposal hook, so a field holding a disposable would have nowhere honest to release it. It + /// stays alive because it is subscribed to the workspace, which lives as long as the process. + /// + /// + private static void ComposeKeepAlive(TerminalWorkspace workspace, MainWindowViewModel viewModel) + { + var keepAlive = new SessionKeepAlive( + workspace, + activeTransfers: () => viewModel.Transfers.ActiveTransfers, + holdsFileSession: () => viewModel.Transfers.HasLiveFileSession); + + // The other end of the same wire: the workspace announces its own sessions ending, and the queue + // announces transfers appearing and finishing, and a Files session connecting or disconnecting. + // Without this the notification would come up when an upload started and stay up after it + // finished, which is the failure this class exists to prevent. + viewModel.Transfers.ActivityChanged += (_, _) => keepAlive.Refresh(); + + // The half that was missing until now: a shell opening. SessionKeepAlive already heard the + // workspace announce a session ending, but nothing announced the opposite — a user who opened a + // shell and backgrounded the app had no foreground service at all, because the only wire in was the + // one for taking it down. TerminalSessionOpened is that other half, forwarded from + // VaultViewModel.SessionOpened, and without this line the service could never come up for a shell + // in the first place, which was precisely the promise this whole arrangement exists to keep. + viewModel.TerminalSessionOpened += (_, _) => keepAlive.Refresh(); + + keepAlive.Refresh(); + } + /// /// Writing to this phone's clipboard. /// diff --git a/src/DodoSSH.Client.Android/Platform/SessionForegroundService.cs b/src/DodoSSH.Client.Android/Platform/SessionForegroundService.cs index 61e5f05..216efb1 100644 --- a/src/DodoSSH.Client.Android/Platform/SessionForegroundService.cs +++ b/src/DodoSSH.Client.Android/Platform/SessionForegroundService.cs @@ -6,7 +6,7 @@ using global::Android.OS; namespace DodoSSH.Client.Android.Platform; /// -/// Keeps the process alive for as long as a shell or a transfer is live. +/// Keeps the process alive for as long as a shell, a transfer, or a connected Files session is live. /// /// /// @@ -41,6 +41,26 @@ internal sealed class SessionForegroundService : Service private const string ChannelId = "dodossh.sessions"; private const int NotificationId = 1; + // API 33+ requires the request to name a code the RequestPermissionsResult callback would be handed + // back — 1 is fine because this head never implements that callback at all, see + // RequestNotificationPermission's own remark for why a result is not worth listening for. + private const int NotificationPermissionRequestCode = 1; + + /// + /// Whether has run for this process without a matching + /// since — i.e. whether Android currently considers this service foregrounded. + /// + /// + /// Volatile because can run on whatever thread called + /// , while this is set from the binder thread Android delivers + /// service lifecycle callbacks on — two threads with no other synchronisation between them, and a stale + /// read here is the difference between updating a notification in place and calling + /// StartForegroundService on a service that is already running, which is what defect 3 was. + /// + private static volatile bool running; + + private static bool notificationPermissionRequested; + /// /// A bound service would tie the sessions' lifetime to a binding, which is the opposite of what is /// wanted here: the point is that they outlive whatever the user does with the interface. @@ -49,7 +69,9 @@ internal sealed class SessionForegroundService : Service public override StartCommandResult OnStartCommand(Intent? intent, StartCommandFlags flags, int startId) { - StartForeground(NotificationId, BuildNotification(intent?.GetStringExtra("summary") ?? "Working")); + running = true; + + StartForeground(NotificationId, BuildNotification(this, intent?.GetStringExtra("summary") ?? "Working")); // NotSticky: if Android does kill this process, the SSH connections died with it and there is // nothing to resume. Restarting the service would produce a notification claiming sessions that no @@ -58,14 +80,36 @@ internal sealed class SessionForegroundService : Service return StartCommandResult.NotSticky; } + /// /// + /// The other half of . Android calls this whether the service stopped itself or + /// was stopped from outside — 's down case calls StopService rather than + /// clearing the flag directly, so this override is the one place that actually knows the service has + /// gone, matching how is the one place that knows it has come up. + /// + public override void OnDestroy() + { + running = false; + + base.OnDestroy(); + } + + /// + /// /// Low importance on purpose. This notification is a receipt, not an alert — it exists because Android /// requires one, and because the user is entitled to know the app is holding connections open. Making /// it buzz would be a notification about nothing having happened. + /// + /// + /// Static, taking the it needs rather than reading this: the instance path + /// through passes the service itself, and the in-place update path through + /// has no service instance at all — only + /// — because posting to an already-running notification never touches the service's own lifecycle. + /// /// - private Notification BuildNotification(string summary) + private static Notification BuildNotification(Context context, string summary) { - var manager = (NotificationManager)GetSystemService(NotificationService)!; + var manager = (NotificationManager)context.GetSystemService(Context.NotificationService)!; if (OperatingSystem.IsAndroidVersionAtLeast(26)) { @@ -79,12 +123,12 @@ internal sealed class SessionForegroundService : Service } var reopen = PendingIntent.GetActivity( - this, + context, 0, - new Intent(this, typeof(MainActivity)).SetFlags(ActivityFlags.SingleTop), + new Intent(context, typeof(MainActivity)).SetFlags(ActivityFlags.SingleTop), PendingIntentFlags.Immutable | PendingIntentFlags.UpdateCurrent); - return new Notification.Builder(this, ChannelId) + return new Notification.Builder(context, ChannelId) .SetContentTitle("DodoSSH") .SetContentText(summary) .SetSmallIcon(global::Android.Resource.Drawable.IcDialogInfo) @@ -93,37 +137,138 @@ internal sealed class SessionForegroundService : Service .Build(); } - /// Starts or stops the service to match what is actually running. + /// Starts, stops, or refreshes the service's notification to match what is actually running. /// Shells with a live channel behind them. /// Transfers still moving bytes. - public static void Reconcile(int liveSessions, int activeTransfers) + /// Whether the Files screen holds a live, idle SFTP connection. + /// + /// Three outcomes, not the two a plain start-or-stop would have. Nothing live stops the service, as + /// always. Something live and the service not yet running starts it. Something live and the service + /// already running is the case that used to call StartForegroundService a second time, which on + /// API 31+ throws ForegroundServiceStartNotAllowedException the instant the app is backgrounded + /// — a transfer finishing in the pocket, one of two shells dying — crashing the process and taking the + /// remaining connections with it. That case — running, and the app backgrounded — now only posts a + /// fresh notification through the already holding the channel open, + /// which needs no foreground-start permission at all. A foregrounded refresh still prefers a real + /// start even over a service that looks like it is running; the inline remark below is why. + /// + public static void Reconcile(int liveSessions, int activeTransfers, bool holdsFileSession) { var context = PhoneEnvironment.Require(); - var intent = new Intent(context, typeof(SessionForegroundService)); - if (liveSessions == 0 && activeTransfers == 0) + if (liveSessions == 0 && activeTransfers == 0 && !holdsFileSession) { - context.StopService(intent); + context.StopService(new Intent(context, typeof(SessionForegroundService))); return; } // The summary says what is actually held, counted rather than generic — the same principle the // delete confirmations follow. "DodoSSH is running" would tell the user nothing they could act on. - intent.PutExtra("summary", Summarise(liveSessions, activeTransfers)); + var summary = Summarise(liveSessions, activeTransfers, holdsFileSession); - context.StartForegroundService(intent); + // In-place only while backgrounded, where it is the only legal move. Foregrounded, a real start is + // always allowed and is preferred even when `running` says the service is up: a disconnect followed + // by a quick reconnect can land here while the StopService just issued is still in flight, and + // posting to that dying service's notification would leave an orphan receipt over an unprotected + // process — restarting instead makes the flag's small lag harmless. A start on a service that + // really is running only re-delivers OnStartCommand, whose StartForeground updates the same + // notification anyway. CurrentActivity is the foreground signal: set on resume, cleared on pause. + if (running && PhoneEnvironment.CurrentActivity is null) + { + var manager = (NotificationManager)context.GetSystemService(Context.NotificationService)!; + manager.Notify(NotificationId, BuildNotification(context, summary)); + + return; + } + + RequestNotificationPermission(context); + Start(context, summary); } - private static string Summarise(int liveSessions, int activeTransfers) + /// + /// Split out of for the try/catch alone, which needs its own remark and would + /// otherwise crowd the three-way branch above it. + /// + private static void Start(Context context, string summary) { - var parts = new List(2); + var intent = new Intent(context, typeof(SessionForegroundService)); + intent.PutExtra("summary", summary); + + try + { + context.StartForegroundService(intent); + } + catch (Java.Lang.IllegalStateException) + { + // ForegroundServiceStartNotAllowedException (API 31+) derives from this, and reaching it here + // means a start-worthy transition — a shell opening, a Files connection completing, the first + // transfer landing in an empty queue — happened while the app was backgrounded, which is + // precisely when Android refuses a new foreground start. There is no retry that helps: by the + // time this catch runs, the moment such a start would have been allowed has already passed. + // Swallowing it is the honest choice and not just the available one — letting the exception + // propagate would crash the process and drop the very shells and transfers this service exists + // to keep alive. A process that keeps running unprotected outlives one that does not run at all. + } + } + + /// + /// Asks for the receipt notification's own permission, the first time this process actually has + /// something to show rather than at launch. + /// + /// + /// At most once per process, via — not to work around a + /// platform limit, since Android already refuses to show the dialogue twice, but because a second call + /// to RequestPermissions after the first is still pending is its own kind of noise. No result is + /// read back: there is nothing this class would do differently for a grant versus a refusal, so a + /// callback would exist only to be empty. What refusal costs is stated rather than hidden — the + /// notification stays invisible — and what it does not cost is the point: the service still starts, + /// still holds the process in the foreground, and the shells and transfers it protects are exactly as + /// safe as if the user had said yes. See the manifest's own comment on this permission. + /// + private static void RequestNotificationPermission(Context context) + { + // Isolated as its own guard clause rather than folded into the compound condition below: the + // platform-compatibility analyzer only recognises a version check as guarding what follows when it + // is the sole condition of its own early return, and PostNotifications is annotated API 33+. + if (!OperatingSystem.IsAndroidVersionAtLeast(33)) + { + return; + } + + // Read into a local rather than referenced again inside the lambda below: the guard clause above + // covers a direct call in this method's own body, but the platform-compatibility analyzer treats a + // lambda as reachable from anywhere and will not extend the guard across that boundary. Capturing + // the already-validated string sidesteps the false positive without weakening the actual check. + var postNotifications = global::Android.Manifest.Permission.PostNotifications; + + if (notificationPermissionRequested + || PhoneEnvironment.CurrentActivity is not { } activity + || context.CheckSelfPermission(postNotifications) == Permission.Granted) + { + return; + } + + notificationPermissionRequested = true; + + activity.RunOnUiThread(() => + activity.RequestPermissions([postNotifications], NotificationPermissionRequestCode)); + } + + private static string Summarise(int liveSessions, int activeTransfers, bool holdsFileSession) + { + var parts = new List(3); if (liveSessions > 0) { parts.Add(liveSessions == 1 ? "1 shell connected" : $"{liveSessions} shells connected"); } + if (holdsFileSession) + { + parts.Add("Files connected"); + } + if (activeTransfers > 0) { parts.Add(activeTransfers == 1 ? "1 transfer running" : $"{activeTransfers} transfers running"); diff --git a/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs b/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs index c20a78a..1f97d3b 100644 --- a/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs +++ b/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs @@ -17,22 +17,34 @@ namespace DodoSSH.Client.Android.Platform; /// tally kept here. It already knows that a session whose shell exited half an hour ago is not live, which /// a counter incremented on open and decremented on close would not. /// +/// +/// Three facts feed , not one: live shells, moving +/// transfers, and an idle-but-connected Files session. The last of those used to be missing entirely — +/// a shell survives backgrounding because somebody opened it, but a Files connection with nothing moving +/// looked, to this class, exactly like nothing being open at all. holdsFileSession below is that +/// gap closed, read the same way the other two facts are: asked, not cached. +/// /// internal sealed class SessionKeepAlive : IDisposable { private readonly TerminalWorkspace workspace; private readonly Func activeTransfers; + private readonly Func holdsFileSession; /// The live shells. /// - /// How many transfers are moving bytes. A delegate rather than a queue, because file transfer is out - /// of this head's first scope — see the decision in docs/android-port.md — and this is the seam it - /// will arrive through rather than a dependency taken before there is anything to depend on. + /// How many transfers are moving bytes. A delegate rather than a queue, because ownership of the + /// transfer queue stays with TransfersViewModel — this class only ever asks it a question. /// - public SessionKeepAlive(TerminalWorkspace workspace, Func activeTransfers) + /// + /// Whether the Files screen holds a live SFTP connection with nothing moving on it — the idle-but- + /// connected case a transfer count alone would miss. See TransfersViewModel.HasLiveFileSession. + /// + public SessionKeepAlive(TerminalWorkspace workspace, Func activeTransfers, Func holdsFileSession) { this.workspace = workspace; this.activeTransfers = activeTransfers; + this.holdsFileSession = holdsFileSession; // Raised on whatever thread the pump unwound on, which is fine: starting and stopping a service is // a binder call and needs no particular thread. Nothing here touches the interface. @@ -41,13 +53,14 @@ internal sealed class SessionKeepAlive : IDisposable /// Re-reads the counts and starts or stops the service to match. /// - /// Called after anything that could change either count — opening a shell, closing a tab, a transfer - /// finishing. Calling it when nothing changed is free: reconciling to the state it is already in is - /// either a redundant startForegroundService on a running service or a stopService on a - /// stopped one, and Android treats both as no-ops. + /// Called after anything that could change any of the three facts — opening a shell, closing a tab, a + /// transfer finishing, a Files connection opening or closing. Calling it when nothing changed is free: + /// reconciling to the state it is already in is either a redundant startForegroundService — or, + /// now, a redundant notification post — on a running service, or a stopService on a stopped one, + /// and Android treats all of those as no-ops. /// public void Refresh() => - SessionForegroundService.Reconcile(workspace.LiveSessionCount, activeTransfers()); + SessionForegroundService.Reconcile(workspace.LiveSessionCount, activeTransfers(), holdsFileSession()); /// public void Dispose() @@ -56,7 +69,7 @@ internal sealed class SessionKeepAlive : IDisposable // The notification goes with the composition root. Leaving it up over a process that is shutting // down is how an SSH client acquires a reputation for a notification you cannot get rid of. - SessionForegroundService.Reconcile(0, 0); + SessionForegroundService.Reconcile(0, 0, false); } private void OnSessionEnded(object? sender, TerminalSessionEndedEventArgs e) => Refresh(); diff --git a/src/DodoSSH.Client.Android/Properties/AndroidManifest.xml b/src/DodoSSH.Client.Android/Properties/AndroidManifest.xml index 70f4063..1aef838 100644 --- a/src/DodoSSH.Client.Android/Properties/AndroidManifest.xml +++ b/src/DodoSSH.Client.Android/Properties/AndroidManifest.xml @@ -12,7 +12,12 @@ - + diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index d3a6371..3eaae25 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs @@ -525,6 +525,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp // list. Detached in DisposeAsync, which is the only point either of them ends. this.workspace.SessionEnded += OnWorkspaceSessionEnded; this.workspace.FontSizeStepRequested += OnFontSizeStepRequested; + this.workspace.RendererReattached += OnRendererReattached; settings = new ClientSettingsStore(paths); @@ -635,6 +636,31 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp private void OnFontSizeStepRequested(object? sender, TerminalFontSizeStepEventArgs e) => Dispatcher.UIThread.Post(() => StepTerminalFontSize(e.Step)); + /// + /// + /// Marshalled for the same reason as : this arrives on the data + /// plane's socket-accept thread, and both properties it reads here — and + /// — are bound to by the interface. + /// + /// + /// fires once the workspace has replayed what it + /// owns — the live sessions. Font size and the choice of active tab are not the workspace's to know; + /// they live here, so this is the other half of putting a reattached page back the way it was. The size + /// is sent exactly as sends it at startup, because nothing + /// has changed — the page has merely forgotten, and this is only a reminder. + /// + /// + private void OnRendererReattached(object? sender, EventArgs e) => + Dispatcher.UIThread.Post(() => + { + _ = workspace.SetFontSizeAsync(TerminalFontSize, CancellationToken.None).AsTask(); + + if (SelectedTab is { } tab) + { + _ = workspace.ActivateSessionAsync(tab.SessionId, CancellationToken.None).AsTask(); + } + }); + [ObservableProperty] private ShellState state = ShellState.Starting; @@ -3051,6 +3077,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp workspace.SessionEnded -= OnWorkspaceSessionEnded; workspace.FontSizeStepRequested -= OnFontSizeStepRequested; + workspace.RendererReattached -= OnRendererReattached; transfers.PropertyChanged -= OnTransfersPropertyChanged; // Stopped here rather than left to the process exiting with it: the loop holds no vault key and diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index c63bfd8..4af48e5 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -620,6 +620,19 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo [ObservableProperty] private string? connectedCipher; + /// + /// Whether there is a live SFTP connection this session would lose by dying — the phone's foreground- + /// service question, not the desktop's. + /// + /// + /// is already the fact that tells a host apart from a bucket, because only + /// a host set it — a bucket is HTTP, per-request, and closes nothing a dying process would have kept + /// open, so it answers false here even while is true. Android reads this to + /// decide whether an idle Files screen with no transfer moving still needs the process kept alive; the + /// desktop has no such question because nothing stops its process for having gone quiet. + /// + internal bool HasLiveFileSession => IsConnected && ConnectedCipher is not null; + /// The accepted host key's algorithm, e.g. ssh-ed25519. See . [ObservableProperty] private string? connectedHostKeyAlgorithm; @@ -745,13 +758,18 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo internal ObservableCollection Transfers { get; } = []; - /// Raised on the UI thread whenever a transfer appears or changes state. + /// + /// Raised on the UI thread whenever a transfer appears or changes state, or a host or bucket connects or + /// disconnects. + /// /// /// For a head that has to tell the operating system what this process is doing — Android's foreground - /// service, which must be up for as long as bytes are moving and down afterwards. An event rather than - /// letting that head watch itself: the collection announces rows arriving and - /// leaving, and the transition that matters most is neither of those but a row going from RUNNING to - /// DONE without moving. + /// service, which must be up for as long as bytes are moving, or a host session sits open, and down + /// afterwards. An event rather than letting that head watch itself: the + /// collection announces rows arriving and leaving, and the transition that matters most is neither of + /// those but a row going from RUNNING to DONE without moving. Connecting and disconnecting are the other + /// two transitions the service cares about — see — and neither touches + /// at all, so they need this same announcement made by hand. /// internal event EventHandler? ActivityChanged; @@ -1075,6 +1093,13 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo // far as the remote's own auth.log is concerned, so a log of ours that omitted it would disagree with // the host's — and anybody comparing the two would be right to believe the host. connected = (ConnectedTo, row.Label, row.EntityId, TimeProvider.System.GetUtcNow()); + + // Raised here rather than from OnIsConnectedChanged, on purpose: IsConnected is set first, above, + // and ConnectedCipher second — a partial method firing off the first assignment would read + // HasLiveFileSession against a ConnectedCipher still holding whatever the previous session left + // there. Only at the end of this method are both facts actually true together. + OnPropertyChanged(nameof(HasLiveFileSession)); + ActivityChanged?.Invoke(this, EventArgs.Empty); } /// @@ -1856,6 +1881,11 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo RemoteTrail.Clear(); SelectedRemoteEntry = null; + // Same ordering reason as the raise at the end of MarkHostConnected: both properties this reads are + // already null above, so the raise belongs after them rather than in OnIsConnectedChanged. This also + // covers OpenBucketAsync, which calls this method first and never itself turns HasLiveFileSession on. + OnPropertyChanged(nameof(HasLiveFileSession)); + ActivityChanged?.Invoke(this, EventArgs.Empty); } /// diff --git a/src/DodoSSH.Client.Shell/WebAssets/terminal.js b/src/DodoSSH.Client.Shell/WebAssets/terminal.js index 9d41115..c2d4664 100644 --- a/src/DodoSSH.Client.Shell/WebAssets/terminal.js +++ b/src/DodoSSH.Client.Shell/WebAssets/terminal.js @@ -3,7 +3,7 @@ /* The renderer half of the terminal data plane. - Two things here are load-bearing and easy to get wrong: + Three things here are load-bearing and easy to get wrong: 1. Output is acknowledged from term.write's completion callback, never on receipt. The acknowledgement returns flow-control credit to the host, so acknowledging early would tell @@ -15,6 +15,14 @@ partial sequences across writes. Decoding here would corrupt any multi-byte character that 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 + 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 + that already has a pane is left alone) and a SESSION_OPENED frame carries a flag telling this + page whether it is a replay: nothing to do for a pane that is still here, and a short banner + for one that is not, because that pane's scrollback genuinely did not survive. */ const SERVER_OUTPUT = 1; @@ -33,6 +41,25 @@ const CLIENT_FONT_SIZE_STEP = 4; const HEADER_LENGTH = 5; const SCROLLBACK_LINES = 5000; +/* + How long to wait before trying the socket again, and how that wait grows. Starting quick matters + because the ordinary case is a page that just finished loading after its WebView came back — the + host's listener has been sitting there the whole time — and capping it matters because there is no + point spacing attempts further apart than a person notices. Forever rather than giving up, because + giving up would need a way to try again and there is none better than the one already here: the page + dies with the app. +*/ +const RECONNECT_INITIAL_DELAY_MS = 1000; +const RECONNECT_MAX_DELAY_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 + does not exist yet means the page reloaded and lost it, and createSession has no way to know which + of its callers that is. +*/ +const REPLAY_BANNER = '\x1b[38;5;244m── the view reconnected; earlier output stayed on the host ──\x1b[0m\r\n'; + /* The size panes are created at, until the host says otherwise — which it does as soon as it has read the stored preference, usually before the first session exists. Kept here as well so a pane opened @@ -192,7 +219,19 @@ function handleKey(event) { return true; } +/** + * Builds a pane for a session, or returns the one already there. + * + * Idempotent because a replay can land on a page that never lost its pane — the socket dropped and + * came back, but this page's own process survived — and asking for a session that already has a pane + * must not build a second one on top of it, orphaning the first one's WebGL context and scrollback. + */ function createSession(sessionId) { + const existing = sessions.get(sessionId); + if (existing) { + return existing; + } + const pane = document.createElement('div'); pane.className = 'pane'; pane.dataset.sessionId = String(sessionId); @@ -290,10 +329,23 @@ function handleFrame(buffer) { const payload = new Uint8Array(buffer, HEADER_LENGTH); switch (opcode) { - case SERVER_SESSION_OPENED: - createSession(sessionId); + case SERVER_SESSION_OPENED: { + // Checked before createSession, which would otherwise erase the answer by creating the pane + // this check is asking about. + const hadPaneAlready = sessions.has(sessionId); + const session = createSession(sessionId); + + // Byte 1 means the host is replaying a session that existed before this socket attached — see + // TerminalWorkspace.ReplayAfterAttachAsync. A replay landing on a pane that is still here has + // nothing left to do beyond the idempotent create above; one landing on a pane that is not means + // this page reloaded and that pane's scrollback went with it, which is worth a line saying so. + if (payload.length > 0 && payload[0] === 1 && !hadPaneAlready) { + session.term.write(REPLAY_BANNER); + } + setStatus(''); break; + } case SERVER_OUTPUT: { const session = sessions.get(sessionId) ?? createSession(sessionId); @@ -414,6 +466,31 @@ function handleFrame(buffer) { } } +/** @type {number | null} */ +let reconnectTimer = null; +let reconnectDelay = RECONNECT_INITIAL_DELAY_MS; + +/** + * Tries the socket again after a wait, unless a try is already pending. + * + * The guard is what keeps 'close' and 'error' from stacking two timers for one failure — a socket + * that fails to open typically fires both, and each would otherwise schedule its own reconnect. + */ +function scheduleReconnect() { + if (reconnectTimer !== null) { + return; + } + + setStatus('Reconnecting the terminal view…'); + + reconnectTimer = setTimeout(() => { + reconnectTimer = null; + connect(); + }, reconnectDelay); + + reconnectDelay = Math.min(reconnectDelay * 2, RECONNECT_MAX_DELAY_MS); +} + function connect() { const token = root.dataset.token; const url = root.dataset.socket; @@ -423,16 +500,22 @@ function connect() { socket = new WebSocket(url, ['dodossh.terminal.v1', `token.${token}`]); socket.binaryType = 'arraybuffer'; - socket.addEventListener('open', () => setStatus('')); + socket.addEventListener('open', () => { + setStatus(''); + + // Back to the quick attempt for whatever the next failure turns out to be. Kept slow between + // attempts within one outage, reset once the outage is actually over. + reconnectDelay = RECONNECT_INITIAL_DELAY_MS; + }); + socket.addEventListener('message', (event) => handleFrame(event.data)); - socket.addEventListener('close', () => { - setStatus('Disconnected from DodoSSH.'); - }); - - socket.addEventListener('error', () => { - setStatus('The terminal connection failed.'); - }); + // 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); } // One observer for the whole root rather than one per pane: resizes arrive in bursts while a 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/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs index 79a6dc9..098b6b4 100644 --- a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs +++ b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs @@ -101,6 +101,13 @@ public sealed class TerminalWorkspace : IAsyncDisposable private readonly Lock sessionGate = new(); private readonly CancellationTokenSource lifetime = new(); + /// + /// The payload that marks a frame as a replay rather + /// than a fresh open. A one-byte non-empty payload, so terminal.js's existing length check (empty + /// payload for a real open) tells the two apart without a second opcode. + /// + private static readonly byte[] ReplayMarker = [1]; + private uint nextSessionId = 1; private Task? server; private int disposed; @@ -125,6 +132,12 @@ public sealed class TerminalWorkspace : IAsyncDisposable // came from. Nothing here decides anything about the size: the shell owns it, because the shell is // what remembers it between launches. dataPlane.FontSizeStepRequested += (_, e) => FontSizeStepRequested?.Invoke(this, e); + + // Fire-and-forget: this fires on the socket-accept thread, in the middle of the data plane's own + // handshake handling, and has no business making that wait on however long a replay takes. See + // ReplayAfterAttachAsync for what "replay" means and why racing the fresh page's own first frames + // is harmless. + dataPlane.SocketAttached += (_, _) => _ = ReplayAfterAttachAsync(); } /// @@ -223,6 +236,26 @@ public sealed class TerminalWorkspace : IAsyncDisposable } } + /// + /// A live session's flow-control window, or null when the id names no session this workspace still has + /// open. + /// + /// + /// A test seam rather than something the shell has ever needed: nothing outside this assembly has a + /// reason to see a pump's credit window rather than what the transport does with it, but + /// 's reset of that window on reattach is exactly the kind of thing + /// that is easy to get backwards, and worth asserting directly rather than only through its side + /// effects. Internal rather than public, reachable from the test assembly through the + /// InternalsVisibleTo this project already declares for it. + /// + internal CreditWindow? CreditsFor(uint sessionId) + { + lock (sessionGate) + { + return sessions.TryGetValue(sessionId, out var session) ? session.Pump.Credits : null; + } + } + /// /// Raised with the session id when a shell ends on its own. /// @@ -254,6 +287,24 @@ public sealed class TerminalWorkspace : IAsyncDisposable /// public event EventHandler? FontSizeStepRequested; + /// + /// Raised once a (re)attached renderer has been sent everything this workspace owns for it. + /// + /// + /// + /// The workspace's own share of "put the page back the way it was" is the sessions — each live one gets + /// its SessionOpened frame again, done by the time this fires. What is left is what the workspace + /// has no business owning: the font size and which tab is selected are both remembered by the shell, not + /// by a terminal, so this is the seam the shell uses to re-push them. See + /// MainWindowViewModel's subscription for the other half. + /// + /// + /// Raised on the socket-accept thread, same as that + /// triggers it — a handler that touches a view model has to marshal. + /// + /// + public event EventHandler? RendererReattached; + /// Starts the loopback listener. public void Start() => server = dataPlane.RunAsync(lifetime.Token); @@ -563,6 +614,71 @@ public sealed class TerminalWorkspace : IAsyncDisposable lifetime.Dispose(); } + /// + /// Rebuilds a freshly (re)attached page's idea of what is running, then tells the shell to rebuild its + /// own. + /// + /// + /// + /// Runs on the socket-accept thread that raised — the + /// constructor wires it up fire-and-forget for exactly that reason, so this method owns its own error + /// handling rather than leaving an unobserved exception for nobody to see. + /// + /// + /// Every live session — one whose Run has not completed — gets two things. Its credit window is + /// reset, because whatever was outstanding was reserved against bytes sent to a page that is now gone; + /// the acknowledgement that would return that credit died with it, and without this reset the session + /// would stall the moment 256 KiB of history had accumulated. And it gets its SessionOpened frame + /// again, marked with so the page can tell a reattach from a session that is + /// genuinely new — the same frame a page that survived the socket drop already has a pane for, and one a + /// reloaded page does not. + /// + /// + /// A session whose shell has already ended gets nothing here. Its scrollback lived only in the page that + /// is gone, and sending a frame that implied otherwise would be exactly the kind of dishonesty this + /// fix is supposed to remove, not add. The tab strip still shows that session ended; nothing about this + /// method changes what or report. + /// + /// + /// This can race the fresh page's own first frames — an early resize, an acknowledgement for output it + /// already had. That is harmless: every frame in both directions names its session, delivery order + /// within a session is preserved by both xterm and the socket, and a frame for a pane the page has not + /// created yet is simply dropped, the same as any frame for a session it does not know — see + /// terminal.js's handleFrame. + /// + /// + private async Task ReplayAfterAttachAsync() + { + KeyValuePair[] live; + + lock (sessionGate) + { + live = [.. sessions.Where(entry => !entry.Value.Run.IsCompleted)]; + } + + try + { + foreach (var (sessionId, session) in live) + { + session.Pump.Credits.Reset(); + + await dataPlane + .SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.SessionOpened, sessionId, ReplayMarker), + CancellationToken.None) + .ConfigureAwait(false); + } + + RendererReattached?.Invoke(this, EventArgs.Empty); + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + // Best-effort, same as every other fire-and-forget path here: a page that dies again mid-replay + // leaves nothing worse than the problem this method exists to fix, and there is no caller on + // this thread left to hand a failure to. + } + } + private async Task RunSessionAsync(uint sessionId, TerminalSessionPump pump) { try diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 6f24e3a..9cb60be 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -8114,6 +8114,57 @@ public sealed class ShellFlowTests : IAsyncLifetime shell.Transfers.HasConnectedPins.ShouldBeFalse(); } + /// + /// The phone's foreground-service question, proven at the view model rather than through Android: a + /// connect that opens an SFTP session is exactly the transition SessionKeepAlive needs to hear + /// about even when no transfer ever moves — see 's own + /// remark for why the queue's own raise, in OnTransferChanged, cannot cover a connect that never + /// touches Transfers at all. + /// + [Fact] + public async Task ConnectingATransfersHost_RaisesActivityChangedAndTurnsOnHasLiveFileSession() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts); + shell.Transfers.SelectedHost = shell.Transfers.Hosts[0]; + + var raised = 0; + shell.Transfers.ActivityChanged += (_, _) => raised++; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + + shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); + shell.Transfers.HasLiveFileSession.ShouldBeTrue(); + raised.ShouldBeGreaterThan(0); + } + + /// + /// The other half: a disconnect is as much a transition the service must hear about as a connect is, + /// because it is the moment the connection promised + /// was open stops being true — and the foreground service would otherwise keep the process alive over a + /// session that has already closed. + /// + [Fact] + public async Task DisconnectingTheTransfersScreen_RaisesActivityChangedAndTurnsOffHasLiveFileSession() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts); + shell.Transfers.SelectedHost = shell.Transfers.Hosts[0]; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + shell.Transfers.HasLiveFileSession.ShouldBeTrue(); + + var raised = 0; + shell.Transfers.ActivityChanged += (_, _) => raised++; + + await shell.Transfers.DisconnectCommand.ExecuteAsync(null); + + shell.Transfers.HasLiveFileSession.ShouldBeFalse(); + raised.ShouldBeGreaterThan(0); + } + /// /// A bucket is an IRemoteFileStore with no HostSecret underneath it, so there is no /// PinnedPaths to read at all — see 's own remark. @@ -8137,15 +8188,56 @@ public sealed class ShellFlowTests : IAsyncLifetime vault.BucketEditorRegion = "eu-west-1"; await vault.SaveObjectStoreCommand.ExecuteAsync(null); + // Remote is what ConnectAsync branches on, and Attach's RefreshHosts has already auto-selected the + // host ReadyToConnectAsync left in the picker — without this line the command below dialled that + // host, and every assertion here passed only because that host happens to have no pins either. The + // ConnectedTo check is the proof the bucket path was actually taken. + shell.Transfers.Remote = RemoteKind.Bucket; shell.Transfers.SelectedBucket = shell.Transfers.Buckets[0]; await shell.Transfers.ConnectCommand.ExecuteAsync(null); + shell.Transfers.ConnectedTo.ShouldBe("s3://backups"); shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); shell.Transfers.ConnectedPinnedPaths.ShouldBeEmpty(); shell.Transfers.HasConnectedPins.ShouldBeFalse(); } + /// + /// A bucket is HTTP, per-request, with nothing open that a dying process would lose — see + /// 's own remark. IsConnected alone would have + /// answered this wrongly, which is exactly why the flag reads ConnectedCipher as well: nothing + /// underneath a bucket ever sets it. + /// + [Fact] + public async Task ConnectingABucket_LeavesHasLiveFileSessionOff() + { + var vault = await ReadyToConnectAsync(); + + shell.Transfers.Attach(vault, knownHosts, buckets: new FakeObjectStoreFactory()); + + vault.NewObjectStoreCommand.Execute(null); + vault.BucketEditorLabel = "Backups"; + vault.BucketEditorBucket = "backups"; + vault.BucketEditorAccessKeyId = "AKIAEXAMPLE"; + vault.BucketEditorSecretAccessKey = "a-secret-access-key"; + vault.BucketEditorRegion = "eu-west-1"; + await vault.SaveObjectStoreCommand.ExecuteAsync(null); + + // ReadyToConnectAsync already left a host in the picker, and Attach's own RefreshHosts auto-selects + // it — so without this the CONNECT command below would dial that host rather than open the bucket, + // and a host with no pins would make ConnectedPinnedPathsEmpty-style assertions pass for the wrong + // reason. Remote is what ConnectAsync actually branches on. + shell.Transfers.Remote = RemoteKind.Bucket; + shell.Transfers.SelectedBucket = shell.Transfers.Buckets[0]; + + await shell.Transfers.ConnectCommand.ExecuteAsync(null); + + shell.Transfers.ConnectedTo.ShouldBe("s3://backups", "proof this opened the bucket rather than the host"); + shell.Transfers.IsConnected.ShouldBeTrue(shell.Transfers.Status); + shell.Transfers.HasLiveFileSession.ShouldBeFalse(); + } + /// A bucket that opens and lists as empty, so a bucket connect can be proven with no network. private sealed class FakeObjectStoreFactory : IObjectStoreFactory { diff --git a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs index abbbbf0..ee33752 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/FakeShellSession.cs @@ -8,6 +8,8 @@ internal sealed class FakeShellSession : ISshShellSession private readonly List written = []; private readonly Lock gate = new(); + private readonly bool blockReads; + private long remaining; private byte pattern; @@ -16,7 +18,19 @@ internal sealed class FakeShellSession : ISshShellSession /// endless producer, which is what a runaway remote process looks like — those sessions are ended /// by disposing the pump rather than by running out of data. /// - internal FakeShellSession(long bytesToProduce = 0) => remaining = bytesToProduce; + /// + /// True for a shell that is open and live but has nothing to say — an idle prompt, rather than either + /// end of the "produces bytes" and "hit end of stream" spectrum + /// covers. then blocks until cancelled, which is what a real idle SSH channel's + /// read does. Exists for tests that need a session whose Run stays live without a background + /// read loop racing the test for control of the pump's credit window — see the reattach tests in + /// TerminalWorkspaceTests. + /// + internal FakeShellSession(long bytesToProduce = 0, bool blockReads = false) + { + remaining = bytesToProduce; + this.blockReads = blockReads; + } /// public bool IsOpen { get; private set; } = true; @@ -52,6 +66,13 @@ internal sealed class FakeShellSession : ISshShellSession { ReadCount++; + if (blockReads) + { + // Never completes on its own. The only way out is the same way a real blocked read ends: the + // token being cancelled, which is what disposing the pump does. + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken).ConfigureAwait(false); + } + await Task.Yield(); cancellationToken.ThrowIfCancellationRequested(); @@ -109,7 +130,8 @@ internal sealed class FakeShellSession : ISshShellSession /// workspace is the layer that decides when a session is over, and that decision is what needs a /// connection whose shell can be made to end on cue. /// -internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) : ISshConnectionFactory +internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue, bool blockShellReads = false) + : ISshConnectionFactory { /// Connections handed out, in order. internal List Connections { get; } = []; @@ -119,7 +141,7 @@ internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) SshConnectionRequest request, CancellationToken cancellationToken) { - var connection = new FakeConnection(request, bytesPerShell); + var connection = new FakeConnection(request, bytesPerShell, blockShellReads); Connections.Add(connection); return Task.FromResult(connection); @@ -127,7 +149,8 @@ internal sealed class FakeConnectionFactory(long bytesPerShell = long.MaxValue) } /// A connection that opens fake shells and records its own disposal. -internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell) : ISshConnection +internal sealed class FakeConnection(SshConnectionRequest request, long bytesPerShell, bool blockShellReads = false) + : ISshConnection { /// public bool IsConnected { get; private set; } = true; @@ -148,7 +171,7 @@ internal sealed class FakeConnection(SshConnectionRequest request, long bytesPer /// public Task OpenShellAsync(TerminalSize size, CancellationToken cancellationToken) { - Shell = new FakeShellSession(bytesPerShell); + Shell = new FakeShellSession(bytesPerShell, blockShellReads); return Task.FromResult(Shell); } 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 ---- diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs index feac62b..52dca61 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs @@ -1,3 +1,6 @@ +using System.Globalization; +using System.Net.WebSockets; +using System.Text; using DodoSSH.Client.Ssh; namespace DodoSSH.Client.Terminal.Tests; @@ -291,6 +294,85 @@ public sealed class TerminalWorkspaceTests connections.Connections.ShouldAllBe(connection => connection.IsDisposed); } + // ---- Reattach ---- + + /// + /// + /// The scenario the whole fix exists for: a page that lost its socket — killed WebView renderer, or + /// simply a reload — reattaches, and the session that was already running has to come back rather than + /// sit there forever with its output going nowhere. + /// + /// + /// The session's shell blocks on every read rather than producing output, which is what an idle prompt + /// looks like and — for this test — is what keeps its Run live without a background read loop + /// competing with this test over the credit window's exact value. + /// + /// + /// Neither assertion below polls, deliberately. The "before" one does not need to: reserving credit is + /// a synchronous call, so it is true the instant it returns. The "after" one does not need to either, + /// for a subtler reason — calls + /// Credits.Reset() and only then awaits sending the replay frame for that same session, with no + /// suspension between the two, so by the time this test has received that frame the reset has + /// necessarily already happened. A poll here would only have hidden a real ordering bug behind a + /// generous timeout instead of catching it. + /// + /// + [Fact] + public async Task ANewRenderer_ReplaysTheLiveSessionAndResetsItsCredits() + { + var connections = new FakeConnectionFactory(blockShellReads: true); + + await using var workspace = CreateWorkspace(connections); + workspace.Start(); + + using var first = await ConnectRendererAsync(workspace); + + var sessionId = await workspace.OpenSessionAsync( + Request(), TerminalSize.Default, TestContext.Current.CancellationToken); + + // The session's own opening frame, sent as soon as the pump starts running. Not a replay, and not + // what this test is about — read and discarded so it cannot be confused for one below. + await ReceiveFrameAsync(first); + + var credits = workspace.CreditsFor(sessionId).ShouldNotBeNull(); + credits.TryReserve(4096); + credits.Outstanding.ShouldBeGreaterThanOrEqualTo( + 4096, "the pump's own read loop may have reserved a buffer's worth on top of this"); + + using var second = await ConnectRendererAsync(workspace); + + var replay = await ReceiveFrameAsync(second); + replay.Opcode.ShouldBe((byte)TerminalServerOpcode.SessionOpened); + replay.SessionId.ShouldBe(sessionId); + replay.Payload.ShouldBe(new byte[] { 1 }, "a replay is flagged so the page can tell it apart from a fresh open"); + + credits.Outstanding.ShouldBe(0, "the replay frame above cannot have been sent before the reset that precedes it"); + } + + /// + /// The other half of a reattach: the workspace has replayed what it owns, and this is the seam the + /// shell uses to replay what it owns instead — the font size and the selected tab, neither of which a + /// terminal session knows anything about. MainWindowViewModel's subscription is what actually + /// does that; this only asserts that the workspace hands it the chance to. + /// + [Fact] + public async Task ANewRenderer_RaisesRendererReattached() + { + var connections = new FakeConnectionFactory(); + + await using var workspace = CreateWorkspace(connections); + workspace.Start(); + + using var first = await ConnectRendererAsync(workspace); + + var reattachedCount = 0; + workspace.RendererReattached += (_, _) => Interlocked.Increment(ref reattachedCount); + + using var second = await ConnectRendererAsync(workspace); + + await WaitUntilAsync(() => Volatile.Read(ref reattachedCount) > 0); + } + // ---- Helpers ---- /// @@ -362,12 +444,76 @@ public sealed class TerminalWorkspaceTests private static InMemoryTerminalAssetProvider StubAssets() => new(new Dictionary(StringComparer.Ordinal) { - [TerminalDataPlane.PagePath] = new("text/html; charset=utf-8", ""u8.ToArray()), + // The placeholders, not a token and URL already filled in — the reattach tests below have to + // connect a real renderer, and doing that by reading them back out of the served page is what + // proves the workspace serves a page a real renderer could actually attach with, rather than + // one that merely looks servable. + [TerminalDataPlane.PagePath] = new( + "text/html; charset=utf-8", + Encoding.UTF8.GetBytes( + $"")), }); private static SshConnectionRequest Request() => 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. + /// + private static async Task ConnectRendererAsync(TerminalWorkspace workspace) + { + using var http = new HttpClient(); + var page = await http.GetStringAsync(workspace.PageUrl, TestContext.Current.CancellationToken); + + var token = ExtractAttribute(page, "data-token"); + var socketUrl = ExtractAttribute(page, "data-socket"); + + var client = new ClientWebSocket(); + client.Options.AddSubProtocol(TerminalDataPlane.SubProtocol); + client.Options.AddSubProtocol($"token.{token}"); + client.Options.SetRequestHeader( + "Origin", + string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{workspace.PageUrl.Port}")); + + try + { + await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken); + } + catch + { + client.Dispose(); + throw; + } + + return client; + } + + private static string ExtractAttribute(string html, string name) + { + var marker = $"{name}=\""; + var start = html.IndexOf(marker, StringComparison.Ordinal) + marker.Length; + var end = html.IndexOf('"', start); + + return html[start..end]; + } + + private static async Task<(byte Opcode, uint SessionId, byte[] Payload)> ReceiveFrameAsync( + ClientWebSocket socket) + { + var buffer = new byte[64 * 1024]; + + var result = await socket.ReceiveAsync(buffer.AsMemory(), TestContext.Current.CancellationToken); + + TerminalFrame.TryRead(buffer.AsSpan(0, result.Count), out var opcode, out var sessionId, out var payload) + .ShouldBeTrue(); + + return (opcode, sessionId, payload.ToArray()); + } + /// /// Polled rather than awaited on a task, because the point is what an observer of the property /// sees: the pump ends on a thread of its own, and the count has to catch up without anyone