Add env vars that are excluded from the process cache key - #23646
Add env vars that are excluded from the process cache key#23646jasonwbarnett wants to merge 1 commit into
Conversation
Environment variable values are part of a process's cache key, which is right by default. But some values unavoidably vary per run and cannot affect what a process produces — a CI build id used to label a report, a token for uploading results — and passing one today means every run computes a key nothing else can match. Concretely: `buildkite-test-collector` is a `[pytest11]` plugin that reads `BUILDKITE_ANALYTICS_TOKEN` and `BUILDKITE_BUILD_ID` from `os.environ`. Getting those into the sandbox requires `[test].extra_env_vars`, which puts ~25 per-job values into every test process's key, so no test result is ever reused across builds. With remote cache writes enabled, each run also stores results under keys that can never be read back. Add `Process.uncached_env`: excluded from the `Command` proto, and so from the action digest that keys both caches, then merged into the environment by `bounded::CommandRunner` — below every caching layer, next to the existing `execution_slot_variable` injection that already works this way. It is also ignored for `PartialEq`/`Hash` so differing values do not split graph nodes. `env` wins a collision, since an explicitly cache-keyed value is the more specific request. Surface it as `[test].uncached_env_vars` and an `uncached_env_vars` field on test targets, threaded through `PexProcess`/`VenvPexProcess` and the pytest runner. `InteractiveProcess.from_process` folds the values into `env`, because an interactive process is never cached and `--debug` should match a normal run. Two limits are inherent and documented rather than designed around: on a cache hit the process does not run, so these values are not observed at all; and under remote execution the `Command` proto is the only channel to the worker, so they cannot be delivered and are dropped with a warning.
c3545cb to
be97f6e
Compare
|
Force-pushed: the first version of this branch was accidentally based on a ~200-commit-old |
|
Verified end-to-end against the real repo this was motivated by, via The option. Same target,
The target field. A probe test asserting
That last pair is the part I found most convincing: the control's failure message proves the new value genuinely reaches the process, so the cache hit in the One wrinkle worth knowing about, and an argument for the docs being explicit: under |
Closes #23645.
Environment variable values are part of a process's cache key, which is right by default. But some values unavoidably vary per run and cannot affect what a process produces, and today passing one means every run computes a key nothing else can match.
The case that motivated this:
buildkite-test-collectoris a[pytest11]plugin that auto-loads in every pytest process and readsBUILDKITE_ANALYTICS_TOKENandBUILDKITE_BUILD_IDstraight fromos.environ. The only way to get those into the sandbox is[test].extra_env_vars, which puts roughly 25 per-job values (job id, build id, commit, agent id, instance id, step id, …) into every test process's key. No test result is ever reused across builds, and withremote_cache_write = trueeach run also writes entries under keys that can never be read back. Those values label a report; they cannot change whether a test passes. Pants had no way to say so.Approach
Process.uncached_envis a second env map that is excluded from theCommandproto, and so from theaction_digestthat keys both the local and remote caches. It is merged intoenvbybounded::CommandRunner, immediately alongside the existingexecution_slot_variableinjection — which already works exactly this way, sincecontext.rswrapsbounded::CommandRunnerin bothremote_cache::CommandRunnerandcache::CommandRunner. TheTODOabove that injection ("they might currently be applied above a cache") describes the property this change relies on deliberately.Details worth calling out for review:
#[derivative(PartialEq = "ignore", Hash = "ignore")], so two processes differing only inuncached_envare the same graph node. Without that they would dedupe to separate nodes that both resolve to the same cache key — harmless but wasteful.env, exclusion from theCommandproto is automatic rather than a filter that a future edit could forget.envwins a name collision. An explicitly cache-keyed value is the more specific request, and honouring it keeps the key honest.InteractiveProcess.from_processfolds the values intoenv. An interactive process is never cached, so the distinction does not apply, and--debugshould behave like a normal run. I only found this because therun_pytesttest helper asserts the two paths agree — a good check.User-facing surface is
[test].uncached_env_varsplus anuncached_env_varsfield on test targets, threaded throughPexProcess/VenvPexProcessandpytest_runner. The field is part ofTestMetadata, so targets wanting different values partition into separate batches — a batch runs as one process, so they have to.Two limits, documented rather than designed around
Commandproto is the only channel to the worker, so values excluded from it cannot be delivered.remote::CommandRunner::runwarns and drops them. I chose warn over error because the contract is that these values cannot affect the result, so the execution is still correct.The obvious risk is misuse — a behaviour-affecting variable in the new field yields silently wrong cached results. I have tried to make the option help and the
Processdocs blunt about that rather than reassuring. If you would rather the footgun were harder to reach (an allowlist, a different name, plugin-API only with notestsurface), I am happy to rework it.Tests
Rust, in
process_execution/src/tests.rs:process_equality_ignores_uncached_env— identity and hash ignore the field, while the same names/values inenvstill separate two processes.uncached_env_is_absent_from_the_action_digest— adding anuncached_envvar, or changing its value, leaves both the command and action digests byte-identical; the same names/values inenvchange both. The second half is what stops this test passing vacuously if a future change starts folding the field into the request.Python, in
pytest_runner_integration_test.py:test_uncached_env_vars— values from both the option and the target field reach the test process, with and without an inline value, and a name in both lists resolves to the cached one.test_partitionparametrisation: targets differing inuncached_env_varssplit into separate batches, and ordering does not affect partitioning.Ran locally:
cargo test -p process_execution(74 passed),cargo clippy -p process_execution -p remoteclean,cargo fmt --all, andpants testoverpytest_runner_integration_test.py,core/goals/test_test.pyandengine/process_test.py(all green), pluspants fmt linton the touched Python.Two suites reported failures in my environment that are not related to this change, for the record: three
pytest_runner_integration_testcases skip for want of a CPython 3.10 on the box, and twoengine/process_test.pycases error at fixture setup withFailed to begin watching the filesystem: Too many open files— this machine'sfs.inotify.max_user_instancesis 128 and the module builds 33RuleRunners. Both pass when run in isolation.Not verified locally: anything requiring a remote-execution server, so the drop-and-warn path in
remote::CommandRunneris reasoned-through rather than exercised. Worth a close look in review.