[m4bard]

Summary

I replaced this branch with a smaller change, because upstream moved underneath the old one. The old version resolved a client's mappings once per batch in DownloadClientGateway. #964 (c07a9a2, #964) fixed the DbContext crash. What I think #964 left is the read volume: a two-item queue poll costs 12 reads of the mapping table (2 plus source files per item), repeated every poll interval. With this change it is 1.

The service still removes rpm_client_{id} on create, update and delete, but nothing populates that key, so every translated path reads the table. I think the #576 restructure (#535) dropped the cache by accident (inferred: git log -S rpm_client_ shows 1d6a8e9, from #576, deleting the read and keeping the three Remove calls, and the commit message does not mention caching).

Changes

Fixed

• GetPathMappingByClientAsync populates the existing key for 10 s and returns a copy to each caller.
• A cold miss is single-flighted per client, so one poll does one read.
• Create, update and delete invalidate under the same lock as the fill, so an in-flight fill cannot overwrite a newer write. Moving a mapping to another client clears both owners.

Only RemotePathMappingService changes. #964's per-operation contexts and the gateway are untouched.

Trade-offs

A mapping edited directly in the database (not through the service) is stale for up to 10 s; the three service methods are the only writers I found. A write on a client now waits for that client's in-flight fill, bounded by one read. The 10 s lifetime and the lock are judgement calls and easy to change.

Testing

• Against bare canary, 4 of the 5 first tests failed: a two-item poll did 12 reads, not 1, and concurrent cold reads did 8. The write-through test passed on canary on purpose, as a control.
• Two further tests (a create during an in-flight cold read is seen by the next read; a move clears the old client) failed against my first version and pass now.
• Full suite at the head: 3741 passed, 0 failed, 150 skipped, against a 3734 baseline on 5e2593a.

Does that match what you would expect, and is 10 s a sensible lifetime?

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

[m4bard]

Pushed a follow-up commit. The batch lookup in the first commit does not cover the whole fan-out, and I would rather say that here than have it reviewed as though it did.

TranslateQueueItemPathsAsync translates three things: RemotePath, ContentPath, and every entry in SourceFiles. The first commit moved the first two onto the pre-resolved mappings and left the SourceFiles loop calling TranslatePathAsync, which still queries per file. That loop is sequential within one item, but it runs inside GetQueueAsync's Task.WhenAll over items, so the overlap I was trying to remove is still there whenever items carry source files.

On a torrent client they usually do. QbittorrentResponseMapper.MapQueueItem sets SourceFiles from the torrent's file list, and Transmission's resolver does the same, so most queue items from those clients arrive with the list already populated.

My own test missed it because the items it builds carry no source files. The new one builds ten items with three files each. It reports 10 concurrent lookups without the follow-up commit and 1 with it, and I ran it with the production line reverted to confirm it fails rather than assuming it would.

One test change came with it. DownloadClientGatewayTests's mapping mock only stubbed TranslatePathAsync. Once the source-file loop moved onto TranslatePath the mock returned null for every path, the two case-only filenames deduped to one, and the case-sensitive row of GetQueueItemAsync_DedupesCaseOnlySourceFilesUsingResolvedSemantics failed. Stubbing both restores it. The same staleness was already there after the first commit, for ContentPath. It just did not happen to move that assertion.

Having missed one inside a method I had already read, I swept the rest rather than leave it at that. #870 says I had not checked the other gateway methods, and I can close that now.

Three places resolve mappings, and all three are right: GetQueueAsync at :99 and FetchDownloadsAsync at :174 each resolve once and then fan out, and GetQueueItemAsync at :145 resolves once for a single item. Both fan-outs go through TranslateQueueItemPathsAsync, so the one line above is the whole of it rather than the first of several.

Outside the gateway there are only two callers of either method, both in RemotePathMappingsController, both serving one request with no fan-out.

Full suite: 3,031 passed, 0 failed, 125 skipped.

The line in the description saying a thirty-item queue goes from up to sixty queries to one held only for items with no source files. I have corrected it to say the lookups per batch, which is the claim the tests actually support.

[m4bard]

Checked against current canary (e1a1d12).

Merge: git merge-tree --write-tree of canary and this head is clean. Control: the same command on a branch known to conflict exits 1 and names the file. Canary has not touched RemotePathMappingService.cs since this branch's base; the merged tree was not run.

Full unfiltered suite on the head as published (d29f381, not merged): 3741 passed, 0 failed, 150 skipped, as the PR says. Canary alone: 3880 / 0 / 152. Not comparable: this branch sits on an older canary.

Fail-without: with only RemotePathMappingService.cs reverted, 4 of the 7 new tests fail (a two-item poll queries 12 times and concurrent cold reads 8, instead of once each, plus two more). The other 3 cannot fail on the old code: there is no cache to go stale. I broke the fix two ways instead: invalidating outside the lock fails the in-flight create test, and not clearing the old owner fails the move test.

Disclosure: drafted with Claude Code at my direction; I ran the cited checks at the stated commits and reviewed this before posting.

[therobbiedavis]

@m4bard Found one race with overlapping mapping moves. Both updates can read the same old client, so the second update misses clearing the intermediate client’s cache. That client can keep translating paths through a mapping it no longer owns until the cache expires.

Reproduced this in a Linux container with SQLite. The regression test fails on this head and passes with the uncached service. A local fix passed the focused tests (53 passed, 2 skipped) and 100 overlapping moves without reproducing it.

The normal mapping API paths passed. I didn’t run real downloader imports, the full suite, or CodeQL.

[therobbiedavis]

Both updates can read A as the old owner. The first moves the mapping to B and clears A/B. If a read fills B’s cache before the second update moves it to C, this only clears A/C and leaves B stale. I reproduced B still translating through that mapping after both requests returned. Please serialize the old-owner lookup, save, and invalidation across writers; locking the cache fill and removal separately doesn’t cover this.

[m4bard]

Thanks, you are right. UpdateAsync read the old owner before taking any lock, so two overlapping moves could both see A, and the second one cleared only A and C. DeleteAsync had the same gap when it raced a move: it read A, the move cleared A and B, a read cached B, and the delete then cleared only A. Your note was about updates; I included delete because it has the same read-then-clear shape.

Fixed in 15856cf. Update and delete now hold one process-wide writer lock (a static SemaphoreSlim, because the service is scoped) across GetByIdAsync, SaveAsync or DeleteAsync, and the cache clears after it. On lock order: a writer takes the writer lock first, then one ClientCacheLocks key at a time, never two keys at once. A cache fill takes only its own key and never the writer lock, so the two can't deadlock. Create has no previous owner to read, so it is left outside the lock.

The tests came first, in 086b8ff, on top of d29f381. UpdateAsync_OverlappingMovesFromOneClient_ClearTheIntermediateClient holds the first update (A to B) right after its lookup and starts a second update (A to C). Once the first update has finished, the test reads B so B is cached, then lets the second update go. At the end B has to be empty. DeleteAsync_OverlappingAMoveFromOneClient_ClearsTheClientItMovedTo runs the same sequence with a delete as the second writer. With the service as it is at d29f381, both tests fail on the B assertion, 5 runs out of 5. With the fix both pass, 5 runs out of 5. If I take the lock off delete again, only the delete test fails. The test helper releases both of its gates in a finally (e15762e), so a failed step cannot leave a writer holding the lock and hang the rest of the run.

Full backend suite on Linux, with nothing filtered out: 3743 passed, 0 failed, 150 skipped on e15762e. On d29f381 it was 3741 passed, 0 failed, 150 skipped. The two extra passes are the new tests, and both runs skipped the same tests. I have not run real downloader imports or CodeQL.

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