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 @@ - +