fix: use cached bootstrap registry files instead of downloading - #55
Conversation
Lookup() only parsed a cached Service Registry file into memory when the cache reported ShouldReload, a state only DiskCache returns. Two bugs followed from that: A caller-populated cache was ignored. MemoryCache reports Good straight after Save(), so a registry embedded with //go:embed left registries[type] nil and triggered a download that also overwrote the embedded copy. An expired file was never refreshed. Once parsed, registries[type] was non-nil and Expired was ignored, so a long lived Client served the same file forever, contrary to the documented 24 hour refresh. Load from the cache whenever nothing is parsed yet and the cached copy is usable, force a download once it expires, and fall back to the expired copy if that download fails. Registry files change rarely, so a stale copy beats a failed lookup, and this makes the "returned even if its State() is Expired" contract of both caches reachable. Fixes #54
|
Warning Review limit reachedNext included review available in 46 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe bootstrap client loads pre-populated cache files into memory. It refreshes expired cache entries and uses cached registry data when downloads fail. New tests cover cache loading, refresh, fallback, and corrupt-cache errors. The module now targets Go 1.26.0, updates two dependencies, and tests only Go 1.26.x. Merge Risk: 🔵 Low · up to The bootstrap client now uses embedded or cached registry data and falls back to stale data after refresh failures. Merge risk is low, but two cache tests may not detect download-behavior regressions, and the CI version comment is inaccurate. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The bootstrap client now loads usable pre-populated cache files, avoids unnecessary downloads, refreshes expired files, and falls back to stale cached data when downloads fail. These changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: e7566881-e48e-41fd-8c69-7fe2da251ac9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
bootstrap/client.gobootstrap/client_test.gogo.mod
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 1c8fbf31-e5d5-4cb7-8d07-61a0224805e7
📒 Files selected for processing (1)
.github/workflows/tests.yml
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
Both tests only checked the answer, so they passed for the wrong reasons. TestLookupUsesPrePopulatedCache would stay green if Lookup downloaded anyway, because the 404 hits the expired-copy fallback and keeps the already-loaded registry. TestLookupFallsBackToExpiredCache would stay green if the refresh attempt were skipped entirely. Assert a httpmock call delta of 0 and 1 respectively.
1 participant
Fixes #54.
Lookup()only loaded a cached Service Registry file into memory when the cache reportedShouldReload, which onlyDiskCacheever returns. A caller who pre-populates aMemoryCache(e.g. with a//go:embededdns.json) therefore got a download anyway, which also overwrote their embedded copy.Fixing that surfaced a second bug:
Expiredwas handled nowhere, so a long-livedClientnever refreshed its registries at all, contrary to the documented 24 hour refresh. Expired files are now re-downloaded, falling back to the stale cached copy if the download fails, since registry files change rarely and a stale copy beats a failed lookup.Three regression tests, each verified to fail without the change, plus a doc example for the embed use case.
Summary by CodeRabbit
New Features
Bug Fixes