refactor(controller): retire gob fallback for TGB-configured repos

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")
-}

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)

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