Prevent duplicate validations from starting during some grace period (#315)

* Added check for existence of the previously created validation set for a given package id&version.
Validation request is discarded if such validation set exists.

* Typo fix

* TryGetOrCreateValidationSetAsync

* OtherRecentValidationSetForPackageExists

* NewValidationRequestDeduplicationWindow

* Log on deduping.

Author: agr

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

@@ -17,7 +17,10 @@ public interface IValidationSetProvider
         /// </summary>
         /// <param name="validationTrackingId">Validation tracking id</param>
         /// <param name="package">Package details from Gallery DB</param>
-        /// <returns><see cref="PackageValidationSet"/> object with information about requested <paramref name="validationTrackingId"/></returns>
-        Task<PackageValidationSet> GetOrCreateValidationSetAsync(Guid validationTrackingId, Package package);
+        /// <returns><see cref="PackageValidationSet"/> object with information about
+        /// requested <paramref name="validationTrackingId"/>. Null if no further processing
+        /// should be made (e.g. duplicate validation request was detected).
+        /// </returns>
+        Task<PackageValidationSet> TryGetOrCreateValidationSetAsync(Guid validationTrackingId, Package package);
     }
 }

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

@@ -50,5 +50,21 @@ public interface IValidationStorageService
         /// <param name="validationResult">The result of the validation.</param>
         /// <returns>Task object tracking the async operation status.</returns>
         Task UpdateValidationStatusAsync(PackageValidation packageValidation, IValidationResult validationResult);
+
+        /// <summary>
+        /// Checks whether a validation set was created within the time range specified
+        /// by <paramref name="recentDuration"/> argument with tracking id that is different
+        /// from the supplied as a <paramref name="currentValidationSetTrackingId"/> argument.
+        /// </summary>
+        /// <param name="packageId">Package ID for which recent validation sets are to be looked up.</param>
+        /// <param name="normalizedVersion">Normalized version of the package.</param>
+        /// <param name="recentDuration">Max amount of time to look back.</param>
+        /// <param name="currentValidationSetTrackingId">Validation set tracking for the currently processed request.</param>
+        /// <returns>True if validation set exists, false otherwise.</returns>
+        Task<bool> OtherRecentValidationSetForPackageExists(
+            string packageId,
+            string normalizedVersion,
+            TimeSpan recentDuration,
+            Guid currentValidationSetTrackingId);
     }
 }

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

@@ -25,5 +25,13 @@ public class ValidationConfiguration
         /// Time to wait between checking the state of a certain validation.
         /// </summary>
         public TimeSpan ValidationMessageRecheckPeriod { get; set; }
+
+        /// <summary>
+        /// The duplication detection window used for new validation requests.
+        /// Validation requests will be ignored if there exists another validation
+        /// request for the same package id and version that was created within
+        /// this window.
+        /// </summary>
+        public TimeSpan NewValidationRequestDeduplicationWindow { get; set; }
     }
 }

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

@@ -4,7 +4,7 @@
 namespace NuGet.Services.Validation.Orchestrator
 {
     /// <summary>
-    /// Options for orchestrator for how to handle valiation failure
+    /// Options for orchestrator for how to handle validation failure
     /// </summary>
     public enum ValidationFailureBehavior
     {

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

@@ -46,7 +46,16 @@ public async Task<bool> HandleAsync(PackageValidationMessageData message)
 
             using (_logger.BeginScope("Handling message for {PackageId} {PackageVersion} validation set {ValidationSetId}", message.PackageId, message.PackageVersion, message.ValidationTrackingId))
             {
-                var validationSet = await _validationSetProvider.GetOrCreateValidationSetAsync(message.ValidationTrackingId, package);
+                var validationSet = await _validationSetProvider.TryGetOrCreateValidationSetAsync(message.ValidationTrackingId, package);
+
+                if (validationSet == null)
+                {
+                    _logger.LogInformation("The validation request for {PackageId} {PackageVersion} validation set {ValidationSetId} is a duplicate. Discarding.",
+                        message.PackageId,
+                        message.PackageVersion,
+                        message.ValidationTrackingId);
+                    return true;
+                }
 
                 await _validationSetProcessor.ProcessValidationsAsync(validationSet, package);
                 await _validationOutcomeProcessor.ProcessValidationOutcomeAsync(validationSet, package);

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

@@ -30,12 +30,21 @@ public ValidationSetProvider(
             _logger = logger ?? throw new ArgumentNullException(nameof(logger));
         }
 
-        public async Task<PackageValidationSet> GetOrCreateValidationSetAsync(Guid validationTrackingId, Package package)
+        public async Task<PackageValidationSet> TryGetOrCreateValidationSetAsync(Guid validationTrackingId, Package package)
         {
             var validationSet = await _validationStorageService.GetValidationSetAsync(validationTrackingId);
 
             if (validationSet == null)
             {
+                var shouldSkip = await _validationStorageService.OtherRecentValidationSetForPackageExists(
+                    package.PackageRegistration.Id,
+                    package.NormalizedVersion,
+                    _validationConfiguration.NewValidationRequestDeduplicationWindow,
+                    validationTrackingId);
+                if (shouldSkip)
+                {
+                    return null;
+                }
                 validationSet = await CreateValidationSet(validationTrackingId, package);
             }
             else

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

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Data.Entity;
+using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 
@@ -101,6 +102,21 @@ public async Task UpdateValidationStatusAsync(PackageValidation packageValidatio
             await _validationContext.SaveChangesAsync();
         }
 
+        public async Task<bool> OtherRecentValidationSetForPackageExists(
+            string packageId,
+            string normalizedVersion,
+            TimeSpan recentDuration,
+            Guid currentValidationSetTrackingId)
+        {
+            var cutoffTimestamp = DateTime.UtcNow - recentDuration;
+            return await _validationContext
+                .PackageValidationSets
+                .AnyAsync(pvs => pvs.PackageId == packageId
+                    && pvs.PackageNormalizedVersion == normalizedVersion
+                    && pvs.Created > cutoffTimestamp
+                    && pvs.ValidationTrackingId != currentValidationSetTrackingId);
+        }
+
         private void AddValidationIssues(PackageValidation packageValidation, IReadOnlyList<IValidationIssue> validationIssues)
         {
             foreach (var validationIssue in validationIssues)

src/NuGet.Services.Validation.Orchestrator/settings.json

@@ -10,7 +10,8 @@
       }
     ],
     "ValidationStorageConnectionString": "",
-    "ValidationMessageRecheckPeriod": "00:01:00"
+    "ValidationMessageRecheckPeriod": "00:01:00",
+    "NewValidationRequestDeduplicationWindow": "00:10:00"
   },
   "Vcs": {
     "ContainerName": "validation",

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

@@ -35,6 +35,34 @@ public async Task WaitsForPackageAvailabilityInGalleryDB()
 
             CorePackageServiceMock.Verify(ps => ps.FindPackageByIdAndVersionStrict(messageData.PackageId, messageData.PackageVersion), Times.Once());
         }
+
+        [Fact]
+        public async Task DropsMessageOnDuplicateValidationRequest()
+        {
+            var messageData = new PackageValidationMessageData("packageId", "1.2.3", Guid.NewGuid());
+            var validationConfiguration = new ValidationConfiguration();
+            var package = new Package();
+
+            CorePackageServiceMock
+                .Setup(ps => ps.FindPackageByIdAndVersionStrict(messageData.PackageId, messageData.PackageVersion))
+                .Returns(package)
+                .Verifiable();
+
+            ValidationSetProviderMock
+                .Setup(vsp => vsp.TryGetOrCreateValidationSetAsync(messageData.ValidationTrackingId, package))
+                .ReturnsAsync(null)
+                .Verifiable();
+
+            var handler = CreateHandler();
+
+            var result = await handler.HandleAsync(messageData);
+
+            Assert.True(result);
+            CorePackageServiceMock
+                .Verify(ps => ps.FindPackageByIdAndVersionStrict(messageData.PackageId, messageData.PackageVersion), Times.Once());
+            ValidationSetProviderMock
+                .Verify(vsp => vsp.TryGetOrCreateValidationSetAsync(messageData.ValidationTrackingId, package), Times.Once());
+        }
     }
 
     public class ValidationMessageHandlerLooseFacts : ValidationMessageHandlerFactsBase
@@ -55,7 +83,7 @@ public ValidationMessageHandlerLooseFacts()
                 .Returns(Package);
 
             ValidationSetProviderMock
-                .Setup(vsp => vsp.GetOrCreateValidationSetAsync(MessageData.ValidationTrackingId, Package))
+                .Setup(vsp => vsp.TryGetOrCreateValidationSetAsync(MessageData.ValidationTrackingId, Package))
                 .ReturnsAsync(ValidationSet);
         }
 
@@ -66,7 +94,7 @@ public async Task MakesSureValidationSetExists()
             await handler.HandleAsync(MessageData);
 
             ValidationSetProviderMock
-                .Verify(vsp => vsp.GetOrCreateValidationSetAsync(MessageData.ValidationTrackingId, Package));
+                .Verify(vsp => vsp.TryGetOrCreateValidationSetAsync(MessageData.ValidationTrackingId, Package));
         }
 
         [Fact]

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

@@ -32,7 +32,7 @@ public async Task TriesToGetValidationSetFirst()
 
             var provider = CreateProvider();
 
-            var set = await provider.GetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package);
+            var set = await provider.TryGetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package);
 
             ValidationStorageMock
                 .Verify(vs => vs.GetValidationSetAsync(ValidationSet.ValidationTrackingId), Times.Once());
@@ -51,7 +51,7 @@ public async Task ThrowsIfPackageIdDoesNotMatchValidationSet()
 
             var provider = CreateProvider();
 
-            var ex = await Assert.ThrowsAsync<Exception>(() => provider.GetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package));
+            var ex = await Assert.ThrowsAsync<Exception>(() => provider.TryGetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package));
             Assert.Contains(ValidationSet.PackageId, ex.Message);
             Assert.Contains(Package.PackageRegistration.Id, ex.Message);
         }
@@ -67,7 +67,7 @@ public async Task ThrowsIfPackageVersionDoesNotMatchValidationSet()
 
             var provider = CreateProvider();
 
-            var ex = await Assert.ThrowsAsync<Exception>(() => provider.GetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package));
+            var ex = await Assert.ThrowsAsync<Exception>(() => provider.TryGetOrCreateValidationSetAsync(ValidationSet.ValidationTrackingId, Package));
             Assert.Contains(ValidationSet.PackageNormalizedVersion, ex.Message);
             Assert.Contains(Package.NormalizedVersion, ex.Message);
         }
@@ -89,6 +89,10 @@ public async Task ProperlyConstructsValidationSet()
                 .ReturnsAsync(null)
                 .Verifiable();
 
+            ValidationStorageMock
+                .Setup(vs => vs.OtherRecentValidationSetForPackageExists(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<TimeSpan>(), validationTrackingId))
+                .ReturnsAsync(false);
+
             PackageValidationSet createdSet = null;
             ValidationStorageMock
                 .Setup(vs => vs.CreateValidationSetAsync(It.IsAny<PackageValidationSet>()))
@@ -101,7 +105,7 @@ public async Task ProperlyConstructsValidationSet()
                 ConfigurationAccessorMock.Object,
                 LoggerMock.Object);
 
-            var returnedSet = await provider.GetOrCreateValidationSetAsync(validationTrackingId, Package);
+            var returnedSet = await provider.TryGetOrCreateValidationSetAsync(validationTrackingId, Package);
             var endOfCallTimestamp = DateTime.UtcNow;
 
             ValidationStorageMock
@@ -127,6 +131,39 @@ public async Task ProperlyConstructsValidationSet()
             Assert.Contains(createdSet.PackageValidations, v => v.Type == validation2);
         }
 
+        [Fact]
+        public async Task GetOrCreateValidationSetAsyncDoesNotCreateDuplicateValidationSet()
+        {
+            Guid validationTrackingId = Guid.NewGuid();
+
+            ValidationStorageMock
+                .Setup(vs => vs.GetValidationSetAsync(validationTrackingId))
+                .ReturnsAsync(null);
+
+            ValidationStorageMock
+                .Setup(vs => vs.OtherRecentValidationSetForPackageExists(
+                    Package.PackageRegistration.Id,
+                    Package.NormalizedVersion,
+                    It.IsAny<TimeSpan>(),
+                    validationTrackingId))
+                .ReturnsAsync(true);
+
+            var provider = CreateProvider();
+            var result = await provider.TryGetOrCreateValidationSetAsync(validationTrackingId, Package);
+
+            Assert.Null(result);
+            ValidationStorageMock
+                .Verify(
+                    vs => vs.OtherRecentValidationSetForPackageExists(
+                        Package.PackageRegistration.Id,
+                        Package.NormalizedVersion,
+                        It.IsAny<TimeSpan>(),
+                        validationTrackingId),
+                    Times.Once);
+            ValidationStorageMock
+                .Verify(vs => vs.CreateValidationSetAsync(It.IsAny<PackageValidationSet>()), Times.Never);
+        }
+
         public ValidationSetProviderFacts()
         {
             ValidationStorageMock = new Mock<IValidationStorageService>(MockBehavior.Strict);