Download stderr/stdout to a temporary FileOutErr

On Windows, we cannot delete file without closing the FileOutputStream, but we may need to write to the FileOutErr in fallback execution, so we cannot close the OutputStreams.

By using a temporary child FileOutErr, we can safely close and delete the stdout/stderr files of it when the download fails and the original FileOutErr will still be writable.

Fixes #8104

RELNOTES: None
PiperOrigin-RevId: 245731397

src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java

@@ -170,7 +170,7 @@ public void onFailure(Throwable t) {
    * @throws IOException in case of a cache miss or if the remote cache is unavailable.
    * @throws ExecException in case clean up after a failed download failed.
    */
-  public void download(ActionResult result, Path execRoot, FileOutErr outErr)
+  public void download(ActionResult result, Path execRoot, FileOutErr origOutErr)
       throws ExecException, IOException, InterruptedException {
     ActionResultMetadata metadata = parseActionResultMetadata(result, execRoot);
 
@@ -197,8 +197,12 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
 
     IOException downloadException = null;
     InterruptedException interruptedException = null;
+    FileOutErr tmpOutErr = null;
     try {
-      downloads.addAll(downloadOutErr(result, outErr));
+      if (origOutErr != null) {
+        tmpOutErr = origOutErr.childOutErr();
+      }
+      downloads.addAll(downloadOutErr(result, tmpOutErr));
     } catch (IOException e) {
       downloadException = e;
     }
@@ -228,9 +232,9 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
           // directories will not be re-created
           execRoot.getRelative(directory.getPath()).deleteTreesBelow();
         }
-        if (outErr != null) {
-          outErr.getOutputPath().delete();
-          outErr.getErrorPath().delete();
+        if (tmpOutErr != null) {
+          tmpOutErr.clearOut();
+          tmpOutErr.clearErr();
         }
       } catch (IOException e) {
         // If deleting of output files failed, we abort the build with a decent error message as
@@ -254,6 +258,12 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr)
       throw downloadException;
     }
 
+    if (tmpOutErr != null) {
+      FileOutErr.dump(tmpOutErr, origOutErr);
+      tmpOutErr.clearOut();
+      tmpOutErr.clearErr();
+    }
+
     List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
     for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
       entry.getKey().createDirectoryAndParents();

src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java

@@ -21,6 +21,7 @@
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import build.bazel.remote.execution.v2.Action;
 import build.bazel.remote.execution.v2.ActionResult;
@@ -85,6 +86,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.Mockito;
 
 /** Tests for {@link AbstractRemoteActionCache}. */
 @RunWith(JUnit4.class)
@@ -689,6 +691,87 @@ public void onErrorWaitForRemainingDownloadsToComplete() throws Exception {
     }
   }
 
+  @Test
+  public void testDownloadWithStdoutStderrOnSuccess() throws Exception {
+    // Tests that fetching stdout/stderr as a digest works and that OutErr is still
+    // writable afterwards.
+    Path stdout = fs.getPath("/execroot/stdout");
+    Path stderr = fs.getPath("/execroot/stderr");
+    FileOutErr outErr = new FileOutErr(stdout, stderr);
+    FileOutErr childOutErr = outErr.childOutErr();
+    FileOutErr spyOutErr = Mockito.spy(outErr);
+    FileOutErr spyChildOutErr = Mockito.spy(childOutErr);
+    when(spyOutErr.childOutErr()).thenReturn(spyChildOutErr);
+
+    DefaultRemoteActionCache cache = newTestCache();
+    Digest digestStdout = cache.addContents("stdout");
+    Digest digestStderr = cache.addContents("stderr");
+
+    ActionResult result =
+        ActionResult.newBuilder()
+            .setExitCode(0)
+            .setStdoutDigest(digestStdout)
+            .setStderrDigest(digestStderr)
+            .build();
+
+    cache.download(result, execRoot, spyOutErr);
+
+    verify(spyOutErr, Mockito.times(2)).childOutErr();
+    verify(spyChildOutErr).clearOut();
+    verify(spyChildOutErr).clearErr();
+    assertThat(outErr.getOutputPath().exists()).isTrue();
+    assertThat(outErr.getErrorPath().exists()).isTrue();
+
+    try {
+      outErr.getOutputStream().write(0);
+      outErr.getErrorStream().write(0);
+    } catch (IOException err) {
+      throw new AssertionError("outErr should still be writable after download finished.", err);
+    }
+  }
+
+  @Test
+  public void testDownloadWithStdoutStderrOnFailure() throws Exception {
+    // Test that when downloading stdout/stderr fails the OutErr is still writable
+    // and empty.
+    Path stdout = fs.getPath("/execroot/stdout");
+    Path stderr = fs.getPath("/execroot/stderr");
+    FileOutErr outErr = new FileOutErr(stdout, stderr);
+    FileOutErr childOutErr = outErr.childOutErr();
+    FileOutErr spyOutErr = Mockito.spy(outErr);
+    FileOutErr spyChildOutErr = Mockito.spy(childOutErr);
+    when(spyOutErr.childOutErr()).thenReturn(spyChildOutErr);
+
+    DefaultRemoteActionCache cache = newTestCache();
+    // Don't add stdout/stderr as a known blob to the remote cache so that downloading it will fail
+    Digest digestStdout = digestUtil.computeAsUtf8("stdout");
+    Digest digestStderr = digestUtil.computeAsUtf8("stderr");
+
+    ActionResult result =
+        ActionResult.newBuilder()
+            .setExitCode(0)
+            .setStdoutDigest(digestStdout)
+            .setStderrDigest(digestStderr)
+            .build();
+    try {
+      cache.download(result, execRoot, spyOutErr);
+      fail("Expected IOException");
+    } catch (IOException e) {
+      verify(spyOutErr, Mockito.times(2)).childOutErr();
+      verify(spyChildOutErr).clearOut();
+      verify(spyChildOutErr).clearErr();
+      assertThat(outErr.getOutputPath().exists()).isFalse();
+      assertThat(outErr.getErrorPath().exists()).isFalse();
+
+      try {
+        outErr.getOutputStream().write(0);
+        outErr.getErrorStream().write(0);
+      } catch (IOException err) {
+        throw new AssertionError("outErr should still be writable after download failed.", err);
+      }
+    }
+  }
+
   @Test
   public void testDownloadMinimalFiles() throws Exception {
     // Test that injecting the metadata for a remote output file works