mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-03 09:48:06 +00:00
fix(store): serialize concurrent installs, and make the lock reentrant
Second bug found while validating the previous commit on hardware. install_plugin's new set-aside/restore had no lock. The web UI runs Flask threaded, so a double-clicked Install button gives two threads the same plugin_id; interleaved, one thread's restore deletes the other's freshly installed copy. _reinstall_with_rollback already guards exactly this with a per-plugin lock, and install_plugin needs the same one. Taking that lock naively deadlocks. _reinstall_with_rollback holds it across its call to install_plugin, and threading.Lock is not reentrant -- so the request thread hangs forever on the standard monorepo update path (update_plugin -> _reinstall_with_rollback -> install_plugin), which is to say on every plugin update. Verified by reverting to a plain Lock: the regression test times out after 10s instead of passing. The per-plugin locks are now RLocks, and install_plugin holds one for its whole set-aside/install/restore sequence. Verified on devpi (Pi, Python 3.13.5, real registry and network): - update_plugin on an up-to-date plugin: True in 5.4s - update_plugin forced through the full reinstall-with-rollback path: True in 13.1s, correct version restored, old copy replaced, no backup directories left behind - install -> reinstall-over-existing -> failed-reinstall-restores: all pass against real downloads - 22 plugins load, no tracebacks, web API and UI 200, steady-state journal 50 lines/min 791 core unit tests pass, including 2 new concurrency tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
co-authored by
Claude Opus 5
parent
fecd9e1385
commit
c615d5a3bb
@@ -150,3 +150,73 @@ class TestUpdatePathStillWorks:
|
||||
assert observed["dirs"] == ["hockey-scoreboard.standalone-backup-migrating"]
|
||||
# And the user still has their plugin.
|
||||
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original"
|
||||
|
||||
|
||||
class TestConcurrency:
|
||||
"""The web UI runs Flask threaded, so a double-clicked Install button puts
|
||||
two threads on the same plugin_id. `_reinstall_with_rollback` already
|
||||
guarded against this; the install wrapper has to as well, or one thread's
|
||||
restore deletes the other's freshly installed copy."""
|
||||
|
||||
def test_rollback_calling_install_does_not_deadlock(self, store, monkeypatch):
|
||||
"""The rollback path holds the per-plugin lock across its call to
|
||||
install_plugin. A non-reentrant lock would hang the request thread
|
||||
forever — this test would time out rather than fail."""
|
||||
import threading
|
||||
|
||||
mgr, plugins_dir = store
|
||||
path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
||||
monkeypatch.setattr(
|
||||
mgr, "_install_plugin_impl",
|
||||
lambda pid, branch=None: bool(_existing_install(plugins_dir, pid, "new")))
|
||||
|
||||
done = threading.Event()
|
||||
result = {}
|
||||
|
||||
def run():
|
||||
result["ok"] = mgr._reinstall_with_rollback("hockey-scoreboard", path)
|
||||
done.set()
|
||||
|
||||
t = threading.Thread(target=run, daemon=True)
|
||||
t.start()
|
||||
assert done.wait(timeout=10), (
|
||||
"install_plugin deadlocked when called from _reinstall_with_rollback "
|
||||
"— the per-plugin lock must be reentrant"
|
||||
)
|
||||
assert result["ok"] is True
|
||||
|
||||
def test_concurrent_installs_serialize(self, store, monkeypatch):
|
||||
"""Two threads installing the same plugin must not interleave their
|
||||
set-aside/restore, and the survivor must be a complete install."""
|
||||
import threading
|
||||
|
||||
mgr, plugins_dir = store
|
||||
_existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
||||
|
||||
in_flight = []
|
||||
overlap = []
|
||||
|
||||
def slow_impl(plugin_id, branch=None):
|
||||
in_flight.append(1)
|
||||
if len(in_flight) > 1:
|
||||
overlap.append(1)
|
||||
threading.Event().wait(0.05)
|
||||
_existing_install(plugins_dir, plugin_id, "installed")
|
||||
in_flight.pop()
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(mgr, "_install_plugin_impl", slow_impl)
|
||||
|
||||
threads = [threading.Thread(target=mgr.install_plugin,
|
||||
args=("hockey-scoreboard",), daemon=True)
|
||||
for _ in range(2)]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join(timeout=10)
|
||||
assert not t.is_alive(), "concurrent install hung"
|
||||
|
||||
assert not overlap, "two installs of the same plugin ran concurrently"
|
||||
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").exists()
|
||||
leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name]
|
||||
assert not leftovers, f"backup left behind: {leftovers}"
|
||||
|
||||
Reference in New Issue
Block a user