Handle Zoekt connection errors in aggregations and blob search
What does this MR do and why?
Both commits fix the same root cause: a ::Search::Zoekt::Errors::ClientConnectionError escaping past the code that is supposed to turn it into a handled search failure. The first commit fixes this for the aggregations action; the second fixes a related gap on the blob search path itself.
1. Return a 503 from SearchController#aggregations
Adds a rescue clause for ::Search::Zoekt::Errors::ClientConnectionError in the aggregations action in ee/app/controllers/ee/search_controller.rb. On this error it calls ::Gitlab::ErrorTracking.log_exception, derives error_type from the class name the same way the existing rescue does (yielding client_connection_error), and renders { error:, error_type: } with status :service_unavailable.
Why:
- The action already treats "search backend unreachable" as a 503 with a structured body, but the existing rescue only lists
::Gitlab::Search::Client::ConnectionErrorand::Gitlab::Search::Client::AuthorizationError, which are Elasticsearch classes. Search::Zoekt::Errors::ClientConnectionErrordescends from a plainStandardErrorwith unrelated ancestry, so it escaped the action.Gitlab::Search::Zoekt::Clientraises it on network errors and on an unparseable JSON body (ee/lib/gitlab/search/zoekt/client.rb, around lines 128 and 158).- Today, an unreachable or slow Zoekt node produces an unhandled exception reported to Sentry as a 500 from
SearchController#aggregations, which reads like an application bug rather than a downstream dependency being down. One event peraggregationsrequest, so it scales with search traffic during an outage. - The response body uses a fixed message,
Search is currently unavailable. Please try again later., rather thane.message. The Zoekt error carries the raw transport failure including the node URL, which must not be echoed to the client. The Elasticsearch classes hardcode a safe message on the class itself (ee/lib/gitlab/search/client.rb, around lines 62-72); the Zoekt one does not, and giving it one would hide the real cause from logs on the search path.
2. Stop the language tally from failing the whole blob search
Search::Zoekt::SearchResults#known_languages in ee/lib/search/zoekt/search_results.rb now rescues ::Search::Zoekt::Errors::ClientConnectionError and returns an empty array.
Why:
#zoekt_searchalready rescuesClientConnectionErrorfor the results query and converts it into@error_message/@is_server_error, surfaced throughfailed?/server_error?. But#zoekt_search_and_wrapcalls#known_languagesto build theSearch::Zoekt::Cachekey, and that call happens before the block that#zoekt_searchrescues.- With the
Unknownlanguage filter applied and a cold aggregation cache,#known_languagesissues the language-tally request itself, so the error escapes past the rescue to the caller. - Who is exposed:
SearchController#count(chain:formatted_count('blobs')thenblobs_and_file_countthensearch_as_found_blobthenzoekt_search_and_wrap), and the GraphQLblobSearchresolver (Resolvers::Search::Blob::BlobSearchResolver), which is what the blob results UI actually calls. Both expect a failed result object, not a raised exception. - Not exposed:
SearchController#show, becausemulti_match?is true for scopeblobswith search typezoekt, sohaml_search_resultsis skipped and the action runs no Zoekt query. Also not exposed:SearchController#autocomplete, becausesearch_autocomplete_optsnever callsGitlab::Search::Zoekt::Client. - Also not exposed: the REST Search API (
lib/api/search.rb) reaches the sameSearchResults#objectschain, so it has the same shape of exposure in principle, but it doesn't take alanguageparameter, soknown_languagesis never invoked and the tally is never attempted. - An empty array is the right fallback because it is the case
CodeQueryBuilder#unknown_atomalready handles: without a known-language list the complement cannot be expressed, so theUnknownfilter is dropped rather than returning a page the user cannot recover from. - The fallback's empty array also flows into
Search::Zoekt::Cache#search_fingerprint(ee/lib/search/zoekt/cache.rb:78-83), whereknown_languages.sortis folded into the SHA256 digest alongside the query, scope, search mode, chunk size, and filters. A degraded search therefore hashes to a different cache key than a healthy one, so it cannot serve its filter-dropped results out of the cache to a later request that successfully built the language complement. No cache poisoning. - This is deliberately not the approach rejected in the closed !251074 (closed). That one rescued inside
#fetch_language_tally, which also swallowed the failure on theaggregationspath and turned it into a 200 with empty buckets. This rescue sits in#known_languages, which is only on the search path. Theaggregationsaction reaches the tally through#language_aggregation_buckets, not#known_languages, so it still returns 503 and the SLI still counts the failure. - Raised by a GitLab Duo review comment on this MR: !254462 (comment 3808884067)
What does not change:
- No user-visible change for the
aggregationsfix.fetchAllAggregationinapp/assets/javascripts/search/store/actions.js(around lines 289-297) rejects on any non-2xx response and commitsRECEIVE_AGGREGATIONS_ERROR, andapp/assets/javascripts/search/sidebar/components/language_filter/index.vuerenders the "Aggregations load error." alert. That was already true for the 500 and stays true for the 503. - No new user-visible failure mode for the blob search fix either. Before it, an
Unknown-filtered blob search with Zoekt down raised an unhandled exception; after it, the request returns the normal error result that#zoekt_searchalready produces for a downed backend, just without theUnknownfilter applied. - No change to SLI accounting on either path.
record_search_errorinapp/controllers/search_controller.rb(around lines 172-181) flags an error when the status is 500 or above, so theaggregationsfailure is still counted, and it is reached through#language_aggregation_buckets, not through#known_languages. - No changelog trailer for either commit: the language sidebar and its filters are behind the
zoekt_language_aggregationsfeature flag (ee/config/feature_flags/gitlab_com_derisk/zoekt_language_aggregations.yml), which isdefault_enabled: false, and neither change is user-facing.
Interaction with MR 254635
MR !254635 (merged) moves the Gitlab::Metrics::GlobalSearchSlis.record_error_rate call in BlobSearchResolver#results out of the if @search_results.failed? branch into an ensure, so it records error: @search_results.nil? || @search_results.server_error? on every exit path. Combined with this MR, the GraphQL blob path with the Unknown language filter applied and a cold cache gets an outcome neither MR describes on its own:
| Tally request | Results request | Recorded by 254635 |
|---|---|---|
| fails | fails | error: true (unchanged in spirit) |
| fails | succeeds | error: false -- known_languages swallows the tally failure, results render with the Unknown filter silently dropped |
The second row is defensible: the user did get results. It's called out here so the outcome is a decision on record rather than an emergent property of merge order between the two MRs. There is no file-level conflict; the two MRs touch different files.
References
- Earlier, closed attempt at the first fix: !251074 (closed). It rescued inside
Search::Zoekt::SearchResults#fetch_language_tallyand returned an empty hash, turning theaggregationsrequest into a 200 with empty buckets. That silently hid the Language section instead of showing the alert, and stopped the SLI from counting the failure. Closed in favour of the controller-level approach in this MR. The second commit here also touchessearch_results.rbbut rescues in#known_languagesinstead, which does not affect theaggregationspath; see the note under "Stop the language tally from failing the whole blob search" above. - GitLab Duo review comment that raised the second issue: !254462 (comment 3808884067)
- Feature flag rollout issue: #605457 (closed)
- Flag definition:
ee/config/feature_flags/gitlab_com_derisk/zoekt_language_aggregations.yml
Screenshots or screen recordings
No UI change. The "Aggregations load error." alert already rendered when the aggregations request failed with a 500; it renders the same way now that the request fails with a 503. The blob search page already has an error state for a downed Zoekt backend; the second commit only makes the Unknown-filtered case reach that same state instead of raising. No screenshots apply.
How to set up and validate locally
- Enable the flag in the rails console:
Feature.enable(:zoekt_language_aggregations) - Make the Zoekt search endpoint unreachable, e.g.
gdk stop gitlab-zoekt-webserver-development-1 gitlab-zoekt-webserver-development-2 - Run an exact-code (blob) search in a Zoekt-indexed namespace.
- Expect: results render, the sidebar shows "Aggregations load error.", and the request to
/search/aggregationsreturns 503 with{"error":"Search is currently unavailable. Please try again later.","error_type":"client_connection_error"}instead of a 500. - Specs for the first commit:
bundle exec rspec ee/spec/controllers/ee/search_controller_spec.rb -e aggregations, 24 examples, 0 failures. Two new examples cover the 503 body and thatrecord_error_rateis still called witherror: true; both fail if the rescue clause is removed. - With the flag still enabled and Zoekt still stopped, apply the
Unknownlanguage filter on a blob search. Before the second fix this raised an unhandled exception; after it, the blob search returns its normal error state and theUnknownfilter is dropped instead. - Specs for the second commit:
bundle exec rspec ee/spec/lib/search/zoekt/search_results_spec.rb, 137 examples, 0 failures. The new example was verified to fail without the rescue, with backtracefetch_language_tallythenknown_languagesthenzoekt_search_and_wrapthensearch_as_found_blobthenblobs_and_file_countthenobjects.
MR acceptance checklist
Check this MR acceptance checklist before merge.