fix(plugins): sub-package reload, symlinked dev plugins, BaseException in update(), config callbacks outside the lock - #741
Merged
Conversation
A plugin that keeps helpers in a package (providers/feed.py, imported as `from providers.feed import ...`) leaves dotted entries in sys.modules. PluginLoader only tracked bare names: `providers` was namespaced and dropped on unload, `providers.feed` stayed. A reload after a store update imported a fresh `providers`, then got the old `feed` back from the module cache, so the new manager.py ran against the old helpers until the display restarted. A load that failed part-way left them behind the same way. Elections (providers/), flights (enrichment/) and olympics (data/, renderers/) ship packages. The loader now records the dotted modules whose file (or, for a namespace package, every __path__ entry) lies inside the plugin directory. They keep their names while the plugin runs, as before, and unregister_plugin_modules() drops them, only while sys.modules still holds that plugin's module. The failed-load cleanup in load_module() drops them too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PluginStoreManager._safe_remove_directory, behind uninstall and behind discarding the set-aside copy after an install or update, handed a symlinked dev plugin (scripts/dev/dev_plugin_setup.sh) to shutil.rmtree, which refuses a symlink. The chmod fallback then walked through the link and set every directory and file in the linked checkout to 0700, and the sudo stage refused the resolved path as outside the plugins directory. The removal failed, the link stayed, and the developer's checkout lost its group/other permissions. A dangling link read as already removed, because exists() follows it, and was left behind. A symlink is now unlinked before any other stage runs, and before the exists() check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
contained_plugin_dir(), the containment check before a plugin's dependencies are installed, resolved the plugin directory and looked for the resolved folder's name among the plugins directory's entries. A dev plugin symlinked in under its id by a name its checkout does not share -- `dev_plugin_setup.sh link-github foo <url>` clones ledmatrix-foo, the repository naming convention, and links it as plugins/foo -- has no such entry, so install_dependencies() returned False and the load failed with "Dependency installation failed", even with no requirements.txt. When the path sits directly in the plugins directory, the entry it names (the link) is looked up first; anything else is resolved and matched by name as before. The answer is still always rebuilt from a name os.scandir() returned for the plugins directory, so a path outside it is still refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On the async update worker, the wrapped update() finished its bookkeeping (_finish: release the plugin lock, drop the pending slot, state back to ENABLED) only for an Exception. asyncio.CancelledError and SystemExit derive from BaseException, so one raised from update() skipped _finish: the plugin kept its lock and stayed RUNNING for the life of the process, never rescheduled, with every display() skipped as busy. PluginExecutor caught only Exception as well, so its thread died with the call never marked complete and an immediate failure was logged and recorded as a timeout. _target_update now runs _finish for any BaseException and re-raises it, and the executor's thread stores it like any other exception, so it is reported as the operation's failure (PluginError) on both the async and the synchronous path. _finish and _record_update_failure take a BaseException. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ConfigService._load_config ran every subscriber while holding _lock. The display's per-plugin subscriber calls PluginManager.apply_config_change, which waits up to PLUGIN_LOCK_TIMEOUT (5 s) for a plugin busy in update(). A save that enables or disables a plugin also flags a reconcile, which the render thread runs: its get_config(), and the unsubscribe() of a plugin it disables, both take _lock, so the panel froze behind every slow callback, up to 5 s per busy plugin. The config is now swapped under _lock and the subscribers are called after it is released, from a copy of the subscriber lists. A separate _notify_lock is held across a whole reload (read, swap, notify), so one reload's notifications still finish before the next one's start. Each callback is checked against the live lists just before it runs, and unsubscribe() waits only for a call of that same callback already in progress (unless it is that callback's own thread), so a callback it removed is not running and will not run once it returns, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (12)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 21 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
Summary
Five plugin-lifecycle bugs, one commit each.
plugin_loader.py)._iter_plugin_bare_modulesskipped any dotted name. Soproviders.feed(elections),enrichment.*(flights) anddata/,renderers/(olympics) stayed insys.modulesafter unload. The reloadedmanager.pythen ran against the old helpers.__path__) is inside the plugin folder are now recorded. They are dropped on unload, only whilesys.modulesstill holds that plugin's object, and on a failed load.store_manager._safe_remove_directory).rmtreerefuses a symlink, so it fell through to the chmod stage. That walked through the link and set the linked repo's files to 0700. The sudo stage was then refused.contained_plugin_dir).link-github foo <url>clones toledmatrix-fooand links it asplugins/foo.install_dependenciesreturned False with norequirements.txtat all.os.scandir()returned for the plugins folder, and a path outside it is still refused.BaseExceptionfromupdate()left the plugin stuck until restart._target_updatecaught onlyException. ACancelledErrororSystemExitskipped_finish, so the lock stayed held and the state stayed RUNNING. The plugin was never rescheduled, and everydisplay()was skipped.BaseException, finishes, and re-raises.PluginExecutor's thread catches it too, so an instant failure is reported as a failure, not as a 30 s timeout.get_config()waited behind it._lock, and subscribers are notified afterwards. A separate_notify_lockkeeps one reload's notifications ahead of the next.unsubscribe()returns, that callback is neither running nor will run.Tests
test_plugin_loader_reload_isolation.py(4)test_store_symlinked_plugin.py(4)test_plugin_loader_symlinked_dir.py(4)test_config_service_notify.py(6)BaseExceptioncases intest_async_plugin_updates.pyandtest_plugin_system.pyscripts/check_types.py: no issues.Part of a bug sweep
This is one of 10 independent fix PRs from one sweep, all based on
mainef69201.main: the same 62 failures and 6 errors on both. These are the known Windows path and file-locking tests. 191 more tests pass.test_backup_manager.py::test_create_backup_contents(os.replace→WinError 5on a temp zip), was a Windows file-lock flake. It passes on rerun, and nothing here touchescreate_backup.main, with a clean start and no errors or render stalls in the journal. ledpi is back on plainmain.🤖 Generated with Claude Code