From 5e08078b9940960f052330523b86564356b24d0f Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:17:07 -0500 Subject: [PATCH 1/6] fix(downloads): resolve a client's path mappings once per batch, not per item DownloadClientGateway.GetQueueAsync fans out over every queue item, and each item translated its paths by calling IRemotePathMappingService.TranslatePathAsync, which queries the repository for that client's mappings on every call. The service, the repository and the ListenArrDbContext behind them are all scoped, so a queue of N items issued up to 2N concurrent queries against a context that permits one at a time. DownloadClientQueuePoller then runs that whole thing inside its own Task.WhenAll across every enabled client, so the fan-out is nested. This is the origin of the trace in upstream #783. The exception surfaces at DownloadClientQueuePoller.FetchAsync because that is where the await unwinds; the class itself holds no repository and no context and never did. Split the lookup from the translation. IRemotePathMappingService gains a TranslatePath overload that takes mappings the caller has already resolved and does no I/O, and TranslatePathAsync keeps its behaviour by fetching and delegating to it. The gateway resolves once per client before each fan-out and passes the result down. The single-item import path also resolves once, where it is one query either way. Asserting on the EF exception would mean racing it, so the test counts overlap directly: a stub records the highest number of lookups in flight. Ten items carrying two translatable paths each report 10 concurrent lookups without the change and 1 with it, and the lookup count is pinned at 1 so a regression fails even if it somehow avoids overlapping. --- .../Downloads/Common/DownloadClientGateway.cs | 23 +++- .../Contracts/IRemotePathMappingService.cs | 13 ++ .../Paths/RemotePathMappingService.cs | 18 ++- ...lientGatewayPathMappingConcurrencyTests.cs | 118 ++++++++++++++++++ 4 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs diff --git a/listenarr.application/Downloads/Common/DownloadClientGateway.cs b/listenarr.application/Downloads/Common/DownloadClientGateway.cs index ee1057f4e..1403e804d 100644 --- a/listenarr.application/Downloads/Common/DownloadClientGateway.cs +++ b/listenarr.application/Downloads/Common/DownloadClientGateway.cs @@ -93,7 +93,11 @@ public async Task> GetQueueAsync(DownloadClientConfiguration cli { var adapter = ResolveAdapter(client); var items = await adapter.GetQueueAsync(client, ct); - var tasks = items.Select(item => TranslateQueueItemPathsAsync(client, item)); + // Resolved once for the batch. Translating each item used to query for the client's + // mappings itself, inside this fan-out, against a scoped repository shared by everything + // else in the scope. + var mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + var tasks = items.Select(item => TranslateQueueItemPathsAsync(mappings, client, item)); return [.. await Task.WhenAll(tasks)]; } @@ -137,7 +141,9 @@ public async Task GetQueueItemAsync( var adapter = ResolveAdapter(client); var item = await adapter.GetImportItemAsync(client, download, queueItem, null, ct); - return await TranslateQueueItemPathsAsync(client, item); + // Single item, so the lookup here is one query either way. + var mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + return await TranslateQueueItemPathsAsync(mappings, client, item); } public async Task> FetchDownloadsAsync(DownloadClientConfiguration client, List downloads, CancellationToken ct = default) @@ -165,7 +171,8 @@ public async Task> FetchDownloadsAsync(DownloadClientConfiguratio ex); } - var tasks = items.Select(item => TranslateQueueItemPathsAsync(client, item)); + var mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + var tasks = items.Select(item => TranslateQueueItemPathsAsync(mappings, client, item)); items = [.. await Task.WhenAll(tasks)]; foreach (QueueItem item in items) @@ -228,20 +235,24 @@ private List GetExternalIds(List downloads) /// Make sure all paths are locally accessible after processing and /// that a proper list of sanitized source files is produced /// + /// Remote path mappings already resolved for this client /// Download client configuration to use for path mapping /// Queue item to translate/sanitize /// - private async Task TranslateQueueItemPathsAsync(DownloadClientConfiguration client, QueueItem item) + private async Task TranslateQueueItemPathsAsync( + IReadOnlyList mappings, + DownloadClientConfiguration client, + QueueItem item) { if (!string.IsNullOrEmpty(item.RemotePath)) { - item.LocalPath = await remotePathMappingService.TranslatePathAsync(client, item.RemotePath); + item.LocalPath = remotePathMappingService.TranslatePath(mappings, client, item.RemotePath); EnsureNativePath(item.LocalPath, client.Name); } if (!string.IsNullOrEmpty(item.ContentPath)) { - item.ContentPath = await remotePathMappingService.TranslatePathAsync(client, item.ContentPath); + item.ContentPath = remotePathMappingService.TranslatePath(mappings, client, item.ContentPath); EnsureNativePath(item.ContentPath, client.Name); } diff --git a/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs index 5261976b1..63e568b2d 100644 --- a/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs +++ b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs @@ -66,4 +66,17 @@ public interface IRemotePathMappingService /// A matching mapping exists but its local side is unavailable or unsafe on this host. /// Task TranslatePathAsync(DownloadClientConfiguration client, string remotePath); + + /// + /// Translates a remote path using mappings the caller has already resolved. + /// + /// + /// For callers translating many paths for one client. Resolving the mappings once and + /// translating from them keeps a parallel batch off the scoped repository, and so off the + /// scoped DbContext behind it, which permits one operation at a time. + /// + string TranslatePath( + IReadOnlyList mappings, + DownloadClientConfiguration client, + string remotePath); } diff --git a/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs b/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs index 8b900ea76..a0e34b1a3 100644 --- a/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs +++ b/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs @@ -123,7 +123,23 @@ public async Task TranslatePathAsync(DownloadClientConfiguration client, return remotePath; } - var mappings = await GetPathMappingByClientAsync(client); + return TranslatePath(await GetPathMappingByClientAsync(client), client, remotePath); + } + + // The mapping lookup and the translation are separated so a caller translating many paths + // for one client can resolve the mappings once. The repository is scoped and so is the + // DbContext behind it, so translating a batch in parallel while each call did its own + // lookup meant concurrent queries on a context that permits one at a time. + public string TranslatePath( + IReadOnlyList mappings, + DownloadClientConfiguration client, + string remotePath) + { + if (string.IsNullOrEmpty(remotePath)) + { + return remotePath; + } + foreach (var mapping in mappings) { if (!TryGetRemoteSemantics( diff --git a/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs new file mode 100644 index 000000000..8b83684c6 --- /dev/null +++ b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs @@ -0,0 +1,118 @@ +/* + * Listenarr - Audiobook Management System + * Copyright (C) 2024-2026 Listenarr Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +using Listenarr.Tests.Common; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Listenarr.Tests.Features.Application.Downloads.Common; + +[Trait("Name", "DownloadClientGatewayPathMappingConcurrencyTests")] +[Trait("Category", "Unit")] +public sealed class DownloadClientGatewayPathMappingConcurrencyTests : BaseTests +{ + // IRemotePathMappingService is scoped and the ListenArrDbContext behind its repository is + // scoped too. GetQueueAsync fans out over every queue item, and each item used to look up the + // client's mappings for itself, so a queue of N items issued N concurrent queries against a + // context that permits one at a time. Asserting on the EF exception would mean racing it, so + // this counts the overlap directly. + private sealed class OverlapRecordingMappingService : IRemotePathMappingService + { + private int _inFlight; + public int MaxConcurrentLookups { get; private set; } + public int LookupCount { get; private set; } + + public async Task> GetPathMappingByClientAsync( + DownloadClientConfiguration client) + { + var now = Interlocked.Increment(ref _inFlight); + lock (this) + { + LookupCount++; + if (now > MaxConcurrentLookups) MaxConcurrentLookups = now; + } + + // A real query is not instantaneous; without this the overlap can go unobserved. + await Task.Delay(20); + + Interlocked.Decrement(ref _inFlight); + return []; + } + + public string TranslatePath( + IReadOnlyList mappings, + DownloadClientConfiguration client, + string remotePath) => remotePath; + + public async Task TranslatePathAsync( + DownloadClientConfiguration client, + string remotePath) + { + var mappings = await GetPathMappingByClientAsync(client); + return TranslatePath(mappings, client, remotePath); + } + + public Task> GetAllAsync() => + Task.FromResult(new List()); + public Task GetByIdAsync(int id) => + Task.FromResult(null); + public Task CreateAsync(RemotePathMapping mapping) => + Task.FromResult(mapping); + public Task UpdateAsync(RemotePathMapping mapping) => + Task.FromResult(mapping); + public Task DeleteAsync(int id) => Task.FromResult(true); + } + + [Fact] + public async Task GetQueueAsync_ResolvesClientMappingsOncePerBatch() + { + var mappingService = new OverlapRecordingMappingService(); + var client = new DownloadClientConfiguration + { + Id = "client-1", + Name = "qbittorrent", + Type = "qBittorrent" + }; + + var items = Enumerable.Range(0, 10) + .Select(i => new QueueItem + { + Id = $"item-{i}", + RemotePath = $"/remote/downloads/book-{i}", + ContentPath = $"/remote/downloads/book-{i}/audio.m4b" + }) + .ToList(); + + var adapter = new Mock(); + adapter.Setup(a => a.GetQueueAsync(client, It.IsAny())) + .ReturnsAsync(items); + var factory = new Mock(); + factory.Setup(f => f.GetByType(It.IsAny())).Returns(adapter.Object); + + var gateway = new DownloadClientGateway( + mappingService, + factory.Object, + new LocalFileSystem(), + new FileSystemSemanticsResolver(), + NullLogger.Instance); + + await gateway.GetQueueAsync(client); + + Assert.Equal(1, mappingService.MaxConcurrentLookups); + // Ten items, each carrying two translatable paths, resolved from one lookup. + Assert.Equal(1, mappingService.LookupCount); + } +} From fb4ac275a8bfef4e6e571788e7a302c299e93ba2 Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:56:14 -0500 Subject: [PATCH 2/6] fix(downloads): translate a queue item's source files from the resolved mappings too The batch lookup added alongside this only covered RemotePath and ContentPath. The SourceFiles loop still called TranslatePathAsync, which queries the repository for the client's mappings on every file. That loop is sequential within an item, but it runs inside GetQueueAsync's Task.WhenAll over items, so the overlap the batch lookup was meant to remove is still there whenever items carry source files. qBittorrent's queue mapper populates SourceFiles from the torrent's file list (QbittorrentResponseMapper.MapQueueItem), and Transmission's does the same, so this is the normal case on a torrent client rather than an edge case. The new test covers items carrying source files, which the existing one did not: it reports 10 concurrent lookups without this change and 1 with it. DownloadClientGatewayTests' mapping mock only stubbed TranslatePathAsync, so it went stale when the source-file loop moved onto TranslatePath and returned null for every path. Stub both. --- .../Downloads/Common/DownloadClientGateway.cs | 2 +- ...lientGatewayPathMappingConcurrencyTests.cs | 47 +++++++++++++++++++ .../Common/DownloadClientGatewayTests.cs | 5 ++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/listenarr.application/Downloads/Common/DownloadClientGateway.cs b/listenarr.application/Downloads/Common/DownloadClientGateway.cs index 1403e804d..80adcacfe 100644 --- a/listenarr.application/Downloads/Common/DownloadClientGateway.cs +++ b/listenarr.application/Downloads/Common/DownloadClientGateway.cs @@ -267,7 +267,7 @@ private async Task TranslateQueueItemPathsAsync( List sourceFiles = []; foreach (string file in item.SourceFiles) { - var sourceFile = await remotePathMappingService.TranslatePathAsync(client, file); + var sourceFile = remotePathMappingService.TranslatePath(mappings, client, file); EnsureNativePath(sourceFile, client.Name); sourceFiles.Add(sourceFile); } diff --git a/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs index 8b83684c6..675f5218a 100644 --- a/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs +++ b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs @@ -115,4 +115,51 @@ public async Task GetQueueAsync_ResolvesClientMappingsOncePerBatch() // Ten items, each carrying two translatable paths, resolved from one lookup. Assert.Equal(1, mappingService.LookupCount); } + + [Fact] + public async Task GetQueueAsync_ResolvesOncePerBatch_WhenItemsCarrySourceFiles() + { + var mappingService = new OverlapRecordingMappingService(); + var client = new DownloadClientConfiguration + { + Id = "client-1", + Name = "qbittorrent", + Type = "qBittorrent" + }; + + // qBittorrent's queue mapper populates SourceFiles from the torrent's file list, so a + // real queue item arrives with one entry per file rather than with the list empty. + var items = Enumerable.Range(0, 10) + .Select(i => new QueueItem + { + Id = $"item-{i}", + RemotePath = $"/remote/downloads/book-{i}", + ContentPath = $"/remote/downloads/book-{i}/audio.m4b", + SourceFiles = + [ + $"/remote/downloads/book-{i}/01.m4b", + $"/remote/downloads/book-{i}/02.m4b", + $"/remote/downloads/book-{i}/03.m4b" + ] + }) + .ToList(); + + var adapter = new Mock(); + adapter.Setup(a => a.GetQueueAsync(client, It.IsAny())) + .ReturnsAsync(items); + var factory = new Mock(); + factory.Setup(f => f.GetByType(It.IsAny())).Returns(adapter.Object); + + var gateway = new DownloadClientGateway( + mappingService, + factory.Object, + new LocalFileSystem(), + new FileSystemSemanticsResolver(), + NullLogger.Instance); + + await gateway.GetQueueAsync(client); + + Assert.Equal(1, mappingService.MaxConcurrentLookups); + Assert.Equal(1, mappingService.LookupCount); + } } diff --git a/tests/Features/Application/Downloads/Common/DownloadClientGatewayTests.cs b/tests/Features/Application/Downloads/Common/DownloadClientGatewayTests.cs index 732f98112..10977aba6 100644 --- a/tests/Features/Application/Downloads/Common/DownloadClientGatewayTests.cs +++ b/tests/Features/Application/Downloads/Common/DownloadClientGatewayTests.cs @@ -390,6 +390,11 @@ public async Task GetQueueItemAsync_DedupesCaseOnlySourceFilesUsingResolvedSeman It.IsAny(), It.IsAny())) .ReturnsAsync((DownloadClientConfiguration _, string path) => path); + mapping.Setup(service => service.TranslatePath( + It.IsAny>(), + It.IsAny(), + It.IsAny())) + .Returns((IReadOnlyList _, DownloadClientConfiguration _, string path) => path); var resolver = new Mock(); resolver.Setup(service => service.ResolveAsync( It.IsAny(), From 0040d90f5569ea4c2a2b19e628ba5f7c9de484ad Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:11:20 -0500 Subject: [PATCH 3/6] fix(downloads): skip the mapping lookup when the queue is empty Resolving the client's path mappings once per batch moved the lookup out of the per-item translation and onto the entry of GetQueueAsync and FetchDownloadsAsync, where it now runs whether or not there is anything to translate. An idle client returning an empty queue is the common case, and before this change it cost no query at all, because the lookup lived inside the loop that had nothing to iterate. Polling several clients every few seconds against one scoped DbContext is the pressure this branch exists to reduce, so the empty case should not be adding to it. Guard both call sites on items.Count > 0 and keep the batch lookup everywhere else. The new test asserts zero lookups for an empty queue; reverting the guard makes it report one. Co-Authored-By: Claude Fable 5.1 --- .../Downloads/Common/DownloadClientGateway.cs | 17 ++++++++-- ...lientGatewayPathMappingConcurrencyTests.cs | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/listenarr.application/Downloads/Common/DownloadClientGateway.cs b/listenarr.application/Downloads/Common/DownloadClientGateway.cs index 80adcacfe..cbc88c7d5 100644 --- a/listenarr.application/Downloads/Common/DownloadClientGateway.cs +++ b/listenarr.application/Downloads/Common/DownloadClientGateway.cs @@ -95,8 +95,14 @@ public async Task> GetQueueAsync(DownloadClientConfiguration cli var items = await adapter.GetQueueAsync(client, ct); // Resolved once for the batch. Translating each item used to query for the client's // mappings itself, inside this fan-out, against a scoped repository shared by everything - // else in the scope. - var mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + // else in the scope. An empty queue is the common case on an idle client and needs no + // lookup at all, which is what it cost before the batch lookup was introduced. + IReadOnlyList mappings = []; + if (items.Count > 0) + { + mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + } + var tasks = items.Select(item => TranslateQueueItemPathsAsync(mappings, client, item)); return [.. await Task.WhenAll(tasks)]; } @@ -171,7 +177,12 @@ public async Task> FetchDownloadsAsync(DownloadClientConfiguratio ex); } - var mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + IReadOnlyList mappings = []; + if (items.Count > 0) + { + mappings = await remotePathMappingService.GetPathMappingByClientAsync(client); + } + var tasks = items.Select(item => TranslateQueueItemPathsAsync(mappings, client, item)); items = [.. await Task.WhenAll(tasks)]; diff --git a/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs index 675f5218a..406527f82 100644 --- a/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs +++ b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs @@ -162,4 +162,38 @@ public async Task GetQueueAsync_ResolvesOncePerBatch_WhenItemsCarrySourceFiles() Assert.Equal(1, mappingService.MaxConcurrentLookups); Assert.Equal(1, mappingService.LookupCount); } + + // An idle client returns an empty queue, and an empty queue has nothing to translate. Before + // the batch lookup was introduced that cost no query at all, because the lookup lived inside + // the per-item translation. A poll every few seconds per client on a shared scoped DbContext + // is exactly the pressure this change exists to reduce, so the empty case must not acquire it. + [Fact] + public async Task GetQueueAsync_DoesNotResolveMappings_WhenTheQueueIsEmpty() + { + var mappingService = new OverlapRecordingMappingService(); + var client = new DownloadClientConfiguration + { + Id = "client-1", + Name = "qbittorrent", + Type = "qBittorrent" + }; + + var adapter = new Mock(); + adapter.Setup(a => a.GetQueueAsync(client, It.IsAny())) + .ReturnsAsync(new List()); + var factory = new Mock(); + factory.Setup(f => f.GetByType(It.IsAny())).Returns(adapter.Object); + + var gateway = new DownloadClientGateway( + mappingService, + factory.Object, + new LocalFileSystem(), + new FileSystemSemanticsResolver(), + NullLogger.Instance); + + var queue = await gateway.GetQueueAsync(client); + + Assert.Empty(queue); + Assert.Equal(0, mappingService.LookupCount); + } } From 56b17cd19d7bf57c22cdb96af7ca43d6645dd01f Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:23:42 -0500 Subject: [PATCH 4/6] fix(downloads): serve a client's path mappings from the cache it already invalidates Resolving the mappings once per batch took the query out of the item fan-out but left one per client poll, and DownloadClientQueuePoller polls up to four clients at once from a single scope. Four concurrent queries on one scoped DbContext is fewer than before and is still the condition EF refuses. RemotePathMappingService already removed rpm_client_{id} from the memory cache on create, update and delete. Only the population was missing, so the key was never occupied and every caller went to the database. Populating it with a ten second lifetime takes the steady-state poll off the repository entirely, which is the shape Readarr uses in its own RemotePathMappingService: All() served from a short-lived cache that the three writers clear. Each caller gets its own list rather than the cached array, so one caller's edit cannot reach the next. A cold miss, and the first poll after each expiry, can still resolve concurrently, because nothing serialises the miss; that window is a real remainder and the PR body says so. Also guard the new public TranslatePath overload with ArgumentNullException.ThrowIfNull and carry the InvalidOperationException contract onto it, which TranslatePathAsync documented and the new member did not, even though the throw sites now live in the new one. Co-Authored-By: Claude Fable 5.1 --- .../Contracts/IRemotePathMappingService.cs | 8 + .../Paths/RemotePathMappingService.cs | 42 ++- .../Paths/RemotePathMappingCacheTests.cs | 246 ++++++++++++++++++ .../Paths/RemotePathMappingServiceTests.cs | 14 + 4 files changed, 306 insertions(+), 4 deletions(-) create mode 100644 tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs diff --git a/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs index 63e568b2d..782413927 100644 --- a/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs +++ b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs @@ -75,6 +75,14 @@ public interface IRemotePathMappingService /// translating from them keeps a parallel batch off the scoped repository, and so off the /// scoped DbContext behind it, which permits one operation at a time. /// + /// Mappings already resolved for this client, most specific first + /// The download client reporting the path + /// The path as reported by the download client + /// The translated local path, or the original path if no mapping matches. + /// is null. + /// + /// A matching mapping exists but its local side is unavailable or unsafe on this host. + /// string TranslatePath( IReadOnlyList mappings, DownloadClientConfiguration client, diff --git a/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs b/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs index a0e34b1a3..d9b411cab 100644 --- a/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs +++ b/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs @@ -37,11 +37,39 @@ public async Task> GetAllAsync() return await remotePathMappingRepository.GetByIdAsync(id); } + // A client's mappings change only when the user edits them, and every queue poll needs the + // whole set for that client. Serving them from the shared memory cache keeps the + // steady-state poll off the repository entirely, which matters because the repository and + // the DbContext behind it are scoped while several clients are polled concurrently from a + // single scope. Readarr does the same thing in + // src/NzbDrone.Core/RemotePathMappings/RemotePathMappingService.cs, where All() is served + // from a cache with a ten second lifetime that add, update and remove clear. + // + // The three writers below already removed this key. Only the population was missing, so + // nothing was ever in the cache and every caller went to the database. + private static readonly TimeSpan ClientCacheLifetime = TimeSpan.FromSeconds(10); + public async Task> GetPathMappingByClientAsync(DownloadClientConfiguration client) { - return await remotePathMappingRepository.GetByClientIdAsync(client.Id); + var cacheKey = ClientCacheKey(client.Id); + + if (cache.TryGetValue(cacheKey, out RemotePathMapping[]? cached) && cached is not null) + { + return [.. cached]; + } + + var mappings = await remotePathMappingRepository.GetByClientIdAsync(client.Id); + + // Cache a private copy and hand every caller its own list. The repository reads these + // untracked, so sharing the instances across scopes is safe, but sharing the list + // itself would let one caller's edit reach the next one. + cache.Set(cacheKey, mappings.ToArray(), ClientCacheLifetime); + + return mappings; } + private static string ClientCacheKey(string downloadClientId) => $"rpm_client_{downloadClientId}"; + public async Task CreateAsync(RemotePathMapping mapping) { mapping.NormalizePaths(); @@ -55,7 +83,7 @@ public async Task CreateAsync(RemotePathMapping mapping) try { - cache.Remove($"rpm_client_{saved.DownloadClientId}"); + cache.Remove(ClientCacheKey(saved.DownloadClientId)); } catch (Exception exception) when (exception is not (OperationCanceledException or OutOfMemoryException or StackOverflowException)) { @@ -84,7 +112,7 @@ public async Task UpdateAsync(RemotePathMapping mapping) "Updated remote path mapping {MappingId} for client {ClientId}: {RemotePath} -> {LocalPath}", saved.Id, saved.DownloadClientId, saved.RemotePath, saved.LocalPath); - try { cache.Remove($"rpm_client_{saved.DownloadClientId}"); } + try { cache.Remove(ClientCacheKey(saved.DownloadClientId)); } catch (Exception caughtEx_2) when (caughtEx_2 is not OperationCanceledException && caughtEx_2 is not OutOfMemoryException && caughtEx_2 is not StackOverflowException) { System.Diagnostics.Debug.WriteLine("Suppressed non-fatal exception in catch block."); @@ -106,7 +134,7 @@ public async Task DeleteAsync(int id) "Deleted remote path mapping {MappingId} for client {ClientId}", id, existing.DownloadClientId); - try { cache.Remove($"rpm_client_{existing.DownloadClientId}"); } + try { cache.Remove(ClientCacheKey(existing.DownloadClientId)); } catch (Exception caughtEx_3) when (caughtEx_3 is not OperationCanceledException && caughtEx_3 is not OutOfMemoryException && caughtEx_3 is not StackOverflowException) { System.Diagnostics.Debug.WriteLine("Suppressed non-fatal exception in catch block."); @@ -135,6 +163,12 @@ public string TranslatePath( DownloadClientConfiguration client, string remotePath) { + // TranslatePathAsync always had the mappings in hand. This overload takes them from a + // caller, so the one contract it adds is that they are actually there. Failing here + // says which argument was wrong; without it an empty-looking translation just returns + // the remote path and the caller never learns that nothing was consulted. + ArgumentNullException.ThrowIfNull(mappings); + if (string.IsNullOrEmpty(remotePath)) { return remotePath; diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs new file mode 100644 index 000000000..9b07238c3 --- /dev/null +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs @@ -0,0 +1,246 @@ +/* + * Listenarr - Audiobook Management System + * Copyright (C) 2024-2026 Listenarr Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Listenarr.Tests.Features.Infrastructure.Configuration.Paths; + +[Trait("Name", "RemotePathMappingCacheTests")] +[Trait("Category", "Unit")] +public sealed class RemotePathMappingCacheTests : IDisposable +{ + // The download queue poller polls several clients at once from one scope, so every client's + // mapping lookup lands on the same scoped DbContext. Counting the queries is the way to say + // whether the steady-state poll touches the database at all; asserting on the EF concurrency + // exception would mean racing it. + private sealed class CountingRepository : IRemotePathMappingRepository + { + private readonly object _gate = new(); + private readonly List _rows = []; + private int _inFlight; + private int _nextId = 1; + + public int QueryCount { get; private set; } + public int MaxConcurrentQueries { get; private set; } + + public void Seed(string downloadClientId, string remotePath, string localPath) + { + lock (_gate) + { + _rows.Add(new RemotePathMapping + { + Id = _nextId++, + DownloadClientId = downloadClientId, + RemotePath = remotePath, + LocalPath = localPath + }); + } + } + + public async Task> GetByClientIdAsync( + string downloadClientId, + CancellationToken ct = default) + { + lock (_gate) + { + QueryCount++; + _inFlight++; + MaxConcurrentQueries = Math.Max(MaxConcurrentQueries, _inFlight); + } + + try + { + // Wide enough that concurrent callers genuinely overlap if they each query. + await Task.Delay(30, ct); + } + finally + { + lock (_gate) + { + _inFlight--; + } + } + + lock (_gate) + { + return [.. _rows.Where(row => row.DownloadClientId == downloadClientId)]; + } + } + + public Task> GetAllAsync(CancellationToken ct = default) + { + lock (_gate) + { + return Task.FromResult>([.. _rows]); + } + } + + public Task GetByIdAsync(int id, CancellationToken ct = default) + { + lock (_gate) + { + return Task.FromResult(_rows.FirstOrDefault(row => row.Id == id)); + } + } + + public Task SaveAsync(RemotePathMapping mapping, CancellationToken ct = default) + { + lock (_gate) + { + var existing = _rows.FirstOrDefault(row => row.Id == mapping.Id && mapping.Id != 0); + if (existing != null) + { + _rows.Remove(existing); + } + else + { + mapping.Id = _nextId++; + } + + _rows.Add(mapping); + return Task.FromResult(mapping); + } + } + + public Task DeleteAsync(int id, CancellationToken ct = default) + { + lock (_gate) + { + var existing = _rows.FirstOrDefault(row => row.Id == id); + if (existing == null) + { + return Task.FromResult(false); + } + + _rows.Remove(existing); + return Task.FromResult(true); + } + } + } + + private readonly CountingRepository _repository = new(); + private readonly MemoryCache _cache = new(new MemoryCacheOptions()); + private readonly RemotePathMappingService _service; + + public RemotePathMappingCacheTests() + { + _service = new RemotePathMappingService( + _repository, + NullLogger.Instance, + _cache); + } + + public void Dispose() => _cache.Dispose(); + + private static DownloadClientConfiguration Client(string id) => new() + { + Id = id, + Name = id, + Type = "qBittorrent" + }; + + // The discriminating one. Remove the cache.Set in GetPathMappingByClientAsync and the eight + // concurrent callers each issue their own query, so QueryCount reads nine instead of one and + // MaxConcurrentQueries reads more than one. + [Fact] + public async Task GetPathMappingByClientAsync_ConcurrentPolls_IssueNoQueryOnceWarm() + { + var client = Client("client-1"); + _repository.Seed(client.Id, "/downloads", "/media/downloads"); + + var warm = await _service.GetPathMappingByClientAsync(client); + Assert.Single(warm); + Assert.Equal(1, _repository.QueryCount); + + var polls = Enumerable.Range(0, 8) + .Select(_ => _service.GetPathMappingByClientAsync(client)); + var results = await Task.WhenAll(polls); + + Assert.Equal(1, _repository.QueryCount); + Assert.Equal(1, _repository.MaxConcurrentQueries); + Assert.All(results, result => Assert.Equal("/downloads", Assert.Single(result).RemotePath)); + } + + // A single cache key for every client would serve one client's mappings to another, which is + // worse than the query it saves. + [Fact] + public async Task GetPathMappingByClientAsync_KeepsEachClientSeparate() + { + var first = Client("client-1"); + var second = Client("client-2"); + _repository.Seed(first.Id, "/downloads/one", "/media/one"); + _repository.Seed(second.Id, "/downloads/two", "/media/two"); + + Assert.Equal("/downloads/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).RemotePath); + Assert.Equal("/downloads/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).RemotePath); + Assert.Equal(2, _repository.QueryCount); + + Assert.Equal("/downloads/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).RemotePath); + Assert.Equal("/downloads/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).RemotePath); + Assert.Equal(2, _repository.QueryCount); + } + + // Caching a set the user can edit is only safe while every writer drops it. The three writers + // already removed this key before anything populated it, so these pin the pairing. + [Fact] + public async Task CreateAsync_DropsTheCachedMappingsForThatClient() + { + var client = Client("client-1"); + _repository.Seed(client.Id, "/downloads", "/media/downloads"); + Assert.Single(await _service.GetPathMappingByClientAsync(client)); + + await _service.CreateAsync(new RemotePathMapping + { + DownloadClientId = client.Id, + RemotePath = "/downloads/second", + LocalPath = "/media/second" + }); + + var afterCreate = await _service.GetPathMappingByClientAsync(client); + Assert.Equal(2, afterCreate.Count); + Assert.Equal(2, _repository.QueryCount); + } + + [Fact] + public async Task DeleteAsync_DropsTheCachedMappingsForThatClient() + { + var client = Client("client-1"); + _repository.Seed(client.Id, "/downloads", "/media/downloads"); + var only = Assert.Single(await _service.GetPathMappingByClientAsync(client)); + + Assert.True(await _service.DeleteAsync(only.Id)); + + Assert.Empty(await _service.GetPathMappingByClientAsync(client)); + Assert.Equal(2, _repository.QueryCount); + } + + // The cached array is the service's own. Handing it out would let one caller's edit reach the + // next poll, which is the kind of fault a cache is expected not to introduce. + [Fact] + public async Task GetPathMappingByClientAsync_DoesNotShareTheListItHandsOut() + { + var client = Client("client-1"); + _repository.Seed(client.Id, "/downloads", "/media/downloads"); + + var first = await _service.GetPathMappingByClientAsync(client); + first.Clear(); + + Assert.Single(await _service.GetPathMappingByClientAsync(client)); + Assert.Equal(1, _repository.QueryCount); + } +} diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs index a4a1b45f3..1a237f20c 100644 --- a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs @@ -241,5 +241,19 @@ await _remotePathMappingRepository.SaveAsync(new RemotePathMappingBuilder() Assert.Equal(Path.Join(localPath, "Author", "book.m4b"), translated); } + + // The synchronous overload is the one contract the caller can get wrong, because it is the + // caller that supplies the mappings. Without the guard a null list reaches the foreach and + // throws NullReferenceException from inside the service, which says nothing about which + // argument was missing. + [Fact] + [Trait("Method", "TranslatePath")] + public void TranslatePath_NullMappings_ThrowsArgumentNullException() + { + var thrown = Assert.Throws( + () => remotePathMappingService.TranslatePath(null!, client, "/downloads/book.m4b")); + + Assert.Equal("mappings", thrown.ParamName); + } } } From 199d3c0595e729157d7e98f4fe50dc5261e653e4 Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:44:37 -0500 Subject: [PATCH 5/6] test(downloads): create mappings through the service so the cache sees them Two problems, both surfaced by running the suite rather than the focused filter. TranslatePathAsync_HappyPath translates once before it saves, which populates the per-client cache with an empty set, and then inserted its mapping straight into the repository. Only the service's own writers clear that key, so the second translate read the stale empty set. Creating through the service is the path every production writer takes, and the assertions are unchanged. That staleness is a real boundary of the cache and not only a test detail, so TranslatePathAsync_DoesNotSeeAMappingWrittenBehindTheService pins it: a row inserted behind the service is not visible until the entry expires. It should fail loudly if the invalidation is ever widened or narrowed by accident. The two cache tests asserted on RemotePath, which the entity normalizes with a trailing separator on the way in, so they compared the value they had written against the value the entity stores. They assert on LocalPath instead, which is what they were about in the first place. Co-Authored-By: Claude Fable 5.1 --- .../Paths/RemotePathMappingCacheTests.cs | 10 +++--- .../Paths/RemotePathMappingServiceTests.cs | 31 ++++++++++++++++++- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs index 9b07238c3..8badf9e0b 100644 --- a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs @@ -173,7 +173,7 @@ public async Task GetPathMappingByClientAsync_ConcurrentPolls_IssueNoQueryOnceWa Assert.Equal(1, _repository.QueryCount); Assert.Equal(1, _repository.MaxConcurrentQueries); - Assert.All(results, result => Assert.Equal("/downloads", Assert.Single(result).RemotePath)); + Assert.All(results, result => Assert.Equal("/media/downloads", Assert.Single(result).LocalPath)); } // A single cache key for every client would serve one client's mappings to another, which is @@ -186,12 +186,12 @@ public async Task GetPathMappingByClientAsync_KeepsEachClientSeparate() _repository.Seed(first.Id, "/downloads/one", "/media/one"); _repository.Seed(second.Id, "/downloads/two", "/media/two"); - Assert.Equal("/downloads/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).RemotePath); - Assert.Equal("/downloads/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).RemotePath); + Assert.Equal("/media/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).LocalPath); + Assert.Equal("/media/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).LocalPath); Assert.Equal(2, _repository.QueryCount); - Assert.Equal("/downloads/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).RemotePath); - Assert.Equal("/downloads/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).RemotePath); + Assert.Equal("/media/one", Assert.Single(await _service.GetPathMappingByClientAsync(first)).LocalPath); + Assert.Equal("/media/two", Assert.Single(await _service.GetPathMappingByClientAsync(second)).LocalPath); Assert.Equal(2, _repository.QueryCount); } diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs index 1a237f20c..d801342fa 100644 --- a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs @@ -87,7 +87,12 @@ public async Task TranslatePathAsync_HappyPath(string remotePath, string localPa { Assert.Equal(given, await remotePathMappingService.TranslatePathAsync(client, given)); - await _remotePathMappingRepository.SaveAsync(new RemotePathMappingBuilder() + // Created through the service, not the repository. The first translate above populates + // the per-client cache with an empty set, and only the service's own writers clear it, + // so a mapping inserted behind the service would not be seen here. That is the cache's + // boundary and it is pinned by + // TranslatePathAsync_DoesNotSeeAMappingWrittenBehindTheService below. + await remotePathMappingService.CreateAsync(new RemotePathMappingBuilder() .WithDownloadClientConfiguration(client) .WithRemotePath(remotePath) .WithLocalPath(localPath) @@ -242,6 +247,30 @@ await _remotePathMappingRepository.SaveAsync(new RemotePathMappingBuilder() Assert.Equal(Path.Join(localPath, "Author", "book.m4b"), translated); } + // The cache is cleared by CreateAsync, UpdateAsync and DeleteAsync, and by nothing else. A + // row inserted straight into the repository, or by another process, is therefore not + // visible until the entry expires. Every production write goes through the service, so this + // is a boundary rather than a bug, but it is a real change from the previous behaviour and + // it should fail loudly if the invalidation is ever widened or narrowed by accident. + [Fact] + [Trait("Method", "TranslatePathAsync")] + public async Task TranslatePathAsync_DoesNotSeeAMappingWrittenBehindTheService() + { + var given = FileUtils.GetAbsolutePath(Path.Join("downloads", "book.m4b")); + var remoteRoot = FileUtils.GetAbsolutePath("downloads"); + var localRoot = FileUtils.GetAbsolutePath("behind-the-service"); + + Assert.Equal(given, await remotePathMappingService.TranslatePathAsync(client, given)); + + await _remotePathMappingRepository.SaveAsync(new RemotePathMappingBuilder() + .WithDownloadClientConfiguration(client) + .WithRemotePath(remoteRoot) + .WithLocalPath(localRoot) + .Build()); + + Assert.Equal(given, await remotePathMappingService.TranslatePathAsync(client, given)); + } + // The synchronous overload is the one contract the caller can get wrong, because it is the // caller that supplies the mappings. Without the guard a null list reaches the foreach and // throws NullReferenceException from inside the service, which says nothing about which From 1e5d6115347c4d795fcfbbcf88964d68527fdb9a Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:48:43 -0500 Subject: [PATCH 6/6] test(downloads): hold the new cache tests to the repository's test conventions BackendArchitectureTests.TestClasses_FollowRepositoryConventions requires every test class carrying a Fact to inherit BaseTests and to declare an exact Name trait and a non-empty Category trait. The new RemotePathMappingCacheTests had the traits and not the base class, so the architecture test failed on the full suite while the focused filter stayed green. Inherit BaseTests and dispose the cache through an override of DisposeAsync. The service under test is still built by hand, because these tests count the repository calls the cache does or does not make, which needs a repository that records them. Co-Authored-By: Claude Fable 5.1 --- .../Paths/RemotePathMappingCacheTests.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs index 8badf9e0b..b7db6369b 100644 --- a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs @@ -15,6 +15,7 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ +using Listenarr.Tests.Common; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Logging.Abstractions; @@ -22,7 +23,7 @@ namespace Listenarr.Tests.Features.Infrastructure.Configuration.Paths; [Trait("Name", "RemotePathMappingCacheTests")] [Trait("Category", "Unit")] -public sealed class RemotePathMappingCacheTests : IDisposable +public sealed class RemotePathMappingCacheTests : BaseTests { // The download queue poller polls several clients at once from one scope, so every client's // mapping lookup lands on the same scoped DbContext. Counting the queries is the way to say @@ -133,6 +134,9 @@ public Task DeleteAsync(int id, CancellationToken ct = default) } } + // Built by hand rather than resolved from the provider: the point of these tests is to count + // the repository calls the cache does or does not make, which needs a repository that records + // them and a cache instance this test owns. private readonly CountingRepository _repository = new(); private readonly MemoryCache _cache = new(new MemoryCacheOptions()); private readonly RemotePathMappingService _service; @@ -145,7 +149,11 @@ public RemotePathMappingCacheTests() _cache); } - public void Dispose() => _cache.Dispose(); + public override async Task DisposeAsync() + { + _cache.Dispose(); + await base.DisposeAsync(); + } private static DownloadClientConfiguration Client(string id) => new() {