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..0429a5aa4f 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,22 @@ 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 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 it's a timeout context, wrap it as a user error - if errors.Is(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 new file mode 100644 index 0000000000..f96270de2d --- /dev/null +++ b/packages/orchestrator/pkg/template/build/builderrors/errors_test.go @@ -0,0 +1,138 @@ +//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) { + t.Parallel() + + ctx := context.Background() + if got := WrapContextAsUserError(ctx, nil); got != nil { + t.Errorf("expected nil, got %v", got) + } +} + +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 !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 + + // 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 !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 + + // 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) { + 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_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()) + 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 !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() + <-buildCtx.Done() // ensure it's expired + + // 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) { + 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) { + 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() + + // 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) + } +}