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