[nextest-runner] ignore SIGTTIN/SIGTTOU while input handling is active#2884
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2884 +/- ##
==========================================
- Coverage 80.70% 80.60% -0.10%
==========================================
Files 118 119 +1
Lines 28228 28263 +35
==========================================
Hits 22781 22781
- Misses 5447 5482 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a suspension issue where nextest would receive SIGTTOU and get suspended when tests spawn interactive shells that take over the foreground process group. The fix blocks SIGTTOU during tcsetattr calls, following the pattern used by established terminal-aware programs like less and Node.js.
Key changes:
- Blocks SIGTTOU around
tcsetattrcalls in the input handler usingsigprocmask - Adds a test helper binary (
grab-foreground) that simulates interactive shell behavior - Includes an integration test (currently ignored) to verify the fix
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| nextest-runner/src/input.rs | Adds SIGTTOU blocking around tcsetattr calls to prevent suspension when in background |
| integration-tests/tests/integration/sigttou.rs | New integration test verifying nextest doesn't suspend when subprocess grabs foreground (currently ignored) |
| integration-tests/test-helpers/grab-foreground.rs | Test helper binary that simulates interactive shell behavior by grabbing foreground process group |
| integration-tests/Cargo.toml | Registers the grab-foreground test helper binary |
| integration-tests/tests/integration/main.rs | Imports the new sigttou test module with unix cfg guard |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When a test spawns an interactive shell (e.g., `zsh -ic`), the shell opens `/dev/tty` directly and calls `tcsetpgrp` to become the foreground process group. This causes nextest to become a background process. If nextest then tries to read from stdin (SIGTTIN) or restore terminal state via `tcsetattr` (SIGTTOU), it gets stopped. The fix is to ignore SIGTTIN and SIGTTOU for the process lifetime once input handling is enabled. This is the same approach zsh uses for job control: https://github.com/zsh-users/zsh/blob/3e72a52/Src/init.c#L1439 Less invasive approaches don't work: 1. Blocking SIGTTOU only around `tcsetattr` calls: This doesn't handle SIGTTIN, which is sent when crossterm's background thread tries to read from stdin while we're a background process. 2. Restoring SIG_DFL after `restore()`: The crossterm input thread may still be running and could trigger SIGTTIN after we restore default signal handling but before the thread exits. 3. Dropping EventStream before restoring SIG_DFL: Dropping the stream doesn't synchronously wait for the crossterm thread to exit, so there's still a race. The signals remain ignored for the process lifetime. This is fine for a test runner since we don't need SIGTTIN/SIGTTOU behavior. Also adds a test helper binary (`grab-foreground`) that simulates what interactive shells do, and an ignored integration test to verify the fix. The test requires an interactive terminal and must be run manually. Fixes: #2878
sunshowers
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a test spawns an interactive shell (e.g.,
zsh -ic), the shell opens/dev/ttydirectly and callstcsetpgrpto become the foreground process group. This causes nextest to become a background process. If nextest then tries to read from stdin (SIGTTIN) or restore terminal state viatcsetattr(SIGTTOU), it gets stopped.The fix is to ignore SIGTTIN and SIGTTOU for the process lifetime once input handling is enabled (which only happens after we've checked that we're in the foreground in the first place). This is the same approach zsh uses for job control: https://github.com/zsh-users/zsh/blob/3e72a52/Src/init.c#L1439
Less invasive approaches don't work:
Blocking SIGTTOU only around
tcsetattrcalls: This doesn't handle SIGTTIN, which is sent when crossterm's background thread tries to read from stdin while we're a background process.Restoring SIG_DFL after
restore(): The crossterm input thread may still be running and could trigger SIGTTIN after we restore default signal handling but before the thread exits.Dropping EventStream before restoring SIG_DFL: Dropping the stream doesn't synchronously wait for the crossterm thread to exit, so there's still a race.
The signals remain ignored for the process lifetime. This is fine for a test runner since we don't need SIGTTIN/SIGTTOU behavior.
Also add a test helper binary (
grab-foreground) that simulates what interactive shells do, and an ignored integration test to verify the fix. (We'll un-ignore it once a version of nextest is out with this fix.)Note that this fixes the signal, though not the larger issue that input handling is broken after a test does something weird like that. At some point we'll probably want to let users specify that some tests should be run under a pty, though that's not today.
Fixes: #2878