From 6598a2e9477836eed5071ff611895231bbc1c81b Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Mon, 14 Sep 2026 21:36:20 -0700 Subject: [PATCH 1/3] fix(zipapp): forward termination signals Use exec when stage 2 owns ZIP cleanup. For Python launchers, forward POSIX termination signals while keeping wait() as the sole child reaper so the application exit status is preserved.\n\nFixes #3809 --- news/3809.fixed.md | 1 + python/private/python_bootstrap_template.txt | 41 +++++++++- python/private/stage1_bootstrap_template.sh | 8 +- python/private/zipapp/zip_main_template.py | 41 +++++++++- python/private/zipapp/zip_shell_template.sh | 11 +-- tests/bootstrap_impls/bin.py | 13 ++++ .../run_binary_zip_yes_test.sh | 65 ++++++++++++++++ tests/py_zipapp/main.py | 31 +++++++- tests/py_zipapp/system_python_zipapp_test.py | 76 +++++++++++++++++++ 9 files changed, 268 insertions(+), 19 deletions(-) create mode 100644 news/3809.fixed.md diff --git a/news/3809.fixed.md b/news/3809.fixed.md new file mode 100644 index 0000000000..13c4ef3ed3 --- /dev/null +++ b/news/3809.fixed.md @@ -0,0 +1 @@ +(zipapp) Forwarded termination signals to the application and preserved its exit status. diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index 482918c038..6d5bd54800 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -28,6 +28,7 @@ if ( from os.path import abspath, dirname, join, basename, normpath import os +import signal import shutil import subprocess @@ -488,6 +489,43 @@ def runfiles_envvar(runfiles_root): return (None, None) +def _run_subprocess(argv, env, cwd): + if IS_WINDOWS: + return subprocess.call(argv, env=env, cwd=cwd) + + child = None + pending_signals = [] + + def forward_signal(signum, _frame): + if child is None: + pending_signals.append(signum) + else: + try: + # Keep wait() as the sole child reaper. Popen.send_signal() may call + # poll(), which can race wait() and lose the child's exit status. + os.kill(child.pid, signum) + except ProcessLookupError: + pass + + previous_handlers = {} + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signum = getattr(signal, name, None) + if signum is not None: + previous_handlers[signum] = signal.signal(signum, forward_signal) + + try: + child = subprocess.Popen(argv, env=env, cwd=cwd) + for signum in pending_signals: + forward_signal(signum, None) + ret_code = child.wait() + finally: + for signum, handler in previous_handlers.items(): + signal.signal(signum, handler) + + if ret_code < 0: + return 128 - ret_code + return ret_code + def execute_file(python_program, main_filename, args, env, runfiles_root, workspace, delete_dirs): # type: (str, str, list[str], dict[str, str], str, str|None, str|None) -> ... @@ -535,8 +573,7 @@ def execute_file(python_program, main_filename, args, env, runfiles_root, print_verbose("run: subproc: environ:", mapping=os.environ) print_verbose("run: subproc: cwd:", workspace) print_verbose("run: subproc: argv:", values=argv) - ret_code = subprocess.call( - argv, env=env, cwd=workspace) + ret_code = _run_subprocess(argv, env=env, cwd=workspace) print_verbose("run: subproc: exit code:", ret_code) if delete_dirs: diff --git a/python/private/stage1_bootstrap_template.sh b/python/private/stage1_bootstrap_template.sh index 4374ff95b0..a9aad0bd0a 100644 --- a/python/private/stage1_bootstrap_template.sh +++ b/python/private/stage1_bootstrap_template.sh @@ -153,7 +153,9 @@ python_exe=$(find_python_interpreter $RUNFILES_DIR $PYTHON_BINARY) # Zip files have to re-create the venv bin/python3 symlink because they # don't contain it already. if [[ "$IS_ZIPFILE" == "1" ]]; then - use_exec=0 + # Stage 2 removes the extracted runfiles through RULES_PYTHON_ZIP_DIR, so + # this bootstrap does not need to remain alive for cleanup. + use_exec=1 # It should always be under runfiles, but double check this. We don't # want to accidentally create symlinks elsewhere. if [[ "$python_exe" != $RUNFILES_DIR/* ]]; then @@ -333,8 +335,8 @@ command=( # for more information. # # However, we can't use exec when there is cleanup to do afterwards. Control -# must return to this process so it can run the trap handlers. Such cases -# occur when zip mode or recreate_venv_at_runtime creates temporary files. +# must return to this process so it can run the trap handlers. This case +# occurs when recreate_venv_at_runtime creates a temporary venv. if [[ "$use_exec" == "0" ]]; then "${command[@]}" exit $? diff --git a/python/private/zipapp/zip_main_template.py b/python/private/zipapp/zip_main_template.py index 709e08815c..1cd6c39a55 100644 --- a/python/private/zipapp/zip_main_template.py +++ b/python/private/zipapp/zip_main_template.py @@ -25,6 +25,7 @@ import os # noqa: E402 import shutil # noqa: E402 +import signal # noqa: E402 import stat # noqa: E402 import subprocess # noqa: E402 import tempfile # noqa: E402 @@ -234,6 +235,44 @@ def create_runfiles_root(): return join(extract_root, "runfiles") +def run_subprocess(subprocess_argv, env, cwd): + if IS_WINDOWS: + return subprocess.call(subprocess_argv, env=env, cwd=cwd) + + child = None + pending_signals = [] + + def forward_signal(signum, _frame): + if child is None: + pending_signals.append(signum) + else: + try: + # Keep wait() as the sole child reaper. Popen.send_signal() may call + # poll(), which can race wait() and lose the child's exit status. + os.kill(child.pid, signum) + except ProcessLookupError: + pass + + previous_handlers = {} + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signum = getattr(signal, name, None) + if signum is not None: + previous_handlers[signum] = signal.signal(signum, forward_signal) + + try: + child = subprocess.Popen(subprocess_argv, env=env, cwd=cwd) + for signum in pending_signals: + forward_signal(signum, None) + ret_code = child.wait() + finally: + for signum, handler in previous_handlers.items(): + signal.signal(signum, handler) + + if ret_code < 0: + return 128 - ret_code + return ret_code + + def execute_file( python_program, main_filename, @@ -276,7 +315,7 @@ def execute_file( print_verbose("subprocess env:", mapping=env) print_verbose("subprocess cwd:", workspace) print_verbose("subprocess argv:", values=subprocess_argv) - ret_code = subprocess.call(subprocess_argv, env=env, cwd=workspace) + ret_code = run_subprocess(subprocess_argv, env=env, cwd=workspace) print_verbose("subprocess exit code:", ret_code) sys.exit(ret_code) finally: diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index d79331444a..595159211c 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh @@ -77,12 +77,5 @@ command=( "$@" ) -# NOTE: because exec isn't used, signals don't propagate to the child -# TODO: Use exec and let the program handle cleanup. Without exec, -# signals don't propagate to the child nicely. -# See https://github.com/bazel-contrib/rules_python/issues/2043#issuecomment-2215469971 -# for more information. -"${command[@]}" -# Explicit exit is needed because the implicit next line the zip file this -# template is prepended to. -exit 0 +# The stage 2 bootstrap removes the extracted runfiles when the program exits. +exec "${command[@]}" diff --git a/tests/bootstrap_impls/bin.py b/tests/bootstrap_impls/bin.py index 0713b5f1be..38a35353d4 100644 --- a/tests/bootstrap_impls/bin.py +++ b/tests/bootstrap_impls/bin.py @@ -13,8 +13,21 @@ # limitations under the License. import os +import signal import sys +if sys.argv[1:] in (["handled"], ["unhandled"]): + if sys.argv[1:] == ["handled"]: + + def handle(signum, _frame): + print(f"received:{signum}", flush=True) + + signal.signal(signal.SIGTERM, handle) + + print(f"ready:{os.getpid()}", flush=True) + signal.pause() + raise SystemExit(0) + print("Hello") print( "RULES_PYTHON_ZIP_DIR:{}".format(sys._xoptions.get("RULES_PYTHON_ZIP_DIR", "UNSET")) diff --git a/tests/bootstrap_impls/run_binary_zip_yes_test.sh b/tests/bootstrap_impls/run_binary_zip_yes_test.sh index 77fe4d3609..6add123fb1 100755 --- a/tests/bootstrap_impls/run_binary_zip_yes_test.sh +++ b/tests/bootstrap_impls/run_binary_zip_yes_test.sh @@ -42,3 +42,68 @@ if ! (echo "$actual" | grep "$expected_pattern" ) >/dev/null; then exit 1 fi +case "$(uname -s)" in + CYGWIN*|MINGW*|MSYS*) exit 0 ;; +esac + +test_dir=$(mktemp -d) +launcher_pid="" +application_pid="" +watchdog_pid="" + +cleanup() { + if [[ -n "${watchdog_pid}" ]]; then + kill "${watchdog_pid}" 2>/dev/null || true + fi + if [[ -n "${launcher_pid}" ]]; then + kill -KILL "${launcher_pid}" 2>/dev/null || true + fi + if [[ -n "${application_pid}" ]]; then + kill -KILL "${application_pid}" 2>/dev/null || true + fi + rm -rf "${test_dir}" +} +trap cleanup EXIT + +run_signal_case() { + local mode="$1" + local expected_exit="$2" + local expected_output="$3" + local log="${test_dir}/${mode}.log" + + "$bin" "${mode}" >"${log}" 2>&1 & + launcher_pid=$! + for _ in {1..100}; do + if grep -F "ready:" "${log}" >/dev/null; then + break + fi + sleep 0.1 + done + grep -F "ready:" "${log}" >/dev/null || return 1 + application_pid=$(sed -n 's/^ready://p' "${log}") + + kill -TERM "${launcher_pid}" + ( + sleep 10 + kill -KILL "${launcher_pid}" "${application_pid}" 2>/dev/null || true + ) & + watchdog_pid=$! + wait "${launcher_pid}" + exit_code=$? + kill "${watchdog_pid}" 2>/dev/null || true + watchdog_pid="" + + if [[ "${exit_code}" != "${expected_exit}" ]]; then + echo "expected exit ${expected_exit}, got ${exit_code}" >&2 + cat "${log}" >&2 + return 1 + fi + if [[ -n "${expected_output}" ]]; then + grep -F "${expected_output}" "${log}" >/dev/null || return 1 + fi + launcher_pid="" + application_pid="" +} + +run_signal_case handled 0 "received:15" || exit 1 +run_signal_case unhandled 143 "" || exit 1 diff --git a/tests/py_zipapp/main.py b/tests/py_zipapp/main.py index 5770170d2c..df0e00c84d 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py @@ -1,7 +1,31 @@ -"A trivial zipapp that prints a message" +"A trivial zipapp that prints a message or waits for a signal." + +import os +import signal +import sys + + +def wait_for_signal(handle_signal): + if handle_signal: + + def handle(signum, _frame): + print(f"received:{signum}", flush=True) + + signal.signal(signal.SIGTERM, handle) + + print(f"ready:{os.getpid()}", flush=True) + signal.pause() + return 0 def main(): + if sys.argv[1:] == ["wait-for-sigterm"]: + return wait_for_signal(handle_signal=True) + if sys.argv[1:] == ["wait-for-unhandled-sigterm"]: + return wait_for_signal(handle_signal=False) + if len(sys.argv) == 3 and sys.argv[1] == "exit": + return int(sys.argv[2]) + print("Hello from zipapp") try: import some_dep @@ -12,13 +36,12 @@ def main(): print(f"dep: {pkgdep.pkgmod}") except ImportError as e: - import sys - e.add_note( "Failed to import a dependency.\n" + "sys.path:\n" + "\n".join(sys.path) ) raise + return 0 if __name__ == "__main__": - main() + raise SystemExit(main()) diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index 7c3e2deeaf..3bc69b0545 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -1,9 +1,41 @@ import os +import signal import subprocess +import sys import unittest class SystemPythonZipAppTest(unittest.TestCase): + def zipapp_command(self, *args, invoke_with_python=False): + zipapp_path = os.environ["TEST_ZIPAPP"] + command = [zipapp_path] + if invoke_with_python: + command.insert(0, sys.executable) + return [*command, *args] + + def start_signal_app(self, mode, invoke_with_python): + process = subprocess.Popen( + self.zipapp_command(mode, invoke_with_python=invoke_with_python), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + ) + self.assertIsNotNone(process.stdout) + ready = process.stdout.readline().strip() + self.assertTrue(ready.startswith("ready:"), ready) + return process, int(ready.removeprefix("ready:")) + + def stop_process(self, process, application_pid): + if process.poll() is None: + process.kill() + process.wait() + try: + os.kill(application_pid, signal.SIGKILL) + except ProcessLookupError: + pass + if process.stdout is not None: + process.stdout.close() + def test_zipapp_runnable(self): zipapp_path = os.environ["TEST_ZIPAPP"] @@ -27,6 +59,50 @@ def test_zipapp_runnable(self): self.assertIn("Hello from zipapp", output) self.assertIn("dep:", output) + @unittest.skipIf(os.name == "nt", "POSIX signals are required") + def test_zipapp_forwards_sigterm(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process, application_pid = self.start_signal_app( + "wait-for-sigterm", invoke_with_python + ) + try: + process.terminate() + output, _ = process.communicate(timeout=10) + self.assertEqual(0, process.returncode, output) + self.assertIn(f"received:{signal.SIGTERM}", output) + finally: + self.stop_process(process, application_pid) + + @unittest.skipIf(os.name == "nt", "POSIX signals are required") + def test_zipapp_preserves_signal_termination(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process, application_pid = self.start_signal_app( + "wait-for-unhandled-sigterm", invoke_with_python + ) + try: + process.terminate() + output, _ = process.communicate(timeout=10) + expected = ( + 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM + ) + self.assertEqual(expected, process.returncode, output) + finally: + self.stop_process(process, application_pid) + + @unittest.skipIf(os.name == "nt", "POSIX signals are required") + def test_zipapp_preserves_nonzero_exit_status(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process = subprocess.run( + self.zipapp_command( + "exit", "17", invoke_with_python=invoke_with_python + ), + check=False, + ) + self.assertEqual(17, process.returncode) + if __name__ == "__main__": unittest.main() From f816e365d918fe7755c8a9f829ce37aacd10947f Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Tue, 15 Sep 2026 09:53:07 -0700 Subject: [PATCH 2/3] test(zipapp): isolate signal cases to POSIX Keep the existing cross-platform zipapp smoke test separate from signal tests that use SIGKILL. Also link the release note to issue #3809. --- news/3809.fixed.md | 1 + tests/py_zipapp/system_python_zipapp_test.py | 52 ++++++++++---------- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/news/3809.fixed.md b/news/3809.fixed.md index 13c4ef3ed3..7761f06f7c 100644 --- a/news/3809.fixed.md +++ b/news/3809.fixed.md @@ -1 +1,2 @@ (zipapp) Forwarded termination signals to the application and preserved its exit status. +([#3809](https://github.com/bazel-contrib/rules_python/issues/3809)) diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index 3bc69b0545..adb321f01c 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -6,6 +6,32 @@ class SystemPythonZipAppTest(unittest.TestCase): + def test_zipapp_runnable(self): + zipapp_path = os.environ["TEST_ZIPAPP"] + + self.assertTrue(os.path.exists(zipapp_path)) + self.assertTrue(os.path.isfile(zipapp_path)) + + try: + output = ( + subprocess.check_output([zipapp_path], stderr=subprocess.STDOUT) + .decode("utf-8") + .strip() + ) + except subprocess.CalledProcessError as e: + self.fail( + "exit code: {}\n" + " command: {}\n" + "===== stdout/stderr start ==={}===== stdout/stderr end ====".format( + e.returncode, e.cmd, e.output.decode("utf-8") + ) + ) + self.assertIn("Hello from zipapp", output) + self.assertIn("dep:", output) + + +@unittest.skipUnless(os.name == "posix", "POSIX signals are required") +class PosixSignalZipAppTest(unittest.TestCase): def zipapp_command(self, *args, invoke_with_python=False): zipapp_path = os.environ["TEST_ZIPAPP"] command = [zipapp_path] @@ -36,30 +62,6 @@ def stop_process(self, process, application_pid): if process.stdout is not None: process.stdout.close() - def test_zipapp_runnable(self): - zipapp_path = os.environ["TEST_ZIPAPP"] - - self.assertTrue(os.path.exists(zipapp_path)) - self.assertTrue(os.path.isfile(zipapp_path)) - - try: - output = ( - subprocess.check_output([zipapp_path], stderr=subprocess.STDOUT) - .decode("utf-8") - .strip() - ) - except subprocess.CalledProcessError as e: - self.fail( - "exit code: {}\n" - " command: {}\n" - "===== stdout/stderr start ==={}===== stdout/stderr end ====".format( - e.returncode, e.cmd, e.output.decode("utf-8") - ) - ) - self.assertIn("Hello from zipapp", output) - self.assertIn("dep:", output) - - @unittest.skipIf(os.name == "nt", "POSIX signals are required") def test_zipapp_forwards_sigterm(self): for invoke_with_python in (False, True): with self.subTest(invoke_with_python=invoke_with_python): @@ -74,7 +76,6 @@ def test_zipapp_forwards_sigterm(self): finally: self.stop_process(process, application_pid) - @unittest.skipIf(os.name == "nt", "POSIX signals are required") def test_zipapp_preserves_signal_termination(self): for invoke_with_python in (False, True): with self.subTest(invoke_with_python=invoke_with_python): @@ -91,7 +92,6 @@ def test_zipapp_preserves_signal_termination(self): finally: self.stop_process(process, application_pid) - @unittest.skipIf(os.name == "nt", "POSIX signals are required") def test_zipapp_preserves_nonzero_exit_status(self): for invoke_with_python in (False, True): with self.subTest(invoke_with_python=invoke_with_python): From 780723a7377b01027d1a4d41f4c298c134d0d377 Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Tue, 15 Sep 2026 10:46:55 -0700 Subject: [PATCH 3/3] test(zipapp): use portable cleanup signal Use SIGTERM when cleaning up a leftover signal fixture. Windows Pyrefly analyzes the POSIX-only test class and does not define signal.SIGKILL. --- tests/py_zipapp/system_python_zipapp_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index adb321f01c..f2bea1457c 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -56,7 +56,7 @@ def stop_process(self, process, application_pid): process.kill() process.wait() try: - os.kill(application_pid, signal.SIGKILL) + os.kill(application_pid, signal.SIGTERM) except ProcessLookupError: pass if process.stdout is not None: