services - play: fix timeout implementation

* We were applying a timeout to the `Popen.wait` method, however, this
  doesn't actually kill the process (as one might expect) when the
  timeout elapses, it just stops waiting for it?!

Author: oliver-sanders

cylc/uiserver/resolvers.py

@@ -27,6 +27,7 @@
     DEVNULL,
     PIPE,
     Popen,
+    TimeoutExpired,
 )
 from textwrap import indent
 from time import time
@@ -306,6 +307,7 @@ async def run_command(
                 The application log, used to record this command invocation.
             timeout:
                 Length of time to wait for the command to complete.
+                The command will be killed if the timeout elapses.
             success_msg:
                 Message to be used in the response if the command succeeds.
             fail_msg:
@@ -341,7 +343,7 @@ async def run_command(
                     env.pop('CYLC_ENV_NAME', None)
                     env['CYLC_VERSION'] = cylc_version
 
-                # run cylc play
+                # run command
                 proc = Popen(
                     cmd,
                     env=env,
@@ -350,11 +352,21 @@ async def run_command(
                     stderr=PIPE,
                     text=True
                 )
-                ret_code = proc.wait(timeout=timeout)
+
+                try:
+                    ret_code = proc.wait(timeout=timeout)
+                except TimeoutExpired as exc:
+                    proc.kill()
+                    ret_code = 124  # mimic `timeout` command error code
+                    # NOTE: preserve any stderr that the command produced this
+                    # far as this may help with debugging
+                    out, err = proc.communicate()
+                    err = str(exc) + (('\n' + err) if err else '')
+                else:
+                    out, err = proc.communicate()
 
                 if ret_code:
                     msg = f"{fail_msg} ({ret_code}): {cmd_repr}"
-                    out, err = proc.communicate()
                     results[wflow] = err.strip() or out.strip() or msg
                     log.error(
                         f"{msg}\n"

cylc/uiserver/tests/test_resolvers.py

@@ -25,12 +25,6 @@
 from subprocess import Popen, TimeoutExpired
 from types import SimpleNamespace
 
-import sys
-if sys.version_info >= (3, 11):
-    from asyncio import timeout
-else:
-    from async_timeout import timeout
-
 from cylc.flow import CYLC_LOG
 from cylc.flow.exceptions import CylcError
 from cylc.flow.id import Tokens
@@ -145,6 +139,7 @@ async def test_start_services(
         return_value=Mock(
             spec=Popen,
             wait=Mock(return_value=0),
+            communicate=lambda: ('out', 'err'),
         )
     )
     monkeypatch.setattr('cylc.uiserver.resolvers.Popen', mock_popen)
@@ -260,7 +255,11 @@ def wait(timeout):
 
     mock_popen = Mock(
         spec=Popen,
-        return_value=Mock(spec=Popen, wait=wait)
+        return_value=Mock(
+            spec=Popen,
+            wait=wait,
+            communicate=lambda: ('out', 'err'),
+        ),
     )
     monkeypatch.setattr('cylc.uiserver.resolvers.Popen', mock_popen)
 
@@ -270,9 +269,9 @@ def wait(timeout):
         {},
         log=Mock(),
     )
-    assert ret == [
-        False, "Command 'cylc play wflow1' timed out after 120 seconds"
-    ]
+    assert ret == (
+        False, "Command 'cylc play wflow1' timed out after 120 seconds\nerr"
+    )
 
 
 @pytest.fixture
@@ -331,7 +330,7 @@ async def test_cat_log(workflow_run_dir, app, fast_sleep):
 
     # note - timeout tests that the cat-log process is being stopped correctly
     first_response = None
-    async with timeout(20):
+    async with asyncio.timeout(20):
         ret = services.cat_log(workflow, app, info)
         actual = ''
         is_first = True
@@ -380,7 +379,7 @@ async def test_cat_log_timeout(workflow_run_dir, app, fast_sleep):
 
     ret = services.cat_log(workflow, app, info)
     responses = []
-    async with timeout(5):
+    async with asyncio.timeout(5):
         async for response in ret:
             responses.append(response)
             await asyncio.sleep(0)