Don't panic on Ctrl-C in confirm prompt by ericmarkmartin · Pull Request #11706 · astral-sh/uv

ericmarkmartin · 2025-02-22T07:23:50Z

Summary

Resolves #11704

Propagate errors from uv_console::confirm up instead of unwrapping them, causing panics.

Test Plan

Regression testing the bug is very difficult, as the behavior of confirm changes based on whether uv is talking to a tty. We can trick it using ptys, but the best rust pty crate I could find only provides blocking reads of the spawned child, which is insufficient to write the regression test.

ericmarkmartin · 2025-02-22T08:04:33Z

Took a brief stab at testing, but failed. Not sure how to test the behavior when we need the subprocess to be interactive in order to see the confirmation prompt. For reference, this is what I attempted to no avail (the snapshot looks like what happens if you answer the confirmation prompt with no).

#[test]
fn ctrl_c_install_confirmation_prompt() -> Result<()> {
    let context = TestContext::new("3.12");

    context.temp_dir.child("pyproject.toml").touch()?;

    let ctrl_c_file = context.temp_dir.child("ctrl_c");

    ctrl_c_file.write_binary(&[0x03])?;

    let file_handle = std::fs::File::open(ctrl_c_file)?;

    uv_snapshot!(context.filters(), context.pip_install()
        .arg("pyproject.toml") // triggers confirmation prompt because user probably wants `-r`
        .stdin(file_handle), @r###"
        "###
    );

    Ok(())
}

ericmarkmartin

I've added a test that seems to work using pty, which I get out of wezterm's pty implementation. One thing to note is that the read out of the pty is non-blocking so if our confirmation prompt changes and becomes smaller, the test will hang, which isn't great. Still thinking of ways to fix that.

Part of making the test workable was a drive-by change so that the confirmation prompt respects the global color setting

ericmarkmartin · 2025-02-24T03:36:36Z

Apparently the fix doesn't work on windows, so converting back to draft while I workshop a bit

konstin · 2025-02-24T14:56:31Z

Can you try forwarding the io error? The handler seems working, but i think the main thread panics too fast.

ericmarkmartin · 2025-02-24T19:02:22Z

Can you try forwarding the io error? The handler seems working, but i think the main thread panics too fast.

Forwarding it out of where? I think the main thread panics because we forward it out of the confirm function and unwrap the containing Result, no?

konstin · 2025-02-25T13:18:27Z

Besides the behavior changes to signal handling, we would ideally not have an .unwrap() call at all, usually we can just propagate the error to the caller through a thiserror derive enum variant.

ericmarkmartin · 2025-02-26T05:20:55Z

Ah okay, that makes sense. I've rolled back the signal handling changes because it requires some changes to the console crate's windows implementation, which I'll try to PR separately but it probably shouldn't block this.

Do you have thoughts on adding back the test with the pseudoterminal? My inclination is that it's not worth it given it's proclivity to hanging

ericmarkmartin · 2025-02-26T06:34:19Z

which I'll try to PR separately

see console-rs/console#235

konstin · 2025-02-26T09:52:21Z

Thanks for making the upstream PR!

I think it is fine this way, it's clear the panic cannot occur anymore given that the unwrap() is gone.

I've changed the error handling in uv/src/lib.rs to forward the io::Error if one occurs.

## Summary  Resolves astral-sh#11704 Propagate errors from `uv_console::confirm` up instead of `unwrap`ping them, causing panics. ## Test Plan  Regression testing the bug is very difficult, as the behavior of `confirm` changes based on whether `uv` is talking to a `tty`. We can trick it using ptys, but the best rust pty crate I could find only provides blocking reads of the spawned child, which is insufficient to write the regression test. --------- Co-authored-by: konstin <[email protected]>

…dler (#11897)  ## Summary Follow on to #11706. In the original PR, I tried to solve the issue by getting rid of the `ctrlc::set_handler` call. Unfortunately, this didn't work on windows due to an issue with the console crate. console 0.15.11 includes console-rs/console#235, which resolves the issue, so now we can get rid of the call.  This change is not super important but I still think it's worthwhile. For one, spinning up a background thread to handle `SIGINT`s when we're going to be raising the `SIGINT` from within the function is more technical complexity than needed, now that there's an easy way to explicitly catch the Ctrl-C from the terminal input. Secondly, `ctrlc::set_handler`'s [docs](https://docs.rs/ctrlc/3.4.5/ctrlc/fn.set_handler.html) advise that you set the handler just once, at the beginning of the program, so this use seems somewhat error prone. In fact, uv already has a second [callsite](https://github.com/astral-sh/uv/blob/461f4d9007160f7061a4fc0c4a5a84c613fdbff7/crates/uv/src/commands/project/add.rs#L596-L611) for this function (though I'm not sure if the two callsites could currently ever both occur on the same run of uv) ## Test Plan I've tested this manually on linux (WSL ubuntu) and windows, though not on aarch64-apple-darwin as I don't have a machine running that. I would appreciate if someone would double-check that it works on such machines. As discussed in the original PR, this change is pretty hard to test due to the fact that the behavior only occurs if stderr is connected to a tty. I experimented with using pseudoterminals to test this but it's still quite tricky due to the lack of x-platform non-blocking reads on the pty.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.6.3` -> `0.6.4` | 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.6.4`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#064) [Compare Source](astral-sh/uv@0.6.3...0.6.4) ##### Enhancements - Upgrade pypy3.10 to v7.3.19 ([#11814](astral-sh/uv#11814)) - Allow configuring log verbosity from the CLI (i.e., `-vvv`) ([#11758](astral-sh/uv#11758)) - Warn when duplicate index names found in single file ([#11824](astral-sh/uv#11824)) ##### Bug fixes - Always store registry index on resolution packages ([#11815](astral-sh/uv#11815)) - Avoid error on relative paths in `uv tool uninstall` ([#11889](astral-sh/uv#11889)) - Avoid silently dropping errors in directory enumeration ([#11890](astral-sh/uv#11890)) - Disable interactive git terminal prompts during fetches ([#11744](astral-sh/uv#11744)) - Discover Windows registry (PEP 514) Python versions across 32/64-bit ([#11801](astral-sh/uv#11801)) - Don't panic on Ctrl-C in confirm prompt ([#11706](astral-sh/uv#11706)) - Fix non-directory in workspace on Windows ([#11833](astral-sh/uv#11833)) - Make interpreter caching robust to OS upgrades ([#11875](astral-sh/uv#11875)) - Respect `include-system-site-packages` in layered environments ([#11873](astral-sh/uv#11873)) - Suggest `uv tool update-shell` in PowerShell ([#11846](astral-sh/uv#11846)) - Update code page to `65001` before setting environment variables in virtual environments ([#11831](astral-sh/uv#11831)) - Use hash instead of full wheel name in wheels bucket ([#11738](astral-sh/uv#11738)) - Fix version string truncation while generating cache_key ([#11830](astral-sh/uv#11830)) - Explicitly handle ctrl-c in confirmation prompt instead of using a signal handler ([#11897](astral-sh/uv#11897)) ##### Performance - Avoid cloning to string when creating cache path ([#11772](astral-sh/uv#11772)) - Avoid redundant clones in version containment check ([#11767](astral-sh/uv#11767)) - Avoid string allocation when enumerating tool names ([#11910](astral-sh/uv#11910)) - Avoid using owned `String` for package name constructors ([#11768](astral-sh/uv#11768)) - Avoid using owned `String` in deserializers ([#11764](astral-sh/uv#11764)) - Migrate to `zlib-rs` (again) ([#11894](astral-sh/uv#11894)) - Remove unnecessary clones when adding package names ([#11771](astral-sh/uv#11771)) - Skip unquote allocation for non-quoted strings ([#11813](astral-sh/uv#11813)) - Use `SmallString` for filenames and URLs ([#11765](astral-sh/uv#11765)) - Use a Boxed slice for version specifiers ([#11766](astral-sh/uv#11766)) - Use matches over contains for extra value parsing ([#11770](astral-sh/uv#11770)) ##### Documentation - Avoid fallback to PyPI in mixed CPU/CUDA example ([#11115](astral-sh/uv#11115)) - Docs: Clarify that setting cache-keys overrides defaults ([#11895](astral-sh/uv#11895)) - Document our MSRV policy ([#11898](astral-sh/uv#11898)) - Fix reference to macOS cache path ([#11845](astral-sh/uv#11845)) - Fix typo in `no_default_groups` documentation and changelog ([#11928](astral-sh/uv#11928)) - Update the "Locking and syncing" page ([#11647](astral-sh/uv#11647)) - Update alternative indexes documentation to use new interface ([#10826](astral-sh/uv#10826)) </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).

ericmarkmartin commented Feb 23, 2025

View reviewed changes

ericmarkmartin force-pushed the ctrl-c-panicking branch from 549329c to 09dab53 Compare February 23, 2025 20:38

ericmarkmartin marked this pull request as draft February 24, 2025 03:29

don't panic off of uv_console::confirm

7498bb5

ericmarkmartin force-pushed the ctrl-c-panicking branch from 09dab53 to 7498bb5 Compare February 26, 2025 05:16

add newline to ctrl-c message

41e9637

ericmarkmartin marked this pull request as ready for review February 26, 2025 06:36

Forward io::Error too

2ef339e

konstin added the bug Something isn't working label Feb 26, 2025

konstin changed the title ~~handle Ctrl-C explicitly in confirm prompt~~ Don't panic on Ctrl-C in confirm prompt Feb 26, 2025

konstin merged commit 6e7ec32 into astral-sh:main Feb 26, 2025
74 checks passed

ericmarkmartin deleted the ctrl-c-panicking branch February 27, 2025 02:33

ericmarkmartin mentioned this pull request Mar 2, 2025

explicitly handle ctrl-c in confirmation prompt instead of signal handler #11897

Merged

BrewTestBot mentioned this pull request Mar 4, 2025

uv 0.6.4 Homebrew/homebrew-core#209642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Don't panic on Ctrl-C in confirm prompt#11706

Don't panic on Ctrl-C in confirm prompt#11706
konstin merged 3 commits intoastral-sh:mainfrom
ericmarkmartin:ctrl-c-panicking

ericmarkmartin commented Feb 22, 2025 •

edited

Loading

Uh oh!

ericmarkmartin commented Feb 22, 2025

Uh oh!

ericmarkmartin left a comment

Uh oh!

ericmarkmartin commented Feb 24, 2025

Uh oh!

konstin commented Feb 24, 2025

Uh oh!

ericmarkmartin commented Feb 24, 2025

Uh oh!

konstin commented Feb 25, 2025 •

edited

Loading

Uh oh!

ericmarkmartin commented Feb 26, 2025

Uh oh!

ericmarkmartin commented Feb 26, 2025

Uh oh!

konstin commented Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

ericmarkmartin commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

ericmarkmartin commented Feb 22, 2025

Uh oh!

ericmarkmartin left a comment

Choose a reason for hiding this comment

Uh oh!

ericmarkmartin commented Feb 24, 2025

Uh oh!

konstin commented Feb 24, 2025

Uh oh!

ericmarkmartin commented Feb 24, 2025

Uh oh!

konstin commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericmarkmartin commented Feb 26, 2025

Uh oh!

ericmarkmartin commented Feb 26, 2025

Uh oh!

konstin commented Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericmarkmartin commented Feb 22, 2025 •

edited

Loading

konstin commented Feb 25, 2025 •

edited

Loading