Safely handle metadata plugin exceptions. (#5965)

When a metadata plugin raises an exception during the auto-tagger
process, the entire operation crashes. This behavior is not desirable,
since metadata lookups can legitimately fail for various reasons (e.g.,
temporary API downtime, network issues, or offline usage).

This PR introduces a safeguard by adding general exception handling
around metadata plugin calls. Instead of causing the whole process to
fail, exceptions from individual plugins are now caught and logged. This
ensures that the auto-tagger continues to function with the remaining
available metadata sources. I used a proxy pattern here as this
seems like an elegant solution to me.

This replaces the efforts from #5910

beets/config_default.yaml

@@ -10,6 +10,8 @@ plugins: [musicbrainz]
 
 pluginpath: []
 
+raise_on_error: no
+
 # --------------- Import ---------------
 
 clutter: ["Thumbs.DB", ".DS_Store"]

beets/metadata_plugins.py

@@ -9,69 +9,98 @@ from __future__ import annotations
 
 import abc
 import re
-from functools import cache, cached_property
+from contextlib import contextmanager, nullcontext
+from functools import cache, cached_property, wraps
 from typing import TYPE_CHECKING, Generic, Literal, TypedDict, TypeVar
 
 import unidecode
 from confuse import NotFoundError
 from typing_extensions import NotRequired
 
+from beets import config, logging
 from beets.util import cached_classproperty
 from beets.util.id_extractors import extract_release_id
 
-from .plugins import BeetsPlugin, find_plugins, notify_info_yielded, send
+from .plugins import BeetsPlugin, find_plugins, notify_info_yielded
 
 if TYPE_CHECKING:
-    from collections.abc import Iterable, Sequence
+    from collections.abc import Callable, Iterable, Iterator, Sequence
 
     from .autotag.hooks import AlbumInfo, Item, TrackInfo
 
+    Ret = TypeVar("Ret")
+
+# Global logger.
+log = logging.getLogger("beets")
+
 
 @cache
 def find_metadata_source_plugins() -> list[MetadataSourcePlugin]:
     """Return a list of all loaded metadata source plugins."""
     # TODO: Make this an isinstance(MetadataSourcePlugin, ...) check in v3.0.0
+    # This should also allow us to remove the type: ignore comments below.
     return [p for p in find_plugins() if hasattr(p, "data_source")]  # type: ignore[misc]
 
 
+@contextmanager
+def handle_plugin_error(plugin: MetadataSourcePlugin, method_name: str):
+    """Safely call a plugin method, catching and logging exceptions."""
+    try:
+        yield
+    except Exception as e:
+        log.error("Error in '{}.{}': {}", plugin.data_source, method_name, e)
+        log.debug("Exception details:", exc_info=True)
+
+
+def _yield_from_plugins(
+    func: Callable[..., Iterable[Ret]],
+) -> Callable[..., Iterator[Ret]]:
+    method_name = func.__name__
+
+    @wraps(func)
+    def wrapper(*args, **kwargs) -> Iterator[Ret]:
+        for plugin in find_metadata_source_plugins():
+            method = getattr(plugin, method_name)
+            with (
+                nullcontext()
+                if config["raise_on_error"]
+                else handle_plugin_error(plugin, method_name)
+            ):
+                yield from filter(None, method(*args, **kwargs))
+
+    return wrapper
+
+
 @notify_info_yielded("albuminfo_received")
-def candidates(*args, **kwargs) -> Iterable[AlbumInfo]:
-    """Return matching album candidates from all metadata source plugins."""
-    for plugin in find_metadata_source_plugins():
-        yield from plugin.candidates(*args, **kwargs)
+@_yield_from_plugins
+def candidates(*args, **kwargs) -> Iterator[AlbumInfo]:
+    yield from ()
 
 
 @notify_info_yielded("trackinfo_received")
-def item_candidates(*args, **kwargs) -> Iterable[TrackInfo]:
-    """Return matching track candidates fromm all metadata source plugins."""
-    for plugin in find_metadata_source_plugins():
-        yield from plugin.item_candidates(*args, **kwargs)
+@_yield_from_plugins
+def item_candidates(*args, **kwargs) -> Iterator[TrackInfo]:
+    yield from ()
+
+
+@notify_info_yielded("albuminfo_received")
+@_yield_from_plugins
+def albums_for_ids(*args, **kwargs) -> Iterator[AlbumInfo]:
+    yield from ()
+
+
+@notify_info_yielded("trackinfo_received")
+@_yield_from_plugins
+def tracks_for_ids(*args, **kwargs) -> Iterator[TrackInfo]:
+    yield from ()
 
 
 def album_for_id(_id: str) -> AlbumInfo | None:
-    """Get AlbumInfo object for the given ID string.
-
-    A single ID can yield just a single album, so we return the first match.
-    """
-    for plugin in find_metadata_source_plugins():
-        if info := plugin.album_for_id(album_id=_id):
-            send("albuminfo_received", info=info)
-            return info
-
-    return None
+    return next(albums_for_ids([_id]), None)
 
 
 def track_for_id(_id: str) -> TrackInfo | None:
-    """Get TrackInfo object for the given ID string.
-
-    A single ID can yield just a single track, so we return the first match.
-    """
-    for plugin in find_metadata_source_plugins():
-        if info := plugin.track_for_id(_id):
-            send("trackinfo_received", info=info)
-            return info
-
-    return None
+    return next(tracks_for_ids([_id]), None)
 
 
 @cache

beets/plugins.py

@@ -35,7 +35,7 @@ from beets.util import unique_list
 from beets.util.deprecation import deprecate_for_maintainers, deprecate_for_user
 
 if TYPE_CHECKING:
-    from collections.abc import Callable, Iterable, Sequence
+    from collections.abc import Callable, Iterable, Iterator, Sequence
 
     from confuse import ConfigView
 
@@ -58,7 +58,6 @@ if TYPE_CHECKING:
     P = ParamSpec("P")
     Ret = TypeVar("Ret", bound=Any)
     Listener = Callable[..., Any]
-    IterF = Callable[P, Iterable[Ret]]
 
 
 PLUGIN_NAMESPACE = "beetsplug"
@@ -548,7 +547,7 @@ def named_queries(model_cls: type[AnyModel]) -> dict[str, FieldQueryType]:
 
 def notify_info_yielded(
     event: EventType,
-) -> Callable[[IterF[P, Ret]], IterF[P, Ret]]:
+) -> Callable[[Callable[P, Iterable[Ret]]], Callable[P, Iterator[Ret]]]:
     """Makes a generator send the event 'event' every time it yields.
     This decorator is supposed to decorate a generator, but any function
     returning an iterable should work.
@@ -556,9 +555,11 @@ def notify_info_yielded(
     'send'.
     """
 
-    def decorator(func: IterF[P, Ret]) -> IterF[P, Ret]:
+    def decorator(
+        func: Callable[P, Iterable[Ret]],
+    ) -> Callable[P, Iterator[Ret]]:
         @wraps(func)
-        def wrapper(*args: P.args, **kwargs: P.kwargs) -> Iterable[Ret]:
+        def wrapper(*args: P.args, **kwargs: P.kwargs) -> Iterator[Ret]:
             for v in func(*args, **kwargs):
                 send(event, info=v)
                 yield v

docs/changelog.rst

@@ -111,6 +111,10 @@ Bug fixes:
   avoiding extra lossy duplicates.
 - :doc:`plugins/discogs`: Fixed unexpected flex attr from the Discogs plugin.
   :bug:`6177`
+- Errors in metadata plugins during autotage process will now be logged but
+  won't crash beets anymore. If you want to raise exceptions instead, set the
+  new configuration option ``raise_on_error`` to ``yes`` :bug:`5903`,
+  :bug:`4789`.
 
 For plugin developers:
 

test/test_metadata_plugins.py

@@ -0,0 +1,83 @@
+from collections.abc import Iterable
+
+import pytest
+
+from beets import metadata_plugins
+from beets.test.helper import PluginMixin
+
+
+class ErrorMetadataMockPlugin(metadata_plugins.MetadataSourcePlugin):
+    """A metadata source plugin that raises errors in all its methods."""
+
+    def candidates(self, *args, **kwargs):
+        raise ValueError("Mocked error")
+
+    def item_candidates(self, *args, **kwargs):
+        for i in range(3):
+            raise ValueError("Mocked error")
+            yield  # This is just to make this a generator
+
+    def album_for_id(self, *args, **kwargs):
+        raise ValueError("Mocked error")
+
+    def track_for_id(self, *args, **kwargs):
+        raise ValueError("Mocked error")
+
+
+class TestMetadataPluginsException(PluginMixin):
+    """Check that errors during the metadata plugins do not crash beets.
+    They should be logged as errors instead.
+    """
+
+    @pytest.fixture(autouse=True)
+    def setup(self):
+        metadata_plugins.find_metadata_source_plugins.cache_clear()
+        self.register_plugin(ErrorMetadataMockPlugin)
+        yield
+        self.unload_plugins()
+
+    @pytest.fixture
+    def call_method(self, method_name, args):
+        def _call():
+            result = getattr(metadata_plugins, method_name)(*args)
+            return list(result) if isinstance(result, Iterable) else result
+
+        return _call
+
+    @pytest.mark.parametrize(
+        "method_name,error_method_name,args",
+        [
+            ("candidates", "candidates", ()),
+            ("item_candidates", "item_candidates", ()),
+            ("albums_for_ids", "albums_for_ids", (["some_id"],)),
+            ("tracks_for_ids", "tracks_for_ids", (["some_id"],)),
+            # Currently, singular methods call plural ones internally and log
+            # errors from there
+            ("album_for_id", "albums_for_ids", ("some_id",)),
+            ("track_for_id", "tracks_for_ids", ("some_id",)),
+        ],
+    )
+    def test_logging(self, caplog, call_method, error_method_name):
+        self.config["raise_on_error"] = False
+
+        call_method()
+
+        assert (
+            f"Error in 'ErrorMetadataMock.{error_method_name}': Mocked error"
+            in caplog.text
+        )
+
+    @pytest.mark.parametrize(
+        "method_name,args",
+        [
+            ("candidates", ()),
+            ("item_candidates", ()),
+            ("album_for_id", ("some_id",)),
+            ("track_for_id", ("some_id",)),
+        ],
+    )
+    def test_raising(self, call_method):
+        self.config["raise_on_error"] = True
+
+        with pytest.raises(ValueError, match="Mocked error"):
+            call_method()