Skip to content

fix: track Ponder fallback routing as a flag, not a URL comparison - #124

Closed
Danswar wants to merge 1 commit into
developfrom
fix/ponder-fallback-routing-flag
Closed

fix: track Ponder fallback routing as a flag, not a URL comparison#124
Danswar wants to merge 1 commit into
developfrom
fix/ponder-fallback-routing-flag

Conversation

@Danswar

@Danswar Danswar commented Jul 23, 2026

Copy link
Copy Markdown

Summary

  • errorLink decides whether a failed Ponder request already went to the fallback by comparing operation.getContext().targetUrl against CONFIG.indexerFallback as strings.
  • This repo's own deploy config sets CONFIG_INDEXER_FALLBACK_URL to the same value as CONFIG_INDEXER_URL (the way to keep the var populated without enabling cross-environment failover). With primary and fallback string-identical, targetUrl === CONFIG.indexerFallback is true on every request regardless of which URL it actually went to, so the warn + retry branch (lines 47-52) can never execute — every network error still falls through to logger.error.
  • Confirmed live in Grafana/Loki: the last 30 ApiApolloConfig network-error lines in prod were all error, none warn, despite this fallback mechanism (fix: reclassify Apollo network-error log severity #117 / fix: attribute network errors to the URL the request was sent to #121) having been deployed since 2026-07-20.
  • Fix: routingLink now stamps usedFallback: isFallbackActive() at send time, and errorLink reads that flag instead of re-deriving it from a URL comparison. This correctly distinguishes primary vs. fallback attempts even when they resolve to the same URL string.

Test plan

  • Reviewed against the existing retry path: on retry, routingLink re-stamps context from the (now fallback-active) state, so a second failure against the fallback correctly falls through to error — behavior unchanged from the intended design, just no longer defeated by same-URL configs.
  • After merge: watch Loki for ApiApolloConfig network-error lines to move from error to warn on the next transient Ponder blip.

(Supersedes #123, closed — that PR's follow-up commit message and description used internal-sounding shorthand that doesn't belong in a public repo.)

sentToFallback compared operation.getContext().targetUrl against
CONFIG.indexerFallback by string value. Once an operator points the
fallback at the same host as the primary (documented in this repo's
own deploy config as the way to keep CONFIG_INDEXER_FALLBACK_URL
populated without enabling cross-environment failover), that
comparison is true on every request regardless of which URL it
actually went to — so the warn+retry branch never fires and every
network error still falls through to logger.error. Confirmed live:
30/30 recent ApiApolloConfig network-error log lines in prod were at
error, none at warn, despite the fallback fix (#117/#121) being
deployed since 2026-07-20.

Stamp the routing decision itself (isFallbackActive() at send time)
instead of re-deriving it from a URL string.
@Danswar

Danswar commented Jul 23, 2026

Copy link
Copy Markdown
Author

3 review passes to 0 findings. Correctness (retry lifecycle, concurrency, the string-comparison edge case this PR fixes) verified clean on the first pass. Two follow-up passes on wording caught and fixed public-repo hygiene issues — internal shorthand terminology that had leaked into a code comment, a commit message, and the PR description — replaced with generic phrasing. Final state: no outstanding issues, CI green.

@Danswar
Danswar marked this pull request as ready for review July 23, 2026 00:42
@Danswar Danswar closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant