diff --git a/docs/adr/0014-android-updates.md b/docs/adr/0014-android-updates.md index bdee213..0b8bb78 100644 --- a/docs/adr/0014-android-updates.md +++ b/docs/adr/0014-android-updates.md @@ -84,6 +84,18 @@ losing the local cache, the outbox and the device key. turned this application on in the unknown-sources screen. Two deliberate answers, neither to a screen DodoSSH controls. + ◆ "Asks Android to ask" is one step longer than it sounds, and reading it as one step is why this + installed nothing at all from the day it shipped until 2026-08-05. An application holding only + `REQUEST_INSTALL_PACKAGES` gets no verdict back from committing a session: what the platform returns + first is `STATUS_PENDING_USER_ACTION`, carrying the confirmation activity in `Intent.EXTRA_INTENT` for + the application to start. **Android does not draw the dialogue on its own.** The commit succeeded, the + session staged, and nobody was ever asked anything. The receiver that starts it is + `InstallSessionReceiver`; the intent naming it has to be explicit, since a mutable pending intent + wrapping an implicit one is refused outright from API 34. Check 17.5 asks for exactly the right thing + — "Android's own installer appears naming the package" — so nothing needed rewording; it wants an + older build installed and a newer one published to run at all, and on the evidence it was never run + against a real pair. + 7. **Applying does not end the process, and the shell had to learn that.** On Windows, applying replaces the files and restarts, so the shell disposes the vault first — that is what zeroes the identity keys, the vault keys and the cache key. On Android the install is a *request* and the answer may be no, so diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 3fe4abf..dfb98f2 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -2091,6 +2091,14 @@ around being broken. An `INSTALL_FAILED_UPDATE_INCOMPATIBLE` means the two build keys — on the nightly channel that means the committed keystore changed, and on the release channel it means the wrong keystore was used. +◆ Two failures worth naming separately, because both look like "the button does nothing" and neither is a +signing problem. **The application closes when INSTALL is pressed** — that is an exception escaping the +command handler, and the message it should have shown is now the Failed line on this screen. **Nothing +happens at all, and the application carries on** — Android was asked to install and nobody started the +confirmation it handed back; see `InstallSessionReceiver` and the ◆ note in ADR 0014 rule 6. This check is +the only thing in the project that can catch either, which is the argument for running it on a real pair of +builds rather than reasoning about it. + ### 17.6 Declining leaves a working session · **the one that would be missed** Repeat 17.5 to the point where Android's installer is on screen, with a terminal open and the vault diff --git a/src/DodoSSH.Client.Android/Platform/AndroidUpdateChannel.cs b/src/DodoSSH.Client.Android/Platform/AndroidUpdateChannel.cs index 32b8381..8a24c33 100644 --- a/src/DodoSSH.Client.Android/Platform/AndroidUpdateChannel.cs +++ b/src/DodoSSH.Client.Android/Platform/AndroidUpdateChannel.cs @@ -298,6 +298,19 @@ internal sealed class AndroidUpdateChannel : IUpdateChannel /// and a grant on every launch, all so the installer can read a file this application already has /// open — where a session hands it the bytes directly. /// + /// + /// ◆ The pending intent handed to Commit is not a notification, and treating it as one is + /// why this silently installed nothing. An application holding REQUEST_INSTALL_PACKAGES + /// rather than the privileged INSTALL_PACKAGES never gets a decision back from a commit: what + /// the platform sends first is STATUS_PENDING_USER_ACTION, carrying — in + /// Intent.EXTRA_INTENT — the activity that draws the confirmation. Android does not draw it on + /// its own. Something has to receive that broadcast and start it, and for the life of this feature + /// nothing did: the intent named a bare action string with no receiver behind it, so the session was + /// written, committed, and left staged forever while the user watched nothing happen. See + /// , which is that something, and note that the intent naming it + /// must be explicit — a mutable pending intent wrapping an implicit one is refused outright from + /// API 34, which is where this build's target sits. + /// /// public void ApplyAndRestart(AvailableUpdate update) { @@ -318,7 +331,19 @@ internal sealed class AndroidUpdateChannel : IUpdateChannel return; } - var installer = packages.PackageInstaller; + HandToTheInstaller(packages.PackageInstaller, path); + } + + /// Writes the fetched APK into a session and commits it. + /// + /// The session is abandoned on any failure, which is not tidiness. One that is created and neither + /// committed nor abandoned stays staged, holding the space its SetSize reserved, and Android + /// caps how many an application may have open at once — so a fault that repeats, which is exactly + /// what a broken updater is, ends up unable to create a session at all. That would be a second and + /// wholly unrelated symptom for the same cause, and the more confusing of the two. + /// + private void HandToTheInstaller(PackageInstaller installer, string path) + { var parameters = new PackageInstaller.SessionParams(PackageInstallMode.FullInstall); var length = new FileInfo(path).Length; @@ -326,8 +351,10 @@ internal sealed class AndroidUpdateChannel : IUpdateChannel var id = installer.CreateSession(parameters); - using (var session = installer.OpenSession(id)) + try { + using var session = installer.OpenSession(id); + using (var destination = session.OpenWrite(InstallSession, 0, length)) using (var source = File.OpenRead(path)) { @@ -339,20 +366,41 @@ internal sealed class AndroidUpdateChannel : IUpdateChannel session.Fsync(destination); } - // A pending intent is how the platform reports what the user decided, and one is required - // whether or not anything listens. Nothing here does: the two outcomes are this process being - // replaced and this process carrying on, and both are already visible without being told. - // Mutable is required from API 31 — the installer fills the result in — and does not exist + // Mutable is required from API 31 — the installer fills the status in — and does not exist // below it, where every pending intent is mutable and naming the flag will not compile // against the older platform. minSdk here is 28, so both cases are real. var flags = OperatingSystem.IsAndroidVersionAtLeast(31) ? PendingIntentFlags.Mutable | PendingIntentFlags.UpdateCurrent : PendingIntentFlags.UpdateCurrent; - var callback = PendingIntent.GetBroadcast(context, 0, new Intent(InstallSession), flags); + // Named at the receiver class rather than carrying an action string, and both halves of that + // matter. An explicit intent is the only kind a mutable pending intent may wrap from API 34, + // so the implicit one that was here throws before commit is even reached on any current + // phone; and a broadcast with a receiver behind it is what gets the confirmation drawn at + // all. See the remarks above. + var callback = PendingIntent.GetBroadcast( + context, + 0, + new Intent(context, typeof(InstallSessionReceiver)), + flags); session.Commit(callback!.IntentSender!); } + catch + { + try + { + installer.AbandonSession(id); + } + catch (Java.Lang.Throwable) + { + // The session could not be abandoned either. Whatever went wrong first is the useful + // half of that, so it is the one allowed to propagate — a cleanup failure thrown from + // here would replace the cause with a consequence. + } + + throw; + } } /// Which release this build's channel reads. @@ -432,6 +480,93 @@ internal sealed class AndroidUpdateChannel : IUpdateChannel } } +/// +/// Starts the confirmation Android hands back when an install session is committed. +/// +/// +/// +/// This is the whole of "the platform draws the confirmation". It does, but only once asked: a +/// commit by an application with REQUEST_INSTALL_PACKAGES answers +/// STATUS_PENDING_USER_ACTION and puts the activity that draws the dialogue in +/// Intent.EXTRA_INTENT, for the application to start. Nothing else in the system will start it. +/// Without a receiver the commit still succeeds, the session still stages, and the update simply never +/// arrives — which is precisely how this shipped. +/// +/// +/// Every other status is ignored on purpose, and that part of the original reasoning survives: the two +/// outcomes worth knowing are this process being replaced and this process carrying on, and both are +/// already visible without being told. A failure the user caused by declining is not an error, and a +/// failure Android caused says so in its own dialogue. +/// +/// +/// Not exported. The only sender is the pending intent this application handed to its own commit, which +/// the system delivers under this application's identity — so nothing outside needs to reach it, and an +/// exported receiver that starts an activity out of an extra is a component anybody could use to launch +/// an arbitrary screen with this application's package on it. +/// +/// +[BroadcastReceiver(Enabled = true, Exported = false)] +internal sealed class InstallSessionReceiver : BroadcastReceiver +{ + /// + public override void OnReceive(Context? context, Intent? intent) + { + if (context is null || intent is null) + { + return; + } + + // Defaulted to a failure rather than to the pending value, so a broadcast arriving without a + // status is treated as nothing to do instead of as a reason to go looking for an intent. + var status = intent.GetIntExtra( + PackageInstaller.ExtraStatus, + (int)PackageInstallStatus.Failure); + + if (status != (int)PackageInstallStatus.PendingUserAction) + { + return; + } + + if (ConfirmationIn(intent) is not { } confirmation) + { + return; + } + + // The activity if there is one, and the application context otherwise with a task of its own — + // the same choice, for the same reason, as SendToTheUnknownSourcesScreen above. A receiver's own + // context cannot start an activity without the flag, and the update loop can perfectly well have + // committed this while the application was backgrounded. + if (PhoneEnvironment.CurrentActivity is { } activity) + { + activity.StartActivity(confirmation); + + return; + } + + confirmation.AddFlags(ActivityFlags.NewTask); + context.StartActivity(confirmation); + } + + /// The activity Android wants started, read the way the running platform allows. + /// + /// The untyped overload is obsolete from API 33 and the typed one does not exist below it, so both + /// are here behind the guard the analyser reads. The same shape as the pending-intent flags in + /// , and for the same reason: minSdk is 28, so both branches run + /// on phones this ships to. + /// + private static Intent? ConfirmationIn(Intent intent) + { + if (OperatingSystem.IsAndroidVersionAtLeast(33)) + { + return intent.GetParcelableExtra( + Intent.ExtraIntent, + Java.Lang.Class.FromType(typeof(Intent))) as Intent; + } + + return intent.GetParcelableExtra(Intent.ExtraIntent) as Intent; + } +} + /// /// Picks the update channel this copy of the phone head gets. /// diff --git a/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs index 413cc5c..880924f 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs @@ -397,7 +397,22 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab return; } - await restart(update).ConfigureAwait(true); + // ◆ Reported rather than allowed to escape, and the phone is why. Where applying ends the process + // a throw here is nearly unreachable; where it does not — see ApplyingEndsTheProcess — the whole + // point is that this returns and the application carries on, so an exception has somewhere to go: + // out of a command handler, past the dispatcher, and into the process. What that looks like to + // somebody holding the phone is pressing INSTALL and having the application close, with nothing + // installed and nothing said. Failed and the message is the same answer CheckNowAsync gives, and + // it is the difference between a bug that describes itself and one that presents as a crash. + try + { + await restart(update).ConfigureAwait(true); + } + catch (Exception exception) when (exception is not OutOfMemoryException) + { + State = UpdateState.Failed; + Status = exception.Message; + } } [RelayCommand] diff --git a/tests/DodoSSH.Client.App.Tests/FakeUpdateChannel.cs b/tests/DodoSSH.Client.App.Tests/FakeUpdateChannel.cs index 36936ac..8e03f2b 100644 --- a/tests/DodoSSH.Client.App.Tests/FakeUpdateChannel.cs +++ b/tests/DodoSSH.Client.App.Tests/FakeUpdateChannel.cs @@ -29,6 +29,14 @@ internal sealed class FakeUpdateChannel : IUpdateChannel /// When set, downloading throws it. internal Exception? DownloadFailure { get; set; } + /// When set, applying throws it — after counting, since the call was still made. + /// + /// The phone's install can fail on the spot rather than by ending the process: a session the platform + /// refuses, or an unknown-sources answer that changed under it. What that must not do is escape a + /// command handler, so this is what lets a test hold the view model to reporting it. + /// + internal Exception? RestartFailure { get; set; } + /// When set, a download waits on it before completing. internal TaskCompletionSource? HoldDownload { get; set; } @@ -97,5 +105,10 @@ internal sealed class FakeUpdateChannel : IUpdateChannel { Restarts++; RestartedWith = update; + + if (RestartFailure is { } failure) + { + throw failure; + } } } diff --git a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs index 862668e..d47bb73 100644 --- a/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs @@ -154,6 +154,31 @@ public sealed class UpdateFlowTests : IDisposable channel.RestartedWith!.Version.ShouldBe("1.3.0"); } + /// + /// ◆ The regression for an install that closed the application instead of installing anything. On the + /// phone applying is a request that returns, so a failure in it throws on the spot rather than being + /// cut short by the process ending — and with nothing catching it, it left a command handler, passed + /// the dispatcher and took the process with it. Pressing INSTALL closed the application, said nothing, + /// and installed nothing, which reads as anything but a bug in the updater. + /// + [Fact] + public async Task AnInstallThatFails_IsReportedRatherThanThrown() + { + channel.Available = new AvailableUpdate("1.3.0"); + channel.ApplyingEndsTheProcess = false; + channel.RestartFailure = new InvalidOperationException("The installer refused the session."); + + var updates = Build(); + + await updates.CheckOnceAsync(Token); + + // Not Should.ThrowAsync: the assertion is that this does not throw at all. + await updates.RestartNowCommand.ExecuteAsync(null); + + updates.State.ShouldBe(UpdateState.Failed); + updates.Status.ShouldBe("The installer refused the session."); + } + [Fact] public async Task RestartingWithNothingReady_DoesNothing() {