diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 501065aed5..768f442102 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -992,10 +992,9 @@ func TestPlanStart_OrdinaryPathBareStartsAStaleRelay(t *testing.T) { // test for the second Copilot review finding on #14296, the symmetric gap: // a service declared provider: whose observed container is NOT a relay (a // stale normal replica left over from before the service became -// provider-backed) must still be started -- the start path only -// special-cases Provider != nil for the ZERO-containers case, so an -// existing non-relay container is always started normally, -// including post_start hooks. planProviderRelayRestart's first version +// provider-backed) must still be started like any ordinary replica, +// post_start hooks included: bare-vs-enriched is decided per container, not +// from the service's declared type. planProviderRelayRestart's first version // silently dropped this case (filtered on isRelayContainer), returning // success without starting anything. func TestPlanStart_ProviderPathEnrichesStaleNonRelayContainer(t *testing.T) { @@ -1034,7 +1033,7 @@ func TestPlanStart_ProviderPathEnrichesStaleNonRelayContainer(t *testing.T) { "a stale non-relay container must get an enriched start (injection applies), like any ordinary replica:\n%s", plan) // A Copilot review finding on the post_start-only version of this fix: // pre_start must also run for this path, gated on the lowest-numbered - // container to start and on isRelayContainer. + // container to start and on isReplicaRelay. if pre == nil { t.Fatalf("expected pre_start hooks to run before starting prov's stale non-relay container:\n%s", plan) } diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index ea7eba795f..56122c4a26 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -446,22 +446,33 @@ func (s *composeService) createContainer(ctx context.Context, project *types.Pro } // startMx serializes the plan executor's ContainerStart calls across the whole -// process. It is a workaround for docker/compose#12846 (added by #12851): with -// Engine 28.0 to 28.2, containers started concurrently could be assigned the -// same published host port, a flaw of the daemon's port allocator. That flaw -// is fixed since Engine 28.3.0 (moby/moby#50054), so beyond it the mutex serves -// no purpose, and it is unclear it ever prevented the duplicates reported. -// Only this path takes it: restart, run, the relay and the hook runners start -// their containers without it. +// process. It was added by docker/compose#12851 as a workaround for +// docker/compose#12846, which was not a race between container starts but a +// flaw of the daemon's port allocator: with Engine 28.0 to 28.2, a container +// with several random published ports, one on 0.0.0.0 and one on a specific +// address, could be given the same host port twice. That flaw is fixed since +// Engine 28.3.0 (moby/moby#50054). A lock around ContainerStart only orders +// the starts, so it was unlikely to help against that flaw, and no other +// reason to keep the mutex is known. +// +// It is not what starts the replicas of a service one after the other, which +// docker/compose#8530 (a port range shared by the replicas) required and +// docker/compose#10067 introduced: that order is the plan's replica chain +// (each replica's start waits for the previous one's), and does not depend on +// this mutex. +// +// Only the plan executor takes it (a relay restarted by a start plan +// included): restart, run, the deployment of the relay and the hook runners +// start their containers without it. // // TODO: remove startMx, and the locking around it, once Engine 28.x is out of // support. // // While it exists, it deliberately stays a package-level variable rather than -// a composeService field: the race lives in the daemon, not in a service -// instance, so two instances (e.g. SDK users) driving the same daemon must -// still exclude each other. A per-instance lock would silently reintroduce the -// duplicate-port assignment between them. +// a composeService field: what it serializes is a call to the daemon, not +// something of a service instance, so two instances (e.g. SDK users) driving +// the same daemon must still exclude each other. A per-instance lock would +// silently stop serializing the starts of two instances. var startMx sync.Mutex func (s *composeService) createMobyContainer(ctx context.Context, project *types.Project, service types.ServiceConfig,