fix(azuredevops_go): skip failing service connections when listing remote repositories

backend/plugins/azuredevops_go/api/remote_helper.go

@@ -82,12 +82,16 @@ func listAzuredevopsRemoteScopes(
 	}
 
 	if projectId != "" {
-		if remote, err := listRemoteRepos(vsc, orgId, projectId); err == nil {
+		if remote, listErr := listRemoteRepos(vsc, orgId, projectId); listErr == nil {
 			children = append(children, remote...)
+		} else {
+			basicRes.GetLogger().Warn(listErr, "failed to list the external repositories of project %s", projectId)
 		}
 
-		if remote, err := listAzuredevopsRepos(vsc, orgId, projectId); err == nil {
+		if remote, listErr := listAzuredevopsRepos(vsc, orgId, projectId); listErr == nil {
 			children = append(children, remote...)
+		} else {
+			basicRes.GetLogger().Warn(listErr, "failed to list the Azure DevOps repositories of project %s", projectId)
 		}
 	}
 	return children, nextPage, nil
@@ -235,19 +239,25 @@ func listRemoteRepos(
 			Provider:        v.Type,
 			ServiceEndpoint: v.Id,
 		}
+		endpointName := v.Name
 
 		g.Go(func() error {
 			repos, err := vsc.GetRemoteRepositories(remoteRepoArgs)
+			if err != nil {
+				// A single service connection that cannot be queried (e.g. expired credentials) must not
+				// hide the repositories of all the other service connections, so skip it and keep going.
+				basicRes.GetLogger().Warn(err, "failed to list the repositories of service connection %q (%s), skipping it", endpointName, remoteRepoArgs.Provider)
+				return nil
+			}
 			mu.Lock()
 			remoteRepos = append(remoteRepos, repos...)
 			mu.Unlock()
-			return err
+			return nil
 		})
 	}
 
-	if err := g.Wait(); err != nil {
-		return nil, errors.Internal.Wrap(err, "failed to call 'GetRemoteRepositories', falling back to empty list")
-	}
+	// The goroutines never return an error, failures are logged and skipped above.
+	_ = g.Wait()
 
 	for _, v := range remoteRepos {
 		pID := orgId + idSeparator + projectId

backend/plugins/azuredevops_go/api/remote_helper_test.go

@@ -0,0 +1,94 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/apache/devlake/core/errors"
+	"github.com/apache/devlake/core/plugin"
+	mockcontext "github.com/apache/devlake/mocks/core/context"
+	mocklog "github.com/apache/devlake/mocks/core/log"
+	"github.com/apache/devlake/plugins/azuredevops_go/api/azuredevops"
+	"github.com/apache/devlake/plugins/azuredevops_go/models"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+)
+
+// fakeApiClient answers GET requests from a fixed map of "path?serviceEndpointId" to response bodies.
+// A missing entry is returned as a request error.
+type fakeApiClient struct {
+	plugin.ApiClient
+	responses map[string]string
+}
+
+func (f *fakeApiClient) Get(path string, query url.Values, _ http.Header) (*http.Response, errors.Error) {
+	key := path
+	if id := query.Get("serviceEndpointId"); id != "" {
+		key += "?" + id
+	}
+	body, ok := f.responses[key]
+	if !ok {
+		return nil, errors.Unauthorized.New("request failed: " + key)
+	}
+	req, _ := http.NewRequest(http.MethodGet, "https://dev.azure.com/"+path, nil)
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     http.Header{},
+		Body:       io.NopCloser(strings.NewReader(body)),
+		Request:    req,
+	}, nil
+}
+
+func TestListRemoteReposSkipsFailingServiceConnection(t *testing.T) {
+	// the failing service connection must be logged once and then skipped
+	logger := new(mocklog.Logger)
+	logger.On("Warn", mock.Anything, mock.Anything, mock.Anything).Once()
+	res := new(mockcontext.BasicRes)
+	res.On("GetLogger").Return(logger)
+	previous := basicRes
+	basicRes = res
+	defer func() { basicRes = previous }()
+
+	client := &fakeApiClient{responses: map[string]string{
+		"org/project/_apis/serviceendpoint/endpoints/": `{"value": [
+			{"id": "broken-endpoint", "name": "broken", "type": "bitbucket"},
+			{"id": "working-endpoint", "name": "working", "type": "github"}
+		]}`,
+		"org/project/_apis/sourceProviders/github/repositories/?working-endpoint": `{"repositories": [
+			{"id": "octo/repo", "sourceProviderName": "GitHub", "name": "repo", "fullName": "octo/repo",
+			 "properties": {"connectedServiceId": "working-endpoint", "externalId": "123"}}
+		]}`,
+		// no entry for the broken endpoint: its request fails
+	}}
+	vsc := azuredevops.NewClient(&models.AzuredevopsConnection{}, client, "")
+
+	children, err := listRemoteRepos(vsc, "org", "project")
+
+	assert.Nil(t, err)
+	if assert.Len(t, children, 1) {
+		assert.Equal(t, "octo/repo", children[0].FullName)
+		assert.Equal(t, "working-endpoint-123", children[0].Data.Id)
+		assert.Equal(t, "org/project", *children[0].ParentId)
+	}
+	logger.AssertExpectations(t)
+}