fix(zipapp): forward termination signals (#4161) zip launchers can intercept termination signals, leaving the Python application running as an orphan. This change uses `exec` for POSIX shell launchers and signal forwarding for POSIX Python launchers, while preserving exit status and extraction cleanup. Covers executable zipapps, `python app.pyz`, and both legacy zip bootstrap implementations. Summary * Apply Frank Liu's #4210 cleanup watcher to both shell ZIP launchers. It removes temporary extractions after exit or signals, including SIGKILL and before zombie reaping. Keep EXIT traps for startup failures and retain persistent caches and verbose-mode shell extractions. * Forward SIGHUP, SIGINT, SIGQUIT, and SIGTERM from Python launchers while preserving inherited `SIG_IGN`. Keep `Popen.wait()` as the sole child reaper. * Remove the unused `RULES_PYTHON_ZIP_DIR` interpreter option; launcher parents and watchers own cleanup. On Windows, clear read-only attributes and retry deletion without replacing the application's exit status on cleanup errors. Fixes #3809 Builds on #4210 --------- Co-authored-by: Richard Levasseur <richardlev@gmail.com> Co-authored-by: Frank Liu <gfrankliu@gmail.com>
diff --git a/.bazelrc.deleted_packages b/.bazelrc.deleted_packages index 5654df1..3eec282 100644 --- a/.bazelrc.deleted_packages +++ b/.bazelrc.deleted_packages
@@ -28,8 +28,8 @@ 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 new file mode 100644 index 0000000..f8c6d09 --- /dev/null +++ b/news/3809.fixed.md
@@ -0,0 +1,3 @@ +(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 ee5aec9..d3f26bb 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt
@@ -28,6 +28,7 @@ from os.path import abspath, dirname, join, basename, normpath import os +import signal import shutil import subprocess @@ -497,6 +498,45 @@ 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: + # 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) + 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) -> ... @@ -544,8 +584,7 @@ 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 4374ff9..ef09812 100644 --- a/python/private/stage1_bootstrap_template.sh +++ b/python/private/stage1_bootstrap_template.sh
@@ -60,12 +60,14 @@ %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,7 +155,8 @@ # 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 + # 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. if [[ "$python_exe" != $RUNFILES_DIR/* ]]; then @@ -297,10 +300,6 @@ 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[@]}") @@ -325,6 +324,52 @@ "$@" ) +# 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 >/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 @@ -333,8 +378,8 @@ # 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 709e088..c93f21b 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,56 @@ 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: + # 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) + 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 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, @@ -269,14 +320,12 @@ # 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) 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: @@ -286,7 +335,11 @@ # 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/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index d793314..f333765 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh
@@ -35,6 +35,7 @@ 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 @@ # 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,19 +72,62 @@ env "${interpreter_env[@]}" "$python_exe" - "-XRULES_PYTHON_ZIP_DIR=$zip_dir" "${interpreter_args[@]}" "${INTERPRETER_ARGS_FROM_TARGET[@]}" "$RUNFILES_DIR/$STAGE2_BOOTSTRAP" "$@" ) -# 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. +# 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 >/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/bootstrap_impls/bin.py b/tests/bootstrap_impls/bin.py index 0713b5f..7ebe0c0 100644 --- a/tests/bootstrap_impls/bin.py +++ b/tests/bootstrap_impls/bin.py
@@ -13,8 +13,22 @@ # 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"file: {__file__}", flush=True) + 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 77fe4d3..43afc5f 100755 --- a/tests/bootstrap_impls/run_binary_zip_yes_test.sh +++ b/tests/bootstrap_impls/run_binary_zip_yes_test.sh
@@ -33,12 +33,98 @@ # How we detect if a zip file was executed from depends on which bootstrap # is used. -# bootstrap_impl=script outputs RULES_PYTHON_ZIP_DIR:<somepath> +# bootstrap_impl=script outputs file:<somepath>/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" exit 1 fi +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/*}" +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="" +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" + + # 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 + break + fi + sleep 0.1 + done + 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 + 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 + local extracted_file + extracted_file=$(sed -n 's/^file: //p' "${log}") + assert_removed_eventually "${extracted_file%%/runfiles/*}" || return 1 + 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/BUILD.bazel b/tests/py_zipapp/BUILD.bazel index e0f878c..ede19ba 100644 --- a/tests/py_zipapp/BUILD.bazel +++ b/tests/py_zipapp/BUILD.bazel
@@ -4,6 +4,7 @@ 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 @@ 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 5770170..595bed7 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py
@@ -1,8 +1,48 @@ -"A trivial zipapp that prints a message" +"A trivial zipapp that prints a message or waits for a signal." + +import os +import signal +import stat +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"file:{__file__}", flush=True) + print(f"ready:{os.getpid()}", flush=True) + signal.pause() + return 0 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 + 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"]: + 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") + print(f"file:{__file__}") + print(f"xoption_zip_dir:{sys._xoptions.get('RULES_PYTHON_ZIP_DIR', 'UNSET')}") try: import some_dep @@ -12,13 +52,12 @@ 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/signals_main.py b/tests/py_zipapp/signals_main.py new file mode 100644 index 0000000..2fc519a --- /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 7c3e2de..2ea85e8 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py
@@ -1,9 +1,40 @@ import os +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}") + + +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_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"] @@ -26,6 +57,172 @@ ) 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) + _assert_removed_eventually(self, extract_root) + + +@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] + if invoke_with_python: + command.insert(0, sys.executable) + return [*command, *args] + + 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:")), extract_root + + def stop_process(self, process, application_pid): + if process.poll() is None: + process.kill() + process.wait() + try: + os.kill(application_pid, signal.SIGTERM) + except ProcessLookupError: + pass + if process.stdout is not None: + process.stdout.close() + + 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, extract_root = 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) + _assert_removed_eventually(self, 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, extract_root = 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) + _assert_removed_eventually(self, 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): + 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(sighup, signal.SIG_IGN), + ) + try: + process.send_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) + + 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) + + 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__":
diff --git a/tests/py_zipapp/zipapp_signals_test.py b/tests/py_zipapp/zipapp_signals_test.py new file mode 100644 index 0000000..e6221f3 --- /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()