Skip to content

Commit f78af7e

Browse files
Run PTY test children through one pty_io::run (#1942) (#1955)
* Start one PTY runner for the PTY suites (#1942) Assisted-by: Claude Code:claude-opus-5-5 * Run PTY test children through pty_io::run Seven PTY suites each hand-rolled the same openpty, spawn, answer, watchdog and collect runner, and the copies drifted: only two pinned TERM, only one could send a second answer, and the update notifier read with its own read_to_end thread. They now all go through pty_io::run (the binary, one answer) or pty_io::run_command (any hermetic command, extra env, several answers), which always pins TERM=xterm-256color. Assisted-by: Claude Code:claude-opus-5-5 * Cancel the PTY watchdog once the child is reaped The detached watchdog slept out its full timeout and then signalled the pid even after the child had exited, so it could hit a reused pid. It now waits on a channel that is dropped after wait(). Also correct the runner's doc (SIGHUP, the newline portable_pty writes on drop, which answers wait for [Y/n]) and the stale update_render note. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 44d22d2 commit f78af7e

8 files changed

Lines changed: 172 additions & 567 deletions

File tree

‎crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs‎

Lines changed: 10 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -10,104 +10,7 @@
1010
use std::path::Path;
1111
use std::time::Duration;
1212

13-
use portable_pty::{native_pty_system, PtySize};
14-
15-
/// Spawn the socket-patch binary inside a PTY, send `input`, and
16-
/// collect all output until the child exits. Returns `(exit_code,
17-
/// output)`. The timeout is enforced via a watchdog thread that
18-
/// kills the child if it doesn't exit in time.
19-
///
20-
/// Three pieces compose:
21-
/// * **Reader thread**: `read_to_end` on the master side.
22-
/// Blocks until EOF, which the kernel sends once both the
23-
/// slave fd (dropped here) and the child's last open fd are
24-
/// closed.
25-
/// * **Watchdog thread**: sleeps `timeout` then sends SIGKILL
26-
/// via a cloned ChildKiller. Detaches; no join needed since
27-
/// the killer is idempotent and the child either exits
28-
/// normally first (kill is a no-op) or is killed (we proceed).
29-
/// * **Main thread**: writes input, closes the writer (sends
30-
/// EOF on the child's stdin), blocks on `child.wait()`, then
31-
/// joins the reader.
32-
///
33-
/// No polling loops, no mpsc channels, no fixed-duration sleeps
34-
/// before sending input — the PTY buffers the input until the
35-
/// child reads it, so timing-coupling isn't needed.
36-
fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) {
37-
run_in_pty_bytes(args, cwd, input.as_bytes(), timeout)
38-
}
39-
40-
/// Byte-level variant of [`run_in_pty`] for input that is not valid
41-
/// UTF-8 (e.g. a Latin-1 paste at an interactive prompt).
42-
fn run_in_pty_bytes(args: &[&str], cwd: &Path, input: &[u8], timeout: Duration) -> (i32, String) {
43-
let pty_system = native_pty_system();
44-
let pair = pty_system
45-
.openpty(PtySize {
46-
rows: 24,
47-
cols: 80,
48-
pixel_width: 0,
49-
pixel_height: 0,
50-
})
51-
.expect("openpty");
52-
53-
// The hermetic environment (`tests/common/hermetic.rs`): an ambient
54-
// SOCKET_YES=true would skip the very confirm prompts this file exists
55-
// to drive, and SOCKET_SILENT=true would suppress the output the
56-
// oracles match. It also forces the update notifier off: this suite
57-
// hands the child a REAL terminal, so the stderr-TTY guard does not
58-
// protect it.
59-
let mut cmd = crate::common::hermetic::pty_binary_command();
60-
for a in args {
61-
cmd.arg(a);
62-
}
63-
cmd.cwd(cwd);
64-
65-
let mut child = pair
66-
.slave
67-
.spawn_command(cmd)
68-
.expect("spawn socket-patch in PTY");
69-
// Drop the slave so the master sees EOF once the child closes its
70-
// own copy of the slave fd on exit.
71-
drop(pair.slave);
72-
73-
// Reader: a single `read_to_end` is sufficient — it blocks until
74-
// EOF, which arrives when (a) the master is dropped (we do that
75-
// below) or (b) the child has exited and its end of the slave is
76-
// closed. The previous design used a chunked read+mpsc loop
77-
// because it interleaved with a try_wait poll; the simplified
78-
// design serializes wait → drop master → read_to_end joins.
79-
let reader_handle =
80-
crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader"));
81-
82-
// Watchdog: detach a thread that kills the child after `timeout`.
83-
// The cloned ChildKiller is independent of the main `child`
84-
// handle, so the watchdog can fire without coordinating with the
85-
// main thread. If the child exits naturally first, the kill is a
86-
// no-op against a dead pid.
87-
let mut killer = child.clone_killer();
88-
std::thread::spawn(move || {
89-
std::thread::sleep(timeout);
90-
let _ = killer.kill();
91-
});
92-
93-
// Writer: send input once a prompt is on screen, then close.
94-
// `ui::confirm` discards typeahead right before it prompts, so input
95-
// written any earlier would be thrown away (see
96-
// `pty_io::send_when_prompted`).
97-
let mut writer = pair.master.take_writer().expect("take writer");
98-
crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input);
99-
drop(writer);
100-
101-
// Block until the child exits (watchdog enforces the timeout).
102-
let status = child.wait().expect("child.wait");
103-
// Drop the master so the reader's `read_to_end` sees EOF and
104-
// returns.
105-
drop(pair.master);
106-
107-
let output = reader_handle.finish();
108-
let code = status.exit_code() as i32;
109-
(code, String::from_utf8_lossy(&output).to_string())
110-
}
13+
use crate::pty_io;
11114

11215
// ---------------------------------------------------------------------------
11316
// `remove` interactive confirmation
@@ -138,7 +41,7 @@ fn remove_interactive_y_proceeds() {
13841
let tmp = tempfile::tempdir().unwrap();
13942
write_remove_manifest(tmp.path());
14043

141-
let (code, output) = run_in_pty(
44+
let (code, output) = pty_io::run(
14245
&[
14346
"remove",
14447
"pkg:npm/__interactive_remove__@1.0.0",
@@ -186,7 +89,7 @@ fn remove_interactive_n_cancels() {
18689
let tmp = tempfile::tempdir().unwrap();
18790
write_remove_manifest(tmp.path());
18891

189-
let (code, output) = run_in_pty(
92+
let (code, output) = pty_io::run(
19093
&[
19194
"remove",
19295
"pkg:npm/__interactive_remove__@1.0.0",
@@ -247,14 +150,16 @@ fn remove_interactive_non_utf8_answer_declines_without_panic() {
247150
// InvalidData error. Regression: `confirm()` unwrapped that error
248151
// and panicked (exit 101) instead of treating the garbage like any
249152
// other unrecognized answer (decline).
250-
let (code, output) = run_in_pty_bytes(
153+
let pty_io::PtyRun { code, output, .. } = pty_io::run_command(
154+
crate::common::hermetic::pty_binary_command(),
251155
&[
252156
"remove",
253157
"pkg:npm/__interactive_remove__@1.0.0",
254158
"--skip-rollback",
255159
],
256160
tmp.path(),
257-
b"\xE9\n",
161+
&[],
162+
&[b"\xE9\n"],
258163
Duration::from_secs(15),
259164
);
260165
assert!(
@@ -329,7 +234,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() {
329234
let tmp = tempfile::tempdir().unwrap();
330235
write_detached_vendor_state(tmp.path(), purl, uuid);
331236

332-
let (code, output) = run_in_pty(
237+
let (code, output) = pty_io::run(
333238
&["remove", purl, "--silent"],
334239
tmp.path(),
335240
"n\n",
@@ -365,7 +270,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() {
365270
// fix that deleted the message instead of gating it.
366271
let tmp2 = tempfile::tempdir().unwrap();
367272
write_detached_vendor_state(tmp2.path(), purl, uuid);
368-
let (loud_code, loud_output) = run_in_pty(
273+
let (loud_code, loud_output) = pty_io::run(
369274
&["remove", purl],
370275
tmp2.path(),
371276
"n\n",
@@ -393,7 +298,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() {
393298
#[test]
394299
fn apply_in_pty_with_no_manifest_prints_friendly_message() {
395300
let tmp = tempfile::tempdir().unwrap();
396-
let (code, output) = run_in_pty(&["apply"], tmp.path(), "", Duration::from_secs(15));
301+
let (code, output) = pty_io::run(&["apply"], tmp.path(), "", Duration::from_secs(15));
397302
assert_eq!(code, 0);
398303
// Assert the full message, not either half of it. The wording names the
399304
// missing manifest, not the `.socket/` folder (hosted/vendored projects

0 commit comments

Comments
 (0)