Merge branch 'release/10.0.1xx' into darc-release/10.0.1xx-d5e70d62-80a9-432c-8bad-50dceb349e82

Author: SimonZhao888

eng/Version.Details.props

@@ -140,8 +140,8 @@ This file should be imported by eng/Versions.props
     <!-- dotnet/core-setup dependencies -->
     <NETStandardLibraryRefPackageVersion>2.1.0</NETStandardLibraryRefPackageVersion>
     <!-- microsoft/testfx dependencies -->
-    <MicrosoftTestingPlatformPackageVersion>1.9.0-preview.25451.2</MicrosoftTestingPlatformPackageVersion>
-    <MSTestPackageVersion>3.11.0-preview.25451.2</MSTestPackageVersion>
+    <MicrosoftTestingPlatformPackageVersion>1.9.0-preview.25452.4</MicrosoftTestingPlatformPackageVersion>
+    <MSTestPackageVersion>3.11.0-preview.25452.4</MSTestPackageVersion>
   </PropertyGroup>
   <!--Property group for alternate package version names-->
   <PropertyGroup>

eng/Version.Details.xml

@@ -553,13 +553,13 @@
       <Uri>https://github.com/dotnet/dotnet</Uri>
       <Sha>7ac1ca67bb1fb8a381c1c94a9f82a97725f0ccf3</Sha>
     </Dependency>
-    <Dependency Name="Microsoft.Testing.Platform" Version="1.9.0-preview.25451.2">
+    <Dependency Name="Microsoft.Testing.Platform" Version="1.9.0-preview.25452.4">
       <Uri>https://github.com/microsoft/testfx</Uri>
-      <Sha>8d29291af97543579834e9109768c5ee89f63638</Sha>
+      <Sha>0d5aa69d6b2cbeebff30536a22d31f4bd7ecf684</Sha>
     </Dependency>
-    <Dependency Name="MSTest" Version="3.11.0-preview.25451.2">
+    <Dependency Name="MSTest" Version="3.11.0-preview.25452.4">
       <Uri>https://github.com/microsoft/testfx</Uri>
-      <Sha>8d29291af97543579834e9109768c5ee89f63638</Sha>
+      <Sha>0d5aa69d6b2cbeebff30536a22d31f4bd7ecf684</Sha>
     </Dependency>
     <Dependency Name="Microsoft.Extensions.Configuration.Ini" Version="10.0.0-rc.2.25427.104">
       <Uri>https://github.com/dotnet/dotnet</Uri>

src/Cli/dotnet/Commands/CliCommandStrings.resx

@@ -2648,4 +2648,22 @@ Proceed?</value>
   <data name="CmdMinimumExpectedTestsDescription" xml:space="preserve">
     <value>Specifies the minimum number of tests that are expected to run.</value>
   </data>
+  <data name="DotnetTestPipeOverlapping" xml:space="preserve">
+    <value>'dotnet' unexpectedly received overlapping messages from the 'dotnet test' named pipe.</value>
+  </data>
+  <data name="DotnetTestPipeIncompleteSize" xml:space="preserve">
+    <value>'dotnet' unexpectedly received less than 4 bytes from the 'dotnet test' named pipe.</value>
+  </data>
+  <data name="InternalLoopAsyncDidNotExitSuccessfullyErrorMessage" xml:space="preserve">
+    <value>Method '{0}' did not exit successfully</value>
+  </data>
+  <data name="DotnetTestPipeFailureHasHandshake" xml:space="preserve">
+    <value>Error disposing 'NamedPipeServer' corresponding to handshake:</value>
+  </data>
+  <data name="DotnetTestPipeFailureWithoutHandshake" xml:space="preserve">
+    <value>Error disposing 'NamedPipeServer', and no handshake was found.</value>
+  </data>
+  <data name="DotnetTestIncompatibleHandshakeVersion" xml:space="preserve">
+    <value>Supported protocol versions sent by Microsoft.Testing.Platform are '{0}'. The SDK supports '{1}', which is incompatible.</value>
+  </data>
 </root>

src/Cli/dotnet/Commands/Test/CliConstants.cs

@@ -65,7 +65,10 @@ internal static class HandshakeMessagePropertyNames
 
 internal static class ProtocolConstants
 {
-    internal const string Version = "1.0.0";
+    /// <summary>
+    /// The protocol versions that are supported by the current SDK. Multiple versions can be present and be semicolon separated.
+    /// </summary>
+    internal const string SupportedVersions = "1.0.0";
 }
 
 internal static class ProjectProperties

src/Cli/dotnet/Commands/Test/CustomEventArgs.cs

@@ -5,19 +5,20 @@
 
 namespace Microsoft.DotNet.Cli.Commands.Test;
 
-internal class HandshakeArgs : EventArgs
+internal sealed class HandshakeArgs : EventArgs
 {
     public Handshake Handshake { get; set; }
+    public bool GotSupportedVersion { get; set; }
 }
 
-internal class HelpEventArgs : EventArgs
+internal sealed class HelpEventArgs : EventArgs
 {
     public string ModulePath { get; set; }
 
     public CommandLineOption[] CommandLineOptions { get; set; }
 }
 
-internal class DiscoveredTestEventArgs : EventArgs
+internal sealed class DiscoveredTestEventArgs : EventArgs
 {
     public string ExecutionId { get; set; }
 
@@ -26,7 +27,7 @@ internal class DiscoveredTestEventArgs : EventArgs
     public DiscoveredTest[] DiscoveredTests { get; set; }
 }
 
-internal class TestResultEventArgs : EventArgs
+internal sealed class TestResultEventArgs : EventArgs
 {
     public string ExecutionId { get; set; }
 
@@ -37,7 +38,7 @@ internal class TestResultEventArgs : EventArgs
     public FailedTestResult[] FailedTestResults { get; set; }
 }
 
-internal class FileArtifactEventArgs : EventArgs
+internal sealed class FileArtifactEventArgs : EventArgs
 {
     public string ExecutionId { get; set; }
 
@@ -46,25 +47,19 @@ internal class FileArtifactEventArgs : EventArgs
     public FileArtifact[] FileArtifacts { get; set; }
 }
 
-internal class SessionEventArgs : EventArgs
+internal sealed class SessionEventArgs : EventArgs
 {
     public TestSession SessionEvent { get; set; }
 }
 
-internal class ErrorEventArgs : EventArgs
+internal sealed class ErrorEventArgs : EventArgs
 {
     public string ErrorMessage { get; set; }
 }
 
-internal class TestProcessExitEventArgs : EventArgs
+internal sealed class TestProcessExitEventArgs : EventArgs
 {
     public List<string> OutputData { get; set; }
     public List<string> ErrorData { get; set; }
     public int ExitCode { get; set; }
 }
-
-internal class ExecutionEventArgs : EventArgs
-{
-    public string ModulePath { get; set; }
-    public string ExecutionId { get; set; }
-}

src/Cli/dotnet/Commands/Test/IPC/NamedPipeServer.cs

@@ -4,14 +4,15 @@
 #nullable disable
 
 using System.Buffers;
+using System.Diagnostics;
 using System.Globalization;
 using System.IO.Pipes;
 
 namespace Microsoft.DotNet.Cli.Commands.Test.IPC;
 
 internal sealed class NamedPipeServer : NamedPipeBase
 {
-    private readonly Func<IRequest, Task<IResponse>> _callback;
+    private readonly Func<NamedPipeServer, IRequest, Task<IResponse>> _callback;
     private readonly NamedPipeServerStream _namedPipeServerStream;
     private readonly CancellationToken _cancellationToken;
     private readonly MemoryStream _serializationBuffer = new();
@@ -24,12 +25,12 @@ internal sealed class NamedPipeServer : NamedPipeBase
 
     public NamedPipeServer(
         PipeNameDescription pipeNameDescription,
-        Func<IRequest, Task<IResponse>> callback,
+        Func<NamedPipeServer, IRequest, Task<IResponse>> callback,
         int maxNumberOfServerInstances,
         CancellationToken cancellationToken,
         bool skipUnknownMessages)
     {
-        _namedPipeServerStream = new((PipeName = pipeNameDescription).Name, PipeDirection.InOut, maxNumberOfServerInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous);
+        _namedPipeServerStream = new((PipeName = pipeNameDescription).Name, PipeDirection.InOut, maxNumberOfServerInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly);
         _callback = callback;
         _cancellationToken = cancellationToken;
         _skipUnknownMessages = skipUnknownMessages;
@@ -55,14 +56,6 @@ public async Task WaitConnectionAsync(CancellationToken cancellationToken)
                     // We are being cancelled, so we don't need to wait anymore
                     return;
                 }
-                catch (Exception ex)
-                {
-                    // TODO: it might be better idea to let this exception bubble to
-                    // TestApplication. There, we should handle it by printing the exception without failing
-                    // the whole dotnet process, and provide more info about the specific test app that failed.
-                    // Potentially even knowing whether it's test host or test host controller that failed?
-                    Environment.FailFast($"[NamedPipeServer] Unhandled exception:{Environment.NewLine}{ex}", ex);
-                }
             }, cancellationToken);
     }
 
@@ -94,6 +87,11 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
             if (currentMessageSize == 0)
             {
                 // We need to read the message size, first 4 bytes
+                if (currentReadBytes < sizeof(int))
+                {
+                    throw new UnreachableException(CliCommandStrings.DotnetTestPipeIncompleteSize);
+                }
+
                 currentMessageSize = BitConverter.ToInt32(_readBuffer, 0);
                 missingBytesToReadOfCurrentChunk = currentReadBytes - sizeof(int);
                 missingBytesToReadOfWholeMessage = currentMessageSize;
@@ -107,6 +105,11 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
                 missingBytesToReadOfWholeMessage -= missingBytesToReadOfCurrentChunk;
             }
 
+            if (missingBytesToReadOfWholeMessage < 0)
+            {
+                throw new UnreachableException(CliCommandStrings.DotnetTestPipeOverlapping);
+            }
+
             // If we have read all the message, we can deserialize it
             if (missingBytesToReadOfWholeMessage == 0)
             {
@@ -124,7 +127,7 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
                 var deserializedObject = (IRequest)requestNamedPipeSerializer.Deserialize(_messageBuffer);
 
                 // Call the callback
-                IResponse response = await _callback(deserializedObject);
+                IResponse response = await _callback(this, deserializedObject);
 
                 // Write the message size
                 _messageBuffer.Position = 0;
@@ -205,21 +208,28 @@ public void Dispose()
             return;
         }
 
-        if (WasConnected)
+        try
         {
-            // If the loop task is null at this point we have race condition, means that the task didn't start yet and we already dispose.
-            // This is unexpected and we throw an exception.
-
-            // To close gracefully we need to ensure that the client closed the stream line 103.
-            if (!_loopTask.Wait(TimeSpan.FromSeconds(90)))
+            if (WasConnected)
             {
-                throw new InvalidOperationException("InternalLoopAsyncDidNotExitSuccessfullyErrorMessage");
+                // If the loop task is null at this point we have race condition, means that the task didn't start yet and we already dispose.
+                // This is unexpected and we throw an exception.
+
+                // To close gracefully we need to ensure that the client closed the stream line 103.
+                if (!_loopTask.Wait(TimeSpan.FromSeconds(90)))
+                {
+                    throw new InvalidOperationException(CliCommandStrings.InternalLoopAsyncDidNotExitSuccessfullyErrorMessage);
+                }
             }
         }
+        finally
+        {
+            // Ensure we are still disposing the resouces correctly, even if _loopTask completes with
+            // an exception, or if the task doesn't complete within the 90 seconds limit.
+            _namedPipeServerStream.Dispose();
+            PipeName.Dispose();
 
-        _namedPipeServerStream.Dispose();
-        PipeName.Dispose();
-
-        _disposed = true;
+            _disposed = true;
+        }
     }
 }

src/Cli/dotnet/Commands/Test/TestApplication.cs

@@ -23,6 +23,7 @@ internal sealed class TestApplication(TestModule module, BuildOptions buildOptio
 
     private Task _testAppPipeConnectionLoop;
     private readonly List<NamedPipeServer> _testAppPipeConnections = [];
+    private readonly Dictionary<NamedPipeServer, HandshakeMessage> _handshakes = new();
 
     public event EventHandler<HandshakeArgs> HandshakeReceived;
     public event EventHandler<HelpEventArgs> HelpRequested;
@@ -35,6 +36,8 @@ internal sealed class TestApplication(TestModule module, BuildOptions buildOptio
 
     public TestModule Module { get; } = module;
 
+    public bool HasFailureDuringDispose { get; private set; }
+
     public async Task<int> RunAsync(TestOptions testOptions)
     {
         if (testOptions.HasFilterMode && !ModulePathExists())
@@ -146,7 +149,7 @@ private async Task WaitConnectionAsync(CancellationToken token)
         {
             while (!token.IsCancellationRequested)
             {
-                NamedPipeServer pipeConnection = new(_pipeNameDescription, OnRequest, NamedPipeServerStream.MaxAllowedServerInstances, token, skipUnknownMessages: true);
+                var pipeConnection = new NamedPipeServer(_pipeNameDescription, OnRequest, NamedPipeServerStream.MaxAllowedServerInstances, token, skipUnknownMessages: true);
                 pipeConnection.RegisterAllSerializers();
 
                 await pipeConnection.WaitConnectionAsync(token);
@@ -173,20 +176,17 @@ private async Task WaitConnectionAsync(CancellationToken token)
         }
     }
 
-    private Task<IResponse> OnRequest(IRequest request)
+    private Task<IResponse> OnRequest(NamedPipeServer server, IRequest request)
     {
         try
         {
             switch (request)
             {
                 case HandshakeMessage handshakeMessage:
-                    if (handshakeMessage.Properties.TryGetValue(HandshakeMessagePropertyNames.ModulePath, out string value))
-                    {
-                        OnHandshakeMessage(handshakeMessage);
-
-                        return Task.FromResult((IResponse)CreateHandshakeMessage(GetSupportedProtocolVersion(handshakeMessage)));
-                    }
-                    break;
+                    _handshakes.Add(server, handshakeMessage);
+                    string negotiatedVersion = GetSupportedProtocolVersion(handshakeMessage);
+                    OnHandshakeMessage(handshakeMessage, negotiatedVersion.Length > 0);
+                    return Task.FromResult((IResponse)CreateHandshakeMessage(negotiatedVersion));
 
                 case CommandLineOptionMessages commandLineOptionMessages:
                     OnCommandLineOptionMessages(commandLineOptionMessages);
@@ -236,15 +236,26 @@ private Task<IResponse> OnRequest(IRequest request)
 
     private static string GetSupportedProtocolVersion(HandshakeMessage handshakeMessage)
     {
-        handshakeMessage.Properties.TryGetValue(HandshakeMessagePropertyNames.SupportedProtocolVersions, out string protocolVersions);
+        if (!handshakeMessage.Properties.TryGetValue(HandshakeMessagePropertyNames.SupportedProtocolVersions, out string protocolVersions) ||
+            protocolVersions is null)
+        {
+            // It's not expected we hit this.
+            // TODO: Maybe we should fail more hard?
+            return string.Empty;
+        }
 
-        string version = string.Empty;
-        if (protocolVersions is not null && protocolVersions.Split(";").Contains(ProtocolConstants.Version))
+        // NOTE: Today, ProtocolConstants.Version is only 1.0.0 (i.e, SDK supports only a single version).
+        // Whenever we support multiple versions in SDK, we should do intersection
+        // between protocolVersions given by MTP, and the versions supported by SDK.
+        // Then we return the "highest" version from the intersection.
+        // The current logic **assumes** that ProtocolConstants.SupportedVersions is a single version.
+        if (protocolVersions.Split(";").Contains(ProtocolConstants.SupportedVersions))
         {
-            version = ProtocolConstants.Version;
+            return ProtocolConstants.SupportedVersions;
         }
 
-        return version;
+        // The version given by MTP is not supported by SDK.
+        return string.Empty;
     }
 
     private static HandshakeMessage CreateHandshakeMessage(string version) =>
@@ -305,9 +316,9 @@ private bool ModulePathExists()
         return true;
     }
 
-    public void OnHandshakeMessage(HandshakeMessage handshakeMessage)
+    public void OnHandshakeMessage(HandshakeMessage handshakeMessage, bool gotSupportedVersion)
     {
-        HandshakeReceived?.Invoke(this, new HandshakeArgs { Handshake = new Handshake(handshakeMessage.Properties) });
+        HandshakeReceived?.Invoke(this, new HandshakeArgs { Handshake = new Handshake(handshakeMessage.Properties), GotSupportedVersion = gotSupportedVersion });
     }
 
     public void OnCommandLineOptionMessages(CommandLineOptionMessages commandLineOptionMessages)
@@ -387,7 +398,35 @@ public void Dispose()
     {
         foreach (var namedPipeServer in _testAppPipeConnections)
         {
-            namedPipeServer.Dispose();
+            try
+            {
+                namedPipeServer.Dispose();
+            }
+            catch (Exception ex)
+            {
+                StringBuilder messageBuilder;
+                if (_handshakes.TryGetValue(namedPipeServer, out var handshake))
+                {
+                    messageBuilder = new StringBuilder(CliCommandStrings.DotnetTestPipeFailureHasHandshake);
+                    messageBuilder.AppendLine();
+                    foreach (var kvp in handshake.Properties)
+                    {
+                        messageBuilder.AppendLine($"{kvp.Key}: {kvp.Value}");
+                    }                    
+                }
+                else
+                {
+                    messageBuilder = new StringBuilder(CliCommandStrings.DotnetTestPipeFailureWithoutHandshake);
+                    messageBuilder.AppendLine();
+                }
+
+                messageBuilder.AppendLine($"RunCommand: {Module.RunProperties.Command}");
+                messageBuilder.AppendLine($"RunArguments: {Module.RunProperties.Arguments}");
+                messageBuilder.AppendLine(ex.ToString());
+
+                HasFailureDuringDispose = true;
+                Reporter.Error.WriteLine(messageBuilder.ToString());
+            }
         }
 
         WaitOnTestApplicationPipeConnectionLoop();