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 67.23% 67.31% +0.08%
==========================================
Files 120 120
Lines 18440 18481 +41
Branches 3134 3137 +3
==========================================
+ Hits 12398 12441 +43
+ Misses 5372 5367 -5
- Partials 670 673 +3
🚀 New features to boost your workflow:
|
|
Related:
|
semohr
force-pushed
the
error_handling_metadata_plugins
branch
2 times, most recently
from
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.
semohr
force-pushed
the
error_handling_metadata_plugins
branch
from
7cb1b4c to
07531a2
Compare
the autotag process.
semohr
force-pushed
the
error_handling_metadata_plugins
branch
from
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.
| """Return a list of all loaded metadata source plugins.""" | ||
| # TODO: Make this an isinstance(MetadataSourcePlugin, ...) check in v3.0.0 | ||
| return [p for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc] | ||
| return [SafeProxy(p) for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc,arg-type] |
There was a problem hiding this comment.
I guess this is where we do this selectively given the new configuration value?
| except Exception as e: | ||
| return self.__handle_exception(self.__plugin.album_for_id, e) | ||
|
|
||
| def track_for_id(self, track_id: str): |
There was a problem hiding this comment.
This needs *args, **kwargs for consistency
| if config["raise_on_error"].get(bool): | ||
| raise e |
There was a problem hiding this comment.
I think we would preferably handle this configuration option in find_metadata_source_plugins. This way, when we have, raise_on_error: no, exceptions will be raised truly directly. This way, we also have a fallback in case something goes bad with SafeProxy implementation, since we have an option to disable it.
There was a problem hiding this comment.
Sounds reasonable. I was thinking we handle it here incase we want to attach the option to each plugin instead. Would allow for more granual control.
Although, I'm not sure we want to go this route.
There was a problem hiding this comment.
I cannot really see a use case for granular control here.
FYI beetcamp already handles its exceptions, where it points users to a URL to submit a GitHub issue when something unexpected happens.
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: