diff --git a/Emby.Server.Implementations/ApplicationHost.cs b/Emby.Server.Implementations/ApplicationHost.cs index 106c938f8a..5f42a24825 100644 --- a/Emby.Server.Implementations/ApplicationHost.cs +++ b/Emby.Server.Implementations/ApplicationHost.cs @@ -792,6 +792,43 @@ namespace Emby.Server.Implementations } } + /// + /// Works out what a configuration update means for the ports this process bound at startup. + /// + /// The HTTP port this process is bound to. + /// The HTTPS port this process is bound to. + /// The HTTP port the shared configuration now carries. + /// The HTTPS port the shared configuration now carries. + /// Whether the shared configuration still marks the port as authorized. + /// Whether this update is another instance's write being applied. + /// What the update requires of this instance. + internal static PortChangeOutcome EvaluatePortChange( + int boundHttpPort, + int boundHttpsPort, + int configuredHttpPort, + int configuredHttpsPort, + bool isPortAuthorized, + bool isApplyingRemoteInvalidation) + { + // Nothing is bound yet, so nothing has gone stale. + if (boundHttpPort == 0 || boundHttpsPort == 0) + { + return default; + } + + if (configuredHttpPort == boundHttpPort && configuredHttpsPort == boundHttpsPort) + { + return default; + } + + // Whoever wrote the change, this process is still listening on a port the configuration no + // longer names, so the pending restart is reported either way. The authorization flag belongs + // to the instance that made the change: it cleared the flag along with the port, and clearing + // it again here would write shared configuration on that instance's behalf and announce it a + // second time. + return new PortChangeOutcome(true, isPortAuthorized && !isApplyingRemoteInvalidation); + } + /// /// Called when [configuration updated]. /// @@ -799,28 +836,24 @@ namespace Emby.Server.Implementations /// The instance containing the event data. private void OnConfigurationUpdated(object sender, EventArgs e) { - var requiresRestart = false; var networkConfiguration = ConfigurationManager.GetNetworkConfiguration(); - // Don't do anything if these haven't been set yet, and don't clear the authorization flag on - // behalf of another instance: it already wrote that flag along with the port change, so - // repeating the save here only races it. - if (HttpPort != 0 && HttpsPort != 0 && !ConfigurationInvalidationContext.IsApplyingRemoteInvalidation) - { - // Need to restart if ports have changed - if (networkConfiguration.InternalHttpPort != HttpPort - || networkConfiguration.InternalHttpsPort != HttpsPort) - { - if (ConfigurationManager.Configuration.IsPortAuthorized) - { - ConfigurationManager.Configuration.IsPortAuthorized = false; - ConfigurationManager.SaveConfiguration(); + var portChange = EvaluatePortChange( + HttpPort, + HttpsPort, + networkConfiguration.InternalHttpPort, + networkConfiguration.InternalHttpsPort, + ConfigurationManager.Configuration.IsPortAuthorized, + ConfigurationInvalidationContext.IsApplyingRemoteInvalidation); - requiresRestart = true; - } - } + if (portChange.ClearsPortAuthorization) + { + ConfigurationManager.Configuration.IsPortAuthorized = false; + ConfigurationManager.SaveConfiguration(); } + var requiresRestart = portChange.RequiresRestart; + if (ValidateSslCertificate(networkConfiguration)) { requiresRestart = true; diff --git a/Emby.Server.Implementations/PortChangeOutcome.cs b/Emby.Server.Implementations/PortChangeOutcome.cs new file mode 100644 index 0000000000..78ee1b5293 --- /dev/null +++ b/Emby.Server.Implementations/PortChangeOutcome.cs @@ -0,0 +1,14 @@ +namespace Emby.Server.Implementations +{ + /// + /// What a configuration update carrying different ports requires of the instance reading it. + /// + /// + /// Whether this process is still bound to a port the shared configuration no longer names, and so has + /// to report a pending restart. + /// + /// + /// Whether this instance is the one that has to clear the port authorization flag and save it. + /// + internal readonly record struct PortChangeOutcome(bool RequiresRestart, bool ClearsPortAuthorization); +} diff --git a/MediaBrowser.Common/Configuration/IConfigurationManager.cs b/MediaBrowser.Common/Configuration/IConfigurationManager.cs index b8e92d6f83..2b8becad96 100644 --- a/MediaBrowser.Common/Configuration/IConfigurationManager.cs +++ b/MediaBrowser.Common/Configuration/IConfigurationManager.cs @@ -90,8 +90,15 @@ namespace MediaBrowser.Common.Configuration /// Drops the locally cached copy of configuration another instance has written to the shared /// configuration directory, so the next read reloads it, and raises the local update event. /// + /// + /// An implementation predating the invalidation bus keeps the default, which reports that it + /// cannot drop its cache rather than quietly leaving it stale. The caller applying a remote + /// notice treats that as a failed apply and logs it. + /// /// The named configuration key, or null for the system configuration. - void InvalidateCachedConfiguration(string? key); + /// The implementation cannot drop its cached configuration. + void InvalidateCachedConfiguration(string? key) + => throw new NotSupportedException(GetType().Name + " cannot drop configuration cached from the shared configuration directory, so writes by other instances will not be picked up."); } public static class ConfigurationManagerExtensions diff --git a/tests/Jellyfin.Server.Implementations.Tests/Configuration/ApplicationHostPortChangeTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Configuration/ApplicationHostPortChangeTests.cs new file mode 100644 index 0000000000..2c3e215ce0 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Configuration/ApplicationHostPortChangeTests.cs @@ -0,0 +1,81 @@ +using Emby.Server.Implementations; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Configuration; + +/// +/// The decision ApplicationHost.OnConfigurationUpdated makes about a port change. The ports this +/// process bound are fixed for its lifetime and the pending-restart flag is per-process, so an instance +/// applying another instance's port change still has to notice its own binding went stale - while leaving +/// the authorization write, and the notice that follows it, to the instance that made the change. +/// +public static class ApplicationHostPortChangeTests +{ + /// + /// The local case, unchanged: clear the authorization flag and report the pending restart. + /// + [Fact] + public static void LocalPortChange_ClearsAuthorizationAndRequiresRestart() + { + var outcome = ApplicationHost.EvaluatePortChange(8096, 8920, 9096, 8920, true, false); + + Assert.True(outcome.RequiresRestart); + Assert.True(outcome.ClearsPortAuthorization); + } + + /// + /// The cross-instance case: the peer wrote the new port and cleared the flag with it, so this + /// instance must not write, but it is still listening on the old port and has to say so. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public static void RemotePortChange_RequiresRestartWithoutWriting(bool isPortAuthorized) + { + var outcome = ApplicationHost.EvaluatePortChange(8096, 8920, 9096, 8920, isPortAuthorized, true); + + Assert.True(outcome.RequiresRestart); + Assert.False(outcome.ClearsPortAuthorization); + } + + /// + /// A second update while a port change is already pending must not write the flag again, and the + /// binding is still stale. + /// + [Fact] + public static void LocalPortChange_WithAuthorizationAlreadyCleared_RequiresRestartWithoutWriting() + { + var outcome = ApplicationHost.EvaluatePortChange(8096, 8920, 9096, 8920, false, false); + + Assert.True(outcome.RequiresRestart); + Assert.False(outcome.ClearsPortAuthorization); + } + + /// + /// An update that leaves the ports alone is not a port change, whoever wrote it. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public static void UnchangedPorts_DoNothing(bool isApplyingRemoteInvalidation) + { + var outcome = ApplicationHost.EvaluatePortChange(8096, 8920, 8096, 8920, true, isApplyingRemoteInvalidation); + + Assert.False(outcome.RequiresRestart); + Assert.False(outcome.ClearsPortAuthorization); + } + + /// + /// Nothing is decided before the ports have been bound. + /// + [Theory] + [InlineData(0, 8920)] + [InlineData(8096, 0)] + public static void UnboundPorts_DoNothing(int boundHttpPort, int boundHttpsPort) + { + var outcome = ApplicationHost.EvaluatePortChange(boundHttpPort, boundHttpsPort, 9096, 9920, true, false); + + Assert.False(outcome.RequiresRestart); + Assert.False(outcome.ClearsPortAuthorization); + } +}