From 020e3386e15e1c46a174b30f1f5de9795bb766bc Mon Sep 17 00:00:00 2001
From: saphid <4596216+saphid@users.noreply.github.com>
Date: Tue, 29 Sep 2026 09:30:03 +1000
Subject: [PATCH] Make media stop race-free and cover start failures

Stop always calls systemctl and treats exit 5 (unit already collected)
as done, so there is no is-active/stop race. Cleanup never masks the
copy error, the play ssh timeout covers the remote worst case, and
tests cover stop exit codes and systemd-run stderr reporting.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---
 tests/test_media.py      | 31 ++++++++++++++++++++++---------
 ui/frame_media_remote.py |  8 +++++---
 ui/server.py             |  4 ++--
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/tests/test_media.py b/tests/test_media.py
index 29ea4a9..76c0d6d 100644
--- a/tests/test_media.py
+++ b/tests/test_media.py
@@ -83,15 +83,28 @@ class Media(unittest.TestCase):
             ssh.assert_not_called()
 
     def test_fake_frame_stop_only_owns_our_unit(self):
-        with patch.object(remote.subprocess, 'run') as run, patch.object(remote, 'status', return_value={}), \
-                patch.object(remote, 'active', return_value=True):
-            remote.run({'action': 'stop'})
-            self.assertEqual(run.call_args.args[0], ['systemctl', '--user', 'stop', 'frame-control-media.service'])
-        # A finished player is already collected; Stop is then a no-op, not an error.
-        with patch.object(remote.subprocess, 'run') as run, patch.object(remote, 'status', return_value={}), \
-                patch.object(remote, 'active', return_value=False):
-            remote.run({'action': 'stop'})
-            run.assert_not_called()
+        for rc in (0, 5):  # 5: already collected ("not loaded"), a no-op
+            with patch.object(remote.subprocess, 'run') as run, patch.object(remote, 'status', return_value={'state': 'ended'}):
+                run.return_value.returncode = rc
+                self.assertEqual(remote.run({'action': 'stop'})['state'], 'ended')
+                self.assertEqual(run.call_args.args[0], ['systemctl', '--user', 'stop', 'frame-control-media.service'])
+        with patch.object(remote.subprocess, 'run') as run, patch.object(remote, 'status', return_value={}):
+            run.return_value.returncode, run.return_value.stderr = 1, 'Access denied'
+            with self.assertRaisesRegex(RuntimeError, 'Access denied'):
+                remote.run({'action': 'stop'})
+
+    def test_start_failure_reports_systemd_error(self):
+        with tempfile.TemporaryDirectory() as d, patch.object(remote, 'ROOT', Path(d)), \
+                patch.object(remote, 'STATUS', Path(d)/'status.json'), \
+                patch.object(remote, 'active', return_value=False), \
+                patch.object(remote.subprocess, 'run') as run:
+            identity = 'b'*32+'/still_SBS.png'
+            (Path(d)/identity).parent.mkdir()
+            (Path(d)/identity).write_bytes(b'x')
+            run.return_value.returncode, run.return_value.stderr = 1, 'Unit already exists'
+            with patch.object(remote, 'probe', return_value=({}, False)), \
+                    self.assertRaisesRegex(RuntimeError, 'Unit already exists'):
+                remote.run({'action': 'play', 'id': identity})
 
     def test_upload_rejects_unplayable_names_and_keeps_copy_error(self):
         with patch.object(server, 'ssh') as ssh, patch.object(server, 'push_file') as push:
diff --git a/ui/frame_media_remote.py b/ui/frame_media_remote.py
index b60b295..3b70bfe 100644
--- a/ui/frame_media_remote.py
+++ b/ui/frame_media_remote.py
@@ -63,9 +63,11 @@ def run(body):
     if action == 'status':
         return status()
     if action == 'stop':
-        # --collect unloads the unit after it exits; stopping it then is a no-op, not an error.
-        if active():
-            subprocess.run(['systemctl', '--user', 'stop', UNIT], check=True, timeout=15)
+        # --collect unloads the unit after it exits; systemctl then exits 5
+        # ("not loaded", verified on the Frame). That's a finished player, not an error.
+        stopped = subprocess.run(['systemctl', '--user', 'stop', UNIT], capture_output=True, text=True, timeout=15)
+        if stopped.returncode not in (0, 5):
+            raise RuntimeError('Could not stop the media player: ' + (stopped.stderr.strip() or 'exit %s' % stopped.returncode))
         return {'message': 'Media player stopped', **status()}
     if action != 'play':
         raise ValueError('Media action must be list, status, play or stop')
diff --git a/ui/server.py b/ui/server.py
index 6109d39..0f75d38 100755
--- a/ui/server.py
+++ b/ui/server.py
@@ -1262,7 +1262,7 @@ for name, source in json.load(sys.stdin).items():
         ssh("python3 -c " + shlex.quote(installer), stdin=json.dumps(sources))
         try:
             out = ssh("python3 ~/.local/share/frame-control/media/frame_media_remote.py",
-                      stdin=json.dumps(body), timeout=60)
+                      stdin=json.dumps(body), timeout=75)  # remote worst case: ffprobe 30 + reset-failed 10 + systemd-run 15 s
         except Failure as e:
             for line in reversed(getattr(e, "stdout", "").splitlines()):
                 try:
@@ -1290,7 +1290,7 @@ def push_media(path):
     except Exception:
         try:
             ssh("rm -rf ~/" + dest)
-        except Failure:
+        except Exception:
             pass  # keep the copy error; an empty folder isn't listed as media
         raise
     return {"message": "Media sent. Choose its layout and press Play.",