Sending validation request right before committing changes to DB (#7084)

* Sending validation request right before committing changes to DB. With tests.

* PR feedback addressed

* Sending failure cleanup for symbol package uploads.

* comment.

Author: agr

src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs

@@ -37,28 +37,19 @@ public AsynchronousPackageValidationInitiator(
             _diagnosticsSource = diagnosticsService.SafeGetSource(nameof(AsynchronousPackageValidationInitiator<TPackageEntity>));
         }
 
+        public PackageStatus GetPackageStatus(TPackageEntity package)
+        {
+            // Give an opportunity to the caller to fail early if StartValidationAsync is guaranteed to fail
+            ValidateAndGetType(package);
+
+            return TargetPackageStatus;
+        }
+
         public async Task<PackageStatus> StartValidationAsync(TPackageEntity package)
         {
-            if (_appConfiguration.ReadOnlyMode)
-            {
-                throw new ReadOnlyModeException(Strings.CannotEnqueueDueToReadOnly);
-            }
+            var validatingType = ValidateAndGetType(package);
 
-            ValidatingType validatingType;
             var entityKey = package.Key == default(int) ? (int?)null : package.Key;
-            if (package is Package)
-            {
-                validatingType = ValidatingType.Package;
-            }
-            else if (package is SymbolPackage)
-            {
-                validatingType = ValidatingType.SymbolPackage;
-            }
-            else
-            {
-                throw new ArgumentException($"Unknown IPackageEntity type: {nameof(package)}");
-            }
-
             var data = new PackageValidationMessageData(
                 package.Id,
                 package.Version,
@@ -75,12 +66,30 @@ public async Task<PackageStatus> StartValidationAsync(TPackageEntity package)
                 await _validationEnqueuer.StartValidationAsync(data, postponeProcessingTill);
             }
 
-            if (_appConfiguration.BlockingAsynchronousPackageValidationEnabled)
+            return TargetPackageStatus;
+        }
+
+        private PackageStatus TargetPackageStatus => _appConfiguration.BlockingAsynchronousPackageValidationEnabled
+                           ? PackageStatus.Validating
+                           : PackageStatus.Available;
+
+        private ValidatingType ValidateAndGetType(TPackageEntity package)
+        {
+            if (_appConfiguration.ReadOnlyMode)
+            {
+                throw new ReadOnlyModeException(Strings.CannotEnqueueDueToReadOnly);
+            }
+
+            if (package is Package)
+            {
+                return ValidatingType.Package;
+            }
+            else if (package is SymbolPackage)
             {
-                return PackageStatus.Validating;
+                return ValidatingType.SymbolPackage;
             }
 
-            return PackageStatus.Available;
+            throw new ArgumentException($"Unknown IPackageEntity type: {nameof(package)}");
         }
     }
 }

src/NuGetGallery/Services/IPackageValidationInitiator.cs

@@ -12,6 +12,19 @@ namespace NuGetGallery
     public interface IPackageValidationInitiator<TPackageEntity> 
         where TPackageEntity: IPackageEntity
     {
+        /// <summary>
+        /// Returns the package status that package should go into when validation is started.
+        /// Async validations typically return <see cref="PackageStatus.Validating"/>.
+        /// Sync, non-blocking or no validation typically return <see cref="PackageStatus.Available"/>.
+        /// Caller still must call <see cref="IPackageValidationInitiator{TPackageEntity}.StartValidationAsync(TPackageEntity)"/>
+        /// to start the actual validation.
+        /// </summary>
+        /// <param name="package">The <see cref="TPackageEntity"/> to get future validation status for.</param>
+        /// <returns>The new package status assuming the validation will be started later.</returns>
+        /// <remarks>This method validates the argument the same way <see cref="StartValidationAsync(TPackageEntity)"/> does
+        /// and will throw on invalid input allowing to fail earlier, before expensive operations are performed.</remarks>
+        PackageStatus GetPackageStatus(TPackageEntity package);
+
         /// <summary>
         /// Starts the validation for the specified IPackageEntity. The validation can be done asynchronously with respect
         /// to the gallery and therefore may not be complete when the returned <see cref="Task"/> completes. This

src/NuGetGallery/Services/IValidationService.cs

@@ -13,18 +13,40 @@ namespace NuGetGallery
     /// </summary>
     public interface IValidationService
     {
+        /// <summary>
+        /// Updates the package with the expected <see cref="PackageStatus"/> that the package will
+        /// have after starting the validation.
+        /// The caller must also call <see cref="StartValidationAsync(Package)"/>
+        /// at later time.
+        /// </summary>
+        /// <param name="package">The package to update</param>
+        /// <remarks>This method only updates the object passed into it, no database commit is performed.</remarks>
+        Task UpdatePackageAsync(Package package);
+
+        /// <summary>
+        /// Updates the symbol package with the expected <see cref="PackageStatus"/> that the package will
+        /// have after starting the validation.
+        /// The caller must also call <see cref="StartValidationAsync(Package)"/>
+        /// at later time.
+        /// </summary>
+        /// <param name="package">The package to update</param>
+        /// <remarks>This method only updates the object passed into it, no database commit is performed.</remarks>
+        Task UpdatePackageAsync(SymbolPackage symbolPackage);
+
         /// <summary>
         /// Starts the asynchronous validation for the provided new package and puts the package in the correct
         /// <see cref="Package.PackageStatusKey"/>. The commit to the database is the responsibility of the caller.
         /// </summary>
         /// <param name="package">The package to start validation for.</param>
+        /// <remarks>This method only updates the object passed into it, no database commit is performed.</remarks>
         Task StartValidationAsync(Package package);
 
         /// <summary>
         /// Starts the asynchronous validation for the provided new symbol package and puts the symbol package in the correct
         /// <see cref="Package.PackageStatusKey"/>. The commit to the database is the responsibility of the caller.
         /// </summary>
         /// <param name="symbolPackage">The symbol package to start validation for.</param>
+        /// <remarks>This method only updates the object passed into it, no database commit is performed.</remarks>
         Task StartValidationAsync(SymbolPackage symbolPackage);
 
         /// <summary>

src/NuGetGallery/Services/ImmediatePackageValidator.cs

@@ -13,6 +13,9 @@ namespace NuGetGallery
     public class ImmediatePackageValidator<TPackageEntity> : IPackageValidationInitiator<TPackageEntity> 
         where TPackageEntity: IPackageEntity
     {
+        public PackageStatus GetPackageStatus(TPackageEntity package)
+            => PackageStatus.Available;
+
         public Task<PackageStatus> StartValidationAsync(TPackageEntity package)
         {
             return Task.FromResult(PackageStatus.Available);

src/NuGetGallery/Services/PackageUploadService.cs

@@ -674,7 +674,7 @@ public async Task<Package> GeneratePackageAsync(
 
         public async Task<PackageCommitResult> CommitPackageAsync(Package package, Stream packageFile)
         {
-            await _validationService.StartValidationAsync(package);
+            await _validationService.UpdatePackageAsync(package);
 
             if (package.PackageStatusKey != PackageStatus.Available
                 && package.PackageStatusKey != PackageStatus.Validating)
@@ -763,12 +763,18 @@ await _coreLicenseFileService.DeleteLicenseFileAsync(
 
             try
             {
+                // Sending the validation request after copying to prevent multiple validation requests
+                // sent when several pushes for the same package happen concurrently. Copying the file
+                // resolves the race and only one request will "win" and reach this code.
+                await _validationService.StartValidationAsync(package);
+
                 // commit all changes to database as an atomic transaction
                 await _entitiesContext.SaveChangesAsync();
             }
             catch (Exception ex)
             {
-                // If saving to the DB fails for any reason we need to delete the package we just saved.
+                // If sending the validation request or saving to the DB fails for any reason
+                // we need to delete the package we just saved.
                 if (package.PackageStatusKey == PackageStatus.Validating)
                 {
                     await _packageFileService.DeleteValidationPackageFileAsync(

src/NuGetGallery/Services/SymbolPackageUploadService.cs

@@ -149,7 +149,7 @@ public async Task<PackageCommitResult> CreateAndUploadSymbolsPackage(Package pac
             var previousSymbolsPackage = package.LatestSymbolPackage();
             var symbolPackage = _symbolPackageService.CreateSymbolPackage(package, packageStreamMetadata);
 
-            await _validationService.StartValidationAsync(symbolPackage);
+            await _validationService.UpdatePackageAsync(symbolPackage);
 
             if (symbolPackage.StatusKey != PackageStatus.Available
                 && symbolPackage.StatusKey != PackageStatus.Validating)
@@ -208,14 +208,19 @@ public async Task<PackageCommitResult> CreateAndUploadSymbolsPackage(Package pac
 
                 try
                 {
+                    // Sending the validation request right before updating the database, so all file operations
+                    // are complete by that time and all possible conflicts are resolved.
+                    await _validationService.StartValidationAsync(symbolPackage);
+
                     // commit all changes to database as an atomic transaction
                     await _entitiesContext.SaveChangesAsync();
                 }
                 catch (Exception ex)
                 {
                     ex.Log();
 
-                    // If saving to the DB fails for any reason we need to delete the package we just saved.
+                    // If sending the validation request or saving to the DB fails for any reason
+                    // we need to delete the package we just saved.
                     if (symbolPackage.StatusKey == PackageStatus.Validating)
                     {
                         await _symbolPackageFileService.DeleteValidationPackageFileAsync(

src/NuGetGallery/Services/ValidationService.cs

@@ -50,14 +50,25 @@ public ValidationService(
             }
         }
 
+        public async Task UpdatePackageAsync(Package package)
+        {
+            var packageStatus = _packageValidationInitiator.GetPackageStatus(package);
+
+            await UpdatePackageInternalAsync(package, packageStatus);
+        }
+
+        public async Task UpdatePackageAsync(SymbolPackage symbolPackage)
+        {
+            var symbolPackageStatus = _symbolPackageValidationInitiator.GetPackageStatus(symbolPackage);
+
+            await UpdateSymbolPackageInternalAsync(symbolPackage, symbolPackageStatus);
+        }
+
         public async Task StartValidationAsync(Package package)
         {
             var packageStatus = await _packageValidationInitiator.StartValidationAsync(package);
 
-            await _packageService.UpdatePackageStatusAsync(
-                package,
-                packageStatus,
-                commitChanges: false);
+            await UpdatePackageInternalAsync(package, packageStatus);
         }
 
         public async Task RevalidateAsync(Package package)
@@ -92,6 +103,34 @@ public IReadOnlyList<ValidationIssue> GetLatestPackageValidationIssues(SymbolPac
             return GetValidationIssues(symbolPackage.Key, symbolPackage.StatusKey, ValidatingType.SymbolPackage);
         }
 
+        public async Task StartValidationAsync(SymbolPackage symbolPackage)
+        {
+            var symbolPackageStatus = await _symbolPackageValidationInitiator.StartValidationAsync(symbolPackage);
+            await UpdateSymbolPackageInternalAsync(symbolPackage, symbolPackageStatus);
+        }
+
+        public async Task RevalidateAsync(SymbolPackage symbolPackage)
+        {
+            await _symbolPackageValidationInitiator.StartValidationAsync(symbolPackage);
+
+            _telemetryService.TrackSymbolPackageRevalidate(symbolPackage.Id, symbolPackage.Version);
+        }
+
+        private async Task UpdatePackageInternalAsync(Package package, PackageStatus packageStatus)
+        {
+            await _packageService.UpdatePackageStatusAsync(
+                package,
+                packageStatus,
+                commitChanges: false);
+        }
+
+        private async Task UpdateSymbolPackageInternalAsync(SymbolPackage symbolPackage, PackageStatus symbolPackageStatus)
+        {
+            await _symbolPackageService.UpdateStatusAsync(symbolPackage,
+                symbolPackageStatus,
+                commitChanges: false);
+        }
+
         private IReadOnlyList<ValidationIssue> GetValidationIssues(int entityKey, PackageStatus status, ValidatingType validatingType)
         {
             IReadOnlyList<ValidationIssue> issues = new ValidationIssue[0];
@@ -118,20 +157,5 @@ private IReadOnlyList<ValidationIssue> GetValidationIssues(int entityKey, Packag
 
             return issues;
         }
-
-        public async Task StartValidationAsync(SymbolPackage symbolPackage)
-        {
-            var symbolPackageStatus = await _symbolPackageValidationInitiator.StartValidationAsync(symbolPackage);
-            await _symbolPackageService.UpdateStatusAsync(symbolPackage,
-                symbolPackageStatus,
-                commitChanges: false);
-        }
-
-        public async Task RevalidateAsync(SymbolPackage symbolPackage)
-        {
-            await _symbolPackageValidationInitiator.StartValidationAsync(symbolPackage);
-
-            _telemetryService.TrackSymbolPackageRevalidate(symbolPackage.Id, symbolPackage.Version);
-        }
     }
 }

tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs

@@ -1489,7 +1489,7 @@ public async Task CommitsAfterSavingSupportedPackageStatuses(PackageStatus packa
                 _package.PackageStatusKey = PackageStatus.FailedValidation;
 
                 _validationService
-                    .Setup(vs => vs.StartValidationAsync(_package))
+                    .Setup(vs => vs.UpdatePackageAsync(_package))
                     .Returns(Task.CompletedTask)
                     .Callback(() => _package.PackageStatusKey = packageStatus);
 
@@ -1508,7 +1508,7 @@ public async Task RejectsUnsupportedPackageStatuses(PackageStatus packageStatus)
                 _package.PackageStatusKey = PackageStatus.Available;
 
                 _validationService
-                    .Setup(vs => vs.StartValidationAsync(_package))
+                    .Setup(vs => vs.UpdatePackageAsync(_package))
                     .Returns(Task.CompletedTask)
                     .Callback(() => _package.PackageStatusKey = packageStatus);
 
@@ -1538,33 +1538,34 @@ public async Task StartsAsynchronousValidation(PackageStatus packageStatus)
 
             [Theory]
             [MemberData(nameof(SupportedPackageStatuses))]
-            public async Task StartsValidationBeforeOtherPackageOperations(PackageStatus packageStatus)
+            public async Task StartsValidationAfterSavingPackage(PackageStatus packageStatus)
             {
                 _package.PackageStatusKey = packageStatus;
 
-                bool otherOperationsDone = false;
+                bool contextSaveDone = false;
+                bool packageSaved = false;
                 _validationService
-                    .Setup(vs => vs.StartValidationAsync(It.IsAny<Package>()))
+                    .Setup(vs => vs.StartValidationAsync(_package))
                     .Returns(Task.CompletedTask)
-                    .Callback(() => Assert.False(otherOperationsDone));
+                    .Callback(() => Assert.True(packageSaved && !contextSaveDone));
 
                 _entitiesContext
                     .Setup(ec => ec.SaveChangesAsync())
                     .Returns(Task.FromResult(1))
-                    .Callback(() => otherOperationsDone = true);
+                    .Callback(() => contextSaveDone = true);
                 _packageFileService
                     .Setup(pfs => pfs.SaveValidationPackageFileAsync(It.IsAny<Package>(), It.IsAny<Stream>()))
                     .Returns(Task.CompletedTask)
-                    .Callback(() => otherOperationsDone = true);
+                    .Callback(() => packageSaved = true);
                 _packageFileService
                     .Setup(x => x.SavePackageFileAsync(It.IsAny<Package>(), It.IsAny<Stream>()))
                     .Returns(Task.CompletedTask)
-                    .Callback(() => otherOperationsDone = true);
+                    .Callback(() => packageSaved = true);
 
                 var result = await _target.CommitPackageAsync(_package, _packageFile);
 
                 _validationService
-                    .Verify(vs => vs.StartValidationAsync(It.IsAny<Package>()),
+                    .Verify(vs => vs.StartValidationAsync(_package),
                     Times.AtLeastOnce);
                 _entitiesContext.Verify(
                     x => x.SaveChangesAsync(),

tests/NuGetGallery.Facts/Services/SymbolPackageUploadServiceFacts.cs

@@ -228,7 +228,7 @@ public async Task WillThrowExceptionIfValidationDoesNotSetValidStatus(PackageSta
             {
                 var validationService = new Mock<IValidationService>();
                 validationService
-                    .Setup(x => x.StartValidationAsync(It.IsAny<SymbolPackage>()))
+                    .Setup(x => x.UpdatePackageAsync(It.IsAny<SymbolPackage>()))
                     .Returns((SymbolPackage sp) =>
                     {
                         sp.StatusKey = invalidStatus;