diff --git a/tests/check.py b/tests/check.py index 466ffc7..63bc6fb 100644 --- a/tests/check.py +++ b/tests/check.py @@ -2,6 +2,7 @@ import os from pathlib import Path import shutil +import signal import subprocess import sys import tempfile @@ -25,6 +26,46 @@ def find_chrome(): raise RuntimeError('Chrome is required for the public browser checks. Install Chrome or set CHROME_BIN to its executable.') +def stop_browser(browser, grace=10): + """Stop profile writers before TemporaryDirectory removes their files.""" + if os.name != 'posix': + browser.terminate() + try: + browser.wait(timeout=grace) + except subprocess.TimeoutExpired: + browser.kill() + browser.wait(timeout=grace) + return + + def running(): + browser.poll() # Reap the parent, even while children are still exiting. + # killpg(..., 0) also sees zombies. They cannot write to the profile, + # and orphaned children must be reaped by the OS, not by this process. + processes = subprocess.check_output(['ps', '-eo', 'pgid=,stat='], text=True) + return any(int(group) == browser.pid and not state.startswith('Z') + for group, state in (line.split() for line in processes.splitlines())) + + for sig in (signal.SIGTERM, signal.SIGKILL): + try: + os.killpg(browser.pid, sig) + except ProcessLookupError: + pass + except PermissionError: + # macOS can report EPERM for a group whose last member is exiting. + # A genuinely live, unsignalable group remains a cleanup failure. + if running(): + raise + deadline = time.monotonic() + grace + while running(): + if time.monotonic() >= deadline: + break + time.sleep(.05) + else: + browser.wait(timeout=grace) + return + raise RuntimeError('Chrome subprocesses did not exit after SIGKILL.') + + def run_browser(chrome): with tempfile.TemporaryDirectory(prefix='quickdash-check-') as temporary: directory = Path(temporary) @@ -34,7 +75,7 @@ def run_browser(chrome): chrome, '--headless', '--no-sandbox', '--disable-gpu', '--no-first-run', '--no-default-browser-check', '--remote-debugging-port=0', '--user-data-dir=' + str(profile), 'about:blank', - ], stdout=log, stderr=subprocess.STDOUT) + ], stdout=log, stderr=subprocess.STDOUT, start_new_session=os.name == 'posix') try: active_port = profile / 'DevToolsActivePort' deadline = time.monotonic() + 30 @@ -51,12 +92,7 @@ def run_browser(chrome): print(log.read(), file=sys.stderr) raise finally: - browser.terminate() - try: - browser.wait(timeout=10) - except subprocess.TimeoutExpired: - browser.kill() - browser.wait() + stop_browser(browser) def main(): diff --git a/tests/test_check.py b/tests/test_check.py index ab660ca..623cc56 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -1,6 +1,11 @@ """The pre-PR command must propagate browser failures and clean up Chrome.""" from pathlib import Path +import os +import select +import signal import subprocess +import sys +import tempfile import unittest from unittest.mock import Mock, patch from tests import check @@ -14,6 +19,7 @@ def test_browser_uses_own_port_and_cleans_up_on_success_or_failure(self): directories = [] def launch(args, **kwargs): + self.assertEqual(kwargs['start_new_session'], os.name == 'posix') profile = Path(next(a.split('=', 1)[1] for a in args if a.startswith('--user-data-dir='))) profile.mkdir() (profile / 'DevToolsActivePort').write_text('43210\n/devtools/browser/test\n') @@ -21,7 +27,7 @@ def launch(args, **kwargs): return browser failure = subprocess.CalledProcessError(1, 'node') if fails else None - with patch.object(check.subprocess, 'Popen', side_effect=launch), patch.object(check.subprocess, 'run', side_effect=failure) as run: + with patch.object(check.subprocess, 'Popen', side_effect=launch), patch.object(check.subprocess, 'run', side_effect=failure) as run, patch.object(check, 'stop_browser') as stop: if fails: with self.assertRaises(subprocess.CalledProcessError): check.run_browser('chrome') @@ -29,10 +35,104 @@ def launch(args, **kwargs): check.run_browser('chrome') self.assertEqual(run.call_args.kwargs['env']['QUICKDASH_CHROME_PORT'], '43210') self.assertTrue(run.call_args.kwargs['check']) - browser.terminate.assert_called_once() - browser.wait.assert_called_once_with(timeout=10) + stop.assert_called_once_with(browser) self.assertFalse(directories[0].exists()) + @unittest.skipUnless(os.name == 'posix', 'POSIX process-group lifecycle') + def test_cleanup_stops_a_profile_writer_even_after_browser_parent_exits(self): + for parent_exits in (False, True): + for ignores_term in (False, True): + with self.subTest(parent_exits=parent_exits, ignores_term=ignores_term): + self.check_profile_writer_cleanup(parent_exits, ignores_term) + + def check_profile_writer_cleanup(self, parent_exits, ignores_term): + # A child keeps making files, like a lingering Chrome subprocess. + # Cover both graceful exit and escalation, including an exited parent. + writer = """ +import os, pathlib, signal, sys, time +if sys.argv[2] == 'True': + signal.signal(signal.SIGTERM, signal.SIG_IGN) +profile = pathlib.Path(sys.argv[1]) +profile.mkdir() +print(os.getpid(), flush=True) +i = 0 +while True: + (profile / str(i)).write_text('background write') + i += 1 + time.sleep(.01) +""" + launcher = """ +import subprocess, sys +child = subprocess.Popen([sys.executable, '-c', *sys.argv[1:4]]) +if sys.argv[4] == 'False': + child.wait() +""" + with tempfile.TemporaryDirectory() as temporary: + profile = Path(temporary) / 'Default' + browser = subprocess.Popen([sys.executable, '-c', launcher, writer, str(profile), + str(ignores_term), str(parent_exits)], + start_new_session=True, stdout=subprocess.PIPE, text=True) + try: + self.assertTrue(select.select([browser.stdout], [], [], 10)[0]) + child = int(browser.stdout.readline()) + if parent_exits: + browser.wait(timeout=10) + os.kill(child, 0) + self.assertTrue(profile.is_dir()) + check.stop_browser(browser, grace=1) + # EOF proves the writer exited; a zombie may remain until the + # OS reaps it, so killpg(..., 0) is not a liveness check. + self.assertTrue(select.select([browser.stdout], [], [], 10)[0]) + self.assertEqual(browser.stdout.read(), '') + self.assertIsNotNone(browser.returncode) + finally: + try: + os.killpg(browser.pid, signal.SIGKILL) + except (ProcessLookupError, PermissionError): + pass + browser.wait(timeout=10) + self.assertTrue(select.select([browser.stdout], [], [], 10)[0]) + browser.stdout.close() + self.assertFalse(profile.exists()) + + @unittest.skipUnless(os.name == 'posix', 'POSIX process-group lifecycle') + def test_cleanup_does_not_wait_for_zombies_or_other_process_groups(self): + browser = Mock(pid=123) + for result in ('', '123 Z\n124 S\n'): + with self.subTest(processes=result), patch.object(check.subprocess, 'check_output', return_value=result), patch.object(check.os, 'killpg') as kill: + check.stop_browser(browser, grace=.1) + kill.assert_called_once_with(123, signal.SIGTERM) + + @unittest.skipUnless(os.name == 'posix', 'POSIX process-group lifecycle') + def test_cleanup_fails_if_a_live_process_cannot_be_stopped(self): + browser = Mock(pid=123) + with patch.object(check.subprocess, 'check_output', return_value='123 S\n'), patch.object(check.os, 'killpg') as kill: + with self.assertRaisesRegex(RuntimeError, 'did not exit'): + check.stop_browser(browser, grace=0) + self.assertEqual([call.args[1] for call in kill.call_args_list], + [signal.SIGTERM, signal.SIGKILL]) + + @unittest.skipUnless(os.name == 'posix', 'POSIX process-group lifecycle') + def test_signal_races_do_not_hide_permission_errors_for_live_processes(self): + browser = Mock(pid=123) + for error in (ProcessLookupError, PermissionError): + with self.subTest(error=error), patch.object(check.subprocess, 'check_output', return_value=''), patch.object(check.os, 'killpg', side_effect=error): + check.stop_browser(browser) + with patch.object(check.subprocess, 'check_output', return_value='123 S\n'), patch.object(check.os, 'killpg', side_effect=PermissionError): + with self.assertRaises(PermissionError): + check.stop_browser(browser) + + def test_non_posix_cleanup_waits_and_escalates_if_necessary(self): + for times_out in (False, True): + with self.subTest(times_out=times_out), patch.object(check.os, 'name', 'nt'): + browser = Mock() + if times_out: + browser.wait.side_effect = [subprocess.TimeoutExpired('chrome', 10), 0] + check.stop_browser(browser) + browser.terminate.assert_called_once() + self.assertEqual(browser.kill.call_count, int(times_out)) + self.assertEqual(browser.wait.call_count, 1 + int(times_out)) + def test_missing_browser_is_an_error_not_a_skipped_check(self): with patch.object(check.os.path, 'isfile', return_value=False): with self.assertRaisesRegex(RuntimeError, 'Chrome is required'):