Adopt Thruster in front of Puma#2018
Conversation
- Gemfile: activate the thruster placeholder (require: false); lockfile churn is thruster-only (0.1.23, statically linked Go binary — runs on Alpine/musl unchanged) - bin/thrust: bundler binstub - Dockerfile: EXPOSE 80, wrap the existing puma -C CMD with bin/thrust, and set LOG_REQUESTS="false" (thruster's per-request Go slog output would double container stdout into the Ougai-keyed Datadog pipeline and re-log silenced health checks) - production.rb: comment out public_file_server.enabled = false so the 8.0 default (true) lets Rails serve public/ behind Thruster; ALSO comment out the far-future public_file_server.headers line — this app has no digest-stamped assets and public/xmlns/*.xsd are canonical but MUTABLE schema files, so a 1-year shared-cache header would be harmful (fleet mutable-public-files exception, 2026-07-20) nginx VOLUMEs/conf stay for now; the sidecar is removed in cru-terraform only after this image deploys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C9vz7gguLNupgNEFGtjLfX
|
I see you added the "On Staging" label, I'll get this merged to the staging branch! |
|
Merge conflict attempting to merge this into staging. Please fix manually. |
Manual merge per stage-branch-merger's conflict comment. Three conflicts: - Gemfile: kept staging's GA4 migration (google-analytics-data replacing the sunset google-apis-analyticsreporting_v4) alongside the stack's changes; staging's crowdin-api ~> 1.13.0 pin also honored (its deliberate change vs the merge base). - Gemfile.lock: regenerated via bundle lock from the branch's CI-verified Rails 8 lock as base — resulting diff is exactly the google-family swap (+ faraday-retry/gapic/grpc transitives) and the crowdin downgrade. - db/schema.rb: [8.0] dumper header (the stack's mechanical re-dump) with staging's newer migration version 2025_11_24_224217 (GT-1290). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C9vz7gguLNupgNEFGtjLfX
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
knutsenm
left a comment
There was a problem hiding this comment.
Verified as part of the fleet stack audit:
- Autonag CMD variant — correct, master's CMD already ran puma via the load-bearing
config/puma.rb.EXPOSE 80,ENV LOG_REQUESTS="false", executablebin/thrust, thruster-only lockfile churn (0.1.23). - Commenting out the far-future
public_file_server.headers(rather than keeping it) is right for this app: no digested assets exist, andpublic/xmlns/*.xsdare live, mutable schema-namespace URLs — exactly the mutable-public-files exception.public_file_server.enabled = falsecommented out per the pattern. - No
CACHE_SIZE=0— acceptable: the app serves authorized JSON:API but emits no publicCache-Controlfrom controllers (grep-verified), so thruster's shared cache can't store user-scoped responses.
Approving with the standard caveats: zero CI has run on this stacked leg, and this repo's branch protection is strict: true — after #2017 merges, this needs the rebase+push the body describes before it can merge. Post-sidecar-removal, smoke-test /cable WebSockets through thruster in staging (the proxy chain changes), which also exercises the x86_64-linux thrust binary on the Alpine/musl image in a real environment.
🤖 Review drafted with Claude Code
Stacked on #2017 (
rails-8.0-upgrade) — depends on #2017; when it merges and its branch is deleted, GitHub auto-retargets this PR tomaster.Adopts Thruster in front of Puma per the standing fleet decision (HTTP caching/compression + static serving; the nginx sidecar is subsequently removed in cru-terraform).
Changes
thrusterplaceholder (require: false). Lockfile churn is thruster-only (0.1.23). The gem ships a statically linked Go binary — runs on this app's Alpine/musl image unchanged, no base-image or PLATFORMS changes needed.EXPOSE 80— Thruster's listen port.["./bin/thrust", "bundle", "exec", "puma", "-C", "config/puma.rb"].ENV LOG_REQUESTS="false"— Thruster's per-request Go slog JSON logging defaults ON and shares container stdout with the app's Ougai stream; leaving it on would double log volume into the Ougai-keyed Datadog pipeline and re-log the health checks Rails silences (silence_healthcheck_path). Fleet decision applied to all thruster PRs.VOLUMEs and conf are intentionally untouched — sidecar removal happens later in cru-terraform, only AFTER this image deploys with Thruster.docs:generateraddocs step is unaffected.public_file_server.enabled = falsecommented out (restored in the upgrade PR for the nginx interim, with a note pointing here) — the 8.0 default (true) now lets Rails servepublic/behind Thruster. Verified by production boot:enabled = true,ActionDispatch::Staticin the middleware stack.public_file_server.headers(cache-control: public, max-age=1yr) is commented out with a why-note. This app has no digest-stamped assets (no asset pipeline), andpublic/serves the canonical-but-mutable XML schema files (public/xmlns/*.xsd— the live schema-namespace URLs, last changed 2026-05-08, CI-validated). A 1-year shared-cache header on those would be harmful, and without digested assets the far-future default has no upside here. This is exactly the "URL-serves genuinely mutable public/ files" case the fleet decision (2026-07-20, which KEEPS the default for asset-pipeline apps) flags for per-app exception.Verification (local — CI does not run on stacked PRs)
ruby.ymlfilterspull_requestto the default branch, so no checks run against arails-8.0-upgradebase. Verified locally on this branch:CI=true RAILS_ENV=test bundle exec rspec: 670 examples, 0 failures, 3 pending (matches Upgrade Rails 7.2.3.1 → 8.0.5 #2017 / CI baseline)standardrb: 0 offensesbundle-audit check --update: 0 vulnerabilities, no ignoresbrakeman -A -q --ensure-latest --no-pager: 0 warnings, exit 0HTTP_PORT=8099/TARGET_PORT=3099, curled through the Thruster port):/monitors/lb→ 200 (X-Cache: miss— through Thruster's cache layer)/systems→ 200 JSON:API payload/xmlns/manifest.xsd→ 200 static frompublic/, no far-future headerLOG_REQUESTS=false: zero request-log lines from Thruster across all requests (without it, one Go slog JSON line per request — confirming the rationale)public_file_server.enabled = true,public_file_server.headers = nil,ActionDispatch::Staticpresentmaster+ push (or close/reopen) to trigger required checks before merging (retarget alone fires no CI).Deploy sequencing
master, rebase+push for green checks → merge.container_port = "80",nginxdisabled) — only after step 2 is live.🤖 Generated with Claude Code
https://claude.ai/code/session_01C9vz7gguLNupgNEFGtjLfX