Merge pull request #17794 from Shadowghost/plugin-install-fixes

Better handle timeouts on plugin operations

3 files changed, 72 insertions, 11 deletions

diff --git a/Emby.Server.Implementations/ScheduledTasks/Tasks/PluginUpdateTask.cs b/Emby.Server.Implementations/ScheduledTasks/Tasks/PluginUpdateTask.cs
index 31153af20f..dd3da2214a 100644
--- a/Emby.Server.Implementations/ScheduledTasks/Tasks/PluginUpdateTask.cs
+++ b/Emby.Server.Implementations/ScheduledTasks/Tasks/PluginUpdateTask.cs
@@ -107,6 +107,11 @@ public class PluginUpdateTask : IScheduledTask, IConfigurableScheduledTask
             {
                 _logger.LogError(ex, "Error updating {Name}", package.Name);
             }
+            catch (TimeoutException ex)
+            {
+                // One slow download must not abort the updates for the remaining plugins.
+                _logger.LogError(ex, "Error downloading {Name}", package.Name);
+            }
             catch (InvalidDataException ex)
             {
                 _logger.LogError(ex, "Error updating {Name}", package.Name);
diff --git a/Emby.Server.Implementations/Updates/InstallationManager.cs b/Emby.Server.Implementations/Updates/InstallationManager.cs
index 174234b96b..cccdb3e6aa 100644
--- a/Emby.Server.Implementations/Updates/InstallationManager.cs
+++ b/Emby.Server.Implementations/Updates/InstallationManager.cs
@@ -11,7 +11,6 @@ using System.Security.Cryptography;
 using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
-using Jellyfin.Data.Events;
 using Jellyfin.Extensions;
 using Jellyfin.Extensions.Json;
 using MediaBrowser.Common.Configuration;
@@ -34,6 +33,9 @@ namespace Emby.Server.Implementations.Updates
     public class InstallationManager : IInstallationManager
     {
         private static readonly SearchValues<char> InvalidPackageNameChars = SearchValues.Create([.. Path.GetInvalidFileNameChars(), '/', '\\']);
+        // Budget for the whole package download. The response headers are already bounded by the
+        // HttpClient timeout; this covers reading the package body, which can be large and slow.
+        private static readonly TimeSpan PackageDownloadTimeout = TimeSpan.FromMinutes(10);
 
         /// <summary>
         /// The logger.
@@ -82,8 +84,8 @@ namespace Emby.Server.Implementations.Updates
             IServerConfigurationManager config,
             IPluginManager pluginManager)
         {
-            _currentInstallations = new List<(InstallationInfo, CancellationTokenSource)>();
-            _completedInstallationsInternal = new ConcurrentBag<InstallationInfo>();
+            _currentInstallations = [];
+            _completedInstallationsInternal = [];
 
             _logger = logger;
             _applicationHost = appHost;
@@ -341,8 +343,9 @@ namespace Emby.Server.Implementations.Updates
 
                 _applicationHost.NotifyPendingRestart();
             }
-            catch (OperationCanceledException)
+            catch (OperationCanceledException) when (linkedToken.IsCancellationRequested)
             {
+                // Only an actually cancelled token is a cancellation.
                 lock (_currentInstallationsLock)
                 {
                     _currentInstallations.Remove(tuple);
@@ -356,7 +359,7 @@ namespace Emby.Server.Implementations.Updates
             }
             catch (Exception ex)
             {
-                _logger.LogError(ex, "Package installation failed");
+                _logger.LogError(ex, "Package installation failed: {Name} {Version}", package.Name, package.Version);
 
                 lock (_currentInstallationsLock)
                 {
@@ -546,12 +549,36 @@ namespace Emby.Server.Implementations.Updates
                 throw new InvalidDataException($"Plugin package name '{package.Name}' resolves outside the plugins directory.");
             }
 
-            using var response = await _httpClientFactory.CreateClient(NamedClient.Default)
-                .GetAsync(new Uri(package.SourceUrl), cancellationToken).ConfigureAwait(false);
-            response.EnsureSuccessStatusCode();
-            Stream stream = await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false);
-            await using (stream.ConfigureAwait(false))
+            // ResponseHeadersRead keeps the body out of the HttpClient timeout, which otherwise covers
+            // the whole download; the package gets the longer budget below instead.
+            using var downloadTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+            downloadTokenSource.CancelAfter(PackageDownloadTimeout);
+            var downloadToken = downloadTokenSource.Token;
+
+            var buffer = new MemoryStream();
+            await using (buffer.ConfigureAwait(false))
             {
+                try
+                {
+                    using var response = await _httpClientFactory.CreateClient(NamedClient.Default)
+                        .GetAsync(new Uri(package.SourceUrl), HttpCompletionOption.ResponseHeadersRead, downloadToken).ConfigureAwait(false);
+                    response.EnsureSuccessStatusCode();
+
+                    // The package is read twice, for the checksum and for the extraction, so it has
+                    // to be buffered: the response stream is not seekable.
+                    await response.Content.CopyToAsync(buffer, downloadToken).ConfigureAwait(false);
+                }
+                catch (OperationCanceledException ex) when (!cancellationToken.IsCancellationRequested)
+                {
+                    // Either our budget above or the HttpClient timeout ran out.
+                    throw new TimeoutException(
+                        $"Downloading the package {package.Name} {package.Version} from {package.SourceUrl} timed out.",
+                        ex);
+                }
+
+                buffer.Position = 0;
+                Stream stream = buffer;
+
                 // CA5351: Do Not Use Broken Cryptographic Algorithms
 #pragma warning disable CA5351
                 cancellationToken.ThrowIfCancellationRequested();
diff --git a/Jellyfin.Api/Controllers/PackageController.cs b/Jellyfin.Api/Controllers/PackageController.cs
index 1f8f963f70..10bcf4e717 100644
--- a/Jellyfin.Api/Controllers/PackageController.cs
+++ b/Jellyfin.Api/Controllers/PackageController.cs
@@ -3,7 +3,9 @@ using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations;
 using System.Linq;
 using System.Threading.Tasks;
+using Jellyfin.Extensions;
 using MediaBrowser.Common.Api;
+using MediaBrowser.Common.Plugins;
 using MediaBrowser.Common.Updates;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Model.Updates;
@@ -23,16 +25,22 @@ public class PackageController : BaseJellyfinApiController
 {
     private readonly IInstallationManager _installationManager;
     private readonly IServerConfigurationManager _serverConfigurationManager;
+    private readonly IPluginManager _pluginManager;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="PackageController"/> class.
     /// </summary>
     /// <param name="installationManager">Instance of the <see cref="IInstallationManager"/> interface.</param>
     /// <param name="serverConfigurationManager">Instance of the <see cref="IServerConfigurationManager"/> interface.</param>
-    public PackageController(IInstallationManager installationManager, IServerConfigurationManager serverConfigurationManager)
+    /// <param name="pluginManager">Instance of the <see cref="IPluginManager"/> interface.</param>
+    public PackageController(
+        IInstallationManager installationManager,
+        IServerConfigurationManager serverConfigurationManager,
+        IPluginManager pluginManager)
     {
         _installationManager = installationManager;
         _serverConfigurationManager = serverConfigurationManager;
+        _pluginManager = pluginManager;
     }
 
     /// <summary>
@@ -48,6 +56,13 @@ public class PackageController : BaseJellyfinApiController
         [FromRoute, Required] string name,
         [FromQuery] Guid? assemblyGuid)
     {
+        // Plugins bundled with the server are not published to any repository, so querying
+        // the configured repositories for them can only ever fail, and does so slowly.
+        if (IsBundledPlugin(name, assemblyGuid))
+        {
+            return NotFound();
+        }
+
         var packages = await _installationManager.GetAvailablePackages().ConfigureAwait(false);
         var result = _installationManager.FilterPackages(
                 packages,
@@ -96,6 +111,11 @@ public class PackageController : BaseJellyfinApiController
         [FromQuery] string? version,
         [FromQuery] string? repositoryUrl)
     {
+        if (IsBundledPlugin(name, assemblyGuid))
+        {
+            return NotFound();
+        }
+
         var packages = await _installationManager.GetAvailablePackages().ConfigureAwait(false);
         if (!string.IsNullOrEmpty(repositoryUrl))
         {
@@ -161,4 +181,13 @@ public class PackageController : BaseJellyfinApiController
         _serverConfigurationManager.SaveConfiguration();
         return NoContent();
     }
+
+    private bool IsBundledPlugin(string name, Guid? assemblyGuid)
+    {
+        var plugin = assemblyGuid is Guid id && !id.IsEmpty()
+            ? _pluginManager.GetPlugin(id)
+            : _pluginManager.Plugins.FirstOrDefault(p => p.Name.Equals(name, StringComparison.OrdinalIgnoreCase));
+
+        return plugin?.Instance?.CanUninstall == false;
+    }
 }