diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 059a1343..095eb206 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -142,9 +142,28 @@ def __init__(self, plugins_dir: str = "plugins", # 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 @@ -2263,6 +2282,74 @@ def uninstall_plugin(self, plugin_id: str) -> bool: self.logger.error(f"Error uninstalling plugin {plugin_id}: {e}") return False + def _reinstall_with_rollback(self, plugin_id: str, plugin_path: Path) -> bool: + """Replace an installed plugin with a fresh install, atomically. + + The old install is renamed aside (not deleted) until the new install + succeeds, then removed; on ANY install failure the old directory is + restored. This is the difference between a failed update and a + destroyed plugin: the previous delete-then-install flow permanently + removed plugins whenever the download failed mid-update (seen in the + field during the monorepo migration on a Pi with broken DNS — every + old-remote plugin was deleted and none could be re-downloaded). + + 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. + """ + 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"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. @@ -2325,10 +2412,7 @@ def update_plugin(self, plugin_id: str) -> bool: f"Plugin {resolved_id} git remote ({local_remote}) differs from registry ({registry_repo}). " f"Reinstalling from registry to migrate to new source." ) - if not self._safe_remove_directory(plugin_path): - self.logger.error(f"Failed to remove old plugin directory for {resolved_id}") - return False - return self.install_plugin(resolved_id) + return self._reinstall_with_rollback(resolved_id, plugin_path) # Check if already up to date if remote_sha and local_sha and remote_sha.startswith(local_sha): @@ -2632,11 +2716,11 @@ def update_plugin(self, plugin_id: str) -> bool: # Plugin is not a git repo but is in registry and has a newer version - reinstall self.logger.info(f"Plugin {plugin_id} not installed via git; re-installing latest archive (registry id: {registry_id})") - # Remove directory and reinstall fresh - if not self._safe_remove_directory(plugin_path): - self.logger.error(f"Failed to remove old plugin directory for {plugin_id}") - return False - return self.install_plugin(registry_id) + # Reinstall with the old version kept aside until the new + # download succeeds — this is the path every routine store + # update takes, and a mid-update network failure must not + # destroy the user's plugin. + return self._reinstall_with_rollback(registry_id, plugin_path) except Exception as e: import traceback diff --git a/test/test_store_update_rollback.py b/test/test_store_update_rollback.py new file mode 100644 index 00000000..2b53da2a --- /dev/null +++ b/test/test_store_update_rollback.py @@ -0,0 +1,171 @@ +"""Tests for atomic plugin updates (store_manager._reinstall_with_rollback). + +Regression for a field data-loss incident: update_plugin's reinstall paths +(monorepo migration AND routine archive updates) deleted the installed +plugin BEFORE downloading its replacement — a mid-update network failure +permanently destroyed the plugin. Seen live: a Pi with broken DNS lost 12 +plugins from one update pass. +""" + +import json +import os +import sys +import threading +import time +from pathlib import Path +from unittest.mock import patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from src.plugin_system.store_manager import PluginStoreManager # noqa: E402 + +PLUGIN_ID = "rollback-test-plugin" + + +@pytest.fixture +def store(tmp_path): + mgr = PluginStoreManager(plugins_dir=str(tmp_path)) + plugin_dir = tmp_path / PLUGIN_ID + plugin_dir.mkdir() + (plugin_dir / "manifest.json").write_text(json.dumps( + {"id": PLUGIN_ID, "name": "Rollback Test", "version": "1.0.0"})) + (plugin_dir / "manager.py").write_text("# old version marker\n") + return mgr, plugin_dir + + +class TestReinstallWithRollback: + def test_failed_install_restores_old_version(self, store): + """The whole point: a failed download must leave the old install.""" + mgr, plugin_dir = store + with patch.object(mgr, "install_plugin", return_value=False): + ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + assert ok is False + assert plugin_dir.exists() + assert "old version marker" in (plugin_dir / "manager.py").read_text() + # no aside debris left behind + leftovers = [p for p in plugin_dir.parent.iterdir() + if "standalone-backup" in p.name] + assert leftovers == [] + + def test_install_exception_restores_old_version(self, store): + mgr, plugin_dir = store + with patch.object(mgr, "install_plugin", + side_effect=RuntimeError("network down")): + ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + assert ok is False + assert plugin_dir.exists() + assert "old version marker" in (plugin_dir / "manager.py").read_text() + + def test_successful_install_removes_aside(self, store): + mgr, plugin_dir = store + + def fake_install(plugin_id): + new_dir = plugin_dir # same path, new content + new_dir.mkdir(exist_ok=True) + (new_dir / "manager.py").write_text("# new version\n") + (new_dir / "manifest.json").write_text(json.dumps( + {"id": PLUGIN_ID, "name": "Rollback Test", "version": "2.0.0"})) + return True + + with patch.object(mgr, "install_plugin", side_effect=fake_install): + ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + assert ok is True + 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_partial_download_debris_is_replaced_by_old_version(self, store): + """A failed install that left a partial directory must still roll back.""" + mgr, plugin_dir = store + + def fake_partial_install(plugin_id): + plugin_dir.mkdir(exist_ok=True) + (plugin_dir / "half-downloaded.tmp").write_text("junk") + return False + + with patch.object(mgr, "install_plugin", side_effect=fake_partial_install): + ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + assert ok is False + assert "old version marker" in (plugin_dir / "manager.py").read_text() + assert not (plugin_dir / "half-downloaded.tmp").exists() + + def test_stale_aside_from_previous_crash_is_cleared(self, store): + mgr, plugin_dir = store + 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) 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 + (relies on the existing '.standalone-backup-' exclusion).""" + mgr, plugin_dir = store + from src.plugin_system.plugin_manager import PluginManager + aside = plugin_dir.parent / f"{PLUGIN_ID}.standalone-backup-migrating" + plugin_dir.rename(aside) + pm = PluginManager(plugins_dir=str(tmp_path), config_manager=None, + display_manager=None, cache_manager=None) + found = pm._scan_directory_for_plugins(Path(tmp_path)) + assert PLUGIN_ID not in found + + +if __name__ == "__main__": + sys.exit(pytest.main([__file__, "-v"]))