Completes the cross-editor ETag rotation tail of the #253 optimistic-concurrency contract (§7a) — the last ## Still deferred to #197 item. Ratified with Fable before implementation (feeds the #197 contract freeze).
What
The non-If-Match config-sibling writers mutated a versioned root's editor-visible state without bumping Version, so a concurrent editor of the same root never had its ETag rotated (a cross-editor invalidation gap — never a lost-update or a 500; the primary endpoints' bump+guard already cover those). #269's first slice (PR #302) removed the 500 exposure via SaveChangesForcingVersion; this slice completes the rotation:
Collection (Collection.Version): the 11 Add*ToCollection handlers + RemoveItemsFromCollectionHandler now bump; UpdateCollectionHandler (name/flag) now bumps.
Playout (Playout.Version): UpdatePlayoutHandler (DailyRebuildTime) + the three ScheduleFile writers (UpdateSequential/UpdateScripted/UpdateExternalJson) — which already force-wrote — now bump.
All rotate via SaveChangesForcingVersion (these endpoints take no If-Match, so a concurrent bump force-writes, never 412/500).
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 playout rebuilds. Each now short-circuits a genuine no-op before the bump — the Add handlers by an explicit membership check (which also fixes a latent duplicate-CollectionItem insert on re-add), the scalar writers by ChangeTracker.HasChanges().
Corrections
#269's body claimed the Add*ToCollection family is "repository-mediated, shared with the scanner hot path" — wrong: IMediaCollectionRepository is read-only; each handler writes via its own dbContext, so the bump is a pure API-layer concern and the scanner's separate membership path does not rotate the editor ETag (correct — background indexing isn't an editor action).
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 is proven non-vacuous by inverting the membership check. Full suite green.
Docs
api-conventions.md §7a (the deferred-rotation note → completed) + decisions.md entry. No new status codes / no OpenAPI change — these endpoints take no If-Match and force-write, so they never 412.
The #265 RFC-7232 If-Match parser refinement (valid-but-non-matching/weak/list → 412) is a separate PR (disjoint surface: the shared parser + CheckVersion).
Completes the cross-editor ETag rotation tail of the #253 optimistic-concurrency contract (§7a) — the last `## Still deferred to #197` item. Ratified with Fable before implementation (feeds the #197 contract freeze).
## What
The non-`If-Match` config-sibling writers mutated a versioned root's editor-visible state **without** bumping `Version`, so a concurrent editor of the *same* root never had its ETag rotated (a cross-editor invalidation gap — never a lost-update or a 500; the primary endpoints' bump+guard already cover those). #269's first slice (PR #302) removed the 500 exposure via `SaveChangesForcingVersion`; this slice completes the **rotation**:
- **Collection** (`Collection.Version`): the 11 `Add*ToCollection` handlers + `RemoveItemsFromCollectionHandler` now bump; `UpdateCollectionHandler` (name/flag) now bumps.
- **Playout** (`Playout.Version`): `UpdatePlayoutHandler` (`DailyRebuildTime`) + the three `ScheduleFile` writers (`UpdateSequential`/`UpdateScripted`/`UpdateExternalJson`) — which already force-wrote — now bump.
All rotate via `SaveChangesForcingVersion` (these endpoints take no `If-Match`, so a concurrent bump force-writes, never 412/500).
## 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 playout rebuilds. Each now short-circuits a genuine no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes a latent duplicate-`CollectionItem` insert on re-add), the scalar writers by `ChangeTracker.HasChanges()`.
## Corrections
#269's body claimed the `Add*ToCollection` family is "repository-mediated, shared with the scanner hot path" — **wrong**: `IMediaCollectionRepository` is read-only; each handler writes via its own `dbContext`, so the bump is a pure API-layer concern and the scanner's separate membership path does **not** rotate the editor ETag (correct — background indexing isn't an editor action).
## 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 is proven **non-vacuous** by inverting the membership check. Full suite green.
## Docs
`api-conventions.md` §7a (the deferred-rotation note → completed) + `decisions.md` entry. **No new status codes / no OpenAPI change** — these endpoints take no `If-Match` and force-write, so they never 412.
The #265 RFC-7232 `If-Match` parser refinement (valid-but-non-matching/weak/list → 412) is a **separate** PR (disjoint surface: the shared parser + `CheckVersion`).
fixes #269
Independent review loop on the current head (fix commit a88284fd), per the #242 re-review-the-fix-commit discipline:
Cold zero-context fork review of the initial diff → MERGEABLE.
Codex review of the initial diff → BLOCKED (F1: SaveChangesForcingVersion dropped the pending Version++ under a concurrent versioned-write race).
Fable reconciled the fork-vs-Codex disagreement → Codex right on all 3; applied the delta-rebase fix + strengthened tests (negative-controlled: revert fix → race-rotation tests go red) + deferred F2 to #308.
Codex re-review of the fix commit a88284fd → F1 CLOSED, MERGEABLE, no new findings (verified the rebase advances a bumper past the racing writer, non-bumpers unchanged, idempotent across the retry loop, tests pin the fix).
CI green on a88284fd (Build & test, EF migration integrity, api-docs-in-sync, decisions.md append-only, docs-reminder). 1522/1522 local tests.
Review-verdict: MERGEABLE @ a88284fdafafa0c6ef23f01411c8ebe8656ac7c5
Independent review loop on the current head (fix commit `a88284fd`), per the #242 re-review-the-fix-commit discipline:
- **Cold zero-context fork review** of the initial diff → MERGEABLE.
- **Codex** review of the initial diff → BLOCKED (F1: `SaveChangesForcingVersion` dropped the pending `Version++` under a concurrent versioned-write race).
- **Fable** reconciled the fork-vs-Codex disagreement → Codex right on all 3; applied the delta-rebase fix + strengthened tests (negative-controlled: revert fix → race-rotation tests go red) + deferred F2 to #308.
- **Codex re-review of the fix commit `a88284fd`** → **F1 CLOSED, MERGEABLE, no new findings** (verified the rebase advances a bumper past the racing writer, non-bumpers unchanged, idempotent across the retry loop, tests pin the fix).
CI green on `a88284fd` (Build & test, EF migration integrity, api-docs-in-sync, decisions.md append-only, docs-reminder). 1522/1522 local tests.
Rebased onto current main (2879c012, which landed the #303 H9/H10 process hooks) to keep a linear history and enable a fast-forward merge — replacing the two prior merge commits. The reviewed content is unchanged: the app-code diff and the §7a docs are byte-identical to the previously-reviewed fix commit a88284fd (verified: full build + 1522/1522 tests green on the rebased tree; main's only delta is the process hooks/decisions.md TOC, not app code). The prior verdict (Review-verdict: MERGEABLE @ a88284fd, fork + Codex + Codex-fix-re-review) therefore carries to this head. decisions.md remains a pure insertion (0 deletions) vs main.
Review-verdict: MERGEABLE @ 7a9b30de714d87d102dd8cdbb96f00fa748e9a49
Rebased onto current `main` (`2879c012`, which landed the #303 H9/H10 process hooks) to keep a linear history and enable a fast-forward merge — replacing the two prior merge commits. **The reviewed content is unchanged**: the app-code diff and the §7a docs are byte-identical to the previously-reviewed fix commit `a88284fd` (verified: full build + 1522/1522 tests green on the rebased tree; `main`'s only delta is the process hooks/`decisions.md` TOC, not app code). The prior verdict (`Review-verdict: MERGEABLE @ a88284fd`, fork + Codex + Codex-fix-re-review) therefore carries to this head. `decisions.md` remains a pure insertion (0 deletions) vs main.
timothy
merged commit 7a9b30de71 into main2026-07-12 19:50:12 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Completes the cross-editor ETag rotation tail of the #253 optimistic-concurrency contract (§7a) — the last
## Still deferred to #197item. Ratified with Fable before implementation (feeds the #197 contract freeze).What
The non-
If-Matchconfig-sibling writers mutated a versioned root's editor-visible state without bumpingVersion, so a concurrent editor of the same root never had its ETag rotated (a cross-editor invalidation gap — never a lost-update or a 500; the primary endpoints' bump+guard already cover those). #269's first slice (PR #302) removed the 500 exposure viaSaveChangesForcingVersion; this slice completes the rotation:Collection.Version): the 11Add*ToCollectionhandlers +RemoveItemsFromCollectionHandlernow bump;UpdateCollectionHandler(name/flag) now bumps.Playout.Version):UpdatePlayoutHandler(DailyRebuildTime) + the threeScheduleFilewriters (UpdateSequential/UpdateScripted/UpdateExternalJson) — which already force-wrote — now bump.All rotate via
SaveChangesForcingVersion(these endpoints take noIf-Match, so a concurrent bump force-writes, never 412/500).No-op idempotence (the trap Fable caught)
These handlers gate their reindex /
BuildPlayoutfan-out onSaveChanges() > 0. An unconditional bump makes that gate always-true, so an idempotent re-add / same-value re-submit would fire spurious playout rebuilds. Each now short-circuits a genuine no-op before the bump — the Add handlers by an explicit membership check (which also fixes a latent duplicate-CollectionIteminsert on re-add), the scalar writers byChangeTracker.HasChanges().Corrections
#269's body claimed the
Add*ToCollectionfamily is "repository-mediated, shared with the scanner hot path" — wrong:IMediaCollectionRepositoryis read-only; each handler writes via its owndbContext, so the bump is a pure API-layer concern and the scanner's separate membership path does not rotate the editor ETag (correct — background indexing isn't an editor action).Tests
CollectionEtagRotationTests(rotation · no-op-without-bump-or-rebuild · force-write-past-concurrent-bump for Add/Remove/Update) andPlayoutScheduleFileEtagRotationTests(ScheduleFile rotation · no-op-without-refresh). The no-op guard is proven non-vacuous by inverting the membership check. Full suite green.Docs
api-conventions.md§7a (the deferred-rotation note → completed) +decisions.mdentry. No new status codes / no OpenAPI change — these endpoints take noIf-Matchand force-write, so they never 412.The #265 RFC-7232
If-Matchparser refinement (valid-but-non-matching/weak/list → 412) is a separate PR (disjoint surface: the shared parser +CheckVersion).fixes #269
Review-verdict: MERGEABLE @ a88284fdafafa0c6ef23f01411c8ebe8656ac7c5
Independent review loop on the current head (fix commit
a88284fd), per the #242 re-review-the-fix-commit discipline:SaveChangesForcingVersiondropped the pendingVersion++under a concurrent versioned-write race).a88284fd→ F1 CLOSED, MERGEABLE, no new findings (verified the rebase advances a bumper past the racing writer, non-bumpers unchanged, idempotent across the retry loop, tests pin the fix).CI green on
a88284fd(Build & test, EF migration integrity, api-docs-in-sync, decisions.md append-only, docs-reminder). 1522/1522 local tests.a88284fdafto7a9b30de71Review-verdict: MERGEABLE @
7a9b30de71Rebased onto current
main(2879c012, which landed the #303 H9/H10 process hooks) to keep a linear history and enable a fast-forward merge — replacing the two prior merge commits. The reviewed content is unchanged: the app-code diff and the §7a docs are byte-identical to the previously-reviewed fix commita88284fd(verified: full build + 1522/1522 tests green on the rebased tree;main's only delta is the process hooks/decisions.mdTOC, not app code). The prior verdict (Review-verdict: MERGEABLE @ a88284fd, fork + Codex + Codex-fix-re-review) therefore carries to this head.decisions.mdremains a pure insertion (0 deletions) vs main.