From bdfc725003e5c53add92a58058558aed321e1168 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Wed, 26 Aug 2026 18:25:25 +0000 Subject: [PATCH] fix(itg): stabilize cycle hashing for BUG-028 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Make BUG-028 incremental cycle hashing independent of invalidation map iteration. - Preserve the cycle entry points used by full graph recomputation. Changes: - Hash invalidated topological roots and root-unreachable cycles in canonical target-name order. - Add property-based coverage over randomized invalidation insertion orders for rooted and rootless cycles. --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- core/itg/graph/BUILD.bazel | 1 + core/itg/graph/update.go | 33 ++++++++++-- core/itg/graph/update_test.go | 94 +++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 4 deletions(-) diff --git a/core/itg/graph/BUILD.bazel b/core/itg/graph/BUILD.bazel index a9281f70..f3b49be7 100644 --- a/core/itg/graph/BUILD.bazel +++ b/core/itg/graph/BUILD.bazel @@ -29,5 +29,6 @@ go_test( "@com_github_bazelbuild_buildtools//build_proto", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@net_pgregory_rapid//:rapid", ], ) diff --git a/core/itg/graph/update.go b/core/itg/graph/update.go index 457f4c15..9d0849b9 100644 --- a/core/itg/graph/update.go +++ b/core/itg/graph/update.go @@ -94,10 +94,33 @@ func (g *OptimizedGraph) UpdateGraph( return err } - // prioritize computing hashes of targets that could create cycles + return g.computeInvalidatedHashes(ctx, allInvalidated) +} + +// computeInvalidatedHashes computes invalidated target hashes in the same +// deterministic traversal order used by full graph hashing. +func (g *OptimizedGraph) computeInvalidatedHashes(ctx context.Context, invalidated IntSet) error { + // Prioritize configured targets that could create cycles. for _, target := range sequentialHashTargets { id, ok := g.TargetNameToID[target] - if !ok || !allInvalidated.Contains(id) { + if !ok || !invalidated.Contains(id) { + continue + } + if _, err := g.computeHashes(ctx, id); err != nil { + return err + } + } + + ids := invalidated.UnsortedList() + slices.SortStableFunc(ids, func(i, j int) int { + return strings.Compare(g.TargetIDToString[i], g.TargetIDToString[j]) + }) + + // Full graph hashing starts from lexicographically sorted topological roots. + // Doing the same here preserves its cycle entry points for rooted cycles. + for _, id := range ids { + target, ok := g.OptimizedTargets[id] + if !ok || len(target.ReverseDeps) != 0 { continue } if _, err := g.computeHashes(ctx, id); err != nil { @@ -105,8 +128,10 @@ func (g *OptimizedGraph) UpdateGraph( } } - // update hashes for invalidated targets - for id := range allInvalidated { + // Any remaining invalidated targets are either already hashed dependencies or + // root-unreachable cycles. The first lexicographic member is their canonical + // cycle breaker, matching full graph hashing's cyclic-target fallback. + for _, id := range ids { if _, err := g.computeHashes(ctx, id); err != nil { return err } diff --git a/core/itg/graph/update_test.go b/core/itg/graph/update_test.go index 71b847c3..19b79b34 100644 --- a/core/itg/graph/update_test.go +++ b/core/itg/graph/update_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/tango/core/targethasher" + "pgregory.net/rapid" ) // fakeSourceHasher is a test double for targethasher.SourceHasher. @@ -232,3 +233,96 @@ func TestComputeHashes(t *testing.T) { assert.Equal(t, h.Sum(nil), got) }) } + +func TestComputeInvalidatedHashesCycleOrderInvariance(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + newTargets func() map[string]*targethasher.Target + fullRoot string + }{ + { + name: "rooted cycle matches full recomputation", + newTargets: func() map[string]*targethasher.Target { + return map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + HashWithoutDeps: []byte("a"), + Deps: []string{"//pkg:b"}, + }, + "//pkg:b": { + Name: "//pkg:b", + RuleType: "go_library", + HashWithoutDeps: []byte("b"), + Deps: []string{"//pkg:a"}, + }, + "//pkg:root": { + Name: "//pkg:root", + RuleType: "go_library", + HashWithoutDeps: []byte("root"), + Deps: []string{"//pkg:b"}, + }, + } + }, + fullRoot: "//pkg:root", + }, + { + name: "rootless cycle uses canonical member", + newTargets: func() map[string]*targethasher.Target { + return map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + HashWithoutDeps: []byte("a"), + Deps: []string{"//pkg:b"}, + }, + "//pkg:b": { + Name: "//pkg:b", + RuleType: "go_library", + HashWithoutDeps: []byte("b"), + Deps: []string{"//pkg:a"}, + }, + } + }, + fullRoot: "//pkg:a", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + expectedTargets := tt.newTargets() + _, err := targethasher.HashRecursively(t.Context(), targethasher.HashParam{ + Targets: expectedTargets, + TargetName: tt.fullRoot, + }) + require.NoError(t, err) + + names := make([]string, 0, len(expectedTargets)) + expectedHashes := make(map[string][]byte, len(expectedTargets)) + for name, target := range expectedTargets { + names = append(names, name) + expectedHashes[name] = target.Hash + } + + ctx := t.Context() + rapid.Check(t, func(rt *rapid.T) { + graph := OptimizeGraph(tt.newTargets()) + invalidated := NewIntSet() + for _, name := range rapid.Permutation(names).Draw(rt, "invalidation-order") { + invalidated.Insert(graph.TargetNameToID[name]) + } + + err := graph.computeInvalidatedHashes(ctx, invalidated) + require.NoError(rt, err) + for name, expected := range expectedHashes { + actual := graph.OptimizedTargets[graph.TargetNameToID[name]].Hash + assert.Equal(rt, expected, actual, "hash mismatch for %s", name) + } + }) + }) + } +}