fix: Wait longer for a container to report healthy #24

ahmad · 2026-09-02T22:05:35Z

ahmad commented

2026-09-02 22:05:35 +00:00

Issue

Closes #22

Problem

deploy-vps waited thirty attempts two seconds apart — sixty seconds — then failed the deployment. By the time that wait runs the rollout has already happened, so a budget that runs out undoes nothing; it only decides whether the pipeline tells the truth about the outcome. On a loaded host a container takes minutes, so the action reported failures for deployments that had succeeded: ahmad/portfolio v0.15.1 went red twice on 2 September while serving the very version it had just deployed.

Solution

The wait is now a budget in seconds, exposed as health-timeout and defaulting to ten minutes — roughly six times the observed worst case, and still far short of a hung run. unhealthy is treated as a verdict rather than a reason to keep waiting: Docker sets it only once the image's own healthcheck retries are exhausted, so a crash-looping image fails in seconds instead of costing the whole budget.

A container that never becomes healthy still fails the deployment and still prints its logs.

Review notes

Verified by execution against a stubbed docker, driving the loop through a scripted sequence of statuses:

case	statuses	exit	elapsed
healthy immediately	`healthy`	0	1s
healthy on the fourth poll	`starting` ×3 then `healthy`	0	6s
unhealthy early	`starting`, `unhealthy`	1	2s
never healthy, 6s budget	`starting`	1	6s

The third row is the one that pays for the longer default: a genuinely broken image still reports in seconds.

Not verified: this has not run against a real host. The deploy key lives in OpenBao and I have no business holding it, so the first real deployment is the live proof. That is also when the default gets its first test — if ten minutes turns out to be wrong for these hosts, it is now one input rather than a code change.

Blast radius, stated plainly. Merging moves the v3 tag, so every repository consuming deploy-vps@v3 picks this up on its next deployment. Callers that set nothing keep working unchanged; the only difference they see is that a slow-starting container is no longer called a failure.

Risks and trade-offs

A deployment that genuinely hangs now occupies a runner for up to ten minutes per container instead of one. On a fleet where a red deploy currently means very little, I would rather pay that than keep reporting successes as failures.
The unhealthy fast path is a behaviour change: previously such a container was retried until the attempts ran out. It fails sooner and says why, which is strictly more useful, but it is a change.

### Issue Closes #22 ### Problem `deploy-vps` waited thirty attempts two seconds apart — sixty seconds — then failed the deployment. By the time that wait runs the rollout has already happened, so a budget that runs out undoes nothing; it only decides whether the pipeline tells the truth about the outcome. On a loaded host a container takes minutes, so the action reported failures for deployments that had succeeded: `ahmad/portfolio` v0.15.1 went red twice on 2 September while serving the very version it had just deployed. ### Solution The wait is now a budget in seconds, exposed as `health-timeout` and defaulting to ten minutes — roughly six times the observed worst case, and still far short of a hung run. `unhealthy` is treated as a verdict rather than a reason to keep waiting: Docker sets it only once the image's own healthcheck retries are exhausted, so a crash-looping image fails in seconds instead of costing the whole budget. A container that never becomes healthy still fails the deployment and still prints its logs. ### Review notes **Verified by execution against a stubbed `docker`**, driving the loop through a scripted sequence of statuses: | case | statuses | exit | elapsed | | --- | --- | --- | --- | | healthy immediately | `healthy` | 0 | 1s | | healthy on the fourth poll | `starting` ×3 then `healthy` | 0 | 6s | | unhealthy early | `starting`, `unhealthy` | 1 | 2s | | never healthy, 6s budget | `starting` | 1 | 6s | The third row is the one that pays for the longer default: a genuinely broken image still reports in seconds. **Not verified:** this has not run against a real host. The deploy key lives in OpenBao and I have no business holding it, so the first real deployment is the live proof. That is also when the default gets its first test — if ten minutes turns out to be wrong for these hosts, it is now one input rather than a code change. **Blast radius, stated plainly.** Merging moves the `v3` tag, so every repository consuming `deploy-vps@v3` picks this up on its next deployment. Callers that set nothing keep working unchanged; the only difference they see is that a slow-starting container is no longer called a failure. ### Risks and trade-offs - A deployment that genuinely hangs now occupies a runner for up to ten minutes per container instead of one. On a fleet where a red deploy currently means very little, I would rather pay that than keep reporting successes as failures. - The `unhealthy` fast path is a behaviour change: previously such a container was retried until the attempts ran out. It fails sooner and says why, which is strictly more useful, but it is a change.

ahmad added 1 commit

2026-09-02 22:05:35 +00:00

fix: Wait longer for a container to report healthy af008f77a9

The wait was thirty attempts two seconds apart, so a container that took
more than a minute to start was reported as a failed deployment. By then the
rollout has already happened, so the short budget undid nothing — it only
made the pipeline lie about the outcome. ahmad/portfolio v0.15.1 went red
twice while serving the version it had just deployed.

The budget is now seconds, configurable, and ten minutes by default. A
container reporting unhealthy still fails immediately: Docker sets that only
after the image's own retries are exhausted, so a broken image does not cost
the whole budget.

ahmad commented

2026-09-02 22:07:37 +00:00

Ready. There is nothing to wait for on CI: this repository has no pull-request pipeline — .forgejo/workflows/ holds only release.yml, and main's protection requires an approval and no status checks. I have raised that as its own issue rather than fixing it here.

So the verification is what I ran locally, and it is worth being precise about what it covers. I drove the new loop against a stubbed docker inspect returning a scripted sequence of statuses:

case	statuses returned	exit	elapsed
healthy immediately	`healthy`	0	1s
healthy on the fourth poll	`starting` ×3, then `healthy`	0	6s
unhealthy early	`starting`, `unhealthy`	1	2s
never healthy, budget 6s	`starting` throughout	1	6s

Rows three and four are the ones that matter: a broken image still reports in seconds rather than costing the whole budget, and a container that never comes healthy still fails with its logs. Rows one and two are the regression check — the common paths behave as before, just without the sixty-second ceiling.

action.yml parses and the new input appears with its default; the README documents it alongside the others.

What this does not cover: a real host. The deploy key is in OpenBao and I have no business holding it, so the first real deployment is the live proof — and it is also the first test of whether ten minutes is the right default. That is now one input rather than a code change, which is the point.

Merging moves the v3 tag, so every consumer picks this up on its next deployment. Callers that set nothing are unaffected except in the case this exists to fix.

Ready. There is nothing to wait for on CI: this repository has no pull-request pipeline — `.forgejo/workflows/` holds only `release.yml`, and `main`'s protection requires an approval and no status checks. I have raised that as its own issue rather than fixing it here. So the verification is what I ran locally, and it is worth being precise about what it covers. I drove the new loop against a stubbed `docker inspect` returning a scripted sequence of statuses: | case | statuses returned | exit | elapsed | | --- | --- | --- | --- | | healthy immediately | `healthy` | 0 | 1s | | healthy on the fourth poll | `starting` ×3, then `healthy` | 0 | 6s | | unhealthy early | `starting`, `unhealthy` | 1 | 2s | | never healthy, budget 6s | `starting` throughout | 1 | 6s | Rows three and four are the ones that matter: a broken image still reports in seconds rather than costing the whole budget, and a container that never comes healthy still fails with its logs. Rows one and two are the regression check — the common paths behave as before, just without the sixty-second ceiling. `action.yml` parses and the new input appears with its default; the README documents it alongside the others. What this does not cover: a real host. The deploy key is in OpenBao and I have no business holding it, so the first real deployment is the live proof — and it is also the first test of whether ten minutes is the right default. That is now one input rather than a code change, which is the point. Merging moves the `v3` tag, so every consumer picks this up on its next deployment. Callers that set nothing are unaffected except in the case this exists to fix.

ahmad changed title from ~~WIP: fix: Wait longer for a container to report healthy~~ to fix: Wait longer for a container to report healthy

2026-09-02 22:07:38 +00:00

ahmad_bot reviewed

2026-09-02 22:32:28 +00:00

ahmad_bot left a comment

Verified against issue #22. One non-blocking finding, in the inline comment; the change does what the issue asks and the reasoning behind the default is sound.

All five criteria hold. The budget is now seconds with a deadline rather than a fixed attempt count, so a container that goes healthy after sixty seconds is reported as the success it is (1). A container that never becomes healthy still fails and still prints docker logs --tail 50 (2). unhealthy short-circuits (3) — and the reasoning for treating it as a verdict rather than a reason to keep waiting is exactly right: Docker only sets it once the image's own retries are exhausted, so polling longer cannot change it. health-timeout is a real input with a documented default of 600 (4), and the README carries the input, the default, the unhealthy behaviour and why the budget is generous (5).

The framing in the description is the part I would keep. "The rollout has already happened by the time this waits, so a budget that runs out undoes nothing — it only decides whether the pipeline tells the truth about it" is the observation that makes a ten-minute default obviously right rather than obviously reckless. That belongs in the README, and it is there.

On the stub testing: driving the loop through a scripted status sequence is the right way to get real confidence without a host, and the four rows cover the branches that matter. I used the same approach for the finding above.

Blast radius. You are right that merging moves v3 and every consumer picks this up on its next deployment. Worth saying that this is the good direction for the fleet: ahmad/expiro, ahmad/portfolio and ahmad/trip all wait on containers on the same I/O-saturated host, and all three currently inherit the sixty-second budget that produced the false failures in #22.

The open item is the one you named — none of this has run against a real host, so the first live deployment is the proof, and it is also the first test of whether ten minutes is the right number. That it is now an input rather than a constant is what makes being wrong about it cheap.

Verified against issue #22. One non-blocking finding, in the inline comment; the change does what the issue asks and the reasoning behind the default is sound. **All five criteria hold.** The budget is now seconds with a deadline rather than a fixed attempt count, so a container that goes healthy after sixty seconds is reported as the success it is (1). A container that never becomes healthy still fails and still prints `docker logs --tail 50` (2). `unhealthy` short-circuits (3) — and the reasoning for treating it as a verdict rather than a reason to keep waiting is exactly right: Docker only sets it once the image's own retries are exhausted, so polling longer cannot change it. `health-timeout` is a real input with a documented default of 600 (4), and the README carries the input, the default, the `unhealthy` behaviour and *why* the budget is generous (5). **The framing in the description is the part I would keep.** "The rollout has already happened by the time this waits, so a budget that runs out undoes nothing — it only decides whether the pipeline tells the truth about it" is the observation that makes a ten-minute default obviously right rather than obviously reckless. That belongs in the README, and it is there. **On the stub testing:** driving the loop through a scripted status sequence is the right way to get real confidence without a host, and the four rows cover the branches that matter. I used the same approach for the finding above. **Blast radius.** You are right that merging moves `v3` and every consumer picks this up on its next deployment. Worth saying that this is the good direction for the fleet: `ahmad/expiro`, `ahmad/portfolio` and `ahmad/trip` all wait on containers on the same I/O-saturated host, and all three currently inherit the sixty-second budget that produced the false failures in #22. The open item is the one you named — none of this has run against a real host, so the first live deployment is the proof, and it is also the first test of whether ten minutes is the right number. That it is now an input rather than a constant is what makes being wrong about it cheap.

deploy-vps/action.yml

					
				@ -328,2 +349,3 @@

				          for attempt in $(seq 1 30); do

				          DEADLINE=$(( $(date +%s) + HEALTH_TIMEOUT ))

				          while :; do

				            STATUS="$(docker inspect --format '{{ .State.Health.Status }}' "${container}" 2>/dev/null || echo unknown)"

unknown is as settled a verdict as unhealthy, but it now costs the whole budget.

This falls back to unknown for two cases that waiting cannot fix: the container does not exist — a wrong or stale name in healthcheck-containers — and the image defines no HEALTHCHECK, so the template has nothing to read. Neither resolves by polling for longer, yet both fall through to the deadline branch.

Raising the default from 60s to 600s makes that ten times more expensive. I ran your loop against a stub docker whose inspect always fails, which is what a mistyped container name looks like:

typo-name did not become healthy within 8s (last status: unknown)
exit=1  elapsed=8s   ← the entire budget, every time

With the real default that is ten minutes per container before anyone learns the name is wrong, where today it is one. It is the same argument you make for unhealthy — a settled state should not cost the budget — applied to the other settled state.

The nuance worth keeping is that unknown can be transient for a moment right after up -d, while unhealthy never is. So rather than failing on the first sight of it:

UNKNOWN_GRACE=$(( $(date +%s) + 30 ))
# ... inside the loop, before the deadline check:
if [ "${STATUS}" = "unknown" ] && [ "$(date +%s)" -ge "${UNKNOWN_GRACE}" ]; then
  echo "${container} is not reporting a health status after 30s — check the name in" >&2
  echo "healthcheck-containers and that the image defines a HEALTHCHECK." >&2
  docker logs --tail 50 "${container}" >&2 || true
  exit 1
fi

That keeps the generous budget for the case it was raised for — a real container genuinely starting slowly — without spending it on a container that was never going to appear. Not blocking: the outcome is still correct today, only slow, and acceptance criterion 3 names unhealthy alone.

**`unknown` is as settled a verdict as `unhealthy`, but it now costs the whole budget.** This falls back to `unknown` for two cases that waiting cannot fix: the container does not exist — a wrong or stale name in `healthcheck-containers` — and the image defines no `HEALTHCHECK`, so the template has nothing to read. Neither resolves by polling for longer, yet both fall through to the deadline branch. Raising the default from 60s to 600s makes that ten times more expensive. I ran your loop against a stub `docker` whose `inspect` always fails, which is what a mistyped container name looks like: ``` typo-name did not become healthy within 8s (last status: unknown) exit=1 elapsed=8s ← the entire budget, every time ``` With the real default that is ten minutes per container before anyone learns the name is wrong, where today it is one. It is the same argument you make for `unhealthy` — a settled state should not cost the budget — applied to the other settled state. The nuance worth keeping is that `unknown` *can* be transient for a moment right after `up -d`, while `unhealthy` never is. So rather than failing on the first sight of it: ```sh UNKNOWN_GRACE=$(( $(date +%s) + 30 )) # ... inside the loop, before the deadline check: if [ "${STATUS}" = "unknown" ] && [ "$(date +%s)" -ge "${UNKNOWN_GRACE}" ]; then echo "${container} is not reporting a health status after 30s — check the name in" >&2 echo "healthcheck-containers and that the image defines a HEALTHCHECK." >&2 docker logs --tail 50 "${container}" >&2 || true exit 1 fi ``` That keeps the generous budget for the case it was raised for — a real container genuinely starting slowly — without spending it on a container that was never going to appear. Not blocking: the outcome is still correct today, only slow, and acceptance criterion 3 names `unhealthy` alone.

ahmad changed title from ~~fix: Wait longer for a container to report healthy~~ to WIP: fix: Wait longer for a container to report healthy

2026-09-02 22:34:55 +00:00

ahmad commented

2026-09-02 22:34:59 +00:00

Fixed in a467713: the loop now fails on a persistent unknown after a thirty-second grace.

You are right, and the argument is the one I made for unhealthy turned on the state I left out. A container reporting no health status is either missing — a wrong or stale name in healthcheck-containers — or built from an image with no HEALTHCHECK, and neither resolves by waiting. Raising the budget made discovering that ten times more expensive, which is a regression I introduced rather than one I inherited.

I kept your nuance about it being briefly true after up -d, so it is a grace rather than an immediate failure, and the message names both causes so the next person does not have to guess which one they have.

Re-ran the whole matrix, with a shortened grace where the case needs one:

case	statuses	exit	elapsed
healthy immediately	`healthy`	0	0s
healthy on the fourth poll	`starting` ×3, `healthy`	0	6s
unhealthy early	`starting`, `unhealthy`	1	2s
unknown forever (grace 6s)	`unknown`	1	6s
unknown then healthy (grace 6s)	`unknown` ×2, `healthy`	0	4s
never healthy (budget 8s)	`starting`	1	8s

Rows four and five are the ones you asked for: a name that will never appear costs the grace rather than the budget, and a container that simply had not registered a status yet still succeeds.

Back to WIP: while this is unreviewed; I will re-mark it ready once you have looked. There is no CI here to wait for — which is #25.

Fixed in `a467713`: the loop now fails on a persistent `unknown` after a thirty-second grace. You are right, and the argument is the one I made for `unhealthy` turned on the state I left out. A container reporting no health status is either missing — a wrong or stale name in `healthcheck-containers` — or built from an image with no `HEALTHCHECK`, and neither resolves by waiting. Raising the budget made discovering that ten times more expensive, which is a regression I introduced rather than one I inherited. I kept your nuance about it being briefly true after `up -d`, so it is a grace rather than an immediate failure, and the message names both causes so the next person does not have to guess which one they have. Re-ran the whole matrix, with a shortened grace where the case needs one: | case | statuses | exit | elapsed | | --- | --- | --- | --- | | healthy immediately | `healthy` | 0 | 0s | | healthy on the fourth poll | `starting` ×3, `healthy` | 0 | 6s | | unhealthy early | `starting`, `unhealthy` | 1 | 2s | | **unknown forever** (grace 6s) | `unknown` | **1** | **6s** | | **unknown then healthy** (grace 6s) | `unknown` ×2, `healthy` | **0** | **4s** | | never healthy (budget 8s) | `starting` | 1 | 8s | Rows four and five are the ones you asked for: a name that will never appear costs the grace rather than the budget, and a container that simply had not registered a status yet still succeeds. Back to `WIP:` while this is unreviewed; I will re-mark it ready once you have looked. There is no CI here to wait for — which is `#25`.

ahmad added 1 commit

2026-09-02 22:34:59 +00:00

fix: Do not spend the budget on a container that never appears a46771341d

A container that reports no health status is either missing — a wrong name
in healthcheck-containers — or built from an image with no HEALTHCHECK, and
waiting fixes neither. Raising the budget to ten minutes made that ten times
more expensive to discover.

It now fails after a thirty-second grace, which still tolerates the moment
after up -d when a real container has not registered a status yet.

ahmad commented

2026-09-02 23:02:42 +00:00

Ready again — and I put it back to WIP: incorrectly last time.

The rule I follow is to move a ready pull request back to WIP: before pushing review fixes, which I did. But the reviewer here only looks at pull requests that have dropped that prefix, so saying "I will re-mark it ready once you have looked" created a wait that could never end: it was waiting for a review, and the review was waiting for it to be ready. My mistake, and the fix is simply to mark it ready as soon as the fix is pushed and verified, which is what I have done.

Nothing has changed since a467713 and its evidence table above. There is no CI in this repository to go green — that gap is #25.

Ready again — and I put it back to `WIP:` incorrectly last time. The rule I follow is to move a ready pull request back to `WIP:` before pushing review fixes, which I did. But the reviewer here only looks at pull requests that have *dropped* that prefix, so saying "I will re-mark it ready once you have looked" created a wait that could never end: it was waiting for a review, and the review was waiting for it to be ready. My mistake, and the fix is simply to mark it ready as soon as the fix is pushed and verified, which is what I have done. Nothing has changed since `a467713` and its evidence table above. There is no CI in this repository to go green — that gap is #25.

ahmad changed title from ~~WIP: fix: Wait longer for a container to report healthy~~ to fix: Wait longer for a container to report healthy

2026-09-02 23:02:44 +00:00

ahmad_bot approved these changes

2026-09-02 23:36:10 +00:00

ahmad_bot left a comment

Approving. Re-reviewed the delta only (af008f77 → a4677134); the rest of the change was verified in review 98.

The finding is properly addressed, and I checked the part of it that could have quietly failed. The fix keys on STATUS = unknown, but that sentinel comes only from the || echo unknown fallback — it fires only if docker inspect actually exits non-zero. For a missing container that is obvious; for the second case the commit names, an image with no HEALTHCHECK, it is not, because there the object exists and only the field is absent. Traced it end to end:

The daemon omits the key entirely — Health *Health carries json:",omitempty" in ContainerState (moby v25.0.5, api/types/types.go:281).
The typed template execution therefore fails: {{ .State.Health.Status }} against a nil *Health gives nil pointer evaluating *Health.Status (reproduced in Go 1.21).
TemplateInspector.Inspect then falls back to the raw JSON — and tryRawInspectFallback re-executes with Option("missingkey=error") (docker/cli v25.0.5, cli/command/inspect/inspector.go:117-131), so the absent key is an error there too rather than the <no value> a plain template would print (also reproduced).

Both branches error, so docker inspect exits non-zero in both cases and the grace fires as documented. Had the fallback not set missingkey=error, this would have printed <no value> at exit 0 and the new branch would never have run for the no-HEALTHCHECK case.

Behaviour, measured against a stub docker driving each status path:

case	budget	result
never resolvable	600s	exit 1 at 31s (was the full 600s)
`unknown` for 6s, then healthy	600s	exit 0 at 6s — the grace covers the moment after `up -d`
`unhealthy`	600s	exit 1 at 0s, unchanged
never resolvable	8s	exit 1 at the deadline, `last status: unknown`
`starting` throughout	10s	exit 1 at the deadline — the slow-start case still gets the whole budget

UNKNOWN_GRACE is recomputed per container inside the for, so one slow container does not eat another's grace. With a budget under 30s the grace never fires and the deadline reports instead — the message still names the status, so nothing is lost. README and code agree.

Two things merging carries, neither a defect in this change: it moves v3, so expiro, portfolio and trip pick it up on their next deployment; and this repository still runs no pull-request pipeline (platform-actions#25), so nothing has executed this outside the stubs above. The first real deployment is the only test of whether ten minutes is the right budget.

Approval does not merge — that stays with you.

Approving. Re-reviewed the delta only (`af008f77` → `a4677134`); the rest of the change was verified in review 98. **The finding is properly addressed, and I checked the part of it that could have quietly failed.** The fix keys on `STATUS = unknown`, but that sentinel comes only from the `|| echo unknown` fallback — it fires only if `docker inspect` actually *exits non-zero*. For a missing container that is obvious; for the second case the commit names, an image with no `HEALTHCHECK`, it is not, because there the object exists and only the field is absent. Traced it end to end: - The daemon omits the key entirely — `Health *Health` carries `json:",omitempty"` in `ContainerState` (moby v25.0.5, `api/types/types.go:281`). - The typed template execution therefore fails: `{{ .State.Health.Status }}` against a nil `*Health` gives `nil pointer evaluating *Health.Status` (reproduced in Go 1.21). - `TemplateInspector.Inspect` then falls back to the raw JSON — and `tryRawInspectFallback` re-executes with `Option("missingkey=error")` (docker/cli v25.0.5, `cli/command/inspect/inspector.go:117-131`), so the absent key is an error there too rather than the `<no value>` a plain template would print (also reproduced). Both branches error, so `docker inspect` exits non-zero in both cases and the grace fires as documented. Had the fallback not set `missingkey=error`, this would have printed `<no value>` at exit 0 and the new branch would never have run for the no-`HEALTHCHECK` case. **Behaviour, measured** against a stub `docker` driving each status path: | case | budget | result | |---|---|---| | never resolvable | 600s | exit 1 at **31s** (was the full 600s) | | `unknown` for 6s, then healthy | 600s | exit 0 at 6s — the grace covers the moment after `up -d` | | `unhealthy` | 600s | exit 1 at 0s, unchanged | | never resolvable | 8s | exit 1 at the deadline, `last status: unknown` | | `starting` throughout | 10s | exit 1 at the deadline — the slow-start case still gets the whole budget | `UNKNOWN_GRACE` is recomputed per container inside the `for`, so one slow container does not eat another's grace. With a budget under 30s the grace never fires and the deadline reports instead — the message still names the status, so nothing is lost. README and code agree. Two things merging carries, neither a defect in this change: it moves `v3`, so expiro, portfolio and trip pick it up on their next deployment; and this repository still runs no pull-request pipeline (`platform-actions#25`), so nothing has executed this outside the stubs above. The first real deployment is the only test of whether ten minutes is the right budget. Approval does not merge — that stays with you.

ahmad merged commit cc1d5eb102 into main

2026-09-02 23:36:40 +00:00

ahmad referenced this pull request from a commit

2026-09-02 23:36:41 +00:00

fix: Wait longer for a container to report healthy (#24)

ahmad deleted branch feature/22-health-wait-budget

2026-09-02 23:36:43 +00:00

ahmad referenced this pull request

2026-09-03 00:03:22 +00:00

deploy-vps healthcheck wait is a hard-coded 60s #23

ahmad referenced this pull request

2026-09-03 00:04:54 +00:00