From 6b94907b70399d509405dbecf4771d1e6356ea5d Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 13:56:47 +0700 Subject: [PATCH 1/5] fix(keeper): warn loudly at startup when the keeper is enabled but its alerts cannot be delivered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real incident driving this: a neighboring repo's price keeper failed silently for 3.5 weeks — process alive (launchd KeepAlive), every RPC call failing (8,455x 403), zero successes, and its alert channel was also broken, so nobody knew. This repo's keeper has the same latent hole: on updatePrice() failure it calls `this.opsAlert?.alert("critical", ...)`, but OpsAlertService is @Optional()-injected and its alert() is a silent no-op whenever alerting is disabled/unconfigured (OpsAlertService.isEnabled() === false) or the service was never injected at all (opsAlert === undefined). "Keeper enabled + alerts unconfigured" is a config state that makes every future updatePrice() failure invisible. Add a startup self-check (KeeperService.checkAlertDeliverability(), called from onApplicationBootstrap right after the keeperEnabled gate): when the keeper is enabled but alerts are not deliverable, logger.error() once at boot, naming the consequence and which env vars to set (OPS_ALERT_ENABLED + OPS_ALERT_BOT_TOKEN/OPS_ALERT_CHAT_ID, or AASTAR_MONITOR_URL/AASTAR_MONITOR_TOKEN). Deliverability is judged solely via OpsAlertService.isEnabled() — the one place that already encodes "opt-in flag AND a transport is actually configured" — so the judgment logic lives in exactly one place and cannot drift from the real alert() gate. This only logs — it never throws, never blocks bootstrap, and never stops ticking. Halting the keeper because alerting is broken would guarantee the exact harm it exists to prevent (a stale on-chain price); the safe response to an alerting gap is to keep working and say so loudly, not to stop. Covered by a dedicated test asserting the keeper still schedules and runs a tick after the self-check logs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/keeper/keeper.service.spec.ts | 126 ++++++++++++++++++++++ src/modules/keeper/keeper.service.ts | 31 ++++++ 2 files changed, 157 insertions(+) diff --git a/src/modules/keeper/keeper.service.spec.ts b/src/modules/keeper/keeper.service.spec.ts index dd3b7ab..d8ae138 100644 --- a/src/modules/keeper/keeper.service.spec.ts +++ b/src/modules/keeper/keeper.service.spec.ts @@ -1,4 +1,5 @@ import { jest } from "@jest/globals"; +import { Logger } from "@nestjs/common"; import { KeeperService } from "./keeper.service.js"; const PAYMASTER = "0x" + "12".repeat(20); @@ -57,6 +58,11 @@ function makeRegistry() { } as any; } +/** Stand-in for OpsAlertService — only `isEnabled()` matters to the self-check. */ +function makeOpsAlert(deliverable: boolean) { + return { isEnabled: () => deliverable, alert: jest.fn() } as any; +} + /** Fixed clock at t=now (unix ms). Allows overriding "now" for time-based guardrail tests. */ function clockAt(nowMs: number) { return () => nowMs; @@ -332,3 +338,123 @@ describe("KeeperService", () => { } }); }); + +/** + * Startup self-check: "keeper enabled but alerts undeliverable" must never go + * unnoticed silently (the real incident that motivates this: a neighboring repo's + * price keeper ran for 3.5 weeks, failing every tick, with a broken alert channel + * — nobody knew). This must ONLY ever log; it must never throw or block startup. + */ +describe("KeeperService — startup self-check: alert deliverability", () => { + let errorSpy: ReturnType; + + beforeEach(() => { + errorSpy = jest.spyOn(Logger.prototype, "error").mockImplementation((() => undefined) as any); + }); + + afterEach(() => { + errorSpy.mockRestore(); + }); + + it("keeper enabled + alerts deliverable ⇒ no self-check error", () => { + const svc = new KeeperService( + makeBlockchain(), + makeNotify(), + makeConfig(), + clockAt(0), + makeRegistry(), + undefined, + makeOpsAlert(true) + ); + svc.onApplicationBootstrap(); + svc.onApplicationShutdown(); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it("keeper enabled + OpsAlertService configured but disabled ⇒ logs an error at startup", () => { + const svc = new KeeperService( + makeBlockchain(), + makeNotify(), + makeConfig(), + clockAt(0), + makeRegistry(), + undefined, + makeOpsAlert(false) + ); + svc.onApplicationBootstrap(); + svc.onApplicationShutdown(); + expect(errorSpy).toHaveBeenCalledTimes(1); + const [msg] = errorSpy.mock.calls[0] as unknown as [string]; + expect(msg).toContain("NOT deliverable"); + expect(msg).toContain("OPS_ALERT_ENABLED"); + }); + + it("keeper enabled + OpsAlertService not injected at all (undefined) ⇒ logs an error", () => { + const svc = new KeeperService( + makeBlockchain(), + makeNotify(), + makeConfig(), + clockAt(0), + makeRegistry(), + undefined, + undefined // no opsAlert provider in this DI graph + ); + svc.onApplicationBootstrap(); + svc.onApplicationShutdown(); + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + + it("keeper NOT enabled ⇒ no self-check noise, regardless of alert state", () => { + const svc = new KeeperService( + makeBlockchain(), + makeNotify(), + makeConfig({ keeperEnabled: false }), + clockAt(0), + makeRegistry(), + undefined, + makeOpsAlert(false) + ); + svc.onApplicationBootstrap(); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it("self-check error does NOT stop the keeper — it still schedules and runs ticks normally", async () => { + jest.useFakeTimers(); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, // no jitter — fire immediately + makeOpsAlert(false) // undeliverable ⇒ self-check logs, but must not block anything + ); + + svc.onApplicationBootstrap(); + // The self-check fired loudly... + expect(errorSpy).toHaveBeenCalledTimes(1); + // ...but scheduling proceeded exactly as if alerting were fine. + expect((svc as any).startupTimer).not.toBeNull(); + + await jest.advanceTimersByTimeAsync(0); + // The first tick actually ran and called updatePrice(). + expect(updates).toHaveLength(1); + // The recurring interval is armed as normal. + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + expect((svc as any).timer).toBeNull(); + } finally { + jest.useRealTimers(); + } + }); +}); diff --git a/src/modules/keeper/keeper.service.ts b/src/modules/keeper/keeper.service.ts index bbc83f9..a5ce0c6 100644 --- a/src/modules/keeper/keeper.service.ts +++ b/src/modules/keeper/keeper.service.ts @@ -90,6 +90,7 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd onApplicationBootstrap(): void { if (!this.enabled) return; + this.checkAlertDeliverability(); if (this.paymasterAddresses.length === 0) { this.logger.warn( "Keeper: KEEPER_ENABLED=true but KEEPER_PAYMASTER_ADDRESS not set — disabled" @@ -126,6 +127,36 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd } } + /** + * Startup self-check (real incident driven — a neighboring repo's price keeper + * failed silently for 3.5 weeks: process alive, every tick failing, zero alerts, + * because its alert channel was also broken and nobody knew). + * + * The keeper's own failure path (`refreshOne` below) only ever *tries* to alert + * via `this.opsAlert?.alert(...)` — that call is a silent no-op whenever + * OpsAlertService is disabled/unconfigured, or isn't injected at all. Loudly + * surface that combination once at startup so it isn't discovered only after an + * updatePrice() failure goes unnoticed. This intentionally does NOT stop the + * keeper: halting on an alerting gap would guarantee the exact harm (a stale + * price) that this keeper exists to prevent — the safe response is to keep + * working and say so loudly, not to stop. + * + * Deliverability is judged solely by `OpsAlertService.isEnabled()` — the single + * place that already encodes "opt-in flag AND a transport is actually + * configured" (see ops-alert.service.ts). Duplicating that condition here would + * let the two checks drift. + */ + private checkAlertDeliverability(): void { + if (this.opsAlert?.isEnabled()) return; + this.logger.error( + "Keeper is ENABLED but operator alerts are NOT deliverable — if updatePrice() " + + "fails, nobody will be notified (this is exactly how a stale-price outage goes " + + "unnoticed). This does not stop the keeper. Configure alerting via " + + "OPS_ALERT_ENABLED=true plus a transport: OPS_ALERT_BOT_TOKEN + OPS_ALERT_CHAT_ID " + + "(Telegram), or AASTAR_MONITOR_URL (+ optional AASTAR_MONITOR_TOKEN) for a webhook." + ); + } + /** Startup phase offset in [0, intervalMs). Visible for testing. */ computeJitterMs(): number { return Math.floor(this.random() * this.intervalMs); From d52bf3967ee1f6f60ff82667084d2461776ca24e Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 13:59:40 +0700 Subject: [PATCH 2/5] fix(keeper): rename the startup self-check to "configured", not "deliverable", and move it past the paymaster guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review fixes from the initial pass: 1. Naming/claim fix. `OpsAlertService.isEnabled()` means "opt-in flag is true AND a transport is set" — it never probes that transport. A configured-but-revoked Telegram bot token still makes isEnabled() return true, so the previous "NOT deliverable" wording and checkAlertDeliverability() name overclaimed: they would have stayed silent for exactly the incident this is modeled on (the neighboring repo's token was configured, just dead/Unauthorized). Renamed to checkAlertConfigured() / "NOT configured", and the log message + docstring now say plainly that configured != deliverable and that only a real alert drill proves delivery works. Test helper/assertions renamed to match (makeOpsAlert(configured), and "NOT configured" instead of "NOT deliverable"). 2. Ordering fix. The check ran before the `paymasterAddresses.length === 0` guard, so `KEEPER_ENABLED=true` with no `KEEPER_PAYMASTER_ADDRESS` (keeper never actually runs) still logged an alert-config error — pure noise. Moved the check to run only after that guard, i.e. only when the keeper will actually schedule ticks. Added a test for this combination: keeper enabled + alerts unconfigured + no paymaster addresses ⇒ only the existing "disabled" warn fires, no alert-config error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/keeper/keeper.service.spec.ts | 35 +++++++++++++++++------ src/modules/keeper/keeper.service.ts | 26 +++++++++++------ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/modules/keeper/keeper.service.spec.ts b/src/modules/keeper/keeper.service.spec.ts index d8ae138..4157b8b 100644 --- a/src/modules/keeper/keeper.service.spec.ts +++ b/src/modules/keeper/keeper.service.spec.ts @@ -59,8 +59,8 @@ function makeRegistry() { } /** Stand-in for OpsAlertService — only `isEnabled()` matters to the self-check. */ -function makeOpsAlert(deliverable: boolean) { - return { isEnabled: () => deliverable, alert: jest.fn() } as any; +function makeOpsAlert(configured: boolean) { + return { isEnabled: () => configured, alert: jest.fn() } as any; } /** Fixed clock at t=now (unix ms). Allows overriding "now" for time-based guardrail tests. */ @@ -340,12 +340,16 @@ describe("KeeperService", () => { }); /** - * Startup self-check: "keeper enabled but alerts undeliverable" must never go + * Startup self-check: "keeper enabled but alerts unconfigured" must never go * unnoticed silently (the real incident that motivates this: a neighboring repo's * price keeper ran for 3.5 weeks, failing every tick, with a broken alert channel * — nobody knew). This must ONLY ever log; it must never throw or block startup. + * + * Scope note (see keeper.service.ts docstring): this checks CONFIGURATION + * (OpsAlertService.isEnabled()), not deliverability — it cannot and does not + * claim to catch a configured-but-dead transport. */ -describe("KeeperService — startup self-check: alert deliverability", () => { +describe("KeeperService — startup self-check: alert configured", () => { let errorSpy: ReturnType; beforeEach(() => { @@ -356,7 +360,7 @@ describe("KeeperService — startup self-check: alert deliverability", () => { errorSpy.mockRestore(); }); - it("keeper enabled + alerts deliverable ⇒ no self-check error", () => { + it("keeper enabled + alerts configured ⇒ no self-check error", () => { const svc = new KeeperService( makeBlockchain(), makeNotify(), @@ -371,7 +375,7 @@ describe("KeeperService — startup self-check: alert deliverability", () => { expect(errorSpy).not.toHaveBeenCalled(); }); - it("keeper enabled + OpsAlertService configured but disabled ⇒ logs an error at startup", () => { + it("keeper enabled + OpsAlertService present but disabled ⇒ logs an error at startup", () => { const svc = new KeeperService( makeBlockchain(), makeNotify(), @@ -385,7 +389,7 @@ describe("KeeperService — startup self-check: alert deliverability", () => { svc.onApplicationShutdown(); expect(errorSpy).toHaveBeenCalledTimes(1); const [msg] = errorSpy.mock.calls[0] as unknown as [string]; - expect(msg).toContain("NOT deliverable"); + expect(msg).toContain("NOT configured"); expect(msg).toContain("OPS_ALERT_ENABLED"); }); @@ -418,6 +422,21 @@ describe("KeeperService — startup self-check: alert deliverability", () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it("keeper enabled + no paymaster addresses (keeper won't actually run) ⇒ no self-check noise", () => { + const svc = new KeeperService( + makeBlockchain(), + makeNotify(), + makeConfig({ keeperPaymasterAddress: "" }), + clockAt(0), + makeRegistry(), + undefined, + makeOpsAlert(false) // alerts unconfigured too, but irrelevant — keeper never runs + ); + svc.onApplicationBootstrap(); + // Only the existing "disabled" warn fires; the alert self-check must not. + expect(errorSpy).not.toHaveBeenCalled(); + }); + it("self-check error does NOT stop the keeper — it still schedules and runs ticks normally", async () => { jest.useFakeTimers(); try { @@ -436,7 +455,7 @@ describe("KeeperService — startup self-check: alert deliverability", () => { NOW_NEAR_EXPIRY, makeRegistry(), () => 0, // no jitter — fire immediately - makeOpsAlert(false) // undeliverable ⇒ self-check logs, but must not block anything + makeOpsAlert(false) // unconfigured ⇒ self-check logs, but must not block anything ); svc.onApplicationBootstrap(); diff --git a/src/modules/keeper/keeper.service.ts b/src/modules/keeper/keeper.service.ts index a5ce0c6..485722a 100644 --- a/src/modules/keeper/keeper.service.ts +++ b/src/modules/keeper/keeper.service.ts @@ -90,13 +90,13 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd onApplicationBootstrap(): void { if (!this.enabled) return; - this.checkAlertDeliverability(); if (this.paymasterAddresses.length === 0) { this.logger.warn( "Keeper: KEEPER_ENABLED=true but KEEPER_PAYMASTER_ADDRESS not set — disabled" ); return; } + this.checkAlertConfigured(); this.lastDayNumber = this.todayNumber(); // Phase-jitter the first tick across [0, intervalMs) so redundant keepers // that boot together don't all fire updatePrice() in the same window. @@ -141,19 +141,29 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd * price) that this keeper exists to prevent — the safe response is to keep * working and say so loudly, not to stop. * - * Deliverability is judged solely by `OpsAlertService.isEnabled()` — the single - * place that already encodes "opt-in flag AND a transport is actually - * configured" (see ops-alert.service.ts). Duplicating that condition here would - * let the two checks drift. + * Scope, precisely: this checks CONFIGURATION, not deliverability. + * `OpsAlertService.isEnabled()` — the single place that already encodes "opt-in + * flag AND a transport is actually configured" (see ops-alert.service.ts) — only + * tells us a transport was *set up*. It does not probe that transport, so a + * configured-but-broken transport (e.g. a revoked Telegram bot token — + * `Unauthorized` on every send) still reports `isEnabled() === true` and this + * check stays silent. That is in fact the shape of the neighboring incident + * this is modeled on: the token was configured, just dead. This check only + * catches the "never configured at all" class (this repo's `deploy/*.env.example` + * templates ship with no `OPS_ALERT_*` entries set, matching that state). + * Catching a configured-but-dead transport requires an actual delivery drill + * before relying on it, not a config read. */ - private checkAlertDeliverability(): void { + private checkAlertConfigured(): void { if (this.opsAlert?.isEnabled()) return; this.logger.error( - "Keeper is ENABLED but operator alerts are NOT deliverable — if updatePrice() " + + "Keeper is ENABLED but operator alerts are NOT configured — if updatePrice() " + "fails, nobody will be notified (this is exactly how a stale-price outage goes " + "unnoticed). This does not stop the keeper. Configure alerting via " + "OPS_ALERT_ENABLED=true plus a transport: OPS_ALERT_BOT_TOKEN + OPS_ALERT_CHAT_ID " + - "(Telegram), or AASTAR_MONITOR_URL (+ optional AASTAR_MONITOR_TOKEN) for a webhook." + "(Telegram), or AASTAR_MONITOR_URL (+ optional AASTAR_MONITOR_TOKEN) for a webhook. " + + "Note: configured is not the same as deliverable — a dead token/webhook still " + + "passes this check; only a real alert drill proves delivery actually works." ); } From 918bbe26f7b67185170c4afee5523a3bc8c3a6d3 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:05:55 +0700 Subject: [PATCH 3/5] fix(keeper): make the alert-config probe crash-proof, and stop overclaiming what it checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round-1 adversarial review findings, both addressed: 1. High: the probe itself must never throw. `checkAlertConfigured()` runs before the timers are armed in `onApplicationBootstrap`, so an uncaught throw from `this.opsAlert?.isEnabled()` — e.g. a partial/mock opsAlert missing `isEnabled`, or an `isEnabled` that itself throws — would abort bootstrap and block scheduling entirely, which is exactly the "block keeper operation" outcome the hard constraint forbids. Wrapped the probe in try/catch: a thrown probe is logged ("could not determine whether an operator-alert path is configured") and swallowed, then scheduling proceeds normally. Also tightened the read to `=== true` only — anything else (undefined, a non-boolean, or a Promise from a mock that violates isEnabled()'s documented synchronous/boolean contract, which is truthy under a loose check) is now treated as "not configured" instead of silently accepted. Added three tests, each asserting BOTH that an error is logged AND that the keeper still schedules and actually runs a tick afterward: opsAlert missing isEnabled, isEnabled() throwing, and isEnabled() returning a Promise. 2. Low: the log message claimed "if updatePrice() fails, nobody will be notified" — false, `refreshOne`'s catch block still calls `notificationService.sendToAccount(...)` on every failure regardless of this check. Reworded to what this check actually establishes: "there is no configured operator-alert path" / "will NOT raise an operator alert". Docstring now states explicitly that this check is scoped to the OpsAlertService path only and says nothing about the separate notificationService path. Updated the matching test assertion. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/keeper/keeper.service.spec.ts | 130 +++++++++++++++++++++- src/modules/keeper/keeper.service.ts | 86 +++++++++----- 2 files changed, 187 insertions(+), 29 deletions(-) diff --git a/src/modules/keeper/keeper.service.spec.ts b/src/modules/keeper/keeper.service.spec.ts index 4157b8b..248db5a 100644 --- a/src/modules/keeper/keeper.service.spec.ts +++ b/src/modules/keeper/keeper.service.spec.ts @@ -389,7 +389,7 @@ describe("KeeperService — startup self-check: alert configured", () => { svc.onApplicationShutdown(); expect(errorSpy).toHaveBeenCalledTimes(1); const [msg] = errorSpy.mock.calls[0] as unknown as [string]; - expect(msg).toContain("NOT configured"); + expect(msg).toContain("no configured operator-alert path"); expect(msg).toContain("OPS_ALERT_ENABLED"); }); @@ -476,4 +476,132 @@ describe("KeeperService — startup self-check: alert configured", () => { jest.useRealTimers(); } }); + + /** + * The probe (`this.opsAlert?.isEnabled()`) runs before the timers are armed, so + * it must never throw into the caller — an uncaught throw here would abort + * `onApplicationBootstrap` entirely and block scheduling, which is exactly the + * "block keeper operation" outcome the hard constraint forbids. Each case below + * must both log an error AND leave the keeper scheduling/ticking normally. + */ + it("opsAlert injected but missing isEnabled entirely ⇒ probe error is caught, keeper still runs", async () => { + jest.useFakeTimers(); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + const malformedOpsAlert = { alert: jest.fn() } as any; // no isEnabled at all + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, + malformedOpsAlert + ); + + svc.onApplicationBootstrap(); + // Calling opsAlert.isEnabled() throws (not a function) — caught and logged, + // never propagated. + expect(errorSpy).toHaveBeenCalledTimes(1); + expect((svc as any).startupTimer).not.toBeNull(); + + await jest.advanceTimersByTimeAsync(0); + expect(updates).toHaveLength(1); + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + } finally { + jest.useRealTimers(); + } + }); + + it("opsAlert.isEnabled() throws when called ⇒ probe error is caught, keeper still runs", async () => { + jest.useFakeTimers(); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + const throwingOpsAlert = { + isEnabled: () => { + throw new Error("boom"); + }, + alert: jest.fn(), + } as any; + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, + throwingOpsAlert + ); + + svc.onApplicationBootstrap(); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect((svc as any).startupTimer).not.toBeNull(); + + await jest.advanceTimersByTimeAsync(0); + expect(updates).toHaveLength(1); + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + } finally { + jest.useRealTimers(); + } + }); + + it("opsAlert.isEnabled() returns a Promise (misbehaving mock) ⇒ treated as not configured, never crashes, keeper still runs", async () => { + jest.useFakeTimers(); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + // isEnabled() is documented as synchronous+boolean; a mock returning a + // Promise violates that contract. A Promise is truthy, so a loose `if` + // would misread it as "configured" — strict `=== true` must not. + const asyncOpsAlert = { + isEnabled: () => Promise.resolve(true), + alert: jest.fn(), + } as any; + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, + asyncOpsAlert + ); + + svc.onApplicationBootstrap(); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect((svc as any).startupTimer).not.toBeNull(); + + await jest.advanceTimersByTimeAsync(0); + expect(updates).toHaveLength(1); + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + } finally { + jest.useRealTimers(); + } + }); }); diff --git a/src/modules/keeper/keeper.service.ts b/src/modules/keeper/keeper.service.ts index 485722a..c3dd6b2 100644 --- a/src/modules/keeper/keeper.service.ts +++ b/src/modules/keeper/keeper.service.ts @@ -132,38 +132,68 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd * failed silently for 3.5 weeks: process alive, every tick failing, zero alerts, * because its alert channel was also broken and nobody knew). * - * The keeper's own failure path (`refreshOne` below) only ever *tries* to alert - * via `this.opsAlert?.alert(...)` — that call is a silent no-op whenever - * OpsAlertService is disabled/unconfigured, or isn't injected at all. Loudly - * surface that combination once at startup so it isn't discovered only after an - * updatePrice() failure goes unnoticed. This intentionally does NOT stop the - * keeper: halting on an alerting gap would guarantee the exact harm (a stale - * price) that this keeper exists to prevent — the safe response is to keep - * working and say so loudly, not to stop. + * The keeper's own failure path (`refreshOne` below) only ever *tries* to raise + * an operator alert via `this.opsAlert?.alert(...)` — that call is a silent + * no-op whenever OpsAlertService is disabled/unconfigured, or isn't injected at + * all. Loudly surface that combination once at startup so it isn't discovered + * only after an updatePrice() failure goes unnoticed on that path. This + * intentionally does NOT stop the keeper: halting on an alerting gap would + * guarantee the exact harm (a stale price) that this keeper exists to prevent — + * the safe response is to keep working and say so loudly, not to stop. * - * Scope, precisely: this checks CONFIGURATION, not deliverability. - * `OpsAlertService.isEnabled()` — the single place that already encodes "opt-in - * flag AND a transport is actually configured" (see ops-alert.service.ts) — only - * tells us a transport was *set up*. It does not probe that transport, so a - * configured-but-broken transport (e.g. a revoked Telegram bot token — - * `Unauthorized` on every send) still reports `isEnabled() === true` and this - * check stays silent. That is in fact the shape of the neighboring incident - * this is modeled on: the token was configured, just dead. This check only - * catches the "never configured at all" class (this repo's `deploy/*.env.example` - * templates ship with no `OPS_ALERT_*` entries set, matching that state). - * Catching a configured-but-dead transport requires an actual delivery drill - * before relying on it, not a config read. + * Scope, precisely — two axes: + * - This is ONLY about the operator-alert path (`OpsAlertService`), i.e. + * whether an *operator* gets paged. It says nothing about + * `notificationService.sendToAccount(...)` in `refreshOne`'s catch block — + * that end-user notification path is separate and unaffected by this check. + * - This checks CONFIGURATION, not deliverability. + * `OpsAlertService.isEnabled()` — the single place that already encodes + * "opt-in flag AND a transport is actually configured" (see + * ops-alert.service.ts) — only tells us a transport was *set up*. It does + * not probe that transport, so a configured-but-broken transport (e.g. a + * revoked Telegram bot token — `Unauthorized` on every send) still reports + * `isEnabled() === true` and this check stays silent. That is in fact the + * shape of the neighboring incident this is modeled on: the token was + * configured, just dead. This check only catches the "never configured at + * all" class (this repo's `deploy/*.env.example` templates ship with no + * `OPS_ALERT_*` entries set, matching that state). Catching a + * configured-but-dead transport requires an actual delivery drill before + * relying on it, not a config read. + * + * Robustness: the probe itself must never throw into the caller — this method + * runs before the timers are armed (see onApplicationBootstrap above), so an + * uncaught throw here would abort scheduling entirely, which is precisely the + * "block keeper operation" outcome the hard constraint forbids. A malformed + * `opsAlert` (missing `isEnabled`, or one that throws/returns something odd) is + * therefore treated the same as "not configured", never allowed to propagate. + * Only the literal boolean `true` counts as "configured" — `isEnabled()` is + * documented as synchronous and boolean-returning, so anything else (undefined, + * a non-boolean, a Promise from a misbehaving mock/partial implementation, + * which is truthy under a loose check) is read as "not configured" rather than + * silently accepted as "probably fine". */ private checkAlertConfigured(): void { - if (this.opsAlert?.isEnabled()) return; + let configured: boolean; + try { + configured = this.opsAlert?.isEnabled() === true; + } catch (e) { + this.logger.error( + "Keeper: could not determine whether an operator-alert path is configured " + + `(the probe itself threw: ${String(e)}). Treating it as NOT configured — ` + + "failing loud, not silent. This does not stop the keeper." + ); + return; + } + if (configured) return; this.logger.error( - "Keeper is ENABLED but operator alerts are NOT configured — if updatePrice() " + - "fails, nobody will be notified (this is exactly how a stale-price outage goes " + - "unnoticed). This does not stop the keeper. Configure alerting via " + - "OPS_ALERT_ENABLED=true plus a transport: OPS_ALERT_BOT_TOKEN + OPS_ALERT_CHAT_ID " + - "(Telegram), or AASTAR_MONITOR_URL (+ optional AASTAR_MONITOR_TOKEN) for a webhook. " + - "Note: configured is not the same as deliverable — a dead token/webhook still " + - "passes this check; only a real alert drill proves delivery actually works." + "Keeper is ENABLED but there is no configured operator-alert path — a failed " + + "updatePrice() will NOT raise an operator alert (this is exactly how a " + + "stale-price outage goes unnoticed). This does not stop the keeper. Configure " + + "alerting via OPS_ALERT_ENABLED=true plus a transport: OPS_ALERT_BOT_TOKEN + " + + "OPS_ALERT_CHAT_ID (Telegram), or AASTAR_MONITOR_URL (+ optional " + + "AASTAR_MONITOR_TOKEN) for a webhook. Note: configured is not the same as " + + "deliverable — a dead token/webhook still passes this check; only a real " + + "alert drill proves delivery actually works." ); } From 208e1807005e5711c2db3909c321a827a732bd99 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:11:13 +0700 Subject: [PATCH 4/5] fix(keeper): close two escape hatches Codex round 2 found in the self-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are structural, not point-fixes for one bad input: 1. A rejected thenable from a misbehaving `isEnabled()` mock was compared with `=== true` and then discarded — the Promise itself was never consumed, so its rejection would surface later as an unhandled rejection (Node 15+ crashes the process on those by default). Round 1's tests only covered `Promise.resolve(true)`, which can't catch this. Fixed by explicitly attaching a no-op rejection handler (`result.then(undefined, () => {})`) whenever the probe result looks thenable — never awaited, since this startup path must stay synchronous. 2. The probe call was wrapped in try/catch, but the diagnostic path (`String(e)` / `this.logger.error(...)`) was not — if the logger itself throws, that escapes before the timers are armed, same as before. Chasing this with a nested catch just relocates the same question ("what if the catch throws") one level down indefinitely. Instead, added ONE boundary at the call site in `onApplicationBootstrap`: the entire `checkAlertConfigured()` call is wrapped in a try/catch that swallows unconditionally and logs nothing (logging is exactly what may have thrown). This makes "the self-check can never block scheduling" a structural invariant enforced in one place, not a property that has to hold at every internal throw site. Added two tests, each asserting BOTH "an error is logged where possible" and "the keeper still schedules and actually runs a tick afterward": - isEnabled() returns a Promise.reject(...) — asserts, via a temporary process 'unhandledRejection' listener flushed across two microtask turns, that the rejection was never surfaced as unhandled. - logger.error() itself throws when checkAlertConfigured() tries to log — asserts onApplicationBootstrap() does not throw and scheduling proceeds normally, exercising the new call-site boundary guard. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/keeper/keeper.service.spec.ts | 96 +++++++++++++++++++++++ src/modules/keeper/keeper.service.ts | 33 +++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/modules/keeper/keeper.service.spec.ts b/src/modules/keeper/keeper.service.spec.ts index 248db5a..9ba169b 100644 --- a/src/modules/keeper/keeper.service.spec.ts +++ b/src/modules/keeper/keeper.service.spec.ts @@ -604,4 +604,100 @@ describe("KeeperService — startup self-check: alert configured", () => { jest.useRealTimers(); } }); + + it( + "opsAlert.isEnabled() returns a REJECTED Promise ⇒ the rejection is consumed " + + "(no unhandledRejection), and the keeper still schedules and runs a tick", + async () => { + jest.useFakeTimers(); + const unhandled = jest.fn(); + process.on("unhandledRejection", unhandled); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + const rejectingOpsAlert = { + isEnabled: () => Promise.reject(new Error("probe rejected")), + alert: jest.fn(), + } as any; + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, + rejectingOpsAlert + ); + + svc.onApplicationBootstrap(); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect((svc as any).startupTimer).not.toBeNull(); + + // Flush the microtask queue so the rejected promise settles — if the + // rejection were left unconsumed, this is where Node would emit + // `unhandledRejection` (a process crash under Node 15+ defaults). + await Promise.resolve(); + await Promise.resolve(); + expect(unhandled).not.toHaveBeenCalled(); + + await jest.advanceTimersByTimeAsync(0); + expect(updates).toHaveLength(1); + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + } finally { + jest.useRealTimers(); + process.off("unhandledRejection", unhandled); + } + } + ); + + it("logger.error itself throws inside the self-check ⇒ the boundary guard swallows it, keeper still runs", async () => { + jest.useFakeTimers(); + // Reuse the describe-level spy (installed in beforeEach) rather than + // re-spying — just swap its implementation to throw for this one test. + // afterEach's errorSpy.mockRestore() still tears it down as usual. + errorSpy.mockImplementation(() => { + throw new Error("logging backend down"); + }); + try { + const updates: boolean[] = []; + const blockchain = makeBlockchain({ + getPriceInfo: async () => ({ updatedAt: 1000n, threshold: 3600n }), + updatePrice: async () => { + updates.push(true); + return "0xTX"; + }, + }); + const svc = new KeeperService( + blockchain, + makeNotify(), + makeConfig({ keeperIntervalMs: 60_000 }), + NOW_NEAR_EXPIRY, + makeRegistry(), + () => 0, + makeOpsAlert(false) // unconfigured ⇒ checkAlertConfigured() will try to log + ); + + // onApplicationBootstrap must not throw even though the diagnostic logger + // itself throws — this is exactly what the call-site boundary try/catch + // (not a nested catch inside checkAlertConfigured) exists to contain. + expect(() => svc.onApplicationBootstrap()).not.toThrow(); + expect((svc as any).startupTimer).not.toBeNull(); + + await jest.advanceTimersByTimeAsync(0); + expect(updates).toHaveLength(1); + expect((svc as any).timer).not.toBeNull(); + + svc.onApplicationShutdown(); + } finally { + jest.useRealTimers(); + } + }); }); diff --git a/src/modules/keeper/keeper.service.ts b/src/modules/keeper/keeper.service.ts index c3dd6b2..f629202 100644 --- a/src/modules/keeper/keeper.service.ts +++ b/src/modules/keeper/keeper.service.ts @@ -96,7 +96,15 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd ); return; } - this.checkAlertConfigured(); + try { + this.checkAlertConfigured(); + } catch { + // Structural boundary, not a point-fix: whatever escapes checkAlertConfigured() + // (a throwing logger, a throwing diagnostic, anything) is swallowed HERE so it + // is structurally impossible for the self-check to abort scheduling below. + // Intentionally do NOT log here — logging is exactly the kind of thing that + // may have thrown; a nested catch just moves the same question one level down. + } this.lastDayNumber = this.todayNumber(); // Phase-jitter the first tick across [0, intervalMs) so redundant keepers // that boot together don't all fire updatePrice() in the same window. @@ -170,12 +178,31 @@ export class KeeperService implements OnApplicationBootstrap, OnApplicationShutd * documented as synchronous and boolean-returning, so anything else (undefined, * a non-boolean, a Promise from a misbehaving mock/partial implementation, * which is truthy under a loose check) is read as "not configured" rather than - * silently accepted as "probably fine". + * silently accepted as "probably fine". A thenable result is additionally given + * a no-op rejection handler (never awaited — this path must stay synchronous) so + * a mock that returns a *rejected* promise can't surface as an unhandled + * rejection later and crash the process (Node 15+ default behavior). + * + * This method's own try/catch only covers the probe + rejection-consumption; it + * does NOT try to also guarantee the diagnostic logging below can't throw — that + * would just push the same question into a nested catch. Instead the CALLER + * (`onApplicationBootstrap`) wraps the whole `checkAlertConfigured()` call in a + * boundary try/catch that swallows anything, unconditionally. That is the single + * place the "never blocks scheduling" invariant is actually enforced. */ private checkAlertConfigured(): void { let configured: boolean; try { - configured = this.opsAlert?.isEnabled() === true; + // Declared type is `boolean`, but a misbehaving mock can violate that at + // runtime (that's exactly the case this guards against) — widen to + // `unknown` so the shape checks below are honest about what's possible. + const result: unknown = this.opsAlert?.isEnabled(); + // Consume a rejection on a misbehaving async mock so it never becomes an + // unhandled rejection down the line. Never `await` — this must stay sync. + if (result != null && typeof (result as { then?: unknown }).then === "function") { + (result as Promise).then(undefined, () => {}); + } + configured = result === true; } catch (e) { this.logger.error( "Keeper: could not determine whether an operator-alert path is configured " + From 55bce1150021c17c8ba0922d800fc3cc05755593 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:16:05 +0700 Subject: [PATCH 5/5] test(keeper): make the rejected-promise test actually wait past Node's unhandledRejection check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3 Low finding: the rejected-Promise test asserted `expect(unhandled).not.toHaveBeenCalled()` after only two `await Promise.resolve()` turns (microtasks). Node's unhandledRejection bookkeeping settles on a LATER turn of the event loop than the microtask queue, so that assertion passed unconditionally, regardless of whether the rejection was actually consumed — a vacuous check. Fixed by waiting a real macrotask turn instead: `setImmediate` (Node's "check" phase) is reliably later than where Node emits `unhandledRejection` for an unconsumed rejection. Since this test runs under jest fake timers, and modern fake timers fake `setImmediate` too, the real function reference is captured via `globalThis.setImmediate` BEFORE calling `jest.useFakeTimers()` in the same test, so it still points at Node's genuine implementation. Verified this isn't vacuous by mutation: temporarily removed the rejection-consuming `result.then(undefined, () => {})` line from `checkAlertConfigured()` in keeper.service.ts, re-ran only this test, and it failed (Jest attributed the real unconsumed rejection to the test — see the round's report for the exact failure output). Restored the line; the test passes again with no other diff to keeper.service.ts. Only the test file changes in this commit — the mutation was not committed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/keeper/keeper.service.spec.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/modules/keeper/keeper.service.spec.ts b/src/modules/keeper/keeper.service.spec.ts index 9ba169b..820ee20 100644 --- a/src/modules/keeper/keeper.service.spec.ts +++ b/src/modules/keeper/keeper.service.spec.ts @@ -607,8 +607,19 @@ describe("KeeperService — startup self-check: alert configured", () => { it( "opsAlert.isEnabled() returns a REJECTED Promise ⇒ the rejection is consumed " + - "(no unhandledRejection), and the keeper still schedules and runs a tick", + "(no unhandledRejection, even after a real macrotask turn), and the keeper " + + "still schedules and runs a tick", async () => { + // Node's unhandledRejection bookkeeping settles on a LATER turn of the event + // loop than the microtask queue — awaiting only `Promise.resolve()` (a + // microtask) resolves before that point, so a check placed there would pass + // whether or not the rejection was actually consumed (a vacuous assertion). + // Waiting a real `setImmediate` (Node's "check" phase, a macrotask) is + // reliably later than that bookkeeping, so seeing nothing fire by then is a + // real assertion. Capture the REAL setImmediate BEFORE enabling fake + // timers: modern fake timers fake setImmediate too, so a reference taken + // now still points at Node's genuine implementation. + const realSetImmediate = globalThis.setImmediate; jest.useFakeTimers(); const unhandled = jest.fn(); process.on("unhandledRejection", unhandled); @@ -639,11 +650,10 @@ describe("KeeperService — startup self-check: alert configured", () => { expect(errorSpy).toHaveBeenCalledTimes(1); expect((svc as any).startupTimer).not.toBeNull(); - // Flush the microtask queue so the rejected promise settles — if the - // rejection were left unconsumed, this is where Node would emit - // `unhandledRejection` (a process crash under Node 15+ defaults). - await Promise.resolve(); - await Promise.resolve(); + // A real macrotask turn — later than the point where Node would have + // emitted `unhandledRejection` for a rejection nobody attached a handler + // to. + await new Promise(resolve => realSetImmediate(resolve)); expect(unhandled).not.toHaveBeenCalled(); await jest.advanceTimersByTimeAsync(0);