Merge dc/DC-088-remove-service-tombstones: removeService generation tombstones + incident closure [glm-grade=B]

dashcaddy-api/__tests__/health-checker.test.js

@@ -591,6 +591,113 @@ describe('HealthChecker', () => {
     });
   });
 
+  describe('DC-088: removeService generation tombstones + incident closure', () => {
+    it('does not leak a serviceGenerations entry and records a tombstone', () => {
+      healthChecker.configureService('svc1', { url: 'http://test.local' });
+      expect(healthChecker.serviceGenerations.has('svc1')).toBe(true);
+
+      healthChecker.removeService('svc1');
+
+      expect(healthChecker.serviceGenerations.has('svc1')).toBe(false);
+      const tomb = healthChecker.removedGenerations.get('svc1');
+      expect(tomb).toBeDefined();
+      expect(tomb.generation).toBeGreaterThan(0);
+      expect(tomb.removedAt).toBeGreaterThan(0);
+    });
+
+    it('re-added service gets a strictly higher generation (no ABA)', () => {
+      healthChecker.configureService('svc1', { url: 'http://test.local' });
+      const gen1 = healthChecker.serviceGenerations.get('svc1');
+
+      healthChecker.removeService('svc1');
+      healthChecker.configureService('svc1', { url: 'http://test.local/v2' });
+
+      const gen2 = healthChecker.serviceGenerations.get('svc1');
+      expect(gen2).toBeGreaterThan(gen1);
+      expect(healthChecker.removedGenerations.has('svc1')).toBe(false);
+    });
+
+    it('closes open incidents for the removed service as resolved', () => {
+      healthChecker.saveConfig = jest.fn();
+      healthChecker.incidents.push({
+        id: 'incident-test-1',
+        serviceId: 'svc1',
+        type: 'outage',
+        status: 'open',
+        createdAt: new Date(Date.now() - 60_000).toISOString()
+      });
+      healthChecker.incidents.push({
+        id: 'incident-other',
+        serviceId: 'svc2',
+        type: 'outage',
+        status: 'open',
+        createdAt: new Date(Date.now() - 60_000).toISOString()
+      });
+      const resolvedSpy = jest.fn();
+      healthChecker.on('incident-resolved', resolvedSpy);
+
+      healthChecker.removeService('svc1');
+
+      const closed = healthChecker.incidents.find(i => i.id === 'incident-test-1');
+      expect(closed.status).toBe('resolved');
+      expect(closed.resolvedBy).toBe('service-removed');
+      expect(closed.resolvedAt).toBeDefined();
+      expect(closed.duration).toBeGreaterThan(0);
+      expect(healthChecker.incidents.find(i => i.id === 'incident-other').status).toBe('open');
+      expect(resolvedSpy).toHaveBeenCalledTimes(1);
+    });
+
+    it('in-flight probe captured before removal is discarded via tombstone', async () => {
+      let resolveProbe;
+      healthChecker.config.services.svc1 = { url: 'http://test.local' };
+      healthChecker._doRequest = jest.fn(() => new Promise(resolve => {
+        resolveProbe = resolve;
+      }));
+
+      const pending = healthChecker.checkService('svc1', healthChecker.config.services.svc1);
+      healthChecker.saveConfig = jest.fn();
+      healthChecker.removeService('svc1');
+      resolveProbe({ healthy: true, statusCode: 200, message: 'late', details: {} });
+      await pending;
+
+      expect(healthChecker.currentStatus.has('svc1')).toBe(false);
+      expect(healthChecker.consecutiveFailures.has('svc1')).toBe(false);
+    });
+
+    it('a rejected in-flight probe after removal does not re-create failure state', async () => {
+      let rejectProbe;
+      healthChecker.config.services.svc1 = { url: 'http://test.local' };
+      healthChecker._doRequest = jest.fn(() => new Promise((resolve, reject) => {
+        rejectProbe = reject;
+      }));
+
+      const pending = healthChecker.checkService('svc1', healthChecker.config.services.svc1);
+      healthChecker.saveConfig = jest.fn();
+      healthChecker.removeService('svc1');
+      rejectProbe(new Error('late failure'));
+      await pending;
+
+      expect(healthChecker.consecutiveFailures.has('svc1')).toBe(false);
+      expect(healthChecker.currentStatus.has('svc1')).toBe(false);
+    });
+
+    it('sweeps expired tombstones in cleanupHistory', () => {
+      healthChecker.removedGenerations.set('svc1', {
+        generation: 1,
+        removedAt: Date.now() - 60 * 60 * 1000 // 1h ago, TTL default 10m
+      });
+      healthChecker.removedGenerations.set('svc2', {
+        generation: 2,
+        removedAt: Date.now() // fresh
+      });
+
+      healthChecker.cleanupHistory();
+
+      expect(healthChecker.removedGenerations.has('svc1')).toBe(false);
+      expect(healthChecker.removedGenerations.has('svc2')).toBe(true);
+    });
+  });
+
   describe('cleanupHistory', () => {
     it('removes entries older than retention period', () => {
       const old = new Date(Date.now() - 35 * 24 * 60 * 60 * 1000).toISOString(); // 35 days ago

dashcaddy-api/src/monitoring/health-checker.js

@@ -32,6 +32,10 @@ const CHECK_INTERVAL = parseInt(process.env.HEALTH_CHECK_INTERVAL || '30000', 10
 const MAX_CHECK_INTERVAL = parseInt(process.env.HEALTH_CHECK_MAX_INTERVAL || '300000', 10); // 5 minutes max backoff
 const MAX_ENTRIES_PER_SERVICE = parseInt(process.env.HEALTH_MAX_ENTRIES || '500', 10); // Cap to prevent disk explosion
 const HISTORY_RETENTION_DAYS = parseInt(process.env.HEALTH_HISTORY_RETENTION || '30', 10);
+// DC-088: how long a removal tombstone outlives the removal itself. Only needs
+// to cover the max in-flight probe lifetime (timeout + scheduling headroom);
+// swept by cleanupHistory so removed services cannot accumulate map entries.
+const REMOVED_GENERATION_TTL_MS = parseInt(process.env.HEALTH_REMOVED_GEN_TTL || '600000', 10);
 
 // DC-086: hysteresis thresholds for badge display.
 // The raw probe result can flap on a single transient blip (Caddy reload,
@@ -72,6 +76,14 @@ class HealthChecker extends EventEmitter {
     this.serviceTimers = new Map(); // serviceId -> timer for per-service backoff
     // Invalidate probe completions that race with removal/reconfiguration.
     this.serviceGenerations = new Map(); // serviceId -> configuration generation
+    // DC-088: monotonically increasing sequence so generation numbers can never
+    // repeat across remove -> re-add cycles (prevents ABA on the stale check).
+    this.generationSeq = 0;
+    // DC-088: serviceId -> { generation, removedAt } tombstones. A live entry in
+    // serviceGenerations means the service is (re)configured; a tombstone with a
+    // HIGHER generation than the captured one marks the capture as stale. Entry
+    // is deleted when the service is removed, so the live map cannot leak.
+    this.removedGenerations = new Map();
   }
 
   /**
@@ -139,6 +151,21 @@ class HealthChecker extends EventEmitter {
     this.cleanupHistory();
   }
 
+  /**
+   * DC-088: true when a probe's captured generation no longer matches the
+   * service's current configuration state. A live serviceGenerations entry
+   * must match exactly. With no live entry the service was never configured
+   * in this process (disk-loaded / direct callers) — stale only if a removal
+   * tombstone with a HIGHER generation exists.
+   */
+  _isStaleCapture(serviceId, generation) {
+    if (this.serviceGenerations.has(serviceId)) {
+      return this.serviceGenerations.get(serviceId) !== generation;
+    }
+    const tomb = this.removedGenerations.get(serviceId);
+    return Boolean(tomb && tomb.generation > generation);
+  }
+
   /**
    * Check a single service
    */
@@ -160,7 +187,7 @@ class HealthChecker extends EventEmitter {
         details: result.details
       };
 
-      if ((this.serviceGenerations.get(serviceId) || 0) !== generation) {
+      if (this._isStaleCapture(serviceId, generation)) {
         return status;
       }
 
@@ -179,9 +206,6 @@ class HealthChecker extends EventEmitter {
     } catch (error) {
       const responseTime = Date.now() - startTime;
 
-      // Increment failure count for backoff
-      this.consecutiveFailures.set(serviceId, (this.consecutiveFailures.get(serviceId) || 0) + 1);
-
       const status = {
         serviceId,
         timestamp: new Date().toISOString(),
@@ -190,10 +214,14 @@ class HealthChecker extends EventEmitter {
         error: error.message
       };
 
-      if ((this.serviceGenerations.get(serviceId) || 0) !== generation) {
+      if (this._isStaleCapture(serviceId, generation)) {
         return status;
       }
 
+      // Increment failure count for backoff — only after the result is known
+      // to be non-stale, so a removed service cannot re-create map entries.
+      this.consecutiveFailures.set(serviceId, (this.consecutiveFailures.get(serviceId) || 0) + 1);
+
       const previousStatus = this.currentStatus.get(serviceId);
       this.recordStatus(serviceId, status);
       this.checkForIncidents(serviceId, status, config, previousStatus);
@@ -660,7 +688,13 @@ class HealthChecker extends EventEmitter {
       this.config.services = {};
     }
 
-    this.serviceGenerations.set(serviceId, (this.serviceGenerations.get(serviceId) || 0) + 1);
+    // DC-088: monotonic instance-wide sequence — a re-added service can never
+    // recycle a previous generation number, and any older in-flight capture is
+    // invalidated by definition.
+    this.generationSeq += 1;
+    this.serviceGenerations.set(serviceId, this.generationSeq);
+    // Re-configuration supersedes any prior removal tombstone.
+    this.removedGenerations.delete(serviceId);
     this.config.services[serviceId] = {
       enabled: config.enabled !== false,
       name: config.name || serviceId,
@@ -683,12 +717,35 @@ class HealthChecker extends EventEmitter {
    * Remove service configuration
    */
   removeService(serviceId) {
-    this.serviceGenerations.set(serviceId, (this.serviceGenerations.get(serviceId) || 0) + 1);
+    // DC-088: tombstone the captured generation instead of leaking an entry.
+    // The live map entry is deleted; an in-flight probe captured BEFORE this
+    // point sees no live entry but a higher tombstone generation, so it is
+    // discarded. configureService clears the tombstone on re-add.
+    this.generationSeq += 1;
+    this.serviceGenerations.delete(serviceId);
+    this.removedGenerations.set(serviceId, {
+      generation: this.generationSeq,
+      removedAt: Date.now()
+    });
     if (this.config.services) {
       delete this.config.services[serviceId];
       this.saveConfig();
     }
 
+    // DC-088: open incidents for a removed service must not linger forever.
+    // Close them through the same resolve path a recovery would, annotated so
+    // history shows why (dashboard renders resolved incidents green + duration).
+    for (const incident of this.incidents) {
+      if (incident.serviceId === serviceId && incident.status === 'open') {
+        incident.status = 'resolved';
+        incident.resolvedAt = new Date().toISOString();
+        incident.duration = new Date(incident.resolvedAt) - new Date(incident.createdAt);
+        incident.resolvedBy = 'service-removed';
+        this.emit('incident-resolved', incident);
+        this.emit('log', 'info', `Incident closed by service removal: ${incident.id}`);
+      }
+    }
+
     this.currentStatus.delete(serviceId);
     this.displayedStatus.delete(serviceId);
     this.consecutiveSinceChange.delete(serviceId);
@@ -714,6 +771,18 @@ class HealthChecker extends EventEmitter {
         this.history[serviceId] = this.history[serviceId].slice(-MAX_ENTRIES_PER_SERVICE);
       }
     }
+
+    // DC-088: sweep expired removal tombstones. After the TTL no probe that
+    // captured a pre-removal generation can still be in flight (timeout is
+    // bounded by performHealthCheck), so the tombstone has done its job.
+    if (this.removedGenerations.size > 0) {
+      const now = Date.now();
+      for (const [serviceId, tomb] of this.removedGenerations) {
+        if (now - tomb.removedAt > REMOVED_GENERATION_TTL_MS) {
+          this.removedGenerations.delete(serviceId);
+        }
+      }
+    }
   }
 
   /**