fix: circuit-break nonRetryable price source failures instead of retrying forever - #102
Merged
prodbycorne merged 10 commits intoJul 17, 2026
Conversation
err.nonRetryable was set on CoinMarketCap 401s but never consulted anywhere — a revoked/misconfigured API key was retried on every price fetch and every scheduled refresh cycle forever, logged identically to a one-off transient blip (SmartDropLabs#95). Adds a small circuit-breaker factory shared by any price source with a permanent-failure mode: open() trips the circuit and logs once at error level (not on every subsequent skip), noteSkipped() logs at most one reminder per configurable interval while open, close() resets on a successful fetch, and getState() exposes open/openUntil for observability. State is intentionally process-local — see the module's doc comment for why that's an acceptable tradeoff here.
Config for the circuit breaker's cooldown window (default 15 minutes) and its "still open" reminder-log cadence (default 5 minutes), under a new priceSources namespace.
fetchPrice already set err.nonRetryable = true on a 401 and threw,
but nothing downstream ever read it (priceOracle.js's
fetchFromAllSources catches every error identically). Rather than
pushing nonRetryable-awareness up into the generic orchestrator
(which shouldn't need to know per-source retry policy), the circuit
check is entirely internal to this module:
- Before attempting a fetch, skip it if the circuit is open
(returns null immediately, no HTTP call, no throw).
- On a 401, open the circuit for circuitCooldownMs and still throw
(unchanged behavior for any caller relying on the existing throw).
- On any successful HTTP round-trip, close the circuit — a 401
means the key was bad *at that moment*, and an operator fixing it
should not require a process restart to clear the circuit.
- 429s and other transient errors are untouched — they already
self-heal on the next cycle and this issue explicitly scopes the
circuit breaker to the nonRetryable (401) case only.
priceOracle.js needs no changes at all under this design.
Per SmartDropLabs#95's own suggestion, checked CoinGecko's actual API behavior: their docs confirm 401 means a missing/invalid API key (403 is a separate CDN/firewall-block category, 429 is rate-limiting — neither indicates a bad key). coingecko.js previously had no 401 handling at all; it now mirrors coinmarketcap.js's treatment exactly — same circuit breaker pattern, same throw-with-nonRetryable-flag on 401, 429/other errors unaffected.
Aggregates circuit-breaker state from every source that has one (currently coingecko and coinmarketcap — stellar_dex has no API-key auth-failure mode) so operators can see at a glance which price sources are currently circuit-broken, per SmartDropLabs#95's observability requirement.
Adds price_source_circuits to the /health response body (and its OpenAPI schema) using priceOracle.getSourceCircuitStates() — the lowest-effort of the observability options SmartDropLabs#95 names (health response / metrics / logs) given SmartDropLabs#92 (health depth) and SmartDropLabs#93 (metrics) haven't landed yet.
Circuit lifecycle: starts closed, open() trips and logs once at error level, repeated open() calls while open don't re-log, stays open until cooldownMs elapses, close() resets immediately, noteSkipped() logs at most once per reminderIntervalMs, and a fresh failure after cooldown re-opens with a new window and a new log line.
…ptance criteria) Covers every acceptance criterion from SmartDropLabs#95: a 401 opens the circuit and the very next fetch skips the HTTP call entirely; the source is retried after the cooldown window; a successful retry closes the circuit; a fresh 401 after cooldown re-opens it with a new window; the first 401 logs distinctly at error level and repeated skips while open don't re-log; and — the explicit regression check SmartDropLabs#95 asks for — 429s are completely unaffected by the circuit breaker.
…circuit breaker coingecko.js had zero test coverage before this change. Covers the existing fetch/asset-mapping/429 behavior plus the new 401 handling: throws nonRetryable on 401, circuit opens and skips the next fetch, retries after cooldown and closes on success, and 403/429 are both explicitly unaffected by the circuit (403 is CoinGecko's separate CDN/firewall-block category, not an auth failure).
priceOracleCircuit.test.js unit-tests getSourceCircuitStates() in isolation (returns a state per source that has one, omits stellar_dex which doesn't). health.test.js exercises the same thing end-to-end through the real Express app, asserting the new price_source_circuits field shape on GET /health.
5 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
2 participants
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
Fixes #95.
coinmarketcap.jsalready tagged401(auth) failures witherr.nonRetryable = true, but nothing downstream ever read it —priceOracle.js'sfetchFromAllSourcescatches every source error identically. A revoked/misconfiguredCOINMARKETCAP_API_KEYwas retried on every price fetch and every scheduled refresh cycle (every 30s by default), forever, logged at the samewarnlevel as an ordinary transient blip.What changed
src/services/sources/circuitBreaker.js(new): a small reusable per-source circuit breaker.open()trips the circuit and logs once aterrorlevel (not on every subsequent skip);noteSkipped()logs at most one reminder per configurable interval while open;close()resets on a successful fetch;getState()exposes{ source, open, openUntil }.coinmarketcap.js: consults the circuit before attempting a fetch (skips the HTTP call entirely while open), opens it on401, closes it on any successful round-trip.priceOracle.jsneeded no changes — the circuit check is entirely internal to the source module, so the generic orchestrator doesn't need to know per-source retry policy.coingecko.js: extended the same treatment to its401case. I checked CoinGecko's actual API docs first (they weren't previously handling any auth-failure status) — confirmed401= missing/invalid key,403= a separate CDN/firewall-block category,429= rate limit. Only401opens the circuit;403/429are explicitly untouched.config.js: newPRICE_SOURCE_CIRCUIT_COOLDOWN_MS(default 15 min) andPRICE_SOURCE_CIRCUIT_REMINDER_MS(default 5 min).priceOracle.js: newgetSourceCircuitStates(), aggregating circuit state from every source that has one (stellar_dexdoesn't — no auth-failure mode).GET /health(+openapi.yaml): now includesprice_source_circuits, the lowest-effort of the observability options the issue names, since the companion health-depth (Health check endpoint reports only Redis status — misses price-source, webhook-worker, and DB connectivity #92) and metrics (Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates #93) issues haven't landed yet.Explicitly out of scope (per the issue's own edge-case notes)
429s are untouched everywhere — they already self-heal via the next-cycle retry cadence, and the issue is explicit this shouldn't change.Test plan
npm test— 210/210 passing across 26 suitestest/circuitBreaker.test.js(circuit lifecycle in isolation)test/coinmarketcap.test.js— covers every acceptance criterion: 401 opens the circuit and the next fetch skips the HTTP call; retried after cooldown; successful retry closes the circuit; fresh 401 after cooldown re-opens with a new window; first failure logs aterror, repeated skips don't re-log; 429s are unaffected (explicit regression check)test/coingecko.test.js— this source had zero prior test coverage; covers existing behavior plus the same circuit-breaker cases, plus an explicit 403-is-unaffected testtest/health.test.js,test/priceOracleCircuit.test.js—price_source_circuitsshape via the real Express app and viagetSourceCircuitStates()directly