Skip to content

Commit 38e8228

Browse files
committed
fix(exec): address interactive TTY streaming review feedback
Server-side (sandbox.rs): - Close SSH channel after EOF so PTY-backed programs (bash, vim, top) terminate on client disconnect instead of leaking the session - Break output loop when tx.send() fails (client gone) to tear down the streaming pipeline promptly - Default zero cols/rows to 80x24 before request_pty, matching the proto "0 = use default" contract and preventing 1x1 terminals for non-CLI clients Client-side (run.rs): - Replace spawn_blocking stdin reader with a detached std::thread so the tokio runtime shutdown does not hang on a thread blocked in stdin.read(). This removes the need for std::process::exit(), allowing save_last_sandbox() in the caller to execute normally - Switch from futures::channel::mpsc with try_send() to tokio::sync::mpsc with blocking_send() for proper backpressure — try_send silently stopped forwarding all input when the channel filled during paste or slow network - Add TaskGuard (abort-on-drop) for the resize task to ensure cleanup on all return paths including early errors
1 parent 54e6810 commit 38e8228

2 files changed

Lines changed: 88 additions & 68 deletions

File tree

crates/openshell-cli/src/run.rs

Lines changed: 65 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -2741,19 +2741,27 @@ impl Drop for RawModeGuard {
27412741
}
27422742
}
27432743

2744+
struct TaskGuard(tokio::task::JoinHandle<()>);
2745+
2746+
impl Drop for TaskGuard {
2747+
fn drop(&mut self) {
2748+
self.0.abort();
2749+
}
2750+
}
2751+
27442752
async fn sandbox_exec_interactive_grpc(
27452753
mut client: crate::tls::GrpcClient,
27462754
sandbox: &Sandbox,
27472755
command: &[String],
27482756
workdir: Option<&str>,
27492757
timeout_seconds: u32,
27502758
) -> Result<i32> {
2751-
use futures::SinkExt;
27522759
use openshell_core::proto::{ExecSandboxInput, ExecSandboxWindowResize, exec_sandbox_input};
2760+
use tokio_stream::wrappers::ReceiverStream;
27532761

27542762
let (cols, rows) = crossterm::terminal::size().unwrap_or((80, 24));
27552763

2756-
let (mut input_tx, input_rx) = futures::channel::mpsc::channel::<ExecSandboxInput>(4096);
2764+
let (input_tx, input_rx) = tokio::sync::mpsc::channel::<ExecSandboxInput>(4096);
27572765

27582766
// Send the start message with exec metadata.
27592767
input_tx
@@ -2774,7 +2782,7 @@ async fn sandbox_exec_interactive_grpc(
27742782
.into_diagnostic()?;
27752783

27762784
let mut stream = client
2777-
.exec_sandbox_interactive(input_rx)
2785+
.exec_sandbox_interactive(ReceiverStream::new(input_rx))
27782786
.await
27792787
.into_diagnostic()?
27802788
.into_inner();
@@ -2783,51 +2791,63 @@ async fn sandbox_exec_interactive_grpc(
27832791
crossterm::terminal::enable_raw_mode().into_diagnostic()?;
27842792
let raw_guard = RawModeGuard;
27852793

2786-
// Stdin reader: read raw bytes one at a time so single keystrokes
2787-
// (e.g. 'q' in top) are forwarded immediately.
2788-
let mut stdin_tx = input_tx.clone();
2789-
tokio::task::spawn_blocking(move || {
2790-
let mut stdin = std::io::stdin().lock();
2791-
let mut buf = [0u8; 4096];
2792-
loop {
2793-
let n = match stdin.read(&mut buf) {
2794-
Ok(0) | Err(_) => break,
2795-
Ok(n) => n,
2796-
};
2797-
if stdin_tx
2798-
.try_send(ExecSandboxInput {
2799-
payload: Some(exec_sandbox_input::Payload::Stdin(buf[..n].to_vec())),
2800-
})
2801-
.is_err()
2802-
{
2803-
break;
2794+
// Stdin reader on a detached OS thread. Using std::thread (not
2795+
// spawn_blocking) so the tokio runtime shutdown doesn't wait for a
2796+
// thread blocked on stdin.read(). The thread exits when the channel
2797+
// closes (blocking_send returns Err) or stdin hits EOF.
2798+
#[cfg(unix)]
2799+
{
2800+
let stdin_tx = input_tx.clone();
2801+
std::thread::spawn(move || {
2802+
let mut stdin = std::io::stdin().lock();
2803+
let mut buf = [0u8; 4096];
2804+
loop {
2805+
match stdin.read(&mut buf) {
2806+
Ok(0) | Err(_) => break,
2807+
Ok(n) => {
2808+
if stdin_tx
2809+
.blocking_send(ExecSandboxInput {
2810+
payload: Some(exec_sandbox_input::Payload::Stdin(
2811+
buf[..n].to_vec(),
2812+
)),
2813+
})
2814+
.is_err()
2815+
{
2816+
break;
2817+
}
2818+
}
2819+
}
28042820
}
2805-
}
2806-
});
2821+
});
2822+
}
28072823

28082824
// SIGWINCH handler: forward terminal resize events.
28092825
#[cfg(unix)]
2810-
let mut resize_tx = input_tx.clone();
2811-
#[cfg(unix)]
2812-
let resize_task = tokio::spawn(async move {
2813-
let mut sig = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::window_change())
2814-
.expect("failed to register SIGWINCH handler");
2815-
while sig.recv().await.is_some() {
2816-
if let Ok((c, r)) = crossterm::terminal::size() {
2817-
let msg = ExecSandboxInput {
2818-
payload: Some(exec_sandbox_input::Payload::Resize(
2819-
ExecSandboxWindowResize {
2820-
cols: u32::from(c),
2821-
rows: u32::from(r),
2822-
},
2823-
)),
2824-
};
2825-
if resize_tx.send(msg).await.is_err() {
2826-
break;
2826+
let resize_task = {
2827+
let resize_tx = input_tx.clone();
2828+
tokio::spawn(async move {
2829+
let mut sig =
2830+
tokio::signal::unix::signal(tokio::signal::unix::SignalKind::window_change())
2831+
.expect("failed to register SIGWINCH handler");
2832+
while sig.recv().await.is_some() {
2833+
if let Ok((c, r)) = crossterm::terminal::size() {
2834+
let msg = ExecSandboxInput {
2835+
payload: Some(exec_sandbox_input::Payload::Resize(
2836+
ExecSandboxWindowResize {
2837+
cols: u32::from(c),
2838+
rows: u32::from(r),
2839+
},
2840+
)),
2841+
};
2842+
if resize_tx.send(msg).await.is_err() {
2843+
break;
2844+
}
28272845
}
28282846
}
2829-
}
2830-
});
2847+
})
2848+
};
2849+
#[cfg(unix)]
2850+
let _resize_guard = TaskGuard(resize_task);
28312851

28322852
let mut exit_code = 0i32;
28332853
let stdout = std::io::stdout();
@@ -2853,15 +2873,12 @@ async fn sandbox_exec_interactive_grpc(
28532873
}
28542874
}
28552875

2856-
#[cfg(unix)]
2857-
resize_task.abort();
2876+
drop(input_tx);
28582877

2859-
// Drop the raw mode guard to restore the terminal before exiting.
2878+
// Drop the raw mode guard to restore the terminal before returning.
28602879
drop(raw_guard);
28612880

2862-
// The spawn_blocking stdin reader is stuck on stdin.read() and cannot be
2863-
// cancelled. Force-exit so the tokio runtime doesn't hang waiting for it.
2864-
std::process::exit(exit_code)
2881+
Ok(exit_code)
28652882
}
28662883

28672884
/// Print a single YAML line with dimmed keys and regular values.

crates/openshell-server/src/grpc/sandbox.rs

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1107,8 +1107,8 @@ pub(super) async fn handle_exec_sandbox_interactive(
11071107
let command_str = build_remote_exec_command(&req)
11081108
.map_err(|e| Status::invalid_argument(format!("command construction failed: {e}")))?;
11091109
let timeout_seconds = req.timeout_seconds;
1110-
let cols = req.cols;
1111-
let rows = req.rows;
1110+
let cols = if req.cols == 0 { 80 } else { req.cols };
1111+
let rows = if req.rows == 0 { 24 } else { req.rows };
11121112

11131113
let sandbox_id = sandbox.object_id().to_string();
11141114

@@ -1565,32 +1565,35 @@ async fn run_interactive_exec_with_russh(
15651565
}
15661566
}
15671567
let _ = write_half.eof().await;
1568+
let _ = write_half.close().await;
15681569
});
15691570

15701571
let mut exit_code: Option<i32> = None;
15711572
while let Some(msg) = read_half.wait().await {
15721573
match msg {
15731574
ChannelMsg::Data { data } => {
1574-
let _ = tx
1575-
.send(Ok(ExecSandboxEvent {
1576-
payload: Some(openshell_core::proto::exec_sandbox_event::Payload::Stdout(
1577-
ExecSandboxStdout {
1578-
data: data.to_vec(),
1579-
},
1580-
)),
1581-
}))
1582-
.await;
1575+
let event = Ok(ExecSandboxEvent {
1576+
payload: Some(openshell_core::proto::exec_sandbox_event::Payload::Stdout(
1577+
ExecSandboxStdout {
1578+
data: data.to_vec(),
1579+
},
1580+
)),
1581+
});
1582+
if tx.send(event).await.is_err() {
1583+
break;
1584+
}
15831585
}
15841586
ChannelMsg::ExtendedData { data, .. } => {
1585-
let _ = tx
1586-
.send(Ok(ExecSandboxEvent {
1587-
payload: Some(openshell_core::proto::exec_sandbox_event::Payload::Stderr(
1588-
ExecSandboxStderr {
1589-
data: data.to_vec(),
1590-
},
1591-
)),
1592-
}))
1593-
.await;
1587+
let event = Ok(ExecSandboxEvent {
1588+
payload: Some(openshell_core::proto::exec_sandbox_event::Payload::Stderr(
1589+
ExecSandboxStderr {
1590+
data: data.to_vec(),
1591+
},
1592+
)),
1593+
});
1594+
if tx.send(event).await.is_err() {
1595+
break;
1596+
}
15941597
}
15951598
ChannelMsg::ExitStatus { exit_status } => {
15961599
let converted = i32::try_from(exit_status).unwrap_or(i32::MAX);

0 commit comments

Comments
 (0)