diff --git a/CHANGELOG.md b/CHANGELOG.md index b9b0001a..c3a83c80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,18 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +### Fixes + +- On-demand no longer restarts a running display. `POST + /display/on-demand/start` treated `start_service` (on by default, and what + "Preview on display", the on-demand dialog and the MQTT bridge all send) as + "restart": it stopped the service, waited 1.5s and started it again, so + every request reloaded every plugin and left the panel blank for seconds. + The running display already reads the request within a quarter of a second, + mid-screen and mid-Vegas included, so the route now only starts the service + when it is not running. `POST /display/on-demand/stop` reads + `stop_service` as a boolean, so `"false"` no longer stops the service. + ## 3.7.0 Sports consolidation stage 3 (#672). No behaviour change: nothing in core diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 52c82201..b0784e10 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -52,8 +52,10 @@ each other. They share three things: | Preview viewer marker | `/tmp/led_matrix_preview_viewer` | web, while a preview is open | display: writes full-rate snapshots only while it is fresh | | Hardware init status | `/tmp/led_matrix_hw_status.json` | display | web: `/api/v3/hardware/status` | -The on-demand start route also restarts `ledmatrix.service` by default so the -request takes effect straight away. +The on-demand start route starts `ledmatrix.service` when it is not running +(`start_service`, on by default) but never restarts a running one: the display +reads the mailbox every `ON_DEMAND_POLL_INTERVAL` (0.25s), from its dwell +sleep, its render loops and Vegas's interrupt check as well as the main loop. ## Display loop diff --git a/docs/REST_API_REFERENCE.md b/docs/REST_API_REFERENCE.md index 7b64043b..34468c93 100644 --- a/docs/REST_API_REFERENCE.md +++ b/docs/REST_API_REFERENCE.md @@ -390,7 +390,7 @@ Request a specific plugin to display on-demand. - `mode` (string, optional): Display mode name (plugin_id inferred if not provided) - `duration` (number, optional): Duration in seconds (0 = until stopped) - `pinned` (boolean, optional): Pin display (pause rotation) -- `start_service` (boolean, optional): (Re)start the display service so it picks the request up (default: true) +- `start_service` (boolean, optional): Start the display service if it is not running (default: true). A running service is never restarted: it picks the request up within about a quarter of a second. When false and the service is stopped, the route returns 400. **Response**: ```json diff --git a/test/test_api_v3_on_demand_restart.py b/test/test_api_v3_on_demand_restart.py index 83f87834..df6c810b 100644 --- a/test/test_api_v3_on_demand_restart.py +++ b/test/test_api_v3_on_demand_restart.py @@ -1,25 +1,27 @@ -"""Regression test: POST /display/on-demand/start restarting a running -service must not import a name that does not exist. +"""POST /display/on-demand/start and /stop must not restart a running display. -display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads -mutable, test-patched attributes back through it (`_pkg.time.time()`, -`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per -the package's own docstring. One spot went further and wrote a genuine -`import` *statement* against that alias -- +The start route used to treat ``start_service`` (default True, and what both +the web UI and the MQTT bridge send) as "restart": with the service running it +ran ``systemctl stop``, slept 1.5s and started it again. Every on-demand or +"Preview on display" click therefore cold-restarted the display process -- +every plugin reloaded, the panel blank for seconds -- to deliver a request the +running process polls for every ON_DEMAND_POLL_INTERVAL anyway (see +test_on_demand_mailbox.py and test_display_pending_changes.py for the display +side: the mailbox is read mid-dwell, mid-screen and mid-Vegas-iteration). - import _pkg.time as time_module +The restart did not buy anything either: a freshly started display restores +only the on-demand session it saved itself (``display_on_demand_config``), so +the new request reached it through the same mailbox, one cold start later. --- but `_pkg` is a local name bound by `import ... as _pkg` in this module, -not a real top-level package, so `import _pkg.time` is not something Python -can resolve; it raises ModuleNotFoundError. That line only runs when the -display service is already running and the caller also asked to (re)start -it, so this endpoint failed on exactly the restart path -- the one where a -cache write recording the new on-demand request had already happened. +This file previously pinned that restart path (it guarded a broken +``import _pkg.time`` inside it). The path is gone; these tests pin its +replacement: a running service is left alone, a stopped one is started (only +when start_service is set), and the request lands in the mailbox either way. -The route wraps its body in `except Exception`, so the failure reached the -caller as a handled 500 with a generic message, not an unhandled crash -- -but a 500 all the same on a request that should have restarted the service -and reported success. +The service helpers are patched where they run. display.py binds +_get_display_service_status by value, while _ensure_display_service_running +(in the package __init__) looks it up in its own module, so both are patched; +_run_systemctl_command is the one place a systemctl command is issued. """ import sys @@ -32,60 +34,137 @@ sys.path.insert(0, str(Path(__file__).parent.parent)) from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 -URL = "/api/v3/display/on-demand/start" +START_URL = "/api/v3/display/on-demand/start" +STOP_URL = "/api/v3/display/on-demand/stop" +MAILBOX = "display_on_demand_request" @pytest.fixture -def restart_path(api_v3_module): - """Force the `service_was_running and start_service` branch. +def service(api_v3_module): + """A display service whose state the test sets; records systemctl calls. - plugin_manager and config_manager are set to None so the route takes - the simplest path to that branch rather than tripping over unrelated - MagicMock plumbing. The cache is the blueprint's cache_manager, which - api_v3_module already set to a MagicMock. _get_display_service_status, - _stop_display_service and _ensure_display_service_running are bound by - value in display.py (see its own docstring), so they are patched on - that submodule rather than on the package. + plugin_manager and config_manager are None so the route skips plugin + resolution (not what is under test here). The cache is the blueprint's + MagicMock cache_manager, so mailbox writes are visible as set() calls. """ api_v3_module.api_v3.plugin_manager = None api_v3_module.api_v3.config_manager = None + state = {"active": True} - with patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \ - patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \ - patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running: - # Active before the request: service_was_running becomes True. - get_status.return_value = {"active": True} - ensure_running.return_value = {"active": True} + def status(): + return {"active": state["active"]} + + def systemctl(args): + if args[-2:] == ["start", "ledmatrix.service"]: + state["active"] = True + elif args[-2:] == ["stop", "ledmatrix.service"]: + state["active"] = False + return {"returncode": 0, "stdout": "", "stderr": ""} + + with patch("web_interface.blueprints.api_v3._get_display_service_status", + side_effect=status), \ + patch("web_interface.blueprints.api_v3.display._get_display_service_status", + side_effect=status), \ + patch("web_interface.blueprints.api_v3._run_systemctl_command", + side_effect=systemctl) as run_systemctl, \ + patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service: yield { - "get_status": get_status, + "state": state, + "systemctl": run_systemctl, "stop_service": stop_service, - "ensure_running": ensure_running, + "cache": api_v3_module.api_v3.cache_manager, } -class TestRestartingARunningService: - def test_it_does_not_500(self, api_v3_client, restart_path): - response = api_v3_client.post( - URL, json={"plugin_id": "weather", "start_service": True}) - body = response.get_json() - assert response.status_code == 200, body - assert body["status"] == "success", body +def _mailbox_writes(cache): + return [c.args[1] for c in cache.set.call_args_list if c.args and c.args[0] == MAILBOX] - def test_the_service_is_actually_stopped_and_restarted( - self, api_v3_client, restart_path): - api_v3_client.post( - URL, json={"plugin_id": "weather", "start_service": True}) - restart_path["stop_service"].assert_called_once() - restart_path["ensure_running"].assert_called_once() - def test_a_service_that_was_not_running_is_not_stopped_first( - self, api_v3_client, restart_path): - # The buggy import sits inside `if service_was_running and - # start_service`, so it only ever fired on the restart path -- - # this is the other side of that branch, unaffected either way, - # kept here so the branch condition itself stays covered. - restart_path["get_status"].return_value = {"active": False} - response = api_v3_client.post( - URL, json={"plugin_id": "weather", "start_service": True}) +def _systemctl_verbs(run_systemctl): + return [c.args[0][-2] for c in run_systemctl.call_args_list] + + +class TestStartWhileTheServiceIsRunning: + @pytest.mark.parametrize("body", [ + {"plugin_id": "weather"}, # "Preview on display", MQTT + {"plugin_id": "weather", "start_service": True}, # on-demand modal, box ticked + {"plugin_id": "weather", "start_service": "true"}, + ]) + def test_the_service_is_not_stopped_or_restarted(self, api_v3_client, service, body): + response = api_v3_client.post(START_URL, json=body) assert response.status_code == 200, response.get_json() - restart_path["stop_service"].assert_not_called() + assert response.get_json()["status"] == "success" + service["stop_service"].assert_not_called() + assert _systemctl_verbs(service["systemctl"]) == [], ( + "a running display service was sent a systemctl command") + + def test_the_request_is_posted_for_the_running_display(self, api_v3_client, service): + response = api_v3_client.post( + START_URL, json={"plugin_id": "weather", "mode": "weather_current", + "duration": 60, "pinned": True}) + data = response.get_json()["data"] + writes = _mailbox_writes(service["cache"]) + assert len(writes) == 1 + assert writes[0]["action"] == "start" + assert writes[0]["request_id"] == data["request_id"] + assert writes[0]["plugin_id"] == "weather" + assert writes[0]["mode"] == "weather_current" + assert writes[0]["duration"] == 60 + assert writes[0]["pinned"] is True + + def test_the_response_reports_the_service_was_not_started(self, api_v3_client, service): + data = api_v3_client.post(START_URL, json={"plugin_id": "weather"}).get_json()["data"] + assert data["service"]["active"] is True + assert data["service"]["started"] is False + + def test_it_answers_without_the_old_restart_pause(self, api_v3_client, service): + # The restart slept 1.5s; nothing here should sleep at all. + with patch("time.sleep") as sleep: + api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + sleep.assert_not_called() + + +class TestStartWhileTheServiceIsStopped: + def test_start_service_starts_it_once_and_never_stops_it(self, api_v3_client, service): + service["state"]["active"] = False + response = api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + assert response.status_code == 200, response.get_json() + assert _systemctl_verbs(service["systemctl"]) == ["start"] + service["stop_service"].assert_not_called() + # Written before the start, so the new process finds it on its first poll. + assert len(_mailbox_writes(service["cache"])) == 1 + + def test_without_start_service_it_is_left_stopped(self, api_v3_client, service): + service["state"]["active"] = False + response = api_v3_client.post( + START_URL, json={"plugin_id": "weather", "start_service": "false"}) + assert response.status_code == 400 + assert _systemctl_verbs(service["systemctl"]) == [] + + def test_a_start_that_fails_is_reported(self, api_v3_client, service): + service["state"]["active"] = False + service["systemctl"].side_effect = lambda args: { + "returncode": 1, "stdout": "", "stderr": "denied"} + response = api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + assert response.status_code == 500 + assert response.get_json()["status"] == "error" + + +class TestStop: + def test_stop_posts_a_stop_request_and_leaves_the_service_running( + self, api_v3_client, service): + response = api_v3_client.post(STOP_URL, json={}) + assert response.status_code == 200, response.get_json() + writes = _mailbox_writes(service["cache"]) + assert [w["action"] for w in writes] == ["stop"] + service["stop_service"].assert_not_called() + assert _systemctl_verbs(service["systemctl"]) == [] + + def test_a_string_false_stop_service_does_not_stop_it(self, api_v3_client, service): + # bool("false") is True: the flag was read raw and stopped the service. + api_v3_client.post(STOP_URL, json={"stop_service": "false"}) + service["stop_service"].assert_not_called() + + def test_stop_service_true_still_stops_it(self, api_v3_client, service): + api_v3_client.post(STOP_URL, json={"stop_service": True}) + service["stop_service"].assert_called_once() diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index 3de25a2f..6cb2ca4c 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -192,8 +192,9 @@ def start_on_demand_display(): resolved_plugin, ) - # Set the on-demand request in cache FIRST (before starting service) - # This ensures the request is available when the service starts/restarts + # Post the request to the mailbox the display process polls + # (DisplayController._poll_on_demand_requests). Written before any + # service start, so a freshly started display finds it on its first poll. cache = _cache_manager() request_id = data.get('request_id') or str(uuid.uuid4()) request_payload = { @@ -207,18 +208,7 @@ def start_on_demand_display(): } cache.set('display_on_demand_request', request_payload) - # Check if display service is running (or will be started) service_status = _get_display_service_status() - service_was_running = service_status.get('active', False) - - # Stop the display service first to ensure clean state when we will restart it - if service_was_running and start_service: - import time as time_module - logger.debug("Stopping display service before starting on-demand mode") - _stop_display_service() - # Wait a brief moment for the service to fully stop - time_module.sleep(1.5) - logger.debug("Display service stopped, now starting with on-demand request") if not service_status.get('active') and not start_service: return jsonify({ @@ -227,6 +217,18 @@ def start_on_demand_display(): 'service_status': service_status }), 400 + # start_service means "start it if it is not running", as the UI's + # checkbox says; _ensure_display_service_running leaves a running service + # alone. This used to stop a running service, sleep 1.5s and start it + # again, so every on-demand or "Preview on display" click -- and every + # MQTT on-demand command, which posts here with the default -- cold- + # restarted the display process: every plugin reloaded and the panel was + # blank for seconds. The restart bought nothing. The running process + # reads this mailbox every ON_DEMAND_POLL_INTERVAL (0.25s), from its + # dwell sleep, its render loops and Vegas's interrupt check as well as + # the main loop, and a restarted one got the request the same way: the + # startup path only restores a session the display itself saved + # (display_on_demand_config), so it loaded nothing it would not have had. service_result = None if start_service: service_result = _ensure_display_service_running() @@ -237,9 +239,6 @@ def start_on_demand_display(): 'message': 'Failed to start display service. Please check service logs or start it manually.', 'service_result': service_result }), 500 - - # Service was restarted (or started fresh) with on-demand request in cache - # The display controller will read the request during initialization or when it polls response_data = { 'request_id': request_id, @@ -254,10 +253,12 @@ def start_on_demand_display(): def stop_on_demand_display(): """Request the display controller to stop on-demand mode.""" data = request.get_json(silent=True) or {} - stop_service = data.get('stop_service', False) + # _coerce_to_bool: bool("false") is True, which stopped the service. + stop_service = _coerce_to_bool(data.get('stop_service', False)) - # Set the stop request in cache FIRST - # The display controller will poll this and restart without the on-demand filter + # The running display reads the stop from the mailbox within + # ON_DEMAND_POLL_INTERVAL and resumes normal rotation in place + # (_clear_on_demand); nothing is restarted. cache = _cache_manager() request_id = data.get('request_id') or str(uuid.uuid4()) request_payload = { @@ -266,10 +267,7 @@ def stop_on_demand_display(): 'timestamp': _pkg.time.time() } cache.set('display_on_demand_request', request_payload) - - # Note: The display controller's _clear_on_demand() will handle the restart - # to restore normal operation with all plugins - + service_result = None if stop_service: service_result = _stop_display_service()