Fix disabled plugins being re-enabled on restart

4 files changed, 116 insertions, 14 deletions

diff --git a/Emby.Server.Implementations/Plugins/PluginManager.cs b/Emby.Server.Implementations/Plugins/PluginManager.cs
index f699c99d85..8d29d6a512 100644
--- a/Emby.Server.Implementations/Plugins/PluginManager.cs
+++ b/Emby.Server.Implementations/Plugins/PluginManager.cs
@@ -255,6 +255,14 @@ namespace Emby.Server.Implementations.Plugins
             }
 
             _plugins.Add(plugin);
+
+            // Updating a disabled plugin must not enable it again.
+            if (plugin.Manifest.Status == PluginStatus.Disabled)
+            {
+                ProcessAlternative(plugin);
+                return;
+            }
+
             EnablePlugin(plugin);
         }
 
@@ -632,9 +640,10 @@ namespace Emby.Server.Implementations.Plugins
                 return;
             }
 
-            var predecessor = _plugins.OrderByDescending(p => p.Version)
-                .FirstOrDefault(p => p.Id.Equals(plugin.Id) && p.IsEnabledAndSupported && p.Version != plugin.Version);
-            if (predecessor is not null)
+            var successor = _plugins.FirstOrDefault(p => p.Id.Equals(plugin.Id)
+                && p.Version > plugin.Version
+                && (p.IsEnabledAndSupported || p.Manifest.Status == PluginStatus.Disabled));
+            if (successor is not null)
             {
                 return;
             }
@@ -763,6 +772,8 @@ namespace Emby.Server.Implementations.Plugins
                 var entry = versions[x];
                 if (!string.Equals(lastName, entry.Name, StringComparison.OrdinalIgnoreCase))
                 {
+                    lastName = string.Empty;
+
                     if (!TryGetPluginDlls(entry, out var allowedDlls))
                     {
                         _logger.LogError("One or more assembly paths was invalid. Marking plugin {Plugin} as \"Malfunctioned\".", entry.Name);
@@ -772,15 +783,18 @@ namespace Emby.Server.Implementations.Plugins
 
                     entry.DllFiles = allowedDlls;
 
+                    // Only clean up older versions when this version will actually be loaded.
                     if (entry.IsEnabledAndSupported)
                     {
                         lastName = entry.Name;
-                        continue;
                     }
+
+                    continue;
                 }
 
                 if (string.IsNullOrEmpty(lastName))
                 {
+                    // Unnamed plugin, so there is nothing to match older versions against.
                     continue;
                 }
 
@@ -891,9 +905,9 @@ namespace Emby.Server.Implementations.Plugins
 
             if (previousVersion is null)
             {
-                // This value is memory only - so that the web will show restart required.
-                plugin.Manifest.Status = PluginStatus.Restart;
-                plugin.Manifest.AutoUpdate = false;
+                // Memory only, so that the web will show restart required. The manifest must keep
+                // holding the persisted state, or a later save would write the wrong state to disk.
+                plugin.RestartRequired = true;
                 return;
             }
 
@@ -906,9 +920,7 @@ namespace Emby.Server.Implementations.Plugins
                 _logger.LogError("Unable to supercede version {Version} of {Name}", previousVersion.Version, previousVersion.Name);
             }
 
-            // This value is memory only - so that the web will show restart required.
-            plugin.Manifest.Status = PluginStatus.Restart;
-            plugin.Manifest.AutoUpdate = false;
+            plugin.RestartRequired = true;
         }
     }
 }
diff --git a/Emby.Server.Implementations/Updates/InstallationManager.cs b/Emby.Server.Implementations/Updates/InstallationManager.cs
index 6a60f7f5f6..174234b96b 100644
--- a/Emby.Server.Implementations/Updates/InstallationManager.cs
+++ b/Emby.Server.Implementations/Updates/InstallationManager.cs
@@ -500,8 +500,9 @@ namespace Emby.Server.Implementations.Updates
             var plugins = _pluginManager.Plugins;
             foreach (var plugin in plugins)
             {
-                // Don't auto update when plugin marked not to, or when it's disabled.
-                if (plugin.Manifest?.AutoUpdate == false || plugin.Manifest?.Status == PluginStatus.Disabled)
+                // Don't auto update when plugin marked not to, or when it's disabled or pending removal.
+                if (plugin.Manifest?.AutoUpdate == false
+                    || plugin.Manifest?.Status is PluginStatus.Disabled or PluginStatus.Deleted)
                 {
                     continue;
                 }
diff --git a/MediaBrowser.Common/Plugins/LocalPlugin.cs b/MediaBrowser.Common/Plugins/LocalPlugin.cs
index 4723be1001..221dda7d5f 100644
--- a/MediaBrowser.Common/Plugins/LocalPlugin.cs
+++ b/MediaBrowser.Common/Plugins/LocalPlugin.cs
@@ -73,6 +73,14 @@ namespace MediaBrowser.Common.Plugins
         public bool IsEnabledAndSupported => _supported && Manifest.Status >= PluginStatus.Active;
 
         /// <summary>
+        /// Gets or sets a value indicating whether a restart is required for the plugin's state to take effect.
+        /// </summary>
+        /// <remarks>
+        /// Memory only. <see cref="Manifest"/> holds the state that is persisted to disk.
+        /// </remarks>
+        public bool RestartRequired { get; set; }
+
+        /// <summary>
         /// Gets a value indicating whether the plugin has a manifest.
         /// </summary>
         public PluginManifest Manifest { get; }
@@ -108,7 +116,7 @@ namespace MediaBrowser.Common.Plugins
         public PluginInfo GetPluginInfo()
         {
             var inst = Instance?.GetPluginInfo() ?? new PluginInfo(Manifest.Name, Version, Manifest.Description, Manifest.Id, true);
-            inst.Status = Manifest.Status;
+            inst.Status = RestartRequired ? PluginStatus.Restart : Manifest.Status;
             inst.HasImage = !string.IsNullOrEmpty(Manifest.ImagePath) || !string.IsNullOrEmpty(Manifest.ImageResourceName);
             return inst;
         }
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Plugins/PluginManagerTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Plugins/PluginManagerTests.cs
index ede9e61536..265b6a7f43 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/Plugins/PluginManagerTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/Plugins/PluginManagerTests.cs
@@ -293,7 +293,84 @@ namespace Jellyfin.Server.Implementations.Tests.Plugins
             Assert.Equal(packageInfo.Versions[0].Version, result.Version);
         }
 
-        private PackageInfo GenerateTestPackage()
+        [Fact]
+        public async Task DisablePlugin_CatalogRefresh_StaysDisabled()
+        {
+            var pluginRoot = Path.Combine(_tempPath, "plugins");
+            var pluginDir = CreateTestPlugin(pluginRoot, "Disable Me", PluginStatus.Active);
+
+            var pluginManager = new PluginManager(new NullLogger<PluginManager>(), null!, null!, pluginRoot, new Version(1, 0));
+            var plugin = Assert.Single(pluginManager.Plugins);
+
+            pluginManager.DisablePlugin(plugin);
+
+            Assert.Equal(PluginStatus.Disabled, pluginManager.LoadManifest(pluginDir).Manifest.Status);
+
+            // The web shows that a restart is required, but the persisted state must not change.
+            Assert.Equal(PluginStatus.Restart, plugin.GetPluginInfo().Status);
+            Assert.Equal(PluginStatus.Disabled, plugin.Manifest.Status);
+            Assert.True(plugin.Manifest.AutoUpdate);
+
+            // Every catalog fetch rewrites the manifests of installed plugins from the in-memory status.
+            var packageInfo = GenerateTestPackage(plugin.Id);
+            await pluginManager.PopulateManifest(packageInfo, new Version(1, 0), pluginDir, plugin.Manifest.Status);
+
+            Assert.Equal(PluginStatus.Disabled, pluginManager.LoadManifest(pluginDir).Manifest.Status);
+        }
+
+        [Fact]
+        public void Constructor_DisabledPluginSortingBeforeEnabledPlugin_IsNotDeleted()
+        {
+            var pluginRoot = Path.Combine(_tempPath, "plugins");
+            var disabledDir = CreateTestPlugin(pluginRoot, "AAA Disabled", PluginStatus.Disabled);
+            CreateTestPlugin(pluginRoot, "ZZZ Active", PluginStatus.Active);
+
+            var pluginManager = new PluginManager(new NullLogger<PluginManager>(), null!, null!, pluginRoot, new Version(1, 0));
+
+            Assert.True(Directory.Exists(disabledDir));
+            Assert.Contains(pluginManager.Plugins, p => string.Equals(p.Name, "AAA Disabled", StringComparison.Ordinal));
+        }
+
+        [Fact]
+        public void LoadAssemblies_DisabledPluginWithSupersededVersion_DoesNotRevertToOldVersion()
+        {
+            var pluginRoot = Path.Combine(_tempPath, "plugins");
+            var id = Guid.NewGuid();
+            var oldDir = CreateTestPlugin(pluginRoot, "Two Versions", PluginStatus.Superseded, new Version(1, 0), id);
+            var newDir = CreateTestPlugin(pluginRoot, "Two Versions_2.0", PluginStatus.Disabled, new Version(2, 0), id, "Two Versions");
+
+            var pluginManager = new PluginManager(new NullLogger<PluginManager>(), null!, null!, pluginRoot, new Version(1, 0));
+
+            Assert.Empty(pluginManager.LoadAssemblies());
+
+            // Neither version may be touched: the old one stays superseded instead of being loaded
+            // as a stand-in for the version the user disabled.
+            Assert.Equal(PluginStatus.Superseded, pluginManager.LoadManifest(oldDir).Manifest.Status);
+            Assert.Equal(PluginStatus.Disabled, pluginManager.LoadManifest(newDir).Manifest.Status);
+        }
+
+        private string CreateTestPlugin(string root, string folderName, PluginStatus status, Version? version = null, Guid? id = null, string? name = null)
+        {
+            var dir = Path.Combine(root, folderName);
+            Directory.CreateDirectory(dir);
+            FileHelper.CreateEmpty(Path.Combine(dir, "some.dll"));
+
+            var manifest = new PluginManifest
+            {
+                Id = id ?? Guid.NewGuid(),
+                Name = name ?? folderName,
+                Status = status,
+                AutoUpdate = true,
+                TargetAbi = "1.0",
+                Version = (version ?? new Version(1, 0)).ToString()
+            };
+
+            File.WriteAllText(Path.Combine(dir, "meta.json"), JsonSerializer.Serialize(manifest, _options));
+
+            return dir;
+        }
+
+        private PackageInfo GenerateTestPackage(Guid? id = null)
         {
             var fixture = new Fixture();
             fixture.Customize<PackageInfo>(c => c.Without(x => x.Versions).Without(x => x.ImageUrl));
@@ -305,6 +382,10 @@ namespace Jellyfin.Server.Implementations.Tests.Plugins
 
             var packageInfo = fixture.Create<PackageInfo>();
             packageInfo.Versions = new[] { versionInfo };
+            if (id.HasValue)
+            {
+                packageInfo.Id = id.Value;
+            }
 
             return packageInfo;
         }