graphqlbackend: support async deletion of external service (#35293)

CHANGELOG.md

@@ -24,6 +24,7 @@ All notable changes to Sourcegraph are documented in this file.
 - Search: `path:` is now a valid filter. It is an alias for the existing `file:` filter. [#34947](https://github.com/sourcegraph/sourcegraph/pull/34947)
 - Search: `-language` is a valid filter, but the web app displays it as invalid. The web app is fixed to reflect validity. [#34949](https://github.com/sourcegraph/sourcegraph/pull/34949)
 - Search-based code intelligence now recognizes local variables in Python, Java, JavaScript, TypeScript, C/C++, C#, Go, and Ruby. [#33689](https://github.com/sourcegraph/sourcegraph/pull/33689)
+- GraphQL API: Added support for async external service deletion. This should be used to delete an external service which cannot be deleted within 75 seconds timeout due to a large number of repos. Usage: add `async` boolean field to `deleteExternalService` mutation. Example: `mutation deleteExternalService(externalService: "id", async: true) { alwaysNil }`
 
 ### Changed
 

cmd/frontend/graphqlbackend/external_services.go

@@ -187,6 +187,7 @@ func (r *schemaResolver) UpdateExternalService(ctx context.Context, args *update
 
 type deleteExternalServiceArgs struct {
 	ExternalService graphql.ID
+	Async           bool
 }
 
 func (r *schemaResolver) DeleteExternalService(ctx context.Context, args *deleteExternalServiceArgs) (*EmptyResponse, error) {
@@ -214,8 +215,25 @@ func (r *schemaResolver) DeleteExternalService(ctx context.Context, args *delete
 		return nil, err
 	}
 
-	if err = r.db.ExternalServices().Delete(ctx, id); err != nil {
-		return nil, err
+	if args.Async {
+		// run deletion in the background and return right away
+		go func() {
+			if err := r.deleteExternalService(context.Background(), id, es); err != nil {
+				log15.Error("Background external service deletion failed", "err", err)
+			}
+		}()
+	} else {
+		if err = r.deleteExternalService(ctx, id, es); err != nil {
+			return nil, err
+		}
+	}
+
+	return &EmptyResponse{}, nil
+}
+
+func (r *schemaResolver) deleteExternalService(ctx context.Context, id int64, es *types.ExternalService) error {
+	if err := r.db.ExternalServices().Delete(ctx, id); err != nil {
+		return err
 	}
 	now := time.Now()
 	es.DeletedAt = now
@@ -228,7 +246,7 @@ func (r *schemaResolver) DeleteExternalService(ctx context.Context, args *delete
 		}
 	}()
 
-	return &EmptyResponse{}, nil
+	return nil
 }
 
 type ExternalServicesArgs struct {

cmd/frontend/graphqlbackend/schema.graphql

@@ -114,7 +114,7 @@ type Mutation {
     """
     Delete an external service. Only site admins may perform this mutation.
     """
-    deleteExternalService(externalService: ID!): EmptyResponse!
+    deleteExternalService(externalService: ID!, async: Boolean = false): EmptyResponse!
     """
     Tests the connection to a mirror repository's original source repository. This is an
     expensive and slow operation, so it should only be used for interactive diagnostics.

dev/authtest/code_intel_test.go

@@ -52,7 +52,7 @@ func TestCodeIntelEndpoints(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}

dev/authtest/repository_test.go

@@ -39,7 +39,7 @@ func TestRepository(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}

dev/gqltest/README.md

@@ -49,7 +49,7 @@ GraphQL-based integration tests are running against a live Sourcegraph instance,
 docker run --publish 7080:7080 --rm sourcegraph/server:insiders
 ```
 
-Once the the instance is live (look for the log line `✱ Sourcegraph is ready at: http://127.0.0.1:7080`), you can open another terminal tab to run these tests under this directory (`dev/gqltest`):
+Once the instance is live (look for the log line `✱ Sourcegraph is ready at: http://127.0.0.1:7080`), you can open another terminal tab to run these tests under this directory (`dev/gqltest`):
 
 ```sh
 → go test -long

dev/gqltest/external_service_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 
@@ -34,13 +35,13 @@ func TestExternalService(t *testing.T) {
 				RepositoryPathPattern: "github.com/{nameWithOwner}",
 			}),
 		})
-		// The repo-updater might not be up yet but it will eventually catch up for the external
+		// The repo-updater might not be up yet, but it will eventually catch up for the external
 		// service we just added, thus it is OK to ignore this transient error.
 		if err != nil && !strings.Contains(err.Error(), "/sync-external-service") {
 			t.Fatal(err)
 		}
 		defer func() {
-			err := client.DeleteExternalService(esID)
+			err := client.DeleteExternalService(esID, false)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -93,13 +94,13 @@ func TestExternalService_AWSCodeCommit(t *testing.T) {
 			},
 		}),
 	})
-	// The repo-updater might not be up yet but it will eventually catch up for the external
+	// The repo-updater might not be up yet, but it will eventually catch up for the external
 	// service we just added, thus it is OK to ignore this transient error.
 	if err != nil && !strings.Contains(err.Error(), "/sync-external-service") {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -145,13 +146,13 @@ func TestExternalService_BitbucketServer(t *testing.T) {
 			RepositoryPathPattern: "bbs/{projectKey}/{repositorySlug}",
 		}),
 	})
-	// The repo-updater might not be up yet but it will eventually catch up for the external
+	// The repo-updater might not be up yet, but it will eventually catch up for the external
 	// service we just added, thus it is OK to ignore this transient error.
 	if err != nil && !strings.Contains(err.Error(), "/sync-external-service") {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -238,9 +239,58 @@ func createPerforceExternalService(t *testing.T) {
 		t.Fatal(err)
 	}
 	t.Cleanup(func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, true)
 		if err != nil {
 			t.Fatal(err)
 		}
 	})
 }
+
+func TestExternalService_AsyncDeletion(t *testing.T) {
+	if len(*bbsURL) == 0 || len(*bbsToken) == 0 || len(*bbsUsername) == 0 {
+		t.Skip("Environment variable BITBUCKET_SERVER_URL, BITBUCKET_SERVER_TOKEN, or BITBUCKET_SERVER_USERNAME is not set")
+	}
+
+	// Set up external service
+	esID, err := client.AddExternalService(gqltestutil.AddExternalServiceInput{
+		Kind:        extsvc.KindBitbucketServer,
+		DisplayName: "gqltest-bitbucket-server",
+		Config: mustMarshalJSONString(struct {
+			URL                   string   `json:"url"`
+			Token                 string   `json:"token"`
+			Username              string   `json:"username"`
+			Repos                 []string `json:"repos"`
+			RepositoryPathPattern string   `json:"repositoryPathPattern"`
+		}{
+			URL:                   *bbsURL,
+			Token:                 *bbsToken,
+			Username:              *bbsUsername,
+			Repos:                 []string{"SOURCEGRAPH/jsonrpc2"},
+			RepositoryPathPattern: "bbs/{projectKey}/{repositorySlug}",
+		}),
+	})
+	// The repo-updater might not be up yet, but it will eventually catch up for the external
+	// service we just added, thus it is OK to ignore this transient error.
+	if err != nil && !strings.Contains(err.Error(), "/sync-external-service") {
+		t.Fatal(err)
+	}
+	err = client.DeleteExternalService(esID, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// This call should return not found error. Retrying for 5 seconds to wait for async deletion to finish
+	err = gqltestutil.Retry(5*time.Second, func() error {
+		_, err = client.UpdateExternalService(gqltestutil.UpdateExternalServiceInput{ID: esID})
+		if err == nil {
+			return gqltestutil.ErrContinueRetry
+		}
+		return err
+	})
+	if err == nil || err == gqltestutil.ErrContinueRetry {
+		t.Fatal("Deleted service should not be found")
+	}
+	if !strings.Contains(err.Error(), "external service not found") {
+		t.Fatalf("Not found error should be returned, got: %s", err.Error())
+	}
+}

dev/gqltest/repository_test.go

@@ -37,7 +37,7 @@ func TestRepository(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -100,7 +100,7 @@ func TestRepository_NameWithSpace(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer func() {
-		err := client.DeleteExternalService(esID)
+		err := client.DeleteExternalService(esID, false)
 		if err != nil {
 			t.Fatal(err)
 		}

dev/gqltest/search_test.go

@@ -102,7 +102,8 @@ func TestSearch(t *testing.T) {
 // based search API. It only supports the methods that streaming supports.
 type searchClient interface {
 	AddExternalService(input gqltestutil.AddExternalServiceInput) (string, error)
-	DeleteExternalService(id string) error
+	UpdateExternalService(input gqltestutil.UpdateExternalServiceInput) (string, error)
+	DeleteExternalService(id string, async bool) error
 
 	SearchRepositories(query string) (gqltestutil.SearchRepositoryResults, error)
 	SearchFiles(query string) (*gqltestutil.SearchFileResults, error)

internal/gqltestutil/external_service.go

@@ -46,21 +46,76 @@ mutation AddExternalService($input: AddExternalServiceInput!) {
 	return resp.Data.AddExternalService.ID, nil
 }
 
+type UpdateExternalServiceInput struct {
+	ID          string  `json:"id"`
+	DisplayName *string `json:"displayName"`
+	Config      *string `json:"config"`
+}
+
+// UpdateExternalService updates existing external service with given input.
+// It returns GraphQL node ID of updated external service.
+//
+// This method requires the authenticated user to be a site admin.
+func (c *Client) UpdateExternalService(input UpdateExternalServiceInput) (string, error) {
+	const query = `
+mutation UpdateExternalService($input: UpdateExternalServiceInput!) {
+	updateExternalService(input: $input) {
+		id
+		warning
+	}
+}
+`
+	variables := map[string]any{
+		"input": input,
+	}
+	var resp struct {
+		Data struct {
+			UpdateExternalService struct {
+				ID      string `json:"id"`
+				Warning string `json:"warning"`
+			} `json:"updateExternalService"`
+		} `json:"data"`
+	}
+	err := c.GraphQL("", query, variables, &resp)
+	if err != nil {
+		return "", errors.Wrap(err, "request GraphQL")
+	}
+
+	// Return the ID along with the warning, so we can still clean up properly.
+	if resp.Data.UpdateExternalService.Warning != "" {
+		return resp.Data.UpdateExternalService.ID, errors.New(resp.Data.UpdateExternalService.Warning)
+	}
+	return resp.Data.UpdateExternalService.ID, nil
+}
+
 // DeleteExternalService deletes the external service by given GraphQL node ID.
 //
 // This method requires the authenticated user to be a site admin.
-func (c *Client) DeleteExternalService(id string) error {
+func (c *Client) DeleteExternalService(id string, async bool) error {
 	const query = `
 mutation DeleteExternalService($externalService: ID!) {
 	 deleteExternalService(externalService: $externalService) {
 		alwaysNil
 	}
 }
+`
+	const asyncQuery = `
+mutation DeleteExternalService($externalService: ID!, $async: Boolean!) {
+	 deleteExternalService(externalService: $externalService, async: $async) {
+		alwaysNil
+	}
+}
 `
 	variables := map[string]any{
 		"externalService": id,
 	}
-	err := c.GraphQL("", query, variables, nil)
+	q := query
+	if async {
+		q = asyncQuery
+		variables["async"] = true
+	}
+
+	err := c.GraphQL("", q, variables, nil)
 	if err != nil {
 		return errors.Wrap(err, "request GraphQL")
 	}