Updates: offer an RPM only to a copy rpm owns, and never read an unknown version as older

On a distribution without rpm, a locally built package that mirrors the spec puts the nightly tree
at /opt/DodoSSH.Nightly, and that path was the whole test for "the RPM installed this". The channel
then asked rpm for the installed version, got nothing because there is no rpm, read the silence as
"0", and offered every nightly as newer, fetching a 56 MB RPM into the profile directory that the
machine could never install, every six hours.

UpdateChannels.LinuxChannel still checks the path first, then asks rpm through the new
LinuxUpdateChannel.ForInstall. A copy rpm has no record of gets an UnavailableUpdateChannel that
says it was installed outside the dodossh-nightly (or dodossh) RPM and should be updated with the
package manager that installed it. UnavailableUpdateChannel gains an optional Reason for that,
UpdateViewModel exposes it as UnsupportedReason, and the desktop preferences page shows it in place
of the sentence about checkouts and unzipped copies. That costs one rpm launch at start, and only
where the path already matched.

CheckAsync no longer falls back to "0". If rpm stops answering after composition, the check throws
instead, so CHECK NOW reports it and the timer stays quiet, and nothing is offered or fetched. The
comment that called the fallback a deliberate choice is replaced with the reason it was wrong.

LinuxUpdateChannelTests: an unknown installed version throws and downloads nothing (replacing the
test that asserted the old behaviour), a copy rpm does not own is unavailable and says why, and a
copy rpm owns gets the channel. The newer, equal and older comparisons were already covered.
UpdateFlowTests covers the reason reaching the view model. Manual check 19.13.

App tests 744/752 with 8 skips, layout tests 202/203 with 1 skip; the full solution passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
laurensandClaude Opus 5.5 committed 2026-10-20 16:39:17 +02:00
1 parent f7354aa7e8
commit e0509c6679
8 files changed
+238 -29

No files matched your search

@@ -127,17 +127,61 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
this.installedVersion = installedVersion;
this.handToInstaller = handToInstaller;
packageName = string.Equals(channel, "nightly", StringComparison.Ordinal)
? "dodossh-nightly"
: "dodossh";
packageName = PackageFor(channel);
}
/// <summary>The channel for a copy under the RPM's install root, if rpm says the package is there.</summary>
internal static IUpdateChannel ForInstall(string channel) =>
ForInstall(channel, InstalledRpmVersion, () => new LinuxUpdateChannel(channel));
/// <summary>
/// The channel for a copy under the RPM's install root, with rpm supplied, which is how the tests
/// reach it.
/// </summary>
/// <remarks>
/// <para>
/// <b>The path says where this copy is and only rpm says who put it there.</b>
/// <c>UpdateChannels.LinuxChannel</c> has already established the first, and on an RPM distribution
/// that is the whole story. It is not on any other: a locally built pacman or dpkg package that mirrors
/// the spec puts the same tree at the same path, and this channel then offered every nightly as newer
/// than a version nobody could read, fetched a package that machine has no way to install, and did it
/// again every six hours.
/// </para>
/// <para>
/// Asked once, at composition, for the reason the rest of this decision is: who installed this copy
/// does not change while it runs. It costs one <c>rpm</c> launch at start on a machine whose path
/// already matched, and nothing at all anywhere else.
/// </para>
/// </remarks>
/// <param name="channel">The build's channel, which names the package.</param>
/// <param name="installedVersion">What rpm says is installed under a package name, or null.</param>
/// <param name="whenOwned">The channel to use when rpm does own this copy.</param>
internal static IUpdateChannel ForInstall(
string channel,
Func<string, string?> installedVersion,
Func<IUpdateChannel> whenOwned)
{
ArgumentException.ThrowIfNullOrEmpty(channel);
ArgumentNullException.ThrowIfNull(installedVersion);
ArgumentNullException.ThrowIfNull(whenOwned);
var package = PackageFor(channel);
return installedVersion(package) is null
? new UnavailableUpdateChannel(
$"This copy of DodoSSH was installed outside the {package} RPM — rpm has no record of that "
+ "package here — and an RPM is the only kind of update it can fetch. Update it with the "
+ "package manager that installed it.")
: whenOwned();
}
/// <inheritdoc />
/// <remarks>
/// True unconditionally, because whether this copy is one that can be updated was already decided by
/// <see cref="UpdateChannels.ForThisMachine"/> — a build run from a checkout never reaches this class
/// at all. The same split Velopack's side uses, where <c>IsInstalled</c> is checked once at
/// composition rather than at every call site.
/// <see cref="UpdateChannels.ForThisMachine"/> and <see cref="ForInstall(string)"/> — a build run from
/// a checkout, or installed by something other than rpm, never reaches this class at all. The same
/// split Velopack's side uses, where <c>IsInstalled</c> is checked once at composition rather than at
/// every call site.
/// </remarks>
public bool IsSupported => true;
@@ -184,9 +228,20 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
$"The {channel} release's {manifestName} could not be read as a manifest.");
}
// ◆ No answer from rpm is a failure, never a version. ForInstall only built this class because
// rpm answered, so silence now is a package removed under a running process or an rpm that
// stopped working — and any stand-in number would make every published build look newer and
// fetch a package for a machine that cannot install it, which is what this once did.
if (installedVersion(packageName) is not { } installed)
{
throw new InvalidOperationException(
$"rpm has no record of {packageName}, so there is no installed version to compare the "
+ $"{channel} release against.");
}
// The one place null is returned, and it means what null is documented to mean: this build is
// current. Compared the way rpm compares, because rpm is what accepts or refuses the install.
if (RpmVersion.Compare(manifest.RpmVersion, installedVersion(packageName) ?? "0") <= 0)
if (RpmVersion.Compare(manifest.RpmVersion, installed) <= 0)
{
return null;
}
@@ -324,6 +379,12 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
? $"{RepositoryApi}/releases/tags/nightly-desktop"
: $"{RepositoryApi}/releases/latest";
/// <summary>The package a channel's builds are published as.</summary>
private static string PackageFor(string channel) =>
string.Equals(channel, "nightly", StringComparison.Ordinal)
? "dodossh-nightly"
: "dodossh";
private static string? Asset(LinuxForgeRelease? release, string name) =>
release?.Assets?.FirstOrDefault(
asset => string.Equals(asset.Name, name, StringComparison.Ordinal))?.DownloadUrl;
@@ -365,9 +426,11 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
/// <c>scripts/ci-linux-rpm.sh</c> performs, and two copies of one rule is a pair that can disagree.
/// </para>
/// <para>
/// A machine that cannot answer gets null, which the caller reads as <c>0</c> and makes every
/// published build look newer. That is the wrong way to fail, chosen deliberately: the failure is then
/// an install PackageKit declines rather than a security fix that never arrives.
/// A machine that cannot answer gets null — no <c>rpm</c> at all, or one with no such package — and
/// null means "rpm does not own this copy", never a version. It used to be read as <c>0</c>, on the
/// argument that the worst case was an install PackageKit declined. That holds only where PackageKit
/// installs RPMs: on any other distribution it made every published build look newer, forever, and
/// fetched a package that machine could not install. See <see cref="ForInstall(string)"/>.
/// </para>
/// </remarks>
private static string? InstalledRpmVersion(string packageName)
@@ -66,21 +66,23 @@ internal static class UpdateChannels
}
}
/// <summary>The Linux channel, if this copy is one a package manager put here.</summary>
/// <summary>The Linux channel, if this copy is one the RPM put here.</summary>
/// <remarks>
/// <para>
/// <b>The question Velopack answers with <c>IsInstalled</c>, answered by looking at where this process
/// lives.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the spec puts it there
/// and nothing else writes to <c>/opt</c> under those names — so a build running from anywhere else is
/// a checkout somebody started, an extracted archive, or a tree copied by hand. None of those can be
/// updated by installing a package over them, and offering it would be an offer that cannot be carried
/// out.
/// lives and then asking rpm.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the
/// spec puts it there — so a build running from anywhere else is a checkout somebody started, an
/// extracted archive, or a tree copied by hand. None of those can be updated by installing a package
/// over them, and offering it would be an offer that cannot be carried out.
/// </para>
/// <para>
/// Checked by path rather than by asking rpm which package owns the executable, which would be the
/// more thorough answer and costs a process launch on every start for a distinction nothing acts on
/// differently. The install root is a constant in two places — here and the packaging script — and
/// they are named in each other's comments.
/// <b>The path is necessary and not sufficient.</b> It used to be the whole check, on the argument
/// that asking rpm cost a process launch for a distinction nothing acted on differently. Something
/// does: a locally built pacman package that mirrors the spec puts the same tree at the same path,
/// and that copy was offered every nightly as an RPM it could never install. So a matching path is
/// then put to rpm — see <see cref="LinuxUpdateChannel.ForInstall(string)"/> — and the launch is paid
/// only where the path already matched. The install root is a constant in two places — here and the
/// packaging script — and they are named in each other's comments.
/// </para>
/// </remarks>
[SupportedOSPlatform("linux")]
@@ -91,7 +93,7 @@ internal static class UpdateChannels
var here = AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar);
return string.Equals(here, root, StringComparison.Ordinal)
? new LinuxUpdateChannel(DesktopChannel.Name)
? LinuxUpdateChannel.ForInstall(DesktopChannel.Name)
: new UnavailableUpdateChannel();
}
}
@@ -84,7 +84,10 @@
IsVisible="{Binding Updates.Status, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
<TextBlock Classes="settingsrowcaption"
Text="This copy of DodoSSH cannot replace itself, so nothing above does anything. That is what a build run from a source checkout looks like, and also what a copy somebody unzipped by hand looks like — it is the installer that registers the update path."
IsVisible="{Binding Updates.IsUnsupported}" />
IsVisible="{Binding Updates.IsUnsupportedForTheUsualReason}" />
<TextBlock Classes="settingsrowcaption"
Text="{Binding Updates.UnsupportedReason}"
IsVisible="{Binding Updates.UnsupportedReason, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
</StackPanel>
</Border>
@@ -141,9 +141,32 @@ public interface IUpdateChannel
/// <see cref="UnavailableDeviceKeyStore"/>: the caller then has one shape to write against, and "no
/// updater here" is a thing the interface can express rather than a case every call site has to remember.
/// </para>
/// <para>
/// <b>One situation outside those four carries a <see cref="Reason"/>.</b> A Linux copy sitting exactly
/// where the RPM puts it that no RPM owns — built locally and installed by another distribution's package
/// manager. It looks installed, so the sentence about checkouts and unzipped copies would describe the
/// wrong machine, and the person reading it needs telling which updater is theirs.
/// </para>
/// </remarks>
public sealed class UnavailableUpdateChannel : IUpdateChannel
{
/// <summary>Creates one for the situations that need no telling apart.</summary>
public UnavailableUpdateChannel()
{
}
/// <summary>Creates one whose situation is worth a sentence of its own.</summary>
/// <param name="reason">What to tell the user, as a sentence they can act on.</param>
public UnavailableUpdateChannel(string reason)
{
ArgumentException.ThrowIfNullOrWhiteSpace(reason);
Reason = reason;
}
/// <summary>Why this copy cannot replace itself, where that is not the usual why.</summary>
public string? Reason { get; }
/// <inheritdoc />
public bool IsSupported => false;
@@ -126,6 +126,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
this.restart = restart;
CurrentVersion = updates.CurrentVersion;
UnsupportedReason = (updates as UnavailableUpdateChannel)?.Reason;
isAutomatic = settings.Read().AutomaticUpdateChecks;
state = updates.IsSupported ? UpdateState.Idle : UpdateState.Unsupported;
}
@@ -133,6 +134,16 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
/// <summary>What this build calls itself.</summary>
internal string CurrentVersion { get; }
/// <summary>Why this copy cannot replace itself, when the channel had something particular to say.</summary>
/// <remarks>
/// Null in the ordinary cases, where the preferences screen's own sentence about checkouts and unzipped
/// copies is the right one. See <see cref="UnavailableUpdateChannel"/> for the case where it is not.
/// </remarks>
internal string? UnsupportedReason { get; }
/// <summary>Unsupported, with nothing more particular to say than the screen's own sentence.</summary>
internal bool IsUnsupportedForTheUsualReason => IsUnsupported && UnsupportedReason is null;
[ObservableProperty]
private UpdateState state;
@@ -248,6 +259,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
{
OnPropertyChanged(nameof(IsSupported));
OnPropertyChanged(nameof(IsUnsupported));
OnPropertyChanged(nameof(IsUnsupportedForTheUsualReason));
OnPropertyChanged(nameof(IsChecking));
OnPropertyChanged(nameof(IsDownloading));
OnPropertyChanged(nameof(IsReady));