diff --git a/listenarr.application/Downloads/Common/DownloadClientGateway.cs b/listenarr.application/Downloads/Common/DownloadClientGateway.cs index ee1057f4e..cbc88c7d5 100644 --- a/listenarr.application/Downloads/Common/DownloadClientGateway.cs +++ b/listenarr.application/Downloads/Common/DownloadClientGateway.cs @@ -93,7 +93,17 @@ 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. 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)]; } @@ -137,7 +147,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 +177,13 @@ public async Task> FetchDownloadsAsync(DownloadClientConfiguratio ex); } - var tasks = items.Select(item => TranslateQueueItemPathsAsync(client, item)); + 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)]; foreach (QueueItem item in items) @@ -228,20 +246,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); } @@ -256,7 +278,7 @@ private async Task TranslateQueueItemPathsAsync(DownloadClientConfigu 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/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs index 5261976b1..782413927 100644 --- a/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs +++ b/listenarr.application/Downloads/Contracts/IRemotePathMappingService.cs @@ -66,4 +66,25 @@ 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. + /// + /// 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, + string remotePath); } diff --git a/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs b/listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs index 8b900ea76..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."); @@ -123,7 +151,29 @@ 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) + { + // 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; + } + 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..406527f82 --- /dev/null +++ b/tests/Features/Application/Downloads/Common/DownloadClientGatewayPathMappingConcurrencyTests.cs @@ -0,0 +1,199 @@ +/* + * 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); + } + + [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); + } + + // 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); + } +} 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(), diff --git a/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs new file mode 100644 index 000000000..b7db6369b --- /dev/null +++ b/tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingCacheTests.cs @@ -0,0 +1,254 @@ +/* + * 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.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Listenarr.Tests.Features.Infrastructure.Configuration.Paths; + +[Trait("Name", "RemotePathMappingCacheTests")] +[Trait("Category", "Unit")] +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 + // 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); + } + } + } + + // 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; + + public RemotePathMappingCacheTests() + { + _service = new RemotePathMappingService( + _repository, + NullLogger.Instance, + _cache); + } + + public override async Task DisposeAsync() + { + _cache.Dispose(); + await base.DisposeAsync(); + } + + 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("/media/downloads", Assert.Single(result).LocalPath)); + } + + // 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("/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("/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); + } + + // 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..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) @@ -241,5 +246,43 @@ 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 + // 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); + } } }