Merge "Ensure a single running instance of edit monitor" into main

tools/edit_monitor/daemon_manager.py

@@ -55,6 +55,7 @@ class DaemonManager:
   def start(self):
     """Writes the pidfile and starts the daemon proces."""
     try:
+      self._stop_any_existing_instance()
       self._write_pid_to_pidfile()
       self._start_daemon_process()
     except Exception as e:
@@ -71,6 +72,22 @@ class DaemonManager:
     except Exception as e:
       logging.exception("Failed to stop daemon manager with error %s", e)
 
+  def _stop_any_existing_instance(self):
+    if not self.pid_file_path.exists():
+      logging.debug("No existing instances.")
+      return
+
+    ex_pid = self._read_pid_from_pidfile()
+
+    if ex_pid:
+      logging.info("Found another instance with pid %d.", ex_pid)
+      self._terminate_process(ex_pid)
+      self._remove_pidfile()
+
+  def _read_pid_from_pidfile(self):
+    with open(self.pid_file_path, "r") as f:
+      return int(f.read().strip())
+
   def _write_pid_to_pidfile(self):
     """Creates a pidfile and writes the current pid to the file.
 

tools/edit_monitor/daemon_manager_test.py

@@ -33,7 +33,7 @@ TEST_PID_FILE_PATH = (
 )
 
 
-def example_daemon(output_file):
+def simple_daemon(output_file):
   with open(output_file, 'w') as f:
     f.write('running daemon target')
 
@@ -69,29 +69,62 @@ class DaemonManagerTest(unittest.TestCase):
     tempfile.tempdir = self.original_tempdir
     super().tearDown()
 
-  def test_start_success(self):
+  def test_start_success_with_no_existing_instance(self):
-    damone_output_file = tempfile.NamedTemporaryFile(
+    self.assert_run_simple_daemon_success()
-        dir=self.working_dir.name, delete=False
+
+  def test_start_success_with_existing_instance_running(self):
+    # Create a long running subprocess
+    p = multiprocessing.Process(target=long_running_daemon)
+    p.start()
+
+    # Create a pidfile with the subprocess pid
+    pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath(
+        'edit_monitor'
     )
-    dm = daemon_manager.DaemonManager(
+    pid_file_path_dir.mkdir(parents=True, exist_ok=True)
-        TEST_BINARY_FILE,
+    with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f:
-        daemon_target=example_daemon,
+      f.write(str(p.pid))
-        daemon_args=(damone_output_file.name,),
+
+    self.assert_run_simple_daemon_success()
+    p.terminate()
+
+  def test_start_success_with_existing_instance_already_dead(self):
+    # Create a pidfile with pid that does not exist.
+    pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath(
+        'edit_monitor'
     )
+    pid_file_path_dir.mkdir(parents=True, exist_ok=True)
+    with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f:
+      f.write('123456')
+
+    self.assert_run_simple_daemon_success()
+
+  def test_start_success_with_existing_instance_from_different_binary(self):
+    # First start an instance based on "some_binary_path"
+    existing_dm = daemon_manager.DaemonManager(
+        "some_binary_path",
+        daemon_target=long_running_daemon,
+    )
+    existing_dm.start()
+
+    self.assert_run_simple_daemon_success()
+    existing_dm.stop()
+
+  @mock.patch('os.kill')
+  def test_start_failed_to_kill_existing_instance(self, mock_kill):
+    mock_kill.side_effect = OSError('Unknown OSError')
+    pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath(
+        'edit_monitor'
+    )
+    pid_file_path_dir.mkdir(parents=True, exist_ok=True)
+    with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f:
+      f.write('123456')
+
+    dm = daemon_manager.DaemonManager(TEST_BINARY_FILE)
     dm.start()
-    dm.daemon_process.join()
 
-    # Verifies the expected pid file is created.
+    # Verify no daemon process is started.
-    expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath(
+    self.assertIsNone(dm.daemon_process)
-        'edit_monitor', TEST_PID_FILE_PATH
-    )
-    self.assertEqual(dm.pid_file_path, expected_pid_file_path)
-    self.assertTrue(expected_pid_file_path.exists())
-
-    # Verifies the daemon process is executed successfully.
-    with open(damone_output_file.name, 'r') as f:
-      contents = f.read()
-      self.assertEqual(contents, 'running daemon target')
 
   def test_start_failed_to_write_pidfile(self):
     pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath(
@@ -151,6 +184,29 @@ class DaemonManagerTest(unittest.TestCase):
     self.assert_no_subprocess_running()
     self.assertTrue(dm.pid_file_path.exists())
 
+  def assert_run_simple_daemon_success(self):
+    damone_output_file = tempfile.NamedTemporaryFile(
+        dir=self.working_dir.name, delete=False
+    )
+    dm = daemon_manager.DaemonManager(
+        TEST_BINARY_FILE,
+        daemon_target=simple_daemon,
+        daemon_args=(damone_output_file.name,),
+    )
+    dm.start()
+    dm.daemon_process.join()
+
+    # Verifies the expected pid file is created.
+    expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath(
+        'edit_monitor', TEST_PID_FILE_PATH
+    )
+    self.assertTrue(expected_pid_file_path.exists())
+
+    # Verify the daemon process is executed successfully.
+    with open(damone_output_file.name, 'r') as f:
+      contents = f.read()
+      self.assertEqual(contents, 'running daemon target')
+
   def assert_no_subprocess_running(self):
     child_pids = self._get_child_processes(os.getpid())
     for child_pid in child_pids: