diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 64bc573b..e13b3e5b 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -149,18 +149,27 @@ class PluginStoreManager: # loser can end up renaming the winner's in-progress install aside # mid-download, stealing its own rollback safety net. Keyed by # plugin_id so unrelated plugins still update concurrently. - self._reinstall_locks: Dict[str, threading.Lock] = {} + # Reentrant: install_plugin takes this lock, and _reinstall_with_rollback + # holds it across its call to install_plugin. A plain Lock would + # self-deadlock on that nesting. + self._reinstall_locks: Dict[str, "threading.RLock"] = {} self._reinstall_locks_guard = threading.Lock() # Ensure plugins directory exists self.plugins_dir.mkdir(exist_ok=True) - def _get_reinstall_lock(self, plugin_id: str) -> threading.Lock: - """Lazily create (or fetch) the per-plugin reinstall lock.""" + def _get_reinstall_lock(self, plugin_id: str): + """Lazily create (or fetch) the per-plugin reinstall lock. + + Reentrant by necessity: `install_plugin` acquires it to protect its + set-aside/restore, and `_reinstall_with_rollback` holds it across its + own call to `install_plugin`. With a plain `Lock` that nesting + deadlocks the request thread. + """ with self._reinstall_locks_guard: lock = self._reinstall_locks.get(plugin_id) if lock is None: - lock = threading.Lock() + lock = threading.RLock() self._reinstall_locks[plugin_id] = lock return lock @@ -1208,45 +1217,54 @@ class PluginStoreManager: The aside name embeds '.standalone-backup-' so plugin discovery (`plugin_manager._scan_directory_for_plugins`) skips it even though it still holds a manifest.json. + + Held under the per-plugin reinstall lock for the same reason + `_reinstall_with_rollback` is: the web UI runs Flask with + threaded=True, so a double-clicked Install button gives two threads the + same plugin_id. Interleaved, one thread's restore would delete the + other's freshly installed copy. The lock is reentrant because the + rollback path already holds it when it calls in here. """ - plugin_path = self.plugins_dir / plugin_id - if not plugin_path.exists(): - return self._install_plugin_impl(plugin_id, branch) + with self._get_reinstall_lock(plugin_id): + plugin_path = self.plugins_dir / plugin_id + if not plugin_path.exists(): + return self._install_plugin_impl(plugin_id, branch) - backup_path = plugin_path.with_name( - f"{plugin_path.name}.standalone-backup-preinstall") - if backup_path.exists() and not self._safe_remove_directory(backup_path): - # Can't stage a safety net. Better to attempt the install than to - # refuse outright, which is what the caller got before this existed. - self.logger.warning( - "Could not clear stale pre-install backup for %s at %s; " - "installing without a rollback net", plugin_id, backup_path) - return self._install_plugin_impl(plugin_id, branch) - - try: - plugin_path.rename(backup_path) - except OSError as e: - self.logger.warning( - "Could not set aside existing install of %s (%s); " - "installing without a rollback net", plugin_id, e) - return self._install_plugin_impl(plugin_id, branch) - - try: - installed = self._install_plugin_impl(plugin_id, branch) - except Exception: - self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) - raise - - if installed: - if not self._safe_remove_directory(backup_path): + backup_path = plugin_path.with_name( + f"{plugin_path.name}.standalone-backup-preinstall") + if backup_path.exists() and not self._safe_remove_directory(backup_path): + # Can't stage a safety net. Better to attempt the install than + # to refuse outright, which is what callers got before this + # existed. self.logger.warning( - "Install of %s succeeded but the previous copy at %s could " - "not be removed; it will be cleared on the next install", - plugin_id, backup_path) - return True + "Could not clear stale pre-install backup for %s at %s; " + "installing without a rollback net", plugin_id, backup_path) + return self._install_plugin_impl(plugin_id, branch) - self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) - return False + try: + plugin_path.rename(backup_path) + except OSError as e: + self.logger.warning( + "Could not set aside existing install of %s (%s); " + "installing without a rollback net", plugin_id, e) + return self._install_plugin_impl(plugin_id, branch) + + try: + installed = self._install_plugin_impl(plugin_id, branch) + except Exception: + self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + raise + + if installed: + if not self._safe_remove_directory(backup_path): + self.logger.warning( + "Install of %s succeeded but the previous copy at %s " + "could not be removed; it will be cleared on the next " + "install", plugin_id, backup_path) + return True + + self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + return False def _restore_preinstall_backup( self, plugin_id: str, plugin_path: Path, backup_path: Path diff --git a/test/test_install_preserves_existing.py b/test/test_install_preserves_existing.py index d1f9ea35..ff5b5554 100644 --- a/test/test_install_preserves_existing.py +++ b/test/test_install_preserves_existing.py @@ -150,3 +150,73 @@ class TestUpdatePathStillWorks: assert observed["dirs"] == ["hockey-scoreboard.standalone-backup-migrating"] # And the user still has their plugin. assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original" + + +class TestConcurrency: + """The web UI runs Flask threaded, so a double-clicked Install button puts + two threads on the same plugin_id. `_reinstall_with_rollback` already + guarded against this; the install wrapper has to as well, or one thread's + restore deletes the other's freshly installed copy.""" + + def test_rollback_calling_install_does_not_deadlock(self, store, monkeypatch): + """The rollback path holds the per-plugin lock across its call to + install_plugin. A non-reentrant lock would hang the request thread + forever — this test would time out rather than fail.""" + import threading + + mgr, plugins_dir = store + path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + monkeypatch.setattr( + mgr, "_install_plugin_impl", + lambda pid, branch=None: bool(_existing_install(plugins_dir, pid, "new"))) + + done = threading.Event() + result = {} + + def run(): + result["ok"] = mgr._reinstall_with_rollback("hockey-scoreboard", path) + done.set() + + t = threading.Thread(target=run, daemon=True) + t.start() + assert done.wait(timeout=10), ( + "install_plugin deadlocked when called from _reinstall_with_rollback " + "— the per-plugin lock must be reentrant" + ) + assert result["ok"] is True + + def test_concurrent_installs_serialize(self, store, monkeypatch): + """Two threads installing the same plugin must not interleave their + set-aside/restore, and the survivor must be a complete install.""" + import threading + + mgr, plugins_dir = store + _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + in_flight = [] + overlap = [] + + def slow_impl(plugin_id, branch=None): + in_flight.append(1) + if len(in_flight) > 1: + overlap.append(1) + threading.Event().wait(0.05) + _existing_install(plugins_dir, plugin_id, "installed") + in_flight.pop() + return True + + monkeypatch.setattr(mgr, "_install_plugin_impl", slow_impl) + + threads = [threading.Thread(target=mgr.install_plugin, + args=("hockey-scoreboard",), daemon=True) + for _ in range(2)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + assert not t.is_alive(), "concurrent install hung" + + assert not overlap, "two installs of the same plugin ran concurrently" + assert (plugins_dir / "hockey-scoreboard" / "marker.txt").exists() + leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name] + assert not leftovers, f"backup left behind: {leftovers}"