diff --git a/internal/controller/syncmanager/controller.go b/internal/controller/syncmanager/controller.go index 458f84e..32c326c 100644 --- a/internal/controller/syncmanager/controller.go +++ b/internal/controller/syncmanager/controller.go @@ -197,19 +197,34 @@ func (r *Reconciler) ensureSyncController(ctx context.Context, log *zap.SugaredL return reconcile.Result{}, fmt.Errorf("failed to create sync controller: %w", err) } - r.syncCancels[key] = ctrlCancel - // time to start the controller; this will spawn a new goroutine if // successful; the new controller will be pre-seeded with all knonwn // (engaged) clusters by the DMCM. - if err = r.dmcm.StartController(ctrlCtx, log.With("prkey", key), syncController); err != nil { - ctrlCancel(errors.New("failed to start sync controller")) - return reconcile.Result{}, fmt.Errorf("failed to start sync controller: %w", err) + err = r.startAndRecord(key, ctrlCancel, func() error { + return r.dmcm.StartController(ctrlCtx, log.With("prkey", key), syncController) + }) + if err != nil { + return reconcile.Result{}, err } return reconcile.Result{}, nil } +// startAndRecord starts a controller and only remembers its cancel function if the start +// succeeded. Otherwise a failed start would block all later retries. +func (r *Reconciler) startAndRecord(key string, cancel context.CancelCauseFunc, start func() error) error { + if err := start(); err != nil { + cancel(errors.New("failed to start sync controller")) + return fmt.Errorf("failed to start sync controller: %w", err) + } + + r.syncCancelsLock.Lock() + r.syncCancels[key] = cancel + r.syncCancelsLock.Unlock() + + return nil +} + func (r *Reconciler) cleanupController(log *zap.SugaredLogger, pubRes *syncagentv1alpha1.PublishedResource) error { key := getPublishedResourceKey(pubRes) log.Infow("Stopping sync controller…", "prkey", key) diff --git a/internal/controller/syncmanager/controller_test.go b/internal/controller/syncmanager/controller_test.go new file mode 100644 index 0000000..e46ba90 --- /dev/null +++ b/internal/controller/syncmanager/controller_test.go @@ -0,0 +1,52 @@ +/* +Copyright 2025 The KCP Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package syncmanager + +import ( + "context" + "errors" + "testing" +) + +func TestStartAndRecord(t *testing.T) { + r := &Reconciler{syncCancels: map[string]context.CancelCauseFunc{}} + + ctx, cancel := context.WithCancelCause(context.Background()) + + if err := r.startAndRecord("key", cancel, func() error { return errors.New("boom") }); err == nil { + t.Fatal("expected an error") + } + + if _, exists := r.syncCancels["key"]; exists { + t.Error("cancel function must not be recorded after a failed start") + } + + if ctx.Err() == nil { + t.Error("context should be cancelled after a failed start") + } + + // a retry must be able to succeed + _, cancel = context.WithCancelCause(context.Background()) + + if err := r.startAndRecord("key", cancel, func() error { return nil }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if _, exists := r.syncCancels["key"]; !exists { + t.Error("cancel function should be recorded after a successful start") + } +}