fix(zipapp): forward termination signals

.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

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))

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,45 @@ 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:
+      # 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) -> ...
@@ -535,8 +575,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:

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,7 +155,8 @@ 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
+  # 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 @@ 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[@]}")
@@ -325,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 >/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 @@ 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 $?

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 @@ 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:
+            # 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 @@ 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)
         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 @@ 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):

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,19 +72,62 @@ command=(
   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[@]}"

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"))