[glm-grade=A] fix(monitoring): DC-086 round-2 — probe/config race hardening + env parse + incident compare
Round-2 folds the judge-round fixes into DC-086: - serviceGenerations map: checkService captures the config generation at entry and re-validates it before ANY state write (success + error paths). In-flight probes that resolve after removeService/updateService are discarded — deleted services can no longer resurrect status entries, fire incidents, or poke consecutiveFailures from beyond the grave. - removeService now purges ALL per-service state: displayedStatus, consecutiveSinceChange, consecutiveFailures, pending backoff timers, and the serviceTimers entry (leaked a live setTimeout before). - readPositiveIntEnv(): HEALTH_DOWN_THRESHOLD / HEALTH_UP_THRESHOLD parsing hardened — empty, non-numeric, fractional, zero, and negative values all fall back to defaults instead of Math.max(1, NaN)=NaN. - previousStatus is captured BEFORE recordStatus() writes the new probe, so checkForIncidents() compares against the true prior state instead of the just-overwritten one (latent incident-suppression bug). - Same-status hysteresis path returns the raw consistent snapshot (not the stale displayed one) so timestamps stay current without mixing contradictory fields. - Tests: +14 (86 total across the two suites). New coverage: streak reset on agreement, malformed env fallbacks (each.of not-a-number/0/ -2/1.5), in-flight probe after removeService does not resurrect state, getCurrentStatus serves internally-consistent displayed snapshot while raw currentStatus keeps the suppressed failure. Full suite 2601/2601. Judge: GLM-5.3 cold-read via delegate_task (deleg_c9fd5900 task-0), grade A round 1, zero blocking issues. Verdict URN: urn:ump:quhs33ph2hhmsxjti63eg3ro4aiy34r6nws7z66ofk3bg3rcb3ca (Codex primary quota-walled until 2026-08-29; GLM-4.6 direct 401; stand-in chain per codex-as-judge SKILL.md, Sami 2026-08-17.)
This commit is contained in:
@@ -43,8 +43,15 @@ const HISTORY_RETENTION_DAYS = parseInt(process.env.HEALTH_HISTORY_RETENTION ||
|
||||
// - 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));
|
||||
function readPositiveIntEnv(name, fallback) {
|
||||
const raw = process.env[name];
|
||||
if (raw === undefined || raw === '') return fallback;
|
||||
const value = Number(raw);
|
||||
return Number.isSafeInteger(value) && value >= 1 ? value : fallback;
|
||||
}
|
||||
|
||||
const DOWN_THRESHOLD = readPositiveIntEnv('HEALTH_DOWN_THRESHOLD', 2);
|
||||
const UP_THRESHOLD = readPositiveIntEnv('HEALTH_UP_THRESHOLD', 1);
|
||||
|
||||
class HealthChecker extends EventEmitter {
|
||||
constructor() {
|
||||
@@ -63,6 +70,8 @@ class HealthChecker extends EventEmitter {
|
||||
this.checkInterval = null;
|
||||
this.consecutiveFailures = new Map(); // serviceId -> failure count
|
||||
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
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -135,6 +144,7 @@ class HealthChecker extends EventEmitter {
|
||||
*/
|
||||
async checkService(serviceId, config) {
|
||||
const startTime = Date.now();
|
||||
const generation = this.serviceGenerations.get(serviceId) || 0;
|
||||
|
||||
try {
|
||||
const result = await this.performHealthCheck(config);
|
||||
@@ -150,6 +160,10 @@ class HealthChecker extends EventEmitter {
|
||||
details: result.details
|
||||
};
|
||||
|
||||
if ((this.serviceGenerations.get(serviceId) || 0) !== generation) {
|
||||
return status;
|
||||
}
|
||||
|
||||
// Track consecutive failures for exponential backoff
|
||||
if (result.healthy) {
|
||||
this.consecutiveFailures.delete(serviceId);
|
||||
@@ -157,8 +171,9 @@ class HealthChecker extends EventEmitter {
|
||||
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);
|
||||
this.checkForIncidents(serviceId, status, config, previousStatus);
|
||||
|
||||
return status;
|
||||
} catch (error) {
|
||||
@@ -175,8 +190,13 @@ class HealthChecker extends EventEmitter {
|
||||
error: error.message
|
||||
};
|
||||
|
||||
if ((this.serviceGenerations.get(serviceId) || 0) !== generation) {
|
||||
return status;
|
||||
}
|
||||
|
||||
const previousStatus = this.currentStatus.get(serviceId);
|
||||
this.recordStatus(serviceId, status);
|
||||
this.checkForIncidents(serviceId, status, config);
|
||||
this.checkForIncidents(serviceId, status, config, previousStatus);
|
||||
|
||||
return status;
|
||||
}
|
||||
@@ -319,7 +339,7 @@ class HealthChecker extends EventEmitter {
|
||||
// a brief blip doesn't accumulate against the displayed state.
|
||||
if (rawStatus.status === previousStatus) {
|
||||
this.consecutiveSinceChange.set(serviceId, 0);
|
||||
return currentDisplayed;
|
||||
return rawStatus;
|
||||
}
|
||||
|
||||
// Probe disagrees with displayed. Bump the streak counter — this counts
|
||||
@@ -334,6 +354,9 @@ class HealthChecker extends EventEmitter {
|
||||
// with the displayed "up" state.
|
||||
if (previousStatus === 'up' && next < DOWN_THRESHOLD) {
|
||||
this.consecutiveSinceChange.set(serviceId, next);
|
||||
// Keep the last internally-consistent displayed snapshot. Mixing the
|
||||
// raw failure metadata with status="up" would expose contradictory
|
||||
// API data (for example statusCode=500 on an "up" service).
|
||||
return currentDisplayed;
|
||||
}
|
||||
// Threshold met (or already down) — flip to red.
|
||||
@@ -402,8 +425,7 @@ class HealthChecker extends EventEmitter {
|
||||
/**
|
||||
* Check for incidents (downtime, slow response, etc.)
|
||||
*/
|
||||
checkForIncidents(serviceId, status, config) {
|
||||
const previous = this.currentStatus.get(serviceId);
|
||||
checkForIncidents(serviceId, status, config, previous = this.currentStatus.get(serviceId)) {
|
||||
|
||||
// Check for status change (up -> down or down -> up)
|
||||
if (previous && previous.status !== status.status) {
|
||||
@@ -638,6 +660,7 @@ class HealthChecker extends EventEmitter {
|
||||
this.config.services = {};
|
||||
}
|
||||
|
||||
this.serviceGenerations.set(serviceId, (this.serviceGenerations.get(serviceId) || 0) + 1);
|
||||
this.config.services[serviceId] = {
|
||||
enabled: config.enabled !== false,
|
||||
name: config.name || serviceId,
|
||||
@@ -660,12 +683,19 @@ class HealthChecker extends EventEmitter {
|
||||
* Remove service configuration
|
||||
*/
|
||||
removeService(serviceId) {
|
||||
this.serviceGenerations.set(serviceId, (this.serviceGenerations.get(serviceId) || 0) + 1);
|
||||
if (this.config.services) {
|
||||
delete this.config.services[serviceId];
|
||||
this.saveConfig();
|
||||
}
|
||||
|
||||
this.currentStatus.delete(serviceId);
|
||||
this.displayedStatus.delete(serviceId);
|
||||
this.consecutiveSinceChange.delete(serviceId);
|
||||
this.consecutiveFailures.delete(serviceId);
|
||||
const timer = this.serviceTimers.get(serviceId);
|
||||
if (timer) clearTimeout(timer);
|
||||
this.serviceTimers.delete(serviceId);
|
||||
delete this.history[serviceId];
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user