From cdaeb3855d61119dd62f249b9c517f507ed3c44a Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 20:52:39 -0400 Subject: [PATCH] fix(web): a refused on-demand start leaves no request in the mailbox POST /api/v3/display/on-demand/start delivered the request (control socket, else the file mailbox) before it checked the display service. With the service stopped the socket is absent, so the request went to the mailbox; the route then answered 400 "Display service is not running" when start_service was off, or 500 "Failed to start display service" when the start failed. The display reads that mailbox with max_age=3600 and never checks a request's timestamp, so the next time it was started it ran the refused request, pinned if asked. The service is now checked before anything is delivered, and nothing is posted when start_service is off and the service is down. When the start itself fails, the request is withdrawn from the mailbox, but only while the mailbox still holds this request_id (the compare-before-delete the display's _consume_on_demand_request uses), so a newer request posted in the meantime is left for the display. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 ++++ test/test_api_v3_on_demand_restart.py | 60 ++++++++++++++++++++++ web_interface/blueprints/api_v3/display.py | 43 ++++++++++++---- 3 files changed, 104 insertions(+), 9 deletions(-) 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.',