Merge pull request #4856 from oliver-sanders/4855

contact: process check should handle dirty JSON

cylc/flow/workflow_files.py

@@ -21,7 +21,6 @@
 from enum import Enum
 from functools import partial
 import glob
-import json
 import logging
 import os
 from pathlib import Path
@@ -50,6 +49,7 @@
     WorkflowFilesError,
     handle_rmtree_err,
 )
+from cylc.flow.loggingutil import CylcLogFormatter, close_log
 from cylc.flow.pathutil import (
     expand_path,
     get_cylc_run_dir,
@@ -76,11 +76,11 @@
     _construct_ssh_cmd,
     construct_ssh_cmd,
 )
-from cylc.flow.workflow_db_mgr import WorkflowDatabaseManager
-from cylc.flow.loggingutil import CylcLogFormatter, close_log
+from cylc.flow.terminal import parse_dirty_json
 from cylc.flow.unicode_rules import WorkflowNameValidator
 from cylc.flow.util import cli_format
 from cylc.flow.wallclock import get_current_time_string
+from cylc.flow.workflow_db_mgr import WorkflowDatabaseManager
 
 if TYPE_CHECKING:
     from optparse import Values
@@ -435,17 +435,27 @@ def _is_process_running(
         # because the process does not exist
         return False
 
+    error = False
     if proc.returncode:
         # the psutil call failed in some other way e.g. network issues
         LOG.debug(
             f'$ {cli_format(cmd)}  # returned {proc.returncode}\n{err}'
         )
+        error = True
+    else:
+        try:
+            process = parse_dirty_json(out)[0]
+        except ValueError:
+            # the JSON cannot be parsed, log it
+            LOG.warning(f'Could not parse JSON:\n{out}')
+            error = True
+
+    if error:
         raise CylcError(
             f'Cannot determine whether workflow is running on {host}.'
             f'\n{command}'
         )
 
-    process = json.loads(out)[0]
     return cli_format(process['cmdline']) == command
 
 

tests/integration/test_workflow_files.py

@@ -17,6 +17,7 @@
 from itertools import product
 import logging
 from pathlib import Path
+from textwrap import dedent
 
 import pytest
 
@@ -28,6 +29,7 @@
 from cylc.flow.workflow_files import (
     ContactFileFields as CFF,
     WorkflowFiles,
+    _is_process_running,
     detect_old_contact_file,
     dump_contact_file,
     load_contact_file,
@@ -203,7 +205,7 @@ def test_detect_old_contact_file_removal_errors(
 
     """
     # patch the is_process_running method
-    def _is_process_running(*args):
+    def mocked_is_process_running(*args):
         nonlocal workflow
         nonlocal process_running
         if not contact_present_after:
@@ -213,7 +215,7 @@ def _is_process_running(*args):
 
     monkeypatch.setattr(
         'cylc.flow.workflow_files._is_process_running',
-        _is_process_running,
+        mocked_is_process_running,
     )
 
     # patch the contact file removal
@@ -265,3 +267,48 @@ def _unlink(*args):
             '\nmocked-os-error'
         ),
     )) is remove_failed
+
+
+def test_is_process_running_dirty_output(monkeypatch, caplog):
+    """Ensure _is_process_running can handle polluted output.
+
+    E.G. this can happen if there is an echo statement in the `.bashrc`.
+    """
+
+    stdout = None
+
+    class Popen():
+
+        def __init__(self, *args, **kwargs):
+            self.returncode = 0
+
+        def communicate(self, *args, **kwargs):
+            nonlocal stdout
+            return (stdout, '')
+
+    monkeypatch.setattr(
+        'cylc.flow.workflow_files.Popen',
+        Popen,
+    )
+
+    # respond with something Cylc should be able to make sense of
+    stdout = dedent('''
+        % simulated stdout pollution %
+        [{
+            "cmdline": ["expected", "command"]
+        }]
+    ''')
+
+    caplog.set_level(logging.WARN, logger=CYLC_LOG)
+    assert _is_process_running('localhost', 1, 'expected command')
+    assert not caplog.record_tuples
+    assert not _is_process_running('localhost', 1, 'slartibartfast')
+    assert not caplog.record_tuples
+
+    # respond with something totally non-sensical
+    stdout = 'sir henry'
+    with pytest.raises(CylcError):
+        _is_process_running('localhost', 1, 'expected command')
+
+    # the command output should be in the debug message
+    assert 'sir henry' in caplog.records[0].message