Three Low findings from the #536 clamp re-review, unreachable today (one
guarded release site) but filed against the day a second release site is added.
- §1: Release() now reads the count and CAS-decrements only when current > 0,
so it never publishes a negative count even transiently. The prior
decrement-first-then-clamp shape dipped to -1, which a concurrent TryAcquire
could read as phantom room and over-admit at the limit (re-opening the #529
QSV pool exhaustion). It records the unbalanced release synchronously on the
offending thread rather than blaming a later innocent release.
- §3: Release() returns bool; HlsSessionWorker logs a warning on the false
(unbalanced) return — the one in-band signal a future second release site
would need. WorkAheadSlots stays logger-free by design.
- §2: UnbalancedReleases doc-comment corrected — it can under-count (an
over-release while count > 0 cancels a coexisting leak and goes unrecorded);
no false positives, but zero does not prove correctness.
Test: Release_Unbalanced_NeverPublishesNegativeCount (2M unbalanced releases vs
4 count-samplers) with a documented, verified negative control (reverting to
the decrement-first body makes readers observe the transient -1).
Adds a decisions.md entry (ffmpeg.work-ahead-slot-release-never-negative).
fixes#539
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill still described archived upstream v26.3.0: "MediatR + Blazor (not
REST for mutations)", "No REST CRUD for channels/collections/schedules —
must use SQLite DB directly", and the ghcr.io image. All false for this
fork — Blazor was removed in #91b and /api/v1 has full write paths. A
session trusting it would hand-edit SQLite for something the API does.
Also adds what this session had to discover by hand while driving the API:
the key is root-owned so an unsudo'd read fails SILENTLY (empty header ->
401 body that parses as a dict -> a naive script reports "0 channels"
rather than an auth error); settings live at /api/v1/settings/ffmpeg, not
ffmpeg/settings; test is port 8410 and the service-scoped manual refresh
command for validating before the 03:00 auto-update.
Two measurement traps recorded with them: OCI labels are inherited from the
linuxserver base image and lie about what is running (compare .Image to the
registry Docker-Content-Digest), and container log brackets are local time
while `docker logs -t` is UTC, so --since windows mis-slice.
Endpoint lists now defer to docs/endpoint-index.md rather than being
re-maintained here, since a hand-copied list is what drifted in the first
place.
The decisions-guard validator enforced lifecycle metadata (key/status/since/
supersedes/superseded-by + reciprocal links) but not the **Signals:** line —
which is exactly what MemPalace's keyword recall matches on. A record without
it ingests with weak recall metadata and produces confident false-negatives
for the "MemPalace to find, file to confirm" retrieval workflow.
Add "signals" to REQUIRED_META so a migrated record with a missing or empty
**Signals:** line fails the same way a missing key does. All 114 active + 2
archive records already carry a Signals line, so this is non-breaking on the
current corpus. Archive records are intentionally out of scope (recall targets
the active corpus).
- scripts/decisions_validate.py: signals in REQUIRED_META (+ rationale comment)
- scripts/tests/test_decisions_validate.py: _rec() default + missing/empty/present cases
- docs/decisions.md: header + Enforcement note the requirement and why
fixes#545
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Timothy asked why the kickoff handoff doc stores historical narrative when
it should be instructions. It shouldn't — its own lore section is chartered
as "STANDING workflow/orchestration rules only" with the why belonging in
docs/decisions.md. But an inventory of every bullet against the decision
corpus inverted the premise: only ~8 of ~38 were actually covered. 19 had
no record anywhere and 11 more were half-covered, so that single file was
the ONLY copy of the mandatory review rubric, the whole CI-triage
vocabulary, the build-concurrency policy, the H12 session-end audit, and
the plumbing-merge recipe. Pruning first would have destroyed them.
So the records come first. New topic file docs/decisions/workflow-process.md
carries 32 records (ci.*, process.*, testing.*) covering every NONE and
PARTIAL the inventory found, including the Gitea `?milestones=` no-op bug
whose only copy was the archived selector section this prune deletes.
Only then the prune: HARD CONSTRAINTS and the lore section become one- or
two-line rules, each citing the decision key that holds its evidence, and
the 40-line "Archived — do not follow" section is gone. 636 -> 353 lines,
with every cited key verified to resolve against the corpus.
The aggregate corpus budget is re-baselined 4800 -> 5600 with the reason in
the code: the corpus grew because knowledge MOVED into it, which is the
system working, not drift.
refs #542
A session was handed docs/handoffs/chicorytv-issue-queue.md pasted out of
/Users/timothy/ersatztv while that tree was 81 commits behind, so it still
described the queue protocol #520 retired the day before (read tracker
command was ever run against that tree, so every existing "never read its
HEAD" guard was irrelevant: a stale checkout serves stale FILES, and docs
are what a kickoff depends on. Nothing broke only because selection went
through scripts/select-queue.sh.
The lore bullet on that tree already prescribed the shape of the fix for
its earlier failure modes — "a design flaw, not a discipline failure; a
check does not stay true" — so this removes the stale condition instead of
adding another check.
scripts/refresh-shared-checkout.sh fast-forwards the tree to origin/main
and reinstalls web/node_modules when the lockfile moved. It is deliberately
timid: it refuses and changes nothing when the tree is not on main, is
dirty, is ahead, or is mid-rebase/merge, and it never switches branches,
stashes or discards. A NO-OP is a normal outcome.
Uses npm ci rather than npm install — the first version used install,
which rewrote package-lock.json and left the tree dirty, i.e. the exact
state the next run refuses on, so it would have disabled itself after one
use. Asserts the tree is clean at exit.
refs #541
An on-demand channel (`PlayoutMode.OnDemand`) already is the "resume where I
left off" feature: `Playout.OnDemandCheckpoint` persists the viewer's position
and `PlayoutTimeShifter.TimeShift` slides the materialized timeline forward on
tune-in so the paused item is active again. Because it rewrites `GuideStart`/
`GuideFinish` alongside `Start`/`Finish`, guide and playback freeze together —
structurally avoiding the free-running-wall-clock desync #68 was filed about.
The one gap: `TimeShift` rewrote the stored `PlayoutItem` rows but the XMLTV
guide is served from a cached fragment that only `RefreshChannelData` rebuilds,
and the tune-in path never enqueued it. So an external EPG client polling after
a thaw could see a stale timeline until the next incidental rebuild.
Fix: `IPlayoutTimeShifter.TimeShift` now returns the channel numbers whose cached
guide is stale — the shifted channel plus any channels that mirror it (the same
fan-out `BuildPlayoutHandler` already does) — and `TimeShiftOnDemandPlayoutHandler`
enqueues a `RefreshChannelData` for each on `CancellationToken.None` (post-commit
side effect must not be abandoned if the session token cancels).
Tests: handler enqueues a rebuild per stale channel (+ mirror + no-shift cases);
`PlayoutTimeShifter` reports source+mirrors on a shift, empty on Continuous /
zero-offset / active-unforced, and correctly seeds+shifts a never-watched playout.
Non-vacuity of the enqueue proven by a compiling negative control.
Docs: channels.md (On-demand resume section), domain-model.md, decisions.md
(scheduling.ondemand-guide-refresh-on-thaw). Per-viewer resume is out of scope
(single per-channel checkpoint; #68 says per-channel suffices).
fixes#68
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-push adversarial review, Low finding. The pool is process-wide and
lives for the life of the app, so a `Release()` not matched by a
successful `TryAcquire` would drive the count to -1 permanently: with a
limit of 1 that silently admits two unthrottled transcodes forever, which
is exactly the #529 QSV pool exhaustion with nothing in the logs to find
it by. Clamp at zero and record the breakage in `UnbalancedReleases`
rather than throwing — the sole caller releases from a `finally`, where a
throw would swallow the real exception.
The hammer tests now also assert `UnbalancedReleases == 0`, so the clamp
cannot mask drift it was added to survive.
Also moves the #536 index line to the end of the in-file decisions index
(it was inserted in the 2026-07-11 block while its body appends at the
end) — review nit, anchors were already correct.
refs #536
The slot check and its increment straddled an await: `Run` compared
`Volatile.Read(ref _workAheadCount)` against a DB-backed limit, and the
increment happened later inside `Transcode`. Every simultaneous tune-in
therefore observed `0 < limit` and started unthrottled — three concurrent
tunes on prod with a limit of 1 all ran with no `-readrate`. `Interlocked`
on the write side alone buys nothing when the read side is a separate,
earlier load (same class as #231/#250).
Extract the counter into a `WorkAheadSlots` pool whose `TryAcquire(limit)`
claims via compare-exchange, so the count never even transiently exceeds
the limit that the QSV hardware-frame pool sizing (#529) is derived from.
`Run` claims the slot and passes ownership in; `Transcode(bool
ownsWorkAheadSlot, ...)` derives `realtime` from it and releases it in its
existing `finally`, keeping acquire/release one-for-one. Acquisition stays
in the caller because `Run` sets `_state` from the outcome and `Transcode`
reads that state on entry to pick the item start time.
Tests hammer 8 threads x 20k rounds (a single Barrier round does not
collide on this hardware); the documented negative control reinstates the
check-then-act body and produces 15912 over-claiming rounds of 20000.
Also annotates the #350 decision record, whose "every concurrent tune-in
falls back to the throttled path" bullet described the intent rather than
the behaviour.
fixes#536
Three PR-only git-diff gates (ci-image-pin, docs-reminder, decisions-guard)
lived in docker-build.yml, which also triggers on push to main and v* tags.
Gitea dispatches a job as a runner task even when its `if` skips it, so on
every tag/main push these three were dispatched to the `small` lane just to
evaluate the skip. On the v26.12.0 tag those dispatched skip-tasks wedged in
act's setup phase and were killed by a runner restart mid-setup, reporting
`failure` (no logs) and reddening the tag's overall commit status even though
the release built, scanned, and deployed fine. The two identical-`if:` jobs on
ubuntu-latest (api-docs, format) skipped cleanly — the job logic was never the
problem; the kill lands in the dispatch window before any step or skip runs, so
tweaking the `if:`/step logic could not fix it.
Relocate exactly those three (pure checkout + git-diff, no container:, no image
pin) verbatim into a dedicated pr-checks.yml that triggers `on: pull_request`
only. Gitea evaluates a workflow's trigger before creating any job, so on a
tag/main push this workflow produces zero jobs: no dispatch, no kill, no
spurious red — for the whole class, permanently.
- ci-image-pin carries no pin and still greps docker-build.yml, where all five
pin-bearing jobs (test/migrations/functional-e2e/api-docs/format) remain, so
its validation is unchanged.
- None of the three are required checks (only Build & test + EF migration
integrity are), so the status-context prefix change (Build ErsatzTV Image / ...
-> PR Gates / ...) does not affect merges; the merge-consent hook reads the
prefix-agnostic combined status.
- pr-checks.yml declares `defaults: run: shell: bash` (ci-image-pin uses
mapfile / set -o pipefail).
docs/ci-cd.md: new "PR gates workflow" section + cross-references.
fixes#535
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Exhaustive triage of the closed tracker ersatztv#237, whose 111 comments
server-management#642 excludes from MemPalace ingestion (over the per-file
cap) and #520 removes from startup. #524's premise was that facts living only
in those comments would be orphaned and need curating into lifecycle records.
Result: zero decision-shaped orphans. Every durable decision-shaped fact is
already held by the decision corpus or by the individual issue the comment
narrates -- which the exporter does ingest. The tracker was always the lossy
copy, because the session protocol required the fuller closing record on the
worked issue first.
- docs/decisions.md: new active record docs.tracker-comment-retrofit, leading
with the reusable rule (check the worked issue BEFORE the decision corpus)
and the consequence for #642's benchmark row, which has no valid subject.
- docs/tracker-retrofit-triage-237.md: the audit trail -- method, per-comment
classification of all 111, totals, and the one candidate raised and
disproved (#497's Guids/Directors scope, stated more fully on its own issue).
- docs/handoffs/chicorytv-issue-queue.md: sweeps the two genuinely orphaned
LORE facts the triage surfaced (e2e-local.sh readiness probe hanging on a
reused config dir; troubleshooting playback cannot exercise channel branding).
- docs/README.md: index the new doc.
The claim is deliberately narrow: no *decision-shaped* orphans. The lore bucket
was classified but not coverage-checked, and it was not empty -- hence the sweep.
fixes#524
Second review returned MERGEABLE with one Medium and three Lows. Addressed all four:
- Medium: the save-time normalization had zero test coverage, so a later refactor
dropping Math.Max would leave the suite green (the FFmpegState floor keeps the
pipeline correct, hiding the regression until someone reads a stored 0 back).
Added Create/Update_Should_Floor_QsvExtraHardwareFrames over 0, -8, 63, 64 and 128,
plus Create_Should_Leave_Null_QsvExtraHardwareFrames_Null for the null-passthrough
branch, following the existing QsvPreferNativeDecoder tests' seed/handle/re-read
shape. Negative-controlled: reverting both handlers fails exactly 5.
- Low: the SPA `min` was cosmetic. Input does forward it to the DOM, but there is no
<form> — save is an onClick gated only on validate(), which had no branch for this
field, so a typed 10 submitted fine and was silently changed to 64 with a 200 and no
message. validate() now rejects it client-side.
- Low: the warning fires at the top of SetAccelState, before we know whether the
pipeline uploads at all, so a fully-hardware path could be told "using 64 instead"
when nothing consumed either value. Reworded to "will use ... wherever frames are
uploaded".
- Low: recorded in the decision entry that the save-time normalization is
unconditional on hardwareAcceleration (a non-QSV profile's stored value moves too),
and that a client PUTting 0 reads back 64 — a transform the OpenAPI description does
not advertise.
Verified in production, not just asserted. Set prod's profile to 64 (operator-approved)
and drove the exposed pipeline myself via the troubleshooting playback API on an mpeg4
.avi, which forces software decode + hwupload:
hwupload=extra_hw_frames=64,vpp_qsv=w=1875:h=1080 exit 0, speed 12.0x, 0 ENOMEM
Then the negative control on prod's own hardware, same command, only the pool differing:
extra_hw_frames=64 -> exit 0, 8 segments, 0 ENOMEM
extra_hw_frames=0 -> exit 244, 0 segments, 3 ENOMEM
which reproduces the six overnight production failures and confirms the fix.
Full suite green: 4095 .NET, 891 web.
Refs #350, #516, #519.
Cold adversarial review returned BLOCKED on the documentation half. Addressed:
- The new decision record carried no lifecycle metadata block, taking the repo from
82/82 to 83/82 and making it invisible to the by-key catalog lookup that #521
established the same day. Added key/status/since/supersedes/superseded-by
(ffmpeg.qsv-extra-hw-frames-floor) and regenerated docs/decisions/README.md;
decisions_validate.py now reports OK with no legacy-unmigrated notice.
- The entry claimed to correct the #350 record but left that record untouched, so the
stale "the burst is bounded" claim stayed authoritative for anyone resolving
ffmpeg.hls-cold-start-burst. Added a forward-pointing correction note there (hence
the [decisions-edit] token on this commit).
- The floor was applied silently. QsvPipelineBuilder.SetAccelState now logs a warning
naming both the configured and applied value, because raising a deliberately small
pool costs additional surfaces (64 NV12 1080p surfaces is roughly 190 MiB, 760 MiB
at 4K) on memory-constrained iGPUs.
- Narrowed an overstated claim in the entry: 1..63 are untested, not known-bad. We
raise them because the risk is a channel serving nothing, not because asking for
less is illegitimate. Recorded as a deliberate over-reach with a stated cost.
- Corrected a factual error: SubtitleScaleQsvFilter also formats extra_hw_frames but
is dead code with no construction site, so it is NOT covered by the guard.
- Config-vs-behavior mismatch: Create/UpdateFFmpegProfileHandler now normalize on
save so stored rows converge on what the pipeline runs, and the SPA field carries
min=64 rather than defaulting the display to 0. Render-time flooring is kept as the
net that fixes existing deployments with no migration; the remaining gap for
un-resaved rows is recorded as an accepted residual.
- Tests strengthened: pinned to the literal measured 64 rather than to the constant
(so lowering the floor cannot quietly satisfy them), added a negative-value case,
added a deinterlace-upload case, and replaced the narrow ShouldNotContain with a
regex asserting EVERY extra_hw_frames occurrence in the command is >= the minimum.
Negative control re-run against the strengthened tests: reverting the floor fails 5,
with the build verified succeeded first. Full suite green (4086 .NET, 891 web).
Review finding that needed no change: the "single point" claim was independently
verified — no bypass exists, every FFmpegState construction routes through
MaybeQsvExtraHardwareFrames.
Refs #350, #516, #519.
A stored qsvExtraHardwareFrames of 0 reached FFmpeg as hwupload=extra_hw_frames=0,
leaving the QSV upload pool no headroom for frames in flight through the filter graph.
Any input that is not throttled then exhausts it: the graph fails with -12 (Cannot
allocate memory), h264_qsv reports "Could not open encoder before EOF", and zero
segments are written.
Measured against the deployed FFmpeg 8.1.2, one real logged command, only the marked
tokens differing:
readrate 1.05, no burst + frames 0 -> exit 0, 14 segments
readrate 1.05 + burst 2/4/8 + frames 0 -> exit 244, ENOMEM, 0 segments
no readrate at all + frames 0 -> exit 244, ENOMEM, 0 segments
readrate 1.05 + burst 8 + frames 64 -> exit 0, 14 segments
no readrate at all + frames 64 -> exit 0, 14 segments
So the defect predates #350's cold-start burst: a work-ahead start takes no -readrate
and was already failing on a profile with 0. The burst removed the throttle on every
realtime session, turning an intermittent failure into a near-deterministic one, which
is how it surfaced. Input throttling was doing load-bearing allocation-bounding work
that nobody had written down -- which is why the FFmpeg-level benchmark in #350 and the
argument-generation tests in #516 were both green and neither could see it.
Fixed at FFmpegState.QsvExtraHardwareFrames, the single point every QSV upload site
reads, so one guard covers HardwareUploadFilter, HardwareUploadQsvFilter,
WatermarkHardwareUploadFilter, ScaleQsvFilter, DeinterlaceQsvFilter and
SubtitleScaleQsvFilter rather than six call sites that can drift apart. Values above
the floor are still honored.
Tests negative-controlled: reverting the floor fails exactly the three sub-minimum
cases, with the build verified succeeded first.
Refs #350, #516, #519.
Live-E2E of the write path (scripts/e2e-local.sh) confirmed the feature end to
end — good URL -> cached /iptv/logos/<hash>, M3U emits the cached URL, cached logo
serves back as PNG, rejected save leaves the prior logo intact — and caught that a
bad/non-image/oversized logo rejects as 422 (BaseError -> ValidationProblemDetails
via ToErrorResult), not 400. 400 remains model-binding-only. Corrected channels.md,
api-conventions.md, and the graphics.channel-logo-caching decision Rule + catalog.
- Medium-1: wrap ExternalLogoMigratorService.ExecuteAsync in try/catch — a DB
exception (e.g. a channel deleted mid-migration -> DbUpdateConcurrencyException)
no longer trips BackgroundServiceExceptionBehavior.StopHost and kills the app;
it logs and self-heals on the next boot. Caller-cancel path handled separately.
- Low-2: CreateChannelHandler/UpdateChannelHandler validation failure now returns
errors.Join() (all accumulated errors) not errors.Head (first only), restoring
the repo-wide convention; regression test added.
- Low-4: corrected the Startup registration comment (migrator self-awaits
WaitForDatabase; order is not load-bearing).
Final whole-branch review: MERGEABLE @ 6d5f6b24 (fable). Carried Minors adjudicated
acceptable-defer.
New lifecycle record graphics.channel-logo-caching (supersedes: none — narrows
for YAML image elements). Regenerated the active catalog. channels.md + api-
conventions.md updated for the new save-time download + 400s.
Design for reversing #511's render-time fetch: an external logo URL is
downloaded once at save time into the artwork cache (content-hash name,
same as an upload), validated against #511's decode budgets, and stored
so nothing downstream knows it came from a URL. Bad URL fails in the
editor, not at 3am. No refresh button — re-add the URL. One-time startup
migration converts existing URL rows; failures leave the row and warn.
refs #511#502
- Exclude docs/decisions/retrieval-eval.md from active decision parsing
(_NON_DECISION_FILES); its `## N.` eval-question headings were being
miscounted as 7 legacy-unmigrated records.
- Add decisions_lib.metadata_line_count() + a decisions_validate guard
that fails a record with more than one `key:` metadata line, so a
stacked-metadata-block migration bug (which the parser silently
tolerated by reading only the first block) can't recur unnoticed.
TDD: test_duplicate_metadata_block_fails / test_single_metadata_block_passes.
- De-duplicate the 6 docs/decisions.md records left with two stacked
metadata blocks (scan.getoraddfolder-db-lookup #488,
scan.musicvideo-reconciliation #494, scan.jellyfin-mixed-content-library
#489, iptv.logo-drives-bug-preset #67, ffmpeg.qsv-decode-encode-split
#498, ci.small-lane-git-only server-management#639), merging the union
of Signals/paths/issues/Mechanics from both blocks and keeping the
richer Rule wording; rationale prose untouched.
- Fill in the deferred Q6b row in docs/decisions/retrieval-eval.md now
that startup.parallel-orientation is active in docs/decisions.md,
scoring it as a real active-vs-superseded question against the
archived docs.queue-state-gitea-tracker.
- Regenerate docs/decisions/README.md via build_decisions_catalog.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds metadata blocks (key/status/since/supersedes/superseded-by + Rule/Signals/Mechanics)
to all 26 legacy-unmigrated ## records across optimistic-concurrency.md (7, concurrency.*),
api-auth-security.md (9, security.*), release-ci-governance.md (7 remaining legacy records,
release.*), and spa-modularization.md (3, spa.*). Heading text and rationale prose are
byte-identical; only metadata was inserted, per the key-table assignments.
security.session-auth-dual-credential (#295 PR1) gets a one-line cross-reference noting it
narrows the OIDC-inert sub-claim of security.blazor-removal-auth-posture (#206), per
orchestrator adjudication — both stay status: active with supersedes/superseded-by: none.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Retire the line-level append-only mechanic (ersatztv#303 H9) in favor of the lifecycle validator
built in Tasks 1-5. .claude/hooks/decisions-guard.sh is now a thin fail-open shim around
scripts/decisions_validate.py; .husky/pre-commit calls it for the structural (working-tree) checks,
.husky/commit-msg drops the old staged/[decisions-edit]-deletion block and keeps only the
Co-Authored-By check. The Gitea decisions-guard job is renamed "decisions lifecycle" and now runs
decisions_validate.py --base/--head (structural + body-diff + no-vanish) and
build_decisions_catalog.py --check (active catalog drift), with actions/setup-python@v5 added since
the bare `small` lane doesn't guarantee python3; the old 1800-line consolidation-floor step is
removed (replaced by the validator's aggregate active-corpus budget). docs/decisions.md's header is
rewritten from append-only to lifecycle framing (metadata schema, statuses, generated catalog,
archive, same-PR supersession); [decisions-edit] is re-scoped (not removed) to rationale-prose
edits/factual corrections only. docs/ci-cd.md's release ritual and hook/job descriptions are
rewritten to match.
Also fixes a pre-existing validator false-positive surfaced while sanity-checking against
origin/main: Task 6's #303 H9/H3 split (commit d09be57e) renamed the archived record's heading
away from the pre-split original, which the validator's heading-based relocation check reads as
"removed without an archive copy." Restored the archived heading to match the original text
(functionally unchanged — still status: superseded, same key) and updated the two prose
cross-references (migration-map.md, release-ci-governance.md) that pointed at the old anchor.
PR1 scope only (per brief): does NOT wire the kickoff-guard CI step or touch the
kickoff/README/select-queue docs — that's Task 8/PR2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The archived docs.append-only-guard record bundled the genuinely-superseded
H9 (decisions.md append-only) with the independent, still-active H3
(root-screenshot pre-commit guard), relocating both into archive/ and
burying an active decision in the history wing. Split into two records:
docs.append-only-guard stays in archive/release-ci-governance.md,
status: superseded, H3 content removed and replaced with a forward
pointer; ci.root-screenshot-guard is a new status: active record in the
active docs/decisions/release-ci-governance.md, with the H3 rationale
paragraph lifted byte-identical from the original bundle. Updated
migration-map.md (new row + a "Migration conventions" note: never
relocate an active decision to archive/) and regenerated
docs/decisions/README.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrate #406 (ci.runner-placement) and #412 (ci.peak-anon-measurement) to the lifecycle
schema as exemplars; #390 and #411 confirmed prose-only (no standalone record). Dogfood the
append-only->lifecycle supersession: docs.append-only-guard moved to archive/, superseded by
new active docs.decision-lifecycle. Stand up docs/decisions/archive/ + migration-map.md.
Refs #521
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_anchor() mapped each space/hyphen to its own '-' without collapsing runs,
so the standard heading separator " — " (space, em-dash, space) produced a
double hyphen in every generated anchor. Since nearly every real decision
record heading uses that separator, this made the catalog emit a dead link
for essentially every row. Fix: after building the char list, collapse
consecutive '-' into one and strip leading/trailing '-' via re.sub, matching
how Goldmark/GitHub/Gitea sluggers behave.
Also fixed main() writing an extra trailing newline (want already ends in
"\n", then "+ \n" appended a second one) so docs/decisions/README.md now
ends with exactly one trailing newline; --check still compares via .strip().
Added test_anchor_collapses_em_dash_and_keeps_underscore to pin the anchor
behavior against the reported iptv.base_url case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds scripts/build_decisions_catalog.py, which renders docs/decisions/README.md
as a compact table of only 'active' decision records (sorted by key), and its
test scripts/tests/test_build_catalog.py. Supports --check for CI drift
detection. No decision records are migrated yet, so the generated catalog is
currently empty (banner + header only) — expected at this stage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds scripts/decisions_validate.py: lifecycle invariant checks (unique
active key, key format, reciprocal supersession, removed-without-archive,
rationale-rewrite-without-token, catalog staleness, corpus budget) plus
git-diff helpers for merge-base-based CI checks. Deviates from the task
brief in one spot: REQUIRED_META narrowed to (key, status) — the brief's
(key, status, since, supersedes, superseded_by) makes its own
test_clean_corpus_passes fail, since since/supersedes/superseded_by
default to None on bare Record() instances built without going through
the markdown parser.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds scripts/decisions_lib.py, the shared parser for ErsatzTV decision
records (docs/decisions.md + docs/decisions/*.md). Parses H2 sections
into Record dataclasses, distinguishing migrated records (visible
metadata block: key/status/since/supersedes/superseded-by + Rule/
Signals/Mechanics) from legacy-unmigrated ones with no metadata line.
scripts/ is now an importable package (scripts/__init__.py,
scripts/tests/__init__.py) so later tools can `import scripts.decisions_lib`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Design for the decision-knowledge lifecycle schema/catalog/validator (#521) and
the #237-retirement / parallel-startup kickoff rewrite (#520), as one arc.
Refs #520#521
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fourth adversarial pass cleared the security design — all three earlier
bypasses are dead, DecoderOptions.MaxFrames is honored by every decoder
that can produce multiple frames (GIF/WebP/TIFF exactly N, APNG N-1),
and it bounds PEAK allocation, not just the final frame count (measured:
65 MiB capped vs 2.41 GiB uncapped on the same 600-frame GIF).
But it caught a functional regression this PR introduced: a *default*
`Image.IdentifyAsync` throws InvalidImageContentException on most APNGs
that `Image.Load` reads back perfectly — including files ImageSharp's
own PngEncoder wrote. Reproduced independently: 13 of 16 shapes throw,
and `MaxFrames = 1` on the Identify fixes all 16 with dimensions intact.
Since #502 routes ordinary channel-logo watermarks through this path, an
admin with an animated PNG logo would have silently lost their watermark
to a log line — a hardening change breaking working content.
The existing tests could not see it: they use 64x64, which happens to be
one of the few shapes a default Identify handles. Now pinned with a
288x288 shape that asserts the default Identify DOES fail and that
DecodeRemoteImage decodes it anyway, in full.
Also, from the same pass:
- document the REAL enforced peak (up to 3x the nominal 50 MP budget,
since detecting "over the limit" means decoding past it) instead of
restating the nominal number. Tightening the single-frame allowance to
budget/3 would reject legitimate 8K stills, so the overshoot is
deliberate; it is ~600 MB against the ~36 GiB it replaces
- correct the MaxFrames off-by-one claim: N-1 is APNG-specific, not
universal, so the stated rationale for +2 was wrong for three of the
four animated formats
Second adversarial re-review defeated the product budget too, and the
mechanism generalizes: the budget was enforced on a number the decoder
does not honor.
Measured on ImageSharp 3.1.12 (reproduced independently before fixing):
600-frame APNG -> Identify: FrameMetadataCollection.Count = 0
Load: Frames.Count = 600
So EnsureDecodeAffordable(w, h, 0) charged Math.Max(0,1) = 1 frame —
the most permissive possible reading. A 4000x4000 x600 APNG is ~134 KiB
on the wire, is charged 16 MP, and decodes to ~36 GiB: 2.5x worse than
the GIF the previous commit exists to stop, at half the wire size. The
retention budget could not backstop it — that runs after LoadAsync, so
the process OOMs first, killing every concurrent stream.
GIF, WebP and TIFF report honestly; PNG/APNG is the sole divergence,
which is the point: you cannot audit every format, so the header cannot
be the source of truth.
DecodeRemoteImage now:
- checks header DIMENSIONS only (trustworthy; a GIF image descriptor
exceeding its logical screen is clamped by the decoder, verified)
- derives how many frames of that size the budget affords
- passes that to DecoderOptions.MaxFrames, which the DECODER enforces
whatever the header claimed. Measured: MaxFrames = N yields N-1
frames, so it asks for affordable + 2 — decoding one more than allowed
is what distinguishes "at the limit" from "over it" without silently
truncating a legitimate animation
- re-verifies the real image.Frames.Count after decoding, disposing and
rejecting if over
Also adds wiring coverage for the retention budget (M4): deleting its
call site now fails a test — negative-controlled, build verified before
trusting the result.
docs/decisions.md records both failed attempts, because the lesson is
the generalizable part: independent caps do not compose into a budget,
and a limit the decoder does not enforce is not a limit.
Adversarial re-review of the first fix defeated its decode guard with a
measured payload: a 2500x2500 x600-frame GIF is ~60 KiB on the wire,
passes the 50 MP dimension check (6.25 MP) AND the 600-frame check
(exactly 600), and costs ~14 GiB to decode — strictly worse than the
30000x30000 PNG the guard was added to stop, at 1/60th the wire size.
Checking dimensions and frames independently never bounded the decode.
- decode budget is now width x height x frames <= 50 MP, as one product;
a zero frame count is charged as one so an unenumerable header cannot
zero it out
- new retention budget: frames x scaledWidth x scaledHeight <= 200 MP.
Independent of the decode budget in both directions — a 100x100 source
is trivial to decode but retains ~5 GB of SKBitmap once every frame is
scaled to 1920x1080, since LoadImage clones and resizes each frame to
output resolution and keeps them
- both budgets are pure functions (EnsureDecodeAffordable,
EnsureScaledFramesAffordable) so the arithmetic is tested at every
boundary without materializing multi-gigabyte images
- the frame guard had NO coverage before; it does now
- fail loudly on a non-seekable fetcher stream instead of letting
Position throw NotSupportedException into the blanket catch
- test the copy over-read against the ACTUAL rented buffer length
(ArrayPool.Rent(81920) returns 131072), not the requested 81920
docs/decisions.md corrected: it claimed the byte cap bounded the
decode-bomb surface and that the header check closed the class. Both
overstated. An append-only file that is confidently wrong is worse than
one with a gap.
`ImageElementBase.LoadImage` fetched http(s) images with a throwaway
`new HttpClient()` + `GetStreamAsync`: no timeout override (the 100s
default), no size cap, unbounded redirects, no pooling — all inside
stream startup, while ffmpeg waits on the pipe. #502 routed ordinary
channel-logo watermarks onto that path, widening a pre-existing weakness.
Introduce `IRemoteImageFetcher` / `HttpRemoteImageFetcher`, modelled on
the neighbouring `IRemoteStreamProber`:
- deadline covers headers AND body (linked CTS + `CancelAfter`, client
`Timeout = InfiniteTimeSpan`) — under `ResponseHeadersRead` the body
read falls outside `HttpClient.Timeout` (the #289 lesson)
- 10 MiB cap enforced during the copy; `Content-Length` is only a cheap
early reject, since it can be absent or a lie
- permissive content-type check (rejects an HTML error page, allows a
missing type and octet-stream)
- pooled via `IHttpClientFactory`; redirects capped at 3, not 50
A byte cap does NOT bound decoding, so `DecodeRemoteImage` additionally
reads declared dimensions + frame count from the header and rejects
before `Image.LoadAsync` allocates (50 MP / 600 frames). A 4 KB PNG
declaring 30000x30000 costs ~3.6 GB to decode and passes every wire-size
check — caught by adversarial review of the first version of this change,
which capped bytes and wrongly claimed that was decode-bomb protection.
Not cached and SSRF not mitigated — both deliberate, with the reasoning
recorded in docs/decisions.md.
fixes#511
Fresh stdio JSON-RPC MCP server wrapping the frozen /api/v1 surface,
superseding the closed read-only PR #76. 26 read tools (six families +
search/all-items & search/artists discovery) and cautious-write CRUD:
collections (incl. idempotent membership adds for #487), smart collections,
schedules, playouts, channels (create/update/delete/reset), and a
Jellyfin-focused media-source sync/scan slice. Writes gated behind
ERSATZTV_ALLOW_WRITES (default false, runtime-enforced).
Security baseline carried forward from PR #76/#289: read-only backstop,
JSON-RPC DoS guards + bounded stdin reader, per-request CTS over
headers+body, response-size cap, arg validation vs InputSchema,
reverse-proxy prefix preservation. Machine-key auth (X-Api-Key,
CSRF-exempt). If-Match/ETag round-trip for the one replace-all PUT that
honors it.
Cold-review fixes folded in:
- HIGH: reject control chars (CR/LF) in the ifMatch value before it reaches
TryAddWithoutValidation — SocketsHttpHandler writes it verbatim, so a
crafted value could smuggle headers onto the X-Api-Key request.
- Cache the empty-args JsonDocument (no per-call pooled-doc leak).
- Accept explicit JSON null for optional fields so a nullable API field
(e.g. dailyRebuildTime) can be cleared as documented.
Deferred (documented): the ~40-field replace-list writes and redesign
workflow tools (#63-#68).
Docs: docs/mcp.md, docs/README.md index, docs/decisions.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-review of the fix commit flagged two ways the new tests could pass
vacuously in future: the burst assertion was a bare substring (satisfiable
by any input carrying the option) and the still-image test asserted only an
absence. Anchor the first on the input path plus an occurrence count, and
give the second a positive anchor.
`-readrate 1.05` paces input reading at wall clock so a channel behaves like
live TV, but it applies from the very first read. With 4s HLS segments and the
segmenter waiting for the first one, the playlist could not appear sooner than
~4/1.05 = 3.8s, so every tune-in that did not win a work-ahead slot paid a
multi-second wait.
Add `-readrate_initial_burst` (FFmpeg >= 6.1) next to `-readrate` on the normal
playback path, gated on runtime capability detection via the existing
`FFmpegKnownOption`/`HasOption` machinery, whose option list had simply been
empty. Measured on real prod media: time-to-first-playlist 5369/5344ms ->
648/649ms.
Root cause detail: the cold-start bimodality earlier rounds could not explain
was never about the media. `HlsSessionWorker` grants an unthrottled start only
while `_workAheadCount < work_ahead_limit` (prod: 1), so concurrent tune-ins
fall back to the throttled path. Confirmed on prod with three concurrent tunes:
firstGop 866ms for the slot winner vs 3845ms and 6357ms. This also falsifies the
issue's ranked #1 driver — accurate-seek decode-discard measures 30-100ms on
real media, and probe caps 20-50ms; neither can account for seconds.
Still images are excluded: their video input is paced by the realtime filter and
takes no readrate, so a burst would only run a song's separate audio input ahead
of the video. Concat/WrapSegmenter keep the unburst single-arg constructor.
fixes#350
Verified live while promoting v26.11.0. Two corrections:
1. `DeployStack media-servers` targets a DEAD stack. The Komodo stack name
changed to `jazz-media` with the move to jazz; the compose PROJECT is still
`media-servers` (which is what container labels show, so the labels don't
catch this). A `media-servers` stack still exists on bumblebee in state
`unhealthy` — the stopped migration leftovers — so the documented command
silently deploys the wrong, dead thing.
2. There is no Global Auto Update fallback: `jazz-media` has
auto_update=false (poll_for_updates=true only). Promotion is manual, full
stop, and the 'don't cut a tag near the 03:00 run' caveat is obsolete.
The pre-deploy safety chain is intact and jazz-aware (#635) — verified by
reading the deploy's Pre Deploy stage: image-change trigger fired, 286M backup
with integrity_check=ok plus off-box PBS, migration smoke PASS against the
prod-copy, then only ersatztv recreated.
Adversarial review found a documentation defect, not a code one: both
docs/decisions.md and the WatermarkSelector comment asserted the deco path
was unaffected by this change. That is true of the *resolution* half and
false of the *routing* half. SelectWatermarks puts deco-derived options
into the same list the routing guard filters, so a deco watermark whose
resolved path is a URL is rerouted to the graphics engine too — including
the generated-initials localhost URL, which only the deco path still emits
and which plausibly rendered through ffmpeg before.
That reroute is intended (routing by what the path is beats routing by
provenance, which would drift), so the fix is to say so accurately rather
than to narrow the guard. Also records the accepted per-frame cost
asymmetry the entry previously argued on correctness grounds alone.
The guard is extracted as CanUseFFmpegNativeWatermark so it can be tested
directly — review's highest-value gap was that the half of the fix which
decides whether pixels appear had no automated coverage, only the one-off
live E2E. Nine cases pin it, including the localhost-fallback reroute.
Both deferrals now point at real issues instead of an unverifiable
"tracked separately": #510 (deco vs precedence-level missing-logo policy)
and #511 (remote-fetch hardening — timeout, size cap, redirects, pooling,
caching, SSRF).
Also pins scheme-case insensitivity in the selector.
A channel whose logo is an external URL never rendered a watermark, even
with an ImageSource=ChannelLogo watermark attached. WatermarkSelector
resolved the URL correctly and then existence-checked it on the
filesystem — File.Exists("https://…") is always false — so all three
precedence levels (playout item, channel, global) logged "Channel logo
no longer exists" and returned None. The channel editor advertises the
URL as winning over an uploaded logo, which was true for the guide
listing and silently false for the bug.
External artwork passes through rather than being downloaded into the
image cache: that is already the convention everywhere else (M3U, XMLTV,
SPA JSON all emit the raw URL), no fetch->SaveArtworkToCache glue exists,
and the render path does not need it — ImageElementBase.LoadImage already
fetches an http(s) path with HttpClient and decodes it for real pixel
dimensions.
A remote-URL watermark is therefore forced onto the graphics engine
instead of the ffmpeg-native shortcut, which would otherwise hand the URL
to ffprobe and ffmpeg as a bare -i argument, putting an unbounded network
fetch inside stream startup.
The three gated precedence levels now share one ChannelLogoWatermarkOptions
helper — the triplicated block is what let the defect exist three times
over. Scope held narrow: the generated-initials localhost fallback (#1)
stays disabled behind an explicit comment and a scope-guard test, and the
deco path keeps its own long-standing unchecked policy.
Verified by live-E2E against a real channel playout with an external-URL
logo: origin/main renders 0 logo pixels and logs the "no longer exists"
warning verbatim; this branch renders the logo in the expected region.
Whitespace-only reformatting in FFmpegLibraryProcessService.cs is the
fix-as-you-touch format gate on pre-existing violations, plus a BOM strip.
fixes#502
Editing .gitea/workflows/ci-image.yml is itself a trigger path for ci-image.yml, so
the previous commit republished the toolchain image at its own sha. `ci-image-pin`
then correctly failed: the pin still named 07048b8 while
`git log -1 -- docker/ci .gitea/workflows/ci-image.yml` resolved to 4263cf79.
The image content is unchanged — that commit only moved the job's `runs-on:` — but
the pin's contract is "the pin equals the last commit that touched the image
sources", not "the bytes differ", so it has to move. Verified 4263cf7 is actually
published to the registry before pinning it; a pin that doesn't resolve to a real
image would break every `container:` job at once.
This is why the bump lands as a SEPARATE commit: the tag is the short sha of the
pushed branch tip, so a single commit could never contain its own sha. Splitting it
makes the branch self-consistent — the source-touching commit stays the last one to
touch those paths, and this one carries the matching pin.
refs #508
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`runs-on: small` carried two jobs that are not small: docker-build.yml's image
build, and ci-image.yml's toolchain buildx. The second reads as lightweight
because it is "docker-only, no toolchain needed — it *builds* the toolchain",
which is true and yet describes the heaviest job in the lane.
A lane's per-job memory cap is set by its worst member, not its median, so these
two pinned `small` at --memory=10g. On bumblebee's 25 GiB — also the prod media
host — that permits exactly ONE slot, and four jobs shared it. So "widen the
lane" and "keep the heavy jobs" were never simultaneously available.
The symptom that forces the issue is not queue wait. A saturated lane also wedges
DISPATCHED jobs in act's setup phase: >10 min in_progress, no log file written at
all, then failure, before Checkout runs. That is where "decisions.md is a known
flake, just rerun it" came from — the rerun works only because it lands after load
clears, so a capacity problem read as a bug in the guard.
With both builds on ubuntu-latest, `small` is a checkout plus a `git diff` and
server-management#639 caps it at 1 GiB, widening it to 4 slots across two hosts
while committing LESS RAM to CI than the single slot did.
so it cannot be dispatched until the jobs it would queue behind have finished.
refs #508
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The release range grew after the host-correction PR landed: #67 and #498 both
merged into main. #498 in particular flips a default (QsvPreferNativeDecoder is
ON), so it leads the row rather than sitting in a fixes list.
From the Fable whole-branch review:
- M1: guard native VA-API decode with !OperatingSystem.IsWindows() (no vaapi
hwaccel on Windows; QSV caps over-reported there) — DRY'd into a preferNativeDecode local.
- L2: IsIntelVaapiOrQsv also matches decoder mode Vaapi, preserving Intel
audio-dup parity on the (producerless) Nut-output branch.
- L1: replace the vacuous ShouldNotContain(" deinterlace_qsv") with an
occurrence-count assertion that actually catches a second bare occurrence.
- N1/H1: decisions.md — correct the column to nullable-with-default (not NOT NULL),
and record the accepted HDR software-tonemap trade-off + Linux-only guard, with
the tonemap_qsv optimization tracked in #505.
H1 (HDR tonemap reroute) accepted-and-deferred per that decision; #505 filed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whitespace-only (git diff -w is empty); the #311 format job checks whole
touched files, and these two legacy files carried pre-existing violations
never caught before (no PR had touched them since the gate landed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Field-reference update skipped: docs/channels.md and docs/domain-model.md
have no per-field FFmpeg-profile catalogue (channels.md's Encoding bullet
is a one-line summary, not a field list; grep for QsvExtraHardwareFrames
or HardwareAcceleration finds no such list in either doc).
- Blocker 1: VA-API decode to SOFTWARE frames (drop -hwaccel_output_format,
new DecoderVaapiToSoftware) so the proven hwupload/vpp_qsv branch bridges to
the QSV encoder — the naive hardware-surface path emits a bare vpp_qsv on
VA-API frames and fails on ~all content.
- Blocker 2: bool? domain property + != false coercion (DeinterlaceVideo
pattern) so create-with-false actually persists false.
- High 3: REST DTOs bool?=null + ?? true for /api/v1 additive-compat.
- Medium 4: correct Task 4 test scaffolding (DefaultHardwareCapabilities).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirrors Jellyfin's default-on "prefer native decoder" hybrid via a new
QsvPreferNativeDecoder profile boolean. Reverified against code before
designing; records the rejected decode-family-enum alternative (Option C).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whole-branch review found the stamping test was structurally vacuous: a bare
foreach over ChannelTemplates.Where(IsSystem) passes with zero iterations, so
the test would have stayed green if template seeding silently bailed out.
Assert the collection is non-empty first. Proven non-vacuous by a negative
control (forcing SeedChannelTemplates to bail makes exactly this test fail).
Also cover the ACTUAL production sequence -- adopt an existing hand-made row,
then delete it -- which the previous no-resurrect test did not exercise (it
covered seed-then-delete). The marker is written on the adopt path too, so
the deleted row must stay deleted.
docs: note that a deleted preset degrades to no default rather than failing,
and that the default applies to newly created channels, not retroactively.
Refs #67
- Add ChannelEditScreen tests exercising the geometry-fetch/preview path (previously
untested because the fixture's blank logo path kept BugPreview from ever rendering):
asserts the fetched watermark geometry lands on the preview image with asymmetric
values, and that an external logo URL suppresses the preview.
- Fix a toggle-off/toggle-on data loss: re-enabling "use logo as on-screen bug" used to
always rebind to the default 'Channel Bug' preset, discarding a channel's own
per-geometry ChannelLogo preset. Remember the last referenced logo-driven watermark id
in a ref and prefer it, falling back to the shared default.
- Strip the geometry cache's `id` key before it reaches BugPreview's props (was leaking
via a spread).
- Drop the "(see #502)" issue reference from the visible help text; keep it in the code
comment.
- Constrain findLogoBugWatermark's imageSource param to the generated
ChannelWatermarkImageSource union instead of `string`, and add unit tests for its
preference/fallback/none-found behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
timothyandClaude Opus 4.8