Fixed: Duplicate notifications for failed health checks

Closes #4462

src/NzbDrone.Core/HealthCheck/HealthCheckService.cs

@@ -4,9 +4,9 @@ using System.Linq;
 using NLog;
 using NzbDrone.Common.Cache;
 using NzbDrone.Common.EnvironmentInfo;
-using NzbDrone.Common.Extensions;
 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;
@@ -28,30 +28,31 @@ namespace NzbDrone.Core.HealthCheck
         private readonly IProvideHealthCheck[] _startupHealthChecks;
         private readonly IProvideHealthCheck[] _scheduledHealthChecks;
         private readonly Dictionary<Type, IEventDrivenHealthCheck[]> _eventDrivenHealthChecks;
-        private readonly IServerSideNotificationService _serverSideNotificationService;
         private readonly IEventAggregator _eventAggregator;
         private readonly ICacheManager _cacheManager;
         private readonly Logger _logger;
 
         private readonly ICached<HealthCheck> _healthCheckResults;
+        private readonly HashSet<IProvideHealthCheck> _pendingHealthChecks;
+        private readonly Debouncer _debounce;
 
         private bool _hasRunHealthChecksAfterGracePeriod;
         private bool _isRunningHealthChecksAfterGracePeriod;
 
         public HealthCheckService(IEnumerable<IProvideHealthCheck> healthChecks,
-                                  IServerSideNotificationService serverSideNotificationService,
                                   IEventAggregator eventAggregator,
                                   ICacheManager cacheManager,
                                   IRuntimeInfo runtimeInfo,
                                   Logger logger)
         {
             _healthChecks = healthChecks.ToArray();
-            _serverSideNotificationService = serverSideNotificationService;
             _eventAggregator = eventAggregator;
             _cacheManager = cacheManager;
             _logger = logger;
 
             _healthCheckResults = _cacheManager.GetCache<HealthCheck>(GetType());
+            _pendingHealthChecks = new HashSet<IProvideHealthCheck>();
+            _debounce = new Debouncer(ProcessHealthChecks, TimeSpan.FromSeconds(5));
 
             _startupHealthChecks = _healthChecks.Where(v => v.CheckOnStartup).ToArray();
             _scheduledHealthChecks = _healthChecks.Where(v => v.CheckOnSchedule).ToArray();
@@ -78,63 +79,86 @@ namespace NzbDrone.Core.HealthCheck
                 .ToDictionary(g => g.Key, g => g.ToArray());
         }
 
-        private void PerformHealthCheck(IProvideHealthCheck[] healthChecks, bool performServerChecks)
+        private void ProcessHealthChecks()
         {
-            var results = healthChecks.Select(c => c.Check())
-                                       .ToList();
+            List<IProvideHealthCheck> healthChecks;
 
-            if (performServerChecks)
+            lock (_pendingHealthChecks)
             {
-                results.AddIfNotNull(_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)
-                    {
-                        _eventAggregator.PublishEvent(new HealthCheckRestoredEvent(previous, !_hasRunHealthChecksAfterGracePeriod));
-                    }
+            try
+            {
+                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, 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;
             }
@@ -146,7 +170,16 @@ namespace NzbDrone.Core.HealthCheck
             {
                 _isRunningHealthChecksAfterGracePeriod = true;
 
-                PerformHealthCheck(_startupHealthChecks, false);
+                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;
@@ -182,9 +215,12 @@ namespace NzbDrone.Core.HealthCheck
                 }
             }
 
-            // TODO: Add debounce
+            lock (_pendingHealthChecks)
+            {
+                filteredChecks.ForEach(h => _pendingHealthChecks.Add(h));
+            }
 
-            PerformHealthCheck(filteredChecks.ToArray(), false);
+            _debounce.Execute();
         }
     }
 }

src/NzbDrone.Core/HealthCheck/ServerSideNotificationService.cs

@@ -8,22 +8,19 @@ 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 HealthCheck GetServerChecks();
-    }
-
-    public class ServerSideNotificationService : IServerSideNotificationService
+    public class ServerSideNotificationService : HealthCheckBase
     {
         private readonly IHttpClient _client;
         private readonly ISonarrCloudRequestBuilder _cloudRequestBuilder;
         private readonly IConfigFileProvider _configFileProvider;
         private readonly Logger _logger;
 
-        public ServerSideNotificationService(IHttpClient client, ISonarrCloudRequestBuilder cloudRequestBuilder, IConfigFileProvider configFileProvider, Logger logger)
+        public ServerSideNotificationService(IHttpClient client, ISonarrCloudRequestBuilder cloudRequestBuilder, IConfigFileProvider configFileProvider, ILocalizationService localizationService, Logger logger)
+            : base(localizationService)
         {
             _client = client;
             _cloudRequestBuilder = cloudRequestBuilder;
@@ -31,7 +28,7 @@ namespace NzbDrone.Core.HealthCheck
             _logger = logger;
         }
 
-        public HealthCheck GetServerChecks()
+        public override HealthCheck Check()
         {
             var request = _cloudRequestBuilder.Services.Create()
                 .Resource("/notification")