Repository navigation
Conversation
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).
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Dumbris
approved these changes
Oct 26, 2026
Dumbris
left a comment
Member
There was a problem hiding this comment.
Verified: the PR test fails on main for the reason described and passes on this branch merged with main; race tests for the touched packages and CI lint (bare and server tags) are clean. Also built and tested together with the rest of this batch, independently reviewed, and cross-reviewed with a second model.
Member
|
Merged. Thank you, @rudycelekli! A transient npm lookup failure is no longer cached as a permanent miss. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 npminstall_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 theregistry-servers:keys, while the guess entries live undernpm:<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.godrivescheckNPMPackageWithClientwith a scriptedhttp.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/: okCGO_ENABLED=0 go vet ./internal/experiments/: okmain(and again withguesser.goreverted), all threeTransientFailureNotCachedsubtests fail:expected: 2, actual: 1transport calls, because the second call returned the cached failure.