fix(keeper): wire health monitors to real outcomes instead of placeholder data - #369
fix(keeper): wire health monitors to real outcomes instead of placeholder data#369Morenikeoa wants to merge 1 commit into
Conversation
…lder data
/health's monitors.{rpc,scan,oracle,db} sub-objects look like genuine
connectivity probes, but recordSuccess()/recordFailure() were never called
anywhere in the codebase -- confirmed by repo-wide grep. lastSuccessTime is
set once at construction and consecutiveFailures stays 0 forever, so
getStatus().healthy is permanently true regardless of actual RPC/DB/oracle
connectivity. Anyone building dashboards or alert rules off
health.monitors.rpc.healthy etc. is reading fabricated-looking "all good"
telemetry that can never go false.
Moved `monitors` out of index.ts into a new src/lib/service-monitors.ts (so
crank.ts and oracle.ts can wire it without a circular import on index.ts) and
recorded real outcomes at the four monitors' most natural existing call
sites:
- rpc: the periodic SOL-balance getBalance() check (index.ts)
- scan: the periodic discover()+crankAll() cycle completing or throwing
(crank.ts)
- oracle: the DexScreener/Jupiter external price fetches' HTTP/network
outcome (oracle.ts)
- db: the single Supabase market-metadata query (crank.ts)
Each existing test file mocking @percolatorct/shared without spreading the
real module needed createServiceMonitors added to its mock (the new module
calls it at import time) -- updated 8 affected test files.
BUG-110 from a clean-room Phase 4 audit pass.
📝 WalkthroughWalkthroughThe PR adds a shared ChangesService Monitor Instrumentation
Sequence Diagram(s)sequenceDiagram
participant KeeperInterval as Keeper SOL-balance interval
participant CrankService as CrankService
participant OracleService as OracleService
participant Monitors as monitors
KeeperInterval->>Monitors: rpc.recordSuccess() after getBalance
KeeperInterval->>Monitors: rpc.recordFailure(error.message) on catch
CrankService->>Monitors: db.recordSuccess() after Supabase query
CrankService->>Monitors: db.recordFailure(error.message) on Supabase error or throw
CrankService->>Monitors: scan.recordSuccess() after a completed cycle
CrankService->>Monitors: scan.recordFailure(error.message) on cycle throw
OracleService->>Monitors: oracle.recordSuccess() after DexScreener/Jupiter HTTP 200
OracleService->>Monitors: oracle.recordFailure(status or error.message) on HTTP error or catch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/services/oracle-deviation.test.ts (1)
37-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the service-monitor stub into shared test scaffolding.
This
makeMonitor/createServiceMonitorsmock is now duplicated across the oracle and crank suites. Centralizing it in one test helper would reduce drift the next time theServiceMonitorcontract changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/oracle-deviation.test.ts` around lines 37 - 71, The service-monitor mock setup is duplicated in the oracle test suite and should be moved into shared test scaffolding. Extract the `makeMonitor` helper and the `createServiceMonitors` mock from this `vi.mock('`@percolatorct/shared`', ...)` block into a reusable test helper used by both oracle and crank tests. Keep the helper aligned with the `ServiceMonitor` contract so updates only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 30-31: The `/health` payload still omits the new DB lane, so
`monitors.db` is exposed but never serialized. Update the health response
builder where `rpc`, `scan`, and `oracle` are collected to also include the DB
monitor from `monitors` (the same object re-exported in `src/index.ts`), and
ensure the `/health` JSON reflects the DB status alongside the other lanes.
In `@src/services/crank.ts`:
- Around line 2074-2079: The scan monitor success path in crank.ts is
incorrectly tied only to the absence of a thrown exception, so a cycle with
logged per-program failures can still call monitors.scan.recordSuccess(). Update
the discover()/crankAll() flow to return or propagate an explicit cycle result
that reflects whether any scan failed, then use that result in the Crank cycle
handling block before calling recordSuccess or recordFailure. Keep the existing
logger.error and monitors.scan methods, but make the success decision based on
the explicit outcome instead of the try/catch alone.
In `@src/services/oracle.ts`:
- Around line 188-195: The `monitors.oracle` updates in `fetchPrice()` and
`peekPrice()` are racing because both the DexScreener and Jupiter branches call
`recordSuccess()`/`recordFailure()` independently, so the last finisher
overwrites the health state. Refactor the oracle health reporting so it is
written once after both upstream requests settle, or route DexScreener and
Jupiter to separate monitor lanes; use the `monitors.oracle` calls in
`fetchPrice`, `peekPrice`, and any shared upstream helper logic to keep the
health status deterministic.
---
Nitpick comments:
In `@tests/services/oracle-deviation.test.ts`:
- Around line 37-71: The service-monitor mock setup is duplicated in the oracle
test suite and should be moved into shared test scaffolding. Extract the
`makeMonitor` helper and the `createServiceMonitors` mock from this
`vi.mock('`@percolatorct/shared`', ...)` block into a reusable test helper used by
both oracle and crank tests. Keep the helper aligned with the `ServiceMonitor`
contract so updates only need to be made in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d8f9c5-6da8-404b-8a16-4cf4725ce543
📒 Files selected for processing (14)
src/index.tssrc/lib/service-monitors.tssrc/services/crank.tssrc/services/oracle.tstests/services/crank-error-code.poc.test.tstests/services/crank-hyperp-detection.poc.test.tstests/services/crank.b-fixes.test.tstests/services/crank.processBatched.test.tstests/services/crank.test.tstests/services/oracle-deviation.test.tstests/services/oracle-stale.test.tstests/services/oracle.b-fixes.test.tstests/services/oracle.test.tstests/services/program-id-allowlist.test.ts
| // Monitoring — alerts to Discord on threshold breaches (see src/lib/service-monitors.ts) | ||
| export { monitors }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose the new db lane in /health.
This re-export makes monitors.db available, but Lines 658-662 still serialize only rpc, scan, and oracle. The DB monitor updates added in src/services/crank.ts never reach /health, so DB outages remain invisible despite this PR’s stated goal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 30 - 31, The `/health` payload still omits the new
DB lane, so `monitors.db` is exposed but never serialized. Update the health
response builder where `rpc`, `scan`, and `oracle` are collected to also include
the DB monitor from `monitors` (the same object re-exported in `src/index.ts`),
and ensure the `/health` JSON reflects the DB status alongside the other lanes.
| // BUG-110: the cycle (discovery + crank pass) completed without | ||
| // throwing — record so /health's monitors.scan reflects real outcomes. | ||
| monitors.scan.recordSuccess().catch(() => {}); | ||
| } catch (err) { | ||
| logger.error("Crank cycle failed", { error: err instanceof Error ? err.message : String(err), stack: err instanceof Error ? err.stack : undefined }); | ||
| monitors.scan.recordFailure(err instanceof Error ? err.message : String(err)).catch(() => {}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don’t treat “didn’t throw” as a successful scan cycle.
Lines 1122-1145 and 1157-1168 inside discover() log per-program scan failures and keep going, so this block can still hit recordSuccess() even when discovery actually failed across the cycle. That leaves /health.monitors.scan green during real scan outages. Base the scan monitor on an explicit cycle result from discover()/crankAll(), not just the absence of an exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/crank.ts` around lines 2074 - 2079, The scan monitor success
path in crank.ts is incorrectly tied only to the absence of a thrown exception,
so a cycle with logged per-program failures can still call
monitors.scan.recordSuccess(). Update the discover()/crankAll() flow to return
or propagate an explicit cycle result that reflects whether any scan failed,
then use that result in the Crank cycle handling block before calling
recordSuccess or recordFailure. Keep the existing logger.error and monitors.scan
methods, but make the success decision based on the explicit outcome instead of
the try/catch alone.
| // BUG-110: record real connectivity outcomes so /health's monitors.oracle | ||
| // reflects whether the external price feeds are actually reachable. | ||
| if (!res.ok) { | ||
| monitors.oracle.recordFailure(`DexScreener HTTP ${res.status}`).catch(() => {}); | ||
| return null; | ||
| } | ||
| monitors.oracle.recordSuccess().catch(() => {}); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid racing two upstreams into one oracle monitor.
fetchPrice() / peekPrice() run DexScreener and Jupiter in parallel, but these branches all mutate the same monitors.oracle lane. If one source fails and the other succeeds, whichever finishes last wins, so /health.monitors.oracle becomes nondeterministic and can hide a degraded single-source state. Record the monitor once after both requests settle, or split the sources into separate lanes.
Also applies to: 228-228, 262-268, 284-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/oracle.ts` around lines 188 - 195, The `monitors.oracle` updates
in `fetchPrice()` and `peekPrice()` are racing because both the DexScreener and
Jupiter branches call `recordSuccess()`/`recordFailure()` independently, so the
last finisher overwrites the health state. Refactor the oracle health reporting
so it is written once after both upstream requests settle, or route DexScreener
and Jupiter to separate monitor lanes; use the `monitors.oracle` calls in
`fetchPrice`, `peekPrice`, and any shared upstream helper logic to keep the
health status deterministic.
|
Independent verification — not an approval (QA/Security own that). Verdict: the branch is stale, not broken. One I'm writing this up carefully because my first reading was wrong, and posting it would have been an unfair accusation. What I first sawRead naively that's "this PR breaks a risk-parameter safety check" — a serious claim about a health-monitoring PR. What's actually going on
main gained So the failure is a stale-branch artefact, not a regression. VerifiedJust merge On the change itselfWiring the health monitors to real outcomes rather than placeholder data is squarely the right direction — I filed and fixed a sibling instance of exactly this in #392, where the v17 conservation invariant reported One thing I checked and want to note approvingly: |
2 participants
Problem
/health'smonitors.{rpc,scan,oracle,db}sub-objects (src/index.ts, backed by@percolatorct/shared'sServiceMonitor) look like genuine connectivity probes — named exactly that way — butrecordSuccess()/recordFailure()were never called anywhere in the codebase. Confirmed via repo-wide grep: the onlyrecordSuccess/recordFailurecalls in the whole repo belonged to an unrelated tracker inrpc-pool.ts.lastSuccessTimeis set once at construction andconsecutiveFailuresstays at0forever, sogetStatus().healthyis permanentlytrueregardless of actual RPC/DB/oracle connectivity, andtimeSinceSuccessMsgrows unbounded without ever being checked against a staleness threshold.Production Impact
Anyone building dashboards or alert rules off
health.monitors.rpc.healthy(etc.) is reading fabricated-looking "all good" telemetry that can never go false — even while the keeper's actual RPC/DB/oracle connectivity is genuinely broken in logs.Fix
Moved
monitorsout ofindex.tsinto a newsrc/lib/service-monitors.ts(socrank.tsandoracle.tscan wire it without creating a circular import onindex.ts), and recorded real outcomes at each monitor's most natural existing call site:rpc— the periodic SOL-balancegetBalance()check (index.ts), a genuine recurring RPC round trip.scan— the periodicdiscover()+crankAll()cycle (crank.ts) completing cleanly vs. throwing.oracle— the DexScreener/Jupiter external price fetches' HTTP/network outcome (oracle.ts) — both fetch functions, both the!res.okbranch and the catch block.db— the single Supabase market-metadata query (crank.ts'sdiscoverMarkets), both theerrorbranch and the catch block.Proof of Fix
New tests:
tests/services/oracle.test.ts: recordsmonitors.oraclesuccess on a reachable fetch, failure on a network error, failure on a non-ok HTTP response (with the status code in the message).tests/services/crank.test.ts: recordsmonitors.dbsuccess on a clean Supabase query, failure on a Supabase-level error, failure when the Supabase call throws; recordsmonitors.scansuccess after a clean periodic cycle.Every existing test file that mocks
@percolatorct/sharedwithout spreading the real module neededcreateServiceMonitorsadded to its mock, since the newservice-monitors.tsmodule calls it at import time — updated 8 affected test files (crank-error-code.poc.test.ts,crank-hyperp-detection.poc.test.ts,crank.b-fixes.test.ts,crank.processBatched.test.ts,program-id-allowlist.test.ts,oracle.b-fixes.test.ts,oracle-stale.test.ts,oracle-deviation.test.ts) plusoracle.test.ts/crank.test.tsdirectly.Test Output
The one failure (
tests/v17-risk-params.poc.test.ts) is pre-existing and unrelated — a stale assertion from commit8ee810d(#345), out of scope here.pnpm build— clean, zero errors.Summary by CodeRabbit
New Features
Bug Fixes