From e4cadfe2b7bb96c6258e66dcca36d6e1d553f3c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:38:11 +0000 Subject: [PATCH 1/2] Write a live goal and the score it makes in one save, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logging a goal at the touchline inserted the row through GameService and then recounted the scoreline through MatchGoalService's own context. Two contexts are two connections, so those were two transactions with a gap between them — and a SQLite lock timeout, a failure on the second save, or Fly.io restarting the container mid-deploy all land in it, leaving the goal on file behind a stale score. Removing a goal had the same shape. GameService now does both halves itself: AddGoalAsync and RemoveGoalAsync take recountScoreline, count the goals through the context they are about to save, and write the row and the scoreline together. MatchGoalService keeps the one thing only a live match knows — the minute the clock showed — and delegates the rest, so goal storage still has a single implementation and no context is passed between services. recountScoreline defaults to false for the result page, where the score is typed by hand and the goal list is allowed to be shorter than it. Game.CountScoreFrom is the recount both this and the final whistle now share, and it stays a recount rather than an increment: a score derived afresh from the goals repairs itself. Two related decisions are settled in writing rather than in code. Creating a game may still save a season before the game, in SeasonService's context — the leftover is an empty season, which is a valid gapless window that the next game on that date resolves to and reuses, and a test pins that reuse. And the seeding insert in MatchPreferencesService.GetAsync, the one read in the app that writes, now saves with CancellationToken.None, so navigating away cannot cancel a write half of the app treats as having happened. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013ys63zAujLcoaS7UP9F2wm --- docs/architecture.md | 5 +- docs/known_issues.md | 12 +++ docs/models.md | 4 + docs/patterns.md | 62 ++++++++++++++- src/FootballFormation.Core/Models/Game.cs | 12 +++ .../Services/GameService.cs | 43 +++++++++- .../Services/MatchClockService.cs | 3 +- .../Services/MatchGoalService.cs | 32 ++------ .../Services/MatchPreferencesService.cs | 8 +- .../GameServiceTests.cs | 39 ++++++++++ .../MatchGoalServiceTests.cs | 78 +++++++++++++++++++ 11 files changed, 265 insertions(+), 33 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index ec4f68a..192ca93 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -77,8 +77,9 @@ Services/ rolling into the next one, the final whistle. The arithmetic a season's statistics are built from. There is no pause — the clock runs from kick-off to the whistle and only a period boundary stops it - MatchGoalService.cs — Goals logged live: storage delegated to GameService, the live minute - and the recomputed scoreline added here + MatchGoalService.cs — Goals logged live: the live minute added here, storage and the + recounted scoreline delegated to GameService, which writes the two in + one save (see patterns.md, "When two rows have to agree") MatchSubstitutionService.cs — The slot swap and the record of it, in one SaveChanges, plus undoing the most recent one of a period, plus SwapPositionsAsync — two players already on trading slots, which writes no substitution row (so the undo diff --git a/docs/known_issues.md b/docs/known_issues.md index f36adde..f650d9f 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -39,6 +39,18 @@ Avoid repeating these mistakes: of the moment the copy was taken; it is the schema state now, so a crash loop cannot write five snapshots of the broken database and prune the only good one. See [deployment.md](deployment.md).) - **The scaffolder ordered a destructive migration wrongly**: `AddSeasonSquads` had to copy `Players.IsGuest` into a new table *and* drop the column; EF emitted the `DropColumn` first, which would have wiped the source before the backfill ran. Always read and reorder the generated `Up()`. +- **A transaction cannot span two `AppDbContext` instances, and nothing warns you**: each operation + opens its own context from the factory (deliberately — see [patterns.md](patterns.md)), and each + context has its own connection. Calling another *service's* write from inside your own therefore + gives you two transactions with a gap between them, even though the code reads like one operation + and every `Result` check passes. The gap is real: SQLite can time out on the lock, the second save + can throw, and a Fly.io deploy restarts the container mid-write. Logging a goal was shaped that + way — insert through `GameService`, recount the scoreline through `MatchGoalService`'s own + context — so an interruption between them left the goal on file behind a stale score. **When one + row is derived from another, load and write both through the same context and one `SaveChanges`**; + `GameService.AddGoalAsync(goal, recountScoreline: true)` is what that looks like. The saves are + counted in `MatchGoalServiceTests`, because from the outside two saves and one look identical + right up until something interrupts them. ## Data / domain - **Deleting a player used to be destructive across every season**: `PlayerService.DeleteAsync` cascades their `GamePlayerPosition` rows and nulls their `GameGoal` scorer, so last season's top scorer disappeared from last season's stats — from a confirm that said nothing about it. Fixed by `ArchivePlayersInsteadOfDeleting`: delete now **refuses** for anyone with a lineup or goal row anywhere, and `Player.IsArchived` is the way to retire someone. Worth knowing when the refusal surprises you: the counts are deliberately **not** scoped to a season, unlike `SeasonSquadService.RemoveMemberAsync`'s, because the cascade is not either. diff --git a/docs/models.md b/docs/models.md index 5cff422..23a0413 100644 --- a/docs/models.md +++ b/docs/models.md @@ -134,6 +134,10 @@ and a page refresh or a second device picks it up exactly where it is. `Game.CountOurGoals(goals)` / `Game.CountTheirGoals(goals)` are the one place the scoreline rule lives: an own goal counts for the opponent, so it is excluded from ours and included in theirs. +`CountScoreFrom(goals)` applies both to a game at once, and it is a **recount** rather than an +increment on purpose — a score derived afresh from the goals repairs itself, which is what lets the +final whistle and the next goal logged both settle a scoreline that drifted. See +[patterns.md](patterns.md). **`Game.IsComplete` decides whether a game counts towards statistics at all**: the final whistle went on the live screen, or the game was never run live and has a final score on file. A match in diff --git a/docs/patterns.md b/docs/patterns.md index bfcc591..fbfe519 100644 --- a/docs/patterns.md +++ b/docs/patterns.md @@ -90,6 +90,66 @@ something*. shared by the layout's picker and whichever page is open, so cancelling it on behalf of one page would take the app bar down with it. +**`MatchPreferencesService.GetAsync` is the one read that writes**, and the one place the rule +needed spelling out inside a method rather than at the call site. It seeds a preferences row for a +season on first read, and both `/settings` and the game dialog hand it a page-lifetime token. The +lookups above the seeding take that token and give up having written nothing, which is right; the +`SaveChangesAsync` that inserts the row takes `CancellationToken.None`, because by then it is a +write and a write finishes. `SeasonService.CloseSeasonGapsAsync` and `EnsureCurrentSeasonAsync` are +the same shape — a read that repairs — but only ever run from startup with no token, so nothing +there has to decide. + +## When two rows have to agree, one context writes both +One `SaveChangesAsync` is a transaction, so almost every mutating method here is atomic without +saying anything: it opens a context, changes what it changes, and saves once. +`GameService.SavePeriodLineupAsync` is the one that needs more and says so — delete-then-insert is +two saves, wrapped in an explicit `BeginTransactionAsync`. + +**What no transaction can cover is two `AppDbContext` instances.** Each operation opens its own from +the factory, for the circuit reason above, and each context has its own connection. So a service +method that calls *another service's* write is two transactions with a gap between them, and a +SQLite lock timeout, a failure on the second save, or Fly.io restarting the container mid-deploy all +land in that gap. That is not hypothetical: the app migrates itself on boot, so a deploy is a +restart. + +The rule that follows: **when one row is derived from another, write them through one context and +one save.** The live scoreline is the worked example. Logging a goal used to insert the row through +`GameService` and then recount the score through `MatchGoalService`'s own context — two saves, and an +interruption between them left the goal on file behind a stale scoreline. It is one save now: +`GameService.AddGoalAsync(goal, recountScoreline: true)` counts the goals already on file, adds the +new one in memory, sets the scoreline, and saves the insert and the update together. +`RemoveGoalAsync` mirrors it. `MatchGoalServiceTests` asserts the count of saves, because "one +write" is the property and it is invisible from the outside. + +Two ways of getting there were considered and rejected, and both are worth not re-proposing: +passing a context or a transaction from one service into another (which breaks the short-lived +context rule that exists for the circuit), and letting `MatchGoalService` store goals itself (a +second implementation of goal storage, which delegating to `GameService` exists to prevent). + +`recountScoreline` defaults to false, and that is the result page: there an admin types the score +and records the goals whose scorer somebody remembered, so the list is allowed to be shorter than +the scoreline and recounting would rewrite a 3-1 as 1-0. Both behaviours are pinned by a test. + +**Recount, never increment.** `Game.CountScoreFrom(goals)` rewrites the scoreline from the goals +rather than nudging it, so a score that did drift is repaired by the next goal logged and by +`MatchClockService.FinishMatchAsync`, which recounts the same way at the final whistle. A derived +value that is recomputed heals; one that is incremented accumulates. + +### The one multi-save write left, on purpose +`GameService.CreateAsync` resolves `SeasonId 0` through `SeasonService.GetOrCreateForDateAsync`, +which may create and save a season in its own context before the game is saved in this one. Stopping +between the two leaves an **empty season** — and that is allowed to stand rather than being made +atomic, because an empty season is a valid gapless window: the next game scheduled on that date +resolves to it and reuses it, so the leftover costs nothing and disappears on its own. +`GameServiceTests` pins that reuse, so the reasoning holds rather than merely being believed. Making +it atomic would need one of the two moves rejected above, which is a poor trade for a leftover with +no consequence. + +**Not in scope, deliberately:** none of this gives writes the page-lifetime token from +`CancellableComponent`. Atomicity says all-or-nothing; it does not say which one is wanted, and for +a write an admin explicitly asked for the answer is *all*. A dropped circuit is not someone changing +their mind. + ## Logging - **Framework**: Microsoft.Extensions.Logging via Serilog - **Sink**: Console + rolling file at `%LOCALAPPDATA%\FootballFormation\logs\` @@ -109,7 +169,7 @@ is actually happening at the touchline rather than along a data-access seam: | --- | --- | | `LiveMatchService` | Reading: `GetLiveAsync` for the live screen, `GetTodaysMatchAsync` for the home banner. Both public, like every other read | | `MatchClockService` | Kick-off, ending a period, starting or rolling into the next, the final whistle — and `BankClock`, the only thing that moves seconds about. No pause: only a period boundary stops the clock | -| `MatchGoalService` | The live minute a goal is stamped with and the scoreline recomputed from the goals on file. Storage itself still delegates to `GameService` | +| `MatchGoalService` | The live minute a goal is stamped with. Storing the goal, and recounting the scoreline in the same save, still delegates to `GameService` | | `MatchSubstitutionService` | The slot swap and the record of it, in one `SaveChanges`, and undoing the most recent one of a period | What made the cut worth making was not the line count: the clock arithmetic and the substitution diff --git a/src/FootballFormation.Core/Models/Game.cs b/src/FootballFormation.Core/Models/Game.cs index 7393274..02b9aca 100644 --- a/src/FootballFormation.Core/Models/Game.cs +++ b/src/FootballFormation.Core/Models/Game.cs @@ -186,6 +186,18 @@ public static int CountOurGoals(IEnumerable goals) => /// Their goals: everything the opponent scored, plus our own goals. public static int CountTheirGoals(IEnumerable goals) => goals.Count(g => !g.CountsForUs); + + /// + /// Rewrites the scoreline from , so a live score is recounted rather + /// than incremented — the recount is what makes it self-correcting. + /// + public void CountScoreFrom(IEnumerable goals) + { + var counted = goals as IReadOnlyCollection ?? goals.ToList(); + + ScoreHome = CountOurGoals(counted); + ScoreAway = CountTheirGoals(counted); + } } /// diff --git a/src/FootballFormation.Core/Services/GameService.cs b/src/FootballFormation.Core/Services/GameService.cs index cc0fb83..dc37601 100644 --- a/src/FootballFormation.Core/Services/GameService.cs +++ b/src/FootballFormation.Core/Services/GameService.cs @@ -168,7 +168,15 @@ public Task SaveScoreAsync( return Result.Success(); }); - public Task> AddGoalAsync(GameGoal goal, CancellationToken cancellationToken = default) => + /// + /// True at the touchline, where the scoreline is the goals: the row and the recounted + /// score go in one SaveChanges, so no interruption can leave one written without the + /// other. False on the result page, where the score is typed by hand and the goal list is + /// allowed to be shorter than it — recounting there would turn a 3-1 into the two goals whose + /// scorer someone remembered. + /// + public Task> AddGoalAsync( + GameGoal goal, bool recountScoreline = false, CancellationToken cancellationToken = default) => ServiceOperation.RunAdminAsync(currentUser, logger, "add goal", cancellationToken, async () => { await using var db = await dbFactory.CreateDbContextAsync(cancellationToken); @@ -180,6 +188,15 @@ public Task> AddGoalAsync(GameGoal goal, CancellationToken canc // service; when a service saves one, the service's clock decides. goal.RecordedAt = UtcNow; + if (recountScoreline) + { + var game = await db.Games.FindAsync([goal.GameId], cancellationToken); + + // The goal is not on file yet, so it is counted in rather than queried back. + if (game is not null) + game.CountScoreFrom([.. await GoalsOnFileAsync(db, goal.GameId, cancellationToken), goal]); + } + db.GameGoals.Add(goal); await db.SaveChangesAsync(cancellationToken); @@ -194,7 +211,9 @@ public Task> AddGoalAsync(GameGoal goal, CancellationToken canc return Result.Success(goal); }); - public Task RemoveGoalAsync(int goalId, CancellationToken cancellationToken = default) => + /// + public Task RemoveGoalAsync( + int goalId, bool recountScoreline = false, CancellationToken cancellationToken = default) => ServiceOperation.RunAdminAsync(currentUser, logger, "remove goal", cancellationToken, async () => { await using var db = await dbFactory.CreateDbContextAsync(cancellationToken); @@ -206,6 +225,17 @@ public Task RemoveGoalAsync(int goalId, CancellationToken cancellationTo return Result.Failure("Goal not found"); } + if (recountScoreline) + { + var game = await db.Games.FindAsync([goal.GameId], cancellationToken); + + // This one is still on file, and the query hands back the very instance the change + // tracker holds, so it is dropped from the count by its id rather than by absence. + if (game is not null) + game.CountScoreFrom((await GoalsOnFileAsync(db, goal.GameId, cancellationToken)) + .Where(g => g.Id != goalId)); + } + db.GameGoals.Remove(goal); await db.SaveChangesAsync(cancellationToken); @@ -213,6 +243,15 @@ public Task RemoveGoalAsync(int goalId, CancellationToken cancellationTo return Result.Success(); }); + /// + /// A game's goals, read through the same context the caller is about to save: a + /// scoreline that follows from the goals has to be committed with them, and a transaction + /// cannot span two instances. See docs/patterns.md. + /// + private static Task> GoalsOnFileAsync( + AppDbContext db, int gameId, CancellationToken cancellationToken) => + db.GameGoals.Where(g => g.GameId == gameId).ToListAsync(cancellationToken); + /// /// True only for an admin. The filter lives in the query rather than in the page so a private /// body never reaches a visitor at all — the result page prerenders server-side, so markup that diff --git a/src/FootballFormation.Core/Services/MatchClockService.cs b/src/FootballFormation.Core/Services/MatchClockService.cs index da5f74f..091a5e3 100644 --- a/src/FootballFormation.Core/Services/MatchClockService.cs +++ b/src/FootballFormation.Core/Services/MatchClockService.cs @@ -242,8 +242,7 @@ public Task> FinishMatchAsync(int gameId, CancellationToken cancell // Recounted here rather than through MatchGoalService: this is the recount that settles // the game, and from here it counts towards the season. var goals = await db.GameGoals.Where(g => g.GameId == gameId).ToListAsync(cancellationToken); - game.ScoreHome = Game.CountOurGoals(goals); - game.ScoreAway = Game.CountTheirGoals(goals); + game.CountScoreFrom(goals); await db.SaveChangesAsync(cancellationToken); logger.LogInformation("Finished game {GameId} at {Home}-{Away} after {Seconds}s", diff --git a/src/FootballFormation.Core/Services/MatchGoalService.cs b/src/FootballFormation.Core/Services/MatchGoalService.cs index f8aca26..748a39a 100644 --- a/src/FootballFormation.Core/Services/MatchGoalService.cs +++ b/src/FootballFormation.Core/Services/MatchGoalService.cs @@ -8,10 +8,11 @@ namespace FootballFormation.Core.Services; /// -/// Goals as they are logged at the touchline. Storage itself is delegated to -/// so there is one implementation of it; what this adds is the two things -/// only a match in progress knows — the minute the clock showed when the ball went in, and the -/// scoreline that follows from the goals now on file. +/// Goals as they are logged at the touchline. What this adds is the one thing only a match in +/// progress knows: the minute the clock showed when the ball went in. Storing the goal — and, at +/// the touchline, recounting the scoreline in the same save — is delegated to +/// , so there is one implementation of it and the two rows are written +/// together rather than across two contexts. /// public class MatchGoalService( IDbContextFactory dbFactory, @@ -57,11 +58,7 @@ public Task> LogGoalAsync( IsOpponentGoal = isOpponentGoal }; - var added = await games.AddGoalAsync(goal, cancellationToken); - if (added.IsFailure) return added; - - await SyncScoreAsync(db, gameId, cancellationToken); - return added; + return await games.AddGoalAsync(goal, recountScoreline: true, cancellationToken); }); /// Removes a goal and pulls the scoreline back in step with what is left. @@ -70,24 +67,9 @@ public Task RemoveGoalAsync( LiveMatchOperation.RunAdminAsync(notifier, currentUser, logger, "remove the goal", cancellationToken, async () => { - await using var db = await dbFactory.CreateDbContextAsync(cancellationToken); - - var removed = await games.RemoveGoalAsync(goalId, cancellationToken); + var removed = await games.RemoveGoalAsync(goalId, recountScoreline: true, cancellationToken); if (removed.IsFailure) return removed.To(); - await SyncScoreAsync(db, gameId, cancellationToken); return Result.Success(gameId); }); - - /// Rewrites the scoreline from the logged goals, so the live score is never guessed at. - private static async Task SyncScoreAsync(AppDbContext db, int gameId, CancellationToken cancellationToken) - { - var game = await db.Games.FindAsync([gameId], cancellationToken); - if (game is null) return; - - var goals = await db.GameGoals.Where(g => g.GameId == gameId).ToListAsync(cancellationToken); - game.ScoreHome = Game.CountOurGoals(goals); - game.ScoreAway = Game.CountTheirGoals(goals); - await db.SaveChangesAsync(cancellationToken); - } } diff --git a/src/FootballFormation.Core/Services/MatchPreferencesService.cs b/src/FootballFormation.Core/Services/MatchPreferencesService.cs index 500b626..0b111e0 100644 --- a/src/FootballFormation.Core/Services/MatchPreferencesService.cs +++ b/src/FootballFormation.Core/Services/MatchPreferencesService.cs @@ -30,7 +30,13 @@ public Task> GetAsync(int seasonId, CancellationToken c prefs = await SeedForAsync(db, seasonId, cancellationToken); db.MatchPreferences.Add(prefs); - await db.SaveChangesAsync(cancellationToken); + + // The one read in the app that writes, and the one place the "reads take the page's + // token, writes do not" rule would otherwise be broken: both /settings and the game + // dialog hand this a token that trips when the visitor navigates away. Everything above + // is cancellable and gives up having written nothing; from here the row exists in + // memory and is worth the one insert it costs, so the save is not. + await db.SaveChangesAsync(CancellationToken.None); logger.LogInformation("Created match preferences for season {SeasonId} (ID: {Id})", seasonId, prefs.Id); return Result.Success(prefs); diff --git a/tests/FootballFormation.Core.Tests/GameServiceTests.cs b/tests/FootballFormation.Core.Tests/GameServiceTests.cs index 78899d9..debd4d9 100644 --- a/tests/FootballFormation.Core.Tests/GameServiceTests.cs +++ b/tests/FootballFormation.Core.Tests/GameServiceTests.cs @@ -195,4 +195,43 @@ public async Task Creating_a_game_without_a_season_resolves_one_from_its_date() Assert.True(created.IsSuccess); Assert.Equal(season.Id, created.Value!.SeasonId); } + + /// + /// Creating a game may save a season first, in its own context, so the two saves are two + /// transactions and something can stop between them. What it leaves behind is an empty season, + /// and this is why that is allowed to stand rather than being wrapped in machinery: a season is + /// a gapless window, so the next attempt resolves to the one already there. See docs/patterns.md. + /// + [Fact] + public async Task A_game_scheduled_into_an_empty_season_joins_it_rather_than_making_a_second_one() + { + var season = await SeedSeasonAsync(); + var stranded = (await Seasons.GetOrCreateForDateAsync(season.EndDate.AddYears(1))).Value!; + + var created = await Games.CreateAsync( + TestData.Game(id: 0, seasonId: 0, date: stranded.StartDate.AddDays(10))); + + Assert.Equal(stranded.Id, created.Value!.SeasonId); + Assert.Equal(2, Read().Seasons.Count()); + } + + /// + /// The counterpart to the touchline recount in MatchGoalServiceTests. Here the score is + /// typed and the goal list is allowed to be shorter than it, so adding a scorer someone + /// remembered afterwards must not rewrite a 3-1 as 1-0. + /// + [Fact] + public async Task A_goal_added_after_the_match_leaves_a_hand_typed_scoreline_alone() + { + var season = await SeedSeasonAsync(); + var players = await SeedPlayersAsync(1); + var game = (await Games.CreateAsync(TestData.Game(id: 0, seasonId: season.Id))).Value!; + await Games.SaveScoreAsync(game.Id, 3, 1); + + await Games.AddGoalAsync(new GameGoal { GameId = game.Id, ScorerId = players[0].Id, Minute = 12 }); + + var saved = Read().Games.Single(); + Assert.Equal(3, saved.ScoreHome); + Assert.Equal(1, saved.ScoreAway); + } } diff --git a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs index ce2f7f9..f0b41ae 100644 --- a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs +++ b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs @@ -1,3 +1,9 @@ +using System.Data.Common; +using FootballFormation.Core.Data; +using FootballFormation.Core.Services; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; + namespace FootballFormation.Core.Tests; /// @@ -87,4 +93,76 @@ public async Task Removing_a_goal_that_is_not_there_leaves_the_scoreline_alone() Assert.True(result.IsFailure); Assert.Equal(1, (await ReloadAsync(game.Id)).ScoreHome); } + + /// + /// The goal row and the scoreline it produces are one write, not two. They used to be a save + /// each, in a context each — and no transaction spans two contexts, so a lock timeout or a + /// deploy restarting the container between them left the goal on file with a stale score + /// beside it. Counting the saves is how that stays fixed. + /// + [Fact] + public async Task A_logged_goal_and_the_scoreline_it_makes_are_written_in_one_save() + { + var game = await SeedGameAsync(); + await MatchClock.StartMatchAsync(game.Id); + var players = await PlayersAsync(); + + var saves = new SaveCountingDbContextFactory(Db.Database.GetDbConnection()); + + var goal = await GoalsOver(saves).LogGoalAsync(game.Id, players[1].Id, null, false, false); + + Assert.True(goal.IsSuccess); + Assert.Equal(1, (await ReloadAsync(game.Id)).ScoreHome); + Assert.Equal(1, saves.Count); + } + + /// + [Fact] + public async Task Removing_a_goal_and_the_scoreline_it_leaves_are_written_in_one_save() + { + var game = await SeedGameAsync(); + await MatchClock.StartMatchAsync(game.Id); + var players = await PlayersAsync(); + var goal = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); + + var saves = new SaveCountingDbContextFactory(Db.Database.GetDbConnection()); + + var removed = await GoalsOver(saves).RemoveGoalAsync(game.Id, goal.Value!.Id); + + Assert.True(removed.IsSuccess); + Assert.Equal(0, (await ReloadAsync(game.Id)).ScoreHome); + Assert.Equal(1, saves.Count); + } + + /// The same service, wired to a factory that can be asked what it was made to write. + private MatchGoalService GoalsOver(IDbContextFactory factory) => + new(factory, + new GameService(factory, Seasons, CurrentUser, Time, NullLogger.Instance), + Notifier, Time, CurrentUser, NullLogger.Instance); + + /// + /// Hands out contexts over the test's one connection, like the base fixture does, and counts + /// the saves made through them. + /// + private sealed class SaveCountingDbContextFactory(DbConnection connection) + : IDbContextFactory + { + private int _count; + + public int Count => _count; + + public AppDbContext CreateDbContext() + { + var db = new AppDbContext(new DbContextOptionsBuilder() + .UseSqlite(connection) + .AddInterceptors(new DateInSqlInterceptor()) + .Options); + + db.SavedChanges += (_, _) => Interlocked.Increment(ref _count); + return db; + } + + public Task CreateDbContextAsync(CancellationToken cancellationToken = default) => + Task.FromResult(CreateDbContext()); + } } From c9636789ea71d9076e89e29530c12ea1fcca6ab1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:51:35 +0000 Subject: [PATCH 2/2] Recount the scoreline after the goal is on file, not before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recount was moved ahead of the insert to fit both writes into one SaveChanges, and that made it a read-modify-write: the count came from the goals the caller had just read, with the new one added in memory. Two admins on the same live match — a coach and an assistant, which is what the notifier exists for — each read n goals and each write a scoreline of n+1, leaving two goal rows behind a score of one until the next recount repaired it. The two-context shape this replaced could not lose that, because each recount ran after its own insert had committed. So the insert goes first again, and an explicit transaction holds the two together instead of a shared save. By the time the recount runs, the goal is on file and SQLite's write lock is held, so a second goal logged in the same moment waits and then counts both. Atomicity is unchanged — one commit rather than one save, and the tests count commits now. RecountScorelineAsync also drops the two workarounds the in-memory count needed: it no longer has to add the pending goal to the list or filter the removed one out by id, because it reads them as the database has them. --- docs/known_issues.md | 12 ++- docs/patterns.md | 28 +++--- src/FootballFormation.Core/Models/Game.cs | 11 ++- .../Services/GameService.cs | 69 +++++++++------ .../MatchGoalServiceTests.cs | 88 +++++++++++++------ 5 files changed, 135 insertions(+), 73 deletions(-) diff --git a/docs/known_issues.md b/docs/known_issues.md index f650d9f..17eb196 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -47,10 +47,16 @@ Avoid repeating these mistakes: can throw, and a Fly.io deploy restarts the container mid-write. Logging a goal was shaped that way — insert through `GameService`, recount the scoreline through `MatchGoalService`'s own context — so an interruption between them left the goal on file behind a stale score. **When one - row is derived from another, load and write both through the same context and one `SaveChanges`**; - `GameService.AddGoalAsync(goal, recountScoreline: true)` is what that looks like. The saves are - counted in `MatchGoalServiceTests`, because from the outside two saves and one look identical + row is derived from another, write both through the same context and commit them once**; + `GameService.AddGoalAsync(goal, recountScoreline: true)` is what that looks like. The commits are + counted in `MatchGoalServiceTests`, because from the outside two commits and one look identical right up until something interrupts them. +- **Collapsing that into a single `SaveChanges` looks tidier and reintroduces a lost update**: it + means counting the goals in memory and adding the new one to the total, which is a + read-modify-write on a row with no concurrency token. Two admins on the same live match — the + thing `LiveMatchNotifier` exists for — each read *n* goals and each write a scoreline of *n+1*, + and the score ends up one behind the goal list until the next recount repairs it. **Recount after + the write, inside the transaction**, where SQLite's write lock has already serialised the two. ## Data / domain - **Deleting a player used to be destructive across every season**: `PlayerService.DeleteAsync` cascades their `GamePlayerPosition` rows and nulls their `GameGoal` scorer, so last season's top scorer disappeared from last season's stats — from a confirm that said nothing about it. Fixed by `ArchivePlayersInsteadOfDeleting`: delete now **refuses** for anyone with a lineup or goal row anywhere, and `Player.IsArchived` is the way to retire someone. Worth knowing when the refusal surprises you: the counts are deliberately **not** scoped to a season, unlike `SeasonSquadService.RemoveMemberAsync`'s, because the cascade is not either. diff --git a/docs/patterns.md b/docs/patterns.md index fbfe519..e789469 100644 --- a/docs/patterns.md +++ b/docs/patterns.md @@ -101,9 +101,10 @@ there has to decide. ## When two rows have to agree, one context writes both One `SaveChangesAsync` is a transaction, so almost every mutating method here is atomic without -saying anything: it opens a context, changes what it changes, and saves once. -`GameService.SavePeriodLineupAsync` is the one that needs more and says so — delete-then-insert is -two saves, wrapped in an explicit `BeginTransactionAsync`. +saying anything: it opens a context, changes what it changes, and saves once. The two that need more +say so with an explicit `BeginTransactionAsync` — `GameService.SavePeriodLineupAsync`, where +delete-then-insert is two saves, and the goal writes below, where the second save has to read what +the first one wrote. **What no transaction can cover is two `AppDbContext` instances.** Each operation opens its own from the factory, for the circuit reason above, and each context has its own connection. So a service @@ -113,13 +114,20 @@ land in that gap. That is not hypothetical: the app migrates itself on boot, so restart. The rule that follows: **when one row is derived from another, write them through one context and -one save.** The live scoreline is the worked example. Logging a goal used to insert the row through -`GameService` and then recount the score through `MatchGoalService`'s own context — two saves, and an -interruption between them left the goal on file behind a stale scoreline. It is one save now: -`GameService.AddGoalAsync(goal, recountScoreline: true)` counts the goals already on file, adds the -new one in memory, sets the scoreline, and saves the insert and the update together. -`RemoveGoalAsync` mirrors it. `MatchGoalServiceTests` asserts the count of saves, because "one -write" is the property and it is invisible from the outside. +commit them once.** The live scoreline is the worked example. Logging a goal used to insert the row +through `GameService` and then recount the score through `MatchGoalService`'s own context — two +saves in two contexts, and an interruption between them left the goal on file behind a stale +scoreline. `GameService.AddGoalAsync(goal, recountScoreline: true)` now opens a transaction, saves +the goal, recounts from the goals **then** on file, and commits both together. `RemoveGoalAsync` +mirrors it. `MatchGoalServiceTests` counts the commits, because "one write" is the property and it +is invisible from the outside until something interrupts the halves. + +**The recount goes after the save, not before it.** Counting the goals in memory and adding the new +one to the total would be one save rather than two, which is tempting — and it would be a +read-modify-write. Two touchline devices logging a goal in the same moment would each read *n* and +each write *n+1*, leaving two goal rows behind a scoreline of one. Counting *after* the insert, with +SQLite's write lock already held, makes the second one wait and then count both. The insert has to +come first, so the two writes need the transaction rather than a shared `SaveChanges`. Two ways of getting there were considered and rejected, and both are worth not re-proposing: passing a context or a transaction from one service into another (which breaks the short-lived diff --git a/src/FootballFormation.Core/Models/Game.cs b/src/FootballFormation.Core/Models/Game.cs index 02b9aca..cbf81a5 100644 --- a/src/FootballFormation.Core/Models/Game.cs +++ b/src/FootballFormation.Core/Models/Game.cs @@ -189,14 +189,13 @@ public static int CountTheirGoals(IEnumerable goals) => /// /// Rewrites the scoreline from , so a live score is recounted rather - /// than incremented — the recount is what makes it self-correcting. + /// than incremented — the recount is what makes it self-correcting. Takes a materialised + /// collection because it reads the set twice, once for each end of the pitch. /// - public void CountScoreFrom(IEnumerable goals) + public void CountScoreFrom(IReadOnlyCollection goals) { - var counted = goals as IReadOnlyCollection ?? goals.ToList(); - - ScoreHome = CountOurGoals(counted); - ScoreAway = CountTheirGoals(counted); + ScoreHome = CountOurGoals(goals); + ScoreAway = CountTheirGoals(goals); } } diff --git a/src/FootballFormation.Core/Services/GameService.cs b/src/FootballFormation.Core/Services/GameService.cs index dc37601..ed950a5 100644 --- a/src/FootballFormation.Core/Services/GameService.cs +++ b/src/FootballFormation.Core/Services/GameService.cs @@ -169,11 +169,11 @@ public Task SaveScoreAsync( }); /// - /// True at the touchline, where the scoreline is the goals: the row and the recounted - /// score go in one SaveChanges, so no interruption can leave one written without the - /// other. False on the result page, where the score is typed by hand and the goal list is - /// allowed to be shorter than it — recounting there would turn a 3-1 into the two goals whose - /// scorer someone remembered. + /// True at the touchline, where the scoreline is the goals: the row and the recount + /// commit together, so no interruption can leave one written without the other. False on the + /// result page, where the score is typed by hand and the goal list is allowed to be shorter + /// than it — recounting there would turn a 3-1 into the two goals whose scorer someone + /// remembered. /// public Task> AddGoalAsync( GameGoal goal, bool recountScoreline = false, CancellationToken cancellationToken = default) => @@ -188,18 +188,16 @@ public Task> AddGoalAsync( // service; when a service saves one, the service's clock decides. goal.RecordedAt = UtcNow; - if (recountScoreline) - { - var game = await db.Games.FindAsync([goal.GameId], cancellationToken); - - // The goal is not on file yet, so it is counted in rather than queried back. - if (game is not null) - game.CountScoreFrom([.. await GoalsOnFileAsync(db, goal.GameId, cancellationToken), goal]); - } + await using var tx = await db.Database.BeginTransactionAsync(cancellationToken); db.GameGoals.Add(goal); await db.SaveChangesAsync(cancellationToken); + if (recountScoreline) + await RecountScorelineAsync(db, goal.GameId, cancellationToken); + + await tx.CommitAsync(cancellationToken); + // Reload with navigation properties if (goal.ScorerId is not null) await db.Entry(goal).Reference(g => g.Scorer).LoadAsync(cancellationToken); @@ -225,32 +223,45 @@ public Task RemoveGoalAsync( return Result.Failure("Goal not found"); } - if (recountScoreline) - { - var game = await db.Games.FindAsync([goal.GameId], cancellationToken); - - // This one is still on file, and the query hands back the very instance the change - // tracker holds, so it is dropped from the count by its id rather than by absence. - if (game is not null) - game.CountScoreFrom((await GoalsOnFileAsync(db, goal.GameId, cancellationToken)) - .Where(g => g.Id != goalId)); - } + await using var tx = await db.Database.BeginTransactionAsync(cancellationToken); db.GameGoals.Remove(goal); await db.SaveChangesAsync(cancellationToken); + if (recountScoreline) + await RecountScorelineAsync(db, goal.GameId, cancellationToken); + + await tx.CommitAsync(cancellationToken); + logger.LogInformation("Removed goal {GoalId}", goalId); return Result.Success(); }); /// - /// A game's goals, read through the same context the caller is about to save: a - /// scoreline that follows from the goals has to be committed with them, and a transaction - /// cannot span two instances. See docs/patterns.md. + /// Rewrites a game's scoreline from the goals on file, through the same context and + /// inside the same transaction as the write that prompted it — and after that write + /// has been saved, so the goal it added or removed is already reflected in what is counted. + /// + /// Both halves of that matter. A transaction cannot span two + /// instances, so counting through a second context would leave a gap nothing can roll back; + /// and counting in memory before the save would make this a read-modify-write, which + /// two touchline devices logging a goal in the same moment would both get wrong. Counting + /// afterwards makes the second one wait for SQLite's write lock and then count both goals. + /// See docs/patterns.md. + /// /// - private static Task> GoalsOnFileAsync( - AppDbContext db, int gameId, CancellationToken cancellationToken) => - db.GameGoals.Where(g => g.GameId == gameId).ToListAsync(cancellationToken); + private static async Task RecountScorelineAsync( + AppDbContext db, int gameId, CancellationToken cancellationToken) + { + var game = await db.Games.FindAsync([gameId], cancellationToken); + if (game is null) return; + + game.CountScoreFrom(await db.GameGoals + .Where(g => g.GameId == gameId) + .ToListAsync(cancellationToken)); + + await db.SaveChangesAsync(cancellationToken); + } /// /// True only for an admin. The filter lives in the query rather than in the page so a private diff --git a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs index f0b41ae..282d7db 100644 --- a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs +++ b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs @@ -1,7 +1,9 @@ using System.Data.Common; using FootballFormation.Core.Data; +using FootballFormation.Core.Models; using FootballFormation.Core.Services; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Logging.Abstractions; namespace FootballFormation.Core.Tests; @@ -95,43 +97,65 @@ public async Task Removing_a_goal_that_is_not_there_leaves_the_scoreline_alone() } /// - /// The goal row and the scoreline it produces are one write, not two. They used to be a save - /// each, in a context each — and no transaction spans two contexts, so a lock timeout or a - /// deploy restarting the container between them left the goal on file with a stale score - /// beside it. Counting the saves is how that stays fixed. + /// The goal row and the scoreline it produces reach the database together. They used to be a + /// save each in a context each — and no transaction spans two contexts, so a lock timeout or a + /// deploy restarting the container between them left the goal on file with a stale score beside + /// it. One commit is the property that fixed it, and it is invisible from the outside until + /// something interrupts the two halves, so it is counted here instead. /// [Fact] - public async Task A_logged_goal_and_the_scoreline_it_makes_are_written_in_one_save() + public async Task A_logged_goal_and_the_scoreline_it_makes_are_committed_together() { var game = await SeedGameAsync(); await MatchClock.StartMatchAsync(game.Id); var players = await PlayersAsync(); - var saves = new SaveCountingDbContextFactory(Db.Database.GetDbConnection()); + var commits = new CommitCountingDbContextFactory(Db.Database.GetDbConnection()); - var goal = await GoalsOver(saves).LogGoalAsync(game.Id, players[1].Id, null, false, false); + var goal = await GoalsOver(commits).LogGoalAsync(game.Id, players[1].Id, null, false, false); Assert.True(goal.IsSuccess); Assert.Equal(1, (await ReloadAsync(game.Id)).ScoreHome); - Assert.Equal(1, saves.Count); + Assert.Equal(1, commits.Count); } - /// + /// [Fact] - public async Task Removing_a_goal_and_the_scoreline_it_leaves_are_written_in_one_save() + public async Task Removing_a_goal_and_the_scoreline_it_leaves_are_committed_together() { var game = await SeedGameAsync(); await MatchClock.StartMatchAsync(game.Id); var players = await PlayersAsync(); var goal = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); - var saves = new SaveCountingDbContextFactory(Db.Database.GetDbConnection()); + var commits = new CommitCountingDbContextFactory(Db.Database.GetDbConnection()); - var removed = await GoalsOver(saves).RemoveGoalAsync(game.Id, goal.Value!.Id); + var removed = await GoalsOver(commits).RemoveGoalAsync(game.Id, goal.Value!.Id); Assert.True(removed.IsSuccess); Assert.Equal(0, (await ReloadAsync(game.Id)).ScoreHome); - Assert.Equal(1, saves.Count); + Assert.Equal(1, commits.Count); + } + + /// + /// The scoreline counts the goal rows as the database has them, not as the caller last saw + /// them. Counting in memory ahead of the insert would read the same way here and be a + /// read-modify-write two touchline devices could both get wrong. + /// + [Fact] + public async Task The_scoreline_counts_a_goal_logged_behind_this_ones_back() + { + var game = await SeedGameAsync(); + await MatchClock.StartMatchAsync(game.Id); + var players = await PlayersAsync(); + + // On file without the scoreline following it — a goal the next recount has to pick up. + Db.GameGoals.Add(new GameGoal { GameId = game.Id, ScorerId = players[1].Id, Minute = 3 }); + await Db.SaveChangesAsync(); + + await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); + + Assert.Equal(2, (await ReloadAsync(game.Id)).ScoreHome); } /// The same service, wired to a factory that can be asked what it was made to write. @@ -142,27 +166,41 @@ private MatchGoalService GoalsOver(IDbContextFactory factory) => /// /// Hands out contexts over the test's one connection, like the base fixture does, and counts - /// the saves made through them. + /// what is committed through them — EF raises this for the transaction it opens around a lone + /// SaveChanges as well as for an explicit one, so two saves that commit once count once. /// - private sealed class SaveCountingDbContextFactory(DbConnection connection) + private sealed class CommitCountingDbContextFactory(DbConnection connection) : IDbContextFactory { - private int _count; + private readonly CommitCounter _counter = new(); - public int Count => _count; + public int Count => _counter.Count; - public AppDbContext CreateDbContext() - { - var db = new AppDbContext(new DbContextOptionsBuilder() + public AppDbContext CreateDbContext() => + new(new DbContextOptionsBuilder() .UseSqlite(connection) - .AddInterceptors(new DateInSqlInterceptor()) + .AddInterceptors(new DateInSqlInterceptor(), _counter) .Options); - db.SavedChanges += (_, _) => Interlocked.Increment(ref _count); - return db; - } - public Task CreateDbContextAsync(CancellationToken cancellationToken = default) => Task.FromResult(CreateDbContext()); + + private sealed class CommitCounter : IDbTransactionInterceptor + { + private int _count; + + public int Count => _count; + + public void TransactionCommitted(DbTransaction transaction, TransactionEndEventData eventData) => + Interlocked.Increment(ref _count); + + public Task TransactionCommittedAsync( + DbTransaction transaction, TransactionEndEventData eventData, + CancellationToken cancellationToken = default) + { + Interlocked.Increment(ref _count); + return Task.CompletedTask; + } + } } }