diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index cefe698e..fa145c5e 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -20,6 +20,7 @@ import ( "fmt" "io" "maps" + "sync" "time" "github.com/uber/tango/core/cachekey" @@ -76,6 +77,95 @@ type job struct { completed bool ctx context.Context cancel context.CancelCauseFunc + revision *pb.BuildDescription +} + +// treehashResolver memoizes treehash reads within a single request. +// It avoids redundant storage reads by caching successfully resolved +// treehash values. Errors are not cached — they are returned to the +// caller so that transient failures remain retryable. +type treehashResolver struct { + storage storage.Storage + emitter *metrics.Emitter + op string + mu sync.Mutex + cache map[string]string +} + +// newTreehashResolver creates a resolver for a single GetChangedTargets request. +func newTreehashResolver(st storage.Storage, e *metrics.Emitter, op string) *treehashResolver { + return &treehashResolver{ + storage: st, + emitter: e, + op: op, + cache: make(map[string]string), + } +} + +// resolve resolves the treehash for a build description. +// It memoizes successful reads; errors are not cached. +func (r *treehashResolver) resolve(ctx context.Context, build *pb.BuildDescription) (string, error) { + entityBuild, err := mapper.ProtoToBuildDescription(build) + if err != nil { + return "", err + } + key := cachekey.GetTreehashCachePath(entityBuild) + + r.mu.Lock() + if val, ok := r.cache[key]; ok { + r.mu.Unlock() + return val, nil + } + r.mu.Unlock() + + value, err := readTreehash(ctx, r.storage, build, r.emitter, r.op) + if err != nil { + return "", err + } + + r.mu.Lock() + r.cache[key] = value + r.mu.Unlock() + return value, nil +} + +// resolveParallel resolves both treehashes concurrently. +// It memoizes successful reads; errors are not cached. +// Returns (treehash1, treehash2, error). +func (r *treehashResolver) resolveParallel(ctx context.Context, first, second *pb.BuildDescription) (string, string, error) { + type result struct { + idx int + hash string + err error + } + + descs := [2]*pb.BuildDescription{first, second} + results := make(chan result, 2) + + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + for i, desc := range descs { + go func(idx int, d *pb.BuildDescription) { + hash, err := r.resolve(ctx, d) + results <- result{idx: idx, hash: hash, err: err} + }(i, desc) + } + + var hashes [2]string + var firstErr error + for range descs { + res := <-results + hashes[res.idx] = res.hash + if res.err != nil && firstErr == nil { + firstErr = res.err + cancel() + } + } + if firstErr != nil { + return "", "", firstErr + } + return hashes[0], hashes[1], nil } // GetChangedTargets returns the changed targets between two revisions. If the @@ -113,19 +203,33 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str maxDist = request.GetOutputConfig().GetMaxDistance() } - // Fast path: stream a previously computed result straight from cache. - if !request.GetBypassCache() { - served, err := c.serveChangedTargetsFromCache(ctx, e, logger, request, stream, maxDist, start) - if err != nil { - return fmt.Errorf("serve from cache: %w", err) - } - if served { - return nil + // Create a request-scoped resolver to memoize treehash reads. + // This avoids redundant storage reads across the request pipeline. + resolver := newTreehashResolver(c.storage, e, opGetChangedTargets) + + // Read both treehashes concurrently to populate the resolver cache. + // This preserves the original concurrent behavior from readTreehashParallel. + treehash1, treehash2, err := resolver.resolveParallel(ctx, request.GetFirstRevision(), request.GetSecondRevision()) + if err != nil { + return fmt.Errorf("read revision treehash: %w", err) + } + if treehash1 == "" || treehash2 == "" { + // One or both treehashes missing; skip cache and recompute. + } else { + // Fast path: stream a previously computed result straight from cache. + if !request.GetBypassCache() { + served, err := c.serveChangedTargetsFromCache(ctx, e, logger, request, stream, maxDist, start, resolver, treehash1, treehash2) + if err != nil { + return fmt.Errorf("serve from cache: %w", err) + } + if served { + return nil + } } } // Fetch both revisions' target graphs concurrently. - firstGraph, secondGraph, err := c.fetchTargetGraphs(ctx, e, logger, request) + firstGraph, secondGraph, err := c.fetchTargetGraphs(ctx, e, logger, request, resolver) if err != nil { return fmt.Errorf("fetch target graphs: %w", err) } @@ -142,7 +246,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str } // Cache the computed result concurrently so it doesn't block the stream send. - c.cacheComparedTargets(logger, request, changedTargetsResponses) + c.cacheComparedTargets(logger, request, changedTargetsResponses, resolver) sendStart := time.Now() if err := sendTrimmedChangedTargets(stream, changedTargetsResponses, maxDist, request.GetOutputConfig()); err != nil { @@ -164,16 +268,9 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str // - (false, nil) on a cache miss or a corrupt blob — the caller should recompute; // - (false, err) on an infra failure or a client disconnect that aborts the request. // -// readTreehash returns ("", nil) on a cache miss (skip cache, recompute) but any -// real storage error surfaces here so an infra failure that disables the cache -// (e.g. a missing-deadline "missing TTL" reject) becomes a visible request failure -// rather than silent degradation. -func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer, maxDist int32, start time.Time) (bool, error) { +// treehash1 and treehash2 are pre-resolved treehash values (may be empty if cache miss). +func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer, maxDist int32, start time.Time, resolver *treehashResolver, treehash1, treehash2 string) (bool, error) { cacheStart := time.Now() - treehash1, treehash2, err := readTreehashParallel(ctx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), e, opGetChangedTargets) - if err != nil { - return false, fmt.Errorf("read revision treehash: %w", err) - } if treehash1 == "" || treehash2 == "" { return false, nil } @@ -241,13 +338,19 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metric // original failure is returned. A client disconnect surfaces as a user-cancelled // error. A graph stored as a TGB blob comes back as its undrained reader; a // gob-era graph is drained into chunks here, inside the concurrent fetch. -func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest) (fetchedGraph, fetchedGraph, error) { +func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, resolver *treehashResolver) (fetchedGraph, fetchedGraph, error) { jobs := make([]*job, 2) for i := 0; i < 2; i++ { // create independent contexts for each job; if one of the jobs fails, the other one should be cancelled to save resources and improve latency ctxNew, cancelNew := context.WithCancelCause(ctx) defer cancelNew(nil) - jobs[i] = &job{ctx: ctxNew, cancel: cancelNew} + var revision *pb.BuildDescription + if i == 0 { + revision = request.GetFirstRevision() + } else { + revision = request.GetSecondRevision() + } + jobs[i] = &job{ctx: ctxNew, cancel: cancelNew, revision: revision} } // Start jobs for both revisions. Success or failure, the result will report to the results channel. @@ -268,12 +371,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, results <- graphResult{order: idx, err: fmt.Errorf("panic in graph fetch: %v", r)} } }() - var revision *pb.BuildDescription - if idx == 0 { - revision = request.GetFirstRevision() - } else { - revision = request.GetSecondRevision() - } + revision := jobs[idx].revision entityBuild, err := mapper.ProtoToBuildDescription(revision) if err != nil { results <- graphResult{order: idx, err: fmt.Errorf("convert build description: %w", err)} @@ -284,7 +382,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, ExcludeFilesRegex: request.GetRequestOptions().GetExtraExcludeFilesRegex(), BypassCache: request.GetBypassCache(), } - graphReader, err := c.getGraph(jobs[idx].ctx, e, entityReq) + graphReader, err := c.getGraph(jobs[idx].ctx, e, entityReq, resolver, revision) if err != nil || graphReader == nil { results <- graphResult{order: idx, err: err} return @@ -376,7 +474,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, // a fire-and-forget goroutine so it does not block the stream send. The responses // is only read (never mutated) by the goroutine and the foreground send, so // concurrent access is safe; the caller must not mutate it. This is best effort. -func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetChangedTargetsRequest, responses []entity.GetChangedTargetsResponse) { +func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetChangedTargetsRequest, responses []entity.GetChangedTargetsResponse, resolver *treehashResolver) { go func() { // Use c.appCtx directly: the cache write is fire-and-forget and must // outlive the request (so a client disconnect doesn't abort it) but @@ -385,14 +483,14 @@ func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetCha // is cancelled on shutdown. Per-operation deadlines are the storage // backend's responsibility — the controller is backend-agnostic and // must not encode any one implementation's I/O budget. - // The treehash reads here are for building the write key, not a cache - // serve attempt, so they pass a no-op emitter to avoid skewing the - // treehash cache hit rate. - treehash1, treehash2, err := readTreehashParallel(c.appCtx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), metrics.Nop(), opGetChangedTargets) + // Use the resolver to get treehash values (memoized from request). + treehash1, err := resolver.resolve(c.appCtx, request.GetFirstRevision()) + if err != nil { + logger.Warn("GetChangedTargets: skipping cache write, failed to read revision treehash", zap.Error(err)) + return + } + treehash2, err := resolver.resolve(c.appCtx, request.GetSecondRevision()) if err != nil { - // Goroutine outlives the handler so we can't return; log loudly and - // abandon the cache write. Surfacing infra failures matters more than - // a missed cache opportunity. logger.Warn("GetChangedTargets: skipping cache write, failed to read revision treehash", zap.Error(err)) return } diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 19c74108..e5d61af2 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -358,13 +358,14 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { storagemock := storagemock.NewMockStorage(ctrl) // A non-NotFound storage error on a treehash read must surface as a failed - // request rather than be silently treated as a cache miss. Both revision - // treehashes are read in parallel, so two Get calls happen; the handler - // returns the first failure (and drops the cancelled sibling's error) - // before any graph fetch happens. + // request rather than be silently treated as a cache miss. The resolver + // reads treehashes concurrently, so both reads are attempted; the first + // error is returned and the sibling is cancelled. injected := errors.New("storage exploded") + // Both reads are attempted concurrently; one will fail and the other will be cancelled. + // The exact number of calls depends on scheduling, so we accept at least 1 call. storagemock.EXPECT().Get(gomock.Any(), gomock.Any()). - Return(storage.DownloadResponse{}, injected).Times(2) + Return(storage.DownloadResponse{}, injected).MinTimes(1) c := NewController(context.Background(), Params{ Logger: zap.NewNop(), @@ -538,8 +539,8 @@ func TestGetChangedTargets_streamChunks(t *testing.T) { default: return storage.DownloadResponse{}, fmt.Errorf("unexpected key: %s", req.Key) } - // readTreehash (×2 pre) + comparison cache miss (×1) + graph computation (×4) + readTreehash (×2 post) = 9 - }).Times(9) + // readTreehash (×2 pre) + comparison cache miss (×1) + graph computation (×2) = 5 + }).Times(5) // Put is launched in a goroutine — use a channel to wait for it before the test ends. putDone := make(chan struct{}, 1) storagemock.EXPECT().Put(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, _ storage.UploadRequest) error { @@ -1400,15 +1401,16 @@ func TestServeChangedTargetsFromCache(t *testing.T) { t.Run("cache miss returns not-served, no error", func(t *testing.T) { ctrl := gomock.NewController(t) st := storagemock.NewMockStorage(ctrl) - // Both treehash reads miss, so the cache path is skipped entirely. + // Cache miss for compared-targets, so the cache path is skipped entirely. st.EXPECT().Get(gomock.Any(), gomock.Any()). - Return(storage.DownloadResponse{}, storage.NewNotFoundError("missing")).Times(2) + Return(storage.DownloadResponse{}, storage.NewNotFoundError("missing")).Times(1) c := newTestController(zaptest.NewLogger(t)) c.storage = st stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) + resolver := newTreehashResolver(c.storage, c.emitter, opGetChangedTargets) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now(), resolver, "treehash1", "treehash2") require.NoError(t, err) assert.False(t, served, "a cache miss must not be served") }) @@ -1445,9 +1447,10 @@ func TestServeChangedTargetsFromCache(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.storage = st stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) + resolver := newTreehashResolver(c.storage, c.emitter, opGetChangedTargets) // No Send expectation: a corrupt blob must not send anything to the client. - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now(), resolver, "", "") require.NoError(t, err) assert.False(t, served, "a corrupt blob must trigger recompute, not a partial send") }) @@ -1480,8 +1483,9 @@ func TestServeChangedTargetsFromCache(t *testing.T) { c.storage = st stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Send(gomock.Any()).Return(nil).Times(2) + resolver := newTreehashResolver(c.storage, c.emitter, opGetChangedTargets) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now(), resolver, "treehash1", "treehash2") require.NoError(t, err) assert.True(t, served, "a clean cache hit must be served") }) @@ -1507,8 +1511,9 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch + resolver := newTreehashResolver(storage.NewMemoryStorage(), c.emitter, opGetChangedTargets) - first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), resolver) require.NoError(t, err) require.Len(t, first.chunks, 1) require.Len(t, second.chunks, 1) @@ -1531,8 +1536,9 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch + resolver := newTreehashResolver(storage.NewMemoryStorage(), c.emitter, opGetChangedTargets) - first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), resolver) require.Error(t, err) assert.ErrorIs(t, err, injected) assert.Zero(t, first) @@ -1552,8 +1558,9 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch + resolver := newTreehashResolver(storage.NewMemoryStorage(), c.emitter, opGetChangedTargets) - _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), resolver) require.Error(t, err) }) @@ -1567,8 +1574,9 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch + resolver := newTreehashResolver(storage.NewMemoryStorage(), c.emitter, opGetChangedTargets) - _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), resolver) require.Error(t, err) }) } @@ -1685,3 +1693,123 @@ func TestSeedAttributesFor(t *testing.T) { assert.Equal(t, map[string]bool{"size": true, "timeout": true}, c.seedAttributesFor("some-remote")) }) } + +// TestGetChangedTargets_TreehashReadCount verifies that treehashes are read +// exactly once per revision during a GetChangedTargets request, eliminating +// redundant storage reads that previously occurred in three separate code paths: +// 1. serveChangedTargetsFromCache (compared-targets cache lookup) +// 2. getGraph (individual graph cache lookup) +// 3. cacheComparedTargets (background cache write) +func TestGetChangedTargets_TreehashReadCount(t *testing.T) { + ctrl := gomock.NewController(t) + stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) + stream.EXPECT().Context().Return(t.Context()) + + var sentResponses []*pb.GetChangedTargetsResponse + stream.EXPECT().Send(gomock.Any()).DoAndReturn(func(resp *pb.GetChangedTargetsResponse, opts ...interface{}) error { + sentResponses = append(sentResponses, resp) + return nil + }).Times(2) + + storagemock := storagemock.NewMockStorage(ctrl) + + // Build first revision graph (2 chunks: Targets + Metadata) + var buf1 bytes.Buffer + enc1 := gob.NewEncoder(&buf1) + enc1.Encode(entity.GetTargetGraphResponse{ + Targets: []entity.OptimizedTarget{ + {ID: 1, Hash: "h1", RuleType: 100}, + {ID: 2, Hash: "h2-old", RuleType: 300}, + }, + }) + enc1.Encode(entity.GetTargetGraphResponse{ + Metadata: &entity.Metadata{ + TargetIDMapping: map[int32]string{1: "//app:target1", 2: "//app:target2"}, + RuleTypeMapping: map[int32]string{100: "go_library", 300: "source file"}, + }, + }) + graph1Bytes := buf1.Bytes() + + // Build second revision graph - target2 has different hash + var buf2 bytes.Buffer + enc2 := gob.NewEncoder(&buf2) + enc2.Encode(entity.GetTargetGraphResponse{ + Targets: []entity.OptimizedTarget{ + {ID: 1, Hash: "h1", RuleType: 100}, + {ID: 2, Hash: "h2-new", RuleType: 300}, // changed hash + }, + }) + enc2.Encode(entity.GetTargetGraphResponse{ + Metadata: &entity.Metadata{ + TargetIDMapping: map[int32]string{1: "//app:target1", 2: "//app:target2"}, + RuleTypeMapping: map[int32]string{100: "go_library", 300: "source file"}, + }, + }) + graph2Bytes := buf2.Bytes() + + // Expected Get calls after optimization: + // - 2 treehash reads (READ A in serveChangedTargetsFromCache) + // - 1 compared-targets cache miss + // - 2 graph reads (using pre-read treehashes, no additional treehash reads) + // Total: 5 storage.Get calls + treehashReadCount := 0 + graphReadCount := 0 + comparedTargetsReadCount := 0 + + storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, req storage.DownloadRequest) (storage.DownloadResponse, error) { + switch { + case strings.Contains(req.Key, "compared-targets"): + comparedTargetsReadCount++ + return storage.DownloadResponse{}, storage.NewNotFoundError(req.Key) + case strings.Contains(req.Key, "treehashes"): + treehashReadCount++ + if strings.Contains(req.Key, "sha1") { + return storage.DownloadResponse{ReadCloser: io.NopCloser(bytes.NewReader([]byte("treehash1")))}, nil + } + return storage.DownloadResponse{ReadCloser: io.NopCloser(bytes.NewReader([]byte("treehash2")))}, nil + case strings.Contains(req.Key, "treehash1"): + graphReadCount++ + return storage.DownloadResponse{ReadCloser: io.NopCloser(bytes.NewReader(graph1Bytes))}, nil + case strings.Contains(req.Key, "treehash2"): + graphReadCount++ + return storage.DownloadResponse{ReadCloser: io.NopCloser(bytes.NewReader(graph2Bytes))}, nil + default: + return storage.DownloadResponse{}, fmt.Errorf("unexpected key: %s", req.Key) + } + }).AnyTimes() + + // Put is launched in a goroutine — use a channel to wait for it before the test ends. + putDone := make(chan struct{}, 1) + storagemock.EXPECT().Put(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, _ storage.UploadRequest) error { + putDone <- struct{}{} + return nil + }) + + c := NewController(context.Background(), Params{ + Logger: zaptest.NewLogger(t), + Storage: storagemock, + Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), + }) + + request := &pb.GetChangedTargetsRequest{ + FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, + SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, + OutputConfig: &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true}, + } + + err := c.GetChangedTargets(request, stream) + require.NoError(t, err) + + select { + case <-putDone: + case <-time.After(time.Second): + assert.Fail(t, "cache write goroutine did not complete in time") + } + + // Before optimization: treehashReadCount == 6 (3 per revision) + // After optimization: treehashReadCount == 2 (1 per revision, shared) + assert.Equal(t, 2, treehashReadCount, "treehash should be read exactly once per revision") + assert.Equal(t, 2, graphReadCount, "graph should be read once per revision") + assert.Equal(t, 1, comparedTargetsReadCount, "compared-targets cache consulted once") +} diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index e59ce2ea..ec6ac553 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -56,7 +56,7 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb if err != nil { return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", err)) } - graphReader, err := c.getGraph(ctx, e, entityReq) + graphReader, err := c.getGraph(ctx, e, entityReq, nil, request.GetBuildDescription()) if err != nil { return fmt.Errorf("get graph: %w", err) } @@ -96,34 +96,52 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb // entries store the full payload and stripping happens at send time, so // letting an orchestrator see it could poison the shared cache with // stripped graphs. -func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entity.GetTargetGraphRequest) (storage.GraphReader, error) { +// If resolver is provided, it is used to resolve the treehash for the build description. +func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entity.GetTargetGraphRequest, resolver *treehashResolver, revision *pb.BuildDescription) (storage.GraphReader, error) { start := time.Now() logger := c.logger.With( zap.Any("build_description", req.Build), ) if !req.BypassCache { - // Look up the the git treehash based on cache path - treehashCachePath := cachekey.GetTreehashCachePath(req.Build) - treehashResponse, err := c.storage.Get(ctx, storage.DownloadRequest{Key: treehashCachePath}) - metrics.RecordCacheLookup(e, opGetTargetGraph, metrics.TreehashCacheLookup, err) - if err != nil { - if storage.IsNotFound(err) { - // Cache miss - blob doesn't exist, need to compute and store target graph - logger.Debug("getGraph: treehash not found", zap.Error(err)) - } else { - // Other errors (network, infra issues) should be retried + var treehashValue string + if resolver != nil { + // Use the resolver to memoize treehash reads. + var err error + treehashValue, err = resolver.resolve(ctx, revision) + if err != nil { return nil, fmt.Errorf("get treehash: %w", err) } + if treehashValue != "" { + logger.Debug("getGraph: using pre-read treehash") + } } else { - defer func() { _ = treehashResponse.ReadCloser.Close() }() - treehashBytes, err := io.ReadAll(treehashResponse.ReadCloser) + // Look up the git treehash based on cache path + treehashCachePath := cachekey.GetTreehashCachePath(req.Build) + treehashResponse, err := c.storage.Get(ctx, storage.DownloadRequest{Key: treehashCachePath}) + metrics.RecordCacheLookup(e, opGetTargetGraph, metrics.TreehashCacheLookup, err) if err != nil { - return nil, fmt.Errorf("read treehash: %w", err) + if storage.IsNotFound(err) { + // Cache miss - blob doesn't exist, need to compute and store target graph + logger.Debug("getGraph: treehash not found", zap.Error(err)) + } else { + // Other errors (network, infra issues) should be retried + return nil, fmt.Errorf("get treehash: %w", err) + } + } else { + defer func() { _ = treehashResponse.ReadCloser.Close() }() + treehashBytes, err := io.ReadAll(treehashResponse.ReadCloser) + if err != nil { + return nil, fmt.Errorf("read treehash: %w", err) + } + treehashValue = string(treehashBytes) + logger.Info("getGraph: treehash found") } - logger.Info("getGraph: treehash found") + } + + if treehashValue != "" { // Download the target graph based on treehash. storageStart := time.Now() - graphReader, err := c.readCachedGraph(ctx, logger, req.Build.Remote, string(treehashBytes), req.Build.Strategy, req.ExcludeFilesRegex) + graphReader, err := c.readCachedGraph(ctx, logger, req.Build.Remote, treehashValue, req.Build.Strategy, req.ExcludeFilesRegex) if ctx.Err() != nil { err = context.Cause(ctx) }