From c622a16261d8c4e364a05caeecee798c54803501 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sat, 5 Sep 2026 07:23:39 +0200 Subject: Don't dispose application singletons after running a code migration --- .../Migrations/CodeMigrationTests.cs | 90 ++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs (limited to 'tests') diff --git a/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs b/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs new file mode 100644 index 0000000000..68dd4486be --- /dev/null +++ b/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs @@ -0,0 +1,90 @@ +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() + .AddTransient(); + services.AddSingleton(services); + + await using var serviceProvider = services.BuildServiceProvider(); + var applicationSingleton = serviceProvider.GetRequiredService(); + 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()); + // 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); + } + + 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 logger) + { + Singleton = singleton; + Transient = transient; + Logger = logger; + } + + public static TestMigration? Performed { get; private set; } + + public ApplicationSingleton Singleton { get; } + + public MigrationTransient Transient { get; } + + public IStartupLogger Logger { get; } + + public Task PerformAsync(CancellationToken cancellationToken) + { + Performed = this; + return Task.CompletedTask; + } + } +} -- cgit v1.2.3 From ca90347dc242e7dc5e49e6fb8938e084564ab2de Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sun, 6 Sep 2026 07:46:18 +0200 Subject: Resolve migration routine loggers from the application container --- .../20250618010000_MigrateLibraryUserData.cs | 2 +- .../Routines/20250730215000_ReseedFolderFlag.cs | 2 +- Jellyfin.Server/Migrations/Stages/CodeMigration.cs | 133 +++++---------------- Jellyfin.Server/ServerSetupApp/StartupLogger.cs | 31 +++++ .../Migrations/CodeMigrationTests.cs | 24 +++- .../ServerSetupApp/StartupLoggerTests.cs | 54 +++++++++ 6 files changed, 137 insertions(+), 109 deletions(-) create mode 100644 tests/Jellyfin.Server.Tests/ServerSetupApp/StartupLoggerTests.cs (limited to 'tests') diff --git a/Jellyfin.Server/Migrations/Routines/20250618010000_MigrateLibraryUserData.cs b/Jellyfin.Server/Migrations/Routines/20250618010000_MigrateLibraryUserData.cs index 8a0a1741f1..291de23b2e 100644 --- a/Jellyfin.Server/Migrations/Routines/20250618010000_MigrateLibraryUserData.cs +++ b/Jellyfin.Server/Migrations/Routines/20250618010000_MigrateLibraryUserData.cs @@ -29,7 +29,7 @@ internal class MigrateLibraryUserData : IAsyncMigrationRoutine private readonly IDbContextFactory _provider; public MigrateLibraryUserData( - IStartupLogger startupLogger, + IStartupLogger startupLogger, IDbContextFactory provider, IServerApplicationPaths paths) { diff --git a/Jellyfin.Server/Migrations/Routines/20250730215000_ReseedFolderFlag.cs b/Jellyfin.Server/Migrations/Routines/20250730215000_ReseedFolderFlag.cs index 502763ac09..c8ee44a670 100644 --- a/Jellyfin.Server/Migrations/Routines/20250730215000_ReseedFolderFlag.cs +++ b/Jellyfin.Server/Migrations/Routines/20250730215000_ReseedFolderFlag.cs @@ -24,7 +24,7 @@ internal class ReseedFolderFlag : IAsyncMigrationRoutine private readonly IDbContextFactory _provider; public ReseedFolderFlag( - IStartupLogger startupLogger, + IStartupLogger startupLogger, IDbContextFactory provider, IServerApplicationPaths paths) { diff --git a/Jellyfin.Server/Migrations/Stages/CodeMigration.cs b/Jellyfin.Server/Migrations/Stages/CodeMigration.cs index 69e5cbc375..30e9f1d3d0 100644 --- a/Jellyfin.Server/Migrations/Stages/CodeMigration.cs +++ b/Jellyfin.Server/Migrations/Stages/CodeMigration.cs @@ -4,7 +4,6 @@ using System.Threading; using System.Threading.Tasks; using Jellyfin.Server.ServerSetupApp; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Logging; namespace Jellyfin.Server.Migrations.Stages; @@ -23,127 +22,49 @@ internal class CodeMigration(Type migrationType, JellyfinMigrationAttribute meta 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 - { - var migrationServices = new MigrationServiceProvider(serviceProvider, logger); - await using (migrationServices.ConfigureAwait(false)) - { - ((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 - { - var migrationServices = new MigrationServiceProvider(serviceProvider, logger); - await using (migrationServices.ConfigureAwait(false)) - { - 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"); } - } - /// - /// Provides the services a migration routine is constructed with. - /// - /// - /// This overlays the migration scoped logging services onto a scope of the application 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. - /// - private sealed class MigrationServiceProvider : IServiceProvider, IServiceProviderIsService, IAsyncDisposable - { - private readonly AsyncServiceScope _scope; - private readonly IStartupLogger _logger; - private readonly IServiceProviderIsService? _isService; - - public MigrationServiceProvider(IServiceProvider serviceProvider, IStartupLogger logger) + if (serviceProvider is null) { - _scope = serviceProvider.CreateAsyncScope(); - _logger = logger; - _isService = _scope.ServiceProvider.GetService(); - } - - public object? GetService(Type serviceType) - { - if (serviceType == typeof(IServiceProvider)) - { - return this; - } - - if (serviceType == typeof(IServiceProviderIsService)) - { - return _isService is null ? null : this; - } - - if (serviceType == typeof(IStartupLogger)) - { - return _logger; - } - - if (serviceType == typeof(StartupLogTopic)) - { - return _logger.Topic; - } - - if (IsCategoryLogger(serviceType)) - { - var category = serviceType.GenericTypeArguments[0]; - var baseLogger = _scope.ServiceProvider.GetRequiredService(typeof(ILogger<>).MakeGenericType(category)); - return Activator.CreateInstance(typeof(NestedStartupLogger<>).MakeGenericType(category), baseLogger, _logger.Topic); - } - - return _scope.ServiceProvider.GetService(serviceType); + await RunAsync(Activator.CreateInstance(MigrationType)!, cancellationToken).ConfigureAwait(false); + return; } - public bool IsService(Type serviceType) + // 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)) { - if (serviceType == typeof(IServiceProvider) - || serviceType == typeof(IServiceProviderIsService) - || serviceType == typeof(IStartupLogger) - || serviceType == typeof(StartupLogTopic) - || IsCategoryLogger(serviceType)) + // Nests everything the routine logs through an injected IStartupLogger under the migrations own topic. + using (StartupLogger.BeginAmbientTopic(logger.Topic)) { - return true; + await RunAsync(ActivatorUtilities.CreateInstance(scope.ServiceProvider, MigrationType), cancellationToken).ConfigureAwait(false); } - - return _isService?.IsService(serviceType) ?? false; - } - - public ValueTask DisposeAsync() - { - return _scope.DisposeAsync(); } + } - private static bool IsCategoryLogger(Type serviceType) - { - return serviceType.IsConstructedGenericType && serviceType.GetGenericTypeDefinition() == typeof(IStartupLogger<>); - } + // 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 class NestedStartupLogger : StartupLogger + private static async Task RunAsync(object routine, CancellationToken cancellationToken) { - public NestedStartupLogger(ILogger logger, StartupLogTopic? topic) : base(logger, topic) + if (routine is IMigrationRoutine migrationRoutine) { + migrationRoutine.Perform(); + return; } + + await ((IAsyncMigrationRoutine)routine).PerformAsync(cancellationToken).ConfigureAwait(false); } +#pragma warning restore CS0618 // Type or member is obsolete } diff --git a/Jellyfin.Server/ServerSetupApp/StartupLogger.cs b/Jellyfin.Server/ServerSetupApp/StartupLogger.cs index 0121854ce3..b72b0c0eab 100644 --- a/Jellyfin.Server/ServerSetupApp/StartupLogger.cs +++ b/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; /// public class StartupLogger : IStartupLogger { + private static readonly AsyncLocal _ambientTopic = new(); + private readonly StartupLogTopic? _topic; /// @@ -17,6 +20,7 @@ public class StartupLogger : IStartupLogger public StartupLogger(ILogger logger) { BaseLogger = logger; + _topic = _ambientTopic.Value; } /// @@ -39,6 +43,18 @@ public class StartupLogger : IStartupLogger /// protected ILogger BaseLogger { get; set; } + /// + /// Makes the topic that loggers created on this execution context attach to. + /// + /// The topic to nest newly created loggers under. + /// A scope that restores the previously ambient topic when disposed. + internal static IDisposable BeginAmbientTopic(StartupLogTopic? topic) + { + var scope = new AmbientTopicScope(_ambientTopic.Value); + _ambientTopic.Value = topic; + return scope; + } + /// 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; + } + } } diff --git a/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs b/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs index 68dd4486be..3bd8581a5f 100644 --- a/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs +++ b/tests/Jellyfin.Server.Tests/Migrations/CodeMigrationTests.cs @@ -20,7 +20,6 @@ public class CodeMigrationTests .RegisterStartupLogger() .AddSingleton() .AddTransient(); - services.AddSingleton(services); await using var serviceProvider = services.BuildServiceProvider(); var applicationSingleton = serviceProvider.GetRequiredService(); @@ -44,6 +43,29 @@ public class CodeMigrationTests Assert.Same(logger.Topic, performed.Logger.Topic); } + [Fact] + public async Task Perform_DoesNotLeakTheMigrationTopic() + { + var services = new ServiceCollection() + .AddLogging() + .RegisterStartupLogger() + .AddSingleton() + .AddTransient(); + + 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>().Topic); + Assert.Null(new StartupLogger(NullLogger.Instance).Topic); + } + private sealed class ApplicationSingleton : IDisposable { public bool IsDisposed { get; private set; } diff --git a/tests/Jellyfin.Server.Tests/ServerSetupApp/StartupLoggerTests.cs b/tests/Jellyfin.Server.Tests/ServerSetupApp/StartupLoggerTests.cs new file mode 100644 index 0000000000..c2894e9647 --- /dev/null +++ b/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); + } + } +} -- cgit v1.2.3