mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user