Usage of hard or symbolic linking leads to NuGet cache corruption

[marcin-krystianc]

Issue Description

MSBuild can use hard or symbolic links to avoid excessive file copies on disk. It is a good method to speed-up builds but at the same time, it is very easy to silently corrupt the NuGet cache.
I have seen that this issue has been mentioned several times already, but it was never explained how and when exactly the NuGet cache gets corrupted:

Related issues:

• Hard linking and NuGet cache #1407
• MSBuild disables hard linking when building in Visual Studio #3788
• Trying to reduce build size without breaking ResolveAssemblyReferences #7014
• [Bug]: Restore corrupts NuGet cache NuGet/Home#12047

Steps to Reproduce

1. Build an executable project with hard or symbolic links enabled
2. Update the version of referenced NuGet package
3. Build the application again, but this time, without using hard/symbolic links.
   MSBuild will try to update dependencies in the application's output folder. Sadly, instead of replacing existing links, it replaces actual files in NuGet cache (thus corrupting it).

• hard-links

dotnet nuget locals --clear all
dotnet new console
dotnet add package newtonsoft.json -v 13.0.1
dotnet build /p:CreateHardLinksForCopyLocalIfPossible=true
dotnet add package newtonsoft.json -v 13.0.2
dotnet build

• symbolic-links

dotnet nuget locals --clear all
dotnet new console
dotnet add package newtonsoft.json -v 13.0.1
dotnet build /p:CreateSymbolicLinksForCopyLocalIfPossible=true
dotnet add package newtonsoft.json -v 13.0.2
dotnet build

In both cases file newtonsoft.json\13.0.1\lib\netstandard2.0\Newtonsoft.Json.dll is silently replaced with newtonsoft.json\13.0.2\lib\net6.0\Newtonsoft.Json.dll:

Expected Behavior

Files in the NuGet cache remain untouched.

Actual Behavior

Files in a NuGet package are silently replaced with files from another version.

Analysis

Both Windows and Linux systems are affected, also it doesn't matter whether hard or symbolic links are used. The problem is that File.Copy operation, instead of replacing the link, replaces the file that the link is linking to. To safely replace a link with a different file or link, File.Delete needs to be called first. Unfortunately, MSBuild calls File.Delete only when the usage of hard or symbolic links is requested. When the build doesn't use hard or symbolic links, then the File.Delete is not called.

Versions & Configurations

[rainersigwald]

Reopening since the change was backed out due to causing #8684.

[marcin-krystianc]

Hi @manfred-brands,

we are very interested in fixing the problem with NuGet cache corruption as soon as possible. Since #8275 had to be reverted, I wanted to ask whether you plan to work on a fix? If not, then I'm more than happy to do it.

[manfred-brands]

Hi @marcin-krystianc,

This is the first time I heard it was reverted. I didn't get any GitHub notifications. I now see the item.mentioned above.

[manfred-brands]

I could add an 'is same file' check in the copy code, but shouldn't msbuild not detect that itself before even calling Copy? The destination file exists and has the same timestamp.

[marcin-krystianc]

I didn't look into details yet, but the #8684 @Forgind explains that msbuild checks whether paths are equal before running the Copy operation, but the checks are on relative paths not on the full paths.

[manfred-brands]

I made a small test program, that emulates a copy to oneself to see how File.Copy behaves when this is done.

I expected one of three outcomes:

1. An exception
2. An empty destination file (as it is truncated before copying)
3. It works as the System recognizes this.

File.Copy("Program.cs", "Program.cs", true);

It throws:

Unhandled exception. System.IO.IOException: The process cannot access the file 'C:\Users\m.brands\source\repos\CopyToSelf\Program.cs' because it is being used by another process.
   at System.IO.FileSystem.CopyFile(String sourceFullPath, String destFullPath, Boolean overwrite)
   at Program.<Main>$(String[] args) in C:\Users\m.brands\source\repos\CopyToSelf\Program.cs:line 4

This would mean that in the current msbuild, this situation already throws an exception.
In the current build this exception is handled in DoCopyWithRetries:

                            // if this was just because the source and destination files are the
                            // same file, that's not a failure.
                            // Note -- we check this exceptional case here, not before the copy, for perf.
                            if (PathsAreIdentical(sourceFileState.Name, destinationFileState.Name))
                            {
                                return true;
                            }

This states that Path.GetFullPath is too expensive to call and we wait for an exception in those few cases where it matters.
With the need to File.Delete first, do you see any other option than having to do this check before trying to delete?