Allow env vars to be excluded from process cache keys by jasonwbarnett · Pull Request #23677 · pantsbuild/pants

jasonwbarnett · 2026-09-15T21:17:41Z

Adds [GLOBAL].cache_key_excluded_env_vars: environment variable names whose values are left out of the key a process's cached result is stored under. The process still receives the variable — only the Command proto that the action digest is taken over omits it.

This is an alternative to #23646 for #23645. That PR added a second env map on Process (uncached_env); review there landed on a list of names applied at key-computation time instead, which is what this is: no new Process field, no plumbing through PexProcess/pytest_runner/target fields, and env vars flow through every existing path untouched unless a name is listed. #23646 is left open so the two shapes can be compared; this one closes #23645 if it is the one taken.

Why

Env var values belong in a process's cache key, which is the right default. But some values a process genuinely needs cannot affect what it produces, and today passing one means every run computes a key nothing else can match. The two cases that motivated this, both from Buildkite CI:

DOCKER_CONFIG — the ECR credentials plugin points it at a fresh temporary directory per job, and it has to be forwarded so crane finds credHelpers. That made oci_pull — pinned to an immutable digest — miss on every build. Only the path string is ever hashed, never the file it names.
BUILDKITE_ANALYTICS_TOKEN / BUILDKITE_BUILD_ID — read straight from os.environ by buildkite-test-collector, a [pytest11] plugin that auto-loads in every pytest process and uploads a report. The only way into the sandbox is [test].extra_env_vars, which puts per-job values into every test process's key; with remote_cache_write = true each run also writes entries under keys that can never be read back.

Both are "the process needs this to authenticate, or to report itself", not "this is an input".

Implementation

One read site. The loop in make_execute_request that fills Command.environment_variables skips excluded names. Since the Command is what the action digest is taken over, exclusion from both the local and the remote cache key follows from that one continue.
The list is global, a OnceLock<Vec<String>> set from Core::make_command_runners during scheduler construction, before any runner exists. Global rather than a Process field because the variables this exists for are noise for every process that receives them, not for one rule's process. Set-once, so a second scheduler in the same process (which pantsd tests create) cannot change cache keys underneath the first.
Patterns are an exact name or a single trailing * — the two shapes extra_env_vars already accepts, so a name can be excluded in the spelling it was declared in.
Remote execution ignores the list entirely, and warns when it is non-empty. Under REAPI the Command is both the cache key and the only channel to the worker, so "exclude from the key but still deliver" is not expressible. There the named variables behave as ordinary cache-keyed env vars rather than silently failing to arrive.

The footgun

A variable named here that does affect a process's output will be served a stale result with nothing to indicate it. The contract is "cannot affect the process's declared outputs" — not the looser "cannot affect the outcome" — and the option help and release note say so bluntly rather than reassuringly. Related: on a cache hit the process does not run at all, so this cannot be used to make something happen on every run. A cached test contributes no analytics upload; arguably more honest, since the test did not run.

If the footgun should be harder to reach — an allowlist, a different name, or no global option at all — I am happy to rework it.

Tests

src/rust/process_execution/src/tests.rs:

cache_key_excluded_env_var_patterns_match_a_name_exactly_or_by_prefix — an exact pattern does not also match as a prefix, and * is special only as the final character.
excluded_env_vars_are_absent_from_the_action_digest — adding an excluded variable, or changing its value, leaves both the action and command digests byte-identical, while a name that is not excluded still changes them. The second half is what stops the test passing vacuously if a future change stops folding env into the Command at all.

Verified locally: cargo test -p process_execution --lib (75 passed) and cargo fmt. Not verified: anything needing a remote-execution server, so the ignore-and-warn branch is reasoned through rather than exercised — worth a close look in review.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DgAHZb2TWf6L113qqaN4sN

Adds `[GLOBAL] cache_key_excluded_env_vars`: names listed there are still passed to every process that asked for them, but are left out of the REAPI `Command` proto the local and remote cache keys are derived from. Without it, any variable whose value differs per run makes every process receiving it uncacheable, however deterministic the process itself is. Two cases motivated this: - `DOCKER_CONFIG`, which the Buildkite ecr plugin sets to a fresh temp directory per job, so a digest-pinned image pull -- an action whose output cannot vary -- missed on every build. - `BUILDKITE*`, which has to reach pytest for buildkite-test-collector to report, and in doing so puts BUILDKITE_BUILD_ID in the key of every test process in the repository. Local execution takes its environment from `Process.env` (local.rs `.envs(&req.env)`), and the `Command` proto is only used for the action digest and for remote *execution*, so an excluded variable still reaches a locally-executed process untouched. Under remote execution the `Command` is the only channel to the worker as well as the cache key, so excluding a name from it and still delivering the value is not expressible. The list is therefore ignored entirely when remote execution is enabled, with a warning, and the named variables behave as ordinary cache-keyed env vars rather than silently failing to arrive. There is precedent for a variable that is deliberately not in the key: `execution_slot_variable` is injected in `bounded.rs`, below both cache runners, and the TODO there notes the layering. This takes the same position declaratively. A global rather than a `Process` field because the variables this exists for are noise for every process that receives them, not for one rule's process -- and because the processes that need it most, like Pants' own pytest runs, are not ones a user constructs. The trade-off is that it is a hole in hermeticity: name a variable that does change output and a stale result will be served with nothing to indicate it. The option help says so. Matching is an exact name or a single trailing `*`, the two shapes `[test] extra_env_vars` already accepts, so a variable can be excluded in the spelling it was declared in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgAHZb2TWf6L113qqaN4sN

benjyw · 2026-09-17T00:55:12Z

+        default=list(DEFAULT_EXECUTION_OPTIONS.cache_key_excluded_env_vars),
+        help=softwrap(
+            """
+            Environment variable names whose values must not affect process cache keys.


Suggested change

Environment variable names whose values must not affect process cache keys.

Environment variable names whose values will not affect process cache keys.

Taken, thanks.

benjyw · 2026-09-17T00:55:55Z

+            """
+            Environment variable names whose values must not affect process cache keys.
+
+            A process still receives these variables; they are simply left out of the key


This is very verbose, let's make it more succinct. And let's end it with "Use with caution!"

Cut it down to three short paragraphs, and it ends on "Use with caution!" now.

benjyw · 2026-09-17T01:00:32Z

+        let cache_key_excluded_env_vars = if remoting_opts.execution_enable {
+            if !exec_strategy_opts.cache_key_excluded_env_vars.is_empty() {
+                log::warn!(
+                    "Ignoring `[GLOBAL] cache_key_excluded_env_vars` because remote execution \


To clarify, we are ignoring them even if the process happens to end up being executed locally? I think that is the correct decision. Especially in the presence of speculation: otherwise we'd have a race between the local and remote processes, and so we'd have unpredictable caching behavior.

Right — with remexec on it's off for every process, wherever that process ends up running.

The speculation race is the actual reason and my comment didn't say it, so I've stolen your wording for it. "The Command is the only channel to the worker" explains why remexec can't honour the option, but not why a locally-executed process shouldn't either.

benjyw · 2026-09-17T22:15:40Z

            """
        ),
    )
+    cache_key_excluded_env_vars = StrListOption(


I think this makes sense as repo-wide config but perhaps it should be in the SubprocessEnvironment subsystem?

I would note that the documentation on that subsystem is a bit of a lie, since currently only pex/uv processes use its one option. But we should should likely fix that fact...

I had a go at this and don't think it fits there, at least not yet.

SubprocessEnvironment.EnvironmentAware gets resolved per-environment at rule time, but this list is needed in make_command_runners, before any rule runs. To drive it from a subsystem it'd have to ride along on each Process — so every rule that builds one has to plumb it through, and anything that doesn't know about the subsystem silently opts out. Which is the Process-field shape your other comment (rightly) pushed me off of.

On the docs being a lie: agreed, and worth fixing. I'd just rather not park this option in a scope that promises more than it delivers while env_vars is the only thing in there. Happy to move it if you still want it there though.

Well, it doesn't have to be EnvironmentAware, it can be on the main subsystem. But let me think on this a little more.

benjyw · 2026-09-17T22:25:20Z

+        // only absent from the `Command` the action digest is taken over. Deliberately a
+        // hole in the hermeticity contract: an excluded variable that does change output
+        // will now be served a stale result.
+        if is_cache_key_excluded_env_var(name) {


Unless I am misreading things, this is also called on the remexec path (in src/rust/process_execution/remote/src/remote.rs), and so the env var will be omitted from the Command in that case, which is not what we want.

Yep, you're right, and that would've been broken outright — thanks for catching it.

make_execute_request builds the Command from the full env again. The exclusion moved up into the two caching runners (process_execution::cache and remote_cache): they key on a Process with the excluded names stripped and pass the untouched one down the stack, so the remexec runner never sees it.

160acaf

It's a little worrying that a major point of this PR (pass env vars through unchanged to remexec) was casually missed by the robot author and not caught by the human one. And the robot author presumably wrote the comment above (the em dash and phrasing are giveaways), so I'm not convinced that enough scrutiny has been applied here.

@jasonwbarnett To proceed with review and eventual merging I would like to have confidence that you have scrutinized this change and are able to explain its details without delegating that explanation to Claude, who we clearly cannot rely on to get this right (or at least not without elaborate prompting).

benjyw · 2026-09-17T22:26:00Z

+/// global rather than a `Process` field because the variables this exists for -- a
+/// per-job `DOCKER_CONFIG` path, `BUILDKITE_*` build metadata -- are noise for every
+/// process that receives them, not for one rule's process.
+static CACHE_KEY_EXCLUDED_ENV_VARS: OnceLock<Vec<String>> = OnceLock::new();


I don't love this as global state. Could it be a member of the scheduler?

Fair, and gone. It's a CacheKeyExcludedEnvVars built in make_command_runners and handed to the two caching runners as a field.

Side benefit: not giving one to the remexec runner is what fixes the bug you found below.

`make_execute_request` is shared with the remote execution path, where the `Command` proto is the only channel by which env vars reach the worker, so dropping a name from it there would keep the variable from arriving at all. It now builds a `Command` from every variable again. Instead the local- and remote-cache `CommandRunner`s hold a `CacheKeyExcludedEnvVars` and key on a `Process` with the excluded names stripped, which also replaces the process-global `OnceLock`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WfhuHthzcBj5QMzHxyvA5

jasonwbarnett · 2026-09-21T18:15:58Z

@benjyw I am not familiar enough with rust or the core of pants and cannot independently assess the LLMs work. Closing. Sorry for the noise. I'll keep running production off of a fork until this is prioritized by someone with the adequate skill and experience.

benjyw self-requested a review September 16, 2026 04:10

jasonwbarnett mentioned this pull request Sep 16, 2026

Add env vars that are excluded from the process cache key #23646

Closed

benjyw reviewed Sep 17, 2026

View reviewed changes

jasonwbarnett closed this Sep 21, 2026

	Environment variable names whose values must not affect process cache keys.
	Environment variable names whose values will not affect process cache keys.

Uh oh!

Conversation

jasonwbarnett commented Sep 15, 2026

Why

Implementation

The footgun

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonwbarnett commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jasonwbarnett Sep 18, 2026 •

edited

Loading

jasonwbarnett Sep 18, 2026 •

edited

Loading

jasonwbarnett Sep 18, 2026 •

edited

Loading

jasonwbarnett Sep 18, 2026 •

edited

Loading

jasonwbarnett Sep 18, 2026 •

edited

Loading

jasonwbarnett commented Sep 21, 2026 •

edited

Loading