Call CleanUpAsync on the validator after reaching a terminal status (#375)

Progress on NuGet/Engineering#1190

Author: joelverhagen

src/NuGet.Services.Validation.Orchestrator/ValidationSetProcessor.cs

@@ -127,10 +127,12 @@ private async Task<bool> ProcessIncompleteValidations(PackageValidationSet valid
 
                         case ValidationStatus.Failed:
                             await _validationStorageService.UpdateValidationStatusAsync(packageValidation, validationResult);
+                            await validator.CleanUpAsync(validationRequest);
                             break;
 
                         case ValidationStatus.Succeeded:
                             await _validationStorageService.UpdateValidationStatusAsync(packageValidation, validationResult);
+                            await validator.CleanUpAsync(validationRequest);
                             // need another iteration to try running new validations
                             tryMoreValidations = true;
                             break;
@@ -217,6 +219,12 @@ private async Task<bool> ProcessNotStartedValidations(PackageValidationSet valid
                     {
                         await _validationStorageService.MarkValidationStartedAsync(packageValidation, validationResult);
 
+                        if (validationResult.Status == ValidationStatus.Succeeded
+                            || validationResult.Status == ValidationStatus.Failed)
+                        {
+                            await validator.CleanUpAsync(validationRequest);
+                        }
+
                         _telemetryService.TrackValidatorStarted(packageValidation.Type);
                     }
 

tests/NuGet.Services.Validation.Orchestrator.Tests/ValidationSetProcessorFacts.cs

@@ -15,9 +15,9 @@
 
 namespace NuGet.Services.Validation.Orchestrator.Tests
 {
-    public class StrictStorageFacts : ValidationSetProcessorFactsBase
+    public class StrictMockValidationSetProcessorFacts : ValidationSetProcessorFactsBase
     {
-        public StrictStorageFacts()
+        public StrictMockValidationSetProcessorFacts()
             : base(validationStorageMockBehavior: MockBehavior.Strict)
         {
         }
@@ -39,14 +39,14 @@ public async Task FailsUnknownValidations()
         }
     }
 
-    public class DefaultMockFacts : ValidationSetProcessorFactsBase
+    public class DefaultMockValidationSetProcessorFacts : ValidationSetProcessorFactsBase
     {
         [Theory]
-        [InlineData(ValidationStatus.NotStarted, false)]
-        [InlineData(ValidationStatus.Incomplete, true)]
-        [InlineData(ValidationStatus.Succeeded, true)]
-        [InlineData(ValidationStatus.Failed, true)]
-        public async Task StartsNotStartedValidations(ValidationStatus startStatus, bool expectStorageUpdate)
+        [InlineData(ValidationStatus.NotStarted, false, false)]
+        [InlineData(ValidationStatus.Incomplete, true, false)]
+        [InlineData(ValidationStatus.Succeeded, true, true)]
+        [InlineData(ValidationStatus.Failed, true, true)]
+        public async Task StartsNotStartedValidations(ValidationStatus startStatus, bool expectStorageUpdate, bool expectCleanup)
         {
             UseDefaultValidatorProvider();
             const string validationName = "validation1";
@@ -96,6 +96,19 @@ public async Task StartsNotStartedValidations(ValidationStatus startStatus, bool
                 TelemetryServiceMock.Verify(
                     ts => ts.TrackValidatorStarted(It.IsAny<string>()), Times.Never);
             }
+
+            if (expectCleanup)
+            {
+                validator.Verify(
+                    x => x.CleanUpAsync(It.IsAny<IValidationRequest>()),
+                    Times.Once);
+            }
+            else
+            {
+                validator.Verify(
+                    x => x.CleanUpAsync(It.IsAny<IValidationRequest>()),
+                    Times.Never);
+            }
         }
 
         [Fact]
@@ -123,8 +136,12 @@ public async Task DoesNotStartValidationWithUnmetPrerequisites()
             var processor = CreateProcessor();
             await processor.ProcessValidationsAsync(ValidationSet, Package);
 
-            validator2
-                .Verify(v => v.StartAsync(It.IsAny<IValidationRequest>()), Times.Never());
+            validator1.Verify(v => v.StartAsync(It.IsAny<IValidationRequest>()), Times.Never);
+            validator1.Verify(v => v.GetResultAsync(It.IsAny<IValidationRequest>()), Times.Once);
+            validator1.Verify(v => v.CleanUpAsync(It.IsAny<IValidationRequest>()), Times.Never);
+            validator2.Verify(v => v.StartAsync(It.IsAny<IValidationRequest>()), Times.Never);
+            validator2.Verify(v => v.GetResultAsync(It.IsAny<IValidationRequest>()), Times.Never);
+            validator2.Verify(v => v.CleanUpAsync(It.IsAny<IValidationRequest>()), Times.Never);
         }
 
         [Fact]
@@ -185,19 +202,23 @@ public async Task HandlesIncompleteValidationsStatusChanges(ValidationStatus tar
                     vs => vs.UpdateValidationStatusAsync(validation, It.Is<IValidationResult>(r => r.Status == targetStatus)), Times.Once());
                 ValidationStorageMock.Verify(
                     vs => vs.UpdateValidationStatusAsync(It.IsAny<PackageValidation>(), It.IsAny<ValidationResult>()), Times.Once());
+                validator.Verify(
+                    x => x.CleanUpAsync(It.IsAny<IValidationRequest>()), Times.Once);
             }
             else
             {
                 ValidationStorageMock.Verify(
                     vs => vs.UpdateValidationStatusAsync(It.IsAny<PackageValidation>(), It.IsAny<ValidationResult>()), Times.Never());
+                validator.Verify(
+                    x => x.CleanUpAsync(It.IsAny<IValidationRequest>()), Times.Never);
             }
             Assert.Equal(targetStatus, validation.ValidationStatus);
         }
 
         [Theory]
         [InlineData(ValidationStatus.Failed)]
         [InlineData(ValidationStatus.Succeeded)]
-        public async Task PersistsValidationIssuesOnStart(ValidationStatus targetStatus)
+        public async Task HandlesTerminalStatusOnStart(ValidationStatus targetStatus)
         {
             UseDefaultValidatorProvider();
             var validator = AddValidation("validation1", TimeSpan.FromDays(1), validationStatus: ValidationStatus.NotStarted);
@@ -228,12 +249,15 @@ public async Task PersistsValidationIssuesOnStart(ValidationStatus targetStatus)
                                 validation,
                                 It.Is<IValidationResult>(r => r.Status == targetStatus && r.Issues.Any())),
                     Times.Once());
+            validator.Verify(
+                x => x.CleanUpAsync(It.Is<IValidationRequest>(y => y != null)),
+                Times.Once);
         }
 
         [Theory]
         [InlineData(ValidationStatus.Failed)]
         [InlineData(ValidationStatus.Succeeded)]
-        public async Task PersistsValidationIssuesOnUpdate(ValidationStatus targetStatus)
+        public async Task HandlesTerminalStatusOnUpdate(ValidationStatus targetStatus)
         {
             UseDefaultValidatorProvider();
             var validator = AddValidation("validation1", TimeSpan.FromDays(1), validationStatus: ValidationStatus.Incomplete);
@@ -263,6 +287,9 @@ public async Task PersistsValidationIssuesOnUpdate(ValidationStatus targetStatus
                                 validation,
                                 It.Is<IValidationResult>(r => r.Status == targetStatus && r.Issues.Any())),
                     Times.Once());
+            validator.Verify(
+                x => x.CleanUpAsync(It.Is<IValidationRequest>(y => y != null)),
+                Times.Once);
         }
 
         [Fact]