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

@@ -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);
+        }
+    }
+}

src/NzbDrone.Common/TPL/Debouncer.cs

@@ -4,12 +4,12 @@ namespace NzbDrone.Common.TPL
 {
     public class Debouncer
     {
-        private readonly Action _action;
+        protected readonly Action _action;
-        private readonly System.Timers.Timer _timer;
+        protected readonly System.Timers.Timer _timer;
-        private readonly bool _executeRestartsTimer;
+        protected readonly bool _executeRestartsTimer;
 
-        private volatile int _paused;
+        protected volatile int _paused;
-        private volatile bool _triggered;
+        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)
             {

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<IEnumerable<IProvideHealthCheck>>(new[] { _healthCheck });
             Mocker.SetConstant<ICacheManager>(Mocker.Resolve<CacheManager>());
+            Mocker.SetConstant<IDebounceManager>(Mocker.Resolve<DebounceManager>());
 
-            Mocker.GetMock<IServerSideNotificationService>()
+            Mocker.GetMock<IDebounceManager>().Setup(s => s.CreateDebouncer(It.IsAny<Action>(), It.IsAny<TimeSpan>()))
-                .Setup(v => v.GetServerChecks())
+                .Returns<Action, TimeSpan>((a, t) => new MockDebouncer(a, t));
-                .Returns(new List<Core.HealthCheck.HealthCheck>());
         }
 
         [Test]

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<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,
+                                  IDebounceManager debounceManager,
-                                  Logger logger)
+                                  IRuntimeInfo runtimeInfo)
         {
             _healthChecks = healthChecks.ToArray();
-            _serverSideNotificationService = serverSideNotificationService;
             _eventAggregator = eventAggregator;
-            _cacheManager = cacheManager;
-            _logger = logger;
 
-            _healthCheckResults = _cacheManager.GetCache<HealthCheck>(GetType());
+            _healthCheckResults = cacheManager.GetCache<HealthCheck>(GetType());
+            _pendingHealthChecks = new HashSet<IProvideHealthCheck>();
+            _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<HealthCheck>();
+            List<IProvideHealthCheck> healthChecks;
 
-            foreach (var healthCheck in healthChecks)
+            lock (_pendingHealthChecks)
             {
-                if (healthCheck is IProvideHealthCheckWithMessage && message != null)
+                healthChecks = _pendingHealthChecks.ToList();
-                {
+                _pendingHealthChecks.Clear();
-                    results.Add(((IProvideHealthCheckWithMessage)healthCheck).Check(message));
-                }
-                else
-                {
-                    results.Add(healthCheck.Check());
-                }
             }
 
-            if (performServerChecks)
+            _debounce.Pause();
-            {
-                results.AddRange(_serverSideNotificationService.GetServerChecks());
-            }
 
-            foreach (var result in results)
+            try
             {
-                if (result.Type == HealthCheckResult.Ok)
+                var results = healthChecks.Select(c => c.Check())
-                {
+                    .ToList();
-                    var previous = _healthCheckResults.Find(result.Source.Name);
-
-                    if (previous != null)
-                    {
-                        _eventAggregator.PublishEvent(new HealthCheckRestoredEvent(previous, !_hasRunHealthChecksAfterGracePeriod));
-                    }
 
-                    _healthCheckResults.Remove(result.Source.Name);
+                foreach (var result in results)
-                }
-                else
                 {
-                    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)
+            var healthChecks = message.Trigger == CommandTrigger.Manual ? _healthChecks  : _scheduledHealthChecks;
-            {
+
-                PerformHealthCheck(_healthChecks, null, true);
+            lock (_pendingHealthChecks)
-            }
-            else
             {
-                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
+            lock (_pendingHealthChecks)
-            PerformHealthCheck(filteredChecks.ToArray(), message);
+            {
+                filteredChecks.ForEach(h => _pendingHealthChecks.Add(h));
+            }
+
+            _debounce.Execute();
         }
     }
 }

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);
-    }
 }

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 class ServerSideNotificationService : HealthCheckBase
-    {
-        public List<HealthCheck> GetServerChecks();
-    }
-
-    public class ServerSideNotificationService : IServerSideNotificationService
     {
         private readonly IHttpClient _client;
         private readonly IConfigFileProvider _configFileProvider;
-        private readonly IHttpRequestBuilderFactory _cloudRequestBuilder;
+        private readonly ILidarrCloudRequestBuilder _cloudRequestBuilder;
         private readonly Logger _logger;
 
-        private readonly ICached<List<HealthCheck>> _cache;
+        private readonly ICached<HealthCheck> _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<List<HealthCheck>>(GetType());
+            _cache = cacheManager.GetCache<HealthCheck>(GetType());
         }
 
-        public List<HealthCheck> GetServerChecks()
+        public override HealthCheck Check()
         {
-            return _cache.Get("ServerChecks", () => RetrieveServerChecks(), TimeSpan.FromHours(2));
+            return _cache.Get("ServerChecks", RetrieveServerChecks, TimeSpan.FromHours(2));
         }
 
-        private List<HealthCheck> RetrieveServerChecks()
+        private HealthCheck RetrieveServerChecks()
         {
-            var request = _cloudRequestBuilder.Create()
+            var request = _cloudRequestBuilder.Services.Create()
-                                      .Resource("/notification")
+                .Resource("/notification")
-                                      .AddQueryParam("version", BuildInfo.Version)
+                .AddQueryParam("version", BuildInfo.Version)
-                                      .AddQueryParam("os", OsInfo.Os.ToString().ToLowerInvariant())
+                .AddQueryParam("os", OsInfo.Os.ToString().ToLowerInvariant())
-                                      .AddQueryParam("arch", RuntimeInformation.OSArchitecture)
+                .AddQueryParam("arch", RuntimeInformation.OSArchitecture)
-                                      .AddQueryParam("runtime", "netcore")
+                .AddQueryParam("runtime", "netcore")
-                                      .AddQueryParam("branch", _configFileProvider.Branch)
+                .AddQueryParam("branch", _configFileProvider.Branch)
-                                      .Build();
+                .Build();
+
             try
             {
-                _logger.Trace("Getting server side health notifications");
+                _logger.Trace("Getting notifications");
+
                 var response = _client.Execute(request);
                 var result = Json.Deserialize<List<ServerNotificationResponse>>(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<HealthCheck>();
+
+                return new HealthCheck(GetType());
             }
         }
     }

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();
+            }
+        }
+    }
+}