Fix NullPointerException in SkyframeLookupContinuation.doLookup().

The code mistakenly assumed that eviction could not occur concurrently with
SkyFunction.compute. Synchronizes abandon and process to avoid concurrent
modification of skyframeLookups.

PiperOrigin-RevId: 979963369
Change-Id: Iee108a1420706d38952f9e9ee825751778d78a0d

src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyframeLookupContinuation.java

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 53566b3..5c3ac8f 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
@@ -36,6 +36,9 @@
  *
  * <p>This class is designed to reside in {@link SkyKeyComputeState}. In particular, note that
  * {@link #abandon} should be called.
+ *
+ * <p>This class is thread-safe because {@link #abandon} may be called concurrently by a background
+ * thread (e.g. state eviction) while {@link #process} is executing on an evaluator thread.
  */
 public final class SkyframeLookupContinuation {
   private final ArrayDeque<SkyframeLookup<?>> skyframeLookups;
@@ -75,7 +78,7 @@
    *     occurring in other threads) or null if a Skyframe restart is needed
    */
   @Nullable
-  public ListenableFuture<?> process(LookupEnvironment env)
+  public synchronized ListenableFuture<?> process(LookupEnvironment env)
       throws InterruptedException, SkyframeDependencyException, LookupAbandonedException {
     return switch (state) {
       case LOOKUP -> doLookup(env);
@@ -90,7 +93,7 @@
    * <p>This must be called if the lookups cannot be completed, for example, if {@link
    * SkyKeyComputeState#close} is called on any containing compute state or if there's an error.
    */
-  public void abandon(LookupAbandonedException exception) {
+  public synchronized void abandon(LookupAbandonedException exception) {
     for (SkyframeLookup<?> lookup : skyframeLookups) {
       lookup.abandon(exception);
     }

src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java

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 c446016..d5cd263 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
@@ -37,6 +37,7 @@
 import com.google.devtools.build.lib.skyframe.serialization.DeferredObjectCodec.DeferredValue;
 import com.google.devtools.build.lib.skyframe.serialization.DependOnFutureShim.ObservedFutureStatus;
 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.lib.skyframe.serialization.SkyValueRetriever.NoCachedData;
 import com.google.devtools.build.lib.skyframe.serialization.SkyValueRetriever.RetrievalContext;
@@ -65,11 +66,16 @@
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
 import org.junit.Test;
@@ -911,6 +917,67 @@
   }
 
   @Test
+  public void abandon_concurrentWithDoLookup_synchronizesSafely() throws Exception {
+    var parent1 = new AtomicReference<Object>();
+    var lookup1 =
+        new SkyframeLookup<AtomicReference<Object>>(
+            new ExampleKey("a"), parent1, AtomicReference::set);
+    var parent2 = new AtomicReference<Object>();
+    var lookup2 =
+        new SkyframeLookup<AtomicReference<Object>>(
+            new ExampleKey("b"), parent2, AtomicReference::set);
+
+    var lookups = new ArrayDeque<SkyframeLookup<?>>();
+    lookups.add(lookup1);
+    lookups.add(lookup2);
+    var resultFuture = SettableFuture.create();
+    var continuation = new SkyframeLookupContinuation(lookups, resultFuture);
+
+    var doLookupEntered = new CountDownLatch(1);
+    var abandonStarted = new CountDownLatch(1);
+    var executor = Executors.newSingleThreadExecutor();
+    Future<?> abandonFuture;
+    try {
+      abandonFuture =
+          executor.submit(
+              () -> {
+                try {
+                  doLookupEntered.await();
+                  abandonStarted.countDown();
+                  continuation.abandon(new StateEvictedException());
+                } catch (InterruptedException e) {
+                  Thread.currentThread().interrupt();
+                }
+              });
+
+      ListenableFuture<?> processResult =
+          continuation.process(
+              new EnvironmentForUtilities(
+                  k -> {
+                    doLookupEntered.countDown();
+                    try {
+                      abandonStarted.await();
+                      // Brief pause to allow the background thread to attempt abandon(),
+                      // verifying that it blocks on continuation's monitor until doLookup finishes.
+                      Thread.sleep(20);
+                    } catch (InterruptedException e) {
+                      Thread.currentThread().interrupt();
+                    }
+                    return null; // Returns null to trigger a restart.
+                  }));
+      abandonFuture.get();
+      assertThat(processResult).isNull();
+
+      // Subsequent process() call after restart: lookups were abandoned, so it returns resultFuture
+      // cleanly.
+      assertThat(continuation.process(new EnvironmentForUtilities(k -> null)))
+          .isSameInstanceAs(resultFuture);
+    } finally {
+      executor.shutdown();
+    }
+  }
+
+  @Test
   public void exceptionWhileWaitingForResult_throwsException() throws Exception {
     var fingerprintValueService = FingerprintValueService.createForAnalysisCacheTesting();
     var analysisCacheServiceData = new HashMap<ByteString, ByteString>();