Skip to content

fix: apply default healthcheck intervals for readycheck sidecars#397

Merged
canercidam merged 1 commit intomainfrom
dvush/faster-startup-checks
Mar 9, 2026
Merged

fix: apply default healthcheck intervals for readycheck sidecars#397
canercidam merged 1 commit intomainfrom
dvush/faster-startup-checks

Conversation

@dvush
Copy link
Contributor

@dvush dvush commented Mar 6, 2026

When ReadyCheck fields (Interval, Timeout, etc.) are zero-valued (e.g. from YAML recipes), Docker interprets "0s" as "use defaults" which means a 30s polling interval. This caused services like ClickHouse to appear to take ~30s to start when they were actually ready in ~2s.

Apply sensible defaults (1s interval, 10s timeout, 30 retries).

When ReadyCheck fields (Interval, Timeout, etc.) are zero-valued
(e.g. from YAML recipes), Docker interprets "0s" as "use defaults"
which means a 30s polling interval. This caused services like
ClickHouse to appear to take ~30s to start when they were actually
ready in ~2s.

Apply sensible defaults (1s interval, 10s timeout, 30 retries).
@dvush dvush requested a review from canercidam as a code owner March 6, 2026 18:25
@claude
Copy link
Contributor

claude bot commented Mar 6, 2026

Claude finished @dvush's task in 57s —— View job


PR Review: fix: apply default healthcheck intervals for readycheck sidecars

  • Read and analyze the changes
  • Provide review feedback

LGTM. The change is well-scoped and solves the right problem. YAML recipes create ReadyCheck{QueryURL: ...} with zero-valued timing fields, which get serialized as "0s" into docker-compose healthcheck config — causing Docker to fall back to its 30s default interval.

One minor observation, not blocking:

The defaults are applied only in the sidecar code path inside Validate(). If a non-QueryURL ReadyCheck is ever constructed with zero-valued fields (e.g. from a future YAML test field), it would hit the same "0s" issue at local_runner.go:676-681. You could consider applying defaults in WithReady() instead to cover all paths, but that's a broader change and not needed for the current use cases since all Test-based checks in components.go already set explicit values.


@canercidam canercidam merged commit 7a16d01 into main Mar 9, 2026
7 checks passed
@canercidam canercidam deleted the dvush/faster-startup-checks branch March 9, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants