Apply cached config during startup#543
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCached config loading no longer emits an initial event. Radiance now applies the on-disk config after subscribers are registered, using helpers to restore country settings and rebuild servers from cached config. A new test verifies cached servers are loaded. ChangesCached config application
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a startup race where the cached-on-disk config could be loaded before NewConfigEvent subscribers were attached, which meant cached servers weren’t applied if a subsequent config fetch failed. The fix applies any already-loaded config snapshot during backend startup (after subscriptions are registered), and adds a regression test.
Changes:
- Stop emitting
NewConfigEventas a side effect of loading the cached config from disk. - Refactor config-application logic in
LocalBackend.Start()intoapplyCurrentConfig()+applyConfig()helpers and apply cached config during startup. - Add a test that verifies cached servers are available immediately after applying the current config.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/config.go | Removes cached-config event emission during disk load, preventing reliance on a timing-sensitive startup side effect. |
| backend/radiance.go | Applies the already-loaded config snapshot during startup and consolidates config-to-runtime update logic into helpers. |
| backend/radiance_test.go | Adds coverage to ensure cached servers are applied/available without waiting for a fresh config fetch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/radiance.go`:
- Around line 233-245: The startup preload in LocalBackend.applyCurrentConfig is
leaving stale servers in r.srvManager because updateServers later skips any
server whose tag already exists. Update the refresh path so same-tag servers
from the newly fetched config replace the cached ones instead of being ignored,
using the existing applyConfig/updateServers flow and the tag-based lookup logic
in LocalBackend to ensure newer options win.
- Around line 233-245: The cached-config bootstrap path in applyCurrentConfig
only calls setCountryCodeFromConfig and applyConfig, so offline URL prewarming
never runs when startup is blocked before live config events arrive. Update
applyCurrentConfig (or the startup flow that calls it) to invoke
RunOfflineURLTests after the cached config is applied, using the existing
LocalBackend methods so the offline prewarm happens for cached servers as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a820d132-8c31-4491-bd57-d4873f9f6327
📒 Files selected for processing (3)
backend/radiance.gobackend/radiance_test.goconfig/config.go
💤 Files with no reviewable changes (1)
- config/config.go
| r.applyCurrentConfig() | ||
| r.confHandler.Start() | ||
| } | ||
|
|
||
| // applyCurrentConfig applies any config already loaded from disk before the | ||
| // fetch loop has a chance to refresh it. | ||
| func (r *LocalBackend) applyCurrentConfig() { | ||
| cfg, err := r.confHandler.GetConfig() | ||
| if err != nil { | ||
| return | ||
| } | ||
| setCountryCodeFromConfig(cfg) | ||
| r.applyConfig(cfg) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Cached startup state now blocks same-tag live config refreshes.
Applying the on-disk config before confHandler.Start() pre-populates r.srvManager, but updateServers() later drops every incoming server whose tag already exists on Lines 648-650. If the first fresh config reuses the cached tags, its newer options never replace the cached ones, so startup can stay pinned to stale endpoints until tags rotate.
Suggested direction
-// prune any servers from the incoming list that already exists to avoid deleting
-// selection history results and closing existing connections
-existingTags := serverTagSet(existing)
-list.Servers = slices.DeleteFunc(list.Servers, func(srv *servers.Server) bool {
- _, exists := existingTags[srv.Tag]
- return exists
-})
+// preserve selection history, but still let fresh config replace cached Lantern entries
+existingByTag := map[string]*servers.Server{}
+for _, srv := range existing {
+ existingByTag[srv.Tag] = srv
+}
+for _, srv := range list.Servers {
+ if prev, ok := existingByTag[srv.Tag]; ok {
+ srv.SelectionHistory = prev.SelectionHistory
+ srv.Credentials = prev.Credentials
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/radiance.go` around lines 233 - 245, The startup preload in
LocalBackend.applyCurrentConfig is leaving stale servers in r.srvManager because
updateServers later skips any server whose tag already exists. Update the
refresh path so same-tag servers from the newly fetched config replace the
cached ones instead of being ignored, using the existing
applyConfig/updateServers flow and the tag-based lookup logic in LocalBackend to
ensure newer options win.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run offline URL tests after applying the cached config.
applyCurrentConfig() only seeds setCountryCodeFromConfig() and applyConfig(), while RunOfflineURLTests() is still only triggered inside the live NewConfigEvent subscriber on Lines 223-230. In the blocked-bootstrap case this PR targets, that means cached servers are present but the offline prewarm never runs, so startup can still stay stuck on the cached-only path.
Suggested fix
func (r *LocalBackend) applyCurrentConfig() {
cfg, err := r.confHandler.GetConfig()
if err != nil {
return
}
setCountryCodeFromConfig(cfg)
r.applyConfig(cfg)
+ if err := r.RunOfflineURLTests(); err != nil && !errors.Is(err, vpn.ErrTunnelAlreadyConnected) {
+ slog.Error("Failed to run offline URL tests after applying cached config", "error", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r.applyCurrentConfig() | |
| r.confHandler.Start() | |
| } | |
| // applyCurrentConfig applies any config already loaded from disk before the | |
| // fetch loop has a chance to refresh it. | |
| func (r *LocalBackend) applyCurrentConfig() { | |
| cfg, err := r.confHandler.GetConfig() | |
| if err != nil { | |
| return | |
| } | |
| setCountryCodeFromConfig(cfg) | |
| r.applyConfig(cfg) | |
| r.applyCurrentConfig() | |
| r.confHandler.Start() | |
| } | |
| // applyCurrentConfig applies any config already loaded from disk before the | |
| // fetch loop has a chance to refresh it. | |
| func (r *LocalBackend) applyCurrentConfig() { | |
| cfg, err := r.confHandler.GetConfig() | |
| if err != nil { | |
| return | |
| } | |
| setCountryCodeFromConfig(cfg) | |
| r.applyConfig(cfg) | |
| if err := r.RunOfflineURLTests(); err != nil && !errors.Is(err, vpn.ErrTunnelAlreadyConnected) { | |
| slog.Error("Failed to run offline URL tests after applying cached config", "error", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/radiance.go` around lines 233 - 245, The cached-config bootstrap path
in applyCurrentConfig only calls setCountryCodeFromConfig and applyConfig, so
offline URL prewarming never runs when startup is blocked before live config
events arrive. Update applyCurrentConfig (or the startup flow that calls it) to
invoke RunOfflineURLTests after the cached config is applied, using the existing
LocalBackend methods so the offline prewarm happens for cached servers as well.
Fixes a startup race where cached config could be loaded before config-event subscribers were attached, leaving cached servers unavailable if a fresh config fetch failed.
Resolves https://github.com/getlantern/engineering/issues/3640
Summary by CodeRabbit
New Features
Bug Fixes
Tests