fix(plugins): improve plugin method calling and add metrics on callbacks #4304

deluan · 2025-07-03T04:03:26Z

Description

This PR refactors the plugin system to improve lifecycle management, method call architecture, and error handling. The changes streamline plugin initialization, enhance logging, and fix several issues with plugin method calls and WebSocket error handling.

Key improvements include:

Refactored wasmBasePlugin to baseCapability for better clarity
Implemented consistent method call patterns across all plugin adapters
Enhanced error handling with unified checkErr function
Improved plugin lifecycle management with proper cleanup for failed initializations
Fixed WebSocket error filtering in Discord plugin
Added missing OnSchedulerCallback method implementation
Streamlined scrobbler stopping logic with unified interface

Type of Change

Refactor

Checklist

Please review and check all that apply:

My code follows the project's coding style
I have tested the changes locally
I have added or updated documentation as needed
I have added tests that prove my fix/feature works (or explain why not)
All existing and new tests pass

How to Test

Run the plugin tests to ensure compatibility:
```
make test PKG=./plugins/...
```
Test plugin initialization scenarios:
- Test with valid plugins that should initialize successfully
- Test with plugins that return errors during initialization to verify proper cleanup
Test WebSocket functionality:
- Use the Discord Rich Presence plugin to verify WebSocket connections work properly
- Verify that expected "use of closed network connection" errors are filtered out
Test scheduler callbacks:
- Verify that plugins implementing SchedulerCallback properly receive and handle scheduled events
Test scrobbler functionality:
- Verify that scrobbler plugins can be started and stopped properly
- Test the new unified stoppableScrobbler interface

Additional Notes

This is a significant refactor of the plugin system architecture, but maintains backward compatibility for existing plugins. The changes focus on internal implementation improvements and don't affect the public plugin API.

Breaking Changes: None for plugin authors - all changes are internal to the plugin system implementation.

Testing Notes: All existing plugin tests have been updated to use the new architecture. The new checkErr function has comprehensive test coverage including edge cases for nil responses and error handling.

gemini-code-assist

Summary of Changes

Hello @deluan, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the plugin system by standardizing how plugin methods are invoked, improving consistency and reducing boilerplate. It also introduces crucial performance metrics for plugin initialization, providing better observability. These changes collectively lead to a more robust, maintainable, and observable plugin architecture.

Highlights

Plugin Method Call Consistency: Refactored WebSocket callbacks to use a unified callMethod pattern, and implemented OnSchedulerCallback in wasmSchedulerCallback for consistent scheduler handling. The scheduler service now uses direct method calls, reducing manual instantiation and boilerplate.
Metrics Integration: Added OnInit metrics capture to track plugin initialization performance and integrated the metrics service into the plugin lifecycle manager. This provides better observability into plugin startup.
Code Cleanup & Maintainability: Unified error handling across plugin callback types, reduced code duplication by leveraging the callMethod helper, and improved type safety with proper plugin type assertions. These changes contribute to a more robust and easier-to-maintain plugin system.
Testing Updates: All relevant test files were updated to include metrics.NewNoopInstance() when creating managers or lifecycle managers, ensuring proper testing of the new metrics integration without requiring a real metrics backend.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors the plugin method invocation to use a consistent callMethod helper, improving code clarity and maintainability. The addition of OnInit metrics enhances observability. I've identified a high-severity issue in plugins/host_websocket.go where an error from callMethod is being ignored, potentially leading to silent failures during plugin instantiation. Additionally, I've suggested logging errors when RecordPluginRequest fails in plugins/plugin_lifecycle_manager.go to aid in debugging.

plugins/host_websocket.go

plugins/plugin_lifecycle_manager.go

github-actions · 2025-07-03T04:12:40Z

Added the OnSchedulerCallback method to the wasmSchedulerCallback struct, enabling it to handle scheduler callback events. This method constructs a SchedulerCallbackRequest and invokes the corresponding plugin method, facilitating better integration with the scheduling system. The changes improve the plugin's ability to respond to scheduled events, enhancing overall functionality. Signed-off-by: Deluan <deluan@navidrome.org>

Modified the executeCallback method to accept an additional parameter, methodName, which specifies the callback method to be executed. This change ensures that the correct method is called for each WebSocket event, improving the accuracy of callback execution for plugins. Signed-off-by: Deluan <deluan@navidrome.org>

Signed-off-by: Deluan <deluan@navidrome.org>

Updated the logging statement in the callMethod function to include the elapsed time as a separate key in the log output. This change enhances the clarity of the logged metrics, making it easier to analyze the performance of plugin requests and troubleshoot any issues that may arise. Signed-off-by: Deluan <deluan@navidrome.org>

Signed-off-by: Deluan <deluan@navidrome.org>

Refactored the logic for stopping scrobbler instances when they are removed. The new implementation introduces a `stoppableScrobbler` interface to simplify the type assertion process, allowing for a more concise and readable code structure. This change ensures that any scrobbler implementing the `Stop` method is properly stopped before removal, improving the overall reliability of the plugin management system. Signed-off-by: Deluan <deluan@navidrome.org>

Enhanced the plugin lifecycle management by implementing error handling in the OnInit method. The changes include the addition of specific error conditions that can be returned during plugin initialization, allowing for better management of plugin states. Additionally, the unregisterPlugin method was updated to ensure proper cleanup of plugins that fail to initialize, improving overall stability and reliability of the plugin system. Signed-off-by: Deluan <deluan@navidrome.org>

Eliminated the LoadAllPlugins, LoadAllMediaAgents, and LoadAllScrobblers methods from the manager implementation as they were not utilized in the codebase. This cleanup reduces complexity and improves maintainability by removing redundant code, allowing for a more streamlined plugin management process. Signed-off-by: Deluan <deluan@navidrome.org>

Configured logging for multiple plugins to remove timestamps and source file/line information, while adding specific prefixes for better identification. Signed-off-by: Deluan <deluan@navidrome.org>

Copilot

Pull Request Overview

This PR refactors plugin callback invocation to use a unified callMethod helper, injects metrics tracking into initialization and callback flows, and cleans up plugin lifecycle handling.

Unified WebSocket and scheduler callbacks through callMethod, enabling consistent error mapping and metrics recording.
Enhanced plugin lifecycle manager and manager implementation to record OnInit metrics and cleanly unregister failed plugins.
Updated tests across the plugin suite to inject a metrics.NewNoopInstance() and verify initialization behavior.

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
plugins/wasm_base_plugin.go	Added `convertError` helper and improved `callMethod` logging
plugins/plugin_lifecycle_manager.go	Refactored `callOnInit` to return errors and record metrics
plugins/manager.go	Inject metrics into lifecycle manager and added `unregisterPlugin`
plugins/host_websocket.go	Updated `executeCallback` to use `callMethod` with metrics
plugins/host_scheduler.go	Switched scheduler callbacks to adapter + `callMethod`
plugins/adapter_scheduler_callback.go	Implemented adapter for scheduler callbacks with `callMethod`
Multiple `*_test.go` files	Updated tests to use `metrics.NewNoopInstance()`

Comments suppressed due to low confidence (1)

plugins/wasm_base_plugin.go:124

convertError is a new utility function handling multiple error cases; consider adding unit tests covering scenarios where resp implements errorResponse (with and without an error message) and where resp is nil to validate correct error mapping.

func convertError[T any](resp T, err error) (T, error) {

plugins/wasm_base_plugin.go

plugins/manager.go

Added functionality to clear the initialization state of a plugin in the lifecycle manager when it is unregistered. This change ensures that the lifecycle state is accurately maintained, preventing potential issues with plugins that may be re-registered after being unregistered. The new method `clearInitialized` was implemented to handle this state management. Signed-off-by: Deluan <deluan@navidrome.org>

Added comprehensive unit tests for the convertError function to ensure correct behavior across various scenarios, including handling nil responses, typed nils, and responses implementing errorResponse. These tests validate that the function returns the expected results without panicking and correctly wraps original errors when necessary. Signed-off-by: Deluan <deluan@navidrome.org>

deluan · 2025-07-04T01:19:54Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces improvements to the plugin system, including method call consistency, metrics integration, and robust error handling during plugin initialization. The changes are well-tested and improve the codebase. I've identified a couple of high-severity issues where errors could be silently ignored, potentially masking problems with plugin instantiation or execution. I've also identified a medium severity issue where an error is not logged. I've provided suggestions to fix these issues.

plugins/adapter_scheduler_callback.go

plugins/host_websocket.go

plugins/manager.go

Refactored the plugin base implementation by renaming `wasmBasePlugin` to `baseCapability` across multiple files. Updated method calls in the `wasmMediaAgent`, `wasmSchedulerCallback`, and `wasmScrobblerPlugin` to align with the new base structure. These changes improve code clarity and maintainability by standardizing the plugin architecture, ensuring consistent usage of the base capabilities across different plugin types. Signed-off-by: Deluan <deluan@navidrome.org>

Added a new method to clean up failed connections, which cancels the heartbeat schedule, closes the WebSocket connection, and removes cache entries. Enhanced the heartbeat check to log failures and trigger the cleanup process on the first failure. These changes ensure better management of user connections and improve the overall reliability of the RPC system. Signed-off-by: Deluan <deluan@navidrome.org>

Copilot AI review requested due to automatic review settings July 3, 2025 04:03

gemini-code-assist bot reviewed Jul 3, 2025

View reviewed changes

deluan changed the title ~~fix(plugins): improve plugin method calling and add OnInit metrics~~ fix(plugins): improve plugin method calling and add metrics on callbacks Jul 3, 2025

This comment was marked as outdated.

Sign in to view

gemini-code-assist bot reviewed Jul 3, 2025

View reviewed changes

plugins/host_websocket.go Outdated Show resolved Hide resolved

plugins/plugin_lifecycle_manager.go Show resolved Hide resolved

deluan added 5 commits July 3, 2025 12:50

fix(plugins): capture OnInit metrics

7fe9068

Signed-off-by: Deluan <deluan@navidrome.org>

fix(plugins): enhance logging for schedule callback execution

187f937

Signed-off-by: Deluan <deluan@navidrome.org>

deluan force-pushed the fix-plugin-method-call branch from 498564a to 187f937 Compare July 3, 2025 16:53

deluan added 4 commits July 3, 2025 13:34

fix(plugins): update logging configuration for plugins

dcbe733

Configured logging for multiple plugins to remove timestamps and source file/line information, while adding specific prefixes for better identification. Signed-off-by: Deluan <deluan@navidrome.org>

deluan force-pushed the fix-plugin-method-call branch from 2836328 to dcbe733 Compare July 4, 2025 00:31

deluan requested a review from Copilot July 4, 2025 00:36

Copilot AI reviewed Jul 4, 2025

View reviewed changes

plugins/wasm_base_plugin.go Outdated Show resolved Hide resolved

plugins/manager.go Show resolved Hide resolved

deluan added 2 commits July 3, 2025 20:52

gemini-code-assist bot reviewed Jul 4, 2025

View reviewed changes

plugins/adapter_scheduler_callback.go Outdated Show resolved Hide resolved

plugins/host_websocket.go Outdated Show resolved Hide resolved

plugins/manager.go Show resolved Hide resolved

deluan added 2 commits July 4, 2025 19:19

deluan merged commit 66eaac2 into master Jul 5, 2025
35 checks passed

deluan deleted the fix-plugin-method-call branch July 5, 2025 12:03

BrewTestBot mentioned this pull request Jul 28, 2025

navidrome 0.58.0 Homebrew/homebrew-core#231529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(plugins): improve plugin method calling and add metrics on callbacks #4304

fix(plugins): improve plugin method calling and add metrics on callbacks #4304

Uh oh!

deluan commented Jul 3, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 3, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

deluan commented Jul 4, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(plugins): improve plugin method calling and add metrics on callbacks #4304

fix(plugins): improve plugin method calling and add metrics on callbacks #4304

Uh oh!

Conversation

deluan commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Checklist

How to Test

Additional Notes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

deluan commented Jul 4, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deluan commented Jul 3, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading