fix(controller): preserve AllTargetsFiles change types for BUG-015

controller/getchangedtargets.go

@@ -428,7 +428,7 @@ func (c *controller) compareFetchedGraphs(ctx context.Context, e *metrics.Emitte
 		) {
 			logger.Info("compareFetchedGraphs: AllTargetsFiles trigger matched (TGB), reporting all targets as changed")
 			e.Counter(opGetChangedTargets, "all_targets_triggered").Inc(1)
-			return c.allTargetsChangedFromTGB(ctx, second.tgb.TGB())
+			return c.allTargetsChangedFromTGB(ctx, first.tgb.TGB(), second.tgb.TGB())
 		}
 		return c.compareTargetGraphsTGB(ctx, e, logger, first.tgb.TGB(), second.tgb.TGB(), seedAttrs)
 	}
@@ -563,7 +563,7 @@ func (c *controller) compareTargetGraphs(ctx context.Context, e *metrics.Emitter
 	if allTargetsFileChanged(firstMetadata, secondMetadata) {
 		logger.Info("compareTargetGraphs: AllTargetsFiles trigger matched, reporting all targets as changed")
 		e.Counter(opGetChangedTargets, "all_targets_triggered").Inc(1)
-		return c.allTargetsChangedFromGraph(ctx, secondTargetsByID, secondMetadata)
+		return c.allTargetsChangedFromGraph(ctx, firstTargetsByID, firstMetadata, secondTargetsByID, secondMetadata)
 	}
 
 	before, err := toDiffGraph(ctx, firstTargetsByID, firstMetadata, seedAttrs)
@@ -718,38 +718,107 @@ func allTargetsFileChanged(first, second *entity.Metadata) bool {
 	return false
 }
 
-// allTargetsChangedFromGraph builds a GetChangedTargetsResponse stream that
-// marks every target in the second graph as ChangeTypeChanged with distance 0.
-// Used when an AllTargetsFiles trigger fires.
-func (c *controller) allTargetsChangedFromGraph(ctx context.Context, targetsByID map[int32]*entity.OptimizedTarget, meta *entity.Metadata) ([]entity.GetChangedTargetsResponse, error) {
-	graph, err := toDiffGraph(ctx, targetsByID, meta, nil)
+// allTargetsChangedFromGraph builds an AllTargetsFiles-triggered response from
+// two graph streams. Targets present in both revisions are promoted to CHANGED
+// at distance 0 while ordinary NEW and DELETED classifications are retained.
+func (c *controller) allTargetsChangedFromGraph(
+	ctx context.Context,
+	beforeTargetsByID map[int32]*entity.OptimizedTarget,
+	beforeMeta *entity.Metadata,
+	afterTargetsByID map[int32]*entity.OptimizedTarget,
+	afterMeta *entity.Metadata,
+) ([]entity.GetChangedTargetsResponse, error) {
+	before, err := toDiffGraph(ctx, beforeTargetsByID, beforeMeta, nil)
 	if err != nil {
 		return nil, err
 	}
-	changed := make([]*targetdiff.ChangedTarget, 0, len(graph))
-	for _, target := range graph {
+	after, err := toDiffGraph(ctx, afterTargetsByID, afterMeta, nil)
+	if err != nil {
+		return nil, err
+	}
+	result, err := allTargetsChanged(ctx, before, after)
+	if err != nil {
+		return nil, err
+	}
+	return c.resultToResponses(result)
+}
+
+// allTargetsChanged retains the ordinary comparison's membership changes and
+// promotes every target present in both revisions to a distance-zero CHANGED
+// result because the global input change may affect it independently of its
+// target hash.
+func allTargetsChanged(ctx context.Context, before, after targetdiff.Graph) (targetdiff.Result, error) {
+	changed := make([]*targetdiff.ChangedTarget, 0, len(before)+len(after))
+	i := 0
+	for name, beforeTarget := range before {
+		if i%cancelCheckInterval == 0 {
+			if err := ctx.Err(); err != nil {
+				return targetdiff.Result{}, context.Cause(ctx)
+			}
+		}
+		i++
+		afterTarget, exists := after[name]
+		if !exists {
+			changed = append(changed, &targetdiff.ChangedTarget{
+				ChangeType: targetdiff.ChangeTypeDeleted,
+				Before:     beforeTarget,
+			})
+			continue
+		}
 		changed = append(changed, &targetdiff.ChangedTarget{
 			ChangeType: targetdiff.ChangeTypeChanged,
-			After:      target,
+			Before:     beforeTarget,
+			After:      afterTarget,
 		})
 	}
-	return c.resultToResponses(targetdiff.Result{ChangedTargets: changed})
+
+	i = 0
+	for name, afterTarget := range after {
+		if i%cancelCheckInterval == 0 {
+			if err := ctx.Err(); err != nil {
+				return targetdiff.Result{}, context.Cause(ctx)
+			}
+		}
+		i++
+		if _, exists := before[name]; exists {
+			continue
+		}
+		changed = append(changed, &targetdiff.ChangedTarget{
+			ChangeType: targetdiff.ChangeTypeNew,
+			After:      afterTarget,
+		})
+	}
+	return targetdiff.Result{ChangedTargets: changed}, nil
 }
 
-// allTargetsChangedFromTGB builds a response stream marking every target in
-// the TGB reader's graph as changed with distance 0. Used when the
-// AllTargetsFiles trigger fires on the TGB comparison path.
-func (c *controller) allTargetsChangedFromTGB(ctx context.Context, r *tgb.Reader) ([]entity.GetChangedTargetsResponse, error) {
-	g, err := r.DecodeGraph()
+// allTargetsChangedFromTGB decodes both TGB revisions and applies the same
+// AllTargetsFiles classification rules as the chunk comparison path.
+func (c *controller) allTargetsChangedFromTGB(ctx context.Context, beforeReader, afterReader *tgb.Reader) ([]entity.GetChangedTargetsResponse, error) {
+	beforeGraph, err := beforeReader.DecodeGraph()
 	if err != nil {
-		return nil, fmt.Errorf("decode TGB graph: %w", err)
+		return nil, fmt.Errorf("decode first TGB graph: %w", err)
 	}
-	targetsByID := make(map[int32]*entity.OptimizedTarget, len(g.Targets))
-	for i := range g.Targets {
-		t := &g.Targets[i]
-		targetsByID[t.ID] = t
-	}
-	return c.allTargetsChangedFromGraph(ctx, targetsByID, &g.Metadata)
+	afterGraph, err := afterReader.DecodeGraph()
+	if err != nil {
+		return nil, fmt.Errorf("decode second TGB graph: %w", err)
+	}
+	beforeTargetsByID := make(map[int32]*entity.OptimizedTarget, len(beforeGraph.Targets))
+	for i := range beforeGraph.Targets {
+		target := &beforeGraph.Targets[i]
+		beforeTargetsByID[target.ID] = target
+	}
+	afterTargetsByID := make(map[int32]*entity.OptimizedTarget, len(afterGraph.Targets))
+	for i := range afterGraph.Targets {
+		target := &afterGraph.Targets[i]
+		afterTargetsByID[target.ID] = target
+	}
+	return c.allTargetsChangedFromGraph(
+		ctx,
+		beforeTargetsByID,
+		&beforeGraph.Metadata,
+		afterTargetsByID,
+		&afterGraph.Metadata,
+	)
 }
 
 // seedAttributesFor returns the RepositoryConfig.SeedAttributes

controller/getchangedtargets_test.go

@@ -208,18 +208,19 @@ func TestCompareTargetGraphs_AllTargetsFileTrigger(t *testing.T) {
 	firstGraph := []entity.GetTargetGraphResponse{
 		{Targets: []entity.OptimizedTarget{
 			{ID: 1, Hash: "h1", RuleType: 100},
-			{ID: 2, Hash: "h2", RuleType: 100},
+			{ID: 2, Hash: "h2-old", RuleType: 100},
+			{ID: 5, Hash: "h5", RuleType: 100},
 		}},
 		{Metadata: &entity.Metadata{
-			TargetIDMapping:      map[int32]string{1: "//app:a", 2: "//app:b"},
+			TargetIDMapping:      map[int32]string{1: "//app:a", 2: "//app:b", 5: "//legacy:deleted"},
 			RuleTypeMapping:      map[int32]string{100: "go_library"},
 			AllTargetsFileHashes: map[string]string{".bazelrc": "old-hash"},
 		}},
 	}
 	secondGraph := []entity.GetTargetGraphResponse{
 		{Targets: []entity.OptimizedTarget{
 			{ID: 1, Hash: "h1", RuleType: 100},
-			{ID: 2, Hash: "h2", RuleType: 100},
+			{ID: 2, Hash: "h2-new", RuleType: 100},
 			{ID: 3, Hash: "h3", RuleType: 100},
 			{ID: 4, Hash: "h4", RuleType: 100, DirectDependencies: []int32{3}},
 		}},
@@ -233,18 +234,54 @@ func TestCompareTargetGraphs_AllTargetsFileTrigger(t *testing.T) {
 	responses, err := c.compareTargetGraphs(t.Context(), c.emitter, zap.NewNop(), firstGraph, secondGraph, nil)
 	require.NoError(t, err)
 
-	var totalChanged int
+	var metadata *entity.Metadata
 	for _, resp := range responses {
-		totalChanged += len(resp.ChangedTargets)
+		if resp.Metadata != nil {
+			metadata = resp.Metadata
+		}
 	}
-	assert.Equal(t, 4, totalChanged, "all targets from second graph should be reported as changed")
-	for _, resp := range responses {
-		for _, ct := range resp.ChangedTargets {
-			assert.Equal(t, entity.ChangeTypeChanged, ct.ChangeType)
-			assert.Equal(t, int32(0), ct.Distance)
-			assert.NotNil(t, ct.NewTarget)
+	require.NotNil(t, metadata)
+
+	byName := make(map[string]*entity.ChangedTarget)
+	for responseIndex := range responses {
+		for targetIndex := range responses[responseIndex].ChangedTargets {
+			changed := &responses[responseIndex].ChangedTargets[targetIndex]
+			target := changed.NewTarget
+			if target == nil {
+				target = changed.OldTarget
+			}
+			require.NotNil(t, target)
+			name := metadata.TargetIDMapping[target.ID]
+			require.NotEmpty(t, name)
+			byName[name] = changed
+		}
+	}
+	require.Len(t, byName, 5)
+
+	assertChange := func(name string, wantType entity.ChangeType, wantOld, wantNew bool) {
+		t.Helper()
+		changed := byName[name]
+		require.NotNil(t, changed, name)
+		assert.Equal(t, wantType, changed.ChangeType, name)
+		assert.Equal(t, int32(0), changed.Distance, name)
+		if wantOld {
+			require.NotNil(t, changed.OldTarget, name)
+			assert.Equal(t, name, metadata.TargetIDMapping[changed.OldTarget.ID])
+		} else {
+			assert.Nil(t, changed.OldTarget, name)
+		}
+		if wantNew {
+			require.NotNil(t, changed.NewTarget, name)
+			assert.Equal(t, name, metadata.TargetIDMapping[changed.NewTarget.ID])
+		} else {
+			assert.Nil(t, changed.NewTarget, name)
 		}
 	}
+	assertChange("//app:a", entity.ChangeTypeChanged, true, true)
+	assertChange("//app:b", entity.ChangeTypeChanged, true, true)
+	assertChange("//app:c", entity.ChangeTypeNew, false, true)
+	assertChange("//lib:util", entity.ChangeTypeNew, false, true)
+	assertChange("//legacy:deleted", entity.ChangeTypeDeleted, true, false)
 }
 
 func TestCompareTargetGraphs_AllTargetsFileNoTrigger(t *testing.T) {

controller/getchangedtargets_tgb_test.go

@@ -171,6 +171,44 @@ func tgbTestGraphChunksWithATFH(hash2 string, atfh map[string]string) []entity.G
 	}
 }
 
+func tgbAllTargetsClassificationBefore(atfh map[string]string) []entity.GetTargetGraphResponse {
+	return []entity.GetTargetGraphResponse{
+		{Targets: []entity.OptimizedTarget{
+			{ID: 1, Hash: tgbHash1, RuleType: 100},
+			{ID: 2, Hash: tgbHash2Old, RuleType: 100, DirectDependencies: []int32{1}},
+			{ID: 3, Hash: tgbHash3, RuleType: 100},
+		}},
+		{Metadata: &entity.Metadata{
+			TargetIDMapping: map[int32]string{
+				1: "//app:stable",
+				2: "//app:changed",
+				3: "//legacy:deleted",
+			},
+			RuleTypeMapping:      map[int32]string{100: "go_library"},
+			AllTargetsFileHashes: atfh,
+		}},
+	}
+}
+
+func tgbAllTargetsClassificationAfter(atfh map[string]string) []entity.GetTargetGraphResponse {
+	return []entity.GetTargetGraphResponse{
+		{Targets: []entity.OptimizedTarget{
+			{ID: 10, Hash: tgbHash1, RuleType: 200},
+			{ID: 20, Hash: tgbHash2New, RuleType: 200, DirectDependencies: []int32{10}},
+			{ID: 40, Hash: tgbHash4, RuleType: 200},
+		}},
+		{Metadata: &entity.Metadata{
+			TargetIDMapping: map[int32]string{
+				10: "//app:stable",
+				20: "//app:changed",
+				40: "//app:new",
+			},
+			RuleTypeMapping:      map[int32]string{200: "go_library"},
+			AllTargetsFileHashes: atfh,
+		}},
+	}
+}
+
 // TestGetChangedTargets_TGBAllTargetsTrigger verifies that the TGB comparison
 // path checks AllTargetsFileHashes and, when a configured file differs,
 // reports every target in the second graph as changed with distance 0.
@@ -218,6 +256,81 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) {
 	assert.EqualValues(t, 0, counterValue(scope, "tgb_native_compare"), "trigger should skip the normal TGB diff")
 }
 
+func TestGetChangedTargets_TGBAllTargetsTriggerPreservesMembershipChanges(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")
+	require.NoError(t, storage.WriteTGBGraph(t.Context(), st,
+		cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash1", entity.ComputationStrategyUnset, nil),
+		tgbAllTargetsClassificationBefore(map[string]string{".bazelrc": "old-hash"})))
+	require.NoError(t, storage.WriteTGBGraph(t.Context(), st,
+		cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil),
+		tgbAllTargetsClassificationAfter(map[string]string{".bazelrc": "new-hash"})))
+
+	scope := tally.NewTestScope("", nil)
+	c := NewController(context.Background(), Params{
+		Logger:       zaptest.NewLogger(t),
+		Storage:      st,
+		Orchestrator: orchestratormock.NewMockOrchestrator(ctrl),
+		Scope:        scope,
+		GraphFormat:  config.GraphFormatTGB,
+	})
+
+	request := changedTargetsRequest()
+	request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true}
+	require.NoError(t, c.GetChangedTargets(request, stream))
+
+	changed, idToName := changedTargetsSent(t, sent)
+	require.Len(t, changed, 4)
+	byName := make(map[string]*pb.ChangedTarget, len(changed))
+	for _, target := range changed {
+		optimized := target.GetNewTarget()
+		if optimized == nil {
+			optimized = target.GetOldTarget()
+		}
+		require.NotNil(t, optimized)
+		name := idToName[optimized.GetId()]
+		require.NotEmpty(t, name)
+		byName[name] = target
+	}
+
+	assertChange := func(name string, wantType pb.ChangeType, wantOld, wantNew bool) {
+		t.Helper()
+		target := byName[name]
+		require.NotNil(t, target, name)
+		assert.Equal(t, wantType, target.GetChangeType(), name)
+		assert.Equal(t, int32(0), target.GetDistance(), name)
+		if wantOld {
+			require.NotNil(t, target.GetOldTarget(), name)
+			assert.Equal(t, name, idToName[target.GetOldTarget().GetId()])
+		} else {
+			assert.Nil(t, target.GetOldTarget(), name)
+		}
+		if wantNew {
+			require.NotNil(t, target.GetNewTarget(), name)
+			assert.Equal(t, name, idToName[target.GetNewTarget().GetId()])
+		} else {
+			assert.Nil(t, target.GetNewTarget(), name)
+		}
+	}
+	assertChange("//app:stable", pb.CHANGE_TYPE_CHANGED, true, true)
+	assertChange("//app:changed", pb.CHANGE_TYPE_CHANGED, true, true)
+	assertChange("//app:new", pb.CHANGE_TYPE_NEW, false, true)
+	assertChange("//legacy:deleted", pb.CHANGE_TYPE_DELETED, true, false)
+
+	assert.EqualValues(t, 1, counterValue(scope, "all_targets_triggered"))
+	assert.EqualValues(t, 0, counterValue(scope, "tgb_native_compare"), "trigger should skip the normal TGB diff")
+}
+
 // TestGetChangedTargets_TGBAllTargetsNoTrigger verifies that the TGB path
 // proceeds with normal comparison when AllTargetsFileHashes match.
 func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) {