Executable list eviction limit only on invalid exes (#1570)

Co-authored-by: Alexandru Macocian <amacocian@microsoft.com>

Daybreak.Core/Configuration/ProjectConfiguration.cs

@@ -255,7 +255,7 @@ public class ProjectConfiguration : PluginConfigurationBase
         services.AddHostedSingleton<IThemeManager, BlazorThemeInteropService>();
         services.AddHostedSingleton<IConnectivityStatus, ConnectivityStatus>();
         services.AddHostedSingleton<ITradeAlertingService, TradeAlertingService>();
-        services.AddHostedSingleton<IGuildWarsExecutableManager, GuildWarsExecutableManager>();
+        services.AddSingleton<IGuildWarsExecutableManager, GuildWarsExecutableManager>();
         services.AddHostedSingleton<IEventNotifierService, EventNotifierService>();
         services.AddHostedSingleton<IApiScanningService, ApiScanningService>();
         services.AddHostedSingleton<GameScreenshotsTheme>();

Daybreak.Core/Services/ExecutableManagement/GuildWarsExecutableManager.cs

@@ -1,36 +1,26 @@
 using Daybreak.Configuration.Options;
 using Daybreak.Shared.Services.ExecutableManagement;
 using Daybreak.Shared.Services.Options;
-using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using System.Core.Extensions;
 using System.Extensions;
+using System.Extensions.Core;
 
 namespace Daybreak.Services.ExecutableManagement;
 
 internal sealed class GuildWarsExecutableManager(
     IOptionsProvider optionsProvider,
     IOptionsMonitor<GuildwarsExecutableOptions> liveUpdateableOptions,
-    ILogger<GuildWarsExecutableManager> logger) : IGuildWarsExecutableManager, IHostedService
+    ILogger<GuildWarsExecutableManager> logger) : IGuildWarsExecutableManager
 {
-    private readonly static TimeSpan ExecutableVerificationLatency = TimeSpan.FromSeconds(5);
+    internal const int MaxExecutables = 10;
     private readonly static SemaphoreSlim ExecutablesSemaphore = new(1, 1);
 
     private readonly IOptionsProvider optionsProvider = optionsProvider.ThrowIfNull();
     private readonly IOptionsMonitor<GuildwarsExecutableOptions> liveUpdateableOptions = liveUpdateableOptions.ThrowIfNull();
     private readonly ILogger<GuildWarsExecutableManager> logger = logger.ThrowIfNull();
 
-    async Task IHostedService.StartAsync(CancellationToken cancellationToken)
-    {
-        await this.VerifyExecutables(cancellationToken);
-    }
-
-    Task IHostedService.StopAsync(CancellationToken cancellationToken)
-    {
-        return Task.CompletedTask;
-    }
-
     public IEnumerable<string> GetExecutableList()
     {
         ExecutablesSemaphore.Wait();
@@ -43,7 +33,6 @@ internal sealed class GuildWarsExecutableManager(
 
     public void AddExecutable(string executablePath)
     {
-        var fullPath = Path.GetFullPath(executablePath);
         ExecutablesSemaphore.Wait();
 
         var options = this.liveUpdateableOptions.CurrentValue;
@@ -53,6 +42,8 @@ internal sealed class GuildWarsExecutableManager(
             list.Insert(0, executablePath);
         }
 
+        this.TrimInvalidExecutables(list);
+
         options.ExecutablePaths = list;
         this.optionsProvider.SaveOption(options);
         ExecutablesSemaphore.Release();
@@ -80,38 +71,26 @@ internal sealed class GuildWarsExecutableManager(
         return IsValidExecutableInternal(executablePath);
     }
 
-    private async Task VerifyExecutables(CancellationToken cancellationToken)
+    /// <summary>
+    /// Soft-caps the stored executable list at <see cref="MaxExecutables"/>. Only entries that no
+    /// longer point to an existing file are evicted, oldest first, and only while the list is over
+    /// capacity. A valid executable is never evicted automatically, even if that keeps the list
+    /// above the cap. This bounds growth from stale entries without losing executables that live on
+    /// a temporarily unavailable volume (e.g. an unmounted removable or network drive), which would
+    /// otherwise be indistinguishable from a deleted file by path alone.
+    /// </summary>
+    private void TrimInvalidExecutables(List<string> executables)
     {
-        var scopedLogger = this.logger.CreateScopedLogger(nameof(this.VerifyExecutables), string.Empty);
-        while (!cancellationToken.IsCancellationRequested)
+        var scopedLogger = this.logger.CreateScopedLogger();
+        for (var i = executables.Count - 1; i >= 0 && executables.Count > MaxExecutables; i--)
         {
-            await ExecutablesSemaphore.WaitAsync(cancellationToken);
-
-            var executables = this.liveUpdateableOptions.CurrentValue.ExecutablePaths;
-            var deletedExecutable = false;
-            for (var i = 0; i < executables.Count; i++)
+            if (IsValidExecutableInternal(executables[i]))
             {
-                var executable = executables[i];
-                if (IsValidExecutableInternal(executable))
-                {
-                    continue;
-                }
-
-                scopedLogger.LogWarning($"Detected deleted executable at {executable}");
-                deletedExecutable = true;
-                executables.Remove(executable);
-                i--;
+                continue;
             }
 
-            if (deletedExecutable)
-            {
-                var options = this.liveUpdateableOptions.CurrentValue;
-                options.ExecutablePaths = executables;
-                this.optionsProvider.SaveOption(options);
-            }
-
-            ExecutablesSemaphore.Release();
-            await Task.Delay(ExecutableVerificationLatency, cancellationToken);
+            scopedLogger.LogInformation("Evicting stale executable while over capacity: {executable}", executables[i]);
+            executables.RemoveAt(i);
         }
     }
 

Daybreak.Tests/Services/ExecutableManagement/GuildWarsExecutableManagerTests.cs

@@ -0,0 +1,214 @@
+using Daybreak.Configuration.Options;
+using Daybreak.Services.ExecutableManagement;
+using Daybreak.Shared.Services.Options;
+using FluentAssertions;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Options;
+using NSubstitute;
+
+namespace Daybreak.Tests.Services.ExecutableManagement;
+
+[TestClass]
+public sealed class GuildWarsExecutableManagerTests
+{
+    private readonly IOptionsMonitor<GuildwarsExecutableOptions> liveOptions = Substitute.For<IOptionsMonitor<GuildwarsExecutableOptions>>();
+    private readonly IOptionsProvider optionsProvider = Substitute.For<IOptionsProvider>();
+    private readonly GuildwarsExecutableOptions options = new();
+    private readonly GuildWarsExecutableManager manager;
+    private string tempDir = string.Empty;
+
+    public GuildWarsExecutableManagerTests()
+    {
+        this.liveOptions.CurrentValue.Returns(this.options);
+        this.manager = new GuildWarsExecutableManager(
+            this.optionsProvider, this.liveOptions, NullLogger<GuildWarsExecutableManager>.Instance);
+    }
+
+    [TestInitialize]
+    public void Setup()
+    {
+        this.tempDir = Path.Combine(Path.GetTempPath(), "db-exe-tests-" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(this.tempDir);
+    }
+
+    [TestCleanup]
+    public void Cleanup()
+    {
+        try
+        {
+            if (Directory.Exists(this.tempDir))
+            {
+                Directory.Delete(this.tempDir, recursive: true);
+            }
+        }
+        catch
+        {
+            // Best effort cleanup.
+        }
+    }
+
+    private string CreateValidExecutable(string name)
+    {
+        var path = Path.Combine(this.tempDir, name);
+        File.WriteAllText(path, "stub");
+        return path;
+    }
+
+    // A path under a directory that does not exist - mirrors a file on an unmounted/removed
+    // volume or a genuinely deleted executable (indistinguishable by path alone).
+    private static string MissingExecutable(string name)
+        => Path.Combine(Path.GetTempPath(), "db-missing-" + Guid.NewGuid().ToString("N"), name);
+
+    [TestMethod]
+    public void AddExecutable_InsertsNewPathAtFront()
+    {
+        var existing = this.CreateValidExecutable("Gw1.exe");
+        var added = this.CreateValidExecutable("Gw2.exe");
+        this.options.ExecutablePaths.Add(existing);
+
+        this.manager.AddExecutable(added);
+
+        this.options.ExecutablePaths.Should().Equal(added, existing);
+        this.optionsProvider.Received().SaveOption(this.options);
+    }
+
+    [TestMethod]
+    public void AddExecutable_DoesNotInsertDuplicate()
+    {
+        var exe = this.CreateValidExecutable("Gw.exe");
+        this.options.ExecutablePaths.Add(exe);
+
+        this.manager.AddExecutable(exe);
+
+        this.options.ExecutablePaths.Should().ContainSingle().Which.Should().Be(exe);
+    }
+
+    [TestMethod]
+    public void AddExecutable_UnderCapacity_DoesNotEvictInvalidEntries()
+    {
+        // An invalid entry (e.g. an unmounted drive) must survive while under the cap.
+        var unavailable = MissingExecutable("Gw.exe");
+        this.options.ExecutablePaths.Add(unavailable);
+        var added = this.CreateValidExecutable("Gw2.exe");
+
+        this.manager.AddExecutable(added);
+
+        this.options.ExecutablePaths.Should().Contain(unavailable);
+        this.options.ExecutablePaths.Should().Contain(added);
+    }
+
+    [TestMethod]
+    public void AddExecutable_OverCapacity_EvictsInvalidAndKeepsValid()
+    {
+        // (cap - 1) valid + 1 invalid == cap stored entries, then add one more valid -> over cap by 1.
+        var valids = new List<string>();
+        for (var i = 0; i < GuildWarsExecutableManager.MaxExecutables - 1; i++)
+        {
+            valids.Add(this.CreateValidExecutable($"valid{i}.exe"));
+        }
+
+        var stale = MissingExecutable("stale.exe");
+        this.options.ExecutablePaths.AddRange(valids);
+        this.options.ExecutablePaths.Add(stale);
+
+        var added = this.CreateValidExecutable("new.exe");
+        this.manager.AddExecutable(added);
+
+        this.options.ExecutablePaths.Should().HaveCount(GuildWarsExecutableManager.MaxExecutables);
+        this.options.ExecutablePaths.Should().NotContain(stale);
+        this.options.ExecutablePaths.Should().Contain(added);
+        this.options.ExecutablePaths.Should().Contain(valids);
+    }
+
+    [TestMethod]
+    public void AddExecutable_OverCapacityAllValid_DoesNotEvictValidEntries()
+    {
+        var valids = new List<string>();
+        for (var i = 0; i < GuildWarsExecutableManager.MaxExecutables; i++)
+        {
+            valids.Add(this.CreateValidExecutable($"valid{i}.exe"));
+        }
+
+        this.options.ExecutablePaths.AddRange(valids);
+
+        var added = this.CreateValidExecutable("new.exe");
+        this.manager.AddExecutable(added);
+
+        // Valid executables are never evicted, even when that keeps the list above the cap.
+        this.options.ExecutablePaths.Should().HaveCount(GuildWarsExecutableManager.MaxExecutables + 1);
+        this.options.ExecutablePaths.Should().Contain(added);
+        this.options.ExecutablePaths.Should().Contain(valids);
+    }
+
+    [TestMethod]
+    public void AddExecutable_OverCapacity_EvictsOldestInvalidFirstAndOnlyAsNeeded()
+    {
+        // (cap - 2) valid + 2 invalid == cap, then add one more -> over by 1 -> evict only the oldest invalid.
+        var valids = new List<string>();
+        for (var i = 0; i < GuildWarsExecutableManager.MaxExecutables - 2; i++)
+        {
+            valids.Add(this.CreateValidExecutable($"valid{i}.exe"));
+        }
+
+        var newerStale = MissingExecutable("newer-stale.exe");
+        var olderStale = MissingExecutable("older-stale.exe");
+        this.options.ExecutablePaths.AddRange(valids);
+        this.options.ExecutablePaths.Add(newerStale);
+        this.options.ExecutablePaths.Add(olderStale); // appended last -> oldest (highest index)
+
+        var added = this.CreateValidExecutable("new.exe");
+        this.manager.AddExecutable(added);
+
+        this.options.ExecutablePaths.Should().HaveCount(GuildWarsExecutableManager.MaxExecutables);
+        this.options.ExecutablePaths.Should().NotContain(olderStale);
+        this.options.ExecutablePaths.Should().Contain(newerStale);
+        this.options.ExecutablePaths.Should().Contain(added);
+    }
+
+    [TestMethod]
+    public void GetExecutableList_ReturnsOnlyValidExecutables()
+    {
+        var valid = this.CreateValidExecutable("Gw.exe");
+        var missing = MissingExecutable("Gw.exe");
+        this.options.ExecutablePaths.Add(valid);
+        this.options.ExecutablePaths.Add(missing);
+
+        var list = this.manager.GetExecutableList().ToList();
+
+        list.Should().ContainSingle().Which.Should().Be(valid);
+    }
+
+    [TestMethod]
+    public void GetExecutableList_DoesNotRemoveInvalidEntriesFromStorage()
+    {
+        // Listing must be non-destructive: a temporarily unavailable executable stays persisted.
+        var missing = MissingExecutable("Gw.exe");
+        this.options.ExecutablePaths.Add(missing);
+
+        this.manager.GetExecutableList();
+
+        this.options.ExecutablePaths.Should().Contain(missing);
+        this.optionsProvider.DidNotReceive().SaveOption(Arg.Any<GuildwarsExecutableOptions>());
+    }
+
+    [TestMethod]
+    public void IsValidExecutable_TrueForExistingFile_FalseForMissingFile()
+    {
+        var valid = this.CreateValidExecutable("Gw.exe");
+
+        this.manager.IsValidExecutable(valid).Should().BeTrue();
+        this.manager.IsValidExecutable(MissingExecutable("Gw.exe")).Should().BeFalse();
+    }
+
+    [TestMethod]
+    public void RemoveExecutable_RemovesEntryAndSaves()
+    {
+        var exe = this.CreateValidExecutable("Gw.exe");
+        this.options.ExecutablePaths.Add(exe);
+
+        this.manager.RemoveExecutable(exe);
+
+        this.options.ExecutablePaths.Should().NotContain(exe);
+        this.optionsProvider.Received().SaveOption(this.options);
+    }
+}