diff options
6 files changed, 137 insertions, 109 deletions
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<JellyfinDbContext> _provider; public MigrateLibraryUserData( - IStartupLogger<MigrateLibraryDb> startupLogger, + IStartupLogger<MigrateLibraryUserData> startupLogger, IDbContextFactory<JellyfinDbContext> 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<JellyfinDbContext> _provider; public ReseedFolderFlag( - IStartupLogger<MigrateLibraryDb> startupLogger, + IStartupLogger<ReseedFolderFlag> startupLogger, IDbContextFactory<JellyfinDbContext> 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"); } - } - /// <summary> - /// Provides the services a migration routine is constructed with. - /// </summary> - /// <remarks> - /// 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 <c>ProviderManager</c> and leave the server broken until the next restart. - /// </remarks> - 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<IServiceProviderIsService>(); - } - - 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<TCategory> : StartupLogger<TCategory> + 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; /// <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; + } + } } 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<ApplicationSingleton>() .AddTransient<MigrationTransient>(); - services.AddSingleton(services); await using var serviceProvider = services.BuildServiceProvider(); var applicationSingleton = serviceProvider.GetRequiredService<ApplicationSingleton>(); @@ -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<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; } 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); + } + } +} |