Persist fetched config and bootstrap from it across restarts#16
Conversation
domainfront's config updater (WithConfigURL) fetches the config on startup and every 12h and applies it to the in-memory front pool, but never persisted it. So a restart reverted to the seed config passed to New (typically the consumer's embedded copy) until the next background fetch completed — and where the config host is blocked (e.g. raw.githubusercontent.com in China) that fetch never succeeds, so the client was pinned to the embedded seed every run. This makes domainfront own the full config lifecycle — fetch → apply → persist → bootstrap — so consumers don't have to fetch/persist the config themselves (which duplicated the updater's fetch): - WithConfigCacheFile(path) persists each successfully fetched config and, on the next start, bootstraps from it in preference to the seed. - New prefers the persisted config (loadPersistedConfig) over the seed when present; a missing/corrupt cache is ignored and never fails construction. - fetchAndApplyConfig persists the raw bytes after a successful apply. The config carries no generation timestamp, so persisted is preferred unconditionally (as the consumer previously did); the updater refreshes it on startup when the host is reachable. Persistence reuses the same non-atomic writeFile as the fronts cache; a torn write fails to parse next boot and falls back to the seed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe client gains an optional on-disk configuration cache, loads valid cached configuration during construction, persists successfully fetched configuration, and writes cache files with owner-only permissions. Tests cover round trips, fallback behavior, corrupt data, startup precedence, and fetch persistence. ChangesConfiguration persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant CacheFile
HTTPServer->>Client: Return gzipped configuration
Client->>Client: Apply configuration
Client->>CacheFile: Persist fetched bytes
Client->>CacheFile: Load persisted configuration during startup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds persistent caching for remotely fetched fronting configuration so clients can bootstrap from the last-known-good config across restarts (especially important when the config host is unreachable), while keeping construction resilient by falling back to the embedded seed config.
Changes:
- Introduces
WithConfigCacheFile(path)to persist fetched configs and bootstrap from them on subsequent startups. - Updates
New()to prefer a previously persisted config over the provided seed config when available. - Adds
config_persist_test.gocovering round-trip persistence, fallback behaviors, and an end-to-end fetch→apply→persist path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| domainfront.go | Adds config cache path option, bootstrapping from persisted config in New, and persistence on successful config refresh. |
| config_persist_test.go | Adds tests validating persistence, bootstrap preference, and safe fallback behavior when cache is absent/corrupt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := writeFile(c.configCachePath, data); err != nil { | ||
| c.log.Warn("Failed to persist config cache", "path", c.configCachePath, "error", err) | ||
| } |
There was a problem hiding this comment.
Agreed — changed writeFile to 0600. Since the fronts cache shares this helper and reveals the same kind of infrastructure, this hardens both. The parent dir was already created 0700, so this makes all on-disk state owner-only.
There was a problem hiding this comment.
Correction / follow-up: reverted this back to 0644 in e62d602. On some platforms (macOS, Windows) the tunnel and the app are separate processes — potentially running as different users — that both read these cache files, so owner-only 0600 would risk breaking cross-process reads (and this shared writeFile also backs the existing fronts cache). The content is also already fetched from a public URL. Leaving this thread open: hardening to owner-only is worth revisiting but needs per-OS validation of the two-process model first, so I didn't want to bundle a potential functional regression into this PR.
- loadPersistedConfig logs open failures other than "not found" (e.g. permission problems) before falling back to the seed, instead of swallowing them silently; a missing cache stays silent (normal first run). - writeFile now writes 0600 instead of 0644. The cached fronting config and fronts reveal circumvention infrastructure (domains/IPs) and shouldn't be world-readable to other local users on shared devices. This also hardens the existing fronts cache, which shares this helper; the parent dir is already 0700. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 0600 hardening would break cross-process reads: on some platforms the tunnel and app are separate processes (potentially different users) that both read these caches. Keep 0644 (the prior value) and document why. The non-ENOENT open-error logging from the prior commit stays. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
🤖 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 `@config_persist_test.go`:
- Around line 130-138: Update the persistence test’s Eventually condition before
loadPersistedConfig to call loadPersistedConfig and verify it returns a non-nil
config containing “fetchedprovider”; remove the os.Stat-only check so polling
waits for a successfully parsed cache payload.
🪄 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: 27b3ba14-54a5-41c3-af59-595f2e93f245
📒 Files selected for processing (3)
config_persist_test.godomainfront.gofileutil.go
…x comment - New now falls back to the seed config when a persisted cache parses but fails applyConfig (e.g. a torn write that decompresses to YAML with no providers). Previously that returned an error and failed construction, breaking the "a bad cache never fails New" contract. Only a seed that itself won't apply fails New now. - Add TestNew_FallsBackWhenPersistedConfigInvalid (parseable-but-no-providers cache → falls back to seed, no error). - Fix the writeFile perms comment: the 0700 parent dir already gates access regardless of file mode, so the prior "0644 for cross-user reads" wording was misleading. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
fileutil.go:23
- The comment implies the 0700 parent directory always “already gates access regardless of file mode”, but os.MkdirAll does not change permissions on an existing directory. If the parent directory already exists with broader perms (e.g. 0755), the file will be written 0644 and may be readable by other users. This also contradicts the PR description’s “owner-only access” claim (from the auto summary). Clarify the comment to reflect the existing-directory case (or change the file mode if owner-only is required).
// The parent dir is created 0700 (owner-only), which already gates access
// regardless of file mode. Keep the historical 0644 rather than tightening
// to 0600: the tunnel and app processes' on-disk sharing model is OS-specific
// and tightening needs per-OS validation first.
…ist test writeFile is non-atomic, so os.Stat can succeed before the content is fully written. Poll loadPersistedConfig until it parses and contains the fetched provider instead, removing the partial-read flake. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- loadPersistedConfig: drop the 'size-bounded by ParseConfigFromReader' claim; that bounds only the compressed bytes, and unbounded gzip decompression is a pre-existing property of ParseConfig (the network fetch path shares it) — out of scope to change here. - writeFile: note MkdirAll only sets 0700 on a dir it creates, so in a pre-existing shared dir the file mode still matters; keep 0644. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
fileutil.go:25
- The PR description’s auto-generated release notes claim the cached config is now restricted to owner-only access, but writeFile() still writes with mode 0644 (and even documents keeping 0644 intentionally). Please update the PR description/release notes to match the actual behavior, or (if owner-only is required) change the implementation after validating the cross-process sharing model on each OS.
// Keep the historical 0644 rather than tightening to 0600. MkdirAll above
// sets 0700 only on a dir it creates; it leaves an existing dir's perms
// untouched, so in a pre-existing shared dir the file mode still matters —
// and the tunnel and app processes' on-disk sharing model is OS-specific.
// Tightening needs per-OS validation first.
return os.WriteFile(path, data, 0o644)
Consistent with the open/parse-failure logs, so diagnosis isn't harder when the cache path varies by platform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
fileutil.go:25
- The PR description’s auto-generated “Security” bullet claims cached config files are now restricted to owner-only access, but writeFile still writes with mode 0644 (and this helper is used for persisted config). To avoid misleading reviewers/operators, please update/remove that claim in the PR description (or, if owner-only is actually desired, change the implementation accordingly after validating the cross-process requirements mentioned here).
// Keep the historical 0644 rather than tightening to 0600. MkdirAll above
// sets 0700 only on a dir it creates; it leaves an existing dir's perms
// untouched, so in a pre-existing shared dir the file mode still matters —
// and the tunnel and app processes' on-disk sharing model is OS-specific.
// Tightening needs per-OS validation first.
return os.WriteFile(path, data, 0o644)
Rewrites NewFronted on top of domainfront's new config-cache support (getlantern/domainfront#16) instead of radiance fetching + persisting the config itself, which duplicated domainfront's own background config fetch. NewFronted now just seeds domainfront with the embedded config and sets WithConfigCacheFile; domainfront fetches configURL off the critical path, persists it, and bootstraps from the persisted copy on the next start in preference to the seed. Startup still does no blocking network I/O (the original fix for the 30s raw.githubusercontent.com stall on cold start in China), but the radiance-side loadBootstrapConfig/parseCachedConfig/ fetchAndCacheConfig + cache-warming goroutine are gone (fronted.go 207→80 lines). Bumps domainfront accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removes the 30s blocking fetch of fronted.yaml.gz from raw.githubusercontent.com on the radiance init critical path (blocked in China → stalled startup → the Pro-users-logged-out cluster). NewFronted now seeds domainfront with the embedded config and sets WithConfigCacheFile; domainfront owns the config lifecycle (fetch off the critical path → persist → bootstrap from the persisted copy, getlantern/domainfront#16). The radiance-side fetch/persist machinery is deleted (fronted.go 207→80 lines). Bumps domainfront.
Problem
The config updater (
WithConfigURL) fetches the fronting config on startup + every 12h and applies it to the in-memory front pool — but never persisted it. So on restart, the client reverted to the seed config passed toNew(typically the consumer's embedded copy) until the next background fetch completed. Where the config host is blocked —raw.githubusercontent.comin China — that fetch never succeeds, so the client was pinned to the embedded seed on every run.This also pushed consumers (radiance) to fetch + persist the config themselves, duplicating the updater's fetch.
Change
Make domainfront own the full config lifecycle — fetch → apply → persist → bootstrap:
WithConfigCacheFile(path)— persists each successfully fetched config and, on the next start, bootstraps from it in preference to the seed.Newprefers the persisted config (loadPersistedConfig) over the seed when present. A missing/corrupt/unreadable cache is ignored and never fails construction (falls back to the seed).fetchAndApplyConfigpersists the raw bytes after a successfulapplyConfig.Design notes
writeFileas the fronts cache. A torn write (crash mid-write) fails to parse on next boot and falls back to the seed — the intended safe fallback.loadPersistedConfigruns inNewbefore goroutines start;persistConfigruns only from the singleconfigUpdatergoroutine. No concurrent writers.Testing
go build,go vet,gofmt -l,go test -race ./...all pass. Newconfig_persist_test.gocovers: persist+load round-trip, absent/corrupt cache → nil (ignored),Newprefers persisted over seed,Newfalls back to seed when cache is absent or corrupt (never errors), and the end-to-endfetchAndApplyConfig→ cache-written path via an httptest server.Follow-up
radiance's
NewFrontedwill be simplified to seed the embedded config +WithConfigCacheFileand drop its own duplicate fetch (supersedes the interim approach in getlantern/radiance#573).Summary by CodeRabbit
New Features
Security
Bug Fixes