fix(search): surface Google Flights ErrorResponse instead of silent empty results - #208
Open
bjgross10767 wants to merge 2 commits into
Open
fix(search): surface Google Flights ErrorResponse instead of silent empty results#208bjgross10767 wants to merge 2 commits into
bjgross10767 wants to merge 2 commits into
Conversation
…mpty
Google Flights occasionally returns HTTP 200 with an `ErrorResponse`
envelope (rate-limit / fingerprint reject / quota): a `wrb.fr` row whose
inner-JSON slot is `null` and a protobuf-typed error blob in `row[5]`.
`parse_first_wrb_payload` correctly returns `None` for that shape, but
`_fetch_flights` then returned `None` -> `SearchFlights.search()` returned
`None` -> the MCP layer reported `{"success": true, "count": 0}`.
Callers had no way to distinguish "rate-limited, retry with backoff" from
"no flights match these filters". Symptoms surfaced for users as punitarani#142
(round-trip empty) and punitarani#200 (general empty results): each round-trip
fires N+1 requests (one outbound + per-top-N expansion), so a few hits
the per-IP quota first and ALL subsequent queries silently fail.
This change:
- Adds `GoogleFlightsRateLimited(SearchClientError)` exception with the
captured shopping session id when extractable.
- Adds `is_rate_limit_response(body)` and `extract_error_session_id(body)`
helpers in `_wire.py`, both based on the stable protobuf type URL
`travel.frontend.flights.ErrorResponse`.
- `SearchFlights._fetch_flights` and `SearchDates._fetch_dates` raise
`GoogleFlightsRateLimited` on the rejected envelope instead of
swallowing it as `None`.
- MCP server's three executors catch the new exception and return a
structured `{"success": False, "code": "RATE_LIMITED",
"retry_after_s": 30, "session_id": "..."}` payload so MCP clients
can implement targeted backoff instead of treating the silent empty
as "no matches".
Tests:
- New `tests/search/test_rate_limit_detection.py` (6 cases) covers
one-way, round-trip, search_dates, and both MCP executors using a
captured real-life rate-limit fixture
(`tests/search/fixtures/flight_search_rate_limit_error.txt`).
- New `TestRateLimitDetection` group in `tests/search/test_wire.py`
(7 cases) covers the wire-level detection + session-id extraction
from the same fixture.
No existing tests changed behaviour: the rejected envelope's
`parse_first_wrb_payload` still returns `None`; the new raise only fires
in the previously-unreachable branch where `inner is None` AND the body
carries the ErrorResponse marker.
Fixes punitarani#142, punitarani#200 partially (surfaces the failure instead of silencing it
— actual retry / backoff is left to callers but the structured error
makes that implementable).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+55
to
+58
| return self._post(url, **kwargs) | ||
|
|
||
|
|
||
| @pytest.fixture |
Contributor
There was a problem hiding this comment.
_RateLimitedClient.get — calls non-existent self._post
The get method calls self._post(url, **kwargs) but no such attribute exists on the class (only post is defined). Any test path that exercises an HTTP GET through this fake client will immediately raise AttributeError instead of returning the mocked rate-limit response, silently hiding the intended coverage. The affected method is currently untriggered by the existing test suite since both _fetch_flights and _fetch_dates use POST, but the latent bug is one future get-based search path away from causing confusing failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/search/test_rate_limit_detection.py
Line: 55-58
Comment:
**Typo in `_RateLimitedClient.get` — calls non-existent `self._post`**
The `get` method calls `self._post(url, **kwargs)` but no such attribute exists on the class (only `post` is defined). Any test path that exercises an HTTP GET through this fake client will immediately raise `AttributeError` instead of returning the mocked rate-limit response, silently hiding the intended coverage. The affected method is currently untriggered by the existing test suite since both `_fetch_flights` and `_fetch_dates` use `POST`, but the latent bug is one future `get`-based search path away from causing confusing failures.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+142
to
+147
| if isinstance(body, bytes): | ||
| try: | ||
| body = body.decode("utf-8", errors="ignore") | ||
| except UnicodeDecodeError: | ||
| return False | ||
| return RATE_LIMIT_ERROR_MARKER in body |
Contributor
There was a problem hiding this comment.
except UnicodeDecodeError is dead code after errors="ignore"
bytes.decode("utf-8", errors="ignore") never raises UnicodeDecodeError — the errors="ignore" flag suppresses decode errors by silently dropping bad bytes. The except clause is therefore unreachable in both is_rate_limit_response and extract_error_session_id. This pattern appears twice in the new code.
Suggested change
| if isinstance(body, bytes): | |
| try: | |
| body = body.decode("utf-8", errors="ignore") | |
| except UnicodeDecodeError: | |
| return False | |
| return RATE_LIMIT_ERROR_MARKER in body | |
| if isinstance(body, bytes): | |
| body = body.decode("utf-8", errors="ignore") | |
| return RATE_LIMIT_ERROR_MARKER in body |
Prompt To Fix With AI
This is a comment left during a code review.
Path: fli/search/_wire.py
Line: 142-147
Comment:
**`except UnicodeDecodeError` is dead code after `errors="ignore"`**
`bytes.decode("utf-8", errors="ignore")` never raises `UnicodeDecodeError` — the `errors="ignore"` flag suppresses decode errors by silently dropping bad bytes. The `except` clause is therefore unreachable in both `is_rate_limit_response` and `extract_error_session_id`. This pattern appears twice in the new code.
```suggestion
if isinstance(body, bytes):
body = body.decode("utf-8", errors="ignore")
return RATE_LIMIT_ERROR_MARKER in body
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+158
to
+163
| if isinstance(body, bytes): | ||
| try: | ||
| body = body.decode("utf-8", errors="ignore") | ||
| except UnicodeDecodeError: | ||
| return None | ||
| if RATE_LIMIT_ERROR_MARKER not in body: |
Contributor
There was a problem hiding this comment.
except UnicodeDecodeError in extract_error_session_id — errors="ignore" makes the clause unreachable.
Suggested change
| if isinstance(body, bytes): | |
| try: | |
| body = body.decode("utf-8", errors="ignore") | |
| except UnicodeDecodeError: | |
| return None | |
| if RATE_LIMIT_ERROR_MARKER not in body: | |
| if isinstance(body, bytes): | |
| body = body.decode("utf-8", errors="ignore") | |
| if RATE_LIMIT_ERROR_MARKER not in body: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: fli/search/_wire.py
Line: 158-163
Comment:
Same dead-code `except UnicodeDecodeError` in `extract_error_session_id` — `errors="ignore"` makes the clause unreachable.
```suggestion
if isinstance(body, bytes):
body = body.decode("utf-8", errors="ignore")
if RATE_LIMIT_ERROR_MARKER not in body:
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+153
to
+212
| class TestMCPServerRateLimit: | ||
| """MCP layer should convert the exception into a structured error. | ||
|
|
||
| Not a vanilla ``success=False, error="Search failed: ..."`` — callers | ||
| keying off the ``code`` field can implement targeted backoff. | ||
| """ | ||
|
|
||
| def test_execute_flight_search_returns_rate_limit_code( | ||
| self, rate_limit_body: str, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| from fli.mcp import server | ||
|
|
||
| rate_limited = _RateLimitedClient(rate_limit_body) | ||
| original_init = SearchFlights.__init__ | ||
|
|
||
| def _patched_init(self, *args: Any, **kwargs: Any) -> None: | ||
| original_init(self, *args, **kwargs) | ||
| self.client = rate_limited | ||
|
|
||
| # Patch SearchFlights to ship the fake client on construction so the | ||
| # MCP layer's own `SearchFlights()` picks it up without extra wiring. | ||
| monkeypatch.setattr(SearchFlights, "__init__", _patched_init, raising=True) | ||
| params = server.FlightSearchParams( | ||
| origin="BZN", | ||
| destination="SEA", | ||
| departure_date="2026-07-15", | ||
| currency="USD", | ||
| ) | ||
| result = server._execute_flight_search(params) | ||
| assert result["success"] is False | ||
| assert result["code"] == "RATE_LIMITED" | ||
| assert result["retry_after_s"] == 30 | ||
| assert result["session_id"] == "avU5aqPqKqioj8oP77Xy6Qc" | ||
| assert result["flights"] == [] | ||
|
|
||
| def test_execute_date_search_returns_rate_limit_code( | ||
| self, rate_limit_body: str, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| from fli.mcp import server | ||
|
|
||
| rate_limited = _RateLimitedClient(rate_limit_body) | ||
| original_init = SearchDates.__init__ | ||
|
|
||
| def _patched_init(self, *args: Any, **kwargs: Any) -> None: | ||
| original_init(self, *args, **kwargs) | ||
| self.client = rate_limited | ||
|
|
||
| monkeypatch.setattr(SearchDates, "__init__", _patched_init, raising=True) | ||
| params = server.DateSearchParams( | ||
| origin="BZN", | ||
| destination="SEA", | ||
| start_date="2026-07-10", | ||
| end_date="2026-07-25", | ||
| currency="USD", | ||
| ) | ||
| result = server._execute_date_search(params) | ||
| assert result["success"] is False | ||
| assert result["code"] == "RATE_LIMITED" | ||
| assert result["retry_after_s"] == 30 | ||
| assert result["dates"] == [] |
Contributor
There was a problem hiding this comment.
_execute_booking_options rate-limit path has no test coverage
The PR adds a GoogleFlightsRateLimited catch block in _execute_booking_options (server.py lines ~766-777) but TestMCPServerRateLimit only exercises _execute_flight_search and _execute_date_search. A test analogous to test_execute_flight_search_returns_rate_limit_code is missing for the booking-options path, so any regression in its structured error response (e.g., wrong key name or missing code field) would go undetected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/search/test_rate_limit_detection.py
Line: 153-212
Comment:
**`_execute_booking_options` rate-limit path has no test coverage**
The PR adds a `GoogleFlightsRateLimited` catch block in `_execute_booking_options` (server.py lines ~766-777) but `TestMCPServerRateLimit` only exercises `_execute_flight_search` and `_execute_date_search`. A test analogous to `test_execute_flight_search_returns_rate_limit_code` is missing for the booking-options path, so any regression in its structured error response (e.g., wrong key name or missing `code` field) would go undetected.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…surface Google Flights' throttle keys off the client TLS / HTTP-2 fingerprint (JA3 + ALPN + h2 frame settings + priority frames), not just IP. A tight burst of flight queries from a single fingerprint (the prior hardcoded `impersonate="chrome"`) exhausts the per-fingerprint quota and Google returns the ErrorResponse envelope surfaced by PR punitarani#208. Switching to random per-request rotation across a small diverse pool (2 Chrome variants + Firefox + Safari + one stable Chrome anchor) spreads requests across multiple browser identities. In a smoke test of 5 cold queries to different routes immediately after the prior session burned the per-fingerprint quota, 3 of 5 succeeded with real flight rows; the other 2 returned the now-explicit RATE_LIMITED code so callers can retry, likely landing on an unblocked fingerprint. This is the second half of the rate-limit resilience pair: - PR punitarani#208: surface the rejected-envelope path as `GoogleFlightsRateLimited` instead of silently returning empty so callers can detect + back off. - This commit: reduce how often callers hit the wall in the first place by widening the fingerprint surface. Together they shift the rate-limit experience from "all flight queries inexplicably return zero results until the IP cools down" to "most queries work; a minority surface a typed retryable error". Pool: chrome120, chrome124, chrome131, firefox133, safari17_0. All verified against `curl_cffi.requests.BrowserType` in the unit test, so a future curl_cffi release that drops one of these surfaces at test time rather than at HTTP time. Tests: - `tests/search/test_impersonate_rotation.py` (5 cases): pool diversity (at least 2 browser families), every member is a valid curl_cffi BrowserType, `pick_impersonate` returns pool members, statistically visits every member across 500 picks, distribution is roughly uniform. - All 38 unit tests in the touched modules pass (33 prior + 5 new), no regressions. The rotation reduces but does not eliminate rate-limit hits — the `GoogleFlightsRateLimited` path remains the safety net. Callers should still handle `code == "RATE_LIMITED"` and back off / retry as the upstream behaviour can change at any time.
3 tasks
|
Heads-up: four open PRs now address this same envelope (#201, #205, #224, and this one) — I posted a comparison with live-captured evidence on #201. Two things from this PR are worth preserving whichever base lands: the structured MCP error payloads + One caution on framing: the code-13 envelope isn't always rate limiting — I can reproduce it deterministically with malformed requests (missing date, wrong location type, missing same-origin header), so surfacing it as Also, the impersonate-rotation commit might get more traction as its own PR — it's a separate (and more contestable) change, and it would be a shame for it to hold up the error surfacing. |
Whirlywack
added a commit
to Whirlywack/skrendam
that referenced
this pull request
Aug 21, 2026
Investigated four plausible "maybe we're using fli wrong" hypotheses. All negative, all worth recording: - Stale vendored fork: our fli/ is at upstream daf9e9a and the only later upstream commit is TypeScript-only. Nothing to pull. - Wrong surface: CLI, MCP and library share one client.py and the same three RPCs, so no surface can be less blocked. There is no REPL. - Retry: 12 identical requests, 3s apart, 0/12. The block is deterministic per request, so upstream PRs punitarani#201/punitarani#205/punitarani#208 would not help. - Byte-vs-char chunk framing (upstream PR punitarani#224): reproduced synthetically, then REFUTED against the 7 real captured Google bodies in tests/search/fixtures/ — 6 contain multi-byte UTF-8 and all decode correctly. Google counts bytes; our decoder is right. We are not discarding decodable European responses. Also records two untested leads (force-IPv4 per issue punitarani#200, curl_cffi string proxy per issue punitarani#50) and an independent A/B whose no-bgr baseline of 1/6 matches our measured 6/40. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DZNr5u3Jbx34RkbJbq7YY
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
Google Flights occasionally returns HTTP 200 with an
ErrorResponseenvelope (rate-limit / fingerprint reject / quota): awrb.frrow whose inner-JSON slot isnulland a protobuf-typed error blob inrow[5].parse_first_wrb_payloadcorrectly returnsNonefor that shape, but_fetch_flightsthen returnedNone->SearchFlights.search()returnedNone-> the MCP layer reported{"success": true, "count": 0}.Callers had no way to distinguish "rate-limited, retry with backoff" from "no flights match these filters". Symptoms surface for users as #142 (round-trip empty) and contribute to #200 (general empty results): each round-trip fires N+1 requests (one outbound + per-top-N expansion), so a few hit the per-IP quota first and ALL subsequent queries silently fail.
Reproduced consistently from a single residential IP: third or fourth round-trip search in a short window returns the error envelope, and one-way queries from the same client also start coming back empty. Switching IP or waiting ~60s restores normal results.
Changes
New exception
GoogleFlightsRateLimited(SearchClientError)infli/search/exceptions.pywith optionalsession_idfrom the rejected envelope, exported fromfli.search.fli/search/_wire.py: addsis_rate_limit_response(body)(substring match against the stable protobuf type URLtravel.frontend.flights.ErrorResponse) andextract_error_session_id(body)(best-effort session-id pull fromrow[5][2][0][1][0][3]).fli/search/flights.py_fetch_flights: whenparse_first_wrb_payloadreturnsNone, checkis_rate_limit_response; if true, raiseGoogleFlightsRateLimitedinstead of silently returningNone. Otherwise behaviour unchanged.fli/search/dates.py_fetch_dates: same treatment forSearchDates.fli/mcp/server.py:_execute_flight_search,_execute_booking_options, and_execute_date_searcheach catch the new exception and return a structured payload:{ "success": false, "error": "Google Flights rejected ...", "code": "RATE_LIMITED", "retry_after_s": 30, "session_id": "...", "flights": [] }MCP clients can key off
code == "RATE_LIMITED"for targeted backoff instead of treating the silent empty as "no matches".Tests
tests/search/test_rate_limit_detection.py(6 cases): exercises one-way, round-trip, search_dates, and both MCP executors via the captured real-life rate-limit fixture using the project's existing fake-client pattern.TestRateLimitDetectiongroup intests/search/test_wire.py(7 cases): wire-level detection + session-id extraction from the same fixture, plus negative cases (empty body, success body, bytes input).tests/search/fixtures/flight_search_rate_limit_error.txt: real captured 325-byte response from aBZN -> SEAround-trip query when the client was rate-limited.Full unit-test suite (213 tests, excluding live-network tests) passes locally on Windows + Python 3.14. The one pre-existing failure in
test_snapshot_fixtures.py::test_jfk_fra_oneworld_has_resultsis a cp1252 encoding issue on Windows unrelated to this change (also fails onmain).Backwards compatibility
GoogleFlightsRateLimitedexception, two helper functions, structured MCP error). No signature changes.parse_first_wrb_payloadstill returnsNone. The new raise only fires in the previously-unreachable branch whereinner is NoneAND the body carries theErrorResponsemarker.Exceptioncontinue to work; users who were catchingSearchClientErrorget a more specific exception type for free.Closes / relates to
Generated with Claude Code
Greptile Summary
This PR fixes a silent-empty-result bug where Google Flights' HTTP 200
ErrorResponseenvelope (rate-limit / fingerprint reject) was indistinguishable from "no matching flights." It introducesGoogleFlightsRateLimited, wires detection into_fetch_flightsand_fetch_dates, and surfaces a structuredcode: "RATE_LIMITED"response at the MCP layer.GoogleFlightsRateLimited(SearchClientError)is added tofli/search/exceptions.pyand exported fromfli.search;is_rate_limit_response(substring match on a stable protobuf type URL) andextract_error_session_id(best-effort path navigation) are added to_wire.py.SearchFlights._fetch_flightsandSearchDates._fetch_datesnow raiseGoogleFlightsRateLimitedwhenparse_first_wrb_payloadreturnsNoneand the body carries the error marker, replacing a silentreturn None.{\"success\": false, \"code\": \"RATE_LIMITED\", \"retry_after_s\": 30, ...}so MCP clients can implement targeted backoff.Confidence Score: 4/5
Safe to merge; the core detection logic is correct and the changes are additive. The one latent bug is confined to a test helper method that no current test path exercises.
The production logic is sound and well-tested via the captured fixture. The test helper
_RateLimitedClient.getreferences the non-existentself._post, which will surface as anAttributeErrorthe first time a GET-based search path is added and tested through this fake client. Bothis_rate_limit_responseandextract_error_session_idalso carry an unreachableexcept UnicodeDecodeErrorblock, and there is no test for the_execute_booking_optionsrate-limit handler.tests/search/test_rate_limit_detection.py — fix the
self._posttypo in_RateLimitedClient.getand add a test for_execute_booking_options. fli/search/_wire.py — remove the deadexcept UnicodeDecodeErrorbranches.Important Files Changed
is_rate_limit_responseandextract_error_session_idhelpers. Both contain an unreachableexcept UnicodeDecodeErrorblock becauseerrors="ignore"is used. The session-id path navigation is fragile but correctly documented as best-effort.GoogleFlightsRateLimited(SearchClientError)with an optionalsession_idattribute. Clean hierarchy, correct__init__, no issues._fetch_flights— raisesGoogleFlightsRateLimitedwhenparse_first_wrb_payloadreturnsNoneand the body contains the error marker. Logic is correct and only fires in the previously-unreachableinner is Nonebranch._fetch_dates. Symmetric and correct.GoogleFlightsRateLimitedcatch blocks to all three executors, returningcode: "RATE_LIMITED"structured errors. Theretry_after_s: 30is hardcoded but acceptable._RateLimitedClient.get(calls non-existentself._post). Also missing a test for_execute_booking_options.is_rate_limit_responseandextract_error_session_id, including positive, negative, bytes, and empty-body cases.GoogleFlightsRateLimitedfrom the publicfli.searchnamespace. Correctly added to both the import and__all__.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[HTTP POST to Google Flights] --> B[response.raise_for_status] B --> C[parse_first_wrb_payload] C -->|non-None| D[Decode flight/date rows] C -->|None| E{is_rate_limit_response?} E -->|Yes| F[extract_error_session_id] F --> G[raise GoogleFlightsRateLimited] E -->|No| H[return None] D --> I[Return results] G --> J{MCP layer catch} J --> K[Return RATE_LIMITED structured error]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[HTTP POST to Google Flights] --> B[response.raise_for_status] B --> C[parse_first_wrb_payload] C -->|non-None| D[Decode flight/date rows] C -->|None| E{is_rate_limit_response?} E -->|Yes| F[extract_error_session_id] F --> G[raise GoogleFlightsRateLimited] E -->|No| H[return None] D --> I[Return results] G --> J{MCP layer catch} J --> K[Return RATE_LIMITED structured error]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(search): surface Google Flights Erro..." | Re-trigger Greptile