CoinMarketCap's nonRetryable auth-failure flag is set but never consulted — invalid keys are retried every 30s forever

[prodbycorne]

Overview

src/services/sources/coinmarketcap.js deliberately tags authentication failures with a custom flag, clearly intending some caller upstream to treat them differently from ordinary transient errors:

async function fetchPrice(assetCode, issuer = null) {
  ...
  try {
    ...
  } catch (err) {
    if (err.response?.status === 401) {
      err.nonRetryable = true;   // <- set, but never read anywhere in the codebase
      logger.warn('CoinMarketCap authentication failed', { assetCode });
      throw err;
    }
    if (err.response?.status === 429) { ... }
    else { ... }
    return null;
  }
}

But src/services/priceOracle.js's fetchFromAllSources() — the only caller of source.fetch — catches every error identically, with no special-casing for err.nonRetryable:

for (const source of SOURCES) {
  try {
    const price = await source.fetch(assetCode, issuer);
    if (price !== null && price > 0) { results.push({ source: source.name, price }); }
  } catch (err) {
    logger.warn('Source fetch failed', { source: source.name, assetCode, error: err.message }); // treats 401 identically to a timeout
  }
}

err.nonRetryable is dead — it is set and thrown, but nothing downstream ever reads it. Practically, this means a misconfigured or revoked COINMARKETCAP_API_KEY (a persistent, non-transient misconfiguration that will never resolve itself without operator intervention) is retried every single price fetch, and, separately, every 30-second scheduled refresh cycle (PRICE_REFRESH_INTERVAL_SECONDS default), forever, indistinguishable in the logs from a one-off transient network blip — both log identically at warn level via the generic 'Source fetch failed' message. An operator has no way to distinguish "CoinMarketCap had a blip a minute ago" from "our CoinMarketCap API key has been dead for three weeks" without manually correlating hundreds of near-identical log lines, and the system keeps wastefully calling out to an endpoint that is certain to fail, on every single cycle, with no backoff specific to this class of permanent failure.

Requirements

• In priceOracle.js's fetchFromAllSources() (or a wrapper introduced for this purpose), check err.nonRetryable and, when true, stop attempting that source for a cooldown window (a "circuit breaker" — open the circuit on a non-retryable failure, and only attempt the source again after a configurable cooldown, e.g. PRICE_SOURCE_CIRCUIT_COOLDOWN_MS) rather than retrying it unconditionally on the very next cycle.
• Log non-retryable failures distinctly (e.g. at error level with a distinguishing message like 'Price source permanently misconfigured') the first time the circuit opens, rather than repeating the same warn every cycle — subsequent skips while the circuit is open should log at most a periodic reminder, not one line per skipped attempt.
• Extend the same nonRetryable/circuit-breaker treatment to coingecko.js if an equivalent permanent-failure class exists for it (e.g. an invalid COINGECKO_API_KEY producing a 401/403 — check CoinGecko's actual API behavior and add the same flag if applicable, for consistency across sources).
• Expose the circuit state (open/closed, and since-when) somewhere observable — either in logs, in the /health response, or via the metrics work from the companion observability issue — so operators can see at a glance which price sources are currently circuit-broken.

Acceptance Criteria

• A simulated CoinMarketCap 401 response causes that source to be skipped on subsequent fetch cycles for the configured cooldown window, rather than being retried every cycle.
• The first occurrence of a non-retryable failure logs distinctly from ordinary transient failures.
• After the cooldown window elapses, the source is attempted again (and the circuit re-opens if it fails again, or closes/resets if it succeeds).
• fetchFromAllSources's behavior for ordinary (retryable) failures is unchanged — this only affects sources flagged nonRetryable.
• New unit tests cover: circuit opens on 401, source is skipped while open, source is retried after cooldown, and successful retry closes the circuit.

Additional Notes

More precise references

• src/services/sources/coinmarketcap.js:74-89 (fetchPrice's catch block): confirmed exact structure — 401 sets err.nonRetryable = true, logs logger.warn('CoinMarketCap authentication failed', { assetCode }), and re-throws (throw err); 429 and all other errors are logged and the function returns null (does not throw).
• src/services/priceOracle.js:77-92 (fetchFromAllSources): confirmed the for...of loop over SOURCES wraps each source.fetch(...) call in a generic try/catch that only reads err.message for the log line (logger.warn('Source fetch failed', { source: source.name, assetCode, error: err.message })) — err.nonRetryable is never referenced anywhere in this file, confirmed via full read.
• src/jobs/priceRefresh.js:11-12: confirmed PRICE_REFRESH_INTERVAL_SECONDS drives cronExpression = \*/${intervalSeconds} * * * * *`— a **seconds-level** cron expression (six-field, not the usual five-field minute-level cron), meaning the "every 30 seconds" framing in the issue is precise if the configured default is 30, not an approximation — worth double-checkingconfig.price.refreshInterval's actual default value during implementation (not independently re-verified this pass since config.js` wasn't re-read), but the mechanism itself (seconds-granularity cron) is confirmed.
• Note the asymmetry between the two failure branches in coinmarketcap.js: the 429 branch does NOT throw (falls through to return null at the end of the function after the if/else if/else chain), while the 401 branch DOES throw — meaning fetchFromAllSources's try/catch only ever actually catches the 401 case from this source (since a 429/other error already returns null without throwation, handled by the if (price !== null && price > 0) check simply not adding it to results). This is an important nuance: the dead err.nonRetryable flag is only ever attached to the one error path that actually propagates as a thrown exception to fetchFromAllSources's catch block — the fix's if (err.nonRetryable) check in that catch block is exactly the right place, but it's worth being explicit in the PR that this is the only path where err.nonRetryable could ever be true today, and that a future contributor extending nonRetryable to coingecko.js (per the issue's own suggestion) must also choose to throw (not silently return null) for it to ever reach this same catch block.

Additional edge cases

• A circuit-breaker needs shared, cross-cycle state — coinmarketcap.js currently has no module-level mutable state at all beyond the lazily-constructed apiClient (line 5, let apiClient = null), so introducing circuit state (open/closed, opened-at timestamp) as a new module-level variable is consistent with the existing pattern, but it means circuit state is per-process, not shared across horizontally-scaled replicas — directly interacting with Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election): without Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98, each replica maintains its own independent circuit state and could each independently retry-then-fail on their own schedule, which is a smaller version of the same "no coordination across replicas" theme as Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98, though arguably lower-severity here since a per-replica circuit breaker still meaningfully reduces total call volume even if not perfectly synchronized.
• The circuit should probably distinguish "known-bad config, don't retry until an operator changes something" (a 401, which won't self-heal by waiting) from "rate-limited, will self-heal shortly" (a 429, which already has an implicit cooldown via next-cycle retry) — the issue's own requirements focus on the 401/nonRetryable case specifically and treat 429 as already reasonably handled by the existing per-cycle retry cadence; worth confirming in the PR that circuit-breaker logic is NOT accidentally applied to 429s too (which would change existing, working behavior unnecessarily and isn't asked for).
• If COINMARKETCAP_API_KEY is rotated/fixed by an operator while the circuit is open, the fix needs the cooldown to actually expire and re-attempt on schedule (not require a process restart to clear the circuit) — this is already implied by the "cooldown window" requirement, but worth being explicit that manual circuit-reset (e.g. via an admin endpoint or just waiting out the cooldown) should be tested, not just circuit-opening.
• resolveMarket() (lines 21-45) can return null for reasons unrelated to authentication (unsupported asset/issuer) — confirmed this path returns early (line 54-56) before ever reaching the try/catch, so it's unaffected by and doesn't interact with the circuit-breaker logic; worth noting explicitly so a reviewer doesn't conflate "source returned null because asset unsupported" with "source is circuit-broken."

Implementation sketch

// coinmarketcap.js — new module-level state
let circuitOpenUntil = 0;
let circuitOpenLoggedAt = 0;

async function fetchPrice(assetCode, issuer = null) {
  if (Date.now() < circuitOpenUntil) {
    // periodic reminder log, not one line per skipped attempt
    if (Date.now() - circuitOpenLoggedAt > config.priceSources.circuitReminderIntervalMs) {
      logger.warn('CoinMarketCap circuit open, skipping fetch', { until: new Date(circuitOpenUntil).toISOString() });
      circuitOpenLoggedAt = Date.now();
    }
    return null;
  }
  ...
  } catch (err) {
    if (err.response?.status === 401) {
      const wasOpen = Date.now() < circuitOpenUntil;
      circuitOpenUntil = Date.now() + config.priceSources.circuitCooldownMs;
      if (!wasOpen) {
        logger.error('Price source permanently misconfigured', { source: 'coinmarketcap', assetCode, cooldownMs: config.priceSources.circuitCooldownMs });
      }
      err.nonRetryable = true;
      throw err;
    }
    ...
  }
}

function getCircuitState() {
  return { open: Date.now() < circuitOpenUntil, until: circuitOpenUntil || null };
}

module.exports = { fetchPrice, getCircuitState };

Add PRICE_SOURCE_CIRCUIT_COOLDOWN_MS to config.js (e.g. default 15 minutes). On a successful fetch after the circuit was previously open, reset circuitOpenUntil = 0 (closes/resets the circuit, per acceptance criteria). Expose getCircuitState() for /health (#92) or /metrics (#93) to surface. priceOracle.js's fetchFromAllSources needs no changes at all under this design — the circuit check is entirely internal to the source module, which keeps the fix localized and avoids the generic catch block in priceOracle.js needing to know about per-source circuit semantics (a simpler design than pushing nonRetryable-awareness up into the orchestrator, and arguably preferable since the orchestrator's job is just to try each source and aggregate, not to manage retry policy per source).

Test/reproduction plan

• Mock axios to return a 401 on getClient().get(...); call fetchPrice twice in a row; assert the second call does NOT make an HTTP request at all (circuit open) and returns null immediately.
• Advance a mocked clock past circuitCooldownMs; call fetchPrice again; assert an HTTP request IS made this time (circuit re-attempted).
• Mock the retried request to succeed; assert getCircuitState().open is now false (circuit closed/reset).
• Mock the retried request to fail with another 401; assert the circuit re-opens with a fresh cooldown window.
• Assert the first 401 logs at error level with the distinguishing message, and a second consecutive skipped attempt within the same open window does NOT produce a duplicate identical warn/error log line (only the periodic-reminder cadence, tested by asserting log call count).
• Regression: a 429 or generic network error still behaves exactly as before (returns null, no circuit interaction) — explicit test asserting circuit state is untouched by these error types.

Cross-references

• Health check endpoint reports only Redis status — misses price-source, webhook-worker, and DB connectivity #92 (health check depth) and Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates #93 (metrics) are both explicitly named in this issue's own requirements as candidate places to expose circuit state (getCircuitState() above is designed to feed either or both) — coordinate the exact shape of the exposed data (open/since-when) so both consumers can use the same function without duplicating logic.
• Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election) — per the edge case above, per-replica circuit state is a smaller-scale version of the same "no cross-replica coordination" gap Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 addresses for scheduled jobs; not a blocking dependency, but worth a shared README note once both land, clarifying that circuit state (unlike job execution) is intentionally per-process and that's an acceptable tradeoff given it only affects retry cadence, not correctness.

[Temi-suwa18]

I'd like to work on this issue because it is a good opportunity to improve the reliability of the price oracle flow by turning the existing nonRetryable signal into an actual recovery mechanism.

My implementation approach would be to add a lightweight per-source circuit breaker that prevents repeated calls after permanent failures. For example:

let circuitOpenUntil = 0;

async function fetchPrice(assetCode, issuer = null) {
  if (Date.now() < circuitOpenUntil) {
    logger.warn('Price source circuit open, skipping request');
    return null;
  }

  try {
    const price = await requestPrice(assetCode, issuer);
    circuitOpenUntil = 0;
    return price;
  } catch (err) {
    if (err.response?.status === 401) {
      circuitOpenUntil = Date.now() + config.priceSourceCooldownMs;

      logger.error('Price source permanently misconfigured', {
        source: 'coinmarketcap',
        cooldownUntil: new Date(circuitOpenUntil),
      });

      err.nonRetryable = true;
    }

    throw err;
  }
}

I would also add tests around the circuit lifecycle:

• a 401 response opens the circuit;
• subsequent refresh cycles skip the source without making another API call;
• the source is retried after the cooldown expires;
• a successful retry closes the circuit;
• normal transient failures like network errors or 429 responses continue behaving as before.

I have experience working with Node.js services, external API integrations, and production reliability patterns. I’d focus on keeping the change isolated, observable, and easy to maintain while improving failure handling for price sources.