From 23160bf35918e3905f117efca02b51c490049505 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 21 Jul 2026 15:42:55 -0700 Subject: [PATCH] fix(cachekey): include ordered request commits Include each change request URL and commit in caller order with length framing so distinct build descriptions cannot share a treehash cache entry. Add Rapid properties asserting that changing a commit or reordering change requests changes the cache path. --- core/cachekey/BUILD.bazel | 2 + core/cachekey/cachekey_test.go | 13 ++---- core/cachekey/treehash_rapid_test.go | 63 ++++++++++++++++++++++++++++ internal/url/url.go | 24 ++++++----- internal/url/url_test.go | 20 ++++----- 5 files changed, 93 insertions(+), 29 deletions(-) create mode 100644 core/cachekey/treehash_rapid_test.go diff --git a/core/cachekey/BUILD.bazel b/core/cachekey/BUILD.bazel index 22bf1ed3..35f9313c 100644 --- a/core/cachekey/BUILD.bazel +++ b/core/cachekey/BUILD.bazel @@ -16,10 +16,12 @@ go_test( srcs = [ "cachekey_test.go", "exclude_regex_rapid_test.go", + "treehash_rapid_test.go", ], embed = [":cachekey"], deps = [ "//entity", + "//internal/url", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@net_pgregory_rapid//:rapid", diff --git a/core/cachekey/cachekey_test.go b/core/cachekey/cachekey_test.go index 5ada359f..48db2da6 100644 --- a/core/cachekey/cachekey_test.go +++ b/core/cachekey/cachekey_test.go @@ -15,13 +15,12 @@ package cachekey import ( - "crypto/md5" - "fmt" "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/uber/tango/entity" + "github.com/uber/tango/internal/url" ) func TestGetGraphByTreeHash(t *testing.T) { @@ -55,16 +54,12 @@ func TestGetTreehashCachePath(t *testing.T) { Remote: "git@github:uber/tango", BaseSha: "deadbeef", ChangeRequests: []entity.ChangeRequest{ - {URL: "github://org/repo/pull/1"}, - {URL: "custom://foo/bar"}, + {URL: "github://org/repo/pull/1", Commit: "abc"}, + {URL: "custom://foo/bar", Commit: "def"}, }, } got := GetTreehashCachePath(desc) - // URLs are sorted then fed individually into the digest (no separator) - h := md5.New() - h.Write([]byte("custom://foo/bar")) - h.Write([]byte("github://org/repo/pull/1")) - want := filepath.Join("uber/tango", "treehashes", "base-sha-deadbeef") + "_request-urls-" + fmt.Sprintf("%x", h.Sum(nil)) + want := filepath.Join("uber/tango", "treehashes", "base-sha-deadbeef") + "_request-urls-" + url.GetReqURLsHash(desc.ChangeRequests) assert.Equal(t, want, got) } diff --git a/core/cachekey/treehash_rapid_test.go b/core/cachekey/treehash_rapid_test.go new file mode 100644 index 00000000..c9fb0089 --- /dev/null +++ b/core/cachekey/treehash_rapid_test.go @@ -0,0 +1,63 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cachekey + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/uber/tango/entity" + "pgregory.net/rapid" +) + +func TestGetTreehashCachePath_commitAffectsKey(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + commit := rapid.StringMatching(`[a-z]+`).Draw(t, "commit") + build := entity.BuildDescription{ + Remote: "git@github:uber/tango", + BaseSha: "base", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://uber/tango/pull/1", Commit: commit}, + }, + } + changed := build + changed.ChangeRequests = append([]entity.ChangeRequest(nil), build.ChangeRequests...) + changed.ChangeRequests[0].Commit = commit + "x" + + require.NotEqual(t, GetTreehashCachePath(build), GetTreehashCachePath(changed)) + }) +} + +func TestGetTreehashCachePath_changeRequestOrderAffectsKey(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + firstCommit := rapid.StringMatching(`[a-z]+`).Draw(t, "firstCommit") + secondCommit := rapid.StringMatching(`[a-z]+`).Draw(t, "secondCommit") + build := entity.BuildDescription{ + Remote: "git@github:uber/tango", + BaseSha: "base", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://uber/tango/pull/1", Commit: firstCommit}, + {URL: "github://uber/tango/pull/2", Commit: secondCommit}, + }, + } + swapped := build + swapped.ChangeRequests = []entity.ChangeRequest{ + build.ChangeRequests[1], + build.ChangeRequests[0], + } + + require.NotEqual(t, GetTreehashCachePath(build), GetTreehashCachePath(swapped)) + }) +} diff --git a/internal/url/url.go b/internal/url/url.go index 6f790185..18fb1eaf 100644 --- a/internal/url/url.go +++ b/internal/url/url.go @@ -17,7 +17,9 @@ package url import ( "crypto/md5" "fmt" - "sort" + "hash" + "io" + "strconv" "strings" "github.com/uber/tango/entity" @@ -30,20 +32,22 @@ func ToShortRemote(remote string) string { return strs[len(strs)-1] } -// GetReqURLsHash returns a fixed-length MD5 hash of the sorted change request URLs. -// Each URL's bytes are fed into the digest individually (no separator) +// GetReqURLsHash returns a fixed-length hash of the ordered change requests. +// Both URL and commit participate because either can change the materialized tree. func GetReqURLsHash(requests []entity.ChangeRequest) string { if len(requests) == 0 { return "" } - urls := make([]string, 0, len(requests)) - for _, req := range requests { - urls = append(urls, req.URL) - } - sort.Strings(urls) h := md5.New() - for _, url := range urls { - h.Write([]byte(url)) + for _, request := range requests { + writeFramedString(h, request.URL) + writeFramedString(h, request.Commit) } return fmt.Sprintf("%x", h.Sum(nil)) } + +func writeFramedString(h hash.Hash, value string) { + io.WriteString(h, strconv.Itoa(len(value))) + h.Write([]byte{':'}) + io.WriteString(h, value) +} diff --git a/internal/url/url_test.go b/internal/url/url_test.go index de3be9c7..9e512d9b 100644 --- a/internal/url/url_test.go +++ b/internal/url/url_test.go @@ -55,10 +55,10 @@ func TestToShortRemote(t *testing.T) { func TestGetReqURLsHash(t *testing.T) { t.Parallel() - md5hex := func(strs ...string) string { + md5hex := func(requests ...entity.ChangeRequest) string { h := md5.New() - for _, s := range strs { - h.Write([]byte(s)) + for _, request := range requests { + fmt.Fprintf(h, "%d:%s%d:%s", len(request.URL), request.URL, len(request.Commit), request.Commit) } return fmt.Sprintf("%x", h.Sum(nil)) } @@ -74,18 +74,18 @@ func TestGetReqURLsHash(t *testing.T) { }, { name: "single", - in: []entity.ChangeRequest{{URL: "github://org/repo/pull/42"}}, - want: md5hex("github://org/repo/pull/42"), + in: []entity.ChangeRequest{{URL: "github://org/repo/pull/42", Commit: "abc"}}, + want: md5hex(entity.ChangeRequest{URL: "github://org/repo/pull/42", Commit: "abc"}), }, { name: "multiple", - in: []entity.ChangeRequest{{URL: "a"}, {URL: "b"}}, - want: md5hex("a", "b"), + in: []entity.ChangeRequest{{URL: "a", Commit: "1"}, {URL: "b", Commit: "2"}}, + want: md5hex(entity.ChangeRequest{URL: "a", Commit: "1"}, entity.ChangeRequest{URL: "b", Commit: "2"}), }, { - name: "multiple sorted", - in: []entity.ChangeRequest{{URL: "b"}, {URL: "a"}}, - want: md5hex("a", "b"), + name: "reverse order", + in: []entity.ChangeRequest{{URL: "b", Commit: "2"}, {URL: "a", Commit: "1"}}, + want: md5hex(entity.ChangeRequest{URL: "b", Commit: "2"}, entity.ChangeRequest{URL: "a", Commit: "1"}), }, } for _, tt := range tests {