From cf0e34359e817f6020f80ed2a8f4da63bcb72787 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Sun, 25 Oct 2026 13:32:01 -0400 Subject: [PATCH] fix(experiments): do not cache a failed npm lookup checkNPMPackageWithClient cached every outcome for the 2h cache TTL, including request errors, timeouts, 5xx and 429 responses. A single failed lookup during a registry search hid the npm install hint for that repository for two hours, and refreshing the registry cache did not help because the guess entries live under their own npm: keys. Cache only the definitive answers (200 and 404). --- internal/experiments/guesser.go | 7 +- .../guesser_transient_cache_test.go | 95 +++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 internal/experiments/guesser_transient_cache_test.go diff --git a/internal/experiments/guesser.go b/internal/experiments/guesser.go index e09a29f3d..716c538f5 100644 --- a/internal/experiments/guesser.go +++ b/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 } diff --git a/internal/experiments/guesser_transient_cache_test.go b/internal/experiments/guesser_transient_cache_test.go new file mode 100644 index 000000000..2df048d49 --- /dev/null +++ b/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) + }) + } +}