Properly handle when git encounters an unrecoverable error (#9423)

* handle git exceptions differently according to how the proxy is invoked, uncovering previously concealed git exceptions
  * git errors from CLI invocations now output the git error prior to early exiting.
  * git errors when proxy is running as a server are now properly returned back to the invoking REST call

tools/test-proxy/Azure.Sdk.Tools.TestProxy.Tests/IntegrationTests/GitStoreIntegrationRestoreTests.cs

@@ -305,7 +305,7 @@ public async Task NonexistentTagFallsBack(string inputJson)
               ""AssetsRepoId"": """",
               ""TagPrefix"": ""main"",
               ""Tag"": ""INVALID_TAG""
-        }", "Invocation of \"git fetch origin refs/tags/INVALID_TAG:refs/tags/INVALID_TAG\" had a non-zero exit code -1")]
+        }", "Invocation of \"git fetch origin refs/tags/INVALID_TAG:refs/tags/INVALID_TAG\" had a non-zero exit code 128")]
         [Trait("Category", "Integration")]
         public async Task InvalidTagThrows(string inputJson, string httpException)
         {

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/Exceptions/GitProcessException.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Net;
@@ -17,6 +17,8 @@ public GitProcessException(CommandResult result)
             Result = result;
         }
 
+        public override string Message { get { return this.Result.StdErr; } }
+
         // Override the ToString so it'll give the command exception's toString which
         // will contain the accurate message and callstack. This is necessary in the
         // event the exception goes unhandled.

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Startup.cs

@@ -80,10 +80,12 @@ private static async Task<int> Run(object commandObj)
             switch (commandObj)
             {
                 case ConfigLocateOptions configOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(configOptions.AssetsJsonPath, TargetLocation);
                     System.Console.WriteLine(await DefaultStore.GetPath(assetsJson));
                     break;
                 case ConfigShowOptions configOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(configOptions.AssetsJsonPath, TargetLocation);
                     using(var f = File.OpenRead(assetsJson))
                     {
@@ -92,6 +94,7 @@ private static async Task<int> Run(object commandObj)
                     }
                     break;
                 case ConfigCreateOptions configOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(configOptions.AssetsJsonPath, TargetLocation);
                     throw new NotImplementedException("Interactive creation of assets.json feature is not yet implemented.");
                 case ConfigOptions configOptions:
@@ -101,14 +104,17 @@ private static async Task<int> Run(object commandObj)
                     StartServer(startOptions);
                     break;
                 case PushOptions pushOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(pushOptions.AssetsJsonPath, TargetLocation);
                     await DefaultStore.Push(assetsJson);
                     break;
                 case ResetOptions resetOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(resetOptions.AssetsJsonPath, TargetLocation);
                     await DefaultStore.Reset(assetsJson);
                     break;
                 case RestoreOptions restoreOptions:
+                    DefaultStore.SetStoreExceptionMode(false);
                     assetsJson = RecordingHandler.GetAssetsJsonLocation(restoreOptions.AssetsJsonPath, TargetLocation);
                     await DefaultStore.Restore(assetsJson);
                     break;

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Store/GitProcessHandler.cs

@@ -27,6 +27,8 @@ public class CommandResult
     public class GitProcessHandler
     {
         public const int RETRY_INTERMITTENT_FAILURE_COUNT = 3;
+
+        public bool ThrowOnException = true;
         /// <summary>
         /// Internal class to hold the minimum supported version of git. If that
         /// version changes we only need to change it here.
@@ -195,16 +197,25 @@ public virtual CommandResult Run(string arguments, string workingDirectory)
                         }
                     }
                 }
-                // exceptions caught here will be to do with inability to start the git process
-                // otherwise all "error" states should be handled by the output to stdErr and non-zero exitcode.
-                catch (Exception e)
+                // exceptions caught here will be to do with inability to recover from a failed git process
+                // rather than endlessly retrying and concealing the git error from the user, immediately report and exit the process with an error code
+                catch (GitProcessException e)
                 {
-                    DebugLogger.LogDebug(e.Message);
+                    // we rethrow here in contexts where we can expect the ASP.NET middleware to handle the thrown exception
+                    if (ThrowOnException)
+                    {
+                        DebugLogger.LogError(e.Result.StdErr);
 
-                    result.ExitCode = -1;
-                    result.CommandException = e;
+                        throw new GitProcessException(e.Result);
+                    }
+                    // otherwise, we log the error, then early exit from the proces (in CLI contexts)
+                    else
+                    {
+                        DebugLogger.LogError("Git ran into an unrecoverable error. Test-Proxy is exiting. The error output from git is: ");
+                        DebugLogger.LogError(e.Result.StdErr);
 
-                    throw new GitProcessException(result);
+                        Environment.Exit(1);
+                    }
                 }
             });
 

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Store/GitStore.cs

@@ -75,6 +75,18 @@ public GitStore(IConsoleWrapper consoleWrapper)
         }
 
         #region push, reset, restore, and other asset repo implementations
+        /// <summary>
+        /// Set the GitHandler exception mode.
+        ///
+        /// When false: unrecoverable git exceptions will print the error, and early exit
+        /// When true: unrecoverable git exceptions will log, then be rethrown for the Exception middleware to handle and return as a valid non-successful http response.
+        /// </summary>
+        /// <param name="throwOnException"></param>
+        public void SetStoreExceptionMode(bool throwOnException)
+        {
+            this.GitHandler.ThrowOnException = throwOnException;
+        }
+
         /// <summary>
         /// Given a config, locate the cloned assets.
         /// </summary>

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Store/IAssetsStore.cs

@@ -33,5 +33,12 @@ public interface IAssetsStore
         /// <param name="pathToAssetsJson"></param>
         /// <returns></returns>
         public abstract Task<NormalizedString> GetPath(string pathToAssetsJson);
+
+        /// <summary>
+        /// Set the mode of the store to throw exceptions or to simply early exit for CLI mode.
+        /// </summary>
+        /// <param name="throwOnException"></param>
+        /// <returns></returns>
+        public abstract void SetStoreExceptionMode(bool throwOnException);
     }
 }

tools/test-proxy/Azure.Sdk.Tools.TestProxy/Store/NullStore.cs

@@ -25,5 +25,6 @@ public AssetsConfiguration ParseConfigurationFile(string pathToAssetsJson)
         }
 
         public Task<NormalizedString> GetPath(string pathToAssetsJson) { return null; }
+        public void SetStoreExceptionMode(bool throwOnException) { }
     }
 }

tools/test-proxy/scripts/test-scripts/CLIIntegration.Tests.ps1

@@ -98,6 +98,26 @@ Describe "AssetsModuleTests" {
             Test-FileVersion -FilePath $localAssetsFilePath -FileName "file4.txt" -ExpectedVersion 1
             Test-FileVersion -FilePath $localAssetsFilePath -FileName "file5.txt" -ExpectedVersion 1
         }
+        It "Should fail to restore an invalid tag" {
+            $recordingJson = [PSCustomObject]@{
+                AssetsRepo           = "Azure/azure-sdk-assets-integration"
+                AssetsRepoPrefixPath = "pull/scenarios"
+                AssetsRepoId         = ""
+                TagPrefix            = "main"
+                Tag                  = "python/tables_fc54d0INVALID"
+            }
+            $files = @(
+                "assets.json"
+            )
+            $testFolder = Describe-TestFolder -AssetsJsonContent $recordingJson -Files $files
+            $assetsFile = Join-Path $testFolder "assets.json"
+            $assetsJsonRelativePath = [System.IO.Path]::GetRelativePath($testFolder, $assetsFile)
+
+            $CommandArgs = "restore --assets-json-path $assetsJsonRelativePath"
+            Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder
+
+            $LASTEXITCODE | Should -Be 1
+        }
         AfterEach {
             Remove-Test-Folder $testFolder
         }
@@ -164,7 +184,7 @@ Describe "AssetsModuleTests" {
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file1.txt" -ExpectedVersion 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file2.txt" -ExpectedVersion 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file3.txt" -ExpectedVersion 1
-    
+
                 # Create a new file and verify
                 Edit-FileVersion -FilePath $localAssetsFilePath -FileName "file4.txt" -Version 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file4.txt" -ExpectedVersion 1
@@ -174,7 +194,7 @@ Describe "AssetsModuleTests" {
                 # Delete a file
                 $fileToRemove = Join-Path -Path $localAssetsFilePath -ChildPath "file2.txt"
                 Remove-Item -Path $fileToRemove
-    
+
                 # Reset answering Y and they should all go back to original restore
                 $CommandArgs = "reset --assets-json-path $assetsJsonRelativePath"
                 Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder -WriteOutput "Y"
@@ -211,7 +231,7 @@ Describe "AssetsModuleTests" {
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file1.txt" -ExpectedVersion 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file2.txt" -ExpectedVersion 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file3.txt" -ExpectedVersion 1
-    
+
                 # Create two new files and verify
                 Edit-FileVersion -FilePath $localAssetsFilePath -FileName "file4.txt" -Version 1
                 Test-FileVersion -FilePath $localAssetsFilePath -FileName "file4.txt" -ExpectedVersion 1
@@ -223,7 +243,7 @@ Describe "AssetsModuleTests" {
                 # Delete a file
                 $fileToRemove = Join-Path -Path $localAssetsFilePath -ChildPath "file2.txt"
                 Remove-Item -Path $fileToRemove
-    
+
                 # Reset answering N and they should remain changed as per the previous changes
                 $CommandArgs = "reset --assets-json-path $assetsJsonRelativePath"
                 Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder -WriteOutput "N"
@@ -446,9 +466,9 @@ Describe "AssetsModuleTests" {
                         TagPrefix            = $created_tag_prefix
                         Tag                  = ""
                     }
-    
+
                     $assetsJsonRelativePath = Join-Path "sdk" "keyvault" "azure-keyvault-keys" "assets.json"
-    
+
                     $files = @(
                         $assetsJsonRelativePath
                     )
@@ -457,16 +477,16 @@ Describe "AssetsModuleTests" {
                     $CommandArgs = "restore --assets-json-path $assetsJsonRelativePath"
                     Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder
                     $LASTEXITCODE | Should -Be 0
-    
+
                     $localAssetsFilePath = Join-Path $testFolder ".assets"
                     $assetsFolder = $(Get-ChildItem $localAssetsFilePath -Directory)[0].FullName
                     mkdir -p $(Join-Path $assetsFolder $creationPath)
-    
+
                     # Create new files. These are in a predictable location with predicatable content so we can generate the same SHA twice in a row.
                     Edit-FileVersion -FilePath $assetsFolder -FileName $file1 -Version 1
                     Edit-FileVersion -FilePath $assetsFolder -FileName $file2 -Version 1
                     Edit-FileVersion -FilePath $assetsFolder -FileName $file3 -Version 1
-                    
+
                     # Push the changes
                     $CommandArgs = "push --assets-json-path $assetsJsonRelativePath"
                     Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder
@@ -489,13 +509,13 @@ Describe "AssetsModuleTests" {
                     Edit-FileVersion -FilePath $newAssetsFolder -FileName $file1 -Version 1
                     Edit-FileVersion -FilePath $newAssetsFolder -FileName $file2 -Version 1
                     Edit-FileVersion -FilePath $newAssetsFolder -FileName $file3 -Version 1
-                    
+
                     $CommandArgs = "push --assets-json-path $assetsJsonRelativePath"
                     Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $newTestFolder
                     $LASTEXITCODE | Should -Be 0
-                    
+
                     $updatedAssets = Update-AssetsFromFile -AssetsJsonContent $newAssetsFile
-    
+
                     $exists = Test-TagExists -AssetsJsonContent $updatedAssets -WorkingDirectory $localAssetsFilePath
                     $exists | Should -Be $true
                 }
@@ -544,7 +564,7 @@ Describe "AssetsModuleTests" {
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file1 -Version 3
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file2 -Version 3
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file3 -Version 3
-                
+
                 # now lets restore again
                 $CommandArgs = "restore --assets-json-path $assetsJsonRelativePath"
                 Invoke-ProxyCommand -TestProxyExe $TestProxyExe -CommandArgs $CommandArgs -MountDirectory $testFolder
@@ -592,7 +612,7 @@ Describe "AssetsModuleTests" {
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file1 -Version 3
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file2 -Version 3
                 Edit-FileVersion -FilePath $assetsFolder -FileName $file3 -Version 3
-               
+
                 # now lets modify the targeted tag. this simulates a user checking out a different branch or commit in their language repo
                 $assetsJsonLocation = Join-Path $testFolder $assetsJsonRelativePath
                 $recordingJson.Tag = "python/tables_fc54d0"