[viceice]

Changes

• download proxy: upstream groups with keepalive for registry.npmjs.org and rubygems.org, plus proxy_set_header Connection '', so the proxy reuses its connections to the registries
• proxy_pass https://$upstream_host/… picks the upstream group of the same name, other hosts are unchanged

After #7833 the node test job got slower (3m55s vs. 3m02s / 3m37s): 2275 npm requests through the proxy, 1739 cache misses, each with a new TLS connection to the registry.

Context

Please select one of the following:

• This closes an existing Issue, Closes: #
• This doesn't close an Issue, but I accept the risk that this PR may be closed if maintainers disagree with its opening or implementation

Part of #7827.

AI assistance disclosure

Did you use AI tools to create any part of this pull request?

Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.

• No — I did not use AI for this contribution.
• Yes — minimal assistance (e.g., IDE autocomplete, small code completions, grammar fixes).
• Yes — substantive assistance (AI-generated non‑trivial portions of code, tests, or documentation).
• Yes — other (please describe):

Proxy config change written with Claude Code (Claude Opus 5.5).

Use of AI in replying to PR comments

Who answers review comments:

• @viceice will read and reply directly. Name the account.
• An agent will draft replies and @username will read them before they are posted. Name the account.
• An agent will draft replies and reply autonomously. This is heavily discouraged, and we prefer that there are humans in the loop
• Nobody has explicitly committed to replying.

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified tests

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • Package downloads through the proxy can now reuse upstream connections, which may make repeated requests to npm and RubyGems more efficient. The proxy also manages pooled connections to these registries, helping streamline communication during downloads.

[qodo-code-review]

PR Summary by Qodo

Reuse download proxy connections to npm and RubyGems

✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

AI Description

• Add keepalive pools for npm and RubyGems to reduce repeated TLS connections during installs.
• Allow upstream connection reuse while leaving routing for other hosts unchanged.

Diagram

graph TD
  A["CDN requests"] --> B["Host mapping"] --> C{"Allowed host?"} -->|Yes| D["Proxy routing"] -->|npm or gems| E["Keepalive pools"]
  D -->|Other hosts| F["Direct upstream"]
  C -->|No| G["403 response"]

Loading

High-Level Assessment

Named upstream groups target the two high-volume registries without changing routing for other hosts. A broader proxy redesign would add complexity without a demonstrated need.

Files changed (1) +16 / -0

Enhancement (1) +16 / -0

nginx.confKeep npm and RubyGems upstream connections open +16/-0

Keep npm and RubyGems upstream connections open

• Adds named upstream groups with keepalive limits of 32 for npm and 16 for RubyGems. Clears the proxied Connection header so HTTP/1.1 connections can be reused; other hosts retain their existing routing.

.github/actions/download-proxy/nginx.conf

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: containerbase/coderabbit/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 32d66b1c-7cb6-4b55-ba64-60661c2e6ba5

📥 Commits

Reviewing files that changed from the base of the PR and between 48a80b6 and 6bc288e.

📒 Files selected for processing: 2

• .github/actions/download-proxy/action.yml
• .github/actions/download-proxy/nginx.conf

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: containerbase/coderabbit/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 44b9b85c-9ffc-4815-a0da-80247d12a107

📥 Commits

Reviewing files that changed from the base of the PR and between 0bdde0e and 48a80b6.

📒 Files selected for processing: 1

• .github/actions/download-proxy/nginx.conf

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The nginx proxy configuration adds keepalive upstream groups for registry.npmjs.org and rubygems.org. It also clears the proxied Connection header.

Changes: Proxy upstream keepalive

Layer / File(s)	Summary
Configure upstream connection reuse .github/actions/download-proxy/nginx.conf	The configuration adds keepalive pools of 32 connections for registry.npmjs.org and 16 for rubygems.org. It clears the proxied Connection header.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 48a80

The registry requests are configured to use the matching keepalive pools. No actionable merge-blocking risk was established.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: keeping download proxy connections to package registries open.
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1)

1. Registry DNS failures stop CI downloads 🐞 Bug

Description

The new upstream members resolve both registry names when nginx loads its configuration, so the
existing resolver ... valid=300s no longer refreshes their addresses. If either name is
unresolvable when nginx -t runs, the entire proxy fails to start; if an address changes later in
the job, registry requests continue using the address captured at startup.

Code

.github/actions/download-proxy/nginx.conf[R29-30]

+  upstream registry.npmjs.org {
+    server registry.npmjs.org:443;

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Static registry upstream members introduce startup-time DNS dependencies and retain stale registry addresses for the lifetime of the proxy.

## Fix Focus Areas
- .github/actions/download-proxy/nginx.conf[29-36]
- .github/actions/download-proxy/action.yml[12-19]

## Recommended Fix
Use dynamically resolved upstream members with a shared upstream zone and `resolve`, while ensuring the installed nginx version supports that directive. Keep registry DNS failures from preventing unrelated downloads through the proxy.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

---

Tip of the day

💡 Did you know, you can open the rule or relationship behind a finding in one click

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[viceice]

Qodo finding not fixed:

• "Registry DNS failures stop CI downloads": if the registries can't be resolved on the runner, the container builds fail anyway; the proxy only lives for one job, so a later address change doesn't matter either.

(Claude, for @viceice)

[qodo-code-review]

Qodo finding not fixed:

• "Registry DNS failures stop CI downloads": if the registries can't be resolved on the runner, the container builds fail anyway; the proxy only lives for one job, so a later address change doesn't matter either.

(Claude, for @viceice)

The objection is partly valid: stale DNS is less significant because the proxy only runs for one job. However, finding 1 still applies to the startup-time lookup:

• action.yml:17 runs nginx -t before the proxy starts.
• The new static upstream entries in nginx.conf:29-36 must resolve both registries during that configuration test.
• If either registry has a transient DNS failure while another permitted download host remains resolvable, nginx -t fails and the entire proxy—and therefore unrelated CI downloads—cannot start.
• The existing resolver ... valid=300s does not make these static upstream members dynamically refreshable.

So the later-address-change concern is secondary, but the startup dependency remains a real failure mode. Dynamic resolution would preserve connection reuse without making proxy startup depend on both registry lookups succeeding at that exact moment.

[github-actions]

🎉 This PR is included in version 14.44.3 🎉

The release is available on:

• GitHub release
• 14.44.3

Your semantic-release bot 📦🚀