Don't unconditionally update ExternalFilesHelper tracking data at the…

… end of handleDiffsWithMissingDiffInformation: we may skip a full-graph traversal in various circumstances. If that happens, we must preserve existing knowledge for next time. This doesn't have a severe impact on performance because the original goal, of not doing repeated scans when a file had fallen out of the graph, is still met.

PiperOrigin-RevId: 420154787

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -471,19 +471,19 @@ private void handleDiffsWithMissingDiffInformation(
       int fsvcThreads)
       throws InterruptedException {
 
-    // We freshly compute knowledge of the presence of external files in the skyframe graph. We use
-    // a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after* we
-    // are done with the graph scan, lest an interrupt during the graph scan causes us to
-    // incorrectly think there are no longer any external files.
-    ExternalFilesHelper tmpExternalFilesHelper =
-        externalFilesHelper.cloneWithFreshExternalFilesKnowledge();
-
     ExternalFilesKnowledge externalFilesKnowledge = externalFilesHelper.getExternalFilesKnowledge();
     if (!pathEntriesWithoutDiffInformation.isEmpty()
         || (externalFilesKnowledge.anyOutputFilesSeen && checkOutputFiles)
         || repositoryHelpersHolder != null
         || externalFilesKnowledge.anyFilesInExternalReposSeen
         || externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+      // We freshly compute knowledge of the presence of external files in the skyframe graph. We
+      // use
+      // a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after* we
+      // are done with the graph scan, lest an interrupt during the graph scan causes us to
+      // incorrectly think there are no longer any external files.
+      ExternalFilesHelper tmpExternalFilesHelper =
+          externalFilesHelper.cloneWithFreshExternalFilesKnowledge();
 
       // Before running the FilesystemValueChecker, ensure that all values marked for invalidation
       // have actually been invalidated (recall that invalidation happens at the beginning of the
@@ -514,7 +514,8 @@ private void handleDiffsWithMissingDiffInformation(
       if (checkOutputFiles) {
         fileTypesToCheck.add(FileType.OUTPUT);
       }
-      if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+      if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
+          || !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
         fileTypesToCheck.add(FileType.EXTERNAL);
       }
       logger.atInfo().log(
@@ -540,9 +541,12 @@ private void handleDiffsWithMissingDiffInformation(
           batchDirtyResult,
           /*numSourceFilesCheckedIfDiffWasMissing=*/ batchDirtyResult.getNumKeysChecked(),
           managedDirectoriesChanged);
-    }
-    if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()
-        && !externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+      // We use the knowledge gained during the graph scan that just completed. Otherwise, naively,
+      // once an external file gets into the Skyframe graph, we'll overly-conservatively always
+      // think the graph needs to be scanned.
+      externalFilesHelper.setExternalFilesKnowledge(
+          tmpExternalFilesHelper.getExternalFilesKnowledge());
+    } else if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
       logger.atInfo().log(
           "About to scan %d external files",
           externalFilesKnowledge.nonOutputExternalFilesSeen.size());
@@ -566,8 +570,7 @@ private void handleDiffsWithMissingDiffInformation(
         batchDirtyResult =
             fsvc.getDirtyKeys(
                 externalDirtyNodes,
-                new ExternalDirtinessChecker(
-                    tmpExternalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
+                new ExternalDirtinessChecker(externalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
       }
       handleChangedFiles(
           ImmutableList.of(),
@@ -579,11 +582,6 @@ private void handleDiffsWithMissingDiffInformation(
         pathEntriesWithoutDiffInformation) {
       pair.getSecond().markProcessed();
     }
-    // We use the knowledge gained during the graph scan that just completed. Otherwise, naively,
-    // once an external file gets into the Skyframe graph, we'll overly-conservatively always think
-    // the graph needs to be scanned.
-    externalFilesHelper.setExternalFilesKnowledge(
-        tmpExternalFilesHelper.getExternalFilesKnowledge());
   }
 
   private void handleChangedFiles(

src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.vfs.DelegateFileSystem;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.NotifyingHelper;
@@ -126,6 +127,34 @@ public void changedFile_statFails_throwsError() throws Exception {
     assertThat(e.getCause()).hasCauseThat().hasCauseThat().isSameInstanceAs(injectedException);
   }
 
+  @Test
+  public void ignoreOutputFilesThenCheckAgainDoesCheck() throws Exception {
+    Path buildFile =
+        write(
+            "foo/BUILD",
+            "genrule(name = 'foo', outs = ['out'], cmd = 'cp $< $@', srcs = ['link'])");
+    Path outputFile = directories.getOutputBase().getChild("linkTarget");
+    FileSystemUtils.writeContentAsLatin1(outputFile, "one");
+    buildFile.getParentDirectory().getChild("link").createSymbolicLink(outputFile.asFragment());
+
+    buildTarget("//foo:foo");
+
+    assertContents("one", "//foo:foo");
+
+    addOptions("--noexperimental_check_output_files");
+    FileSystemUtils.writeContentAsLatin1(outputFile, "two");
+
+    buildTarget("//foo:foo");
+
+    assertContents("one", "//foo:foo");
+
+    addOptions("--experimental_check_output_files");
+
+    buildTarget("//foo:foo");
+
+    assertContents("two", "//foo:foo");
+  }
+
   @Test
   public void externalSymlink_doesNotTriggerFullGraphTraversal() throws Exception {
     addOptions("--symlink_prefix=/");