From 810bc48d3f11fea6c59db903ea02d5980111d62c Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:14:10 +0200 Subject: [PATCH 1/7] Tell the keep-alive wire when the Files session opens and closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HasLiveFileSession answers the phone's foreground-service question — is there a connection here that dying with the process would sever — and a bucket answers no, because HTTP holds nothing open. ActivityChanged now also fires at the end of MarkHostConnected and CloseSessionAsync, where both facts it reads are finally true together. Also makes the bucket pins test actually open a bucket: it never set Remote, so CONNECT dialled the auto-selected host, and its assertions passed only because that host had no pins either. --- .../ViewModels/TransfersViewModel.cs | 40 +++++++- .../ShellFlowTests.cs | 92 +++++++++++++++++++ 2 files changed, 127 insertions(+), 5 deletions(-) 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/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 { From 48ea5e22d5477037eb66c851322b3dd0eac7cffe Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:14:17 +0200 Subject: [PATCH 2/7] Actually keep the phone's sessions alive when the app is backgrounded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The foreground service existed, and four defects in its wiring meant it mostly did not run. A shell opening was never announced to it — only the ending was — so the service never came up for a shell at all. An idle connected Files session counted as nothing. Every refresh restarted the service, which Android 12+ answers with a crash the moment the app is backgrounded — a transfer finishing in the pocket took the remaining connections with it. And POST_NOTIFICATIONS was declared but never requested, so on Android 13+ the receipt was silently invisible. Updates while backgrounded now go through the notification manager; a foregrounded refresh still prefers a real start, so a stop still in flight cannot leave an orphan receipt over an unprotected process. --- src/DodoSSH.Client.Android/App.axaml.cs | 73 +++++--- .../Platform/SessionForegroundService.cs | 177 ++++++++++++++++-- .../Platform/SessionKeepAlive.cs | 33 +++- .../Properties/AndroidManifest.xml | 7 +- 4 files changed, 241 insertions(+), 49 deletions(-) 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 @@ - + From 3977f6887065e2e5d985533b6ead52a6d2e9715e Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:14:24 +0200 Subject: [PATCH 3/7] Record the keep-alive corrections in the port notes and the manual checks The port doc's backgrounding decision now carries the four corrections rather than describing a wiring that was not true, and Phase 14 gains the checks a phone can actually run: a backgrounded shell surviving, an idle Files connection surviving, the permission ask arriving at the first thing worth showing, and a refusal costing the notification and nothing else. --- docs/android-port.md | 41 ++++++++++++++++++++++++++++++++--- docs/manual-checks.md | 50 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/docs/android-port.md b/docs/android-port.md index 6a137b4..de71e79 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: @@ -280,13 +283,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 +553,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..49d6bcf 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -2075,9 +2075,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 From 095774c49803982a02a651d283e26c1e5d14fd84 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:54:20 +0200 Subject: [PATCH 4/7] Take a returning renderer's socket over instead of refusing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One attach per process was WebView2's truth, not Android's: the phone kills the WebView's renderer independently of the app process, the page reloads, and its fresh socket was answered 409 by a guard that never reset — with no way back short of restarting the app. Only our own page knows the token, so a second valid upgrade is that page returning; it now displaces the old socket, which may never notice it is dead on its own, since a killed renderer sends no FIN. A send into the dead socket also no longer escapes as a fault. It used to unwind the pump's flush loop, after which nothing drained the credit window and the still-live shell froze behind it for good — including the BCL quirk where such a send surfaces as an OperationCanceledException nobody's token asked for. --- .../TerminalDataPlane.cs | 102 +++++++++++++++--- .../TerminalDataPlaneTests.cs | 85 ++++++++++++++- 2 files changed, 169 insertions(+), 18 deletions(-) diff --git a/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs b/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs index 50bdd91..6e88232 100644 --- a/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs +++ b/src/DodoSSH.Client.Terminal/TerminalDataPlane.cs @@ -66,7 +66,6 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable new(TaskCreationOptions.RunContinuationsAsynchronously); private WebSocket? socket; - private int accepted; private int disposed; /// Where the renderer's files come from. @@ -106,6 +105,26 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable /// public event EventHandler? FontSizeStepRequested; + /// + /// Raised after a socket attaches — the first one, and every later takeover. + /// + /// + /// + /// Raised after has been swapped in but before starts + /// consuming it, on the socket-accept thread — the same thread that is in the middle of + /// for this connection. is this event's one + /// subscriber, and it uses the ordering to replay session state before anything the fresh page sends + /// (a resize, an early acknowledgement) can be dispatched; see its remark for why the two racing is + /// harmless regardless. + /// + /// + /// Unlike , which resolves once and answers "has a renderer ever attached" + /// for , this fires every time — because a takeover + /// is exactly the case was never meant to describe again. + /// + /// + public event EventHandler? SocketAttached; + /// Registers a session so inbound frames can be routed to it. public void Register(uint sessionId, TerminalSessionPump pump) { @@ -148,8 +167,9 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable // Each connection on its own task, and deliberately not awaited. An upgraded WebSocket // lives for the whole session, so handling connections in sequence would leave the accept // loop parked inside the receive loop and every later request unanswered — the page's - // script and stylesheet among them. Concurrency needs no coordination here because the - // single-attach guard is an interlocked exchange. + // script and stylesheet among them. Two upgrades racing each other need no coordination + // here either, because the takeover in UpgradeAsync swaps the shared socket field with an + // interlocked exchange rather than assuming it is the only writer. _ = HandleConnectionAsync(client, linked.Token); } } @@ -192,6 +212,29 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable .SendAsync(frame, WebSocketMessageType.Binary, endOfMessage: true, cancellationToken) .ConfigureAwait(false); } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Not a cancellation despite the type: .NET's ManagedWebSocket wraps a send that fails because + // the underlying connection is already gone — which is exactly what a killed renderer's socket + // looks like — in an OperationCanceledException of its own manufacture, regardless of whether + // anyone actually cancelled anything. The filter is what tells the two apart: if the caller's + // own token were the cause, IsCancellationRequested would be true here and this catch does not + // apply, so a real cancellation still propagates. Everything below about why this must not + // fault the caller applies here exactly as it does to the exception types in the next catch. + } + catch (Exception exception) + when (exception is WebSocketException or ObjectDisposedException + or InvalidOperationException or IOException) + { + // The state check above is not atomic with the send, and a WebView renderer process killed by + // Android leaves its socket reporting Open long after nobody is reading from the other end. This + // has to read as "nobody listening" — the same as the no-socket case above — and never as a + // fault: SendAsync is called from TerminalSessionPump.SendOutputAsync inside the flush loop, and + // letting this exception escape would fault that loop. A faulted flush loop stops draining the + // credit window, the reader blocks once it fills, and the SSH session behind it freezes for good + // while LiveSessionCount still counts it as running. A dropped frame is recoverable — a frozen + // session is not. + } finally { sendGate.Release(); @@ -267,6 +310,25 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable .ConfigureAwait(false); } + /// + /// + /// Takeover, not rejection. A valid upgrade always wins the socket, even when one is already + /// attached — the old socket is aborted and the newcomer takes its place. Refusing a second attach used + /// to be the rule, on the theory that one renderer lives for the whole process. That is WebView2's + /// truth and not Android's: the platform kills the WebView's renderer process under memory pressure or + /// simply for being backgrounded, the page reloads, and the reload's socket is a second valid upgrade — + /// refusing it left the terminal permanently unreachable with no way back short of restarting the app. + /// + /// + /// Refusing protects nothing here anyway: only our own page knows the token (see the type-level remark + /// on what the token defends against), so a second valid upgrade is our page, reattaching. + /// Waiting for the old socket to notice it is dead and close on its own is not a safer alternative + /// either — a killed renderer process sends no TCP FIN, so the old receive loop can sit unaware for the + /// whole 30-second keepalive interval, and every reload landing in that window would still find the + /// door held shut by a socket nobody is on the other end of. Taking over immediately is what makes a + /// reload actually reattach. + /// + /// private async Task UpgradeAsync( Stream stream, HttpRequestLine request, @@ -280,16 +342,6 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable return; } - if (Interlocked.Exchange(ref accepted, 1) == 1) - { - // One renderer, one socket. A second attach would be either a bug or something else on the - // machine having found the port. - await WriteResponseAsync( - stream, "409 Conflict", "text/plain", "Already attached"u8.ToArray(), cancellationToken) - .ConfigureAwait(false); - return; - } - var key = request.Headers.GetValueOrDefault("sec-websocket-key")!; var accept = ComputeHandshakeAccept(key); @@ -314,10 +366,28 @@ public sealed class TerminalDataPlane : ITerminalTransport, IAsyncDisposable KeepAliveInterval = TimeSpan.FromSeconds(30), }); - socket = webSocket; - rendererAttached.TrySetResult(); + // Whatever was attached before is displaced, not merely overwritten: Exchange hands back the old + // reference so it can be aborted rather than left to linger as a socket nothing reads from again. + // Abort rather than a graceful close — a close frame would wait on a peer that, per the remark + // above, may never notice it should reply, and the newcomer already proved it is our page. + var previous = Interlocked.Exchange(ref socket, webSocket); + previous?.Abort(); - await ReceiveLoopAsync(webSocket, cancellationToken).ConfigureAwait(false); + rendererAttached.TrySetResult(); + SocketAttached?.Invoke(this, EventArgs.Empty); + + try + { + await ReceiveLoopAsync(webSocket, cancellationToken).ConfigureAwait(false); + } + finally + { + // Cleared only if the field still holds this connection's own socket. A takeover has already + // swapped in a newer one by the time an aborted receive loop unwinds to here, and clearing the + // field regardless would race the newcomer: whichever of the two finished last would win, and + // it must always be the newcomer, never this one going away. + Interlocked.CompareExchange(ref socket, null, webSocket); + } } /// diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs index 1c0df1f..6d3ac4d 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalDataPlaneTests.cs @@ -142,15 +142,96 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable await ConnectAsync(origin: "https://evil.example")); } + /// + /// The truth this replaced: a second valid attach used to be a 409, on the theory that one renderer + /// lives for the whole process. Android's WebView does not honour that theory — its renderer process is + /// routinely killed and the page reloads with a fresh socket — so a second valid attach is now a + /// takeover. This asserts both halves: the newcomer gets the connection, and the displaced socket + /// actually goes rather than lingering as a phantom nothing is reading from. + /// [Fact] - public async Task ASecondRenderer_IsRejected() + public async Task ASecondRenderer_TakesOver_AndTheFirstSocketIsDropped() { Start(); using var first = await ConnectAsync(); first.State.ShouldBe(WebSocketState.Open); - await Should.ThrowAsync(async () => await ConnectAsync()); + using var second = await ConnectAsync(); + second.State.ShouldBe(WebSocketState.Open); + + // The first socket was aborted rather than closed gracefully — Abort skips the close handshake + // entirely, so there is no Close frame for this side to see coming. What a receive on it sees + // instead is the connection simply gone, which the client surfaces as an exception rather than as + // a state that quietly flips on its own; nothing here reads from the socket otherwise, so the + // state alone would not move. + var firstBuffer = new byte[16]; + await Should.ThrowAsync(async () => + await first.ReceiveAsync(firstBuffer.AsMemory(), TestContext.Current.CancellationToken)); + + await using var session = new FakeShellSession(bytesToProduce: 64); + await using var pump = CreatePump(session); + plane.Register(SessionId, pump); + + var run = pump.RunAsync(TestContext.Current.CancellationToken); + + var opened = await ReceiveAsync(second); + opened.Opcode.ShouldBe((byte)TerminalServerOpcode.SessionOpened); + + var output = await ReceiveAsync(second); + output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output); + output.Payload.Length.ShouldBe(64); + + await SendAsync( + second, + (byte)TerminalClientOpcode.Acknowledge, + TerminalFrame.CreateAcknowledgementPayload((uint)output.Payload.Length)); + + await run; + } + + /// + /// + /// The other half of the takeover: a renderer process that dies without a close handshake — which is + /// what a killed Android WebView actually does, no FIN, nothing — must not fault the send path. A + /// faulted send would propagate into 's flush loop and freeze a live + /// session; see 's remark for why. Disposing the client socket + /// abruptly, with no close handshake sent, is the closest this harness gets to that: the server-side + /// socket is left believing itself open until it actually tries to write to it. + /// + /// + /// Driven straight through rather than through a pump, because + /// a pump adds nothing here — the point is entirely about the transport's own contract, and a session + /// layered on top would only leave it unclear whether a passing test proved the transport never threw or + /// merely that the frames never happened to need a live socket. + /// + /// + [Fact] + public async Task SendAsync_DoesNotThrow_WhenTheAttachedRendererDiedWithoutClosing_AndAFreshAttachStillReceives() + { + Start(); + + var first = await ConnectAsync(); + first.State.ShouldBe(WebSocketState.Open); + first.Dispose(); + + // Whether this particular send lands on the OS's send buffer before the peer's absence is noticed, + // or fails immediately, is not the point — either way it must not throw. + await Should.NotThrowAsync(async () => + await plane.SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "before"u8.ToArray()), + TestContext.Current.CancellationToken)); + + using var second = await ConnectAsync(); + + await Should.NotThrowAsync(async () => + await plane.SendAsync( + TerminalFrame.Create((byte)TerminalServerOpcode.Output, SessionId, "after"u8.ToArray()), + TestContext.Current.CancellationToken)); + + var output = await ReceiveAsync(second); + output.Opcode.ShouldBe((byte)TerminalServerOpcode.Output); + Encoding.UTF8.GetString(output.Payload).ShouldBe("after"); } // ---- Frames ---- From 4d1f07f253bca792febcd008d742db98c28b3adf Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:54:29 +0200 Subject: [PATCH 5/7] Replay the live sessions to a renderer that just attached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each live session gets its credit window reset — the unacknowledged bytes died with the old page, and their acknowledgement is never coming — and its SessionOpened frame again, flagged as a replay so the page can tell a reattach from a genuinely new session. A session whose shell already ended gets nothing: its scrollback lived only in the page that is gone, and a frame implying otherwise would lie. RendererReattached is the seam for what the workspace has no business owning: the font size and the selected tab live in the shell, which re-pushes them from its own subscription. --- .../TerminalWorkspace.cs | 116 ++++++++++++++ .../FakeShellSession.cs | 33 +++- .../TerminalWorkspaceTests.cs | 148 +++++++++++++++++- 3 files changed, 291 insertions(+), 6 deletions(-) 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.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/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 From aaff81272a44a80961571ac8b7675c8aa5855b39 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:54:38 +0200 Subject: [PATCH 6/7] Teach the page and the shell to put a reattached view back together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page's socket now retries itself forever with backoff — a dropped socket is an ordinary event on a phone, not the end of the terminal's life — and createSession is idempotent, so a replay landing on a pane that survived changes nothing. A replay creating a pane that did not survive writes one dim line saying the earlier output stayed on the host, because that is the truth about a reloaded page's scrollback. The shell answers RendererReattached with the two things only it owns: the font size, and which tab is active. --- .../ViewModels/MainWindowViewModel.cs | 27 +++++ .../WebAssets/terminal.js | 105 ++++++++++++++++-- 2 files changed, 121 insertions(+), 11 deletions(-) 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/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 From 3f5979d6399682fd88364935bc9019a6c48917bf Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 10:54:45 +0200 Subject: [PATCH 7/7] Record the renderer-reattach correction and its phone checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The port notes carry the third correction of this round: the data plane assumed a renderer that attaches once and lives forever, which no foreground service can make true of Android's separate WebView renderer process. Phase 11 gains the two checks a phone can run — close and reopen a connection, and a backgrounded shell surviving its renderer being killed, banner and all. --- docs/android-port.md | 31 +++++++++++++++++++++++++++++++ docs/manual-checks.md | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/docs/android-port.md b/docs/android-port.md index de71e79..7724b45 100644 --- a/docs/android-port.md +++ b/docs/android-port.md @@ -238,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 diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 49d6bcf..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