From c52b0c344f3f363990c63dd11534374ac02fd822 Mon Sep 17 00:00:00 2001 From: Frank Liu Date: Sat, 3 Oct 2026 23:25:48 +0000 Subject: [PATCH 1/3] fix(zipapp): exec the interpreter so signals reach the Python process The self-executable `py_zipapp_binary` launcher (`zip_shell_template.sh`) runs the interpreter as a child process. Signals sent to the PID the caller started, e.g. by a process supervisor stopping a long-running service, only reach the launcher shell, so the Python program never sees them. If the supervisor then escalates to `SIGKILL`, the shell dies, the Python program keeps running as an orphan (still holding its ports and stdio), and the `EXIT` trap that removes the temporary extraction directory never runs, leaking the whole extraction on every restart. This is the zipapp equivalent of #2043, which #2047 fixed for the non-zip bootstrap; the launcher still had a TODO pointing at it. To fix, `exec` the interpreter, as the non-zip bootstrap does. Because `exec` replaces the shell, the `EXIT` trap can no longer clean up the temporary directory, so just before `exec` the launcher starts a small detached watcher that removes the directory once the PID (now the Python process) exits. Unlike the trap, this also works after `SIGKILL`. The watcher is double-forked so it is not a child of the Python program (which could otherwise reap it via `os.wait()`), and its stdio is redirected so callers capturing the output (e.g. `$(zipapp)`) don't wait on it. The trap stays in place until the `exec`, so early failures still clean up. Nothing changes when `RULES_PYTHON_EXTRACT_ROOT` or `RULES_PYTHON_BOOTSTRAP_VERBOSE` is set; those directories are intentionally kept. * Adds `//tests/py_zipapp:zipapp_signals_test`, which checks that the started PID is the Python process, that `SIGTERM` reaches it, that the extraction directory is removed after both `SIGTERM` and `SIGKILL`, and that exit codes propagate. Against the previous launcher it fails (`39 != 35` for the PID check, and the `SIGKILL` case times out because the orphan keeps stdout open). --- news/zipapp-exec-signals.fixed.md | 6 ++ python/private/zipapp/zip_shell_template.sh | 30 ++++++-- tests/py_zipapp/BUILD.bazel | 27 +++++++ tests/py_zipapp/signals_main.py | 26 +++++++ tests/py_zipapp/zipapp_signals_test.py | 80 +++++++++++++++++++++ 5 files changed, 162 insertions(+), 7 deletions(-) create mode 100644 news/zipapp-exec-signals.fixed.md create mode 100644 tests/py_zipapp/signals_main.py create mode 100644 tests/py_zipapp/zipapp_signals_test.py diff --git a/news/zipapp-exec-signals.fixed.md b/news/zipapp-exec-signals.fixed.md new file mode 100644 index 0000000000..d4eafb2704 --- /dev/null +++ b/news/zipapp-exec-signals.fixed.md @@ -0,0 +1,6 @@ +(zipapp) The self-executable {obj}`py_zipapp_binary` launcher now `exec`s the +Python interpreter, so signals sent to the PID the caller started (e.g. by a +process supervisor) reach the Python program instead of only the launcher +shell. Its temporary extraction directory is now also removed when the program +is killed with `SIGKILL`. +([#2043](https://github.com/bazel-contrib/rules_python/issues/2043)) diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index d79331444a..54b3f62558 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 @@ -77,12 +79,26 @@ 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. +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. + trap - EXIT + launcher_pid=$$ + ( ( + while kill -0 "$launcher_pid" 2>/dev/null; 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. -"${command[@]}" -# Explicit exit is needed because the implicit next line the zip file this -# template is prepended to. -exit 0 +exec "${command[@]}" 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/signals_main.py b/tests/py_zipapp/signals_main.py new file mode 100644 index 0000000000..1c5be788c2 --- /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={sys._xoptions.get('RULES_PYTHON_ZIP_DIR', '')}", flush=True) + while True: + time.sleep(60) + + +if __name__ == "__main__": + 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..5ad26e22f3 --- /dev/null +++ b/tests/py_zipapp/zipapp_signals_test.py @@ -0,0 +1,80 @@ +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): + proc = subprocess.Popen([self.zipapp_path], stdout=subprocess.PIPE, text=True) + 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 the launcher doesn't exec, the Python process outlives it and keeps + # stdout open, so 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.SIGKILL) + 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_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 d9a6687f826ccc1f5b3fdb476f2cb7ee0fe9322c Mon Sep 17 00:00:00 2001 From: Frank Liu Date: Sat, 3 Oct 2026 23:54:23 +0000 Subject: [PATCH 2/3] fix(zipapp): don't let a zombie or reused PID delay cleanup The cleanup watcher polled `kill -0`, which still succeeds while the exited interpreter is a zombie and can't tell a reused PID from the original process, so cleanup could wait on the caller reaping it or on an unrelated process. Compare a process identity instead: the state and start time from /proc//stat (Linux) or `ps` (e.g. macOS), treating a zombie as exited. * Adds a test that the extraction directory is removed before the killed process is reaped. * The test only registers the fallback kill of the reported Python PID when it differs from the started PID, so it can't signal a reused PID. --- python/private/zipapp/zip_shell_template.sh | 27 ++++++++++++++++++++- tests/py_zipapp/zipapp_signals_test.py | 16 +++++++++--- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index 54b3f62558..882e2340db 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh @@ -79,6 +79,30 @@ 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 @@ -87,8 +111,9 @@ if [[ -n "$cleanup_zip_dir" ]]; then # Python process, and it doesn't hold this process's stdio open. trap - EXIT launcher_pid=$$ + launcher_identity=$(process_identity "$launcher_pid") ( ( - while kill -0 "$launcher_pid" 2>/dev/null; do + while [[ "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do sleep 1 done rm -fr "$zip_dir" diff --git a/tests/py_zipapp/zipapp_signals_test.py b/tests/py_zipapp/zipapp_signals_test.py index 5ad26e22f3..312d38936b 100644 --- a/tests/py_zipapp/zipapp_signals_test.py +++ b/tests/py_zipapp/zipapp_signals_test.py @@ -21,9 +21,10 @@ def _start(self): 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 the launcher doesn't exec, the Python process outlives it and keeps - # stdout open, so make sure it's killed too. - self.addCleanup(self._kill_pid, pid) + 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 @@ -70,6 +71,15 @@ def test_extracted_files_removed_after_sigkill(self): 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. + os.kill(proc.pid, signal.SIGKILL) + + self.assertRemovedEventually(zip_dir) + def test_exit_code_is_propagated(self): result = subprocess.run([self.zipapp_path, "--exit-code=3"], timeout=60) From 14ac260d3267ac67dd4f5c22165fa27130cc2444 Mon Sep 17 00:00:00 2001 From: Frank Liu Date: Sun, 4 Oct 2026 05:09:39 +0000 Subject: [PATCH 3/3] fix(zipapp): keep the cleanup watcher alive on process-group signals The watcher stays in the caller's process group, so a terminal's Ctrl-C (SIGINT), a hangup, or a supervisor signaling the whole group killed it along with the program and leaked the extraction directory. Bash's implicit SIGINT ignore for background jobs doesn't extend to the commands the watcher runs (`sleep`, command substitutions), and when those died the watcher exited too. Ignore INT, QUIT, HUP and TERM in the watcher with `trap ''`, which its commands inherit. It still exits on its own once the process is gone. * Adds a test that signals the whole process group with SIGINT, SIGHUP and SIGTERM and checks the extraction directory is removed. --- python/private/zipapp/zip_shell_template.sh | 7 ++++++- tests/py_zipapp/zipapp_signals_test.py | 22 +++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index 882e2340db..47f85d75d4 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh @@ -108,11 +108,16 @@ if [[ -n "$cleanup_zip_dir" ]]; then # 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. + # 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. trap - EXIT launcher_pid=$$ launcher_identity=$(process_identity "$launcher_pid") ( ( + trap '' INT QUIT HUP TERM while [[ "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do sleep 1 done diff --git a/tests/py_zipapp/zipapp_signals_test.py b/tests/py_zipapp/zipapp_signals_test.py index 312d38936b..4831c09150 100644 --- a/tests/py_zipapp/zipapp_signals_test.py +++ b/tests/py_zipapp/zipapp_signals_test.py @@ -12,8 +12,13 @@ class ZipAppSignalsTest(unittest.TestCase): def setUp(self): self.zipapp_path = os.environ["TEST_ZIPAPP"] - def _start(self): - proc = subprocess.Popen([self.zipapp_path], stdout=subprocess.PIPE, text=True) + 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() @@ -80,6 +85,19 @@ def test_extracted_files_removed_before_process_is_reaped(self): 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)