Skip to content
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

[Bug]: in RC1 copy is failing to overwrite on Linux in some cases that previously succeeded #9250

Closed
danmoseley opened this issue Sep 22, 2023 · 7 comments
Assignees
Labels
bug Iteration:2023December Iteration:2024February Priority:1 Work that is critical for the release, but we could probably ship without triaged

Comments

@danmoseley
Copy link
Member

danmoseley commented Sep 22, 2023

Issue Description

This was on an internal Mariner deployment. The message is access denied eg

...../dotnet/7.0.401/sdk/7.0.401/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Publish.targets(267,5): error MSB3027: Could not copy "obj/Release/net7.0/DOTNET_7_APP.dll" to "/home/site/wwwroot/DOTNET_7_APP.dll". Exceeded retry count of 10. Failed.  [/tmp/8dbb9829ae9e326/DOTNET_7_APP.csproj]

began in 7.0.400 and MSBUILDDISABLEFEATURESFROMVERSION=17.8 is confirmed to make it go away.

Below is @rainersigwald theory

I don't mean to keep the theory a secret. In 17.7, we
changed the MSBuild Copy task
to delete the destination file before overwriting it, because (when it was a link) we would sometimes write
through
the link, corrupting a file in (for example) the NuGet cache. I'm wondering if it's the
delete
that's failing here, when overwriting would be fine. Setting that env var will disable that change (and others, so it's not a go-to-prod solution)

We don't know why the overwrite was working but the delete isn't, and can get traces.

We need to get some fix into RC2. Perhaps reverting the copy task at least for Linux?

Steps to Reproduce

.

Expected Behavior

.

Actual Behavior

.

Analysis

No response

Versions & Configurations

No response

@danmoseley danmoseley added bug needs-triage Have yet to determine what bucket this goes in. labels Sep 22, 2023
@danmoseley danmoseley added this to the 8.0RC2 milestone Sep 22, 2023
rainersigwald added a commit to rainersigwald/msbuild that referenced this issue Sep 22, 2023
This allows an opt-in workaround for dotnet#9250 that affected deployment
processes can use, mitigating the risk of entirely reverting dotnet#8685.
rainersigwald added a commit that referenced this issue Sep 22, 2023
This allows an opt-in workaround for #9250 that affected deployment
processes can use, mitigating the risk of entirely reverting #8685.
@rainersigwald
Copy link
Member

For 8.0.100-RC2, we added an explicit control to opt back into the old behavior MSBUILDCOPYWITHOUTDELETE=1.

Long term I like this proposal from @danmoseley

What about falling back to regular copy if access denied on the delete?

@rainersigwald
Copy link
Member

rainersigwald commented Sep 22, 2023

Long term I like this proposal from @danmoseley

What about falling back to regular copy if access denied on the delete?

Well, I liked it. But in fact that's what we're already doing

msbuild/src/Tasks/Copy.cs

Lines 288 to 291 in 3847162

if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) && destinationFileState.FileExists && !destinationFileState.IsReadOnly)
{
FileUtilities.DeleteNoThrow(destinationFileState.Name);
}

Where

internal static void DeleteNoThrow(string path)
{
try
{
File.Delete(FixFilePath(path));
}
catch (Exception ex) when (ExceptionHandling.IsIoRelatedException(ex))
{
}
}

and

internal static bool IsIoRelatedException(Exception e)
{
// These all derive from IOException
// DirectoryNotFoundException
// DriveNotFoundException
// EndOfStreamException
// FileLoadException
// FileNotFoundException
// PathTooLongException
// PipeException
return e is UnauthorizedAccessException
|| e is NotSupportedException
|| (e is ArgumentException && !(e is ArgumentNullException))
|| e is SecurityException
|| e is IOException;
}

So unless there's some other exception being fired here (when I try to overwrite a running app on Windows I get UnauthorizedAccessException) something else is going on.

@AR-May AR-May added Priority:1 Work that is critical for the release, but we could probably ship without Iteration:2023October and removed needs-triage Have yet to determine what bucket this goes in. labels Sep 26, 2023
@YuliiaKovalova YuliiaKovalova self-assigned this Oct 5, 2023
@YuliiaKovalova
Copy link
Member

the ticket is blocked by the undergoing release process in the App Services team.
Postponed by December.

@YuliiaKovalova
Copy link
Member

The package with patched msbuild was given to app services. We are waiting for extended logs results.

@rokonec
Copy link
Contributor

rokonec commented Feb 1, 2024

@YuliiaKovalova Please update status of this issue.

@YuliiaKovalova
Copy link
Member

App Service team hasn't gathered logs yet.
Without them we can't proceed.

@YuliiaKovalova
Copy link
Member

We have agreed to close it since the requested info wasn't provided and there is a doable way to workaround the issue.
It can be reconsidered after receiving the missed parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Iteration:2023December Iteration:2024February Priority:1 Work that is critical for the release, but we could probably ship without triaged
Projects
None yet
Development

No branches or pull requests

6 participants