Merge pull request #17791 from Shadowghost/fix-code-migration

Don't dispose application singletons after running a code migration

Jellyfin.Server/Migrations/JellyfinMigrationService.cs

@@ -189,7 +189,7 @@ internal class JellyfinMigrationService
     /// <param name="stage">The stage to migrate.</param>
     /// <param name="serviceProvider">The service provider handed to the migrations.</param>
     /// <returns>A value indicating whether at least one migration has been applied.</returns>
-    public async Task<bool> MigrateStepAsync(JellyfinMigrationStageTypes stage, IServiceProvider? serviceProvider)
+    public async Task<bool> MigrateStepAsync(JellyfinMigrationStageTypes stage, IServiceProvider serviceProvider)
     {
         var logger = _startupLogger.With(_loggerFactory.CreateLogger<JellyfinMigrationService>()).BeginGroup($"Migrate stage {stage}.");
         ICollection<CodeMigration> migrationStage = (Migrations.FirstOrDefault(e => e.Stage == stage) as ICollection<CodeMigration>) ?? [];
@@ -453,10 +453,10 @@ internal class JellyfinMigrationService
     private class InternalCodeMigration : IInternalMigration
     {
         private readonly CodeMigration _codeMigration;
-        private readonly IServiceProvider? _serviceProvider;
+        private readonly IServiceProvider _serviceProvider;
         private JellyfinDbContext _dbContext;
 
-        public InternalCodeMigration(CodeMigration codeMigration, IServiceProvider? serviceProvider, JellyfinDbContext dbContext)
+        public InternalCodeMigration(CodeMigration codeMigration, IServiceProvider serviceProvider, JellyfinDbContext dbContext)
         {
             _codeMigration = codeMigration;
             _serviceProvider = serviceProvider;

Jellyfin.Server/Migrations/Routines/20250618010000_MigrateLibraryUserData.cs

@@ -29,7 +29,7 @@ internal class MigrateLibraryUserData : IAsyncMigrationRoutine
     private readonly IDbContextFactory<JellyfinDbContext> _provider;
 
     public MigrateLibraryUserData(
-            IStartupLogger<MigrateLibraryDb> startupLogger,
+            IStartupLogger<MigrateLibraryUserData> startupLogger,
             IDbContextFactory<JellyfinDbContext> provider,
             IServerApplicationPaths paths)
     {

Jellyfin.Server/Migrations/Routines/20250730215000_ReseedFolderFlag.cs

@@ -24,7 +24,7 @@ internal class ReseedFolderFlag : IAsyncMigrationRoutine
     private readonly IDbContextFactory<JellyfinDbContext> _provider;
 
     public ReseedFolderFlag(
-            IStartupLogger<MigrateLibraryDb> startupLogger,
+            IStartupLogger<ReseedFolderFlag> startupLogger,
             IDbContextFactory<JellyfinDbContext> provider,
             IServerApplicationPaths paths)
     {

Jellyfin.Server/Migrations/Stages/CodeMigration.cs

@@ -4,8 +4,6 @@ using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Server.ServerSetupApp;
 using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.DependencyInjection.Extensions;
-using Microsoft.Extensions.Logging;
 
 namespace Jellyfin.Server.Migrations.Stages;
 
@@ -22,66 +20,45 @@ internal class CodeMigration(Type migrationType, JellyfinMigrationAttribute meta
         return Metadata.Order.ToString("yyyyMMddHHmmsss", CultureInfo.InvariantCulture) + "_" + Metadata.Name!;
     }
 
-    private IServiceCollection MigrationServices(IServiceProvider serviceProvider, IStartupLogger logger)
+    public async Task Perform(IServiceProvider serviceProvider, IStartupLogger logger, CancellationToken cancellationToken)
     {
-        var childServiceCollection = new ServiceCollection()
-            .AddSingleton(serviceProvider)
-            .AddSingleton(logger)
-            .AddSingleton(typeof(IStartupLogger<>), typeof(NestedStartupLogger<>))
-            .AddSingleton<StartupLogTopic>(logger.Topic!);
-
-        foreach (ServiceDescriptor service in serviceProvider.GetRequiredService<IServiceCollection>())
-        {
-            if (service.Lifetime == ServiceLifetime.Singleton && !service.ServiceType.IsGenericTypeDefinition)
-            {
-                childServiceCollection.AddSingleton(service.ServiceType, _ => serviceProvider.GetService(service.ServiceType)!);
-                continue;
-            }
-
-            childServiceCollection.Add(service);
-        }
-
-        return childServiceCollection;
-    }
-
-    public async Task Perform(IServiceProvider? serviceProvider, IStartupLogger logger, CancellationToken cancellationToken)
-    {
-#pragma warning disable CS0618 // Type or member is obsolete
-        if (typeof(IMigrationRoutine).IsAssignableFrom(MigrationType))
-        {
-            if (serviceProvider is null)
-            {
-                ((IMigrationRoutine)Activator.CreateInstance(MigrationType)!).Perform();
-            }
-            else
-            {
-                using var migrationServices = MigrationServices(serviceProvider, logger).BuildServiceProvider();
-                ((IMigrationRoutine)ActivatorUtilities.CreateInstance(migrationServices, MigrationType)).Perform();
-#pragma warning restore CS0618 // Type or member is obsolete
-            }
-        }
-        else if (typeof(IAsyncMigrationRoutine).IsAssignableFrom(MigrationType))
-        {
-            if (serviceProvider is null)
-            {
-                await ((IAsyncMigrationRoutine)Activator.CreateInstance(MigrationType)!).PerformAsync(cancellationToken).ConfigureAwait(false);
-            }
-            else
-            {
-                using var migrationServices = MigrationServices(serviceProvider, logger).BuildServiceProvider();
-                await ((IAsyncMigrationRoutine)ActivatorUtilities.CreateInstance(migrationServices, MigrationType)).PerformAsync(cancellationToken).ConfigureAwait(false);
-            }
-        }
-        else
+        if (!IsMigrationRoutine(MigrationType))
         {
             throw new InvalidOperationException($"The type {MigrationType} does not implement either IMigrationRoutine or IAsyncMigrationRoutine and is not a valid migration type");
         }
-    }
 
-    private class NestedStartupLogger<TCategory> : StartupLogger<TCategory>
-    {
-        public NestedStartupLogger(ILogger logger, StartupLogTopic topic) : base(logger, topic)
+        // The routine runs against a scope of the applications own container. Copying the application service
+        // descriptors into a child container instead would make that child container the owner of every singleton it
+        // forwards, so disposing it after the migration would also dispose the applications own instance of services
+        // like the ProviderManager and leave the server broken until the next restart.
+        var scope = serviceProvider.CreateAsyncScope();
+        await using (scope.ConfigureAwait(false))
         {
+            // Nests everything the routine logs through an injected IStartupLogger under the migrations own topic.
+            using (StartupLogger.BeginAmbientTopic(logger.Topic))
+            {
+                await RunAsync(ActivatorUtilities.CreateInstance(scope.ServiceProvider, MigrationType), cancellationToken).ConfigureAwait(false);
+            }
         }
     }
+
+    // The obsolete IMigrationRoutine is still implemented by every routine that predates the async interface, so
+    // the members that have to touch it are grouped here behind a single suppression.
+#pragma warning disable CS0618 // Type or member is obsolete
+    private static bool IsMigrationRoutine(Type migrationType)
+    {
+        return typeof(IMigrationRoutine).IsAssignableFrom(migrationType) || typeof(IAsyncMigrationRoutine).IsAssignableFrom(migrationType);
+    }
+
+    private static async Task RunAsync(object routine, CancellationToken cancellationToken)
+    {
+        if (routine is IMigrationRoutine migrationRoutine)
+        {
+            migrationRoutine.Perform();
+            return;
+        }
+
+        await ((IAsyncMigrationRoutine)routine).PerformAsync(cancellationToken).ConfigureAwait(false);
+    }
+#pragma warning restore CS0618 // Type or member is obsolete
 }

Jellyfin.Server/Program.cs

@@ -181,9 +181,7 @@ namespace Jellyfin.Server
                     })
                     .ConfigureAppConfiguration(config => config.ConfigureAppConfiguration(options, appPaths, startupConfig))
                     .UseSerilog()
-                    .ConfigureServices(e => e
-                        .RegisterStartupLogger()
-                        .AddSingleton<IServiceCollection>(e))
+                    .ConfigureServices(e => e.RegisterStartupLogger())
                     .Build();
 
                 /*
@@ -308,7 +306,6 @@ namespace Jellyfin.Server
                 .AddSingleton<ServerApplicationPaths>(appPaths)
                 .RegisterStartupLogger();
 
-            migrationStartupServiceProvider.AddSingleton(migrationStartupServiceProvider);
             var startupService = migrationStartupServiceProvider.BuildServiceProvider();
 
             PrepareDatabaseProvider(startupService);

Jellyfin.Server/ServerSetupApp/StartupLogger.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Globalization;
+using System.Threading;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 
@@ -8,6 +9,8 @@ namespace Jellyfin.Server.ServerSetupApp;
 /// <inheritdoc/>
 public class StartupLogger : IStartupLogger
 {
+    private static readonly AsyncLocal<StartupLogTopic?> _ambientTopic = new();
+
     private readonly StartupLogTopic? _topic;
 
     /// <summary>
@@ -17,6 +20,7 @@ public class StartupLogger : IStartupLogger
     public StartupLogger(ILogger logger)
     {
         BaseLogger = logger;
+        _topic = _ambientTopic.Value;
     }
 
     /// <summary>
@@ -39,6 +43,18 @@ public class StartupLogger : IStartupLogger
     /// </summary>
     protected ILogger BaseLogger { get; set; }
 
+    /// <summary>
+    /// Makes <paramref name="topic"/> the topic that loggers created on this execution context attach to.
+    /// </summary>
+    /// <param name="topic">The topic to nest newly created loggers under.</param>
+    /// <returns>A scope that restores the previously ambient topic when disposed.</returns>
+    internal static IDisposable BeginAmbientTopic(StartupLogTopic? topic)
+    {
+        var scope = new AmbientTopicScope(_ambientTopic.Value);
+        _ambientTopic.Value = topic;
+        return scope;
+    }
+
     /// <inheritdoc/>
     public IStartupLogger BeginGroup(FormattableString logEntry)
     {
@@ -121,4 +137,19 @@ public class StartupLogger : IStartupLogger
             Topic.Children.Add(startupEntry);
         }
     }
+
+    private sealed class AmbientTopicScope : IDisposable
+    {
+        private readonly StartupLogTopic? _previous;
+
+        public AmbientTopicScope(StartupLogTopic? previous)
+        {
+            _previous = previous;
+        }
+
+        public void Dispose()
+        {
+            _ambientTopic.Value = _previous;
+        }
+    }
 }

tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs

@@ -0,0 +1,112 @@
+using System;
+using System.Threading;
+using System.Threading.Tasks;
+using Jellyfin.Server.Migrations;
+using Jellyfin.Server.Migrations.Stages;
+using Jellyfin.Server.ServerSetupApp;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging.Abstractions;
+using Xunit;
+
+namespace Jellyfin.Server.Tests.Migrations;
+
+public class CodeMigrationTests
+{
+    [Fact]
+    public async Task Perform_LeavesApplicationSingletonsAlive()
+    {
+        var services = new ServiceCollection()
+            .AddLogging()
+            .RegisterStartupLogger()
+            .AddSingleton<ApplicationSingleton>()
+            .AddTransient<MigrationTransient>();
+
+        await using var serviceProvider = services.BuildServiceProvider();
+        var applicationSingleton = serviceProvider.GetRequiredService<ApplicationSingleton>();
+        var logger = new StartupLogger(NullLogger.Instance).BeginGroup($"Test migration");
+
+        var migration = new CodeMigration(
+            typeof(TestMigration),
+            new JellyfinMigrationAttribute("2026-09-05T10:00:00", nameof(TestMigration)),
+            null);
+        await migration.Perform(serviceProvider, logger, CancellationToken.None);
+
+        var performed = TestMigration.Performed;
+        Assert.NotNull(performed);
+        // The migration has to run against the applications own services, and they have to outlive it.
+        Assert.Same(applicationSingleton, performed.Singleton);
+        Assert.False(applicationSingleton.IsDisposed);
+        Assert.Same(applicationSingleton, serviceProvider.GetRequiredService<ApplicationSingleton>());
+        // Services created for the migration itself are still owned by the migration.
+        Assert.True(performed.Transient.IsDisposed);
+        // The startup logger has to stay attached to the topic of the running migration.
+        Assert.Same(logger.Topic, performed.Logger.Topic);
+    }
+
+    [Fact]
+    public async Task Perform_DoesNotLeakTheMigrationTopic()
+    {
+        var services = new ServiceCollection()
+            .AddLogging()
+            .RegisterStartupLogger()
+            .AddSingleton<ApplicationSingleton>()
+            .AddTransient<MigrationTransient>();
+
+        await using var serviceProvider = services.BuildServiceProvider();
+        var logger = new StartupLogger(NullLogger.Instance).BeginGroup($"Test migration");
+
+        var migration = new CodeMigration(
+            typeof(TestMigration),
+            new JellyfinMigrationAttribute("2026-09-05T10:00:00", nameof(TestMigration)),
+            null);
+        await migration.Perform(serviceProvider, logger, CancellationToken.None);
+
+        // The topic belongs to the migration that ran, so loggers resolved afterwards must not still write into it.
+        Assert.Null(serviceProvider.GetRequiredService<IStartupLogger<CodeMigrationTests>>().Topic);
+        Assert.Null(new StartupLogger(NullLogger.Instance).Topic);
+    }
+
+    private sealed class ApplicationSingleton : IDisposable
+    {
+        public bool IsDisposed { get; private set; }
+
+        public void Dispose()
+        {
+            IsDisposed = true;
+        }
+    }
+
+    private sealed class MigrationTransient : IDisposable
+    {
+        public bool IsDisposed { get; private set; }
+
+        public void Dispose()
+        {
+            IsDisposed = true;
+        }
+    }
+
+    private sealed class TestMigration : IAsyncMigrationRoutine
+    {
+        public TestMigration(ApplicationSingleton singleton, MigrationTransient transient, IStartupLogger<TestMigration> logger)
+        {
+            Singleton = singleton;
+            Transient = transient;
+            Logger = logger;
+        }
+
+        public static TestMigration? Performed { get; private set; }
+
+        public ApplicationSingleton Singleton { get; }
+
+        public MigrationTransient Transient { get; }
+
+        public IStartupLogger<TestMigration> Logger { get; }
+
+        public Task PerformAsync(CancellationToken cancellationToken)
+        {
+            Performed = this;
+            return Task.CompletedTask;
+        }
+    }
+}

tests/Jellyfin.Server.Tests/ServerSetupApp/StartupLoggerTests.cs

@@ -0,0 +1,54 @@
+using Jellyfin.Server.ServerSetupApp;
+using Microsoft.Extensions.Logging.Abstractions;
+using Xunit;
+
+namespace Jellyfin.Server.Tests.ServerSetupApp;
+
+public class StartupLoggerTests
+{
+    [Fact]
+    public void BeginAmbientTopic_AttachesNewLoggersToTheTopic()
+    {
+        var migration = new StartupLogger(NullLogger.Instance).BeginGroup($"Migration");
+
+        using (StartupLogger.BeginAmbientTopic(migration.Topic))
+        {
+            Assert.Same(migration.Topic, new StartupLogger(NullLogger.Instance).Topic);
+        }
+    }
+
+    [Fact]
+    public void BeginAmbientTopic_RestoresThePreviousTopic()
+    {
+        var root = new StartupLogger(NullLogger.Instance);
+        var outer = root.BeginGroup($"Outer");
+        var inner = outer.BeginGroup($"Inner");
+
+        Assert.Null(new StartupLogger(NullLogger.Instance).Topic);
+
+        using (StartupLogger.BeginAmbientTopic(outer.Topic))
+        {
+            using (StartupLogger.BeginAmbientTopic(inner.Topic))
+            {
+                Assert.Same(inner.Topic, new StartupLogger(NullLogger.Instance).Topic);
+            }
+
+            // Leaving a nested topic has to fall back to the enclosing one, not to the setup UI root.
+            Assert.Same(outer.Topic, new StartupLogger(NullLogger.Instance).Topic);
+        }
+
+        Assert.Null(new StartupLogger(NullLogger.Instance).Topic);
+    }
+
+    [Fact]
+    public void BeginGroup_KeepsAnExplicitTopicOverTheAmbientOne()
+    {
+        var migration = new StartupLogger(NullLogger.Instance).BeginGroup($"Migration");
+        var unrelated = new StartupLogger(NullLogger.Instance).BeginGroup($"Unrelated");
+
+        using (StartupLogger.BeginAmbientTopic(migration.Topic))
+        {
+            Assert.Same(unrelated.Topic, unrelated.With(NullLogger.Instance).Topic);
+        }
+    }
+}