Check if action cache should be updated only if it's enabled
Prevents action rewinding related failures when the action cache is
disabled.
RELNOTES: None.
PiperOrigin-RevId: 228230225
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index 4b72809..3484906 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -101,6 +101,11 @@
return !executionFilter.apply(action);
}
+ /** Whether the action cache is enabled. */
+ public boolean enabled() {
+ return cacheConfig.enabled();
+ }
+
/**
* Checks whether one of existing output paths is already used as a key.
* If yes, returns it - otherwise uses first output file as a key
@@ -326,14 +331,12 @@
}
}
- public void afterExecution(
+ public void updateActionCache(
Action action, Token token, MetadataHandler metadataHandler, Map<String, String> clientEnv)
throws IOException {
- if (!cacheConfig.enabled()) {
- // Action cache is disabled, don't generate digests.
- return;
- }
- Preconditions.checkArgument(token != null);
+ Preconditions.checkState(
+ cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action);
+ Preconditions.checkArgument(token != null, "token unexpectedly null, action: %s", action);
String key = token.cacheKey;
if (actionCache.get(key) != null) {
// This cache entry has already been updated by a shared action. We don't need to do it again.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 09abbdb..16b5bd5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -738,7 +738,7 @@
}
}
Preconditions.checkState(!env.valuesMissing(), action);
- skyframeActionExecutor.afterExecution(
+ skyframeActionExecutor.updateActionCacheIfReallyExecuted(
action, metadataHandler, state.token, clientEnv, actionLookupData);
return state.value;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index a6a20bf..edf40b7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -488,6 +488,9 @@
}
private boolean actionReallyExecuted(Action action, ActionLookupData actionLookupData) {
+ // TODO(b/19539699): This method is used only when the action cache is enabled. It is
+ // incompatible with action rewinding, which removes entries from buildActionMap. Action
+ // rewinding is used only with a disabled action cache.
Pair<ActionLookupData, ?> cachedRun =
Preconditions.checkNotNull(
buildActionMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())),
@@ -665,12 +668,15 @@
return token;
}
- void afterExecution(
+ void updateActionCacheIfReallyExecuted(
Action action,
MetadataHandler metadataHandler,
Token token,
Map<String, String> clientEnv,
ActionLookupData actionLookupData) {
+ if (!actionCacheChecker.enabled()) {
+ return;
+ }
if (!actionReallyExecuted(action, actionLookupData)) {
// If an action shared with this one executed, then we need not update the action cache, since
// the other action will do it. Moreover, this action is not aware of metadata acquired
@@ -678,7 +684,7 @@
return;
}
try {
- actionCacheChecker.afterExecution(action, token, metadataHandler, clientEnv);
+ actionCacheChecker.updateActionCache(action, token, metadataHandler, clientEnv);
} catch (IOException e) {
// Skyframe has already done all the filesystem access needed for outputs and swallows
// IOExceptions for inputs. So an IOException is impossible here.
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
index eba285b..23d5661 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
@@ -109,7 +109,7 @@
action, null, clientEnv, null, metadataHandler);
if (token != null) {
// Real action execution would happen here.
- cacheChecker.afterExecution(action, token, metadataHandler, clientEnv);
+ cacheChecker.updateActionCache(action, token, metadataHandler, clientEnv);
}
}
