Skip to content

Optimize rename_or_delete to stop retrying after EXDEV #1659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 29, 2025

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Apr 20, 2025

Fixes microsoft/vcpkg#45130

Please note that there is already code in place to handle std::errc::cross_device_link for binary caching:

if (can_attempt_rename)
{
// if we are already on the same filesystem we can just rename into place
m_fs.rename_or_delete(zip_path, archive_path, ec);
}
if (!can_attempt_rename || (ec && ec == std::make_error_condition(std::errc::cross_device_link)))
{
// either we need to make a copy or the rename failed because buildtrees and the binary
// cache write target are on different filesystems, copy to a sibling in that directory and rename
// into place
m_fs.copy_file(zip_path, archive_temp_path, CopyOptions::overwrite_existing, ec);
if (!ec)
{
m_fs.rename_or_delete(archive_temp_path, archive_path, ec);
}
}

However, copy-and-delete was only tried after calling fs.rename_or_delete() which tries to move the file after increasing timeouts. I decided to not move the copy-and-delete behavior to fs.rename_or_delete() because the file is first copied to a temproary location in binarycaching.cpp.

Files on different filesystems can't be renamed:

Linux: https://man7.org/linux/man-pages/man2/rename.2.html

Windows seems to support moving files but not folders across devices: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefile

std::errc::cross_device_link: https://en.cppreference.com/w/cpp/error/errno_macros

Stack Overflow: https://stackoverflow.com/questions/43206198/what-does-the-exdev-cross-device-link-not-permitted-error-mean

@Thomas1664 Thomas1664 changed the title Optimize rename_or_delete to stop retrying after std::errc::cross_device_link Optimize rename_or_delete to stop retrying after s EXDEV Apr 20, 2025
@Thomas1664 Thomas1664 changed the title Optimize rename_or_delete to stop retrying after s EXDEV Optimize rename_or_delete to stop retrying after EXDEV Apr 20, 2025
@@ -231,15 +233,16 @@ namespace
std::error_code ec;
if (can_attempt_rename)
{
// if we are already on the same filesystem we can just rename into place
m_fs.rename_or_delete(zip_path, archive_path, ec);
Copy link
Contributor Author

@Thomas1664 Thomas1664 Apr 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before #802, files were copied to the binary cache, meaning the original file in zip_path did still exist after this function ended. However, this file is gone with rename_or_delete()

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement!

@BillyONeal BillyONeal merged commit d8ed13a into microsoft:main Apr 29, 2025
7 checks passed
@Thomas1664 Thomas1664 deleted the rename-or-delete branch April 29, 2025 21:02
@@ -2045,6 +2045,11 @@ namespace vcpkg
this->remove_all(old_path, ec);
return false;
}
else if (ec == std::make_error_condition(std::errc::cross_device_link))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only that ec was already lost at this point, due to L2041.

#1669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vcpkg-tool] Slow binary cache submission due to rename() EXDEV retries
3 participants