From 61e61a59eb99affecc383f18250928c0eb76f21f Mon Sep 17 00:00:00 2001 From: Thiago Kenji Okada Date: Thu, 1 May 2025 21:32:10 +0100 Subject: [PATCH 1/4] nixos-rebuild-ng: kill underlying remote process `nixos-rebuild-ng` explicitly don't allocate a pseudo-TTY for SSH because this causes lots of issues depending on the use case (for example, multiplexing multiple SSH sessions). Sadly not using a pseudo-TTY also cause other issues, like the fact that using Ctrl+C (SIGINT) doesn't kill the underlying process because SSH doesn't support it. We can't really start using pseudo-TTY unless we want to overcomplicate the code for parsing results (pseudo-TTY mangles the stdout/stderr together), so we need to handle killing the underlying remote process manually. This is what this commit does, when we receive a `KeyboardInterrupt` exception while calling `run_wrapper`, we will check if it is a remote process and send a `pkill --full` with the arguments (this should ensure that we don't kill other process, but we can't guarantee it). This assumes the user has `procps` installed, but I think it is a safe assumption since this seems to be a core package. Sadly nothing we can do if the user doesn't have `procps` installed, the good thing is that the worst that can happen is that we will silent fail and the process will stay in background until it finishes. Fix #403269. --- .../src/nixos_rebuild/process.py | 67 ++++++++++++++++--- .../src/tests/test_process.py | 31 +++++++++ 2 files changed, 90 insertions(+), 8 deletions(-) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py index de37bd615512..b21010f54f40 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py @@ -1,6 +1,7 @@ import atexit import logging import os +import re import shlex import subprocess from collections.abc import Sequence @@ -21,6 +22,8 @@ SSH_DEFAULT_OPTS: Final = [ "ControlPersist=60", ] +type Args = Sequence[str | bytes | os.PathLike[str] | os.PathLike[bytes]] + @dataclass(frozen=True) class Remote: @@ -82,7 +85,7 @@ atexit.register(cleanup_ssh) def run_wrapper( - args: Sequence[str | bytes | os.PathLike[str] | os.PathLike[bytes]], + args: Args, *, check: bool = True, extra_env: dict[str, str] | None = None, @@ -93,6 +96,8 @@ def run_wrapper( "Wrapper around `subprocess.run` that supports extra functionality." env = None process_input = None + run_args = args + if remote: if extra_env: extra_env_args = [f"{env}={value}" for env, value in extra_env.items()] @@ -103,7 +108,7 @@ def run_wrapper( process_input = remote.sudo_password + "\n" else: args = ["sudo", *args] - args = [ + run_args = [ "ssh", *remote.opts, *SSH_DEFAULT_OPTS, @@ -119,32 +124,39 @@ def run_wrapper( if extra_env: env = os.environ | extra_env if sudo: - args = ["sudo", *args] + run_args = ["sudo", *run_args] logger.debug( "calling run with args=%r, kwargs=%r, extra_env=%r", - args, + run_args, kwargs, extra_env, ) try: r = subprocess.run( - args, + run_args, check=check, env=env, input=process_input, - # Hope nobody is using NixOS with non-UTF8 encodings, but "surrogateescape" - # should still work in those systems. + # Hope nobody is using NixOS with non-UTF8 encodings, but + # "surrogateescape" should still work in those systems. text=True, errors="surrogateescape", **kwargs, ) if kwargs.get("capture_output") or kwargs.get("stderr") or kwargs.get("stdout"): - logger.debug("captured output stdout=%r, stderr=%r", r.stdout, r.stderr) + logger.debug( + "captured output with stdout=%r, stderr=%r", r.stdout, r.stderr + ) return r + except KeyboardInterrupt: + # sudo commands are activation only and unlikely to be long running + if remote and not sudo: + _kill_long_running_ssh_process(args, remote) + raise except subprocess.CalledProcessError: if sudo and remote and remote.sudo_password is None: logger.error( @@ -152,3 +164,42 @@ def run_wrapper( + "--ask-sudo-password?" ) raise + + +# SSH does not send the signals to the process when running without usage of +# pseudo-TTY (that causes a whole other can of worms), so if the process is +# long running (e.g.: a build) this will result in the underlying process +# staying alive. +# See: https://stackoverflow.com/a/44354466 +# Issue: https://github.com/NixOS/nixpkgs/issues/403269 +def _kill_long_running_ssh_process(args: Args, remote: Remote) -> None: + logger.info("cleaning-up remote process, please wait...") + + # We need to escape both the shell and regex here (since pkill interprets + # its arguments as regex) + quoted_args = re.escape(shlex.join(str(a) for a in args)) + logger.debug("killing remote process using pkill with args=%r", quoted_args) + r = subprocess.run( + [ + "ssh", + *remote.opts, + *SSH_DEFAULT_OPTS, + remote.host, + "--", + "pkill", + "--signal", + "SIGINT", + "--full", + "--", + quoted_args, + ], + check=False, + capture_output=True, + text=True, + ) + logger.debug( + "remote pkill captured output with stdout=%r, stderr=%r, returncode=%s", + r.stdout, + r.stderr, + r.returncode, + ) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_process.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_process.py index 6458f54c06ef..8ac92bb4848b 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_process.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_process.py @@ -96,6 +96,37 @@ def test_run(mock_run: Any) -> None: ) +@patch(get_qualified_name(p.subprocess.run), autospec=True) +def test__kill_long_running_ssh_process(mock_run: Any) -> None: + p._kill_long_running_ssh_process( + [ + "nix", + "--extra-experimental-features", + "nix-command flakes", + "build", + "/nix/store/la0c8nmpr9xfclla0n4f3qq9iwgdrq4g-nixos-system-sankyuu-nixos-25.05.20250424.f771eb4.drv^*", + ], + m.Remote("user@localhost", opts=[], sudo_password=None), + ) + mock_run.assert_called_with( + [ + "ssh", + *p.SSH_DEFAULT_OPTS, + "user@localhost", + "--", + "pkill", + "--signal", + "SIGINT", + "--full", + "--", + r"nix\ \-\-extra\-experimental\-features\ 'nix\-command\ flakes'\ build\ '/nix/store/la0c8nmpr9xfclla0n4f3qq9iwgdrq4g\-nixos\-system\-sankyuu\-nixos\-25\.05\.20250424\.f771eb4\.drv\^\*'", + ], + check=False, + capture_output=True, + text=True, + ) + + def test_remote_from_name(monkeypatch: MonkeyPatch) -> None: monkeypatch.setenv("NIX_SSHOPTS", "") assert m.Remote.from_arg("user@localhost", None, False) == m.Remote( From 062eaf7379974675dc6c00445f29ce2a4753db0b Mon Sep 17 00:00:00 2001 From: Thiago Kenji Okada Date: Fri, 2 May 2025 09:48:51 +0100 Subject: [PATCH 2/4] nixos-rebuild-ng: alert user if we can't clean-up remote process --- .../src/nixos_rebuild/process.py | 61 +++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py index b21010f54f40..d6d798fdc62d 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py @@ -179,27 +179,40 @@ def _kill_long_running_ssh_process(args: Args, remote: Remote) -> None: # its arguments as regex) quoted_args = re.escape(shlex.join(str(a) for a in args)) logger.debug("killing remote process using pkill with args=%r", quoted_args) - r = subprocess.run( - [ - "ssh", - *remote.opts, - *SSH_DEFAULT_OPTS, - remote.host, - "--", - "pkill", - "--signal", - "SIGINT", - "--full", - "--", - quoted_args, - ], - check=False, - capture_output=True, - text=True, - ) - logger.debug( - "remote pkill captured output with stdout=%r, stderr=%r, returncode=%s", - r.stdout, - r.stderr, - r.returncode, - ) + cleanup_interrupted = False + + try: + r = subprocess.run( + [ + "ssh", + *remote.opts, + *SSH_DEFAULT_OPTS, + remote.host, + "--", + "pkill", + "--signal", + "SIGINT", + "--full", + "--", + quoted_args, + ], + check=False, + capture_output=True, + text=True, + ) + logger.debug( + "remote pkill captured output with stdout=%r, stderr=%r, returncode=%s", + r.stdout, + r.stderr, + r.returncode, + ) + except KeyboardInterrupt: + cleanup_interrupted = True + raise + finally: + if cleanup_interrupted or r.returncode: + logger.warning( + "could not clean-up remote process, the command %s may still be running in host '%s'", + args, + remote.host, + ) From 2e06b6da564f481031f4a5045a637673effcd87f Mon Sep 17 00:00:00 2001 From: Thiago Kenji Okada Date: Fri, 2 May 2025 12:56:07 +0100 Subject: [PATCH 3/4] nixos-rebuild-ng: mark logger as Final --- .../by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py | 4 ++-- pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py | 2 +- pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py index 173462767b05..fd03e5b69ef4 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/__init__.py @@ -5,7 +5,7 @@ import os import sys from pathlib import Path from subprocess import CalledProcessError, run -from typing import assert_never +from typing import Final, assert_never from . import nix, tmpdir from .constants import EXECUTABLE, WITH_NIX_2_18, WITH_REEXEC, WITH_SHELL_FILES @@ -13,7 +13,7 @@ from .models import Action, BuildAttr, Flake, ImageVariants, NRError, Profile from .process import Remote, cleanup_ssh from .utils import Args, LogFormatter, tabulate -logger = logging.getLogger() +logger: Final = logging.getLogger() logger.setLevel(logging.INFO) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py index 8ea339e7b064..0929cf7fe4aa 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py @@ -45,7 +45,7 @@ SWITCH_TO_CONFIGURATION_CMD_PREFIX: Final = [ "--service-type=exec", "--unit=nixos-rebuild-switch-to-configuration", ] -logger = logging.getLogger(__name__) +logger: Final = logging.getLogger(__name__) def build( diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py index d6d798fdc62d..0c556411331e 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py @@ -11,7 +11,7 @@ from typing import Final, Self, TypedDict, Unpack from . import tmpdir -logger = logging.getLogger(__name__) +logger: Final = logging.getLogger(__name__) SSH_DEFAULT_OPTS: Final = [ "-o", From b74e861c28643c55b7cc4bf909cd91cb907bb790 Mon Sep 17 00:00:00 2001 From: Thiago Kenji Okada Date: Fri, 2 May 2025 12:58:33 +0100 Subject: [PATCH 4/4] nixos-rebuild-ng: use Final in constants.py --- .../ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py index f879130f0336..ef706af7b342 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/constants.py @@ -1,9 +1,11 @@ +from typing import Final + # Build-time flags # Use strings to avoid breaking standalone (e.g.: `python -m nixos_rebuild`) # usage -EXECUTABLE = "@executable@" +EXECUTABLE: Final[str] = "@executable@" # Use either `== "true"` if the default (e.g.: `python -m nixos_rebuild`) is # `False` or `!= "false"` if the default is `True` -WITH_NIX_2_18 = "@withNix218@" != "false" # type: ignore -WITH_REEXEC = "@withReexec@" == "true" # type: ignore -WITH_SHELL_FILES = "@withShellFiles@" == "true" # type: ignore +WITH_NIX_2_18: Final[bool] = "@withNix218@" != "false" +WITH_REEXEC: Final[bool] = "@withReexec@" == "true" +WITH_SHELL_FILES: Final[bool] = "@withShellFiles@" == "true"