From ca7fee2358115185a445e14dc9a11b730ebfaf61 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Wed, 5 Aug 2026 22:31:33 +0200 Subject: [PATCH] Start the confirmation Android hands back, so an update can install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing INSTALL closed the application, installed nothing and said nothing. That is two independent faults in one method, either of which breaks it on its own, and they hid each other: the first kills the process before the second can be observed, and the second is silent by construction. The pending intent handed to commit was implicit — an action string with no component behind it. A mutable pending intent may not wrap one of those from API 34, and this head targets 36, so every current phone threw IllegalArgumentException before commit was reached. Nothing caught it, so it left the command handler, passed the dispatcher and took the process with it. That is the closing. Below 34, where it did not throw, it still installed nothing. An application holding REQUEST_INSTALL_PACKAGES rather than the privileged INSTALL_PACKAGES gets no verdict back from a commit: what the platform answers first is STATUS_PENDING_USER_ACTION, carrying the activity that draws the dialogue in EXTRA_INTENT for the application to start. Android does not draw it on its own. The comment here asserted the opposite — that a pending intent is required whether or not anything listens, and that nothing needed to — so no receiver was ever written, and the session was written, committed and left staged forever. So there is a receiver now, not exported because the only sender is this application's own commit, and the intent naming it is explicit, which is the same change that stops the throw. Sessions are abandoned when anything fails, since one created and neither committed nor abandoned stays staged against a per-application cap — a repeating fault would have started failing at CreateSession instead, which is the same bug wearing a completely unrelated face. The reporting is the part worth keeping even after the cause is gone. Where applying ends the process an exception has nowhere to go; where it does not, which is this head's whole shape, it goes out through the dispatcher. RestartNowAsync now answers the way CheckNowAsync already did, and the regression test asserts the absence of a throw rather than the presence of one. ADR 0014 rule 6 gets the correction in place: "asks Android to ask" is one step longer than it reads. Check 17.5 needed no rewording — it asks for the installer appearing by name, which is exactly the thing that never happened — so what it gets instead is the two symptoms named, because both present as a dead button. It is the only thing in the project that can catch either, and it plainly was never run against a real pair of builds. Note for whoever takes the next nightly: a broken updater cannot install its own fix. The phone is running the code this commit replaces, so the first build carrying it has to be sideloaded by hand; the ones after that install normally. Compile-verified and manifest-verified — the receiver reaches the generated manifest — and 321 tests pass. Not run on a device, which is what 17.5 is for. --- docs/adr/0014-android-updates.md | 12 ++ docs/manual-checks.md | 8 + .../Platform/AndroidUpdateChannel.cs | 149 +++++++++++++++++- .../ViewModels/UpdateViewModel.cs | 17 +- .../FakeUpdateChannel.cs | 13 ++ .../UpdateFlowTests.cs | 25 +++ 6 files changed, 216 insertions(+), 8 deletions(-) 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() {