Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions cmd/repo-updater/repoupdater/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ func externalServiceValidate(ctx context.Context, req *protocol.ExternalServiceS
var mockRepoLookup func(protocol.RepoLookupArgs) (*protocol.RepoLookupResult, error)

func (s *Server) repoLookup(ctx context.Context, args protocol.RepoLookupArgs) (result *protocol.RepoLookupResult, err error) {
// Sourcegraph.com: this is on the user path, do not block for ever if codehost is being
// Sourcegraph.com: this is on the user path, do not block forever if codehost is being
// bad. Ideally block before cloudflare 504s the request (1min).
// Other: we only speak to our database, so response should be in a few ms.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
Expand Down Expand Up @@ -472,36 +472,39 @@ func (s *Server) repoLookup(ctx context.Context, args protocol.RepoLookupArgs) (
return s.remoteRepoSync(ctx, codehost, string(args.Repo))
}

// We don't sync private repos on demand
if repo.Private {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behaviour though, doesn't it? Before we were not syncing the repo, but we'd return a valid result in https://github.com/sourcegraph/sourcegraph/pull/17302/files#diff-736069860c34f463e5653fc6e82948c934a6771434326e0b79d0e11f63c20f72L513-L520

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, good catch. I'll revert

}

// TODO a queue with single flighting to speak to remote for args.Repo?

// We have (potentially stale) data we can return to the user right now. Do that
// rather than blocking. This should only happen for public repos, private repos
// are ignored since if they do exist in our DB they would have been added by a
// user owned code host connection in which case they'll be kept up to date by
// our background syncer.
if !repo.Private {
go func() {
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
repoResult, err := s.remoteRepoSync(ctx, codehost, string(args.Repo))
if err != nil {
log15.Error("async remoteRepoSync failed", "repo", args.Repo, "error", err)
return
}
go func() {
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
repoResult, err := s.remoteRepoSync(ctx, codehost, string(args.Repo))
if err != nil {
log15.Error("async remoteRepoSync failed", "repo", args.Repo, "error", err)
return
}

// Since we are only dealing with public repos here we can safely assume that
// when a repository stored in the database is not accessible anymore, no other
// external service should have access to it, we can then remove it.
if repoResult.ErrorNotFound || repoResult.ErrorUnauthorized {
err = s.Store.UpsertRepos(ctx, repo.With(func(r *types.Repo) {
r.DeletedAt = s.Now()
}))
if err != nil {
log15.Error("failed to delete inaccessible repo", "repo", args.Repo, "error", err)
}
// Since we are only dealing with public repos here we can safely assume that
// when a repository stored in the database is not accessible anymore, no other
// external service should have access to it, we can then remove it.
if repoResult.ErrorNotFound || repoResult.ErrorUnauthorized {
err = s.Store.UpsertRepos(ctx, repo.With(func(r *types.Repo) {
r.DeletedAt = s.Now()
}))
if err != nil {
log15.Error("failed to delete inaccessible repo", "repo", args.Repo, "error", err)
}
}()
}
}
}()
}

if repo == nil {
Expand Down