fix(store): serialize concurrent updates per plugin, check cleanup results

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/common/snapshot_policy.py

@@ -1,68 +0,0 @@
-"""Snapshot write policy for the display preview mirror.
-
-The display service mirrors frames to /tmp/led_matrix_preview.png, which
-serves two consumers with different needs:
-
-- The web UI's live preview (SSE reader in web_interface/app.py) wants
-  fresh frames — but only while a browser is actually watching.
-- The health check (web_interface/blueprints/api_v3.py, hardware status)
-  uses the file's AGE as a liveness proxy: age >= 60s reads as degraded.
-
-PNG-encoding every frame at 5 fps forever — identical frames, no viewers —
-was one of the biggest fixed CPU costs on the Pi. This module is the pure
-decision logic (extracted so it's unit-testable off-Pi; display_manager
-imports rgbmatrix unconditionally and can't be):
-
-    WRITE  — encode + atomically replace the snapshot file
-    TOUCH  — os.utime only: keeps the health-check mtime fresh and lets
-             the SSE reader (mtime-gated) resend at a low rate, without
-             paying for a PNG encode of an unchanged frame
-    SKIP   — do nothing
-
-Policy:
-- With a fresh viewer marker: changed frames write at up to 1/VIEWER_INTERVAL.
-- Without viewers: changed frames still write at 1/IDLE_INTERVAL so the
-  preview page shows something recent on open.
-- Unchanged frames are never re-encoded; the mtime is touched every
-  TOUCH_INTERVAL so the health check (60s threshold) never degrades.
-
-If any constant here changes, re-check the health threshold in
-api_v3.py (get_hardware_status) — TOUCH_INTERVAL must stay well under it.
-"""
-
-from enum import Enum
-
-# Snapshot cadence with a browser preview open (seconds).
-VIEWER_INTERVAL = 0.2
-# Snapshot cadence with no viewers — cheap freshness for page-open (seconds).
-IDLE_INTERVAL = 30.0
-# Max age of the last write/touch before bumping mtime for the health
-# check. MUST stay well under api_v3's 60s degraded threshold.
-TOUCH_INTERVAL = 20.0
-# A viewer marker older than this no longer counts as a live viewer.
-VIEWER_MARKER_FRESH_SEC = 5.0
-
-
-class SnapshotAction(Enum):
-    WRITE = "write"
-    TOUCH = "touch"
-    SKIP = "skip"
-
-
-def decide(now: float, last_write_ts: float, last_touch_ts: float,
-           viewer_fresh: bool, frame_changed: bool) -> SnapshotAction:
-    """Decide what to do with the current frame.
-
-    Args:
-        now: current monotonic-ish timestamp (same clock as the ts args)
-        last_write_ts: when a frame was last actually encoded+written
-        last_touch_ts: when the file mtime was last bumped (write or touch)
-        viewer_fresh: a browser preview is currently watching
-        frame_changed: the frame differs from the last WRITTEN frame
-    """
-    interval = VIEWER_INTERVAL if viewer_fresh else IDLE_INTERVAL
-    if frame_changed and (now - last_write_ts) >= interval:
-        return SnapshotAction.WRITE
-    if (now - max(last_write_ts, last_touch_ts)) >= TOUCH_INTERVAL:
-        return SnapshotAction.TOUCH
-    return SnapshotAction.SKIP

src/display_manager.py

@@ -31,23 +31,13 @@ if os.getenv("EMULATOR", "false") == "true":
 else:
     from rgbmatrix import RGBMatrix, RGBMatrixOptions
 from contextlib import contextmanager
-from pathlib import Path
 from PIL import Image, ImageDraw, ImageFont
 import time
 from typing import Dict, Any, List, Optional
 import logging
 import math
-import zlib
 import freetype
 
-from src.common import snapshot_policy
-from src.common.permission_utils import (
-    ensure_directory_permissions,
-    ensure_file_permissions,
-    get_assets_dir_mode,
-    get_assets_file_mode,
-)
-
 # Get logger without configuring
 logger = logging.getLogger(__name__)
 logger.setLevel(logging.INFO)  # Set to INFO level
@@ -194,19 +184,10 @@ class DisplayManager:
         # Avoids re-measuring the same string+font on every display() call.
         # Cleared on _load_fonts() so stale entries don't survive a font reload.
         self._text_width_cache: Dict[tuple, int] = {}
-        # Snapshot mirror for web preview + health check (service writes, web
+        # Snapshot settings for web preview integration (service writes, web reads)
-        # reads). Cadence/skip decisions live in src/common/snapshot_policy.py:
-        # full rate only while the web SSE broadcaster keeps the viewer marker
-        # fresh; unchanged frames are never re-encoded, only mtime-touched.
         self._snapshot_path = "/tmp/led_matrix_preview.png"  # nosec B108 - fixed path intentional; web UI reads same path
-        self._viewer_marker_path = "/tmp/led_matrix_preview_viewer"  # nosec B108 - touched by web SSE broadcaster
+        self._snapshot_min_interval_sec = 0.2  # max ~5 fps
         self._last_snapshot_ts = 0.0
-        self._last_snapshot_touch_ts = 0.0
-        self._last_snapshot_digest: Optional[int] = None
-        self._snapshot_dir_prepared = False
-        self._viewer_check_ts = 0.0
-        self._viewer_fresh = False
-        self._viewer_was_fresh = False
         
         # Scrolling state tracking for graceful updates
         self._scrolling_state = {
@@ -1147,56 +1128,27 @@ class DisplayManager:
             'deferred_update_ttl': self._scrolling_state['deferred_update_ttl']
         }
 
-    def _viewer_is_fresh(self, now: float) -> bool:
-        """True when a browser preview is watching (marker file touched by
-        the web SSE broadcaster). The marker is stat'd at most once per
-        second — at 125 fps loops a per-call stat would be pure overhead."""
-        if (now - self._viewer_check_ts) >= 1.0:
-            self._viewer_check_ts = now
-            try:
-                marker_age = now - os.stat(self._viewer_marker_path).st_mtime
-                self._viewer_fresh = marker_age < snapshot_policy.VIEWER_MARKER_FRESH_SEC
-            except OSError:
-                self._viewer_fresh = False
-        return self._viewer_fresh
-
     def _write_snapshot_if_due(self) -> None:
-        """Mirror the current frame to the preview snapshot when the policy
+        """Write the current image to a PNG snapshot file at a limited frequency."""
-        says it's worth it — see src/common/snapshot_policy.py. Unchanged
-        frames are never re-encoded; without viewers the cadence drops to
-        the idle keepalive."""
         try:
             now = time.time()
-            viewer_fresh = self._viewer_is_fresh(now)
+            if (now - self._last_snapshot_ts) < self._snapshot_min_interval_sec:
-            if viewer_fresh and not self._viewer_was_fresh:
-                # A preview just opened: let the next changed frame through
-                # immediately instead of waiting out the idle interval.
-                self._last_snapshot_ts = 0.0
-            self._viewer_was_fresh = viewer_fresh
-
-            digest = zlib.adler32(self.image.tobytes())
-            action = snapshot_policy.decide(
-                now, self._last_snapshot_ts, self._last_snapshot_touch_ts,
-                viewer_fresh, digest != self._last_snapshot_digest)
-            if action is snapshot_policy.SnapshotAction.SKIP:
                 return
-            if action is snapshot_policy.SnapshotAction.TOUCH:
+            # Ensure directory exists with proper permissions
-                # mtime bump only: keeps the health check (snapshot age)
+            from pathlib import Path
-                # green without paying for a PNG encode of an unchanged frame
+            from src.common.permission_utils import (
-                os.utime(self._snapshot_path, None)
+                ensure_directory_permissions,
-                self._last_snapshot_touch_ts = now
+                ensure_file_permissions,
-                return
+                get_assets_dir_mode,
-
+                get_assets_file_mode
-            # WRITE: ensure directory permissions once, not per frame
+            )
             snapshot_path_obj = Path(self._snapshot_path)
-            if not self._snapshot_dir_prepared:
+            # Only ensure permissions on non-system directories
-                # Never modify /tmp permissions - it has special system
+            # Never modify /tmp permissions - it has special system permissions (1777)
-                # permissions (1777) that must not be changed or it breaks
+            # that must not be changed or it breaks apt and other system tools
-                # apt and other system tools
+            parent_dir = snapshot_path_obj.parent
-                parent_dir = snapshot_path_obj.parent
+            if parent_dir and str(parent_dir) != '/tmp':  # nosec B108 - guard to skip /tmp for permission ops
-                if parent_dir and str(parent_dir) != '/tmp':  # nosec B108 - guard to skip /tmp for permission ops
+                ensure_directory_permissions(parent_dir, get_assets_dir_mode())
-                    ensure_directory_permissions(parent_dir, get_assets_dir_mode())
-                self._snapshot_dir_prepared = True
             # Write atomically: temp then replace
             tmp_path = f"{self._snapshot_path}.tmp"
             self.image.save(tmp_path, format='PNG')
@@ -1211,8 +1163,6 @@ class DisplayManager:
             except Exception:
                 pass
             self._last_snapshot_ts = now
-            self._last_snapshot_touch_ts = now
-            self._last_snapshot_digest = digest
         except Exception as e:
             # Snapshot failures should never break display; log at debug to avoid noise
             logger.debug(f"Snapshot write skipped: {e}")

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
@@ -2263,6 +2282,74 @@ class PluginStoreManager:
             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 @@ class PluginStoreManager:
                             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):
+                        return self._reinstall_with_rollback(resolved_id, plugin_path)
-                            self.logger.error(f"Failed to remove old plugin directory for {resolved_id}")
-                            return False
-                        return self.install_plugin(resolved_id)
 
                     # Check if already up to date
                     if remote_sha and local_sha and remote_sha.startswith(local_sha):
@@ -2632,11 +2716,11 @@ class PluginStoreManager:
             # 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
+            # Reinstall with the old version kept aside until the new
-            if not self._safe_remove_directory(plugin_path):
+            # download succeeds — this is the path every routine store
-                self.logger.error(f"Failed to remove old plugin directory for {plugin_id}")
+            # update takes, and a mid-update network failure must not
-                return False
+            # destroy the user's plugin.
-            return self.install_plugin(registry_id)
+            return self._reinstall_with_rollback(registry_id, plugin_path)
 
         except Exception as e:
             import traceback

test/test_snapshot_policy.py

@@ -1,93 +0,0 @@
-"""Tests for the snapshot write policy (src/common/snapshot_policy.py).
-
-The invariants that matter:
-- unchanged frames are NEVER re-encoded (the old code PNG-encoded identical
-  frames at 5 fps, 24/7)
-- the file mtime never goes stale enough to trip the health check's 60s
-  degraded threshold (api_v3 get_hardware_status)
-- a viewer gets full cadence; no viewer drops to the idle keepalive
-"""
-
-import os
-import sys
-
-import pytest
-
-sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
-
-from src.common.snapshot_policy import (  # noqa: E402
-    IDLE_INTERVAL,
-    TOUCH_INTERVAL,
-    VIEWER_INTERVAL,
-    SnapshotAction,
-    decide,
-)
-
-
-class TestViewerCadence:
-    def test_changed_frame_with_viewer_writes_at_full_rate(self):
-        assert decide(now=100.0, last_write_ts=100.0 - VIEWER_INTERVAL,
-                      last_touch_ts=0, viewer_fresh=True,
-                      frame_changed=True) is SnapshotAction.WRITE
-
-    def test_changed_frame_with_viewer_respects_min_interval(self):
-        assert decide(now=100.0, last_write_ts=100.0 - VIEWER_INTERVAL / 2,
-                      last_touch_ts=100.0, viewer_fresh=True,
-                      frame_changed=True) is SnapshotAction.SKIP
-
-    def test_unchanged_frame_with_viewer_never_writes(self):
-        """A static screen with a viewer must not burn PNG encodes."""
-        assert decide(now=100.0, last_write_ts=90.0, last_touch_ts=90.0,
-                      viewer_fresh=True,
-                      frame_changed=False) is SnapshotAction.SKIP
-
-
-class TestIdleCadence:
-    def test_changed_frame_without_viewer_waits_for_idle_interval(self):
-        assert decide(now=100.0, last_write_ts=100.0 - IDLE_INTERVAL / 2,
-                      last_touch_ts=100.0, viewer_fresh=False,
-                      frame_changed=True) is SnapshotAction.SKIP
-
-    def test_changed_frame_without_viewer_writes_at_idle_rate(self):
-        assert decide(now=100.0, last_write_ts=100.0 - IDLE_INTERVAL,
-                      last_touch_ts=0, viewer_fresh=False,
-                      frame_changed=True) is SnapshotAction.WRITE
-
-
-class TestHealthKeepalive:
-    def test_stale_mtime_gets_touched(self):
-        """Whatever else happens, mtime must be bumped within TOUCH_INTERVAL
-        so the health check (60s threshold) never reads the display as dead."""
-        assert decide(now=100.0, last_write_ts=100.0 - TOUCH_INTERVAL,
-                      last_touch_ts=100.0 - TOUCH_INTERVAL, viewer_fresh=False,
-                      frame_changed=False) is SnapshotAction.TOUCH
-
-    def test_touch_applies_with_viewer_too(self):
-        """Viewer watching a static screen: no writes, but health stays green."""
-        assert decide(now=100.0, last_write_ts=100.0 - TOUCH_INTERVAL - 1,
-                      last_touch_ts=100.0 - TOUCH_INTERVAL - 1, viewer_fresh=True,
-                      frame_changed=False) is SnapshotAction.TOUCH
-
-    def test_recent_touch_suppresses_another(self):
-        assert decide(now=100.0, last_write_ts=0.0,
-                      last_touch_ts=100.0 - TOUCH_INTERVAL / 2, viewer_fresh=False,
-                      frame_changed=False) is SnapshotAction.SKIP
-
-    def test_touch_interval_stays_under_health_threshold(self):
-        """api_v3's hardware status treats snapshot age >= 60s as degraded.
-        Keep a 2x margin so scheduling jitter can't trip it."""
-        assert TOUCH_INTERVAL <= 30
-
-    def test_worst_case_mtime_age_is_bounded(self):
-        """Simulate any interleaving: from any state, within one policy call
-        after TOUCH_INTERVAL elapses, mtime gets refreshed (WRITE or TOUCH)."""
-        for viewer in (True, False):
-            for changed in (True, False):
-                action = decide(now=1000.0, last_write_ts=900.0,
-                                last_touch_ts=900.0, viewer_fresh=viewer,
-                                frame_changed=changed)
-                assert action in (SnapshotAction.WRITE, SnapshotAction.TOUCH)
-
-
-if __name__ == "__main__":
-    sys.exit(pytest.main([__file__, "-v"]))

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"]))

web_interface/app.py

@@ -610,22 +610,8 @@ def display_preview_generator():
     import io
     
     snapshot_path = "/tmp/led_matrix_preview.png"  # nosec B108 - fixed path matches display_manager; only read here
-    # Viewer marker: this generator only runs while the broadcaster has
-    # subscribers (it exits with no clients), so touching the marker each
-    # loop tells the DISPLAY service a browser is actually watching — it
-    # only pays for full-rate PNG snapshot encodes while this stays fresh
-    # (see src/common/snapshot_policy.py).
-    viewer_marker_path = "/tmp/led_matrix_preview_viewer"  # nosec B108 - fixed path matches display_manager
     last_modified = None
     
-    def _touch_viewer_marker():
-        try:
-            with open(viewer_marker_path, 'a'):
-                pass
-            os.utime(viewer_marker_path, None)
-        except OSError:
-            pass  # display side treats a missing marker as "no viewer"
-    
     # Get display dimensions from config
     try:
         main_config = config_manager.load_config()
@@ -641,7 +627,6 @@ def display_preview_generator():
     
     while True:
         try:
-            _touch_viewer_marker()
             # Check if snapshot file exists and has been modified
             if os.path.exists(snapshot_path):
                 current_modified = os.path.getmtime(snapshot_path)