Write a live goal and the score it makes in one commit, not two

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

docs/known_issues.md

@@ -39,6 +39,24 @@ 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, 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.

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

docs/patterns.md

@@ -90,6 +90,74 @@ 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. 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
+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
+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
+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 +177,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

src/FootballFormation.Core/Models/Game.cs

@@ -186,6 +186,17 @@ public static int CountOurGoals(IEnumerable<GameGoal> goals) =>
     /// <summary>Their goals: everything the opponent scored, plus our own goals.</summary>
     public static int CountTheirGoals(IEnumerable<GameGoal> goals) =>
         goals.Count(g => !g.CountsForUs);
+
+    /// <summary>
+    /// Rewrites the scoreline from <paramref name="goals"/>, so a live score is recounted rather
+    /// 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.
+    /// </summary>
+    public void CountScoreFrom(IReadOnlyCollection<GameGoal> goals)
+    {
+        ScoreHome = CountOurGoals(goals);
+        ScoreAway = CountTheirGoals(goals);
+    }
 }
 
 /// <summary>

src/FootballFormation.Core/Services/GameService.cs

@@ -168,7 +168,15 @@ public Task<Result> SaveScoreAsync(
             return Result.Success();
         });
 
-    public Task<Result<GameGoal>> AddGoalAsync(GameGoal goal, CancellationToken cancellationToken = default) =>
+    /// <param name="recountScoreline">
+    /// True at the touchline, where the scoreline <em>is</em> 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.
+    /// </param>
+    public Task<Result<GameGoal>> 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,9 +188,16 @@ public Task<Result<GameGoal>> AddGoalAsync(GameGoal goal, CancellationToken canc
             // service; when a service saves one, the service's clock decides.
             goal.RecordedAt = UtcNow;
 
+            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);
@@ -194,7 +209,9 @@ public Task<Result<GameGoal>> AddGoalAsync(GameGoal goal, CancellationToken canc
             return Result.Success(goal);
         });
 
-    public Task<Result> RemoveGoalAsync(int goalId, CancellationToken cancellationToken = default) =>
+    /// <inheritdoc cref="AddGoalAsync(GameGoal, bool, CancellationToken)" path="/param[@name='recountScoreline']"/>
+    public Task<Result> 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,13 +223,46 @@ public Task<Result> RemoveGoalAsync(int goalId, CancellationToken cancellationTo
                 return Result.Failure("Goal not found");
             }
 
+            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();
         });
 
+    /// <summary>
+    /// Rewrites a game's scoreline from the goals on file, through the <em>same</em> context and
+    /// inside the <em>same</em> 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.
+    /// <para>
+    /// Both halves of that matter. A transaction cannot span two <see cref="AppDbContext"/>
+    /// instances, so counting through a second context would leave a gap nothing can roll back;
+    /// and counting in memory <em>before</em> 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.
+    /// </para>
+    /// </summary>
+    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);
+    }
+
     /// <param name="includePrivate">
     /// 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

src/FootballFormation.Core/Services/MatchClockService.cs

@@ -242,8 +242,7 @@ public Task<Result<Game>> 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",

src/FootballFormation.Core/Services/MatchGoalService.cs

@@ -8,10 +8,11 @@
 namespace FootballFormation.Core.Services;
 
 /// <summary>
-/// Goals as they are logged at the touchline. Storage itself is delegated to
-/// <see cref="GameService"/> 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
+/// <see cref="GameService"/>, so there is one implementation of it and the two rows are written
+/// together rather than across two contexts.
 /// </summary>
 public class MatchGoalService(
     IDbContextFactory<AppDbContext> dbFactory,
@@ -57,11 +58,7 @@ public Task<Result<GameGoal>> 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);
         });
 
     /// <summary>Removes a goal and pulls the scoreline back in step with what is left.</summary>
@@ -70,24 +67,9 @@ public Task<Result> 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<int>();
 
-            await SyncScoreAsync(db, gameId, cancellationToken);
             return Result.Success(gameId);
         });
-
-    /// <summary>Rewrites the scoreline from the logged goals, so the live score is never guessed at.</summary>
-    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);
-    }
 }