diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 27854c3cc9a..489e6c519df 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -29,6 +29,9 @@ Change wave checks around features will be removed in the release that accompani ## Current Rotation of Change Waves +### 18.7 +- [Copy task retries on ERROR_ACCESS_DENIED on non-Windows platforms to handle transient lock conflicts (e.g. macOS CoW filesystems)](https://github.com/dotnet/msbuild/issues/13463) + ### 18.6 - [AbsolutePath.GetCanonicalForm optimization - avoid expensive Path.GetFullPath calls when paths don't need canonicalization](https://github.com/dotnet/msbuild/pull/13369) - [TaskHostTask forwards request-level global properties (e.g. MSBuildRestoreSessionId) to out-of-proc TaskHost in -mt mode](https://github.com/dotnet/msbuild/pull/13443) diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index e759dcafbcd..52ab8929eeb 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -34,7 +34,8 @@ internal static class ChangeWaves internal static readonly Version Wave18_4 = new Version(18, 4); internal static readonly Version Wave18_5 = new Version(18, 5); internal static readonly Version Wave18_6 = new Version(18, 6); - internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4, Wave18_5, Wave18_6]; + internal static readonly Version Wave18_7 = new Version(18, 7); + internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4, Wave18_5, Wave18_6, Wave18_7]; /// /// Special value indicating that all features behind all Change Waves should be enabled. diff --git a/src/Tasks.UnitTests/Copy_Tests.cs b/src/Tasks.UnitTests/Copy_Tests.cs index ab10d6d2f95..80456a77bb3 100644 --- a/src/Tasks.UnitTests/Copy_Tests.cs +++ b/src/Tasks.UnitTests/Copy_Tests.cs @@ -749,7 +749,18 @@ public void DoNotNormallyCopyOverReadOnlyFile(bool isUseHardLinks, bool isUseSym string destinationContent = File.ReadAllText(destination); Assert.Equal("This is a destination file.", destinationContent); - ((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // did not do retries as it was r/o + // On Windows, ERROR_ACCESS_DENIED is not retried (it's a real ACL or r/o bit issue). + // On non-Windows with Wave18_7 enabled (default), access denied can be a transient lock + // (e.g. macOS CoW filesystem), so we retry; retries will ultimately fail for a genuinely read-only file. + if (NativeMethodsShared.IsWindows) + { + ((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); + } + else + { + // non-Windows: retries are expected due to possible transient EACCES + ((MockEngine)t.BuildEngine).AssertLogContains("MSB3026"); + } } finally { @@ -760,6 +771,61 @@ public void DoNotNormallyCopyOverReadOnlyFile(bool isUseHardLinks, bool isUseSym } } + /// + /// When Wave18_7 is disabled, non-Windows should NOT retry on ERROR_ACCESS_DENIED (old behavior preserved via opt-out). + /// + [Theory] + [MemberData(nameof(GetHardLinksSymLinks))] + public void DoNotRetryCopyOverReadOnlyFileWhenWave18_7Disabled(bool isUseHardLinks, bool isUseSymbolicLinks) + { + using TestEnvironment env = TestEnvironment.Create(_testOutputHelper); + + // TODO: Remove test when Wave18_7 rotates out + ChangeWaves.ResetStateForTests(); + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave18_7.ToString()); + + TransientTestFile source = env.CreateFile("source.tmp", "This is a source file."); + TransientTestFile destination = env.CreateFile("destination.tmp", "This is a destination file."); + + File.SetAttributes(destination.Path, FileAttributes.ReadOnly); + try + { + ITaskItem sourceItem = new TaskItem(source.Path); + ITaskItem destinationItem = new TaskItem(destination.Path); + ITaskItem[] sourceFiles = { sourceItem }; + ITaskItem[] destinationFiles = { destinationItem }; + + var engine = new MockEngine(_testOutputHelper); + var t = new Copy + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + RetryDelayMilliseconds = 1, // speed up tests! + BuildEngine = engine, + SourceFiles = sourceFiles, + DestinationFiles = destinationFiles, + SkipUnchangedFiles = true, + // OverwriteReadOnlyFiles defaults to false + UseHardlinksIfPossible = isUseHardLinks, + UseSymboliclinksIfPossible = isUseSymbolicLinks, + }; + + // Should fail: target is readonly + t.Execute().ShouldBeFalse(); + + // Expect for there to have been no copies. + t.CopiedFiles.ShouldBeEmpty(); + + File.ReadAllText(destination.Path).ShouldBe("This is a destination file."); + + // With Wave18_7 disabled, ERROR_ACCESS_DENIED should not be retried on any platform (old behavior). + engine.AssertLogDoesntContain("MSB3026"); + } + finally + { + File.SetAttributes(destination.Path, FileAttributes.Normal); + } + } + /// /// If MSBUILDALWAYSOVERWRITEREADONLYFILES is set, then overwrite read-only even when /// OverwriteReadOnlyFiles is false diff --git a/src/Tasks/Copy.cs b/src/Tasks/Copy.cs index bbe9f834c4c..ccc9e57dbe7 100644 --- a/src/Tasks/Copy.cs +++ b/src/Tasks/Copy.cs @@ -1039,7 +1039,10 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF // to a failure to reset the readonly bit properly, in which case retrying will succeed. This seems to be // a pretty edge scenario, but since some of our internal builds appear to be hitting it, provide a secret // environment variable to allow overriding the default behavior and forcing retries in this circumstance as well. - if (!_alwaysRetryCopy) + // On at least some non-Windows platforms (e.g. macOS https://github.com/dotnet/msbuild/issues/13463), access denied can also indicate a + // transient lock conflict (e.g. EACCES from a concurrent clonefile/flock), so we only skip retries for + // access denied on Windows. + if (!_alwaysRetryCopy && (NativeMethodsShared.IsWindows || !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_7))) { throw; }