Skip to content

Commit 32c5f1e

Browse files
authored
Fix #59: Ctrl-C no longer breaks the user's shell (#60)
Closes #59. Reported by @antoyo and reproduced by both of us against this branch. ## Symptom Running a Playwright program and hitting Ctrl-C left the user's shell unable to interpret arrow keys — they echoed as raw \`^[[A\` etc. instead of recalling history. \`stty sane\` recovered. Reproduced on Arch Linux + alacritty/mate-terminal (antoyo) and macOS + zsh + Apple Terminal (us). ## Root cause Three things had to be true simultaneously: 1. The Node driver shared a process group with our Rust process, so Ctrl-C SIGINT'd both. Node, dying alongside us with chromium events still in flight, crashed on EPIPE while writing back to the closed pipe. Its stack-trace formatter wrote terminal-capability queries to stderr. 2. The Node driver's stderr was inherited from our tty, so those queries reached the user's terminal directly. 3. The terminal answered the queries on stdin, polluting the buffer that zsh's line editor was about to read. zle's setup couldn't engage cleanly. Result: arrow keys fell through to kernel echo in canonical mode. \`stty -a\` was byte-identical before and after — termios was never the issue, despite that being our first hypothesis. ## Fix (three commits, layered) 1. [\`8764fd3\`](8764fd3) — defensive tty-guard: \`Playwright::launch\` snapshots stdin termios at first call, a SIGINT handler restores it before exiting, and \`Drop\` closes the driver stdin pipe before SIGKILL fallback. Belt-and-suspenders; not the load-bearing fix. 2. [\`a63ed59\`](a63ed59) — spawn the Node driver in its own process group via \`process_group(0)\`. Removes (1) above: SIGINT only signals our Rust process; Node sees its stdin close and runs \`gracefullyProcessExitDoNotHang\` instead of crashing on EPIPE. 3. [\`b8ab822\`](b8ab822) — pipe the Node driver's stderr instead of inheriting it. A background task drains the pipe and forwards each line via \`tracing::debug!\` (target \`playwright_rs::driver_stderr\`), so driver diagnostics are still available without ever touching the user's tty. Removes (2) above and is the actual fix for the symptom antoyo confirmed. ## Tests - \`tests/sigint_termios.rs\` (initial WIP commit, [\`d068022\`](d068022)) — expect(1)-driven harness in a real pty across four scenarios (Ctrl-C during chromium launch, after page load, double Ctrl-C, full-profile bash). Asserts \`stty -a\` is byte-identical before/after, normalizing transient flags. Skipped on Windows via \`#[cfg(unix)]\`. Did not reproduce the bug on any test environment we have, but stays as a regression check. - All existing stability/cleanup tests still pass. ## Compatibility - Opt-out for the SIGINT handler via \`PLAYWRIGHT_NO_SIGNAL_HANDLER=1\` for users with their own handlers. - Driver stderr was previously inherited; users relying on Node-side diagnostics in their terminal can enable tracing on \`playwright_rs::driver_stderr\` to recover them. Will cut a v0.12.3 patch after merge.
1 parent dc34f3a commit 32c5f1e

13 files changed

Lines changed: 464 additions & 19 deletions

File tree

.github/workflows/test.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,10 @@ jobs:
9393
- name: Run clippy
9494
run: cargo clippy --all-targets --all-features -- -D warnings
9595

96+
- name: Install expect (issue #59 reproducer harness)
97+
if: runner.os == 'Linux'
98+
run: sudo apt-get update && sudo apt-get install -y expect
99+
96100
- name: Build
97101
run: cargo build --verbose
98102

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- **Driver stderr no longer pollutes the user's terminal** — the Node driver's stderr is now piped and drained into `tracing::debug!` (target `playwright_rs::driver_stderr`) instead of being inherited from our process. Inheriting it meant any escape sequences the driver wrote (terminal-capability queries, error formatting) ended up on the user's tty, where the terminal's responses then disrupted shell line-editing after a Ctrl-C. Closes [#59](https://github.com/padamson/playwright-rust/issues/59) — terminal left in a state where arrow keys echo as raw `^[[A` instead of recalling history.
13+
- **Quiet shutdown on Ctrl-C (Unix only)** — the Node driver is also spawned in its own process group via `process_group(0)`. SIGINT in the user's shell only signals our Rust process; Node sees its stdin close cleanly and runs `gracefullyProcessExitDoNotHang` instead of crashing on EPIPE while chromium events are still in flight. Eliminates the noisy stack trace previously printed on Ctrl-C.
14+
- **Defensive tty restore (Unix only)**`Playwright::launch` snapshots stdin's termios at first call and a SIGINT handler restores it before exiting; `Drop` also closes the driver stdin pipe before falling back to SIGKILL. Belt-and-suspenders for any subprocess that does manage to clobber tty state. Opt-out via `PLAYWRIGHT_NO_SIGNAL_HANDLER=1`.
15+
1016
## [0.12.2] - 2026-04-27
1117

1218
### Added

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/playwright/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ futures-util = "0.3"
3737
url = "2.5"
3838
image = { version = "0.25", default-features = false, features = ["png"] }
3939

40+
[target.'cfg(unix)'.dependencies]
41+
libc = "0.2"
42+
4043
[dev-dependencies]
4144
anyhow = { workspace = true }
4245
axum = { version = "0.8", features = ["ws"] }
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Reproducer for issue #59: hitting Ctrl-C after Playwright launches
2+
// chromium leaves the terminal in non-canonical mode (arrow keys produce
3+
// raw escape sequences, line editing broken).
4+
//
5+
// Used by the `tests/integration/sigint_termios.rs` integration test via
6+
// the expect-based harness in `tests/integration/sigint_termios_harness.exp`.
7+
//
8+
// Prints `[sigint-repro] READY` on stderr once chromium is launched and
9+
// the page has navigated, then sleeps for 30 seconds. The expect harness
10+
// waits for the READY line, snapshots stty, sends Ctrl-C, snapshots stty,
11+
// and asserts the two snapshots are byte-identical.
12+
use playwright_rs::Playwright;
13+
14+
#[tokio::main]
15+
async fn main() -> Result<(), Box<dyn std::error::Error>> {
16+
eprintln!("[sigint-repro] launching playwright");
17+
let pw = Playwright::launch().await?;
18+
let browser = pw.chromium().launch().await?;
19+
let page = browser.new_page().await?;
20+
page.goto("data:text/html,<h1>repro</h1>", None).await?;
21+
22+
eprintln!("[sigint-repro] READY");
23+
24+
tokio::time::sleep(std::time::Duration::from_secs(30)).await;
25+
26+
browser.close().await?;
27+
Ok(())
28+
}

crates/playwright/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ pub mod api;
158158
mod assertions;
159159
mod error;
160160
pub mod protocol;
161+
mod tty_guard;
161162

162163
/// Playwright server version bundled with this crate.
163164
///

crates/playwright/src/protocol/playwright.rs

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,15 @@ impl Playwright {
100100
use crate::server::playwright_server::PlaywrightServer;
101101
use crate::server::transport::PipeTransport;
102102

103+
// Snapshot stdin termios and install a SIGINT handler that
104+
// restores it before exiting. Defends against subprocesses that
105+
// clobber the controlling terminal's mode and don't restore on
106+
// abrupt exit (issue #59). Idempotent — multiple Playwright
107+
// instances share the snapshot/handler. Opt-out via the
108+
// PLAYWRIGHT_NO_SIGNAL_HANDLER env var.
109+
crate::tty_guard::save_if_tty();
110+
crate::tty_guard::install_signal_handler();
111+
103112
// 1. Launch Playwright server
104113
tracing::debug!("Launching Playwright server");
105114
let mut server = PlaywrightServer::launch().await?;
@@ -423,30 +432,44 @@ impl ChannelOwner for Playwright {
423432
impl Drop for Playwright {
424433
/// Ensures Playwright server is shut down when Playwright is dropped.
425434
///
426-
/// This is critical on Windows to prevent process hangs when tests complete.
427-
/// The Drop implementation will attempt to kill the server process synchronously.
435+
/// On Unix, the driver's `cli/driver.js` listens for stdin close as a
436+
/// graceful-exit signal — closing the pipe lets it tear down browsers
437+
/// cleanly instead of leaving them as orphans. We drop stdin first,
438+
/// then `start_kill` as a SIGKILL fallback (still issued
439+
/// synchronously because we don't want to block the Drop).
440+
///
441+
/// On Windows, tokio's blocking stdio threadpool requires we drop
442+
/// the pipes before killing or the cleanup hangs.
428443
///
429-
/// Note: For graceful shutdown, prefer calling `playwright.shutdown().await`
430-
/// explicitly before dropping.
444+
/// Also restores any termios snapshot we took at launch time
445+
/// (defends against subprocesses that left the tty in raw mode —
446+
/// issue #59).
447+
///
448+
/// For fully graceful shutdown, prefer calling
449+
/// `playwright.shutdown().await` explicitly before dropping.
431450
fn drop(&mut self) {
432451
if let Some(mut server) = self.server.lock().take() {
433-
tracing::debug!("Drop: Force-killing Playwright server");
452+
tracing::debug!("Drop: shutting down Playwright server");
434453

435-
// We can't call async shutdown in Drop, so use blocking kill
436-
// This is less graceful but ensures the process terminates
454+
// Close stdin first — driver.js's `process.stdin.on("close",
455+
// gracefullyProcessExitDoNotHang)` triggers cleanup of
456+
// browser children. On Windows we drop all stdio handles to
457+
// avoid the blocking-threadpool hang.
458+
drop(server.process.stdin.take());
437459
#[cfg(windows)]
438460
{
439-
// On Windows: Close stdio pipes before killing
440-
drop(server.process.stdin.take());
441461
drop(server.process.stdout.take());
442462
drop(server.process.stderr.take());
443463
}
444464

445-
// Force kill the process
465+
// SIGKILL fallback — non-blocking. If the driver was already
466+
// exiting cleanly via stdin-close, this is a no-op.
446467
if let Err(e) = server.process.start_kill() {
447468
tracing::warn!("Failed to kill Playwright server in Drop: {}", e);
448469
}
449470
}
471+
472+
crate::tty_guard::restore();
450473
}
451474
}
452475

crates/playwright/src/server/playwright_server.rs

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,61 @@ impl PlaywrightServer {
5353
// The driver should already be downloaded by build.rs
5454
let (node_exe, cli_js) = get_driver_executable()?;
5555

56-
// Launch the server process
57-
let mut child = Command::new(&node_exe)
58-
.arg(&cli_js)
56+
// Launch the server process. Stderr is piped (not inherited)
57+
// because the Node driver writes terminal-capability queries
58+
// and other escape sequences to its stderr while alive. With
59+
// stderr inherited, those bytes clobber the user's tty and
60+
// break shell line-editing after a Ctrl-C while the driver is
61+
// still gracefully shutting down chromium (see #59). We drain
62+
// the piped stderr in a background task and forward each line
63+
// via `tracing::debug!` so users with tracing enabled can
64+
// still see driver diagnostics.
65+
let mut cmd = Command::new(&node_exe);
66+
cmd.arg(&cli_js)
5967
.arg("run-driver")
6068
.env("PW_LANG_NAME", "rust")
6169
.env("PW_LANG_NAME_VERSION", env!("CARGO_PKG_RUST_VERSION"))
6270
.env("PW_CLI_DISPLAY_VERSION", env!("CARGO_PKG_VERSION"))
6371
.stdin(std::process::Stdio::piped())
6472
.stdout(std::process::Stdio::piped())
65-
.stderr(std::process::Stdio::inherit())
73+
.stderr(std::process::Stdio::piped());
74+
75+
// Put the Node driver in its own process group so a Ctrl-C in
76+
// the user's shell (which sends SIGINT to the foreground process
77+
// group) doesn't reach Node. When our process dies, Node's stdin
78+
// pipe closes and the driver runs `gracefullyProcessExitDoNotHang`
79+
// — a quiet, browser-aware shutdown. Without this isolation, Node
80+
// gets SIGINT'd alongside us and races a noisy EPIPE error path
81+
// that writes terminal-capability queries to stderr; the
82+
// terminal's responses then pollute bash's stdin buffer and
83+
// disrupt readline. See issue #59.
84+
// process_group is on tokio::process::Command directly (Unix
85+
// only). Pgid 0 means "make the child its own group leader"
86+
// (PGID == child PID).
87+
#[cfg(unix)]
88+
{
89+
cmd.process_group(0);
90+
}
91+
92+
let mut child = cmd
6693
.spawn()
6794
.map_err(|e| Error::LaunchFailed(format!("Failed to spawn process: {}", e)))?;
6895

96+
// Drain Node's stderr in a background task. Without an active
97+
// reader the kernel pipe buffer would eventually fill and
98+
// block the driver's writes; we don't want that. Bytes are
99+
// forwarded line-by-line via `tracing::debug!` so they're
100+
// accessible when needed without polluting the terminal.
101+
if let Some(stderr) = child.stderr.take() {
102+
tokio::spawn(async move {
103+
use tokio::io::{AsyncBufReadExt, BufReader};
104+
let mut lines = BufReader::new(stderr).lines();
105+
while let Ok(Some(line)) = lines.next_line().await {
106+
tracing::debug!(target: "playwright_rs::driver_stderr", "{}", line);
107+
}
108+
});
109+
}
110+
69111
// Check if process started successfully
70112
// Give it a moment to potentially fail
71113
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;

crates/playwright/src/tty_guard.rs

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// Defensive Ctrl-C handling for stdin termios state.
2+
//
3+
// Background: when a Playwright session is interrupted with SIGINT (Ctrl-C),
4+
// the spawned Node driver and its chromium subprocesses can clobber the
5+
// controlling terminal's `termios` state in subtle ways and never restore
6+
// it, leaving the user's shell in non-canonical mode where arrow keys
7+
// echo as raw `^[[D` sequences instead of moving the cursor (issue #59).
8+
//
9+
// The exact subprocess responsible has not been pinpointed — the symptom
10+
// reproduces in some terminal environments but not others, and we have
11+
// not been able to recreate it inside an `expect`-allocated pty across
12+
// macOS/Linux ARM64/Linux x64 CI runners. The defensive fix here is
13+
// agnostic to which process is the offender:
14+
//
15+
// 1. At `Playwright::launch`, snapshot stdin's termios if stdin is a tty.
16+
// 2. Install a one-shot SIGINT handler that restores the snapshot before
17+
// letting the process die with the conventional 130 exit code.
18+
// 3. The `Drop` impl on `Playwright` also restores the snapshot so the
19+
// same protection applies to graceful exits and panics.
20+
//
21+
// The signal handler can be disabled by setting the
22+
// `PLAYWRIGHT_NO_SIGNAL_HANDLER` environment variable (any non-empty
23+
// value) — for users who manage their own SIGINT handlers and don't
24+
// want this library overriding them.
25+
//
26+
// Stub on Windows: the symptom in #59 is Unix-specific and Windows uses
27+
// a different console-mode model. The whole module compiles to no-ops
28+
// on Windows.
29+
30+
#[cfg(unix)]
31+
mod imp {
32+
use parking_lot::Mutex;
33+
use std::sync::OnceLock;
34+
35+
static SAVED: OnceLock<Mutex<Option<libc::termios>>> = OnceLock::new();
36+
static HANDLER_INSTALLED: OnceLock<()> = OnceLock::new();
37+
38+
/// Snapshot stdin's termios if stdin is a tty. Idempotent — only the
39+
/// first call records a snapshot; later calls are no-ops so we never
40+
/// overwrite the original "clean" state with a possibly-already-clobbered
41+
/// one.
42+
pub(crate) fn save_if_tty() {
43+
let cell = SAVED.get_or_init(|| Mutex::new(None));
44+
let mut guard = cell.lock();
45+
if guard.is_some() {
46+
return;
47+
}
48+
// SAFETY: tcgetattr writes to the termios pointer on success and
49+
// ignores it on failure. Stdin (fd 0) is always a valid file
50+
// descriptor in a hosted environment.
51+
unsafe {
52+
if libc::isatty(0) != 1 {
53+
return;
54+
}
55+
let mut t: libc::termios = std::mem::zeroed();
56+
if libc::tcgetattr(0, &mut t) == 0 {
57+
*guard = Some(t);
58+
}
59+
}
60+
}
61+
62+
/// Restore the saved termios to stdin. No-op if nothing was saved
63+
/// (stdin wasn't a tty or save_if_tty was never called).
64+
pub(crate) fn restore() {
65+
let Some(cell) = SAVED.get() else { return };
66+
let guard = cell.lock();
67+
let Some(t) = *guard else { return };
68+
// SAFETY: tcsetattr reads from the termios pointer and applies
69+
// it. Failure is acceptable (e.g. stdin no longer a tty) and we
70+
// ignore the return code.
71+
unsafe {
72+
let _ = libc::tcsetattr(0, libc::TCSANOW, &t);
73+
}
74+
}
75+
76+
/// Install a one-shot SIGINT handler that restores termios and exits
77+
/// with code 130. Returns immediately if already installed or if the
78+
/// `PLAYWRIGHT_NO_SIGNAL_HANDLER` env var is set.
79+
pub(crate) fn install_signal_handler() {
80+
if std::env::var_os("PLAYWRIGHT_NO_SIGNAL_HANDLER").is_some() {
81+
return;
82+
}
83+
if HANDLER_INSTALLED.set(()).is_err() {
84+
return;
85+
}
86+
87+
tokio::spawn(async move {
88+
// tokio::signal::ctrl_c registers a SIGINT listener via the
89+
// tokio runtime; multiple listeners can coexist with user
90+
// handlers, but our task gets to act first and exit the
91+
// process cleanly.
92+
if tokio::signal::ctrl_c().await.is_ok() {
93+
restore();
94+
// 128 + SIGINT(2) = 130, the conventional exit code for
95+
// Ctrl-C interrupted programs.
96+
std::process::exit(130);
97+
}
98+
});
99+
}
100+
}
101+
102+
#[cfg(not(unix))]
103+
mod imp {
104+
pub(crate) fn save_if_tty() {}
105+
pub(crate) fn restore() {}
106+
pub(crate) fn install_signal_handler() {}
107+
}
108+
109+
pub(crate) use imp::{install_signal_handler, restore, save_if_tty};

0 commit comments

Comments
 (0)