fix: keep callback faults out of the frame-decode fallback

Review follow-up on the previous two commits.

Widening the control-message except tuple put the callback dispatch
inside it, so an _on_new_cycle() that raised ValueError, TypeError or
AttributeError sent a perfectly good control packet to the legacy PNG
decoder -- which reported it as an image decode error and buried the
real fault. Split the two: whether the payload parses as JSON decides
frame vs control message, a second guard covers reading the fields of an
attacker-shaped body, and the callback fires outside both. It still
cannot kill the receive thread; the loop's own handler catches it, and
now says what actually went wrong.

The logo download's temp file was a fixed "<name>.part". Two plugins
asking for the same logo at once would interleave writes into it,
publish the mixture, or delete each other's partial. mkstemp gives each
download its own name in the same directory, so os.replace stays atomic.
Its descriptor is adopted by fdopen before the request runs, since a
request that raises before the write would otherwise leak the fd --
quietly, because load_logo_with_download swallows that.

Two test fixes: the oversized-frame test replaced PIL.Image.open
process-wide, the same hazard the clock helper documents, and Ruff B007
on an unused loop variable.

Full suite: 3355 passed, coverage 54%.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

src/common/logo_helper.py

@@ -7,6 +7,7 @@ Extracted from LEDMatrix core to provide reusable functionality for plugins.
 
 import logging
 import os
+import tempfile
 from pathlib import Path
 from typing import Dict, List, Optional, Union
 
@@ -283,12 +284,22 @@ class LogoHelper:
         # Ensure directory exists with proper permissions
         ensure_directory_permissions(file_path.parent, get_assets_dir_mode())
 
-        tmp_path = file_path.with_name(file_path.name + '.part')
+        # A unique temp name, not a fixed "<name>.part": two plugins can
+        # ask for the same logo at once, and a shared name would let them
+        # interleave writes into one file, publish the mixture, or delete
+        # each other's partial. Same directory, so os.replace stays atomic.
+        fd, tmp_name = tempfile.mkstemp(
+            dir=str(file_path.parent), prefix=file_path.name + '.', suffix='.part')
+        tmp_path = Path(tmp_name)
         try:
-            with self.session.get(url, timeout=30, stream=True) as response:
-                response.raise_for_status()
-                downloaded = 0
-                with open(tmp_path, 'wb') as f:
+            # fdopen outermost so the descriptor mkstemp handed back is
+            # always adopted and closed, including when the request itself
+            # raises — load_logo_with_download swallows that, so a leak
+            # here would accumulate quietly on a URL that keeps failing.
+            with os.fdopen(fd, 'wb') as f:
+                with self.session.get(url, timeout=30, stream=True) as response:
+                    response.raise_for_status()
+                    downloaded = 0
                     for chunk in response.iter_content(chunk_size=64 * 1024):
                         if not chunk:
                             continue

src/common/sync_manager.py

@@ -495,13 +495,43 @@ class DisplaySyncManager:
                     except Exception as exc:
                         self.logger.debug("Sync: frame decode error: %s", exc)
                 else:
-                    # No magic prefix: try control-message JSON, and treat a
-                    # parse failure as a legacy (pre-magic) PNG frame. Both
-                    # wire formats are self-describing, so no size heuristic
-                    # is needed — a >512-byte control message used to be
-                    # misrouted into image decode and silently dropped.
+                    # No magic prefix. Whether the payload parses as JSON
+                    # decides between a control message and a legacy
+                    # (pre-magic) PNG frame — both wire formats are
+                    # self-describing, so no size heuristic is needed. A
+                    # >512-byte control message used to be misrouted into
+                    # image decode and silently dropped.
                     try:
                         msg = json.loads(data.decode("utf-8"))
+                    except (json.JSONDecodeError, UnicodeDecodeError):
+                        # Not JSON — try a legacy PNG frame.
+                        try:
+                            img = Image.open(io.BytesIO(data))
+                            if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H:
+                                # Same cap the TCP image path applies: decode
+                                # is deferred until load(), so check first.
+                                self.logger.debug(
+                                    "Sync: rejected oversized legacy frame %dx%d from %s",
+                                    img.width, img.height, sender_ip,
+                                )
+                                continue
+                            img.load()
+                            self._handle_received_frame(img, sender_ip)
+                        except Exception as exc:
+                            self.logger.debug("Sync: frame decode error: %s", exc)
+                        continue
+
+                    # It parsed, so it is a control message and never a
+                    # frame. Read and validate its fields under a guard —
+                    # a UDP payload is attacker-shaped, so a non-object
+                    # body makes .get() raise AttributeError and an "sx"
+                    # carrying a non-numeric x raises ValueError/TypeError
+                    # — but dispatch the callback *outside* it. Running
+                    # the callback in here would let a fault in someone
+                    # else's code read as a malformed packet and be
+                    # logged as one.
+                    fire_new_cycle = False
+                    try:
                         t = msg.get("t")
                         if t == "hello_ack":
                             self._leader_ip = sender_ip
@@ -525,36 +555,16 @@ class DisplaySyncManager:
                                     sender_ip,
                                 )
                                 self.write_status_file()
-                                if self._on_new_cycle:
-                                    self._on_new_cycle()  # build initial scroll image
+                                fire_new_cycle = True  # build initial scroll image
                         elif t == "nc":
                             # Leader started a new scroll cycle — rebuild local image
-                            if self._on_new_cycle:
-                                self._on_new_cycle()
-                    except (json.JSONDecodeError, UnicodeDecodeError, KeyError,
-                            AttributeError, TypeError, ValueError):
-                        # Not a control message — try legacy PNG frame.
-                        # The tuple is wide because a UDP payload is
-                        # attacker-shaped: valid-but-non-object JSON makes
-                        # msg.get() raise AttributeError, and an "sx" with a
-                        # non-numeric x raises ValueError/TypeError from
-                        # float(). Those must land here, not in the outer
-                        # handler, which would skip this fallback and pay
-                        # the error backoff for one malformed packet.
-                        try:
-                            img = Image.open(io.BytesIO(data))
-                            if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H:
-                                # Same cap the TCP image path applies: decode
-                                # is deferred until load(), so check first.
-                                self.logger.debug(
-                                    "Sync: rejected oversized legacy frame %dx%d from %s",
-                                    img.width, img.height, sender_ip,
-                                )
-                                continue
-                            img.load()
-                            self._handle_received_frame(img, sender_ip)
-                        except Exception as exc:
-                            self.logger.debug("Sync: frame decode error: %s", exc)
+                            fire_new_cycle = True
+                    except (KeyError, AttributeError, TypeError, ValueError) as exc:
+                        self.logger.debug("Sync: malformed control message: %s", exc)
+                        continue
+
+                    if fire_new_cycle and self._on_new_cycle:
+                        self._on_new_cycle()
 
             except socket.timeout:
                 continue

test/test_logo_helper.py

@@ -16,6 +16,7 @@ Regression coverage for two fixed bugs:
 """
 
 import logging
+import tempfile
 from pathlib import Path
 from unittest.mock import MagicMock, patch
 
@@ -290,6 +291,38 @@ class TestDownloadLogo:
         assert not path.exists()
         assert list(tmp_path.glob("*.part")) == []
 
+    def test_concurrent_downloads_do_not_share_a_temp_file(self, helper, tmp_path):
+        # Two plugins can ask for the same logo at once. A fixed
+        # "<name>.part" would let them interleave writes into one file and
+        # publish the mixture; each download gets its own temp name.
+        path = tmp_path / "PHI.png"
+        seen = []
+        real_mkstemp = tempfile.mkstemp
+
+        def record(*args, **kwargs):
+            fd, name = real_mkstemp(*args, **kwargs)
+            seen.append(name)
+            return fd, name
+
+        with patch("src.common.logo_helper.tempfile.mkstemp", side_effect=record):
+            helper.session.get = MagicMock(return_value=fake_response(png_bytes()))
+            helper._download_logo("http://x/logo.png", path)
+            helper.session.get = MagicMock(return_value=fake_response(png_bytes()))
+            helper._download_logo("http://x/logo.png", path)
+
+        assert len(seen) == 2 and seen[0] != seen[1]
+        assert path.exists()
+        assert list(tmp_path.glob("*.part")) == []  # both cleaned up
+
+    def test_request_failure_leaves_no_temp_file(self, helper, tmp_path):
+        # mkstemp creates the file up front, so an error before any bytes
+        # arrive still has something to clean up.
+        helper.session.get = MagicMock(
+            side_effect=requests.RequestException("connection reset"))
+        with pytest.raises(requests.RequestException):
+            helper._download_logo("http://x/logo.png", tmp_path / "PHI.png")
+        assert list(tmp_path.glob("*")) == []
+
     def test_non_image_response_is_deleted_and_raises(self, helper, tmp_path):
         # Regression: undecodable bytes stayed on disk, so every later
         # load_logo() call hit the corrupt file instead of re-downloading.

test/test_sync_manager.py

@@ -511,6 +511,28 @@ class TestFollowerRecvLoop:
             assert mgr.get_latest_scroll_x() is None
             sleeps.assert_not_called()
 
+    def test_callback_failure_is_not_mistaken_for_a_malformed_packet(self, monkeypatch):
+        # A payload that parses is a control message, full stop. If the
+        # callback it triggers raises one of the types the field guard
+        # catches, that fault belongs to the callback: it must not send
+        # the packet to the image decoder, which would report it as a
+        # decode error and bury the real cause. The loop still survives
+        # it — the outer handler catches it like any other fault.
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+
+        def boom():
+            raise ValueError("callback is broken")
+
+        mgr._on_new_cycle = boom
+        fake_clock(monkeypatch, sleep_fn=MagicMock())
+        self._drive(mgr, json.dumps({"t": "nc"}).encode())
+
+        logged = " | ".join(str(c) for c in mgr.logger.debug.call_args_list)
+        assert "callback is broken" in logged
+        assert "frame decode error" not in logged
+        assert "malformed control message" not in logged
+
     def test_oversized_legacy_frame_is_rejected_before_decode(self, monkeypatch):
         # The UDP path is reachable by any host on the LAN, so it caps
         # dimensions before load() just as the TCP image server does.
@@ -522,7 +544,16 @@ class TestFollowerRecvLoop:
             def load(self):
                 raise AssertionError("load() must not run past the cap")
 
-        monkeypatch.setattr(sync_manager.Image, "open", lambda *a, **kw: Huge())
+        # Rebind the module's reference rather than mutating PIL.Image
+        # itself, which would hand Huge() to every caller in the process
+        # — including daemon threads earlier tests left running. Same
+        # reasoning as fake_clock above. The other names the receive loop
+        # reads off this reference pass through to the real module.
+        monkeypatch.setattr(sync_manager, "Image", SimpleNamespace(
+            open=lambda *a, **kw: Huge(),
+            frombuffer=Image.frombuffer,
+            DecompressionBombError=Image.DecompressionBombError,
+        ))
         self._drive(mgr, b"\x89PNG not really but not JSON either")
         assert mgr.get_latest_frame() is None
 
@@ -895,7 +926,7 @@ class TestRealSocketHandshake:
         leader = follower = None
         # The free-port probe is inherently racy — the port can be taken
         # between release and rebind — so retry rather than fail on it.
-        for attempt in range(5):
+        for _attempt in range(5):
             # Probed on loopback: this only needs a port number, and the
             # manager's own bind is what has to succeed. If the port turns
             # out to be taken on another interface, the retry below covers