feat(484): suppress the media-server sweep on projection failures; reject the ratio threshold #612

2026-07-25T15:36:17+02:00

timothy commented

2026-07-25 15:36:17 +02:00

fixes #484

#477 added MediaServerReconciliationGuard, which refuses a library sweep when the incoming set is exactly empty. #484 deferred two extensions. This resolves both.

Decision 1 — the ratio / missing-fraction threshold is REJECTED

It is a two-sided heuristic: set low it silently suppresses legitimate bulk deletions, set high it misses the partial fetch it exists for, and there is no telemetry to tune it with. Decisively, the failure it approximates is exactly observable by other means (below), and a genuine bulk deletion produces zero projection failures — so the deterministic signal has no false positives where the heuristic has unbounded ones. Accepting an unbounded false-suppression risk to approximate something we can measure exactly is a bad trade.

Decision 2 — projection-failure detection is IMPLEMENTED

The API clients mapped each server item through a private ProjectTo* and dropped None results via .Somes(). A drop caused by a swallowed exception is indistinguishable from a deletion downstream, so one projection regression could mass-flag a healthy library — and EmptyTrashHandler deletes FileNotFound rows permanently.

The load-bearing distinction: .Somes() was hiding two different things.

Deliberate skips — guard-clause None, no exception (STRM files, LocationType non-FileSystem/Virtual, unknown item types). Permanent and expected.
Failure drops — catch (Exception) { LogWarning; return None; }. These are the ones indistinguishable from a deletion.

Only the second class suppresses the sweep. Conflating them would have permanently disabled reconciliation for any library containing a single STRM file — a regression, not a safe default. Deliberate_Skips_Do_Not_Suppress_The_Sweep is the guard on that.

Emby's skips differ from Jellyfin's (MediaSources empty, LocationType == "Virtual" — not STRM). Plex needs no counting at all: its projections return bare entities with no Option and no catch, so a bad item throws and unwinds.

Shape

MediaServerProjectionResult<T> three-states each private mapper, so there is no public churn. A MediaServerProjectionFailureCounter is an optional trailing param on only the enumerations that feed a sweep, so no other call site changed. Deliberately not done: widening IAsyncEnumerable<Tuple<T,int>>, which appears in ~90 signatures across the Core interfaces, three clients and ~15 scanners — disproportionate and risky for this issue.

Review — first pass BLOCKED, and it was right

The cold reviewer confirmed the mechanism correct and not inert, then found that the TV season and episode sweeps were still unguarded. The deferral had been inherited from #477, whose rationale is "blast radius is one show's seasons / one season's episodes." That holds for a per-parent empty fetch. It does not hold for a projection failure, which is systematic by construction — one bad code path fires on every parent, so every season enumerates zero episodes and the whole episode library is swept in a single scan.

Fixed: the counter is threaded into the season/episode enumerations for Jellyfin and Emby, with a per-show counter for the season sweep and a per-season counter for the episode sweep. All six sweeps are now guarded.

Deviation, deliberate and verified. The nested sweeps use a second entry point, ShouldFlagMissingDescendants, rather than ShouldFlagMissing. Importing #477's empty-fetch branch at those levels would have silently reversed #477's scope decision and broken #476's cascade, which depends on an emptied parent still sweeping its children. Both entry points delegate to one private static RefuseForProjectionFailures, so the failure predicate cannot drift. The reviewer verified the #476 dependency against the source rather than accepting the claim.

Re-review verdict MERGEABLE @ 35de3b60, with the join tests confirmed to catch a rewiring to a fresh counter at five separate places.

Documented rather than engineered around, in the decision record:

Emby's MediaSources-empty skip is response-shape-dependent, unlike Jellyfin's genuinely permanent guards. A Refit/DTO drift presents as a mass Skip, which leaves the sweep enabled; a total one is caught by #477's zero branch, a partial one is not. Re-classifying it would reintroduce STRM-style permanent suppression, so it is stated as a residual limitation.
The WithLibrary fail-closed branch is unreachable on every counted path — kept as defensive, but the comment no longer presents it as active protection.
Log-contract note: when both refusals apply, only the #484 message is emitted, not #477's "returned zero items" line.

Verification

Scanner.Tests 1504, Infrastructure.Tests 114, ErsatzTV.Tests 1873, Core.Tests 652 (+1 skipped), Architecture.Tests 5 — all 0 failed, re-run green after rebasing on origin/main. Negative controls run in both polarities. decisions_validate OK, catalog regenerated, BOM clean, format gate clean.

No live-E2E: this changes scanner reconciliation logic, not an API write-path handler or UI, and the failure path requires a projection regression that cannot be induced against a real server without shipping broken code. Exercised by substituted-throw client tests instead. Follow-up filed as #604 (JellyfinCollectionScanner's remove-all-then-re-add over GetCollectionItems has the same seam, uncounted).

fixes #484 #477 added `MediaServerReconciliationGuard`, which refuses a library sweep when the incoming set is *exactly* empty. #484 deferred two extensions. This resolves both. ## Decision 1 — the ratio / missing-fraction threshold is REJECTED It is a two-sided heuristic: set low it silently suppresses legitimate bulk deletions, set high it misses the partial fetch it exists for, and there is no telemetry to tune it with. Decisively, the failure it approximates is **exactly observable** by other means (below), and a genuine bulk deletion produces zero projection failures — so the deterministic signal has no false positives where the heuristic has unbounded ones. Accepting an unbounded false-suppression risk to approximate something we can measure exactly is a bad trade. ## Decision 2 — projection-failure detection is IMPLEMENTED The API clients mapped each server item through a private `ProjectTo*` and dropped `None` results via `.Somes()`. A drop caused by a swallowed exception is indistinguishable from a deletion downstream, so one projection regression could mass-flag a healthy library — and `EmptyTrashHandler` deletes `FileNotFound` rows permanently. **The load-bearing distinction:** `.Somes()` was hiding two different things. - **Deliberate skips** — guard-clause `None`, no exception (STRM files, `LocationType` non-FileSystem/Virtual, unknown item types). Permanent and expected. - **Failure drops** — `catch (Exception) { LogWarning; return None; }`. These are the ones indistinguishable from a deletion. Only the second class suppresses the sweep. Conflating them would have permanently disabled reconciliation for any library containing a single STRM file — a regression, not a safe default. `Deliberate_Skips_Do_Not_Suppress_The_Sweep` is the guard on that. Emby's skips differ from Jellyfin's (`MediaSources` empty, `LocationType == "Virtual"` — not STRM). Plex needs no counting at all: its projections return bare entities with no `Option` and no `catch`, so a bad item throws and unwinds. ### Shape `MediaServerProjectionResult<T>` three-states each **private** mapper, so there is no public churn. A `MediaServerProjectionFailureCounter` is an **optional trailing param** on only the enumerations that feed a sweep, so no other call site changed. Deliberately *not* done: widening `IAsyncEnumerable<Tuple<T,int>>`, which appears in ~90 signatures across the Core interfaces, three clients and ~15 scanners — disproportionate and risky for this issue. ## Review — first pass BLOCKED, and it was right The cold reviewer confirmed the mechanism correct and not inert, then found that the **TV season and episode sweeps were still unguarded**. The deferral had been inherited from #477, whose rationale is *"blast radius is one show's seasons / one season's episodes."* That holds for a **per-parent empty fetch**. It does not hold for a projection failure, which is **systematic by construction** — one bad code path fires on every parent, so every season enumerates zero episodes and the whole episode library is swept in a single scan. Fixed: the counter is threaded into the season/episode enumerations for Jellyfin and Emby, with a **per-show** counter for the season sweep and a **per-season** counter for the episode sweep. All six sweeps are now guarded. **Deviation, deliberate and verified.** The nested sweeps use a second entry point, `ShouldFlagMissingDescendants`, rather than `ShouldFlagMissing`. Importing #477's empty-fetch branch at those levels would have silently reversed #477's scope decision *and* broken #476's cascade, which depends on an emptied parent still sweeping its children. Both entry points delegate to one `private static RefuseForProjectionFailures`, so the failure predicate cannot drift. The reviewer verified the #476 dependency against the source rather than accepting the claim. Re-review verdict **MERGEABLE @ 35de3b60**, with the join tests confirmed to catch a rewiring to a fresh counter at five separate places. Documented rather than engineered around, in the decision record: - Emby's `MediaSources`-empty skip is **response-shape-dependent**, unlike Jellyfin's genuinely permanent guards. A Refit/DTO drift presents as a mass Skip, which leaves the sweep enabled; a *total* one is caught by #477's zero branch, a **partial** one is not. Re-classifying it would reintroduce STRM-style permanent suppression, so it is stated as a residual limitation. - The `WithLibrary` fail-closed branch is unreachable on every counted path — kept as defensive, but the comment no longer presents it as active protection. - Log-contract note: when both refusals apply, only the #484 message is emitted, not #477's "returned zero items" line. ## Verification Scanner.Tests **1504**, Infrastructure.Tests **114**, ErsatzTV.Tests **1873**, Core.Tests **652** (+1 skipped), Architecture.Tests **5** — all 0 failed, re-run green after rebasing on `origin/main`. Negative controls run in both polarities. `decisions_validate` OK, catalog regenerated, BOM clean, format gate clean. No live-E2E: this changes scanner reconciliation logic, not an API write-path handler or UI, and the failure path requires a projection regression that cannot be induced against a real server without shipping broken code. Exercised by substituted-throw client tests instead. Follow-up filed as #604 (`JellyfinCollectionScanner`'s remove-all-then-re-add over `GetCollectionItems` has the same seam, uncounted).

timothy commented

2026-07-25 15:38:52 +02:00

Independent cold review-only pass, two rounds.

Round 1 returned BLOCKED on a real scope gap: the mechanism was confirmed correct and not inert everywhere it reached, but the TV season and episode sweeps were still unguarded, on a deferral rationale inherited from #477 ("blast radius is one show") that does not survive a systematic projection failure. Round 2 over the fix commit returned MERGEABLE @ 35de3b60, having independently verified the ShouldFlagMissingDescendants deviation against #476's cascade rather than accepting the claim, confirmed per-show/per-season counter scoping, and confirmed the join tests would catch a rewiring to a fresh counter at five separate places.

Delta since that reviewed sha: one commit, docs(484), correcting two interface comments that still said "three"/"two" counted enumerations after the nested ones were wired — the reviewer's own Low finding. Comment text only, no code, no test change; I made that edit and verified BOM, format gate and a clean ErsatzTV.Core build. Everything the reviewer examined is otherwise unchanged.

Reviewer-family note for the audit trail: process.independent-review-rubric prefers a different model family than the implementer. The Codex CLI is not installed in this environment, so the rubric's stated fallback was used — a fresh agent with no implementation role, cold-briefed. Recording the exemption rather than leaving it silent.

Branch rebased onto abeef63f; the docs/decisions.md conflict was append/append against #445's record and was resolved by keeping both and regenerating the catalog (no drift). Re-verified at head: Scanner.Tests 1504, Infrastructure.Tests 114, Core.Tests 652 (+1 skipped), all 0 failed.

Review-verdict: MERGEABLE @ 88165ec7

Independent cold review-only pass, two rounds. Round 1 returned BLOCKED on a real scope gap: the mechanism was confirmed correct and not inert everywhere it reached, but the TV season and episode sweeps were still unguarded, on a deferral rationale inherited from #477 ("blast radius is one show") that does not survive a systematic projection failure. Round 2 over the fix commit returned MERGEABLE @ 35de3b60, having independently verified the `ShouldFlagMissingDescendants` deviation against #476's cascade rather than accepting the claim, confirmed per-show/per-season counter scoping, and confirmed the join tests would catch a rewiring to a fresh counter at five separate places. Delta since that reviewed sha: one commit, `docs(484)`, correcting two interface comments that still said "three"/"two" counted enumerations after the nested ones were wired — the reviewer's own Low finding. Comment text only, no code, no test change; I made that edit and verified BOM, format gate and a clean `ErsatzTV.Core` build. Everything the reviewer examined is otherwise unchanged. Reviewer-family note for the audit trail: `process.independent-review-rubric` prefers a different model family than the implementer. The Codex CLI is not installed in this environment, so the rubric's stated fallback was used — a fresh agent with no implementation role, cold-briefed. Recording the exemption rather than leaving it silent. Branch rebased onto `abeef63f`; the `docs/decisions.md` conflict was append/append against #445's record and was resolved by keeping both and regenerating the catalog (no drift). Re-verified at head: Scanner.Tests 1504, Infrastructure.Tests 114, Core.Tests 652 (+1 skipped), all 0 failed. Review-verdict: MERGEABLE @ 88165ec7

timothy force-pushed feat/484-scanner-guard-threshold from 88165ec742 to 7cc12881de

2026-07-25 16:37:32 +02:00

Compare

timothy commented

2026-07-25 16:37:44 +02:00

Rebased onto fd70e6eb after #605 and #611 landed. #605 changed both docs/decisions.md and the decisions schema tooling (optional stale-after / Sources metadata), so the catalog was regenerated with the new build_decisions_catalog.py rather than carrying the old generated output forward. The docs/decisions.md conflict was again append/append — both records kept, no hunk lost.

No code changed in the rebase; the only edits were the conflict resolution and the regenerated catalog. Re-verified at the new head: decisions_validate OK, catalog in sync (clean git status after regeneration), ErsatzTV.Scanner.Tests 1504 passed / 0 failed.

The review substance is unchanged from the MERGEABLE @ 35de3b60 pass plus the docs(484) comment correction already described above.

Review-verdict: MERGEABLE @ 7cc12881

Rebased onto `fd70e6eb` after #605 and #611 landed. #605 changed both `docs/decisions.md` and the decisions schema tooling (optional `stale-after` / `Sources` metadata), so the catalog was regenerated with the **new** `build_decisions_catalog.py` rather than carrying the old generated output forward. The `docs/decisions.md` conflict was again append/append — both records kept, no hunk lost. No code changed in the rebase; the only edits were the conflict resolution and the regenerated catalog. Re-verified at the new head: `decisions_validate` OK, catalog in sync (clean `git status` after regeneration), `ErsatzTV.Scanner.Tests` 1504 passed / 0 failed. The review substance is unchanged from the MERGEABLE @ 35de3b60 pass plus the `docs(484)` comment correction already described above. Review-verdict: MERGEABLE @ 7cc12881

timothy scheduled this pull request to auto merge when all checks succeed 2026-07-25 16:37:50 +02:00

timothy commented

2026-07-25 16:39:52 +02:00

Correction to the reviewer-family note above, for the audit trail.

I claimed the Codex CLI was unavailable in this environment and invoked process.independent-review-rubric's fallback. That claim was wrong. A single codex:codex-rescue dispatch failed early in this session with an install error, and I generalized from it; the same agent was verified working elsewhere the same day and did find a real Blocker on another PR. The failure was transient or pre-install, not a property of the environment.

So the exemption recorded above was not actually warranted — a genuine cross-family reviewer was reachable and I should have retried rather than writing the capability off after one failed dispatch. The two cold same-family passes that did run were substantive (the first returned BLOCKED on the nested TV sweeps), so I do not think the conclusion here is wrong, but the process note should not stand as written.

Doing it properly on #613, which is the riskiest of the three and still open: a cross-family Codex review is running there now.

Correction to the reviewer-family note above, for the audit trail. I claimed the Codex CLI was unavailable in this environment and invoked `process.independent-review-rubric`'s fallback. That claim was wrong. A single `codex:codex-rescue` dispatch failed early in this session with an install error, and I generalized from it; the same agent was verified working elsewhere the same day and did find a real Blocker on another PR. The failure was transient or pre-install, not a property of the environment. So the exemption recorded above was not actually warranted — a genuine cross-family reviewer was reachable and I should have retried rather than writing the capability off after one failed dispatch. The two cold same-family passes that did run were substantive (the first returned BLOCKED on the nested TV sweeps), so I do not think the conclusion here is wrong, but the process note should not stand as written. Doing it properly on #613, which is the riskiest of the three and still open: a cross-family Codex review is running there now.

timothy merged commit eb4de0cc2b into main

2026-07-25 17:08:08 +02:00

timothy deleted branch feat/484-scanner-guard-threshold

2026-07-25 17:08:08 +02:00

timothy referenced this issue from a commit

2026-07-25 17:08:09 +02:00

Merge pull request 'feat(484): suppress the media-server sweep on projection failures; reject the ratio threshold' (#612) from feat/484-scanner-guard-threshold into main

timothy referenced this pull request

2026-07-25 17:09:56 +02:00

Media-server scanner: ratio-threshold + projection-failure detection for the empty-fetch guard #484

timothy referenced this issue from a commit

2026-07-25 17:28:04 +02:00