Surfaces errors in autoindex configuration#58879
Conversation
This leads to bad UX, because the user has no idea that something went wrong, and is just presented with an empty jobs UI
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff ea79dcf...69b096e.
|
| if errors.As(err, &inference.LimitError{}) { | ||
| limitErr = err | ||
| } else { | ||
| return nil, err | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
This error was meant to be exposed in the GraphQL API as per https://github.com/sourcegraph/sourcegraph/pull/47748 where it was introduced. Removing this assignment here would stop that from working.
There was a problem hiding this comment.
Yes, exactly. That's what I was describing in the PR description.
With the code as is it never made it to the GraphQL API, as
result, err := r.autoindexSvc.InferIndexConfiguration(ctx, r.repositoryID, "", "", true)
always returns nil if err is set, even when it is set to LimitErr.
Repeating from the PR description: l wrote 67ed1c3 to show a change that would actually let limitErr make it to the GraphQL API, but the UI doesn't seem to handle that case at all and just produces a bad UX that way.
There was a problem hiding this comment.
I think the problem needs to be fixed in the frontend where we should pattern match against this error and show a more useful message.
There was a problem hiding this comment.
I don't think the frontend can do much better than show the error as is with this PR:
If we actually wanted to make the error message more useful we'd need to include what files the script tried to read that led to this error.
There was a problem hiding this comment.
See these two GraphQL queries:
- https://sourcegraph.com/github.com/sourcegraph/sourcegraph@ea79dcfb62e9c60bf062dfbcd2573a81dc442c43/-/blob/client/web/src/enterprise/codeintel/configuration/backend.ts?L62-82
- https://sourcegraph.com/github.com/sourcegraph/sourcegraph@ea79dcfb62e9c60bf062dfbcd2573a81dc442c43/-/blob/client/web/src/enterprise/codeintel/configuration/backend.ts?L84-102
They're accessing the configuration and parsedConfiguration fields, but not checking the limitError field.
I'm not 100% sure if the idiomatic thing (wrt GraphQL) is to return a non-error with the limit error inside the payload... That seems off, but that's what the end of this function is doing currently. I think we should change that to return limitErr instead of nil.
There was a problem hiding this comment.
I've filed https://github.com/sourcegraph/sourcegraph/issues/58885 for improving the UX here.
| resolver.limitErr = err | ||
| } | ||
| return resolver, err |
There was a problem hiding this comment.
@kritzcreek what do you think about this fix instead? The main difference is that we're also setting the limitErr field as defined in the API contract.
There was a problem hiding this comment.
I see what you're saying now. This would return partial data and the error from the GraphQL request, right?
There was a problem hiding this comment.
Yeah, I think we should at least attempt to respect the API contract. Whether the API is a good one/one we want to tweak/deprecate is a separate question.
varungandhi-src
left a comment
There was a problem hiding this comment.
Can you cross-check that my suggested fix still works? If so, we can merge the PR.
Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com> (cherry picked from commit f92302f)
Fixes #58453
Test plan
Add the
vitejs/viterepo to your local sg instance, and view the autoindex configuration page.After this PR you should see the real error:
Error fetching index configuration: Inference limit: requested content for more than 100 (100) filesinstead of a nil pointer exception.
Implementation notes
I added two commits, but the first is completely overridden by the second. In 67ed1c3 I tried to 'interpret' what the code was trying to do in the first place, and return a 'succesful' result even if the evaluation ran into a limit error. That produces very confusing UX though, because now it looks like everything is fine on the configuration page, but there are no build jobs. I think it's better to surface the error in the UI here.
This change leaves things in a bit of an awkward spot. The assignment to
limitErris dead code. I don't think the special handling for limit errors make sense here, and it doesn't work anyway (because somewhere on the code path it's not treated different from other errors). I tried ripping it out, but my go-foo isn't quite there yet. I could either open an issue for the follow-up cleanup, or we could try to pair on it @varungandhi-src WDYT?