Fix #2514 - Use optimistic concurrency to prevent multiple UpdateIsLa…

…test calls on same package (#3548)

src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Data.Entity;
 using System.Data.Entity.Infrastructure;
 using System.Data.Entity.SqlServer;
@@ -15,19 +16,41 @@ public EntitiesConfiguration()
         {
             // Configure Connection Resiliency / Retry Logic
             // See https://msdn.microsoft.com/en-us/data/dn456835.aspx and msdn.microsoft.com/en-us/data/dn307226
-            SetExecutionStrategy("System.Data.SqlClient", () => SuspendExecutionStrategy
-                ? (IDbExecutionStrategy)new DefaultExecutionStrategy() : new SqlAzureExecutionStrategy());
+            SetExecutionStrategy("System.Data.SqlClient", () => UseRetriableExecutionStrategy
+                ? new SqlAzureExecutionStrategy() : (IDbExecutionStrategy)new DefaultExecutionStrategy());
         }
 
-        public static bool SuspendExecutionStrategy
+        private static bool UseRetriableExecutionStrategy
         {
             get
             {
-                return (bool?)CallContext.LogicalGetData("SuspendExecutionStrategy") ?? false;
+                return (bool?)CallContext.LogicalGetData(nameof(UseRetriableExecutionStrategy)) ?? true;
             }
             set
             {
-                CallContext.LogicalSetData("SuspendExecutionStrategy", value);
+                CallContext.LogicalSetData(nameof(UseRetriableExecutionStrategy), value);
+            }
+        }
+
+        public static IDisposable SuspendRetriableExecutionStrategy()
+        {
+            return new RetriableExecutionStrategySuspension();
+        }
+
+        private class RetriableExecutionStrategySuspension : IDisposable
+        {
+            private readonly bool _originalValue;
+
+            internal RetriableExecutionStrategySuspension()
+            {
+                _originalValue = UseRetriableExecutionStrategy;
+
+                UseRetriableExecutionStrategy = false;
+            }
+
+            public void Dispose()
+            {
+                UseRetriableExecutionStrategy = _originalValue;
             }
         }
     }

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Data.Entity;
+using System.Data.Entity.Infrastructure;
 using System.Threading.Tasks;
 
 namespace NuGetGallery
@@ -68,6 +69,11 @@ public void SetCommandTimeout(int? seconds)
             ObjectContext.CommandTimeout = seconds;
         }
 
+        public DbChangeTracker GetChangeTracker()
+        {
+            return ChangeTracker;
+        }
+
         public Database GetDatabase()
         {
             return Database;

src/NuGetGallery.Core/Entities/IEntitiesContext.cs

@@ -1,12 +1,15 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Data.Entity;
+using System.Data.Entity.Infrastructure;
 using System.Threading.Tasks;
 
 namespace NuGetGallery
 {
     public interface IEntitiesContext
+        : IDisposable
     {
         IDbSet<CuratedFeed> CuratedFeeds { get; set; }
         IDbSet<CuratedPackage> CuratedPackages { get; set; }
@@ -20,6 +23,8 @@ public interface IEntitiesContext
         IDbSet<T> Set<T>() where T : class;
         void DeleteOnCommit<T>(T entity) where T : class;
         void SetCommandTimeout(int? seconds);
+
+        DbChangeTracker GetChangeTracker();
         Database GetDatabase();
     }
 }

src/NuGetGallery/Controllers/ApiController.cs

@@ -393,6 +393,9 @@ await AuditingService.SaveAuditRecordAsync(
                             throw;
                         }
 
+                        // Handle in separate transaction because of concurrency check with retry
+                        await PackageService.UpdateIsLatestAsync(package.PackageRegistration);
+
                         IndexingService.UpdatePackage(package);
 
                         // Write an audit record
@@ -470,6 +473,13 @@ public virtual async Task<ActionResult> DeletePackage(string id, string version)
             }
 
             await PackageService.MarkPackageUnlistedAsync(package);
+
+            // Handle in separate transaction because of concurrency check with retry. Due to using
+            // separate transactions, we must always call UpdateIsLatest on delete/unlist. This is
+            // because a concurrent thread could be marking the package as latest before this thread
+            // is able to commit the delete /unlist.
+            await PackageService.UpdateIsLatestAsync(package.PackageRegistration);
+
             IndexingService.UpdatePackage(package);
             return new EmptyResult();
         }
@@ -503,6 +513,10 @@ public virtual async Task<ActionResult> PublishPackage(string id, string version
             }
 
             await PackageService.MarkPackageListedAsync(package);
+
+            // handle in separate transaction because of concurrency check with retry
+            await PackageService.UpdateIsLatestAsync(package.PackageRegistration);
+
             IndexingService.UpdatePackage(package);
             return new EmptyResult();
         }

src/NuGetGallery/Controllers/PackagesController.cs

@@ -996,11 +996,20 @@ internal virtual async Task<ActionResult> Edit(string id, string version, bool?
             {
                 action = "unlisted";
                 await _packageService.MarkPackageUnlistedAsync(package);
+
+                // Handle in separate transaction because of concurrency check with retry. Due to using
+                // separate transactions, we must always call UpdateIsLatest on delete/unlist. This is
+                // because a concurrent thread could be marking the package as latest before this thread
+                // is able to commit the delete /unlist.
+                await _packageService.UpdateIsLatestAsync(package.PackageRegistration);
             }
             else
             {
                 action = "listed";
                 await _packageService.MarkPackageListedAsync(package);
+
+                // handle in separate transaction because of concurrency check with retry
+                await _packageService.UpdateIsLatestAsync(package.PackageRegistration);
             }
             TempData["Message"] = String.Format(
                 CultureInfo.CurrentCulture,
@@ -1192,6 +1201,9 @@ public virtual async Task<ActionResult> VerifyPackage(VerifyPackageRequest formD
                     throw;
                 }
 
+                // handle in separate transaction because of concurrency check with retry
+                await _packageService.UpdateIsLatestAsync(package.PackageRegistration);
+
                 // tell Lucene to update index for the new package
                 _indexingService.UpdateIndex();
 

src/NuGetGallery/Services/IPackageService.cs

@@ -16,13 +16,23 @@ public interface IPackageService
         IEnumerable<PackageRegistration> FindPackageRegistrationsByOwner(User user);
         IEnumerable<Package> FindDependentPackages(Package package);
 
-        Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true);
+        /// <summary>
+        /// Updates IsLatest/IsLatestStable flags after a package create, update or delete operation.
+        /// 
+        /// Due to the manual optimistic concurrency check added to the underlying implementation,
+        /// IsLatest/IsLatestStable changes will be applied in memory and should not be committed with EF.
+        /// Callers should ensure that all other commits are complete before calling UpdateIsLatestAsync.
+        /// </summary>
+        /// <param name="packageRegistration"></param>
+        /// <returns></returns>
+        Task UpdateIsLatestAsync(PackageRegistration packageRegistration);
 
         /// <summary>
         /// Populate the related database tables to create the specified package for the specified user.
         /// </summary>
         /// <remarks>
         /// This method doesn't upload the package binary to the blob storage. The caller must do it after this call.
+        /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
         /// </remarks>
         /// <param name="nugetPackage">The package to be created.</param>
         /// <param name="packageStreamMetadata">The package stream's metadata.</param>
@@ -33,10 +43,49 @@ public interface IPackageService
 
         Package EnrichPackageFromNuGetPackage(Package package, PackageArchiveReader packageArchive, PackageMetadata packageMetadata, PackageStreamMetadata packageStreamMetadata, User user);
 
+        /// <summary>
+        /// Publishes a package by listing it.
+        /// </summary>
+        /// <remarks>
+        /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
+        /// </remarks>
+        /// <param name="id">ID for the package to publish</param>
+        /// <param name="version">Package version</param>
+        /// <param name="commitChanges">Whether to commit changes to the database.</param>
+        /// <returns></returns>
         Task PublishPackageAsync(string id, string version, bool commitChanges = true);
+
+        /// <summary>
+        /// Publishes a package by listing it.
+        /// </summary>
+        /// <remarks>
+        /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
+        /// </remarks>
+        /// <param name="package">The package to publish</param>
+        /// <param name="commitChanges">Whether to commit changes to the database.</param>
+        /// <returns></returns>
         Task PublishPackageAsync(Package package, bool commitChanges = true);
 
+        /// <summary>
+        /// Mark a package as unlisted.
+        /// </summary>
+        /// <remarks>
+        /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
+        /// </remarks>
+        /// <param name="package">The package to unlist</param>
+        /// <param name="commitChanges">Whether to commit changes to the database.</param>
+        /// <returns></returns>
         Task MarkPackageUnlistedAsync(Package package, bool commitChanges = true);
+
+        /// <summary>
+        /// Mark a package as listed.
+        /// </summary>
+        /// <remarks>
+        /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
+        /// </remarks>
+        /// <param name="package">The package to list.</param>
+        /// <param name="commitChanges">Whether to commit changes to the database.</param>
+        /// <returns></returns>
         Task MarkPackageListedAsync(Package package, bool commitChanges = true);
 
         Task<PackageOwnerRequest> CreatePackageOwnerRequestAsync(PackageRegistration package, User currentOwner, User newOwner);