Uh oh!
There was an error while loading. Please reload this page.
ci(deploy): gate the deploy on the container reporting healthy - #89
Conversation
Warning Review limit reached
Next review available in:46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
The Deploy workflow ended at `docker compose up -d`, which returns once the container has started, not once it works. The last thing it observed of a deploy was `Up Less than a second (health: starting)` and it went green on that, so a container whose `storage.Open` had died reported success identically to one serving traffic. `scripts/wait-for-healthy.sh` blocks on the container's own HEALTHCHECK and fails the job on unhealthy, on an exit or restart, on a service that defines no healthcheck, or on timeout (default 120 s). Every failure dumps `docker compose ps`, the last health-probe output and the container logs, so the reason is in the run log rather than on the runner. A crash loop fails in under a second instead of waiting out the timeout, since a deploy has just recreated the container and any restart means the process died on its own. `workflow_dispatch` gains a `health_timeout` input for the deploy that legitimately needs longer: a graceful stop checkpoints the WAL and the next open is milliseconds, but a start following a hard kill replays it and can take minutes. Raising the input for that one deploy keeps the default tight for the rest. The CI smoke job runs the same script in place of its curl-until-ready loop, so a break in the gate surfaces on a PR instead of on a deploy. The image healthcheck gains `--start-interval=5s`: `--interval=30s` also paced the probes during `start-period`, so a container ready in two seconds still reported `starting` for thirty and the gate would have waited out all of it. Co-Authored-By: Wayland <wayland@agents.flopbut.local> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9d3bb0e to
c6b1f77CompareThe gate failed on any non-zero RestartCount, which is not evidence about the deploy being gated. `up -d` leaves an already-current container in place rather than recreating it, and a container that crashed once and recovered carries the count for the rest of its life — so the rule fired on a container that was running, legitimately still starting, and about to report healthy. It landed hardest on the case the `health_timeout` input exists for: a hard kill bumps the count via the restart policy and is also what leaves a WAL to replay, so the longest legitimate start was the one guaranteed to be failed at 0 s. Baseline the count when the wait begins and fail only on an increase. A container sitting in `restarting` still fails immediately — a healthy one is never in that state, so it costs no detection speed: a permanent crash loop is now caught at 0 s by state rather than by count. Co-Authored-By: Wayland <wayland@agents.flopbut.local> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `health_timeout` workflow_dispatch input was expanded straight into the run script, where it is a shell-injection sink on the runner that hosts production; route it through `env:` so it arrives as an argument instead. Once it is an argument, a non-numeric value reaches the `-ge` timeout test and errors on every iteration. `set -e` does not fire inside an `if`, so the guard silently never fired and the wait looped until the job's own limit — a bad input hung the deploy rather than failing it. Validate up front. Co-Authored-By: Daedalus <daedalus@agents.flopbut.local> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fl0p
commented
Aug 14, 2026
Squash-merged locally to `main` as 9c7c38d (per the agent-identity rule, so author+committer stay an agent), which is why GitHub does not show this as merged. Reviewed and merged with two changes pushed on top of f348264 (392f808):
On the two judgement calls: both accepted. Probing the image's own HEALTHCHECK (`/healthz`) rather than `/api/v1/health` is the better call and not just an acceptable deviation — `degraded` on a failed retention worker is a pre-existing background fault, not a bad deploy, and gating on the container's own probe keeps one definition of health that `compose ps` and the restart policy already agree with. The CI smoke change is scope-adjacent in the right direction: the gate is now exercised on every PR. On the restart-count baseline you flagged: the reasoning holds, and the layering is what makes it safe. I reproduced a crash loop where the baseline was already polluted (container carried 2 restarts before the wait began) and the `restarting` state check still failed it at 0s — so the baseline cannot blind the gate to a live crash loop, it only stops a historical count from condemning a healthy start. The two rules are complementary, not redundant. I re-verified the gate end to end on real containers rather than trusting the happy path: healthy → pass 6s; no HEALTHCHECK → fail; crash loop → fail 0s with the process output in the dump; alive but never ready (`Up (health: starting)`, the exact v10 signature) → fail at the timeout with logs. First real deploy of this gate to production went green — `wait-for-healthy: 'cotel' healthy after 7s`. |
Why
deploy.ymlended atdocker compose up -d,docker compose ps,docker image prune.-dreturns when the container is started, not when it works, so the job's last observation of the v10 deploy was:and it went green on that. A deploy whose
storage.Opendies reported success identically to one serving traffic.What changed
scripts/wait-for-healthy.shblocks on the container's own HEALTHCHECK and fails the job on:healthywithin the timeoutrestarting(crash loop)unhealthystartingat the timeoutHEALTHCHECKEvery failure dumps
docker compose ps, the last health-probe output anddocker compose logs --tail=200, so the reason lands in the run log instead of needing shell access to the runner. The dump lives in the script rather than anif: failure()step so it works identically when run locally.workflow_dispatchgains ahealth_timeoutinput (default 120). A graceful stop checkpoints the WAL so the next open is milliseconds; a start following a hard kill replays it and can take minutes. Raising the input for that one deploy keeps the default tight for every other one — the alternative, a default wide enough for the worst case, is a gate nobody trusts.restartingstate has died on its own, and any restart observed during the wait says the same. Checked before the probe status, because a restarting container also reads asunhealthyand that name hides the real fault. Only restarts seen during the wait count — see the second commit.curl-until-ready loop, so a break in the gate surfaces on a PR instead of on a deploy to production.--start-interval=5son the image HEALTHCHECK.--interval=30salso paced probes duringstart-period, so a container ready in 2 s still reportedstartingfor 30 and the gate would have waited out all of it.Scope item 3 (compose healthcheck): the healthcheck is defined in the
Dockerfileand compose inherits it from the image — verified on a live compose-created container,Test=["CMD","/usr/local/bin/cotel","-healthcheck"],StartPeriod=10m, which is well above any measured open. Left in one place rather than duplicated intodocker-compose.ymlwhere the two could drift; the "noHEALTHCHECK" row above is what keeps that honest — drop it and the deploy fails loudly instead of silently skipping the gate.Verified — both ways, against real containers
Built the image from this branch and ran the gate against isolated compose projects (own project names and volumes; nothing shared touched).
Passing:
healthy after 6s, exit 0healthy after 6s, exit 0 —db ready: schema/migrations applied in 2.831s--force-recreate) on that warm 109 MB DBhealthy after 6s, exit 0 —db ready … in 14msThe 6 s is the probe cadence, not the database. Against the 120 s default that is ~20× headroom on the worst start measured here. Confirmed the copy is real data and the WAL was replayed:
SELECT COUNT(*) FROM spans→ 33 862; graceful stop loggedcheckpoint complete in 66ms.Failing — the gate catching a deliberately broken start:
COTEL_DB_PATH=/nonexistent/dir/cotel.duckdb(crash loop)start_periodreported unhealthystart_period— the exacthealth: startingstate the v10 deploy went green onThe broken-DB run dumped the actual cause into the log:
and the unhealthy run surfaced the probe's own stderr:
Both workflow files parse (
yaml.safe_load);bash -nclean.Second commit: the restart rule had a false positive
I went back at the one rule in the first commit I had taken on faith —
RestartCount > 0 → crash loop— and it is wrong in a way that aims at the worst case.Two facts, both verified on this box rather than assumed:
up -ddoes not recreate a container whose image and config are unchanged. It reportsContainer … Runningand leaves it — so the gate does not always face a fresh container.state=running restarts=1, permanently.Together the rule fires on a container that is running, legitimately still starting, and about to report healthy. And it lands hardest on the case
health_timeoutwas added for: a hard kill bumps the count via the restart policy and is what leaves a WAL to replay, so the longest legitimate start was the one guaranteed to be failed at 0 s. A gate that fails the deploy it was built to accommodate is the flapping this ticket warns about.Now the count is baselined when the wait begins and only an increase fails; a container sitting in
restartingstill fails immediately, since a healthy one is never in that state.Verified old script vs new against the same container, in an isolated compose project:
is restarting … the process exited on its own, DuckDB error in the dumprestarted 1x during the waitDetection got faster, not slower: a permanent crash loop is now caught by container state rather than by counting restarts. README table and CHANGELOG updated to match.
Docs
README gains "The deploy waits for healthy" under Startup and deploys — the condition table, the failure dump, local usage, and the timeout guidance including when to raise
health_timeout. CHANGELOG entries under Unreleased → Changed.Not verified here
The gate has not yet run on the self-hosted runner against the live production database — that happens on the first merge to
main, which is itself the deploy. If it trips there, the run log will now say why. This box has no production container, so the production-sized evidence above is a byte-copy of production run locally.One deviation from the ticket, for your call
The ticket says to use
/api/v1/health. The gate instead uses theHEALTHCHECKalready in the image, which probes/healthzviacotel -healthcheck— I think that is the better signal here, but say the word and I will switch it:/healthzreturns 200 only once the database is open, which is exactly the readiness question a deploy gate asks./api/v1/healthalso reportsdegradedwhen the retention worker has failed. That is real information, but it is not "this deploy is broken" — it would fail a deploy over a background job that was already failing before it, and a gate that fails for reasons unrelated to the deploy is the flappy gate the ticket warns about.cotel -healthcheckis self-contained, so the runtime image still needs no curl/wget, and it is already covered byTestRunHealthcheck.Either way the gate reads the container's health status, not the endpoint directly, so swapping the probe is a one-line
Dockerfilechange with no effect on the script.Note: no automatic rollback
A failed gate leaves the broken container in place and fails the job loudly; it does not roll back. That is deliberate — a v10-style migration has no downgrade path (an older binary starts against a v10 database and then errors on every query naming the dropped column), so an automatic rollback could turn a visible failure into a subtler one. Recovery stays manual and informed. Happy to file a follow-up if you want rollback considered separately.