diff options
8 files changed, 229 insertions, 58 deletions
diff --git a/Jellyfin.Server/Migrations/JellyfinMigrationService.cs b/Jellyfin.Server/Migrations/JellyfinMigrationService.cs index 5ef039a843..a5b6bc4604 100644 --- a/Jellyfin.Server/Migrations/JellyfinMigrationService.cs +++ b/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; 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 971b47608f..71706811b8 100644 --- a/Jellyfin.Server/Migrations/Stages/CodeMigration.cs +++ b/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!); + if (!IsMigrationRoutine(MigrationType)) + { + throw new InvalidOperationException($"The type {MigrationType} does not implement either IMigrationRoutine or IAsyncMigrationRoutine and is not a valid migration type"); + } - foreach (ServiceDescriptor service in serviceProvider.GetRequiredService<IServiceCollection>()) + // 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 (service.Lifetime == ServiceLifetime.Singleton && !service.ServiceType.IsGenericTypeDefinition) + // Nests everything the routine logs through an injected IStartupLogger under the migrations own topic. + using (StartupLogger.BeginAmbientTopic(logger.Topic)) { - childServiceCollection.AddSingleton(service.ServiceType, _ => serviceProvider.GetService(service.ServiceType)!); - continue; + await RunAsync(ActivatorUtilities.CreateInstance(scope.ServiceProvider, MigrationType), cancellationToken).ConfigureAwait(false); } - - childServiceCollection.Add(service); } - - return childServiceCollection; } - public async Task Perform(IServiceProvider? serviceProvider, IStartupLogger logger, CancellationToken cancellationToken) - { + // 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 - 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 - { - throw new InvalidOperationException($"The type {MigrationType} does not implement either IMigrationRoutine or IAsyncMigrationRoutine and is not a valid migration type"); - } + 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/Program.cs b/Jellyfin.Server/Program.cs index 58861fc476..35eaff6532 100644 --- a/Jellyfin.Server/Program.cs +++ b/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); 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 new file mode 100644 index 0000000000..3bd8581a5f --- /dev/null +++ b/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; + } + } +} 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); + } + } +} |