From 275908ec1188f73eb986f6513c8e3ff48a55da7c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:26:44 +0200 Subject: [PATCH 1/3] fix(locking): atomic EntityLocker flags + single-owner release contract (#231) The six plain-bool lock flags (Plex, Trakt, Emby/Jellyfin/Plex collections, troubleshooting playback) used a non-atomic check-then-set, so two concurrent Lock* callers could both win. Convert them to int flags mutated only via Interlocked.CompareExchange, so the caller that wins the 0->1 transition is the sole owner and the only one that fires the change event. The three ConcurrentDictionary-backed kinds (Library/Playout/RemoteMediaSource) were already atomic; drop their redundant ContainsKey pre-checks. Define the ownership contract (tokenless single-owner discipline, no interface change) on IEntityLocker and in docs/decisions.md: a true from Lock* confers ownership of exactly one release; Unlock* on an unlocked slot returns false, fires no event, and logs a warning (the double-release / non-owner tripwire). Adds EntityLockerTests (real locker, parallel-caller races) proving exactly one winner per kind, one-releaser-per-slot, and event-fires-once-per-transition. Ref #231. Scan-lifecycle call-site fixes that consume this contract land in the same PR (#232); the BuildPlayout/subtitle finally-gating is #234. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Interfaces/Locking/IEntityLocker.cs | 14 + .../Locking/EntityLockerTests.cs | 270 ++++++++++++++++++ .../Locking/EntityLocker.cs | 93 +++--- docs/decisions.md | 34 +++ 4 files changed, 370 insertions(+), 41 deletions(-) create mode 100644 ErsatzTV.Infrastructure.Tests/Locking/EntityLockerTests.cs diff --git a/ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs b/ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs index d1d33f790..880bc022c 100644 --- a/ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs +++ b/ErsatzTV.Core/Interfaces/Locking/IEntityLocker.cs @@ -1,5 +1,19 @@ namespace ErsatzTV.Core.Interfaces.Locking; +/// +/// Advisory "an operation on entity X is in progress" mutex layer, registered as a process-wide singleton. +/// Each lock kind is a single exclusive slot (not reentrant or counted): a second acquire while held returns false. +/// +/// +/// Ownership contract (see docs/decisions.md 2026-07-11, issue #231): every Lock* call returns +/// true iff it transitioned the slot to locked. A true confers ownership of exactly one release — +/// performed either in the acquiring scope (release in a finally gated on the captured return value) or by +/// the single designated releaser it hands off to (the consumer of a message enqueued while holding the lock, with +/// a compensating unlock if the enqueue itself throws). Callers must never release a lock they did not acquire. +/// Unlock* on an already-unlocked slot is a no-op that returns false, raises no event, and logs a +/// warning — the tripwire for double-release / non-owner-release bugs; it deliberately does not throw, so the +/// primitive stays safe to call from finally paths. +/// public interface IEntityLocker { event EventHandler OnLibraryChanged; diff --git a/ErsatzTV.Infrastructure.Tests/Locking/EntityLockerTests.cs b/ErsatzTV.Infrastructure.Tests/Locking/EntityLockerTests.cs new file mode 100644 index 000000000..008ef98cc --- /dev/null +++ b/ErsatzTV.Infrastructure.Tests/Locking/EntityLockerTests.cs @@ -0,0 +1,270 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Notifications; +using ErsatzTV.Infrastructure.Locking; +using MediatR; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Infrastructure.Tests.Locking; + +// Verifies the atomicity + single-owner release contract from docs/decisions.md 2026-07-11 (issue #231). +// These run against the real EntityLocker (not a mock) so the Interlocked/ConcurrentDictionary state is exercised. +[TestFixture] +public class EntityLockerTests +{ + private const int ParallelCallers = 64; + + private static EntityLocker CreateLocker() => + new(Substitute.For(), NullLogger.Instance); + + // Every exclusive single-slot kind, adapted to a uniform bool lock/unlock/is-locked shape. + // Library bakes in a fixed id; RemoteMediaSource a concrete media-source type. Playout is async + // (tested separately) and so is not in this source. + private static IEnumerable ExclusiveWinnerKinds() + { + foreach (LockKind kind in EventCountingKinds()) + { + yield return kind; + } + + yield return new LockKind + { + Name = "RemoteMediaSource", + Lock = l => l.LockRemoteMediaSource(), + Unlock = l => l.UnlockRemoteMediaSource(), + IsLocked = l => l.IsRemoteMediaSourceLocked() + }; + } + + // The kinds whose change event is a plain EventHandler, so a subscriber can count transitions. + private static IEnumerable EventCountingKinds() + { + yield return new LockKind + { + Name = "Plex", + Lock = l => l.LockPlex(), + Unlock = l => l.UnlockPlex(), + IsLocked = l => l.IsPlexLocked(), + Subscribe = (l, h) => l.OnPlexChanged += h + }; + yield return new LockKind + { + Name = "Trakt", + Lock = l => l.LockTrakt(), + Unlock = l => l.UnlockTrakt(), + IsLocked = l => l.IsTraktLocked(), + Subscribe = (l, h) => l.OnTraktChanged += h + }; + yield return new LockKind + { + Name = "EmbyCollections", + Lock = l => l.LockEmbyCollections(), + Unlock = l => l.UnlockEmbyCollections(), + IsLocked = l => l.AreEmbyCollectionsLocked(), + Subscribe = (l, h) => l.OnEmbyCollectionsChanged += h + }; + yield return new LockKind + { + Name = "JellyfinCollections", + Lock = l => l.LockJellyfinCollections(), + Unlock = l => l.UnlockJellyfinCollections(), + IsLocked = l => l.AreJellyfinCollectionsLocked(), + Subscribe = (l, h) => l.OnJellyfinCollectionsChanged += h + }; + yield return new LockKind + { + Name = "PlexCollections", + Lock = l => l.LockPlexCollections(), + Unlock = l => l.UnlockPlexCollections(), + IsLocked = l => l.ArePlexCollectionsLocked(), + Subscribe = (l, h) => l.OnPlexCollectionsChanged += h + }; + yield return new LockKind + { + Name = "TroubleshootingPlayback", + Lock = l => l.LockTroubleshootingPlayback(), + Unlock = l => l.UnlockTroubleshootingPlayback(), + IsLocked = l => l.IsTroubleshootingPlaybackLocked(), + Subscribe = (l, h) => l.OnTroubleshootingPlaybackChanged += h + }; + yield return new LockKind + { + Name = "Library", + Lock = l => l.LockLibrary(1), + Unlock = l => l.UnlockLibrary(1), + IsLocked = l => l.IsLibraryLocked(1), + Subscribe = (l, h) => l.OnLibraryChanged += h + }; + } + + // Stress the acquire race over many rounds: a single-round 64-thread barrier does NOT reliably + // collide the non-atomic check-then-set window on fast hardware (verified: it green-lit a + // deliberately broken flag), so this hammers Lock* for thousands of rounds — each round every + // thread races one acquire, exactly one must win, then the winner releases to reset. Any round + // with a winner count != 1 fails the whole test. Against the atomic CAS this is invariant; + // against a non-atomic flag some round double-acquires. + [Test] + [TestCaseSource(nameof(ExclusiveWinnerKinds))] + public void Lock_ParallelCallers_ExactlyOneWinner(LockKind kind) + { + const int threads = 8; + const int rounds = 20_000; + + EntityLocker locker = CreateLocker(); + var winners = 0; + var badRounds = 0; + + using var startRound = new Barrier(threads); + using var endRound = new Barrier( + threads, + _ => + { + if (Volatile.Read(ref winners) != 1) + { + Interlocked.Increment(ref badRounds); + } + + // reset for the next round: release the (single) held lock and clear the counter + kind.Unlock(locker); + Volatile.Write(ref winners, 0); + }); + + var workers = new Thread[threads]; + for (var t = 0; t < threads; t++) + { + workers[t] = new Thread(() => + { + for (var r = 0; r < rounds; r++) + { + startRound.SignalAndWait(); + if (kind.Lock(locker)) + { + Interlocked.Increment(ref winners); + } + + endRound.SignalAndWait(); + } + }); + workers[t].Start(); + } + + foreach (Thread worker in workers) + { + worker.Join(); + } + + badRounds.ShouldBe(0); + kind.IsLocked(locker).ShouldBeFalse(); + } + + [Test] + public async Task LockPlayout_ParallelCallers_ExactlyOneWinner_PublishesOnce() + { + var mediator = Substitute.For(); + var locker = new EntityLocker(mediator, NullLogger.Instance); + const int playoutId = 7; + using var barrier = new Barrier(ParallelCallers); + + Task[] tasks = Enumerable.Range(0, ParallelCallers) + .Select(_ => Task.Run(async () => + { + barrier.SignalAndWait(); + return await locker.LockPlayout(playoutId); + })) + .ToArray(); + + bool[] results = await Task.WhenAll(tasks); + + results.Count(r => r).ShouldBe(1); + locker.IsPlayoutLocked(playoutId).ShouldBeTrue(); + await mediator.Received(1).Publish( + Arg.Is(n => n.PlayoutId == playoutId && n.IsLocked), + Arg.Any()); + } + + [Test] + [TestCaseSource(nameof(EventCountingKinds))] + public void Lock_WhenAlreadyLocked_ReturnsFalse_AndFiresNoSecondEvent(LockKind kind) + { + EntityLocker locker = CreateLocker(); + var eventCount = 0; + kind.Subscribe!(locker, (_, _) => eventCount++); + + kind.Lock(locker).ShouldBeTrue(); + kind.Lock(locker).ShouldBeFalse(); + + kind.IsLocked(locker).ShouldBeTrue(); + eventCount.ShouldBe(1); + } + + [Test] + [TestCaseSource(nameof(EventCountingKinds))] + public void Unlock_WhenNotLocked_ReturnsFalse_AndFiresNoEvent(LockKind kind) + { + EntityLocker locker = CreateLocker(); + var eventCount = 0; + kind.Subscribe!(locker, (_, _) => eventCount++); + + kind.Unlock(locker).ShouldBeFalse(); + + eventCount.ShouldBe(0); + kind.IsLocked(locker).ShouldBeFalse(); + } + + [Test] + [TestCaseSource(nameof(EventCountingKinds))] + public void Unlock_AfterLock_ReturnsTrue_ThenSecondUnlockReturnsFalse(LockKind kind) + { + EntityLocker locker = CreateLocker(); + var eventCount = 0; + kind.Subscribe!(locker, (_, _) => eventCount++); + + kind.Lock(locker).ShouldBeTrue(); + kind.Unlock(locker).ShouldBeTrue(); + kind.Unlock(locker).ShouldBeFalse(); + + kind.IsLocked(locker).ShouldBeFalse(); + eventCount.ShouldBe(2); // one lock transition + one unlock transition, the no-op unlock fires nothing + } + + [Test] + [TestCaseSource(nameof(EventCountingKinds))] + public async Task ParallelUnlock_AfterSingleLock_ExactlyOneReleaser(LockKind kind) + { + EntityLocker locker = CreateLocker(); + kind.Lock(locker).ShouldBeTrue(); + + var unlockEvents = 0; + kind.Subscribe!(locker, (_, _) => Interlocked.Increment(ref unlockEvents)); + using var barrier = new Barrier(ParallelCallers); + + Task[] tasks = Enumerable.Range(0, ParallelCallers) + .Select(_ => Task.Run(() => + { + barrier.SignalAndWait(); + return kind.Unlock(locker); + })) + .ToArray(); + + bool[] results = await Task.WhenAll(tasks); + + results.Count(r => r).ShouldBe(1); + kind.IsLocked(locker).ShouldBeFalse(); + unlockEvents.ShouldBe(1); + } + + public sealed class LockKind + { + public required string Name { get; init; } + public required Func Lock { get; init; } + public required Func Unlock { get; init; } + public required Func IsLocked { get; init; } + + // null for kinds whose change event is not a plain EventHandler (RemoteMediaSource is EventHandler). + public Action? Subscribe { get; init; } + + public override string ToString() => Name; + } +} diff --git a/ErsatzTV.Infrastructure/Locking/EntityLocker.cs b/ErsatzTV.Infrastructure/Locking/EntityLocker.cs index 006202021..eb3cea7c6 100644 --- a/ErsatzTV.Infrastructure/Locking/EntityLocker.cs +++ b/ErsatzTV.Infrastructure/Locking/EntityLocker.cs @@ -2,20 +2,25 @@ using ErsatzTV.Core.Interfaces.Locking; using ErsatzTV.Core.Notifications; using MediatR; +using Microsoft.Extensions.Logging; namespace ErsatzTV.Infrastructure.Locking; -public class EntityLocker(IMediator mediator) : IEntityLocker +public class EntityLocker(IMediator mediator, ILogger logger) : IEntityLocker { private readonly ConcurrentDictionary _lockedLibraries = new(); private readonly ConcurrentDictionary _lockedPlayouts = new(); private readonly ConcurrentDictionary _lockedRemoteMediaSourceTypes = new(); - private bool _embyCollections; - private bool _jellyfinCollections; - private bool _plex; - private bool _plexCollections; - private bool _trakt; - private bool _troubleshootingPlayback; + + // 0 = unlocked, 1 = locked. Mutated only via Interlocked.CompareExchange so the caller that wins + // the 0 -> 1 transition is the sole owner and the only one that fires the change event + // (see docs/decisions.md 2026-07-11, issue #231). + private int _embyCollections; + private int _jellyfinCollections; + private int _plex; + private int _plexCollections; + private int _trakt; + private int _troubleshootingPlayback; public event EventHandler OnLibraryChanged; public event EventHandler OnPlexChanged; @@ -28,7 +33,7 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool LockLibrary(int libraryId) { - if (!_lockedLibraries.ContainsKey(libraryId) && _lockedLibraries.TryAdd(libraryId, 0)) + if (_lockedLibraries.TryAdd(libraryId, 0)) { OnLibraryChanged?.Invoke(this, EventArgs.Empty); return true; @@ -45,6 +50,9 @@ public class EntityLocker(IMediator mediator) : IEntityLocker return true; } + logger.LogWarning( + "UnlockLibrary called for library {LibraryId} that was not locked (double-release or non-owner release)", + libraryId); return false; } @@ -53,9 +61,8 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool LockPlex() { - if (!_plex) + if (Interlocked.CompareExchange(ref _plex, 1, 0) == 0) { - _plex = true; OnPlexChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -65,17 +72,17 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockPlex() { - if (_plex) + if (Interlocked.CompareExchange(ref _plex, 0, 1) == 1) { - _plex = false; OnPlexChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning("UnlockPlex called but Plex was not locked (double-release or non-owner release)"); return false; } - public bool IsPlexLocked() => _plex; + public bool IsPlexLocked() => Volatile.Read(ref _plex) == 1; public bool IsRemoteMediaSourceLocked() => _lockedRemoteMediaSourceTypes.ContainsKey(typeof(TMediaSource)); @@ -84,8 +91,7 @@ public class EntityLocker(IMediator mediator) : IEntityLocker { Type mediaSourceType = typeof(TMediaSource); - if (!_lockedRemoteMediaSourceTypes.ContainsKey(mediaSourceType) && - _lockedRemoteMediaSourceTypes.TryAdd(mediaSourceType, 0)) + if (_lockedRemoteMediaSourceTypes.TryAdd(mediaSourceType, 0)) { OnRemoteMediaSourceChanged?.Invoke(this, mediaSourceType); return true; @@ -104,14 +110,16 @@ public class EntityLocker(IMediator mediator) : IEntityLocker return true; } + logger.LogWarning( + "UnlockRemoteMediaSource called for {MediaSourceType} that was not locked (double-release or non-owner release)", + mediaSourceType.Name); return false; } public bool LockTrakt() { - if (!_trakt) + if (Interlocked.CompareExchange(ref _trakt, 1, 0) == 0) { - _trakt = true; OnTraktChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -121,23 +129,22 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockTrakt() { - if (_trakt) + if (Interlocked.CompareExchange(ref _trakt, 0, 1) == 1) { - _trakt = false; OnTraktChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning("UnlockTrakt called but Trakt was not locked (double-release or non-owner release)"); return false; } - public bool IsTraktLocked() => _trakt; + public bool IsTraktLocked() => Volatile.Read(ref _trakt) == 1; public bool LockEmbyCollections() { - if (!_embyCollections) + if (Interlocked.CompareExchange(ref _embyCollections, 1, 0) == 0) { - _embyCollections = true; OnEmbyCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -147,23 +154,23 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockEmbyCollections() { - if (_embyCollections) + if (Interlocked.CompareExchange(ref _embyCollections, 0, 1) == 1) { - _embyCollections = false; OnEmbyCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning( + "UnlockEmbyCollections called but Emby collections were not locked (double-release or non-owner release)"); return false; } - public bool AreEmbyCollectionsLocked() => _embyCollections; + public bool AreEmbyCollectionsLocked() => Volatile.Read(ref _embyCollections) == 1; public bool LockJellyfinCollections() { - if (!_jellyfinCollections) + if (Interlocked.CompareExchange(ref _jellyfinCollections, 1, 0) == 0) { - _jellyfinCollections = true; OnJellyfinCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -173,23 +180,23 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockJellyfinCollections() { - if (_jellyfinCollections) + if (Interlocked.CompareExchange(ref _jellyfinCollections, 0, 1) == 1) { - _jellyfinCollections = false; OnJellyfinCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning( + "UnlockJellyfinCollections called but Jellyfin collections were not locked (double-release or non-owner release)"); return false; } - public bool AreJellyfinCollectionsLocked() => _jellyfinCollections; + public bool AreJellyfinCollectionsLocked() => Volatile.Read(ref _jellyfinCollections) == 1; public bool LockPlexCollections() { - if (!_plexCollections) + if (Interlocked.CompareExchange(ref _plexCollections, 1, 0) == 0) { - _plexCollections = true; OnPlexCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -199,21 +206,22 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockPlexCollections() { - if (_plexCollections) + if (Interlocked.CompareExchange(ref _plexCollections, 0, 1) == 1) { - _plexCollections = false; OnPlexCollectionsChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning( + "UnlockPlexCollections called but Plex collections were not locked (double-release or non-owner release)"); return false; } - public bool ArePlexCollectionsLocked() => _plexCollections; + public bool ArePlexCollectionsLocked() => Volatile.Read(ref _plexCollections) == 1; public async Task LockPlayout(int playoutId) { - if (!_lockedPlayouts.ContainsKey(playoutId) && _lockedPlayouts.TryAdd(playoutId, 0)) + if (_lockedPlayouts.TryAdd(playoutId, 0)) { await mediator.Publish(new PlayoutUpdatedNotification(playoutId, true)); return true; @@ -230,6 +238,9 @@ public class EntityLocker(IMediator mediator) : IEntityLocker return true; } + logger.LogWarning( + "UnlockPlayout called for playout {PlayoutId} that was not locked (double-release or non-owner release)", + playoutId); return false; } @@ -237,9 +248,8 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool LockTroubleshootingPlayback() { - if (!_troubleshootingPlayback) + if (Interlocked.CompareExchange(ref _troubleshootingPlayback, 1, 0) == 0) { - _troubleshootingPlayback = true; OnTroubleshootingPlaybackChanged?.Invoke(this, EventArgs.Empty); return true; } @@ -249,15 +259,16 @@ public class EntityLocker(IMediator mediator) : IEntityLocker public bool UnlockTroubleshootingPlayback() { - if (_troubleshootingPlayback) + if (Interlocked.CompareExchange(ref _troubleshootingPlayback, 0, 1) == 1) { - _troubleshootingPlayback = false; OnTroubleshootingPlaybackChanged?.Invoke(this, EventArgs.Empty); return true; } + logger.LogWarning( + "UnlockTroubleshootingPlayback called but troubleshooting playback was not locked (double-release or non-owner release)"); return false; } - public bool IsTroubleshootingPlaybackLocked() => _troubleshootingPlayback; + public bool IsTroubleshootingPlaybackLocked() => Volatile.Read(ref _troubleshootingPlayback) == 1; } diff --git a/docs/decisions.md b/docs/decisions.md index d74609ce1..209d2c2f2 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -399,3 +399,37 @@ Every new issue gets an explicit end-of-session triage verdict — gate-blocker slot) or backlog (label only) — so review findings adjust the queue only through that step and the arc doesn't drift. The handoff file keeps only the **static kickoff prompt** and the **append-only Lessons lore** (per-session prompts are gone; task context lives in issue bodies). + +## 2026-07-11 — EntityLocker: atomic flags + single-owner release discipline, no owner tokens (#231) + +`EntityLocker` (process-wide singleton, `ErsatzTV.Infrastructure/Locking/EntityLocker.cs`) is the +advisory "operation on entity X is in progress" mutex layer. Adversarial review (#20/F5, →#231) +found three defects: six plain-`bool` flags with a non-atomic check-then-set (two threads could both +acquire and both return `true`), tokenless `Unlock*` letting any caller release another owner's lock +(e.g. `BuildPlayoutHandler` ignored `LockPlayout`'s return then unconditionally unlocked in +`finally`), and one batch taking a single `LockLibrary` released after the *first* of two enqueued +scan units (so the second ran unlocked). + +**Model chosen: tokenless atomic flag + documented single-owner release discipline.** The six bools +became `int`s guarded by `Interlocked.CompareExchange` (0/1), so `Lock*`'s `bool` return is now a +reliable "this call won the transition" signal and the change event fires exactly once per +transition. The contract (XML-doc'd on `IEntityLocker`): a `true` from `Lock*` confers ownership of +exactly one release — performed either in the acquiring scope (`finally`, gated on the captured +bool) or by the single designated releaser the acquirer hands off to (the consumer of the message +enqueued while holding the lock, with a compensating unlock if the enqueue throws — the established +`TraktController.EnqueueWithTraktLock` / scheduler `unlock: last` tail pattern). Callers must never +release a lock they did not acquire. `Unlock*` on an already-unlocked slot returns `false`, fires no +event, and logs a Warning — the loud tripwire for double-release bugs; it does not throw or +`Debug.Assert`, because an advisory flag must stay safe to release in `finally` paths. The three +`ConcurrentDictionary`-backed kinds (Library/Playout/RemoteMediaSource) were already atomic +(`TryAdd`/`TryRemove`) and kept their semantics (the redundant `ContainsKey` pre-checks were dropped +as tidy-up); the interface signature is unchanged across its ~40 call sites. + +**Rejected:** owner tokens/leases — the acquirer and releaser for Library/Trakt/Plex/Collections +locks are different code correlated only by entity id across in-memory `Channel` queues, so a +token would have to travel inside ~8 background-request message types for a defect that discipline +plus the now-trustworthy atomic return value already prevents. Counted/reentrant locks — wrong +semantics: these are exclusive in-progress flags; two holders is the failure mode, not a feature. +Call-site fixes this model prescribes land separately: #232 (scan lifecycle — enqueue only after a +successful lock, compensating unlock on enqueue failure, one release per acquisition in batches) and +#234 (BuildPlayout/subtitles gate their `finally` unlock on the captured acquire result). -- 2.47.3 From 088644e8c01a0f6cc443ad9805a8308e01b0d159 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:38:08 +0200 Subject: [PATCH 2/3] feat(libraries): honest scan lifecycle + one-lock-one-release discipline (#232) Scan queue handler now returns a QueueLibraryScanResult enum (Queued|NotFound|SyncDisabled|AlreadyScanning) instead of a lying bool; LibrariesController.ScanLibrary maps them to 202/404/422/409 with ProblemDetails. Guard the lock->enqueue with the EnqueueWithTraktLock compensating-unlock pattern. ScannerService now releases every library/collection lock in a finally so a handler exception can't leak the lock. Plex "Shows" scheduler batch (one lock, two messages) now has only the trailing SynchronizePlexNetworks carry the single release (Unlock flag), mirroring the scheduler Trakt tail-token precedent. Guard the other lock->enqueue producers (Create/UpdateLocalLibrary, UpdateTraktList) with compensating unlock. SPA drops the PENDING_GRACE_TICKS heuristic now that the POST reports 202/409/404/422 directly: 202 -> pending+poll, 409 -> reconcile (no error toast), 404/422 -> surface error. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/CreateLocalLibraryHandler.cs | 12 +- .../Commands/QueueLibraryScanByLibraryId.cs | 10 +- .../QueueLibraryScanByLibraryIdHandler.cs | 29 +- .../Commands/UpdateLocalLibraryHandler.cs | 12 +- .../Commands/UpdateTraktListHandler.cs | 12 +- .../Commands/SynchronizePlexLibraryById.cs | 9 +- .../Plex/Commands/SynchronizePlexNetworks.cs | 8 +- ...QueueLibraryScanByLibraryIdHandlerTests.cs | 224 +++++++++++ .../Controllers/LibrariesControllerTests.cs | 50 +++ .../OpenApiErrorResponseContractTests.cs | 3 + .../Controllers/Api/LibrariesController.cs | 27 +- ErsatzTV/Services/ScannerService.cs | 370 ++++++++++-------- ErsatzTV/Services/SchedulerService.cs | 8 +- ErsatzTV/wwwroot/openapi/v1.json | 64 ++- docs/api-conventions.md | 16 + web/src/App.test.tsx | 92 ++--- web/src/api/libraries.ts | 136 +++---- 17 files changed, 756 insertions(+), 326 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs diff --git a/ErsatzTV.Application/Libraries/Commands/CreateLocalLibraryHandler.cs b/ErsatzTV.Application/Libraries/Commands/CreateLocalLibraryHandler.cs index 4a575af29..7dc41a2c1 100644 --- a/ErsatzTV.Application/Libraries/Commands/CreateLocalLibraryHandler.cs +++ b/ErsatzTV.Application/Libraries/Commands/CreateLocalLibraryHandler.cs @@ -44,7 +44,17 @@ public class CreateLocalLibraryHandler : LocalLibraryHandlerBase, if (_entityLocker.LockLibrary(localLibrary.Id)) { - await _scannerWorkerChannel.WriteAsync(new ForceScanLocalLibrary(localLibrary.Id)); + try + { + await _scannerWorkerChannel.WriteAsync(new ForceScanLocalLibrary(localLibrary.Id)); + } + catch + { + // the scanner only unlocks when it receives the message; if the enqueue fails + // after we acquired the lock, release it here or it is held forever. + _entityLocker.UnlockLibrary(localLibrary.Id); + throw; + } } return ProjectToViewModel(localLibrary); diff --git a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs index b01cf6db3..024b47be3 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs @@ -1,3 +1,11 @@ namespace ErsatzTV.Application.Libraries; -public record QueueLibraryScanByLibraryId(int LibraryId) : IRequest; +public enum QueueLibraryScanResult +{ + Queued, + NotFound, + SyncDisabled, + AlreadyScanning +} + +public record QueueLibraryScanByLibraryId(int LibraryId) : IRequest; diff --git a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs index 2b62826c7..6bb6a5f4b 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs @@ -17,9 +17,11 @@ public class QueueLibraryScanByLibraryIdHandler( IEntityLocker locker, ChannelWriter scannerWorker, ILogger logger) - : IRequestHandler + : IRequestHandler { - public async Task Handle(QueueLibraryScanByLibraryId request, CancellationToken cancellationToken) + public async Task Handle( + QueueLibraryScanByLibraryId request, + CancellationToken cancellationToken) { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); @@ -40,10 +42,17 @@ public class QueueLibraryScanByLibraryIdHandler( if (!shouldSyncItems) { logger.LogWarning("Library sync is disabled for library id {Id}", library.Id); - return false; + return QueueLibraryScanResult.SyncDisabled; } - if (locker.LockLibrary(library.Id)) + // A true from LockLibrary confers ownership of exactly one release; a false means a scan + // is already in progress and we own no release. + if (!locker.LockLibrary(library.Id)) + { + return QueueLibraryScanResult.AlreadyScanning; + } + + try { logger.LogDebug("Queued library scan for library id {Id}", library.Id); @@ -78,10 +87,18 @@ public class QueueLibraryScanByLibraryIdHandler( break; } } + catch + { + // the scanner only unlocks when it receives the message; if enqueueing fails + // (e.g. request aborted / channel completed) after we acquired the lock, release + // it here or it is held forever (EnqueueWithTraktLock pattern). + locker.UnlockLibrary(library.Id); + throw; + } - return true; + return QueueLibraryScanResult.Queued; } - return false; + return QueueLibraryScanResult.NotFound; } } diff --git a/ErsatzTV.Application/Libraries/Commands/UpdateLocalLibraryHandler.cs b/ErsatzTV.Application/Libraries/Commands/UpdateLocalLibraryHandler.cs index 7914d667e..b20728c12 100644 --- a/ErsatzTV.Application/Libraries/Commands/UpdateLocalLibraryHandler.cs +++ b/ErsatzTV.Application/Libraries/Commands/UpdateLocalLibraryHandler.cs @@ -96,7 +96,17 @@ public class UpdateLocalLibraryHandler : LocalLibraryHandlerBase, if (_entityLocker.LockLibrary(existing.Id)) { - await _scannerWorkerChannel.WriteAsync(new ForceScanLocalLibrary(existing.Id)); + try + { + await _scannerWorkerChannel.WriteAsync(new ForceScanLocalLibrary(existing.Id)); + } + catch + { + // the scanner only unlocks when it receives the message; if the enqueue fails + // after we acquired the lock, release it here or it is held forever. + _entityLocker.UnlockLibrary(existing.Id); + throw; + } } } diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs index e7a486ce7..772d5ed62 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateTraktListHandler.cs @@ -55,7 +55,17 @@ public class UpdateTraktListHandler( if (entityLocker.LockTrakt()) { - await workerChannel.WriteAsync(new MatchTraktListItems(traktList.Id), cancellationToken); + try + { + await workerChannel.WriteAsync(new MatchTraktListItems(traktList.Id), cancellationToken); + } + catch + { + // the background handler only unlocks when it receives the message; if the + // enqueue fails after we acquired the lock, release it here or it is held forever. + entityLocker.UnlockTrakt(); + throw; + } } } else if (traktList.PlaylistId is not null) diff --git a/ErsatzTV.Application/Plex/Commands/SynchronizePlexLibraryById.cs b/ErsatzTV.Application/Plex/Commands/SynchronizePlexLibraryById.cs index c9b704f36..67e21b5d9 100644 --- a/ErsatzTV.Application/Plex/Commands/SynchronizePlexLibraryById.cs +++ b/ErsatzTV.Application/Plex/Commands/SynchronizePlexLibraryById.cs @@ -7,15 +7,20 @@ public interface ISynchronizePlexLibraryById : IRequest false; public bool DeepScan => false; } -public record ForceSynchronizePlexLibraryById(int PlexLibraryId, bool DeepScan) : ISynchronizePlexLibraryById +public record ForceSynchronizePlexLibraryById(int PlexLibraryId, bool DeepScan, bool Unlock = true) + : ISynchronizePlexLibraryById { public bool ForceScan => true; } diff --git a/ErsatzTV.Application/Plex/Commands/SynchronizePlexNetworks.cs b/ErsatzTV.Application/Plex/Commands/SynchronizePlexNetworks.cs index f0730c1e1..ac2d150cc 100644 --- a/ErsatzTV.Application/Plex/Commands/SynchronizePlexNetworks.cs +++ b/ErsatzTV.Application/Plex/Commands/SynchronizePlexNetworks.cs @@ -2,5 +2,9 @@ using ErsatzTV.Core; namespace ErsatzTV.Application.Plex; -public record SynchronizePlexNetworks(int PlexLibraryId, bool ForceScan) : IRequest>, - IScannerBackgroundServiceRequest; +// Unlock defaults true so single-message callers release the library lock as before. In the Plex +// "Shows" scheduler batch this message runs LAST and carries the single release (the preceding +// library message runs with Unlock: false). +public record SynchronizePlexNetworks(int PlexLibraryId, bool ForceScan, bool Unlock = true) + : IRequest>, + IScannerBackgroundServiceRequest; diff --git a/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs b/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs new file mode 100644 index 000000000..c25cde87b --- /dev/null +++ b/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs @@ -0,0 +1,224 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Libraries; +using ErsatzTV.Application.MediaSources; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Locking; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using ThreadingChannel = System.Threading.Channels.Channel; + +namespace ErsatzTV.Tests.Application.Libraries; + +[TestFixture] +public class QueueLibraryScanByLibraryIdHandlerTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task Handle_Should_Return_NotFound_When_Library_Missing() + { + var locker = Substitute.For(); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult result = await handler.Handle( + new QueueLibraryScanByLibraryId(9999), + CancellationToken.None); + + result.ShouldBe(QueueLibraryScanResult.NotFound); + locker.DidNotReceive().LockLibrary(Arg.Any()); + } + + [Test] + public async Task Handle_Should_Return_SyncDisabled_When_Item_Sync_Off() + { + int libraryId = await SeedSyncDisabledPlexLibrary(); + + var locker = Substitute.For(); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult result = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId), + CancellationToken.None); + + result.ShouldBe(QueueLibraryScanResult.SyncDisabled); + locker.DidNotReceive().LockLibrary(Arg.Any()); + } + + [Test] + public async Task Handle_Should_Queue_And_Enqueue_When_Lock_Acquired() + { + int libraryId = await SeedLocalLibrary(); + + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult result = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId), + CancellationToken.None); + + result.ShouldBe(QueueLibraryScanResult.Queued); + locker.Received(1).LockLibrary(libraryId); + locker.DidNotReceive().UnlockLibrary(Arg.Any()); + channel.Reader.TryRead(out IScannerBackgroundServiceRequest? request).ShouldBeTrue(); + request.ShouldBeOfType(); + } + + [Test] + public async Task Handle_Should_Return_AlreadyScanning_When_Lock_Not_Acquired() + { + int libraryId = await SeedLocalLibrary(); + + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(false); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult result = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId), + CancellationToken.None); + + result.ShouldBe(QueueLibraryScanResult.AlreadyScanning); + channel.Reader.TryRead(out _).ShouldBeFalse(); + locker.DidNotReceive().UnlockLibrary(Arg.Any()); + } + + [Test] + public async Task Two_Concurrent_Scans_Should_Be_Queued_Then_AlreadyScanning() + { + int libraryId = await SeedLocalLibrary(); + + var locker = Substitute.For(); + // first acquire wins, second loses (a scan is already in progress) + locker.LockLibrary(libraryId).Returns(true, false); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult first = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId), + CancellationToken.None); + QueueLibraryScanResult second = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId), + CancellationToken.None); + + first.ShouldBe(QueueLibraryScanResult.Queued); + second.ShouldBe(QueueLibraryScanResult.AlreadyScanning); + } + + [Test] + public async Task Handle_Should_Unlock_And_Rethrow_When_Enqueue_Throws() + { + int libraryId = await SeedLocalLibrary(); + + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + Channel channel = ThreadingChannel.CreateUnbounded(); + // a completed writer makes WriteAsync throw ChannelClosedException, simulating a failed enqueue + channel.Writer.Complete(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + await Should.ThrowAsync(() => + handler.Handle(new QueueLibraryScanByLibraryId(libraryId), CancellationToken.None)); + + locker.Received(1).UnlockLibrary(libraryId); + } + + [Test] + public async Task Handle_Should_Unlock_When_Enqueue_Canceled() + { + int libraryId = await SeedLocalLibrary(); + + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + + // a writer whose enqueue is aborted (e.g. request cancelled after the lock was taken) + var writer = Substitute.For>(); + writer.WriteAsync(Arg.Any(), Arg.Any()) + .Returns(ValueTask.FromException(new OperationCanceledException())); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, writer); + + await Should.ThrowAsync(() => + handler.Handle(new QueueLibraryScanByLibraryId(libraryId), CancellationToken.None)); + + locker.Received(1).UnlockLibrary(libraryId); + } + + private QueueLibraryScanByLibraryIdHandler CreateHandler( + IEntityLocker locker, + ChannelWriter writer) => + new( + _db.Factory, + locker, + writer, + NullLogger.Instance); + + private async Task SeedLocalLibrary() + { + await using TvContext context = _db.CreateContext(); + var source = new LocalMediaSource + { + Libraries = + [ + new LocalLibrary + { + Name = "Local Movies", + MediaKind = LibraryMediaKind.Movies, + Paths = [] + } + ] + }; + await context.LocalMediaSources.AddAsync(source); + await context.SaveChangesAsync(); + return source.Libraries[0].Id; + } + + private async Task SeedSyncDisabledPlexLibrary() + { + await using TvContext context = _db.CreateContext(); + var source = new PlexMediaSource + { + ServerName = "Plex Server", + ProductVersion = "1", + Platform = "Linux", + PlatformVersion = "1", + ClientIdentifier = "plex", + Connections = [], + PathReplacements = [], + Libraries = + [ + new PlexLibrary + { + Name = "Plex Movies", + MediaKind = LibraryMediaKind.Movies, + Key = "1", + ShouldSyncItems = false, + Paths = [] + } + ] + }; + await context.PlexMediaSources.AddAsync(source); + await context.SaveChangesAsync(); + return source.Libraries[0].Id; + } +} diff --git a/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs b/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs index 0971c747c..95ebbdf6b 100644 --- a/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs @@ -95,4 +95,54 @@ public class LibrariesControllerTests result.ShouldBeOfType(); } + + [Test] + public async Task ScanLibrary_Should_Return_202_When_Queued() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueLibraryScanResult.Queued); + + IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(r => r.LibraryId == 7), + Arg.Any()); + } + + [Test] + public async Task ScanLibrary_Should_Return_404_When_NotFound() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueLibraryScanResult.NotFound); + + IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + + var notFound = result.ShouldBeOfType(); + notFound.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status404NotFound); + } + + [Test] + public async Task ScanLibrary_Should_Return_409_When_AlreadyScanning() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueLibraryScanResult.AlreadyScanning); + + IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + + var conflict = result.ShouldBeOfType(); + conflict.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status409Conflict); + } + + [Test] + public async Task ScanLibrary_Should_Return_422_When_SyncDisabled() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueLibraryScanResult.SyncDisabled); + + IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + + var unprocessable = result.ShouldBeOfType(); + unprocessable.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status422UnprocessableEntity); + } } diff --git a/ErsatzTV.Tests/Controllers/OpenApiErrorResponseContractTests.cs b/ErsatzTV.Tests/Controllers/OpenApiErrorResponseContractTests.cs index 58b7358e1..f4de94901 100644 --- a/ErsatzTV.Tests/Controllers/OpenApiErrorResponseContractTests.cs +++ b/ErsatzTV.Tests/Controllers/OpenApiErrorResponseContractTests.cs @@ -271,6 +271,9 @@ public class OpenApiErrorResponseContractTests [TestCase("/api/trakt/lists/{id}", "put", "422")] [TestCase("/api/troubleshoot/playback/subtitles/{mediaItemId}", "get", "404")] [TestCase("/api/libraries/{id}/scan-show", "post", "404")] + [TestCase("/api/libraries/{id}/scan", "post", "404")] + [TestCase("/api/libraries/{id}/scan", "post", "409")] + [TestCase("/api/libraries/{id}/scan", "post", "422")] public void Static_OpenApi_Should_Document_ProblemDetails_For_Api_Error_Responses( string path, string method, diff --git a/ErsatzTV/Controllers/Api/LibrariesController.cs b/ErsatzTV/Controllers/Api/LibrariesController.cs index 844a73859..d8525a52d 100644 --- a/ErsatzTV/Controllers/Api/LibrariesController.cs +++ b/ErsatzTV/Controllers/Api/LibrariesController.cs @@ -22,10 +22,29 @@ public class LibrariesController(ITelevisionRepository televisionRepository, IMe [HttpPost("/api/libraries/{id:int}/scan")] [Tags("Libraries")] [EndpointSummary("Scan library")] - public async Task ScanLibrary(int id) => - await mediator.Send(new QueueLibraryScanByLibraryId(id)) - ? new OkResult() - : new NotFoundResult(); + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] + public async Task ScanLibrary(int id, CancellationToken cancellationToken) + { + QueueLibraryScanResult result = await mediator.Send(new QueueLibraryScanByLibraryId(id), cancellationToken); + return result switch + { + QueueLibraryScanResult.Queued => new AcceptedResult(), + QueueLibraryScanResult.AlreadyScanning => ApiResults.ConflictProblem( + "Library scan in progress", + $"A scan for library {id} is already in progress."), + QueueLibraryScanResult.SyncDisabled => new UnprocessableEntityObjectResult( + new ProblemDetails + { + Status = StatusCodes.Status422UnprocessableEntity, + Title = "Library sync is disabled", + Detail = $"Item sync is disabled for library {id}." + }), + _ => ApiResults.NotFoundProblem($"Library {id} does not exist.") + }; + } [HttpPost("/api/libraries/{id:int}/scan-show")] [Tags("Libraries")] diff --git a/ErsatzTV/Services/ScannerService.cs b/ErsatzTV/Services/ScannerService.cs index a61ea5bd4..cafd5380b 100644 --- a/ErsatzTV/Services/ScannerService.cs +++ b/ErsatzTV/Services/ScannerService.cs @@ -104,31 +104,36 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either scanResult = await mediator.Send(request, cancellationToken); - scanResult.BiIter( - name => _logger.LogDebug( - "Done scanning local library {Library}", - name), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug( - "Scan is not required for local library {LibraryId} at this time", - request.LibraryId); - } - else - { - _logger.LogWarning( - "Unable to scan local library {LibraryId}: {Error}", - request.LibraryId, - error.Value); - } - }); - - if (entityLocker.IsLibraryLocked(request.LibraryId)) + try { - entityLocker.UnlockLibrary(request.LibraryId); + Either scanResult = await mediator.Send(request, cancellationToken); + scanResult.BiIter( + name => _logger.LogDebug( + "Done scanning local library {Library}", + name), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug( + "Scan is not required for local library {LibraryId} at this time", + request.LibraryId); + } + else + { + _logger.LogWarning( + "Unable to scan local library {LibraryId}: {Error}", + request.LibraryId, + error.Value); + } + }); + } + finally + { + if (entityLocker.IsLibraryLocked(request.LibraryId)) + { + entityLocker.UnlockLibrary(request.LibraryId); + } } } @@ -156,29 +161,36 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - name => _logger.LogDebug("Done synchronizing plex library {Name}", name), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug( - "Scan is not required for plex library {LibraryId} at this time", - request.PlexLibraryId); - } - else - { - _logger.LogWarning( - "Unable to synchronize plex library {LibraryId}: {Error}", - request.PlexLibraryId, - error.Value); - } - }); - - if (entityLocker.IsLibraryLocked(request.PlexLibraryId)) + try { - entityLocker.UnlockLibrary(request.PlexLibraryId); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + name => _logger.LogDebug("Done synchronizing plex library {Name}", name), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug( + "Scan is not required for plex library {LibraryId} at this time", + request.PlexLibraryId); + } + else + { + _logger.LogWarning( + "Unable to synchronize plex library {LibraryId}: {Error}", + request.PlexLibraryId, + error.Value); + } + }); + } + finally + { + // request.Unlock is false when a later message in the same batch (SynchronizePlexNetworks) + // owns the single release for this library lock. + if (request.Unlock && entityLocker.IsLibraryLocked(request.PlexLibraryId)) + { + entityLocker.UnlockLibrary(request.PlexLibraryId); + } } } @@ -190,24 +202,29 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - _ => _logger.LogDebug("Done synchronizing plex collections"), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug("Scan is not required for plex collections at this time"); - } - else - { - _logger.LogWarning("Unable to synchronize plex collections: {Error}", error.Value); - } - }); - - if (entityLocker.ArePlexCollectionsLocked()) + try { - entityLocker.UnlockPlexCollections(); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + _ => _logger.LogDebug("Done synchronizing plex collections"), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug("Scan is not required for plex collections at this time"); + } + else + { + _logger.LogWarning("Unable to synchronize plex collections: {Error}", error.Value); + } + }); + } + finally + { + if (entityLocker.ArePlexCollectionsLocked()) + { + entityLocker.UnlockPlexCollections(); + } } } @@ -219,29 +236,36 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - _ => _logger.LogDebug("Done synchronizing plex networks for library {LibraryId}", request.PlexLibraryId), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug( - "Scan is not required for plex networks in library {LibraryId} at this time", - request.PlexLibraryId); - } - else - { - _logger.LogWarning( - "Unable to synchronize plex networks for library {LibraryId}: {Error}", - request.PlexLibraryId, - error.Value); - } - }); - - if (entityLocker.IsLibraryLocked(request.PlexLibraryId)) + try { - entityLocker.UnlockLibrary(request.PlexLibraryId); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + _ => _logger.LogDebug( + "Done synchronizing plex networks for library {LibraryId}", + request.PlexLibraryId), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug( + "Scan is not required for plex networks in library {LibraryId} at this time", + request.PlexLibraryId); + } + else + { + _logger.LogWarning( + "Unable to synchronize plex networks for library {LibraryId}: {Error}", + request.PlexLibraryId, + error.Value); + } + }); + } + finally + { + if (request.Unlock && entityLocker.IsLibraryLocked(request.PlexLibraryId)) + { + entityLocker.UnlockLibrary(request.PlexLibraryId); + } } } @@ -269,29 +293,34 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - name => _logger.LogDebug("Done synchronizing jellyfin library {Name}", name), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug( - "Scan is not required for jellyfin library {LibraryId} at this time", - request.JellyfinLibraryId); - } - else - { - _logger.LogWarning( - "Unable to synchronize jellyfin library {LibraryId}: {Error}", - request.JellyfinLibraryId, - error.Value); - } - }); - - if (entityLocker.IsLibraryLocked(request.JellyfinLibraryId)) + try { - entityLocker.UnlockLibrary(request.JellyfinLibraryId); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + name => _logger.LogDebug("Done synchronizing jellyfin library {Name}", name), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug( + "Scan is not required for jellyfin library {LibraryId} at this time", + request.JellyfinLibraryId); + } + else + { + _logger.LogWarning( + "Unable to synchronize jellyfin library {LibraryId}: {Error}", + request.JellyfinLibraryId, + error.Value); + } + }); + } + finally + { + if (entityLocker.IsLibraryLocked(request.JellyfinLibraryId)) + { + entityLocker.UnlockLibrary(request.JellyfinLibraryId); + } } } @@ -303,24 +332,29 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - _ => _logger.LogDebug("Done synchronizing jellyfin collections"), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug("Scan is not required for jellyfin collections at this time"); - } - else - { - _logger.LogWarning("Unable to synchronize jellyfin collections: {Error}", error.Value); - } - }); - - if (entityLocker.AreJellyfinCollectionsLocked()) + try { - entityLocker.UnlockJellyfinCollections(); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + _ => _logger.LogDebug("Done synchronizing jellyfin collections"), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug("Scan is not required for jellyfin collections at this time"); + } + else + { + _logger.LogWarning("Unable to synchronize jellyfin collections: {Error}", error.Value); + } + }); + } + finally + { + if (entityLocker.AreJellyfinCollectionsLocked()) + { + entityLocker.UnlockJellyfinCollections(); + } } } @@ -346,29 +380,34 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - name => _logger.LogDebug("Done synchronizing emby library {Name}", name), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug( - "Scan is not required for emby library {LibraryId} at this time", - request.EmbyLibraryId); - } - else - { - _logger.LogWarning( - "Unable to synchronize emby library {LibraryId}: {Error}", - request.EmbyLibraryId, - error.Value); - } - }); - - if (entityLocker.IsLibraryLocked(request.EmbyLibraryId)) + try { - entityLocker.UnlockLibrary(request.EmbyLibraryId); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + name => _logger.LogDebug("Done synchronizing emby library {Name}", name), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug( + "Scan is not required for emby library {LibraryId} at this time", + request.EmbyLibraryId); + } + else + { + _logger.LogWarning( + "Unable to synchronize emby library {LibraryId}: {Error}", + request.EmbyLibraryId, + error.Value); + } + }); + } + finally + { + if (entityLocker.IsLibraryLocked(request.EmbyLibraryId)) + { + entityLocker.UnlockLibrary(request.EmbyLibraryId); + } } } @@ -380,24 +419,29 @@ public class ScannerService : BackgroundService IMediator mediator = scope.ServiceProvider.GetRequiredService(); IEntityLocker entityLocker = scope.ServiceProvider.GetRequiredService(); - Either result = await mediator.Send(request, cancellationToken); - result.BiIter( - _ => _logger.LogDebug("Done synchronizing emby collections"), - error => - { - if (error is ScanIsNotRequired) - { - _logger.LogDebug("Scan is not required for emby collections at this time"); - } - else - { - _logger.LogWarning("Unable to synchronize emby collections: {Error}", error.Value); - } - }); - - if (entityLocker.AreEmbyCollectionsLocked()) + try { - entityLocker.UnlockEmbyCollections(); + Either result = await mediator.Send(request, cancellationToken); + result.BiIter( + _ => _logger.LogDebug("Done synchronizing emby collections"), + error => + { + if (error is ScanIsNotRequired) + { + _logger.LogDebug("Scan is not required for emby collections at this time"); + } + else + { + _logger.LogWarning("Unable to synchronize emby collections: {Error}", error.Value); + } + }); + } + finally + { + if (entityLocker.AreEmbyCollectionsLocked()) + { + entityLocker.UnlockEmbyCollections(); + } } } } diff --git a/ErsatzTV/Services/SchedulerService.cs b/ErsatzTV/Services/SchedulerService.cs index 42e792c36..072c74deb 100644 --- a/ErsatzTV/Services/SchedulerService.cs +++ b/ErsatzTV/Services/SchedulerService.cs @@ -229,11 +229,15 @@ public class SchedulerService : BackgroundService if (_entityLocker.LockLibrary(library.Id)) { + // one LockLibrary ⇄ one release: when a networks scan follows, it (the last message) + // carries the release; the library message runs with Unlock: false. + bool networksFollow = library.MediaKind is LibraryMediaKind.Shows; + await _scannerWorkerChannel.WriteAsync( - new SynchronizePlexLibraryByIdIfNeeded(library.Id), + new SynchronizePlexLibraryByIdIfNeeded(library.Id, Unlock: !networksFollow), cancellationToken); - if (library.MediaKind is LibraryMediaKind.Shows) + if (networksFollow) { await _scannerWorkerChannel.WriteAsync( new SynchronizePlexNetworks(library.Id, false), diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index b2366655f..6657b3bf5 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -5416,8 +5416,68 @@ } ], "responses": { - "200": { - "description": "OK" + "202": { + "description": "Accepted" + }, + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "422": { + "description": "Unprocessable Entity", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } } } } diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 52375bffa..69f99086c 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -119,6 +119,22 @@ action. Precedent for the 409 shape: `TraktController` (its private `ConflictPro controller's list projection) rather than adding a push channel. The SPA reads it and, on a 409, refreshes the list to pick up the flag. +### 3b. Map a "queue a background job" outcome to status codes with an enum, not a `bool` + +When an endpoint *starts* a background operation guarded by an `IEntityLocker` lock, return an +**outcome enum from the handler** and map it in the controller — don't collapse distinct outcomes +into a lying `bool`/200. Exemplar (issue #232): `QueueLibraryScanByLibraryId` → +`QueueLibraryScanResult { Queued | NotFound | SyncDisabled | AlreadyScanning }`, mapped by +`LibrariesController.ScanLibrary` to **202** (`AcceptedResult`, queued), **404** +(`ApiResults.NotFoundProblem`), **422** (`UnprocessableEntityObjectResult` + `ProblemDetails`, a +domain precondition such as sync-disabled), and **409** (`ApiResults.ConflictProblem`, the lock is +already held = already scanning). Here the acquired lock **is** the running job, so +`LockLibrary(id) == false` means "already scanning" → 409 (a variant of §3a where the lock is the +operation itself, not a mutation racing it). Add `[ProducesResponseType]` for 202/404/409/422 and +`typeof(ProblemDetails)` on the error ones. **Guard the lock→enqueue** with the +`EnqueueWithTraktLock` compensating-unlock pattern (`TraktController`): if a `WriteAsync` throws +after a successful `Lock*`, `Unlock*` in a `catch` and rethrow — one lock ⇄ exactly one release. + `NotFoundError : BaseError` lives in `ErsatzTV.Core/Errors/NotFoundError.cs` — return it from a handler's validation when a lookup fails, so the controller-side mapping falls out for free. diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index e9d6289d4..0cf8e2809 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -1249,24 +1249,21 @@ describe('ChicoryTV SPA scaffold', () => { expect(clearCount).toBeGreaterThan(0); }); - it('re-enables the scan button and stops polling when a queued scan never appears (grace window expiry)', async () => { - const intervalHandlers: Array<() => void> = []; - let clearCount = 0; - vi.spyOn(window, 'setInterval').mockImplementation((handler: TimerHandler, timeout?: number) => { - if (typeof handler === 'function' && (timeout ?? 0) >= 10000) { - intervalHandlers.push(handler as () => void); - } - return intervalHandlers.length; - }); - vi.spyOn(window, 'clearInterval').mockImplementation(() => { - clearCount += 1; - }); - + it('reconciles a 409 "already scanning" against scan-status without an error toast (#232)', async () => { + // POST /scan now returns 409 when a scan is already in progress. That is benign - the SPA + // must NOT show an error; it reconciles against scan-status (which reports the active scan) + // and keeps the button disabled while that scan runs. mockDashboardApi({ - libraryScanStatuses: [], + libraryScanStatusSequence: [ + [], // initial screen load: nothing active yet + [{ libraryId: 31, percent: 0.3 }] // post-409 reconcile fetch: the in-progress scan + ], mediaSources: [ mediaSource({ libraries: [library({ id: 31, itemCount: 10, mediaKind: 'Movies', name: 'Movies' })] }) - ] + ], + mutationFailures: { + '/api/libraries/31/scan': { status: 409 } + } }); render(); @@ -1279,42 +1276,27 @@ describe('ChicoryTV SPA scaffold', () => { await waitFor(() => { expect(window.fetch).toHaveBeenCalledWith('/api/libraries/31/scan', expect.objectContaining({ method: 'POST' })); }); - await waitFor(() => { - expect(fetchCount('/api/libraries/scan-status')).toBe(2); - }); - expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); - expect(intervalHandlers.length).toBeGreaterThan(0); - // The id never shows up in scan-status. It survives a couple of ticks... - await runPollTick(intervalHandlers); - expect(fetchCount('/api/libraries/scan-status')).toBe(3); + // The reconcile fetch surfaces the already-running scan; the button stays disabled... + expect(await screen.findByText('30%')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); - // ...but expires once the grace window runs out, freeing the button and the poll. - await runPollTick(intervalHandlers); - expect(screen.getByRole('button', { name: 'Scan Movies' })).not.toBeDisabled(); - expect(clearCount).toBeGreaterThan(0); + // ...and no error toast is shown for the benign conflict. + expect(screen.queryByText(/Request failed/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Unable to scan/)).not.toBeInTheDocument(); }); - it('re-enables the scan button and stops polling when scan-status errors repeatedly after a trigger (grace window on error)', async () => { - const intervalHandlers: Array<() => void> = []; - let clearCount = 0; - vi.spyOn(window, 'setInterval').mockImplementation((handler: TimerHandler, timeout?: number) => { - if (typeof handler === 'function' && (timeout ?? 0) >= 10000) { - intervalHandlers.push(handler as () => void); - } - return intervalHandlers.length; - }); - vi.spyOn(window, 'clearInterval').mockImplementation(() => { - clearCount += 1; - }); - + it('surfaces a 422 "sync disabled" scan rejection and leaves the button enabled (#232)', async () => { + // POST /scan now returns 422 when item sync is disabled for the library. Nothing was queued, + // so the button must not be stuck disabled and the error must be surfaced. mockDashboardApi({ libraryScanStatuses: [], - libraryScanStatusFailAfterTrigger: true, mediaSources: [ mediaSource({ libraries: [library({ id: 31, itemCount: 10, mediaKind: 'Movies', name: 'Movies' })] }) - ] + ], + mutationFailures: { + '/api/libraries/31/scan': { detail: 'Item sync is disabled for library 31.', status: 422 } + } }); render(); @@ -1324,29 +1306,8 @@ describe('ChicoryTV SPA scaffold', () => { fireEvent.click(screen.getByRole('button', { name: 'Scan Movies' })); - await waitFor(() => { - expect(window.fetch).toHaveBeenCalledWith('/api/libraries/31/scan', expect.objectContaining({ method: 'POST' })); - }); - - // The immediate post-trigger status fetch errors (first grace-tick burn), but the - // button must stay disabled and the poll must stay armed - a transient failure must - // not be indistinguishable from "give up immediately". - await waitFor(() => { - expect(fetchCount('/api/libraries/scan-status')).toBe(2); - }); - expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); - expect(intervalHandlers.length).toBeGreaterThan(0); - - // scan-status keeps erroring on every poll tick... - await runPollTick(intervalHandlers); - expect(fetchCount('/api/libraries/scan-status')).toBe(3); - expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); - - // ...but the same grace budget burns down on failures too, so persistent failure - // eventually frees the button and stops the interval instead of polling forever. - await runPollTick(intervalHandlers); + expect(await screen.findByText('Item sync is disabled for library 31.')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Scan Movies' })).not.toBeDisabled(); - expect(clearCount).toBeGreaterThan(0); }); it('shows Libraries API errors and retries', async () => { @@ -3910,7 +3871,8 @@ function mockDashboardApi({ scanStatusShouldFail = true; } - return Promise.resolve(new Response(null, { status: 200 })); + // Backend contract (#232): a successful queue is 202 Accepted, not a lying 200. + return Promise.resolve(new Response(null, { status: 202 })); } if (path === '/api/playouts') { diff --git a/web/src/api/libraries.ts b/web/src/api/libraries.ts index b2b1e0d01..c771f1352 100644 --- a/web/src/api/libraries.ts +++ b/web/src/api/libraries.ts @@ -28,11 +28,6 @@ type LibrariesScreenState = | { data: null; error: string; status: 'error' } | { data: null; error: null; status: 'loading' }; -// A trigger'd scan disappears from the button-disabled set once either: it has been -// observed at least once in scan-status (promoted to "active"), or this many poll -// ticks pass without ever appearing (the scanner never picked it up / it failed silently). -const PENDING_GRACE_TICKS = 3; - export function getMediaSources(): Promise { return request('/api/media-sources'); } @@ -66,35 +61,17 @@ export function scanShow(libraryId: number, params: ScanShowParams): Promise, - graceTicks: Map, - isSeenActive: (libraryId: number) => boolean -): Set { +// Drops a pending id that has been observed active in scan-status; scan-status is authoritative +// from that point. POST /scan now returns 202 only for a real (forced) scan, so every pending id +// eventually appears in scan-status - no "give up after N polls" grace heuristic is needed +// (the POST tells the client directly when nothing was queued: 409/404/422 instead of a lying 200). +function prunePromotedPending(pendingIds: Set, isSeenActive: (libraryId: number) => boolean): Set { const nextPending = new Set(); pendingIds.forEach((libraryId) => { - if (isSeenActive(libraryId)) { - // Seen active at least once - normal active/inactive pruning takes over. - graceTicks.delete(libraryId); - return; + if (!isSeenActive(libraryId)) { + nextPending.add(libraryId); } - - const ticksRemaining = (graceTicks.get(libraryId) ?? PENDING_GRACE_TICKS) - 1; - - if (ticksRemaining <= 0) { - // Grace window expired without ever appearing in scan-status - give up on it. - graceTicks.delete(libraryId); - return; - } - - graceTicks.set(libraryId, ticksRemaining); - nextPending.add(libraryId); }); return nextPending; @@ -112,7 +89,6 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta // guard against double-submits without waiting for a render. const pendingIdsRef = useRef>(new Set()); const activeIdsRef = useRef>(new Set()); - const pendingGraceTicksRef = useRef>(new Map()); useEffect(() => { activeRef.current = true; @@ -160,13 +136,10 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta const hadScanInProgress = hadScanInProgressRef.current; hadScanInProgressRef.current = scanStatuses.length > 0; - // Compute the pruned/promoted pending set - and write the grace-tick and pending - // refs - OUTSIDE the setState updater. Updaters must be pure: StrictMode double- - // invokes them (which would double-decrement grace ticks) and concurrent rendering - // may invoke-and-discard one. Mirrors how activeIdsRef is written above. - const nextPending = pruneGraceExpiredPending(pendingIdsRef.current, pendingGraceTicksRef.current, (libraryId) => - activeScanIds.has(libraryId) - ); + // Compute the promoted pending set - and write the pending ref - OUTSIDE the setState + // updater. Updaters must be pure (StrictMode double-invokes them and concurrent rendering + // may invoke-and-discard one). Mirrors how activeIdsRef is written above. + const nextPending = prunePromotedPending(pendingIdsRef.current, (libraryId) => activeScanIds.has(libraryId)); pendingIdsRef.current = nextPending; @@ -188,32 +161,10 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta } }) .catch(() => { - if (!activeRef.current) { - return; - } - - // A status-fetch failure never promotes a pending id to active, so it never - // survives via "seen active" - it just burns down the same grace budget as a - // successful poll that never saw it. This keeps a persistently-erroring endpoint - // from leaving the scan button disabled and the poll interval running forever. - // Active-scan state (activeIdsRef / scanStatuses) is left untouched: a transient - // failure must not kill an in-progress scan's UI. - const nextPending = pruneGraceExpiredPending(pendingIdsRef.current, pendingGraceTicksRef.current, () => false); - - pendingIdsRef.current = nextPending; - - setState((current) => { - if (current.status !== 'success') { - return current; - } - - return { - data: current.data, - error: current.error, - pendingLibraryIds: nextPending, - status: 'success' - }; - }); + // A transient status-fetch failure is left to resolve on the next poll: pending and + // active-scan state are untouched so an in-progress scan's UI is not disturbed. A 202 + // is only returned for a real scan, so pending ids reconcile against scan-status on the + // next successful poll rather than needing a give-up budget here. }); }, [loadSources]); @@ -227,7 +178,6 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta hadScanInProgressRef.current = scanStatuses.length > 0; pendingIdsRef.current = new Set(); activeIdsRef.current = new Set(scanStatuses.map((scan) => scan.libraryId)); - pendingGraceTicksRef.current = new Map(); setState({ data: { scanStatuses, sources }, error: null, @@ -266,14 +216,40 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta load(); }, [load]); + const clearPending = useCallback((libraryId: number) => { + const pendingIds = new Set(pendingIdsRef.current); + pendingIds.delete(libraryId); + pendingIdsRef.current = pendingIds; + + if (!activeRef.current) { + return; + } + + setState((current) => { + if (current.status !== 'success') { + return current; + } + + const pendingLibraryIds = new Set(current.pendingLibraryIds); + pendingLibraryIds.delete(libraryId); + + return { + data: current.data, + error: current.error, + pendingLibraryIds, + status: 'success' + }; + }); + }, []); + const triggerScan = useCallback((libraryId: number): Promise => { if (pendingIdsRef.current.has(libraryId) || activeIdsRef.current.has(libraryId)) { // Already pending or active - ignore the duplicate submission. return Promise.resolve(); } + // Optimistically mark pending; the POST response tells us how to reconcile. pendingIdsRef.current = new Set(pendingIdsRef.current).add(libraryId); - pendingGraceTicksRef.current.set(libraryId, PENDING_GRACE_TICKS); setState((current) => { if (current.status !== 'success') { @@ -289,12 +265,23 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta }); return scanLibrary(libraryId) - .then(() => loadScanStatuses()) + .then(() => { + // 202 Accepted - a scan is genuinely queued. Poll scan-status; the pending flag is + // promoted to "active" once the scan appears there. + void loadScanStatuses(); + }) .catch((error: unknown) => { - const pendingIds = new Set(pendingIdsRef.current); - pendingIds.delete(libraryId); - pendingIdsRef.current = pendingIds; - pendingGraceTicksRef.current.delete(libraryId); + if (error instanceof ApiError && error.status === 409) { + // 409 Conflict - the library is already scanning. Benign: drop the optimistic pending + // and reconcile against scan-status (an active scan keeps the button disabled). No toast. + clearPending(libraryId); + void loadScanStatuses(); + return; + } + + // 404 (no such library) / 422 (sync disabled) / network error - nothing was queued, so + // don't leave the button disabled; surface the error. + clearPending(libraryId); if (!activeRef.current) { return; @@ -305,18 +292,15 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta return current; } - const pendingLibraryIds = new Set(current.pendingLibraryIds); - pendingLibraryIds.delete(libraryId); - return { data: current.data, error: messageFromLibrariesError(error, 'Unable to scan library'), - pendingLibraryIds, + pendingLibraryIds: current.pendingLibraryIds, status: 'success' }; }); }); - }, [loadScanStatuses]); + }, [clearPending, loadScanStatuses]); if (state.status === 'success') { const activeLibraryIds = new Set(state.data.scanStatuses.map((scan) => scan.libraryId)); -- 2.47.3 From 702b0121cdc9156713989e97665717dccc35b870 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:54:48 +0200 Subject: [PATCH 3/3] fix(review): bounded scan-pending net + scheduler compensating unlock (#232) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (fork + Codex, both flagged) of PR #241: - SPA (both reviewers): removing PENDING_GRACE_TICKS wholesale reintroduced a stuck scan button. A 202'd scan that finishes between 10s polls (short/empty library) is never observed active, so its optimistic pending flag wedged the button disabled until reload. Restore a BOUNDED grace net (pruneGraceExpiredPending) — re-scoped honestly: it absorbs the inherent queue->observed-active lag and the fast-completion race, NOT the removed lying-200 compensation (the POST now returns 409/404/422 honestly). Bounds pending to PENDING_GRACE_TICKS * pollMs (~30s). - SPA 409 (Codex): on "already scanning" the button was cleared+reconciled, but a scan-status still lagging the in-progress scan re-enabled the button and let the user fire repeated 409s. Keep the pending flag on 409 (no toast) so the button stays disabled; polling promotes or expires it. - Scheduler (Codex): the Plex-Shows tail-token batch and the local/Jellyfin/Emby scan enqueues had no compensating unlock — a WriteAsync failure after LockLibrary (cancellation on shutdown) stranded the library lock. Wrap each acquired-lock enqueue in try/catch → UnlockLibrary → rethrow (the Plex catch covers both writes, since the library message carries Unlock: false and the un-enqueued networks message was the sole releaser). Tests: two new App.test.tsx cases — grace-window expiry re-enables the button, and 409 keeps it disabled through the queue->active lag. Ref #232. Co-Authored-By: Claude Opus 4.8 (1M context) --- ErsatzTV/Services/SchedulerService.cs | 61 +++++++++++++++---- web/src/App.test.tsx | 86 ++++++++++++++++++++++++++ web/src/api/libraries.ts | 88 ++++++++++++++++++++++----- 3 files changed, 207 insertions(+), 28 deletions(-) diff --git a/ErsatzTV/Services/SchedulerService.cs b/ErsatzTV/Services/SchedulerService.cs index 072c74deb..0845bf6fa 100644 --- a/ErsatzTV/Services/SchedulerService.cs +++ b/ErsatzTV/Services/SchedulerService.cs @@ -211,7 +211,16 @@ public class SchedulerService : BackgroundService { if (_entityLocker.LockLibrary(libraryId)) { - await _scannerWorkerChannel.WriteAsync(new ScanLocalLibraryIfNeeded(libraryId), cancellationToken); + try + { + await _scannerWorkerChannel.WriteAsync(new ScanLocalLibraryIfNeeded(libraryId), cancellationToken); + } + catch + { + // the message that would release this lock never got enqueued - release it here + _entityLocker.UnlockLibrary(libraryId); + throw; + } } } } @@ -233,15 +242,25 @@ public class SchedulerService : BackgroundService // carries the release; the library message runs with Unlock: false. bool networksFollow = library.MediaKind is LibraryMediaKind.Shows; - await _scannerWorkerChannel.WriteAsync( - new SynchronizePlexLibraryByIdIfNeeded(library.Id, Unlock: !networksFollow), - cancellationToken); - - if (networksFollow) + try { await _scannerWorkerChannel.WriteAsync( - new SynchronizePlexNetworks(library.Id, false), + new SynchronizePlexLibraryByIdIfNeeded(library.Id, Unlock: !networksFollow), cancellationToken); + + if (networksFollow) + { + await _scannerWorkerChannel.WriteAsync( + new SynchronizePlexNetworks(library.Id, false), + cancellationToken); + } + } + catch + { + // if enqueueing the batch fails partway (the library message carries Unlock: false, + // so the not-yet-enqueued networks message was the sole releaser), release here + _entityLocker.UnlockLibrary(library.Id); + throw; } } } @@ -267,9 +286,17 @@ public class SchedulerService : BackgroundService if (_entityLocker.LockLibrary(library.Id)) { - await _scannerWorkerChannel.WriteAsync( - new SynchronizeJellyfinLibraryByIdIfNeeded(library.Id), - cancellationToken); + try + { + await _scannerWorkerChannel.WriteAsync( + new SynchronizeJellyfinLibraryByIdIfNeeded(library.Id), + cancellationToken); + } + catch + { + _entityLocker.UnlockLibrary(library.Id); + throw; + } } } @@ -294,9 +321,17 @@ public class SchedulerService : BackgroundService if (_entityLocker.LockLibrary(library.Id)) { - await _scannerWorkerChannel.WriteAsync( - new SynchronizeEmbyLibraryByIdIfNeeded(library.Id), - cancellationToken); + try + { + await _scannerWorkerChannel.WriteAsync( + new SynchronizeEmbyLibraryByIdIfNeeded(library.Id), + cancellationToken); + } + catch + { + _entityLocker.UnlockLibrary(library.Id); + throw; + } } } diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index 0cf8e2809..e2094c1e5 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -1310,6 +1310,92 @@ describe('ChicoryTV SPA scaffold', () => { expect(screen.getByRole('button', { name: 'Scan Movies' })).not.toBeDisabled(); }); + it('re-enables the scan button after the grace window if a 202 scan never appears active (#232)', async () => { + // A 202 genuinely queues a (forced) scan, but a short/empty library can finish before the next + // 10s poll ever observes it in scan-status - or the scanner may never register it. The optimistic + // "pending" flag must not wedge the button disabled forever: it expires after PENDING_GRACE_TICKS + // polls that never see the library active, then the button re-enables. + const intervalHandlers: Array<() => void> = []; + vi.spyOn(window, 'setInterval').mockImplementation((handler: TimerHandler, timeout?: number) => { + if (typeof handler === 'function' && (timeout ?? 0) >= 10000) { + intervalHandlers.push(handler as () => void); + } + return intervalHandlers.length; + }); + vi.spyOn(window, 'clearInterval').mockImplementation(() => 0); + + mockDashboardApi({ + // initial load, immediate post-POST fetch, then every poll: the scan never shows up + libraryScanStatusSequence: [[], [], [], [], []], + mediaSources: [ + mediaSource({ libraries: [library({ id: 31, itemCount: 10, mediaKind: 'Movies', name: 'Movies' })] }) + ] + }); + + render(); + + fireEvent.click(screen.getByRole('link', { name: 'Libraries' })); + expect((await screen.findAllByText('Movies')).length).toBeGreaterThan(0); + + fireEvent.click(screen.getByRole('button', { name: 'Scan Movies' })); + + await waitFor(() => { + expect(window.fetch).toHaveBeenCalledWith('/api/libraries/31/scan', expect.objectContaining({ method: 'POST' })); + }); + + // Optimistically disabled; the immediate post-POST fetch ([]) burned one grace tick but keeps it pending. + await waitFor(() => { + expect(fetchCount('/api/libraries/scan-status')).toBe(2); + }); + expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); + + // Grace is PENDING_GRACE_TICKS (3): the immediate fetch used one, two more unseen polls expire it. + await runPollTick(intervalHandlers); + expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); + + await runPollTick(intervalHandlers); + expect(screen.getByRole('button', { name: 'Scan Movies' })).not.toBeDisabled(); + }); + + it('keeps the scan button disabled on a 409 even when scan-status has not yet caught up (#232)', async () => { + // 409 means the library is already scanning (a lock is held). The SPA must NOT re-enable the + // button just because scan-status has not yet listed the in-progress scan (the queue->active + // lag) - otherwise the user can fire repeated 409s. The pending flag is retained (button stays + // disabled, no error toast) and is reconciled/expired by the normal polling path. + mockDashboardApi({ + libraryScanStatusSequence: [ + [], // initial screen load + [] // post-409 reconcile fetch: the in-progress scan is not visible in scan-status yet + ], + mediaSources: [ + mediaSource({ libraries: [library({ id: 31, itemCount: 10, mediaKind: 'Movies', name: 'Movies' })] }) + ], + mutationFailures: { + '/api/libraries/31/scan': { status: 409 } + } + }); + + render(); + + fireEvent.click(screen.getByRole('link', { name: 'Libraries' })); + expect((await screen.findAllByText('Movies')).length).toBeGreaterThan(0); + + fireEvent.click(screen.getByRole('button', { name: 'Scan Movies' })); + + await waitFor(() => { + expect(window.fetch).toHaveBeenCalledWith('/api/libraries/31/scan', expect.objectContaining({ method: 'POST' })); + }); + // Reconcile fetch happened and returned nothing active, yet the button stays disabled... + await waitFor(() => { + expect(fetchCount('/api/libraries/scan-status')).toBe(2); + }); + expect(screen.getByRole('button', { name: 'Scan Movies' })).toBeDisabled(); + + // ...and no error toast is shown for the benign conflict. + expect(screen.queryByText(/Request failed/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Unable to scan/)).not.toBeInTheDocument(); + }); + it('shows Libraries API errors and retries', async () => { // The backend has no exception middleware, so a real failure is a bare 500 with no // ProblemDetails body - do not invent one here (mirrors the scan-trigger-404 test below). diff --git a/web/src/api/libraries.ts b/web/src/api/libraries.ts index c771f1352..66361eae9 100644 --- a/web/src/api/libraries.ts +++ b/web/src/api/libraries.ts @@ -61,17 +61,46 @@ export function scanShow(libraryId: number, params: ScanShowParams): Promise, isSeenActive: (libraryId: number) => boolean): Set { +// How many scan-status polls a pending library may go WITHOUT ever being observed active before we +// give up on it. This is NOT the old lying-200 compensation (the POST now honestly returns +// 409/404/422 when nothing was queued): a 202 truly queues a scan, but there is still inherent lag +// between "queued" and the scan appearing in GetActiveScans, and a short/empty force-scan can even +// finish between two 10s polls. Without this bound such a scan would never be observed active and +// its optimistic "pending" flag would wedge the button disabled until reload. Bounds that to +// PENDING_GRACE_TICKS * pollMs (~30s). +const PENDING_GRACE_TICKS = 3; + +// Reconcile optimistic "pending" scans against the authoritative active-scan set. A pending id seen +// active at least once is dropped from pending (normal active/inactive pruning takes over from +// there). A pending id not yet seen active is kept, but only for PENDING_GRACE_TICKS polls: the +// per-id counter is decremented each tick and the id is dropped once it hits zero. Mutates the +// grace-ticks map in place (delete on promote/expire, set on decrement); callers must still write +// pendingIdsRef.current with the returned set themselves, and must call this exactly once per tick +// before that write to keep it a single, ref-free computation safe under StrictMode double-invocation. +function pruneGraceExpiredPending( + pendingIds: Set, + graceTicks: Map, + isSeenActive: (libraryId: number) => boolean +): Set { const nextPending = new Set(); pendingIds.forEach((libraryId) => { - if (!isSeenActive(libraryId)) { - nextPending.add(libraryId); + if (isSeenActive(libraryId)) { + // Seen active at least once - normal active/inactive pruning takes over. + graceTicks.delete(libraryId); + return; } + + const ticksRemaining = (graceTicks.get(libraryId) ?? PENDING_GRACE_TICKS) - 1; + + if (ticksRemaining <= 0) { + // Grace window expired without ever appearing in scan-status - give up on it. + graceTicks.delete(libraryId); + return; + } + + graceTicks.set(libraryId, ticksRemaining); + nextPending.add(libraryId); }); return nextPending; @@ -89,6 +118,8 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta // guard against double-submits without waiting for a render. const pendingIdsRef = useRef>(new Set()); const activeIdsRef = useRef>(new Set()); + // Per-pending-id countdown of remaining polls before we give up waiting for it to appear active. + const pendingGraceTicksRef = useRef>(new Map()); useEffect(() => { activeRef.current = true; @@ -139,7 +170,11 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta // Compute the promoted pending set - and write the pending ref - OUTSIDE the setState // updater. Updaters must be pure (StrictMode double-invokes them and concurrent rendering // may invoke-and-discard one). Mirrors how activeIdsRef is written above. - const nextPending = prunePromotedPending(pendingIdsRef.current, (libraryId) => activeScanIds.has(libraryId)); + const nextPending = pruneGraceExpiredPending( + pendingIdsRef.current, + pendingGraceTicksRef.current, + (libraryId) => activeScanIds.has(libraryId) + ); pendingIdsRef.current = nextPending; @@ -161,10 +196,28 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta } }) .catch(() => { - // A transient status-fetch failure is left to resolve on the next poll: pending and - // active-scan state are untouched so an in-progress scan's UI is not disturbed. A 202 - // is only returned for a real scan, so pending ids reconcile against scan-status on the - // next successful poll rather than needing a give-up budget here. + if (!activeRef.current) { + return; + } + + // A persistently failing scan-status poll must not leave optimistically-pending buttons + // stuck disabled forever. Burn a grace tick for every pending id (none can be confirmed + // active while the fetch is failing) so pending still expires within the bounded window. + const nextPending = pruneGraceExpiredPending(pendingIdsRef.current, pendingGraceTicksRef.current, () => false); + pendingIdsRef.current = nextPending; + + setState((current) => { + if (current.status !== 'success') { + return current; + } + + return { + data: current.data, + error: current.error, + pendingLibraryIds: nextPending, + status: 'success' + }; + }); }); }, [loadSources]); @@ -177,6 +230,7 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta hadScanInProgressRef.current = scanStatuses.length > 0; pendingIdsRef.current = new Set(); + pendingGraceTicksRef.current = new Map(); activeIdsRef.current = new Set(scanStatuses.map((scan) => scan.libraryId)); setState({ data: { scanStatuses, sources }, @@ -220,6 +274,7 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta const pendingIds = new Set(pendingIdsRef.current); pendingIds.delete(libraryId); pendingIdsRef.current = pendingIds; + pendingGraceTicksRef.current.delete(libraryId); if (!activeRef.current) { return; @@ -250,6 +305,7 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta // Optimistically mark pending; the POST response tells us how to reconcile. pendingIdsRef.current = new Set(pendingIdsRef.current).add(libraryId); + pendingGraceTicksRef.current.set(libraryId, PENDING_GRACE_TICKS); setState((current) => { if (current.status !== 'success') { @@ -272,9 +328,11 @@ export function useLibrariesScreenQuery(pollMs = 10000): LibrariesScreenQuerySta }) .catch((error: unknown) => { if (error instanceof ApiError && error.status === 409) { - // 409 Conflict - the library is already scanning. Benign: drop the optimistic pending - // and reconcile against scan-status (an active scan keeps the button disabled). No toast. - clearPending(libraryId); + // 409 Conflict - the library is already scanning (a lock is held). Benign: no toast. Keep + // the optimistic pending flag (do NOT clear it) so the button stays disabled - clearing it + // would re-enable the button whenever scan-status has not yet caught up to the in-progress + // scan (the queue->active lag), letting the user fire repeated 409s. The pending flag is + // promoted to "active" once the scan appears, or expires via the grace window if it never does. void loadScanStatuses(); return; } -- 2.47.3