Skip to content

fix(keeper): warn loudly at startup when the keeper is enabled but no operator-alert path is configured [DO NOT MERGE — B6 freeze] - #377

Open
jhfnetboy wants to merge 5 commits into
masterfrom
fix/keeper-warn-when-alert-undeliverable
Open

jhfnetboy wants to merge 5 commits into
masterfrom
fix/keeper-warn-when-alert-undeliverable

Conversation

@jhfnetboy

Copy link
Copy Markdown
Member

⛔ DO NOT MERGE — B6 zero-merge freeze. This repo is under the CC-115 B6 freeze. Opening this PR is allowed; merging is not, until DSR rules on the merge boundary (asked in CC-122). Tracked there.

Why

A sibling repo's price keeper failed silently for ~3.5 weeks: launchd KeepAlive kept the process alive, every tick failed (8,455 403 App is inactive, 0 successful updatePrice() calls), and its alert channel was also broken (22,497 [telegram] sendMessage failed: Unauthorized). Process alive + work failing + alarm broken is invisible from every angle.

This repo's KeeperService has the same latent shape. It does alert on failure:

this.opsAlert?.alert("critical", `keeper updatePrice failed for ${paymaster}: ${msg}`);

but OpsAlertService is @Optional()-injected and called through ?., and deploy/.env.*.example carry no OPS_ALERT_* at all. With alerting unconfigured, that line is a silent no-op. The author has asked for this keeper to be enabled as a second keeper in the SP 5.5.0 upgrade window (CC-122), so the gap stops being theoretical.

What

A startup self-check: when the keeper is enabled and has paymaster targets, but no operator-alert path is configured, it logs an ERROR saying so.

Scope, stated precisely — this checks CONFIGURATION, not deliverability. It is judged solely by OpsAlertService.isEnabled() (opt-in flag AND a transport set), the single place alert() itself uses to decide whether to no-op — so the two cannot drift. It does not probe the transport: a configured-but-dead token (the actual failure mode of the incident above) still passes. Only a real alert drill before enabling proves delivery. The log text and docstring say this explicitly. It also concerns only the OpsAlertService path; the separate notificationService.sendToAccount(...) path is untouched and unaffected.

It never stops the keeper. Halting on an alerting gap would guarantee the stale price the keeper exists to prevent. The safe state is keep working and say so loudly. This is enforced structurally: the call site is wrapped in a boundary whose catch deliberately does nothing (not even log — logging is exactly what may have thrown), so nothing inside the self-check can abort scheduling.

Ordering: the check runs after the empty-KEEPER_PAYMASTER_ADDRESS guard, so it is silent when the keeper won't actually run.

Review record

Written by a cheaper model, reviewed by the planner, then Codex (Tier 1) adversarial review — 3 rounds:

Round Verdict Findings
1 REQUEST_CHANGES High: isEnabled() could throw before timers are armed (partial mock / throwing getter), blocking the keeper. Low: log over-claimed "nobody will be notified" — notificationService still fires.
2 REQUEST_CHANGES Round-1 Low fixed. High ×2: a rejected Promise from isEnabled() went unconsumed (unhandled rejection can crash Node 15+); the diagnostic path (String(e), logger.error) could itself throw. Resolved with the call-site boundary + consuming thenable rejections — enforcing the invariant at the boundary rather than chasing each internal throw, which ends the "what if the handler throws" regress.
3 APPROVE One Low: the rejected-Promise test asserted after only two microtask flushes, but Node emits unhandledRejection on a later event-loop turn — the assertion passed regardless of correctness.

The round-3 Low was fixed by waiting a real macrotask turn (a setImmediate reference captured before fake timers are installed), and mutation-verified: with the rejection-consuming line removed the test fails (Jest attributes the real unconsumed rejection to it); with it restored the test passes. The mutation is not committed.

Earlier, before Codex, the planner's own review sent back a naming over-claim: the check was first called checkAlertDeliverability, which a dead-but-configured token would have passed — the very incident it cites.

Tests

src/modules/keeper/keeper.service.spec.ts — 40 passing across keeper + ops-alert. Every error-path test asserts not only that the error was logged but that the timer was armed and a tick actually ran updatePrice(), so none can pass against an implementation that blocks:

  • enabled + configured ⇒ silent · enabled + unconfigured ⇒ error · OpsAlertService not injected ⇒ error · keeper disabled ⇒ silent · empty paymaster list ⇒ only the pre-existing "disabled" warn
  • isEnabled missing / throwing / returning a Promise / returning a rejected Promise (no unhandledRejection) ⇒ error, keeper runs
  • logger.error itself throws ⇒ keeper runs

npx tsc --noEmit clean · npm run lint:check 0 errors (20 pre-existing warnings in unrelated files, unchanged).

Not in this PR (needed before the keeper is actually enabled — see CC-122)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs

jhfnetboy and others added 5 commits September 27, 2026 13:56
…s alerts cannot be delivered

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…verable", and move it past the paymaster guard

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…aiming what it checks

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…-check

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…s unhandledRejection check

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs

@clestons clestons left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — 启动自检本身设计正确,「绝不阻塞 keeper」的结构性保证经真实验证成立。

我自己做的验证(不只是信任已披露的 3 轮 Codex 对抗)

核心不变式:checkAlertConfigured() 里发生的任何事都到不了外面。
onApplicationBootstrap() 用一个外层 try { this.checkAlertConfigured(); } catch {}(keeper.service.ts:99-107)整个包住调用,且刻意不在 catch 里 log("logging 本身可能就是抛出的源头")。跑了完整测试套件(26 个全过,含 isEnabled() 同步抛出 / 返回 rejected Promise / logger.error 本身抛出 三种真实场景,每条都断言 startupTimer 非空且 updatePrice() 真的跑了一次)。

反向变异验证了 Round 3 披露的那条最精细的测试:PR 说"rejection-consuming 那一行的变异测试没有提交",我自己动手做了——把 checkAlertConfigured() 里消费 rejection 的 (result as Promise<unknown>).then(undefined, () => {}) 删掉,重跑 -t "REJECTED Promise":测试正确变红,Jest 把真实的未消费 rejection 精确 attribute 到这条测试上,报错信息、堆栈都和 PR 描述的一致。还原后确认 git status 干净。

核对了"isEnabled() 是 alert() 唯一用来判断是否 no-op 的地方"这个信任基石:直接读了 ops-alert.service.ts 源码——alert() 第一行就是 if (!this.enabled) return;,isEnabled() 原样返回同一个 this.enabled 字段,该字段只在构造函数里算一次(opt-in flag && (hasTelegram || hasWebhook))。两者物理上不可能走岔,这个检查确实不会"我看着都配置好了,实际 alert() 还是在 no-op"。

核对了"只查配置,不探测可达性"的披露是真的:isEnabled() 全程只读 ConfigService,没有任何网络调用——一个被吊销的 Telegram token 依然会通过这个检查,PR 自己的文档注释也这么写了,属实。

顺序核实:diff 的 hunk context 显示新 try/catch 紧跟在既有的空 KEEPER_PAYMASTER_ADDRESS 守卫的 return; 之后插入,符合"keeper 实际不会跑时这条检查保持沉默"的描述。

R1a/R1b

GATE 全干净(无 security/concurrency/hidden-state 信号),R1a 无发现;R1b 报了一条 Low(日志里带 OPS_ALERT_ENABLED 等环境变量名"暴露配置细节")——驳回:这是给运维看的配置指引,环境变量名不是密钥,日志的目的就是让人照着这行配起来,这正是这条 PR 存在的理由。R1a 另提了一句"检查 onApplicationBootstrap 有没有别的调用方会不同地观察到被吞掉的异常"——grep 了整个仓库,这是 NestJS 生命周期接口,7 个服务各自独立实现,互不调用,无交互问题。

CI

Security Audit 和聚合门禁 CI Success 报红,但与本 PR 无关:npm audit 命中的是 @nestjs/platform-express/multer 两条已知的 High 级依赖告警(本仓库既有的审计欠账,本 PR 没有碰 package.json/lockfile,gh pr diff --name-only 只有 keeper.service.ts 和它的 spec)。Tests/Build/Type Check/Code Quality/Smart Contracts/CodeQL 全绿。

pre-pr-check

--base origin/master:0 block / 1 review(B1,断言词注释密度——checkAlertConfigured() 文档确实写得很细,逐条对应下面的真实测试,合理)。

备注

本仓库处于 CC-115 B6 零合并冻结,此 PR 头部已自带 "DO NOT MERGE" 横幅,等 DSR 就合并边界拍板(CC-122)。这条备注不影响本次 review 结论——本项目从不合并任何 PR,此说明纯粹为了完整记录。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants