From 7ecd443e6290f30d05600eb3803069cfd27de967 Mon Sep 17 00:00:00 2001 From: GabrielDuf Date: Thu, 22 Oct 2026 08:44:55 -0400 Subject: [PATCH 1/2] Disable package managers whose tool is missing on first run PackageManager.IsEnabled() returns !DisabledManagers[Name], so a manager is enabled unless an entry says otherwise. On a fresh machine the dictionary is empty, every manager counts as enabled, and each tool that is not installed ends up enabled with Status.Found == false. That shows an Error "Not found" badge in the settings pages and the TUI Managers page, so a new user gets a warning for every tool they never installed. Initialize() now records a default enabled state right after looking for the executable, but only when DisabledManagers has no entry for the manager. An explicit user choice always wins. A missing manager is recorded as disabled and gets the normal "is disabled" status; a found manager is recorded as enabled, so a later uninstall still shows "Not found" instead of silently disabling it. Because the default is written per manager, managers added in later versions are covered too. PEInterface initializes managers in parallel and SetDictionaryItem is a read-modify-write of the whole dictionary file, so the check-and-write runs under a static lock; without it, concurrent first-run writes lose each other's entries. The --fake-data sandbox seeds an explicit enabled entry for its fake not-found manager (Cargo) when none exists, so the fake data set and the TUI E2E tests keep the not-found state they exist to show. InitializeEnabledManagerWithoutExecutableStaysNotReady now sets an explicit enabled entry so it still tests an enabled manager whose executable is missing. Co-Authored-By: Claude Opus 5.5 --- .../Manager/PackageManager.cs | 24 ++++++++ .../PackageManagerTests.cs | 61 +++++++++++++++++++ .../FakeData/FakeDataEnvironment.cs | 6 ++ 3 files changed, 91 insertions(+) diff --git a/src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs b/src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs index 6478de69f7..8dc88b0dc4 100644 --- a/src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs +++ b/src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs @@ -118,6 +118,16 @@ public virtual void Initialize() out string path, out string callArguments ); + + if (_recordDefaultEnabledState(found)) + { + Status = new() { Version = CoreTools.Translate("{0} is disabled", DisplayName) }; + Logger.ImportantInfo( + $"{Name} was not found on the system and has been disabled by default" + ); + return; + } + Status = new ManagerStatus(); Status.Found = found; Status.ExecutablePath = path; @@ -305,6 +315,20 @@ public bool IsEnabled() return !Settings.GetDictionaryItem(Settings.K.DisabledManagers, Name); } + private static readonly object _defaultEnabledStateLock = new(); + + private bool _recordDefaultEnabledState(bool found) + { + lock (_defaultEnabledStateLock) + { + if (Settings.DictionaryContainsKey(Settings.K.DisabledManagers, Name)) + return false; + + Settings.SetDictionaryItem(Settings.K.DisabledManagers, Name, !found); + return !found; + } + } + /// /// Returns true if the manager is enabled and available (the required executable files were found). Returns false otherwise /// diff --git a/src/UniGetUI.PackageEngine.Tests/PackageManagerTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageManagerTests.cs index 129471f010..d0d60a054b 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageManagerTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageManagerTests.cs @@ -81,6 +81,7 @@ public void InitializeEnabledManagerWithoutExecutableStaysNotReady() var manager = CreateManager(); manager.ExecutableFound = false; manager.ExecutablePath = CreateExecutable("missing-manager.exe"); + Settings.SetDictionaryItem(Settings.K.DisabledManagers, manager.Name, false); manager.Initialize(); @@ -94,6 +95,66 @@ public void InitializeEnabledManagerWithoutExecutableStaysNotReady() ); } + [Fact] + public void FirstInitializeDisablesManagerWhoseExecutableIsMissing() + { + var manager = CreateManager(); + manager.ExecutableFound = false; + manager.ExecutablePath = CreateExecutable("never-installed-manager.exe"); + + manager.Initialize(); + + Assert.False(manager.IsEnabled()); + Assert.False(manager.IsReady()); + Assert.False(manager.Status.Found); + Assert.True(Settings.GetDictionaryItem(Settings.K.DisabledManagers, manager.Name)); + Assert.Equal( + CoreTools.Translate("{0} is disabled", manager.DisplayName), + manager.Status.Version + ); + } + + [Fact] + public void FirstInitializeKeepsFoundManagerEnabledAfterItsExecutableDisappears() + { + var manager = CreateManager(); + manager.ExecutablePath = CreateExecutable("installed-manager.exe"); + + manager.Initialize(); + + Assert.True(manager.IsReady()); + Assert.True(Settings.DictionaryContainsKey(Settings.K.DisabledManagers, manager.Name)); + Assert.False(Settings.GetDictionaryItem(Settings.K.DisabledManagers, manager.Name)); + + manager.ExecutableFound = false; + manager.Initialize(); + + Assert.True(manager.IsEnabled()); + Assert.False(manager.IsReady()); + Assert.False(manager.Status.Found); + } + + [Fact] + public void FirstInitializeDisablesOnlyMissingManagersWhenInitializedConcurrently() + { + var managers = Enumerable.Range(0, 16).Select(i => + { + var manager = CreateManager(); + manager.ExecutableFound = i % 2 == 0; + manager.ExecutablePath = CreateExecutable($"{manager.Name}.exe"); + return manager; + }).ToList(); + + Parallel.ForEach(managers, manager => manager.Initialize()); + + foreach (var manager in managers) + { + Assert.True(Settings.DictionaryContainsKey(Settings.K.DisabledManagers, manager.Name)); + Assert.Equal(manager.ExecutableFound, manager.IsEnabled()); + Assert.Equal(manager.ExecutableFound, manager.IsReady()); + } + } + [Fact] public void InitializeEnabledManagerWithExecutableBecomesReady() { diff --git a/src/UniGetUI.Tui/FakeData/FakeDataEnvironment.cs b/src/UniGetUI.Tui/FakeData/FakeDataEnvironment.cs index de92a03c52..afc05ee337 100644 --- a/src/UniGetUI.Tui/FakeData/FakeDataEnvironment.cs +++ b/src/UniGetUI.Tui/FakeData/FakeDataEnvironment.cs @@ -86,6 +86,12 @@ public static FakeDataEnvironment Activate(string? sandboxDirectory = null) // The Devolutions Agent broker is a real out-of-process service; never route fake operations there. Settings.Set(Settings.K.UseAgentBroker, false); + foreach (FakeManagerProfile profile in FakeCatalog.Managers.Where(p => !p.Found)) + { + if (!Settings.DictionaryContainsKey(Settings.K.DisabledManagers, profile.Name)) + Settings.SetDictionaryItem(Settings.K.DisabledManagers, profile.Name, false); + } + env.Managers = FakeCatalog.Managers.Select(p => (IPackageManager)new FakePackageManager(p, env)).ToArray(); Current = env; return env; From 8063f1ac069e990960e80f18f793276395b257ca Mon Sep 17 00:00:00 2001 From: GabrielDuf Date: Thu, 22 Oct 2026 09:11:09 -0400 Subject: [PATCH 2/2] Keep a cold dictionary load from overwriting a concurrent write Settings._getDictionary read and deserialized the file on a cache miss, then published the result with a plain assignment. A SetDictionary that landed in between was replaced in the cache by the older snapshot, and the next SetDictionaryItem wrote that snapshot back to disk without the other writer's entry. With managers initializing in parallel on first run, one manager could load an empty DisabledManagers while another saved its default under the PackageManager lock, losing that default. A cold load now publishes through _publishLoadedDictionary, which uses GetOrAdd. If its own dictionary is the one stored, it returns its value as before; if a write or another load published first, it reads again through the cache-hit path and returns the newer value. The fix sits in the settings engine rather than in a cache warm-up before parallel initialization, so every cold reader (UI, IPC) is covered. SetDictionary still assigns directly, since a write always replaces the cache. The publish step is its own internal method so the race can be tested deterministically: ADictionaryLoadPublishedAfterAWriteKeepsTheWrite writes first and publishes a stale load second, the exact interleaving of the bug, with no threads or timing. It fails with the old plain assignment and passes with GetOrAdd. ADictionaryLoadIntoAColdCacheIsPublished covers the normal cold load. UniGetUI.Core.Settings gets the same InternalsVisibleTo.cs file the other core projects use. Co-Authored-By: Claude Opus 5.5 --- .../SettingsTest.cs | 45 +++++++++++++++++++ .../InternalsVisibleTo.cs | 3 ++ .../SettingsEngine_Dictionaries.cs | 12 ++++- 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 src/UniGetUI.Core.Settings/InternalsVisibleTo.cs diff --git a/src/UniGetUI.Core.Settings.Tests/SettingsTest.cs b/src/UniGetUI.Core.Settings.Tests/SettingsTest.cs index 1b65c89e75..b6d8a1a649 100644 --- a/src/UniGetUI.Core.Settings.Tests/SettingsTest.cs +++ b/src/UniGetUI.Core.Settings.Tests/SettingsTest.cs @@ -388,6 +388,51 @@ string[] strArray ); } + [Fact] + public void ADictionaryLoadPublishedAfterAWriteKeepsTheWrite() + { + Settings.ResetSettings(); + string file = Path.Join( + CoreData.UniGetUIUserConfigurationDirectory, + $"{Settings.ResolveKey(Settings.K.DisabledManagers)}.json" + ); + var staleLoad = new Dictionary { ["Stale"] = true }; + + Settings.SetDictionary( + Settings.K.DisabledManagers, + new Dictionary { ["Written"] = true } + ); + + Assert.False(Settings._publishLoadedDictionary(Settings.K.DisabledManagers, staleLoad)); + Assert.True( + Settings.DictionaryContainsKey(Settings.K.DisabledManagers, "Written") + ); + Assert.False( + Settings.DictionaryContainsKey(Settings.K.DisabledManagers, "Stale") + ); + + Settings.SetDictionaryItem(Settings.K.DisabledManagers, "Other", false); + string onDisk = File.ReadAllText(file); + Assert.Contains("Written", onDisk); + Assert.DoesNotContain("Stale", onDisk); + + Settings.ResetSettings(); + } + + [Fact] + public void ADictionaryLoadIntoAColdCacheIsPublished() + { + Settings.ResetSettings(); + var load = new Dictionary { ["Loaded"] = true }; + + Assert.True(Settings._publishLoadedDictionary(Settings.K.DisabledManagers, load)); + Assert.True( + Settings.DictionaryContainsKey(Settings.K.DisabledManagers, "Loaded") + ); + + Settings.ResetSettings(); + } + [Fact] public static void EnsureAllKeysResolve() { diff --git a/src/UniGetUI.Core.Settings/InternalsVisibleTo.cs b/src/UniGetUI.Core.Settings/InternalsVisibleTo.cs new file mode 100644 index 0000000000..f92abd5c22 --- /dev/null +++ b/src/UniGetUI.Core.Settings/InternalsVisibleTo.cs @@ -0,0 +1,3 @@ +using System.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("UniGetUI.Core.Settings.Tests")] diff --git a/src/UniGetUI.Core.Settings/SettingsEngine_Dictionaries.cs b/src/UniGetUI.Core.Settings/SettingsEngine_Dictionaries.cs index d85e127f06..14f37f46df 100644 --- a/src/UniGetUI.Core.Settings/SettingsEngine_Dictionaries.cs +++ b/src/UniGetUI.Core.Settings/SettingsEngine_Dictionaries.cs @@ -71,11 +71,14 @@ private static readonly ConcurrentDictionary< } } - _dictionarySettings[key] = value.ToDictionary( + Dictionary loaded = value.ToDictionary( kvp => (object)kvp.Key, kvp => (object?)kvp.Value ); - return value; + if (_publishLoadedDictionary(key, loaded)) + return value; + + return _getDictionary(key); } catch (InvalidOperationException) { @@ -89,6 +92,11 @@ private static readonly ConcurrentDictionary< } } + internal static bool _publishLoadedDictionary(K key, Dictionary loaded) + { + return ReferenceEquals(_dictionarySettings.GetOrAdd(key, loaded), loaded); + } + // Returns an empty dictionary if the setting doesn't exist and null if the types are invalid public static IReadOnlyDictionary? GetDictionary(K settingsKey) where KeyT : notnull