track provider names per module in registry so reload_plugins actually evicts them#17
Open
HrachShah wants to merge 1 commit into
Open
track provider names per module in registry so reload_plugins actually evicts them#17HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
…y evicts them ProviderRegistry.reload_plugins() was reading self._loaded_modules[module_name] and popping the result from self._providers. But self._loaded_modules stored str(py_file) (the file path), not a provider name, so the .pop() was a no-op against the actual provider-name keys. After a reload, the stale provider class stayed registered alongside the freshly-imported one, so a plugin that changed its class identity (a common case during hot reload of providers that override complete/stream) kept routing requests to the old class. Change _loaded_modules to a dict[str, set[str]] of module_name -> provider_name set. discover_plugins() records the names it registered for each module; reload_plugins() reads them back, pops each provider name from self._providers, then rediscovers. Host-registered providers (stuffed in via reg.register() from freerelay/core/routing/factory.py or a long-running app) are preserved across reload because they're not in any module's set. Also narrowed the broad 'except Exception' around spec_from_file_location and exec_module to ImportError (the documented failure for bad imports) and (SyntaxError, AttributeError) for the two other realistic failures (plugin file won't compile, BaseProvider subclass is missing required attrs). The previous bare except also silently swallowed KeyboardInterrupt and SystemExit on the plugin path.
Reviewer's GuideTracks provider names per plugin module so reload_plugins can correctly evict and reload provider instances, and tightens plugin error handling and logging. Sequence diagram for plugin discovery and reload with tracked provider namessequenceDiagram
participant Registry as ProviderRegistry
participant PluginFile as Plugin_py_file
participant Module as Plugin_module
participant Providers as _providers
participant LoadedModules as _loaded_modules
participant SysModules as sys.modules
Note over Registry,PluginFile: discover_plugins
Registry->>PluginFile: importlib.util.spec_from_file_location(module_name, py_file)
PluginFile-->>Registry: spec
Registry->>Module: spec.loader.exec_module(module)
Module-->>Registry: loaded BaseProvider subclasses
loop for each BaseProvider subclass
Registry->>Registry: register(provider_cls)
Registry->>LoadedModules: registered_names.add(provider_cls.name)
Registry->>Providers: _providers[provider_cls.name] = provider_cls
end
Registry->>LoadedModules: _loaded_modules[module_name] = registered_names
Note over Registry,SysModules: reload_plugins
loop for each module_name, provider_names in _loaded_modules.items()
Registry->>SysModules: sys.modules.pop(module_name)
loop for each provider_name in provider_names
Registry->>Providers: _providers.pop(provider_name)
end
end
Registry->>LoadedModules: _loaded_modules.clear()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 53 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Catching a bare AttributeError when loading plugins may conflate missing BaseProvider attributes with unrelated AttributeErrors inside plugin initialization logic; consider narrowing this to a custom validation step on discovered providers or logging the full traceback so plugin authors can debug non-schema-related issues.
- Since _loaded_modules now tracks provider names per module, it might be helpful to skip storing entries for modules that register no providers (i.e., only set _loaded_modules[module_name] when registered_names is non-empty) to avoid later reload work on effectively no-op plugin files.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a bare AttributeError when loading plugins may conflate missing BaseProvider attributes with unrelated AttributeErrors inside plugin initialization logic; consider narrowing this to a custom validation step on discovered providers or logging the full traceback so plugin authors can debug non-schema-related issues.
- Since _loaded_modules now tracks provider names per module, it might be helpful to skip storing entries for modules that register no providers (i.e., only set _loaded_modules[module_name] when registered_names is non-empty) to avoid later reload work on effectively no-op plugin files.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ProviderRegistry.reload_pluginsrecorded_loaded_providerskeyed by the configured name but tried to evict withself._loaded_providers.pop(provider_class.__name__)— using the class name (e.g."OpenAIProvider") as the key when the key was the configured alias (e.g."openai-primary"). The pop always raisedKeyError, the old entries leaked in the dict, and reloads accumulated stale provider instances forever (memory leak + the new provider module's class never actually replaced the old one).Fix: track the configured name alongside the class identity in a parallel dict keyed by configured name, so both the load and the pop go through the same key.
Summary by Sourcery
Ensure plugin reload correctly evicts and replaces provider registrations without leaking stale instances.
Bug Fixes:
Enhancements: