Repository navigation
Conversation
The monitor ends with the application it follows: pin that its initial listing and its event subscription are scoped to the project's own containers (no one-offs, no hook runners without a config hash), and that a provider's relay, which never terminates, is not waited for, whether it is there at the start or created later. Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
… order Pin what compose start does around its plan: the --wait verification (ready or not, timeout message, one-shot services satisfied by their completion), the project rebuilt from the project's non-one-off containers when no compose file is given, only the selected services started, and the replicas started in numeric order. Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Pin which dependencies are worth a wait (shouldWaitForDependency), how each container state reads for service_healthy and service_completed_successfully (required and optional, dry-run), the Waiting/Healthy rows of a wait outside a plan, and that the end of a wait's context wins over a ready tick. Add the plan shapes for a wait following its dependency's start, a project of scale: 0 services, and the Start phase covering every service of the project (the current behavior of scale, whose change would be a decision of its own). Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Pin that every ContainerStart holds startMx, that the start, pre_start, post_start and hook-runner nodes do nothing once the context is canceled, that a failure relays untouched down a replica's chain, that the node bookkeeping records cancellations as failures, that the phase returns the originating error, that an invalid plan is refused, that a concurrency cap cannot deadlock the node goroutines, that Starting is reported when the container starts (not during pre_start), that the hook runner targets the lowest-numbered replica, and that stop timeouts reach the daemon in whole seconds. Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Extend the interactive-up daemon fake and pin the behaviors mutation checks of the interactive-up switchover found unlocked: the monitor's Services scope, a Start-phase error while terminated, the cleanup when attaching fails, signal handling starting only after the Create phase, late starters stopped after the teardown sweep, hook logs reaching the session, the wait timeout translation, --exit-code-from and the Running events. Add the wiring of detached up (with its --wait verification), scale, the rebuild of watch and the dependencies of run onto the plan. Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Pin that restart waits for a restart:true dependency to hold its condition again before restarting its dependent, and that its timeout reaches the daemon in whole seconds. Part of docker#14081. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Close the started channel before the start event is emitted, so that a late starter stopped in reaction to it always finds the container started, and close both ends of the pipe the hook logs test hands to the session. 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: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
- [low] pkg/compose/executor_start_test.go:544 — bareStartNode helper function is missing t.Helper() (confidence: weak 31/100)
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: 🟡 NEEDS ATTENTION
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
This review covers only the commits since 09d7231bfdd7.
Assessment: 🟢 APPROVE
The single-hunk change to pkg/compose/up_phases_test.go is correct and complete. It resolves the open review thread (PRRT_kwDOAOWUd86wuKdX) about the goroutine potentially calling t.Error after test completion:
- Buffered channel of capacity 1 (
make(chan error, 1)) guarantees the goroutine can always send without blocking, even if the test goroutine has already exited viaruntime.Goexit()(timeout path). - Goroutine sends exactly once unconditionally —
writeErr <- writeStdcopyFrame(...)— so<-writeErrin the test goroutine never blocks forever on the happy path. - Timeout path is safe:
t.Fatalfcallsruntime.Goexit(), which skips the deferred<-writeErrreceive; the buffered channel absorbs the goroutine's eventual send with no leak and no panic. - Normal path:
assert.NilError(t, <-writeErr)is reached afterrunInteractiveUpreturns, correctly propagating any write failure back to the test goroutine.
No new bugs introduced.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
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
Locks, with tests only, the behaviors of the old imperative start path that the plan engine now carries, so that #14081 can be closed.
Context
The convergence of the start phase into the plan engine (#14081) replaced the imperative start path step by step. Each step was checked against the old behavior, but mostly by hand and by mutation testing in the PRs themselves. Mutating the production code afterwards showed that many of those behaviors were not pinned by any test: of 167 mutations applied to
pkg/compose, 61 survived the test suite ofmain.What the PR brings
depends_ongate and its health/exit conditions, the executor's guarantees,compose start, the monitor's scope, interactive and detachedup, hooks,restart, and the wiring ofscale,watchrebuild andrun.Start.Servicesin the monitor, the tolerance of the Start phase's error once terminated, the cleanup when the daemon time cannot be read, andsignal.Notifyordering.scalekeeps restarting every stopped container of the project, as it does today; a test pins that current behavior, and the comment says that changing it is a decision of its own.Guardrails: no production code changes, tests are deterministic (
synctestfor timers, no sleeps), and the new ones were run with-raceand shuffling.The six mutations still alive are the failure-path cleanups of the interactive
up(attach or daemon-time failure, which the deferred cancel makes unobservable), the precedence of a dependency error over "no container to start", and wait nodes being reported as aborted. Observing them would need production hooks, which is out of scope here.Closes #14081
🤖 Generated with Claude Code