From c41a7e0cccdf7435c5ea198585fdaf3a910c896f 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 (cherry picked from commit c0e54773e213f526a5046fa46aa7b57532471128) Mock debouncer for testing (cherry picked from commit bb7b2808e2f70389157408809ec47cc8860b4938) --- src/NzbDrone.Common/TPL/DebounceManager.cs | 17 +++ src/NzbDrone.Common/TPL/Debouncer.cs | 14 +- .../HealthCheck/HealthCheckServiceFixture.cs | 10 +- .../HealthCheck/HealthCheckService.cs | 126 ++++++++++++------ .../ServerSideNotificationService.cs | 46 +++---- src/NzbDrone.Test.Common/MockDebouncer.cs | 21 +++ 6 files changed, 156 insertions(+), 78 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 0fa101525..7f8435961 100644 --- a/src/NzbDrone.Common/TPL/Debouncer.cs +++ b/src/NzbDrone.Common/TPL/Debouncer.cs @@ -4,11 +4,11 @@ namespace NzbDrone.Common.TPL { public class Debouncer { - private readonly Action _action; - private readonly System.Timers.Timer _timer; + protected readonly Action _action; + protected readonly System.Timers.Timer _timer; - private volatile int _paused; - private volatile bool _triggered; + protected volatile int _paused; + protected volatile bool _triggered; public Debouncer(Action action, TimeSpan debounceDuration) { @@ -27,7 +27,7 @@ namespace NzbDrone.Common.TPL } } - public void Execute() + public virtual void Execute() { lock (_timer) { @@ -39,7 +39,7 @@ namespace NzbDrone.Common.TPL } } - public void Pause() + public virtual void Pause() { lock (_timer) { @@ -48,7 +48,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 550849232..a9a89dc48 100644 --- a/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs +++ b/src/NzbDrone.Core/HealthCheck/HealthCheckService.cs @@ -6,6 +6,7 @@ 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,35 +28,35 @@ 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, + IDebounceManager debounceManager, IRuntimeInfo runtimeInfo, Logger logger) { _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(); _eventDrivenHealthChecks = GetEventDrivenHealthChecks(); - _startupGracePeriodEndTime = runtimeInfo.StartTime.AddMinutes(15); + _startupGracePeriodEndTime = runtimeInfo.StartTime + TimeSpan.FromMinutes(15); } public List Results() @@ -77,70 +78,93 @@ namespace NzbDrone.Core.HealthCheck .ToDictionary(g => g.Key, g => g.ToArray()); } - private void PerformHealthCheck(IProvideHealthCheck[] healthChecks, bool performServerChecks = false) + private void ProcessHealthChecks() { - var results = healthChecks.Select(c => - { - _logger.Trace("Check health -> {0}", c.GetType().Name); - var result = c.Check(); - _logger.Trace("Check health <- {0}", c.GetType().Name); - - return result; - }) - .ToList(); + List healthChecks; - if (performServerChecks) + lock (_pendingHealthChecks) { - results.AddRange(_serverSideNotificationService.GetServerChecks()); + healthChecks = _pendingHealthChecks.ToList(); + _pendingHealthChecks.Clear(); } - foreach (var result in results) - { - if (result.Type == HealthCheckResult.Ok) - { - var previous = _healthCheckResults.Find(result.Source.Name); + _debounce.Pause(); - if (previous != null) + try + { + var results = healthChecks.Select(c => { - _eventAggregator.PublishEvent(new HealthCheckRestoredEvent(previous, !_hasRunHealthChecksAfterGracePeriod)); - } + _logger.Trace("Check health -> {0}", c.GetType().Name); + var result = c.Check(); + _logger.Trace("Check health <- {0}", c.GetType().Name); - _healthCheckResults.Remove(result.Source.Name); - } - else + return result; + }) + .ToList(); + + 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, true); - } - else + var healthChecks = message.Trigger == CommandTrigger.Manual ? _healthChecks : _scheduledHealthChecks; + + lock (_pendingHealthChecks) { - PerformHealthCheck(_scheduledHealthChecks, true); + foreach (var healthCheck in healthChecks) + { + _pendingHealthChecks.Add(healthCheck); + } } + + ProcessHealthChecks(); } public void HandleAsync(ApplicationStartedEvent message) { - PerformHealthCheck(_startupHealthChecks, 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; } @@ -151,7 +175,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; @@ -183,11 +216,16 @@ namespace NzbDrone.Core.HealthCheck if (eventDrivenHealthCheck.ShouldExecute(message, previouslyFailed)) { filteredChecks.Add(eventDrivenHealthCheck.HealthCheck); + continue; } } - // TODO: Add debounce - PerformHealthCheck(filteredChecks.ToArray()); + lock (_pendingHealthChecks) + { + filteredChecks.ForEach(h => _pendingHealthChecks.Add(h)); + } + + _debounce.Execute(); } } } diff --git a/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs b/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs index dd742bdf6..51420e63b 100644 --- a/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs +++ b/src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs @@ -9,50 +9,43 @@ 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 IProwlarrCloudRequestBuilder _cloudRequestBuilder; private readonly IConfigFileProvider _configFileProvider; - private readonly IHttpRequestBuilderFactory _cloudRequestBuilder; private readonly Logger _logger; - private readonly ICached> _cache; + private readonly ICached _cache; - public ServerSideNotificationService(IHttpClient client, - IConfigFileProvider configFileProvider, - IProwlarrCloudRequestBuilder cloudRequestBuilder, - ICacheManager cacheManager, - Logger logger) + public ServerSideNotificationService(IHttpClient client, IProwlarrCloudRequestBuilder cloudRequestBuilder, IConfigFileProvider configFileProvider, ICacheManager cacheManager, ILocalizationService localizationService, Logger logger) + : base(localizationService) { _client = client; _configFileProvider = configFileProvider; - _cloudRequestBuilder = cloudRequestBuilder.Services; + _cloudRequestBuilder = cloudRequestBuilder; _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)); } - private List RetrieveServerChecks() + private HealthCheck RetrieveServerChecks() { if (BuildInfo.IsDebug) { - return new List(); + return new HealthCheck(GetType()); } - var request = _cloudRequestBuilder.Create() + var request = _cloudRequestBuilder.Services.Create() .Resource("/notification") .AddQueryParam("version", BuildInfo.Version) .AddQueryParam("os", OsInfo.Os.ToString().ToLowerInvariant()) @@ -63,17 +56,22 @@ namespace NzbDrone.Core.HealthCheck 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"); - } + _logger.Error(ex, "Failed to retrieve notifications"); - return new List(); + 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(); + } + } + } +}