Skip to content
Snippets Groups Projects

LSIF: Do not ensure revision for LSIF result commit resolution

Created by: efritz

This fixes the following scenario:

  1. A user uploads an LSIF dump for some repo R at commit C. The upload proxy ensures this commit exists and proxies the upload to the lsif-server (we can hold the invariant that if an LSIF upload exists, the commit existed at some point in time on gitserver).
  2. The user force-pushes such that commit C is no longer on the codehost.
  3. The LSIF-server returns a result with commit C in a reference result (or similar query). This includes a Repository.Commit resolver to fire, which will try to fetch from the codehost when the commit isn't in gitserver.

This operation is too expensive for this particular code path and should simply fail-fast on this edge case of the resolution. After chatting with @keegancsmith and @mrnugget a bit, it seems like it's safe to say that this commit will exist on gitserver if it had existed on gitserver in the past and it also exists on the codehost.

Before this change such conditions cause a multi-second GraphQL query in the API explorer. After this change the results are filtered without a noticeable delay.

Reviwers: This PR simply threads additional variables to expose the NoEnsureCommit option to the RepositoryResolver.Commit method. If there is a cleaner solution I had not found, please suggest it!

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: mrnugget

    Reviwers: This PR simply threads additional variables to expose the NoEnsureCommit option to the RepositoryResolver.Commit method. If there is a cleaner solution I had not found, please suggest it!

    The only other way that comes up is to introduce another method for the "NoEnsure"/with-opts path:

    func (s *repos) ResolveRev(ctx context.Context, repo *types.Repo, rev string) (commitID api.CommitID, err error) {
    	return s.ResolveRev(ctx, repo, rev, nil)
    }
    func (s *repos) ResolveRevWithOpts(ctx context.Context, repo *types.Repo, rev string, opt *git.ResolveRevisionOptions) (commitID api.CommitID, err error) {
    	// [...]
    }

    That way you don't have to pass in nil if you don't need options (which seems to be the majority of usecases).

    But I don't know how Go-ey that naming is :)

  • Created by: efritz

    @keegancsmith This is inside of the enterprise resolvers, which doesn't have access to toGitCommitResolver or the resolver's inputRev as they are not exported.

    I'll see what I can massage to make a more palatable diff.

  • Created by: codecov[bot]

    Codecov Report

    Merging #7488 into master will increase coverage by <.01%. The diff coverage is 100%.

    @@            Coverage Diff            @@
    ##           master   #7488      +/-   ##
    =========================================
    + Coverage   40.09%   40.1%   +<.01%     
    =========================================
      Files        1252    1252              
      Lines       65825   65827       +2     
      Branches     6165    6165              
    =========================================
    + Hits        26391   26397       +6     
    + Misses      37032   37029       -3     
    + Partials     2402    2401       -1
    Impacted Files Coverage Δ
    cmd/frontend/graphqlbackend/repository.go 25.57% <100%> (+0.68%) :arrow_up:
    cmd/frontend/internal/goroutine/goroutine.go 100% <0%> (+57.14%) :arrow_up:
Please register or sign in to reply
Loading