From 4990fca1e1afc69544b13153edce66dc6d65e57f Mon Sep 17 00:00:00 2001
From: Ed Schouten <eschouten@apple.com>
Date: Mon, 14 Aug 2023 08:46:05 +0000
Subject: [PATCH] build-runfiles: remove temporary file prior to creating it

When building with Remote Output Service, bb_clientd has the ability to
restore snapshots of previous bazel-out/ directories. Though the file
contents of these snapshots are identical to what's created in the past,
the files will be read-only. This is because the files may be shared by
multiple snapshots.

We have noticed that most of Bazel is fine with that. Most of the times
Bazel is a good citizen, where it removes any files before recreating
them. We did notice a very rare case where build-runfiles tries to make
in-place modifications to a temporary file that it maintains. This
change ensures that build-runfiles stops doing this.
---
 src/main/tools/build-runfiles.cc            |  4 +++
 src/test/shell/integration/runfiles_test.sh | 27 +++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/src/main/tools/build-runfiles.cc b/src/main/tools/build-runfiles.cc
index 9a2ef97991353a..8d6b00a972a119 100644
--- a/src/main/tools/build-runfiles.cc
+++ b/src/main/tools/build-runfiles.cc
@@ -117,6 +117,10 @@ class RunfilesCreator {
 
   void ReadManifest(const std::string &manifest_file, bool allow_relative,
                     bool use_metadata) {
+    if (unlink(temp_filename_.c_str()) != 0 && errno != ENOENT) {
+      PDIE("removing temporary file at '%s/%s'", output_base_.c_str(),
+           temp_filename_.c_str());
+    }
     FILE *outfile = fopen(temp_filename_.c_str(), "w");
     if (!outfile) {
       PDIE("opening '%s/%s' for writing", output_base_.c_str(),
diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh
index 2cdc1848aa8e59..d88b3c5e23ba2b 100755
--- a/src/test/shell/integration/runfiles_test.sh
+++ b/src/test/shell/integration/runfiles_test.sh
@@ -427,5 +427,32 @@ EOF
   expect_log_once "Runfiles must not contain middleman artifacts"
 }
 
+function test_removal_of_old_tempfiles() {
+  cat > BUILD << EOF
+sh_binary(
+    name = "foo",
+    srcs = ["foo.sh"],
+)
+EOF
+  touch foo.sh
+  chmod +x foo.sh
+
+  # Build once to create a runfiles directory.
+  bazel build //:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed"
+
+  # Remove the MANIFEST file that was created by the previous build.
+  # Create an inaccessible file in the place where build-runfiles writes
+  # its temporary results.
+  rm ${PRODUCT_NAME}-bin/foo.runfiles/MANIFEST
+  touch ${PRODUCT_NAME}-bin/foo.runfiles/MANIFEST.tmp
+  chmod 0 ${PRODUCT_NAME}-bin/foo.runfiles/MANIFEST.tmp
+
+  # Even with the inaccessible temporary file in place, build-runfiles
+  # should complete successfully. The MANIFEST file should be recreated.
+  bazel build //:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed"
+  [[ -f ${PRODUCT_NAME}-bin/foo.runfiles/MANIFEST ]] \
+    || fail "MANIFEST file not recreated"
+}
+
 
 run_suite "runfiles"