From 3ccf66754aa1a5cb6b6544c9754e433e52db9799 Mon Sep 17 00:00:00 2001 From: Hermes Date: Sat, 22 Aug 2026 15:55:26 -0700 Subject: [PATCH] [glm-grade=B] fix(monitoring): DC-088 removeService generation tombstones + incident closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - serviceGenerations no longer leaks entries: removeService deletes the live entry and records a TTL'd (10min) tombstone swept by cleanupHistory - monotonic instance-wide generationSeq prevents generation reuse across remove->re-add cycles (ABA) and supersedes tombstones on re-configure - _isStaleCapture(): presence-aware stale check — live entry must match exactly; no entry is stale only under a higher-generation tombstone (preserves correct behavior for disk-loaded never-configured services) - catch path increments consecutiveFailures only after the stale check, so a late-rejected probe cannot resurrect state for a removed service - open incidents for a removed service close via the standard resolve path (resolvedBy=service-removed, WS/SSE incident-resolved broadcast) - 6 regression tests; full suite 2608/2608 green Judge: GLM-5.3 cold read (Codex stand-in), verdict B/ship, zero blockers --- .../__tests__/health-checker.test.js | 107 ++++++++++++++++++ .../src/monitoring/health-checker.js | 83 ++++++++++++-- 2 files changed, 183 insertions(+), 7 deletions(-) diff --git a/dashcaddy-api/__tests__/health-checker.test.js b/dashcaddy-api/__tests__/health-checker.test.js index 64b6ca1..4fdee6a 100644 --- a/dashcaddy-api/__tests__/health-checker.test.js +++ 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 diff --git a/dashcaddy-api/src/monitoring/health-checker.js b/dashcaddy-api/src/monitoring/health-checker.js index 572d342..e1c7eca 100644 --- a/dashcaddy-api/src/monitoring/health-checker.js +++ b/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); + } + } + } } /**