Improve resiliency of concurrent use the downloads directory.

include/vcpkg/archives.h

@@ -36,12 +36,7 @@ namespace vcpkg
                          MessageSink& status_sink,
                          const Path& archive,
                          const Path& to_path);
-    // set `to_path` to `archive` contents.
-    void set_directory_to_archive_contents(const Filesystem& fs,
-                                           const ToolCache& tools,
-                                           MessageSink& status_sink,
-                                           const Path& archive,
-                                           const Path& to_path);
+    // extract `archive` to a sibling temporary subdirectory of `to_path` and returns that path
     Path extract_archive_to_temp_subdirectory(const Filesystem& fs,
                                               const ToolCache& tools,
                                               MessageSink& status_sink,

include/vcpkg/base/files.h

@@ -246,6 +246,16 @@ namespace vcpkg
         void rename_with_retry(const Path& old_path, const Path& new_path, std::error_code& ec) const;
         void rename_with_retry(const Path& old_path, const Path& new_path, LineInfo li) const;
 
+        // Rename old_path -> new_path, but consider new_path already existing as acceptable.
+        // Traditionally used to interact with downloads, or git tree cache, where multiple
+        // instances of vcpkg may be trying to do the same action at the same time.
+        //
+        // Returns whether the rename actually happened.
+        //
+        // If `old_path` and `new_path` resolve to the same file, the behavior is undefined.
+        bool rename_cas_like(const Path& old_path, const Path& new_path, std::error_code& ec) const;
+        bool rename_cas_like(const Path& old_path, const Path& new_path, LineInfo li) const;
+
         virtual void rename_or_copy(const Path& old_path,
                                     const Path& new_path,
                                     StringLiteral temp_suffix,

src/vcpkg-test/files.cpp

@@ -914,6 +914,100 @@ TEST_CASE ("copy_file", "[files]")
     CHECK_EC_ON_FILE(temp_dir, ec);
 }
 
+TEST_CASE ("rename", "[files]")
+{
+    urbg_t urbg;
+
+    auto& fs = setup();
+
+    auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
+    INFO("temp dir is: " << temp_dir.native());
+
+    static constexpr StringLiteral FileTxt = "file.txt";
+    fs.remove_all(temp_dir, VCPKG_LINE_INFO);
+    fs.create_directory(temp_dir, VCPKG_LINE_INFO);
+    auto temp_dir_a = temp_dir / "a";
+    fs.create_directory(temp_dir_a, VCPKG_LINE_INFO);
+    auto temp_dir_a_file = temp_dir_a / FileTxt;
+    auto temp_dir_b = temp_dir / "b";
+    auto temp_dir_b_file = temp_dir_b / FileTxt;
+
+    static constexpr StringLiteral text_file_contents = "hello there";
+    fs.write_contents(temp_dir_a_file, text_file_contents, VCPKG_LINE_INFO);
+
+    // try rename_with_retry
+    {
+        fs.rename_with_retry(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO);
+        REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
+        REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
+
+        // put things back
+        fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
+        REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
+        REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
+    }
+
+    // try rename_cas_like directory, target does not exist
+    {
+        REQUIRE(fs.rename_cas_like(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO));
+        REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
+        REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
+
+        // put things back
+        fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
+        REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
+        REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
+    }
+
+    // try rename_cas_like directory, target exists
+    {
+        fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
+        fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO);
+
+        // Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared
+        REQUIRE(!fs.rename_cas_like(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO));
+        REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
+        REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
+
+        // put things back
+        fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
+        REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
+        REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
+    }
+
+    // try rename_cas_like file, target does not exist
+    {
+        fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
+        REQUIRE(fs.rename_cas_like(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO));
+        REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO));
+        REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
+
+        // put things back
+        fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO);
+        REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
+        REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO));
+        fs.remove(temp_dir_b, VCPKG_LINE_INFO);
+    }
+
+    // try rename_cas_like file, target exists
+    {
+        fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
+        fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO);
+        // Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared
+        // Also note that POSIX rename() will just delete the target like we want by itself so
+        // this returns true.
+        REQUIRE(fs.rename_cas_like(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO));
+        REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO));
+        REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
+
+        // put things back
+        fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO);
+        REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
+        REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO));
+        fs.remove(temp_dir_b, VCPKG_LINE_INFO);
+    }
+}
+
 TEST_CASE ("copy_symlink", "[files]")
 {
     urbg_t urbg;

src/vcpkg/archives.cpp

@@ -310,18 +310,6 @@ namespace vcpkg
                                msg::path = archive);
     }
 
-    void set_directory_to_archive_contents(const Filesystem& fs,
-                                           const ToolCache& tools,
-                                           MessageSink& status_sink,
-                                           const Path& archive,
-                                           const Path& to_path)
-
-    {
-        fs.remove_all(to_path, VCPKG_LINE_INFO);
-        Path to_path_partial = extract_archive_to_temp_subdirectory(fs, tools, status_sink, archive, to_path);
-        fs.rename_with_retry(to_path_partial, to_path, VCPKG_LINE_INFO);
-    }
-
     bool ZipTool::compress_directory_to_zip(DiagnosticContext& context,
                                             const Filesystem& fs,
                                             const Path& source,

src/vcpkg/base/files.cpp

@@ -2008,6 +2008,44 @@ namespace vcpkg
         }
     }
 
+    bool Filesystem::rename_cas_like(const Path& old_path, const Path& new_path, LineInfo li) const
+    {
+        std::error_code ec;
+        bool result = this->rename_cas_like(old_path, new_path, ec);
+        if (ec)
+        {
+            exit_filesystem_call_error(li, ec, __func__, {old_path, new_path});
+        }
+
+        return result;
+    }
+
+    bool Filesystem::rename_cas_like(const Path& old_path, const Path& new_path, std::error_code& ec) const
+    {
+        this->rename(old_path, new_path, ec);
+        using namespace std::chrono_literals;
+        for (const auto& delay : {10ms, 100ms, 1000ms, 10000ms})
+        {
+            if (!ec)
+            {
+                return true;
+            }
+            else if (ec == std::make_error_condition(std::errc::directory_not_empty) ||
+                     ec == std::make_error_condition(std::errc::file_exists) || this->exists(new_path, ec))
+            {
+                // either the rename failed with a target already exists error, or the target explicitly exists,
+                // assume another process 'won' the 'CAS'.
+                this->remove_all(old_path, ec);
+                return false;
+            }
+
+            std::this_thread::sleep_for(delay);
+            this->rename(old_path, new_path, ec);
+        }
+
+        return false;
+    }
+
     bool Filesystem::remove(const Path& target, LineInfo li) const
     {
         std::error_code ec;

src/vcpkg/configure-environment.cpp

@@ -155,7 +155,8 @@ namespace vcpkg
         auto& fs = paths.get_filesystem();
 
         // if artifacts is deployed in development, with Visual Studio, or with the One Liner, it will be deployed here
-        Path vcpkg_artifacts_path = get_exe_path_of_current_process();
+        const Path exe_path = get_exe_path_of_current_process();
+        Path vcpkg_artifacts_path = exe_path;
         vcpkg_artifacts_path.replace_filename("vcpkg-artifacts");
         vcpkg_artifacts_path.make_preferred();
         Path vcpkg_artifacts_main_path = vcpkg_artifacts_path / "main.js";
@@ -177,9 +178,9 @@ namespace vcpkg
 #endif // ^^^ !VCPKG_STANDALONE_BUNDLE_SHA
             if (out_of_date)
             {
-                fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO);
-                auto temp = get_exe_path_of_current_process();
-                temp.replace_filename("vcpkg-artifacts-temp");
+                // This is racy; the reason for the temp-path-then-rename dance is to get only the
+                // 'vcpkg-artifacts' directory out of the standalone bundle, while extracting
+                // the standalone bundle over VCPKG_ROOT would overwrite scripts/triplets/etc.
                 auto maybe_tarball = download_vcpkg_standalone_bundle(
                     console_diagnostic_context, paths.get_asset_cache_settings(), fs, paths.downloads);
                 auto tarball = maybe_tarball.get();
@@ -188,10 +189,12 @@ namespace vcpkg
                     Checks::exit_fail(VCPKG_LINE_INFO);
                 }
 
-                set_directory_to_archive_contents(fs, paths.get_tool_cache(), null_sink, *tarball, temp);
+                Path temp = extract_archive_to_temp_subdirectory(
+                    fs, paths.get_tool_cache(), null_sink, *tarball, vcpkg_artifacts_path);
+                fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO);
                 fs.rename_with_retry(temp / "vcpkg-artifacts", vcpkg_artifacts_path, VCPKG_LINE_INFO);
-                fs.remove(*tarball, VCPKG_LINE_INFO);
                 fs.remove_all(temp, VCPKG_LINE_INFO);
+                fs.remove(*tarball, VCPKG_LINE_INFO);
 #if defined(VCPKG_STANDALONE_BUNDLE_SHA)
                 fs.write_contents(vcpkg_artifacts_version_path, VCPKG_BASE_VERSION_AS_STRING, VCPKG_LINE_INFO);
 #endif // ^^^ VCPKG_STANDALONE_BUNDLE_SHA
@@ -233,7 +236,7 @@ namespace vcpkg
         }
 
         cmd.string_arg("--vcpkg-root").string_arg(paths.root);
-        cmd.string_arg("--z-vcpkg-command").string_arg(get_exe_path_of_current_process());
+        cmd.string_arg("--z-vcpkg-command").string_arg(exe_path);
 
         cmd.string_arg("--z-vcpkg-artifacts-root").string_arg(paths.artifacts());
         cmd.string_arg("--z-vcpkg-downloads").string_arg(paths.downloads);

src/vcpkg/tools.cpp

@@ -871,12 +871,14 @@ namespace vcpkg
             if (tool_data.is_archive)
             {
                 status_sink.println(Color::none, msgExtractingTool, msg::tool_name = tool_data.name);
-                set_directory_to_archive_contents(fs, *this, status_sink, download_path, tool_dir_path);
+                Path to_path_partial =
+                    extract_archive_to_temp_subdirectory(fs, *this, status_sink, download_path, tool_dir_path);
+                fs.rename_cas_like(to_path_partial, tool_dir_path, IgnoreErrors{});
             }
             else
             {
                 fs.create_directories(exe_path.parent_path(), IgnoreErrors{});
-                fs.rename(download_path, exe_path, IgnoreErrors{});
+                fs.rename_cas_like(download_path, exe_path, IgnoreErrors{});
             }
 
             if (!fs.exists(exe_path, IgnoreErrors{}))

src/vcpkg/vcpkgpaths.cpp

@@ -1199,12 +1199,6 @@ namespace vcpkg
             return format_filesystem_call_error(ec, "create_directory", {git_tree_temp});
         }
 
-        fs.remove_all(destination, ec);
-        if (ec)
-        {
-            return format_filesystem_call_error(ec, "remove_all", {destination});
-        }
-
         auto git_archive = git_cmd_builder(dot_git_dir, git_tree_temp)
                                .string_arg("read-tree")
                                .string_arg("-m")
@@ -1232,11 +1226,11 @@ namespace vcpkg
             return error;
         }
 
-        fs.rename_with_retry(git_tree_temp, destination, ec);
+        fs.rename_cas_like(git_tree_temp, destination, ec);
         if (ec)
         {
             return error_prefix().append(
-                format_filesystem_call_error(ec, "rename_with_retry", {git_tree_temp, destination}));
+                format_filesystem_call_error(ec, "rename_cas_like", {git_tree_temp, destination}));
         }
 
         fs.remove(git_tree_index, IgnoreErrors{});