From a97ceb098f1eb1cbc02587450bbfef800ca140dd Mon Sep 17 00:00:00 2001 From: Gustavo Castro Date: Thu, 2 Jul 2026 18:17:50 +0200 Subject: [PATCH] fix: don't kill in-flight sibling contexts when one context fails --- internal/apperr/apperr.go | 37 ++++++ internal/cli/root.go | 24 +++- internal/planner/apply.go | 7 +- internal/planner/destroy.go | 10 +- internal/planner/orchestrate.go | 121 ++++++++++++++++-- internal/planner/orchestrate_test.go | 184 +++++++++++++++++++++++++++ internal/planner/prune.go | 5 +- 7 files changed, 370 insertions(+), 18 deletions(-) diff --git a/internal/apperr/apperr.go b/internal/apperr/apperr.go index 237134d..324001f 100644 --- a/internal/apperr/apperr.go +++ b/internal/apperr/apperr.go @@ -118,6 +118,43 @@ func (c *ContextError) Unwrap() error { return c.Err } +// AbortedError marks a child result that did not fail on its own merits but +// was cut short because a sibling running under the same fail-fast +// orchestration failed first (e.g. its context was canceled, killing an +// in-flight subprocess). Callers such as printUserFriendly must render these +// distinctly from genuine failures and must not attach failure hints to them. +type AbortedError struct { + // ContextName is the context (e.g. Docker context/host) that was aborted. + ContextName string + // Err is the underlying error observed on the aborted child, if any + // (typically context.Canceled or a wrapped form of it). + Err error +} + +func (a *AbortedError) Error() string { + if a == nil { + return "aborted: another context failed" + } + if a.ContextName != "" { + return fmt.Sprintf("context %s: aborted: another context failed", a.ContextName) + } + return "aborted: another context failed" +} + +func (a *AbortedError) Unwrap() error { + if a == nil { + return nil + } + return a.Err +} + +// IsAborted reports whether err is (or wraps) an *AbortedError, i.e. a child +// that was cut short by a sibling's failure rather than failing on its own. +func IsAborted(err error) bool { + var aborted *AbortedError + return errors.As(err, &aborted) +} + // MultiError groups multiple errors and supports errors.Is/errors.As traversal. type MultiError struct { Errors []error diff --git a/internal/cli/root.go b/internal/cli/root.go index df4dc87..7043939 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -300,12 +300,32 @@ func printUserFriendly(err error) { // printMultiErrorDetail prints, for each child error in a MultiError, its // deepest underlying message (e.g. captured compose/docker stderr) along with // a per-failure actionable hint when one of the known patterns matches. +// +// Children that were only aborted because a sibling context failed first +// (apperr.AbortedError, e.g. a killed in-flight subprocess from fail-fast +// cancellation) are rendered distinctly as "aborted: another context failed" +// and never receive a failure hint, so they cannot be mistaken for genuine +// failures in the summary. func printMultiErrorDetail(multi *apperr.MultiError) { for _, child := range multi.Errors { - detail := apperr.DeepestMessage(child) + contextName := "" var ctxErr *apperr.ContextError if errors.As(child, &ctxErr) { - fmt.Fprintf(os.Stderr, "context %s: %s\n", ctxErr.ContextName, detail) + contextName = ctxErr.ContextName + } + + if apperr.IsAborted(child) { + if contextName != "" { + fmt.Fprintf(os.Stderr, "context %s: aborted: another context failed\n", contextName) + } else { + fmt.Fprintln(os.Stderr, "aborted: another context failed") + } + continue + } + + detail := apperr.DeepestMessage(child) + if contextName != "" { + fmt.Fprintf(os.Stderr, "context %s: %s\n", contextName, detail) } else { fmt.Fprintf(os.Stderr, "%s\n", detail) } diff --git a/internal/planner/apply.go b/internal/planner/apply.go index e60188f..08bdc6a 100644 --- a/internal/planner/apply.go +++ b/internal/planner/apply.go @@ -32,8 +32,11 @@ func (p *Planner) ApplyWithPlan(ctx context.Context, cfg manifest.Config, plan * "stacks", len(allStacks), "filesets", len(allFilesets)) - // Process each context (parallel by default, sequential with --sequential) - err := p.ExecuteAcrossContexts(ctx, &cfg, func(ctx context.Context, contextName string) error { + // Process each context (parallel by default, sequential with --sequential). + // Apply mutates state (compose up, volume/network creation), so contexts + // always run to completion: a failure on one host must never cancel an + // in-flight compose up on another, healthy host. + err := p.ExecuteAcrossContextsMode(ctx, &cfg, RunToCompletion, func(ctx context.Context, contextName string) error { contextConfig := cfg.Contexts[contextName] // Get Docker client for this context diff --git a/internal/planner/destroy.go b/internal/planner/destroy.go index 087e14a..80b5d60 100644 --- a/internal/planner/destroy.go +++ b/internal/planner/destroy.go @@ -77,7 +77,10 @@ func (p *Planner) BuildDestroyPlan(ctx context.Context, cfg manifest.Config) (*P var mu sync.Mutex - err := p.ExecuteAcrossContexts(ctx, &cfg, func(ctx context.Context, contextName string) error { + // BuildDestroyPlan only discovers/lists resources (no mutation), so it is + // safe to fail fast: canceling a sibling's in-flight discovery call loses + // nothing. + err := p.ExecuteAcrossContextsMode(ctx, &cfg, FailFast, func(ctx context.Context, contextName string) error { client := p.getClientForContext(contextName, &cfg) if client == nil { return apperr.New("planner.BuildDestroyPlan", apperr.Precondition, "docker client not available for context %s", contextName) @@ -208,7 +211,10 @@ func (p *Planner) DestroyWithOptions(ctx context.Context, cfg manifest.Config, o } scope := newDestroyScope(&cfg) - err := p.ExecuteAcrossContexts(ctx, &cfg, func(ctx context.Context, contextName string) error { + // Destroy mutates state (removes containers/networks/volumes), so contexts + // always run to completion: a failure on one host must never cancel + // in-flight teardown work on another host. + err := p.ExecuteAcrossContextsMode(ctx, &cfg, RunToCompletion, func(ctx context.Context, contextName string) error { client := p.getClientForContext(contextName, &cfg) if client == nil { return apperr.New("planner.Destroy", apperr.Precondition, "docker client not available for context %s", contextName) diff --git a/internal/planner/orchestrate.go b/internal/planner/orchestrate.go index 44a6f51..1823792 100644 --- a/internal/planner/orchestrate.go +++ b/internal/planner/orchestrate.go @@ -17,9 +17,33 @@ type ContextResult struct { Err error } +// ExecutionMode controls how executeParallel reacts to a child error. +type ExecutionMode int + +const ( + // FailFast cancels sibling goroutines as soon as one context errors. + // Appropriate for read-only/discovery operations where an in-flight + // docker command can be safely interrupted. Any sibling error caused by + // that cancellation is classified as aborted, not as a genuine failure. + FailFast ExecutionMode = iota + // RunToCompletion lets every context run to completion regardless of + // sibling errors. Required for mutating operations (apply/destroy/prune) + // where killing an in-flight `compose up`/`compose down` on an unrelated, + // healthy host is worse than waiting for it to finish. + RunToCompletion +) + // ExecuteAcrossContexts runs fn for each context, either in parallel or sequentially -// based on the planner's parallel flag. +// based on the planner's parallel flag. Mutating operations (apply/destroy/prune) +// must run in RunToCompletion mode so a failure on one context never kills an +// in-flight mutation on another; read-only/discovery operations may use FailFast. func (p *Planner) ExecuteAcrossContexts(ctx context.Context, cfg *manifest.Config, fn func(ctx context.Context, contextName string) error) error { + return p.ExecuteAcrossContextsMode(ctx, cfg, RunToCompletion, fn) +} + +// ExecuteAcrossContextsMode is like ExecuteAcrossContexts but lets the caller +// pick the orchestration mode explicitly. +func (p *Planner) ExecuteAcrossContextsMode(ctx context.Context, cfg *manifest.Config, mode ExecutionMode, fn func(ctx context.Context, contextName string) error) error { contextNames := sortedKeys(cfg.Contexts) if len(contextNames) == 0 { return nil @@ -28,7 +52,7 @@ func (p *Planner) ExecuteAcrossContexts(ctx context.Context, cfg *manifest.Confi if !p.parallel || len(contextNames) == 1 { return executeSequential(ctx, contextNames, fn) } - return executeParallel(ctx, contextNames, fn) + return executeParallel(ctx, contextNames, mode, fn) } func executeSequential(ctx context.Context, contextNames []string, fn func(ctx context.Context, contextName string) error) error { @@ -43,23 +67,38 @@ func executeSequential(ctx context.Context, contextNames []string, fn func(ctx c return nil } -func executeParallel(ctx context.Context, contextNames []string, fn func(ctx context.Context, contextName string) error) error { - ctx, cancel := context.WithCancel(ctx) - defer cancel() +func executeParallel(parentCtx context.Context, contextNames []string, mode ExecutionMode, fn func(ctx context.Context, contextName string) error) error { + runCtx := parentCtx + var cancel context.CancelFunc + if mode == FailFast { + runCtx, cancel = context.WithCancel(parentCtx) + defer cancel() + } var mu sync.Mutex var errs []ContextResult var wg sync.WaitGroup + // firstFailure records which context triggered the cancellation, so that + // exactly that one is never classified as aborted even though, by the + // time we get around to sorting/classifying, runCtx.Err() is already set. + firstFailure := "" + cancelled := false for _, name := range contextNames { wg.Add(1) go func(contextName string) { defer wg.Done() - if err := fn(ctx, contextName); err != nil { + if err := fn(runCtx, contextName); err != nil { mu.Lock() errs = append(errs, ContextResult{ContextName: contextName, Err: err}) + if mode == FailFast && !cancelled { + cancelled = true + firstFailure = contextName + } mu.Unlock() - cancel() // signal other goroutines to stop + if mode == FailFast && cancel != nil { + cancel() // signal other goroutines to stop + } } }(name) } @@ -69,17 +108,45 @@ func executeParallel(ctx context.Context, contextNames []string, fn func(ctx con if len(errs) == 0 { return nil } + + // Sort errors by context name for deterministic output + sort.Slice(errs, func(i, j int) bool { return errs[i].ContextName < errs[j].ContextName }) + + if mode == FailFast { + // Only classify children as aborted when the cancellation was OURS: + // the parent context is still alive while the run context is + // canceled, which can only mean our fail-fast cancel() fired in + // response to firstFailure. If the parent itself was canceled (user + // Ctrl-C, upstream deadline), no sibling caused anything — leave all + // errors as-is; upstream already renders user-canceled runs. + if parentCtx.Err() == nil && runCtx.Err() != nil { + errs = classifyAborted(errs, firstFailure) + } + } + if len(errs) == 1 { return errs[0].Err } - // Sort errors by context name for deterministic output - sort.Slice(errs, func(i, j int) bool { return errs[i].ContextName < errs[j].ContextName }) + // Build the summary message so aborted contexts (cut short only because a + // sibling failed first) are never worded as failures alongside the + // genuine one(s) — design requirement: the aggregate must clearly + // distinguish the two and never present an aborted context as failed. var msgs []string wrapped := make([]error, 0, len(errs)) + failureCount := 0 for _, r := range errs { - msgs = append(msgs, fmt.Sprintf("context %s: %v", r.ContextName, r.Err)) wrapped = append(wrapped, &apperr.ContextError{ContextName: r.ContextName, Err: r.Err}) + if apperr.IsAborted(r.Err) { + msgs = append(msgs, fmt.Sprintf("context %s: aborted: another context failed", r.ContextName)) + continue + } + failureCount++ + msgs = append(msgs, fmt.Sprintf("context %s: %v", r.ContextName, r.Err)) + } + summary := fmt.Sprintf("%d context(s) failed", failureCount) + if failureCount != len(errs) { + summary = fmt.Sprintf("%s (%d aborted after a sibling failure)", summary, len(errs)-failureCount) } // Preserve each child's underlying cause (rather than pre-stringifying with // %v) so the deepest error detail, e.g. captured compose stderr, survives @@ -88,6 +155,38 @@ func executeParallel(ctx context.Context, contextNames []string, fn func(ctx con Op: "planner.ExecuteAcrossContexts", Kind: apperr.External, Err: &apperr.MultiError{Errors: wrapped}, - Msg: fmt.Sprintf("multiple context errors:\n %s", strings.Join(msgs, "\n ")), + Msg: fmt.Sprintf("%s:\n %s", summary, strings.Join(msgs, "\n ")), } } + +// classifyAborted rewrites child errors that only failed because our +// fail-fast cancel() fired in response to firstFailure's genuine failure. +// It must only be called when that cancellation is known to be ours (the +// caller checks that the parent context is still alive while the run +// context is canceled) — otherwise a user Ctrl-C or an upstream deadline +// would mislabel every child as "aborted: another context failed" when no +// sibling failed at all. +// +// Under that gate every non-first child error is cancellation-induced by +// construction, regardless of its shape: some chain context.Canceled (a +// docker call that observes ctx.Done() before starting its own work), while +// others are opaque — exec.CommandContext SIGKILLs an in-flight `docker +// compose` subprocess and the resulting "signal: killed" *exec.ExitError +// carries no context.Canceled anywhere in its chain, so errors.Is matching +// alone could never recognize it. +// +// firstFailure is the context name that actually triggered cancel(); it is +// never classified as aborted — even when its error happens to chain +// context.Canceled — because it is the genuine failure by construction. +func classifyAborted(errs []ContextResult, firstFailure string) []ContextResult { + out := make([]ContextResult, 0, len(errs)) + for _, r := range errs { + if r.ContextName == firstFailure { + out = append(out, r) + continue + } + out = append(out, ContextResult{ContextName: r.ContextName, Err: &apperr.AbortedError{ContextName: r.ContextName, Err: r.Err}}) + } + return out +} + diff --git a/internal/planner/orchestrate_test.go b/internal/planner/orchestrate_test.go index b9e74fe..b5d1140 100644 --- a/internal/planner/orchestrate_test.go +++ b/internal/planner/orchestrate_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "strings" + "sync" "sync/atomic" "testing" "time" @@ -178,6 +180,188 @@ func TestExecuteAcrossContexts_ContextCancellation(t *testing.T) { } } +// TestExecuteAcrossContextsMode_RunToCompletion_NoSiblingCancellation verifies +// the mutating-mode contract: when one context fails, every other context +// still runs to completion (its work is not interrupted), and a sibling that +// completes successfully does not appear anywhere in the error aggregate. +func TestExecuteAcrossContextsMode_RunToCompletion_NoSiblingCancellation(t *testing.T) { + p := New().WithParallel(true) + cfg := twoContextConfig() + + release := make(chan struct{}) + var betaCompleted atomic.Bool + var betaSawCancel atomic.Bool + + err := p.ExecuteAcrossContextsMode(context.Background(), cfg, RunToCompletion, func(ctx context.Context, name string) error { + if name == "alpha" { + // Fail immediately. + return fmt.Errorf("bad image tag on alpha") + } + // beta: simulate a long-running docker compose up that must not be + // killed just because alpha failed. + select { + case <-ctx.Done(): + betaSawCancel.Store(true) + case <-time.After(100 * time.Millisecond): + } + close(release) + betaCompleted.Store(true) + return nil + }) + + <-release + + if !betaCompleted.Load() { + t.Fatal("expected beta to run to completion despite alpha's failure") + } + if betaSawCancel.Load() { + t.Fatal("expected beta's context to never be canceled in RunToCompletion mode") + } + + if err == nil { + t.Fatal("expected an error since alpha failed") + } + var e *apperr.E + if errors.As(err, &e) { + var multi *apperr.MultiError + if errors.As(e.Err, &multi) { + t.Fatalf("expected a single genuine failure (alpha only), got MultiError with %d entries", len(multi.Errors)) + } + } + if !strings.Contains(err.Error(), "alpha") { + t.Fatalf("expected error to reference alpha, got: %v", err) + } + if strings.Contains(err.Error(), "beta") { + t.Fatalf("beta succeeded and must not appear in the error aggregate, got: %v", err) + } +} + +// TestExecuteAcrossContextsMode_FailFast_DistinguishesAbortedFromGenuine +// covers the read-only/discovery fail-fast path: one context fails for a +// real reason, a sibling is canceled as a side effect and returns an opaque +// "killed" style error (mirroring exec.CommandContext's SIGKILL behavior, +// which carries no context.Canceled in its chain). The aggregate must +// clearly distinguish the two: exactly one genuine failure, and the +// cancellation-induced error must not be presented as a failure. +func TestExecuteAcrossContextsMode_FailFast_DistinguishesAbortedFromGenuine(t *testing.T) { + p := New().WithParallel(true) + cfg := twoContextConfig() + + err := p.ExecuteAcrossContextsMode(context.Background(), cfg, FailFast, func(ctx context.Context, name string) error { + if name == "alpha" { + return apperr.New("planner.Discover", apperr.External, "bad image tag") + } + // beta: block until canceled, then return an opaque error with no + // context.Canceled in its chain, mirroring "signal: killed" from a + // SIGKILLed docker compose subprocess. + <-ctx.Done() + return errors.New("signal: killed") + }) + + if err == nil { + t.Fatal("expected an aggregate error") + } + + var e *apperr.E + if !errors.As(err, &e) { + t.Fatalf("expected *apperr.E, got %T", err) + } + var multi *apperr.MultiError + if !errors.As(e.Err, &multi) { + t.Fatalf("expected *apperr.MultiError, got %T", e.Err) + } + if len(multi.Errors) != 2 { + t.Fatalf("expected 2 child entries (1 genuine + 1 aborted), got %d", len(multi.Errors)) + } + + genuineCount := 0 + abortedCount := 0 + for _, child := range multi.Errors { + var ctxErr *apperr.ContextError + if !errors.As(child, &ctxErr) { + t.Fatalf("expected *apperr.ContextError, got %T", child) + } + if apperr.IsAborted(child) { + abortedCount++ + if ctxErr.ContextName != "beta" { + t.Fatalf("expected beta to be the aborted context, got %s", ctxErr.ContextName) + } + } else { + genuineCount++ + if ctxErr.ContextName != "alpha" { + t.Fatalf("expected alpha to be the genuine failure, got %s", ctxErr.ContextName) + } + } + } + if genuineCount != 1 { + t.Fatalf("expected exactly 1 genuine failure, got %d", genuineCount) + } + if abortedCount != 1 { + t.Fatalf("expected exactly 1 aborted context, got %d", abortedCount) + } + + // The top-level summary message must not word the aborted context as a + // failure alongside the genuine one. + if !strings.Contains(e.Msg, "1 context(s) failed") { + t.Fatalf("expected summary to report exactly 1 failed context, got: %s", e.Msg) + } + if !strings.Contains(e.Msg, "aborted") { + t.Fatalf("expected summary to call out the aborted context, got: %s", e.Msg) + } +} + +// TestExecuteAcrossContextsMode_FailFast_ParentCancelNotAborted verifies +// that cancellation arriving via the PARENT context (user Ctrl-C, upstream +// deadline) is never misattributed to a sibling failure: when the parent is +// canceled while both children are in flight and both return +// context.Canceled-chained errors, no child may be wrapped in AbortedError +// and the aggregate must not claim a sibling failed. +func TestExecuteAcrossContextsMode_FailFast_ParentCancelNotAborted(t *testing.T) { + p := New().WithParallel(true) + cfg := twoContextConfig() + + parentCtx, cancelParent := context.WithCancel(context.Background()) + defer cancelParent() + + var inFlight sync.WaitGroup + inFlight.Add(2) + go func() { + // Cancel the parent only once both children are in flight. + inFlight.Wait() + cancelParent() + }() + + err := p.ExecuteAcrossContextsMode(parentCtx, cfg, FailFast, func(ctx context.Context, name string) error { + inFlight.Done() + <-ctx.Done() + return fmt.Errorf("docker compose config for %s: %w", name, context.Canceled) + }) + + if err == nil { + t.Fatal("expected an error since both children were canceled") + } + if apperr.IsAborted(err) { + t.Fatalf("no child may be classified as aborted when the parent was canceled, got: %v", err) + } + var e *apperr.E + if errors.As(err, &e) { + var multi *apperr.MultiError + if errors.As(e.Err, &multi) { + for _, child := range multi.Errors { + if apperr.IsAborted(child) { + t.Fatalf("child wrongly classified as aborted on parent cancellation: %v", child) + } + if !errors.Is(child, context.Canceled) { + t.Fatalf("expected child to preserve context.Canceled chain, got: %v", child) + } + } + } + } + if strings.Contains(err.Error(), "aborted") { + t.Fatalf("aggregate must not mention sibling-induced aborts on parent cancellation, got: %v", err) + } +} + func TestExecuteAcrossContexts_SingleContextRunsSequential(t *testing.T) { p := New().WithParallel(true) // parallel enabled but only 1 context cfg := &manifest.Config{ diff --git a/internal/planner/prune.go b/internal/planner/prune.go index f7a4af7..3ed3553 100644 --- a/internal/planner/prune.go +++ b/internal/planner/prune.go @@ -30,7 +30,10 @@ func (p *Planner) PruneWithPlanOptions(ctx context.Context, cfg manifest.Config, return apperr.New("planner.Prune", apperr.Precondition, "docker client not configured") } - err := p.ExecuteAcrossContexts(ctx, &cfg, func(ctx context.Context, contextName string) error { + // Prune mutates state (removes containers/networks/volumes), so contexts + // always run to completion: a failure on one host must never cancel + // in-flight cleanup work on another host. + err := p.ExecuteAcrossContextsMode(ctx, &cfg, RunToCompletion, func(ctx context.Context, contextName string) error { client := p.getClientForContext(contextName, &cfg) if client == nil { return apperr.New("planner.Prune", apperr.Precondition, "docker client not available for context %s", contextName)