From fb21dd0f196aa653f3c89dc9c7b53e33487b0ad8 Mon Sep 17 00:00:00 2001 From: Mark McDowall Date: Sun, 10 Sep 2023 13:07:57 -0700 Subject: [PATCH] Fixed: Duplicate notifications for failed health checks Mock debouncer for testing (cherry picked from commit c0e54773e213f526a5046fa46aa7b57532471128) (cherry picked from commit bb7b2808e2f70389157408809ec47cc8860b4938) --- src/NzbDrone.Common/TPL/DebounceManager.cs | 17 +++ src/NzbDrone.Common/TPL/Debouncer.cs | 17 +-- .../HealthCheck/HealthCheckServiceFixture.cs | 10 +- .../HealthCheck/HealthCheckService.cs | 126 ++++++++++-------- .../HealthCheck/IProvideHealthCheck.cs | 7 - .../ServerSideNotificationService.cs | 57 ++++---- src/NzbDrone.Test.Common/MockDebouncer.cs | 21 +++ 7 files changed, 159 insertions(+), 96 deletions(-) create mode 100644 src/NzbDrone.Common/TPL/DebounceManager.cs create mode 100644 src/NzbDrone.Test.Common/MockDebouncer.cs diff --git a/src/NzbDrone.Common/TPL/DebounceManager.cs b/src/NzbDrone.Common/TPL/DebounceManager.cs new file mode 100644 index 000000000..60803a3a9 --- /dev/null +++ b/src/NzbDrone.Common/TPL/DebounceManager.cs @@ -0,0 +1,17 @@ +using System; + +namespace NzbDrone.Common.TPL +{ + public interface IDebounceManager + { + Debouncer CreateDebouncer(Action action, TimeSpan debounceDuration); + } + + public class DebounceManager : IDebounceManager + { + public Debouncer CreateDebouncer(Action action, TimeSpan debounceDuration) + { + return new Debouncer(action, debounceDuration); + } + } +} diff --git a/src/NzbDrone.Common/TPL/Debouncer.cs b/src/NzbDrone.Common/TPL/Debouncer.cs index f10d4bc4d..555f161bb 100644 --- a/src/NzbDrone.Common/TPL/Debouncer.cs +++ b/src/NzbDrone.Common/TPL/Debouncer.cs @@ -4,12 +4,12 @@ namespace NzbDrone.Common.TPL { public class Debouncer { - private readonly Action _action; - private readonly System.Timers.Timer _timer; - private readonly bool _executeRestartsTimer; + protected readonly Action _action; + protected readonly System.Timers.Timer _timer; + protected readonly bool _executeRestartsTimer; - private volatile int _paused; - private volatile bool _triggered; + protected volatile int _paused; + protected volatile bool _triggered; public Debouncer(Action action, TimeSpan debounceDuration, bool executeRestartsTimer = false) { @@ -29,11 +29,12 @@ namespace NzbDrone.Common.TPL } } - public void Execute() + public virtual void Execute() { lock (_timer) { _triggered = true; + if (_executeRestartsTimer) { _timer.Stop(); @@ -46,7 +47,7 @@ namespace NzbDrone.Common.TPL } } - public void Pause() + public virtual void Pause() { lock (_timer) { @@ -55,7 +56,7 @@ namespace NzbDrone.Common.TPL } } - public void Resume() + public virtual void Resume() { lock (_timer) { diff --git a/src/NzbDrone.Core.Test/HealthCheck/HealthCheckServiceFixture.cs b/src/NzbDrone.Core.Test/HealthCheck/HealthCheckServiceFixture.cs index 74802a39e..4ec0860ea 100644 --- a/src/NzbDrone.Core.Test/HealthCheck/HealthCheckServiceFixture.cs +++ b/src/NzbDrone.Core.Test/HealthCheck/HealthCheckServiceFixture.cs @@ -1,10 +1,14 @@ +using System; using System.Collections.Generic; using FluentAssertions; +using Moq; using NUnit.Framework; using NzbDrone.Common.Cache; using NzbDrone.Common.Messaging; +using NzbDrone.Common.TPL; using NzbDrone.Core.HealthCheck; using NzbDrone.Core.Test.Framework; +using NzbDrone.Test.Common; namespace NzbDrone.Core.Test.HealthCheck { @@ -19,10 +23,10 @@ namespace NzbDrone.Core.Test.HealthCheck Mocker.SetConstant>(new[] { _healthCheck }); Mocker.SetConstant(Mocker.Resolve()); + Mocker.SetConstant(Mocker.Resolve()); - Mocker.GetMock() - .Setup(v => v.GetServerChecks()) - .Returns(new List()); + Mocker.GetMock().Setup(s => s.CreateDebouncer(It.IsAny(), It.IsAny())) + .Returns((a, t) => new MockDebouncer(a, t)); } [Test] diff --git a/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs b/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs index 13546bfd7..03b872676 100644 --- a/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs +++ b/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs @@ -1,11 +1,11 @@ using System; using System.Collections.Generic; using System.Linq; -using NLog; using NzbDrone.Common.Cache; using NzbDrone.Common.EnvironmentInfo; using NzbDrone.Common.Messaging; using NzbDrone.Common.Reflection; +using NzbDrone.Common.TPL; using NzbDrone.Core.Lifecycle; using NzbDrone.Core.Messaging.Commands; using NzbDrone.Core.Messaging.Events; @@ -27,30 +27,27 @@ namespace NzbDrone.Core.HealthCheck private readonly IProvideHealthCheck[] _startupHealthChecks; private readonly IProvideHealthCheck[] _scheduledHealthChecks; private readonly Dictionary _eventDrivenHealthChecks; - private readonly IServerSideNotificationService _serverSideNotificationService; private readonly IEventAggregator _eventAggregator; - private readonly ICacheManager _cacheManager; - private readonly Logger _logger; private readonly ICached _healthCheckResults; + private readonly HashSet _pendingHealthChecks; + private readonly Debouncer _debounce; private bool _hasRunHealthChecksAfterGracePeriod; private bool _isRunningHealthChecksAfterGracePeriod; public HealthCheckService(IEnumerable healthChecks, - IServerSideNotificationService serverSideNotificationService, IEventAggregator eventAggregator, ICacheManager cacheManager, - IRuntimeInfo runtimeInfo, - Logger logger) + IDebounceManager debounceManager, + IRuntimeInfo runtimeInfo) { _healthChecks = healthChecks.ToArray(); - _serverSideNotificationService = serverSideNotificationService; _eventAggregator = eventAggregator; - _cacheManager = cacheManager; - _logger = logger; - _healthCheckResults = _cacheManager.GetCache(GetType()); + _healthCheckResults = cacheManager.GetCache(GetType()); + _pendingHealthChecks = new HashSet(); + _debounce = debounceManager.CreateDebouncer(ProcessHealthChecks, TimeSpan.FromSeconds(5)); _startupHealthChecks = _healthChecks.Where(v => v.CheckOnStartup).ToArray(); _scheduledHealthChecks = _healthChecks.Where(v => v.CheckOnSchedule).ToArray(); @@ -77,74 +74,86 @@ namespace NzbDrone.Core.HealthCheck .ToDictionary(g => g.Key, g => g.ToArray()); } - private void PerformHealthCheck(IProvideHealthCheck[] healthChecks, IEvent message = null, bool performServerChecks = false) + private void ProcessHealthChecks() { - var results = new List(); + List healthChecks; - foreach (var healthCheck in healthChecks) + lock (_pendingHealthChecks) { - if (healthCheck is IProvideHealthCheckWithMessage && message != null) - { - results.Add(((IProvideHealthCheckWithMessage)healthCheck).Check(message)); - } - else - { - results.Add(healthCheck.Check()); - } + healthChecks = _pendingHealthChecks.ToList(); + _pendingHealthChecks.Clear(); } - if (performServerChecks) - { - results.AddRange(_serverSideNotificationService.GetServerChecks()); - } + _debounce.Pause(); - foreach (var result in results) + try { - if (result.Type == HealthCheckResult.Ok) - { - var previous = _healthCheckResults.Find(result.Source.Name); - - if (previous != null) - { - _eventAggregator.PublishEvent(new HealthCheckRestoredEvent(previous, !_hasRunHealthChecksAfterGracePeriod)); - } + var results = healthChecks.Select(c => c.Check()) + .ToList(); - _healthCheckResults.Remove(result.Source.Name); - } - else + foreach (var result in results) { - if (_healthCheckResults.Find(result.Source.Name) == null) + if (result.Type == HealthCheckResult.Ok) { - _eventAggregator.PublishEvent(new HealthCheckFailedEvent(result, !_hasRunHealthChecksAfterGracePeriod)); + var previous = _healthCheckResults.Find(result.Source.Name); + + if (previous != null) + { + _eventAggregator.PublishEvent(new HealthCheckRestoredEvent(previous, !_hasRunHealthChecksAfterGracePeriod)); + } + + _healthCheckResults.Remove(result.Source.Name); } + else + { + if (_healthCheckResults.Find(result.Source.Name) == null) + { + _eventAggregator.PublishEvent(new HealthCheckFailedEvent(result, !_hasRunHealthChecksAfterGracePeriod)); + } - _healthCheckResults.Set(result.Source.Name, result); + _healthCheckResults.Set(result.Source.Name, result); + } } } + finally + { + _debounce.Resume(); + } _eventAggregator.PublishEvent(new HealthCheckCompleteEvent()); } public void Execute(CheckHealthCommand message) { - if (message.Trigger == CommandTrigger.Manual) - { - PerformHealthCheck(_healthChecks, null, true); - } - else + var healthChecks = message.Trigger == CommandTrigger.Manual ? _healthChecks : _scheduledHealthChecks; + + lock (_pendingHealthChecks) { - PerformHealthCheck(_scheduledHealthChecks, null, true); + foreach (var healthCheck in healthChecks) + { + _pendingHealthChecks.Add(healthCheck); + } } + + ProcessHealthChecks(); } public void HandleAsync(ApplicationStartedEvent message) { - PerformHealthCheck(_startupHealthChecks, null, true); + lock (_pendingHealthChecks) + { + foreach (var healthCheck in _startupHealthChecks) + { + _pendingHealthChecks.Add(healthCheck); + } + } + + ProcessHealthChecks(); } public void HandleAsync(IEvent message) { - if (message is HealthCheckCompleteEvent) + if (message is HealthCheckCompleteEvent || message is ApplicationStartedEvent) { return; } @@ -155,7 +164,16 @@ namespace NzbDrone.Core.HealthCheck { _isRunningHealthChecksAfterGracePeriod = true; - PerformHealthCheck(_startupHealthChecks); + lock (_pendingHealthChecks) + { + foreach (var healthCheck in _startupHealthChecks) + { + _pendingHealthChecks.Add(healthCheck); + } + } + + // Call it directly so it's not debounced and any alerts can be sent. + ProcessHealthChecks(); // Update after running health checks so new failure notifications aren't sent 2x. _hasRunHealthChecksAfterGracePeriod = true; @@ -191,8 +209,12 @@ namespace NzbDrone.Core.HealthCheck } } - // TODO: Add debounce - PerformHealthCheck(filteredChecks.ToArray(), message); + lock (_pendingHealthChecks) + { + filteredChecks.ForEach(h => _pendingHealthChecks.Add(h)); + } + + _debounce.Execute(); } } } diff --git a/src/NzbDrone.Core/HealthCheck/IProvideHealthCheck.cs b/src/NzbDrone.Core/HealthCheck/IProvideHealthCheck.cs index 46e7048ef..d71f2653f 100644 --- a/src/NzbDrone.Core/HealthCheck/IProvideHealthCheck.cs +++ b/src/NzbDrone.Core/HealthCheck/IProvideHealthCheck.cs @@ -1,5 +1,3 @@ -using NzbDrone.Common.Messaging; - namespace NzbDrone.Core.HealthCheck { public interface IProvideHealthCheck @@ -8,9 +6,4 @@ namespace NzbDrone.Core.HealthCheck bool CheckOnStartup { get; } bool CheckOnSchedule { get; } } - - public interface IProvideHealthCheckWithMessage : IProvideHealthCheck - { - HealthCheck Check(IEvent message); - } } diff --git a/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs b/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs index 0be0557ba..e09a49812 100644 --- a/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs +++ b/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs @@ -9,63 +9,68 @@ using NzbDrone.Common.EnvironmentInfo; using NzbDrone.Common.Http; using NzbDrone.Common.Serializer; using NzbDrone.Core.Configuration; +using NzbDrone.Core.Localization; namespace NzbDrone.Core.HealthCheck { - public interface IServerSideNotificationService - { - public List GetServerChecks(); - } - - public class ServerSideNotificationService : IServerSideNotificationService + public class ServerSideNotificationService : HealthCheckBase { private readonly IHttpClient _client; private readonly IConfigFileProvider _configFileProvider; - private readonly IHttpRequestBuilderFactory _cloudRequestBuilder; + private readonly ILidarrCloudRequestBuilder _cloudRequestBuilder; private readonly Logger _logger; - private readonly ICached> _cache; + private readonly ICached _cache; public ServerSideNotificationService(IHttpClient client, - IConfigFileProvider configFileProvider, ILidarrCloudRequestBuilder cloudRequestBuilder, + IConfigFileProvider configFileProvider, + ILocalizationService localizationService, ICacheManager cacheManager, Logger logger) + : base(localizationService) { _client = client; + _cloudRequestBuilder = cloudRequestBuilder; _configFileProvider = configFileProvider; - _cloudRequestBuilder = cloudRequestBuilder.Services; _logger = logger; - _cache = cacheManager.GetCache>(GetType()); + _cache = cacheManager.GetCache(GetType()); } - public List GetServerChecks() + public override HealthCheck Check() { - return _cache.Get("ServerChecks", () => RetrieveServerChecks(), TimeSpan.FromHours(2)); + return _cache.Get("ServerChecks", RetrieveServerChecks, TimeSpan.FromHours(2)); } - private List RetrieveServerChecks() + private HealthCheck RetrieveServerChecks() { - var request = _cloudRequestBuilder.Create() - .Resource("/notification") - .AddQueryParam("version", BuildInfo.Version) - .AddQueryParam("os", OsInfo.Os.ToString().ToLowerInvariant()) - .AddQueryParam("arch", RuntimeInformation.OSArchitecture) - .AddQueryParam("runtime", "netcore") - .AddQueryParam("branch", _configFileProvider.Branch) - .Build(); + var request = _cloudRequestBuilder.Services.Create() + .Resource("/notification") + .AddQueryParam("version", BuildInfo.Version) + .AddQueryParam("os", OsInfo.Os.ToString().ToLowerInvariant()) + .AddQueryParam("arch", RuntimeInformation.OSArchitecture) + .AddQueryParam("runtime", "netcore") + .AddQueryParam("branch", _configFileProvider.Branch) + .Build(); + try { - _logger.Trace("Getting server side health notifications"); + _logger.Trace("Getting notifications"); + var response = _client.Execute(request); var result = Json.Deserialize>(response.Content); - return result.Select(x => new HealthCheck(GetType(), x.Type, x.Message, x.WikiUrl)).ToList(); + + var checks = result.Select(x => new HealthCheck(GetType(), x.Type, x.Message, x.WikiUrl)).ToList(); + + // Only one health check is supported, services returns an ordered list, so use the first one + return checks.FirstOrDefault() ?? new HealthCheck(GetType()); } catch (Exception ex) { - _logger.Error(ex, "Failed to retrieve server notifications"); - return new List(); + _logger.Error(ex, "Failed to retrieve notifications"); + + return new HealthCheck(GetType()); } } } diff --git a/src/NzbDrone.Test.Common/MockDebouncer.cs b/src/NzbDrone.Test.Common/MockDebouncer.cs new file mode 100644 index 000000000..aa85135b2 --- /dev/null +++ b/src/NzbDrone.Test.Common/MockDebouncer.cs @@ -0,0 +1,21 @@ +using System; +using NzbDrone.Common.TPL; + +namespace NzbDrone.Test.Common +{ + public class MockDebouncer : Debouncer + { + public MockDebouncer(Action action, TimeSpan debounceDuration) + : base(action, debounceDuration) + { + } + + public override void Execute() + { + lock (_timer) + { + _action(); + } + } + } +}