From 641296990df21a952e93b10623c5ae2cbe4322b2 Mon Sep 17 00:00:00 2001 From: Chuck Date: Sun, 12 Jul 2026 17:00:05 -0400 Subject: [PATCH] fix(store): plugin updates keep the old install until the new one succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reinstall paths in update_plugin — the monorepo-migration remote switch AND the routine archive update every store user hits — deleted the installed plugin directory BEFORE downloading its replacement. A mid-update failure (bad network, registry error) permanently destroyed the plugin. Seen in the field: a Pi with broken DNS lost 12 plugins in one update pass during the monorepo migration. New _reinstall_with_rollback: rename the old install aside (using the '.standalone-backup-' name pattern plugin discovery already excludes), run install_plugin, remove the aside on success — restore it on ANY failure, clearing partial-download debris first. A stale aside from a previous crash is cleared before starting. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam --- src/plugin_system/store_manager.py | 68 ++++++++++++++--- test/test_store_update_rollback.py | 117 +++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 test/test_store_update_rollback.py diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 059a1343..6a1583bf 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -2263,6 +2263,59 @@ 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. + """ + 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: + 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: + 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. @@ -2325,10 +2378,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 +2682,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..a649518a --- /dev/null +++ b/test/test_store_update_rollback.py @@ -0,0 +1,117 @@ +"""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 +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): + ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) + assert ok is False + assert plugin_dir.exists() + + 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"]))