Lock user stats/grades for update

.github/workflows/dotnet.yml

@@ -5,38 +5,8 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  build:
-    name: Build
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v4
-      - name: Setup .NET
-        uses: actions/setup-dotnet@v4
-        with:
-          dotnet-version: 8.0.x
-      - name: Cache NuGet packages
-        uses: actions/cache@v4
-        with:
-          path: ~/.nuget/packages
-          key: ${{ runner.os }}-nuget-${{ hashFiles('**/*.csproj') }}
-          restore-keys: |
-            ${{ runner.os }}-nuget-
-      - name: Restore dependencies
-        run: dotnet restore
-      - name: Build
-        run: dotnet build --no-restore
-      - name: Upload build artifacts
-        uses: actions/upload-artifact@v4
-        with:
-          name: build-artifacts
-          path: |
-            **/bin/
-            **/obj/
-          retention-days: 1
-
   test:
     name: Test Group ${{ matrix.group }}
-    needs: build
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
@@ -67,12 +37,12 @@ jobs:
         uses: actions/setup-dotnet@v4
         with:
           dotnet-version: 8.0.x
-      - name: Download build artifacts
-        uses: actions/download-artifact@v4
-        with:
-          name: build-artifacts
       - name: Docker compose test environment
-        run: docker compose -f docker-compose.tests.yml up -d --wait
+        run: docker compose -f docker-compose.tests.yml up -d
+      - name: Restore dependencies
+        run: dotnet restore
+      - name: Build
+        run: dotnet build --no-restore
       - name: Run tests (Group ${{ matrix.group }}/16)
         env:
           TEST_GROUP: ${{ matrix.group }}
@@ -128,4 +98,4 @@ jobs:
             FAILED=1
           fi
 
-          exit $FAILED
+          exit $FAILED

Sunrise.Processing.Tests/Scores/Processors/UserGradesScoreProcessorTests.cs

@@ -229,6 +229,41 @@ public async Task TestOnDeletionWithPromotedReplacementReplacesGradeCounts()
         Assert.Equal(1, userGrades.CountS);
     }
 
+    [Fact]
+    public async Task TestOnDeletionWithPromotedReplacementWithSameGradeNoChangesRequired()
+    {
+        // Arrange
+        var processor = new UserGradesScoreProcessor(Database);
+        var user = await CreateTestUser();
+        var userStats = await Database.Users.Stats.GetUserStats(user.Id, GameMode.Standard);
+        Assert.NotNull(userStats);
+        var userGrades = new UserGrades
+        {
+            UserId = user.Id,
+            GameMode = GameMode.Standard,
+            CountA = 1
+        };
+
+        var promotedReplacement = CreateScore(user, submissionStatus: SubmissionStatus.Best);
+        var score = CreateScore(user);
+        var originalState = ScoreStateSnapshot.Capture(score);
+
+        var context = ScoreCommitContextFactory.Create(
+            ScoreTaskType.Delete,
+            score,
+            user,
+            userStats,
+            userGrades,
+            userPersonalBestScores: new UserBeatmapPeers(new UserPersonalBestScores(promotedReplacement), new UserPersonalBestScores(promotedReplacement)),
+            originalState: originalState);
+
+        // Act
+        await processor.OnDeletion(context);
+
+        // Assert
+        Assert.Equal(1, userGrades.CountA);
+    }
+
     [Fact]
     public async Task TestOnDeletionWithNonBestOriginalStateKeepsGradesUnchanged()
     {

Sunrise.Processing/Scores/Pipeline/ScoreCommitPipeline.cs

@@ -56,6 +56,9 @@ private async Task ExecuteCommitAsync(
 
         ctx.UserPersonalBestScores = peers;
 
+        await _database.Users.Stats.LockAndRefreshUserStats(ctx.UserStats, ct);
+        await _database.Users.Grades.LockAndRefreshUserGrades(ctx.UserGrades, ct);
+
         foreach (var processor in _processors)
         {
             await DispatchProcessor(processor, ctx);

Sunrise.Processing/Scores/Processors/UserGradesScoreProcessor.cs

@@ -39,6 +39,8 @@ protected override Task OnRestorationInternal(ScoreCommitContext ctx)
 
     protected override async Task AfterExecution(ScoreCommitContext ctx)
     {
+        // NOTE: Ideally we should have atomic update here, but we have an assumption that pp calculation and beatmap retrieval would
+        // be the heaviest operations. Thus, just relying on lock FOR UPDATES is enough in this context.
         var updateUserGradesResult = await database.Users.Grades.UpdateUserGrades(ctx.UserGrades);
         if (updateUserGradesResult.IsFailure)
             throw new ApplicationException("Failed to persist user grades: " + updateUserGradesResult.Error);
@@ -57,6 +59,10 @@ private static void IncrementWithScore(ScoreCommitContext ctx)
         if (!IsOverallBestScore(score, previousOverallBest))
             return;
 
+        // If the grade hasn't changed, we can return early and skip any DB calls.
+        if (previousOverallBest?.Grade == score.Grade)
+            return;
+
         if (previousOverallBest != null)
             UpdateUserGradesCount(userGrades, previousOverallBest.Grade, -1);
 
@@ -77,10 +83,14 @@ private static void DecrementWithScore(ScoreCommitContext ctx)
         if (!IsOverallBestScore(score, promotedOverallBest))
             return;
 
-        UpdateUserGradesCount(userGrades, score.Grade, -1);
+        // If the grade hasn't changed, we can return early and skip any DB calls.
+        if (promotedOverallBest?.Grade == score.Grade)
+            return;
 
         if (promotedOverallBest != null)
             UpdateUserGradesCount(userGrades, promotedOverallBest.Grade, 1);
+
+        UpdateUserGradesCount(userGrades, score.Grade, -1);
     }
 
     private static bool IsOverallBestScore(Score score, Score? peer)
@@ -99,6 +109,9 @@ private static bool IsOverallBestScore(Score score, Score? peer)
 
     private static void UpdateUserGradesCount(UserGrades userGrades, string grade, int delta)
     {
+        if (delta is > 1 or < -1)
+            throw new ArgumentOutOfRangeException(nameof(delta));
+
         switch (grade)
         {
             case "XH": userGrades.CountXH = Math.Max(0, userGrades.CountXH + delta); break;

Sunrise.Processing/Scores/Processors/UserStatsScoreProcessor.cs

@@ -42,6 +42,8 @@ protected override async Task OnRestorationInternal(ScoreCommitContext ctx)
 
     protected override async Task AfterExecution(ScoreCommitContext ctx)
     {
+        // NOTE: Ideally we should have atomic update here, but we have an assumption that pp calculation and beatmap retrieval would
+        // be the heaviest operations. Thus, just relying on lock FOR UPDATES is enough in this context.
         var updateUserStatsResult = await database.Users.Stats.UpdateUserStats(ctx.UserStats, ctx.User);
         if (updateUserStatsResult.IsFailure)
             throw new ApplicationException("Failed to persist user stats: " + updateUserStatsResult.Error);

Sunrise.Shared/Database/DatabaseService.cs

@@ -35,6 +35,12 @@ public sealed class DatabaseService(
     public readonly ScoreSubmissionRequestRepository ScoreSubmissionRequests = scoreSubmissionRequestRepository;
     public readonly ScoreProcessingTaskRepository ScoreProcessingTasks = scoreProcessingTaskRepository;
     public readonly UserRepository Users = userRepository;
+    private readonly List<Func<Task>> _afterCommitActions = [];
+
+    public void RegisterAfterCommitAction(Func<Task> action)
+    {
+        _afterCommitActions.Add(action);
+    }
 
     public async Task FlushAndUpdateRedisCache(bool isSoftFlush = true)
     {
@@ -100,7 +106,10 @@ void OnSavingChanges(object? sender, SavingChangesEventArgs e)
             await DbContext.SaveChangesAsync();
 
             if (!isCurrentlyInOtherTransactionScope && transaction != null)
+            {
                 await transaction.CommitAsync(ct);
+                await RunAfterCommitActions();
+            }
 
             return Result.Success();
         }
@@ -109,13 +118,19 @@ void OnSavingChanges(object? sender, SavingChangesEventArgs e)
             if (!isCurrentlyInOtherTransactionScope && transaction != null)
                 await transaction.RollbackAsync(ct);
 
+            if (!isCurrentlyInOtherTransactionScope)
+                _afterCommitActions.Clear();
+
             return Result.Failure($"{ex.Message}\n{ex.InnerException?.Message}");
         }
         catch (Exception ex)
         {
             if (!isCurrentlyInOtherTransactionScope && transaction != null)
                 await transaction.RollbackAsync(ct);
 
+            if (!isCurrentlyInOtherTransactionScope)
+                _afterCommitActions.Clear();
+
             logger.LogWarning(ex, "Failed to process db transaction");
 
             return Result.Failure($"{ex.Message}\n{ex.InnerException?.Message}");
@@ -150,4 +165,22 @@ void OnSavingChanges(object? sender, SavingChangesEventArgs e)
             }
         }
     }
+
+    private async Task RunAfterCommitActions()
+    {
+        var actions = _afterCommitActions.ToArray();
+        _afterCommitActions.Clear();
+
+        foreach (var action in actions)
+        {
+            try
+            {
+                await action();
+            }
+            catch (Exception ex)
+            {
+                logger.LogWarning(ex, "Failed to run after-commit database action");
+            }
+        }
+    }
 }

Sunrise.Shared/Database/Interceptor/SlowQueryLoggerInterceptor.cs

@@ -28,4 +28,59 @@ public override DbDataReader ReaderExecuted(DbCommand command, CommandExecutedEv
 
         return base.ReaderExecuted(command, eventData, result);
     }
+
+
+    public override int NonQueryExecuted(
+        DbCommand command,
+        CommandExecutedEventData eventData,
+        int result)
+    {
+        if (eventData.Duration.TotalMilliseconds > SlowQueryThresholdMilliseconds)
+        {
+            Log.Warning("Slow Query Detected. ({Milliseconds}ms): {QueryString}", eventData.Duration.TotalMilliseconds, command.CommandText);
+        }
+
+        return base.NonQueryExecuted(command, eventData, result);
+    }
+
+    public override async ValueTask<int> NonQueryExecutedAsync(
+        DbCommand command,
+        CommandExecutedEventData eventData,
+        int result,
+        CancellationToken cancellationToken = default)
+    {
+        if (eventData.Duration.TotalMilliseconds > SlowQueryThresholdMilliseconds)
+        {
+            Log.Warning("Slow Query Detected. ({Milliseconds}ms): {QueryString}", eventData.Duration.TotalMilliseconds, command.CommandText);
+        }
+
+        return await base.NonQueryExecutedAsync(command, eventData, result, cancellationToken);
+    }
+
+    public override object? ScalarExecuted(
+        DbCommand command,
+        CommandExecutedEventData eventData,
+        object? result)
+    {
+        if (eventData.Duration.TotalMilliseconds > SlowQueryThresholdMilliseconds)
+        {
+            Log.Warning("Slow Query Detected. ({Milliseconds}ms): {QueryString}", eventData.Duration.TotalMilliseconds, command.CommandText);
+        }
+
+        return base.ScalarExecuted(command, eventData, result);
+    }
+
+    public override async ValueTask<object?> ScalarExecutedAsync(
+        DbCommand command,
+        CommandExecutedEventData eventData,
+        object? result,
+        CancellationToken cancellationToken = default)
+    {
+        if (eventData.Duration.TotalMilliseconds > SlowQueryThresholdMilliseconds)
+        {
+            Log.Warning("Slow Query Detected. ({Milliseconds}ms): {QueryString}", eventData.Duration.TotalMilliseconds, command.CommandText);
+        }
+
+        return await base.ScalarExecutedAsync(command, eventData, result, cancellationToken);
+    }
 }

Sunrise.Shared/Database/Services/Users/UserGradesService.cs

@@ -1,4 +1,5 @@
 using CSharpFunctionalExtensions;
+using EntityFrameworkCore.Locking;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
 using Sunrise.Shared.Database.Extensions;
@@ -33,6 +34,22 @@ public async Task<Result> UpdateUserGrades(UserGrades userGrades)
         });
     }
 
+    public async Task LockAndRefreshUserGrades(UserGrades userGrades, CancellationToken ct = default)
+    {
+        var lockedUserGrades = await dbContext.UserGrades
+            .AsNoTracking()
+            .Where(ug => userGrades.Id != 0
+                ? ug.Id == userGrades.Id
+                : ug.UserId == userGrades.UserId && ug.GameMode == userGrades.GameMode)
+            .ForUpdate()
+            .SingleOrDefaultAsync(ct);
+
+        if (lockedUserGrades == null)
+            return;
+
+        CopyUserGradesValues(lockedUserGrades, userGrades);
+    }
+
     public async Task<UserGrades?> GetUserGrades(int userId, GameMode mode, CancellationToken ct = default)
     {
         var grades = await dbContext.UserGrades.Where(e => e.UserId == userId && e.GameMode == mode).FirstOrDefaultAsync(cancellationToken: ct);
@@ -55,4 +72,17 @@ public async Task<Result> UpdateUserGrades(UserGrades userGrades)
 
         return grades;
     }
+
+    private static void CopyUserGradesValues(UserGrades source, UserGrades target)
+    {
+        target.Id = source.Id;
+        target.CountXH = source.CountXH;
+        target.CountX = source.CountX;
+        target.CountSH = source.CountSH;
+        target.CountS = source.CountS;
+        target.CountA = source.CountA;
+        target.CountB = source.CountB;
+        target.CountC = source.CountC;
+        target.CountD = source.CountD;
+    }
 }