Improve SIGINT handling in `uv run` by zanieb · Pull Request #11009 · astral-sh/uv

zanieb · 2025-01-28T00:36:30Z

There should be two functional changes here:

If we receive SIGINT twice, forward it to the child process
If the uv run child process changes its PGID, then forward SIGINT

Previously, we never forwarded SIGINT to a child process. Instead, we
relied on shell to do so.

On Windows, we still do nothing but eat the Ctrl-C events we receive.
I cannot see an easy way to send them to the child.

The motivation for these changes should be explained in the comments.

Closes #10952 (in which Ray changes its PGID)
Replaces the (much simpler) #10989 with a more comprehensive approach.

See #6738 (comment) for some previous context.

zanieb · 2025-01-28T00:40:02Z

Frankly, I'm still not entirely sure why we wouldn't forward all signals in the same way that we do with SIGTERM. If it wasn't a huge pain, I would probably do it today? But since it is I might just wait until someone complains that something isn't working?

zanieb · 2025-01-28T16:05:22Z

A downside of forwarding Ctrl-C on the second send is that a process can receive it three times... but I don't know of programs that have special-casing for that.

zanieb · 2025-01-28T17:00:38Z

The first SIGINT is not forwarded (but is received because the shell sends it directly). The second SIGINT is forwarded

import signal, os; 

print(f'PID: {os.getpid()}');
print(f'GPID: {os.getpgid(os.getpid())}');

# We display a newline before SIGINT to separate from ^C in shells
signal.signal(signal.SIGINT, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
signal.signal(signal.SIGTERM, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))

signal.valid_signals

# Hang until we receive an input, e.g., Ctrl-D
try:
    input()
except EOFError:
    pass

❯ uv run -v example.py
DEBUG uv 0.5.24+8 (adf534009 2025-01-27)
DEBUG Found project root: `/Users/zb/workspace/uv`
DEBUG Project `uv` is marked as unmanaged
DEBUG No project found; searching for Python interpreter
DEBUG Reading Python requests from version file at `/Users/zb/workspace/uv/.python-version`
DEBUG Searching for Python 3.12 in virtual environments, managed installations, or search path
DEBUG Found `cpython-3.12.8-macos-aarch64-none` at `/Users/zb/workspace/uv/.venv/bin/python3` (virtual environment)
DEBUG Using Python 3.12.8 interpreter at: /Users/zb/workspace/uv/.venv/bin/python3
DEBUG Running `python example.py`
PID: 84848
GPID: 84847
^CSIGINT
DEBUG Received SIGINT, assuming the child received it as part of the process group
^CSIGINT
DEBUG Received SIGINT, forwarding to child at 84848
SIGINT

SIGTERM is forwarded immediately (and only received once)

❯ uv run -v example.py
DEBUG uv 0.5.24+8 (adf534009 2025-01-27)
DEBUG Found project root: `/Users/zb/workspace/uv`
DEBUG Project `uv` is marked as unmanaged
DEBUG No project found; searching for Python interpreter
DEBUG Reading Python requests from version file at `/Users/zb/workspace/uv/.python-version`
DEBUG Searching for Python 3.12 in virtual environments, managed installations, or search path
DEBUG Found `cpython-3.12.8-macos-aarch64-none` at `/Users/zb/workspace/uv/.venv/bin/python3` (virtual environment)
DEBUG Using Python 3.12.8 interpreter at: /Users/zb/workspace/uv/.venv/bin/python3
DEBUG Running `python example.py`
PID: 85335
GPID: 85334
DEBUG Received SIGTERM, forwarding to child at 85335
SIGTERM

When the PGID of the child is changed, we forward the first SIGINT

import signal, os; 

print(f'PID: {os.getpid()}');

# Change PGID
os.setpgid(0, 0)

print(f'GPID: {os.getpgid(os.getpid())}');

signal.signal(signal.SIGINT, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
signal.signal(signal.SIGTERM, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))

# Sleep for 20s; we can't use `input()` or it breaks output for some reason
try:
    import time
    time.sleep(20)
except EOFError:
    pass

❯ uv run -v example.py
DEBUG uv 0.5.24+8 (adf534009 2025-01-27)
DEBUG Found project root: `/Users/zb/workspace/uv`
DEBUG Project `uv` is marked as unmanaged
DEBUG No project found; searching for Python interpreter
DEBUG Reading Python requests from version file at `/Users/zb/workspace/uv/.python-version`
DEBUG Searching for Python 3.12 in virtual environments, managed installations, or search path
DEBUG Found `cpython-3.12.8-macos-aarch64-none` at `/Users/zb/workspace/uv/.venv/bin/python3` (virtual environment)
DEBUG Using Python 3.12.8 interpreter at: /Users/zb/workspace/uv/.venv/bin/python3
DEBUG Running `python example.py`
PID: 84317
GPID: 84317
^CDEBUG Received SIGINT, forwarding to child at 84317
SIGINT

zanieb · 2025-01-28T17:38:41Z

Test coverage pretty painful to add here.

There should be two functional changes here: - If we receive SIGINT twice, forward it to the child process - If the `uv run` child process changes its PGID, then forward SIGINT Previously, we never forwarded SIGINT to a child process. Instead, we relied on shell to do so.

BurntSushi

Nice comment. I buy this. I'm unsure about Windows though.

BurntSushi · 2025-01-28T19:44:22Z

crates/uv/src/commands/run.rs

+    // signals which has semantic meaning for some programs, i.e., slow exit on the first signal and
+    // fast exit on the second. The exception to this is if a child process changes its process
+    // group, in which case the shell will _not_ send SIGINT to the child process and uv must take
+    // ownership of forwarding the signal.


Ah now I get it. Thanks for explaining this here!

BurntSushi · 2025-01-28T19:47:49Z

crates/uv/src/commands/run.rs

+                    }
+
+                    debug!("Received SIGINT, forwarding to child at {child_pid}");
+                    let _ = signal::kill(child_pid, signal::Signal::SIGINT);


Does it make sense to log an error here if one occurs?

Probably yeah. This is the existing behavior so I prefer to retain but we could add logging in a separate patch.

BurntSushi · 2025-01-28T19:49:47Z

crates/uv/src/commands/run.rs

    }?;

+    // On Windows, we just ignore the console CTRL_C_EVENT and assume it will always be sent to the
+    // child by the console. There's not a clear programmatic way to forward the signal anyway.


Does the signal actually get sent to the child? Or maybe my general question here is, will ^C still work to terminate the child?

Yeah so this should retain our existing behavior — it's just moved from a platform-wide block to a Windows-specific block. The note is just that we can't implement the more complicated behavior we do above for Unix.

I also had Aria test this assumption in PowerShell and the initial change adding the Ctrl-C handling fixed various bugs.

Ah nice! On the ball.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.24` -> `0.5.25` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.25`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0525) [Compare Source](astral-sh/uv@0.5.24...0.5.25) ##### Enhancements - Allow installation of manylinux wheels on loongarch64 ([#10927](astral-sh/uv#10927)) - Allow optional `=` for editables in `requirements.txt` ([#10954](astral-sh/uv#10954)) - Add Windows aarch64 to the release binaries ([#10885](astral-sh/uv#10885)) ##### Bug fixes - Use spec-compliant (`128+n`) exit codes for `uv run` and `uv tool run` on Unix ([#10781](astral-sh/uv#10781)) - Fix best-interpreter lookups when there is an invalid interpreter in the `PATH` ([#11030](astral-sh/uv#11030)) - Guard against concurrent cache writes on Windows ([#11007](astral-sh/uv#11007)) - Prioritize package preferences with greater package versions ([#10963](astral-sh/uv#10963)) - Reject `--editable` flag on non-directory requirements ([#10994](astral-sh/uv#10994)) - Respect `--no-sources` for `uv pip install` workspace discovery ([#11003](astral-sh/uv#11003)) - Set `JEMALLOC_SYS_WITH_LG_PAGE=16` in ARM Docker builds ([#10943](astral-sh/uv#10943)) - Update `riscv64` Python downloads to allow install on `riscv64gc` ([#10937](astral-sh/uv#10937)) - Fix file persist retries on Windows ([#11008](astral-sh/uv#11008)) - Fix incorrect error message when specifying `tool.uv.sources.(package).workspace` with other options ([#11013](astral-sh/uv#11013)) - Improve SIGINT handling in `uv run` ([#11009](astral-sh/uv#11009)) ##### Documentation - Add `SECURITY` policy ([#11035](astral-sh/uv#11035)) - Add `Requires-Python` upper bound behavior to the docs ([#10964](astral-sh/uv#10964)) - Add a troubleshooting section and reproducible example guide ([#10947](astral-sh/uv#10947)) - Add documentation for `uv add -r` ([#10926](astral-sh/uv#10926)) - Amend `requires-python` rules in resolver documentation ([#10993](astral-sh/uv#10993)) - Reference workspaces in `--no-sources` documentation ([#10995](astral-sh/uv#10995)) - Update documentation for activating virtual environments in different shell ([#11000](astral-sh/uv#11000)) - Add Docker SHA pinning tip ([#10955](astral-sh/uv#10955)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

* main: (53 commits) Shorten "Using existing Python versions" nav item so it fits on one line (astral-sh#11077) docs: suggest copy linking for GitLab integration guide (astral-sh#11067) Refactor `uv tool run` hint into separate function (astral-sh#11069) Fix typo in no-deps docs/comments/cli description (astral-sh#11073) Allow `--no-dev --invert` in `uv tree` (astral-sh#11068) Add docs for signal handling (astral-sh#11041) Add a bit more context about SIGTERM and PID 1 (astral-sh#11036) Reflow CLI documentation comments (astral-sh#11040) doc typo: unnecessary backslashes to represent brackets in markdown (astral-sh#11059) Update Dependabot links (astral-sh#11054) Document `gather_credentials` (astral-sh#11024) Link to our MRE documentation in the issue template (astral-sh#11045) Avoid sharing state between universal and non-universal resolves (astral-sh#11051) Mark metadata as dynamic when reading from built wheel cache (astral-sh#11046) Fix formatting of `RUST_LOG` documentation (astral-sh#10053) Bump version to 0.5.25 (astral-sh#11042) Add CVE disclosure to security policy (astral-sh#11037) Guard against concurrent cache writes on Windows (astral-sh#11007) Add SECURITY policy (astral-sh#11035) Improve SIGINT handling in `uv run` (astral-sh#11009) ...

zanieb force-pushed the zb/signals branch from d332b40 to adf5340 Compare January 28, 2025 01:43

zanieb marked this pull request as ready for review January 28, 2025 17:16

zanieb force-pushed the zb/signals branch 2 times, most recently from 46cba66 to 7ea0bdf Compare January 28, 2025 17:38

zanieb requested review from Gankra and charliermarsh January 28, 2025 18:32

zanieb force-pushed the zb/signals branch from 7ea0bdf to 303c91f Compare January 28, 2025 18:32

zanieb added 2 commits January 28, 2025 13:21

Refactor a litte

4d0814f

zanieb force-pushed the zb/signals branch from 303c91f to 4d0814f Compare January 28, 2025 19:21

BurntSushi approved these changes Jan 28, 2025

View reviewed changes

zanieb merged commit fe6126a into main Jan 28, 2025
73 checks passed

zanieb deleted the zb/signals branch January 28, 2025 20:00

BrewTestBot mentioned this pull request Jan 28, 2025

uv 0.5.25 Homebrew/homebrew-core#205828

Merged

zanieb mentioned this pull request Feb 18, 2025

Consider execv for uv run on Unix #3095

Open

zanieb mentioned this pull request Mar 11, 2025

"uv tool run" swallows SIGINT without forwarding to the underlying process #12108

Closed

amotl mentioned this pull request Apr 3, 2025

uv run not running to completion #12658

Open

zanieb mentioned this pull request Jul 18, 2025

Elide traceback when python -m uv in interrupted with Ctrl-C on Windows #14715

Merged

Comments

Conversation

zanieb commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented Jan 28, 2025

Uh oh!

zanieb commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented Jan 28, 2025

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

BurntSushi Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

BurntSushi Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

BurntSushi Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

BurntSushi Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zanieb commented Jan 28, 2025 •

edited

Loading

zanieb commented Jan 28, 2025 •

edited

Loading

zanieb commented Jan 28, 2025 •

edited

Loading