Expose log fields, upstream timeouts, cache inactive, and cache lock as options - #3
Merged
colemickens merged 3 commits intoJul 29, 2026
Conversation
…d cache lock as options Surfaces nginx behaviors that previously required forking the hardcoded config, all behind options that default to a no-op so existing consumers see no change on upgrade: - extraLogFields: append fields (e.g. $upstream_cache_status) to log_format - upstreamConnectTimeout/upstreamReadTimeout + narinfoMissOnError: bound the narinfo passthrough and remap upstream failures to 404 so Nix sees a clean miss and falls through instead of failing the build on a 5xx/timeout - cacheInactive: set proxy_cache_path inactive= (defaults to null, i.e. nginx's 10m default; set to cacheLifetime for a bursty pull-through cache) - cacheLock + cacheLockTimeout/cacheLockAge: enable proxy_cache_lock to collapse concurrent fetches of the same uncached NAR Documents each option in the README and exercises them in the test VM.
📝 WalkthroughWalkthroughThe PR adds FlakeHub Edge Cache options for DNS resolution, access-log fields, cache inactivity, upstream timeouts, and cache locking. It wires these options into generated nginx configuration, updates VM settings, and documents them in the README. ChangesFlakeHub Edge Cache controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NginxResolver
participant NginxCache
participant Upstream
NginxResolver->>Upstream: Resolve the cache address with IPv4/IPv6 and timeout settings
NginxCache->>Upstream: Fetch nar or narinfo with configured TLS and timeouts
NginxCache->>NginxCache: Coalesce cache misses using cache-lock settings
NginxCache->>NginxCache: Record cache and upstream fields in access logs
🚥 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 |
Resolve FlakeHub Cache per request while preserving the expected TLS name, verifying its certificate, and bounding DNS lookup time. Add configurable resolver, TLS, and error-log settings together with upstream diagnostics for failed and retried requests.
colemickens
previously approved these changes
Jul 28, 2026
colemickens
reviewed
Jul 28, 2026
colemickens
reviewed
Jul 28, 2026
colemickens
reviewed
Jul 28, 2026
colemickens
reviewed
Jul 28, 2026
Member
|
@achen318 thanks for this! There's some great stuff in here, I'm sorry we didn't look at this sooner. I left a few comments. A couple of which I could do in a follow-up commit, if you'd prefer. I am curious for your thoughts on the code->404 conversion that I commented on though. |
colemickens
reviewed
Jul 28, 2026
Contributor
Author
|
No worries, thanks for reviewing! I can add some improvements to this PR for you to take a look. |
…rop 404 remap - Rename upstreamConnectTimeout to upstreamResolveTimeout; it drives both resolver_timeout and proxy_connect_timeout. - Default extraLogFields to the cache observability fields ($upstream_cache_status, $upstream_bytes_received, $request_time) instead of requiring consumers to opt in; the option remains for customization. - Drop the errorLog option and log errors to stderr so they land in the service manager's journal. - Apply the connect timeout to the /nar passthrough as well; read/send timeouts stay narinfo-only since they bound the gap between reads and could abort slow but progressing NAR transfers. - Remove narinfoMissOnError: on Nix >= 2.32 (NixOS/nix#13301) graceful substituter fallback makes the 5xx-to-404 remap unnecessary for correctness, and truthful errors avoid both negative narinfo cache poisoning on clients and the observability gap of synthetic misses.
achen318
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/default.nix (1)
219-221: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument cache-lock bypass behavior consistently.
Nginx can release waiters after
cacheLockTimeout, andcacheLockAgecan permit another origin request. Both descriptions should state that requests normally wait, subject to these limits. (nginx.org)
modules/default.nix#L219-L221: qualify “only the first populates the cache and the rest wait.”README.md#L167-L173: mirror the same qualification in the public documentation.🤖 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 `@modules/default.nix` around lines 219 - 221, Qualify the proxy_cache_lock descriptions in modules/default.nix lines 219-221 and README.md lines 167-173 so they state that concurrent requests normally wait for the first cache fill, but may bypass waiting when cacheLockTimeout expires or cacheLockAge permits another origin request; keep the behavior description consistent in both locations.Source: MCP tools
🤖 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.
Outside diff comments:
In `@modules/default.nix`:
- Around line 219-221: Qualify the proxy_cache_lock descriptions in
modules/default.nix lines 219-221 and README.md lines 167-173 so they state that
concurrent requests normally wait for the first cache fill, but may bypass
waiting when cacheLockTimeout expires or cacheLockAge permits another origin
request; keep the behavior description consistent in both locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d248e707-3bc5-47f4-8ca2-771847137a79
📒 Files selected for processing (3)
README.mdmodules/default.nixtest/vm.nix
Member
|
Thanks @achen318 ! |
colemickens
enabled auto-merge
July 29, 2026 22:26
colemickens
approved these changes
Jul 29, 2026
Merged
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
2 participants
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.
This updates the edge cache to resolve
cache.flakehub.comfor each request instead of retaining the address chosen when nginx starts. Both proxy paths preserve the expected Host and SNI name, verify TLS, and expose diagnostics for failed DNS lookups and upstream connection attempts.It also exposes nginx behaviors that previously required replacing the module's generated configuration:
dnsResolverIPv6: enable or disable IPv6 DNS answers.sslVerify,sslTrustedCertificate, andsslVerifyDepth: configure upstream certificate verification.extraLogFields: append custom access-log fields. The default format now includes cache status, upstream bytes and request time, plus upstream status, address, and connect time.upstreamResolveTimeout: bound DNS resolution and upstream connection attempts for both the narinfo and NAR passthroughs.upstreamReadTimeout: bound the narinfo passthrough read/send time (narinfo-only, so slow but progressing NAR transfers are never aborted).cacheInactive: configure inactive cache-entry eviction independently from freshness.cacheLock,cacheLockTimeout, andcacheLockAge: collapse concurrent fetches for the same uncached NAR.The cache, timeout, and lock options remain opt-in. Request-time DNS resolution, upstream TLS verification, the observability log fields, and journal-based error logging are enabled by default.
Validation
nixfmt --checkpassed for all Nix files.nix flake check --no-buildpassed on x86_64 Linux.nginx -t.