From 782731052d75ca23a4fd47ae2beff25dec9acdb7 Mon Sep 17 00:00:00 2001 From: Chuck Date: Mon, 13 Jul 2026 09:11:21 -0400 Subject: [PATCH] fix(store): serialize concurrent updates per plugin, check cleanup results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #405 flagged two things in _reinstall_with_rollback, both verified against current code: - Real race: the web UI runs Flask with threaded=True and there's a single update route, so two overlapping requests for the same plugin_id (double-click, two tabs) can interleave. The loser could rename the winner's in-progress install aside mid-download, deleting its own rollback safety net — worse than the bug this function exists to fix. Added a lazy per-plugin_id lock dict (mirrors the plugin_manager per-plugin lock pattern) held for the whole function. - _safe_remove_directory's return value was ignored at both call sites. Stale-aside cleanup failure now aborts cleanly instead of falling through to a rename that would fail anyway with a less useful error; post-success backup-removal failure now logs instead of failing silently (still returns True — the update itself succeeded, and the next update self-heals the leftover aside). Left the third nitpick (test_stale_aside_from_previous_crash_is_cleared) addressed by asserting the stale dir is actually gone and that install_plugin was reached, rather than just the end-to-end result. Added a concurrency regression test asserting install_plugin never runs for the same plugin_id while another call is in flight. --- src/plugin_system/store_manager.py | 104 +++++++++++++++++++---------- test/test_store_update_rollback.py | 56 +++++++++++++++- 2 files changed, 124 insertions(+), 36 deletions(-) diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 6a1583bf..095eb206 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -142,9 +142,28 @@ class PluginStoreManager: # then get the result from the warm cache (double-checked locking). self._registry_fetch_lock = threading.Lock() + # Per-plugin locks for _reinstall_with_rollback: the web UI runs + # Flask with threaded=True, so two overlapping requests for the + # same plugin_id (double-click, two browser tabs) would otherwise + # both rename the same directory aside — one succeeds, and the + # 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] = {} + 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.""" + with self._reinstall_locks_guard: + lock = self._reinstall_locks.get(plugin_id) + if lock is None: + lock = threading.Lock() + self._reinstall_locks[plugin_id] = lock + return lock + def _record_cache_backoff(self, cache_dict: Dict, cache_key: str, cache_timeout: int, payload: Any) -> None: """Bump a cache entry's timestamp so subsequent lookups hit the @@ -2277,45 +2296,60 @@ class PluginStoreManager: The aside name embeds '.standalone-backup-' so plugin discovery (plugin_manager._scan_directory_for_plugins) ignores it even though it still contains a manifest.json. + + Held for the whole operation under a per-plugin_id lock: two + overlapping requests for the same plugin (double-click, two + browser tabs — the web UI runs Flask with threaded=True) must not + interleave their renames, or the second could steal the first's + rollback safety net mid-install. Other plugin_ids are unaffected. """ - backup_path = plugin_path.with_name( - f"{plugin_path.name}.standalone-backup-migrating") - # A stale aside from a previous crash would block the rename - if backup_path.exists(): - self._safe_remove_directory(backup_path) - try: - plugin_path.rename(backup_path) - except OSError as e: + with self._get_reinstall_lock(plugin_id): + backup_path = plugin_path.with_name( + f"{plugin_path.name}.standalone-backup-migrating") + # A stale aside from a previous crash would block the rename + if backup_path.exists(): + if not self._safe_remove_directory(backup_path): + self.logger.error( + f"Could not clear stale backup for {plugin_id} at " + f"{backup_path}; leaving old install in place") + return False + try: + plugin_path.rename(backup_path) + except OSError as e: + self.logger.error( + f"Could not set aside old plugin directory for {plugin_id}: {e}") + return False + + try: + installed = self.install_plugin(plugin_id) + except Exception as e: + self.logger.error(f"Reinstall of {plugin_id} raised: {e}") + installed = False + + if installed: + if not self._safe_remove_directory(backup_path): + self.logger.warning( + f"Update of {plugin_id} succeeded but the old backup " + f"at {backup_path} could not be removed; it will be " + f"cleared on the next update") + return True + + # Install failed (bad network, registry error...) — put the old + # version back so the user still has a working plugin. self.logger.error( - f"Could not set aside old plugin directory for {plugin_id}: {e}") + f"Reinstall of {plugin_id} failed; restoring previous version") + try: + if plugin_path.exists(): + # partial download debris from the failed install + self._safe_remove_directory(plugin_path) + backup_path.rename(plugin_path) + self.logger.info(f"Restored previous install of {plugin_id}") + except OSError as e: + self.logger.error( + f"CRITICAL: could not restore {plugin_id} from {backup_path}: {e}. " + f"The previous install is preserved there — rename it back manually.") return False - try: - installed = self.install_plugin(plugin_id) - except Exception as e: - self.logger.error(f"Reinstall of {plugin_id} raised: {e}") - installed = False - - if installed: - self._safe_remove_directory(backup_path) - return True - - # Install failed (bad network, registry error...) — put the old - # version back so the user still has a working plugin. - self.logger.error( - f"Reinstall of {plugin_id} failed; restoring previous version") - try: - if plugin_path.exists(): - # partial download debris from the failed install - self._safe_remove_directory(plugin_path) - backup_path.rename(plugin_path) - self.logger.info(f"Restored previous install of {plugin_id}") - except OSError as e: - self.logger.error( - f"CRITICAL: could not restore {plugin_id} from {backup_path}: {e}. " - f"The previous install is preserved there — rename it back manually.") - return False - def update_plugin(self, plugin_id: str) -> bool: """ Update a plugin to the latest commit on its upstream branch. diff --git a/test/test_store_update_rollback.py b/test/test_store_update_rollback.py index a649518a..2b53da2a 100644 --- a/test/test_store_update_rollback.py +++ b/test/test_store_update_rollback.py @@ -10,6 +10,8 @@ plugins from one update pass. import json import os import sys +import threading +import time from pathlib import Path from unittest.mock import patch @@ -95,10 +97,62 @@ class TestReinstallWithRollback: stale = plugin_dir.parent / f"{PLUGIN_ID}.standalone-backup-migrating" stale.mkdir() (stale / "old.txt").write_text("stale") - with patch.object(mgr, "install_plugin", return_value=False): + with patch.object(mgr, "install_plugin", return_value=False) as mock_install: ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + # The reinstall itself still fails (mocked) and the old install is + # restored, but the stale aside must not have survived — otherwise + # it would have blocked this run's own rename (or a future one). + assert not stale.exists() + mock_install.assert_called_once_with(PLUGIN_ID) assert ok is False assert plugin_dir.exists() + assert "old version marker" in (plugin_dir / "manager.py").read_text() + + def test_concurrent_updates_for_same_plugin_are_serialized(self, store): + """Two overlapping requests for the same plugin_id (double-click, + two browser tabs — the web UI runs Flask with threaded=True) must + not interleave: the loser must wait for the winner to finish + rather than renaming the winner's in-progress install aside and + stealing its rollback safety net.""" + mgr, plugin_dir = store + + active = 0 + max_active = 0 + guard = threading.Lock() + + def fake_install(plugin_id): + nonlocal active, max_active + with guard: + active += 1 + max_active = max(max_active, active) + time.sleep(0.05) + plugin_dir.mkdir(exist_ok=True) + (plugin_dir / "manager.py").write_text("# new version\n") + (plugin_dir / "manifest.json").write_text(json.dumps( + {"id": PLUGIN_ID, "name": "Rollback Test", "version": "2.0.0"})) + with guard: + active -= 1 + return True + + results = [] + + def worker(): + results.append(mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir)) + + with patch.object(mgr, "install_plugin", side_effect=fake_install): + threads = [threading.Thread(target=worker) for _ in range(2)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=5) + + assert max_active == 1, "install_plugin ran concurrently for the same plugin_id" + assert results == [True, True] + assert plugin_dir.exists() + assert "new version" in (plugin_dir / "manager.py").read_text() + leftovers = [p for p in plugin_dir.parent.iterdir() + if "standalone-backup" in p.name] + assert leftovers == [] def test_aside_name_is_invisible_to_discovery(self, store, tmp_path): """The aside still contains a manifest.json — discovery must skip it