Skip to content

Commit cc87697

Browse files
Sewer56Sewer56
authored
Merge pull request #45 from Sewer56/fix/bash-timeout-kill-failure
Fixed: surface kill() failures in bash timeout handling
2 parents 373b4c9 + 5ec6a26 commit cc87697

5 files changed

Lines changed: 116 additions & 52 deletions

File tree

‎src/llm-coding-tools-core/src/error.rs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,15 @@ pub enum ToolError {
3333
#[error("timeout: {0}")]
3434
Timeout(String),
3535

36+
/// Timeout with kill failure - process may still be running.
37+
#[error("timeout: {message}\n(kill failed: {kill_error})")]
38+
TimeoutWithKillFailure {
39+
/// Timeout message including context.
40+
message: String,
41+
/// Kill error message.
42+
kill_error: String,
43+
},
44+
3645
/// Validation failed.
3746
#[error("validation error: {0}")]
3847
Validation(String),
@@ -74,4 +83,15 @@ mod tests {
7483
let err: ToolError = glob_err.into();
7584
assert!(matches!(err, ToolError::InvalidPattern(_)));
7685
}
86+
87+
#[test]
88+
fn timeout_with_kill_failure_displays_both_contexts() {
89+
let err = ToolError::TimeoutWithKillFailure {
90+
message: "command timed out after 100ms".into(),
91+
kill_error: "permission denied".into(),
92+
};
93+
let display = err.to_string();
94+
assert!(display.contains("command timed out after 100ms"));
95+
assert!(display.contains("kill failed: permission denied"));
96+
}
7797
}

‎src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
//! Blocking shell command execution.
22
3-
use super::{BashOutput, PIPE_BUFFER_CAPACITY};
3+
use super::{
4+
timeout_error_with_kill_failure, timeout_message_with_buffered_output, BashOutput,
5+
PIPE_BUFFER_CAPACITY,
6+
};
47
use crate::error::{ToolError, ToolResult};
58
use process_wrap::std::*;
69
use std::io::Read;
@@ -9,6 +12,12 @@ use std::process::Stdio;
912
use std::thread;
1013
use std::time::{Duration, Instant};
1114

15+
enum WaitOutcome {
16+
Exited(std::process::ExitStatus),
17+
TimedOut { kill_error: Option<std::io::Error> },
18+
WaitError(std::io::Error),
19+
}
20+
1221
/// Executes a shell command with optional working directory and timeout.
1322
///
1423
/// Uses bash on Unix, cmd on Windows. Process tree is killed on timeout via:
@@ -90,22 +99,20 @@ pub fn execute_command(
9099

91100
let start = Instant::now();
92101

93-
// Poll for completion with timeout
94-
let exit_status = loop {
102+
// Poll for completion with timeout.
103+
let wait_outcome = loop {
95104
match child.try_wait() {
96-
Ok(Some(status)) => break Ok(status),
105+
Ok(Some(status)) => break WaitOutcome::Exited(status),
97106
Ok(None) => {
98107
if start.elapsed() >= timeout {
99108
// Kill entire process tree via Job Object (Windows) or process group (Unix)
100-
let _ = child.kill();
101-
break Err(ToolError::Timeout(format!(
102-
"command timed out after {}ms",
103-
timeout.as_millis()
104-
)));
109+
break WaitOutcome::TimedOut {
110+
kill_error: child.kill().err(),
111+
};
105112
}
106113
thread::sleep(Duration::from_millis(10));
107114
}
108-
Err(e) => break Err(ToolError::Execution(e.to_string())),
115+
Err(e) => break WaitOutcome::WaitError(e),
109116
}
110117
};
111118

@@ -118,13 +125,17 @@ pub fn execute_command(
118125
.map_err(|_| ToolError::Execution("stderr reader thread panicked".to_string()))?;
119126

120127
// Return result
121-
match exit_status {
122-
Ok(status) => Ok(BashOutput {
128+
match wait_outcome {
129+
WaitOutcome::Exited(status) => Ok(BashOutput {
123130
exit_code: status.code(),
124131
stdout: String::from_utf8_lossy(&stdout_data).into_owned(),
125132
stderr: String::from_utf8_lossy(&stderr_data).into_owned(),
126133
}),
127-
Err(e) => Err(e),
134+
WaitOutcome::TimedOut { kill_error } => Err(timeout_error_with_kill_failure(
135+
timeout_message_with_buffered_output(timeout, &stdout_data, &stderr_data),
136+
kill_error.map(|e| e.to_string()),
137+
)),
138+
WaitOutcome::WaitError(e) => Err(ToolError::Execution(e.to_string())),
128139
}
129140
}
130141

@@ -166,7 +177,10 @@ mod tests {
166177
};
167178

168179
let result = execute_command(cmd, None, Duration::from_millis(100));
169-
assert!(matches!(result, Err(ToolError::Timeout(_))));
180+
assert!(matches!(
181+
result,
182+
Err(ToolError::Timeout(_) | ToolError::TimeoutWithKillFailure { .. })
183+
));
170184
}
171185

172186
#[test]

‎src/llm-coding-tools-core/src/tools/bash/mod.rs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,54 @@
11
//! Shell command execution operation.
22
3+
use crate::error::ToolError;
34
use crate::ToolOutput;
45
use core::fmt::Write;
56
use serde::Serialize;
7+
use std::time::Duration;
68

79
/// Default buffer capacity for stdout/stderr pipe reads.
810
/// 32KB covers typical command output without reallocations.
911
const PIPE_BUFFER_CAPACITY: usize = 32 * 1024;
1012

13+
#[inline]
14+
fn timeout_message_with_buffered_output(
15+
timeout: Duration,
16+
stdout_data: &[u8],
17+
stderr_data: &[u8],
18+
) -> String {
19+
let stdout = String::from_utf8_lossy(stdout_data);
20+
let stderr = String::from_utf8_lossy(stderr_data);
21+
22+
let mut message = String::with_capacity(stdout.len() + stderr.len() + 64);
23+
let _ = write!(message, "command timed out after {}ms", timeout.as_millis());
24+
25+
if !stdout.is_empty() {
26+
message.push('\n');
27+
message.push_str(&stdout);
28+
}
29+
30+
if !stderr.is_empty() {
31+
if stdout.is_empty() || !stdout.ends_with('\n') {
32+
message.push('\n');
33+
}
34+
message.push_str("[stderr]\n");
35+
message.push_str(&stderr);
36+
}
37+
38+
message
39+
}
40+
41+
#[inline]
42+
fn timeout_error_with_kill_failure(message: String, kill_error: Option<String>) -> ToolError {
43+
match kill_error {
44+
Some(kill_error) => ToolError::TimeoutWithKillFailure {
45+
message,
46+
kill_error,
47+
},
48+
None => ToolError::Timeout(message),
49+
}
50+
}
51+
1152
/// Result of shell command execution.
1253
#[derive(Debug, Clone, Serialize)]
1354
pub struct BashOutput {

‎src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs‎

Lines changed: 14 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
//! Tokio-based async shell command execution.
22
3-
use super::{BashOutput, PIPE_BUFFER_CAPACITY};
3+
use super::{
4+
timeout_error_with_kill_failure, timeout_message_with_buffered_output, BashOutput,
5+
PIPE_BUFFER_CAPACITY,
6+
};
47
use crate::error::{ToolError, ToolResult};
5-
use core::fmt::Write;
68
use parking_lot::Mutex;
79
use process_wrap::tokio::*;
810
use std::path::Path;
@@ -79,35 +81,6 @@ async fn await_pipe_drain_task_with_grace(task: PipeDrainTask, grace: Duration)
7981
take_pipe_buffer(buffer)
8082
}
8183

82-
#[inline]
83-
fn timeout_with_buffered_output(
84-
timeout: Duration,
85-
stdout_data: &[u8],
86-
stderr_data: &[u8],
87-
) -> ToolError {
88-
let stdout = String::from_utf8_lossy(stdout_data);
89-
let stderr = String::from_utf8_lossy(stderr_data);
90-
91-
// Base message + outputs + stderr label.
92-
let mut message = String::with_capacity(stdout.len() + stderr.len() + 64);
93-
let _ = write!(message, "command timed out after {}ms", timeout.as_millis());
94-
95-
if !stdout.is_empty() {
96-
message.push('\n');
97-
message.push_str(&stdout);
98-
}
99-
100-
if !stderr.is_empty() {
101-
if stdout.is_empty() || !stdout.ends_with('\n') {
102-
message.push('\n');
103-
}
104-
message.push_str("[stderr]\n");
105-
message.push_str(&stderr);
106-
}
107-
108-
ToolError::Timeout(message)
109-
}
110-
11184
/// Executes a shell command with optional working directory and timeout.
11285
///
11386
/// Uses bash on Unix, cmd on Windows. Process tree is killed on timeout via:
@@ -201,17 +174,16 @@ pub async fn execute_command(
201174
None => {
202175
// Timeout: explicitly kill the process tree (Job Object on Windows,
203176
// process group on Unix), then briefly await pipe drains for buffered output.
204-
let _ = Pin::from(child.kill()).await;
177+
let kill_result = Pin::from(child.kill()).await;
205178

206179
let (stdout_data, stderr_data) = tokio::join!(
207180
await_pipe_drain_task_with_grace(stdout_task, PIPE_DRAIN_GRACE_PERIOD),
208181
await_pipe_drain_task_with_grace(stderr_task, PIPE_DRAIN_GRACE_PERIOD)
209182
);
210183

211-
Err(timeout_with_buffered_output(
212-
timeout,
213-
&stdout_data,
214-
&stderr_data,
184+
Err(timeout_error_with_kill_failure(
185+
timeout_message_with_buffered_output(timeout, &stdout_data, &stderr_data),
186+
kill_result.err().map(|e| e.to_string()),
215187
))
216188
}
217189
}
@@ -259,7 +231,10 @@ mod tests {
259231
};
260232

261233
let result = execute_command(cmd, None, Duration::from_millis(100)).await;
262-
assert!(matches!(result, Err(ToolError::Timeout(_))));
234+
assert!(matches!(
235+
result,
236+
Err(ToolError::Timeout(_) | ToolError::TimeoutWithKillFailure { .. })
237+
));
263238
}
264239

265240
#[tokio::test]
@@ -272,7 +247,8 @@ mod tests {
272247

273248
let result = execute_command(cmd, None, Duration::from_millis(500)).await;
274249
match result {
275-
Err(ToolError::Timeout(message)) => {
250+
Err(ToolError::Timeout(message))
251+
| Err(ToolError::TimeoutWithKillFailure { message, .. }) => {
276252
assert!(message.contains("stdout-before-timeout"));
277253
assert!(message.contains("stderr-before-timeout"));
278254
}

‎src/llm-coding-tools-serdesai/src/convert.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ pub(crate) fn core_error_to_serdes(tool_name: &str, err: CoreError) -> SerdesErr
7979
| CoreError::Http(_)
8080
| CoreError::Execution(_)
8181
| CoreError::Timeout(_)
82+
| CoreError::TimeoutWithKillFailure { .. }
8283
| CoreError::Json(_) => SerdesError::execution_failed(err.to_string()),
8384
}
8485
}
@@ -151,6 +152,18 @@ mod tests {
151152
assert!(!matches!(serdes_err, SerdesError::ValidationFailed { .. }));
152153
}
153154

155+
#[test]
156+
fn timeout_with_kill_failure_maps_to_execution_failed() {
157+
let core_err = CoreError::TimeoutWithKillFailure {
158+
message: "timed out".into(),
159+
kill_error: "operation not permitted".into(),
160+
};
161+
let serdes_err = core_error_to_serdes("test_tool", core_err);
162+
assert!(!matches!(serdes_err, SerdesError::ValidationFailed { .. }));
163+
assert!(serdes_err.message().contains("timed out"));
164+
assert!(serdes_err.message().contains("operation not permitted"));
165+
}
166+
154167
#[test]
155168
fn to_serdes_result_maps_success() {
156169
let core_result: CoreResult<ToolOutput> = Ok(ToolOutput::new("success"));

0 commit comments

Comments
 (0)