[Architecture] Responsability of download client adapters#599
Conversation
therobbiedavis
left a comment
therobbiedavis
left a comment
There was a problem hiding this comment.
Some logic questions, also this needs to be rebased.
|
|
||
| public async Task<List<Download>> FetchDownloadsAsync(DownloadClientConfiguration client, List<Download> downloads, CancellationToken ct = default) | ||
| { | ||
| var ids = GetExternalIds(downloads); |
There was a problem hiding this comment.
This path now only polls downloads that already have a stored external ID. If a legacy (or hopefully not a new record) is missing ClientDownloadId/TorrentHash, the adapter gets an empty ID list and the download can stay stuck without progress updates. Probably a reasonable tradeoff though, but maybe we should include a view for users to see "orphaned" records? wdyt
There was a problem hiding this comment.
I believe other *arr silently remove the download in case it becomes orphaned (no external ID or external ID no longer found in the download client), this seems like a good idea as in that case, automatic search will probably pick it up again later or the user can re-start a manual search (audiobook would be shown in the wanted list with no active download on it).
Also, I expect the majority of orphaned download to come from, either migration to this version or later (one time issue and the software is in canary phase anyway) or from download client manualy removing the queue item from their end (probably due to user input or configuration)
So, if we remove them directly (or even after retrying some times to get it from the adapter), by design we no longer have orphaned download and thus the view is probably not useful :p But if we dont go in that direction, then yes, we could bring a new oprhaned status meaning the download can no longer be linked with anything on the adapter side and the user could then remove only or remove and re-search them
I can see an issue with removing orphaned download: If the adapter is no longer reachable or experience technical issues, then it will send an empty list of queue items and we may delete them by error in those case. We can mitigate that by always checking if the adapter is up and running (using the TestAdapter method) first
| { | ||
| var adapter = ResolveAdapter(client); | ||
| return adapter.GetRecentHistoryAsync(client, limit, ct); | ||
| var items = await adapter.GetQueueAsync(client, ids, ct); |
There was a problem hiding this comment.
I think this changes the queue endpoint fetching the client queue to fetching only downloads already tracked in our DB. That drops untracked queue items and completed external downloads before DownloadQueueService can reconcile or display them.
There was a problem hiding this comment.
My reasoning is the following: If all adapters have to return an external ID when we add a download: It means that every Listenarr Download has an external ID. So, any Download without an external ID should be dropped and any queue item without external ID should be ignored.
By fetching the whole queue and deciding what to use or not in the application, we gather informations that are not related to Listenarr and should probably never leave the adapter layer (by design, it solves some issues I witnessed where all queue items are shown for an adapter which is not expected)
2 participants
Summary
As discussed on Discord and in #592, IDownloadClientAdapter implementations do not yet have a clear resonsability list. This PR aim at fixing that and defining a cleaner interface for future adapter integration while trying to keep it open for new features.
The main changes is the removal of FetchDownloadsAsync from Adapters, as context, they were moved from DownloadMonitor I believe and it was already a duplicate of GetQueueAsync.
I also removed unused GetRecentHistoryAsync and stubs of methods to use DownloadClientItem instead of QueueItem.
I found CombineWithBasePath was duplicated like 6 or 7 times so I removed it too.
Changes
Changed
Fixed
Removed
Notes