Fix DbContext concurrency crash in remote-path-mapping reads during polling

[dny238]

Fixes #963.

EfRemotePathMappingRepository injected a scoped ListenArrDbContext directly and shared it across all reads in a scope. During download polling, DownloadClientGateway fans out Task.WhenAll over every queue item, each calling RemotePathMappingService.TranslatePathAsync several times, so many GetByClientIdAsync reads run concurrently on that one context / SQLite connection. That tears down an active statement mid-query, poisons the pooled connection, and every later query — including library search — fails until restart.

Change

Convert the repository to the per-operation IDbContextFactory<ListenArrDbContext> pattern already used across the codebase (e.g. EfDownloadProcessingJobRepository). Each read/write gets its own short-lived context/connection, so the existing download fan-out is safe as designed. No behavior change.

Why the suite missed it

Every persistence/concurrency test uses ;Pooling=False; production leaves pooling on. Added a concurrent-read regression test to RemotePathMappingServiceTests (64 parallel GetPathMappingByClientAsync for one client), which fails against a shared context and passes with the factory.

Tests

RemotePathMapping suite (21) + DownloadClientGateway/DownloadQueue (49) green; full build clean.

[m4bard]

Your diagnosis matches something I filed in August from the other end of the same call path, and I
think the two changes need each other rather than competing. Worth sorting out before either of us
rebases.

#871 goes at the call site. DownloadClientGateway.GetQueueAsync fans out over queue items, and
every item called TranslatePathAsync, which queried the repository for that client's mappings on
each call. A queue of N items issued N concurrent reads against one scoped context. That PR resolves
the mappings once per batch and passes them down, so the concurrent reads stop happening at all.

Yours makes the repository safe when they do happen. Different guarantees, and I do not think
either subsumes the other.

With only #964, the tearing stops but a poll still issues one query per queue item. Query volume was
what sent me looking in the first place; the crash was just what made it visible.

With only #871, the gateway stops fanning out, but the repository is still one scoped context behind
concurrent callers, and the next thing that reaches it concurrently reopens the same hole.

So I read them as complementary. I would rather hear whether you agree than assume it.

One practical thing. We both append a new test at the end of the class in
tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs, and both hunks
start at line 241. Whichever lands second gets a conflict there. Trivial to resolve, two independent
tests sitting side by side, but easier to know about now than to meet mid-merge.

There is also an ordering question I would rather not answer by myself. If #964 lands first and
every repository read gets its own context, the batch resolution in #871 becomes a query-volume
improvement rather than a correctness fix, and the way I worded that PR would need changing. I am
happy to do that. I just do not want to pick an order that happens to suit my own PR and present it
as the obvious one.

I have not run your branch against mine. The conflict above is read off the two diffs, not observed
in a merge.

Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and
reviewed this before posting.

[dny238]

Agreed on all counts — complementary, not competing, and your framing is exactly how I'd put it.

#964 makes the repository safe for any concurrent caller (a primitive-level guarantee that doesn't depend on how the call site behaves). #871 removes the fan-out and resolves per batch so those concurrent reads mostly stop happening, and — from the file list — caches them too. Neither subsumes the other: with only #964 the tearing stops but the per-item query volume you were chasing remains; with only #871 the gateway stops fanning out, but the repository is still one scoped context behind the next concurrent caller that reaches it.

Worth noting: I actually considered adding service-level caching to #964 and deliberately left it out to keep this a minimal repository-safety change. I didn't know #871 existed at the time, but it's turned out for the best — the caching/batch layer clearly belongs in your PR, and if I'd added it here we'd have collided on exactly that.

On ordering: I think #964 reads naturally as landing first, precisely because it's the caller-independent guarantee — once the repository is safe per-operation, #871 becomes a clean query-volume optimization rather than the thing standing between the app and a crash, which is the more honest framing for it anyway. But I hold that loosely: if #871 goes first, #964 still stands as defense-in-depth for any other concurrent caller, and I'm glad to rebase either way. Since you raised the ordering fairly, I'd rather the two of us (or the maintainer) agree on it than have me pick the order that flatters my own PR.

On the test collision in RemotePathMappingServiceTests.cs: confirmed — mine is GetPathMappingByClientAsync_ConcurrentReads_DoNotShareOneDbContext, appended at the same spot. Two independent tests that sit fine side by side; happy to be the one who resolves it whenever the second of us rebases. I'll link #871 from this PR so the relationship is visible.

Disclosure: drafted with Claude Code at my direction; I reviewed it before posting.

[m4bard]

Thanks for not taking the ordering that suited you.

#964 first. I agree with your reasoning, and I think it holds regardless of whose PR it is, because yours is the only one of the two that is safe for a caller nobody has written yet.

Your read of the file list is right, by the way: #871 does cache as well as batch. RemotePathMappingService.GetPathMappingByClientAsync serves out of a memory cache with a ten second lifetime that create, update and delete clear (listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs:50-71 on that branch). Reading that code back, though, there is no lock around the miss, so a cold cache under the gateway fan-out still lets several callers through to the repository at the same time. It narrows the window without closing it. Which is an argument for your ordering over mine: #964 removes the class of bug and #871 only removes most of the traffic.

So I will take the rewording. Once #964 is in, #871 stops being the thing standing between the app and a crash, and its description should talk about query volume and cache lifetime instead. I would rather fix that myself than leave a reviewer reading a PR that still argues for a crash fix which has already landed.

On the test collision, I can upgrade what I said last time. Previously I had read it off the two diffs. I have now run it, with git merge-tree --write-tree --name-only on the two PR heads, 77ac33399 and 1e5d61153:

• each merges clean onto canary a630572e9 on its own
• merged with each other, exactly one file conflicts, tests/Features/Infrastructure/Configuration/Paths/RemotePathMappingServiceTests.cs
• the conflict is your GetPathMappingByClientAsync_ConcurrentReads_DoNotShareOneDbContext against my TranslatePathAsync_DoesNotSeeAMappingWrittenBehindTheService, both appended at the end of the class
• nothing else in either branch touches anything the other one touches

Since #964 goes first, #871 is the one that rebases, so I will take that resolution too. You offered, but it is merge work that only exists because of an order I have just agreed to, and it is a couple of minutes on my side.

Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.