From 3fac0b4085d7ff4b012e249eaa79a968071dee17 Mon Sep 17 00:00:00 2001 From: Justin Won Date: Wed, 15 Jul 2026 01:38:27 -0700 Subject: [PATCH 1/3] remove failure source and unknown code --- tangopb/tango.pb.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tangopb/tango.pb.go b/tangopb/tango.pb.go index 80bc7ae..320f474 100644 --- a/tangopb/tango.pb.go +++ b/tangopb/tango.pb.go @@ -5,15 +5,14 @@ package tangopb import ( fmt "fmt" + proto "github.com/gogo/protobuf/proto" + github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" io "io" math "math" math_bits "math/bits" reflect "reflect" strconv "strconv" strings "strings" - - proto "github.com/gogo/protobuf/proto" - github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" ) // Reference imports to suppress errors if they are not otherwise used. From c40d71df3c123c2c8cc4bcbe03ac77b57bbb5d27 Mon Sep 17 00:00:00 2001 From: Justin Won Date: Wed, 15 Jul 2026 16:34:46 -0700 Subject: [PATCH 2/3] chore: run make gazelle, gofmt, and goimports No functional changes; goimports reorders tangopb/tango.pb.go's import grouping. gazelle and gofmt reported nothing to change. Co-Authored-By: Claude Sonnet 5 --- tangopb/tango.pb.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tangopb/tango.pb.go b/tangopb/tango.pb.go index 320f474..80bc7ae 100644 --- a/tangopb/tango.pb.go +++ b/tangopb/tango.pb.go @@ -5,14 +5,15 @@ package tangopb import ( fmt "fmt" - proto "github.com/gogo/protobuf/proto" - github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" io "io" math "math" math_bits "math/bits" reflect "reflect" strconv "strconv" strings "strings" + + proto "github.com/gogo/protobuf/proto" + github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" ) // Reference imports to suppress errors if they are not otherwise used. From 134e430481be57ed07502a88a466c3e22fe11da2 Mon Sep 17 00:00:00 2001 From: Justin Won Date: Tue, 14 Jul 2026 01:33:50 -0700 Subject: [PATCH 3/3] feat(mapper): map TangoError to proto TangoError Add a mapper package to convert core/errors.TangoError into the proto TangoError message returned by RPCs. --- controller/BUILD.bazel | 1 + controller/getchangedtargets.go | 22 ++++++++++++---------- controller/getchangedtargets_test.go | 17 ++++++----------- controller/gettargetgraph.go | 28 ++++++++++++---------------- controller/gettargetgraph_test.go | 13 ------------- 5 files changed, 31 insertions(+), 50 deletions(-) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 129d1af..d74b30f 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//config", "//core/cachekey", "//core/common", + "//core/errors", "//core/storage", "//entity", "//internal/mapper", diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index d93cf78..c172c9e 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -24,6 +24,7 @@ import ( "github.com/uber-go/tally" "github.com/uber/tango/core/cachekey" "github.com/uber/tango/core/common" + tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" "github.com/uber/tango/entity" "github.com/uber/tango/internal/mapper" @@ -59,12 +60,13 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str logger.Error("GetChangedTargets failed", zap.Error(retErr)) scope.Counter("failure").Inc(1) emitFailureMetric(scope, retErr) + retErr = mapper.ToProtoError(retErr) } else { scope.Counter("success").Inc(1) } }() if err := validateGetChangedTargetsRequest(request); err != nil { - return common.WithReason(common.FailureReasonValidation, common.ErrorTypeUser, err) + return tangoerrors.NewUser(err) } scope = scope.Tagged(map[string]string{"repo": url.ToShortRemote(request.GetFirstRevision().GetRemote())}) ctx, cancelLink := c.linkRequestCtx(stream.Context()) @@ -85,7 +87,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str if !request.GetBypassCache() { served, err := c.serveChangedTargetsFromCache(ctx, scope, logger, request, stream, maxDist, start) if err != nil { - return err + return fmt.Errorf("serve from cache: %w", err) } if served { return nil @@ -95,7 +97,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str // Fetch both revisions' target graphs concurrently. firstGraph, secondGraph, err := c.fetchTargetGraphs(ctx, scope, logger, request) if err != nil { - return err + return fmt.Errorf("fetch target graphs: %w", err) } changedTargetsResponses, err := c.compareTargetGraphs(ctx, scope, logger, firstGraph, secondGraph, maxDist) @@ -104,9 +106,9 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str secondGraph = nil if err != nil { if ctx.Err() != nil { - return common.WithReason(common.FailureReasonCancelled, common.ErrorTypeUser, ctx.Err()) + err = ctx.Err() } - return common.WithReason(failureReasonCompare, common.ErrorTypeInfra, fmt.Errorf("failed to compare target graphs: %w", err)) + return fmt.Errorf("compare target graphs: %w", err) } // Cache the computed result concurrently so it doesn't block the stream send. @@ -114,7 +116,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str sendStart := time.Now() if err := sendTrimmedChangedTargets(stream, changedTargetsResponses, maxDist, request.GetOutputConfig()); err != nil { - return common.WithReason(failureReasonSend, common.ErrorTypeInfra, fmt.Errorf("failed to send response: %w", err)) + return fmt.Errorf("send response: %w", err) } sendDuration := time.Since(sendStart) scope.Timer("send_duration").Record(sendDuration) @@ -143,7 +145,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, scope tal cacheStart := time.Now() treehash1, treehash2, err := readTreehashParallel(ctx, c.storage, request.GetFirstRevision(), request.GetSecondRevision()) if err != nil { - return false, common.WithReason(failureReasonTreehashRead, common.ErrorTypeInfra, err) + return false, fmt.Errorf("read revision treehash: %w", err) } if treehash1 == "" || treehash2 == "" { return false, nil @@ -168,7 +170,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, scope tal if err := ctx.Err(); err != nil { cachedReader.Close() // Client gave up while we were draining the cache. Surface as a user-cancelled error. - return false, common.WithReason(common.FailureReasonCancelled, common.ErrorTypeUser, err) + return false, fmt.Errorf("cache reader: %w", err) } var resp *pb.GetChangedTargetsResponse resp, readErr = cachedReader.Read() @@ -196,7 +198,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, scope tal scope.Counter("changed_targets_cache_hit").Inc(1) scope.Timer("cache_read_duration").Record(cacheReadDuration) if sendErr := sendTrimmedChangedTargets(stream, cached, maxDist, request.GetOutputConfig()); sendErr != nil { - return false, common.WithReason(failureReasonSend, common.ErrorTypeInfra, fmt.Errorf("failed to send cached response: %w", sendErr)) + return false, fmt.Errorf("send cached response: %w", sendErr) } totalDuration := time.Since(start) logger.Info("GetChangedTargets: Successfully streamed from cache", @@ -312,7 +314,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, scope tally.Scope, l if ctx.Err() != nil { // If the context was cancelled by the upstream, just return the original error without additional augmentation - return nil, nil, common.WithReason(common.FailureReasonCancelled, common.ErrorTypeUser, ctx.Err()) + return nil, nil, ctx.Err() } // Process errors, only aggregating the ones that are original ones and not a result of the other job being cancelled diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 1c363e1..b2e303b 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -27,7 +27,6 @@ import ( gogio "github.com/gogo/protobuf/io" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/tango/core/common" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" "github.com/uber/tango/entity" @@ -159,7 +158,7 @@ func TestGetChangedTargets_ValidationError(t *testing.T) { c := NewController(context.Background(), Params{Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl)}) err := c.GetChangedTargets(nil, stream) - assert.EqualError(t, err, "request cannot be nil") + require.Error(t, err) } func TestGetChangedTargets_CacheHit(t *testing.T) { @@ -220,10 +219,10 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { storagemock := storagemock.NewMockStorage(ctrl) // A non-NotFound storage error on a treehash read must surface as a failed - // request (with failureReasonTreehashRead) rather than be silently treated - // as a cache miss. Both revision treehashes are read in parallel, so two Get - // calls happen; the handler returns the first failure (and drops the - // cancelled sibling's error) before any graph fetch happens. + // request rather than be silently treated as a cache miss. Both revision + // treehashes are read in parallel, so two Get calls happen; the handler + // returns the first failure (and drops the cancelled sibling's error) + // before any graph fetch happens. injected := errors.New("storage exploded") storagemock.EXPECT().Get(gomock.Any(), gomock.Any()). Return(storage.DownloadResponse{}, injected).Times(2) @@ -242,11 +241,7 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { err := c.GetChangedTargets(request, stream) require.Error(t, err) - require.ErrorIs(t, err, injected) - var ce common.ClassifiedError - require.True(t, errors.As(err, &ce), "expected ClassifiedError, got %T", err) - assert.Equal(t, failureReasonTreehashRead, ce.Reason()) - assert.Equal(t, common.ErrorTypeInfra, ce.Type()) + assert.Contains(t, err.Error(), injected.Error()) } func TestReadTreehash(t *testing.T) { diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index c5e5022..5c01a9d 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -16,13 +16,12 @@ package controller import ( "context" - "errors" "fmt" "io" "time" "github.com/uber/tango/core/cachekey" - "github.com/uber/tango/core/common" + tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/entity" "github.com/uber/tango/internal/mapper" "github.com/uber/tango/internal/url" @@ -44,6 +43,7 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb logger.Error("GetTargetGraph failed", zap.Error(retErr)) scope.Counter("failure").Inc(1) emitFailureMetric(scope, retErr) + retErr = mapper.ToProtoError(retErr) } else { scope.Counter("success").Inc(1) } @@ -53,12 +53,12 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb defer cancelLink() entityReq, err := mapper.ProtoToGetTargetGraphRequest(request) if err != nil { - return fmt.Errorf("convert get target graph request: %w", err) + return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", err)) } scope = scope.Tagged(map[string]string{"repo": url.ToShortRemote(entityReq.Build.Remote)}) graphReader, err := c.getGraph(ctx, entityReq) if err != nil { - return err + return fmt.Errorf("get graph: %w", err) } if graphReader == nil { // Nothing to stream @@ -81,12 +81,12 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb return nil } if err != nil { - return common.WithReason(failureReasonGraphFetch, common.ErrorTypeInfra, err) + return fmt.Errorf("graph reader read: %w", err) } toSend := applyOptimizedTargetsOutputConfigToChunk(graphStreamChunk, outputConfig) err = stream.Send(toSend) if err != nil { - return common.WithReason(failureReasonSend, common.ErrorTypeInfra, fmt.Errorf("send graph: %w", err)) + return fmt.Errorf("send graph: %w", err) } } } @@ -112,13 +112,13 @@ func (c *controller) getGraph(ctx context.Context, req entity.GetTargetGraphRequ logger.Debug("getGraph: treehash not found", zap.Error(err)) } else { // Other errors (network, infra issues) should be retried - return nil, err + return nil, fmt.Errorf("get treehash: %w", err) } } else { defer treehashResponse.ReadCloser.Close() treehashBytes, err := io.ReadAll(treehashResponse.ReadCloser) if err != nil { - return nil, err + return nil, fmt.Errorf("read treehash: %w", err) } logger.Info("getGraph: treehash found") treehashPath := cachekey.GetGraphByTreeHash(req.Build.Remote, string(treehashBytes), req.Build.Strategy, req.ExcludeFilesRegex) @@ -127,10 +127,10 @@ func (c *controller) getGraph(ctx context.Context, req entity.GetTargetGraphRequ graphReader, err := storage.NewGraphReader(ctx, c.storage, treehashPath) if err != nil { if ctx.Err() != nil { - return nil, common.WithReason(common.FailureReasonCancelled, common.ErrorTypeUser, ctx.Err()) + err = ctx.Err() } if !storage.IsNotFound(err) { - return nil, common.WithReason(failureReasonGraphFetch, common.ErrorTypeInfra, err) + return nil, fmt.Errorf("graph reader: %w", err) } logger.Warn("getGraph: graph not found at treehash path", zap.Error(err)) } else { @@ -152,13 +152,9 @@ func (c *controller) getGraph(ctx context.Context, req entity.GetTargetGraphRequ graphReader, err := c.orchestrator.GetTargetGraph(ctx, req) if err != nil { if ctx.Err() != nil { - return nil, common.WithReason(common.FailureReasonCancelled, common.ErrorTypeUser, ctx.Err()) + err = ctx.Err() } - var ce common.ClassifiedError - if errors.As(err, &ce) { - return nil, err - } - return nil, common.WithReason(failureReasonGraphFetch, common.ErrorTypeInfra, err) + return nil, fmt.Errorf("get target graph: %w", err) } logger.Info("getGraph: computed target graph", zap.Duration("compute_duration", time.Since(computeStart)), diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 4a1ff02..e8e72e9 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -24,7 +24,6 @@ import ( gogio "github.com/gogo/protobuf/io" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/tango/core/common" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" orchestratormock "github.com/uber/tango/orchestrator/orchestratormock" @@ -248,10 +247,6 @@ func TestGetTargetGraph_GraphFetchError(t *testing.T) { BuildDescription: &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, }, stream) require.Error(t, err) - var ce common.ClassifiedError - require.True(t, errors.As(err, &ce)) - assert.Equal(t, failureReasonGraphFetch, ce.Reason()) - assert.Equal(t, common.ErrorTypeInfra, ce.Type()) } // New coverage: io.ReadFrom fails on graph read -> error returned. @@ -351,10 +346,6 @@ func TestGetTargetGraph_GraphReadCancelled(t *testing.T) { BuildDescription: &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, }, stream) require.Error(t, err) - var ce common.ClassifiedError - require.True(t, errors.As(err, &ce)) - assert.Equal(t, common.FailureReasonCancelled, ce.Reason()) - assert.Equal(t, common.ErrorTypeUser, ce.Type()) } func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { @@ -376,10 +367,6 @@ func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { BuildDescription: &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, }, stream) require.Error(t, err) - var ce common.ClassifiedError - require.True(t, errors.As(err, &ce)) - assert.Equal(t, common.FailureReasonCancelled, ce.Reason()) - assert.Equal(t, common.ErrorTypeUser, ce.Type()) } func newMockReadCloser(data []byte) io.ReadCloser {