From 109d21c58ed50e631c6398d8e5b0933060ab5caf Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 11:24:08 +0100 Subject: [PATCH 1/2] docs(compose): correct what the startMx comment says about docker/compose#12846 docker/compose#12846 was not a race between container starts but a flaw of the daemon's port allocator, fixed in Engine 28.3.0: the mutex added as its workaround could not help, and the comment said the opposite. Say what the mutex does and does not provide, and that the sequential start of a service's replicas (#8530) is the plan's replica chain, not this mutex. Also fix the comment of the test of a stale non-relay container of a provider-backed service, which still described a special case the start path no longer has: planServiceStart routes every provider to planProviderStart, which decides bare-vs-enriched per container. Comments only: the token stream of every Go file, comments excluded, is unchanged. Part of #14081 Signed-off-by: Nicolas De Loof --- pkg/compose/reconcile_start_test.go | 8 ++++---- pkg/compose/service_containers.go | 28 +++++++++++++++++++--------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 501065aed5..3e96df8abe 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -992,10 +992,10 @@ 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: an existing non-relay container of +// a provider-backed service is started like any ordinary replica, post_start +// hooks included, the bare-vs-enriched choice being made per container and +// 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) { diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index ea7eba795f..9a853317f5 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -446,11 +446,21 @@ 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. +// 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 could not 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 this path takes it: restart, run, the relay and the hook runners start // their containers without it. // @@ -458,10 +468,10 @@ func (s *composeService) createContainer(ctx context.Context, project *types.Pro // 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, From f5ddec27025f1897b9b1ed6f5a331b75a08a7489 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 11:27:58 +0100 Subject: [PATCH 2/2] docs(compose): tighten the startMx and provider test comments after review Say that a relay restarted by a start plan does take startMx while the deployment of the relay does not, that the lock was unlikely (rather than unable) to help against #12846, and name isReplicaRelay, which is what gates pre_start. Rewrap the test comment. Comments only. Part of #14081 Signed-off-by: Nicolas De Loof --- pkg/compose/reconcile_start_test.go | 9 ++++----- pkg/compose/service_containers.go | 9 +++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 3e96df8abe..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: an existing non-relay container of -// a provider-backed service is started like any ordinary replica, post_start -// hooks included, the bare-vs-enriched choice being made per container and -// not from the service's declared type. 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 9a853317f5..56122c4a26 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -452,8 +452,8 @@ func (s *composeService) createContainer(ctx context.Context, project *types.Pro // 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 could not help against that flaw, and no other reason to -// keep the mutex is known. +// 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 @@ -461,8 +461,9 @@ func (s *composeService) createContainer(ctx context.Context, project *types.Pro // (each replica's start waits for the previous one's), and does not depend on // this mutex. // -// Only this path takes it: restart, run, the relay and the hook runners start -// their containers without it. +// 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.