From 1a81f38ee3063d7589b72770f625a6f8aea609c8 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 7 Oct 2026 11:06:29 -0400 Subject: [PATCH 1/5] refactor(controller): retire gob fallback for TGB-configured repos TGB-configured repositories now read only the TGB blob from the cache. The gob fallback path is removed: a corrupt or missing TGB blob propagates the error directly so the orchestrator recomputes and overwrites, rather than serving a stale gob blob from an earlier format era. This mirrors the matching internal monorepo change. Production has been fully on TGB long enough that the gob fallback was dead code obscuring real cache state. Co-Authored-By: Claude Opus 4.6 (1M context) --- controller/gettargetgraph.go | 20 ++++------- controller/gettargetgraph_test.go | 58 +++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 82e61790..6b6b0c2e 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -167,24 +167,18 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit return graphReader, nil } -// readCachedGraph opens the cached graph for a resolved treehash, preferring -// the TGB blob when the repository is configured for it and falling back to -// the gob stream for entries written before the format flip. A TGB blob that -// exists but fails validation is treated as a miss (the orchestrator will -// recompute and overwrite it), not an infra failure. Returns a not-found -// error when neither format is present. +// readCachedGraph opens the cached graph for a resolved treehash. +// TGB-configured repos read only TGB; corrupt or missing TGB propagates +// — the recovery is to recompute (which overwrites), not to serve a stale +// gob blob from an earlier format era. func (c *controller) readCachedGraph(ctx context.Context, logger *zap.Logger, graphFormat, repositoryID, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) (storage.GraphReader, error) { if graphFormat == config.GraphFormatTGB { tgbPath := cachekey.GetTGBGraphByTreeHash(repositoryID, treehash, strategy, excludeFilesRegex) graphReader, err := storage.NewTGBGraphReader(ctx, c.storage, tgbPath, c.maxMessageBytes) - if err == nil { - return graphReader, nil - } - if errors.Is(err, storage.ErrCorruptTGB) { - logger.Warn("readCachedGraph: corrupt TGB blob, falling back", zap.String("path", tgbPath), zap.Error(err)) - } else if !storage.IsNotFound(err) { - return nil, err + if err != nil && errors.Is(err, storage.ErrCorruptTGB) { + logger.Warn("readCachedGraph: corrupt TGB blob", zap.String("path", tgbPath), zap.Error(err)) } + return graphReader, err } gobPath := cachekey.GetGraphByTreeHash(repositoryID, treehash, strategy, excludeFilesRegex) return storage.NewGraphReader(ctx, c.storage, gobPath) diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 260268a2..dd558bee 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/config" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" "github.com/uber/tango/entity" @@ -369,6 +370,63 @@ func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { require.Error(t, err) } +func TestGetTargetGraph_TGB_NotFound_DoesNotFallBackToGob(t *testing.T) { + ctrl := gomock.NewController(t) + stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) + stream.EXPECT().Context().Return(context.Background()) + + store := storagemock.NewMockStorage(ctrl) + gomock.InOrder( + // treehash lookup succeeds + store.EXPECT().Get(gomock.Any(), gomock.Any()). + Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte("treehash-abc"))}, nil), + // TGB blob not found — no further Get calls (no gob fallback) + store.EXPECT().Get(gomock.Any(), gomock.Any()). + Return(storage.DownloadResponse{}, storage.NewNotFoundError("tgb/graph")), + ) + orch := orchestratormock.NewMockOrchestrator(ctrl) + graphReader := newGraphReader(t, entity.GetTargetGraphResponse{Targets: []entity.OptimizedTarget{}}) + orch.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(graphReader, nil) + stream.EXPECT().Send(gomock.Any()).Return(nil) + c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, + GraphConfig: staticGraphConfig(config.GraphConfig{Format: config.GraphFormatTGB}), + Logger: zaptest.NewLogger(t), + Storage: store, + Orchestrator: orch, + }) + err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ + BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, + }, stream) + require.NoError(t, err) +} + +func TestGetTargetGraph_TGB_Corrupt_DoesNotFallBackToGob(t *testing.T) { + ctrl := gomock.NewController(t) + stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) + stream.EXPECT().Context().Return(context.Background()) + + store := storagemock.NewMockStorage(ctrl) + gomock.InOrder( + // treehash lookup succeeds + store.EXPECT().Get(gomock.Any(), gomock.Any()). + Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte("treehash-abc"))}, nil), + // TGB blob exists but is corrupt (invalid bytes → ErrCorruptTGB) + store.EXPECT().Get(gomock.Any(), gomock.Any()). + Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte("not-a-tgb-blob"))}, nil), + ) + c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, + GraphConfig: staticGraphConfig(config.GraphConfig{Format: config.GraphFormatTGB}), + Logger: zaptest.NewLogger(t), + Storage: store, + }) + err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ + BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, + }, stream) + require.Error(t, err) +} + func newMockReadCloser(data []byte) io.ReadCloser { if data == nil { return nil From 93baefdf497dcc3d9e36bdd168c1c58245671ebd Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 7 Oct 2026 11:40:45 -0400 Subject: [PATCH 2/5] chore: retrigger CI From 6759a23e14d676448e3d5399229ed77065d01c45 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 7 Oct 2026 12:22:07 -0400 Subject: [PATCH 3/5] build(controller): increase test size to large for CI timeout The controller test suite exceeds Bazel's default 300s timeout on GitHub Actions runners. Bump size to "large" (900s) so CI completes reliably. --- controller/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 8fe1f823..6090a46b 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -38,6 +38,7 @@ go_library( go_test( name = "controller_test", + size = "large", srcs = [ "controller_test.go", "distance_filter_test.go", From 1a0b4eb5a19b66262fbaa8f91f96b66eb74e7bef Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 7 Oct 2026 15:05:21 -0400 Subject: [PATCH 4/5] build(controller): bump test size to enormous for coverage CI Coverage instrumentation makes the controller test suite significantly slower. The "large" (900s) timeout is not enough under bazel coverage on CI runners. Bump to "enormous" (3600s). --- controller/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 6090a46b..0001e125 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -38,7 +38,7 @@ go_library( go_test( name = "controller_test", - size = "large", + size = "enormous", srcs = [ "controller_test.go", "distance_filter_test.go", From fc514d0108344ce2f5a30c8c21de3223b814174a Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Fri, 9 Oct 2026 10:02:34 -0400 Subject: [PATCH 5/5] fix(controller): remove mixed-format fallback test that caused CI timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TGBMixedFormatFallsBack test validated the gob fallback path that was retired in this branch. With the fallback removed, the mock orchestrator received an unexpected call inside a goroutine, killing it via t.Fatalf without sending on the results channel — hanging fetchTargetGraphs forever. Revert the enormous test size bump that was a workaround for the hang. --- controller/BUILD.bazel | 1 - controller/getchangedtargets_tgb_test.go | 49 ------------------------ 2 files changed, 50 deletions(-) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 0001e125..8fe1f823 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -38,7 +38,6 @@ go_library( go_test( name = "controller_test", - size = "enormous", srcs = [ "controller_test.go", "distance_filter_test.go", diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index d2d1ded3..c4180733 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -374,52 +374,3 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { assert.EqualValues(t, 0, counterValue(scope, "all_targets_triggered")) assert.EqualValues(t, 1, counterValue(scope, "tgb_native_compare"), "should use normal TGB diff") } - -// TestGetChangedTargets_TGBMixedFormatFallsBack covers the transitional -// window right after a format flip: one revision's graph exists only as a -// pre-flip gob stream, the other as a TGB blob. The comparison must fall back -// to the incumbent chunk pipeline (draining the TGB reader's decoded form) -// and still produce the same answer. -func TestGetChangedTargets_TGBMixedFormatFallsBack(t *testing.T) { - ctrl := gomock.NewController(t) - stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) - stream.EXPECT().Context().Return(t.Context()) - var sent []*pb.GetChangedTargetsResponse - stream.EXPECT().Send(gomock.Any()).DoAndReturn(func(resp *pb.GetChangedTargetsResponse, _ ...interface{}) error { - sent = append(sent, resp) - return nil - }).AnyTimes() - - st := storage.NewMemoryStorage() - seedTreehash(t, st, "sha1", "treehash1") - seedTreehash(t, st, "sha2", "treehash2") - // First revision predates the flip: gob only, at the gob key. - require.NoError(t, storage.WriteGraphStream(t.Context(), st, - cachekey.GetGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash1", entity.ComputationStrategyUnset, nil), - tgbTestGraphChunks(tgbHash2Old))) - require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash2", entity.ComputationStrategyUnset, nil), - tgbTestGraphChunks(tgbHash2New))) - - scope := tally.NewTestScope("", nil) - c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: st, - Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), - Scope: scope, - GraphConfig: staticGraphConfig(config.GraphConfig{Format: config.GraphFormatTGB}), - }) - - request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) - - changed, idToName := changedTargetsSent(t, sent) - require.Len(t, changed, 1) - assert.Equal(t, tgbHash2Old, changed[0].GetOldTarget().GetHash()) - assert.Equal(t, tgbHash2New, changed[0].GetNewTarget().GetHash()) - assert.Equal(t, "//app:target2", idToName[changed[0].GetNewTarget().GetId()]) - - assert.EqualValues(t, 0, counterValue(scope, "tgb_native_compare"), "mixed formats must use the incumbent pipeline") -}