GeezerVison is showing github.com — fix/search: print partial results when there are graphql errors by marcleblanc2 · Pull Request #1384 · sourcegraph/src-cli
Conversation
Draft: this change is necessary but not sufficient for #1150. See "Why this does not fix #1150 yet" below. Leaving it as a draft until one of the follow-up options is picked and implemented on this branch.
Relates to #1150
Problem
src search discards every result and exits non-zero whenever the GraphQL response carries errors. request.Do in internal/api returns GraphQlErrors and drops the data as soon as an errors array is present. #1282 fixed the same class of problem for src repos list by switching to DoRaw and treating errors-alongside-data as warnings; this PR applies the same treatment to src search.
Change
cmd/src/search.go: the GraphQL call moves into runSearch, which uses DoRaw. Errors that arrive alongside results are returned as warnings; errors with no results are still a hard error. The handler prints results as before, and prints warning: N errors during search; rerun with -v to inspect them to stderr (full path + message + raw JSON with -v). JSON output on stdout stays clean.
cmd/src/repos_list.go: formatRepositoryListWarnings becomes formatGraphQLWarnings(activity, warnings) so both commands share it.
cmd/src/search_test.go: TestRunSearchHandlesGraphQLErrors covers one result's file.content erroring with the other results kept, and the no-results case (hard error).
CHANGELOG.md entry under Unreleased / Fixed.
Why this does not fix #1150 yet
The reporter's error is on search.results.results[309].file.content. Every hop from that field up to search is non-null in the schema:
content: String! → file: GitBlob! → results: [SearchResult!]! → results: SearchResults! → search(...): Search
Sourcegraph's graphql-go fork implements spec null propagation (internal/exec/exec.go): an error on a non-null field nulls its parent, and so on up to the first nullable ancestor. Here that is search, so the server returns data.search: null and the 300+ good results are never sent. Confirmed against a 8.1 instance by forcing an error under a FileMatch (bad highlight(...) arguments):
{"data": {"search": null, "site": {"buildVersion": "..."}},
"errors": [{"message": "...", "path": ["search", "results", "results", 0, "file", "highlight"]}]}
With this PR, runSearch sees zero results plus errors and returns the hard error: same outcome as before. #1282 works for repos list only because Repository.defaultBranch happens to be nullable, so a failure there nulls one field instead of the whole list.
Root cause
src search requests the full content of every matching file. The only consumer in the CLI is structural-search highlighting in text mode (searchHighlightMatch / applyHighlightsForFile); -json passes it through. That per-result gitserver read is what fails when a repository disappears between the Zoekt index and the content fetch. The web app does not hit this because it uses streaming search, which never fetches content.
Options
- Drop
content from the query unless the query contains patterntype:structural. Small change, removes the failure mode, and makes every search much lighter on the instance. Cost: file.content disappears from -json output for non-structural searches, which is a contract change (the reporter was using it).
- Two-phase fetch. Run the search without
content, then one aliased batch query (r0: repository(name: ...) { commit(rev: ...) { blob(path: ...) { content } } }, r1: ...) for the matched files. Query.repository is nullable, so a missing repository nulls only its own alias, and the partial-data handling in this PR has something to keep. Preserves the JSON contract; costs a second round trip. Recommended.
- Server-side: not feasible without a breaking schema change (
content: String! → String).
Other callers with the same data-plus-errors pattern
Checked every .Do( caller in cmd/ and internal/. The pattern only matters where a nullable boundary sits between the failing field and the data we want:
src repos get: same RepositoryFields fragment and nullable defaultBranch as repos list. A repository with an unresolvable HEAD returns everything else and the CLI throws it away. Same fix as fix/repo-list: print partial data when there are graphql errors #1282.
src batch ResolveWorkspacesForBatchSpec: repository.defaultBranch { target { oid } }, defaultBranch nullable. One unresolvable repository drops all workspaces. Arguably should still fail, but a warning naming the repository is better than the raw error dump.
users list, orgs list, users prune, extsvc list, search-jobs *, codeowners *, mutations: Postgres-backed fields only, no per-node calls to gitserver or code hosts. Leave as is.
src lsp hover / definitions / references: single-result queries; partial data is just null.
src snapshot summary, internal/instancehealth: site: Site!, externalServices: ...!, permissionsSyncJobs: ...! are non-null, so any error nulls all of data. Cannot be fixed client-side. Separately, instancehealth still queries site.monitoringStatistics, which was removed from the schema in sourcegraph/sourcegraph@a83a00798bb (Dec 2024), so that query fails validation on 6.x+ instances. Separate bug.
A more general alternative: change request.Do to always unmarshal data even when errors is present, returning GraphQlErrors alongside. Callers that return err keep their current behaviour; callers that want partial data stop short-circuiting. Would have made #1282 and this PR a few lines each.
Verification
go test ./... passes.
- Stub GraphQL server returning two
FileMatch results plus the repository does not exist error from the issue (the shape a server would send without null propagation): before (origin/main) GraphQL errors: {...}, exit 1, no results; after, both results in -json on stdout, exit 0, warning: 1 errors during search; rerun with -v to inspect them on stderr, full path/message/JSON with -v.
- Real instance (Sourcegraph 8.1): happy-path JSON and text searches up to
count:2000 unchanged, exit 0, no warnings. All repositories on that instance are cloned, so the reporter's race could not be reproduced there; the null-propagation behaviour above was confirmed instead with the forced highlight error.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
src search discarded every result and exited non-zero when the GraphQL
response carried errors for individual results, such as
"repository does not exist" on one result's file.content. Treat errors
that arrive alongside results as warnings (printed to stderr, in full
with -v) and only fail hard when no results came back, matching the
behaviour src repos list got in #1282.
Fixes #1150
Amp-Thread-ID: https://ampcode.com/threads/T-01a18b53-40bb-7118-975d-9c1b224f5602
Co-authored-by: Amp <[email protected]>
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
repository does not exist
Uh oh!
There was an error while loading. Please reload this page.
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.
|