alertsService.evaluateAll()/evaluateForAsset() perform O(assets × alerts) redundant sequential Redis round-trips every price-refresh cycle

[prodbycorne]

Overview

alertsService.evaluateAll() — invoked once per price-refresh cycle (default every 30 seconds, per PRICE_REFRESH_INTERVAL_SECONDS) — performs a full, unbatched, sequential Redis round-trip per alert for every distinct watched asset, redundantly re-fetching and re-scanning the entire alerts collection once per asset, rather than grouping the already-fetched alert list by asset in memory a single time.

async function evaluateAll() {
  const allAlerts = await list();                       // fetches + parses EVERY alert, once
  const assets = [...new Set(allAlerts.map((a) => a.asset))];

  for (const asset of assets) {
    const cached = await cache.get(`price:${asset}`);
    if (!cached || cached.price == null) continue;
    await evaluateForAsset(asset, cached.price);          // re-fetches + re-scans EVERY alert, AGAIN, per asset
  }
}

async function evaluateForAsset(asset, priceUsd) {
  const redis = cache.getClient();
  const ids = await redis.zrevrange(IDS_KEY, 0, -1);       // ALL alert ids, every asset, every call

  for (const id of ids) {
    const alert = await cache.get(alertKey(id));           // one sequential, unbatched await per alert id
    if (!alert || alert.asset !== asset.toUpperCase()) continue;
    ...
  }
}

evaluateAll() already does the work of fetching and parsing every alert exactly once, via list(), purely to compute the distinct set of watched assets — then, for each of those N distinct assets, calls evaluateForAsset(asset, price), which independently re-fetches the full ID list via ZREVRANGE and re-fetches + re-parses every single alert record from Redis, one at a time, sequentially (await cache.get(...) inside a plain for...of loop, no Promise.all/pipelining), discarding everything that doesn't match the current asset.

For a deployment with A distinct watched assets and M total configured alerts, one full evaluateAll() cycle performs on the order of A × M sequential Redis GET round-trips (plus A redundant ZREVRANGE calls), when the already-available allAlerts array from the initial list() call could simply be filtered/grouped by asset in memory, once, entirely eliminating the need for evaluateForAsset to touch Redis again at all beyond what's needed to actually fire/update a matching alert. At a modest scale — say 50 watched assets and 2,000 configured alerts, plausible for any moderately active deployment — this is 100,000 sequential Redis round-trips every 30 seconds, purely for evaluation bookkeeping that already had all the data it needed in memory after the first list() call. Each of those round-trips is await-ed one at a time rather than batched (no MGET, no Promise.all, no Redis pipeline), so the wall-clock cost compounds directly with network round-trip latency rather than being amortized — a cycle that could complete in a handful of batched round-trips instead takes as many sequential round-trips as there are (asset, alert) pairs to check.

This directly threatens the correctness of the whole cron cycle under load: priceRefresh.js's cron callback awaits alertsService.evaluateAll() before moving on to subscriptionManager.notifyPriceUpdates(freshPrices), and if a bloated evaluation cycle ever takes longer than PRICE_REFRESH_INTERVAL_SECONDS, it directly feeds into the overlapping-cron-cycle race condition described in already-open issue #71 — this issue is a very plausible, concrete mechanism by which that theoretical overlap risk becomes a practical one as the number of configured alerts grows.

Requirements

• Refactor evaluateAll()/evaluateForAsset() so the alert list is fetched and parsed from Redis exactly once per cycle, then grouped by asset in memory (e.g. a Map<asset, Alert[]> built from the single list() call), with evaluateForAsset-equivalent logic operating purely against that in-memory grouping rather than re-querying Redis per asset.
• If per-asset re-evaluation from a fresh Redis read is intentionally desired for some correctness reason (e.g. to pick up alerts created concurrently mid-cycle) — which does not appear to be the current design's actual intent, given evaluateAll() already computes its asset list from a single upfront snapshot — that reasoning should be made explicit in a code comment; otherwise, eliminate the redundant re-fetch.
• Where per-alert Redis access remains necessary (e.g. writing last_fired_at back after a repeat alert fires, or removing a non-repeat alert), keep it, but scope it to only the alerts actually being acted on, not to a full re-scan of every alert per asset.

Acceptance Criteria

• evaluateAll() performs exactly one full alerts fetch (list() or equivalent) per cycle, not one per distinct asset.
• The number of Redis round-trips performed by a full evaluateAll() cycle scales with the number of matching (alert, asset) pairs actually requiring action, not with distinct_assets × total_alerts.
• A test with multiple distinct watched assets and a mocked Redis client asserts the total number of cache.get/redis.zrevrange-equivalent calls during evaluateAll() does not grow multiplicatively with the number of assets.
• Existing test/alerts.test.js behavior (which alerts fire, cooldown handling, non-repeat removal) is unchanged — this is a performance refactor, not a behavior change.

Additional Notes

More precise references

• src/services/alerts.js:129-138 (evaluateAll): confirmed const allAlerts = await list(); followed by const assets = [...new Set(allAlerts.map((a) => a.asset))]; — confirming the full alert set genuinely is already fetched and available in memory at this point.
• src/services/alerts.js:103-127 (evaluateForAsset): confirmed const ids = await redis.zrevrange(IDS_KEY, 0, -1); (line 105) followed by a for (const id of ids) { const alert = await cache.get(alertKey(id)); ... } loop (lines 107-108) — confirming both the redundant full-ID-list re-fetch and the sequential, unbatched, per-alert cache.get calls.
• src/services/alerts.js:59-64 (list): confirmed this is the same underlying data (redis.zrevrange(IDS_KEY, 0, -1) + Promise.all(ids.map((id) => cache.get(alertKey(id))))) evaluateAll() already fetches upfront — notably, list() itself does batch its per-alert fetches via Promise.all, unlike evaluateForAsset()'s sequential loop, making the redundancy in evaluateForAsset even more wasteful by comparison (it re-does the same work, less efficiently).
• src/jobs/priceRefresh.js:22-30: confirmed the cron callback does await priceOracle.refreshAllCachedPrices(); await alertsService.evaluateAll(); sequentially, meaning evaluateAll()'s duration is directly additive to the total cycle time this job is measured/health-checked against (priceRefresh.js's own getHealth() stall detection is based on lastSuccessAt, which is only set after both calls complete).

Additional edge cases

• The fix should preserve exact current behavior for repeat vs. non-repeat alerts, cooldown timing (COOLDOWN_MS), and the order alerts are evaluated in if that order matters anywhere (it doesn't appear to, based on the current code, but worth confirming no hidden ordering dependency exists before refactoring).
• Worth checking whether a secondary Redis index (e.g. a alerts:by_asset:{asset} Set, maintained incrementally on create/remove) would be an even better long-term fix than "fetch once, group in memory" for very large alert counts, since "fetch once, group in memory" still requires pulling every alert into the process on every cycle — an index would let evaluateForAsset-equivalent logic only ever touch the alerts relevant to assets whose price actually changed this cycle. That's a larger design change; the acceptance criteria above only require eliminating the redundant per-asset re-fetch, which is the concretely measured A× multiplier this issue is about, but worth raising as a follow-up direction in the PR discussion.

Test/reproduction plan

const redisSpy = jest.spyOn(cache.getClient(), 'zrevrange');
const cacheGetSpy = jest.spyOn(cache, 'get');
// seed 5 distinct assets, 200 total alerts spread across them, and matching price: cache entries
await alertsService.evaluateAll();
// Currently: redisSpy called 6 times (1 from list() + 5 from evaluateForAsset, one per asset),
// cacheGetSpy called on the order of 5 × 200 = 1000 times for alert records alone.
// After fix: redisSpy called once total; cacheGetSpy called ~200 times total (once per alert, not per asset×alert).

Cross-references

• Scheduled price-refresh cron has no reentrancy guard — slow cycles overlap and race on the cache #71 (open) — "Scheduled price-refresh cron has no reentrancy guard — slow cycles overlap and race on the cache." This issue is a concrete, quantifiable mechanism by which a cycle's duration grows unboundedly with alert count, directly increasing the likelihood of triggering Scheduled price-refresh cron has no reentrancy guard — slow cycles overlap and race on the cache #71's overlap scenario in any deployment with a non-trivial number of configured alerts.
• change_pct price alerts never rebase their baseline — repeat alerts fire every cooldown window indefinitely once triggered #74 (open) — "change_pct price alerts never rebase their baseline — repeat alerts fire every cooldown window indefinitely once triggered." Same file/function neighborhood (alertsService); worth a shared review pass.
• Price cache-miss stampede: concurrent requests for the same asset all hit CoinGecko/CoinMarketCap simultaneously #69 (open) — "Price cache-miss stampede." Different mechanism, same general theme of price-refresh-cycle-triggered Redis load; worth reviewing together for anyone doing a broader price-refresh-cycle performance pass.

[davidishere1]

I’m interested in working on this because the current implementation creates unnecessary Redis overhead by repeatedly fetching and scanning the full alerts collection for each watched asset. I’d approach the fix by fetching the alerts once, grouping them by asset in memory, and then evaluating each group without redundant Redis round-trips.

I have 5+ years of programming experience, with a strong focus on building efficient, reliable, and maintainable software. I’m comfortable analyzing performance bottlenecks, working with backend systems and Redis, and making targeted improvements without changing existing behavior. I’d be excited to contribute a clean, well-tested solution to this issue.

[grantfox-oss]

🎉 This issue has been marked as completed on GrantFox as part of the Third Campaign campaign!

@davidishere1's PR #139 was approved and merged by @prodbycorne.

🏆 @davidishere1: You earned 35 FoxPoints for this contribution! Your current tier: Explorer (48 total points). Track your full progress on GrantFox.

👏 Great work, @davidishere1! Keep contributing to SmartDropLabs.