From 6afba079e69f32151b031dc2d56dacafbfd4da55 Mon Sep 17 00:00:00 2001 From: Andrzej J Skalski Date: Wed, 3 Jun 2026 13:48:59 +0200 Subject: [PATCH 1/5] Exclude PassUnsafeEnv values from remote action digest Make PassUnsafeEnv behave on remote execution like it does for the local cache: the real values are still passed to the executed action, but they no longer contribute to the remote action digest, so changing them does not cause remote cache misses. This generalizes the existing stamped/unstamped decoupling: results are looked up and stored under a "cache-key" action digest that omits the volatile values, while the real action (with the values present) is executed and then backfilled into the cache under the cache-key digest. Controlled by the new Remote.ExcludePassUnsafeEnvVarsFromDigest config option, which defaults to true. --- ChangeLog | 4 +++ docs/config.html | 15 +++++++++ src/core/config.go | 38 +++++++++++----------- src/remote/action.go | 53 ++++++++++++++++++++++++++----- src/remote/remote.go | 66 +++++++++++++++++++++++++++++---------- src/remote/remote_test.go | 66 +++++++++++++++++++++++++++++++++++---- 6 files changed, 193 insertions(+), 49 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4182d8031f..78a251b9e0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,10 @@ Version 17.30.0 * Make `git_*` built-in function failures more informative (#3525) * Fix go coverage when parsing blocks out of order (#3519) * Update to golang 1.26 (#3498) + * Exclude `PassUnsafeEnv` values from the remote action digest by default, so + changing them no longer causes remote cache misses (matching local cache + behaviour). Controlled by the new `Remote.ExcludePassUnsafeEnvVarsFromDigest` + config option. Version 17.29.1 --------------- diff --git a/docs/config.html b/docs/config.html index c7f8cb8b92..80540398c0 100644 --- a/docs/config.html +++ b/docs/config.html @@ -701,6 +701,15 @@

PassEnv, environment variable values are not used to calculate build target hashes.

+

+ When building remotely, the + Remote.ExcludePassUnsafeEnvVarsFromDigest + option (enabled by default) likewise keeps these values out of the + remote action cache key, so changing them does not cause remote cache + misses. The values are still passed to the executed action. Note that a + cached result may therefore be reused (and shared between users) even if + the PassUnsafeEnv values differ from those it was originally built with. +

  • @@ -1016,6 +1025,12 @@

    VerifyOutputs {{ index .ConfigHelpText "remote.verifyoutputs" }}

    • +
  • +
    +

    ExcludePassUnsafeEnvVarsFromDigest (bool)

    +

    {{ index .ConfigHelpText "remote.excludepassunsafeenvvarsfromdigest" }}

    +
    +

  • UploadDirs (bool)

    diff --git a/src/core/config.go b/src/core/config.go index a54d97b6ca..68fd06f687 100644 --- a/src/core/config.go +++ b/src/core/config.go @@ -409,6 +409,7 @@ func DefaultConfiguration() *Configuration { config.Remote.NumExecutors = 20 // kind of arbitrary config.Remote.Secure = true config.Remote.VerifyOutputs = true + config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true config.Remote.UploadDirs = true config.Remote.CacheDuration = cli.Duration(10000 * 24 * time.Hour) // Effectively forever. config.Remote.Shell = "bash" @@ -518,7 +519,7 @@ type Configuration struct { Xattrs bool `help:"True (the default) to attempt to use xattrs to record file metadata. If false Please will fall back to using additional files where needed, which is more compatible but has slightly worse performance."` Nonce string `help:"This is an arbitrary string that is added to the hash of every build target. It provides a way to force a rebuild of everything when it's changed.\nWe will bump the default of this whenever we think it's required - although it's been a pretty long time now and we hope that'll continue."` PassEnv []string `help:"A list of environment variables to pass from the current environment to build rules. For example\n\nPassEnv = HTTP_PROXY\n\nwould copy your HTTP_PROXY environment variable to the build env for any rules."` - PassUnsafeEnv []string `help:"Similar to PassEnv, a list of environment variables to pass from the current environment to build rules. Unlike PassEnv, the environment variable values are not used when calculating build target hashes."` + PassUnsafeEnv []string `help:"Similar to PassEnv, a list of environment variables to pass from the current environment to build rules. Unlike PassEnv, the environment variable values are not used when calculating build target hashes. For remote execution this is controlled by Remote.ExcludePassUnsafeEnvVarsFromDigest, which by default also keeps these values out of the remote action cache key."` HTTPProxy cli.URL `help:"A URL to use as a proxy server for downloads. Only applies to internal ones - e.g. self-updates or remote_file rules."` HashCheckers []string `help:"Set of hash algos supported by the 'hashes' argument on build rules. Defaults to: sha1,sha256,blake3." options:"sha1,sha256,blake3,xxhash,crc32,crc64"` HashFunction string `help:"The hash function to use internally for build actions." options:"sha1,sha256,blake3,xxhash,crc32,crc64"` @@ -562,23 +563,24 @@ type Configuration struct { ExcludeableTargets []BuildLabel `help:"If set, only targets that match these wildcards will be allowed to opt out of the sandbox"` } `help:"A config section describing settings relating to sandboxing of build actions."` Remote struct { - URL string `help:"URL for the remote server."` - CASURL string `help:"URL for the CAS service, if it is different to the main one."` - AssetURL string `help:"URL for the remote asset server, if it is different to the main one."` - NumExecutors int `help:"Maximum number of remote executors to use simultaneously."` - Instance string `help:"Remote instance name to request; depending on the server this may be required."` - Name string `help:"A name for this worker instance. This is attached to artifacts uploaded to remote storage." example:"agent-001"` - DisplayURL string `help:"A URL to browse the remote server with (e.g. using buildbarn-browser). Only used when printing hashes."` - TokenFile string `help:"A file containing a token that is attached to outgoing RPCs to authenticate them. This is somewhat bespoke; we are still investigating further options for authentication."` - Timeout cli.Duration `help:"Timeout for connections made to the remote server."` - Secure bool `help:"Whether to use TLS for communication or not."` - VerifyOutputs bool `help:"Whether to verify all outputs are present after a cached remote execution action. Depending on your server implementation, you may require this to ensure files are really present."` - UploadDirs bool `help:"Uploads individual directory blobs after build actions. This might not be necessary with some servers, but if you aren't sure, you should leave it on."` - OptionalOutputsRequired bool `help:"Requires that any optional outputs of build actions (optional test outputs, coverage when not opted out of) are produced. By default this is a non-fatal failure, but the actions may not cache remotely."` - Shell string `help:"Path to the shell to use to execute actions in. Default is 'bash' which will be looked up by the server."` - Platform []string `help:"Platform properties to request from remote workers, in the format key=value."` - CacheDuration cli.Duration `help:"Length of time before we re-check locally cached build actions. Default is unlimited."` - BuildID string `help:"ID of the build action that's being run, to attach to remote requests. If not set then one is automatically generated."` + URL string `help:"URL for the remote server."` + CASURL string `help:"URL for the CAS service, if it is different to the main one."` + AssetURL string `help:"URL for the remote asset server, if it is different to the main one."` + NumExecutors int `help:"Maximum number of remote executors to use simultaneously."` + Instance string `help:"Remote instance name to request; depending on the server this may be required."` + Name string `help:"A name for this worker instance. This is attached to artifacts uploaded to remote storage." example:"agent-001"` + DisplayURL string `help:"A URL to browse the remote server with (e.g. using buildbarn-browser). Only used when printing hashes."` + TokenFile string `help:"A file containing a token that is attached to outgoing RPCs to authenticate them. This is somewhat bespoke; we are still investigating further options for authentication."` + Timeout cli.Duration `help:"Timeout for connections made to the remote server."` + Secure bool `help:"Whether to use TLS for communication or not."` + VerifyOutputs bool `help:"Whether to verify all outputs are present after a cached remote execution action. Depending on your server implementation, you may require this to ensure files are really present."` + ExcludePassUnsafeEnvVarsFromDigest bool `help:"Whether to exclude the values of PassUnsafeEnv environment variables from the remote action digest. When true (the default) PassUnsafeEnv values are still passed to the executed action but do not contribute to the cache key, so changing them does not cause remote cache misses (matching local cache behaviour). Note that this means a cached result may be reused (and shared between users) even if the PassUnsafeEnv values differ from those it was built with."` + UploadDirs bool `help:"Uploads individual directory blobs after build actions. This might not be necessary with some servers, but if you aren't sure, you should leave it on."` + OptionalOutputsRequired bool `help:"Requires that any optional outputs of build actions (optional test outputs, coverage when not opted out of) are produced. By default this is a non-fatal failure, but the actions may not cache remotely."` + Shell string `help:"Path to the shell to use to execute actions in. Default is 'bash' which will be looked up by the server."` + Platform []string `help:"Platform properties to request from remote workers, in the format key=value."` + CacheDuration cli.Duration `help:"Length of time before we re-check locally cached build actions. Default is unlimited."` + BuildID string `help:"ID of the build action that's being run, to attach to remote requests. If not set then one is automatically generated."` } `help:"Settings related to remote execution & caching using the Google remote execution APIs. This section is still experimental and subject to change."` Size map[string]*Size `help:"Named sizes of targets; these are the definitions of what can be passed to the 'size' argument."` Cover struct { diff --git a/src/remote/action.go b/src/remote/action.go index 2098ec48c3..ee313d4846 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -40,7 +40,7 @@ func (c *Client) uploadAction(target *core.BuildTarget, isTest, isRun bool, run } inputRootEntry, inputRootDigest := c.protoEntry(inputRoot) ch <- inputRootEntry - command, err = c.buildCommand(target, inputRoot, isTest, isRun, target.Stamp, run) + command, err = c.buildCommand(target, inputRoot, isTest, isRun, target.Stamp, false, run) if err != nil { return err } @@ -60,13 +60,14 @@ func (c *Client) uploadAction(target *core.BuildTarget, isTest, isRun bool, run } // buildAction creates a build action for a target and returns the command and the action digest. No uploading is done. -func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp bool, run int) (*pb.Command, *pb.Digest, error) { +// If canonical is true the values of PassUnsafeEnv variables are excluded from the digest (see buildCommand). +func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp, canonical bool, run int) (*pb.Command, *pb.Digest, error) { inputRoot, err := c.uploadInputs(nil, target, isTest) if err != nil { return nil, nil, err } inputRootDigest := c.digestMessage(inputRoot) - command, err := c.buildCommand(target, inputRoot, isTest, false, stamp, run) + command, err := c.buildCommand(target, inputRoot, isTest, false, stamp, canonical, run) if err != nil { return nil, nil, err } @@ -81,10 +82,13 @@ func (c *Client) buildAction(target *core.BuildTarget, isTest, stamp bool, run i } // buildCommand builds the command for a single target. -func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, isTest, isRun, stamp bool, run int) (*pb.Command, error) { +// If canonical is true, the values of PassUnsafeEnv variables are stripped from the environment so that +// they do not contribute to the action digest; this is used to compute a stable cache-key action that is +// never actually executed (see Client.build). +func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, isTest, isRun, stamp, canonical bool, run int) (*pb.Command, error) { state := c.state.ForTarget(target) if isTest { - return c.buildTestCommand(state, target, run) + return c.buildTestCommand(state, target, canonical, run) } else if isRun { return c.buildRunCommand(state, target) } @@ -126,14 +130,43 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, cmd = "true" } cmd, err := core.ReplaceSequences(state, target, cmd) + env := c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun) + if canonical { + c.stripUnsafeEnv(target, env) + } return &pb.Command{ Platform: c.targetPlatformProperties(target), Arguments: process.BashCommand(c.shellPath, commandPrefixBuilder.String()+cmd, state.Config.Build.ExitOnError), - EnvironmentVariables: c.buildEnv(target, c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun), target.Sandbox), + EnvironmentVariables: c.buildEnv(target, env, target.Sandbox), OutputPaths: outs, }, err } +// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this target. +func (c *Client) excludesUnsafeEnv(target *core.BuildTarget) bool { + return c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest && target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0 +} + +// stripUnsafeEnv removes the values of PassUnsafeEnv variables from the given environment so that they do +// not contribute to the action digest. Variables that are also listed in PassEnv are left intact, since +// those values are intentionally part of the cache key. +func (c *Client) stripUnsafeEnv(target *core.BuildTarget, env core.BuildEnv) { + if !c.excludesUnsafeEnv(target) { + return + } + safe := map[string]bool{} + if target.PassEnv != nil { + for _, e := range *target.PassEnv { + safe[e] = true + } + } + for _, e := range *target.PassUnsafeEnv { + if !safe[e] { + delete(env, e) + } + } +} + // stampedBuildEnvironment returns a build environment, optionally with a stamp if stamp is true. func (c *Client) stampedBuildEnvironment(state *core.BuildState, target *core.BuildTarget, inputRoot *pb.Directory, stamp, isRuntime bool) core.BuildEnv { if target.IsFilegroup { @@ -146,7 +179,7 @@ func (c *Client) stampedBuildEnvironment(state *core.BuildState, target *core.Bu } // buildTestCommand builds a command for a target when testing. -func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarget, run int) (*pb.Command, error) { +func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarget, canonical bool, run int) (*pb.Command, error) { paths := target.Test.Outputs if target.NeedCoverage(state) { paths = append(paths, core.CoverageFile) @@ -159,6 +192,10 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg commandPrefix += `export TEST="$TEST_DIR/` + outs[0] + `" && ` } cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state)) + env := core.TestEnvironment(state, target, ".", run) + if canonical { + c.stripUnsafeEnv(target, env) + } return &pb.Command{ Platform: &pb.Platform{ Properties: []*pb.Platform_Property{ @@ -169,7 +206,7 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg }, }, Arguments: process.BashCommand(c.shellPath, commandPrefix+cmd, state.Config.Build.ExitOnError), - EnvironmentVariables: c.buildEnv(nil, core.TestEnvironment(state, target, ".", run), target.Test.Sandbox), + EnvironmentVariables: c.buildEnv(nil, env, target.Test.Sandbox), OutputPaths: paths, }, err } diff --git a/src/remote/remote.go b/src/remote/remote.go index ee7ec220df..2767c1f994 100644 --- a/src/remote/remote.go +++ b/src/remote/remote.go @@ -412,36 +412,40 @@ func (c *Client) Run(target *core.BuildTarget) error { // build implements the actual build of a target. func (c *Client) build(target *core.BuildTarget) (*core.BuildMetadata, *pb.ActionResult, *pb.Digest, error) { needStdout := target.PostBuildFunction != nil - // If we're gonna stamp the target, first check the unstamped equivalent that we store results under. - // This implements the rules of stamp whereby we don't force rebuilds every time e.g. the SCM revision changes. - var unstampedDigest *pb.Digest - if target.Stamp { - command, digest, err := c.buildAction(target, false, false, 0) + // Some targets carry volatile inputs that we deliberately keep out of the cache key: the stamp + // (e.g. the SCM revision) and the values of PassUnsafeEnv variables (when configured). For these we + // store & look up results under a "cache-key" action digest that omits those inputs, while still + // executing the real action that includes them. This implements the rule whereby e.g. the SCM + // revision or an unsafe env var changing doesn't force a rebuild. + useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(target) + var cacheKeyDigest *pb.Digest + if useCacheKeyDigest { + command, digest, err := c.buildAction(target, false, false, true, 0) if err != nil { return nil, nil, nil, err } else if metadata, ar := c.maybeRetrieveResults(target, command, digest, false, needStdout, 0); metadata != nil { c.unstampedBuildActionDigests.Put(target.Label, digest) return metadata, ar, digest, nil } - unstampedDigest = digest + cacheKeyDigest = digest } - command, stampedDigest, err := c.buildAction(target, false, true, 0) + command, stampedDigest, err := c.buildAction(target, false, true, false, 0) if err != nil { return nil, nil, nil, err } metadata, ar, err := c.execute(target, command, stampedDigest, false, needStdout, 0) - if target.Stamp && err == nil { - err = c.verifyActionResult(target, command, unstampedDigest, ar, c.state.Config.Remote.VerifyOutputs, false) + if useCacheKeyDigest && err == nil { + err = c.verifyActionResult(target, command, cacheKeyDigest, ar, c.state.Config.Remote.VerifyOutputs, false) if err == nil { - // Store results under unstamped digest too. - c.locallyCacheResults(target, unstampedDigest, metadata) + // Store results under the cache-key digest too. + c.locallyCacheResults(target, cacheKeyDigest, metadata) } c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{ InstanceName: c.instance, - ActionDigest: unstampedDigest, + ActionDigest: cacheKeyDigest, ActionResult: ar, }) - c.unstampedBuildActionDigests.Put(target.Label, unstampedDigest) + c.unstampedBuildActionDigests.Put(target.Label, cacheKeyDigest) } else { c.unstampedBuildActionDigests.Put(target.Label, stampedDigest) } @@ -608,11 +612,39 @@ func (c *Client) Test(target *core.BuildTarget, run int) (metadata *core.BuildMe if err := c.CheckInitialised(); err != nil { return nil, err } - command, digest, err := c.buildAction(target, true, false, run) - if err != nil { - return nil, err + // As in build(), if PassUnsafeEnv values are excluded from the digest we look results up under a + // cache-key digest that omits them, while executing the real action that includes them. + useCacheKeyDigest := c.excludesUnsafeEnv(target) + var ar *pb.ActionResult + if useCacheKeyDigest { + cacheKeyCommand, cacheKeyDigest, cErr := c.buildAction(target, true, false, true, run) + if cErr != nil { + return nil, cErr + } + if md, r := c.maybeRetrieveResults(target, cacheKeyCommand, cacheKeyDigest, true, false, run); md != nil { + metadata, ar = md, r + } else { + command, digest, bErr := c.buildAction(target, true, false, false, run) + if bErr != nil { + return nil, bErr + } + metadata, ar, err = c.execute(target, command, digest, true, false, run) + if err == nil && ar != nil { + // Backfill the cache-key digest so subsequent runs with differing PassUnsafeEnv values hit. + c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{ + InstanceName: c.instance, + ActionDigest: cacheKeyDigest, + ActionResult: ar, + }) + } + } + } else { + command, digest, bErr := c.buildAction(target, true, false, false, run) + if bErr != nil { + return nil, bErr + } + metadata, ar, err = c.execute(target, command, digest, true, false, run) } - metadata, ar, err := c.execute(target, command, digest, true, false, run) if ar != nil { _, dlErr := c.client.DownloadActionOutputs(context.Background(), ar, target.TestDir(run), c.fileMetadataCache) diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 05af2cd9ba..1eecde01e0 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -166,7 +166,7 @@ func TestNoAbsolutePaths(t *testing.T) { target.AddOutput("remote_test") target.AddSource(core.FileLabel{Package: "package", File: "file"}) target.AddTool(tool.Label) - cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, 0) + cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0) testDir := os.Getenv("TEST_DIR") for _, env := range cmd.EnvironmentVariables { if !strings.HasPrefix(env.Value, "//") { @@ -185,7 +185,7 @@ func TestNoAbsolutePaths2(t *testing.T) { target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target5"}) target.AddOutput("remote_test") target.AddTool(core.SystemPathLabel{Path: []string{os.Getenv("TMP_DIR")}, Name: "remote_test"}) - cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, 0) + cmd, _ := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0) for _, env := range cmd.EnvironmentVariables { if !strings.HasPrefix(env.Value, "//") { assert.False(t, filepath.IsAbs(env.Value), "Env var %s has an absolute path: %s", env.Name, env.Value) @@ -199,17 +199,71 @@ func TestRemoteFilesHashConsistently(t *testing.T) { target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "download"}) target.IsRemoteFile = true target.AddSource(core.URLLabel("https://localhost/file")) - cmd, digest, err := c.buildAction(target, false, false, 0) + cmd, digest, err := c.buildAction(target, false, false, false, 0) assert.NoError(t, err) // After we change this path, the rule should still give back the same protos since it is // not relevant to how we fetch a remote asset. c.state.Config.Build.Path = []string{"/usr/bin/nope"} - cmd2, digest2, err := c.buildAction(target, false, false, 0) + cmd2, digest2, err := c.buildAction(target, false, false, false, 0) assert.NoError(t, err) assert.Equal(t, cmd, cmd2) assert.Equal(t, digest, digest2) } +// envContainsValue reports whether any environment variable in the command has the given value. +func envContainsValue(cmd *pb.Command, value string) bool { + for _, e := range cmd.EnvironmentVariables { + if e.Value == value { + return true + } + } + return false +} + +func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { + c := newClientInstance("unsafe_env") + assert.True(t, c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest, "should default to true") + + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "unsafe"}) + target.AddOutput("out.txt") + target.Command = "echo hello > $OUT" + unsafe := []string{"MY_UNSAFE_VAR"} + target.PassUnsafeEnv = &unsafe + target.BuildTimeout = time.Minute + + t.Setenv("MY_UNSAFE_VAR", "first") + canon1, canonDigest1, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + real1, realDigest1, err := c.buildAction(target, false, true, false, 0) + require.NoError(t, err) + + t.Setenv("MY_UNSAFE_VAR", "second") + canon2, canonDigest2, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + _, realDigest2, err := c.buildAction(target, false, true, false, 0) + require.NoError(t, err) + + // The cache-key (canonical) digest is stable across changes to the PassUnsafeEnv value, and the value + // never appears in the canonical command's environment. + assert.Equal(t, canonDigest1.Hash, canonDigest2.Hash) + assert.False(t, envContainsValue(canon1, "first")) + assert.False(t, envContainsValue(canon2, "second")) + + // The executed action still includes the real value, so its digest changes when the value changes. + assert.NotEqual(t, realDigest1.Hash, realDigest2.Hash) + assert.True(t, envContainsValue(real1, "first")) + + // With the feature disabled the value contributes to the cache-key digest as before. + c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = false + t.Setenv("MY_UNSAFE_VAR", "first") + _, disabledDigest1, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + t.Setenv("MY_UNSAFE_VAR", "second") + _, disabledDigest2, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + assert.NotEqual(t, disabledDigest1.Hash, disabledDigest2.Hash) +} + func TestOutDirsSetOutsOnTarget(t *testing.T) { c := newClientInstance("mock") @@ -318,7 +372,7 @@ func TestTargetPlatform(t *testing.T) { c := newClientInstance("platform_test") c.platform = convertPlatform(c.state.Config.Remote.Platform) // Bit of a hack but we can't go through the normal path. target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target"}) - cmd, err := c.buildCommand(target, &pb.Directory{}, false, false, false, 0) + cmd, err := c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0) assert.NoError(t, err) assert.Equal(t, &pb.Platform{ Properties: []*pb.Platform_Property{ @@ -330,7 +384,7 @@ func TestTargetPlatform(t *testing.T) { }, cmd.Platform) //nolint:staticcheck target.Labels = []string{"remote-platform-property:size=chomky"} - cmd, err = c.buildCommand(target, &pb.Directory{}, false, false, false, 0) + cmd, err = c.buildCommand(target, &pb.Directory{}, false, false, false, false, 0) assert.NoError(t, err) assert.Equal(t, &pb.Platform{ Properties: []*pb.Platform_Property{ From 982996f22c693f9362bf590376f605b85429af59 Mon Sep 17 00:00:00 2001 From: Andrzej J Skalski Date: Wed, 3 Jun 2026 14:05:53 +0200 Subject: [PATCH 2/5] Add end-to-end remote test for PassUnsafeEnv cache hits Add TestPassUnsafeEnvRemoteCacheHitAcrossValues, which builds a target twice against the in-process test server with differing PassUnsafeEnv values (using separate clients with empty local caches) and asserts the second build is a remote cache hit rather than a re-execution. Adds an execution counter to the test server to verify the action is executed exactly once. --- src/remote/impl_test.go | 4 ++++ src/remote/remote_test.go | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/remote/impl_test.go b/src/remote/impl_test.go index 338871a678..16d8abc9f5 100644 --- a/src/remote/impl_test.go +++ b/src/remote/impl_test.go @@ -8,6 +8,7 @@ import ( "net" "os" "regexp" + "sync/atomic" "testing" "cloud.google.com/go/longrunning/autogen/longrunningpb" @@ -55,6 +56,7 @@ type testServer struct { blobs map[string][]byte bytestreams map[string][]byte mockActionResult *pb.ActionResult + executions atomic.Int64 } func (s *testServer) GetCapabilities(ctx context.Context, req *pb.GetCapabilitiesRequest) (*pb.ServerCapabilities, error) { @@ -86,6 +88,7 @@ func (s *testServer) Reset() { s.blobs = map[string][]byte{} s.bytestreams = map[string][]byte{} s.mockActionResult = nil + s.executions.Store(0) } func (s *testServer) GetActionResult(ctx context.Context, req *pb.GetActionResultRequest) (*pb.ActionResult, error) { @@ -249,6 +252,7 @@ func (s *testServer) QueryWriteStatus(ctx context.Context, req *bs.QueryWriteSta } func (s *testServer) Execute(req *pb.ExecuteRequest, srv pb.Execution_ExecuteServer) error { + s.executions.Add(1) mm := func(msg protoreflect.ProtoMessage) *anypb.Any { a := &anypb.Any{} a.MarshalFrom(msg) diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 1eecde01e0..21f90139e7 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -1,6 +1,7 @@ package remote import ( + "fmt" "os" "path/filepath" "strings" @@ -264,6 +265,54 @@ func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { assert.NotEqual(t, disabledDigest1.Hash, disabledDigest2.Hash) } +// TestPassUnsafeEnvRemoteCacheHitAcrossValues is an end-to-end test against the in-process testServer +// that proves changing a PassUnsafeEnv value does not re-execute the action: the first build executes +// and backfills the cache-key digest, and a second build with a *different* value (and a fresh client, +// so a cold local cache) is served from the remote action cache instead of being re-executed. +func TestPassUnsafeEnvRemoteCacheHitAcrossValues(t *testing.T) { + server.Reset() + defer server.Reset() + + // Use unique instance names per run so each client's (disk-backed) local metadata store is empty. + // This guarantees the first build is a cold miss and forces the second build through the shared + // remote action cache (which is keyed by digest only, so it is shared across instances). Both names + // are something other than "test"/"mock" so the test server takes its default execution branch. + base := fmt.Sprintf("unsafe-%d", time.Now().UnixNano()) + + newTarget := func() *core.BuildTarget { + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target2"}) + target.AddSource(core.FileLabel{File: "src1.txt", Package: "package"}) + target.AddSource(core.FileLabel{File: "src2.txt", Package: "package"}) + target.AddOutput("out2.txt") + target.BuildTimeout = time.Minute + target.Command = "echo hello && echo test > $OUT" + unsafe := []string{"MY_UNSAFE_VAR"} + target.PassUnsafeEnv = &unsafe + return target + } + + // First build: cold cache, so the action is executed once and the result is backfilled under the + // value-independent cache-key digest. + c1 := newClientInstance(base + "-a") + require.NoError(t, c1.CheckInitialised()) + require.True(t, c1.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest) + t.Setenv("MY_UNSAFE_VAR", "first") + md1, _, _, err := c1.build(newTarget()) + require.NoError(t, err) + assert.False(t, md1.Cached, "first build should not be cached") + assert.EqualValues(t, 1, server.executions.Load(), "first build should execute the action once") + + // Second build: different PassUnsafeEnv value and a separate client with an empty local cache. It + // must be a remote cache hit on the cache-key digest, not a re-execution. + c2 := newClientInstance(base + "-b") + require.NoError(t, c2.CheckInitialised()) + t.Setenv("MY_UNSAFE_VAR", "second") + md2, _, _, err := c2.build(newTarget()) + require.NoError(t, err) + assert.True(t, md2.Cached, "build with a different PassUnsafeEnv value should be a cache hit") + assert.EqualValues(t, 1, server.executions.Load(), "changing only PassUnsafeEnv must not re-execute the action") +} + func TestOutDirsSetOutsOnTarget(t *testing.T) { c := newClientInstance("mock") From 1254c1a1ff4103ac46d3f467bfe0497fc6d0e7b0 Mon Sep 17 00:00:00 2001 From: Andrzej J Skalski Date: Wed, 3 Jun 2026 14:37:23 +0200 Subject: [PATCH 3/5] Default Remote.ExcludePassUnsafeEnvVarsFromDigest to disabled Flip the new option to be off by default, so PassUnsafeEnv values contribute to the remote action digest unless explicitly opted in. Update help text, docs, changelog and tests accordingly. --- ChangeLog | 8 ++++---- docs/config.html | 14 ++++++++------ src/core/config.go | 3 +-- src/remote/remote_test.go | 27 +++++++++++++++------------ 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/ChangeLog b/ChangeLog index 78a251b9e0..5fea3e80ed 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,10 +6,10 @@ Version 17.30.0 * Make `git_*` built-in function failures more informative (#3525) * Fix go coverage when parsing blocks out of order (#3519) * Update to golang 1.26 (#3498) - * Exclude `PassUnsafeEnv` values from the remote action digest by default, so - changing them no longer causes remote cache misses (matching local cache - behaviour). Controlled by the new `Remote.ExcludePassUnsafeEnvVarsFromDigest` - config option. + * Add `Remote.ExcludePassUnsafeEnvVarsFromDigest` config option (disabled by + default) which, when enabled, excludes `PassUnsafeEnv` values from the remote + action digest so changing them no longer causes remote cache misses (matching + local cache behaviour). Version 17.29.1 --------------- diff --git a/docs/config.html b/docs/config.html index 80540398c0..e901cbe627 100644 --- a/docs/config.html +++ b/docs/config.html @@ -702,13 +702,15 @@

    target hashes.

    - When building remotely, the + When building remotely, the values are by default included in the + remote action cache key, so changing them does cause remote cache + misses. Enabling the Remote.ExcludePassUnsafeEnvVarsFromDigest - option (enabled by default) likewise keeps these values out of the - remote action cache key, so changing them does not cause remote cache - misses. The values are still passed to the executed action. Note that a - cached result may therefore be reused (and shared between users) even if - the PassUnsafeEnv values differ from those it was originally built with. + option keeps these values out of the remote action cache key (while + still passing them to the executed action), so changing them no longer + causes remote cache misses. Note that with it enabled a cached result + may be reused (and shared between users) even if the PassUnsafeEnv + values differ from those it was originally built with.

    • diff --git a/src/core/config.go b/src/core/config.go index 68fd06f687..ba8f987cf9 100644 --- a/src/core/config.go +++ b/src/core/config.go @@ -409,7 +409,6 @@ func DefaultConfiguration() *Configuration { config.Remote.NumExecutors = 20 // kind of arbitrary config.Remote.Secure = true config.Remote.VerifyOutputs = true - config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true config.Remote.UploadDirs = true config.Remote.CacheDuration = cli.Duration(10000 * 24 * time.Hour) // Effectively forever. config.Remote.Shell = "bash" @@ -574,7 +573,7 @@ type Configuration struct { Timeout cli.Duration `help:"Timeout for connections made to the remote server."` Secure bool `help:"Whether to use TLS for communication or not."` VerifyOutputs bool `help:"Whether to verify all outputs are present after a cached remote execution action. Depending on your server implementation, you may require this to ensure files are really present."` - ExcludePassUnsafeEnvVarsFromDigest bool `help:"Whether to exclude the values of PassUnsafeEnv environment variables from the remote action digest. When true (the default) PassUnsafeEnv values are still passed to the executed action but do not contribute to the cache key, so changing them does not cause remote cache misses (matching local cache behaviour). Note that this means a cached result may be reused (and shared between users) even if the PassUnsafeEnv values differ from those it was built with."` + ExcludePassUnsafeEnvVarsFromDigest bool `help:"Whether to exclude the values of PassUnsafeEnv environment variables from the remote action digest. When enabled, PassUnsafeEnv values are still passed to the executed action but do not contribute to the cache key, so changing them does not cause remote cache misses (matching local cache behaviour). This is disabled by default; note that enabling it means a cached result may be reused (and shared between users) even if the PassUnsafeEnv values differ from those it was built with."` UploadDirs bool `help:"Uploads individual directory blobs after build actions. This might not be necessary with some servers, but if you aren't sure, you should leave it on."` OptionalOutputsRequired bool `help:"Requires that any optional outputs of build actions (optional test outputs, coverage when not opted out of) are produced. By default this is a non-fatal failure, but the actions may not cache remotely."` Shell string `help:"Path to the shell to use to execute actions in. Default is 'bash' which will be looked up by the server."` diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 21f90139e7..96cf5eaa99 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -223,7 +223,7 @@ func envContainsValue(cmd *pb.Command, value string) bool { func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { c := newClientInstance("unsafe_env") - assert.True(t, c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest, "should default to true") + assert.False(t, c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest, "should default to false") target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "unsafe"}) target.AddOutput("out.txt") @@ -232,6 +232,18 @@ func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { target.PassUnsafeEnv = &unsafe target.BuildTimeout = time.Minute + // With the feature disabled (the default) the value contributes to the cache-key digest. + t.Setenv("MY_UNSAFE_VAR", "first") + _, disabledDigest1, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + t.Setenv("MY_UNSAFE_VAR", "second") + _, disabledDigest2, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + assert.NotEqual(t, disabledDigest1.Hash, disabledDigest2.Hash) + + // Enable the feature; now the value must be excluded from the cache-key digest. + c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true + t.Setenv("MY_UNSAFE_VAR", "first") canon1, canonDigest1, err := c.buildAction(target, false, false, true, 0) require.NoError(t, err) @@ -253,16 +265,6 @@ func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { // The executed action still includes the real value, so its digest changes when the value changes. assert.NotEqual(t, realDigest1.Hash, realDigest2.Hash) assert.True(t, envContainsValue(real1, "first")) - - // With the feature disabled the value contributes to the cache-key digest as before. - c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = false - t.Setenv("MY_UNSAFE_VAR", "first") - _, disabledDigest1, err := c.buildAction(target, false, false, true, 0) - require.NoError(t, err) - t.Setenv("MY_UNSAFE_VAR", "second") - _, disabledDigest2, err := c.buildAction(target, false, false, true, 0) - require.NoError(t, err) - assert.NotEqual(t, disabledDigest1.Hash, disabledDigest2.Hash) } // TestPassUnsafeEnvRemoteCacheHitAcrossValues is an end-to-end test against the in-process testServer @@ -294,8 +296,8 @@ func TestPassUnsafeEnvRemoteCacheHitAcrossValues(t *testing.T) { // First build: cold cache, so the action is executed once and the result is backfilled under the // value-independent cache-key digest. c1 := newClientInstance(base + "-a") + c1.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true require.NoError(t, c1.CheckInitialised()) - require.True(t, c1.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest) t.Setenv("MY_UNSAFE_VAR", "first") md1, _, _, err := c1.build(newTarget()) require.NoError(t, err) @@ -305,6 +307,7 @@ func TestPassUnsafeEnvRemoteCacheHitAcrossValues(t *testing.T) { // Second build: different PassUnsafeEnv value and a separate client with an empty local cache. It // must be a remote cache hit on the cache-key digest, not a re-execution. c2 := newClientInstance(base + "-b") + c2.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true require.NoError(t, c2.CheckInitialised()) t.Setenv("MY_UNSAFE_VAR", "second") md2, _, _, err := c2.build(newTarget()) From c729688e87c10d3c08ddf8e23804a3c4a37b5f78 Mon Sep 17 00:00:00 2001 From: Andrzej J Skalski Date: Wed, 3 Jun 2026 17:05:06 +0200 Subject: [PATCH 4/5] Also exclude config-level PassUnsafeEnv from remote digest The previous change only stripped per-target pass_unsafe_env from the cache-key command. Values declared via the global [Build] PassUnsafeEnv config keyword enter the build environment through config.GetBuildEnv() and were still left in the canonical command, so changing them still caused remote cache misses. Strip both config-level and target-level PassUnsafeEnv (keeping anything also listed in PassEnv), and treat a non-empty config-level list as enough to enable the cache-key split. This matches the local cache, which excludes both from its hash. --- src/remote/action.go | 42 ++++++++++++++++++++-------- src/remote/impl_test.go | 10 +++++++ src/remote/remote.go | 4 +-- src/remote/remote_test.go | 59 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/src/remote/action.go b/src/remote/action.go index ee313d4846..efbd6630d9 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -132,7 +132,7 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, cmd, err := core.ReplaceSequences(state, target, cmd) env := c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun) if canonical { - c.stripUnsafeEnv(target, env) + c.stripUnsafeEnv(state, target, env) } return &pb.Command{ Platform: c.targetPlatformProperties(target), @@ -142,29 +142,47 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, }, err } -// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this target. -func (c *Client) excludesUnsafeEnv(target *core.BuildTarget) bool { - return c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest && target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0 +// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this +// target. This considers both the global [Build] PassUnsafeEnv config keyword and the target's own +// pass_unsafe_env attribute, mirroring how the local cache excludes both from its hash. +func (c *Client) excludesUnsafeEnv(state *core.BuildState, target *core.BuildTarget) bool { + if !c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest { + return false + } + if len(state.Config.Build.PassUnsafeEnv) > 0 { + return true + } + return target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0 } // stripUnsafeEnv removes the values of PassUnsafeEnv variables from the given environment so that they do -// not contribute to the action digest. Variables that are also listed in PassEnv are left intact, since -// those values are intentionally part of the cache key. -func (c *Client) stripUnsafeEnv(target *core.BuildTarget, env core.BuildEnv) { - if !c.excludesUnsafeEnv(target) { +// not contribute to the action digest. Both the global [Build] PassUnsafeEnv config keyword and the +// target's pass_unsafe_env attribute are considered. Variables that are also listed in PassEnv (config or +// target level) are left intact, since those values are intentionally part of the cache key. +func (c *Client) stripUnsafeEnv(state *core.BuildState, target *core.BuildTarget, env core.BuildEnv) { + if !c.excludesUnsafeEnv(state, target) { return } safe := map[string]bool{} + for _, e := range state.Config.Build.PassEnv { + safe[e] = true + } if target.PassEnv != nil { for _, e := range *target.PassEnv { safe[e] = true } } - for _, e := range *target.PassUnsafeEnv { - if !safe[e] { - delete(env, e) + strip := func(vars []string) { + for _, e := range vars { + if !safe[e] { + delete(env, e) + } } } + strip(state.Config.Build.PassUnsafeEnv) + if target.PassUnsafeEnv != nil { + strip(*target.PassUnsafeEnv) + } } // stampedBuildEnvironment returns a build environment, optionally with a stamp if stamp is true. @@ -194,7 +212,7 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state)) env := core.TestEnvironment(state, target, ".", run) if canonical { - c.stripUnsafeEnv(target, env) + c.stripUnsafeEnv(state, target, env) } return &pb.Command{ Platform: &pb.Platform{ diff --git a/src/remote/impl_test.go b/src/remote/impl_test.go index 16d8abc9f5..2b26b758e2 100644 --- a/src/remote/impl_test.go +++ b/src/remote/impl_test.go @@ -34,6 +34,13 @@ func newClient() *Client { } func newClientInstance(name string) *Client { + return newClientInstanceWith(name, nil) +} + +// newClientInstanceWith builds a client, optionally applying configure to the configuration before the +// build state (and the client's async initialisation) is created. This matters for config values that are +// captured once, e.g. the build environment derived from PassUnsafeEnv. +func newClientInstanceWith(name string, configure func(*core.Configuration)) *Client { config := core.DefaultConfiguration() config.Build.Path = []string{"/usr/local/bin", "/usr/bin", "/bin"} config.Build.HashFunction = "sha256" @@ -41,6 +48,9 @@ func newClientInstance(name string) *Client { config.Remote.Instance = name config.Remote.Secure = false config.Remote.Platform = []string{"OSFamily=linux"} + if configure != nil { + configure(config) + } state := core.NewBuildState(config) state.Config.Remote.URL = "127.0.0.1:9987" state.Config.Remote.AssetURL = state.Config.Remote.URL diff --git a/src/remote/remote.go b/src/remote/remote.go index 2767c1f994..0888802421 100644 --- a/src/remote/remote.go +++ b/src/remote/remote.go @@ -417,7 +417,7 @@ func (c *Client) build(target *core.BuildTarget) (*core.BuildMetadata, *pb.Actio // store & look up results under a "cache-key" action digest that omits those inputs, while still // executing the real action that includes them. This implements the rule whereby e.g. the SCM // revision or an unsafe env var changing doesn't force a rebuild. - useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(target) + useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(c.state.ForTarget(target), target) var cacheKeyDigest *pb.Digest if useCacheKeyDigest { command, digest, err := c.buildAction(target, false, false, true, 0) @@ -614,7 +614,7 @@ func (c *Client) Test(target *core.BuildTarget, run int) (metadata *core.BuildMe } // As in build(), if PassUnsafeEnv values are excluded from the digest we look results up under a // cache-key digest that omits them, while executing the real action that includes them. - useCacheKeyDigest := c.excludesUnsafeEnv(target) + useCacheKeyDigest := c.excludesUnsafeEnv(c.state.ForTarget(target), target) var ar *pb.ActionResult if useCacheKeyDigest { cacheKeyCommand, cacheKeyDigest, cErr := c.buildAction(target, true, false, true, run) diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 96cf5eaa99..29630144cc 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -267,6 +267,65 @@ func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) { assert.True(t, envContainsValue(real1, "first")) } +// TestStripUnsafeEnvConfigLevel checks the stripping logic removes values from the global [Build] +// PassUnsafeEnv config keyword while retaining PassEnv values (which are intentionally part of the cache key). +func TestStripUnsafeEnvConfigLevel(t *testing.T) { + c := newClientInstance("strip") + c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true + c.state.Config.Build.PassUnsafeEnv = []string{"CFG_UNSAFE"} + c.state.Config.Build.PassEnv = []string{"CFG_SAFE"} + + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "x"}) + unsafe := []string{"TGT_UNSAFE"} + target.PassUnsafeEnv = &unsafe + + env := core.BuildEnv{ + "CFG_UNSAFE": "secret", + "CFG_SAFE": "keep", + "TGT_UNSAFE": "alsosecret", + "OTHER": "v", + } + c.stripUnsafeEnv(c.state.ForTarget(target), target, env) + + _, cfgUnsafePresent := env["CFG_UNSAFE"] + _, tgtUnsafePresent := env["TGT_UNSAFE"] + assert.False(t, cfgUnsafePresent, "config-level PassUnsafeEnv should be stripped") + assert.False(t, tgtUnsafePresent, "target-level PassUnsafeEnv should be stripped") + assert.Equal(t, "keep", env["CFG_SAFE"], "PassEnv must be retained") + assert.Equal(t, "v", env["OTHER"], "unrelated env must be retained") +} + +// TestConfigPassUnsafeEnvExcludedFromDigest checks that values declared via the global [Build] +// PassUnsafeEnv config keyword (not just the per-target attribute) are excluded from the cache-key digest. +// Because config-level values are captured once per config object, this uses a separate client per value +// and sets the config before the client (and its async init) is created. +func TestConfigPassUnsafeEnvExcludedFromDigest(t *testing.T) { + canonicalAndReal := func(value string) (string, string) { + t.Setenv("MY_CFG_UNSAFE", value) + c := newClientInstanceWith(fmt.Sprintf("cfg-unsafe-%d", time.Now().UnixNano()), func(config *core.Configuration) { + config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true + config.Build.PassUnsafeEnv = []string{"MY_CFG_UNSAFE"} + }) + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "cfgunsafe"}) + target.AddOutput("out.txt") + target.Command = "echo hello > $OUT" + target.BuildTimeout = time.Minute + canon, canonDigest, err := c.buildAction(target, false, false, true, 0) + require.NoError(t, err) + require.False(t, envContainsValue(canon, value), "config-level PassUnsafeEnv value must not be in the canonical command") + real, realDigest, err := c.buildAction(target, false, true, false, 0) + require.NoError(t, err) + require.True(t, envContainsValue(real, value), "executed action should include the config-level value") + return canonDigest.Hash, realDigest.Hash + } + + canonFirst, realFirst := canonicalAndReal("first") + canonSecond, realSecond := canonicalAndReal("second") + + assert.Equal(t, canonFirst, canonSecond, "config-level PassUnsafeEnv value must not affect the cache-key digest") + assert.NotEqual(t, realFirst, realSecond, "the executed action should still include the config-level value") +} + // TestPassUnsafeEnvRemoteCacheHitAcrossValues is an end-to-end test against the in-process testServer // that proves changing a PassUnsafeEnv value does not re-execute the action: the first build executes // and backfills the cache-key digest, and a second build with a *different* value (and a fresh client, From db71c1c80a5ea185ac567b764f1213b58ee50e05 Mon Sep 17 00:00:00 2001 From: Andrzej J Skalski Date: Thu, 4 Jun 2026 14:13:00 +0200 Subject: [PATCH 5/5] Simplify Test() cache-key flow and clarify digest map Collapse the duplicated buildAction+execute paths in Test() into a single compute-then-download flow, condense excludesUnsafeEnv to one expression, and document that unstampedBuildActionDigests now holds the cache-key digest. --- src/remote/action.go | 9 ++------- src/remote/remote.go | 40 +++++++++++++++++++--------------------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/remote/action.go b/src/remote/action.go index efbd6630d9..3370986351 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -146,13 +146,8 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory, // target. This considers both the global [Build] PassUnsafeEnv config keyword and the target's own // pass_unsafe_env attribute, mirroring how the local cache excludes both from its hash. func (c *Client) excludesUnsafeEnv(state *core.BuildState, target *core.BuildTarget) bool { - if !c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest { - return false - } - if len(state.Config.Build.PassUnsafeEnv) > 0 { - return true - } - return target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0 + return c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest && + (len(state.Config.Build.PassUnsafeEnv) > 0 || (target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0)) } // stripUnsafeEnv removes the values of PassUnsafeEnv variables from the given environment so that they do diff --git a/src/remote/remote.go b/src/remote/remote.go index 0888802421..aa31d02f62 100644 --- a/src/remote/remote.go +++ b/src/remote/remote.go @@ -76,7 +76,10 @@ type Client struct { subrepoTrees map[core.BuildLabel]*pb.Tree outputMutex sync.RWMutex - // The unstamped build action digests. Stamped and test digests are not stored. + // The cache-key build action digests. Stamped and test digests are not stored. + // For most targets this is the plain (unstamped) action digest, but for stamped targets and those + // whose PassUnsafeEnv values are excluded from the digest it is the canonical digest that omits those + // volatile inputs (see Client.build); that is the digest results are stored & looked up under. // This isn't just a cache - it is needed because building a target can modify the target and things like plz hash // --detailed and --shell will fail to get the right action digest. unstampedBuildActionDigests actionDigestMap @@ -614,36 +617,31 @@ func (c *Client) Test(target *core.BuildTarget, run int) (metadata *core.BuildMe } // As in build(), if PassUnsafeEnv values are excluded from the digest we look results up under a // cache-key digest that omits them, while executing the real action that includes them. - useCacheKeyDigest := c.excludesUnsafeEnv(c.state.ForTarget(target), target) var ar *pb.ActionResult - if useCacheKeyDigest { - cacheKeyCommand, cacheKeyDigest, cErr := c.buildAction(target, true, false, true, run) + var cacheKeyDigest *pb.Digest + if c.excludesUnsafeEnv(c.state.ForTarget(target), target) { + command, digest, cErr := c.buildAction(target, true, false, true, run) if cErr != nil { return nil, cErr } - if md, r := c.maybeRetrieveResults(target, cacheKeyCommand, cacheKeyDigest, true, false, run); md != nil { - metadata, ar = md, r - } else { - command, digest, bErr := c.buildAction(target, true, false, false, run) - if bErr != nil { - return nil, bErr - } - metadata, ar, err = c.execute(target, command, digest, true, false, run) - if err == nil && ar != nil { - // Backfill the cache-key digest so subsequent runs with differing PassUnsafeEnv values hit. - c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{ - InstanceName: c.instance, - ActionDigest: cacheKeyDigest, - ActionResult: ar, - }) - } + if metadata, ar = c.maybeRetrieveResults(target, command, digest, true, false, run); ar == nil { + cacheKeyDigest = digest // Cache miss; remember to backfill once we've executed the real action. } - } else { + } + if ar == nil { command, digest, bErr := c.buildAction(target, true, false, false, run) if bErr != nil { return nil, bErr } metadata, ar, err = c.execute(target, command, digest, true, false, run) + if err == nil && ar != nil && cacheKeyDigest != nil { + // Backfill the cache-key digest so later runs with differing PassUnsafeEnv values hit. + c.client.UpdateActionResult(context.Background(), &pb.UpdateActionResultRequest{ + InstanceName: c.instance, + ActionDigest: cacheKeyDigest, + ActionResult: ar, + }) + } } if ar != nil {