Allow clients to opt in to caching AC results in the proxy via a platform property by iain-macdonald · Pull Request #12563 · buildbuddy-io/buildbuddy

iain-macdonald · 2026-06-25T20:03:17Z

Fixes: https://github.com/buildbuddy-io/buildbuddy-internal/issues/7611

…form property

buildbuddy-io

PR lets clients opt into having the action-cache proxy cache AC results locally via the cache-action-results-in-proxy platform property, falling through to a hardcoded 15-minute default for actionCacheTTL. The change is small and well-contained, with the BUILD dependency, import, IsTrue/RemoteHeaderOverrides usage, and precedence ordering all correct.

Additional findings (lines outside the diff)

enterprise/server/remote_execution/platform/platform.go:112 — Stray blank line inside the const block, just below the new property name.

buildbuddy-io · 2026-06-25T20:55:41Z

+
+	// Clients may opt into this caching via a platform property as well.
+	for _, prop := range platform.RemoteHeaderOverrides(ctx) {
+		if strings.EqualFold(prop.GetName(), platform.CacheActionResultsInProxyPropertyName) && platform.IsTrue(prop.GetValue()) {


Per-request opt-in makes the local-AC TTL asymmetric between writes and reads, which can break the REv2 consistency guarantee the code claims (lines 393-395) and serve stale results.

With the old experiment-flag mechanism, actionCacheTTL returned a non-zero TTL uniformly for every request from a group, so every UpdateActionResult refreshed the local AC entry (line 397: ttl > 0) and reads always saw the latest write. The new path derives the TTL from a per-request header, so writes and reads can disagree about whether caching is active:

Invocation A (header set) writes result v1 → remote updated, local entry cached with fresh mtime, TTL 15 min.

Invocation B (header not set) writes v2 to the same proxy → ttl == 0, so line 397 skips the local update; remote now holds v2 but the local entry still points at v1.

Invocation A (header set) reads within 15 min → ttlFastPathEligible && isLocalActionResultFresh is true (line 287), so it serves the stale v1 without revalidating against the remote.

Because --remote_header is set per-invocation, mixed opt-in across clients/invocations sharing one proxy is a realistic configuration, and the comment at lines 393-395 ("ensures the REv2 guarantee that GetActionResult serves the most recent UpdateActionResult for all requests to the same endpoint") no longer holds for that mix. Worth either gating the read fast-path on a consistent signal or updating the comment to scope the guarantee to clients that opt in uniformly.

Yeah, agreed, don't think I'd like to fix that at this time though.

This actually seems OK to me. The two invocations live in slightly different worlds. That said, it seems like it would be any easy fix: always write the local action in UpdateActionResult. It will just be ignored most of the time, and shouldn't take up much space in the cache.

iain-macdonald · 2026-06-26T17:48:42Z

Converting this to draft because I realized that it will probably break the cache scorecard (as the current experiment does) because the remote hit tracker doesn't report locally-served AC hits, I think.

iain-macdonald · 2026-06-26T21:10:56Z

Converting this to draft because I realized that it will probably break the cache scorecard (as the current experiment does) because the remote hit tracker doesn't report locally-served AC hits, I think.

Never mind:

buildbuddy/enterprise/server/action_cache_server_proxy/action_cache_server_proxy.go

Lines 274 to 289 in 8b375fb

    
           if ttlFastPathEligible && s.isLocalActionResultFresh(localACMD, ttl) { 
        
           	// Skip checking for existence of output files. The app recently 
        
           	// validated or updated this result, which refreshed the referenced 
        
           	// outputs' atime. With remote_download_minimal, this proxy 
        
           	// often won't have every output locally. 
        
           	sizeBytes := int64(proto.Size(localResult)) 
        
           	s.recordLocalACHit(ctx, req, localResult, sizeBytes) 
        
           	labels := prometheus.Labels{ 
        
           		metrics.StatusLabel:           status.MetricsLabel(nil), 
        
           		metrics.CacheHitMissStatus:    metrics.HitStatusLabel, 
        
           		metrics.CacheProxyRequestType: metrics.DefaultCacheProxyRequestLabel, 
        
           	} 
        
           	metrics.ActionCacheProxiedReadRequests.With(labels).Inc() 
        
           	metrics.ActionCacheProxiedReadBytes.With(labels).Add(float64(sizeBytes)) 
        
           	return localResult, nil 
        
           }

vanja-p · 2026-06-28T01:28:24Z

-		return 0
+
+	// Clients may opt into this caching via a platform property as well.
+	for _, prop := range platform.RemoteHeaderOverrides(ctx) {


does RemoteHeaderOverrides survive multiple cache proxy hops?

vanja-p · 2026-06-28T01:30:34Z

-	if efp == nil {
-		return 0
+	if efp != nil {
+		ttlSeconds := efp.Int64(ctx, "cache_proxy.action_cache_ttl_seconds", 0)


maybe we should allow experiments to turn off platform overrides by setting a negative value?

vanja-p · 2026-06-28T01:31:32Z

 	RetryPropertyName                        = "retry"
 	PersistentVolumesPropertyName            = "persistent-volumes"
 	execrootPathPropertyName                 = "execroot-path"
+	CacheActionResultsInProxyPropertyName    = "cache-action-results-in-proxy"


this could use a doc to document the behavior and supported values.

vanja-p · 2026-06-28T01:32:11Z

+	// Clients may opt into this caching via a platform property as well.
+	for _, prop := range platform.RemoteHeaderOverrides(ctx) {
+		if strings.EqualFold(prop.GetName(), platform.CacheActionResultsInProxyPropertyName) && platform.IsTrue(prop.GetValue()) {
+			return clientActionCacheProxyTTLDefault


If we're letting the client configure this, seems like we might as well let them configure the TTL as the header value, or default to 15min if they don't set a value?

vanja-p · 2026-06-28T01:36:00Z

+
+	// Clients may opt into this caching via a platform property as well.
+	for _, prop := range platform.RemoteHeaderOverrides(ctx) {
+		if strings.EqualFold(prop.GetName(), platform.CacheActionResultsInProxyPropertyName) && platform.IsTrue(prop.GetValue()) {


This actually seems OK to me. The two invocations live in slightly different worlds. That said, it seems like it would be any easy fix: always write the local action in UpdateActionResult. It will just be ignored most of the time, and shouldn't take up much space in the cache.

Allow clients to opt in to caching AC results in the proxy via a plat…

545b5ae

…form property

iain-macdonald force-pushed the im/275fb7ee5fb branch from 6b7723b to 545b5ae Compare June 25, 2026 20:42

iain-macdonald marked this pull request as ready for review June 25, 2026 20:53

buildbuddy-io Bot reviewed Jun 25, 2026

View reviewed changes

iain-macdonald requested a review from vanja-p June 25, 2026 21:27

iain-macdonald marked this pull request as draft June 26, 2026 17:48

iain-macdonald marked this pull request as ready for review June 26, 2026 21:10

vanja-p reviewed Jun 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow clients to opt in to caching AC results in the proxy via a platform property#12563

Allow clients to opt in to caching AC results in the proxy via a platform property#12563
iain-macdonald wants to merge 1 commit into
masterfrom
im/275fb7ee5fb

iain-macdonald commented Jun 25, 2026 •

edited

Loading

Uh oh!

buildbuddy-io Bot left a comment

Uh oh!

buildbuddy-io Bot Jun 25, 2026

Uh oh!

iain-macdonald Jun 25, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

iain-macdonald commented Jun 26, 2026

Uh oh!

iain-macdonald commented Jun 26, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

vanja-p Jun 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iain-macdonald commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buildbuddy-io Bot left a comment

Choose a reason for hiding this comment

Additional findings (lines outside the diff)

Uh oh!

buildbuddy-io Bot Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

iain-macdonald Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

iain-macdonald commented Jun 26, 2026

Uh oh!

iain-macdonald commented Jun 26, 2026

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

vanja-p Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iain-macdonald commented Jun 25, 2026 •

edited

Loading