fix(aviation): fail-closed flight price search; gate demo behind explicit opt-in (#3756)#3795
Merged
Merged
Conversation
…icit opt-in (#3756) The flight-price search route silently fell back to randomized demo quotes whenever TRAVELPAYOUTS_API_TOKEN was missing, the upstream call failed, or the provider returned zero results. UI labelled this with a single 11px gray "Indicative prices" footnote — indistinguishable from live data at a glance for a self-hoster following the README. Demo data now requires explicit AVIATION_DEMO_PRICES=1 server env opt-in. Default path fails closed and surfaces the failure mode to the UI: - missing_credentials → "Live flight pricing requires TRAVELPAYOUTS_API_TOKEN. Search Google Flights instead →" with a deep link - no_results → "No live prices found for IST → LHR" (also covers swallowed upstream errors — the Travelpayouts provider catches fetch failures internally and surfaces them as empty data; documented in the handler) - upstream_error → "Flight pricing provider temporarily unavailable. Search Google Flights instead →" (reserved for synchronous handler failures that bubble up out of the provider) - ok → live quotes rendered normally When AVIATION_DEMO_PRICES=1 IS set, demo quotes render with a prominent amber banner: "⚠ DEMO DATA — synthetic distance-based estimates, not live market quotes". No more silent footnote. Proto adds `degraded: bool` + `error: string` fields to SearchFlightPricesResponse — mirrors the searchGoogleFlights pattern. README updated to clarify that flight pricing requires the env var; self-hosters without it now see an actionable error rather than fake prices. Tests cover all four reachable paths (default-off × {missing_creds, no_results}, demo-on × {missing_creds, no_results}). The `upstream_error` discriminator is reserved in the proto for future provider changes that surface fetch failures explicitly; not directly testable today because the current Travelpayouts provider swallows them internally.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
Greptile SummaryThis PR addresses issue #3756 by making the flight-price search route fail-closed: demo data now requires explicit
Confidence Score: 3/5The core fail-closed logic on the server is correct, but the client-side circuit breaker caches degraded error responses to localStorage for 10 minutes, which can leave the UI stuck showing the old error state after the server condition changes. The fetchFlightPrices call omits a shouldCache guard, so every degraded:true response (e.g. missing_credentials for self-hosters without a token) is persisted to localStorage for the full 10-minute TTL. A self-hoster who sets the token and refreshes will still see the credentials-required message for up to 10 minutes. Every other breaker in the file that can return empty results guards against this with shouldCache. src/services/aviation/index.ts — the shouldCache guard on breakerPrices.execute Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as AviationCommandBar
participant Svc as fetchFlightPrices (CB)
participant Server as search-flight-prices.ts
participant TP as Travelpayouts
UI->>Svc: fetchFlightPrices(origin, dest, date)
alt Circuit breaker cache hit (up to 10 min, incl. degraded)
Svc-->>UI: "cached { degraded, error, quotes }"
else Cache miss
Svc->>Server: searchFlightPrices RPC
alt TRAVELPAYOUTS_API_TOKEN unset
Server-->>Svc: "{ quotes:[], degraded:true, error:missing_credentials }"
else Token set
Server->>TP: searchPricesTravelpayouts(...)
alt Quotes returned
TP-->>Server: "{ quotes: [...] }"
Server-->>Svc: "{ quotes:[...], degraded:false, error:empty }"
else Empty result
TP-->>Server: "{ quotes: [] }"
alt "AVIATION_DEMO_PRICES=1"
Server-->>Svc: "{ quotes:[demo...], isDemoMode:true, degraded:true, error:no_results }"
else Default fail-closed
Server-->>Svc: "{ quotes:[], degraded:true, error:no_results }"
end
else Throws
TP--xServer: exception
alt "AVIATION_DEMO_PRICES=1"
Server-->>Svc: "{ quotes:[demo...], isDemoMode:true, degraded:true, error:upstream_error }"
else Default
Server-->>Svc: "{ quotes:[], degraded:true, error:upstream_error }"
end
end
end
Svc-->>UI: result cached 10 min including degraded states
end
UI->>UI: render per-state message or quotes plus optional demo banner
Reviews (1): Last reviewed commit: "fix(aviation): fail-closed flight price ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| provider: resp.provider, | ||
| }; | ||
| }, { quotes: [], isDemoMode: true, isIndicative: true, provider: 'demo' }, { cacheKey }); | ||
| }, fallback, { cacheKey }); |
Contributor
There was a problem hiding this comment.
persistCache: true) because no shouldCache guard is provided. If a user's session receives missing_credentials or no_results, that response is stored and replayed from the circuit-breaker cache for the next 10 minutes — even after the server token is configured. Every other breaker in this file that guards against caching empty/degraded results (e.g. breakerDelays uses shouldCache: (r) => r.length > 0) applies this guard; breakerPrices should do the same.
Suggested change
| }, fallback, { cacheKey }); | |
| }, fallback, { cacheKey, shouldCache: (r) => !r.degraded && r.quotes.length > 0 }); |
Comment on lines
+235
to
+237
| } else if (error === 'no_results') { | ||
| msg = `No live prices found for ${escapeHtml(intent.origin)} → ${escapeHtml(intent.destination)}.`; | ||
| } |
Contributor
There was a problem hiding this comment.
no_results branch doesn't include a Google Flights fallback link, unlike missing_credentials and upstream_error. A user who lands here (valid token, route genuinely has no Travelpayouts coverage) has no obvious next step — the other two error states both offer "Search Google Flights instead →" which would be equally useful here.
Suggested change
| } else if (error === 'no_results') { | |
| msg = `No live prices found for ${escapeHtml(intent.origin)} → ${escapeHtml(intent.destination)}.`; | |
| } | |
| } else if (error === 'no_results') { | |
| msg = `No live prices found for ${escapeHtml(intent.origin)} → ${escapeHtml(intent.destination)}. <a href="${gflLink}" target="_blank" rel="noopener" style="color:var(--accent,#60a5fa)">Search Google Flights instead →</a>`; | |
| } |
PR #3795 self-review surfaced 9 findings; this commit addresses all of them. P1 — fail-closed responses were being pinned in the persistent cache: - src/services/aviation/index.ts: breakerPrices.execute now passes shouldCache: (r) => r.quotes.length > 0 && !r.degraded. Without this, a 10-min IndexedDB entry held by `persistCache:true` would keep showing "credentials required" to users even after the operator restored TRAVELPAYOUTS_API_TOKEN. The breaker also evicts existing entries that fail the predicate on the next call, so the fix is forward AND retroactive. Matches the pattern in the cluster recipe redis-cache-staleness-gotchas/.../circuit-breaker-persist-cache-eviction. P2: - AviationCommandBar.ts: unknown future `error` discriminators now get a generic-but-honest "Flight pricing unavailable (X)" message with the Google Flights fallback link, instead of the misleading silent "No prices found." default. The dead `degraded:false + empty quotes` branch is folded into a proper origin→destination wording. - tests/search-flight-prices-fail-closed.test.mts: adds a service-layer regression-test suite that source-greps the breaker fallback object (asserting it is empty + degraded + error:'upstream_error' + never demo) AND asserts the new shouldCache predicate is present on the breaker call. Pattern: source-grep regression for unexercisable defensive branches (couldn't run the service module directly because of the test-import-vite-env-dev-transitive trap — both patterns documented in test-ci-gotchas). P3: - server/.../search-flight-prices.ts: tightened emptyDegraded(provider) from `string` to `'none' | 'travelpayouts_data'` union; extracted DegradedError + DegradedProvider type aliases. Added TODO comment on the no_results branch noting the provider follow-up (teach travelpayouts_data.ts to distinguish fetch-error vs empty-result so the proto's `upstream_error` value becomes reachable through normal network failures, not just synchronous handler crashes). - AviationCommandBar.ts: extracted gflFallbackLink constant — removes the duplicate <a href="..."> markup across 3 branches. - tests/...: added isIndicative assertions to each scenario as a contract guard (was P3/Q in the review). - README: split the long sentence about TRAVELPAYOUTS_API_TOKEN into two readable paragraphs. All 6 tests pass; typecheck clean.
Owner
Author
|
Self-review fixes landed in commit feb7f79: P1 — `src/services/aviation/index.ts` breakerPrices.execute now passes `shouldCache: (r) => r.quotes.length > 0 && !r.degraded`. Without this, the breaker's `persistCache:true` would pin a degraded "credentials required" response in IndexedDB for the 10-min TTL even after the operator restored the token. The predicate also evicts existing cached entries that fail it on the next call, so the fix is forward AND retroactive. P2:
P3:
All 6 tests pass; typecheck clean. CI re-running. |
…ailed (#3795 review-2) Two findings from the second review pass on PR #3795: P1 — Stale cached flight prices kept masking fail-closed degraded states ======================================================================= The first review-pass fix added shouldCache to breakerPrices.execute, which correctly prevents NEW degraded responses from being cached. But the circuit breaker's stale-while-revalidate refresh path only skipped the write — it never evicted the existing stale entry. So a user who previously cached real quotes kept seeing them indefinitely even after the server started returning missing_credentials / upstream_error / no_results: Call N-1: cache holds real quotes (shouldCache passes on read) TTL passes Call N: stale 'real' served (SWR branch); bg refresh fetches degraded response; shouldCache fails; skip write. Call N+1: stale 'real' STILL in cache; SWR cycle repeats forever. Fix in src/utils/circuit-breaker.ts: when the SWR refresh returns a result that fails shouldCache, EVICT the existing cache entry (in-memory + persistent). Next call sees no cache → falls through to the live path → surfaces the degraded shape to the UI. Generalises beyond flight-prices: every breaker caller that pairs `shouldCache` with `persistCache:true` + SWR had the same latent bug. Unit test in tests/circuit-breaker-swr-eviction.test.mts. P2 — Travelpayouts transient failures cached as no_results for 1-2h =================================================================== The fetcher wrapper used `.then(d => d ?? [])`, converting fetchTp's null (upstream failure) into [] before cachedFetchJson saw it. This defeated the NEG_SENTINEL short-TTL protection: cachedFetchJson treated [] as a valid result and wrote it to Redis with the normal 3600-7200s TTL. The handler then mapped 0 quotes to error:'no_results', so a transient outage looked like a route-level absence for 1-2 hours and the UI couldn't distinguish "provider down" from "no fares here." Fix in server/worldmonitor/aviation/v1/_providers/travelpayouts_data.ts: - Stop swallowing null in the cachedFetchJson fetcher. Let null propagate so cachedFetchJson caches NEG_SENTINEL for the default 2-minute negative TTL. - Track `upstreamFailed: boolean` across the v3 / month-matrix / latest branches and add it to the TravelpayoutsResult interface. Fix in server/worldmonitor/aviation/v1/search-flight-prices.ts: - Use upstreamFailed to pick error:'upstream_error' vs error:'no_results' on the empty-quotes path. - Remove the now-obsolete TODO + comment claiming `upstream_error` was reserved for future work; it's reachable now through normal HTTP/network failures. Tests in tests/search-flight-prices-fail-closed.test.mts: - Renamed and clarified the no_results test to stub HTTP 200 + empty data (genuine no-fares-on-this-route). - Added an explicit HTTP 500 test asserting error:'upstream_error'. - Same split applied to the demo opt-in suite. All 9 tests pass; typecheck clean.
…ket-quote behaviour)
Pre-push test surfaced a semantic conflict from the previous commit:
tests/market-quote-cache-keying.test.mjs has an explicit assertion
("SWR background refresh respects shouldCache predicate") that the
existing stale entry MUST be preserved when a SWR refresh fails
shouldCache — protects against transient upstream blips wiping
previously-good cached quotes. My previous always-evict change broke
that contract.
Fix: make eviction opt-in via a new `evictOnRefreshFailure?: boolean`
option on `breaker.execute(...)`. Default false → preserves the
existing protect-good-data behaviour for market quotes and every
other caller. Opt-in true for flight-prices and any other surface
where the degraded state is itself the important signal users need
to see (fail-closed contract).
Updated src/services/aviation/index.ts fetchFlightPrices to pass
`evictOnRefreshFailure: true`.
Tests:
- tests/circuit-breaker-swr-eviction.test.mts now has two cases:
the opt-in eviction (was the only test before), AND a new test
verifying that default behaviour preserves stale entries — same
intent as the market-quote test, expressed against a synthetic
breaker.
- tests/search-flight-prices-fail-closed.test.mts adds an assertion
that the breaker call includes `evictOnRefreshFailure: true`.
All 28 affected tests pass (10 in market-quote, 9 in flight-prices,
2 in circuit-breaker-swr, plus the rest); typecheck clean.
Owner
Author
|
Pushed two commits addressing both review findings: Commit 0461053 — initial review-2 fixes:
Commit 88972a6 — pre-push test caught a semantic conflict: Fix: made eviction opt-in via a new `evictOnRefreshFailure?: boolean` option on `breaker.execute(...)` (default false). Flight-prices opts in (`evictOnRefreshFailure: true`); market quotes and every other caller keep the preserve-stale behaviour they currently depend on. Tests now cover both modes:
Final test count: 28/28 pass across the 3 affected files; typecheck clean. |
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
1 participant
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
Per issue #3756, the flight-price search route silently fell back to randomized demo quotes whenever `TRAVELPAYOUTS_API_TOKEN` was missing, the upstream call failed, or the provider returned zero results. UI labelled this with a single 11px gray "Indicative prices" footnote — indistinguishable from live data at a glance for a self-hoster following the README's "no env vars required" promise.
Change
Server (`server/worldmonitor/aviation/v1/search-flight-prices.ts`):
Proto (`search_flight_prices.proto`):
Service layer (`src/services/aviation/index.ts`):
UI (`src/components/AviationCommandBar.ts`):
README (`README.md`):
Test plan
Tests cover all four reachable paths:
The `upstream_error` discriminator is reserved in the proto for future provider changes that surface fetch failures explicitly; not directly testable today because the current Travelpayouts provider catches them internally and surfaces them as empty data (documented in the handler).
Acceptance criteria check (from issue)
Closes #3756