From 262b80a421e751f634f9fe49684c5f036af1b133 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 09:38:22 -0700 Subject: [PATCH 1/6] feat(tgb): encode AllTargetsFileHashes as column 22 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TGB support for the AllTargetsFileHashes sidecar so the AllTargetsFiles feature works regardless of graph_format. Column 22 stores a length-prefixed sequence of (key, value) string pairs, zstd-compressed. Old readers skip unknown column IDs; new readers on old blobs get nil — no format version bump needed. The TGB comparison path in the controller now checks AllTargetsFileHashes via a cheap single-column decode before the full columnar diff, matching the gob path's behavior. Co-Authored-By: Claude Sonnet 5 --- controller/getchangedtargets.go | 32 ++++++++++++++ internal/tgb/encode.go | 19 +++++++++ internal/tgb/format.go | 7 ++- internal/tgb/reader.go | 75 ++++++++++++++++++++++++++++----- 4 files changed, 121 insertions(+), 12 deletions(-) diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index c2c50aa1..9fc18ae7 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -414,6 +414,22 @@ func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetCha // where exactly one revision's blob predates a format flip). func (c *controller) compareFetchedGraphs(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, first, second fetchedGraph, seedAttrs map[string]bool) ([]entity.GetChangedTargetsResponse, error) { if first.tgb != nil && second.tgb != nil { + firstATFH, err := first.tgb.TGB().AllTargetsFileHashes() + if err != nil { + return nil, fmt.Errorf("read AllTargetsFileHashes from first TGB: %w", err) + } + secondATFH, err := second.tgb.TGB().AllTargetsFileHashes() + if err != nil { + return nil, fmt.Errorf("read AllTargetsFileHashes from second TGB: %w", err) + } + if allTargetsFileChanged( + &entity.Metadata{AllTargetsFileHashes: firstATFH}, + &entity.Metadata{AllTargetsFileHashes: secondATFH}, + ) { + 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.compareTargetGraphsTGB(ctx, e, logger, first.tgb.TGB(), second.tgb.TGB(), seedAttrs) } firstChunks, err := first.materializeChunks() @@ -720,6 +736,22 @@ func (c *controller) allTargetsChangedFromGraph(ctx context.Context, targetsByID return c.resultToResponses(targetdiff.Result{ChangedTargets: changed}) } +// 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() + if err != nil { + return nil, fmt.Errorf("decode 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) +} + // seedAttributesFor returns the RepositoryConfig.SeedAttributes // allowlist configured for the given remote, or nil when the repository has no // override configured — meaning every attribute is considered a valid signal diff --git a/internal/tgb/encode.go b/internal/tgb/encode.go index 15e051bb..bb5ae27a 100644 --- a/internal/tgb/encode.go +++ b/internal/tgb/encode.go @@ -736,6 +736,10 @@ func (e *encoder) write(w io.Writer) error { {colBlockStart, codecZstd, blockStartRaw}, } + if len(e.g.Metadata.AllTargetsFileHashes) > 0 { + rawCols = append(rawCols, rawCol{colAllTargetsFileHashes, codecZstd, encodeStringMap(e.g.Metadata.AllTargetsFileHashes)}) + } + // Compress zstd columns (optionally parallel). type compressedCol struct { id uint64 @@ -829,6 +833,21 @@ func (e *encoder) write(w io.Writer) error { return nil } +// encodeStringMap serializes a map[string]string as a length-prefixed sequence +// of (key, value) pairs: count (uvarint), then for each pair the key length +// (uvarint), key bytes, value length (uvarint), value bytes. +func encodeStringMap(m map[string]string) []byte { + var buf []byte + buf = binary.AppendUvarint(buf, uint64(len(m))) + for k, v := range m { + buf = binary.AppendUvarint(buf, uint64(len(k))) + buf = append(buf, k...) + buf = binary.AppendUvarint(buf, uint64(len(v))) + buf = append(buf, v...) + } + return buf +} + // TruncateHashes returns a new Graph with each target's Hash field truncated // to n raw bytes and re-hex-encoded. This is necessary for round-trip tests // because TGB only stores the first n bytes of the hash, and Decode can only diff --git a/internal/tgb/format.go b/internal/tgb/format.go index 4950031a..958c5aa5 100644 --- a/internal/tgb/format.go +++ b/internal/tgb/format.go @@ -104,6 +104,10 @@ const ( // — ensureDepOffsets builds a full offset table and DepsCSR reads in bulk // — so the encoder stopped writing it. Do not reuse the ID. colNodeIndex = uint64(21) + // colAllTargetsFileHashes (22) stores the AllTargetsFileHashes sidecar: + // a length-prefixed sequence of (key, value) string pairs. Old readers + // that don't know this ID skip it; new readers on old blobs get nil. + colAllTargetsFileHashes = uint64(22) ) // colCodec values stored in the directory. @@ -135,7 +139,8 @@ var colNames = map[uint64]string{ colFlags: "FLAGS", colBlockDigest: "BLOCK_DIGEST", colBlockStart: "BLOCK_START", - colNodeIndex: "NODE_INDEX", + colNodeIndex: "NODE_INDEX", + colAllTargetsFileHashes: "ALL_TARGETS_FILE_HASHES", } // ─── Header ─────────────────────────────────────────────────────────────────── diff --git a/internal/tgb/reader.go b/internal/tgb/reader.go index 86b9436a..10b5ae2f 100644 --- a/internal/tgb/reader.go +++ b/internal/tgb/reader.go @@ -179,17 +179,27 @@ func (r *Reader) DecodeGraph() (*Graph, error) { // Build rule-type mapping: dict-index → string (already in ruleTypeMap). // Tag mapping similarly. - g := &Graph{ - Targets: targets, - Metadata: entity.Metadata{ - TargetIDMapping: targetIDMap, - RuleTypeMapping: ruleTypeMap, - TagMapping: tagMap, - AttributeNameMapping: attrNameMap, - AttributeStringValueMapping: attrValMap, - }, - } - return g, nil + meta := entity.Metadata{ + TargetIDMapping: targetIDMap, + RuleTypeMapping: ruleTypeMap, + TagMapping: tagMap, + AttributeNameMapping: attrNameMap, + AttributeStringValueMapping: attrValMap, + } + + if _, ok := r.cols[colAllTargetsFileHashes]; ok { + data, err := r.getColumn(colAllTargetsFileHashes) + if err != nil { + return nil, fmt.Errorf("tgb: decode AllTargetsFileHashes: %w", err) + } + m, err := decodeStringMap(data) + if err != nil { + return nil, fmt.Errorf("tgb: decode AllTargetsFileHashes: %w", err) + } + meta.AllTargetsFileHashes = m + } + + return &Graph{Targets: targets, Metadata: meta}, nil } // ─── zstd decompression ─────────────────────────────────────────────────────── @@ -1052,6 +1062,49 @@ func (r *Reader) getColumnLocked(id uint64) ([]byte, error) { return out, nil } +// decodeStringMap deserializes the format produced by encodeStringMap: count +// (uvarint), then count (key-length, key, value-length, value) pairs. +func decodeStringMap(data []byte) (map[string]string, error) { + n, sz := binary.Uvarint(data) + if sz <= 0 { + return nil, fmt.Errorf("truncated count") + } + data = data[sz:] + m := make(map[string]string, n) + for i := uint64(0); i < n; i++ { + kLen, sz := binary.Uvarint(data) + if sz <= 0 || uint64(len(data)-sz) < kLen { + return nil, fmt.Errorf("truncated key at entry %d", i) + } + data = data[sz:] + key := string(data[:kLen]) + data = data[kLen:] + + vLen, sz := binary.Uvarint(data) + if sz <= 0 || uint64(len(data)-sz) < vLen { + return nil, fmt.Errorf("truncated value at entry %d", i) + } + data = data[sz:] + m[key] = string(data[:vLen]) + data = data[vLen:] + } + return m, nil +} + +// AllTargetsFileHashes returns the sidecar file-hash map stored in column 22, +// or nil when the column is absent (old blobs or repos without AllTargetsFiles). +// Only the single column is decompressed — no full graph decode. +func (r *Reader) AllTargetsFileHashes() (map[string]string, error) { + if _, ok := r.cols[colAllTargetsFileHashes]; !ok { + return nil, nil + } + data, err := r.getColumn(colAllTargetsFileHashes) + if err != nil { + return nil, err + } + return decodeStringMap(data) +} + func (r *Reader) ensurePkgDict() error { if r.pkgDict != nil { return nil From 81bb01ed3a2b56c69fab58bd17f635d8eec8df84 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 09:48:10 -0700 Subject: [PATCH 2/6] test(tgb): add AllTargetsFileHashes round-trip tests Verify that AllTargetsFileHashes survives TGB encode/decode via both full Decode and the lazy Reader.AllTargetsFileHashes() accessor. Covers present, absent, and empty-map cases. Also fixes canonicalise() to preserve AllTargetsFileHashes so existing round-trip tests remain correct when graphs carry the sidecar. Co-Authored-By: Claude Sonnet 5 --- internal/tgb/BUILD.bazel | 2 ++ internal/tgb/tgb_test.go | 55 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/internal/tgb/BUILD.bazel b/internal/tgb/BUILD.bazel index 32c324c2..ff05fdf0 100644 --- a/internal/tgb/BUILD.bazel +++ b/internal/tgb/BUILD.bazel @@ -31,5 +31,7 @@ go_test( deps = [ ":tgb", "//entity", + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", ], ) diff --git a/internal/tgb/tgb_test.go b/internal/tgb/tgb_test.go index f85ff4c1..22db69fe 100644 --- a/internal/tgb/tgb_test.go +++ b/internal/tgb/tgb_test.go @@ -9,6 +9,8 @@ import ( "sort" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/uber/tango/entity" "github.com/uber/tango/internal/tgb" ) @@ -169,6 +171,7 @@ func canonicalise(g *tgb.Graph) *tgb.Graph { TagMapping: tagMap, AttributeNameMapping: anMap, AttributeStringValueMapping: avMap, + AllTargetsFileHashes: g.Metadata.AllTargetsFileHashes, }, } } @@ -310,6 +313,58 @@ func TestTinyGraphRoundTrip(t *testing.T) { } } +func TestAllTargetsFileHashesRoundTrip(t *testing.T) { + t.Parallel() + + t.Run("present", func(t *testing.T) { + t.Parallel() + g := buildTinyGraph() + g.Metadata.AllTargetsFileHashes = map[string]string{ + ".bazelrc": "abc123", + "tools/bazel": "def456", + "rules/go/sdk.bzl": "789ghi", + } + + data := mustEncode(t, g, tgb.EncodeOptions{HashBytes: 8, BlockSize: 4}) + got := mustDecode(t, data) + + assert.Equal(t, g.Metadata.AllTargetsFileHashes, got.Metadata.AllTargetsFileHashes) + + r, err := tgb.NewReader(data) + require.NoError(t, err) + atfh, err := r.AllTargetsFileHashes() + require.NoError(t, err) + assert.Equal(t, g.Metadata.AllTargetsFileHashes, atfh) + }) + + t.Run("absent", func(t *testing.T) { + t.Parallel() + g := buildTinyGraph() + + data := mustEncode(t, g, tgb.EncodeOptions{HashBytes: 8, BlockSize: 4}) + got := mustDecode(t, data) + + assert.Nil(t, got.Metadata.AllTargetsFileHashes) + + r, err := tgb.NewReader(data) + require.NoError(t, err) + atfh, err := r.AllTargetsFileHashes() + require.NoError(t, err) + assert.Nil(t, atfh) + }) + + t.Run("empty map not encoded", func(t *testing.T) { + t.Parallel() + g := buildTinyGraph() + g.Metadata.AllTargetsFileHashes = map[string]string{} + + data := mustEncode(t, g, tgb.EncodeOptions{HashBytes: 8, BlockSize: 4}) + got := mustDecode(t, data) + + assert.Nil(t, got.Metadata.AllTargetsFileHashes) + }) +} + // ─── Test: property test over random small graphs ───────────────────────────── func TestRandomGraphRoundTrip(t *testing.T) { From 8e6961748c06ad432eb9d60bc29b4dd17c201e0c Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 10:16:09 -0700 Subject: [PATCH 3/6] fix(tgbdiff): propagate AllTargetsFileHashes through MergeChunks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MergeChunks was not merging AllTargetsFileHashes from metadata chunks, so the field was silently dropped before TGB encoding. This caused the AllTargetsFiles trigger to never fire on the TGB comparison path. Also adds controller-level TGB tests for both trigger and no-trigger cases, verifying end-to-end that AllTargetsFileHashes survives the full write → read → compare pipeline. Co-Authored-By: Claude Sonnet 5 --- controller/getchangedtargets_tgb_test.go | 104 +++++++++++++++++++++++ internal/tgbdiff/adapt.go | 6 ++ 2 files changed, 110 insertions(+) diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index 3485720c..cb75ca96 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -151,6 +151,110 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { assert.EqualValues(t, 0, counterValue(scope, "tgb_shadow_error")) } +// tgbTestGraphChunksWithATFH builds a two-target graph with AllTargetsFileHashes +// set in the metadata, for testing the TGB AllTargetsFiles trigger path. +func tgbTestGraphChunksWithATFH(hash2 string, atfh map[string]string) []entity.GetTargetGraphResponse { + return []entity.GetTargetGraphResponse{ + {Targets: []entity.OptimizedTarget{ + {ID: 1, Hash: tgbHash1, RuleType: 100}, + {ID: 2, Hash: hash2, RuleType: 100, DirectDependencies: []int32{1}}, + }}, + {Metadata: &entity.Metadata{ + TargetIDMapping: map[int32]string{1: "//app:target1", 2: "//app:target2"}, + RuleTypeMapping: map[int32]string{100: "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. +func TestGetChangedTargets_TGBAllTargetsTrigger(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), + tgbTestGraphChunksWithATFH(tgbHash2Old, map[string]string{".bazelrc": "old-hash"}))) + require.NoError(t, storage.WriteTGBGraph(t.Context(), st, + cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + tgbTestGraphChunksWithATFH(tgbHash2Old, 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, _ := changedTargetsSent(t, sent) + require.Len(t, changed, 2, "all targets from second graph should be reported as changed") + for _, ct := range changed { + assert.Equal(t, pb.CHANGE_TYPE_CHANGED, ct.GetChangeType()) + assert.Equal(t, int32(0), ct.GetDistance()) + assert.NotNil(t, ct.GetNewTarget()) + } + 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) { + 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), + tgbTestGraphChunksWithATFH(tgbHash2Old, map[string]string{".bazelrc": "same-hash"}))) + require.NoError(t, storage.WriteTGBGraph(t.Context(), st, + cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + tgbTestGraphChunksWithATFH(tgbHash2New, map[string]string{".bazelrc": "same-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, _ := changedTargetsSent(t, sent) + require.Len(t, changed, 1, "only the hash-flipped target should be changed") + 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 diff --git a/internal/tgbdiff/adapt.go b/internal/tgbdiff/adapt.go index ade1105e..bc67e122 100644 --- a/internal/tgbdiff/adapt.go +++ b/internal/tgbdiff/adapt.go @@ -79,6 +79,12 @@ func MergeChunks(chunks []entity.GetTargetGraphResponse) *tgb.Graph { for k, v := range m.AttributeStringValueMapping { g.Metadata.AttributeStringValueMapping[k] = v } + for k, v := range m.AllTargetsFileHashes { + if g.Metadata.AllTargetsFileHashes == nil { + g.Metadata.AllTargetsFileHashes = make(map[string]string, len(m.AllTargetsFileHashes)) + } + g.Metadata.AllTargetsFileHashes[k] = v + } } } return g From 5fe166d564e2ccfdae922704fc327429675e2cc8 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 14:28:54 -0700 Subject: [PATCH 4/6] fix(tgb): sort keys in encodeStringMap for deterministic encoding Map iteration order is random in Go, so the same logical AllTargetsFileHashes could produce different byte sequences across runs, violating the deterministic computation property and causing spurious cache misses. Co-Authored-By: Claude Sonnet 5 --- internal/tgb/encode.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/tgb/encode.go b/internal/tgb/encode.go index bb5ae27a..081f29d0 100644 --- a/internal/tgb/encode.go +++ b/internal/tgb/encode.go @@ -837,13 +837,19 @@ func (e *encoder) write(w io.Writer) error { // of (key, value) pairs: count (uvarint), then for each pair the key length // (uvarint), key bytes, value length (uvarint), value bytes. func encodeStringMap(m map[string]string) []byte { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + var buf []byte buf = binary.AppendUvarint(buf, uint64(len(m))) - for k, v := range m { + for _, k := range keys { buf = binary.AppendUvarint(buf, uint64(len(k))) buf = append(buf, k...) - buf = binary.AppendUvarint(buf, uint64(len(v))) - buf = append(buf, v...) + buf = binary.AppendUvarint(buf, uint64(len(m[k]))) + buf = append(buf, m[k]...) } return buf } From 959591c42b02c8dd8fb62148ee775c63bc8d6cf5 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 14:35:22 -0700 Subject: [PATCH 5/6] fix(tgb): gofmt fixes, dedupe AllTargetsFileHashes decode in DecodeGraph - gofmt alignment in colNames map and a test's Params struct literal - DecodeGraph now reuses Reader.AllTargetsFileHashes() instead of duplicating the column-decode logic inline --- controller/getchangedtargets_tgb_test.go | 16 ++++----- internal/tgb/format.go | 42 ++++++++++++------------ internal/tgb/reader.go | 16 ++++----- 3 files changed, 35 insertions(+), 39 deletions(-) diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index cb75ca96..b8fe4614 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -192,11 +192,11 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: st, + Logger: zaptest.NewLogger(t), + Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), - Scope: scope, - GraphFormat: config.GraphFormatTGB, + Scope: scope, + GraphFormat: config.GraphFormatTGB, }) request := changedTargetsRequest() @@ -238,11 +238,11 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: st, + Logger: zaptest.NewLogger(t), + Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), - Scope: scope, - GraphFormat: config.GraphFormatTGB, + Scope: scope, + GraphFormat: config.GraphFormatTGB, }) request := changedTargetsRequest() diff --git a/internal/tgb/format.go b/internal/tgb/format.go index 958c5aa5..27a72722 100644 --- a/internal/tgb/format.go +++ b/internal/tgb/format.go @@ -119,27 +119,27 @@ const ( // ─── Column names (for ColumnStats) ────────────────────────────────────────── var colNames = map[uint64]string{ - colPkgDict: "PKG_DICT", - colNameDict: "NAME_DICT", - colNodePkg: "NODE_PKG", - colNodeName: "NODE_NAME", - colHash: "HASH", - colDeg: "DEG", - colDeps: "DEPS", - colRuleType: "RULETYPE", - colRuleTypeDict: "RULETYPE_DICT", - colTagDeg: "TAG_DEG", - colTags: "TAGS", - colTagDict: "TAG_DICT", - colAttrDeg: "ATTR_DEG", - colAttrName: "ATTR_NAME", - colAttrValue: "ATTR_VALUE", - colAttrNameDict: "ATTR_NAME_DICT", - colAttrValueDict: "ATTR_VALUE_DICT", - colFlags: "FLAGS", - colBlockDigest: "BLOCK_DIGEST", - colBlockStart: "BLOCK_START", - colNodeIndex: "NODE_INDEX", + colPkgDict: "PKG_DICT", + colNameDict: "NAME_DICT", + colNodePkg: "NODE_PKG", + colNodeName: "NODE_NAME", + colHash: "HASH", + colDeg: "DEG", + colDeps: "DEPS", + colRuleType: "RULETYPE", + colRuleTypeDict: "RULETYPE_DICT", + colTagDeg: "TAG_DEG", + colTags: "TAGS", + colTagDict: "TAG_DICT", + colAttrDeg: "ATTR_DEG", + colAttrName: "ATTR_NAME", + colAttrValue: "ATTR_VALUE", + colAttrNameDict: "ATTR_NAME_DICT", + colAttrValueDict: "ATTR_VALUE_DICT", + colFlags: "FLAGS", + colBlockDigest: "BLOCK_DIGEST", + colBlockStart: "BLOCK_START", + colNodeIndex: "NODE_INDEX", colAllTargetsFileHashes: "ALL_TARGETS_FILE_HASHES", } diff --git a/internal/tgb/reader.go b/internal/tgb/reader.go index 10b5ae2f..c684cfed 100644 --- a/internal/tgb/reader.go +++ b/internal/tgb/reader.go @@ -187,16 +187,12 @@ func (r *Reader) DecodeGraph() (*Graph, error) { AttributeStringValueMapping: attrValMap, } - if _, ok := r.cols[colAllTargetsFileHashes]; ok { - data, err := r.getColumn(colAllTargetsFileHashes) - if err != nil { - return nil, fmt.Errorf("tgb: decode AllTargetsFileHashes: %w", err) - } - m, err := decodeStringMap(data) - if err != nil { - return nil, fmt.Errorf("tgb: decode AllTargetsFileHashes: %w", err) - } - meta.AllTargetsFileHashes = m + atfh, err := r.AllTargetsFileHashes() + if err != nil { + return nil, fmt.Errorf("tgb: decode AllTargetsFileHashes: %w", err) + } + if atfh != nil { + meta.AllTargetsFileHashes = atfh } return &Graph{Targets: targets, Metadata: meta}, nil From c51d4b7c57d220791ea39b414573609857d264c9 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 19 Aug 2026 14:57:36 -0700 Subject: [PATCH 6/6] test(controller): expand AllTargetsFiles trigger tests to 4 targets Makes it clearer that all targets in the graph are reported as changed when the trigger fires, not just the ones that happen to have different hashes. Co-Authored-By: Claude Sonnet 5 --- controller/getchangedtargets_test.go | 5 +++-- controller/getchangedtargets_tgb_test.go | 10 +++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 1403e162..2b5f0dc1 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -221,9 +221,10 @@ func TestCompareTargetGraphs_AllTargetsFileTrigger(t *testing.T) { {ID: 1, Hash: "h1", RuleType: 100}, {ID: 2, Hash: "h2", RuleType: 100}, {ID: 3, Hash: "h3", RuleType: 100}, + {ID: 4, Hash: "h4", RuleType: 100, DirectDependencies: []int32{3}}, }}, {Metadata: &entity.Metadata{ - TargetIDMapping: map[int32]string{1: "//app:a", 2: "//app:b", 3: "//app:c"}, + TargetIDMapping: map[int32]string{1: "//app:a", 2: "//app:b", 3: "//app:c", 4: "//lib:util"}, RuleTypeMapping: map[int32]string{100: "go_library"}, AllTargetsFileHashes: map[string]string{".bazelrc": "new-hash"}, }}, @@ -236,7 +237,7 @@ func TestCompareTargetGraphs_AllTargetsFileTrigger(t *testing.T) { for _, resp := range responses { totalChanged += len(resp.ChangedTargets) } - assert.Equal(t, 3, totalChanged, "all targets from second graph should be reported as changed") + 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) diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index b8fe4614..b8df732b 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -40,6 +40,8 @@ var ( tgbHash1 = strings.Repeat("aa", 20) tgbHash2Old = strings.Repeat("bb", 20) tgbHash2New = strings.Repeat("cc", 20) + tgbHash3 = strings.Repeat("dd", 20) + tgbHash4 = strings.Repeat("ee", 20) ) // tgbTestGraphChunks builds the two-target graph the gob-era streamChunks @@ -151,16 +153,18 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { assert.EqualValues(t, 0, counterValue(scope, "tgb_shadow_error")) } -// tgbTestGraphChunksWithATFH builds a two-target graph with AllTargetsFileHashes +// tgbTestGraphChunksWithATFH builds a four-target graph with AllTargetsFileHashes // set in the metadata, for testing the TGB AllTargetsFiles trigger path. func tgbTestGraphChunksWithATFH(hash2 string, atfh map[string]string) []entity.GetTargetGraphResponse { return []entity.GetTargetGraphResponse{ {Targets: []entity.OptimizedTarget{ {ID: 1, Hash: tgbHash1, RuleType: 100}, {ID: 2, Hash: hash2, RuleType: 100, DirectDependencies: []int32{1}}, + {ID: 3, Hash: tgbHash3, RuleType: 100}, + {ID: 4, Hash: tgbHash4, RuleType: 100, DirectDependencies: []int32{3}}, }}, {Metadata: &entity.Metadata{ - TargetIDMapping: map[int32]string{1: "//app:target1", 2: "//app:target2"}, + TargetIDMapping: map[int32]string{1: "//app:target1", 2: "//app:target2", 3: "//lib:util", 4: "//lib:core"}, RuleTypeMapping: map[int32]string{100: "go_library"}, AllTargetsFileHashes: atfh, }}, @@ -204,7 +208,7 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { require.NoError(t, c.GetChangedTargets(request, stream)) changed, _ := changedTargetsSent(t, sent) - require.Len(t, changed, 2, "all targets from second graph should be reported as changed") + require.Len(t, changed, 4, "all targets from second graph should be reported as changed") for _, ct := range changed { assert.Equal(t, pb.CHANGE_TYPE_CHANGED, ct.GetChangeType()) assert.Equal(t, int32(0), ct.GetDistance())