diff --git a/FactorioWebInterface/FactorioWebInterface.csproj b/FactorioWebInterface/FactorioWebInterface.csproj index ee30595..784196f 100644 --- a/FactorioWebInterface/FactorioWebInterface.csproj +++ b/FactorioWebInterface/FactorioWebInterface.csproj @@ -109,7 +109,7 @@ - + diff --git a/FactorioWebInterface/Services/Discord/ChannelUpdater.cs b/FactorioWebInterface/Services/Discord/ChannelUpdater.cs index 33db80a..e2563be 100644 --- a/FactorioWebInterface/Services/Discord/ChannelUpdater.cs +++ b/FactorioWebInterface/Services/Discord/ChannelUpdater.cs @@ -17,6 +17,15 @@ public sealed class ChannelUpdater : IChannelUpdater { private static TimeSpan requestTimeout = TimeSpan.FromSeconds(10); private static TimeSpan throttleTimeout = TimeSpan.FromMinutes(5); + // Discord has an undocumented rate limit of 2 name changes per channel per 10 minutes + // (separate from topic changes). Name changes over the limit can be dropped silently, + // the request succeeds but the name is left unchanged, so after sending a name change + // wait for the gateway to update the cached channel and check the name actually changed. + private static TimeSpan nameVerificationDelay = TimeSpan.FromSeconds(10); + private static TimeSpan nameRateLimitedRetryTimeout = TimeSpan.FromMinutes(15); + // When a request is cancelled (e.g. waiting out a rate limit pause exceeded requestTimeout) + // back off instead of retrying immediately, else the retries tight loop every requestTimeout. + private static TimeSpan cancelledRetryTimeout = TimeSpan.FromMinutes(1); private readonly IFactorioServerDataService _factorioServerDataService; private readonly ILogger _logger; @@ -73,12 +82,21 @@ private async void QueueConsumer(ChannelReader reader) try { - await DoUpdate(); - await _timeSystem.Delay(throttleTimeout); + bool nameChangePending = await DoUpdate(); + if (nameChangePending) + { + ScheduleUpdate(); + await _timeSystem.Delay(nameRateLimitedRetryTimeout); + } + else + { + await _timeSystem.Delay(throttleTimeout); + } } catch (OperationCanceledException) { ScheduleUpdate(); + await _timeSystem.Delay(cancelledRetryTimeout); } catch (Exception ex) { @@ -87,15 +105,22 @@ private async void QueueConsumer(ChannelReader reader) } } - private async Task DoUpdate() + private async Task DoUpdate() { var status = await GetChannelStatus(); string? name = status.Name; string? topic = status.Topic; + // Only send the name when it actually changes, so routine topic updates don't + // count towards the name change rate limit. + if (name != null && NamesEquivalent(channel.Name, name)) + { + name = null; + } + if (name == null && topic == null) { - return; + return false; } void Modify(TextChannelProperties props) @@ -120,6 +145,36 @@ void Modify(TextChannelProperties props) }; await channel.ModifyAsync(Modify, requestOptions); + + if (name == null) + { + return false; + } + + await _timeSystem.Delay(nameVerificationDelay); + return !NamesEquivalent(channel.Name, name); + } + + // Discord normalizes channel names (e.g. lowercases them and replaces spaces), + // so an applied name can differ from the requested one by exact comparison. + private static bool NamesEquivalent(string? currentName, string requestedName) + { + if (currentName == null || currentName.Length != requestedName.Length) + { + return false; + } + + for (int i = 0; i < requestedName.Length; i++) + { + if (Normalize(currentName[i]) != Normalize(requestedName[i])) + { + return false; + } + } + + return true; + + static char Normalize(char c) => c == ' ' ? '-' : char.ToLowerInvariant(c); } private Task GetChannelStatus() diff --git a/FactorioWebInterfaceTests/Services/Discord/ChannelUpdaterTests/ScheduleUpdate.cs b/FactorioWebInterfaceTests/Services/Discord/ChannelUpdaterTests/ScheduleUpdate.cs index 4dad2de..02a0a27 100644 --- a/FactorioWebInterfaceTests/Services/Discord/ChannelUpdaterTests/ScheduleUpdate.cs +++ b/FactorioWebInterfaceTests/Services/Discord/ChannelUpdaterTests/ScheduleUpdate.cs @@ -76,7 +76,8 @@ public async Task AfterModify_WaitsBeforeNextModify() // Assert. await modifyEvent.WaitAsyncWithTimeout(5000); - Assert.NotNull(name); + // The name was applied by the first modify, so the second one only sends the topic. + Assert.Null(name); Assert.NotNull(topic); } @@ -173,6 +174,93 @@ public async Task CanceledException_ReSchedules() Assert.NotNull(topic); } + [Fact] + public async Task WhenChannelNameAlreadyCorrect_DoesNotSendName() + { + // Arrange. + string? name = null; + string? topic = null; + + var sent = new TaskCompletionSource(); + + var channelUpdater = MakeChannelUpdater((n, t) => + { + name = n; + topic = t; + sent.SetResult(default); + }, + initialChannelName: "s1-offline"); + + // Act. + channelUpdater.ScheduleUpdate(); + await sent.Task.TimeoutAfter(1000); + + // Assert. + Assert.Null(name); + Assert.NotNull(topic); + } + + [Fact] + public async Task NameComparison_IgnoresDiscordNormalization() + { + // Arrange. + string? name = null; + string? topic = null; + + var sent = new TaskCompletionSource(); + + var channelUpdater = MakeChannelUpdater((n, t) => + { + name = n; + topic = t; + sent.SetResult(default); + }, + initialChannelName: "S1 Offline"); + + // Act. + channelUpdater.ScheduleUpdate(); + await sent.Task.TimeoutAfter(1000); + + // Assert. + Assert.Null(name); + Assert.NotNull(topic); + } + + [Fact] + public async Task WhenNameChangeNotApplied_RetriesNameChange() + { + // Arrange. + int count = 0; + string? firstName = null; + string? secondName = null; + var modifyEvent = new AsyncManualResetEvent(); + + var channelUpdater = MakeChannelUpdater((n, t) => + { + count++; + + if (count == 1) + { + firstName = n; + } + else if (count == 2) + { + secondName = n; + modifyEvent.Set(); + } + }, + applyNameChanges: false); + + // Act. + channelUpdater.ScheduleUpdate(); + await modifyEvent.WaitAsyncWithTimeout(5000); + channelUpdater.Dispose(); + + // Assert. + Assert.NotNull(firstName); + Assert.NotNull(secondName); + } + [Fact] public async Task AfterDispose_DoesNotModify() { @@ -203,7 +291,9 @@ private ChannelUpdater MakeChannelUpdater( Action? callback = null, IFactorioServerDataService? factorioServerDataService = null, TestLogger? logger = null, - ITimeSystem? timeSystem = null) + ITimeSystem? timeSystem = null, + string initialChannelName = "old-name", + bool applyNameChanges = true) { if (factorioServerDataService == null) { @@ -225,19 +315,28 @@ private ChannelUpdater MakeChannelUpdater( factorioServerDataService, logger ?? new TestLogger(), timeSystem, - MakeTextChannel(callback ?? ((_, __) => { })), + MakeTextChannel(callback ?? ((_, __) => { }), initialChannelName, applyNameChanges), "1"); } - private static ITextChannel MakeTextChannel(Action callback) + private static ITextChannel MakeTextChannel(Action callback, string initialChannelName = "old-name", bool applyNameChanges = true) { + string channelName = initialChannelName; + var channel = new Mock(MockBehavior.Strict); + channel.SetupGet(x => x.Name).Returns(() => channelName); channel.Setup(x => x.ModifyAsync(It.IsAny>(), It.IsAny())) .Returns((Action func, RequestOptions _) => { var prop = new TextChannelProperties(); func(prop); callback(prop.Name.GetValueOrDefault(), prop.Topic.GetValueOrDefault()); + + if (applyNameChanges && prop.Name.IsSpecified) + { + channelName = prop.Name.Value; + } + return Task.CompletedTask; });