chore(deploy): version nginx http tuning#451
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces ChangesNginx http-context tuning rollout
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CodeRabbit body-note assessment:
|
The new conf.d/wrzdj-tuning.conf is included into the http{} block, but the
base /etc/nginx/nginx.conf (stock Ubuntu, and the live VPS) already declares
gzip, server_tokens, and ssl_protocols there. Repeating them in the include is
a fatal "directive is duplicate" error, so `nginx -t` fails and setup-nginx.sh
skips its reload -- re-running it on a running deployment (or a fresh Ubuntu
install) silently never applies the update.
Drop the three redundant directives from tuning.conf; they are already covered
by lower layers:
- gzip on: kept enabled by the base http block; the include only adds the
proxied-compression settings (gzip_proxied/gzip_types/...) it leaves
commented out.
- server_tokens off: already set by the base nginx.conf and the per-vhost
templates.
- ssl_protocols: pinned to TLS 1.2/1.3 per-vhost by the certbot
options-ssl-nginx.conf include (server level overrides http level).
Document the live-update procedure (re-run setup-nginx.sh; idempotent,
fail-closed via nginx -t) in DEPLOYMENT.md.
Verified with `nginx -t` against a replica of the live nginx.conf http block
plus a vhost exercising the wrzdj_edge rate-limit zone: before, EXIT 1
("gzip directive is duplicate"); after, test successful.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up: made the http-tuning include installable on an already-live boxWhile validating the rollout path against the live host (read-only), I hit a blocker: Fix: drop the three redundant directives from
All the actual value of the PR (API/JSON gzip, edge rate-limiting, connection timeouts) is preserved, and the deploy script never has to touch the co-tenant–shared distro Verified with Also added an "Update nginx config on a running deployment" section to Two issue items intentionally left as follow-ups: global TLS pin (item #3 — already 1.2/1.3 per-vhost) and |
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 `@deploy/DEPLOYMENT.md`:
- Around line 332-335: The `/health` endpoint in the gzip verification curl
command likely returns a response too small to exceed the gzip_min_length
threshold of 1024 bytes set in deploy/nginx/tuning.conf, causing the check to
fail even when gzip is properly configured. Replace the `/health` endpoint with
a larger JSON endpoint that will reliably exceed 1024 bytes (such as a list or
details endpoint), or temporarily reduce the gzip_min_length value in
deploy/nginx/tuning.conf during validation to properly verify that gzip
compression is active.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a63b36b-e243-4cd7-b5dc-884c42eff646
📒 Files selected for processing (2)
deploy/DEPLOYMENT.mddeploy/nginx/tuning.conf
The post-reload gzip check curled /health with -I (HEAD). /health returns
{"status":"ok"} (16 bytes), far below gzip_min_length (1024), and a HEAD
response has no body for nginx to compress — so the check would never show
Content-Encoding: gzip even when gzip is correctly configured. Use a GET
against /openapi.json (application/json, well over 1024 bytes) and dump
response headers so the verification actually demonstrates compression.
Addresses CodeRabbit review on PR #451.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
deploy/nginx/tuning.confas a versioned http-context include installed to/etc/nginx/conf.d/wrzdj-tuning.confbysetup-nginx.sh.limit_req_zonedefaults.limit_reqto non-SSE API/app proxy locations while keeping the public event stream location explicitly exempt.ssl http2listen options to avoidprotocol options redefinedreload warnings.Closes #447
Design decisions
worker_connectionsandworker_rlimit_nofileare not intuning.confbecause they areevents/main-context directives and nginx rejects them inside/etc/nginx/conf.dhttp-context includes. The versioned include keeps only directives valid inhttp{}; full main/events ownership should be a separate deployment change if needed.$binary_remote_addrat60r/swithburst=180 nodelayin vhosts. This is intentionally generous for shared venue NATs while still shedding clear floods before they reach FastAPI/slowapi.limit_req; long-lived EventSource connections stay exempt from edge throttling and keepproxy_buffering offplus the 24h read timeout.listen ... http2syntax for Ubuntu nginx compatibility. Docker nginx 1.27 reports that syntax as deprecated, but the config test no longer reports theprotocol options redefinedwarning.Verification
bash -n deploy/setup-nginx.sh deploy/setup-user.shshellcheck deploy/setup-nginx.sh deploy/setup-user.shtuning.confis tracked/not ignored, has no template variables, has four active non-SSElimit_reqdirectives, and the SSE location has no activelimit_req.APP_DOMAIN=app.example.test API_DOMAIN=api.example.test PORT_API=8000 PORT_FRONTEND=3000 ./deploy/setup-nginx.shin an environment without/etc/nginx/sites-available; confirmed the manual-copy output includeswrzdj-tuning.conf.logging.confandtuning.conf, then rannginx:1.27-alpine nginx -t; syntax passed and noprotocol options redefinedwarning appeared.Not run live against production: gzip header check, real SSE stream, TLS scan, edge 429 behavior, and full app/API/overlay/upload smoke tests require an installed nginx deployment with real certs and running upstream services.
Summary by CodeRabbit
New Features
Improvements
Documentation