From 1a2429810586aa614cfa5fc3cb6b00f0cd3fb808 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 19:08:25 +0200 Subject: [PATCH] fix(#253 PR2): close review findings (ETag/items consistency + SPA load ordering) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent Codex review of #268 found two Blockers the fork missed + two Mediums: - Blocker: replace PUTs returned the handler's item snapshot but re-queried the root for the ETag separately, so a racing writer could pair stale items with a newer ETag (silent overwrite). All four controllers now reload root-then-items (version-first, fail-safe) and 404 when the root is gone between commit and reload — matching the Block reference. Fixes the Blocker + the Medium '200 without ETag' case together. - Blocker: PlaylistsScreen loaded items+root via Promise.all (concurrent), pairing a stale name with the current ETag; now sequential (items-with-meta first, then root). - Medium: SchedulesScreen loadItems now marks not-loaded/loading up front so canEdit is false through the 412 conflict reload (no stale-draft edits lost). Controller unit-test mocks updated to stub the new reload query. Full suite green (ErsatzTV.Tests 1334, web 667, check:api no drift). --- .../DecoTemplateControllerTests.cs | 6 ++++++ .../Controllers/PlaylistControllerTests.cs | 20 ++++++++++++++++++ .../Controllers/ScheduleControllerTests.cs | 8 +++++++ .../Controllers/TemplateControllerTests.cs | 6 ++++++ .../Controllers/Api/DecoTemplateController.cs | 6 +++++- .../Controllers/Api/PlaylistController.cs | 15 ++++++++++--- .../Controllers/Api/ScheduleController.cs | 21 +++++++++++++------ .../Controllers/Api/TemplateController.cs | 7 ++++++- web/src/screens/PlaylistsScreen.tsx | 10 +++++++-- web/src/screens/SchedulesScreen.tsx | 6 ++++++ 10 files changed, 92 insertions(+), 13 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs b/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs index 044c83450..e5ed3b39c 100644 --- a/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs @@ -268,6 +268,8 @@ public class DecoTemplateControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>( [MakeItem(10, "Morning Bumper", TimeSpan.FromHours(6), TimeSpan.FromHours(7))])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeItem(10, "Morning Bumper", TimeSpan.FromHours(6), TimeSpan.FromHours(7))]); IActionResult result = await _controller.Replace( 4, @@ -359,6 +361,8 @@ public class DecoTemplateControllerTests .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning", version: 4))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>([])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); IActionResult result = await _controller.Replace( 4, @@ -380,6 +384,8 @@ public class DecoTemplateControllerTests .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning"))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>([])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); await _controller.Replace( 4, diff --git a/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs b/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs index 8ca6377e9..be24d0f38 100644 --- a/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs @@ -355,6 +355,22 @@ public class PlaylistControllerTests false, true) })); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List + { + new( + 100, + 0, + CollectionType.Movie, + null, + null, + null, + new NamedMediaItemViewModel(55, "The Movie"), + PlaybackOrder.Shuffle, + null, + false, + true) + }); IActionResult result = await _controller.Update( 4, @@ -407,6 +423,8 @@ public class PlaylistControllerTests .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 3))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>(new List())); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); IActionResult result = await _controller.Update( 4, @@ -426,6 +444,8 @@ public class PlaylistControllerTests .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>(new List())); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); await _controller.Update( 4, diff --git a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs index 506d62cdd..a0494db99 100644 --- a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs @@ -259,6 +259,10 @@ public class ScheduleControllerTests List items = [MakeOneItem(21), MakeOneItem(22)]; _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>(items)); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeSchedule(4, "Daily"))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(items); IActionResult result = await _controller.ReplaceItems( 4, @@ -313,6 +317,8 @@ public class ScheduleControllerTests .Returns(Right>([MakeOneItem(21)])); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.Some(MakeSchedule(4, "Daily", version: 4))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeOneItem(21)]); IActionResult result = await _controller.ReplaceItems( 4, @@ -334,6 +340,8 @@ public class ScheduleControllerTests .Returns(Right>([MakeOneItem(21)])); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.Some(MakeSchedule(4, "Daily", version: 1))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeOneItem(21)]); await _controller.ReplaceItems( 4, diff --git a/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs b/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs index 4a5f9c595..ffe27ace4 100644 --- a/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs @@ -257,6 +257,8 @@ public class TemplateControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>( [MakeItem(10, "Cartoons", TimeSpan.FromHours(6), 60)])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeItem(10, "Cartoons", TimeSpan.FromHours(6), 60)]); IActionResult result = await _controller.Replace( 4, @@ -346,6 +348,8 @@ public class TemplateControllerTests .Returns(Option.Some(MakeTemplate(4, 7, "Morning", version: 4))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>([])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); IActionResult result = await _controller.Replace( 4, @@ -367,6 +371,8 @@ public class TemplateControllerTests .Returns(Option.Some(MakeTemplate(4, 7, "Morning"))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>([])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); await _controller.Replace( 4, diff --git a/ErsatzTV/Controllers/Api/DecoTemplateController.cs b/ErsatzTV/Controllers/Api/DecoTemplateController.cs index 1c8dc80fb..53c8e122e 100644 --- a/ErsatzTV/Controllers/Api/DecoTemplateController.cs +++ b/ErsatzTV/Controllers/Api/DecoTemplateController.cs @@ -212,10 +212,14 @@ public class DecoTemplateController(IMediator mediator) : ControllerBase return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async items => + Right: async _ => { + // Reload the root (version) FIRST, then the items, so the emitted ETag is never newer than + // the returned items (issue #253 fail-safe ordering; matches BlockController). Option refreshed = await mediator.Send(new GetDecoTemplateById(id), cancellationToken); + List items = + await mediator.Send(new GetDecoTemplateItems(id), cancellationToken); return refreshed.Match( Some: vm => { diff --git a/ErsatzTV/Controllers/Api/PlaylistController.cs b/ErsatzTV/Controllers/Api/PlaylistController.cs index c2762af42..be9e152fd 100644 --- a/ErsatzTV/Controllers/Api/PlaylistController.cs +++ b/ErsatzTV/Controllers/Api/PlaylistController.cs @@ -222,12 +222,21 @@ public class PlaylistController(IMediator mediator) : ControllerBase await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async items => + Right: async _ => { + // Reload the root (version) FIRST, then the items, so the emitted ETag is never newer than + // the returned items (issue #253 fail-safe ordering; matches BlockController). A None root + // (deleted between commit and reload) is a 404, never a 200 without an ETag. Option refreshed = await mediator.Send(new GetPlaylistById(id), cancellationToken); - refreshed.Do(vm => ConcurrencyHeaders.SetETag(Response, vm.Version)); - return (IActionResult)new OkObjectResult(items.Map(ProjectToItemResponse).ToList()); + List items = await mediator.Send(new GetPlaylistItems(id), cancellationToken); + return refreshed.Match( + Some: vm => + { + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult(items.Map(ProjectToItemResponse).ToList()); + }, + None: () => ApiResults.NotFoundProblem()); }); } diff --git a/ErsatzTV/Controllers/Api/ScheduleController.cs b/ErsatzTV/Controllers/Api/ScheduleController.cs index 86b006b1b..c325f7824 100644 --- a/ErsatzTV/Controllers/Api/ScheduleController.cs +++ b/ErsatzTV/Controllers/Api/ScheduleController.cs @@ -180,15 +180,24 @@ public class ScheduleController(IMediator mediator) : ControllerBase return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async items => + Right: async _ => { - // Return the new ETag so a same-tab second save doesn't 412 against its own write. + // Reload the root (version) FIRST, then the items, so the emitted ETag is never newer than + // the returned items (issue #253 fail-safe ordering; matches BlockController). A None root + // (deleted between commit and reload) is a 404, never a 200 without an ETag. Option refreshed = await mediator.Send(new GetProgramScheduleById(id), cancellationToken); - refreshed.IfSome(vm => ConcurrencyHeaders.SetETag(Response, vm.Version)); - - return (IActionResult)new OkObjectResult( - items.Select(ScheduleItemResponseMapper.ProjectToResponseModel).ToList()); + List items = + await mediator.Send(new GetProgramScheduleItems(id), cancellationToken); + return refreshed.Match( + Some: vm => + { + // Return the new ETag so a same-tab second save doesn't 412 against its own write. + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult( + items.Select(ScheduleItemResponseMapper.ProjectToResponseModel).ToList()); + }, + None: () => ApiResults.NotFoundProblem()); }); } diff --git a/ErsatzTV/Controllers/Api/TemplateController.cs b/ErsatzTV/Controllers/Api/TemplateController.cs index 624320e38..5bf3a11f8 100644 --- a/ErsatzTV/Controllers/Api/TemplateController.cs +++ b/ErsatzTV/Controllers/Api/TemplateController.cs @@ -200,9 +200,14 @@ public class TemplateController(IMediator mediator) : ControllerBase return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async items => + Right: async _ => { + // Reload the root (version) FIRST, then the items, so the emitted ETag is never newer than + // the returned items (issue #253 fail-safe ordering; matches BlockController). Returning the + // handler's item snapshot alongside a separately re-queried version could pair stale items + // with a newer ETag — a client would then silently overwrite the interleaving write. Option refreshed = await mediator.Send(new GetTemplateById(id), cancellationToken); + List items = await mediator.Send(new GetTemplateItems(id), cancellationToken); return refreshed.Match( Some: vm => { diff --git a/web/src/screens/PlaylistsScreen.tsx b/web/src/screens/PlaylistsScreen.tsx index f6714dc5b..dd0f7fdc6 100644 --- a/web/src/screens/PlaylistsScreen.tsx +++ b/web/src/screens/PlaylistsScreen.tsx @@ -391,8 +391,14 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o activeRef.current = true; // Read items + ETag FIRST, then the root metadata, so the ETag is never newer than the data // the draft is built from (issue #253) — any resulting inconsistency fails safe via a 412 on - // save rather than a silent overwrite. - Promise.all([getPlaylistItemsWithMeta(playlistId), getPlaylistById(playlistId)]) + // save rather than a silent overwrite. These MUST be sequential, not Promise.all: a concurrent + // root read could return a stale name paired with the current items ETag, and a later save would + // then silently restore that stale name. + getPlaylistItemsWithMeta(playlistId) + .then(async (itemsMeta) => { + const playlist = await getPlaylistById(playlistId); + return [itemsMeta, playlist] as const; + }) .then(([itemsMeta, playlist]) => { if (!activeRef.current) { return; diff --git a/web/src/screens/SchedulesScreen.tsx b/web/src/screens/SchedulesScreen.tsx index d269b560e..618830ddc 100644 --- a/web/src/screens/SchedulesScreen.tsx +++ b/web/src/screens/SchedulesScreen.tsx @@ -137,6 +137,12 @@ export function SchedulesScreen() { // ---- Load items for the active schedule -------------------------------- const loadItems = useCallback((scheduleId: number) => { const seq = ++itemSeq.current; + // Mark not-loaded/loading up front so `canEdit` (itemsLoaded && !saving) is false for the whole + // (re)load window — including the 412 conflict reload. Otherwise a slow reload GET would leave the + // rejected/stale draft editable, and edits made during that window would be silently discarded when + // the reload re-seeds the baseline (issue #253 / #242 mutation gating). + setItemsLoading(true); + setItemsLoaded(false); getScheduleItemsWithMeta(scheduleId) .then(({ data: response, etag }) => { if (!activeRef.current || itemSeq.current !== seq) {