[Bug]: Synchronize loved tracks fails if metadata not found

StaticRocket commented 3 months ago

Bug description

If a metadata query fails the entire sync stops.

Steps to reproduce

Like a song without any metadata
Trigger the "Synchronize loved tracks" task
Observe early exit of task

Expected behavior

Metadata query failures should be logged indicating what track failed, but it should not kill the sync.

Actual behavior

No response

Jellyfin logs

Jellyfin.Plugin.ListenBrainz.Exceptions.MetadataException: No metadata in response
   at Jellyfin.Plugin.ListenBrainz.Clients.MusicBrainzClient.GetAudioItemMetadata(BaseItem item)
   at Jellyfin.Plugin.ListenBrainz.Tasks.LovedTracksSyncTask.<>c__DisplayClass22_0.<HandleFavoriteSync>b__3(BaseItem i)
   at System.Linq.Enumerable.SelectListIterator`2.MoveNext()
   at Jellyfin.Plugin.ListenBrainz.Tasks.LovedTracksSyncTask.HandleFavoriteSync(IProgress`1 progress, UserConfig userConfig, CancellationToken cancellationToken)
   at Jellyfin.Plugin.ListenBrainz.Tasks.LovedTracksSyncTask.ExecuteAsync(IProgress`1 progress, CancellationToken cancellationToken)
   at Emby.Server.Implementations.ScheduledTasks.ScheduledTaskWorker.ExecuteInternal(TaskOptions options)

Plugin version

4.0.0.2

Jellyfin Version

10.9.6

Additional info

No response

lyarenei commented 3 months ago

Hi @StaticRocket, thanks for the report and sorry for the late reply. I'm currently pretty far away from my usual setup, so I tried to fix the code, but I am not able to test it until I get back.

If you would like to test the fix in the meantime, you can temporarily change to a development repo (https://repo.xkrivo.net/jellyfin-dev/manifest.json), where you can install plugin version 4.0.1.0. Please let me know if there are any issues, thanks!

StaticRocket commented 3 months ago

Cool, just pulled it and triggered a sync. Will let you know how it goes in a few hours. Thanks for the quick turnaround on this!

StaticRocket commented 3 months ago

Yep, that works pretty well. Seems like there are some rate limiting issues with ListenBrainz though:

Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 3 seconds
[2024-06-19 04:03:21.721 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 9 seconds
[2024-06-19 04:03:34.404 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 3 seconds
[2024-06-19 04:03:37.538 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 9 seconds

But that's a different issue. Thanks again!

lyarenei commented 3 months ago

Yep, that works pretty well.

Glad to hear that! 🙂

Seems like there are some rate limiting issues with ListenBrainz though:

Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 3 seconds
[2024-06-19 04:03:21.721 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 9 seconds
[2024-06-19 04:03:34.404 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 3 seconds
[2024-06-19 04:03:37.538 -05:00] [WRN] [39] Jellyfin.Plugin.ListenBrainz.FavoriteSyncTask: Request failed, will retry after 9 seconds

But that's a different issue. Thanks again!

These will be probably from MusicBrainz as they allow on average 1 request per second, so these are kind of expected.

StaticRocket commented 3 months ago

Yeah, but the MusicBrains plugin itself has a rate limit setting set to 2 seconds by default. I bumped it up to 3 seconds for that sync...

Does the listenbrainz plugin do an extra query on top of that ignoring the rate limit setting?

lyarenei commented 3 months ago

Yeah, but the MusicBrains plugin itself has a rate limit setting set to 2 seconds by default. I bumped it up to 3 seconds for that sync...

Does the listenbrainz plugin do an extra query on top of that ignoring the rate limit setting?

The current plugin system inherited from Emby is pretty much built on the concept that a plugin is completely standalone entity - inter-plugin communication is not possible (or at least I'm not aware of any way) and a plugin should ideally not have any dependencies (as I found in #89). So yeah, modifying that setting of MusicBrainz plugin did not change anything. :)

I'm not sure where Jellyfin got the information about 1 request every 2 seconds, but maybe they are just conservative. Anyway, I did not plan to allow customizing this behavior as MusicBrainz API should be used pretty infrequently (basically only for additional metadata - which are not needed in this sync task), and so ocassional retry wouldn't be anything worth dealing with. The current situation is a bit different, due to absence of recording MBIDs.

StaticRocket commented 3 months ago

Ah, roger. I was expecting something like that to prevent plugins from snooping api keys from other plugins and such.

Now that last comment is a bit interesting. The query should only fire in the case an MBID is missing? It may just be the timeout logic going crazy, but I'm fairly certain I'm sending more requests than I have untagged media in my library according to these logs...

lyarenei commented 3 months ago

The query should only fire in the case an MBID is missing? It may just be the timeout logic going crazy, but I'm fairly certain I'm sending more requests than I have untagged media in my library according to these logs...

Oh, I was talking with an ideal scenario in mind - in which case the recording MBIDs are provided by Jellyfin (if they are written in the file metadata). Currently they are not, so recording MBID needs to be fetched from MusicBrainz evey time it's needed. That's why you see a lot of requests and why the sync process takes so long.

lyarenei commented 3 months ago

Fixed in plugin versions 4.0.1.0 (JF 10.9.x) and 3.4.2.0 (JF 10.8.x)

lyarenei / jellyfin-plugin-listenbrainz