Specifying a TRX report filename that already exists causes the Microsoft Testing Platform to hang and crash

[AdamMaras]

Describe the bug

When running a unit test suite using the Microsoft Testing Platform and the TRX report extension, if one uses the --report-trx-filename command-line option to specify a filename that already exists, the test suite hangs for 30 seconds (doing a tight loop of I/O retries that will not succeed) at the end, and exits unsuccessfully. When specifying a TRX report filename explicitly, the report extension should overwrite the existing file with the contents of the new test report. This occurs with the latest release of the Microsoft.Testing.Extensions.TrxReport package, version 1.5.0.

Steps To Reproduce

First, start by creating a new unit testing project (this example uses xUnit 3) and adding the TRX report extension to the project:

dotnet new install xunit.v3.templates
dotnet new xunit3 -n TrxHangCrash
dotnet add ./TrxHangCrash package Microsoft.Testing.Extensions.TrxReport

Then, modify ./TrxHangCrash/TrxHangCrash.csproj to use the Microsoft Testing Platform:

  <PropertyGroup>
    <TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport>
  </PropertyGroup>

Finally, run the test suite twice, specifying the same report filename twice:

dotnet test ./TrxHangCrash -- --report-trx --report-trx-filename report.trx
dotnet test ./TrxHangCrash -- --report-trx --report-trx-filename report.trx

Expected behavior

The test suite completes, and the report.trx file is overwritten with the contents of the new test report.

Actual behavior

The test suite hangs for 30 seconds at the end of the tests, and then fails with the following stack trace in the test log:

Unhandled exception. System.IO.IOException: The file '/redacted/TrxHangCrash/bin/Debug/net8.0/TestResults/report.trx' already exists.
   at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirError)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String path, OpenFlags flags, Int32 mode, Boolean failForSymlink, Boolean& wasSymlink, Func`4 createOpenException)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, UnixFileMode openPermissions, Int64& fileLength, UnixFileMode& filePermissions, Boolean failForSymlink, Boolean& wasSymlink, Func`4 createOpenException)
   at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at Microsoft.Testing.Platform.Helpers.SystemFileSystem.NewFileStream(String path, FileMode mode) in /_/src/Platform/Microsoft.Testing.Platform/Helpers/System/SystemFileSystem.cs:line 14
   at Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxReportEngine.<>c__DisplayClass27_0.<<GenerateReportAsync>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxReportEngine.RetryWhenIOExceptionAsync(Func`1 func) in /_/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs:line 223
   at Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxReportEngine.GenerateReportAsync(String testHostCrashInfo, Boolean isTestHostCrashed, Boolean keepReportFileStreamOpen) in /_/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs:line 150
   at Microsoft.Testing.Extensions.TrxReport.Abstractions.TrxReportGenerator.OnTestSessionFinishingAsync(SessionUid sessionUid, CancellationToken cancellationToken) in /_/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxDataConsumer.cs:line 242
   at Microsoft.Testing.Platform.Hosts.CommonTestHost.NotifyTestSessionEndAsync(SessionUid sessionUid, BaseMessageBus baseMessageBus, ServiceProvider serviceProvider, CancellationToken cancellationToken) in /_/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:line 206
   at Microsoft.Testing.Platform.Hosts.CommonTestHost.ExecuteRequestAsync(ProxyOutputDevice outputDevice, ITestSessionContext testSessionInfo, ServiceProvider serviceProvider, BaseMessageBus baseMessageBus, ITestFramework testFramework, ClientInfo client) in /_/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:line 137
   at Microsoft.Testing.Platform.Hosts.ConsoleTestHost.InternalRunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Hosts/ConsoleTestHost.cs:line 86
   at Microsoft.Testing.Platform.Hosts.ConsoleTestHost.InternalRunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Hosts/ConsoleTestHost.cs:line 118
   at Microsoft.Testing.Platform.Hosts.CommonTestHost.RunTestAppAsync(CancellationToken testApplicationCancellationToken) in /_/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:line 110
   at Microsoft.Testing.Platform.Hosts.CommonTestHost.RunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:line 34
   at Microsoft.Testing.Platform.Hosts.CommonTestHost.RunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:line 71
   at Microsoft.Testing.Platform.Builder.TestApplication.RunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Builder/TestApplication.cs:line 244
   at Xunit.Runner.InProc.SystemConsole.TestingPlatform.TestPlatformTestFramework.RunAsync(String[] args, Action`2 extensionRegistration) in D:\a\xunit\xunit\src\xunit.v3.runner.inproc.console\TestingPlatform\TestPlatformTestFramework.cs:line 270
   at XunitAutoGeneratedEntryPoint.Main(String[] args) in /redacted/.nuget/packages/xunit.v3.core/1.0.0/_content/EntryPoint-xunit.cs:line 9

Additional context

There's some IO retry logic during TRX report generation that seems to account for filename collisions when the time is part of the TRX report filename, but it seems as though the new file stream being opened in TrxReportEngine.GenerateReportAsync could use FileMode.Create instead of FileMode.CreateNew when the report filename is explicitly provided instead of being generated.

[Youssef1313]

@AdamMaras Thanks for the report. These are very good points, and I think the behavior can be improved here.

@Evangelink To summarize:

1. The retry logic seems pointless if we will try with the same file name again.
2. The retry logic keeps going on repeatedly for 30 seconds. It looks like 30 seconds is too much
3. We are not disposing the file stream

An open question for me is whether we should overwrite or fail when it already exists. I'm afraid it could happen that user is unintentionally writing to an existing trx file and end up losing that file. Maybe we can introduce a flag --force-write-if-exists?