Remove isFailed shadow state, using Future as the single source of truth. Previously, SkyframeLookup tracked failure using a separate isFailed boolean updated after AbstractFuture.setException(). This introduced a TOCTOU race window, did not reflect cancellation status, and caused IllegalStateException in throwDependencyExceptionIfFailed() when abandon() raced with completed lookups. This change: 1. Removes isFailed from SkyframeLookup entirely. 2. Evaluates lookup status directly via Futures.getDone() in throwDependencyExceptionIfFailed(), handling ExecutionException and CancellationException. PiperOrigin-RevId: 979961181 Change-Id: Ia530a6cf9942e32adbc645c8e1b8b875fbe97876
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java index 7e50be2..8ec9244 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java
@@ -35,7 +35,6 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult.QueryDepCallback; -import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ByteString; import com.google.protobuf.CodedInputStream; import java.io.ByteArrayInputStream; @@ -609,9 +608,6 @@ private final T parent; private final FieldSetter<? super T> setter; - /** Set true if the Skyframe dependency has an exception. */ - private boolean isFailed = false; - @VisibleForTesting SkyframeLookup(SkyKey key, T parent, FieldSetter<? super T> setter) { this.key = key; @@ -639,20 +635,9 @@ return true; } - boolean isFailed() { - return isFailed; - } - void abandon(LookupAbandonedException exception) { setException(exception); } - - @Override - @CanIgnoreReturnValue - protected boolean setException(Throwable t) { - this.isFailed = true; - return super.setException(t); - } } /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupContinuation.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupContinuation.java index 8836a97..53566b3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupContinuation.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupContinuation.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.util.ArrayDeque; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; @@ -173,13 +174,10 @@ private void throwDependencyExceptionIfFailed(SkyframeLookup<?> lookup) throws SkyframeDependencyException, LookupAbandonedException { - if (!lookup.isFailed()) { - return; - } - this.state = State.ENDED; try { var unused = Futures.getDone(lookup); } catch (ExecutionException e) { + this.state = State.ENDED; Throwable cause = e.getCause(); if (cause instanceof SkyframeDependencyException sde) { abandon(new PeerFailedException(sde)); @@ -190,7 +188,11 @@ throw lae; } throw new AssertionError("unexpected exception: " + lookup, cause); + } catch (CancellationException e) { + this.state = State.ENDED; + var lae = new LookupAbandonedException(e); + abandon(lae); + throw lae; } - throw new IllegalStateException("should have thrown an exception: " + lookup); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java index 0dc66f7..c446016 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe.serialization; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.util.concurrent.Futures.getDone; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.devtools.build.lib.skyframe.serialization.DependOnFutureShim.ObservedFutureStatus.DONE; import static com.google.devtools.build.lib.skyframe.serialization.DependOnFutureShim.ObservedFutureStatus.NOT_DONE; @@ -707,6 +708,85 @@ } @Test + public void skyframeLookupError_withPriorSuccessfulLookup_preservesSuccessfulLookup() + throws Exception { + var fingerprintValueService = FingerprintValueService.createForAnalysisCacheTesting(); + var analysisCacheServiceData = new HashMap<ByteString, ByteString>(); + var state = new RetrievalContext(); + RemoteAnalysisCacheClient analysisCacheClient = + createFakeAnalysisCacheClient(analysisCacheServiceData); + + var key = new TrivialKey("a"); + + var lookupKey0 = new ExampleKey("a"); + var lookupKey1 = new ExampleKey("b"); + var multiLookupValue = + new MultiLookupValue(new ExampleValue(lookupKey0, 3), new ExampleValue(lookupKey1, 5)); + uploadKeyValuePair( + key, + multiLookupValue, + COMPRESSION_SERVICE, + fingerprintValueService, + analysisCacheServiceData); + + RetrievalResult result = + createSkyValueRetriever(fingerprintValueService, codecs, CONSTANT_FOR_TESTING) + .tryRetrieve( + new EnvironmentForUtilities(k -> null), + SkyValueRetrieverTest::alwaysDoneDependOnFuture, + analysisCacheClient, + key, + state); + + assertThat(result).isEqualTo(RESTART); + assertThat(state.getState()).isInstanceOf(WaitingForLookupContinuation.class); + + var lookups = + ImmutableList.copyOf( + ((WaitingForLookupContinuation) state.getState()) + .continuation() + .getSkyframeLookupsForTesting()); + assertThat(lookups).hasSize(2); + + var error = new Exception(); + var thrown = + assertThrows( + SerializationException.class, + () -> + createSkyValueRetriever(fingerprintValueService, codecs, CONSTANT_FOR_TESTING) + .tryRetrieve( + new EnvironmentForUtilities( + k -> { + if (k.equals(lookupKey0)) { + return new ExampleValue(lookupKey0, 3); + } + if (k.equals(lookupKey1)) { + return error; + } + return null; + }), + SkyValueRetrieverTest::alwaysDoneDependOnFuture, + analysisCacheClient, + key, + state)); + assertThat(thrown) + .hasMessageThat() + .contains("skyframe dependency error during deserialization for " + key); + assertThat(thrown).hasCauseThat().isInstanceOf(SkyframeDependencyException.class); + assertThat(thrown).hasCauseThat().hasCauseThat().isSameInstanceAs(error); + + // Verifies that the successful lookup is NOT marked failed. + assertThat(lookups.get(0).isDone()).isTrue(); + assertThat(getDone(lookups.get(0))).isNull(); + + // Verifies that the failed lookup has the expected error. + assertThat(lookups.get(1).isDone()).isTrue(); + var thrownByLookup1 = assertThrows(ExecutionException.class, lookups.get(1)::get).getCause(); + assertThat(thrownByLookup1).isInstanceOf(SkyframeDependencyException.class); + assertThat(thrownByLookup1).hasCauseThat().isSameInstanceAs(error); + } + + @Test public void skyframeLookupStateEvicted_handlesEvictionGracefully() throws Exception { var fingerprintValueService = FingerprintValueService.createForAnalysisCacheTesting(); var analysisCacheServiceData = new HashMap<ByteString, ByteString>();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupCollectorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupCollectorTest.java index 8b4be93..d2b5e16 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupCollectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupCollectorTest.java
@@ -16,10 +16,13 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.skyframe.serialization.SharedValueDeserializationContext.PeerFailedException; import com.google.devtools.build.lib.skyframe.serialization.SharedValueDeserializationContext.SkyframeLookup; +import com.google.devtools.build.lib.skyframe.serialization.SharedValueDeserializationContext.StateEvictedException; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; @@ -66,8 +69,40 @@ assertHasPeerFailure(lookup3, exception); } + @Test + public void abandon_onCompletedLookup_preservesSuccessResult() throws Exception { + var parent = new AtomicReference<Object>(); + var key = createDummyKey(); + var lookup = new SkyframeLookup<AtomicReference<Object>>(key, parent, AtomicReference::set); + var value = new SkyValue() {}; + + lookup.acceptValue(key, value); + assertThat(lookup.isDone()).isTrue(); + assertThat(Futures.getDone(lookup)).isNull(); + assertThat(parent.get()).isSameInstanceAs(value); + + // Calling abandon on an already-completed lookup must not change the result. + lookup.abandon(new StateEvictedException()); + assertThat(lookup.isDone()).isTrue(); + assertThat(Futures.getDone(lookup)).isNull(); + } + + @Test + public void tryHandleException_completesWithSkyframeDependencyException() { + var parent = new AtomicReference<Object>(); + var key = createDummyKey(); + var lookup = new SkyframeLookup<AtomicReference<Object>>(key, parent, AtomicReference::set); + var exception = new Exception("skyframe error"); + + assertThat(lookup.tryHandleException(key, exception)).isTrue(); + assertThat(lookup.isDone()).isTrue(); + var thrown = assertThrows(ExecutionException.class, lookup::get); + assertThat(thrown).hasCauseThat().isInstanceOf(SkyframeDependencyException.class); + assertThat(thrown).hasCauseThat().hasCauseThat().isSameInstanceAs(exception); + } + private static void assertHasPeerFailure(SkyframeLookup<?> lookup, Exception exception) { - assertThat(lookup.isFailed()).isTrue(); + assertThat(lookup.isDone()).isTrue(); var thrown = assertThrows(ExecutionException.class, lookup::get); var cause = thrown.getCause(); assertThat(cause).isInstanceOf(PeerFailedException.class);