From 92874cf5da9e11c8c03a4f1e099c90fd9f2f913f Mon Sep 17 00:00:00 2001 From: yishuiliunian Date: Sat, 4 Apr 2026 05:43:22 +0800 Subject: [PATCH] refactor: replace raw u64 timeouts with Duration for type-safe time handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Timeout values were passed as raw u64 (milliseconds or seconds depending on context) across Backend trait, error types, shell execution, and the Bash tool. This caused unit confusion bugs — notably tests passing 200 (seconds, not ms) causing a 200s test wait. Replace all internal timeout parameters with std::time::Duration: - Backend trait: exec/exec_streaming now accept Duration - ToolIoError::Timeout and ToolError::Timeout now hold Duration - ResourceLimits: default_timeout_ms/fetch_timeout_secs → Duration fields - Shell/shell_stream: accept Duration directly, no from_millis conversion - Bash tool: MAX_TIMEOUT as Duration, bg_ops/format use Duration - TimeoutSecs: output changed from to_millis_clamped to to_duration_clamped Fix test timeout values that were incorrectly specified as milliseconds: - background_task_test: 5000 → 5 (seconds) - background_task_edge_test: 200 → 1 (seconds), fixing 200s slow test --- crates/loopal-backend/src/limits.rs | 16 +++++----- crates/loopal-backend/src/local.rs | 9 +++--- crates/loopal-backend/src/net.rs | 2 +- crates/loopal-backend/src/shell.rs | 6 ++-- crates/loopal-backend/src/shell_stream.rs | 6 ++-- crates/loopal-error/src/errors.rs | 5 ++-- crates/loopal-error/src/io_error.rs | 5 ++-- .../tests/suite/error_conversion_test.rs | 6 ++-- crates/loopal-tool-api/src/backend.rs | 9 +++--- crates/loopal-tool-api/src/backend_types.rs | 14 +++++++-- .../tests/suite/background_task_edge_test.rs | 2 +- .../tests/suite/background_task_test.rs | 2 +- crates/tools/process/bash/src/bg_ops.rs | 5 ++-- crates/tools/process/bash/src/format.rs | 4 +-- crates/tools/process/bash/src/lib.rs | 29 +++++++++---------- 15 files changed, 66 insertions(+), 54 deletions(-) diff --git a/crates/loopal-backend/src/limits.rs b/crates/loopal-backend/src/limits.rs index 8e7326d9..b90aa732 100644 --- a/crates/loopal-backend/src/limits.rs +++ b/crates/loopal-backend/src/limits.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use loopal_tool_api::{DEFAULT_MAX_OUTPUT_BYTES, DEFAULT_MAX_OUTPUT_LINES}; /// Resource limits applied by `LocalBackend`. @@ -15,10 +17,10 @@ pub struct ResourceLimits { pub max_grep_matches: usize, /// Maximum HTTP response body size in bytes. pub max_fetch_bytes: usize, - /// Default shell command timeout (ms). - pub default_timeout_ms: u64, - /// HTTP fetch timeout (seconds). - pub fetch_timeout_secs: u64, + /// Default shell command timeout. + pub default_timeout: Duration, + /// HTTP fetch timeout. + pub fetch_timeout: Duration, } impl Default for ResourceLimits { @@ -29,9 +31,9 @@ impl Default for ResourceLimits { max_output_bytes: DEFAULT_MAX_OUTPUT_BYTES, max_glob_results: 10_000, max_grep_matches: 500, - max_fetch_bytes: 5 * 1024 * 1024, // 5 MB - default_timeout_ms: 300_000, // 5 min - fetch_timeout_secs: 30, + max_fetch_bytes: 5 * 1024 * 1024, // 5 MB + default_timeout: Duration::from_secs(300), // 5 min + fetch_timeout: Duration::from_secs(30), } } } diff --git a/crates/loopal-backend/src/local.rs b/crates/loopal-backend/src/local.rs index c3523204..5e493113 100644 --- a/crates/loopal-backend/src/local.rs +++ b/crates/loopal-backend/src/local.rs @@ -1,6 +1,7 @@ //! `LocalBackend` — production `Backend` for local filesystem + OS sandbox. use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use async_trait::async_trait; use loopal_config::ResolvedPolicy; @@ -151,12 +152,12 @@ impl Backend for LocalBackend { &self.cwd } - async fn exec(&self, command: &str, timeout_ms: u64) -> Result { + async fn exec(&self, command: &str, timeout: Duration) -> Result { shell::exec_command( &self.cwd, self.policy.as_ref(), command, - timeout_ms, + timeout, &self.limits, ) .await @@ -165,14 +166,14 @@ impl Backend for LocalBackend { async fn exec_streaming( &self, command: &str, - timeout_ms: u64, + timeout: Duration, tail: Arc, ) -> Result { shell_stream::exec_command_streaming( &self.cwd, self.policy.as_ref(), command, - timeout_ms, + timeout, &self.limits, tail, ) diff --git a/crates/loopal-backend/src/net.rs b/crates/loopal-backend/src/net.rs index 006c9d76..9b9ecc5e 100644 --- a/crates/loopal-backend/src/net.rs +++ b/crates/loopal-backend/src/net.rs @@ -29,7 +29,7 @@ pub async fn fetch_url( } let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(limits.fetch_timeout_secs)) + .timeout(limits.fetch_timeout) .build() .map_err(|e| ToolIoError::Network(e.to_string()))?; diff --git a/crates/loopal-backend/src/shell.rs b/crates/loopal-backend/src/shell.rs index 6af674d9..fc946eea 100644 --- a/crates/loopal-backend/src/shell.rs +++ b/crates/loopal-backend/src/shell.rs @@ -20,7 +20,7 @@ pub async fn exec_command( cwd: &Path, policy: Option<&ResolvedPolicy>, command: &str, - timeout_ms: u64, + timeout: Duration, limits: &ResourceLimits, ) -> Result { let (program, args, env) = build_command(cwd, policy, command); @@ -34,9 +34,9 @@ pub async fn exec_command( } } - let output = tokio::time::timeout(Duration::from_millis(timeout_ms), cmd.output()) + let output = tokio::time::timeout(timeout, cmd.output()) .await - .map_err(|_| ToolIoError::Timeout(timeout_ms))? + .map_err(|_| ToolIoError::Timeout(timeout))? .map_err(|e| ToolIoError::ExecFailed(format!("spawn failed: {e}")))?; let stdout_raw = String::from_utf8_lossy(&output.stdout); diff --git a/crates/loopal-backend/src/shell_stream.rs b/crates/loopal-backend/src/shell_stream.rs index 0e6adf1b..b3e7729d 100644 --- a/crates/loopal-backend/src/shell_stream.rs +++ b/crates/loopal-backend/src/shell_stream.rs @@ -50,7 +50,7 @@ pub async fn exec_command_streaming( cwd: &Path, policy: Option<&ResolvedPolicy>, command: &str, - timeout_ms: u64, + timeout: Duration, limits: &ResourceLimits, tail: Arc, ) -> Result { @@ -101,7 +101,7 @@ pub async fn exec_command_streaming( // only runs after pipes close (readers finish → child exited), so on // timeout the child is still inside child_arc (not taken/dropped). let child_for_wait = Arc::clone(&child_arc); - let wait_result = tokio::time::timeout(Duration::from_millis(timeout_ms), async { + let wait_result = tokio::time::timeout(timeout, async { let (r1, r2) = tokio::join!(stdout_task, stderr_task); let _ = (r1, r2); let child_opt = child_for_wait.lock().unwrap().take(); @@ -126,7 +126,7 @@ pub async fn exec_command_streaming( Err(_timeout) => { let partial = tail.snapshot(); Ok(ExecOutcome::TimedOut { - timeout_ms, + timeout, partial_output: partial, handle: ProcessHandle(Box::new(TimedOutProcessData { child: child_arc, diff --git a/crates/loopal-error/src/errors.rs b/crates/loopal-error/src/errors.rs index 48015f6c..636a30c0 100644 --- a/crates/loopal-error/src/errors.rs +++ b/crates/loopal-error/src/errors.rs @@ -1,3 +1,4 @@ +use std::time::Duration; use thiserror::Error; #[derive(Debug, Error)] @@ -71,8 +72,8 @@ pub enum ToolError { #[error("Execution failed: {0}")] ExecutionFailed(String), - #[error("Timeout after {0}ms")] - Timeout(u64), + #[error("Timeout after {0:?}")] + Timeout(Duration), } #[derive(Debug, Error)] diff --git a/crates/loopal-error/src/io_error.rs b/crates/loopal-error/src/io_error.rs index 426a905a..ee7af99b 100644 --- a/crates/loopal-error/src/io_error.rs +++ b/crates/loopal-error/src/io_error.rs @@ -1,3 +1,4 @@ +use std::time::Duration; use thiserror::Error; /// Opaque handle for passing implementation-specific data through error @@ -37,8 +38,8 @@ pub enum ToolIoError { #[error("exec failed: {0}")] ExecFailed(String), - #[error("timeout after {0}ms")] - Timeout(u64), + #[error("timeout after {0:?}")] + Timeout(Duration), #[error("network error: {0}")] Network(String), diff --git a/crates/loopal-error/tests/suite/error_conversion_test.rs b/crates/loopal-error/tests/suite/error_conversion_test.rs index 0bb6d648..9afe1599 100644 --- a/crates/loopal-error/tests/suite/error_conversion_test.rs +++ b/crates/loopal-error/tests/suite/error_conversion_test.rs @@ -27,7 +27,7 @@ fn test_loopal_error_from_provider_error() { #[test] fn test_loopal_error_from_tool_error() { - let tool_err = ToolError::Timeout(5000); + let tool_err = ToolError::Timeout(std::time::Duration::from_secs(5)); let err: LoopalError = tool_err.into(); assert!(matches!(err, LoopalError::Tool(_))); } @@ -75,8 +75,8 @@ fn test_tool_error_display_execution_failed() { #[test] fn test_tool_error_display_timeout() { - let err = ToolError::Timeout(30000); - assert_eq!(format!("{err}"), "Timeout after 30000ms"); + let err = ToolError::Timeout(std::time::Duration::from_secs(30)); + assert_eq!(format!("{err}"), "Timeout after 30s"); } // --- ConfigError Display --- diff --git a/crates/loopal-tool-api/src/backend.rs b/crates/loopal-tool-api/src/backend.rs index 47cfe171..a6645d83 100644 --- a/crates/loopal-tool-api/src/backend.rs +++ b/crates/loopal-tool-api/src/backend.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use async_trait::async_trait; use loopal_error::{ProcessHandle, ToolIoError}; @@ -21,7 +22,7 @@ pub enum ExecOutcome { /// The `handle` carries the child so the caller can register it as a /// background task. TimedOut { - timeout_ms: u64, + timeout: Duration, partial_output: String, handle: ProcessHandle, }, @@ -99,7 +100,7 @@ pub trait Backend: Send + Sync { // --- Command execution --- /// Execute a shell command synchronously (with timeout). - async fn exec(&self, command: &str, timeout_ms: u64) -> Result; + async fn exec(&self, command: &str, timeout: Duration) -> Result; /// Execute a shell command with streaming output capture. /// @@ -112,10 +113,10 @@ pub trait Backend: Send + Sync { async fn exec_streaming( &self, command: &str, - timeout_ms: u64, + timeout: Duration, _tail: Arc, ) -> Result { - self.exec(command, timeout_ms) + self.exec(command, timeout) .await .map(ExecOutcome::Completed) } diff --git a/crates/loopal-tool-api/src/backend_types.rs b/crates/loopal-tool-api/src/backend_types.rs index 38946151..584119fc 100644 --- a/crates/loopal-tool-api/src/backend_types.rs +++ b/crates/loopal-tool-api/src/backend_types.rs @@ -3,6 +3,8 @@ //! These are deliberately simple structs so that tool crates only depend //! on `loopal-tool-api` (a leaf crate) for their I/O interface. +use std::time::Duration; + /// Result of a file read operation. #[derive(Debug, Clone)] pub struct ReadResult { @@ -167,9 +169,15 @@ impl TimeoutSecs { self.0 } - /// Convert to milliseconds, clamped to `max_ms`. - pub fn to_millis_clamped(&self, max_ms: u64) -> u64 { - (self.0.saturating_mul(1000)).min(max_ms) + /// Convert to a `Duration`. + pub const fn to_duration(&self) -> Duration { + Duration::from_secs(self.0) + } + + /// Convert to a `Duration`, clamped to `max`. + pub fn to_duration_clamped(&self, max: Duration) -> Duration { + let d = Duration::from_secs(self.0); + if d > max { max } else { d } } } diff --git a/crates/tools/process/background/tests/suite/background_task_edge_test.rs b/crates/tools/process/background/tests/suite/background_task_edge_test.rs index 87ddf279..ecee3e06 100644 --- a/crates/tools/process/background/tests/suite/background_task_edge_test.rs +++ b/crates/tools/process/background/tests/suite/background_task_edge_test.rs @@ -115,7 +115,7 @@ async fn test_output_timeout() { let output = bash .execute( - json!({"process_id": pid, "block": true, "timeout": 200}), + json!({"process_id": pid, "block": true, "timeout": 1}), &ctx, ) .await diff --git a/crates/tools/process/background/tests/suite/background_task_test.rs b/crates/tools/process/background/tests/suite/background_task_test.rs index 65751a63..d2a422c9 100644 --- a/crates/tools/process/background/tests/suite/background_task_test.rs +++ b/crates/tools/process/background/tests/suite/background_task_test.rs @@ -78,7 +78,7 @@ async fn test_bash_background_and_output() { let output = bash .execute( - json!({"process_id": pid, "block": true, "timeout": 5000}), + json!({"process_id": pid, "block": true, "timeout": 5}), &ctx, ) .await diff --git a/crates/tools/process/bash/src/bg_ops.rs b/crates/tools/process/bash/src/bg_ops.rs index 49d7c849..9c75c42a 100644 --- a/crates/tools/process/bash/src/bg_ops.rs +++ b/crates/tools/process/bash/src/bg_ops.rs @@ -11,7 +11,7 @@ pub async fn bg_output( store: &BackgroundTaskStore, process_id: &str, block: bool, - timeout_ms: u64, + timeout: Duration, ) -> ToolResult { let cloned = store.with_task(process_id, |task| { ( @@ -26,7 +26,6 @@ pub async fn bg_output( }; if block { - let deadline = Duration::from_millis(timeout_ms); let wait = async { loop { if *watch_rx.borrow() != TaskStatus::Running { @@ -37,7 +36,7 @@ pub async fn bg_output( } } }; - if tokio::time::timeout(deadline, wait).await.is_err() { + if tokio::time::timeout(timeout, wait).await.is_err() { let output = output_buf.lock().unwrap().clone(); return ToolResult::success(format!("{output}\n[Status: Running (timed out waiting)]")); } diff --git a/crates/tools/process/bash/src/format.rs b/crates/tools/process/bash/src/format.rs index 5614c9c4..e25c558d 100644 --- a/crates/tools/process/bash/src/format.rs +++ b/crates/tools/process/bash/src/format.rs @@ -32,10 +32,10 @@ pub fn format_exec_result(output: ExecResult) -> ToolResult { /// Format a timeout-to-background conversion into a success `ToolResult`. pub fn format_converted_to_background( task_id: &str, - timeout_ms: u64, + timeout: std::time::Duration, partial_output: &str, ) -> ToolResult { - let timeout_secs = timeout_ms / 1000; + let timeout_secs = timeout.as_secs(); let mut msg = format!( "Command timed out after {timeout_secs}s and was moved to background.\n\ process_id: {task_id}\n\ diff --git a/crates/tools/process/bash/src/lib.rs b/crates/tools/process/bash/src/lib.rs index 74ea2045..7c06619b 100644 --- a/crates/tools/process/bash/src/lib.rs +++ b/crates/tools/process/bash/src/lib.rs @@ -13,6 +13,7 @@ mod bg_ops; mod format; use std::sync::Arc; +use std::time::Duration; use async_trait::async_trait; use loopal_error::{LoopalError, ToolIoError}; @@ -36,7 +37,7 @@ impl BashTool { const DEFAULT_TIMEOUT_SECS: u64 = 300; const DEFAULT_BG_TIMEOUT_SECS: u64 = 30; -const MAX_TIMEOUT_MS: u64 = 600_000; +const MAX_TIMEOUT: Duration = Duration::from_secs(600); #[async_trait] impl Tool for BashTool { @@ -93,7 +94,7 @@ impl Tool for BashTool { &self.store, pid, block, - timeout.to_millis_clamped(MAX_TIMEOUT_MS), + timeout.to_duration_clamped(MAX_TIMEOUT), ) .await); } @@ -128,16 +129,16 @@ async fn exec_foreground( input: &Value, ctx: &ToolContext, ) -> Result { - let timeout_ms = - TimeoutSecs::from_tool_input(input, DEFAULT_TIMEOUT_SECS).to_millis_clamped(MAX_TIMEOUT_MS); + let timeout = + TimeoutSecs::from_tool_input(input, DEFAULT_TIMEOUT_SECS).to_duration_clamped(MAX_TIMEOUT); let exec_result = if let Some(ref tail) = ctx.output_tail { ctx.backend - .exec_streaming(command, timeout_ms, tail.clone()) + .exec_streaming(command, timeout, tail.clone()) .await } else { ctx.backend - .exec(command, timeout_ms) + .exec(command, timeout) .await .map(ExecOutcome::Completed) }; @@ -145,7 +146,7 @@ async fn exec_foreground( match exec_result { Ok(ExecOutcome::Completed(output)) => Ok(format::format_exec_result(output)), Ok(ExecOutcome::TimedOut { - timeout_ms, + timeout, partial_output, handle, }) => { @@ -153,13 +154,11 @@ async fn exec_foreground( bg_convert::register(store, handle, command).unwrap_or_else(|| "(unknown)".into()); Ok(format::format_converted_to_background( &task_id, - timeout_ms, + timeout, &partial_output, )) } - Err(ToolIoError::Timeout(ms)) => { - Err(LoopalError::Tool(loopal_error::ToolError::Timeout(ms))) - } + Err(ToolIoError::Timeout(d)) => Err(LoopalError::Tool(loopal_error::ToolError::Timeout(d))), Err(e) => Ok(ToolResult::error(e.to_string())), } } @@ -169,16 +168,16 @@ mod tests { use super::*; #[test] - fn timeout_secs_converts_to_millis() { + fn timeout_secs_converts_to_duration() { let t = TimeoutSecs::from_tool_input(&json!({"timeout": 120}), 0); assert_eq!(t.as_secs(), 120); - assert_eq!(t.to_millis_clamped(MAX_TIMEOUT_MS), 120_000); + assert_eq!(t.to_duration_clamped(MAX_TIMEOUT), Duration::from_secs(120)); } #[test] fn timeout_secs_clamps_to_max() { let t = TimeoutSecs::from_tool_input(&json!({"timeout": 700}), 0); - assert_eq!(t.to_millis_clamped(MAX_TIMEOUT_MS), MAX_TIMEOUT_MS); + assert_eq!(t.to_duration_clamped(MAX_TIMEOUT), MAX_TIMEOUT); } #[test] @@ -193,7 +192,7 @@ mod tests { fn timeout_secs_zero_yields_zero() { let t = TimeoutSecs::from_tool_input(&json!({"timeout": 0}), DEFAULT_TIMEOUT_SECS); assert_eq!(t.as_secs(), 0); - assert_eq!(t.to_millis_clamped(MAX_TIMEOUT_MS), 0); + assert_eq!(t.to_duration_clamped(MAX_TIMEOUT), Duration::ZERO); } #[test]