Re-apply HA fork patches on upstream v12.0 #3
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: unkin/jellyfin-ha-src#3
Reference in New Issue
Block a user
Delete Branch "benvin/rebase-v12"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fork main still sat on a pre-10.11.0 upstream tree 2,119 commits behind, with 123 of its 206 commits being upstream backports and SharedVersion.cs hand-edited to claim 10.11.7. This rebuilds the fork from the v12.0 tag and re-ports only the HA work.
6c073e19dd)Included Infeature (#15516) c9f71d8531Prevents raw ISO-639-2 values (e.g. "Greek, Modern (1453-)" from cluttering the audio and subtitle display names by truncating them at the first comma or semicolon ("Greek"). Applies to MediaStreamRepository and ProbeResultNormalizer.ClearProfileImageAsync removed the ProfileImage instance attached to the passed-in User, but that instance can carry a stale, never-persisted (temporary) key because UpdateUserAsync creates the persisted image on a separately loaded entity and never copies the generated key back. Removing that detached entity on a fresh DbContext made EF Core throw InvalidOperationException ('ImageInfo.Id has a temporary value'), leaving the profile image impossible to delete or replace. Load the tracked, persisted user and remove its actual ProfileImage, matching the removal pattern already used in UpdateUserAsync. Adds regression tests covering the temporary-key case and the no-image no-op (the first fails before this change and passes after). Fixes #13137 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>BaseItemDto.Genres, .Tags, and .ProviderIds are plain auto-properties with no default initializer, so they deserialize to null when a client omits them from a partial POST /Items/{itemId} body. The OpenAPI spec documents every BaseItemDto field as optional, but ItemUpdateController.UpdateItem fed these three properties straight into Distinct()/Select()/ToList() without a null check, so a request that (for example) only sets Tags throws ArgumentNullException("source") once it reaches the unguarded Genres line, before Tags is even processed. Guard all three assignments with the same "if (request.X is not null)" pattern already used for the neighboring Studios/Taglines/ProductionLocations fields in this method, so omitted fields are left unchanged instead of crashing the request. Adds ItemUpdateControllerTests covering the reported repro (only Tags supplied) and a companion case asserting existing Genres/ProviderIds are preserved when omitted from the payload. Signed-off-by: zerafachris <christopher.zerafa@blocklabs.io>`GetProgressiveAudioFullCommandLine` forced the raw PCM muxer and a bogus sample rate whenever the audio encoder was `pcm_*`, regardless of the container the client asked for. Two separate failures came out of it: - `-ar ` + `state.BaseRequest.AudioBitRate` used a *bitrate* as a *sample rate*, and `AudioBitRate` is optional. When it is absent the argument degrades to a bare `-ar`, ffmpeg aborts with `Expected number for ar but found: -ar` / `Error opening output files: Invalid argument` (exit 234) and the request fails with HTTP 500. Every `GET /Audio/{id}/stream.wav` that does not carry an explicit `AudioBitRate` hits this. The sample rate was already being set correctly a few lines below from `OutputAudioSampleRate`, so the line is dropped rather than repaired. - `-f s16le` overrode the muxer even for a real container. A request that did supply a bitrate (`/Audio/{id}/universal` passes `MaxStreamingBitrate`) survived the first bug but produced raw headerless samples served under an `audio/wav` content type, so clients saw a body with no RIFF header. The raw muxer is now only forced when the requested container is actually raw PCM, which keeps the I2S/MCU route from #10321 working. Also drop the `containerInternal = ".pcm"` assignment in `StreamingHelpers.GetStreamingState`: it is written after `state.OutputContainer` has already been read from the same variable and is never read again, so it has no effect and only obscures where the output container comes from. Verified against ffmpeg 8.1.2 with a 96 kHz FLAC source: before, the wav command line exits 234; after, it produces a valid `RIFF/WAVE` 48 kHz stereo `pcm_s16le` file, while the raw `.pcm` route still yields exactly 2 s x 48000 x 2ch x 2 B = 384000 bytes of headerless samples.`GetItemValues` -- the shared path behind `/Artists`, `/AlbumArtists`, `/Genres`, `/MusicGenres` and `/Studios` -- disabled the total record count whenever the query carried no `Limit`: if (!filter.Limit.HasValue) { filter.EnableTotalRecordCount = false; } A request without an explicit limit therefore came back with N entries in `Items` and `TotalRecordCount = 0`. Clients that page on the reported total -- the documented contract every other list endpoint honours -- read that as an empty library. `/Items` and `/Persons` do not share this path and report the count correctly, which is what makes the inconsistency visible from the outside. Measured against master with a 62-track music library: GET /Artists?UserId=... -> TotalRecordCount=0 Items=5 GET /Artists?UserId=...&limit=100 -> TotalRecordCount=5 Items=5 Dropping the block costs nothing: `representativeIds` is materialised into a `List<Guid>` a few lines below regardless, so `.Count` was already available and the count is now reported from it. Callers that genuinely want to skip the count still can -- `EnableTotalRecordCount = false` is honoured as before. The block also mutated the caller's own query object, so a query instance reused across calls silently lost its total after the first limitless one. That is covered by a test as well.If left at Default, sqlite3_enable_shared_cache is process-global, so a plugin enabling it makes these connections share a cache too. Contention then surfaces as SQLITE_LOCKED ("database table is locked"), which the busy handler does not cover, busy_timeout is skipped and the command fails at CommandTimeout instead.Review
Confirmed the "real" diff is
git diff v12.0 HEAD(93 files, +12720/-992) — v12.0 is a direct ancestor of this branch, matching the PR description. Full solution (dotnet build Jellyfin.sln -c Release) builds clean, 0 warnings/0 errors, against the v12.0 base. Reviewed the port for correctness rather than themergeable=falsecross-history noise, per instructions.High severity
1. Lease-aware transcode cleanup is non-functional — the one claimed HA behavior that doesn't actually work.
Jellyfin.Api/Controllers/DynamicHlsController.cs:1599-1618(RegisterTranscodeSessionAsync), the only production call site that ever creates aTranscodeSession, hardcodes:even though
playlistPathis in scope at both call sites (line ~310 and ~1523) and could supply real values.Emby.Server.Implementations/ScheduledTasks/Tasks/DeleteTranscodeFileTask.cs:153-166(IsFileProtectedByActiveSession) guards both checks with!string.IsNullOrEmpty(session.ManifestPath)/!string.IsNullOrEmpty(session.SegmentPathPrefix), which are always false against real sessions — so the "skip files belonging to an active session" branch can never trigger in production.DeleteTranscodeFileTaskwill delete files belonging to a live HA transcode session exactly as if this feature didn't exist. Tests don't catch it:DeleteTranscodeFileTaskTests.csbuildsTranscodeSessionobjects directly with realistic paths (bypassing the controller entirely), and the two "HA takeover" test classes never call the controller (see finding #3). Fix: threadplaylistPath/ a segment-prefix derived fromstate.OutputFilePathintoRegisterTranscodeSessionAsyncand populate the two fields for real.Medium severity
2.
RenewLeaseAsyncis a non-atomic, ownership-blind read-modify-write — a takeover can be silently reverted.Emby.Server.Implementations/MediaEncoding/RedisTranscodeSessionStore.cs:96-116: reads the session, extendsLeaseExpiresUtc, and writes the whole object back, including theOwnerPodfield it just read.ITranscodeSessionStore.RenewLeaseAsync(MediaBrowser.Controller/MediaEncoding/ITranscodeSessionStore.cs:54) doesn't even take a caller/pod identity, so there's no way to check "do I still own this lease" before writing. Sequence: pod A's lease expires mid-transcode → pod B calls the atomicTryTakeoverAsync(Lua script, correct) and wins → pod A's in-flightRenewLeaseAsync(started before expiry, per the 10s heartbeat loop inDynamicHlsController.StartLeaseRenewal, line 1623) completes its GET-modify-SET and overwrites B's takeover with A's staleOwnerPod, resetting the TTL. Two pods can now believe they own the same session/output files. Contrast withTryTakeoverAsync(lines 127-145), which correctly uses theTakeoverScriptLua script for atomicity. Fix: renew via a Lua script (like takeover) that only extends TTL/LeaseExpiresUtcwhensession['OwnerPod'] == ARGV[callingPod]; add a pod parameter to the interface method.3.
RedisTranscodeSessionStoreTests.csnever touchesRedisTranscodeSessionStore— the Lua scripts have zero test coverage.Despite the name,
tests/Jellyfin.Server.Implementations.Tests/MediaEncoding/RedisTranscodeSessionStoreTests.cstestsInMemoryTranscodeSessionStore, "as a reference implementation (no real Redis required)" (line 12-13 doc comment). It never references the realRedisTranscodeSessionStoretype anywhere. Grepping the wholetests/tree forRedisTranscodeSessionStore(the class, not the interface) turns up nothing — not even aCategory=RequiresDockerintegration test, unlike the Postgres provider which gets realTestcontainers.PostgreSql-backed tests (PostgreSqlProviderTests.cs,PostgreSqlConcurrencyTests.cs). So theTakeoverScriptLua atomicity, theRenewLeaseAsyncrace in finding #2, and TTL/PXhandling are all completely unexercised — the "unit tests" for this class are testing a different, presumably-correct hand-written implementation and asserting the interface contract holds against that. This is precisely why #2 shipped undetected. Fix: add aTestcontainers-backed (orStackExchange.Redistest-server) suite that actually instantiatesRedisTranscodeSessionStoreand exercises the Lua paths, gatedCategory=RequiresDockerlike the Postgres tests, or at minimum rename this file/class so it stops implying coverage it doesn't have.Low / advisory
4. Test naming overstates coverage of the actual HA glue code.
tests/Jellyfin.Api.Tests/Controllers/DynamicHlsHaTakeoverTests.csandDynamicHlsSessionRegistrationTests.cscarry doc comments saying tests "will be wired intoDynamicHlsController" (stale future tense) — but neither ever instantiates or callsDynamicHlsController. Both test the same hand-rolled in-memory fake ofITranscodeSessionStoreas finding #3. The controller's actual glue (IsHaTakeoverAsync,RegisterTranscodeSessionAsync,StartLeaseRenewal) has zero executing unit coverage, which is exactly how finding #1 got through. By contrast,LiveStreamHaRecordTests.csis a good test — it exercises realSessionManager.CloseLiveStreamIfNeededAsyncvia mocks and verifies fail-open behavior on a store exception. Recommend renaming the misleading classes and adding real controller-level coverage (or removing the "controller" framing from the doc comments).5. Orphaned
TryGetLiveStreamAsyncclaim verified — but the write side is now pure overhead.Grep confirms
TryGetLiveStreamAsyncis called only from test code; production only callsSetLiveStreamAsync/DeleteLiveStreamAsync(SessionManager.cs:916,:360). Claim checks out. ButSetLiveStreamAsyncfires fromUpdateLiveStreamActiveSessionMappingson every playback-start and every playback-progress tick for a live stream (SessionManager.cs:810,:964) — i.e. two Redis writes per heartbeat for a record nothing ever reads back. If v12.0 genuinely closes live streams safely without needing durable takeover records, the write path (and theLiveStreamSessiontype /Set/TryGet/DeleteLiveStreamAsyncmethods) should come out entirely rather than leaving a half-finished feature that writes but never reads. If it's meant to support future takeover-pod recovery, that consumer is still missing.6. Fail-open is real and matches docs, asymmetric cleanup fail-closed is a sound but undocumented deviation.
RedisScanLeaderLease.TryAcquireOrRenewAsync(RedisScanLeaderLease.cs:73-79) explicitly catches and returnstrueon any Redis error, with a clear comment;DynamicHlsController's three HA hooks each swallow exceptions individually and continue in non-HA mode — consistent fail-open, as documented. Worth flagging for awareness: a sustained Redis outage means every pod runsRefreshLibrary/OptimizeDatabaseTask/etc. concurrently, which is the exact failure this feature exists to prevent — acceptable per the author's own "every instance scanning is preferable to no instance scanning" trade-off, not a defect. Conversely,DeleteTranscodeFileTask.ExecuteAsync(lines 90-98) fails closed on aGetActiveSessionsAsyncerror (skips the whole cleanup pass) — the safer choice given the cost of a wrong guess, but worth a line in the docs noting cleanup is the one place that intentionally does not fail open.7.
ScanLeaderOptions.GatedTaskKeys— claim verified correct.All 8 default keys (
RefreshLibrary,RefreshPeople,RefreshChapterImages,AudioNormalization,TaskExtractMediaSegments,KeyframeExtraction,CleanupUserDataTask,OptimizeDatabaseTask) resolve to realIScheduledTask.Keyvalues in v12.0;CleanCollectionsAndPlaylistsis genuinely gone upstream.ScanLeaderOptionsTests.csproves this via reflection over the actual task assemblies rather than a hardcoded list — good test, not a rubber stamp.8. Gating scope correct.
ScheduledTaskWorker.OnTriggerTriggered(lines 280-294) only gates the timer-driven trigger path; manual/API-triggered execution is untouched, matching the documented "manual runs are never gated."9. CI config compliant.
.woodpecker/ci.yaml's singlebuild-teststep has bothserviceAccountName: jellyfin-ha-srcand realisticresources.requests/limits. TheBackupServiceTestsexclusion (FullyQualifiedName!~BackupServiceTests) is well-commented and, on inspection, that class contains exactly one test method in this tree — so the filter isn't broader than what's documented.10. Postgres provider. Builds clean against the v12.0
JellyfinDbContext; single20260911134055_InitialPostgreSqlmigration as claimed, so a fresh Postgres install gets exactly that one migration. Postgres tests are properly taggedCategory=RequiresDockerand use realTestcontainers.PostgreSql— better rigor than the Redis side (see #3), which never touches a real or containerized Redis at all.11. PR conventions. Body is 671 chars / 7 lines (within the ≤800/≤10 budget), present tense, no user or AI/session references. This is a legitimately atomic change — a full-history rebase can't be meaningfully split further.
Summary
One real functional bug (#1, lease-aware cleanup doesn't clean up anything differently than before) and one real concurrency bug (#2, renewal can revert a takeover) in the two areas this PR most needed to get right — and #3 explains why: the file named for the Redis-backed store never actually tests it. Everything else checked out: gated-task keys, fail-open semantics, CI resourcing/service account, Postgres migration coherence, and the orphaned-
TryGetLiveStreamAsyncclaim are all independently verified correct. Not blocking on the rebase mechanics or PR hygiene — those are clean.6755feb862toc438ec17ed