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

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)
+		})
+	}
+}