Use rename_or_delete when finalizing a downloaded .part file

[Bugale Bugalit (bugale)]

Fixes #2096.

Concurrent vcpkg processes download an asset to <dest>.<pid>.part and then rename it onto the shared <dest>. The part files are distinct, but the rename is unserialized, and on Windows it fails with ERROR_ACCESS_DENIED while another process holds the freshly-renamed destination open — which is exactly what that process does next, hashing it or extracting it as a tool.

rename_or_delete is the compare-and-swap that already handles this pattern for the extracted tool directory, a few frames up in download_tool:

auto maybe_partial_path = extract_archive_to_temp_subdirectory(context, fs, *this, download_path, tool_dir_path);
if (auto partial_path = maybe_partial_path.get())
{
    if (!fs.rename_or_delete(context, *partial_path, tool_dir_path))

It is the right primitive for the download too, and in fact more obviously so: the destination is content-addressed and check_downloaded_file_hash runs on the line immediately before, so a destination that already exists is bit-identical and discarding the loser's copy is a no-op. It also retries transient failures with backoff, which the bare rename did not.

The DiagnosticContext overload returns Optional<bool>, whose operator bool is has_value, so !result distinguishes a genuine failure from "lost the CAS, which is fine" — the same discrimination download_tool relies on.

Three call sites finalize a part file and all three are changed: the direct download, and the two asset-cache-script paths (with and without an expected hash). The third has no hash to verify, so which copy survives is arbitrary either way — it was MOVEFILE_REPLACE_EXISTING (last writer wins) and is now first writer wins. Happy to drop that one if you would rather keep the change to the hash-verified paths.

Verified against the reproducer in the issue: six concurrent vcpkg install invocations for different triplets against a cold VCPKG_DOWNLOADS go from 15/18 failures to passing, once the destination is no longer contended.

[Copilot]

Pull request overview

This PR addresses Windows concurrency failures when multiple vcpkg processes finalize downloads from per-process *.part files into a shared destination, by switching finalization to the existing Filesystem::rename_or_delete() compare-and-swap helper.

Changes:

• Replace fs.rename(...) with fs.rename_or_delete(context, ...) when promoting *.part downloads to their final destination.
• Apply the same finalization change to both direct download and asset-cache-script download paths (with and without expected hash).

Suppressed comments (1)

src/vcpkg/base/downloads.cpp:881

• In the no-maybe_sha512 branch, finalizing the script-produced .part file now uses rename_or_delete(), which can treat an existing download_path as an acceptable outcome and return Success even if we couldn’t replace the destination (for example, if it’s open in another process). Since there is no expected hash here, we can’t validate that the pre-existing destination is equivalent to what the script produced.

If this path is intended to behave as “write the script’s output to download_path”, consider using fs.rename(context, ...) here so we still fail when we can’t actually publish the newly-produced file.

        if (fs.exists(download_path_part_path, VCPKG_LINE_INFO))
        {
            if (!fs.rename_or_delete(context, download_path_part_path, download_path))
            {
                return DownloadPrognosis::OtherError;
            }

            return DownloadPrognosis::Success;

[Billy O'Neal (BillyONeal)]

I think this is in fact the intended outcome. If two processes race when there is no known SHA we are already doomed.

[Bugale Bugalit (bugale)]

That was my thinking too. On the two hashed paths the destination is content-addressed and we check the hash on the line right above, so losing the race is harmless. With no SHA there's nothing to compare anyway, and the old code just killed the process, so this felt like the better choice.

[Billy O'Neal (BillyONeal)]

This exists check is a bit suspicious given what you're already fixing here.

[Bugale Bugalit (bugale)]

I looked into it and I think it's fine, though not for an obvious reason.
It's not a cross-process race. The part path here is fmt::format("{}.{}.part", ..., get_process_id()) a few lines up, so only this process can create or delete it.
It's also doing real work. rename_or_delete doesn't fail fast when the source is missing, it walks the retry loop sleeping 10ms, 100ms, 1s and 10s before giving up. So dropping the check gets you one of two bad outcomes: an ~11 second stall ending in a generic filesystem error instead of msgAssetCacheScriptFailedToWriteFile, or, if download_path happens to already exist, rename_or_delete reports a lost CAS and we return Success even though the script produced nothing.

The part I do think is off is the overload. exists(..., VCPKG_LINE_INFO) exits the process on a filesystem error, inside a function that reports everything else through context. There's no DiagnosticContext overload of exists though, only ec and LineInfo, so converting it means either swallowing the error or writing the report by hand.
Let me know if you want me to do it as part of this PR

[Billy O'Neal (BillyONeal)]

rename_or_delete "Returns whether the rename actually happened." so I'm not sure the target already existing should be an error as this does?

[Bugale Bugalit (bugale)]

It isn't treated as an error, but I can see why it might be confusing, so I've changed it.
The DiagnosticContext overload returns Optional<bool>, not bool. Optional's operator bool is has_value(), so the ! there means "the call failed", not "the rename didn't happen". Losing the CAS gives you a filled Optional holding false, which passes the check and falls through to Success.
That's way too subtle when the inner type is also a bool, so all three places are now .has_value().

BTW about the doc comment you quoted, "if old_path and new_path are files, this function always returns true" isn't true once processes race, which is what #2096 is. The rename fails with ERROR_ACCESS_DENIED, not file_exists, so it doesn't match the error code checks. It gets caught by the this->exists(new_path, local_ec) at the end of that same condition, which deletes our .part and returns false.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/vcpkg/base/downloads.cpp:838

• Same as above: prefer relying on Optional<bool>’s operator bool instead of explicitly calling .has_value(), to match existing usage (src/vcpkg/tools.cpp:1023).

                        if (!fs.rename_or_delete(context, download_path_part_path, download_path).has_value())

src/vcpkg/base/downloads.cpp:876

• Same as above: use if (!fs.rename_or_delete(...)) rather than .has_value() for consistency with other call sites and to emphasize the intended semantics (nullopt == error; value == ok, regardless of CAS outcome).

            if (!fs.rename_or_delete(context, download_path_part_path, download_path).has_value())

src/vcpkg/base/downloads.cpp:647

• Filesystem::rename_or_delete(DiagnosticContext&, ...) returns Optional<bool> and is already used elsewhere via its operator bool (i.e., if (!fs.rename_or_delete(...)) checks has_value()). Using .has_value() here is redundant and diverges from the established call pattern (e.g. src/vcpkg/tools.cpp:1023), which makes the intent (“error vs. CAS-lost”) a bit harder to read.

This issue also appears in the following locations of the same file:

• line 838
• line 876

        if (!fs.rename_or_delete(context, download_path_part_path, download_path).has_value())

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/vcpkg/base/downloads.cpp:650

• rename_or_delete() returning false (but with a value) means we treated the operation as “CAS lost” and deleted our .part file. When an expected SHA is provided, this path can still return Success even if the existing destination file does not match the expected hash (e.g. the destination existed from some other source and was held open, causing rename() to fail). To preserve the hash contract, consider verifying download_path against maybe_sha512 when rename_or_delete() reports CAS-loss.

        if (!fs.rename_or_delete(context, download_path_part_path, download_path).has_value())
        {
            return DownloadPrognosis::OtherError;
        }

src/vcpkg/base/downloads.cpp:841

• Here rename_or_delete() may return false (with a value) if the destination exists and the rename failed (e.g. destination held open on Windows). When maybe_sha512 is set, returning Success on that path without re-checking the final download_path can violate the expected-hash contract if the pre-existing destination bytes differ. Consider verifying the destination’s SHA512 when rename_or_delete() reports CAS-loss.

                        if (!fs.rename_or_delete(context, download_path_part_path, download_path).has_value())
                        {
                            return DownloadPrognosis::OtherError;
                        }