Files
ersatztv/docs/decisions/optimistic-concurrency.md
T
timothyandClaude Opus 4.8 a2c056dd7a
Build ErsatzTV Image / Docs update reminder (push) Has been skipped
Build ErsatzTV Image / decisions.md append-only (push) Has been skipped
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Has been skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Has been skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Has been cancelled
Build ErsatzTV Image / Build & test (.NET) (push) Has been cancelled
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Has been cancelled
Build ErsatzTV Image / Functional E2E (curl contracts) (push) Has been cancelled
docs(release): prepare v26.9.0 promotion [decisions-edit]
Consolidate docs/decisions.md (1923 -> 1028) by extracting four cohesive
topic clusters into docs/decisions/ (optimistic-concurrency,
api-auth-security, release-ci-governance, spa-modularization) — content
relocated verbatim (lossless; all rationale + reversals preserved), main
Index rebuilt to reference the topic files plus the remaining in-file
entries, docs/README.md points back at the decisions Index. Add the
v26.9.0 row to the ci-cd.md version table.

refs #340

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-16 22:17:23 +02:00

257 lines
22 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Optimistic concurrency — ETag / If-Match / Version (#253, #259, #265, #269)
Why the replace-all and config-bearing aggregates carry optimistic concurrency, and how the
contract evolved (including the reversals). The **mechanics** — headers, status codes, the
recipe — live in `docs/api-conventions.md` §7a–§7c; this file preserves the *decision
rationale*, relocated from the append-only `docs/decisions.md` at the v26.9.0 consolidation.
Issue trail: #253 (PR1 infra + PR3 Diff/Scalar fan-out), #259 (stable child identity for
schedule-item replace), #265 (RFC 7232 If-Match semantics), #269 (non-If-Match force-write +
cross-editor ETag rotation). Refs #197.
## Contents
- [2026-07-11 — Optimistic-concurrency contract for replace-all PUTs (#253 PR1: infra + Block reference)](#2026-07-11--optimistic-concurrency-contract-for-replace-all-puts-253-pr1-infra--block-reference)
- [2026-07-11 — #253 PR3: Diff + Scalar concurrency fan-out (Collection / Playout×2 / MultiCollection / RerunCollection)](#2026-07-11--253-pr3-diff--scalar-concurrency-fan-out-collection--playout2--multicollection--reruncollection)
- [2026-07-11 — Stable child identity for schedule-item replace (#259, split from #252/#253)](#2026-07-11--stable-child-identity-for-schedule-item-replace-259-split-from-252253)
- [2026-07-12 (#269 — non-If-Match root writers force-write past a concurrent Version bump)](#2026-07-12-269--non-if-match-root-writers-force-write-past-a-concurrent-version-bump)
- [2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)](#2026-07-12--cross-editor-etag-rotation-completed-for-collectionplayout-config-siblings-269)
- [2026-07-12 — If-Match evaluates per RFC 7232: valid-but-non-matching → 412, only grammar violations → 400 (#265)](#2026-07-12--if-match-evaluates-per-rfc-7232-valid-but-non-matching--412-only-grammar-violations--400-265)
---
## 2026-07-11 — Optimistic-concurrency contract for replace-all PUTs (#253 PR1: infra + Block reference)
Replace-all aggregate PUTs had **no** optimistic concurrency — a stale second tab silently overwrote a
fresher edit (200, no signal) across ~10 aggregate surfaces. PR1 lands the shared contract on the Block
reference aggregate; PRs 24 fan it out. The full ratified design + independent-review hardening is
[#253#issuecomment-8472](http://192.168.1.95:3000/timothy/ersatztv/issues/253#issuecomment-8472);
the mechanics live in `api-conventions.md` §7a. Decisions frozen here:
- **Token = uniform plain `int Version`** on each root implementing `IVersionedAggregate`, EF-mapped
`.IsConcurrencyToken()`, one dual-provider migration (`AddAggregateVersions`, `defaultValue: 0`). **Not**
a reused `DateUpdated` (tick-collision, SQLite TEXT precision, couples UI cosmetics to correctness) and
**not** a MySQL-native rowversion (portability over provider-native).
- **412 Precondition Failed**, not 409 — 409 stays the §3a EntityLocker "build in progress" guard;
distinct codes → distinct SPA UX. New `PreconditionFailedError : BaseError` → 412 in `ApiResults.ToErrorResult`.
- **Pre-check AND EF token both required.** The handler pre-check (a standalone `Either` introduced AFTER
the validation pipeline — never via `Apply`, which `Join()`-flattens the subtype to 422) gives a clean
412; the unconditional `root.Version++` + `IsConcurrencyToken` UPDATE-guard + a `SaveChangesWithConcurrencyGuard`
backstop closes the residual load→save TOCTOU (`DbUpdateConcurrencyException` → 412).
- **Unconditional bump** (not "only when a child changed"): EF writes the root row only when a scalar
differs, so a no-op PUT-back must still bump to fire the token and rotate every other client's ETag.
- **Config-only aggregate boundary**: every mutating handler of a root's *editor-visible config state*
bumps `Version` (incl. bulk `ExecuteUpdate/Delete` writers via `.SetProperty`); regenerated build output
(playout items/history) is outside the token — neither bumped nor guarded.
- **Header-only ETag**, strong tag of the decimal `Version`; parsed/emitted by `ConcurrencyHeaders`. The
successful PUT returns the new ETag (else a same-tab second save 412s against its own write).
- **Phasing**: Phase 1 (this arc) = a missing `If-Match` force-writes (zero breakage) while the SPA starts
echoing; Phase 2 (a later PR) flips missing → **428** after every editor echoes and one release soaks.
`If-Match: *` stays the scripted force-write escape hatch.
- **Child stable-identity is OUT of #253** (the "moved fill-group item inherits the wrong slot's state"
concern on the positional reconcile) — root-anchored versioning is orthogonal to it; split to **#259**.
- **If-Match status semantics** (non-canonical/weak/list → 400) are fail-safe; the stricter RFC 7232
"valid-but-non-matching → 412" refinement is deferred to #197 (**#265**).
## 2026-07-11 — #253 PR3: Diff + Scalar concurrency fan-out (Collection / Playout×2 / MultiCollection / RerunCollection)
**Context.** PR3 of the #253 optimistic-concurrency arc fans the frozen Block recipe (api-conventions §7a)
across the five Diff/Scalar aggregates. Three judgment calls beyond the mechanical copy:
**H1 — Playout `catch(Exception)`→422.** The two Playout replace handlers wrap `SaveChangesAsync` in a
`catch(Exception)` that maps any exception to a bare `BaseError` (→ 422). Rather than let the guard's
concurrency failure be reshaped into a 422, the guarded save (`SaveChangesWithConcurrencyGuard`) returns a
`PreconditionFailedError` **Left as a value** and the handler returns it before the post-commit block —
so it never reaches the catch. Proven by the pre-check-subtype tests (a `.Apply` flatten would fail
`ShouldBeOfType<PreconditionFailedError>`) plus a non-vacuous Playout racing-save test.
**M2 — the `SaveChangesAsync() > 0` gates.** RerunCollection and Collection-custom-order run their
playout-refresh **unconditionally** on a successful save (the unconditional `Version++` makes the old gate
always-true; the "nothing changed" branch is dead). MultiCollection is the exception: it saved the name
first specifically so a name-only change wouldn't rebuild playouts, so we bump `Version` on that **first**
save and leave the **second** (items) save's `> 0` gate intact — a name-only edit still bumps + rotates the
ETag but does not rebuild. Enumerating every behavior the gate provided before reworking it (the #232 lesson).
**Sibling-writer scope (deferred).** §7a's config-only boundary says every writer of an aggregate's
editor-visible config bumps `Version`. PR3 ships the five primary endpoints' full contract + the one
design-named bulk writer (`UpdateDefaultDecoHandler`, safe via `.SetProperty`). It **defers** the other
same-root non-bulk config writers (`UpdateCollectionHandler`, `RemoveItemsFromCollectionHandler`,
`UpdatePlayoutHandler`, the `ScheduleFile` handlers) and the repository-mediated `Add*ToCollection` family.
Rationale: the primary endpoints' own bump+guard fully cover the two-tab lost-update the issue targets;
the deferred writers only affect cross-editor ETag *rotation*, and adding an unconditional bump to a handler
that uses plain `SaveChangesAsync` (not the guard) converts a latent lost-update into a **new 500**
(`DbUpdateConcurrencyException`) — doing it safely needs a uniform guard+bump+412 pass of its own, better
done with the #197 contract work. Tracked as a follow-up issue.
**VMs.** `Playout.Version` surfaces via `PlayoutNameViewModel` (required arg); the three collection VMs
(`MediaCollectionViewModel`, `MultiCollectionViewModel`, `RerunCollectionViewModel`) carry `int Version = 0`
(defaulted — 0 for the selection-placeholder constructions, real value from the Mapper projection).
Header-only via ETag, never echoed in a response body (the Block precedent).
**Post-merge addendum (PR3 review, #269).** Activating the `Version` token means EF guards *every* root
UPDATE, so non-participating root-scalar writers that use plain `SaveChangesAsync` (playout settings /
schedule-file / on-demand-checkpoint, collection name) would 500 on a concurrent bump. The realistic
UPDATE writers were fixed in-PR with `ConcurrencyExtensions.SaveChangesForcingVersion` (Phase-1
force-write on conflict: adopt the stored token, retry, never revert the concurrent bump). The deferral
above is re-scoped to the DELETE handlers + repository `Add*` writers only (→ #269).
## 2026-07-11 — Stable child identity for schedule-item replace (#259, split from #252/#253)
`PUT /api/schedules/{id}/items` now reconciles by an optional round-tripped child id, not by array
position, so an item's persisted fill-group/shuffle state (`PlayoutScheduleItemFillGroupIndex`, FK
`OnDelete(Cascade)`) follows the logical item across reorders/inserts instead of being inherited by
whatever previously held its new slot. Contract + rules in **api-conventions §7c**. Key decisions:
- **`ScheduleItemRequest.Id` (`int?`)**: null/absent/`0` ⇒ new item (controller normalizes `0`→null so the
handler is two-state). Any id present ⇒ id-based reconcile; a fully id-less payload keeps the verbatim
positional fallback (legacy; retires with the §7a Phase-2 `If-Match`→428 flip).
- **Unknown or duplicate id ⇒ 422, nothing persisted**; the guards live in the handler **after** §7a
`CheckVersion`, so **412 precedes 422** — a client that is both version-stale and id-stale gets the reload
signal, not a payload-bug signal. Rationale for reject-not-insert on an unknown id: under Phase-1
force-write a stale id is a live lost-update signal, so silently inserting-as-new would duplicate the item
and return a different id than the client sent (the exact class §7a exists to surface). This is also the
correct #197 posture — never honor an unrecognized identifier.
- **Scope = schedule items only.** Blocks/templates/deco-templates/playlists stay positional: their children
are stateless config rows (no FK'd state to misattribute; #3/#4 have no GET child id). Child ids are added
only where a child row anchors server-side state; the contract can be retrofitted per-endpoint later
(field stays optional) — so this is not #197 ossification pressure.
- **TPT subtype change at a matched id** stays delete+insert (EF can't retype in place); state resets and a
new id is returned, so the SPA must re-seed item state from the PUT response (a stale id on a second save
now 422s).
## 2026-07-12 (#269 — non-If-Match root writers force-write past a concurrent Version bump)
**Routing the aggregate delete handlers + `UpdateProgramScheduleHandler` through `SaveChangesForcingVersion`.**
Once #253 made each replace-all root's `Version` an `IsConcurrencyToken`, EF started guarding *every*
UPDATE **and DELETE** of that row with `WHERE Version=@orig` — so any writer that is not part of the
If-Match contract but still saves via plain `SaveChangesAsync` throws an unhandled
`DbUpdateConcurrencyException`→**500** if a replace-all editor bumps the row in its narrow load→save window.
PR3 already force-wrote the exposed *UPDATE* siblings (Playout settings/`ScheduleFile`/checkpoint,
`UpdateCollectionHandler`); a completeness sweep for #269 found the gap was wider than reported —
**18 writers** in total, all on plain `SaveChangesAsync`. **The correct exposure filter is "any handler
that leaves a versioned root `Modified` or `Deleted`", NOT just `Version`-bumpers + deletes** — an early
sweep used the narrower filter and a review of PR #302 caught what it missed (`ErasePlayoutHistory` below):
- the **nine versioned-root delete handlers** (`DeletePlayout`/`DeleteCollection`/`DeleteMultiCollection`/
`DeleteRerunCollection`/`DeletePlaylist`/`DeleteBlock`/`DeleteTemplate`/`DeleteDecoTemplate`/
`DeleteProgramSchedule`) — a DELETE is now token-guarded too;
- `UpdateProgramScheduleHandler` (bumps `Version` then saved plainly — the ProgramSchedule case PR3 only
*suspected*);
- the **seven item add/remove bumpers** that PR2 wired to bump their root but left on plain save —
`AddProgramScheduleItem`/`DeleteProgramScheduleItem` and the five
`Add{Items,Movie,Show,Season,Episode}ToPlaylist` handlers;
- **`ErasePlayoutHistoryHandler`** — modifies Playout root **scalars** (`Seed`/`Anchor`/`OnDemandCheckpoint`)
**without** bumping `Version`, inside an explicit transaction with no try/catch → the one the bumper-only
filter missed; reachable via `POST /api/playouts/{id}/erase-items-and-history`.
All now save through `ConcurrencyExtensions.SaveChangesForcingVersion`.
**Two deliberate boundaries (documented, not gaps):** (1) the background build/time-shift Playout-scalar
writers (`BuildPlayoutHandler` via `PlayoutBuilder`'s `Anchor`/`Seed`; `PlayoutTimeShifter`'s
`OnDemandCheckpoint`) are token-guarded too but **intentionally left on plain save** — they never surface a
request-path 500 (`BuildPlayoutHandler` catches → a build-failure `BaseError`; `PlayoutTimeShifter` runs only
via the background worker), and force-writing would be *wrong*: a concurrent config edit that bumped
`Version` also enqueues a rebuild, so failing the in-flight build and letting the rebuild redo it with fresh
config is correct (force-writing would persist output built from stale config). (2)
Item-add force-write can leave a duplicate/gap `Index` (accepted Phase-1 effect): the handler computes the
new index from its stale child list, so if a concurrent replace-all grew the list the item lands at a
now-colliding index (no unique constraint on `PlaylistItem.Index`/`ProgramScheduleItem.Index`) — non-
corrupting, self-correcting on the next edit, still strictly better than the pre-#269 500; a
reload-and-recompute-on-conflict refinement is a candidate for #197. Decision:
**force-write, not 412** — these endpoints take no `If-Match` (an unconditional DELETE/settings-edit should
win over a concurrent editor), matching the Phase-1 force-write posture. A delete has no ETag to rotate, so
it needs only the force-write, not a `Version` bump. A genuine row-deletion race (two concurrent deletes)
still surfaces as a `DbUpdateConcurrencyException` — accepted (rare, non-corrupting, the resource is already
gone). **Still deferred to #197:** *cross-editor ETag rotation* for the non-bumping config siblings and the
scanner-shared `Add*ToCollection` family (they don't 500 — they insert children / `ExecuteDelete`, neither
of which is token-guarded — they just don't rotate an open editor's ETag). Non-vacuously tested by racing a
bump *through the handler* via a pre-tracked context (`RootWriterForceVersionTests`), plus an explicit
negative control proving the plain-save path throws.
## 2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)
The #253 optimistic-concurrency contract (§7a) had a documented tail: the non-If-Match config-sibling
writers of a versioned root mutated editor-visible state **without** bumping `Version`, so editing through
them did not rotate an open editor's ETag (a cross-editor invalidation gap — never a lost-update or a 500,
which the primary endpoints' bump+guard already cover). #269's first slice (PR #302) removed the 500 exposure
by routing those writers through `SaveChangesForcingVersion`; this slice completes the **rotation**.
Handlers now bumping `Version` (all via `SaveChangesForcingVersion`, since they take no `If-Match` → a
concurrent replace-all bump force-writes, never 412/500): the Collection `Add*ToCollection` family (11
handlers) and `RemoveItemsFromCollectionHandler` bump `Collection.Version`; `UpdateCollectionHandler`
(name/flag), `UpdatePlayoutHandler` (`DailyRebuildTime`), and the three `ScheduleFile` writers
(`UpdateSequential`/`UpdateScripted`/`UpdateExternalJsonPlayout`) — which already force-wrote — now also bump.
Decisions frozen (ratified with Fable before implementation, feeding the #197 contract freeze):
- **Rotate on every editor-visible config change, no per-aggregate carve-outs.** §7a's config-only boundary
("every mutating handler of a root's editor-visible config bumps `Version`") already held for Playlist
`Add*`/schedule item writers; the Collection/Playout siblings were an inconsistency, not a judgment call. A
membership add rotating an open custom-order editor's ETag (→ 412 → reload) is correct: its list is genuinely
stale. Blast radius of the aggressive-but-safe rotation is a reload, never data loss.
- **No-op idempotence — the trap Fable caught.** These handlers gate their reindex/`BuildPlayout` fan-out on
`SaveChanges() > 0`. An *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value
re-submit would fire spurious rebuilds across every playout using the aggregate. Fix: short-circuit a genuine
no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes the latent
duplicate-`CollectionItem` insert on a *sequential* re-add; two *concurrent* same-item adds can still both
pass the check and the loser 500s on the composite-PK unique violation — `SaveChangesForcingVersion` catches
only `DbUpdateConcurrencyException`, not `DbUpdateException`. That race is narrow and pre-existing, deferred
to #308), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the
three `ScheduleFile` writers) by `ChangeTracker.HasChanges()`. A no-op neither bumps nor rebuilds nor rotates
the ETag — which is itself correct (nothing changed).
- **The `Add*ToCollection` family is not repository-mediated.** #269's original framing ("repository-mediated,
shared with the scanner hot path") was wrong: `IMediaCollectionRepository` is read-only; each handler loads
the `Collection` into its own `dbContext` and writes directly. So the rotation bump is a pure API-layer
concern and the scanner's separate membership-write path is untouched — a background scan does **not** rotate
the editor ETag (correct: background indexing is not an editor action).
- **Force-write rebases the bump, never adopts the stored token verbatim (Codex review of this PR).**
`SaveChangesForcingVersion` originally resolved a conflict by setting current=original=stored — which
silently *discarded* a sibling's pending `Version++` when a versioned writer committed in its load→save
window (sibling loads 1, bumps to pending 2, concurrent PUT commits 2 → retry wrote 2, so the concurrent
writer's ETag "2" stayed valid and the rotation was lost under exactly the race it exists for). Fixed in
this PR (it affects all 25 bumpers routed through the helper, including the pre-existing playlist/schedule
ones): the retry now rebases — original = stored, current = stored + (pending current pending original) —
so a bumper lands at stored+1 and a non-bumper (delta 0, e.g. `ErasePlayoutHistory`) adopts stored unchanged.
The race tests assert the post-race Version (3, not 2) and fail against the verbatim-adopt implementation.
- **No new status codes.** These endpoints take no `If-Match` and force-write, so they never 412; no
`[ProducesResponseType(...412...)]` and no OpenAPI regen (response types unchanged). Only §7a prose changes.
Tests: `CollectionEtagRotationTests` (rotation + no-op-without-bump-or-rebuild + force-write-past-concurrent-bump
for Add/Remove/Update) and `PlayoutScheduleFileEtagRotationTests` (ScheduleFile rotation + no-op-without-refresh),
the no-op guard proven non-vacuous by inverting the membership check. The `#265` RFC-7232 If-Match parser
refinement (valid-but-non-matching/weak/list → 412 not 400) is a **separate** PR (disjoint surface: the shared
parser + `CheckVersion`, not the handler saves). Refs #253 #269 #197 · `api-conventions.md` §7a.
## 2026-07-12 — If-Match evaluates per RFC 7232: valid-but-non-matching → 412, only grammar violations → 400 (#265)
Closing the last #253 concurrency-contract piece. `ConcurrencyHeaders.ParseIfMatch` previously classified
**any** non-canonical/weak/list `If-Match` value as `Malformed → 400` (a deliberate fail-safe: reject rather
than risk a stale write, deferred from the reference-aggregate PR). That was RFC-incorrect. Per **RFC 7232
§3.1**, a syntactically-valid entity-tag that simply doesn't strong-match must return **412 Precondition
Failed**, and **400** is reserved for a genuine grammar violation.
**What changed.** The parser is now a real RFC 7232 entity-tag/list parser (`If-Match = "*" / 1#entity-tag`).
It **scans** the list (it does *not* `Split(',')` — a comma is a valid `etagc`, so it can appear inside a quoted
opaque-tag: `"3,5"` is ONE tag, and a comma separates members only outside the quotes), trims only RFC OWS
(SP/HTAB — not `string.Trim()`, which would strip NBSP and let `" * "` masquerade as the `*` force-write),
validates each member as `[ "W/" ] DQUOTE *etagc DQUOTE`, and collects the versions of the **strong** members
whose opaque text is the exact canonical decimal we emit. Outcomes:
- **weak** (`W/"3"`), **empty** (`""`), **non-canonical** (`"03"`, `"3.0"`, `"+3"`), **out-of-range**
(`"99999999999999999999"`) → valid tags that contribute no version → **412** (a `Version`-kind with an
*empty* candidate set is a guaranteed no-match).
- **list** (`"3", "5"`) → any strong member that matches proceeds; weak/non-canonical members drop out.
- genuine grammar violations (unquoted `3`, SP inside the tag `" 3 "`, unterminated `"3`, `garbage`, a
separator-only header) → **400**.
**Type reshape.** `IfMatchCondition.ExpectedVersion : Option<int>``ExpectedVersions : Option<Seq<int>>`
(`None` = force-write; `Some(set)` = strong-match against the set, empty ⇒ always 412), and
`VersionedAggregateExtensions.CheckVersion(Option<int>)``CheckVersion(Option<Seq<int>>)` = set membership.
This threads through all 10 replace/update commands + handlers + request mappers + 9 controllers uniformly; no
wire-contract change (400 and 412 were already declared on every PUT; the field is header-derived and internal,
so no OpenAPI/DTO change).
*Why now, not #197:* it is the shared parser all replace-all PUTs copy, and the 412-vs-404 ordering the issue
worried about was already correct (each handler loads/validates → 404 before `CheckVersion`). *Why safe:* the
first-party SPA only ever echoes the single canonical strong tag we emit, so no shipped client changes behavior;
the change only makes a hand-written/tooling `If-Match` get the RFC-correct status. Docs: `api-conventions.md`
§7a. Refs #265 #253 #197.