From dfb70388e607acd346c460913a96c50161406e1f Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Wed, 1 Jul 2026 13:11:22 +0800 Subject: [PATCH 1/3] fix: WrapContextAsUserError should not misclassify internal timeouts as user cancellations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, WrapContextAsUserError used errors.Is(err, context.Canceled) to detect user cancellations. This traverses the entire error chain, matching context.Canceled from internal child-context cancellations (e.g., WaitForEnvd 60s timeout) — not just user-initiated cancellations. The fix changes WrapContextAsUserError to accept the build-level context and check buildCtx.Err() directly. Only when the build context itself is canceled or timed out will the error be classified as a user error. Internal timeouts now propagate their real error messages instead of being replaced with the misleading 'build was cancelled'. Fixes #3154 --- .../pkg/template/build/builder.go | 2 +- .../pkg/template/build/builderrors/errors.go | 21 ++-- .../template/build/builderrors/errors_test.go | 105 ++++++++++++++++++ 3 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 packages/orchestrator/pkg/template/build/builderrors/errors_test.go diff --git a/packages/orchestrator/pkg/template/build/builder.go b/packages/orchestrator/pkg/template/build/builder.go index 1e2a07f473..534a6ce65d 100644 --- a/packages/orchestrator/pkg/template/build/builder.go +++ b/packages/orchestrator/pkg/template/build/builder.go @@ -190,7 +190,7 @@ func (b *Builder) Build(ctx context.Context, paths storage.Paths, cfg config.Tem e = errors.Join(e, ctx.Err()) } - e = builderrors.WrapContextAsUserError(e) + e = builderrors.WrapContextAsUserError(ctx, e) if e != nil { childSpan.RecordError(e, trace.WithAttributes( telemetry.WithTemplateID(cfg.TemplateID), diff --git a/packages/orchestrator/pkg/template/build/builderrors/errors.go b/packages/orchestrator/pkg/template/build/builderrors/errors.go index 5a9852fa5d..518275ad63 100644 --- a/packages/orchestrator/pkg/template/build/builderrors/errors.go +++ b/packages/orchestrator/pkg/template/build/builderrors/errors.go @@ -26,9 +26,10 @@ func IsUserError(err error) bool { return phases.UnwrapPhaseBuildError(err) != nil } -// WrapContextAsUserError wraps context.Canceled as a user error if no user error already exists. -// This ensures that user-initiated cancellations are tracked as user errors in metrics. -func WrapContextAsUserError(err error) error { +// WrapContextAsUserError wraps context errors as user errors only when the build-level +// context itself was canceled or timed out. This prevents internal child-context +// cancellations (e.g., envd init timeout) from being misclassified as user cancellations. +func WrapContextAsUserError(buildCtx context.Context, err error) error { if err == nil { return nil } @@ -38,13 +39,19 @@ func WrapContextAsUserError(err error) error { return err } - // If it's a canceled context, wrap it as a user error - if errors.Is(err, context.Canceled) { + // Only classify as user cancellation/timeout if the build-level context was actually done. + // Internal child-context cancellations should not be treated as user errors. + if buildCtx.Err() == nil { + return err + } + + // If the build context was canceled, wrap it as a user cancellation + if buildCtx.Err() == context.Canceled { return phases.NewPhaseBuildError(phases.PhaseMeta{}, ErrCanceled) } - // If it's a timeout context, wrap it as a user error - if errors.Is(err, context.DeadlineExceeded) { + // If the build context deadline was exceeded, wrap it as a user timeout + if buildCtx.Err() == context.DeadlineExceeded { return phases.NewPhaseBuildError(phases.PhaseMeta{}, ErrTimeout) } diff --git a/packages/orchestrator/pkg/template/build/builderrors/errors_test.go b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go new file mode 100644 index 0000000000..ca641c5d3b --- /dev/null +++ b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go @@ -0,0 +1,105 @@ +//go:build linux + +package builderrors + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/e2b-dev/infra/packages/orchestrator/pkg/template/build/phases" +) + +func TestWrapContextAsUserError_NilError(t *testing.T) { + ctx := context.Background() + if got := WrapContextAsUserError(ctx, nil); got != nil { + t.Errorf("expected nil, got %v", got) + } +} + +func TestWrapContextAsUserError_AlreadyUserError(t *testing.T) { + ctx := context.Background() + userErr := phases.NewPhaseBuildError(phases.PhaseMeta{}, errors.New("user mistake")) + got := WrapContextAsUserError(ctx, userErr) + if got != userErr { + t.Errorf("expected original user error returned as-is, got %v", got) + } +} + +func TestWrapContextAsUserError_InternalTimeout_NotMisclassified(t *testing.T) { + // Simulate: build context is still active, but error contains context.Canceled + // from an internal child-context timeout (e.g., WaitForEnvd). + buildCtx := context.Background() // not canceled + + // This is what doRequestWithInfiniteRetries returns when a child context is canceled + internalErr := fmt.Errorf("%w with cause: %w", context.Canceled, errors.New("syncing took too long")) + + got := WrapContextAsUserError(buildCtx, internalErr) + + // Should NOT be classified as user error — the build context is still alive + if IsUserError(got) { + t.Errorf("internal timeout should not be classified as user error, got: %v", got) + } + // Should preserve the original error + if got != internalErr { + t.Errorf("expected original error preserved, got: %v", got) + } +} + +func TestWrapContextAsUserError_UserCancellation(t *testing.T) { + // Simulate: user cancels the build → build context is canceled + buildCtx, cancel := context.WithCancel(context.Background()) + cancel() // user canceled + + someErr := errors.New("some operation failed") + got := WrapContextAsUserError(buildCtx, someErr) + + if !IsUserError(got) { + t.Errorf("expected user error when build context is canceled, got: %v", got) + } + if !errors.Is(got, ErrCanceled) { + t.Errorf("expected ErrCanceled, got: %v", got) + } +} + +func TestWrapContextAsUserError_BuildDeadlineExceeded(t *testing.T) { + // Simulate: build context deadline exceeded + buildCtx, cancel := context.WithTimeout(context.Background(), 0) + defer cancel() + <-buildCtx.Done() // ensure it's expired + + someErr := errors.New("some operation failed") + got := WrapContextAsUserError(buildCtx, someErr) + + if !IsUserError(got) { + t.Errorf("expected user error when build context timed out, got: %v", got) + } + if !errors.Is(got, ErrTimeout) { + t.Errorf("expected ErrTimeout, got: %v", got) + } +} + +func TestWrapContextAsUserError_InternalCanceledError_BuildContextAlive(t *testing.T) { + // The key regression test: error chain contains context.Canceled from a child + // context, but the build context is NOT canceled. Must NOT be treated as user error. + buildCtx := context.Background() + + // Simulate WaitForEnvd timeout chain: + // child context canceled → doRequestWithInfiniteRetries wraps ctx.Err() + childCtx, childCancel := context.WithCancelCause(buildCtx) + childCancel(errors.New("syncing took too long")) + + err := fmt.Errorf("failed to init new envd: %w", + fmt.Errorf("%w with cause: %w", childCtx.Err(), context.Cause(childCtx))) + + got := WrapContextAsUserError(buildCtx, err) + + if IsUserError(got) { + t.Errorf("internal child-context cancellation must not be classified as user error, got: %v", got) + } + // The original error should be preserved + if !errors.Is(got, context.Canceled) { + t.Errorf("original error chain should be preserved, got: %v", got) + } +} From 0293212023f70b454403132aaa7b3e8c3509edd0 Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Wed, 1 Jul 2026 13:34:23 +0800 Subject: [PATCH 2/3] fix: add double check for context error in WrapContextAsUserError Address review feedback: check both buildCtx.Err() AND errors.Is(err, ...) to avoid discarding unrelated errors (e.g., disk I/O errors) that happen to coincide with build context cancellation. Added test case for build context canceled with unrelated error. --- .../pkg/template/build/builderrors/errors.go | 11 +++++---- .../template/build/builderrors/errors_test.go | 23 +++++++++++++++++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/packages/orchestrator/pkg/template/build/builderrors/errors.go b/packages/orchestrator/pkg/template/build/builderrors/errors.go index 518275ad63..0429a5aa4f 100644 --- a/packages/orchestrator/pkg/template/build/builderrors/errors.go +++ b/packages/orchestrator/pkg/template/build/builderrors/errors.go @@ -45,13 +45,16 @@ func WrapContextAsUserError(buildCtx context.Context, err error) error { return err } - // If the build context was canceled, wrap it as a user cancellation - if buildCtx.Err() == context.Canceled { + // If the build context was canceled AND the error chain contains context.Canceled, + // wrap it as a user cancellation. The double check ensures we don't discard + // unrelated errors (e.g., disk errors) that happen to coincide with context cancellation. + if buildCtx.Err() == context.Canceled && errors.Is(err, context.Canceled) { return phases.NewPhaseBuildError(phases.PhaseMeta{}, ErrCanceled) } - // If the build context deadline was exceeded, wrap it as a user timeout - if buildCtx.Err() == context.DeadlineExceeded { + // If the build context deadline was exceeded AND the error chain contains + // context.DeadlineExceeded, wrap it as a user timeout. + if buildCtx.Err() == context.DeadlineExceeded && errors.Is(err, context.DeadlineExceeded) { return phases.NewPhaseBuildError(phases.PhaseMeta{}, ErrTimeout) } diff --git a/packages/orchestrator/pkg/template/build/builderrors/errors_test.go b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go index ca641c5d3b..7dff17191e 100644 --- a/packages/orchestrator/pkg/template/build/builderrors/errors_test.go +++ b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go @@ -52,7 +52,8 @@ func TestWrapContextAsUserError_UserCancellation(t *testing.T) { buildCtx, cancel := context.WithCancel(context.Background()) cancel() // user canceled - someErr := errors.New("some operation failed") + // Error must contain context.Canceled (as it would in real code via errors.Join or wrapping) + someErr := fmt.Errorf("some operation failed: %w", buildCtx.Err()) got := WrapContextAsUserError(buildCtx, someErr) if !IsUserError(got) { @@ -63,13 +64,31 @@ func TestWrapContextAsUserError_UserCancellation(t *testing.T) { } } +func TestWrapContextAsUserError_BuildContextCanceled_ErrorUnrelated(t *testing.T) { + // Build context is canceled, but the error is unrelated (e.g., disk error). + // Should NOT be wrapped as user error. + buildCtx, cancel := context.WithCancel(context.Background()) + cancel() + + diskErr := errors.New("disk I/O error") + got := WrapContextAsUserError(buildCtx, diskErr) + + if IsUserError(got) { + t.Errorf("unrelated error should not be classified as user error when build context is canceled, got: %v", got) + } + if got != diskErr { + t.Errorf("expected original error preserved, got: %v", got) + } +} + func TestWrapContextAsUserError_BuildDeadlineExceeded(t *testing.T) { // Simulate: build context deadline exceeded buildCtx, cancel := context.WithTimeout(context.Background(), 0) defer cancel() <-buildCtx.Done() // ensure it's expired - someErr := errors.New("some operation failed") + // Error must contain context.DeadlineExceeded (as it would in real code) + someErr := fmt.Errorf("some operation failed: %w", buildCtx.Err()) got := WrapContextAsUserError(buildCtx, someErr) if !IsUserError(got) { From ff2479266ba29d41532fd1c7fd085d99d0538429 Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Wed, 1 Jul 2026 15:10:45 +0800 Subject: [PATCH 3/3] fix: address golangci-lint errors in tests - Add t.Parallel() to all test functions (paralleltest) - Replace != error comparisons with errors.Is (errorlint) --- .../template/build/builderrors/errors_test.go | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/orchestrator/pkg/template/build/builderrors/errors_test.go b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go index 7dff17191e..f96270de2d 100644 --- a/packages/orchestrator/pkg/template/build/builderrors/errors_test.go +++ b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go @@ -12,6 +12,8 @@ import ( ) func TestWrapContextAsUserError_NilError(t *testing.T) { + t.Parallel() + ctx := context.Background() if got := WrapContextAsUserError(ctx, nil); got != nil { t.Errorf("expected nil, got %v", got) @@ -19,15 +21,19 @@ func TestWrapContextAsUserError_NilError(t *testing.T) { } func TestWrapContextAsUserError_AlreadyUserError(t *testing.T) { + t.Parallel() + ctx := context.Background() userErr := phases.NewPhaseBuildError(phases.PhaseMeta{}, errors.New("user mistake")) got := WrapContextAsUserError(ctx, userErr) - if got != userErr { + if !errors.Is(got, userErr) { t.Errorf("expected original user error returned as-is, got %v", got) } } func TestWrapContextAsUserError_InternalTimeout_NotMisclassified(t *testing.T) { + t.Parallel() + // Simulate: build context is still active, but error contains context.Canceled // from an internal child-context timeout (e.g., WaitForEnvd). buildCtx := context.Background() // not canceled @@ -42,12 +48,14 @@ func TestWrapContextAsUserError_InternalTimeout_NotMisclassified(t *testing.T) { t.Errorf("internal timeout should not be classified as user error, got: %v", got) } // Should preserve the original error - if got != internalErr { + if !errors.Is(got, internalErr) { t.Errorf("expected original error preserved, got: %v", got) } } func TestWrapContextAsUserError_UserCancellation(t *testing.T) { + t.Parallel() + // Simulate: user cancels the build → build context is canceled buildCtx, cancel := context.WithCancel(context.Background()) cancel() // user canceled @@ -65,6 +73,8 @@ func TestWrapContextAsUserError_UserCancellation(t *testing.T) { } func TestWrapContextAsUserError_BuildContextCanceled_ErrorUnrelated(t *testing.T) { + t.Parallel() + // Build context is canceled, but the error is unrelated (e.g., disk error). // Should NOT be wrapped as user error. buildCtx, cancel := context.WithCancel(context.Background()) @@ -76,12 +86,14 @@ func TestWrapContextAsUserError_BuildContextCanceled_ErrorUnrelated(t *testing.T if IsUserError(got) { t.Errorf("unrelated error should not be classified as user error when build context is canceled, got: %v", got) } - if got != diskErr { + if !errors.Is(got, diskErr) { t.Errorf("expected original error preserved, got: %v", got) } } func TestWrapContextAsUserError_BuildDeadlineExceeded(t *testing.T) { + t.Parallel() + // Simulate: build context deadline exceeded buildCtx, cancel := context.WithTimeout(context.Background(), 0) defer cancel() @@ -100,6 +112,8 @@ func TestWrapContextAsUserError_BuildDeadlineExceeded(t *testing.T) { } func TestWrapContextAsUserError_InternalCanceledError_BuildContextAlive(t *testing.T) { + t.Parallel() + // The key regression test: error chain contains context.Canceled from a child // context, but the build context is NOT canceled. Must NOT be treated as user error. buildCtx := context.Background()