feat(deploy): health-checked rolling restarts — container-local /readyz, aligned shutdown budget, pgbus-health probe - #388
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThe change adds container-local supervisor readiness, the ChangesHealth and shutdown lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant HealthApp
participant HealthProbe
Supervisor->>HealthApp: provide local readiness callback
HealthProbe->>HealthApp: request /readyz
HealthApp->>Supervisor: read readiness snapshot
Supervisor-->>HealthApp: return status and child counts
HealthApp-->>HealthProbe: return 200 or 503
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)spec/pgbus/configuration_spec.rbast-grep timed out on this file Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pgbus/process/supervisor.rb (1)
919-929: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
shutdown_timeoutbeforeSupervisor#runenters itsensurepath.eager_validation = falseallows invalid values to reachshutdown;Time.now + config.shutdown_timeoutcan then raiseTypeErrorand mask the boot error.🤖 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 `@lib/pgbus/process/supervisor.rb` around lines 919 - 929, Validate config.shutdown_timeout before Supervisor#run can enter its ensure-driven shutdown path, rejecting invalid values during startup rather than allowing shutdown to perform Time.now arithmetic on them. Update the existing Supervisor startup/config validation flow and preserve the shutdown deadline behavior for valid timeout values.
🤖 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 `@docs/app/models/doc.rb`:
- Line 24: In docs/app/models/doc.rb, keep only the docs declaration for
“Rolling restarts” at line 45 under the Operations group and remove the
duplicate declarations at lines 24, 37, 49, 56, and 61, so the navigation
registers that page once.
In `@lib/pgbus/configuration.rb`:
- Around line 785-790: Update validate_shutdown_timeout! to accept nil only or a
finite, positive real numeric value, rejecting Float::INFINITY and non-real
Numeric implementations before shutdown calculations; add coverage for infinite
and non-real Numeric inputs while preserving the existing ConfigurationError and
message behavior.
In `@lib/pgbus/health_probe.rb`:
- Around line 45-52: Update the validation flow in HealthProbe#run to reject
ports outside 1–65535 and return usage_failure before calling probe. Also
validate the raw timeout input before conversion so non-numeric --timeout values
such as "abc" return the usage exit code 2 instead of becoming 0.0; preserve
valid timeout handling.
In `@lib/pgbus/process/supervisor.rb`:
- Around line 131-139: Update the boot lifecycle around mark_booted,
boot_processes, and fork_worker/fork_consumer to track the configured child-fork
intent separately from `@forks.size`, incrementing only for roles that should
actually boot and excluding legitimately disabled or empty roles. Set
`@expected_children` from this intent count at boot completion, then compare it
with the fork table so failed or rescued forks keep readiness unhealthy rather
than lowering the baseline.
In `@README.md`:
- Line 1202: Update the fenced code block in README.md to specify the text
language identifier, using ```text while preserving the existing timeline
content and formatting.
---
Outside diff comments:
In `@lib/pgbus/process/supervisor.rb`:
- Around line 919-929: Validate config.shutdown_timeout before Supervisor#run
can enter its ensure-driven shutdown path, rejecting invalid values during
startup rather than allowing shutdown to perform Time.now arithmetic on them.
Update the existing Supervisor startup/config validation flow and preserve the
shutdown deadline behavior for valid timeout values.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1269691f-8c85-4b10-be8c-56d10b9736d4
📒 Files selected for processing (21)
CHANGELOG.mdREADME.mddocs/app/models/config_reference.rbdocs/app/models/doc.rbdocs/app/views/docs/pages/observability.rbdocs/app/views/docs/pages/rolling_restarts.rbexe/pgbus-healthlib/pgbus/configuration.rblib/pgbus/health_probe.rblib/pgbus/process/consumer.rblib/pgbus/process/readiness_snapshot.rblib/pgbus/process/supervisor.rblib/pgbus/process/worker.rblib/pgbus/web/health_app.rbspec/pgbus/configuration_spec.rbspec/pgbus/health_probe_spec.rbspec/pgbus/process/consumer_spec.rbspec/pgbus/process/readiness_snapshot_spec.rbspec/pgbus/process/supervisor_spec.rbspec/pgbus/process/worker_spec.rbspec/pgbus/web/health_app_spec.rb
…yz, aligned shutdown budget, pgbus-health probe
The supervisor's standalone /readyz previously answered with the
cluster-wide HealthAnalyzer verdict, so during a rolling deploy a
freshly-booted container passed the orchestrator's health gate on the
strength of the OLD container's still-heartbeating workers. It now
answers container-local: 200 only once THIS supervisor verified its
connection, bootstrapped queues, and every child it forked is alive —
503 BOOTING / DEGRADED (child in crash backoff) / DRAINING otherwise.
The supervisor publishes an immutable ReadinessSnapshot per monitor
pass (refreshed after reap-and-restart, so clean recycles never flap);
the Rails-mounted HealthApp keeps the cluster verdict.
Shutdown budgets become alignable: new config.shutdown_timeout (default
drain_timeout + 5) replaces the supervisor's hardcoded 30s SIGKILL
deadline, Consumer's drain wait follows drain_timeout instead of a
hardcoded 30s, and Worker's post-drain residual wait drops to 5s.
New pgbus-health executable: stdlib-only probe (never loads Bundler or
the gem) for docker HEALTHCHECK blocks — exit 0/1/2.
## Test Coverage
- configuration_spec: shutdown_timeout derivation, validation, warning
- readiness_snapshot_spec: ready?/status truth table
- supervisor_spec: boot/degrade/drain transitions, /readyz wiring,
configurable SIGKILL deadline
- worker_spec / consumer_spec: pool-wait bounds
- health_app_spec: local-readiness verdicts + error path
- health_probe_spec: real-socket probe against HealthServer, exit
codes, no-gem-load guard
## Verification
- [x] bundle exec rake rubocop — 546 files, no offenses
- [x] touched specs green (747 examples)
- [x] full suite vs main, same seed: identical 2 pre-existing i18n
failures, zero regressions
Refs #386
New Operations page covering the health-gated rolling restart flow: the container-local readiness gate, pgbus-health HEALTHCHECK wiring, the stop_timeout > shutdown_timeout > drain_timeout alignment rule, overlap-window duplicate-supervisor safety, and the read_ct-vs-deploy- kill DLQ caveat. Observability page's probe table now distinguishes mounted (cluster verdict) from standalone (container-local) /readyz, and the config reference documents shutdown_timeout (drift spec green). Refs #386
1e2041d to
c0de308
Compare
…shutdown_timeout, probe input validation, docs registry dedupe CodeRabbit review on #388: - Readiness baseline now counts INTENDED boot-time forks (max of attempt count and fork table), so a failed fork at boot leaves the gate DEGRADED instead of silently lowering expected — a rolling deploy can no longer promote a container missing workers. Restart re-forks after boot never inflate the baseline. - validate! rejects non-finite / non-real shutdown_timeout (INFINITY previously blew up Supervisor#shutdown before child cleanup). - pgbus-health exits 2 on out-of-range ports (was a SocketError backtrace) and on non-numeric/non-positive --timeout (was a silent 0.0 deadline that failed every probe); SocketError added to the probe rescue. - docs page generator had injected the Rolling restarts registry line after every group — deduped to one entry under Operations. - README: language identifier on the budget-alignment fence (MD040). Refs #386
Summary
Implements #386 — makes the job container a correct participant in health-gated rolling restarts (Kamal
dash-style: start new → poll healthcheck → stop old)./readyzon the standalone health server (lib/pgbus/web/health_app.rb,lib/pgbus/process/supervisor.rb, newlib/pgbus/process/readiness_snapshot.rb).health_portserver no longer serves the cluster-wide verdict — a new container can't pass the deploy gate on the old container's workers. States:BOOTING/OK/DEGRADED/DRAINING; the supervisor swaps an immutable snapshot per monitor pass, so the accept thread never touches@forks. The Rails-mountedHealthAppkeeps the cluster verdict.lib/pgbus/configuration.rb,supervisor.rb,worker.rb,consumer.rb). Newconfig.shutdown_timeout, defaultdrain_timeout + 5, replaces the supervisor's hardcoded 30s SIGKILL deadline; Consumer's drain wait followsdrain_timeout; Worker's post-drain residual wait drops 30s → 5s. Documented rule: orchestratorstop_timeout>shutdown_timeout>drain_timeout.pgbus-healthprobe (exe/pgbus-health,lib/pgbus/health_probe.rb): stdlib-only, loads neither Bundler nor the gem (spec-enforced), exit 0/1/2 — cheap enough for 1–5s dockerHEALTHCHECKintervals, no curl needed.Added+Changed, behavior change flagged).Closes #386
Test plan
bundle exec rake rubocop— 546 files, no offenses (root task; docs apprake lintalso clean)main, same seed (--seed 20260803, excluding the locally-unloadablespec/rubocopcop spec): identical 2 pre-existing i18n failures on both, zero regressions; branch adds 26 examplesshutdown_timeout)exe/pgbus-healthprocess against a realHealthServer— exit 0 on a ready snapshot, exit 1 on a draining oneDeviations & judgment calls
expectedchildren = fork-table size afterboot_processes, stamped once. A child whoseforkitself failed at boot (EAGAIN/ENOMEM) is not counted, so readiness could reportOKwithout it — accepted because deriving the expectation from config is wrong in legitimate cases (scheduler role declines to boot when no recurring tasks exist). Rare failure, conservative direction (over-ready only for a child that also crash-looped at fork time).graceful_shutdown/immediate_shutdown, not only from the monitor loop, soDRAININGis visible to the very next probe instead of up to 1s later.--timeoutand--pathflags beyond the issue's minimum (--port/env); parsing is hand-rolled (~15 lines) rather than optparse to keep the load profile at exactlysocket.{"status":"ERROR"}+ logged) — the issue specified only the happy-path body shape.mainis 2 i18n failures plus one load error (spec/rubocop/cop/pgbus/no_ruby_timeout_spec.rbcan'trequire "rubocop/pgbus"locally; pre-existing, excluded from the comparison run).pgbus start) replaced with an equivalent that needs no DB: the probe exe against a realHealthServer+ local snapshot, both readiness polarities. The DB-dependent boot path is covered by the existing supervisor#runspecs.Summary by CodeRabbit
New Features
pgbus-healthprobe for deployment health checks.Documentation
Bug Fixes