Safely handle metadata plugin exceptions. - #5965
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5965 +/- ##
==========================================
+ Coverage 68.87% 68.95% +0.08%
==========================================
Files 140 140
Lines 18666 18685 +19
Branches 3063 3058 -5
==========================================
+ Hits 12856 12885 +29
+ Misses 5162 5153 -9
+ Partials 648 647 -1
🚀 New features to boost your workflow:
|
|
Related:
|
cbf389c to
88741c3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds exception handling to metadata plugin operations to prevent crashes during the auto-tagger process. When individual metadata plugins encounter errors, the system will now log the issues and continue processing with other available metadata sources instead of failing completely.
- Introduces safe wrapper functions (
_safe_calland_safe_yield_from) around metadata plugin method calls - Updates all plugin interface methods to use exception handling
- Adds comprehensive test coverage for error scenarios in metadata plugins
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| beets/metadata_plugins.py | Core implementation of exception handling with safe wrapper functions and updated plugin interface calls |
| test/test_metadata_plugins.py | New test file with mock error plugin and comprehensive test coverage for all metadata plugin methods |
| docs/changelog.rst | Documentation of the bug fix in the changelog |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Reviewer's GuideThis PR implements robust exception handling around metadata plugin calls by introducing safe-call and safe-yield helpers with logging, updating all plugin invocations to use these wrappers, refining related type signatures, and adding tests and a changelog entry to validate and document the new behavior. Flow diagram for exception handling in plugin callsflowchart TD
A["AutoTagger calls plugin method"] --> B{Exception raised?}
B -- Yes --> C["Log error"]
C --> D["Continue to next plugin"]
B -- No --> E["Process plugin result"]
E --> D
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The log.error calls use curly-brace placeholders but Python’s logging module expects %-style formatting or preformatted strings—update them to use %s or f-strings so the plugin name and exception actually interpolate.
- As mentioned in the PR limitations, consider adding a config flag to let users opt in to failing on plugin exceptions instead of always swallowing errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The log.error calls use curly-brace placeholders but Python’s logging module expects %-style formatting or preformatted strings—update them to use %s or f-strings so the plugin name and exception actually interpolate.
- As mentioned in the PR limitations, consider adding a config flag to let users opt in to failing on plugin exceptions instead of always swallowing errors.
## Individual Comments
### Comment 1
<location> `test/test_metadata_plugins.py:63` </location>
<code_context>
+ with caplog.at_level("ERROR"):
+ # Call the method to trigger the error
+ ret = getattr(metadata_plugins, method_name)(*args)
+ if isinstance(ret, Iterable):
+ list(ret)
+
</code_context>
<issue_to_address>
Test does not verify that normal (non-error) plugins still work alongside error-raising plugins.
Add a test that registers both an error-raising and a normal plugin to confirm that exceptions in one do not affect the results from others.
</issue_to_address>
### Comment 2
<location> `test/test_metadata_plugins.py:67` </location>
<code_context>
+ list(ret)
+
+ # Check that an error was logged
+ assert len(caplog.records) == 1
+ logs = [record.getMessage() for record in caplog.records]
+ assert logs == ["Error in 'ErrorMetadataMockPlugin': Mocked error"]
</code_context>
<issue_to_address>
Test only checks for error-level logs, but does not verify that debug-level logs (with exception details) are emitted.
Please add assertions to check that debug-level logs with exception details are present, ensuring complete error context is captured.
</issue_to_address>
### Comment 3
<location> `test/test_metadata_plugins.py:69` </location>
<code_context>
+ # Check that an error was logged
+ assert len(caplog.records) == 1
+ logs = [record.getMessage() for record in caplog.records]
+ assert logs == ["Error in 'ErrorMetadataMockPlugin': Mocked error"]
+ caplog.clear()
</code_context>
<issue_to_address>
Test only checks for a single error log per method call, but does not verify that repeated calls do not accumulate unexpected logs.
Add a test to confirm that multiple calls to the method do not produce extra or unexpected log entries.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
It does feel like we should pause the import until the user has acknowledged the issue (maybe providing options to skip importing the album or continue with existing metadata?), as otherwise you could end up with missing metadata that is awkward to fix later if for example you had network issues or the provider was down. Not sure how this should interact with |
|
A prompt makes sense in theory, but I'm really hesitant to add any user interaction because it breaks headless scripts, automation, and make monkey-patching way harder which is a big part of my beets use case. Instead, I'm leaning towards adding a simple config option that lets you tell the plugin to just fail hard if it runs into trouble, so your import would stop and you'd know right away. On a practical level, I'm also not convinced that skipping these errors is a major issue in the first place. The plugin's job is to suggest |
henry-oberholtzer
left a comment
henry-oberholtzer
left a comment
There was a problem hiding this comment.
Somehow completely missed this PR when you first put it in - but I've been encountering this exact issue a lot myself and it makes wanting to use batch import really discouraging, especially with the Musicbrainz rate limiting. This is a great fix for it.
This comment was marked as outdated.
This comment was marked as outdated.
07531a2 to
fc600e6
Compare
|
@henry-oberholtzer I rethought my approach here a bit and opted to use a proxy pattern instead. Seems a bit more clear and maintainable imo. |
|
@snejus Would you mind having a look here too? I would be happy to know your thoughts on this. Im pretty happy with the general implementation now and can finalize this if we are happy with the approach. I like the proxy approach (while a bit complex) it is very unintrusive, only hooking into our existing logic at one place. |
|
I'm not the most familiar with proxies, but this looks good to me, same logic, neater application. |
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
That's a very clean way to handle exceptions, nicely done! See my note about at which point should the new configuration come into play here.
f924c5c to
a69f575
Compare
|
@semohr As far as I remember this was initially implemented using decorator pattern. Why did you switch to the proxy pattern? I'm most likely missing context, but, I think, the decorator pattern would allow this to be type-checked? |
|
I'm thinking something in these lines indeed def _safe_call(
plugin: MetadataSourcePlugin,
method_name: str,
*args,
**kwargs,
):
"""Safely call a plugin method, catching and logging exceptions."""
try:
method = getattr(plugin, method_name)
return method(*args, **kwargs)
except Exception as e:
log.error(
"Error in '{}.{}': {}",
plugin.data_source,
method_name,
e,
)
log.debug("Exception details:", exc_info=True)
return None
@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():
if result := _safe_call(plugin, "candidates", *args, **kwargs):
yield from resultAnother question: does |
|
This is mostly how i had it before. This does not catch the errors during the yield and handling this properly requires a bit more work. Also typing is stripped here. I think retaining the initial behavior is wanted, sometimes one doesn't want to continue if an error occurs as some crucial lookup (for some) might be missing. See also #5965 (comment) |
A pure decorator-based approach may be the way to go forward, then: @contextmanager
def handle_plugin_error(method_name: str):
"""Safely call a plugin method, catching and logging exceptions."""
try:
yield
except Exception as e:
log.error("Error in '{}': {}", method_name, e)
log.debug("Exception details:", exc_info=True)
def safe_yield(func: IterF[P, Ret]) -> IterF[P, Ret]:
@wraps(func)
def wrapper(*args: P.args, **kwargs: P.kwargs) -> Iterable[Ret]:
with handle_plugin_error(func.__qualname__):
yield from func(*args, **kwargs)
return wrapper
@notify_info_yielded("albuminfo_received")
@safe_yield
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)To make sure this decorator is suitable for
Presumably, this depends on us handling errors somewhere? Otherwise, right now, an unhandled plugin error kills beets execution. |
This seems like the desired behavior in that case for me. E.g. I want the process to exit completely if musicbrainz is currently down.
The issue with that approach is that it does not log the plugin name correctly (here the method_name would be I can try to restore my initial decorator based approach but it is way more complex and more intrusive. I think the proxy based approach is more maintainable. |
|
Hi all, chiming in because Im currently stumbling across this a lot in beets-flask - hitting API timeouts with musicbrainz every third run or so. I looked through the current PR state and am wondering what the downside of the proxy pattern is? The only downside I could imagine is, that the ProxyClass has to be updated when new abstract methods are added to the MetaPlugins, but I suppose this is in some form also the case with the decorators. |
|
The proxy is only ever used in the metadata_plugins.py file. Plugin developers should not interact with it in any way as far as I can see.
Isn't this the case for all config values in beets? Once they are loaded with confuse, future calls to the same confuse object will always return the value in memory. I think having a live relodable config is out of scope here.
Agreed it does not feel too pythonic. Feelings aside tho, it does work and is pretty nonintrussive which is a big plus from the maintainability side for me. We might want to create a full decorator based implementation to compare in another branch. Im traveling currently but might come around it, I guees latest in a month or so. |
|
Decorator-based approach that should satisfy each of our requirements: @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 handle_plugin_error(plugin, method_name):
yield from filter(None, method(*args, **kwargs))
return wrapper
@notify_info_yielded("albuminfo_received")
@_yield_from_plugins
def candidates(*args, **kwargs) -> Iterator[AlbumInfo]:
yield from ()
@notify_info_yielded("trackinfo_received")
@_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:
return next(albums_for_ids([_id]), None)
def track_for_id(_id: str) -> TrackInfo | None:
return next(tracks_for_ids([_id]), None) |
|
Looks reasonable 👍 Some details ofc missing but we should be able to adapt that. |
0aae80e to
cece9b2
Compare
the autotag process.
cece9b2 to
2196bd8
Compare
|
Going to merge this in as it is now since I'm keen to have this in the new release. We can revise the details a bit later if needed! |
|
Just seen the change. We should find another way here imo. |
Beets 2.5.1 crashes the entire import when MusicBrainz returns a transient network error (connection reset). This was fixed in 2.6.0 (beetbox/beets#5965). Nixpkgs-unstable has 2.6.2, so build the full beets stack from unstable python3.
Description
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
Decisions needed
How do we want to name the configuration option which controls if an error should be propagated and also where/how do we want this to be documented?
Todos: