From 6598a2e9477836eed5071ff611895231bbc1c81b Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Mon, 14 Sep 2026 21:36:20 -0700 Subject: [PATCH 1/6] 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/6] 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/6] 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: From c3cf1abdd5b811c5db971503c5bf9f6647ddeeb0 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 13 Oct 2026 20:50:56 -0700 Subject: [PATCH 4/6] test(zipapp): verify temp dir cleanup, -X option isolation, and SIG_IGN Add test assertions checking that extracted zip temporary directories are cleaned up after exit or signal termination, RULES_PYTHON_ZIP_DIR is not leaked into user sys._xoptions or set when RULES_PYTHON_EXTRACT_ROOT is used, and SIG_IGN signal dispositions are preserved. --- .../run_binary_zip_yes_test.sh | 8 ++ tests/py_zipapp/main.py | 3 + tests/py_zipapp/system_python_zipapp_test.py | 100 +++++++++++++++++- 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/tests/bootstrap_impls/run_binary_zip_yes_test.sh b/tests/bootstrap_impls/run_binary_zip_yes_test.sh index 6add123fb1..b92d575979 100755 --- a/tests/bootstrap_impls/run_binary_zip_yes_test.sh +++ b/tests/bootstrap_impls/run_binary_zip_yes_test.sh @@ -46,6 +46,14 @@ case "$(uname -s)" in CYGWIN*|MINGW*|MSYS*) exit 0 ;; esac +# Verify that temporary extracted zip directories are cleaned up when the binary exits. +extracted_file=$(echo "$actual" | sed -n 's/^file: //p') +extracted_root="${extracted_file%%/runfiles/*}" +if [[ -n "$extracted_root" && -e "$extracted_root" ]]; then + echo "ERROR: temporary zip directory was not cleaned up after exit: $extracted_root" >&2 + exit 1 +fi + test_dir=$(mktemp -d) launcher_pid="" application_pid="" diff --git a/tests/py_zipapp/main.py b/tests/py_zipapp/main.py index df0e00c84d..b4ffc102f7 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py @@ -13,6 +13,7 @@ def handle(signum, _frame): signal.signal(signal.SIGTERM, handle) + print(f"file:{__file__}", flush=True) print(f"ready:{os.getpid()}", flush=True) signal.pause() return 0 @@ -27,6 +28,8 @@ def main(): return int(sys.argv[2]) print("Hello from zipapp") + print(f"file:{__file__}") + print(f"xoption_zip_dir:{sys._xoptions.get('RULES_PYTHON_ZIP_DIR', 'UNSET')}") try: import some_dep diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index f2bea1457c..b060daa7f9 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -2,9 +2,21 @@ import signal import subprocess import sys +import tempfile +import time import unittest +def _extract_root_from_output(output): + for line in output.splitlines(): + if line.startswith("file:"): + file_path = line.removeprefix("file:").strip() + runfiles_marker = f"{os.sep}runfiles{os.sep}" + if runfiles_marker in file_path: + return file_path.split(runfiles_marker, 1)[0] + raise AssertionError(f"Could not find file: line in output:\n{output}") + + class SystemPythonZipAppTest(unittest.TestCase): def test_zipapp_runnable(self): zipapp_path = os.environ["TEST_ZIPAPP"] @@ -28,6 +40,17 @@ def test_zipapp_runnable(self): ) self.assertIn("Hello from zipapp", output) self.assertIn("dep:", output) + self.assertIn( + "xoption_zip_dir:UNSET", + output, + f"RULES_PYTHON_ZIP_DIR -X option leaked into user main.py:\n{output}", + ) + + extract_root = _extract_root_from_output(output) + self.assertFalse( + os.path.exists(extract_root), + f"Temporary zip extraction dir leaked after exit: {extract_root}", + ) @unittest.skipUnless(os.name == "posix", "POSIX signals are required") @@ -39,17 +62,21 @@ def zipapp_command(self, *args, invoke_with_python=False): command.insert(0, sys.executable) return [*command, *args] - def start_signal_app(self, mode, invoke_with_python): + def start_signal_app(self, mode, invoke_with_python, preexec_fn=None): process = subprocess.Popen( self.zipapp_command(mode, invoke_with_python=invoke_with_python), stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, + preexec_fn=preexec_fn, ) self.assertIsNotNone(process.stdout) + file_line = process.stdout.readline().strip() + self.assertTrue(file_line.startswith("file:"), file_line) + extract_root = _extract_root_from_output(file_line) ready = process.stdout.readline().strip() self.assertTrue(ready.startswith("ready:"), ready) - return process, int(ready.removeprefix("ready:")) + return process, int(ready.removeprefix("ready:")), extract_root def stop_process(self, process, application_pid): if process.poll() is None: @@ -65,7 +92,7 @@ def stop_process(self, process, application_pid): 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( + process, application_pid, extract_root = self.start_signal_app( "wait-for-sigterm", invoke_with_python ) try: @@ -73,13 +100,17 @@ def test_zipapp_forwards_sigterm(self): output, _ = process.communicate(timeout=10) self.assertEqual(0, process.returncode, output) self.assertIn(f"received:{signal.SIGTERM}", output) + self.assertFalse( + os.path.exists(extract_root), + f"Temporary zip extraction dir leaked after handled SIGTERM: {extract_root}", + ) finally: self.stop_process(process, application_pid) 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( + process, application_pid, extract_root = self.start_signal_app( "wait-for-unhandled-sigterm", invoke_with_python ) try: @@ -89,6 +120,67 @@ def test_zipapp_preserves_signal_termination(self): 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM ) self.assertEqual(expected, process.returncode, output) + self.assertFalse( + os.path.exists(extract_root), + f"Temporary zip extraction dir leaked after unhandled SIGTERM: {extract_root}", + ) + finally: + self.stop_process(process, application_pid) + + def test_zipapp_preserves_extract_root_cache(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + with tempfile.TemporaryDirectory() as extract_root_env: + env = {**os.environ, "RULES_PYTHON_EXTRACT_ROOT": extract_root_env} + proc = subprocess.run( + self.zipapp_command(invoke_with_python=invoke_with_python), + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + check=True, + ) + extract_root = _extract_root_from_output(proc.stdout) + self.assertTrue( + extract_root.startswith(extract_root_env), + f"Expected {extract_root} to be under {extract_root_env}", + ) + self.assertTrue( + os.path.exists(extract_root), + f"RULES_PYTHON_EXTRACT_ROOT cache dir was unexpectedly deleted: {extract_root}", + ) + self.assertIn( + "xoption_zip_dir:UNSET", + proc.stdout, + "RULES_PYTHON_ZIP_DIR should not be exposed when " + f"RULES_PYTHON_EXTRACT_ROOT is set:\n{proc.stdout}", + ) + + def test_zipapp_preserves_sig_ign(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, + preexec_fn=lambda: signal.signal(signal.SIGHUP, signal.SIG_IGN), + ) + try: + process.send_signal(signal.SIGHUP) + time.sleep(0.2) + self.assertIsNone( + process.poll(), + f"Process died from SIGHUP (returncode={process.returncode}) instead of ignoring SIG_IGN", + ) + process.terminate() + output, _ = process.communicate(timeout=10) + expected = ( + 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM + ) + self.assertEqual( + expected, + process.returncode, + f"Process died from SIGHUP instead of ignoring SIG_IGN:\n{output}", + ) finally: self.stop_process(process, application_pid) From d8a4dad06324f06c581173da2a3def5b44995f6f Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Tue, 13 Oct 2026 21:38:24 -0700 Subject: [PATCH 5/6] fix(zipapp): restore cleanup and preserve ignored signals Use the detached watcher from #4210 for both shell ZIP launchers, retaining EXIT traps for startup failures. Keep persistent and verbose extraction directories, and stop passing the unused cleanup -X option into user code. Preserve inherited SIG_IGN in both Python forwarding helpers. Add Frank Liu's process and cleanup regressions, extend ignored-signal and legacy cleanup coverage, and make the maintainer's SIGHUP test compatible with Windows static checking. Co-authored-by: Frank Liu --- .bazelrc.deleted_packages | 2 +- news/3809.fixed.md | 3 +- python/private/python_bootstrap_template.txt | 4 +- python/private/stage1_bootstrap_template.sh | 55 ++++++++- python/private/zipapp/zip_main_template.py | 6 +- python/private/zipapp/zip_shell_template.sh | 56 ++++++++- tests/bootstrap_impls/bin.py | 1 + .../run_binary_zip_yes_test.sh | 27 +++-- tests/py_zipapp/BUILD.bazel | 27 +++++ tests/py_zipapp/main.py | 7 ++ tests/py_zipapp/signals_main.py | 26 +++++ tests/py_zipapp/system_python_zipapp_test.py | 49 +++++--- tests/py_zipapp/zipapp_signals_test.py | 108 ++++++++++++++++++ 13 files changed, 336 insertions(+), 35 deletions(-) create mode 100644 tests/py_zipapp/signals_main.py create mode 100644 tests/py_zipapp/zipapp_signals_test.py diff --git a/.bazelrc.deleted_packages b/.bazelrc.deleted_packages index 5654df1266..3eec28298e 100644 --- a/.bazelrc.deleted_packages +++ b/.bazelrc.deleted_packages @@ -28,8 +28,8 @@ common --deleted_packages=gazelle/manifest/hasher common --deleted_packages=gazelle/manifest/test common --deleted_packages=gazelle/modules_mapping common --deleted_packages=gazelle/python -common --deleted_packages=gazelle/pythonconfig common --deleted_packages=gazelle/python/private +common --deleted_packages=gazelle/pythonconfig common --deleted_packages=tests/integration/bzlmod_lockfile common --deleted_packages=tests/integration/compile_pip_requirements common --deleted_packages=tests/integration/compile_pip_requirements_test_from_external_repo diff --git a/news/3809.fixed.md b/news/3809.fixed.md index 7761f06f7c..f8c6d09df3 100644 --- a/news/3809.fixed.md +++ b/news/3809.fixed.md @@ -1,2 +1,3 @@ -(zipapp) Forwarded termination signals to the application and preserved its exit status. +(zipapp) Forwarded termination signals to the application, preserved inherited +ignored signals and exit status, and cleaned up temporary extraction directories. ([#3809](https://github.com/bazel-contrib/rules_python/issues/3809)) diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index 6d5bd54800..895d154d3a 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -511,7 +511,9 @@ def _run_subprocess(argv, env, cwd): 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) + # Ignored signals must remain ignored across the child exec. + if signal.getsignal(signum) != signal.SIG_IGN: + previous_handlers[signum] = signal.signal(signum, forward_signal) try: child = subprocess.Popen(argv, env=env, cwd=cwd) diff --git a/python/private/stage1_bootstrap_template.sh b/python/private/stage1_bootstrap_template.sh index a9aad0bd0a..ef098127fd 100644 --- a/python/private/stage1_bootstrap_template.sh +++ b/python/private/stage1_bootstrap_template.sh @@ -60,12 +60,14 @@ declare -a INTERPRETER_ARGS_FROM_TARGET=( %interpreter_args% ) +cleanup_zip_dir="" if [[ "$IS_ZIPFILE" == "1" ]]; then # NOTE: Macs have an old version of mktemp, so we must use only the # minimal functionality of it. zip_dir=$(mktemp -d) if [[ -n "$zip_dir" && -z "${RULES_PYTHON_BOOTSTRAP_VERBOSE:-}" ]]; then + cleanup_zip_dir=1 trap 'rm -fr "$zip_dir"' EXIT fi # unzip emits a warning and exits with code 1 when there is extraneous data, @@ -153,8 +155,7 @@ 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 - # Stage 2 removes the extracted runfiles through RULES_PYTHON_ZIP_DIR, so - # this bootstrap does not need to remain alive for cleanup. + # A detached watcher below owns cleanup after exec replaces this bootstrap. use_exec=1 # It should always be under runfiles, but double check this. We don't # want to accidentally create symlinks elsewhere. @@ -299,10 +300,6 @@ if [[ -z "${PYTHONSAFEPATH+x}" ]]; then interpreter_env+=("PYTHONSAFEPATH=${PYTHONSAFEPATH-1}") fi -if [[ "$IS_ZIPFILE" == "1" ]]; then - interpreter_args+=("-XRULES_PYTHON_ZIP_DIR=$zip_dir") -fi - if [[ -n "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" ]]; then read -a additional_interpreter_args <<< "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" interpreter_args+=("${additional_interpreter_args[@]}") @@ -327,6 +324,52 @@ command=( "$@" ) +# Prints an identifier for a running process that changes if the PID is reused, +# or nothing once the process has exited. A zombie counts as exited, so it +# doesn't matter whether the caller has reaped it yet. +process_identity() { + local stat fields + if [[ -r "/proc/$1/stat" ]]; then + stat=$(< "/proc/$1/stat") || return 0 + # The command name is in parentheses and may contain spaces, so split the + # fields after the last ")": field 3 (state) through field 22 (start time). + read -r -a fields <<< "${stat##*) }" + if [[ "${fields[0]}" != "Z" ]]; then + echo "${fields[19]}" + fi + elif command -v ps >/dev/null 2>&1; then + stat=$(ps -o stat=,lstart= -p "$1" 2>/dev/null) + stat="${stat#"${stat%%[![:space:]]*}"}" + if [[ -n "$stat" && "$stat" != Z* ]]; then + echo "${stat#* }" + fi + elif kill -0 "$1" 2>/dev/null; then + echo "running" + fi +} + +if [[ -n "$cleanup_zip_dir" ]]; then + # exec replaces this shell, so the EXIT trap can't remove the extracted files. + # Instead, start a watcher that removes them once this PID, which becomes the + # Python process, exits. Unlike the trap, this also works when the process is + # killed with SIGKILL. The watcher is double-forked so it isn't a child of the + # Python process, and it doesn't hold this process's stdio open. It stays in + # the caller's process group, so it ignores the signals sent to a whole group + # (e.g. Ctrl-C in a terminal) and exits on its own once the process is gone. + # Unlike the implicit SIGINT ignore for background jobs, `trap ''` also + # applies to the commands it runs. + launcher_pid=$$ + launcher_identity=$(process_identity "$launcher_pid") + ( ( + trap '' INT QUIT HUP TERM + while [[ -n "$launcher_identity" && + "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do + sleep 1 + done + rm -fr "$zip_dir" + ) /dev/null 2>&1 & ) +fi + # We use `exec` instead of a child process so that signals sent directly (e.g. # using `kill`) to this process (the PID seen by the calling process) are # received by the Python process. Otherwise, this process receives the signal diff --git a/python/private/zipapp/zip_main_template.py b/python/private/zipapp/zip_main_template.py index 1cd6c39a55..4f366ecb58 100644 --- a/python/private/zipapp/zip_main_template.py +++ b/python/private/zipapp/zip_main_template.py @@ -257,7 +257,9 @@ def forward_signal(signum, _frame): 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) + # Ignored signals must remain ignored across the child exec. + if signal.getsignal(signum) != signal.SIG_IGN: + previous_handlers[signum] = signal.signal(signum, forward_signal) try: child = subprocess.Popen(subprocess_argv, env=env, cwd=cwd) @@ -308,8 +310,6 @@ def execute_file( # workspace after the process finishes so control must return here. try: subprocess_argv = [python_program] - if not EXTRACT_ROOT: - subprocess_argv.append(f"-XRULES_PYTHON_ZIP_DIR={dirname(runfiles_root)}") subprocess_argv.append(main_filename) subprocess_argv += args print_verbose("subprocess env:", mapping=env) diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index 595159211c..f333765622 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh @@ -35,6 +35,7 @@ if [[ -n "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" ]]; then fi +cleanup_zip_dir="" if [[ -n "$RULES_PYTHON_EXTRACT_ROOT" ]]; then zip_dir="$RULES_PYTHON_EXTRACT_ROOT/$EXTRACT_DIR/$ZIP_HASH" if [[ ! -e "$zip_dir/__main__.py" ]]; then @@ -49,6 +50,7 @@ else # Unzip emits a warning and exits 1 with the prelude ( unzip -q -d "$zip_dir" "$0" 2>/dev/null || true ) if [[ -n "$zip_dir" && -z "${RULES_PYTHON_BOOTSTRAP_VERBOSE:-}" ]]; then + cleanup_zip_dir=1 trap 'rm -fr "$zip_dir"' EXIT fi fi @@ -70,12 +72,62 @@ command=( env "${interpreter_env[@]}" "$python_exe" - "-XRULES_PYTHON_ZIP_DIR=$zip_dir" "${interpreter_args[@]}" "${INTERPRETER_ARGS_FROM_TARGET[@]}" "$RUNFILES_DIR/$STAGE2_BOOTSTRAP" "$@" ) -# The stage 2 bootstrap removes the extracted runfiles when the program exits. +# Prints an identifier for a running process that changes if the PID is reused, +# or nothing once the process has exited. A zombie counts as exited, so it +# doesn't matter whether the caller has reaped it yet. +process_identity() { + local stat fields + if [[ -r "/proc/$1/stat" ]]; then + stat=$(< "/proc/$1/stat") || return 0 + # The command name is in parentheses and may contain spaces, so split the + # fields after the last ")": field 3 (state) through field 22 (start time). + read -r -a fields <<< "${stat##*) }" + if [[ "${fields[0]}" != "Z" ]]; then + echo "${fields[19]}" + fi + elif command -v ps >/dev/null 2>&1; then + stat=$(ps -o stat=,lstart= -p "$1" 2>/dev/null) + stat="${stat#"${stat%%[![:space:]]*}"}" + if [[ -n "$stat" && "$stat" != Z* ]]; then + echo "${stat#* }" + fi + elif kill -0 "$1" 2>/dev/null; then + echo "running" + fi +} + +if [[ -n "$cleanup_zip_dir" ]]; then + # exec replaces this shell, so the EXIT trap can't remove the extracted files. + # Instead, start a watcher that removes them once this PID, which becomes the + # Python process, exits. Unlike the trap, this also works when the process is + # killed with SIGKILL. The watcher is double-forked so it isn't a child of the + # Python process, and it doesn't hold this process's stdio open. It stays in + # the caller's process group, so it ignores the signals sent to a whole group + # (e.g. Ctrl-C in a terminal) and exits on its own once the process is gone. + # Unlike the implicit SIGINT ignore for background jobs, `trap ''` also + # applies to the commands it runs. + launcher_pid=$$ + launcher_identity=$(process_identity "$launcher_pid") + ( ( + trap '' INT QUIT HUP TERM + while [[ -n "$launcher_identity" && + "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do + sleep 1 + done + rm -fr "$zip_dir" + ) /dev/null 2>&1 & ) +fi + +# We use `exec` instead of a child process so that signals sent directly (e.g. +# using `kill`) to this process (the PID seen by the calling process) are +# received by the Python process. Otherwise, this process receives the signal +# and the Python process keeps running. +# See https://github.com/bazel-contrib/rules_python/issues/2043#issuecomment-2215469971 +# for more information. exec "${command[@]}" diff --git a/tests/bootstrap_impls/bin.py b/tests/bootstrap_impls/bin.py index 38a35353d4..7ebe0c047e 100644 --- a/tests/bootstrap_impls/bin.py +++ b/tests/bootstrap_impls/bin.py @@ -24,6 +24,7 @@ def handle(signum, _frame): signal.signal(signal.SIGTERM, handle) + print(f"file: {__file__}", flush=True) print(f"ready:{os.getpid()}", flush=True) signal.pause() raise SystemExit(0) diff --git a/tests/bootstrap_impls/run_binary_zip_yes_test.sh b/tests/bootstrap_impls/run_binary_zip_yes_test.sh index b92d575979..43afc5fda5 100755 --- a/tests/bootstrap_impls/run_binary_zip_yes_test.sh +++ b/tests/bootstrap_impls/run_binary_zip_yes_test.sh @@ -33,9 +33,9 @@ actual=$($bin) # How we detect if a zip file was executed from depends on which bootstrap # is used. -# bootstrap_impl=script outputs RULES_PYTHON_ZIP_DIR: +# bootstrap_impl=script outputs file:/runfiles/... # bootstrap_impl=system_python outputs file:.*Bazel.runfiles (or .exe.runfiles on Windows) -expected_pattern="RULES_PYTHON_ZIP_DIR:/\|file:.*Bazel.runfiles\|file:.*\.exe\.runfiles" +expected_pattern="file:.*/runfiles/\|file:.*Bazel.runfiles\|file:.*\.exe\.runfiles" if ! (echo "$actual" | grep "$expected_pattern" ) >/dev/null; then echo "expected output to match: $expected_pattern" echo "but got: $actual" @@ -49,10 +49,16 @@ esac # Verify that temporary extracted zip directories are cleaned up when the binary exits. extracted_file=$(echo "$actual" | sed -n 's/^file: //p') extracted_root="${extracted_file%%/runfiles/*}" -if [[ -n "$extracted_root" && -e "$extracted_root" ]]; then - echo "ERROR: temporary zip directory was not cleaned up after exit: $extracted_root" >&2 - exit 1 -fi +assert_removed_eventually() { + local path="$1" + for _ in {1..150}; do + [[ ! -e "$path" ]] && return 0 + sleep 0.1 + done + echo "ERROR: temporary zip directory was not cleaned up after exit: $path" >&2 + return 1 +} +assert_removed_eventually "$extracted_root" || exit 1 test_dir=$(mktemp -d) launcher_pid="" @@ -79,7 +85,8 @@ run_signal_case() { local expected_output="$3" local log="${test_dir}/${mode}.log" - "$bin" "${mode}" >"${log}" 2>&1 & + # An inherited ignored SIGHUP must remain ignored through both bootstraps. + ( trap '' HUP; exec "$bin" "${mode}" ) >"${log}" 2>&1 & launcher_pid=$! for _ in {1..100}; do if grep -F "ready:" "${log}" >/dev/null; then @@ -90,6 +97,9 @@ run_signal_case() { grep -F "ready:" "${log}" >/dev/null || return 1 application_pid=$(sed -n 's/^ready://p' "${log}") + kill -HUP "${launcher_pid}" + sleep 0.2 + kill -0 "${application_pid}" || return 1 kill -TERM "${launcher_pid}" ( sleep 10 @@ -109,6 +119,9 @@ run_signal_case() { if [[ -n "${expected_output}" ]]; then grep -F "${expected_output}" "${log}" >/dev/null || return 1 fi + local extracted_file + extracted_file=$(sed -n 's/^file: //p' "${log}") + assert_removed_eventually "${extracted_file%%/runfiles/*}" || return 1 launcher_pid="" application_pid="" } diff --git a/tests/py_zipapp/BUILD.bazel b/tests/py_zipapp/BUILD.bazel index e0f878cbe1..ede19baff8 100644 --- a/tests/py_zipapp/BUILD.bazel +++ b/tests/py_zipapp/BUILD.bazel @@ -4,6 +4,7 @@ load("//python:py_library.bzl", "py_library") load("//python:py_test.bzl", "py_test") load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility load("//python/zipapp:py_zipapp_binary.bzl", "py_zipapp_binary") +load("//tests/support:support.bzl", "SUPPORTS_BOOTSTRAP_SCRIPT") py_binary( name = "venv_bin", @@ -120,3 +121,29 @@ py_library( experimental_venvs_site_packages = "//python/config_settings:venvs_site_packages", imports = ["site-packages"], ) + +py_binary( + name = "signals_bin", + srcs = ["signals_main.py"], + config_settings = { + "//python/config_settings:bootstrap_impl": "script", + }, + main = "signals_main.py", + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) + +py_zipapp_binary( + name = "signals_zipapp", + binary = ":signals_bin", + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) + +py_test( + name = "zipapp_signals_test", + srcs = ["zipapp_signals_test.py"], + data = [":signals_zipapp"], + env = { + "TEST_ZIPAPP": "$(location :signals_zipapp)", + }, + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) diff --git a/tests/py_zipapp/main.py b/tests/py_zipapp/main.py index b4ffc102f7..d4170f3f92 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py @@ -20,6 +20,13 @@ def handle(signum, _frame): def main(): + if sys.argv[1:] == ["check-ignored-signals"]: + ignored = all( + signal.getsignal(sig) == signal.SIG_IGN + for sig in (signal.SIGHUP, signal.SIGINT, signal.SIGQUIT, signal.SIGTERM) + ) + print(f"ignored-signals:{ignored}") + return 0 if sys.argv[1:] == ["wait-for-sigterm"]: return wait_for_signal(handle_signal=True) if sys.argv[1:] == ["wait-for-unhandled-sigterm"]: diff --git a/tests/py_zipapp/signals_main.py b/tests/py_zipapp/signals_main.py new file mode 100644 index 0000000000..2fc519a2ee --- /dev/null +++ b/tests/py_zipapp/signals_main.py @@ -0,0 +1,26 @@ +"A zipapp that reports its PID and extraction directory, then waits for SIGTERM." + +import os +import signal +import sys +import time + + +def main(): + if len(sys.argv) > 1 and sys.argv[1].startswith("--exit-code="): + sys.exit(int(sys.argv[1].split("=", 1)[1])) + + def on_sigterm(signum, frame): + # print() isn't reentrant; the signal may arrive while main() prints. + os.write(sys.stdout.fileno(), b"got SIGTERM\n") + sys.exit(0) + + signal.signal(signal.SIGTERM, on_sigterm) + print(f"pid={os.getpid()}", flush=True) + print(f"zip_dir={__file__.split(os.sep + 'runfiles' + os.sep, 1)[0]}", flush=True) + while True: + time.sleep(60) + + +if __name__ == "__main__": + main() diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index b060daa7f9..b7fc550588 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -17,6 +17,15 @@ def _extract_root_from_output(output): raise AssertionError(f"Could not find file: line in output:\n{output}") +def _assert_removed_eventually(test, path): + # Shell launchers exec the app and leave cleanup to a detached watcher. + deadline = time.monotonic() + 15 + while os.path.exists(path): + if time.monotonic() >= deadline: + test.fail(f"Temporary zip extraction dir leaked after exit: {path}") + time.sleep(0.1) + + class SystemPythonZipAppTest(unittest.TestCase): def test_zipapp_runnable(self): zipapp_path = os.environ["TEST_ZIPAPP"] @@ -47,10 +56,7 @@ def test_zipapp_runnable(self): ) extract_root = _extract_root_from_output(output) - self.assertFalse( - os.path.exists(extract_root), - f"Temporary zip extraction dir leaked after exit: {extract_root}", - ) + _assert_removed_eventually(self, extract_root) @unittest.skipUnless(os.name == "posix", "POSIX signals are required") @@ -100,10 +106,7 @@ def test_zipapp_forwards_sigterm(self): output, _ = process.communicate(timeout=10) self.assertEqual(0, process.returncode, output) self.assertIn(f"received:{signal.SIGTERM}", output) - self.assertFalse( - os.path.exists(extract_root), - f"Temporary zip extraction dir leaked after handled SIGTERM: {extract_root}", - ) + _assert_removed_eventually(self, extract_root) finally: self.stop_process(process, application_pid) @@ -120,10 +123,7 @@ def test_zipapp_preserves_signal_termination(self): 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM ) self.assertEqual(expected, process.returncode, output) - self.assertFalse( - os.path.exists(extract_root), - f"Temporary zip extraction dir leaked after unhandled SIGTERM: {extract_root}", - ) + _assert_removed_eventually(self, extract_root) finally: self.stop_process(process, application_pid) @@ -157,15 +157,16 @@ def test_zipapp_preserves_extract_root_cache(self): ) def test_zipapp_preserves_sig_ign(self): + sighup = getattr(signal, "SIGHUP") 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, - preexec_fn=lambda: signal.signal(signal.SIGHUP, signal.SIG_IGN), + preexec_fn=lambda: signal.signal(sighup, signal.SIG_IGN), ) try: - process.send_signal(signal.SIGHUP) + process.send_signal(sighup) time.sleep(0.2) self.assertIsNone( process.poll(), @@ -195,6 +196,26 @@ def test_zipapp_preserves_nonzero_exit_status(self): ) self.assertEqual(17, process.returncode) + def test_zipapp_inherits_ignored_signals(self): + def ignore_signals(): + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signal.signal(getattr(signal, name), signal.SIG_IGN) + + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process = subprocess.run( + self.zipapp_command( + "check-ignored-signals", invoke_with_python=invoke_with_python + ), + preexec_fn=ignore_signals, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + timeout=10, + check=True, + ) + self.assertIn("ignored-signals:True", process.stdout) + if __name__ == "__main__": unittest.main() diff --git a/tests/py_zipapp/zipapp_signals_test.py b/tests/py_zipapp/zipapp_signals_test.py new file mode 100644 index 0000000000..e6221f39ed --- /dev/null +++ b/tests/py_zipapp/zipapp_signals_test.py @@ -0,0 +1,108 @@ +import os +import signal +import subprocess +import time +import unittest + +# How long to wait for the extracted files to be removed after the zipapp exits. +_CLEANUP_TIMEOUT_SECONDS = 15 + + +class ZipAppSignalsTest(unittest.TestCase): + def setUp(self): + self.zipapp_path = os.environ["TEST_ZIPAPP"] + + def _start(self, new_session=False): + proc = subprocess.Popen( + [self.zipapp_path], + stdout=subprocess.PIPE, + text=True, + start_new_session=new_session, + ) + self.addCleanup(self._kill, proc) + assert proc.stdout is not None + pid_line = proc.stdout.readline().strip() + zip_dir_line = proc.stdout.readline().strip() + if not pid_line.startswith("pid=") or not zip_dir_line.startswith("zip_dir="): + self.fail(f"unexpected zipapp output: {pid_line!r} {zip_dir_line!r}") + pid = int(pid_line.split("=", 1)[1]) + if pid != proc.pid: + # The launcher didn't exec, so the Python process can outlive it and + # keep stdout open. Make sure it's killed too. + self.addCleanup(self._kill_pid, pid) + zip_dir = zip_dir_line.split("=", 1)[1] + self.assertTrue(os.path.isdir(zip_dir), f"{zip_dir} does not exist") + return proc, pid, zip_dir + + def _kill(self, proc): + if proc.poll() is None: + proc.kill() + try: + proc.communicate(timeout=10) + except subprocess.TimeoutExpired: + pass + + def _kill_pid(self, pid): + try: + os.kill(pid, signal.SIGTERM) + except ProcessLookupError: + pass + + def assertRemovedEventually(self, path): + deadline = time.monotonic() + _CLEANUP_TIMEOUT_SECONDS + while os.path.exists(path): + if time.monotonic() > deadline: + self.fail(f"{path} was not removed after the zipapp exited") + time.sleep(0.2) + + def test_signal_reaches_python_process(self): + proc, pid, zip_dir = self._start() + # The launcher must exec the interpreter, so the PID the caller started + # is the Python process and signals sent to it are delivered there. + self.assertEqual(pid, proc.pid) + + proc.send_signal(signal.SIGTERM) + output, _ = proc.communicate(timeout=30) + + self.assertIn("got SIGTERM", output) + self.assertEqual(proc.returncode, 0) + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_after_sigkill(self): + proc, _, zip_dir = self._start() + + proc.kill() + proc.communicate(timeout=30) + + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_before_process_is_reaped(self): + proc, _, zip_dir = self._start() + + # Leave the exited process as a zombie: cleanup must not wait for the + # caller to reap it. + proc.kill() + + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_after_process_group_signal(self): + # A terminal's Ctrl-C (SIGINT), hangup (SIGHUP) or a supervisor stopping + # the whole process group (SIGTERM) signals every process in the group, + # including the cleanup watcher, which must survive to do its job. + for sig in (signal.SIGINT, signal.SIGHUP, signal.SIGTERM): + with self.subTest(signal=sig.name): + proc, _, zip_dir = self._start(new_session=True) + + os.killpg(proc.pid, sig) + proc.communicate(timeout=30) + + self.assertRemovedEventually(zip_dir) + + def test_exit_code_is_propagated(self): + result = subprocess.run([self.zipapp_path, "--exit-code=3"], timeout=60) + + self.assertEqual(result.returncode, 3) + + +if __name__ == "__main__": + unittest.main() From 6eb499af26757f71ad35ff7365bfa637c55c1f7d Mon Sep 17 00:00:00 2001 From: James Neufeld Date: Tue, 13 Oct 2026 21:49:13 -0700 Subject: [PATCH 6/6] fix(zipapp): remove read-only extracted files on Windows The new cleanup assertion exposed that archived read-only modes prevent Windows rmtree from deleting generated bootstrap and venv files. Retry deletion after clearing the read-only attribute without replacing the application exit status on other cleanup errors. Add an end-to-end fixture that creates a read-only file in the extracted runfiles. The focused zipapp test, Windows-stub type check, and changed-file hooks pass locally. --- python/private/zipapp/zip_main_template.py | 16 +++++++++++++++- tests/py_zipapp/main.py | 6 ++++++ tests/py_zipapp/system_python_zipapp_test.py | 8 ++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/python/private/zipapp/zip_main_template.py b/python/private/zipapp/zip_main_template.py index 4f366ecb58..c93f21b4f2 100644 --- a/python/private/zipapp/zip_main_template.py +++ b/python/private/zipapp/zip_main_template.py @@ -275,6 +275,16 @@ def forward_signal(signum, _frame): return ret_code +def remove_readonly(function, path, _exc_info): + # Windows won't unlink extracted files whose archived mode is read-only. + try: + os.chmod(path, stat.S_IWRITE) + function(path) + except OSError as error: + # Cleanup errors must not replace the application's exit status. + print_verbose("cleanup: failed to remove:", path, error) + + def execute_file( python_program, main_filename, @@ -325,7 +335,11 @@ def execute_file( # whole temporary directory. extract_root = dirname(runfiles_root) print_verbose("cleanup: rmtree: ", extract_root) - shutil.rmtree(extract_root, True) + if IS_WINDOWS: + # onerror is supported by older bootstrap interpreters too. + shutil.rmtree(extract_root, onerror=remove_readonly) + else: + shutil.rmtree(extract_root, True) def finish_venv_setup(runfiles_root): diff --git a/tests/py_zipapp/main.py b/tests/py_zipapp/main.py index d4170f3f92..595bed754e 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py @@ -2,6 +2,7 @@ import os import signal +import stat import sys @@ -20,6 +21,11 @@ def handle(signum, _frame): def main(): + if sys.argv[1:] == ["readonly-file"]: + path = os.path.join(os.path.dirname(__file__), "readonly-cleanup-test") + with open(path, "w") as file: + file.write("Cleanup must remove read-only extracted files.\n") + os.chmod(path, stat.S_IREAD) if sys.argv[1:] == ["check-ignored-signals"]: ignored = all( signal.getsignal(sig) == signal.SIG_IGN diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index b7fc550588..2ea85e8fc3 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -27,6 +27,14 @@ def _assert_removed_eventually(test, path): class SystemPythonZipAppTest(unittest.TestCase): + def test_zipapp_cleans_readonly_files(self): + output = subprocess.check_output( + [os.environ["TEST_ZIPAPP"], "readonly-file"], + stderr=subprocess.STDOUT, + text=True, + ) + _assert_removed_eventually(self, _extract_root_from_output(output)) + def test_zipapp_runnable(self): zipapp_path = os.environ["TEST_ZIPAPP"]