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 https://github.com/bazelbuild/bazel/issues/8104
RELNOTES: None
PiperOrigin-RevId: 245731397
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
index da0920b..8d09d7a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -170,7 +170,7 @@
* @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 @@
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 @@
// 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 @@
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();
diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
index 150289b..fd63ab8 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
+++ b/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)
@@ -690,6 +692,87 @@
}
@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
