diff --git a/.github/workflows/dotnet.yml b/.github/workflows/dotnet.yml index 70304601..bbe354dc 100644 --- a/.github/workflows/dotnet.yml +++ b/.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 \ No newline at end of file diff --git a/Sunrise.Processing.Tests/Scores/Processors/UserGradesScoreProcessorTests.cs b/Sunrise.Processing.Tests/Scores/Processors/UserGradesScoreProcessorTests.cs index a97cc7f0..1f82b8eb 100644 --- a/Sunrise.Processing.Tests/Scores/Processors/UserGradesScoreProcessorTests.cs +++ b/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() { diff --git a/Sunrise.Processing/Scores/Pipeline/ScoreCommitPipeline.cs b/Sunrise.Processing/Scores/Pipeline/ScoreCommitPipeline.cs index 489cdd71..06572843 100644 --- a/Sunrise.Processing/Scores/Pipeline/ScoreCommitPipeline.cs +++ b/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); diff --git a/Sunrise.Processing/Scores/Processors/UserGradesScoreProcessor.cs b/Sunrise.Processing/Scores/Processors/UserGradesScoreProcessor.cs index 93c147e4..1238572a 100644 --- a/Sunrise.Processing/Scores/Processors/UserGradesScoreProcessor.cs +++ b/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; diff --git a/Sunrise.Processing/Scores/Processors/UserStatsScoreProcessor.cs b/Sunrise.Processing/Scores/Processors/UserStatsScoreProcessor.cs index f20d35cd..0d283942 100644 --- a/Sunrise.Processing/Scores/Processors/UserStatsScoreProcessor.cs +++ b/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); diff --git a/Sunrise.Shared/Database/DatabaseService.cs b/Sunrise.Shared/Database/DatabaseService.cs index 1294179c..c3d92b39 100644 --- a/Sunrise.Shared/Database/DatabaseService.cs +++ b/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> _afterCommitActions = []; + + public void RegisterAfterCommitAction(Func 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,6 +118,9 @@ 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) @@ -116,6 +128,9 @@ void OnSavingChanges(object? sender, SavingChangesEventArgs e) 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"); + } + } + } } diff --git a/Sunrise.Shared/Database/Interceptor/SlowQueryLoggerInterceptor.cs b/Sunrise.Shared/Database/Interceptor/SlowQueryLoggerInterceptor.cs index 9757feaa..5f4d46f5 100644 --- a/Sunrise.Shared/Database/Interceptor/SlowQueryLoggerInterceptor.cs +++ b/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 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 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); + } } \ No newline at end of file diff --git a/Sunrise.Shared/Database/Services/Users/UserGradesService.cs b/Sunrise.Shared/Database/Services/Users/UserGradesService.cs index 4b86936f..97550ad0 100644 --- a/Sunrise.Shared/Database/Services/Users/UserGradesService.cs +++ b/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 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 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 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; + } } \ No newline at end of file diff --git a/Sunrise.Shared/Database/Services/Users/UserStatsService.cs b/Sunrise.Shared/Database/Services/Users/UserStatsService.cs index d6ef7bc1..80f7abcc 100644 --- a/Sunrise.Shared/Database/Services/Users/UserStatsService.cs +++ b/Sunrise.Shared/Database/Services/Users/UserStatsService.cs @@ -1,4 +1,5 @@ using CSharpFunctionalExtensions; +using EntityFrameworkCore.Locking; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; using Sunrise.Shared.Database.Extensions; @@ -35,16 +36,29 @@ public async Task AddUserStats(UserStats stats, User user) }); } - public async Task UpdateUserStats(UserStats stats, User user) + public async Task UpdateUserStats(UserStats stats, User user, CancellationToken ct = default) { return await ResultUtil.TryExecuteAsync(async () => { - var addOrUpdateUserRanksResult = await Ranks.AddOrUpdateUserRanks(stats, user); - if (addOrUpdateUserRanksResult.IsFailure) - throw new ApplicationException(addOrUpdateUserRanksResult.Error); - dbContext.UpdateEntity(stats); - await dbContext.SaveChangesAsync(); + + if (dbContext.Database.CurrentTransaction != null) + { + databaseService.Value.RegisterAfterCommitAction(async () => + { + var addOrUpdateUserRanksResult = await Ranks.AddOrUpdateUserRanks(stats, user); + if (addOrUpdateUserRanksResult.IsFailure) + _logger.LogWarning("Failed to update user ranks after stats update: {Error}", addOrUpdateUserRanksResult.Error); + }); + + return; + } + + await dbContext.SaveChangesAsync(ct); + + var updateRanksResult = await Ranks.AddOrUpdateUserRanks(stats, user); + if (updateRanksResult.IsFailure) + throw new ApplicationException(updateRanksResult.Error); }); } @@ -71,6 +85,22 @@ public async Task UpdateUserStats(UserStats stats, User user) return stats; } + public async Task LockAndRefreshUserStats(UserStats stats, CancellationToken ct = default) + { + var lockedStats = await dbContext.UserStats + .AsNoTracking() + .Where(us => stats.Id != 0 + ? us.Id == stats.Id + : us.UserId == stats.UserId && us.GameMode == stats.GameMode) + .ForUpdate() + .SingleOrDefaultAsync(ct); + + if (lockedStats == null) + return; + + CopyUserStatsValues(lockedStats, stats); + } + public async Task> GetUsersStats(GameMode mode, LeaderboardSortType leaderboardSortType, List? userIds = null, QueryOptions? options = null, bool addMissingUserStats = true, CancellationToken ct = default) { var statsQuery = dbContext.UserStats.Where(e => e.GameMode == mode); @@ -128,4 +158,26 @@ await AddUserStats(new UserStats return stats; } + + private static void CopyUserStatsValues(UserStats source, UserStats target) + { + var rank = target.LocalProperties.Rank; + + target.Id = source.Id; + target.UserId = source.UserId; + target.GameMode = source.GameMode; + target.Accuracy = source.Accuracy; + target.TotalScore = source.TotalScore; + target.RankedScore = source.RankedScore; + target.PlayCount = source.PlayCount; + target.PerformancePoints = source.PerformancePoints; + target.MaxCombo = source.MaxCombo; + target.PlayTime = source.PlayTime; + target.TotalHits = source.TotalHits; + target.BestGlobalRank = source.BestGlobalRank; + target.BestGlobalRankDate = source.BestGlobalRankDate; + target.BestCountryRank = source.BestCountryRank; + target.BestCountryRankDate = source.BestCountryRankDate; + target.LocalProperties.Rank = rank; + } } \ No newline at end of file diff --git a/Sunrise.Tests/IntegrationDatabaseFixture.cs b/Sunrise.Tests/IntegrationDatabaseFixture.cs index 26b3973f..7ce49e46 100644 --- a/Sunrise.Tests/IntegrationDatabaseFixture.cs +++ b/Sunrise.Tests/IntegrationDatabaseFixture.cs @@ -51,6 +51,8 @@ await Task.WhenAll( public async Task ResetAsync() { using var scope = App.Server.Services.CreateScope(); + + App.MockHttpClient?.ClearMocks(); await ClearSingletonState(scope);