From f8b99f9b5a14ffab4b329f25dc3ea428227d006f Mon Sep 17 00:00:00 2001 From: Hermes Date: Thu, 20 Aug 2026 04:46:17 -0700 Subject: [PATCH] =?UTF-8?q?DC-086=20service-status=20flicker=20fix=20?= =?UTF-8?q?=E2=80=94=20asymmetric=20hysteresis?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dashboard badges perpetually flip green/red for a few seconds at a time, never stable. Root cause: health-checker emitted 'status-check' on every probe (every 30s) and dashboard-ws forwarded every one as 'status-change' to the browser with no diff; live-events.js then unconditionally called setBadge(). A single transient 5xx (Caddy reload, container restart, TLS handshake blip) flipped the badge and the next green probe flipped back. Fix: _computeDisplayedStatus applies asymmetric hysteresis — DOWN_THRESHOLD (default 2, env-tunable HEALTH_DOWN_THRESHOLD) consecutive probes that disagree with the displayed 'up' state flip to red; UP_THRESHOLD (default 1, HEALTH_UP_THRESHOLD) flips back to green. History and consecutiveFailures still record every raw probe so postmortem analysis is unchanged. Only the SSE broadcast is filtered. getCurrentStatus now returns the displayed status so a page reload shows the same badge as the live stream. 10 new tests cover first-emit, same-status-dedup, the actual flicker bug (one-down-then-up keeps green), two-down flips red, one-up recovers fast, long-steady-green produces exactly one emit, and env-var tuning. All 63 existing health-checker tests still pass. Full suite: 2484/2484. --- .../health-checker-hysteresis.test.js | 170 ++++++++++++++++++ .../src/monitoring/health-checker.js | 130 ++++++++++++-- 2 files changed, 289 insertions(+), 11 deletions(-) create mode 100644 dashcaddy-api/__tests__/health-checker-hysteresis.test.js diff --git a/dashcaddy-api/__tests__/health-checker-hysteresis.test.js b/dashcaddy-api/__tests__/health-checker-hysteresis.test.js new file mode 100644 index 0000000..49dd2ed --- /dev/null +++ b/dashcaddy-api/__tests__/health-checker-hysteresis.test.js @@ -0,0 +1,170 @@ +/** + * Tests for DC-086: asymmetric hysteresis on the dashboard service badge. + * + * - First probe always emits (no prior state). + * - Same-status probe does NOT re-emit (dedup against repeated green). + * - One "down" then back to "up" keeps the badge green (no flicker). + * - Two consecutive "down" probes flip the badge to red. + * - One "up" after a down streak flips back to green (fast recovery). + * - History retains every raw probe even when no emit happens. + * - getCurrentStatus returns displayed status, not raw. + */ + +'use strict'; + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +// Use an isolated data dir so test history doesn't pollute the real one. +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dashcaddy-hyst-')); +process.env.HEALTH_DATA_DIR = tmpDir; +process.env.HEALTH_CONFIG_FILE = path.join(tmpDir, 'health-config.json'); +process.env.HEALTH_HISTORY_FILE = path.join(tmpDir, 'health-history.json'); + +const { HealthChecker } = require('../src/monitoring/health-checker'); +// Module exports a singleton instance, not a class — see module.exports in +// src/monitoring/health-checker.js. The test creates fresh state by replacing +// the relevant maps on the singleton in beforeEach. +const healthCheckerSingleton = require('../src/monitoring/health-checker'); + +function makeUp(serviceId = 'svc1') { + return { + serviceId, + timestamp: new Date().toISOString(), + status: 'up', + responseTime: 50, + statusCode: 200, + message: 'Service is healthy', + details: { headers: {}, bodyLength: 12 } + }; +} + +function makeDown(serviceId = 'svc1') { + return { + serviceId, + timestamp: new Date().toISOString(), + status: 'down', + responseTime: 50, + statusCode: 500, + message: 'fail', + details: { headers: {}, bodyLength: 0 } + }; +} + +describe('DC-086: hysteresis on the dashboard badge', () => { + let hc; + let emitSpy; + + beforeEach(() => { + // Reset the singleton's per-test state so each case starts clean. + healthCheckerSingleton.displayedStatus = new Map(); + healthCheckerSingleton.consecutiveSinceChange = new Map(); + healthCheckerSingleton.currentStatus = new Map(); + healthCheckerSingleton.history = {}; + healthCheckerSingleton.removeAllListeners('status-check'); + emitSpy = jest.fn(); + healthCheckerSingleton.on('status-check', emitSpy); + hc = healthCheckerSingleton; + }); + + test('first probe (no prior state) emits', () => { + hc.recordStatus('svc1', makeUp()); + expect(emitSpy).toHaveBeenCalledTimes(1); + expect(emitSpy.mock.calls[0][0].status).toBe('up'); + }); + + test('second probe with same status does NOT re-emit', () => { + hc.recordStatus('svc1', makeUp()); + hc.recordStatus('svc1', makeUp()); + expect(emitSpy).toHaveBeenCalledTimes(1); + }); + + test('one "down" then "up" keeps the badge green (the flicker bug)', () => { + hc.recordStatus('svc1', makeUp()); // baseline: green, emit 1 + hc.recordStatus('svc1', makeDown()); // one blip — keep green, no emit + hc.recordStatus('svc1', makeUp()); // recovered — still green, no emit + expect(emitSpy).toHaveBeenCalledTimes(1); + expect(hc.displayedStatus.get('svc1').status).toBe('up'); + }); + + test('two consecutive "down" probes flip the badge to red', () => { + hc.recordStatus('svc1', makeUp()); // baseline: green + hc.recordStatus('svc1', makeDown()); // blip #1 — keep green (counter=1) + hc.recordStatus('svc1', makeDown()); // blip #2 — flip red (counter=2 >= DOWN_THRESHOLD) + expect(emitSpy).toHaveBeenCalledTimes(2); + expect(emitSpy.mock.calls[1][0].status).toBe('down'); + expect(hc.displayedStatus.get('svc1').status).toBe('down'); + }); + + test('one "up" after a down streak flips back to green (fast recovery)', () => { + hc.recordStatus('svc1', makeUp()); + hc.recordStatus('svc1', makeDown()); + hc.recordStatus('svc1', makeDown()); // now red + expect(hc.displayedStatus.get('svc1').status).toBe('down'); + + hc.recordStatus('svc1', makeUp()); // first green — flip back + expect(emitSpy).toHaveBeenCalledTimes(3); + expect(emitSpy.mock.calls[2][0].status).toBe('up'); + expect(hc.displayedStatus.get('svc1').status).toBe('up'); + }); + + test('history retains every raw probe even when no emit happens', () => { + hc.recordStatus('svc1', makeUp()); + hc.recordStatus('svc1', makeDown()); // blip, no emit + hc.recordStatus('svc1', makeUp()); // recovery, no emit + expect(hc.history['svc1'].length).toBe(3); + expect(hc.history['svc1'][0].status).toBe('up'); + expect(hc.history['svc1'][1].status).toBe('down'); + expect(hc.history['svc1'][2].status).toBe('up'); + }); + + test('getCurrentStatus returns the displayed status, not the raw probe', () => { + hc.recordStatus('svc1', makeUp()); + hc.recordStatus('svc1', makeDown()); // raw=down, displayed=up + const out = hc.getCurrentStatus(); + expect(out['svc1'].status).toBe('up'); // shown to API consumers + }); + + test('a long steady-green run produces exactly ONE emit (no per-probe spam)', () => { + for (let i = 0; i < 50; i++) hc.recordStatus('svc1', makeUp()); + expect(emitSpy).toHaveBeenCalledTimes(1); + }); + + test('a long steady-green-then-steady-red transition: 1 emit (up), 1 emit (red)', () => { + for (let i = 0; i < 10; i++) hc.recordStatus('svc1', makeUp()); + expect(emitSpy).toHaveBeenCalledTimes(1); + hc.recordStatus('svc1', makeDown()); + hc.recordStatus('svc1', makeDown()); // flips to red + expect(emitSpy).toHaveBeenCalledTimes(2); + for (let i = 0; i < 10; i++) hc.recordStatus('svc1', makeDown()); + expect(emitSpy).toHaveBeenCalledTimes(2); // no further broadcasts + }); + + test('DOWN_THRESHOLD env var is honored', () => { + const origDown = process.env.HEALTH_DOWN_THRESHOLD; + process.env.HEALTH_DOWN_THRESHOLD = '3'; + jest.resetModules(); + const HC2Module = require('../src/monitoring/health-checker'); + // Module is a singleton with DOWN_THRESHOLD captured at module load — + // resetModules gives us a fresh module-level instance with the new env. + const hc2 = HC2Module; + hc2.displayedStatus = new Map(); + hc2.consecutiveSinceChange = new Map(); + hc2.currentStatus = new Map(); + hc2.history = {}; + hc2.removeAllListeners('status-check'); + const spy = jest.fn(); + hc2.on('status-check', spy); + hc2.recordStatus('svc1', makeUp()); + hc2.recordStatus('svc1', makeDown()); // 1 + hc2.recordStatus('svc1', makeDown()); // 2 — still green (need 3) + expect(spy).toHaveBeenCalledTimes(1); + expect(hc2.displayedStatus.get('svc1').status).toBe('up'); + hc2.recordStatus('svc1', makeDown()); // 3 — flip + expect(spy).toHaveBeenCalledTimes(2); + expect(hc2.displayedStatus.get('svc1').status).toBe('down'); + if (origDown === undefined) delete process.env.HEALTH_DOWN_THRESHOLD; + else process.env.HEALTH_DOWN_THRESHOLD = origDown; + }); +}); \ No newline at end of file diff --git a/dashcaddy-api/src/monitoring/health-checker.js b/dashcaddy-api/src/monitoring/health-checker.js index 35e5819..44ec2f7 100644 --- a/dashcaddy-api/src/monitoring/health-checker.js +++ b/dashcaddy-api/src/monitoring/health-checker.js @@ -33,12 +33,31 @@ const MAX_CHECK_INTERVAL = parseInt(process.env.HEALTH_CHECK_MAX_INTERVAL || '30 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-086: hysteresis thresholds for badge display. +// The raw probe result can flap on a single transient blip (Caddy reload, +// container CPU steal, network hiccup, mid-flight TLS handshake). Showing +// every probe result as-is to the dashboard creates the "perpetual flicker" +// UX. Asymmetric thresholds: going red is slow (don't false-alarm), going +// green is fast (don't keep showing red after recovery). +// - DOWN_THRESHOLD = N consecutive "down" probes before the badge flips to red +// - UP_THRESHOLD = N consecutive "up" probes before the badge flips back to green +// Single probe flips to green on purpose — false-positive-green is much less +// painful than perpetual-red (operators notice red, ignore green). +const DOWN_THRESHOLD = Math.max(1, parseInt(process.env.HEALTH_DOWN_THRESHOLD || '2', 10)); +const UP_THRESHOLD = Math.max(1, parseInt(process.env.HEALTH_UP_THRESHOLD || '1', 10)); + class HealthChecker extends EventEmitter { constructor() { super(); this.config = this.loadConfig(); this.history = this.loadHistory(); this.currentStatus = new Map(); + // DC-086: the status the dashboard SHOULD display (post-hysteresis). + // Distinct from currentStatus, which is the latest raw probe result. + this.displayedStatus = new Map(); + // DC-086: counter of consecutive healthy/unhealthy probes since the + // last displayed-status change. Reset to 0 whenever displayed status flips. + this.consecutiveSinceChange = new Map(); this.incidents = []; this.checking = false; this.checkInterval = null; @@ -273,27 +292,106 @@ class HealthChecker extends EventEmitter { return true; } + /** + * Compute the displayed status for a service given the latest raw probe + * result. Applies asymmetric hysteresis: + * - Going DOWN: requires DOWN_THRESHOLD (default 2) consecutive "down" + * probes since the last display-state change. A single blip keeps the + * badge green. + * - Going UP: requires UP_THRESHOLD (default 1) consecutive "up" probes. + * Any single "up" after a down streak flips back to green so the badge + * doesn't linger red after the service has recovered. + * + * Returns the displayed status object (same shape as the raw status) so + * recordStatus can use it both for the displayed map and as the broadcast + * payload when the displayed status actually changes. + */ + _computeDisplayedStatus(serviceId, rawStatus) { + const currentDisplayed = this.displayedStatus.get(serviceId); + const previousStatus = currentDisplayed ? currentDisplayed.status : null; + + // If no prior state, accept the raw probe as-is (first-check bootstrap). + if (!previousStatus) { + return rawStatus; + } + + // Probe agrees with current displayed → no change, reset the counter so + // a brief blip doesn't accumulate against the displayed state. + if (rawStatus.status === previousStatus) { + this.consecutiveSinceChange.set(serviceId, 0); + return currentDisplayed; + } + + // Probe disagrees with displayed. Bump the streak counter — this counts + // CONSECUTIVE probes that disagree with what's shown, regardless of + // whether the raw value itself changed between probes. That's what + // makes "down, down" flip after threshold but "down, up, down" not flip. + const prev = this.consecutiveSinceChange.get(serviceId) || 0; + const next = prev + 1; + + if (rawStatus.status === 'down') { + // Going DOWN: need DOWN_THRESHOLD consecutive probes that disagree + // with the displayed "up" state. + if (previousStatus === 'up' && next < DOWN_THRESHOLD) { + this.consecutiveSinceChange.set(serviceId, next); + return currentDisplayed; + } + // Threshold met (or already down) — flip to red. + this.consecutiveSinceChange.set(serviceId, 0); + return rawStatus; + } + + // rawStatus.status === 'up' (must be — the equal-to-displayed case above + // already returned). Going UP after a down streak: need UP_THRESHOLD. + if (previousStatus === 'down' && next < UP_THRESHOLD) { + this.consecutiveSinceChange.set(serviceId, next); + return currentDisplayed; + } + this.consecutiveSinceChange.set(serviceId, 0); + return rawStatus; + } + /** * Record service status + * + * DC-086: history + consecutiveFailures are updated for EVERY probe + * (operators want full probe history for postmortems). The dashboard's + * `status-check` event is only emitted when the DISPLAYED status changes, + * so the badge stops re-rendering on every probe. */ recordStatus(serviceId, status) { - // Update current status + // Update current (raw) status — used by checkForIncidents and history. this.currentStatus.set(serviceId, status); - // Add to history + // Add raw probe to history (full fidelity — operators rely on this). if (!this.history[serviceId]) { this.history[serviceId] = []; } this.history[serviceId].push(status); - + // Cap entries to prevent unbounded growth (disk explosion fix) if (this.history[serviceId].length > MAX_ENTRIES_PER_SERVICE) { this.history[serviceId] = this.history[serviceId].slice(-MAX_ENTRIES_PER_SERVICE); } - // Emit status event - this.emit('status-check', status); + // Compute the post-hysteresis displayed status; only emit when it changes. + // _computeDisplayedStatus compares the raw probe against the DISPLAYED + // status (not the previous raw status), so the "consecutive since + // change" counter doesn't depend on the order of writes here. + const displayed = this._computeDisplayedStatus(serviceId, status); + const previousDisplayed = this.displayedStatus.get(serviceId); + const displayChanged = + !previousDisplayed || previousDisplayed.status !== displayed.status; + + this.displayedStatus.set(serviceId, displayed); + + if (displayChanged) { + // Emit with the displayed status so the dashboard renders the same + // state the hysteresis just decided. The raw probe result is still + // in `history` and `currentStatus` for anyone who wants it. + this.emit('status-check', displayed); + } // Save history periodically if (Math.random() < 0.05) { // 5% chance (every ~20 checks) @@ -445,19 +543,29 @@ class HealthChecker extends EventEmitter { } /** - * Get current status for all services + * Get current status for all services. + * + * DC-086: returns the DISPLAYED status (post-hysteresis), not the latest + * raw probe. A page reload should show the same badge state the live + * SSE stream is currently showing — otherwise an operator who reloads + * the page after a single blip sees red even though the hysteresis kept + * the badge green for them. */ getCurrentStatus() { const result = {}; - - for (const [serviceId, status] of this.currentStatus.entries()) { + + for (const [serviceId, rawStatus] of this.currentStatus.entries()) { const config = this.config.services[serviceId]; const uptime24h = this.calculateUptime(serviceId, 24); const uptime7d = this.calculateUptime(serviceId, 168); const avgResponseTime = this.calculateAverageResponseTime(serviceId, 24); - + + // Prefer the displayed status if we've already computed one; fall back + // to the raw probe on the very first call (before recordStatus has run). + const displayed = this.displayedStatus.get(serviceId) || rawStatus; + result[serviceId] = { - ...status, + ...displayed, name: config?.name || serviceId, uptime: { '24h': uptime24h, @@ -467,7 +575,7 @@ class HealthChecker extends EventEmitter { sla: config?.sla }; } - + return result; }