Address retry response file review feedback

src/Platform/Microsoft.Testing.Extensions.Retry/InternalAPI/InternalAPI.Unshipped.txt

@@ -1,2 +1,5 @@
 #nullable enable
 static Microsoft.Testing.Extensions.Policy.RetryArgumentsBuilder.BuildAttemptArgumentsAsync(Microsoft.Testing.Platform.Helpers.IFileSystem! fileSystem, string![]! executableArguments, string![]! originalExecutableArguments, System.Collections.Generic.List<int>! indexToCleanup, string! currentTryResultFolder, string! retryRootFolder, string! pipeName, string![]? lastListOfFailedId, int attemptCount) -> System.Threading.Tasks.Task<System.Collections.Generic.List<string!>!>!
+static Microsoft.Testing.Extensions.Policy.RetryArgumentsBuilder.GetArgumentsResponseFilePath(string! retryRootFolder, int attemptCount) -> string!
+static Microsoft.Testing.Extensions.Policy.RetryArgumentsBuilder.GetFilterUidsResponseFilePath(string! retryRootFolder, int attemptCount) -> string!
+static Microsoft.Testing.Extensions.Policy.RetryOrchestrator.LogResponseFileFallbackWarningAsync(Microsoft.Testing.Platform.Logging.ILogger! logger, string![]! originalExecutableArguments, System.Collections.Generic.List<string!>! finalArguments, string! generatedResponseFilePath) -> System.Threading.Tasks.Task!

src/Platform/Microsoft.Testing.Extensions.Retry/RetryArgumentsBuilder.cs

@@ -20,6 +20,12 @@ internal static class RetryArgumentsBuilder
     private const int CommandLineLengthLimit = 30_000;
     private const int PerArgumentOverhead = 3;
 
+    internal static string GetArgumentsResponseFilePath(string retryRootFolder, int attemptCount)
+        => Path.Combine(retryRootFolder, $"retry-arguments-{attemptCount}.rsp");
+
+    internal static string GetFilterUidsResponseFilePath(string retryRootFolder, int attemptCount)
+        => Path.Combine(retryRootFolder, $"retry-filter-uids-{attemptCount}.rsp");
+
     /// <summary>
     /// Computes the indices of the original executable arguments that must be dropped when restarting the test
     /// host, namely the retry-specific options and the result-directory option (which is re-injected per attempt).
@@ -143,9 +149,10 @@ public static async Task<List<string>> BuildAttemptArgumentsAsync(
             }
         }
 
-        if (directPrefixArguments is not null && !finalArguments.Any(argument => argument.IndexOf('"') >= 0))
+        if (directPrefixArguments is not null
+            && !finalArguments.Skip(directPrefixArguments.Count).Any(argument => argument.IndexOf('"') >= 0))
         {
-            string responseFilePath = Path.Combine(retryRootFolder, $"retry-arguments-{attemptCount}.rsp");
+            string responseFilePath = GetArgumentsResponseFilePath(retryRootFolder, attemptCount);
             using (IFileStream stream = fileSystem.NewFileStream(responseFilePath, FileMode.Create, FileAccess.Write))
             using (var writer = new StreamWriter(stream.Stream))
             {
@@ -203,7 +210,7 @@ public static async Task<List<string>> BuildAttemptArgumentsAsync(
                 // Use a response file to avoid exceeding command-line length limits.
                 // Write to retryRootFolder (not the per-attempt folder) so it won't be included
                 // in the final results move.
-                string responseFilePath = Path.Combine(retryRootFolder, $"retry-filter-uids-{attemptCount}.rsp");
+                string responseFilePath = GetFilterUidsResponseFilePath(retryRootFolder, attemptCount);
                 using (IFileStream stream = fileSystem.NewFileStream(responseFilePath, FileMode.Create, FileAccess.Write))
                 using (var writer = new StreamWriter(stream.Stream))
                 {

src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs

@@ -223,8 +223,8 @@ await outputDevice.DisplayAsync(
             RetryTestHostRunner.AttemptResult attemptResult;
             string[] generatedResponseFilePaths =
             [
-                Path.Combine(retryRootFolder, $"retry-arguments-{attemptCount}.rsp"),
-                Path.Combine(retryRootFolder, $"retry-filter-uids-{attemptCount}.rsp"),
+                RetryArgumentsBuilder.GetArgumentsResponseFilePath(retryRootFolder, attemptCount),
+                RetryArgumentsBuilder.GetFilterUidsResponseFilePath(retryRootFolder, attemptCount),
             ];
             try
             {
@@ -239,6 +239,12 @@ await outputDevice.DisplayAsync(
                     lastListOfFailedId,
                     attemptCount).ConfigureAwait(false);
 
+                await LogResponseFileFallbackWarningAsync(
+                    logger,
+                    originalExecutableArguments,
+                    finalArguments,
+                    generatedResponseFilePaths[0]).ConfigureAwait(false);
+
                 attemptResult = await RetryTestHostRunner.RunAttemptAsync(
                     _serviceProvider,
                     this,
@@ -507,6 +513,21 @@ private static bool IsHotReloadEnabled(IEnvironment environment)
         => environment.GetEnvironmentVariable(EnvironmentVariableConstants.DOTNET_WATCH) == "1"
         || environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_HOTRELOAD_ENABLED) == "1";
 
+    internal static async Task LogResponseFileFallbackWarningAsync(
+        ILogger logger,
+        string[] originalExecutableArguments,
+        List<string> finalArguments,
+        string generatedResponseFilePath)
+    {
+        if (originalExecutableArguments.Any(argument => argument.StartsWith("@", StringComparison.Ordinal))
+            && !finalArguments.Contains($"@{generatedResponseFilePath}"))
+        {
+            await logger.LogWarningAsync(
+                "Retry arguments could not be regenerated in a response file because an argument contains a literal double quote. "
+                + "The retry command line may exceed the operating system limit.").ConfigureAwait(false);
+        }
+    }
+
     private static void CollectRecoveredArtifacts(
         IFileSystem fileSystem,
         string manifestPath,

src/Platform/Microsoft.Testing.Platform/CommandLine/ResponseFileHelper.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.Testing.Platform.CommandLine;
 using Microsoft.Testing.Platform.Resources;
 
 // Most of the core logic is from:
@@ -20,28 +21,31 @@ internal static bool TryReadResponseFile(
         ICollection<string> errors,
         [NotNullWhen(true)] out string[]? newArguments)
     {
+        var readContext = new ResponseFileReadContext(rspFilePath, diagnosticPath);
         try
         {
             newArguments = [.. ExpandResponseFile(
                 rspFilePath,
+                diagnosticPath,
                 [with(
                     RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
                         ? StringComparer.OrdinalIgnoreCase
-                        : StringComparer.Ordinal)])];
+                        : StringComparer.Ordinal)],
+                readContext)];
             return true;
         }
         catch (FileNotFoundException)
         {
-            errors.Add(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineParserResponseFileNotFound, diagnosticPath));
+            errors.Add(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineParserResponseFileNotFound, readContext.DiagnosticPath));
         }
         catch (Exception e) when (e is IOException or UnauthorizedAccessException)
         {
             errors.Add(
                 string.Format(
                     CultureInfo.InvariantCulture,
                     PlatformResources.CommandLineParserFailedToReadResponseFile,
-                    diagnosticPath,
-                    GetExceptionDetail(e, rspFilePath, diagnosticPath)));
+                    readContext.DiagnosticPath,
+                    GetExceptionDetail(e, readContext.ActualPath, readContext.DiagnosticPath)));
         }
         catch (FormatException e)
         {
@@ -52,8 +56,8 @@ internal static bool TryReadResponseFile(
                 string.Format(
                     CultureInfo.InvariantCulture,
                     PlatformResources.CommandLineParserFailedToReadResponseFile,
-                    diagnosticPath,
-                    GetExceptionDetail(e, rspFilePath, diagnosticPath)));
+                    readContext.DiagnosticPath,
+                    GetExceptionDetail(e, readContext.ActualPath, readContext.DiagnosticPath)));
         }
 
         newArguments = null;
@@ -63,8 +67,13 @@ internal static bool TryReadResponseFile(
         static string GetExceptionDetail(Exception exception, string actualPath, string diagnosticPath)
             => actualPath == diagnosticPath ? exception.ToString() : exception.GetType().Name;
 
-        static IEnumerable<string> ExpandResponseFile(string filePath, HashSet<string> activeResponseFiles)
+        static IEnumerable<string> ExpandResponseFile(
+            string filePath,
+            string diagnosticPath,
+            HashSet<string> activeResponseFiles,
+            ResponseFileReadContext readContext)
         {
+            readContext.SetCurrentFile(filePath, diagnosticPath);
             string fullPath = Path.GetFullPath(filePath);
             if (!activeResponseFiles.Add(fullPath))
             {
@@ -74,24 +83,46 @@ static IEnumerable<string> ExpandResponseFile(string filePath, HashSet<string> a
             try
             {
                 string[] lines = File.ReadAllLines(filePath);
-
+                List<string> arguments = [];
                 for (int i = 0; i < lines.Length; i++)
                 {
-                    string line = lines[i];
+                    arguments.AddRange(SplitLine(lines[i], i + 1));
+                }
 
-                    foreach (string argument in SplitLine(line, i + 1))
+                for (int argumentIndex = 0; argumentIndex < arguments.Count; argumentIndex++)
+                {
+                    string argument = arguments[argumentIndex];
+                    if (argument.StartsWith("@", StringComparison.Ordinal))
                     {
-                        if (argument.StartsWith("@", StringComparison.Ordinal))
+                        string nestedDiagnosticPath;
+                        if (filePath != diagnosticPath)
                         {
-                            foreach (string nestedArgument in ExpandResponseFile(argument.Substring(1), activeResponseFiles))
-                            {
-                                yield return nestedArgument;
-                            }
+                            nestedDiagnosticPath = diagnosticPath;
                         }
                         else
                         {
-                            yield return argument;
+                            string redactedNestedArgument = CommandLineArgumentsRedactor.RedactArgument([.. arguments], argumentIndex);
+                            nestedDiagnosticPath = redactedNestedArgument.StartsWith("@", StringComparison.Ordinal)
+                                ? redactedNestedArgument[1..]
+                                : redactedNestedArgument;
+                        }
+
+                        // Nested response files intentionally use the process working directory, just like
+                        // top-level response files, rather than the containing response file's directory.
+                        foreach (string nestedArgument in ExpandResponseFile(
+                            argument[1..],
+                            nestedDiagnosticPath,
+                            activeResponseFiles,
+                            readContext))
+                        {
+                            yield return nestedArgument;
                         }
+
+                        readContext.SetCurrentFile(filePath, diagnosticPath);
+                    }
+                    else
+                    {
+                        yield return argument;
                     }
                 }
             }
@@ -117,6 +148,19 @@ static IEnumerable<string> SplitLine(string line, int lineNumber)
         }
     }
 
+    private sealed class ResponseFileReadContext(string actualPath, string diagnosticPath)
+    {
+        public string ActualPath { get; private set; } = actualPath;
+
+        public string DiagnosticPath { get; private set; } = diagnosticPath;
+
+        public void SetCurrentFile(string currentActualPath, string currentDiagnosticPath)
+        {
+            ActualPath = currentActualPath;
+            DiagnosticPath = currentDiagnosticPath;
+        }
+    }
+
     private enum Boundary
     {
         TokenStart,

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs

@@ -164,11 +164,15 @@ public async Task RetryFailedTests_WithArgumentsInResponseFile_Succeeds(bool ret
                     { "METHOD1", "1" },
                     { "FAIL", "0" },
                     { "RESULTDIR", resultDirectory },
+                    { "CHECK_RETRY_RESPONSE_FILE_CLEANUP", "1" },
                 },
                 cancellationToken: TestContext.CancellationToken);
 
             testHostResult.AssertExitCodeIs(ExitCode.Success);
             testHostResult.AssertOutputContains("Retry summary: Passed! after 2/2 attempts");
+            Assert.IsEmpty(
+                Directory.GetFiles(resultDirectory, "retry-*.rsp", SearchOption.AllDirectories),
+                "Generated retry response files must be deleted after each attempt.");
         }
         finally
         {
@@ -1001,6 +1005,13 @@ public async Task ExecuteRequestAsync(ExecuteRequestContext context)
         var uidFilter = filter as TestNodeUidListFilter;
         var treeNodeFilter = filter as TreeNodeFilter;
 
+        if (Environment.GetEnvironmentVariable("CHECK_RETRY_RESPONSE_FILE_CLEANUP") == "1"
+            && Environment.GetEnvironmentVariable("TESTINGPLATFORM_DOTNETTEST_ATTEMPTNUMBER") == "2"
+            && Directory.GetFiles(Path.Combine(resultDir, "Retries"), "retry-arguments-1.rsp", SearchOption.AllDirectories).Length != 0)
+        {
+            throw new InvalidOperationException("The response file from retry attempt 1 still exists during attempt 2.");
+        }
+
         var testMethod1Identifier = new TestMethodIdentifierProperty(string.Empty, string.Empty, "DummyClassName", "TestMethod1", 0, Array.Empty<string>(), string.Empty);
         var testMethod2Identifier = new TestMethodIdentifierProperty(string.Empty, string.Empty, "DummyClassName", "TestMethod2", 0, Array.Empty<string>(), string.Empty);
         var testMethod3Identifier = new TestMethodIdentifierProperty(string.Empty, string.Empty, "DummyClassName", "TestMethod3", 0, Array.Empty<string>(), string.Empty);

test/UnitTests/Microsoft.Testing.Extensions.UnitTests/RetryArgumentsBuilderTests.cs

@@ -231,6 +231,81 @@ public async Task BuildAttemptArgumentsAsync_WithOriginalResponseFile_WritesClea
             Encoding.UTF8.GetString(memoryStream.ToArray()));
     }
 
+    [TestMethod]
+    public async Task BuildAttemptArgumentsAsync_WithQuotedDirectPrefix_WritesSuffixToResponseFile()
+    {
+        string retryRoot = Path.Combine("results", "Retries", "run");
+        string responseFilePath = Path.Combine(retryRoot, "retry-arguments-1.rsp");
+        string[] executableArguments = ["exec", "quoted\"prefix", "--keep", "value"];
+        using var memoryStream = new MemoryStream();
+        var fileStream = new Mock<IFileStream>(MockBehavior.Strict);
+        fileStream.SetupGet(stream => stream.Stream).Returns(memoryStream);
+        fileStream.Setup(stream => stream.Dispose());
+        var fileSystem = new Mock<IFileSystem>(MockBehavior.Strict);
+        fileSystem
+            .Setup(fs => fs.NewFileStream(responseFilePath, FileMode.Create, FileAccess.Write))
+            .Returns(fileStream.Object);
+
+        List<string> actual = await RetryArgumentsBuilder.BuildAttemptArgumentsAsync(
+            fileSystem.Object,
+            executableArguments,
+            ["exec", "quoted\"prefix", "@original.rsp"],
+            [],
+            Path.Combine(retryRoot, "1"),
+            retryRoot,
+            "pipe-name",
+            lastListOfFailedId: null,
+            attemptCount: 1).ConfigureAwait(false);
+
+        Assert.AreSequenceEqual(
+            [
+                "exec",
+                "quoted\"prefix",
+                $"@{responseFilePath}",
+                $"--{PlatformCommandLineProvider.ResultDirectoryOptionKey}",
+                Path.Combine(retryRoot, "1"),
+                $"--{RetryCommandLineOptionsProvider.RetryFailedTestsPipeNameOptionName}",
+                "pipe-name",
+            ],
+            actual);
+        Assert.AreEqual(
+            $"\"--keep\"{Environment.NewLine}\"value\"{Environment.NewLine}",
+            Encoding.UTF8.GetString(memoryStream.ToArray()));
+    }
+
+    [TestMethod]
+    public async Task BuildAttemptArgumentsAsync_WithMultipleOriginalResponseFiles_WritesEntireExpandedSuffix()
+    {
+        string retryRoot = Path.Combine("results", "Retries", "run");
+        string responseFilePath = Path.Combine(retryRoot, "retry-arguments-1.rsp");
+        string[] executableArguments = ["exec", "--first", "a", "--between", "b", "--second", "c", "--after", "d"];
+        using var memoryStream = new MemoryStream();
+        var fileStream = new Mock<IFileStream>(MockBehavior.Strict);
+        fileStream.SetupGet(stream => stream.Stream).Returns(memoryStream);
+        fileStream.Setup(stream => stream.Dispose());
+        var fileSystem = new Mock<IFileSystem>(MockBehavior.Strict);
+        fileSystem
+            .Setup(fs => fs.NewFileStream(responseFilePath, FileMode.Create, FileAccess.Write))
+            .Returns(fileStream.Object);
+
+        List<string> actual = await RetryArgumentsBuilder.BuildAttemptArgumentsAsync(
+            fileSystem.Object,
+            executableArguments,
+            ["exec", "@first.rsp", "--between", "b", "@second.rsp", "--after", "d"],
+            [],
+            Path.Combine(retryRoot, "1"),
+            retryRoot,
+            "pipe-name",
+            lastListOfFailedId: null,
+            attemptCount: 1).ConfigureAwait(false);
+
+        Assert.AreEqual("exec", actual[0]);
+        Assert.AreEqual($"@{responseFilePath}", actual[1]);
+        Assert.AreEqual(
+            string.Join(Environment.NewLine, executableArguments.Skip(1).Select(argument => $"\"{argument}\"")) + Environment.NewLine,
+            Encoding.UTF8.GetString(memoryStream.ToArray()));
+    }
+
     [TestMethod]
     public Task BuildAttemptArgumentsAsync_WithNullFailedIds_KeepsOriginalFiltersAndMinimumExpectedTests()
         => AssertFirstAttemptKeepsOriginalFiltersAsync(lastListOfFailedId: null);

test/UnitTests/Microsoft.Testing.Extensions.UnitTests/RetryTests.cs

@@ -30,6 +30,42 @@ public class RetryTests
 {
     private const string ContosoPackageSid = "S-1-15-2-1990679259-4123976751-842158434-3026549936-2944832882-252165955-409282942";
 
+    [DataRow(true)]
+    [DataRow(false)]
+    [TestMethod]
+    public async Task LogResponseFileFallbackWarningAsync_OnlyQuotedSuffixLogsWarning(bool quoteIsInDirectPrefix)
+    {
+        const string GeneratedResponseFilePath = "retry-arguments-1.rsp";
+        string[] originalArguments = quoteIsInDirectPrefix
+            ? ["quoted\"prefix", "@original.rsp"]
+            : ["@original.rsp", "quoted\"suffix"];
+        List<string> finalArguments = quoteIsInDirectPrefix
+            ? ["quoted\"prefix", $"@{GeneratedResponseFilePath}"]
+            : ["quoted\"suffix"];
+        var logger = new Mock<ILogger>();
+        logger
+            .Setup(value => value.LogAsync(
+                LogLevel.Warning,
+                It.IsAny<string>(),
+                null,
+                It.IsAny<Func<string, Exception?, string>>()))
+            .Returns(Task.CompletedTask);
+
+        await RetryOrchestrator.LogResponseFileFallbackWarningAsync(
+            logger.Object,
+            originalArguments,
+            finalArguments,
+            GeneratedResponseFilePath);
+
+        logger.Verify(
+            value => value.LogAsync(
+                LogLevel.Warning,
+                It.Is<string>(message => message.Contains("literal double quote", StringComparison.Ordinal)),
+                null,
+                It.IsAny<Func<string, Exception?, string>>()),
+            quoteIsInDirectPrefix ? Times.Never : Times.Once);
+    }
+
     [TestMethod]
     [OSCondition(ConditionMode.Include, OperatingSystems.Windows, IgnoreMessage = "AppContainer pipe authorization is Windows-only.")]
     public void RetryPipeServer_UsesControllerAuthorizedSecurityIdentities()