fix: don't cache transient errors in version negotiation

Replace sync.Once with sync.Mutex so that only successful version
lookups are cached. Errors (context cancellation, network blips) are
returned without caching, allowing subsequent calls to retry.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>

Author: 2 people

pkg/compose/compose.go

@@ -494,49 +494,62 @@ func (s *composeService) isSwarmEnabled(ctx context.Context) (bool, error) {
 	return swarmEnabled.val, swarmEnabled.err
 }
 
+// runtimeVersionCache caches a version string after a successful lookup.
+// Errors (including context cancellation) are not cached so that
+// subsequent calls can retry with a fresh context.
 type runtimeVersionCache struct {
-	once sync.Once
-	val  string
-	err  error
+	mu  sync.Mutex
+	val string
 }
 
 // RuntimeVersion returns the raw API version reported by the daemon.
 // Callers that need the negotiated/effective client API version should use
 // CurrentAPIVersion instead.
 func (s *composeService) RuntimeVersion(ctx context.Context) (string, error) {
-	s.runtimeVersion.once.Do(func() {
-		version, err := s.apiClient().ServerVersion(ctx, client.ServerVersionOptions{})
-		if err != nil {
-			s.runtimeVersion.err = err
-			return
-		}
-		s.runtimeVersion.val = version.APIVersion
-	})
-	return s.runtimeVersion.val, s.runtimeVersion.err
+	s.runtimeVersion.mu.Lock()
+	defer s.runtimeVersion.mu.Unlock()
+	if s.runtimeVersion.val != "" {
+		return s.runtimeVersion.val, nil
+	}
+	version, err := s.apiClient().ServerVersion(ctx, client.ServerVersionOptions{})
+	if err != nil {
+		return "", err
+	}
+	s.runtimeVersion.val = version.APIVersion
+	return s.runtimeVersion.val, nil
 }
 
 // CurrentAPIVersion returns the API version currently used by the Docker client.
 // Trigger negotiation first so version-gated request shaping matches the version
 // that subsequent API calls will actually use.
+//
+// Lock ordering: currentAPIVersion.mu must be acquired before runtimeVersion.mu
+// (via the RuntimeVersion fallback). No code path should reverse this order.
 func (s *composeService) CurrentAPIVersion(ctx context.Context) (string, error) {
-	s.currentAPIVersion.once.Do(func() {
-		cli := s.apiClient()
-		_, err := cli.Ping(ctx, client.PingOptions{NegotiateAPIVersion: true})
-		if err != nil {
-			s.currentAPIVersion.err = err
-			return
-		}
+	s.currentAPIVersion.mu.Lock()
+	defer s.currentAPIVersion.mu.Unlock()
+	if s.currentAPIVersion.val != "" {
+		return s.currentAPIVersion.val, nil
+	}
 
-		version := cli.ClientVersion()
-		if version != "" {
-			s.currentAPIVersion.val = version
-			return
-		}
+	cli := s.apiClient()
+	_, err := cli.Ping(ctx, client.PingOptions{NegotiateAPIVersion: true})
+	if err != nil {
+		return "", err
+	}
 
-		// Defensive fallback for unexpected client implementations or mocks that
-		// do not populate ClientVersion after a successful negotiated ping.
-		s.currentAPIVersion.val, s.currentAPIVersion.err = s.RuntimeVersion(ctx)
-	})
+	version := cli.ClientVersion()
+	if version != "" {
+		s.currentAPIVersion.val = version
+		return s.currentAPIVersion.val, nil
+	}
 
-	return s.currentAPIVersion.val, s.currentAPIVersion.err
+	// Defensive fallback for unexpected client implementations or mocks that
+	// do not populate ClientVersion after a successful negotiated ping.
+	val, err := s.RuntimeVersion(ctx)
+	if err != nil {
+		return "", err
+	}
+	s.currentAPIVersion.val = val
+	return s.currentAPIVersion.val, nil
 }

pkg/compose/convergence_test.go

@@ -521,3 +521,68 @@ func TestCurrentAPIVersionCachesNegotiation(t *testing.T) {
 	assert.NilError(t, err)
 	assert.Equal(t, version, "1.43")
 }
+
+func TestRuntimeVersionRetriesOnTransientError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient := mocks.NewMockAPIClient(mockCtrl)
+	cli := mocks.NewMockCli(mockCtrl)
+	tested := &composeService{dockerCli: cli}
+
+	cli.EXPECT().Client().Return(apiClient).AnyTimes()
+
+	// First call: ServerVersion fails with a transient error
+	firstCall := apiClient.EXPECT().ServerVersion(gomock.Any(), gomock.Any()).
+		Return(client.ServerVersionResult{}, context.DeadlineExceeded).Times(1)
+
+	// Second call: succeeds
+	apiClient.EXPECT().ServerVersion(gomock.Any(), gomock.Any()).
+		Return(client.ServerVersionResult{APIVersion: "1.48"}, nil).Times(1).After(firstCall)
+
+	_, err := tested.RuntimeVersion(t.Context())
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+
+	version, err := tested.RuntimeVersion(t.Context())
+	assert.NilError(t, err)
+	assert.Equal(t, version, "1.48")
+
+	// Third call returns cached value
+	version, err = tested.RuntimeVersion(t.Context())
+	assert.NilError(t, err)
+	assert.Equal(t, version, "1.48")
+}
+
+func TestCurrentAPIVersionRetriesOnTransientError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	apiClient := mocks.NewMockAPIClient(mockCtrl)
+	cli := mocks.NewMockCli(mockCtrl)
+	tested := &composeService{dockerCli: cli}
+
+	cli.EXPECT().Client().Return(apiClient).AnyTimes()
+
+	// First call: Ping fails with a transient error
+	firstCall := apiClient.EXPECT().Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}).
+		Return(client.PingResult{}, context.DeadlineExceeded).Times(1)
+
+	// Second call: Ping succeeds after the transient failure
+	apiClient.EXPECT().Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}).
+		Return(client.PingResult{APIVersion: "1.44"}, nil).Times(1).After(firstCall)
+	apiClient.EXPECT().ClientVersion().Return("1.44").Times(1)
+
+	// First call should return the transient error
+	_, err := tested.CurrentAPIVersion(t.Context())
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+
+	// Second call should succeed — error was not cached
+	version, err := tested.CurrentAPIVersion(t.Context())
+	assert.NilError(t, err)
+	assert.Equal(t, version, "1.44")
+
+	// Third call should return the cached value without calling Ping again
+	version, err = tested.CurrentAPIVersion(t.Context())
+	assert.NilError(t, err)
+	assert.Equal(t, version, "1.44")
+}