Skip to content

fix(enrichment): preserve recs on MusicBrainz HTTP failure #474

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

Merged
RicherTunes merged 1 commit into from
Feb 13, 2026
Merged

fix(enrichment): preserve recs on MusicBrainz HTTP failure #474

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
1 change: 1 addition & 0 deletions Brainarr.Plugin/Services/Enrichment/MusicBrainzResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public async Task<List<Recommendation>> EnrichWithMbidsAsync(List<Recommendation
catch (Exception ex)
{
_logger.Debug(ex, $"MBID resolution error for '{rec.Artist} - {rec.Album}'");
result.Add(rec);
}

// Throttling handled centrally via RateLimiter("musicbrainz")
Expand Down
14 changes: 14 additions & 0 deletions Brainarr.Tests/Services/Core/BrainarrOrchestratorTopUpTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using NzbDrone.Core.ImportLists.Brainarr.Services.Styles;
using NzbDrone.Core.ImportLists.Brainarr.Services.Registry;
using NzbDrone.Core.ImportLists.Brainarr.Services.Core;
using NzbDrone.Core.ImportLists.Brainarr.Services.Enrichment;
using NzbDrone.Core.ImportLists.Brainarr.Services.Resilience;
using NzbDrone.Core.Music;
using NzbDrone.Core.Parser.Model;
Expand Down Expand Up @@ -111,6 +112,17 @@ public async Task FetchRecommendations_WithTopUpEnabled_FillsToTarget()

var duplicateFilter = new DuplicateFilterService(artistService.Object, albumService.Object, _logger);

// Pass-through resolvers: no external HTTP calls to MusicBrainz.
// Without these mocks the constructor creates real resolvers that hit the
// MusicBrainz API, making the test non-deterministic (items silently dropped
// on HTTP failure even after the catch-block production fix).
var mbidResolver = new Mock<IMusicBrainzResolver>();
mbidResolver.Setup(r => r.EnrichWithMbidsAsync(It.IsAny<List<Recommendation>>(), It.IsAny<CancellationToken>()))
.Returns((List<Recommendation> recs, CancellationToken _) => Task.FromResult(recs));
var artistResolver = new Mock<IArtistMbidResolver>();
artistResolver.Setup(r => r.EnrichArtistsAsync(It.IsAny<List<Recommendation>>(), It.IsAny<CancellationToken>()))
.Returns((List<Recommendation> recs, CancellationToken _) => Task.FromResult(recs));

var orchestrator = new BrainarrOrchestrator(
_logger,
providerFactory.Object,
Expand All @@ -121,6 +133,8 @@ public async Task FetchRecommendations_WithTopUpEnabled_FillsToTarget()
modelDetection.Object,
http.Object,
duplicationPrevention,
mbidResolver: mbidResolver.Object,
artistResolver: artistResolver.Object,
breakerRegistry: breakerRegistry.Object,
duplicateFilter: duplicateFilter);

Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,4 @@ Claude Code will automatically apply the appropriate specialist context based on
|------|-----------|-----|
| `E2EHermeticGateTests.LogRedaction_*` | NLog config race — tests use `TestLogger.Create()` which mutates global `LogManager.Configuration` but class lacked `[Collection("LoggingTests")]`, allowing parallel execution with other NLog tests | Add `[Collection("LoggingTests")]` to `E2EHermeticGateTests` |
| `LoggerWarnOnceTests.WarnOnceWithEvent_Logs_OnlyOnce_PerKey` | Static `_warnOnceKeys` dictionary persists across tests — if another test used the same event+key combo, this test sees 0 logs | Call `LoggerExtensions.ClearWarnOnceKeysForTests()` in constructor |
| `BrainarrOrchestratorTopUpTests.FetchRecommendations_WithTopUpEnabled_FillsToTarget` | `DuplicationPreventionService` adds items to history during initial pipeline pass; when the final merge re-runs dedup, "Lana Del Rey" is falsely treated as already recommended. Normalization mismatch between `DuplicationPreventionService.NormalizeString()` (regex whitespace collapse) and `TopUpPlanner.NormalizeTopUpKey()` (no whitespace collapse) amplifies the issue. | Unify normalization logic: `TopUpPlanner.NormalizeTopUpKey()` should use the same regex whitespace normalization as `DuplicationPreventionService`. Also consider not adding items to dedup history until final merge is complete. |
| `BrainarrOrchestratorTopUpTests.FetchRecommendations_WithTopUpEnabled_FillsToTarget` | **Fixed.** Two issues: (1) `MusicBrainzResolver.EnrichWithMbidsAsync` catch block silently dropped recommendations on HTTP failure instead of preserving them (production bug). (2) Test created real resolvers that hit the MusicBrainz API, making results non-deterministic. | Production fix: added `result.Add(rec)` in catch block. Test fix: injected pass-through mock resolvers to eliminate external HTTP calls. |
Loading