fix: resolve 10 critical monitoring service issues

- Fix timer management race conditions with execution tracking
- Add thread-safe locking to MonitorState operations
- Implement graceful timer disposal with timeout protection
- Add atomic database transactions for state consistency
- Fix state initialization with automatic inconsistency correction
- Replace hardcoded monitoring gap detection with dynamic per-endpoint analysis
- Fix asymmetric state transition logic for mutual exclusivity
- Implement robust HTTP URL construction with protocol detection
- Add complete cancellation token support to all probe operations
- Fix shutdown cancellation logic with proper timeout handling

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

Author: hemanandr

ThingConnect.Pulse.Server/Services/Monitoring/MonitorState.cs

@@ -5,9 +5,11 @@ namespace ThingConnect.Pulse.Server.Services.Monitoring;
 /// <summary>
 /// Per-endpoint in-memory state for outage detection and flap damping.
 /// Tracks success/fail streaks and manages state transitions.
+/// Thread-safe with internal locking.
 /// </summary>
 public sealed class MonitorState
 {
+    private readonly object _lock = new object();
     /// <summary>
     /// The last publicly reported status (UP/DOWN). Null if never determined.
     /// </summary>
@@ -40,12 +42,19 @@ public sealed class MonitorState
     /// </summary>
     public bool ShouldTransitionToDown(int threshold = 2)
     {
-        // If never initialized, transition immediately on first failure
-        if (LastPublicStatus == null && FailStreak >= 1)
-            return true;
+        lock (_lock)
+        {
+            // Must have enough failures to trigger transition
+            if (FailStreak < Math.Max(1, threshold))
+                return false;
+                
+            // Handle null status (never initialized) - transition on first failure
+            if (LastPublicStatus == null)
+                return FailStreak >= 1;
 
-        // Otherwise require threshold for state change from UP to DOWN
-        return LastPublicStatus != UpDown.down && FailStreak >= threshold;
+            // Only transition if currently UP (not already DOWN)
+            return LastPublicStatus == UpDown.up;
+        }
     }
 
     /// <summary>
@@ -55,49 +64,96 @@ public bool ShouldTransitionToDown(int threshold = 2)
     /// </summary>
     public bool ShouldTransitionToUp(int threshold = 2)
     {
-        // If never initialized, transition immediately on first success
-        if (LastPublicStatus == null && SuccessStreak >= 1)
-            return true;
+        lock (_lock)
+        {
+            // Must have enough successes to trigger transition
+            if (SuccessStreak < Math.Max(1, threshold))
+                return false;
+                
+            // Handle null status (never initialized) - transition on first success
+            if (LastPublicStatus == null)
+                return SuccessStreak >= 1;
 
-        // Otherwise require threshold for state change from DOWN to UP
-        return LastPublicStatus != UpDown.up && SuccessStreak >= threshold;
+            // Only transition if currently DOWN (not already UP)
+            return LastPublicStatus == UpDown.down;
+        }
     }
 
     /// <summary>
     /// Records a successful check result and updates streaks.
     /// </summary>
     public void RecordSuccess()
     {
-        SuccessStreak++;
-        FailStreak = 0;
+        lock (_lock)
+        {
+            SuccessStreak++;
+            FailStreak = 0;
+        }
     }
 
     /// <summary>
     /// Records a failed check result and updates streaks.
     /// </summary>
     public void RecordFailure()
     {
-        FailStreak++;
-        SuccessStreak = 0;
+        lock (_lock)
+        {
+            FailStreak++;
+            SuccessStreak = 0;
+        }
     }
 
     /// <summary>
     /// Transitions the state to DOWN and records the change timestamp.
     /// </summary>
     public void TransitionToDown(long timestamp, long outageId)
     {
-        LastPublicStatus = UpDown.down;
-        LastChangeTs = timestamp;
-        OpenOutageId = outageId;
+        lock (_lock)
+        {
+            LastPublicStatus = UpDown.down;
+            LastChangeTs = timestamp;
+            OpenOutageId = outageId;
+        }
     }
 
     /// <summary>
     /// Transitions the state to UP and records the change timestamp.
     /// </summary>
     public void TransitionToUp(long timestamp)
     {
-        LastPublicStatus = UpDown.up;
-        LastChangeTs = timestamp;
-        OpenOutageId = null;
+        lock (_lock)
+        {
+            LastPublicStatus = UpDown.up;
+            LastChangeTs = timestamp;
+            OpenOutageId = null;
+        }
+    }
+
+    /// <summary>
+    /// Restores streak counters to previous values (used for rollback on transaction failures).
+    /// </summary>
+    public void RestoreStreakCounters(int successStreak, int failStreak)
+    {
+        lock (_lock)
+        {
+            SuccessStreak = successStreak;
+            FailStreak = failStreak;
+        }
+    }
+
+    /// <summary>
+    /// Validates that transition logic maintains mutual exclusivity.
+    /// This is used for debugging and ensuring state machine correctness.
+    /// </summary>
+    public bool ValidateTransitionMutualExclusivity(int threshold = 2)
+    {
+        lock (_lock)
+        {
+            bool shouldTransitionDown = ShouldTransitionToDown(threshold);
+            bool shouldTransitionUp = ShouldTransitionToUp(threshold);
+            
+            // Both transitions should never be true simultaneously
+            return !(shouldTransitionDown && shouldTransitionUp);
+        }
     }
 }

ThingConnect.Pulse.Server/Services/Monitoring/MonitoringBackgroundService.cs

@@ -13,6 +13,7 @@ public sealed class MonitoringBackgroundService : BackgroundService
     private readonly ILogger<MonitoringBackgroundService> _logger;
     private readonly SemaphoreSlim _concurrencySemaphore;
     private readonly ConcurrentDictionary<Guid, Timer> _endpointTimers = new();
+    private readonly ConcurrentDictionary<Guid, bool> _probeExecuting = new();
     private readonly int _maxConcurrentProbes;
 
     public MonitoringBackgroundService(IServiceProvider serviceProvider,
@@ -55,18 +56,67 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken)
             }
         }
 
-        // Handle graceful shutdown
-        using (IServiceScope scope = _serviceProvider.CreateScope())
+        // Handle graceful shutdown with cancellation support
+        try
         {
-            IOutageDetectionService outageService = scope.ServiceProvider.GetRequiredService<IOutageDetectionService>();
-            await outageService.HandleGracefulShutdownAsync("Service stopping", CancellationToken.None);
+            using (IServiceScope scope = _serviceProvider.CreateScope())
+            {
+                IOutageDetectionService outageService = scope.ServiceProvider.GetRequiredService<IOutageDetectionService>();
+                
+                // Create a timeout for graceful shutdown (30 seconds max)
+                // This allows force shutdown while giving reasonable time for data safety
+                using var shutdownCts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
+                using var combinedCts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken, shutdownCts.Token);
+                
+                await outageService.HandleGracefulShutdownAsync("Service stopping", combinedCts.Token);
+                
+                _logger.LogInformation("Graceful shutdown completed successfully");
+            }
+        }
+        catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
+        {
+            _logger.LogWarning("Graceful shutdown was cancelled - service forced to stop");
+        }
+        catch (OperationCanceledException)
+        {
+            _logger.LogWarning("Graceful shutdown timed out (30s) - proceeding with forced shutdown");
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Error during graceful shutdown - proceeding with forced shutdown");
         }
 
-        // Clean up timers
-        foreach (Timer timer in _endpointTimers.Values)
+        // Clean up timers gracefully with cancellation support
+        var timerCleanupTasks = new List<Task>();
+        foreach (var kvp in _endpointTimers.ToList()) // ToList to avoid modification during enumeration
         {
-            timer.Dispose();
+            timerCleanupTasks.Add(StopTimerGracefullyAsync(kvp.Value, kvp.Key));
+        }
+        
+        try
+        {
+            // Create timeout for timer cleanup (30 seconds max) while respecting external cancellation
+            using var timerCleanupCts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
+            using var combinedCleanupCts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken, timerCleanupCts.Token);
+            
+            // Wait for all timers to shutdown gracefully (with timeout and cancellation)
+            await Task.WhenAll(timerCleanupTasks).WaitAsync(combinedCleanupCts.Token);
+            
+            _logger.LogInformation("All timers stopped gracefully");
         }
+        catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
+        {
+            _logger.LogWarning("Timer cleanup was cancelled - service forced to stop");
+        }
+        catch (OperationCanceledException)
+        {
+            _logger.LogWarning("Timer cleanup timed out (30s) - some timers may not have stopped cleanly");
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Error during timer cleanup");
+        }
+        
         _endpointTimers.Clear();
 
         _logger.LogInformation("Monitoring background service stopped");
@@ -91,7 +141,8 @@ private async Task RefreshEndpointsAsync(CancellationToken cancellationToken)
         {
             if (_endpointTimers.TryRemove(endpointId, out Timer? timer))
             {
-                timer.Dispose();
+                await StopTimerGracefullyAsync(timer, endpointId);
+                _probeExecuting.TryRemove(endpointId, out _); // Clean up execution tracking
                 _logger.LogInformation("Stopped monitoring endpoint: {EndpointId}", endpointId);
             }
         }
@@ -103,7 +154,16 @@ private async Task RefreshEndpointsAsync(CancellationToken cancellationToken)
 
             if (_endpointTimers.TryGetValue(endpoint.Id, out Timer? existingTimer))
             {
-                // Update existing timer if interval changed
+                // Stop timer to prevent race condition, then restart with new interval
+                existingTimer.Change(Timeout.Infinite, Timeout.Infinite);
+                
+                // Wait briefly if probe is currently executing to avoid immediate restart
+                if (_probeExecuting.TryGetValue(endpoint.Id, out bool isExecuting) && isExecuting)
+                {
+                    await Task.Delay(100); // Brief delay to let current execution complete
+                }
+                
+                // Restart with new interval
                 existingTimer.Change(TimeSpan.Zero, TimeSpan.FromMilliseconds(intervalMs));
             }
             else
@@ -134,6 +194,9 @@ private async Task ProbeEndpointAsync(Guid endpointId)
             return;
         }
 
+        // Mark probe as executing to prevent timer race conditions
+        _probeExecuting.TryAdd(endpointId, true);
+
         try
         {
             using IServiceScope scope = _serviceProvider.CreateScope();
@@ -171,17 +234,66 @@ private async Task ProbeEndpointAsync(Guid endpointId)
         }
         finally
         {
+            // Clear execution flag
+            _probeExecuting.TryRemove(endpointId, out _);
             _concurrencySemaphore.Release();
         }
     }
 
+    /// <summary>
+    /// Gracefully stops a timer by disabling it, waiting for current execution to complete, then disposing.
+    /// </summary>
+    private async Task StopTimerGracefullyAsync(Timer timer, Guid endpointId)
+    {
+        try
+        {
+            // Stop the timer from firing again
+            timer.Change(Timeout.Infinite, Timeout.Infinite);
+
+            // Wait for current probe execution to complete (with timeout)
+            int maxWaitMs = 30000; // 30 seconds max wait
+            int waitedMs = 0;
+            const int checkIntervalMs = 100;
+
+            while (waitedMs < maxWaitMs && _probeExecuting.TryGetValue(endpointId, out bool isExecuting) && isExecuting)
+            {
+                await Task.Delay(checkIntervalMs);
+                waitedMs += checkIntervalMs;
+            }
+
+            if (waitedMs >= maxWaitMs)
+            {
+                _logger.LogWarning("Timeout waiting for probe execution to complete for endpoint: {EndpointId}", endpointId);
+            }
+
+            // Now safe to dispose
+            timer.Dispose();
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Error during graceful timer shutdown for endpoint: {EndpointId}", endpointId);
+            timer.Dispose(); // Force dispose on error
+        }
+    }
+
     public override void Dispose()
     {
         _concurrencySemaphore?.Dispose();
 
+        // Force dispose all remaining timers (should already be cleaned up in ExecuteAsync)
         foreach (Timer timer in _endpointTimers.Values)
         {
-            timer.Dispose();
+            try
+            {
+                // Stop timer first, then dispose (no graceful wait in synchronous dispose)
+                timer.Change(Timeout.Infinite, Timeout.Infinite);
+                timer.Dispose();
+            }
+            catch (Exception ex)
+            {
+                // Log but don't throw during disposal
+                _logger?.LogWarning(ex, "Error disposing timer during service disposal");
+            }
         }
 
         base.Dispose();