Skip to content

fix(qbittorrent): one unreadable torrent should not truncate the queue poll #830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m4bard wants to merge 1 commit into
base: canary
Choose a base branch
from
Open

fix(qbittorrent): one unreadable torrent should not truncate the queue poll #830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions listenarr.infrastructure/DownloadClients/Qbittorrent/QbittorrentItemFetchWorkflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,31 @@ public async Task<List<DownloadClientItem>> GetItemsAsync(DownloadClientConfigur

foreach (var torrent in torrents)
{
items.Add(QbittorrentResponseMapper.MapDownloadClientItem(
torrent,
client,
removeCompletedDownloads,
globalMaxRatioEnabled,
globalMaxRatio,
globalMaxSeedingTimeEnabled,
globalMaxSeedingTime));
// Same per-item isolation as the queue fetch. This list is what completion and
// import decisions are made from, so a torrent lost here is not just a missing
// row in a view: everything after it stops being considered for import at all.
try
{
items.Add(QbittorrentResponseMapper.MapDownloadClientItem(
torrent,
client,
removeCompletedDownloads,
globalMaxRatioEnabled,
globalMaxRatio,
globalMaxSeedingTimeEnabled,
globalMaxSeedingTime));
}
catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException)
{
var hash = torrent.TryGetValue("hash", out var hashEl) && hashEl.ValueKind == JsonValueKind.String
? hashEl.GetString() ?? string.Empty
: string.Empty;
logger.LogWarning(
ex,
"Skipping unreadable qBittorrent torrent {TorrentHash} for client {ClientId}; the rest of the item list is unaffected",
LogRedaction.SanitizeText(hash),
LogRedaction.SanitizeText(client.Id));
}
}
}
catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException)
Expand Down
34 changes: 26 additions & 8 deletions listenarr.infrastructure/DownloadClients/Qbittorrent/QbittorrentQueueFetchWorkflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,35 @@ public async Task<List<QueueItem>> GetQueueAsync(DownloadClientConfiguration cli

foreach (var torrent in torrents)
{
var hash = torrent.TryGetValue("hash", out var hashEl) ? hashEl.GetString() ?? string.Empty : string.Empty;
var hash = torrent.TryGetValue("hash", out var hashEl) && hashEl.ValueKind == JsonValueKind.String
? hashEl.GetString() ?? string.Empty
: string.Empty;

List<Dictionary<string, JsonElement>> files = [];
using var filesResp = await httpClient.GetAsync($"{baseUrl}/api/v2/torrents/files?hash={hash}", ct);
if (filesResp.IsSuccessStatusCode)
// One torrent that cannot be read must not take the rest of the response with
// it. Without this, an exception raised while mapping torrent N escapes the
// loop and is caught only by the handler below, so torrents N..end are dropped
// while the poll still reports itself as a healthy live snapshot: the queue
// simply appears shorter, with nothing to say a row was lost.
try
{
var filesJson = await filesResp.Content.ReadAsStringAsync(ct);
files = JsonSerializer.Deserialize<List<Dictionary<string, JsonElement>>>(filesJson) ?? [];
}
List<Dictionary<string, JsonElement>> files = [];
using var filesResp = await httpClient.GetAsync($"{baseUrl}/api/v2/torrents/files?hash={hash}", ct);
if (filesResp.IsSuccessStatusCode)
{
var filesJson = await filesResp.Content.ReadAsStringAsync(ct);
files = JsonSerializer.Deserialize<List<Dictionary<string, JsonElement>>>(filesJson) ?? [];
}

items.Add(QbittorrentResponseMapper.MapQueueItem(torrent, client, files));
items.Add(QbittorrentResponseMapper.MapQueueItem(torrent, client, files));
}
catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException)
{
logger.LogWarning(
ex,
"Skipping unreadable qBittorrent torrent {TorrentHash} for client {ClientId}; the rest of the queue is unaffected",
LogRedaction.SanitizeText(hash),
LogRedaction.SanitizeText(client.Id));
}
}
}
catch (DownloadClientAdapterPollingException)
Expand Down
51 changes: 51 additions & 0 deletions tests/Features/Infrastructure/DownloadClients/Qbittorrent/QbittorrentAdapterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,57 @@ public async Task GetQueueAsync_WithoutIds_ReturnsEmpty_OnQueueRequestFailure()
Assert.Empty(items);
}


// A queue response whose middle torrent carries `downloaded` in the given JSON token form.
// The torrents either side of it are well formed, so anything missing from the result is
// attributable to that one field.
private static string QueueWithMalformedMiddleTorrent(string malformedDownloaded) => $$"""
[
{
"hash": "aaaa1111", "name": "First", "progress": 0.5, "size": 1000,
"downloaded": 500, "state": "downloading", "save_path": "/downloads/a"
},
{
"hash": "bbbb2222", "name": "Second", "progress": 0.5, "size": 1000,
"downloaded": {{malformedDownloaded}}, "state": "downloading", "save_path": "/downloads/b"
},
{
"hash": "cccc3333", "name": "Third", "progress": 0.5, "size": 1000,
"downloaded": 700, "state": "downloading", "save_path": "/downloads/c"
}
]
""";

// qBittorrent documents `downloaded` as an integer, so the typed accessor reading it is
// right about the normal case. It was not resilient about the abnormal one: a value in
// another token form threw out of the mapper, out of the loop walking the response, and
// took every torrent after it along with it, while the poll still reported itself as a
// healthy live snapshot.
//
// "600.5" is a JSON number that is not an integer (FormatException from GetInt64) and
// "\"600\"" is a quoted one (InvalidOperationException). The quoted form is the shape
// already reported against the NZBGet adapter in #618 and #619.
[Theory]
[InlineData("600.5")]
[InlineData("\"600\"")]
[InlineData("6e2")]
public async Task GetQueueAsync_WhenOneTorrentIsUnreadable_DropsOnlyThatTorrent(string malformedDownloaded)
{
var apiMock = _provider.GetRequiredService<QbittorrentApiMock>();
apiMock.InfoResponseOverride = QueueWithMalformedMiddleTorrent(malformedDownloaded);
var gateway = (DownloadClientGateway)_provider.GetRequiredService<IDownloadClientGateway>();
var adapter = (QbittorrentAdapter)gateway.ResolveAdapter(_client);

var items = await adapter.GetQueueAsync(_client);

// The torrent AFTER the unreadable one is the whole point. Asserting only that the
// list is non-empty would pass on the truncating behaviour, because the first torrent
// is mapped before anything throws.
Assert.Contains(items, item => item.Id == "aaaa1111");
Assert.Contains(items, item => item.Id == "cccc3333");
Assert.DoesNotContain(items, item => item.Id == "bbbb2222");
Assert.Equal(2, items.Count);
}
[Fact]
public async Task MarkItemAsImportedAsync_SetsConfiguredPostImportCategory()
{
Expand Down