From d307022a3ebd59a8545d4109e51e4b426a8ef491 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Wed, 26 Aug 2026 18:18:49 +0000 Subject: [PATCH] fix(controller): preserve AllTargetsFiles change types for BUG-015 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: BUG-015 preserves protocol ChangeType semantics when an AllTargetsFiles global trigger fires. Intent: - Keep revision membership changes distinguishable from global invalidation. - Return complete old and new target payloads for targets present in both revisions. Changes: - Retain ordinary NEW and DELETED results before promoting shared targets to distance-zero CHANGED. - Apply identical classification behavior to chunk-backed and TGB-backed comparisons. - Cover stable, hash-changed, added, and deleted targets across both graph formats. --- Generated by the 🪄 pr-create skill in devexp-agent-marketplace --- controller/getchangedtargets.go | 115 ++++++++++++++++++----- controller/getchangedtargets_test.go | 59 +++++++++--- controller/getchangedtargets_tgb_test.go | 113 ++++++++++++++++++++++ 3 files changed, 253 insertions(+), 34 deletions(-) diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 9fc18ae7..c049bea4 100644 --- a/controller/getchangedtargets.go +++ b/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 diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 2b5f0dc1..b2b4ce7d 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -208,10 +208,11 @@ 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"}, }}, @@ -219,7 +220,7 @@ func TestCompareTargetGraphs_AllTargetsFileTrigger(t *testing.T) { 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) { diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index b8df732b..d8b43d18 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/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) {