Improve error message for cylc clean remote timeout

Author: MetRonnie

cylc/flow/clean.py

@@ -384,7 +384,7 @@ def remote_clean(
                 remote_clean_cmd(platform=platforms[0]), target, platforms
             )
         )
-    failed_targets: Dict[str, PlatformError] = {}
+    failed_targets: Dict[str, Union[PlatformError, str]] = {}
     # Handle subproc pool results almost concurrently:
     while queue:
         item = queue.popleft()
@@ -395,7 +395,12 @@ def remote_clean(
         out, err = item.proc.communicate()
         if out:
             LOG.info(f"[{item.install_target}]\n{out}")
-        if ret_code:
+        if ret_code == 124:
+            failed_targets[item.install_target] = (
+                f"cylc clean timed out after {timeout}s. You can increase "
+                "this timeout using the --timeout option."
+            )
+        elif ret_code:
             this_platform = item.platforms.pop(0)
             excp = PlatformError(
                 PlatformError.MSG_TIDY,
@@ -423,9 +428,9 @@ def remote_clean(
             LOG.debug(f"[{item.install_target}]\n{err}")
         sleep(0.2)
     if failed_targets:
-        for target, excp in failed_targets.items():
+        for target, info in failed_targets.items():
             LOG.error(
-                f"Could not clean {id_} on install target: {target}\n{excp}"
+                f"Could not clean {id_} on install target: {target}\n{info}"
             )
         raise CylcError(f"Remote clean failed for {id_}")
 

tests/functional/cylc-clean/01-remote.t

@@ -25,7 +25,7 @@ SSH_CMD="$(cylc config -d -i "[platforms][${CYLC_TEST_PLATFORM}]ssh command") ${
 if ! $SSH_CMD command -v 'tree' > '/dev/null'; then
     skip_all "'tree' command not available on remote host ${CYLC_TEST_HOST}"
 fi
-set_test_number 10
+set_test_number 12
 
 # Generate random name for symlink dirs to avoid any clashes with other tests
 SYM_NAME="$(mktemp -u)"
@@ -108,7 +108,13 @@ __TREE__
 
 # -----------------------------------------------------------------------------
 
-TEST_NAME="cylc-clean"
+TEST_NAME="cylc-clean-timeout"
+run_fail "$TEST_NAME" cylc clean --timeout 'PT0,1S' "$WORKFLOW_NAME"
+dump_std "$TEST_NAME"
+grep_ok 'cylc clean timed out after 0.1s' "${TEST_NAME}.stderr"
+
+
+TEST_NAME="cylc-clean-ok"
 run_ok "$TEST_NAME" cylc clean "$WORKFLOW_NAME"
 dump_std "$TEST_NAME"
 

tests/unit/test_clean.py

@@ -970,25 +970,35 @@ def mocked_remote_clean_cmd_side_effect(id_, platform, timeout, rm_dirs):
      ('PT1M2S', '62.0')]
 )
 def test_remote_clean__timeout(
-    monkeymock: MonkeyMock, monkeypatch: pytest.MonkeyPatch,
-    timeout: str, expected: str,
+    monkeymock: MonkeyMock,
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+    timeout: str,
+    expected: str,
 ):
-    """Test that remote_clean() accepts a timeout in ISO 8601 format or
-    number of seconds."""
+    """Test remote_clean() timeout.
+
+    - It should accept ISO 8601 format or number of seconds.
+    - It should give a sensible error message for return code 124.
+    """
+    caplog.set_level(logging.ERROR, CYLC_LOG)
     mock_remote_clean_cmd = monkeymock(
         'cylc.flow.clean._remote_clean_cmd',
         spec=_remote_clean_cmd,
         return_value=mock.Mock(
-            spec=Popen, poll=lambda: 0, communicate=lambda: ('', '')
+            spec=Popen, poll=lambda: 124, communicate=lambda: ('', '')
         )
     )
     monkeypatch.setattr(
         'cylc.flow.clean.get_install_target_to_platforms_map',
         lambda *a, **k: {'picard': [PLATFORMS['stargazer']]}
     )
 
-    cylc_clean.remote_clean('blah', 'blah', timeout)
-    assert mock_remote_clean_cmd.call_args.kwargs['timeout'] == expected
+    with pytest.raises(CylcError):
+        cylc_clean.remote_clean('blah', 'blah', timeout)
+    _, kwargs = mock_remote_clean_cmd.call_args
+    assert kwargs['timeout'] == expected
+    assert f"cylc clean timed out after {expected}s" in caplog.text
 
 
 @pytest.mark.parametrize(