fix(up): enforce healthcheck.timeout and treat start_period as a grace window - #151
Open
Mikimoto wants to merge 1 commit into
Open
fix(up): enforce healthcheck.timeout and treat start_period as a grace window#151Mikimoto wants to merge 1 commit into
Mikimoto wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(up): enforce
healthcheck.timeoutand treatstart_periodas a grace windowSummary
depends_on: condition: service_healthyworks today (#61, #75, #117), but two of the fivehealthcheckfields do not behave as Compose specifies:timeoutis decoded and never applied. A probe that blocks stallsupindefinitely.start_periodis implemented as an unconditional pre-sleep, not as Compose's grace window.Every service that declares one pays it in full even when it is ready immediately.
Both are in
waitUntilServiceIsHealthy.1.
timeoutis parsed but never appliedHealthcheck.timeoutis decoded (Codable Structs/Healthcheck.swift:79) and has parsing tests,but
waitUntilServiceIsHealthy(Commands/ComposeUp.swift:1247) never reads it:streamCommand(Commands/ComposeUp.swift:1481) resumes its continuation only fromprocess.terminationHandler, so the probe is unbounded.grep -rn timeout Sources/returns hitsonly in
Healthcheck.swiftplus unrelated comments about the start wait.Reproducer
docker compose uphangunhealthy; fails in ~5s withdependency failed to startcontainer-compose uponmainupis silent for ~30 minutesThis is not synthetic. Healthchecks that talk to a socket block for real:
bash's
/dev/tcphas no connect timeout, so against a filtered (not refused) port it blocks untilthe kernel gives up.
timeout: 5sis exactly what is supposed to bound it.2.
start_periodis a pre-sleep, not a grace windowCompose semantics are the other way round: probes do run during
start_period, their failuresdo not consume
retries, and the first success ends the window immediately. Today the firstprobe cannot run until the whole window has elapsed, so
start_period: 40scosts 40 seconds evenfor a service that is ready in 200 ms.
upstarts services sequentially (#128 tracks parallelising it), so the cost is additive across theproject. Measured on a real 23-service compose file (YAML merge keys resolved, so anchor-inherited
values are counted): 18 services declare a healthcheck — 9 at
start_period: 10s, 4 at 15s, 1 at20s, 3 at 30s, 1 at 40s. That is 300 seconds of mandatory sleep before those services are first
probed, regardless of how quickly they actually become ready.
What this PR changes
streamCommandgains an optionaltimeout:. When it elapses and the child is still running theprocess is terminated; the termination handler then fires with a non-zero status, so the caller
sees an ordinary failed check. This matches Docker, where a timed-out probe counts as one failure
rather than as an error.
waitUntilServiceIsHealthypasseshealthcheck.timeout(Compose default 30s) to every probe.start_periodbecomes a grace window: probes run atintervalinside it, failures there do notconsume the retry budget, and the first success returns.
timeout:defaults tonil, so the two existingstreamCommandcall sites (run, and theforeground service stream) keep their current unbounded behaviour. No other call site changes.
Tests
New
Tests/Container-Compose-StaticTests/HealthcheckTimeoutTests.swift:streamCommandterminates a child that overruns its timeout:sleep 30withtimeout: 1returns non-zero in ~1.1 s. On
mainthe same call takes 30 s and returns 0.timeoutresolves to the Compose default of 30 s, not to "no timeout".These need no running
containerdaemon.Verification
Run on macOS 27.0 / Swift 6.4, branched from
main@6e6aaf0:swift buildswift test(static suites)Negative control: with
process.terminate()deleted from the new timeout branch, thestreamCommand terminates a child that overruns its timeouttest fails on both expectations andtakes 30.039 s instead of 1.090 s. The test does discriminate the fix from its absence.
Not covered: an end-to-end
container-compose upagainst a live daemon. Thestart_periodchangehas no unit test for the same reason, only the reasoning above and the existing dynamic suite.
Notes and limits
container execdoes not guarantee the process inside the guest isreaped. Docker has the same caveat with its own exec-based probes.
interval, so withinterval > start_periodit can overshoot thewindow by up to one interval. Docker behaves the same way.
start_periodas ahard floor to mask a startup race will now proceed earlier. That is the Compose-correct behaviour,
but it is worth a line in the release notes.
container recover. Container-Compose still treats the health wait as a one-shot gate during
up.That is a larger change and belongs with full support: depends_on #68.