You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test(e2e): lock the behavior of the foreground up - #14300
It adds end-to-end scenarios that pin the observable behavior of the foreground (non-detached) compose up, so the upcoming switch of its machinery to the plan engine can be reviewed against a fixed specification.
Context
Epic #14081 converges the start phase into the plan engine. The foreground up is the riskiest consumer: it creates, then attaches the log printer and the monitor, then starts, with each phase currently driven by its own code path. Most e2e coverage of the start phase runs detached (up -d); the foreground path is covered by the cascade, exit-code and Ctrl+C tests, but not for hooks, health-gated dependencies, containers that exit as soon as they start, restarts, recreation, dependency timeouts or dry-run.
What the PR brings
Seven scenarios, each a one-line obligation on the foreground up:
post_start output is streamed into the session, and the service sees what pre_start wrote (pre_start's own output is not streamed by a foreground up today, and is deliberately not asserted);
--abort-on-container-exit notices a container that exits the instant it starts, instead of hanging;
a container that exited earlier is started again, not recreated;
a service is not started before its service_healthy dependency is healthy;
after a config change the service is recreated and the new container's output is streamed;
a dependency that stays unhealthy fails the up with a timeout, the dependent being created but never started;
--dry-run plans the create phase only.
Tests only, no production code. Every scenario passes on the current code, and the health-gating one was checked to fail when the dependency is removed. The use_api_socket case is left out for now: it could not be validated in the environment this was written in.
Why this is the right next brick
The switchover that follows moves the interactive up from two snapshots to a single plan. Locking the current behavior first means any difference it introduces shows up as a failing scenario rather than as a surprise in review.
Adds seven foreground compose up scenarios to protect behavior during the plan-engine migration in #14081. The current stacked diff also includes underlying lifecycle changes from earlier PRs.
Changes:
Covers hooks, immediate exits, restarts, health gates, recreation, timeouts, and dry-run.
Moves lifecycle callers onto shared planning and execution.
Adds regression coverage for ordering, deadlines, providers, and observed state.
The reason will be displayed to describe this comment to others. Learn more.
Assessment: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
[medium] pkg/e2e/up_interactive_test.go:99 — OutputNotContains("Start") is too broad a substring check for dry-run verification (confidence: weak 52/100)
The reason will be displayed to describe this comment to others. Learn more.
[medium] TestExecutePlanCreatePhaseFailureCancelsCreatePhase may hang 5 s on some scheduler orderings
NetworkCreate returns errors.New("boom") immediately — no blocking. When the Go scheduler lets the network goroutine complete before the volume goroutine enters acquireSlot, phaseCancel fires and cancels the shared nodeCtx. The volume runNode then exits at acquireSlot(nodeCtx, …) without ever calling VolumeCreate, so volumeCreateDone is never closed. The select block below waits the full 5 s before t.Fatal fires.
The gomock.EXPECT().VolumeCreate(…) (no .Times(0)) also requires exactly one call, so gomock's test-cleanup assertion adds a second failure mode.
Suggested fix: make the volume goroutine block beforeacquireSlot until an external signal confirms it started, so the test can guarantee VolumeCreate is entered before the network failure propagates:
// in the VolumeCreate mock:started:=make(chanstruct{})
apiClient.EXPECT().VolumeCreate(gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, _ client.VolumeCreateOptions) (client.VolumeCreateResult, error) {
close(started) // signal: we are inside the calldeferclose(volumeCreateDone)
<-ctx.Done()
volumeCreateCtxErr=ctx.Err()
return client.VolumeCreateResult{}, ctx.Err()
})
// make NetworkCreate wait until VolumeCreate has startedapiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()).
DoAndReturn(func(...) (client.NetworkCreateResult, error) {
<-startedreturn client.NetworkCreateResult{}, errors.New("boom")
})
Honor optional dependency timeouts during condition checks
pkg/compose/service_containers.go:235
Optional timeouts are skipped only when the loop observes ctx.Done(). If the wait deadline expires during ContainerInspect for service_completed_successfully, checkDependencyCompleted returns the deadline error and the post-check return bypasses this branch. The deployment then fails despite required: false. Apply the same own-timeout check before returning condition-check errors, preserving errors from inherited deadlines, and add a test with an inspection blocked until the wait deadline.
Preserve shared-resource consumers during scoped reconciliation
pkg/compose/watch.go:679
Narrowing the model before resource reconciliation hides consumers of shared networks and volumes: servicesUsingNetwork and servicesUsingVolume inspect only project.Services. For example, after a same-name network configuration change, watch --no-up rebuilding one of two independent services sharing that network plans to disconnect only the selected service. Network removal then fails because the other service remains attached, leaving the selected service stopped. Preserve the full consumer set for shared-resource reconciliation while scoping container/start operations, or explicitly skip shared-resource convergence when consumers are excluded.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
This review covers only the commits pushed since 3b8be6e3cdda4d4e75f42e4745bac2671d6ebdd3.
Assessment: 🟢 APPROVE
The incremental diff is small and well-targeted: four changes across the hook-logs compose fixture and the interactive test file.
TestUpInteractiveHookLogs: The pre_start command correctly switches from tee (which emitted to stdout) to a plain redirect, aligning the fixture with the documented behavior that foreground up does not stream pre_start output. The assertion is updated to test the hook's effect (app-read:pre-start-file-content) rather than its output. Sound.
TestUpInteractiveImmediateExit: Wrapping ServiceState("long", "exited") in Eventually(..., 10*time.Second) addresses the race between the stop signal and the daemon's state update that was flagged in the prior review. Good fix.
TestUpInteractiveDependencyTimeout: The added comment clearly documents the ServiceState("web", "created") invariant, resolving the ambiguity raised in the prior review.
TestUpDryRunCreatesNothing: Narrowing OutputNotContains("Start") to OutputNotContains(s.Project()+"-app-1 Start") avoids a false-negative on unrelated output that could happen to contain the word "Start". The narrower string is still a reliable signal for the intended contract.
Wait timeout starts before dependent prerequisites finish
pkg/compose/executor.go:400
The timeout now starts before all of the dependent's prerequisites finish starting. waitConditionNode waits only for the dependency it polls; the old InDependencyOrder traversal finished every prerequisite's start chain before calling waitDependencies. For example, with web depending on healthy db and started worker, db becoming healthy at 8 seconds, and worker's startup hook taking 10 seconds, --wait-timeout 3 previously succeeded but can now fail around 3 seconds. Preserve the dependent's prerequisite-start barrier for timed waits and add a multi-dependency regression. Shared wait nodes need special handling so the fix does not introduce dependency cycles.
Optional dependency timeouts during inspection incorrectly fail startup
pkg/compose/service_containers.go:235
The new optional-timeout handling only covers expiry between polls. For an optional service_completed_successfully dependency, a timeout during ContainerInspect propagates through isServiceCompleted and checkDependencyCompleted, then returns at lines 269–270 without reaching this branch. Startup therefore fails instead of skipping the dependency. Apply the same own-versus-inherited deadline handling to polling errors, and test an inspect that blocks until the wait deadline expires; the current short-timeout tests expire before the first poll.
Narrowing project.Services also hides shared infrastructure consumers from servicesUsingNetwork and servicesUsingVolume. If a same-name shared network needs recreation during a rebuild, an unrelated running service is excluded from the disconnect plan, so NetworkRemove fails on its remaining endpoint after the selected containers have already been stopped. Approved volume recreation has the analogous in-use failure. Keep the full model available for infrastructure reconciliation and scope service create/start work separately.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
Assessment: 🟢 APPROVE
Seven well-structured e2e scenarios that pin the observable foreground up behaviors before the upcoming plan-engine switchover. The production-code changes refactor the executor into a two-phase (Create → Start) barrier model with per-wait-node timeout scoping — the logic is sound, edge cases (ScopeStart relay restart, optional-dependency deadline inheritance) are correctly handled, and the test suite is comprehensive.
Lower-confidence findings (not posted inline)
[low] pkg/compose/reconcile.go:1369 — planProviderStart's else-if condition fires when a service node exists but is already in ends, not only when the node is absent (confidence: negligible — unreachable via any valid project plan; maintenance hazard only)
[low] pkg/compose/service_containers_test.go:867 — timing-dependent test: time.Sleep(10ms) after a 1ms deadline to guarantee expiry; robust in practice but theoretically racy on heavily-loaded CI (confidence: negligible)
[low] pkg/compose/service_containers.go:163 — three-level context nesting (caller → timeout-wrap → errgroup) is correct but subtle; origCtx comment already documents the invariant well (informational only, no action needed)
Copilot review finding on docker#14300: if a sibling Create-phase failure ever
stopped canceling the other node, VolumeCreate's mock blocks on its ctx
while exec.run waits for it, so the test's own five-second guard could
never fire and the regression would hang until the suite-wide timeout.
Give run a deadline of its own and assert the cancellation is the
sibling's (context.Canceled), not that fallback.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Assessment: 🟢 APPROVE
The refactor is well-structured and the e2e scenarios are sound. No CONFIRMED or LIKELY bugs were found in the added code.
Lower-confidence findings (not posted inline)
[low] pkg/compose/executor.go:419 — translateWaitTimeout returns a brand-new opaque error, severing the context.DeadlineExceeded chain from errors.Is callers (confidence: weak 26/100). No current caller is affected; a future caller checking errors.Is(err, context.DeadlineExceeded) would get a false negative. Consider fmt.Errorf("timeout waiting for dependencies: %w", err) to preserve the chain.
[low] pkg/compose/service_containers.go:230 — origCtx.Err() == nil disambiguation for optional-dependency timeout has an inherent sub-nanosecond race window where an inherited parent deadline firing at the exact same instant could be swallowed as a "this call's own timeout" (confidence: weak 26/100). Acknowledged as a known limitation in the code comment; impact is dropping a warning instead of propagating an error for an optional dependency. Pre-existing technique, first applied to this new code surface.
[low] pkg/compose/watch.go:693 — rebuild() now returns createAndStart errors; the distinct create-phase vs. start-phase failure log messages are collapsed into one combined message (confidence: weak 26/100). The rebuild scheduler discards the returned error with _ = s.rebuild(...) so no runtime impact; this is purely a diagnostic observability note.
[low] pkg/e2e/up_interactive_test.go:67 — TestUpInteractiveWaitsForHealthy has no service-state assertion after up exits (confidence: weak 26/100). If a log-streaming race causes up to exit before web prints its output, OutputContains("web-saw-ready") would fail with a misleading diagnostic. Adding a ServiceState/Eventually(ServiceState(...)) guard for web's final state would make failures self-diagnosing.
[low] pkg/e2e/up_interactive_test.go:99 — TestUpDryRunCreatesNothing uses OutputNotContains(s.Project()+"-app-1 Start") to assert no start event is emitted (confidence: weak 26/100). If the dry-run progress renderer emits "Starting" rather than "Start", the check passes vacuously. Worth verifying the substring matches the renderer's actual verb.
Copilot review finding on docker#14300: if a sibling Create-phase failure ever
stopped canceling the other node, VolumeCreate's mock blocks on its ctx
while exec.run waits for it, so the test's own five-second guard could
never fire and the regression would hang until the suite-wide timeout.
Give run a deadline of its own and assert the cancellation is the
sibling's (context.Canceled), not that fallback.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review finding on #14300: if a sibling Create-phase failure ever
stopped canceling the other node, VolumeCreate's mock blocks on its ctx
while exec.run waits for it, so the test's own five-second guard could
never fire and the regression would hang until the suite-wide timeout.
Give run a deadline of its own and assert the cancellation is the
sibling's (context.Canceled), not that fallback.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Most e2e coverage of the start phase runs detached; the foreground
`up`, whose create and start phases are driven by separate code paths,
had none for hooks, health-gated dependencies, a container that exits as
soon as it starts, restarts of exited containers, recreation, dependency
timeouts or dry-run. Add scenarios that fix these observables before the
machinery behind it changes.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…reams
The hook scenario matched "pre-start-hook-ran" in the output, but the
service's own line "app-read:pre-start-hook-ran" contains it, so the
assertion passed without any pre_start output reaching the session. A
foreground up streams post_start's output but not pre_start's today
(checked on main too). Lock what actually holds: pre_start's effect, read
by the service through a value distinct from anything it logs.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Wait for the long-running service's final state after an abort instead of
reading it the instant the command returns, document why the dependent is
expected in the created state on a dependency timeout, and narrow the
dry-run "no start" check to the service's own lines.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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
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.
docker-agent
left a comment
Follows #14296 (merged). Independent of #14302 and #14304.
What this PR does, in one sentence
It adds end-to-end scenarios that pin the observable behavior of the foreground (non-detached)
compose up, so the upcoming switch of its machinery to the plan engine can be reviewed against a fixed specification.Context
Epic #14081 converges the start phase into the plan engine. The foreground
upis the riskiest consumer: it creates, then attaches the log printer and the monitor, then starts, with each phase currently driven by its own code path. Most e2e coverage of the start phase runs detached (up -d); the foreground path is covered by the cascade, exit-code and Ctrl+C tests, but not for hooks, health-gated dependencies, containers that exit as soon as they start, restarts, recreation, dependency timeouts or dry-run.What the PR brings
Seven scenarios, each a one-line obligation on the foreground
up:post_startoutput is streamed into the session, and the service sees whatpre_startwrote (pre_start's own output is not streamed by a foregrounduptoday, and is deliberately not asserted);--abort-on-container-exitnotices a container that exits the instant it starts, instead of hanging;service_healthydependency is healthy;upwith a timeout, the dependent being created but never started;--dry-runplans the create phase only.Tests only, no production code. Every scenario passes on the current code, and the health-gating one was checked to fail when the dependency is removed. The
use_api_socketcase is left out for now: it could not be validated in the environment this was written in.Why this is the right next brick
The switchover that follows moves the interactive
upfrom two snapshots to a single plan. Locking the current behavior first means any difference it introduces shows up as a failing scenario rather than as a surprise in review.Part of #14081.
🤖 Generated with Claude Code