diff --git a/docs/notes/2.34.x.md b/docs/notes/2.34.x.md index 42c83a4357e..089b0f87c80 100644 --- a/docs/notes/2.34.x.md +++ b/docs/notes/2.34.x.md @@ -33,6 +33,16 @@ Pants option config files are now parsed as TOML 1.1 rather than TOML 1.0. This ### Goals +The `test` goal has a new `[test].uncached_env_vars` option, and test targets a matching +`uncached_env_vars` field, for environment variables that should reach test processes without +contributing to their cache key. This is for values that unavoidably vary between runs but cannot +change a test's result — a CI build id used to label a report, a token for uploading results. +Naming such a variable in `extra_env_vars` gives every run a distinct cache key, so no test result +is ever reused. + +Note that on a cache hit the test does not run, so these values are not observed at all, and that +they are dropped under remote execution. + ### Backends #### Docker @@ -93,6 +103,11 @@ Linked Go binaries and test binaries now record a Go toolchain build ID, as `go ### Plugin API changes +`Process` has a new `uncached_env` field, holding environment variables that are excluded from the +process's cache key and merged into its environment after the cache is consulted. Use it only for +values that cannot change what the process produces; see the field's documentation for the +caveats. + `Target`, `TargetAdaptor`, `SourceBlock`, `SourceBlocks`, `TextBlock` and `Hunk` are now backed by native Rust implementations. They are still importable from their previous locations and their public constructors, attributes and methods are unchanged, but they are no longer Python dataclasses and they are built in `__new__` rather than `__init__`, so `dataclasses.is_dataclass`, `dataclasses.fields` and `dataclasses.replace` no longer apply to them. Defining subclasses in Python, and setting attributes on those subclasses, continues to work as before. `Field` subclasses now resolve `default`, `required` and `none_is_valid_value`, and whether `removal_version` is set, once per field type and cache the result. A field type that computes any of these dynamically, for example through a descriptor returning a different value on each access, will now only ever see the first value. diff --git a/src/python/pants/backend/python/goals/pytest_runner.py b/src/python/pants/backend/python/goals/pytest_runner.py index ccb1215d292..954c3dc43e1 100644 --- a/src/python/pants/backend/python/goals/pytest_runner.py +++ b/src/python/pants/backend/python/goals/pytest_runner.py @@ -188,6 +188,7 @@ class TestMetadata: interpreter_constraints: InterpreterConstraints extra_env_vars: tuple[str, ...] + uncached_env_vars: tuple[str, ...] xdist_concurrency: int | None resolve: str environment: str @@ -298,12 +299,17 @@ async def setup_pytest_for_target( EnvironmentVarsRequest(request.metadata.extra_env_vars), **implicitly() ) + field_set_uncached_env_get = environment_vars_subset( + EnvironmentVarsRequest(request.metadata.uncached_env_vars), **implicitly() + ) + ( pytest_pex, requirements_pex, prepared_sources, field_set_source_files, field_set_extra_env, + field_set_uncached_env, extra_output_directory_digest, ) = await concurrently( pytest_pex_get, @@ -311,6 +317,7 @@ async def setup_pytest_for_target( prepared_sources_get, field_set_source_files_get, field_set_extra_env_get, + field_set_uncached_env_get, extra_output_directory_digest_get, ) @@ -421,6 +428,12 @@ async def setup_pytest_for_target( **field_set_extra_env, } + uncached_env = { + **test_extra_env.uncached_env, + # Same precedence rule as `extra_env` above: the target's own value wins. + **field_set_uncached_env, + } + # Cache test runs only if they are successful, or not at all if `--test-force`. cache_scope = test_subsystem.default_process_cache_scope @@ -464,6 +477,7 @@ async def setup_pytest_for_target( *field_set_source_files.files, ), extra_env=extra_env, + uncached_env=uncached_env, input_digest=input_digest, output_directories=(_EXTRA_OUTPUT_DIR,), output_files=output_files, @@ -501,6 +515,7 @@ async def partition_python_tests( [field_set], python_setup ), extra_env_vars=field_set.extra_env_vars.sorted(), + uncached_env_vars=field_set.uncached_env_vars.sorted(), xdist_concurrency=field_set.xdist_concurrency.value, resolve=field_set.resolve.normalized_value(python_setup), environment=field_set.environment.value, diff --git a/src/python/pants/backend/python/goals/pytest_runner_integration_test.py b/src/python/pants/backend/python/goals/pytest_runner_integration_test.py index e79b46a6042..7d4bcc0cf22 100644 --- a/src/python/pants/backend/python/goals/pytest_runner_integration_test.py +++ b/src/python/pants/backend/python/goals/pytest_runner_integration_test.py @@ -588,6 +588,45 @@ def test_args(): assert result.exit_code == 0 +def test_uncached_env_vars(rule_runner: PythonRuleRunner) -> None: + rule_runner.write_files( + { + f"{PACKAGE}/test_uncached_env_vars.py": dedent( + """\ + import os + + def test_args(): + assert os.getenv("ARG_UNCACHED_WITH_VALUE") == "arg_uncached_with_value" + assert os.getenv("ARG_UNCACHED_WITHOUT_VALUE") == "arg_uncached_without_value" + assert os.getenv("TARGET_UNCACHED_VAR") == "target_uncached_var" + # A name in both lists resolves to the cached value: it is the more specific + # request, and honouring it keeps the process's key honest. + assert os.getenv("IN_BOTH_LISTS") == "from_extra_env_vars" + """ + ), + f"{PACKAGE}/BUILD": dedent( + """\ + python_tests( + uncached_env_vars=("TARGET_UNCACHED_VAR=target_uncached_var",), + extra_env_vars=("IN_BOTH_LISTS=from_extra_env_vars",), + ) + """ + ), + } + ) + tgt = rule_runner.get_target(Address(PACKAGE, relative_file_path="test_uncached_env_vars.py")) + result = run_pytest( + rule_runner, + [tgt], + extra_args=[ + "--test-uncached-env-vars=['ARG_UNCACHED_WITH_VALUE=arg_uncached_with_value', " + "'ARG_UNCACHED_WITHOUT_VALUE', 'IN_BOTH_LISTS=from_uncached_env_vars']" + ], + env={"ARG_UNCACHED_WITHOUT_VALUE": "arg_uncached_without_value"}, + ) + assert result.exit_code == 0 + + def test_pytest_addopts_test_extra_env(rule_runner: PythonRuleRunner) -> None: rule_runner.write_files( { @@ -875,6 +914,18 @@ def test_debug_adaptor_request_argv(rule_runner: PythonRuleRunner) -> None: "python_tests(overrides={'test_2.py': {'extra_env_vars': ['BAR', 'FOO']}})", [[f"{PACKAGE}/test_1.py", f"{PACKAGE}/test_2.py", f"{PACKAGE}/test_3.py"]], ], + # A batch runs as one process, so targets wanting different uncached env must split too: + [ + "__defaults__(dict(python_tests=dict(batch_compatibility_tag='default', uncached_env_vars=['BUILD_ID'])))", + "python_tests(overrides={'test_2.py': {'uncached_env_vars': []}})", + [[f"{PACKAGE}/test_1.py", f"{PACKAGE}/test_3.py"], [f"{PACKAGE}/test_2.py"]], + ], + # Order of uncached_env_vars shouldn't affect partitioning: + [ + "__defaults__(dict(python_tests=dict(batch_compatibility_tag='default', uncached_env_vars=['FOO', 'BAR'])))", + "python_tests(overrides={'test_2.py': {'uncached_env_vars': ['BAR', 'FOO']}})", + [[f"{PACKAGE}/test_1.py", f"{PACKAGE}/test_2.py", f"{PACKAGE}/test_3.py"]], + ], # Partition on different environments: [ "__defaults__(dict(python_tests=dict(batch_compatibility_tag='default')))", diff --git a/src/python/pants/backend/python/subsystems/pytest.py b/src/python/pants/backend/python/subsystems/pytest.py index cf5fe5d8604..20f7627a074 100644 --- a/src/python/pants/backend/python/subsystems/pytest.py +++ b/src/python/pants/backend/python/subsystems/pytest.py @@ -16,6 +16,7 @@ PythonTestsExtraEnvVarsField, PythonTestSourceField, PythonTestsTimeoutField, + PythonTestsUncachedEnvVarsField, PythonTestsXdistConcurrencyField, SkipPythonTestsField, ) @@ -41,6 +42,7 @@ class PythonTestFieldSet(TestFieldSet): timeout: PythonTestsTimeoutField runtime_package_dependencies: RuntimePackageDependenciesField extra_env_vars: PythonTestsExtraEnvVarsField + uncached_env_vars: PythonTestsUncachedEnvVarsField xdist_concurrency: PythonTestsXdistConcurrencyField batch_compatibility_tag: PythonTestsBatchCompatibilityTagField resolve: PythonResolveField diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index cde24ca170b..aafe99792b1 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -29,6 +29,7 @@ TestExtraEnvVarsField, TestsBatchCompatibilityTagField, TestSubsystem, + TestUncachedEnvVarsField, ) from pants.core.target_types import ResolveLikeField, ResolveLikeFieldToValueRequest from pants.engine.addresses import Address, Addresses @@ -1357,6 +1358,10 @@ class PythonTestsExtraEnvVarsField(TestExtraEnvVarsField): pass +class PythonTestsUncachedEnvVarsField(TestUncachedEnvVarsField): + pass + + class PythonTestsXdistConcurrencyField(IntField): alias = "xdist_concurrency" help = help_text( @@ -1397,6 +1402,7 @@ class SkipPythonTestsField(BoolField): PythonTestsBatchCompatibilityTagField, RuntimePackageDependenciesField, PythonTestsExtraEnvVarsField, + PythonTestsUncachedEnvVarsField, InterpreterConstraintsField, SkipPythonTestsField, EnvironmentField, diff --git a/src/python/pants/backend/python/util_rules/pex.py b/src/python/pants/backend/python/util_rules/pex.py index 24cc1f5eef6..a01d054c6d8 100644 --- a/src/python/pants/backend/python/util_rules/pex.py +++ b/src/python/pants/backend/python/util_rules/pex.py @@ -1318,6 +1318,7 @@ class PexProcess: input_digest: Digest | None working_directory: str | None extra_env: FrozenDict[str, str] + uncached_env: FrozenDict[str, str] output_files: tuple[str, ...] | None output_directories: tuple[str, ...] | None timeout_seconds: int | None @@ -1335,6 +1336,7 @@ def __init__( input_digest: Digest | None = None, working_directory: str | None = None, extra_env: Mapping[str, str] | None = None, + uncached_env: Mapping[str, str] | None = None, output_files: Iterable[str] | None = None, output_directories: Iterable[str] | None = None, timeout_seconds: int | None = None, @@ -1349,6 +1351,7 @@ def __init__( object.__setattr__(self, "input_digest", input_digest) object.__setattr__(self, "working_directory", working_directory) object.__setattr__(self, "extra_env", FrozenDict(extra_env or {})) + object.__setattr__(self, "uncached_env", FrozenDict(uncached_env or {})) object.__setattr__(self, "output_files", tuple(output_files) if output_files else None) object.__setattr__( self, "output_directories", tuple(output_directories) if output_directories else None @@ -1383,6 +1386,7 @@ async def setup_pex_process(request: PexProcess, pex_environment: PexEnvironment input_digest=input_digest, working_directory=request.working_directory, env=env, + uncached_env=request.uncached_env, output_files=request.output_files, output_directories=request.output_directories, append_only_caches={ @@ -1405,6 +1409,7 @@ class VenvPexProcess: input_digest: Digest | None working_directory: str | None extra_env: FrozenDict[str, str] + uncached_env: FrozenDict[str, str] output_files: tuple[str, ...] | None output_directories: tuple[str, ...] | None timeout_seconds: int | None @@ -1423,6 +1428,7 @@ def __init__( input_digest: Digest | None = None, working_directory: str | None = None, extra_env: Mapping[str, str] | None = None, + uncached_env: Mapping[str, str] | None = None, output_files: Iterable[str] | None = None, output_directories: Iterable[str] | None = None, timeout_seconds: int | None = None, @@ -1438,6 +1444,7 @@ def __init__( object.__setattr__(self, "input_digest", input_digest) object.__setattr__(self, "working_directory", working_directory) object.__setattr__(self, "extra_env", FrozenDict(extra_env or {})) + object.__setattr__(self, "uncached_env", FrozenDict(uncached_env or {})) object.__setattr__(self, "output_files", tuple(output_files) if output_files else None) object.__setattr__( self, "output_directories", tuple(output_directories) if output_directories else None @@ -1479,6 +1486,7 @@ async def setup_venv_pex_process( input_digest=input_digest, working_directory=request.working_directory, env=request.extra_env, + uncached_env=request.uncached_env, output_files=request.output_files, output_directories=request.output_directories, append_only_caches=append_only_caches, diff --git a/src/python/pants/core/goals/test.py b/src/python/pants/core/goals/test.py index d90a68a0f84..571cd20c430 100644 --- a/src/python/pants/core/goals/test.py +++ b/src/python/pants/core/goals/test.py @@ -568,6 +568,30 @@ class EnvironmentAware: """ ), ) + uncached_env_vars = StrListOption( + help=softwrap( + f""" + Additional environment variables to include in test processes, which do NOT + contribute to the test's cache key. + + {EXTRA_ENV_VARS_USAGE_HELP} + + Use this only for values that cannot change a test's result but unavoidably vary + between runs — a CI build id or job id used to label a report, a token for + uploading results. Such a value in `[test].extra_env_vars` gives every run a + distinct cache key, so no test result is ever reused. + + Anything that can change whether a test passes belongs in `extra_env_vars` + instead. Putting it here means a cached result produced with a different value + gets reused. + + Two limits are worth knowing. On a cache hit the test does not run, so these + values are not observed at all — they cannot be used to make something happen on + every run. And under remote execution they are dropped, because the only channel + to the worker is the request that the cache key is computed from. + """ + ), + ) debug = BoolOption( default=False, @@ -818,6 +842,27 @@ def sorted(self) -> tuple[str, ...]: return tuple(sorted(self.value or ())) +class TestUncachedEnvVarsField(StringSequenceField, metaclass=ABCMeta): + alias = "uncached_env_vars" + help = help_text( + f""" + Additional environment variables to include in test processes, which do NOT contribute to + the test's cache key. + + {EXTRA_ENV_VARS_USAGE_HELP} + + This will be merged with and override values from `[test].uncached_env_vars`. + + See that option for when this is the right tool: values that unavoidably vary between runs + but cannot change whether a test passes. Anything that can change the result belongs in + `extra_env_vars`. + """ + ) + + def sorted(self) -> tuple[str, ...]: + return tuple(sorted(self.value or ())) + + class TestsBatchCompatibilityTagField(StringField, metaclass=ABCMeta): alias = "batch_compatibility_tag" @@ -1189,15 +1234,20 @@ def _format_test_rerun_command(results: Iterable[TestResult]) -> None | str: @dataclass(frozen=True) class TestExtraEnv: env: EnvironmentVars + uncached_env: EnvironmentVars @rule async def get_filtered_environment(test_env_aware: TestSubsystem.EnvironmentAware) -> TestExtraEnv: - return TestExtraEnv( - await environment_vars_subset( + env, uncached_env = await concurrently( + environment_vars_subset( EnvironmentVarsRequest(test_env_aware.extra_env_vars), **implicitly() - ) + ), + environment_vars_subset( + EnvironmentVarsRequest(test_env_aware.uncached_env_vars), **implicitly() + ), ) + return TestExtraEnv(env, uncached_env) @memoized diff --git a/src/python/pants/engine/process.py b/src/python/pants/engine/process.py index 872d47062b5..0a74401e57a 100644 --- a/src/python/pants/engine/process.py +++ b/src/python/pants/engine/process.py @@ -111,6 +111,7 @@ class Process: use_nailgun: tuple[str, ...] working_directory: str | None env: FrozenDict[str, str] + uncached_env: FrozenDict[str, str] = dataclasses.field(compare=False) append_only_caches: FrozenDict[str, str] output_files: tuple[str, ...] output_directories: tuple[str, ...] @@ -134,6 +135,7 @@ def __init__( use_nailgun: Iterable[str] = (), working_directory: str | None = None, env: Mapping[str, str] | None = None, + uncached_env: Mapping[str, str] | None = None, append_only_caches: Mapping[str, str] | None = None, output_files: Iterable[str] | None = None, output_directories: Iterable[str] | None = None, @@ -152,6 +154,15 @@ def __init__( that are not explicitly populated. For example, $PATH will not be defined by default, unless populated through the `env` parameter. + `env` values are part of the process's cache key, so a value that varies per run gives the + process a key nothing else can match. `uncached_env` is the escape hatch for values that + unavoidably vary but cannot change what the process produces — a CI build id used only to + label a report, an upload token. Those are excluded from the cache key and merged into the + environment after the cache is consulted. You own that invariant: if a value there can + change the output, cached results will be wrong. Note also that on a cache hit the process + does not run, so `uncached_env` cannot be used to make something happen on every run, and + that it is dropped under remote execution. + Usually, you will want to provide input files/directories via the parameter `input_digest`. The process will then be able to access these paths through relative paths. If you want to give multiple input digests, first merge them with `merge_digests()`. Files larger than @@ -187,6 +198,7 @@ def __init__( object.__setattr__(self, "use_nailgun", tuple(use_nailgun)) object.__setattr__(self, "working_directory", working_directory) object.__setattr__(self, "env", FrozenDict(env or {})) + object.__setattr__(self, "uncached_env", FrozenDict(uncached_env or {})) object.__setattr__(self, "append_only_caches", FrozenDict(append_only_caches or {})) object.__setattr__(self, "output_files", tuple(output_files or ())) object.__setattr__(self, "output_directories", tuple(output_directories or ())) @@ -483,7 +495,10 @@ def from_process( ) -> InteractiveProcess: return InteractiveProcess( argv=process.argv, - env=process.env, + # An interactive process is never cached, so the distinction `uncached_env` draws does + # not apply here and the values simply belong in the environment. Folding them in keeps + # `--debug` faithful to a normal run; leaving them out silently drops them. + env={**process.uncached_env, **process.env}, description=process.description, input_digest=process.input_digest, forward_signals_to_process=forward_signals_to_process, diff --git a/src/rust/engine/src/nodes/execute_process.rs b/src/rust/engine/src/nodes/execute_process.rs index c2d1b8ab5e2..bb88406acc0 100644 --- a/src/rust/engine/src/nodes/execute_process.rs +++ b/src/rust/engine/src/nodes/execute_process.rs @@ -77,6 +77,8 @@ impl ExecuteProcess { ) -> Result { let env = externs::getattr_from_str_frozendict(value, "env"); + let uncached_env = externs::getattr_from_str_frozendict(value, "uncached_env"); + let working_directory = externs::getattr_as_optional_string(value, "working_directory") .map_err(|e| format!("Failed to get `working_directory` from field: {e}"))? .map(RelativePath::new) @@ -163,6 +165,7 @@ impl ExecuteProcess { Ok(Process { argv: externs::getattr(value, "argv")?, env, + uncached_env, working_directory, input_digests, output_files, diff --git a/src/rust/process_execution/pe_nailgun/src/lib.rs b/src/rust/process_execution/pe_nailgun/src/lib.rs index e59bc161238..c0af2313103 100644 --- a/src/rust/process_execution/pe_nailgun/src/lib.rs +++ b/src/rust/process_execution/pe_nailgun/src/lib.rs @@ -65,6 +65,7 @@ fn construct_nailgun_server_request( description: format!("nailgun server for {nailgun_name}"), level: log::Level::Info, execution_slot_variable: None, + uncached_env: BTreeMap::new(), env: client_request.env, append_only_caches: client_request.append_only_caches, ..client_request diff --git a/src/rust/process_execution/remote/src/remote.rs b/src/rust/process_execution/remote/src/remote.rs index 1b2906fc957..0645620612b 100644 --- a/src/rust/process_execution/remote/src/remote.rs +++ b/src/rust/process_execution/remote/src/remote.rs @@ -905,6 +905,24 @@ impl process_execution::CommandRunner for CommandRunner { _workunit: &mut RunningWorkunit, request: Process, ) -> Result { + // The `Command` proto is the only channel to the worker, and `uncached_env` is defined as + // excluded from it, so there is nowhere to put these values. Warn rather than fail: the + // contract is that they cannot affect what the process produces, so a remote execution + // without them is still correct, just missing whatever they were labelling. + if !request.uncached_env.is_empty() { + warn!( + "Dropping uncached env var(s) {} for {}: they cannot be delivered under remote \ + execution. See the `uncached_env` docs on `Process`.", + request + .uncached_env + .keys() + .map(String::as_str) + .collect::>() + .join(", "), + request.description, + ); + } + // Retrieve capabilities for this server. let capabilities = self.get_capabilities().await?; trace!("RE capabilities: {:?}", capabilities); diff --git a/src/rust/process_execution/remote/src/remote_tests.rs b/src/rust/process_execution/remote/src/remote_tests.rs index d7452285a6a..8c5e55cc926 100644 --- a/src/rust/process_execution/remote/src/remote_tests.rs +++ b/src/rust/process_execution/remote/src/remote_tests.rs @@ -103,6 +103,7 @@ async fn make_execute_request() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -197,6 +198,7 @@ async fn make_execute_request_deduplicates_and_sorts_output_paths() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -237,6 +239,7 @@ async fn make_execute_request_with_root_output_directory_uses_empty_output_path( append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -277,6 +280,7 @@ async fn make_execute_request_with_instance_name() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -395,6 +399,7 @@ async fn make_execute_request_with_cache_key_gen_version() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -673,6 +678,7 @@ async fn make_execute_request_with_timeout() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -777,6 +783,7 @@ async fn make_execute_request_with_append_only_caches() { }, jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, @@ -938,6 +945,7 @@ async fn make_execute_request_using_immutable_inputs() { append_only_caches: BTreeMap::new(), jdk_home: None, execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, cache_scope: ProcessCacheScope::Always, diff --git a/src/rust/process_execution/src/bounded.rs b/src/rust/process_execution/src/bounded.rs index 808f8e714eb..8472105c800 100644 --- a/src/rust/process_execution/src/bounded.rs +++ b/src/rust/process_execution/src/bounded.rs @@ -147,6 +147,17 @@ impl crate::CommandRunner for CommandRunner { ); } + // Merged here rather than at construction because every caching layer wraps this + // runner: by this point the action digest has been computed and the cache consulted, so + // these values reach the process without entering its key. `env` wins a collision, + // since an explicitly cache-keyed value is the more specific request. + for (name, value) in &process.uncached_env { + process + .env + .entry(name.clone()) + .or_insert_with(|| value.clone()); + } + let has_concurrency_template = matches!(process.concurrency, Some(ProcessConcurrency::Range { .. })) || process.concurrency_available > 0; diff --git a/src/rust/process_execution/src/lib.rs b/src/rust/process_execution/src/lib.rs index 8bb45bdc53d..8e07b9bbc97 100644 --- a/src/rust/process_execution/src/lib.rs +++ b/src/rust/process_execution/src/lib.rs @@ -597,6 +597,30 @@ pub struct Process { /// pub env: BTreeMap, + /// + /// Environment variables to set for the execution which do NOT contribute to the process's + /// identity: they are excluded from the `Command` proto, and so from the action digest that + /// keys the local and remote caches. + /// + /// This exists for values that are observably irrelevant to a process's output but unavoidably + /// vary per run — a CI build id used only to label a report, an upload token. Putting such a + /// value in `env` gives every run a distinct cache key and destroys reuse. + /// + /// This is a deliberate hole in the hermeticity contract, and the caller owns the invariant: + /// if a value here can change what the process produces, cached results will be wrong. Two + /// consequences worth stating, because neither is fixable here: + /// + /// * On a cache hit the process does not run at all, so nothing observes these values. + /// They cannot be used to make something happen on every run. + /// * Under remote execution the `Command` proto is the only channel to the worker, so + /// values excluded from it cannot be delivered. They are dropped, and + /// `remote::CommandRunner` warns. + /// + /// Merged into `env` by `bounded::CommandRunner`, below every caching layer. + /// + #[derivative(PartialEq = "ignore", Hash = "ignore")] + pub uncached_env: BTreeMap, + /// /// A relative path to a directory existing in the `input_files` digest to execute the process /// from. Defaults to the `input_files` root. @@ -694,6 +718,7 @@ impl Process { Process { argv, env: BTreeMap::new(), + uncached_env: BTreeMap::new(), working_directory: None, input_digests: InputDigests::default(), output_files: BTreeSet::new(), @@ -726,6 +751,14 @@ impl Process { self } + /// + /// Replaces the cache-excluded environment for this process. See `uncached_env`. + /// + pub fn uncached_env(mut self, uncached_env: BTreeMap) -> Process { + self.uncached_env = uncached_env; + self + } + /// /// Replaces the working_directory for this process. /// diff --git a/src/rust/process_execution/src/tests.rs b/src/rust/process_execution/src/tests.rs index de4f1940fdb..e28a2ddacb5 100644 --- a/src/rust/process_execution/src/tests.rs +++ b/src/rust/process_execution/src/tests.rs @@ -12,6 +12,7 @@ use fs::RelativePath; use prost_types::Timestamp; use protos::pb::build::bazel::remote::execution::v2 as remexec; use remexec::ExecutedActionMetadata; +use store::Store; use tempfile::TempDir; use tokio::io::AsyncWriteExt; use workunit_store::RunId; @@ -57,6 +58,72 @@ fn process_equality() { assert_ne!(hash(&a), hash(&d)); } +#[test] +fn process_equality_ignores_uncached_env() { + fn hash(hashable: &Hashable) -> u64 { + let mut hasher = DefaultHasher::new(); + hashable.hash(&mut hasher); + hasher.finish() + } + + let uncached = |value: &str| { + Process::new(vec![]) + .uncached_env(BTreeMap::from([("BUILD_ID".to_owned(), value.to_owned())])) + }; + let cached = |value: &str| { + Process::new(vec![]).env(BTreeMap::from([("BUILD_ID".to_owned(), value.to_owned())])) + }; + + // The whole point: differing only in `uncached_env` must not change the process's identity, + // otherwise each value gets its own graph node and its own cache lookup. + assert_eq!(uncached("first"), uncached("second")); + assert_eq!(hash(&uncached("first")), hash(&uncached("second"))); + + // The same values in `env` must still separate them, or the exemption has leaked. + assert_ne!(cached("first"), cached("second")); + assert_ne!(hash(&cached("first")), hash(&cached("second"))); +} + +#[tokio::test] +async fn uncached_env_is_absent_from_the_action_digest() { + let store_dir = TempDir::new().unwrap(); + let executor = task_executor::Executor::new(); + let store = Store::local_only(executor, store_dir.path()).unwrap(); + + let digest_of = |process: Process| { + let store = store.clone(); + async move { crate::get_digest(&process, None, None, &store, None).await } + }; + + let base = Process::new(vec!["/bin/true".to_owned()]); + let with_uncached = base + .clone() + .uncached_env(BTreeMap::from([("BUILD_ID".to_owned(), "1234".to_owned())])); + let with_other_uncached = base + .clone() + .uncached_env(BTreeMap::from([("BUILD_ID".to_owned(), "5678".to_owned())])); + let with_cached = base + .clone() + .env(BTreeMap::from([("BUILD_ID".to_owned(), "1234".to_owned())])); + + let (base_action, base_command) = digest_of(base).await; + let (uncached_action, uncached_command) = digest_of(with_uncached).await; + let (other_action, _) = digest_of(with_other_uncached).await; + let (cached_action, cached_command) = digest_of(with_cached).await; + + // Adding an uncached var, or changing its value, must leave both digests untouched: the + // command digest is what the action digest is built from, and the action digest is the cache + // key. + assert_eq!(base_command, uncached_command); + assert_eq!(base_action, uncached_action); + assert_eq!(base_action, other_action); + + // The identical name/value in `env` must change them, confirming this test would catch a + // regression that started folding `uncached_env` into the request. + assert_ne!(base_command, cached_command); + assert_ne!(base_action, cached_action); +} + #[test] fn process_result_metadata_to_and_from_executed_action_metadata() { let env = ProcessExecutionEnvironment { diff --git a/src/rust/process_executor/src/main.rs b/src/rust/process_executor/src/main.rs index 18b6952b56d..44da5b203b9 100644 --- a/src/rust/process_executor/src/main.rs +++ b/src/rust/process_executor/src/main.rs @@ -418,6 +418,7 @@ async fn make_request_from_flat_args( append_only_caches: BTreeMap::new(), jdk_home: args.command.jdk.clone(), execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: args.command.concurrency_available.unwrap_or(0), concurrency: args.command.concurrency.clone(), cache_scope: ProcessCacheScope::Always, @@ -529,6 +530,7 @@ async fn extract_request_from_action_digest( Duration::from_nanos(timeout.nanos as u64 + timeout.seconds as u64 * 1000000000) }), execution_slot_variable: None, + uncached_env: BTreeMap::new(), concurrency_available: 0, concurrency: None, description: "".to_string(),