GH-142591: Tachyon does not handle non-existent file/module (#142592)

Co-authored-by: Pablo Galindo Salgado <pablogsal@gmail.com>
This commit is contained in:
Savannah Ostrowski 2025-12-13 20:58:40 -08:00 committed by GitHub
parent 52daab111b
commit f893e8f256
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 148 additions and 63 deletions

View file

@ -5,6 +5,7 @@ This module is used internally by the sample profiler to coordinate
the startup of target processes. It should not be called directly by users.
"""
import importlib.util
import os
import sys
import socket
@ -138,7 +139,7 @@ def _execute_module(module_name: str, module_args: List[str]) -> None:
"""
# Replace sys.argv to match how Python normally runs modules
# When running 'python -m module args', sys.argv is ["__main__.py", "args"]
sys.argv = [f"__main__.py"] + module_args
sys.argv = ["__main__.py"] + module_args
try:
runpy.run_module(module_name, run_name="__main__", alter_sys=True)
@ -215,22 +216,31 @@ def main() -> NoReturn:
# Set up execution environment
_setup_environment(cwd)
# Determine execution type and validate target exists
is_module = target_args[0] == "-m"
if is_module:
if len(target_args) < 2:
raise ArgumentError("Module name required after -m")
module_name = target_args[1]
module_args = target_args[2:]
if importlib.util.find_spec(module_name) is None:
raise TargetError(f"Module not found: {module_name}")
else:
script_path = target_args[0]
script_args = target_args[1:]
# Match the path resolution logic in _execute_script
check_path = script_path if os.path.isabs(script_path) else os.path.join(cwd, script_path)
if not os.path.isfile(check_path):
raise TargetError(f"Script not found: {script_path}")
# Signal readiness to profiler
_signal_readiness(sync_port)
# Execute the target
if target_args[0] == "-m":
# Module execution
if len(target_args) < 2:
raise ArgumentError("Module name required after -m")
module_name = target_args[1]
module_args = target_args[2:]
if is_module:
_execute_module(module_name, module_args)
else:
# Script execution
script_path = target_args[0]
script_args = target_args[1:]
_execute_script(script_path, script_args, cwd)
except CoordinatorError as e:

View file

@ -1,10 +1,13 @@
"""Command-line interface for the sampling profiler."""
import argparse
import importlib.util
import os
import selectors
import socket
import subprocess
import sys
import time
from .sample import sample, sample_live
from .pstats_collector import PstatsCollector
@ -92,6 +95,54 @@ def _parse_mode(mode_string):
return mode_map[mode_string]
def _check_process_died(process):
"""Check if process died and raise an error with stderr if available."""
if process.poll() is None:
return # Process still running
# Process died - try to get stderr for error message
stderr_msg = ""
if process.stderr:
try:
stderr_msg = process.stderr.read().decode().strip()
except (OSError, UnicodeDecodeError):
pass
if stderr_msg:
raise RuntimeError(stderr_msg)
raise RuntimeError(f"Process exited with code {process.returncode}")
def _wait_for_ready_signal(sync_sock, process, timeout):
"""Wait for the ready signal from the subprocess, checking for early death."""
deadline = time.monotonic() + timeout
sel = selectors.DefaultSelector()
sel.register(sync_sock, selectors.EVENT_READ)
try:
while True:
_check_process_died(process)
remaining = deadline - time.monotonic()
if remaining <= 0:
raise socket.timeout("timed out")
if not sel.select(timeout=min(0.1, remaining)):
continue
conn, _ = sync_sock.accept()
try:
ready_signal = conn.recv(_RECV_BUFFER_SIZE)
finally:
conn.close()
if ready_signal != _READY_MESSAGE:
raise RuntimeError(f"Invalid ready signal received: {ready_signal!r}")
return
finally:
sel.close()
def _run_with_sync(original_cmd, suppress_output=False):
"""Run a command with socket-based synchronization and return the process."""
# Create a TCP socket for synchronization with better socket options
@ -117,24 +168,24 @@ def _run_with_sync(original_cmd, suppress_output=False):
) + tuple(target_args)
# Start the process with coordinator
# Suppress stdout/stderr if requested (for live mode)
# When suppress_output=True (live mode), capture stderr so we can
# report errors if the process dies before signaling ready.
# When suppress_output=False (normal mode), let stderr inherit so
# script errors print to the terminal.
popen_kwargs = {}
if suppress_output:
popen_kwargs["stdin"] = subprocess.DEVNULL
popen_kwargs["stdout"] = subprocess.DEVNULL
popen_kwargs["stderr"] = subprocess.DEVNULL
popen_kwargs["stderr"] = subprocess.PIPE
process = subprocess.Popen(cmd, **popen_kwargs)
try:
# Wait for ready signal with timeout
with sync_sock.accept()[0] as conn:
ready_signal = conn.recv(_RECV_BUFFER_SIZE)
_wait_for_ready_signal(sync_sock, process, _SYNC_TIMEOUT)
if ready_signal != _READY_MESSAGE:
raise RuntimeError(
f"Invalid ready signal received: {ready_signal!r}"
)
# Close stderr pipe if we were capturing it
if process.stderr:
process.stderr.close()
except socket.timeout:
# If we timeout, kill the process and raise an error
@ -632,6 +683,25 @@ def _handle_attach(args):
def _handle_run(args):
"""Handle the 'run' command."""
# Validate target exists before launching subprocess
if args.module:
# Temporarily add cwd to sys.path so we can find modules in the
# current directory, matching the coordinator's behavior
cwd = os.getcwd()
added_cwd = False
if cwd not in sys.path:
sys.path.insert(0, cwd)
added_cwd = True
try:
if importlib.util.find_spec(args.target) is None:
sys.exit(f"Error: Module not found: {args.target}")
finally:
if added_cwd:
sys.path.remove(cwd)
else:
if not os.path.exists(args.target):
sys.exit(f"Error: Script not found: {args.target}")
# Check if live mode is requested
if args.live:
_handle_live_run(args)
@ -644,7 +714,10 @@ def _handle_run(args):
cmd = (sys.executable, args.target, *args.args)
# Run with synchronization
process = _run_with_sync(cmd, suppress_output=False)
try:
process = _run_with_sync(cmd, suppress_output=False)
except RuntimeError as e:
sys.exit(f"Error: {e}")
# Use PROFILING_MODE_ALL for gecko format
mode = (
@ -732,7 +805,10 @@ def _handle_live_run(args):
cmd = (sys.executable, args.target, *args.args)
# Run with synchronization, suppressing output for live mode
process = _run_with_sync(cmd, suppress_output=True)
try:
process = _run_with_sync(cmd, suppress_output=True)
except RuntimeError as e:
sys.exit(f"Error: {e}")
mode = _parse_mode(args.mode)

View file

@ -13,7 +13,9 @@ except ImportError:
"Test only runs when _remote_debugging is available"
)
from test.support import is_emscripten
from test.support import is_emscripten, requires_subprocess
from profiling.sampling.cli import main
class TestSampleProfilerCLI(unittest.TestCase):
@ -62,6 +64,7 @@ class TestSampleProfilerCLI(unittest.TestCase):
self.assertEqual(coordinator_cmd[5:], expected_target_args)
@unittest.skipIf(is_emscripten, "socket.SO_REUSEADDR does not exist")
@requires_subprocess()
def test_cli_module_argument_parsing(self):
test_args = ["profiling.sampling.cli", "run", "-m", "mymodule"]
@ -70,8 +73,9 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("importlib.util.find_spec", return_value=True),
):
from profiling.sampling.cli import main
self._setup_sync_mocks(mock_socket, mock_popen)
main()
@ -80,6 +84,7 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock_sample.assert_called_once()
@unittest.skipIf(is_emscripten, "socket.SO_REUSEADDR does not exist")
@requires_subprocess()
def test_cli_module_with_arguments(self):
test_args = [
"profiling.sampling.cli",
@ -96,9 +101,10 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("importlib.util.find_spec", return_value=True),
):
self._setup_sync_mocks(mock_socket, mock_popen)
from profiling.sampling.cli import main
main()
self._verify_coordinator_command(
@ -115,9 +121,10 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("os.path.exists", return_value=True),
):
self._setup_sync_mocks(mock_socket, mock_popen)
from profiling.sampling.cli import main
main()
self._verify_coordinator_command(mock_popen, ("myscript.py",))
@ -139,6 +146,8 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("os.path.exists", return_value=True),
):
# Use the helper to set up mocks consistently
mock_process = self._setup_sync_mocks(mock_socket, mock_popen)
@ -148,7 +157,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
None,
]
from profiling.sampling.cli import main
main()
# Verify the coordinator command was called
@ -181,7 +189,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -196,18 +203,12 @@ class TestSampleProfilerCLI(unittest.TestCase):
with (
mock.patch("sys.argv", test_args),
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
self.assertRaises(FileNotFoundError) as cm, # Expect FileNotFoundError, not SystemExit
self.assertRaises(SystemExit) as cm,
):
self._setup_sync_mocks(mock_socket, mock_popen)
# Override to raise FileNotFoundError for non-existent script
mock_popen.side_effect = FileNotFoundError("12345")
from profiling.sampling.cli import main
main()
# Verify the error is about the non-existent script
self.assertIn("12345", str(cm.exception))
self.assertIn("12345", str(cm.exception.code))
def test_cli_no_target_specified(self):
# In new CLI, must specify a subcommand
@ -218,7 +219,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -226,6 +226,7 @@ class TestSampleProfilerCLI(unittest.TestCase):
self.assertIn("invalid choice", error_msg)
@unittest.skipIf(is_emscripten, "socket.SO_REUSEADDR does not exist")
@requires_subprocess()
def test_cli_module_with_profiler_options(self):
test_args = [
"profiling.sampling.cli",
@ -248,9 +249,10 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("importlib.util.find_spec", return_value=True),
):
self._setup_sync_mocks(mock_socket, mock_popen)
from profiling.sampling.cli import main
main()
self._verify_coordinator_command(mock_popen, ("-m", "mymodule"))
@ -278,9 +280,10 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("os.path.exists", return_value=True),
):
self._setup_sync_mocks(mock_socket, mock_popen)
from profiling.sampling.cli import main
main()
self._verify_coordinator_command(
@ -297,7 +300,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -305,6 +307,7 @@ class TestSampleProfilerCLI(unittest.TestCase):
self.assertIn("required: target", error_msg) # argparse error for missing positional arg
@unittest.skipIf(is_emscripten, "socket.SO_REUSEADDR does not exist")
@requires_subprocess()
def test_cli_long_module_option(self):
test_args = [
"profiling.sampling.cli",
@ -319,9 +322,10 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("profiling.sampling.cli.sample") as mock_sample,
mock.patch("subprocess.Popen") as mock_popen,
mock.patch("socket.socket") as mock_socket,
mock.patch("profiling.sampling.cli._wait_for_ready_signal"),
mock.patch("importlib.util.find_spec", return_value=True),
):
self._setup_sync_mocks(mock_socket, mock_popen)
from profiling.sampling.cli import main
main()
self._verify_coordinator_command(
@ -346,6 +350,7 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch(
"profiling.sampling.cli._run_with_sync"
) as mock_run_with_sync,
mock.patch("os.path.exists", return_value=True),
):
mock_process = mock.MagicMock()
mock_process.pid = 12345
@ -356,7 +361,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock_process.poll.return_value = None
mock_run_with_sync.return_value = mock_process
from profiling.sampling.cli import main
main()
mock_run_with_sync.assert_called_once_with(
@ -412,8 +416,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
),
]
from profiling.sampling.cli import main
for test_args, expected_error_keyword in test_cases:
with (
mock.patch("sys.argv", test_args),
@ -436,7 +438,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
# Check that sample was called (exact filename depends on implementation)
@ -471,8 +472,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
),
]
from profiling.sampling.cli import main
for test_args, expected_filename, expected_format in test_cases:
with (
mock.patch("sys.argv", test_args),
@ -489,7 +488,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()),
):
with self.assertRaises(SystemExit):
from profiling.sampling.cli import main
main()
def test_cli_mutually_exclusive_format_options(self):
@ -508,7 +506,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()),
):
with self.assertRaises(SystemExit):
from profiling.sampling.cli import main
main()
def test_argument_parsing_basic(self):
@ -518,14 +515,11 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
mock_sample.assert_called_once()
def test_sort_options(self):
from profiling.sampling.cli import main
sort_options = [
("nsamples", 0),
("tottime", 1),
@ -542,7 +536,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
mock_sample.assert_called_once()
@ -556,7 +549,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
mock_sample.assert_called_once()
@ -572,7 +564,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
mock_sample.assert_called_once()
@ -587,7 +578,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.argv", test_args),
mock.patch("profiling.sampling.cli.sample") as mock_sample,
):
from profiling.sampling.cli import main
main()
mock_sample.assert_called_once()
@ -603,7 +593,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()),
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -617,7 +606,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -633,7 +621,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -650,7 +637,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -667,7 +653,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -685,7 +670,6 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
@ -702,10 +686,25 @@ class TestSampleProfilerCLI(unittest.TestCase):
mock.patch("sys.stderr", io.StringIO()) as mock_stderr,
self.assertRaises(SystemExit) as cm,
):
from profiling.sampling.cli import main
main()
self.assertEqual(cm.exception.code, 2) # argparse error
error_msg = mock_stderr.getvalue()
self.assertIn("--all-threads", error_msg)
self.assertIn("incompatible with --async-aware", error_msg)
@unittest.skipIf(is_emscripten, "subprocess not available")
def test_run_nonexistent_script_exits_cleanly(self):
"""Test that running a non-existent script exits with a clean error."""
with mock.patch("sys.argv", ["profiling.sampling.cli", "run", "/nonexistent/script.py"]):
with self.assertRaises(SystemExit) as cm:
main()
self.assertIn("Script not found", str(cm.exception.code))
@unittest.skipIf(is_emscripten, "subprocess not available")
def test_run_nonexistent_module_exits_cleanly(self):
"""Test that running a non-existent module exits with a clean error."""
with mock.patch("sys.argv", ["profiling.sampling.cli", "run", "-m", "nonexistent_module_xyz"]):
with self.assertRaises(SystemExit) as cm:
main()
self.assertIn("Module not found", str(cm.exception.code))