diff --git a/CHANGELOG.md b/CHANGELOG.md index 77ae8b1b..2eb610a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -557,6 +557,16 @@ policies are unchanged. turning a category on or off in of-the-day always failed. The params now reach the wrapper on its stdin; the script still receives them as JSON on its own stdin, as before. +- An on-demand request that `/api/v3/display/on-demand/start` refuses no + longer runs later. The route posted the request to the display's mailbox + before checking the service, and the display reads that mailbox for an + hour without looking at a request's age. So with "Start display service" + unticked and the display stopped, the answer was "Display service is not + running", yet the next time the display was started it ran that plugin, + pinned if the request said so. The same happened after "Failed to start + display service". The route now checks the service first and posts + nothing when it refuses, and a request it posted before a failed start is + taken back out of the mailbox, unless a newer one has replaced it. - The display schedule turns the panel off at exactly the end time. A window now runs from its start time up to, but not including, its end time: with 07:00-23:00 the panel is on at 07:00 and off at 23:00. Before, the end diff --git a/test/test_api_v3_on_demand_restart.py b/test/test_api_v3_on_demand_restart.py index 17d31edd..c6482133 100644 --- a/test/test_api_v3_on_demand_restart.py +++ b/test/test_api_v3_on_demand_restart.py @@ -150,6 +150,66 @@ class TestStartWhileTheServiceIsStopped: assert response.get_json()["status"] == "error" +class _Mailbox: + """The CacheManager calls the routes make, over a dict.""" + + def __init__(self): + self.entries = {} + + def set(self, key, value, ttl=None): + self.entries[key] = value + + def get(self, key, max_age=300, memory_ttl=None): + return self.entries.get(key) + + def delete(self, key): + self.entries.pop(key, None) + + +class TestARefusedStartLeavesNoRequestBehind: + """A start the route answers with an error must not run later. + + The request used to be posted before the service was checked, and the + display reads the mailbox for an hour without looking at a request's + age. So "Display service is not running" (start_service off) or "Failed + to start display service" left the request waiting, and the next time + the display started -- minutes later, by hand -- it ran that plugin, + pinned if the request said so. + """ + + @pytest.fixture + def mailbox(self, api_v3_module, service): + box = _Mailbox() + api_v3_module.api_v3.cache_manager = box + service["state"]["active"] = False + return box + + def test_without_start_service_nothing_is_posted(self, api_v3_client, service, mailbox): + response = api_v3_client.post(START_URL, json={ + "plugin_id": "weather", "pinned": True, "start_service": False}) + assert response.status_code == 400 + assert MAILBOX not in mailbox.entries + + def test_a_start_that_fails_takes_its_request_back(self, api_v3_client, service, mailbox): + 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 MAILBOX not in mailbox.entries + + def test_a_newer_request_in_the_mailbox_is_left_alone(self, api_v3_client, service, mailbox): + newer = {"request_id": "someone-else", "action": "start", "plugin_id": "clock"} + + def start_fails_after_another_post(args): + mailbox.entries[MAILBOX] = newer + return {"returncode": 1, "stdout": "", "stderr": "denied"} + + service["systemctl"].side_effect = start_fails_after_another_post + response = api_v3_client.post(START_URL, json={"plugin_id": "weather"}) + assert response.status_code == 500 + assert mailbox.entries[MAILBOX] is newer + + class TestStop: def test_stop_posts_a_stop_request_and_leaves_the_service_running( self, api_v3_client, service): diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index cbc83946..90b9853d 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -69,6 +69,26 @@ def _deliver_on_demand(payload): return 'mailbox', reason +def _withdraw_on_demand(request_id): + """Take a start request the route has refused back out of the mailbox. + + The display reads the mailbox for an hour without looking at a + request's age, so one left there after an error answer ran whenever the + display next started. Only this request is removed: the mailbox is + re-read and cleared only while it still holds this request_id, as the + display's _consume_on_demand_request does, so a newer request posted in + the meantime stays for the display to take. + """ + cache = _cache_manager() + try: + current = cache.get('display_on_demand_request', max_age=3600, memory_ttl=0) + if isinstance(current, dict) and current.get('request_id') == request_id: + cache.delete('display_on_demand_request') + except Exception: # the route is answering an error already + logger.warning("Could not withdraw on-demand request %s from the mailbox", + request_id, exc_info=True) + + @api_v3.route('/display/current', methods=['GET']) def get_display_current(): """The latest display preview, as the /stream/display SSE stream sends it. @@ -242,6 +262,18 @@ def start_on_demand_display(): resolved_plugin, ) + # Checked before anything is delivered: a request posted and then + # refused here stayed in the mailbox and ran when the display was next + # started, long after the caller was told it had failed. + service_status = _get_display_service_status() + + if not service_status.get('active') and not start_service: + return jsonify({ + 'status': 'error', + 'message': 'Display service is not running. Please start the display service or enable "Start Service" option.', + 'service_status': service_status + }), 400 + # Deliver the request over the control socket, or post it to the # mailbox the display process polls (DisplayController. # _poll_on_demand_requests). Done before any service start: a stopped @@ -259,15 +291,6 @@ def start_on_demand_display(): } transport, socket_error = _deliver_on_demand(request_payload) - service_status = _get_display_service_status() - - if not service_status.get('active') and not start_service: - return jsonify({ - 'status': 'error', - 'message': 'Display service is not running. Please start the display service or enable "Start Service" option.', - '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 @@ -285,6 +308,8 @@ def start_on_demand_display(): service_result = _ensure_display_service_running() # Check if service actually started if service_result and not service_result.get('active'): + if transport == 'mailbox': + _withdraw_on_demand(request_id) return jsonify({ 'status': 'error', 'message': 'Failed to start display service. Please check service logs or start it manually.',