fix(alerts): evaluateAll() no longer redundantly re-fetches every alert per asset - #139
Merged
prodbycorne merged 7 commits intoAug 20, 2026
Conversation
… loop Pulls the trigger/cooldown/fire/persist logic into evaluateAlertList(alerts, priceUsd), operating on an already-fetched array. evaluateForAsset now builds a matching-alerts array first, then delegates to it. Pure extraction — evaluateForAsset's own fetch strategy and evaluateAll's per-asset calls are unchanged, so behavior is identical; this just gives the next commits a shared function to call instead of duplicating the eval logic.
…e.all Matches list()'s existing pattern instead of a sequential await-per-id loop. Order is preserved (Promise.all resolves in input order), so which alerts match and in what order evaluateAlertList sees them is unchanged — this is strictly a round-trip-count improvement to evaluateForAsset's own fetch, on top of the extraction in the previous commit.
…tead of re-fetching per asset This is the actual fix for SmartDropLabs#132. evaluateAll no longer calls evaluateForAsset once per distinct asset (which independently re-queried Redis for the full alert list every time — O(assets * alerts) round-trips). It now groups the alerts it already fetched via list() into a Map<asset, Alert[]> and calls evaluateAlertList directly per group, so a full cycle costs one alerts fetch plus one price lookup per distinct asset, regardless of alert count.
…nct assets Regression test for the O(assets * alerts) redundant re-fetch: with 3 distinct assets and 2 alerts each, asserts zrevrange is called exactly once (from list()) and cache.get exactly (alerts + assets) times, never assets * alerts. Verified this fails against the pre-fix evaluateAll (4 zrevrange calls, one per asset plus list()) and passes against the fix.
… re-reading per asset Per the issue's own requirement: if a fresh per-asset Redis read were intentionally desired for a correctness reason, that reasoning should be a code comment; otherwise the redundant re-fetch should go. There isn't one — an alert created mid-cycle is simply picked up on the next 30s cycle either way — so this documents that explicitly rather than leaving it implicit.
…rough evaluateAll Two correctness checks for the new in-memory grouping: an alert only fires off its own asset's price (not a different asset evaluated in the same cycle), and non-repeat removal / repeat last_fired_at persistence both work correctly when multiple assets are processed in one evaluateAll() call — this code path wasn't previously exercised directly (only via evaluateForAsset).
…martDropLabs#132 Per the issue's 'additional edge cases' section: fetch-once-and-group still pulls every alert into the process each cycle. A per-asset Redis index would scope evaluateAll to only alerts for assets whose price changed, but that's a larger design change than this issue's acceptance criteria call for (eliminating the redundant per-asset re-fetch) — documenting it as a follow-up rather than expanding this fix's scope.
Contributor
|
ci passed |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
alertsService.evaluateAll()already fetches and parses every alert once vialist()purely to compute the distinct set of watched assets — then calledevaluateForAsset(asset, price)once per distinct asset, which independently re-fetched the full ID list and re-fetched/re-parsed every single alert from Redis again, discarding everything that didn't match. ForAdistinct assets andMtotal alerts, one cycle performed on the order ofA × Msequential Redis round-trips instead ofO(M).evaluateAll()now groups the alerts it already has fromlist()into aMap<asset, Alert[]>in memory and evaluates each group directly, so a full cycle costs exactly one alerts fetch plus one price lookup per distinct asset — never more Redis round-trips than there are alerts + assets.evaluateForAsset()itself is unchanged in behavior (still does a fresh Redis read when called directly — see the code comment on why that's intentional), just with its own id→alert fetch batched viaPromise.allinstead of a sequential loop.refactor: extract the trigger/cooldown/fire/persist logic intoevaluateAlertList(alerts, priceUsd), operating on an already-fetched arrayperf: batchevaluateForAsset's own id→alert fetch viaPromise.allfix: the actual fix —evaluateAll()groups its singlelist()fetch by asset in memory instead of callingevaluateForAssetper assettest: regression test assertingevaluateAll()'s Redis call count doesn't scale with distinct asset count (verified it fails against the pre-fix code: 4zrevrangecalls instead of 1)docs: explicit code comment on whyevaluateAll()takes one snapshot rather than re-reading per asset (no correctness reason — an alert created mid-cycle is picked up next cycle either way)test: multi-asset isolation + remove/persist correctness through the new groupedevaluateAll()code pathdocs: note a secondary-index follow-up direction (per the issue's "additional edge cases") as a documented out-of-scope improvement, not implemented hereAcceptance criteria
evaluateAll()performs exactly one full alerts fetch (list()) per cycle, not one per distinct assetdistinct_assets × total_alertscache.get/zrevrangecalls don't grow multiplicatively with asset counttest/alerts.test.jsbehavior (which alerts fire, cooldown, non-repeat removal) is unchanged — all 22 original tests pass unmodifiedTest plan
npx jest test/alerts.test.js— 25/25 passing (22 original + 3 new)npx jest— 373/373 passing across all 39 suites, run against a local Redis matching CI's service containernpx @redocly/cli lint openapi.yaml— passes (untouched by this change)evaluateAll(4zrevrangecalls for 3 assets) and passes against the fix (1 call)Closes #132