fix(experiments): do not cache a failed npm lookup (#1787)

## Bug

`Guesser.checkNPMPackageWithClient` (`internal/experiments/guesser.go`)
writes its result to the cache manager on every exit path, including the
failure paths: request errors, timeouts, non-200/404 statuses (5xx,
429), and an unparseable body. Those entries use the default 2h TTL, and
the cache is checked before the network on the next call.

A failed lookup tells us nothing about whether the package exists, but
it gets cached as `exists: false`. For two hours, every registry search
that involves that repository loses its npm `install_cmd` /
`repository_info`.

## Trigger

The runtime's registry search (`Runtime.SearchRegistryServers`) builds a
guesser with the shared cache manager. Batch lookups use a 3s client
timeout. One slow npm response, a brief network drop or an npm 5xx
during a search caches the failure. Refreshing the registry
(`RefreshRegistryCache`) does not help, because it only drops the
`registry-servers:` keys, while the guess entries live under
`npm:<package>`.

## Fix

Cache only the definitive answers: 200 (package found) and 404 (package
does not exist). Failed lookups are returned without being cached, so
the next search tries again.

## Test

`guesser_transient_cache_test.go` drives `checkNPMPackageWithClient`
with a scripted `http.RoundTripper`, so it makes no real network calls:

- `TestCheckNPMPackage_TransientFailureNotCached`: the first response is
a network error, a 503 or a 429, and the second is a 200. The second
call must reach the transport and report the package as found.
- `TestCheckNPMPackage_DefinitiveAnswersCached`: a 200 and a 404 are
each still served from the cache on the second call.

## Commands run

- `CGO_ENABLED=0 go test -count=1 -run
'TestCheckNPMPackage_(TransientFailureNotCached|DefinitiveAnswersCached)|TestGuesser_CacheEntriesAreStampedInternal|TestCaching|TestNilCacheManager'
./internal/experiments/`: ok
- `CGO_ENABLED=0 go vet ./internal/experiments/`: ok
- On current `main` (and again with `guesser.go` reverted), all three
`TransientFailureNotCached` subtests fail: `expected: 2, actual: 1`
transport calls, because the second call returned the cached failure.

Author: rudycelekli

internal/experiments/guesser.go

@@ -278,7 +278,6 @@ func (g *Guesser) checkNPMPackageWithClient(ctx context.Context, packageName str
 	req, err := http.NewRequestWithContext(ctx, "GET", npmURL, http.NoBody)
 	if err != nil {
 		info.Error = fmt.Sprintf("Failed to create request: %v", err)
-		g.cacheInfo(cacheKey, info)
 		return info
 	}
 
@@ -288,11 +287,13 @@ func (g *Guesser) checkNPMPackageWithClient(ctx context.Context, packageName str
 	resp, err := client.Do(req)
 	if err != nil {
 		info.Error = fmt.Sprintf("Request failed: %v", err)
-		g.cacheInfo(cacheKey, info)
 		return info
 	}
 	defer resp.Body.Close()
 
+	// Only a 200 or a 404 is cached. A failed lookup (timeout, network error,
+	// 5xx, 429) says nothing about the package, and caching it would hide the
+	// npm hint for the whole TTL, past a registry cache refresh.
 	if resp.StatusCode == 404 {
 		// Package doesn't exist
 		g.cacheInfo(cacheKey, info)
@@ -301,14 +302,12 @@ func (g *Guesser) checkNPMPackageWithClient(ctx context.Context, packageName str
 
 	if resp.StatusCode != 200 {
 		info.Error = fmt.Sprintf("HTTP %d: %s", resp.StatusCode, resp.Status)
-		g.cacheInfo(cacheKey, info)
 		return info
 	}
 
 	var npmInfo NPMPackageInfo
 	if err := json.NewDecoder(resp.Body).Decode(&npmInfo); err != nil {
 		info.Error = fmt.Sprintf("Failed to parse response: %v", err)
-		g.cacheInfo(cacheKey, info)
 		return info
 	}
 

internal/experiments/guesser_transient_cache_test.go

@@ -0,0 +1,95 @@
+package experiments
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// scriptedTransport answers each request with the next scripted response.
+type scriptedTransport struct {
+	responses []func(*http.Request) (*http.Response, error)
+	calls     int
+}
+
+func (s *scriptedTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+	i := s.calls
+	s.calls++
+	if i >= len(s.responses) {
+		return nil, errors.New("unexpected request")
+	}
+	return s.responses[i](r)
+}
+
+func npmStatus(code int, body string) func(*http.Request) (*http.Response, error) {
+	return func(r *http.Request) (*http.Response, error) {
+		return &http.Response{
+			StatusCode: code,
+			Status:     http.StatusText(code),
+			Body:       io.NopCloser(strings.NewReader(body)),
+			Header:     http.Header{"Content-Type": []string{"application/json"}},
+			Request:    r,
+		}, nil
+	}
+}
+
+const npmFoundBody = `{"name":"@acme/tool","description":"d","dist-tags":{"latest":"1.0.0"}}`
+
+// A failed lookup (timeout, network error, 5xx) says nothing about whether the
+// package exists, so it must not be cached for the 2h TTL: a registry refresh
+// does not clear the guess cache, and the npm install hint would stay missing.
+func TestCheckNPMPackage_TransientFailureNotCached(t *testing.T) {
+	cases := map[string]func(*http.Request) (*http.Response, error){
+		"network error": func(*http.Request) (*http.Response, error) {
+			return nil, errors.New("dial tcp: i/o timeout")
+		},
+		"server error": npmStatus(http.StatusServiceUnavailable, ""),
+		"rate limited": npmStatus(http.StatusTooManyRequests, ""),
+	}
+	for name, failure := range cases {
+		t.Run(name, func(t *testing.T) {
+			guesser, db := setupTestGuesser(t)
+			defer db.Close()
+
+			rt := &scriptedTransport{responses: []func(*http.Request) (*http.Response, error){
+				failure,
+				npmStatus(http.StatusOK, npmFoundBody),
+			}}
+			client := &http.Client{Transport: rt}
+
+			first := guesser.checkNPMPackageWithClient(context.Background(), "@acme/tool", client)
+			require.False(t, first.Exists)
+
+			second := guesser.checkNPMPackageWithClient(context.Background(), "@acme/tool", client)
+			assert.Equal(t, 2, rt.calls, "the retry must reach the registry, not the cached failure")
+			assert.True(t, second.Exists)
+			assert.Equal(t, "1.0.0", second.Version)
+		})
+	}
+}
+
+func TestCheckNPMPackage_DefinitiveAnswersCached(t *testing.T) {
+	for name, answer := range map[string]func(*http.Request) (*http.Response, error){
+		"found":     npmStatus(http.StatusOK, npmFoundBody),
+		"not found": npmStatus(http.StatusNotFound, ""),
+	} {
+		t.Run(name, func(t *testing.T) {
+			guesser, db := setupTestGuesser(t)
+			defer db.Close()
+
+			rt := &scriptedTransport{responses: []func(*http.Request) (*http.Response, error){answer}}
+			client := &http.Client{Transport: rt}
+
+			first := guesser.checkNPMPackageWithClient(context.Background(), "@acme/tool", client)
+			second := guesser.checkNPMPackageWithClient(context.Background(), "@acme/tool", client)
+			assert.Equal(t, 1, rt.calls)
+			assert.Equal(t, first.Exists, second.Exists)
+		})
+	}
+}