Repository navigation
refactor(compose): drop the timeout parameter of waitDependencies - #14400
Merged
ndeloof merged 3 commits intoOct 27, 2026
Merged
Conversation
Its four callers (up --wait, start --wait, run, restart) all pass 0, so the wrapper it built around the context was dead: a wait only gets a timeout of its own through the plan's OpWaitCondition nodes, which bound it with exec.waitTimeout in execBoundedWaitCondition and never go through waitDependencies. A deadline on the caller's context, which is how up and start --wait bound their wait, is untouched. The tests that passed a timeout now exercise the same semantics through the live paths: a deadline on the caller's context for waitDependencies, and execBoundedWaitCondition with exec.waitTimeout for the wait's own timeout (tolerated on an optional dependency, an error on a required one). Part of docker#14081 Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Now that waitDependencies has no timeout parameter, the plan's wait nodes are the only ones to wrap the context of a wait: tell so in the comment of waitDependency, which compares the wrapped context with the original. Comments only. Part of docker#14081 Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…line class Review follow-ups of the removal of waitDependencies' timeout parameter: the unsupported-condition test lost the 2s bound the parameter gave it, so a regression that waits would hang instead of failing; and the test comparing a timeout that expires during the inspection with one that expires between polls now also compares whether they are a deadline. Also reword the comment of execWaitCondition, which still said that waitDependencies threads a timeout through its waits. Part of docker#14081 Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
24 of 25 tasks
dock-tor
approved these changes
Oct 27, 2026
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.
What this PR does, in one sentence
Removes the
timeoutparameter ofwaitDependencies, which every caller sets to 0, and moves the tests that used it onto the paths that really bound a wait.Context
waitDependencieswraps its context incontext.WithTimeoutwhentimeout > 0. All four callers outside the tests —up --wait,start --wait,runandrestart— pass a literal0, and nothing else can reach this unexported method. So the wrapper is dead. The two ways a wait really gets a deadline do not use it:upandstart --waitwrap the context themselves before the call, and the plan'sOpWaitConditionnodes are bounded byexec.waitTimeoutinexecBoundedWaitCondition, which callswaitDependencyReacheddirectly.That dead parameter is what the unit tests of the dependency waits were built on, which hides where the semantics they lock actually live.
What the PR brings
waitDependenciesloses itstimeoutparameter and the dead branch; its callers drop the0. Nothing else changes in the production code, apart from two comments that said who gives a wait a timeout of its own.waitDependencies(the--waitroute), andexecBoundedWaitConditionwithexec.waitTimeoutfor the wait's own timeout, tolerated on an optional dependency and an error on a required one.Why this is no functional change
waitDependencies(method value, interface, reflection, other package), and the four callers passed a literal0.mainand from this branch, the binaries have the same symbols; onlywaitDependencieschanges size (1552 → 1280 bytes), next to the usual line tables and build info.example-providerin thePATH, andTestComposeModel).go vet,gofmt, the linter (only the known findings) andgo test ./pkg/... -racepass, exceptpkg/watch, which fails the same way onmainon macOS.No coverage lost
Seven mutations of
waitDependencyReached,waitDependenciesandexecBoundedWaitCondition, run againstTestWaitDependencyDeadlineandTestOptionalDependencyInspectFailure:waitDependenciesno longer reports "timeout waiting for dependencies"execBoundedWaitConditionignoresexec.waitTimeoutexecBoundedWaitConditionno longer markserrWaitTimeoutThe last two were only caught by the executor tests; they now are by these as well.
Part of #14081 (Lot 3).
🤖 Generated with Claude Code