diff --git a/server/src/domain/notifications/notification.message-builder.ts b/server/src/domain/notifications/notification.message-builder.ts index f53f9d609..55b2ae1ef 100644 --- a/server/src/domain/notifications/notification.message-builder.ts +++ b/server/src/domain/notifications/notification.message-builder.ts @@ -39,7 +39,7 @@ export class NotificationMessageBuilder implements INotificationMessageBuilder { clientHost, metadata: { teamId: monitor.teamId, - notificationReason: decision.notificationReason || "status_change", + notificationReason: decision.notificationReason, }, }; } diff --git a/server/src/domain/notifications/notification.type.ts b/server/src/domain/notifications/notification.type.ts index b696bf752..34899386d 100644 --- a/server/src/domain/notifications/notification.type.ts +++ b/server/src/domain/notifications/notification.type.ts @@ -1,4 +1,5 @@ import type { HardwareMetricKey } from "@/domain/monitors/monitor.type.js"; +import { MonitorActionDecision } from "@/worker/worker.interface.js"; export const NotificationChannels = [ "email", @@ -119,6 +120,6 @@ export interface NotificationMessage { clientHost: string; metadata: { teamId: string; - notificationReason: string; + notificationReason: MonitorActionDecision["notificationReason"]; }; } diff --git a/server/src/service/statusService.ts b/server/src/service/statusService.ts index 48b75f1da..b51b9698b 100755 --- a/server/src/service/statusService.ts +++ b/server/src/service/statusService.ts @@ -21,7 +21,6 @@ const SERVICE_NAME = "StatusService"; const HARDWARE_ALERT_COUNTER_START = 5; export interface IStatusService { - updateRunningStats(check: Check, monitor: Monitor): Promise; updateMonitorStatus(check: Check, monitor: Monitor): Promise; } @@ -37,32 +36,19 @@ export class StatusService implements IStatusService { this.monitorStatsRepository = monitorStatsRepository; } - async updateRunningStats(check: Check, monitor: Monitor) { + private tryUpdateRunningStats = async (check: Check, monitor: Monitor) => { try { await this.monitorStatsRepository.updateByMonitorId(monitor.id, { status: check.status === true, responseTime: check.responseTime ?? 0, now: Date.now(), }); - return true; } catch (error: unknown) { - this.logger.error({ - service: SERVICE_NAME, - message: error instanceof Error ? error.message : "Unknown error", - method: "updateRunningStats", - stack: error instanceof Error ? error.stack : undefined, - }); - return false; - } - } - - private tryUpdateRunningStats = async (check: Check, monitor: Monitor) => { - const statsOk = await this.updateRunningStats(check, monitor); - if (!statsOk) { this.logger.warn({ service: SERVICE_NAME, - method: "updateMonitorStatus", - message: `Stats update failed for monitor ${monitor.id}`, + message: error instanceof Error ? error.message : "Unknown error", + method: "tryUpdateRunningStats", + stack: error instanceof Error ? error.stack : undefined, }); } }; @@ -160,7 +146,7 @@ export class StatusService implements IStatusService { updateMonitorStatus = async (check: Check, monitor: Monitor): Promise => { try { - const { status, statusCode } = check; + const { status } = check; // Update running stats await this.tryUpdateRunningStats(check, monitor); @@ -188,81 +174,62 @@ export class StatusService implements IStatusService { statusChanged = newStatus === "down"; } - // Not enough data points yet — record the check and return - if (projectedWindow.length < monitor.statusWindowSize) { - const updated = await this.monitorsRepository.updateStatusWindowAndChecks( - monitor.id, - monitor.teamId, - check.status, - checkSnapshot, - monitor.statusWindowSize, - MAX_RECENT_CHECKS, - patch - ); - - return { - monitor: updated, - statusChanged, - prevStatus, - code: statusCode, - timestamp: Date.now(), - }; - } - - // First evaluate reachability status changes, which apply to all monitor types - // and take precedence over hardware breaches. - const reachabilityResult = this.computeReachability(newStatus, projectedWindow, monitor.statusWindowThreshold); - if (reachabilityResult.transitioned) { - newStatus = reachabilityResult.nextStatus; - statusChanged = true; - } - - // Evaluate hardware threshold breaches let thresholdBreaches: HardwareBreaches | undefined; - if (monitor.type === "hardware") { - const hardware = this.computeHardwareStatus({ - currentStatus: newStatus, - reachabilityDown: newStatus === "down", - metrics: check, - thresholds: { - cpu: monitor.cpuAlertThreshold, - memory: monitor.memoryAlertThreshold, - disk: monitor.diskAlertThreshold, - temp: monitor.tempAlertThreshold, - }, - counters: { - cpu: monitor.cpuAlertCounter, - memory: monitor.memoryAlertCounter, - disk: monitor.diskAlertCounter, - temp: monitor.tempAlertCounter, - }, - }); - patch.cpuAlertCounter = hardware.nextCounters.cpu; - patch.memoryAlertCounter = hardware.nextCounters.memory; - patch.diskAlertCounter = hardware.nextCounters.disk; - patch.tempAlertCounter = hardware.nextCounters.temp; - thresholdBreaches = hardware.breaches; - if (hardware.transitioned) { - newStatus = hardware.nextStatus; + if (projectedWindow.length >= monitor.statusWindowSize) { + // First evaluate reachability status changes, which apply to all monitor types + // and take precedence over hardware breaches. + const reachabilityResult = this.computeReachability(newStatus, projectedWindow, monitor.statusWindowThreshold); + if (reachabilityResult.transitioned) { + newStatus = reachabilityResult.nextStatus; statusChanged = true; } - } - if (monitor.type === "docker" && check.containers) { - const breaches = findContainerBreaches(monitor, check.containers); - const docker = this.computeDockerStatus({ - currentStatus: newStatus, - reachabilityDown: newStatus === "down", - breaching: breaches.length > 0, - }); - if (docker.transitioned) { - newStatus = docker.nextStatus; - statusChanged = true; + // Evaluate hardware threshold breaches + if (monitor.type === "hardware") { + const hardware = this.computeHardwareStatus({ + currentStatus: newStatus, + reachabilityDown: newStatus === "down", + metrics: check, + thresholds: { + cpu: monitor.cpuAlertThreshold, + memory: monitor.memoryAlertThreshold, + disk: monitor.diskAlertThreshold, + temp: monitor.tempAlertThreshold, + }, + counters: { + cpu: monitor.cpuAlertCounter, + memory: monitor.memoryAlertCounter, + disk: monitor.diskAlertCounter, + temp: monitor.tempAlertCounter, + }, + }); + + patch.cpuAlertCounter = hardware.nextCounters.cpu; + patch.memoryAlertCounter = hardware.nextCounters.memory; + patch.diskAlertCounter = hardware.nextCounters.disk; + patch.tempAlertCounter = hardware.nextCounters.temp; + thresholdBreaches = hardware.breaches; + if (hardware.transitioned) { + newStatus = hardware.nextStatus; + statusChanged = true; + } } - } - patch.status = newStatus; + if (monitor.type === "docker" && check.containers) { + const breaches = findContainerBreaches(monitor, check.containers); + const docker = this.computeDockerStatus({ + currentStatus: newStatus, + reachabilityDown: newStatus === "down", + breaching: breaches.length > 0, + }); + if (docker.transitioned) { + newStatus = docker.nextStatus; + statusChanged = true; + } + } + patch.status = newStatus; + } // Single atomic write: push arrays + set status/counters const updated = await this.monitorsRepository.updateStatusWindowAndChecks( @@ -279,8 +246,6 @@ export class StatusService implements IStatusService { monitor: updated, statusChanged, prevStatus, - code: statusCode, - timestamp: new Date().getTime(), thresholdBreaches, }; } catch (error: unknown) { diff --git a/server/src/types/network.ts b/server/src/types/network.ts index 3ca2f107f..a1b23c26c 100644 --- a/server/src/types/network.ts +++ b/server/src/types/network.ts @@ -182,8 +182,6 @@ export type StatusChangeResult = { monitor: Monitor; statusChanged: boolean; prevStatus: MonitorStatus; - code: number; - timestamp: number; thresholdBreaches?: HardwareBreaches; }; diff --git a/server/src/worker/reactors/reactor.notification.ts b/server/src/worker/reactors/reactor.notification.ts index 91b9b7cd4..be65475e0 100644 --- a/server/src/worker/reactors/reactor.notification.ts +++ b/server/src/worker/reactors/reactor.notification.ts @@ -7,7 +7,6 @@ export class NotificationReactor implements IMonitorReactor { constructor(private notificationService: INotificationsService) {} react = async (evaluation: MonitorEvaluation) => { - if (!evaluation.decision.shouldSendNotification) return; await this.notificationService.handleNotifications(evaluation.monitor, evaluation.check, evaluation.decision); }; } diff --git a/server/test/helpers/heartbeatTestHarness.ts b/server/test/helpers/heartbeatTestHarness.ts index f99be611c..d1166f238 100644 --- a/server/test/helpers/heartbeatTestHarness.ts +++ b/server/test/helpers/heartbeatTestHarness.ts @@ -10,6 +10,8 @@ import { InMemoryMonitorsRepository } from "./InMemoryMonitorsRepository.ts"; import { InMemoryIncidentsRepository } from "./InMemoryIncidentsRepository.ts"; import { createMockLogger } from "./createMockLogger.ts"; import type { Monitor } from "../../src/domain/monitors/monitor.type.ts"; +import type { Check } from "../../src/domain/checks/check.type.ts"; +import type { MonitorActionDecision } from "../../src/worker/worker.interface.ts"; import type { MonitorStatusResponse } from "../../src/types/network.ts"; import type { MaintenanceWindow } from "../../src/domain/maintenance-windows/maintenance-window.type.ts"; import type { EgressStatus } from "../../src/domain/egress/egress.type.ts"; @@ -117,7 +119,13 @@ export function createHeartbeatTestHarness(): HeartbeatTestHarness { nextEgressStatus = status; }; - const notificationReactor = new NotificationReactor(notificationsService as any); + // The real NotificationsService ignores decisions with shouldSendNotification false; the stub is reached only for + // the ones it would act on, so tests can assert on handleNotifications calls as "notifications sent". + const guardedNotificationsService = { + handleNotifications: (monitor: Monitor, check: Check, decision: MonitorActionDecision) => + decision.shouldSendNotification ? notificationsService.handleNotifications(monitor, check, decision) : Promise.resolve(false), + }; + const notificationReactor = new NotificationReactor(guardedNotificationsService as any); const incidentReactor = new IncidentReactor(incidentService as any); const reactorDispatcher = new ReactorDispatcher(logger, [notificationReactor, incidentReactor]); diff --git a/server/test/integration/evaluateCursorMultiWorker.test.ts b/server/test/integration/evaluateCursorMultiWorker.test.ts index a3fe75b04..5f40d2eef 100644 --- a/server/test/integration/evaluateCursorMultiWorker.test.ts +++ b/server/test/integration/evaluateCursorMultiWorker.test.ts @@ -133,7 +133,7 @@ describe("Evaluate cursor with more than one processing worker", () => { statusService: { updateMonitorStatus: async (check: Check, monitor: Monitor) => { evaluatedCheckIds.push(check.id); - return { monitor, statusChanged: false, prevStatus: monitor.status, code: 200, timestamp: 0 }; + return { monitor, statusChanged: false, prevStatus: monitor.status }; }, } as any, dispatcher: { dispatch: jest.fn().mockResolvedValue(undefined) } as any, diff --git a/server/test/unit/services/notificationMessageBuilder.test.ts b/server/test/unit/services/notificationMessageBuilder.test.ts index 199d80101..472542e09 100644 --- a/server/test/unit/services/notificationMessageBuilder.test.ts +++ b/server/test/unit/services/notificationMessageBuilder.test.ts @@ -144,15 +144,6 @@ describe("NotificationMessageBuilder", () => { expect(msg.severity).toBe("success"); expect(msg.content.title).toBe("Thresholds Resolved: Test Monitor"); }); - - it("uses notificationReason from decision, falling back to status_change", () => { - const monitor = makeMonitor({ status: "down" }); - const decision = makeDecision({ notificationReason: null }); - - const msg = builder.buildMessage(monitor, makeCheck(), decision, "https://app.example.com"); - - expect(msg.metadata.notificationReason).toBe("status_change"); - }); }); // ── determineNotificationType (via buildMessage) ───────────────────── diff --git a/server/test/unit/services/statusService.test.ts b/server/test/unit/services/statusService.test.ts index 50005312d..031df1a46 100644 --- a/server/test/unit/services/statusService.test.ts +++ b/server/test/unit/services/statusService.test.ts @@ -131,92 +131,86 @@ const makeExistingStats = (overrides?: Record) => ({ // ── Tests ──────────────────────────────────────────────────────────────────── describe("StatusService", () => { - // ── updateRunningStats ─────────────────────────────────────────────────── + // ── running stats (via updateMonitorStatus) ────────────────────────────── - describe("updateRunningStats", () => { - // The service now delegates the entire running-stats computation to the repository's - // atomic `updateByMonitorId`, so the service-level tests only verify forwarding and error - // handling. The math (running average, uptimePercentage, timeOfLastFailure state machine) - // is a repository-level concern and is exercised by the repository tests. + describe("running stats", () => { + // The service delegates the running-stats computation to the repository's atomic `updateByMonitorId`, + // so these cases only verify what is forwarded and that a failure never blocks the status update. + // The math (running average, uptimePercentage, timeOfLastFailure) is exercised by the repository tests. + + const run = async (check: Check) => { + const monitor = makeMonitor(); + const deps = createService(); + (deps.monitorsRepository.findById as jest.Mock).mockResolvedValue(monitor); + (deps.monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); + const result = await deps.service.updateMonitorStatus(check, monitor); + return { ...deps, result }; + }; it("forwards a successful check to updateByMonitorId", async () => { - const { service, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); + const { monitorStatsRepository } = await run(makeCheck({ responseTime: 50 })); - const result = await service.updateRunningStats(makeCheck({ responseTime: 50 }), makeMonitor()); - - expect(result).toBe(true); expect(monitorStatsRepository.updateByMonitorId).toHaveBeenCalledWith( "mon-1", - expect.objectContaining({ - status: true, - responseTime: 50, - now: expect.any(Number), - }) + expect.objectContaining({ status: true, responseTime: 50, now: expect.any(Number) }) ); }); it("forwards a failed check to updateByMonitorId", async () => { - const { service, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); + const { monitorStatsRepository } = await run(makeCheck({ status: false, responseTime: 100 })); - const result = await service.updateRunningStats(makeCheck({ status: false, responseTime: 100 }), makeMonitor()); - - expect(result).toBe(true); expect(monitorStatsRepository.updateByMonitorId).toHaveBeenCalledWith("mon-1", expect.objectContaining({ status: false, responseTime: 100 })); }); it("passes 0 when responseTime is undefined", async () => { - const { service, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); - - await service.updateRunningStats(makeCheck({ responseTime: undefined as any }), makeMonitor()); + const { monitorStatsRepository } = await run(makeCheck({ responseTime: undefined as any })); expect(monitorStatsRepository.updateByMonitorId).toHaveBeenCalledWith("mon-1", expect.objectContaining({ responseTime: 0 })); }); it("forwards responseTime of 0 as 0 (falsy but defined)", async () => { - const { service, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); - - await service.updateRunningStats(makeCheck({ responseTime: 0 }), makeMonitor()); + const { monitorStatsRepository } = await run(makeCheck({ responseTime: 0 })); expect(monitorStatsRepository.updateByMonitorId).toHaveBeenCalledWith("mon-1", expect.objectContaining({ responseTime: 0 })); }); it("coerces a non-true status to false (e.g. undefined)", async () => { - const { service, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockResolvedValue({}); - - await service.updateRunningStats(makeCheck({ status: undefined as any }), makeMonitor()); + const { monitorStatsRepository } = await run(makeCheck({ status: undefined as any })); expect(monitorStatsRepository.updateByMonitorId).toHaveBeenCalledWith("mon-1", expect.objectContaining({ status: false })); }); - it("returns false and logs error when updateByMonitorId throws", async () => { - const { service, logger, monitorStatsRepository } = createService(); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockRejectedValue(new Error("db write failed")); + it("warns once and still updates the status when updateByMonitorId throws", async () => { + const monitor = makeMonitor({ statusWindow: [true, true, true, true], statusWindowSize: 5, status: "up" }); + const { service, logger, monitorsRepository, monitorStatsRepository } = createService(); + (monitorsRepository.findById as jest.Mock).mockResolvedValue(monitor); + (monitorStatsRepository.updateByMonitorId as jest.Mock).mockRejectedValue(new Error("stats db down")); - const result = await service.updateRunningStats(makeCheck(), makeMonitor()); + const result = await service.updateMonitorStatus(makeCheck({ status: true }), monitor); - expect(result).toBe(false); - expect(logger.error).toHaveBeenCalledWith( + expect(result.monitor.status).toBe("up"); + expect(monitorsRepository.updateStatusWindowAndChecks).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( expect.objectContaining({ service: "StatusService", - method: "updateRunningStats", - message: "db write failed", + method: "tryUpdateRunningStats", + message: "stats db down", + stack: expect.any(String), }) ); + expect(logger.error).not.toHaveBeenCalled(); }); - it("logs error with 'Unknown error' for non-Error exceptions", async () => { - const { service, logger, monitorStatsRepository } = createService(); + it("warns with 'Unknown error' for non-Error rejections", async () => { + const monitor = makeMonitor(); + const { service, logger, monitorsRepository, monitorStatsRepository } = createService(); + (monitorsRepository.findById as jest.Mock).mockResolvedValue(monitor); (monitorStatsRepository.updateByMonitorId as jest.Mock).mockRejectedValue("string error"); - const result = await service.updateRunningStats(makeCheck(), makeMonitor()); + await service.updateMonitorStatus(makeCheck(), monitor); - expect(result).toBe(false); - expect(logger.error).toHaveBeenCalledWith(expect.objectContaining({ message: "Unknown error", stack: undefined })); + expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Unknown error", stack: undefined })); }); }); @@ -640,37 +634,6 @@ describe("StatusService", () => { expect(result.monitor.status).toBe("down"); }); - it("logs a warning but still succeeds when running-stats update fails mid-flight", async () => { - const monitor = makeMonitor({ statusWindow: [true, true, true, true], statusWindowSize: 5, status: "up" }); - const { service, logger, monitorsRepository, monitorStatsRepository } = createService(); - (monitorsRepository.findById as jest.Mock).mockResolvedValue(monitor); - (monitorsRepository.updateById as jest.Mock).mockImplementation((_id: unknown, _tid: unknown, m: unknown) => Promise.resolve(m)); - (monitorStatsRepository.updateByMonitorId as jest.Mock).mockRejectedValue(new Error("stats db down")); - - const result = await service.updateMonitorStatus(makeCheck({ status: true }), monitor); - - expect(result.monitor.status).toBe("up"); - expect(logger.warn).toHaveBeenCalledWith( - expect.objectContaining({ - service: "StatusService", - method: "updateMonitorStatus", - message: expect.stringContaining("Stats update failed"), - }) - ); - }); - - it("returns code and timestamp in result", async () => { - const monitor = makeMonitor({ statusWindow: [true, true, true, true], statusWindowSize: 5 }); - const { service, monitorsRepository } = createService(); - (monitorsRepository.findById as jest.Mock).mockResolvedValue(monitor); - (monitorsRepository.updateById as jest.Mock).mockImplementation((_id: unknown, _tid: unknown, m: unknown) => Promise.resolve(m)); - - const result = await service.updateMonitorStatus(makeCheck({ statusCode: 201 }), monitor); - - expect(result.code).toBe(201); - expect(result.timestamp).toBeGreaterThan(0); - }); - // ── Hardware threshold breach tests ────────────────────────────────── describe("hardware threshold breaches", () => { diff --git a/server/test/unit/services/workerPipeline.test.ts b/server/test/unit/services/workerPipeline.test.ts index bbb389573..fbaf2eb73 100644 --- a/server/test/unit/services/workerPipeline.test.ts +++ b/server/test/unit/services/workerPipeline.test.ts @@ -64,8 +64,6 @@ const makeStatusChange = (overrides: Partial & { status?: st monitor: { id: "m1", status } as any, statusChanged: false, prevStatus: "up", - code: 200, - timestamp: 0, ...rest, } as StatusChangeResult; }; @@ -706,7 +704,7 @@ describe("WorkerPipeline", () => { it("returns an evaluation that opens an incident on a down transition", async () => { const check = makeCheck({ status: false, statusCode: 500, message: "Error" }); - const statusChange = makeStatusChange({ monitor: makeMonitor({ status: "down" }), statusChanged: true, prevStatus: "up", code: 500 }); + const statusChange = makeStatusChange({ monitor: makeMonitor({ status: "down" }), statusChanged: true, prevStatus: "up" }); const { pipeline } = createPipeline({ statusService: { updateMonitorStatus: jest.fn().mockResolvedValue(statusChange) }, }); @@ -802,7 +800,7 @@ describe("WorkerPipeline", () => { }); it("creates incident and notifies when monitor goes down", async () => { - const decision = await decide(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up", code: 500 })); + const decision = await decide(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up" })); expect(decision).toMatchObject({ shouldCreateIncident: true, shouldSendNotification: true, @@ -854,7 +852,7 @@ describe("WorkerPipeline", () => { it("leaves threshold breaches off a down decision even when the status change carries them", async () => { const thresholdBreaches = { cpu: true, memory: false, disk: false, temp: false }; - const decision = await decide(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up", code: 500, thresholdBreaches })); + const decision = await decide(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up", thresholdBreaches })); expect(decision).not.toHaveProperty("thresholdBreaches"); }); @@ -876,8 +874,8 @@ describe("WorkerPipeline", () => { it("starts every decision clean after a transition on the same pipeline", async () => { const updateMonitorStatus = jest .fn() - .mockResolvedValueOnce(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up", code: 500 })) - .mockResolvedValueOnce(makeStatusChange({ status: "down", statusChanged: false, prevStatus: "down", code: 500 })); + .mockResolvedValueOnce(makeStatusChange({ status: "down", statusChanged: true, prevStatus: "up" })) + .mockResolvedValueOnce(makeStatusChange({ status: "down", statusChanged: false, prevStatus: "down" })); const { pipeline } = createPipeline({ statusService: { updateMonitorStatus } }); const failing = makeCheck({ status: false, statusCode: 500, message: "Error" });