Don't mark ErrorInfo transient if one of its child ErrorInfos is transient.
Parents should depend on their children's transience to force their re-evaluation, not say directly that they are transient. This can lead to an inconsistency between whether an ErrorInfo says it is transient and whether the node depends on the ErrorTransienceValue.
This should reduce the number of edges in the graph in case of transient error without sacrificing correctness.
--
MOS_MIGRATED_REVID=101537029
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
index 6873d19..09925a3 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -76,7 +76,6 @@
ImmutableList.Builder<CycleInfo> cycleBuilder = ImmutableList.builder();
Exception firstException = null;
SkyKey firstChildKey = null;
- boolean isTransient = false;
boolean isCatastrophic = false;
// Arbitrarily pick the first error.
for (ErrorInfo child : childErrors) {
@@ -86,14 +85,15 @@
}
builder.addTransitive(child.rootCauses);
cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles));
- isTransient |= child.isTransient();
isCatastrophic |= child.isCatastrophic();
}
this.rootCauses = builder.build();
this.exception = firstException;
this.rootCauseOfException = firstChildKey;
this.cycles = cycleBuilder.build();
- this.isTransient = isTransient;
+ // Parent errors should not be transient -- we depend on the child's transience, if any, to
+ // force re-evaluation if necessary.
+ this.isTransient = false;
this.isCatastrophic = isCatastrophic;
}
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 31df0f9..69fd465 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -836,6 +836,33 @@
assertThat(cycleInfo.getPathToCycle()).isEmpty();
}
+ @Test
+ public void parentOfCycleAndTransientNotTransient() throws Exception {
+ initializeTester();
+ SkyKey cycleKey1 = GraphTester.toSkyKey("cycleKey1");
+ SkyKey cycleKey2 = GraphTester.toSkyKey("cycleKey2");
+ SkyKey mid = GraphTester.toSkyKey("mid");
+ SkyKey errorKey = GraphTester.toSkyKey("errorKey");
+ tester.getOrCreate(cycleKey1).addDependency(cycleKey2).setComputedValue(COPY);
+ tester.getOrCreate(cycleKey2).addDependency(cycleKey1).setComputedValue(COPY);
+ tester.getOrCreate(errorKey).setHasTransientError(true);
+ tester.getOrCreate(mid).addErrorDependency(errorKey, new StringValue("recovered"))
+ .setComputedValue(COPY);
+ SkyKey top = GraphTester.toSkyKey("top");
+ CountDownLatch topEvaluated = new CountDownLatch(2);
+ tester.getOrCreate(top).setBuilder(new ChainedFunction(topEvaluated, null, null, false,
+ new StringValue("unused"), ImmutableList.of(mid, cycleKey1)));
+ ErrorInfo errorInfo = tester.evalAndGetError(top);
+ assertThat(topEvaluated.getCount()).isEqualTo(1);
+ assertThat(errorInfo.isTransient()).isFalse();
+ assertWithMessage(errorInfo.toString())
+ .that(errorInfo.getCycleInfo())
+ .containsExactly(
+ new CycleInfo(ImmutableList.of(top), ImmutableList.of(cycleKey1, cycleKey2)));
+ assertThat(errorInfo.getException()).hasMessage("Type:errorKey");
+ assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey);
+ }
+
/**
* Regression test: IllegalStateException in BuildingState.isReady(). The ParallelEvaluator used
* to assume during cycle-checking that all values had been built as fully as possible -- that
