Skip to content

Commit

Permalink
Fix CacheContentBehavior::Defer with a remote cache (pantsbuild#16439)
Browse files Browse the repository at this point in the history
`CacheContentBehavior::{Fetch,Defer}` were incorrectly, respectively 1) still using a remote store, 2) not using a remote store.

Fix a swapped condition, and improve test coverage to confirm that those cases can successfully hit.

[ci skip-build-wheels]
  • Loading branch information
stuhood committed Aug 8, 2022
1 parent ffa15ba commit fb55bbc
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 13 deletions.
4 changes: 2 additions & 2 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -1195,8 +1195,8 @@ class BootstrapOptions:
fetching it.
The `defer` behavior, on the other hand, will neither fetch nor validate the cache
content before calling a cache hit a hit. This "defers" actually consuming the cache
entry until a consumer consumes it.
content before calling a cache hit a hit. This "defers" actually fetching the cache
entry until Pants needs it (which may be never).
The `defer` mode is the most network efficient (because it will completely skip network
requests in many cases), followed by the `validate` mode (since it can still skip
Expand Down
2 changes: 1 addition & 1 deletion src/rust/engine/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ impl Core {
)?;

let store = if (exec_strategy_opts.remote_cache_read || exec_strategy_opts.remote_cache_write)
&& remoting_opts.cache_content_behavior == CacheContentBehavior::Defer
&& remoting_opts.cache_content_behavior == CacheContentBehavior::Fetch
{
// In remote cache mode with eager fetching, the only interaction with the remote CAS
// should be through the remote cache code paths. Thus, the store seen by the rest of the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def run() -> tuple[FileDigest, dict[str, int]]:
ProcessOutputEntries,
[
Process(
["/bin/bash", "-c", "sleep 1; echo content > file.txt"],
["/bin/bash", "-c", "/bin/sleep 1 && echo content > file.txt"],
description="Create file.txt",
output_files=["file.txt"],
level=LogLevel.INFO,
Expand All @@ -146,14 +146,21 @@ def run() -> tuple[FileDigest, dict[str, int]]:
assert metrics1["remote_cache_requests"] == 1
assert metrics1["remote_cache_requests_uncached"] == 1

# Then, remove the content from the remote store and run again.
assert cas.remove(file_digest1)
# Confirm that we can hit the cache.
file_digest2, metrics2 = run()
assert file_digest1 == file_digest2
# Validate both that we hit the cache, and that we backtracked to actually run the process.
assert metrics2["remote_cache_requests"] == 1
assert metrics2["remote_cache_requests_cached"] == 1
assert metrics2["backtrack_attempts"] == 1
assert "backtrack_attempts" not in metrics2

# Then, remove the content from the remote store and run again.
assert cas.remove(file_digest1)
file_digest3, metrics3 = run()
assert file_digest1 == file_digest3
# Validate both that we hit the cache, and that we backtracked to actually run the process.
assert metrics3["remote_cache_requests"] == 1
assert metrics3["remote_cache_requests_cached"] == 1
assert metrics3["backtrack_attempts"] == 1


class MockRunSubsystem(GoalSubsystem):
Expand All @@ -170,7 +177,7 @@ async def mock_run(workspace: Workspace, dist_dir: DistDir, mock_run: MockRunSub
result = await Get(
ProcessResult,
Process(
["/bin/bash", "-c", "sleep 1 ; echo content > file.txt"],
["/bin/bash", "-c", "/bin/sleep 1 && echo content > file.txt"],
description="Create file.txt",
output_files=["file.txt"],
level=LogLevel.INFO,
Expand Down Expand Up @@ -258,12 +265,18 @@ def run() -> dict[str, int]:
assert metrics1["remote_cache_requests"] == 1
assert metrics1["remote_cache_requests_uncached"] == 1

# Ensure that we can hit the cache.
metrics2 = run()
assert metrics2["remote_cache_requests"] == 1
assert metrics2["remote_cache_requests_cached"] == 1
assert "backtrack_attempts" not in metrics2

# Then, remove the content from the remote store and run again.
assert cas.remove(
FileDigest("434728a410a78f56fc1b5899c3593436e61ab0c731e9072d95e96db290205e53", 8)
)
metrics2 = run()
metrics3 = run()
# Validate that we missed the cache, and that we didn't backtrack.
assert metrics2["remote_cache_requests"] == 1
assert metrics2["remote_cache_requests_uncached"] == 1
assert "backtrack_attempts" not in metrics2
assert metrics3["remote_cache_requests"] == 1
assert metrics3["remote_cache_requests_uncached"] == 1
assert "backtrack_attempts" not in metrics3

0 comments on commit fb55bbc

Please sign in to comment.