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 da0920bd8cf31e..8d09d7aed962e8 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 @@ 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 symlinksInDirectories = new ArrayList<>(); for (Entry 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 150289bab3c492..fd63ab88f69a86 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) @@ -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