From 33a5fbce9b467ec2f528298ed577ee3ba1240f5e Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 22:56:28 +1000 Subject: [PATCH] retry building the quick connect store in the startup probe, not just reading it --- .../ApplicationHost.cs | 43 ++++++++++++++----- Jellyfin.Server/Program.cs | 2 +- .../QuickConnect/QuickConnectStartupTests.cs | 38 +++++++++++++++- .../QuickConnectStoreWiringTests.cs | 9 ++-- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/Emby.Server.Implementations/ApplicationHost.cs b/Emby.Server.Implementations/ApplicationHost.cs index 4826d7e6d6..83291a8ebb 100644 --- a/Emby.Server.Implementations/ApplicationHost.cs +++ b/Emby.Server.Implementations/ApplicationHost.cs @@ -52,6 +52,7 @@ using Jellyfin.Server.Implementations.SystemBackupService; using MediaBrowser.Common; using MediaBrowser.Common.Configuration; using MediaBrowser.Common.Events; +using MediaBrowser.Common.Extensions; using MediaBrowser.Common.Net; using MediaBrowser.Common.Plugins; using MediaBrowser.Common.Updates; @@ -670,29 +671,45 @@ namespace Emby.Server.Implementations } /// - /// Reads the quick connect store here so a store that cannot be reached stops startup, rather than - /// being discovered on the first request that needs it. A read rather than a resolve because a - /// shared store built with abortConnect=false constructs without touching the network, and - /// retried until so a starting instance rides out the blip - /// a running one already tolerates. + /// Builds and reads the quick connect store here so a store that cannot be reached stops startup, + /// rather than being discovered on the first request that needs it. Both halves sit inside the + /// retry: a store built with abortConnect=false constructs without touching the network and + /// only the read settles it, while one built without that option connects eagerly and fails at the + /// resolve. Retried until so a starting instance rides out + /// the blip a running one already tolerates. /// private async Task ProbeQuickConnectStoreAsync() { - var store = Resolve(); var startTimestamp = Stopwatch.GetTimestamp(); + var reported = false; while (true) { try { + // A throwing singleton factory is not cached, so the resolve is retried along with the read. + var store = Resolve(); await store.GetRequestBySecretAsync(StartupProbeSecret).ConfigureAwait(false); return; } catch (Exception ex) { - if (Stopwatch.GetElapsedTime(startTimestamp) + _quickConnectProbeRetryDelay < _quickConnectProbeDeadline) + var elapsed = Stopwatch.GetElapsedTime(startTimestamp); + if (elapsed + _quickConnectProbeRetryDelay < _quickConnectProbeDeadline) { - Logger.LogWarning(ex, "Quick connect store is not reachable yet, retrying."); + if (!reported) + { + reported = true; + Logger.LogWarning( + ex, + "Quick connect store is not reachable yet, retrying for up to {Seconds}s.", + (int)_quickConnectProbeDeadline.TotalSeconds); + } + else + { + Logger.LogDebug(ex, "Quick connect store is still not reachable, retrying."); + } + await Task.Delay(_quickConnectProbeRetryDelay).ConfigureAwait(false); continue; } @@ -701,8 +718,14 @@ namespace Emby.Server.Implementations ex, "Quick connect is configured against the shared valkey/Redis store at {Key} and it is UNREACHABLE after {Seconds}s, so the server will not start. Bring valkey up, or clear that setting to keep quick connect state on this instance alone.", TranscodeStoreOptions.RedisConnectionStringKey, - (int)_quickConnectProbeDeadline.TotalSeconds); - throw; + (int)elapsed.TotalSeconds); + + if (ex is ServiceUnavailableException) + { + throw; + } + + throw new ServiceUnavailableException("Quick connect store is unreachable.", ex); } } } diff --git a/Jellyfin.Server/Program.cs b/Jellyfin.Server/Program.cs index 01fc5cd2d6..0a850d56a0 100644 --- a/Jellyfin.Server/Program.cs +++ b/Jellyfin.Server/Program.cs @@ -267,7 +267,7 @@ namespace Jellyfin.Server if (_setupServer!.IsAlive && !configurationCompleted) { _setupServer!.SoftStop(); - if (options.StartupMode is null or Configuration.StartupMode.MediaServer && !IsRunningInContainer()) + if ((options.StartupMode is null or Configuration.StartupMode.MediaServer) && !IsRunningInContainer()) { await Task.Delay(_setupServerHoldAfterFailedStart).ConfigureAwait(false); } diff --git a/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStartupTests.cs b/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStartupTests.cs index f6e29aa099..fe8fa82010 100644 --- a/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStartupTests.cs +++ b/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStartupTests.cs @@ -46,6 +46,7 @@ public sealed class QuickConnectStartupTests : IAsyncLifetime private const string RedisConnectionStringVariable = "Jellyfin__TranscodeStore__RedisConnectionString"; private const string FfmpegNoValidationVariable = "JELLYFIN_FFMPEG__NOVALIDATION"; private const string DeadStore = "127.0.0.1:1,abortConnect=false,connectTimeout=250,connectRetry=0,syncTimeout=250"; + private const string EagerDeadStore = "127.0.0.1:1,connectTimeout=250,connectRetry=0,syncTimeout=250"; private RedisTestServer _redis = null!; @@ -85,6 +86,33 @@ public sealed class QuickConnectStartupTests : IAsyncLifetime && entry.Contains("valkey", StringComparison.Ordinal)); } + /// + /// Three of the four connection strings the chart documents leave abortConnect at its default, + /// which connects eagerly, so the multiplexer is what fails and it fails while the store is being + /// built rather than on a read. The probe has to retry the build as well as the read and end on the + /// same message, not let a bare out. + /// + /// + /// The core initialisation migrations are skipped here because JellyfinMigrationService takes + /// an IBackupService eagerly, which reaches the multiplexer through the library manager, so on + /// this shape they fail before the probe is reached at all. That ordering is a separate problem from + /// what the probe does when it runs. + /// + [Fact] + public void EagerlyConnectingStoreDownPastTheDeadline_StopsStartupWithTheSameMessage() + { + Environment.SetEnvironmentVariable(RedisConnectionStringVariable, EagerDeadStore); + + using var server = new StartupHarness(runCoreInitialisationMigrations: false); + + Assert.ThrowsAny(() => server.Services); + Assert.Contains( + server.CriticalEntries, + entry => entry.Contains("Quick connect", StringComparison.Ordinal) + && entry.Contains("UNREACHABLE", StringComparison.Ordinal) + && entry.Contains("valkey", StringComparison.Ordinal)); + } + /// /// A store that is away when the probe first reads it but back inside the deadline lets the server /// come up. A running instance already rides out a valkey blip; a starting one has to as well, or a @@ -288,6 +316,7 @@ public sealed class QuickConnectStartupTests : IAsyncLifetime private readonly ConcurrentBag _disposables = new(); private readonly ConcurrentQueue<(LogLevel Level, string Message)> _entries = new(); private readonly Action? _beforeInitializeServices; + private readonly bool _runCoreInitialisationMigrations; private readonly string _root = Path.Combine( Path.GetTempPath(), "jellyfin-quickconnect-startup", @@ -298,9 +327,10 @@ public sealed class QuickConnectStartupTests : IAsyncLifetime StartupHelpers.PerformStaticInitialization(); } - public StartupHarness(Action? beforeInitializeServices = null) + public StartupHarness(Action? beforeInitializeServices = null, bool runCoreInitialisationMigrations = true) { _beforeInitializeServices = beforeInitializeServices; + _runCoreInitialisationMigrations = runCoreInitialisationMigrations; } public IReadOnlyCollection CriticalEntries => Messages(LogLevel.Critical); @@ -356,7 +386,11 @@ public sealed class QuickConnectStartupTests : IAsyncLifetime var configuration = host.Services.GetRequiredService(); Program.ApplyStartupMigrationAsync(appPaths, configuration, new StartupOptions()).GetAwaiter().GetResult(); - Program.ApplyCoreMigrationsAsync(host.Services, JellyfinMigrationStageTypes.CoreInitialisation).GetAwaiter().GetResult(); + if (_runCoreInitialisationMigrations) + { + Program.ApplyCoreMigrationsAsync(host.Services, JellyfinMigrationStageTypes.CoreInitialisation).GetAwaiter().GetResult(); + } + _beforeInitializeServices?.Invoke(); appHost.InitializeServices(configuration).GetAwaiter().GetResult(); Program.ApplyCoreMigrationsAsync(host.Services, JellyfinMigrationStageTypes.AppInitialisation).GetAwaiter().GetResult(); diff --git a/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStoreWiringTests.cs b/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStoreWiringTests.cs index f16fee5c40..73c33bedf5 100644 --- a/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStoreWiringTests.cs +++ b/tests/Jellyfin.Server.Tests/QuickConnect/QuickConnectStoreWiringTests.cs @@ -108,9 +108,11 @@ public sealed class QuickConnectStoreWiringTests : IAsyncLifetime } /// - /// An unreachable connection string that connects eagerly, the default, cannot even build the store. - /// The lazily connecting form a deployment uses builds one, and the startup read in - /// is what stops the server coming up on that. + /// An unreachable connection string that connects eagerly, the default, throws while the store is + /// being built rather than on a read. A failed singleton factory is not cached, so every resolve + /// throws afresh, which is what lets the startup probe in + /// retry the build and report an eager store's outage as the same operator-facing failure it reports + /// for the lazily connecting form. /// [Fact] public void UnreachableRedisAtStartup_FailsClosed() @@ -120,6 +122,7 @@ public sealed class QuickConnectStoreWiringTests : IAsyncLifetime using var provider = BuildProvider(); Assert.ThrowsAny(() => provider.GetRequiredService()); + Assert.ThrowsAny(() => provider.GetRequiredService()); } private static QuickConnectResult NewRequest() => new QuickConnectResult(