Skip to content

Commit fe7426c

Browse files
Parth SharmaParth Sharma
authored andcommitted
Stop the shells' commands on cancel and time-up; strip CR from the marker cwd
- Terminal: on macOS the end marker line can end in \r\n, so the parsed cwd kept a trailing \r and relative read_file/apply_patch paths failed. The marker line is now trimmed of \r before parsing. - Terminal: Session::kill (end of run, --timeout-s, IPC abort/kill/closed stdin, Terminal drop) only hung up the shell. Job control puts the running command and each background job in its own process group, so they survived with ppid 1. kill now sends SIGTERM (and SIGCONT) to every process group in the shell's session, waits up to 1 s, then SIGKILLs what is left, the shell included. Groups are listed from /proc on Linux (by session id) and with `ps -t <pty>` on macOS, plus the foreground group and the shell's own group. - Terminal: a finished background session and the "did not stop on Ctrl-C" fallback only hang up (SIGTERM/SIGKILL to the foreground command, SIGHUP to the shell), so `nohup` servers started there keep running as before. --linger is unchanged: it never calls kill. - IPC: SIGTERM/SIGINT now drop the session (and its shells) before exiting with 128+signal, instead of dying with the commands still running.
1 parent b35c983 commit fe7426c

10 files changed

Lines changed: 397 additions & 7 deletions

File tree

‎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/cli/src/ipc.rs‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -704,15 +704,39 @@ pub(crate) async fn run(cli: Cli) -> ! {
704704
let _ = stdout.flush().await;
705705
}
706706
});
707-
// `session` drops its shells and MCP servers before returning.
708-
let code = session(cli, out.clone()).await;
707+
// `session` drops its shells and MCP servers before returning. A stop
708+
// signal drops it too, so the commands the shells run do not outlive us.
709+
let code = tokio::select! {
710+
code = session(cli, out.clone()) => code,
711+
signal = stop_signal() => 128 + signal,
712+
};
709713
out.send(Value::Null);
710714
let _ = writer.await;
711715
// The stdin reader may still be waiting in a blocking read the runtime
712716
// would wait for (a client may keep its end open after `session_end`).
713717
std::process::exit(code)
714718
}
715719

720+
/// Waits for SIGTERM or SIGINT and returns its number.
721+
async fn stop_signal() -> i32 {
722+
use tokio::signal::unix::{signal, SignalKind};
723+
let wait_for = |kind: SignalKind| async move {
724+
match signal(kind) {
725+
Ok(mut stream) => {
726+
stream.recv().await;
727+
kind.as_raw_value()
728+
}
729+
// No handler was installed, so the signal keeps its default
730+
// action (ending the process) and this branch never fires.
731+
Err(_) => std::future::pending().await,
732+
}
733+
};
734+
tokio::select! {
735+
number = wait_for(SignalKind::terminate()) => number,
736+
number = wait_for(SignalKind::interrupt()) => number,
737+
}
738+
}
739+
716740
async fn session(cli: Cli, out: Out) -> i32 {
717741
let mut lines = BufReader::new(tokio::io::stdin()).lines();
718742
let fatal = |out: &Out, why: String| {

‎crates/cli/tests/ipc.rs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,63 @@ fn steering_and_abort() {
325325
assert!(is("system", "session_end")(rest.last().unwrap()));
326326
}
327327

328+
fn process_exists(pid: &str) -> bool {
329+
Command::new("kill")
330+
.args(["-0", pid])
331+
.stderr(Stdio::null())
332+
.status()
333+
.unwrap()
334+
.success()
335+
}
336+
337+
#[test]
338+
fn sigterm_stops_the_running_command() {
339+
let tmp = tempfile::tempdir().unwrap();
340+
let work = tmp.path().join("work");
341+
std::fs::create_dir_all(&work).unwrap();
342+
let model = script(
343+
tmp.path(),
344+
"s.yaml",
345+
"- tool_calls: [{name: exec, arguments: {cmd: \"sh -c 'echo $$ > pid.txt; exec sleep 300'\", yield_ms: 60000}}]\n\
346+
- text: \"unreachable\"\n",
347+
);
348+
let mut s = Session::start(tmp.path(), &tmp.path().join("out"));
349+
s.send(json!({"type": "initialize", "protocol": 1, "options": {"model": model, "dir": work, "sessionId": "sig"}}));
350+
s.until(is("system", "session_start"));
351+
s.send(json!({"type": "user", "content": "sleep"}));
352+
s.until(|m| m["type"] == "tool_use");
353+
let pid_file = work.join("pid.txt");
354+
let mut pid = String::new();
355+
for _ in 0..100 {
356+
pid = std::fs::read_to_string(&pid_file)
357+
.unwrap_or_default()
358+
.trim()
359+
.to_string();
360+
if !pid.is_empty() {
361+
break;
362+
}
363+
std::thread::sleep(Duration::from_millis(50));
364+
}
365+
assert!(process_exists(&pid), "the command did not start: {pid:?}");
366+
367+
let gitagent = s.child.id().to_string();
368+
assert!(Command::new("kill")
369+
.args(["-TERM", &gitagent])
370+
.status()
371+
.unwrap()
372+
.success());
373+
let code = s.child.wait().unwrap().code();
374+
assert_eq!(code, Some(143));
375+
// The shell is gone at once; its orphaned command is reaped shortly after.
376+
for _ in 0..100 {
377+
if !process_exists(&pid) {
378+
return;
379+
}
380+
std::thread::sleep(Duration::from_millis(50));
381+
}
382+
panic!("pid {pid} survived SIGTERM");
383+
}
384+
328385
#[test]
329386
fn startup_problems_and_blocking_hooks() {
330387
let tmp = tempfile::tempdir().unwrap();

‎crates/terminal/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ workspace = true
1010

1111
[dependencies]
1212
anyhow.workspace = true
13+
nix = { version = "0.28", default-features = false, features = ["signal", "process"] }
1314
portable-pty = "0.9.0"
1415
tokio.workspace = true
1516

‎crates/terminal/src/buffer.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,10 @@ impl OutputBuffer {
8080
return None;
8181
};
8282
let line = String::from_utf8_lossy(&self.data[line_start..line_start + nl]);
83-
let (code, cwd) = line.split_once(' ').unwrap_or((&line, ""));
83+
// A terminal that still maps `\n` to `\r\n` (seen on macOS) ends
84+
// the line with `\r`, which would otherwise end up in the cwd.
85+
let line = line.trim_end_matches('\r');
86+
let (code, cwd) = line.split_once(' ').unwrap_or((line, ""));
8487
let Ok(exit) = code.parse::<i32>() else {
8588
// Not our marker (for example the text of the printf format).
8689
from = line_start;
@@ -170,6 +173,17 @@ mod tests {
170173
assert_eq!(t.output, b"x\n__GA_END_2__ 0 /\n\n__GA_END_3__ %s %s\ny");
171174
}
172175

176+
#[test]
177+
fn strips_carriage_return_from_the_marker_cwd() {
178+
let mut b = OutputBuffer::new(1 << 20);
179+
b.push(b"out\r\n__GA_END_4__ 0 /tmp/dir\r\nrest");
180+
let (m, t) = b.take_marker(4).unwrap();
181+
assert_eq!(m.exit, 0);
182+
assert_eq!(m.cwd, "/tmp/dir");
183+
assert_eq!(t.output, b"out\r");
184+
assert_eq!(b.take_all().output, b"rest");
185+
}
186+
173187
#[test]
174188
fn partial_take_holds_back_a_split_marker() {
175189
let mut b = OutputBuffer::new(1 << 20);

‎crates/terminal/src/lib.rs‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
mod buffer;
1010
mod keys;
1111
mod manager;
12+
mod stop;
1213

1314
pub use keys::decode_keys;
1415
pub use manager::{Terminal, MAIN_SESSION, MAX_SESSIONS};
@@ -18,6 +19,7 @@ use std::path::{Path, PathBuf};
1819
use std::time::{Duration, Instant};
1920

2021
use anyhow::{bail, Context, Result};
22+
use nix::unistd::Pid;
2123
use portable_pty::{native_pty_system, Child, CommandBuilder, MasterPty, PtySize};
2224
use tokio::sync::mpsc;
2325
use tokio::time::timeout_at;
@@ -109,6 +111,14 @@ enum ReadEvent {
109111
Eof,
110112
}
111113

114+
/// What ending a shell stops besides the shell.
115+
enum Ending {
116+
/// Every process in the shell's session, background jobs included.
117+
Session,
118+
/// Only the command in the terminal's foreground.
119+
Foreground,
120+
}
121+
112122
struct Shell {
113123
master: Box<dyn MasterPty + Send>,
114124
writer: Box<dyn Write + Send>,
@@ -287,9 +297,34 @@ impl Session {
287297
}
288298
}
289299

290-
/// Stops the shell and everything started from it.
300+
/// Stops the shell and everything started from it: the running command
301+
/// and background jobs, which job control puts in groups of their own.
291302
pub fn kill(&mut self) {
303+
self.end_shell(Ending::Session);
304+
}
305+
306+
/// Stops the shell and the command it is running. Background jobs get
307+
/// the shell's SIGHUP, as when a terminal window closes, so jobs started
308+
/// with `nohup` (e.g. a server) keep running.
309+
pub(crate) fn hang_up(&mut self) {
310+
self.end_shell(Ending::Foreground);
311+
}
312+
313+
fn end_shell(&mut self, ending: Ending) {
292314
if let Some(mut shell) = self.shell.take() {
315+
if let Some(pid) = shell.child.process_id().and_then(|p| i32::try_from(p).ok()) {
316+
let pid = Pid::from_raw(pid);
317+
let foreground = shell.master.process_group_leader().map(Pid::from_raw);
318+
match ending {
319+
Ending::Session => {
320+
let tty = shell.master.tty_name();
321+
stop::stop_session(pid, tty.as_deref(), foreground);
322+
}
323+
Ending::Foreground => stop::stop_foreground(pid, foreground),
324+
}
325+
}
326+
// SIGHUP, then SIGKILL, to the shell. Errors only mean it already
327+
// exited; `wait` then just reaps it.
293328
let _ = shell.child.kill();
294329
let _ = shell.child.wait();
295330
}
@@ -414,7 +449,7 @@ impl Session {
414449
]);
415450
}
416451
}
417-
self.kill();
452+
self.hang_up();
418453
Ok(vec![
419454
"The previous command did not stop on Ctrl-C, so its shell was killed.".into(),
420455
])

‎crates/terminal/src/manager.rs‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,12 @@ impl Terminal {
8484
let mut session = Session::start(cfg).await?;
8585
let mut r = session.exec(cmd, yield_ms).await?;
8686
r.session = id;
87+
// A finished session only hangs up, so a server it started with
88+
// `nohup` keeps running for the rest of the run (and for `--linger`).
8789
if session.is_running() {
8890
self.sessions.insert(id, session);
91+
} else {
92+
session.hang_up();
8993
}
9094
Ok(r)
9195
}
@@ -103,7 +107,9 @@ impl Terminal {
103107
r.session = id;
104108
// A background session is done once its command finishes.
105109
if id != MAIN_SESSION && !session.is_running() {
106-
self.sessions.remove(&id);
110+
if let Some(mut done) = self.sessions.remove(&id) {
111+
done.hang_up();
112+
}
107113
}
108114
Ok(r)
109115
}
@@ -251,4 +257,34 @@ mod tests {
251257
t.kill_all();
252258
assert!(t.background_sessions().is_empty());
253259
}
260+
261+
#[tokio::test]
262+
async fn a_finished_background_session_leaves_nohup_jobs_until_the_terminal_ends() {
263+
use crate::tests::{parse_pid, process_exists, wait_until_gone};
264+
let (_d, mut t) = start().await;
265+
let r = t
266+
.exec("nohup sleep 300 >/dev/null 2>&1 & echo $!", 5000, true)
267+
.await
268+
.unwrap();
269+
assert!(r.finished);
270+
assert!(t.background_sessions().is_empty());
271+
let pid = parse_pid(&r);
272+
tokio::time::sleep(Duration::from_millis(500)).await;
273+
assert!(
274+
process_exists(pid),
275+
"the nohup job was stopped with its session"
276+
);
277+
278+
let r = t.exec("sleep 300 & echo $!", 5000, false).await.unwrap();
279+
let main_job = parse_pid(&r);
280+
drop(t);
281+
assert!(wait_until_gone(main_job).await, "pid {main_job} survived");
282+
// The finished session's job is no longer the terminal's to stop.
283+
assert!(process_exists(pid));
284+
nix::sys::signal::kill(
285+
nix::unistd::Pid::from_raw(pid),
286+
nix::sys::signal::Signal::SIGKILL,
287+
)
288+
.expect("stopping the nohup job");
289+
}
254290
}

0 commit comments

Comments
 (0)