httpcli: make caching transport unwrappable (#49877)

For some reason, we had panics in production because caching transport wasnt unwrappable. Being unwrappable seems benign, so we'll implement it for that ``` panic: httpcli.ExternalTransportOpt: http.Client.Transport cannot be cast as a *http.Transport: *httpcache.Transport goroutine 7639 [running]: github.com/sourcegraph/sourcegraph/internal/extsvc/crates.NewClient({0xc0024c4a68, 0x15}, 0xc0002120c0) /home/noah/Sourcegraph/sourcegraph/internal/extsvc/crates/client.go:23 +0x125 github.com/sourcegraph/sourcegraph/cmd/gitserver/shared.getVCSSyncer({0x2556538, 0xc0029bc690}, {0x256ba50, 0xc0024c1ad0}, {0x25698a0, 0xc002402438}, 0x11?, {0xc002ccc378, 0x11}, {0xc00006045e, ...}, ...) /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/shared/shared.go:518 +0x830 github.com/sourcegraph/sourcegraph/cmd/gitserver/shared.Main.func2({0x2556538?, 0xc0029bc690?}, {0xc002ccc378?, 0xc002ccc378?}) /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/shared/shared.go:147 +0x89 github.com/sourcegraph/sourcegraph/cmd/gitserver/server.(*Server).doBackgroundRepoUpdate(0xc00244db00, {0xc002ccc348, 0x11}, {0x0, 0x0}) /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/server/server.go:2703 +0x3e6 github.com/sourcegraph/sourcegraph/cmd/gitserver/server.(*Server).doRepoUpdate.func1.1() /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/server/server.go:2639 +0x1d2 sync.(*Once).doSlow(0x0?, 0x4a34ea?) /nix/store/dnvng8cdwc2i6g48qxijqmch3w8l3s85-go-1.20/share/go/src/sync/once.go:74 +0xc2 sync.(*Once).Do(...) /nix/store/dnvng8cdwc2i6g48qxijqmch3w8l3s85-go-1.20/share/go/src/sync/once.go:65 github.com/sourcegraph/sourcegraph/cmd/gitserver/server.(*Server).doRepoUpdate.func1() /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/server/server.go:2631 +0x125 created by github.com/sourcegraph/sourcegraph/cmd/gitserver/server.(*Server).doRepoUpdate /home/noah/Sourcegraph/sourcegraph/cmd/gitserver/server/server.go:2629 +0x4b8 ``` ## Test plan Ran locally, no more panic
sourcegraph · Mar 22, 2023 · 0bdcf03 · 0bdcf03
1 parent 4a5a592
commit 0bdcf03
Show file tree

Hide file tree

Showing 14 changed files with 85 additions and 33 deletions.
diff --git a/cmd/gitserver/server/vcs_syncer_python_packages_test.go b/cmd/gitserver/server/vcs_syncer_python_packages_test.go
@@ -266,6 +266,6 @@ func newTestClient(t testing.TB, name string, update bool) *pypi.Client {
 
 	doer := httpcli.NewFactory(nil, httptestutil.NewRecorderOpt(rec))
 
-	c := pypi.NewClient("urn", []string{"https://pypi.org/simple"}, doer)
+	c, _ := pypi.NewClient("urn", []string{"https://pypi.org/simple"}, doer)
 	return c
 }
diff --git a/cmd/gitserver/shared/shared.go b/cmd/gitserver/shared/shared.go
@@ -491,7 +491,10 @@ func getVCSSyncer(
 		if err != nil {
 			return nil, err
 		}
-		cli := npm.NewHTTPClient(urn, c.Registry, c.Credentials, httpcli.ExternalClientFactory)
+		cli, err := npm.NewHTTPClient(urn, c.Registry, c.Credentials, httpcli.ExternalClientFactory)
+		if err != nil {
+			return nil, err
+		}
 		return server.NewNpmPackagesSyncer(c, depsSvc, cli), nil
 	case extsvc.TypeGoModules:
 		var c schema.GoModulesConnection
@@ -507,23 +510,32 @@ func getVCSSyncer(
 		if err != nil {
 			return nil, err
 		}
-		cli := pypi.NewClient(urn, c.Urls, httpcli.ExternalClientFactory)
+		cli, err := pypi.NewClient(urn, c.Urls, httpcli.ExternalClientFactory)
+		if err != nil {
+			return nil, err
+		}
 		return server.NewPythonPackagesSyncer(&c, depsSvc, cli, reposDir), nil
 	case extsvc.TypeRustPackages:
 		var c schema.RustPackagesConnection
 		urn, err := extractOptions(&c)
 		if err != nil {
 			return nil, err
 		}
-		cli := crates.NewClient(urn, httpcli.ExternalClientFactory)
+		cli, err := crates.NewClient(urn, httpcli.ExternalClientFactory)
+		if err != nil {
+			return nil, err
+		}
 		return server.NewRustPackagesSyncer(&c, depsSvc, cli), nil
 	case extsvc.TypeRubyPackages:
 		var c schema.RubyPackagesConnection
 		urn, err := extractOptions(&c)
 		if err != nil {
 			return nil, err
 		}
-		cli := rubygems.NewClient(urn, c.Repository, httpcli.ExternalClientFactory)
+		cli, err := rubygems.NewClient(urn, c.Repository, httpcli.ExternalClientFactory)
+		if err != nil {
+			return nil, err
+		}
 		return server.NewRubyPackagesSyncer(&c, depsSvc, cli), nil
 	}
 	return &server.GitRepoSyncer{}, nil

diff --git a/internal/extsvc/crates/client.go b/internal/extsvc/crates/client.go
@@ -17,12 +17,15 @@ type Client struct {
 	limiter *ratelimit.InstrumentedLimiter
 }
 
-func NewClient(urn string, httpfactory *httpcli.Factory) *Client {
-	uncached, _ := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+func NewClient(urn string, httpfactory *httpcli.Factory) (*Client, error) {
+	uncached, err := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+	if err != nil {
+		return nil, err
+	}
 	return &Client{
 		uncachedClient: uncached,
 		limiter:        ratelimit.DefaultRegistry.Get(urn),
-	}
+	}, nil
 }
 
 func (c *Client) Get(ctx context.Context, url string) (io.ReadCloser, error) {

diff --git a/internal/extsvc/npm/npm.go b/internal/extsvc/npm/npm.go
@@ -57,16 +57,22 @@ type HTTPClient struct {
 
 var _ Client = &HTTPClient{}
 
-func NewHTTPClient(urn string, registryURL string, credentials string, httpfactory *httpcli.Factory) *HTTPClient {
-	uncached, _ := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
-	cached, _ := httpfactory.Doer()
+func NewHTTPClient(urn string, registryURL string, credentials string, httpfactory *httpcli.Factory) (*HTTPClient, error) {
+	uncached, err := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+	if err != nil {
+		return nil, err
+	}
+	cached, err := httpfactory.Doer()
+	if err != nil {
+		return nil, err
+	}
 	return &HTTPClient{
 		registryURL:    registryURL,
 		uncachedClient: uncached,
 		cachedClient:   cached,
 		limiter:        ratelimit.DefaultRegistry.Get(urn),
 		credentials:    credentials,
-	}
+	}, nil
 }
 
 type PackageInfo struct {

diff --git a/internal/extsvc/npm/npm_test.go b/internal/extsvc/npm/npm_test.go
@@ -34,7 +34,7 @@ func newTestHTTPClient(t *testing.T) (client *HTTPClient, stop func()) {
 	t.Helper()
 	recorderFactory, stop := httptestutil.NewRecorderFactory(t, *updateRecordings, t.Name())
 
-	client = NewHTTPClient("urn", "https://registry.npmjs.org", "", recorderFactory)
+	client, _ = NewHTTPClient("urn", "https://registry.npmjs.org", "", recorderFactory)
 	return client, stop
 }
 
@@ -73,7 +73,7 @@ func TestCredentials(t *testing.T) {
 	defer server.Close()
 
 	ctx := context.Background()
-	client := NewHTTPClient("urn", server.URL, credentials, httpcli.ExternalClientFactory)
+	client, _ := NewHTTPClient("urn", server.URL, credentials, httpcli.ExternalClientFactory)
 
 	presentDep, err := reposource.ParseNpmVersionedPackage("left-pad@1.3.0")
 	require.NoError(t, err)

diff --git a/internal/extsvc/pypi/client.go b/internal/extsvc/pypi/client.go
@@ -56,15 +56,21 @@ type Client struct {
 	limiter *ratelimit.InstrumentedLimiter
 }
 
-func NewClient(urn string, urls []string, httpfactory *httpcli.Factory) *Client {
-	uncached, _ := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
-	cached, _ := httpfactory.Doer()
+func NewClient(urn string, urls []string, httpfactory *httpcli.Factory) (*Client, error) {
+	uncached, err := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+	if err != nil {
+		return nil, err
+	}
+	cached, err := httpfactory.Doer()
+	if err != nil {
+		return nil, err
+	}
 	return &Client{
 		urls:           urls,
 		uncachedClient: uncached,
 		cachedClient:   cached,
 		limiter:        ratelimit.DefaultRegistry.Get(urn),
-	}
+	}, nil
 }
 
 // Project returns the Files of the simple-API /<project>/ endpoint.

diff --git a/internal/extsvc/pypi/client_test.go b/internal/extsvc/pypi/client_test.go
@@ -448,6 +448,6 @@ func newTestClient(t testing.TB, name string, update bool) *Client {
 
 	doer := httpcli.NewFactory(nil, httptestutil.NewRecorderOpt(rec))
 
-	c := NewClient("urn", []string{"https://pypi.org/simple"}, doer)
+	c, _ := NewClient("urn", []string{"https://pypi.org/simple"}, doer)
 	return c
 }
diff --git a/internal/extsvc/rubygems/client.go b/internal/extsvc/rubygems/client.go
@@ -22,13 +22,16 @@ type Client struct {
 	limiter *ratelimit.InstrumentedLimiter
 }
 
-func NewClient(urn string, registryURL string, httpfactory *httpcli.Factory) *Client {
-	uncached, _ := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+func NewClient(urn string, registryURL string, httpfactory *httpcli.Factory) (*Client, error) {
+	uncached, err := httpfactory.Doer(httpcli.NewCachedTransportOpt(httpcli.NoopCache{}, false))
+	if err != nil {
+		return nil, err
+	}
 	return &Client{
 		registryURL:    registryURL,
 		uncachedClient: uncached,
 		limiter:        ratelimit.DefaultRegistry.Get(urn),
-	}
+	}, nil
 }
 
 func (c *Client) GetPackageContents(ctx context.Context, dep reposource.VersionedPackage) (body io.ReadCloser, err error) {

diff --git a/internal/extsvc/rubygems/client_test.go b/internal/extsvc/rubygems/client_test.go
@@ -28,7 +28,8 @@ func newTestHTTPClient(t *testing.T) (client *Client, stop func()) {
 	t.Helper()
 	recorderFactory, stop := httptestutil.NewRecorderFactory(t, *updateRecordings, t.Name())
 
-	return NewClient("rubygems_urn", "https://rubygems.org", recorderFactory), stop
+	client, _ = NewClient("rubygems_urn", "https://rubygems.org", recorderFactory)
+	return client, stop
 }
 
 func TestGetPackageContents(t *testing.T) {

diff --git a/internal/httpcli/client.go b/internal/httpcli/client.go
@@ -438,10 +438,13 @@ func NewCachedTransportOpt(c httpcache.Cache, markCachedResponses bool) Opt {
 			cli.Transport = http.DefaultTransport
 		}
 
-		cli.Transport = &httpcache.Transport{
-			Transport:           cli.Transport,
-			Cache:               c,
-			MarkCachedResponses: markCachedResponses,
+		cli.Transport = &wrappedTransport{
+			RoundTripper: &httpcache.Transport{
+				Transport:           cli.Transport,
+				Cache:               c,
+				MarkCachedResponses: markCachedResponses,
+			},
+			Wrapped: cli.Transport,
 		}
 
 		return nil

diff --git a/internal/repos/npm_packages.go b/internal/repos/npm_packages.go
@@ -27,14 +27,17 @@ func NewNpmPackagesSource(ctx context.Context, svc *types.ExternalService, cf *h
 		return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err)
 	}
 
+	client, err := npm.NewHTTPClient(svc.URN(), c.Registry, c.Credentials, cf)
+	if err != nil {
+		return nil, err
+	}
+
 	return &PackagesSource{
 		svc:        svc,
 		configDeps: c.Dependencies,
 		scheme:     dependencies.NpmPackagesScheme,
 		/* depsSvc initialized in SetDependenciesService */
-		src: &npmPackagesSource{
-			client: npm.NewHTTPClient(svc.URN(), c.Registry, c.Credentials, cf),
-		},
+		src: &npmPackagesSource{client},
 	}, nil
 }
 

diff --git a/internal/repos/python_packages.go b/internal/repos/python_packages.go
@@ -25,11 +25,16 @@ func NewPythonPackagesSource(ctx context.Context, svc *types.ExternalService, cf
 		return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err)
 	}
 
+	client, err := pypi.NewClient(svc.URN(), c.Urls, cf)
+	if err != nil {
+		return nil, err
+	}
+
 	return &PackagesSource{
 		svc:        svc,
 		configDeps: c.Dependencies,
 		scheme:     dependencies.PythonPackagesScheme,
-		src:        &pythonPackagesSource{client: pypi.NewClient(svc.URN(), c.Urls, cf)},
+		src:        &pythonPackagesSource{client},
 	}, nil
 }
 

diff --git a/internal/repos/ruby_packages.go b/internal/repos/ruby_packages.go
@@ -25,11 +25,16 @@ func NewRubyPackagesSource(ctx context.Context, svc *types.ExternalService, cf *
 		return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err)
 	}
 
+	client, err := rubygems.NewClient(svc.URN(), c.Repository, cf)
+	if err != nil {
+		return nil, err
+	}
+
 	return &PackagesSource{
 		svc:        svc,
 		configDeps: c.Dependencies,
 		scheme:     dependencies.RubyPackagesScheme,
-		src:        &rubyPackagesSource{client: rubygems.NewClient(svc.URN(), c.Repository, cf)},
+		src:        &rubyPackagesSource{client},
 	}, nil
 }
 

diff --git a/internal/repos/rust_packages.go b/internal/repos/rust_packages.go
@@ -25,11 +25,16 @@ func NewRustPackagesSource(ctx context.Context, svc *types.ExternalService, cf *
 		return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err)
 	}
 
+	client, err := crates.NewClient(svc.URN(), cf)
+	if err != nil {
+		return nil, err
+	}
+
 	return &PackagesSource{
 		svc:        svc,
 		configDeps: c.Dependencies,
 		scheme:     dependencies.RustPackagesScheme,
-		src:        &rustPackagesSource{client: crates.NewClient(svc.URN(), cf)},
+		src:        &rustPackagesSource{client},
 	}, nil
 }