From 1c3eea1053875ee6ebebcb1dab35245cc8567352 Mon Sep 17 00:00:00 2001 From: Fabro Date: Mon, 4 May 2026 19:13:15 +0000 Subject: [PATCH] fabro(01KQT1V2W1R6ZH72CFT2QDJ39Q): implement (succeeded) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fabro-Run: 01KQT1V2W1R6ZH72CFT2QDJ39Q Fabro-Completed: 5 Fabro-Checkpoint: 89ea5f732a8e0d373da4cfe74e5d476e94ac47ff ⚒️ Generated with [Fabro](https://fabro.sh) --- docs/public/reference/sdk.mdx | 4 +- lib/crates/fabro-agent/README.md | 4 +- lib/crates/fabro-agent/src/cli.rs | 2 +- lib/crates/fabro-agent/src/memory.rs | 100 ++- lib/crates/fabro-agent/src/session.rs | 212 +++++-- lib/crates/fabro-agent/src/skills.rs | 48 +- lib/crates/fabro-agent/src/subagent.rs | 2 +- lib/crates/fabro-agent/src/v4a_patch.rs | 2 +- .../fabro-agent/tests/it/parity_matrix.rs | 8 +- .../fabro-cli/src/commands/run/runner.rs | 42 +- .../fabro-cli/tests/it/workflow/real_cli.rs | 1 + lib/crates/fabro-core/src/executor.rs | 50 +- lib/crates/fabro-core/src/stall.rs | 37 +- lib/crates/fabro-retro/src/retro_agent.rs | 5 +- lib/crates/fabro-sandbox/src/daytona/mod.rs | 17 +- lib/crates/fabro-sandbox/src/docker.rs | 14 +- lib/crates/fabro-sandbox/src/local.rs | 14 +- lib/crates/fabro-sandbox/src/sandbox.rs | 13 +- lib/crates/fabro-sandbox/src/worktree.rs | 2 +- .../tests/daytona_streaming_live.rs | 11 +- .../fabro-sandbox/tests/docker_streaming.rs | 2 +- lib/crates/fabro-server/Cargo.toml | 3 +- lib/crates/fabro-server/src/server.rs | 17 +- .../src/server/handler/lifecycle.rs | 16 +- lib/crates/fabro-types/src/run_event/misc.rs | 14 + lib/crates/fabro-types/src/run_event/mod.rs | 6 + .../fabro-workflow/src/devcontainer_bridge.rs | 126 ++-- .../fabro-workflow/src/event/convert.rs | 62 ++ lib/crates/fabro-workflow/src/event/events.rs | 26 + lib/crates/fabro-workflow/src/event/names.rs | 2 + .../fabro-workflow/src/event/stored_fields.rs | 4 +- .../fabro-workflow/src/handler/agent.rs | 13 + .../fabro-workflow/src/handler/command.rs | 13 +- .../fabro-workflow/src/handler/fan_in.rs | 5 + .../fabro-workflow/src/handler/human.rs | 7 +- .../fabro-workflow/src/handler/llm/api.rs | 581 +++++++++++++++--- .../fabro-workflow/src/handler/llm/cli.rs | 545 +++++++++++++--- .../src/handler/manager_loop.rs | 17 +- lib/crates/fabro-workflow/src/handler/mod.rs | 1 - .../fabro-workflow/src/handler/parallel.rs | 3 + .../fabro-workflow/src/handler/prompt.rs | 9 +- .../fabro-workflow/src/lifecycle/git.rs | 4 +- .../fabro-workflow/src/operations/fork.rs | 2 + .../fabro-workflow/src/operations/start.rs | 26 +- .../fabro-workflow/src/pipeline/execute.rs | 4 +- .../src/pipeline/execute/tests.rs | 20 +- .../fabro-workflow/src/pipeline/finalize.rs | 6 +- .../fabro-workflow/src/pipeline/initialize.rs | 27 +- .../fabro-workflow/src/pipeline/retro.rs | 6 +- lib/crates/fabro-workflow/src/run_metadata.rs | 2 +- lib/crates/fabro-workflow/src/run_options.rs | 8 +- lib/crates/fabro-workflow/src/services.rs | 63 +- .../tests/it/daytona_integration.rs | 16 +- .../tests/it/git_integration.rs | 3 +- .../fabro-workflow/tests/it/integration.rs | 435 +++++++------ 55 files changed, 1965 insertions(+), 717 deletions(-) diff --git a/docs/public/reference/sdk.mdx b/docs/public/reference/sdk.mdx index 4e907270b..c070e11cf 100644 --- a/docs/public/reference/sdk.mdx +++ b/docs/public/reference/sdk.mdx @@ -42,7 +42,7 @@ async fn main() -> Result<(), Box> { let config = SessionOptions::default(); let mut session = Session::new(client, profile, sandbox, config); - session.initialize().await; + session.initialize().await?; // Subscribe to events before sending input let mut events = session.subscribe(); @@ -936,4 +936,4 @@ Register it on the client: ```rust client.register_provider(Arc::new(MyProvider)).await?; -``` +``` \ No newline at end of file diff --git a/lib/crates/fabro-agent/README.md b/lib/crates/fabro-agent/README.md index 4c25523d9..ce1afce1b 100644 --- a/lib/crates/fabro-agent/README.md +++ b/lib/crates/fabro-agent/README.md @@ -140,7 +140,7 @@ let config = SessionConfig { // 5. Create and initialize the session let mut session = Session::new(client, profile, env, config, None); -session.initialize().await; +session.initialize().await?; // 6. Subscribe to events (for UI rendering) let mut rx = session.subscribe(); @@ -232,4 +232,4 @@ profile.register_subagent_tools(manager, factory, 0); - **Tool output truncation** -- Per-tool character and line limits with head/tail or tail-only truncation modes - **Environment variable filtering** -- `LocalSandbox` strips secrets (`*_API_KEY`, `*_SECRET`, `*_TOKEN`, `*_PASSWORD`, `*_CREDENTIAL`) from subprocess environments - **Command timeouts** -- Configurable per-command with process group cleanup (SIGTERM then SIGKILL) -- **Project doc discovery** -- Automatically discovers `AGENTS.md`, `CLAUDE.md`, `GEMINI.md`, or `.codex/instructions.md` based on provider, with a 32KB budget +- **Project doc discovery** -- Automatically discovers `AGENTS.md`, `CLAUDE.md`, `GEMINI.md`, or `.codex/instructions.md` based on provider, with a 32KB budget \ No newline at end of file diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 1276bba26..3b216d284 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -724,7 +724,7 @@ pub async fn run_with_args_and_client( }); // Initialize and run - session.initialize().await; + session.initialize().await?; let result = session.process_input(&args.prompt).await; if matches!(output_format, OutputFormat::Text) { diff --git a/lib/crates/fabro-agent/src/memory.rs b/lib/crates/fabro-agent/src/memory.rs index 8c5554537..071435c30 100644 --- a/lib/crates/fabro-agent/src/memory.rs +++ b/lib/crates/fabro-agent/src/memory.rs @@ -1,8 +1,10 @@ use std::collections::HashSet; use fabro_model::Provider; +use tokio_util::sync::CancellationToken; use tracing::{debug, info, warn}; +use crate::error::{Error, InterruptReason}; use crate::sandbox::Sandbox; const BUDGET_BYTES: usize = 32768; @@ -12,7 +14,8 @@ pub async fn discover_memory( git_root: &str, working_dir: &str, provider: Provider, -) -> Vec { + cancel_token: &CancellationToken, +) -> Result, Error> { let directories = build_directory_walk(git_root, working_dir); let candidate_filenames: Vec<&str> = match provider { @@ -34,8 +37,15 @@ pub async fn discover_memory( for dir in &directories { for filename in &candidate_filenames { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } let path = format!("{dir}/{filename}"); - if let Ok(content) = env.read_file(&path, None, None).await { + let read_result = env.read_file(&path, None, None).await; + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + if let Ok(content) = read_result { if content.is_empty() { warn!(path = %path, "Project doc file empty, skipping"); continue; @@ -68,7 +78,7 @@ pub async fn discover_memory( let total_bytes: usize = results.iter().map(std::string::String::len).sum(); info!(files = results.len(), total_bytes, "Project docs loaded"); - results + Ok(results) } fn build_directory_walk(git_root: &str, working_dir: &str) -> Vec { @@ -117,6 +127,8 @@ mod tests { use std::collections::HashMap; use std::sync::Arc; + use tokio_util::sync::CancellationToken; + use super::*; use crate::sandbox::Sandbox; use crate::test_support::MockSandbox; @@ -129,7 +141,15 @@ mod tests { files, ..Default::default() }); - let docs = discover_memory(env.as_ref(), "/repo", "/repo", Provider::Anthropic).await; + let docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(docs.len(), 1); assert_eq!(docs[0], "Agent instructions"); } @@ -146,8 +166,15 @@ mod tests { files: files.clone(), ..Default::default() }); - let anthropic_docs = - discover_memory(env.as_ref(), "/repo", "/repo", Provider::Anthropic).await; + let anthropic_docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(anthropic_docs.len(), 2); assert_eq!(anthropic_docs[0], "agents"); assert_eq!(anthropic_docs[1], "claude"); @@ -156,7 +183,15 @@ mod tests { files: files.clone(), ..Default::default() }); - let openai_docs = discover_memory(env.as_ref(), "/repo", "/repo", Provider::OpenAi).await; + let openai_docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::OpenAi, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(openai_docs.len(), 2); assert_eq!(openai_docs[0], "agents"); assert_eq!(openai_docs[1], "copilot"); @@ -165,7 +200,15 @@ mod tests { files, ..Default::default() }); - let gemini_docs = discover_memory(env.as_ref(), "/repo", "/repo", Provider::Gemini).await; + let gemini_docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::Gemini, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(gemini_docs.len(), 2); assert_eq!(gemini_docs[0], "agents"); assert_eq!(gemini_docs[1], "gemini"); @@ -184,7 +227,15 @@ mod tests { files, ..Default::default() }); - let docs = discover_memory(env.as_ref(), "/repo", "/repo", Provider::Anthropic).await; + let docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(docs.len(), 2); assert_eq!(docs[0], large_content); // Second doc should be truncated to fit remaining budget @@ -201,7 +252,15 @@ mod tests { files, ..Default::default() }); - let docs = discover_memory(env.as_ref(), "/repo", "/repo", Provider::Anthropic).await; + let docs = discover_memory( + env.as_ref(), + "/repo", + "/repo", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(docs.len(), 1); assert_eq!(docs[0], "shared instructions"); } @@ -215,7 +274,15 @@ mod tests { files, ..Default::default() }); - let docs = discover_memory(env.as_ref(), "/repo", "/repo/src", Provider::Anthropic).await; + let docs = discover_memory( + env.as_ref(), + "/repo", + "/repo/src", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(docs.len(), 1); assert_eq!(docs[0], "shared instructions"); } @@ -231,8 +298,15 @@ mod tests { files, ..Default::default() }); - let docs = - discover_memory(env.as_ref(), "/repo", "/repo/src/app", Provider::Anthropic).await; + let docs = discover_memory( + env.as_ref(), + "/repo", + "/repo/src/app", + Provider::Anthropic, + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(docs.len(), 3); assert_eq!(docs[0], "root agents"); assert_eq!(docs[1], "src agents"); diff --git a/lib/crates/fabro-agent/src/session.rs b/lib/crates/fabro-agent/src/session.rs index 14105009b..9e8d5ac4b 100644 --- a/lib/crates/fabro-agent/src/session.rs +++ b/lib/crates/fabro-agent/src/session.rs @@ -130,13 +130,24 @@ impl Session { /// Initialize session by discovering project docs and capturing environment /// context. Call before `process_input`. - pub async fn initialize(&mut self) { + /// + /// # Errors + /// + /// Returns `Error::Interrupted(InterruptReason::Cancelled)` if the + /// session's cancel token fires during initialization. + pub async fn initialize(&mut self) -> Result<(), Error> { + let cancel_token = self.cancel_token.clone(); + self.event_emitter .emit(self.id.clone(), AgentEvent::SessionStarted { provider: Some(self.provider_profile.provider().to_string()), model: Some(self.provider_profile.model().to_string()), }); + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let doc_root = self .config .git_root @@ -147,8 +158,9 @@ impl Session { &doc_root, self.sandbox.working_directory(), self.provider_profile.provider(), + &cancel_token, ) - .await; + .await?; // Discover skills let skill_dirs = if let Some(dirs) = &self.config.skill_dirs { @@ -158,7 +170,7 @@ impl Session { let skills_str = skills_dir.to_string_lossy().to_string(); default_skill_dirs(Some(&skills_str), self.config.git_root.as_deref()) }; - self.skills = discover_skills(self.sandbox.as_ref(), &skill_dirs).await; + self.skills = discover_skills(self.sandbox.as_ref(), &skill_dirs, &cancel_token).await?; debug!(skill_count = self.skills.len(), "Skills discovered"); // Register use_skill tool when skills are available @@ -175,7 +187,7 @@ impl Session { if !self.config.mcp_servers.is_empty() { // Resolve Sandbox transports: start the server inside the sandbox, // then rewrite the config to Http using the sandbox's preview URL. - let mcp_servers = self.resolve_sandbox_mcp_servers().await; + let mcp_servers = self.resolve_sandbox_mcp_servers(&cancel_token).await?; let mut manager = McpConnectionManager::new(); let results = manager.start_servers(&mcp_servers).await; @@ -209,7 +221,7 @@ impl Session { } // Populate environment context - self.env_context = self.build_env_context().await; + self.env_context = self.build_env_context(&cancel_token).await?; debug!( is_git_repo = self.env_context.is_git_repo, model = %self.env_context.model, @@ -224,19 +236,30 @@ impl Session { self.config.user_instructions.as_deref(), &self.skills, ); + + Ok(()) } /// Resolve `McpTransport::Sandbox` configs by starting the MCP server /// inside the sandbox and rewriting the transport to `Http` with the /// sandbox's preview URL. - async fn resolve_sandbox_mcp_servers(&self) -> Vec { + async fn resolve_sandbox_mcp_servers( + &self, + cancel_token: &CancellationToken, + ) -> Result, Error> { let mut resolved = Vec::with_capacity(self.config.mcp_servers.len()); for config in &self.config.mcp_servers { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } match &config.transport { McpTransport::Sandbox { command, port, env } => { let port = *port; - match self.start_sandbox_mcp_server(command, port, env).await { + match self + .start_sandbox_mcp_server(command, port, env, cancel_token) + .await? + { Ok((url, headers)) => { info!( server = %config.name, @@ -268,17 +291,24 @@ impl Session { } } - resolved + Ok(resolved) } /// Start an MCP server inside the sandbox and return (url, headers) for /// HTTP connection. + /// + /// The outer `Result` surfaces fatal cancellation as + /// `Error::Interrupted(InterruptReason::Cancelled)` (the running MCP + /// process group is terminated before returning). The inner `Result` + /// captures non-fatal startup failures that the caller logs and turns + /// into an `McpServerFailed` event. async fn start_sandbox_mcp_server( &self, command: &[String], port: u16, env: &std::collections::HashMap, - ) -> Result<(String, std::collections::HashMap), String> { + cancel_token: &CancellationToken, + ) -> Result), String>, Error> { let sandbox = self.sandbox.as_ref(); let cmd_str = command @@ -296,27 +326,63 @@ impl Session { quoted = fabro_sandbox::shell_quote(&inner) ); let env_ref = if env.is_empty() { None } else { Some(env) }; - let launch_result = sandbox - .exec_command(&launch_script, 30_000, None, env_ref, None) - .await - .map_err(|e| format!("Failed to launch MCP server: {}", e.display_with_causes()))?; - let pid = launch_result.stdout.trim(); - info!(pid, port, "MCP server process launched in sandbox"); + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let launch_result = match sandbox + .exec_command( + &launch_script, + 30_000, + None, + env_ref, + Some(cancel_token.child_token()), + ) + .await + { + Ok(result) => result, + Err(e) => { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + return Ok(Err(format!( + "Failed to launch MCP server: {}", + e.display_with_causes() + ))); + } + }; + + let pid = launch_result.stdout.trim().to_string(); + info!(pid = %pid, port, "MCP server process launched in sandbox"); // Wait for the server to start listening on the port let poll_cmd = format!( "for i in $(seq 1 30); do ss -tln | grep -q ':{port} ' && echo ready && exit 0; sleep 1; done; echo timeout" ); let poll_result = sandbox - .exec_command(&poll_cmd, 60_000, None, None, None) - .await - .map_err(|e| { - format!( + .exec_command( + &poll_cmd, + 60_000, + None, + None, + Some(cancel_token.child_token()), + ) + .await; + + if cancel_token.is_cancelled() { + kill_mcp_pid(sandbox, &pid).await; + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + + let poll_result = match poll_result { + Ok(result) => result, + Err(e) => { + return Ok(Err(format!( "Failed to poll MCP server readiness: {}", e.display_with_causes() - ) - })?; + ))); + } + }; if poll_result.stdout.trim() != "ready" { // Grab stderr for debugging @@ -326,51 +392,80 @@ impl Session { 10_000, None, None, - None, + Some(cancel_token.child_token()), ) .await .map(|r| r.stdout) .unwrap_or_default(); - return Err(format!( + return Ok(Err(format!( "MCP server did not start listening on port {port} within 30s. stderr:\n{stderr}" - )); + ))); } // Get the preview URL for the port, or fall back to localhost for local // sandboxes - if let Some(url_and_headers) = sandbox - .get_preview_url(port) - .await - .map_err(|e| e.display_with_causes())? - { - Ok(url_and_headers) + let preview = match sandbox.get_preview_url(port).await { + Ok(p) => p, + Err(e) => return Ok(Err(e.display_with_causes())), + }; + + if cancel_token.is_cancelled() { + kill_mcp_pid(sandbox, &pid).await; + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + + if let Some(url_and_headers) = preview { + Ok(Ok(url_and_headers)) } else { info!(port, "No preview URL available, using localhost"); - Ok(( + Ok(Ok(( format!("http://localhost:{port}"), std::collections::HashMap::new(), - )) + ))) } } - async fn build_env_context(&self) -> EnvContext { + async fn build_env_context( + &self, + cancel_token: &CancellationToken, + ) -> Result { let today = chrono::Local::now().format("%Y-%m-%d").to_string(); let model_name = self.provider_profile.model().to_string(); + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + // Detect git info via sandbox let git_branch = self .sandbox - .exec_command("git rev-parse --abbrev-ref HEAD", 5000, None, None, None) + .exec_command( + "git rev-parse --abbrev-ref HEAD", + 5000, + None, + None, + Some(cancel_token.child_token()), + ) .await .ok() .filter(fabro_sandbox::ExecResult::is_success) .map(|r| r.stdout.trim().to_string()); + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let is_git_repo = git_branch.is_some(); let git_status_short = if is_git_repo { self.sandbox - .exec_command("git status --short", 5000, None, None, None) + .exec_command( + "git status --short", + 5000, + None, + None, + Some(cancel_token.child_token()), + ) .await .ok() .filter(fabro_sandbox::ExecResult::is_success) @@ -380,9 +475,19 @@ impl Session { None }; + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let git_recent_commits = if is_git_repo { self.sandbox - .exec_command("git log --oneline -10", 5000, None, None, None) + .exec_command( + "git log --oneline -10", + 5000, + None, + None, + Some(cancel_token.child_token()), + ) .await .ok() .filter(fabro_sandbox::ExecResult::is_success) @@ -392,7 +497,11 @@ impl Session { None }; - EnvContext { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + + Ok(EnvContext { git_branch, is_git_repo, current_date: today, @@ -400,7 +509,7 @@ impl Session { knowledge_cutoff: self.provider_profile.knowledge_cutoff().unwrap_or_default(), git_status_short, git_recent_commits, - } + }) } #[must_use] @@ -1031,6 +1140,23 @@ const fn is_auth_error(err: &LlmError) -> bool { ) } +/// Best-effort kill of a sandbox MCP server process group. Used when +/// `start_sandbox_mcp_server` is cancelled after spawning a detached +/// `setsid` child but before reporting readiness. Errors from the sandbox +/// are logged and swallowed; the caller is already returning a Cancelled +/// error. +async fn kill_mcp_pid(sandbox: &dyn Sandbox, pid: &str) { + let pid = pid.trim(); + if pid.is_empty() { + return; + } + let script = + format!("kill -TERM -{pid} 2>/dev/null; sleep 1; kill -KILL -{pid} 2>/dev/null; true"); + if let Err(err) = sandbox.exec_command(&script, 5_000, None, None, None).await { + warn!(pid, error = %err.display_with_causes(), "Failed to kill MCP server process group during cancellation"); + } +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -1297,7 +1423,7 @@ mod tests { let mut session = make_session(vec![text_response("Hello")]).await; let mut rx = session.subscribe(); - session.initialize().await; + session.initialize().await.unwrap(); session.process_input("Hi").await.unwrap(); session.close(); @@ -1825,7 +1951,7 @@ mod tests { let mut session = make_session(responses).await; let mut rx = session.subscribe(); - session.initialize().await; + session.initialize().await.unwrap(); session.process_input("one").await.unwrap(); session.process_input("two").await.unwrap(); session.close(); @@ -1858,7 +1984,7 @@ mod tests { ..Default::default() }; let mut session = Session::new(client, profile, env, config, None); - session.initialize().await; + session.initialize().await.unwrap(); session.process_input("test").await.unwrap(); // Verify user instructions are included in the system prompt @@ -2647,7 +2773,7 @@ mod tests { let mut rx = session.subscribe(); // Initialize starts the MCP server and registers tools - session.initialize().await; + session.initialize().await.unwrap(); // Verify McpServerReady event was emitted let mut mcp_ready = false; @@ -2842,7 +2968,7 @@ mod tests { #[tokio::test] async fn process_input_emits_processing_end_on_idle_transition() { let mut session = make_session(vec![text_response("Hello")]).await; - session.initialize().await; + session.initialize().await.unwrap(); let mut rx = session.subscribe(); session.process_input("Hi").await.unwrap(); diff --git a/lib/crates/fabro-agent/src/skills.rs b/lib/crates/fabro-agent/src/skills.rs index a4d00226e..fe5e3445a 100644 --- a/lib/crates/fabro-agent/src/skills.rs +++ b/lib/crates/fabro-agent/src/skills.rs @@ -1,7 +1,9 @@ use std::sync::Arc; use fabro_llm::types::ToolDefinition; +use tokio_util::sync::CancellationToken; +use crate::error::{Error, InterruptReason}; use crate::sandbox::Sandbox; use crate::tool_registry::RegisteredTool; use crate::tools::required_str; @@ -224,17 +226,35 @@ pub fn default_skill_dirs(fabro_skills_dir: Option<&str>, git_root: Option<&str> dirs } -pub async fn discover_skills(env: &dyn Sandbox, dirs: &[String]) -> Vec { +pub async fn discover_skills( + env: &dyn Sandbox, + dirs: &[String], + cancel_token: &CancellationToken, +) -> Result, Error> { let mut skills_by_name: std::collections::HashMap = std::collections::HashMap::new(); for dir in dirs { - let Ok(paths) = env.glob("*/SKILL.md", Some(dir)).await else { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let glob_result = env.glob("*/SKILL.md", Some(dir)).await; + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let Ok(paths) = glob_result else { continue; }; for path in paths { - let Ok(content) = env.read_file(&path, None, None).await else { + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let read_result = env.read_file(&path, None, None).await; + if cancel_token.is_cancelled() { + return Err(Error::Interrupted(InterruptReason::Cancelled)); + } + let Ok(content) = read_result else { continue; }; @@ -246,7 +266,7 @@ pub async fn discover_skills(env: &dyn Sandbox, dirs: &[String]) -> Vec { let mut skills: Vec = skills_by_name.into_values().collect(); skills.sort_by(|a, b| a.name.cmp(&b.name)); - skills + Ok(skills) } #[cfg(test)] @@ -454,7 +474,9 @@ name: trimmed ..Default::default() }; - let skills = discover_skills(&env, &["/skills".into()]).await; + let skills = discover_skills(&env, &["/skills".into()], &CancellationToken::new()) + .await + .unwrap(); assert_eq!(skills.len(), 1); assert_eq!(skills[0].name, "commit"); assert_eq!(skills[0].description, "Make a commit"); @@ -477,7 +499,9 @@ name: trimmed ..Default::default() }; - let skills = discover_skills(&env, &["/skills".into()]).await; + let skills = discover_skills(&env, &["/skills".into()], &CancellationToken::new()) + .await + .unwrap(); assert_eq!(skills.len(), 1); assert_eq!(skills[0].name, "good"); } @@ -485,7 +509,9 @@ name: trimmed #[tokio::test] async fn discover_empty_dirs() { let env = MockSandbox::default(); - let skills = discover_skills(&env, &[]).await; + let skills = discover_skills(&env, &[], &CancellationToken::new()) + .await + .unwrap(); assert!(skills.is_empty()); } @@ -514,7 +540,13 @@ name: trimmed }; // discover_skills iterates dirs in order; later dirs override earlier names - let skills = discover_skills(&env, &["/global".into(), "/project".into()]).await; + let skills = discover_skills( + &env, + &["/global".into(), "/project".into()], + &CancellationToken::new(), + ) + .await + .unwrap(); assert_eq!(skills.len(), 1); assert_eq!(skills[0].description, "Project commit"); } diff --git a/lib/crates/fabro-agent/src/subagent.rs b/lib/crates/fabro-agent/src/subagent.rs index 0db683603..04a58d18b 100644 --- a/lib/crates/fabro-agent/src/subagent.rs +++ b/lib/crates/fabro-agent/src/subagent.rs @@ -111,7 +111,7 @@ impl SubAgentManager { let task_prompt_for_spawn = task_prompt.clone(); let task = tokio::spawn(async move { - session.initialize().await; + session.initialize().await?; session.process_input(&task_prompt_for_spawn).await?; let turns = session.history().turns(); let last_text = turns.iter().rev().find_map(|t| match t { diff --git a/lib/crates/fabro-agent/src/v4a_patch.rs b/lib/crates/fabro-agent/src/v4a_patch.rs index c96745d33..78aa5a1ca 100644 --- a/lib/crates/fabro-agent/src/v4a_patch.rs +++ b/lib/crates/fabro-agent/src/v4a_patch.rs @@ -1466,7 +1466,7 @@ def farewell(name): SessionOptions::default(), None, ); - session.initialize().await; + session.initialize().await.unwrap(); session .process_input("Update the greeting functions") .await diff --git a/lib/crates/fabro-agent/tests/it/parity_matrix.rs b/lib/crates/fabro-agent/tests/it/parity_matrix.rs index 8b8630e4d..78e37d741 100644 --- a/lib/crates/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/crates/fabro-agent/tests/it/parity_matrix.rs @@ -170,7 +170,7 @@ macro_rules! provider_test { async fn [<$prefix _ $scenario>]() { let tmp = tempfile::tempdir().expect("failed to create tempdir"); let mut session = make_session($provider, $model, tmp.path(), None).await; - session.initialize().await; + session.initialize().await.unwrap(); [](&mut session, tmp.path()).await; } } @@ -195,7 +195,7 @@ macro_rules! openai_twin_provider_test { tmp.path(), Some(twin), ).await; - session.initialize().await; + session.initialize().await.unwrap(); [](&mut session, tmp.path()).await; } } @@ -670,7 +670,7 @@ macro_rules! reasoning_effort_tests { }; let mut session = make_session_with_config($provider, $model, tmp.path(), config, None).await; - session.initialize().await; + session.initialize().await.unwrap(); session .process_input("Say hello") .await @@ -749,7 +749,7 @@ macro_rules! loop_detection_tests { }; let mut session = make_session_with_config($provider, $model, tmp.path(), config, None).await; - session.initialize().await; + session.initialize().await.unwrap(); session .process_input("Repeatedly read the file /dev/null") .await diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 288c45afd..36de6b516 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -8,7 +8,6 @@ use std::collections::HashMap; use std::io::{BufRead as StdBufRead, BufReader as StdBufReader}; use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use anyhow::{Context, Result, anyhow}; @@ -34,6 +33,7 @@ use fabro_workflow::runtime_store::{RunStoreBackend, RunStoreHandle}; use tokio::signal::unix::{SignalKind, signal}; use tokio::sync::{Mutex, RwLock as AsyncRwLock, mpsc}; use tokio::time::sleep; +use tokio_util::sync::CancellationToken; use crate::args::RunWorkerMode; use crate::server_client; @@ -86,10 +86,10 @@ pub(crate) async fn execute( worker_token.to_owned(), ))); let interviewer = Arc::new(ControlInterviewer::new()); - let cancel_token = Arc::new(AtomicBool::new(false)); - spawn_worker_control_stream(Arc::clone(&interviewer), Arc::clone(&cancel_token))?; + let cancel_token = CancellationToken::new(); + spawn_worker_control_stream(Arc::clone(&interviewer), cancel_token.clone())?; let run_control = RunControlState::new(); - install_signal_handlers(Arc::clone(&run_control), Arc::clone(&cancel_token))?; + install_signal_handlers(Arc::clone(&run_control), cancel_token.clone())?; let vault = load_worker_vault(storage_dir.as_deref())?; let github_app = { let vault_guard = match &vault { @@ -100,7 +100,7 @@ pub(crate) async fn execute( }; let services = StartServices { run_id, - cancel_token: Some(Arc::clone(&cancel_token)), + cancel_token: cancel_token.clone(), emitter: Arc::new(Emitter::new(run_id)), interviewer, run_store: run_store.clone(), @@ -162,7 +162,7 @@ enum WorkerControlStreamEvent { )] fn spawn_worker_control_stream( interviewer: Arc, - cancel_token: Arc, + cancel_token: CancellationToken, ) -> Result<()> { let (event_tx, event_rx) = mpsc::unbounded_channel(); tokio::spawn(handle_worker_control_stream_events( @@ -205,7 +205,7 @@ fn read_worker_control_stream_blocking( async fn handle_worker_control_stream_events( interviewer: Arc, - cancel_token: Arc, + cancel_token: CancellationToken, mut event_rx: mpsc::UnboundedReceiver, ) { while let Some(event) = event_rx.recv().await { @@ -225,7 +225,7 @@ async fn handle_worker_control_stream_events( async fn apply_worker_control_line( interviewer: &ControlInterviewer, - cancel_token: &AtomicBool, + cancel_token: &CancellationToken, line: &str, ) { if line.trim().is_empty() { @@ -243,7 +243,7 @@ async fn apply_worker_control_line( .await; } WorkerControlMessage::RunCancel => { - cancel_token.store(true, Ordering::SeqCst); + cancel_token.cancel(); interviewer.interrupt_all().await; } } @@ -561,7 +561,7 @@ fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { fn install_signal_handlers( run_control: Arc, - cancel_token: Arc, + cancel_token: CancellationToken, ) -> Result<()> { #[cfg(unix)] { @@ -581,17 +581,17 @@ fn install_signal_handlers( }); let mut terminate = signal(SignalKind::terminate())?; - let terminate_cancel = Arc::clone(&cancel_token); + let terminate_cancel = cancel_token.clone(); tokio::spawn(async move { while terminate.recv().await.is_some() { - terminate_cancel.store(true, Ordering::SeqCst); + terminate_cancel.cancel(); } }); let mut interrupt = signal(SignalKind::interrupt())?; tokio::spawn(async move { while interrupt.recv().await.is_some() { - cancel_token.store(true, Ordering::SeqCst); + cancel_token.cancel(); } }); } @@ -606,7 +606,6 @@ fn install_signal_handlers( )] mod tests { use std::sync::Arc; - use std::sync::atomic::{AtomicBool, Ordering}; use chrono::Utc; use fabro_auth::{AuthCredential, AuthDetails}; @@ -623,6 +622,7 @@ mod tests { }; use fabro_vault::{SecretType, Vault}; use fabro_workflow::event::RunEventSink; + use tokio_util::sync::CancellationToken; use super::{ WorkerControlStreamEvent, WorkerTitlePhase, apply_worker_control_line, @@ -823,7 +823,7 @@ mod tests { #[tokio::test] async fn worker_control_line_routes_answer_by_question_id() { let interviewer = Arc::new(ControlInterviewer::new()); - let cancel_token = Arc::new(AtomicBool::new(false)); + let cancel_token = CancellationToken::new(); let mut question = Question::new("Approve?", QuestionType::YesNo); question.id = "q-1".to_string(); let ask_interviewer = Arc::clone(&interviewer); @@ -838,13 +838,13 @@ mod tests { let answer = answer_task.await.unwrap().answer; assert_eq!(answer.value, AnswerValue::Yes); - assert!(!cancel_token.load(Ordering::SeqCst)); + assert!(!cancel_token.is_cancelled()); } #[tokio::test] async fn worker_control_line_cancel_sets_cancel_token_and_interrupts_pending_interviews() { let interviewer = Arc::new(ControlInterviewer::new()); - let cancel_token = Arc::new(AtomicBool::new(false)); + let cancel_token = CancellationToken::new(); let mut question = Question::new("Approve?", QuestionType::YesNo); question.id = "q-1".to_string(); let ask_interviewer = Arc::clone(&interviewer); @@ -860,7 +860,7 @@ mod tests { let answer = answer_task.await.unwrap().answer; assert_eq!(answer.value, AnswerValue::Interrupted); - assert!(cancel_token.load(Ordering::SeqCst)); + assert!(cancel_token.is_cancelled()); } #[tokio::test] @@ -893,7 +893,7 @@ mod tests { #[tokio::test] async fn worker_control_event_loop_eof_interrupts_pending_interviews() { let interviewer = Arc::new(ControlInterviewer::new()); - let cancel_token = Arc::new(AtomicBool::new(false)); + let cancel_token = CancellationToken::new(); let mut question = Question::new("Approve?", QuestionType::YesNo); question.id = "q-1".to_string(); let ask_interviewer = Arc::clone(&interviewer); @@ -905,14 +905,14 @@ mod tests { handle_worker_control_stream_events( Arc::clone(&interviewer), - Arc::clone(&cancel_token), + cancel_token.clone(), event_rx, ) .await; let answer = answer_task.await.unwrap().answer; assert_eq!(answer.value, AnswerValue::Interrupted); - assert!(!cancel_token.load(Ordering::SeqCst)); + assert!(!cancel_token.is_cancelled()); } #[tokio::test] diff --git a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs b/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs index 1454a6765..efa322731 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs @@ -34,6 +34,7 @@ async fn run_real_cli_test(provider: Provider, model: &str) { &emitter, &env, None, + tokio_util::sync::CancellationToken::new(), ) .await .unwrap_or_else(|_| panic!("CLI backend ({provider}/{model}) should succeed")); diff --git a/lib/crates/fabro-core/src/executor.rs b/lib/crates/fabro-core/src/executor.rs index ac08d4606..d9ca0d6cc 100644 --- a/lib/crates/fabro-core/src/executor.rs +++ b/lib/crates/fabro-core/src/executor.rs @@ -1,5 +1,6 @@ use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; +#[cfg(test)] +use std::sync::atomic::Ordering; use std::time::Instant; use tokio::time::sleep; @@ -18,7 +19,7 @@ use crate::state::ExecutionState; #[derive(Default)] pub struct ExecutorOptions { - pub cancel_token: Option>, + pub cancel_token: Option, pub stall_token: Option, pub max_node_visits: Option, } @@ -58,7 +59,7 @@ impl ExecutorBuilder { } #[must_use] - pub fn cancel_token(mut self, token: Arc) -> Self { + pub fn cancel_token(mut self, token: CancellationToken) -> Self { self.options.cancel_token = Some(token); self } @@ -95,7 +96,7 @@ impl Executor { loop { // Check cancellation if let Some(ref token) = self.options.cancel_token { - if token.load(Ordering::Relaxed) { + if token.is_cancelled() { state.cancelled = true; let outcome = Outcome::fail("run cancelled"); self.lifecycle.on_run_end(&outcome, &state).await; @@ -500,7 +501,8 @@ mod tests { #[tokio::test] async fn executor_builder_sets_cancel_token() { - let token = Arc::new(AtomicBool::new(true)); // already cancelled + let token = CancellationToken::new(); + token.cancel(); // already cancelled let g = linear_graph(&["start", "end"]); let state = ExecutionState::new(&g).unwrap(); let executor = @@ -511,6 +513,38 @@ mod tests { assert!(matches!(result, Err(Error::Cancelled))); } + #[tokio::test] + async fn executor_cancel_token_fired_during_run_returns_cancelled() { + // Cancel token fired by a handler during the first node; the executor + // checks cancellation at the next node boundary and returns Cancelled. + let token = CancellationToken::new(); + let token_clone = token.clone(); + + struct CancellingHandler(CancellationToken); + #[async_trait] + impl NodeHandler for CancellingHandler { + async fn execute( + &self, + _node: &TestNode, + _context: &Context, + _g: &TestGraph, + ) -> Result { + self.0.cancel(); + Ok(Outcome::success()) + } + } + + let g = linear_graph(&["start", "work", "end"]); + let state = ExecutionState::new(&g).unwrap(); + let executor = ExecutorBuilder::new( + Arc::new(CancellingHandler(token_clone)) as Arc> + ) + .cancel_token(token) + .build(); + let result = executor.run(&g, state).await; + assert!(matches!(result, Err(Error::Cancelled))); + } + // ---- Step 9: Terminal nodes, goal gates, visit limits ---- #[tokio::test] @@ -908,10 +942,10 @@ mod tests { #[tokio::test] async fn executor_cancellation_stops_run() { - let token = Arc::new(AtomicBool::new(false)); + let token = CancellationToken::new(); let token_clone = token.clone(); - struct CancellingHandler(Arc); + struct CancellingHandler(CancellationToken); #[async_trait] impl NodeHandler for CancellingHandler { async fn execute( @@ -921,7 +955,7 @@ mod tests { _g: &TestGraph, ) -> Result { // Cancel after first node - self.0.store(true, Ordering::Relaxed); + self.0.cancel(); Ok(Outcome::success()) } } diff --git a/lib/crates/fabro-core/src/stall.rs b/lib/crates/fabro-core/src/stall.rs index 8068900bc..2319cbbf8 100644 --- a/lib/crates/fabro-core/src/stall.rs +++ b/lib/crates/fabro-core/src/stall.rs @@ -5,6 +5,7 @@ use std::time::Duration; use tokio::sync::Notify; use tokio::task::JoinHandle; use tokio::time::sleep; +use tokio_util::sync::CancellationToken; /// Trait for receiving stall timeout notifications. pub trait ActivityMonitor: Send + Sync { @@ -16,11 +17,11 @@ pub trait ActivityMonitor: Send + Sync { /// Watches for inactivity and fires a stall timeout if no activity is /// reported within the configured duration. pub struct StallWatchdog { - timeout: Duration, - cancel_token: Arc, - activity: Arc, - shutdown: Arc, - monitor: Arc, + timeout: Duration, + stall_token: CancellationToken, + activity: Arc, + shutdown: Arc, + monitor: Arc, } /// Guard that resets the stall timer on activity. Drop to stop watching. @@ -33,12 +34,12 @@ pub struct StallGuard { impl StallWatchdog { pub fn new( timeout: Duration, - cancel_token: Arc, + stall_token: CancellationToken, monitor: Arc, ) -> Self { Self { timeout, - cancel_token, + stall_token, activity: Arc::new(Notify::new()), shutdown: Arc::new(AtomicBool::new(false)), monitor, @@ -51,7 +52,7 @@ impl StallWatchdog { let activity = self.activity.clone(); let shutdown = self.shutdown.clone(); let timeout = self.timeout; - let cancel_token = self.cancel_token; + let stall_token = self.stall_token; let monitor = self.monitor; let handle = tokio::spawn(async move { @@ -66,7 +67,7 @@ impl StallWatchdog { "Stall timeout: no activity detected" ); monitor.on_stall_timeout(timeout); - cancel_token.store(true, Ordering::Relaxed); + stall_token.cancel(); return; } () = activity.notified() => { @@ -136,7 +137,7 @@ mod tests { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn stall_watchdog_cancels_on_inactivity() { - let cancel = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); let monitor = TestMonitor::new(); let watchdog = StallWatchdog::new(Duration::from_millis(50), cancel.clone(), monitor.clone()); @@ -145,13 +146,13 @@ mod tests { // Wait for timeout to fire sleep(Duration::from_millis(100)).await; - assert!(cancel.load(Ordering::Relaxed)); + assert!(cancel.is_cancelled()); assert_eq!(monitor.stalls(), 1); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn stall_watchdog_resets_on_activity() { - let cancel = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); let monitor = TestMonitor::new(); let watchdog = StallWatchdog::new(Duration::from_millis(80), cancel.clone(), monitor.clone()); @@ -164,17 +165,17 @@ mod tests { // After another 50ms (100ms total, but only 50ms since activity), should not // have timed out sleep(Duration::from_millis(50)).await; - assert!(!cancel.load(Ordering::Relaxed)); + assert!(!cancel.is_cancelled()); // Wait long enough for timeout after last activity (80ms + margin) sleep(Duration::from_millis(60)).await; - assert!(cancel.load(Ordering::Relaxed)); + assert!(cancel.is_cancelled()); assert_eq!(monitor.stalls(), 1); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn stall_watchdog_clean_shutdown_on_success() { - let cancel = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); let monitor = TestMonitor::new(); let watchdog = StallWatchdog::new(Duration::from_millis(50), cancel.clone(), monitor.clone()); @@ -187,13 +188,13 @@ mod tests { sleep(Duration::from_millis(100)).await; // Should NOT have triggered - assert!(!cancel.load(Ordering::Relaxed)); + assert!(!cancel.is_cancelled()); assert_eq!(monitor.stalls(), 0); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn stall_guard_cleanup_on_drop() { - let cancel = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); let monitor = TestMonitor::new(); let watchdog = StallWatchdog::new(Duration::from_millis(50), cancel.clone(), monitor.clone()); @@ -206,6 +207,6 @@ mod tests { sleep(Duration::from_millis(150)).await; // Cancel should not be set - assert!(!cancel.load(Ordering::Relaxed)); + assert!(!cancel.is_cancelled()); } } diff --git a/lib/crates/fabro-retro/src/retro_agent.rs b/lib/crates/fabro-retro/src/retro_agent.rs index f6275d7d6..30d5dd2d6 100644 --- a/lib/crates/fabro-retro/src/retro_agent.rs +++ b/lib/crates/fabro-retro/src/retro_agent.rs @@ -204,7 +204,10 @@ pub async fn run_retro_agent( // Optionally forward agent events via the callback let event_forwarder_handle = event_callback.map(|cb| spawn_retro_event_forwarder(&session, cb)); - session.initialize().await; + session + .initialize() + .await + .context("Retro agent session initialization failed")?; let prompt = build_retro_prompt(RETRO_DATA_DIR); diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index 03b3ed2d9..51a45e92d 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -1307,7 +1307,7 @@ impl Sandbox for DaytonaSandbox { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, @@ -1397,7 +1397,7 @@ impl Sandbox for DaytonaSandbox { &session, &command_id, session_exec.exit_code, - Duration::from_millis(timeout_ms), + timeout_ms, cancel_token.unwrap_or_default(), &mut stream_task, ) @@ -1730,7 +1730,7 @@ async fn wait_for_completion( session: &DaytonaSession, command_id: &str, initial_exit_code: Option, - timeout: Duration, + timeout_ms: Option, cancel_token: CancellationToken, stream_task: &mut JoinHandle>, ) -> crate::Result { @@ -1742,8 +1742,13 @@ async fn wait_for_completion( }); } - let timeout_sleep = time::sleep(timeout); - tokio::pin!(timeout_sleep); + let timeout_future = async { + match timeout_ms { + Some(ms) => time::sleep(Duration::from_millis(ms)).await, + None => std::future::pending::<()>().await, + } + }; + tokio::pin!(timeout_future); loop { tokio::select! { () = time::sleep(Duration::from_millis(250)) => { @@ -1765,7 +1770,7 @@ async fn wait_for_completion( }); } } - () = &mut timeout_sleep => { + () = &mut timeout_future => { return Ok(WaitOutcome { exit_code: None, termination: CommandTermination::TimedOut, diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index e6562e5d6..27c25732d 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -362,7 +362,7 @@ impl DockerSandbox { async fn docker_exec_shell_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, @@ -380,7 +380,13 @@ impl DockerSandbox { controlled_command, ]; - let timeout_duration = Duration::from_millis(timeout_ms); + let timeout_future = async { + match timeout_ms { + Some(ms) => time::sleep(Duration::from_millis(ms)).await, + None => std::future::pending::<()>().await, + } + }; + tokio::pin!(timeout_future); let token = cancel_token.unwrap_or_default(); let container_id = self.container_id()?.to_string(); @@ -399,7 +405,7 @@ impl DockerSandbox { joined .map_err(|e| crate::Error::context("Docker exec stream task failed", e))?? } - () = time::sleep(timeout_duration) => { + () = &mut timeout_future => { termination = CommandTermination::TimedOut; self.request_docker_exec_stop(&stop_file).await?; output_task @@ -1192,7 +1198,7 @@ impl Sandbox for DockerSandbox { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index 8b69b7377..5c5a6e1ba 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -1,5 +1,5 @@ use std::path::{Path, PathBuf}; -use std::time::Instant; +use std::time::{Duration, Instant}; use async_trait::async_trait; use fabro_static::EnvVars; @@ -327,7 +327,7 @@ impl Sandbox for LocalSandbox { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, @@ -367,7 +367,13 @@ impl Sandbox for LocalSandbox { .spawn() .map_err(|e| crate::Error::context("Failed to spawn command", e))?; - let timeout_duration = std::time::Duration::from_millis(timeout_ms); + let timeout_future = async { + match timeout_ms { + Some(ms) => time::sleep(Duration::from_millis(ms)).await, + None => std::future::pending::<()>().await, + } + }; + tokio::pin!(timeout_future); let token = cancel_token.unwrap_or_default(); let stdout_pipe = child.stdout.take(); @@ -387,7 +393,7 @@ impl Sandbox for LocalSandbox { .map_err(|e| crate::Error::context("Failed to wait for process", e))?; (CommandTermination::Exited, status.code()) } - () = time::sleep(timeout_duration) => { + () = &mut timeout_future => { sigterm_then_kill(&mut child).await; (CommandTermination::TimedOut, None) } diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index e30012969..2ca67bb16 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -93,7 +93,7 @@ macro_rules! delegate_sandbox { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, @@ -607,14 +607,21 @@ pub trait Sandbox: Send + Sync { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, output_callback: CommandOutputCallback, ) -> crate::Result { + let fallback_timeout_ms = timeout_ms.unwrap_or(u64::MAX); let result = self - .exec_command(command, timeout_ms, working_dir, env_vars, cancel_token) + .exec_command( + command, + fallback_timeout_ms, + working_dir, + env_vars, + cancel_token, + ) .await?; if !result.stdout.is_empty() { output_callback( diff --git a/lib/crates/fabro-sandbox/src/worktree.rs b/lib/crates/fabro-sandbox/src/worktree.rs index f412c87d4..d7941dde0 100644 --- a/lib/crates/fabro-sandbox/src/worktree.rs +++ b/lib/crates/fabro-sandbox/src/worktree.rs @@ -236,7 +236,7 @@ impl Sandbox for WorktreeSandbox { async fn exec_command_streaming( &self, command: &str, - timeout_ms: u64, + timeout_ms: Option, working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, diff --git a/lib/crates/fabro-sandbox/tests/daytona_streaming_live.rs b/lib/crates/fabro-sandbox/tests/daytona_streaming_live.rs index 10c385ae9..518d6bbbd 100644 --- a/lib/crates/fabro-sandbox/tests/daytona_streaming_live.rs +++ b/lib/crates/fabro-sandbox/tests/daytona_streaming_live.rs @@ -63,7 +63,7 @@ mod daytona_streaming_live { sandbox_for_exec .exec_command_streaming( "printf 'live-out\\n'; printf 'live-err\\n' >&2; sleep 30", - 60_000, + Some(60_000), None, None, Some(cancel_for_exec), @@ -186,7 +186,14 @@ mod daytona_streaming_live { let chunks = Arc::new(Mutex::new(Vec::new())); let callback = capture_callback(Arc::clone(&chunks)); let result = sandbox - .exec_command_streaming(command, timeout_ms, None, None, cancel_token, callback) + .exec_command_streaming( + command, + Some(timeout_ms), + None, + None, + cancel_token, + callback, + ) .await?; let chunks = chunks.lock().await.clone(); diff --git a/lib/crates/fabro-sandbox/tests/docker_streaming.rs b/lib/crates/fabro-sandbox/tests/docker_streaming.rs index 437c3fce1..848f149fb 100644 --- a/lib/crates/fabro-sandbox/tests/docker_streaming.rs +++ b/lib/crates/fabro-sandbox/tests/docker_streaming.rs @@ -49,7 +49,7 @@ async fn streaming_timeout_terminates_docker_exec_before_returning() { let result = sandbox .exec_command_streaming( &format!("trap '' HUP TERM; echo start; sleep 5 # {marker}"), - 200, + Some(200), None, None, None, diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index 11451e094..3bfa871a7 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -56,6 +56,7 @@ globset.workspace = true tower = "0.5" tower-http = { version = "0.6", features = ["trace"] } tokio-stream = { workspace = true, features = ["sync"] } +tokio-util.workspace = true base64.workspace = true jsonwebtoken.workspace = true hkdf.workspace = true @@ -108,4 +109,4 @@ tokio-util.workspace = true fabro-macros = { path = "../fabro-macros" } fabro-sandbox = { path = "../fabro-sandbox", features = ["test-support"] } fabro-test = { workspace = true } -fabro-types = { path = "../fabro-types", features = ["test-support"] } +fabro-types = { path = "../fabro-types", features = ["test-support"] } \ No newline at end of file diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index eb35ef8b0..85abdaaf7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -111,6 +111,7 @@ use tokio::task::spawn_blocking; use tokio::time::{sleep, timeout}; use tokio_stream::StreamExt; use tokio_stream::wrappers::{BroadcastStream, UnboundedReceiverStream}; +use tokio_util::sync::CancellationToken; use tower::{ServiceExt, service_fn}; use tracing::{Instrument, debug, error, info, warn}; use ulid::Ulid; @@ -197,7 +198,7 @@ struct ManagedRun { event_tx: Option>, checkpoint: Option, cancel_tx: Option>, - cancel_token: Option>, + cancel_token: Option, worker_pid: Option, worker_pgid: Option, run_dir: Option, @@ -1488,7 +1489,7 @@ async fn delete_run_internal( if let Some(mut managed_run) = managed_run { if let Some(token) = &managed_run.cancel_token { - token.store(true, Ordering::SeqCst); + token.cancel(); } if let Some(answer_transport) = managed_run.answer_transport.clone() { let _ = answer_transport.cancel_run().await; @@ -2581,12 +2582,12 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { }; let (cancel_tx, cancel_rx) = oneshot::channel::<()>(); - let cancel_token = Arc::new(AtomicBool::new(false)); + let cancel_token = CancellationToken::new(); let (event_tx, _) = broadcast::channel(256); managed_run.status = RunStatus::Starting; managed_run.cancel_tx = Some(cancel_tx); - managed_run.cancel_token = Some(Arc::clone(&cancel_token)); + managed_run.cancel_token = Some(cancel_token.clone()); managed_run.event_tx = Some(event_tx); ( @@ -2679,7 +2680,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { }; let server_settings = state.server_settings(); let github_settings = &server_settings.server.integrations.github; - if cancel_token.load(Ordering::SeqCst) { + if cancel_token.is_cancelled() { finish_cancelled_run_before_execution(&state, run_id).await; return; } @@ -2714,7 +2715,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { let github_app = match github_app_result { Ok(github_app) => github_app, Err(e) => { - if cancel_token.load(Ordering::SeqCst) { + if cancel_token.is_cancelled() { finish_cancelled_run_before_execution(&state, run_id).await; return; } @@ -2741,7 +2742,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { .collect(); let services = operations::StartServices { run_id, - cancel_token: Some(Arc::clone(&cancel_token)), + cancel_token: cancel_token.clone(), emitter: Arc::clone(&emitter), interviewer: Arc::clone(&interview_runtime), run_store: run_store.clone().into(), @@ -2765,7 +2766,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { let result = tokio::select! { result = execution => ExecutionResult::Completed(Box::new(result)), _ = cancel_rx => { - cancel_token.store(true, Ordering::SeqCst); + cancel_token.cancel(); ExecutionResult::CancelledBySignal } }; diff --git a/lib/crates/fabro-server/src/server/handler/lifecycle.rs b/lib/crates/fabro-server/src/server/handler/lifecycle.rs index 1201bf8f7..50d556c66 100644 --- a/lib/crates/fabro-server/src/server/handler/lifecycle.rs +++ b/lib/crates/fabro-server/src/server/handler/lifecycle.rs @@ -1,13 +1,13 @@ use std::sync::Arc; use super::super::{ - ApiError, AppState, FailureReason, ForkRequest, ForkResponse, IntoResponse, Json, Ordering, - Path, Principal, RequiredUser, Response, RewindRequest, RewindResponse, Router, - RunAnswerTransport, RunControlAction, RunExecutionMode, RunId, RunStatus, RunStatusResponse, - StartRunRequest, State, StatusCode, Storage, TimelineEntryResponse, WORKER_CANCEL_GRACE, - WorkflowError, append_control_request, get, load_pending_control, managed_run, operations, - parse_run_id_path, persist_cancelled_run_status, post, reject_if_archived, sleep, - update_live_run_from_event, workflow_event, + ApiError, AppState, FailureReason, ForkRequest, ForkResponse, IntoResponse, Json, Path, + Principal, RequiredUser, Response, RewindRequest, RewindResponse, Router, RunAnswerTransport, + RunControlAction, RunExecutionMode, RunId, RunStatus, RunStatusResponse, StartRunRequest, + State, StatusCode, Storage, TimelineEntryResponse, WORKER_CANCEL_GRACE, WorkflowError, + append_control_request, get, load_pending_control, managed_run, operations, parse_run_id_path, + persist_cancelled_run_status, post, reject_if_archived, sleep, update_live_run_from_event, + workflow_event, }; pub(super) fn routes() -> Router> { @@ -249,7 +249,7 @@ async fn cancel_run( } if let Some(token) = &cancel_token { - token.store(true, Ordering::SeqCst); + token.cancel(); } let sent_cancel_signal = if let Some(cancel_tx) = cancel_tx { let _ = cancel_tx.send(()); diff --git a/lib/crates/fabro-types/src/run_event/misc.rs b/lib/crates/fabro-types/src/run_event/misc.rs index 1570050a4..5123d4c8b 100644 --- a/lib/crates/fabro-types/src/run_event/misc.rs +++ b/lib/crates/fabro-types/src/run_event/misc.rs @@ -239,6 +239,20 @@ pub struct AgentCliCompletedProps { pub duration_ms: u64, } +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct AgentCliCancelledProps { + pub stdout: String, + pub stderr: String, + pub duration_ms: u64, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct AgentCliTimedOutProps { + pub stdout: String, + pub stderr: String, + pub duration_ms: u64, +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct PullRequestCreatedProps { pub pr_url: String, diff --git a/lib/crates/fabro-types/src/run_event/mod.rs b/lib/crates/fabro-types/src/run_event/mod.rs index 9d5fbf11d..11f16c9ca 100644 --- a/lib/crates/fabro-types/src/run_event/mod.rs +++ b/lib/crates/fabro-types/src/run_event/mod.rs @@ -252,6 +252,10 @@ pub enum EventBody { AgentCliStarted(AgentCliStartedProps), #[serde(rename = "agent.cli.completed")] AgentCliCompleted(AgentCliCompletedProps), + #[serde(rename = "agent.cli.cancelled")] + AgentCliCancelled(AgentCliCancelledProps), + #[serde(rename = "agent.cli.timed_out")] + AgentCliTimedOut(AgentCliTimedOutProps), #[serde(rename = "pull_request.created")] PullRequestCreated(PullRequestCreatedProps), #[serde(rename = "pull_request.failed")] @@ -433,6 +437,8 @@ impl EventBody { Self::CommandCompleted(_) => "command.completed", Self::AgentCliStarted(_) => "agent.cli.started", Self::AgentCliCompleted(_) => "agent.cli.completed", + Self::AgentCliCancelled(_) => "agent.cli.cancelled", + Self::AgentCliTimedOut(_) => "agent.cli.timed_out", Self::PullRequestCreated(_) => "pull_request.created", Self::PullRequestFailed(_) => "pull_request.failed", Self::DevcontainerResolved(_) => "devcontainer.resolved", diff --git a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs index 8c530a177..37e2cf255 100644 --- a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs +++ b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; -use std::sync::atomic::AtomicBool; use std::time::Instant; use fabro_agent::sandbox::Sandbox; @@ -7,10 +5,10 @@ use fabro_devcontainer::DevcontainerSpec; use fabro_sandbox::daytona::{DaytonaSnapshotConfig, DockerfileSource}; use futures::future::try_join_all; use sha2::{Digest, Sha256}; +use tokio_util::sync::CancellationToken; use crate::error::Error; use crate::event::{Emitter, Event}; -use crate::handler::sandbox_cancel_token; /// Compute a deterministic snapshot name from Dockerfile content. pub fn snapshot_name_for_dockerfile(dockerfile: &str) -> String { @@ -39,7 +37,7 @@ pub async fn run_devcontainer_lifecycle( phase: &str, commands: &[fabro_devcontainer::Command], timeout_ms: u64, - cancel_requested: Option>, + cancel_token: CancellationToken, ) -> Result<(), Error> { if commands.is_empty() { return Ok(()); @@ -61,7 +59,7 @@ pub async fn run_devcontainer_lifecycle( &format!("sh -c {}", shlex::try_quote(s).unwrap_or_else(|_| s.into())), index, timeout_ms, - cancel_requested.clone(), + cancel_token.clone(), ) .await?; } @@ -78,7 +76,7 @@ pub async fn run_devcontainer_lifecycle( &joined, index, timeout_ms, - cancel_requested.clone(), + cancel_token.clone(), ) .await?; } @@ -92,7 +90,7 @@ pub async fn run_devcontainer_lifecycle( ); let phase = phase.to_string(); let name = name.clone(); - let cancel_requested = cancel_requested.clone(); + let cancel_token = cancel_token.clone(); async move { let cmd_start = Instant::now(); emitter.emit(&Event::DevcontainerLifecycleCommandStarted { @@ -100,14 +98,14 @@ pub async fn run_devcontainer_lifecycle( command: name.clone(), index, }); - let cancel_token = sandbox_cancel_token(cancel_requested); + let child_token = cancel_token.child_token(); let result = sandbox .exec_command( &command, timeout_ms, None, None, - cancel_token.clone(), + Some(child_token.clone()), ) .await .map_err(|e| { @@ -115,12 +113,10 @@ pub async fn run_devcontainer_lifecycle( "Devcontainer {phase} parallel command '{name}' failed: {e}" )) })?; - if let Some(token) = &cancel_token { - if token.is_cancelled() { - return Err(Error::Cancelled); - } - token.cancel(); + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); } + child_token.cancel(); let cmd_duration = crate::millis_u64(cmd_start.elapsed()); if !result.is_success() { let exit_code = result.display_exit_code(); @@ -175,7 +171,7 @@ async fn run_single_lifecycle_command( command: &str, index: usize, timeout_ms: u64, - cancel_requested: Option>, + cancel_token: CancellationToken, ) -> Result<(), Error> { emitter.emit(&Event::DevcontainerLifecycleCommandStarted { phase: phase.to_string(), @@ -183,19 +179,17 @@ async fn run_single_lifecycle_command( index, }); let cmd_start = Instant::now(); - let cancel_token = sandbox_cancel_token(cancel_requested); + let child_token = cancel_token.child_token(); let result = sandbox - .exec_command(command, timeout_ms, None, None, cancel_token.clone()) + .exec_command(command, timeout_ms, None, None, Some(child_token.clone())) .await .map_err(|e| { Error::engine_with_source(format!("Devcontainer {phase} command failed"), &e) })?; - if let Some(token) = &cancel_token { - if token.is_cancelled() { - return Err(Error::Cancelled); - } - token.cancel(); + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); } + child_token.cancel(); let cmd_duration = crate::millis_u64(cmd_start.elapsed()); if !result.is_success() { let exit_code = result.display_exit_code(); @@ -227,7 +221,6 @@ async fn run_single_lifecycle_command( #[cfg(test)] mod tests { use std::collections::HashMap; - use std::sync::atomic::AtomicBool; use std::sync::{Arc, Mutex}; use async_trait::async_trait; @@ -437,9 +430,16 @@ mod tests { let sandbox = TestSandbox::new(); let emitter = Emitter::default(); let commands = vec![fabro_devcontainer::Command::Shell("echo hi".to_string())]; - run_devcontainer_lifecycle(&sandbox, &emitter, "on_create", &commands, 300_000, None) - .await - .unwrap(); + run_devcontainer_lifecycle( + &sandbox, + &emitter, + "on_create", + &commands, + 300_000, + CancellationToken::new(), + ) + .await + .unwrap(); let captured = sandbox.captured_commands(); assert_eq!(captured.len(), 1); assert!(captured[0].contains("echo hi"), "command: {}", captured[0]); @@ -453,9 +453,16 @@ mod tests { "echo".to_string(), "hi".to_string(), ])]; - run_devcontainer_lifecycle(&sandbox, &emitter, "on_create", &commands, 300_000, None) - .await - .unwrap(); + run_devcontainer_lifecycle( + &sandbox, + &emitter, + "on_create", + &commands, + 300_000, + CancellationToken::new(), + ) + .await + .unwrap(); let captured = sandbox.captured_commands(); assert_eq!(captured.len(), 1); assert!( @@ -475,9 +482,16 @@ mod tests { }); let sandbox = TestSandbox::new(); let commands = vec![fabro_devcontainer::Command::Shell("echo hi".to_string())]; - run_devcontainer_lifecycle(&sandbox, &emitter, "on_create", &commands, 300_000, None) - .await - .unwrap(); + run_devcontainer_lifecycle( + &sandbox, + &emitter, + "on_create", + &commands, + 300_000, + CancellationToken::new(), + ) + .await + .unwrap(); let events = events.lock().unwrap(); let started = events[0].properties().unwrap(); assert_eq!(events[0].event_name(), "devcontainer.lifecycle.started"); @@ -515,9 +529,15 @@ mod tests { }); let sandbox = TestSandbox::with_exit_code(1); let commands = vec![fabro_devcontainer::Command::Shell("false".to_string())]; - let result = - run_devcontainer_lifecycle(&sandbox, &emitter, "on_create", &commands, 300_000, None) - .await; + let result = run_devcontainer_lifecycle( + &sandbox, + &emitter, + "on_create", + &commands, + 300_000, + CancellationToken::new(), + ) + .await; assert!(result.is_err()); let events = events.lock().unwrap(); let failed = events @@ -550,9 +570,16 @@ mod tests { events_clone.lock().unwrap().push(event.clone()); }); let sandbox = TestSandbox::new(); - run_devcontainer_lifecycle(&sandbox, &emitter, "on_create", &[], 300_000, None) - .await - .unwrap(); + run_devcontainer_lifecycle( + &sandbox, + &emitter, + "on_create", + &[], + 300_000, + CancellationToken::new(), + ) + .await + .unwrap(); assert!(events.lock().unwrap().is_empty()); } @@ -564,9 +591,16 @@ mod tests { map.insert("install".to_string(), "npm install".to_string()); map.insert("build".to_string(), "npm run build".to_string()); let commands = vec![fabro_devcontainer::Command::Parallel(map)]; - run_devcontainer_lifecycle(&sandbox, &emitter, "post_create", &commands, 300_000, None) - .await - .unwrap(); + run_devcontainer_lifecycle( + &sandbox, + &emitter, + "post_create", + &commands, + 300_000, + CancellationToken::new(), + ) + .await + .unwrap(); let captured = sandbox.captured_commands(); assert_eq!(captured.len(), 2); } @@ -576,7 +610,8 @@ mod tests { let sandbox = TestSandbox::waiting_for_cancel(); let emitter = Emitter::default(); let commands = vec![fabro_devcontainer::Command::Shell("sleep 5".to_string())]; - let cancel_requested = Arc::new(AtomicBool::new(true)); + let cancel_token = CancellationToken::new(); + cancel_token.cancel(); let result = run_devcontainer_lifecycle( &sandbox, @@ -584,7 +619,7 @@ mod tests { "on_create", &commands, 300_000, - Some(cancel_requested), + cancel_token, ) .await; @@ -600,7 +635,8 @@ mod tests { map.insert("install".to_string(), "sleep 5".to_string()); map.insert("build".to_string(), "sleep 5".to_string()); let commands = vec![fabro_devcontainer::Command::Parallel(map)]; - let cancel_requested = Arc::new(AtomicBool::new(true)); + let cancel_token = CancellationToken::new(); + cancel_token.cancel(); let result = run_devcontainer_lifecycle( &sandbox, @@ -608,7 +644,7 @@ mod tests { "post_create", &commands, 300_000, - Some(cancel_requested), + cancel_token, ) .await; diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index 815aaec11..5f5f66ad8 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -1006,6 +1006,26 @@ fn event_body_from_event(event: &Event) -> EventBody { exit_code: *exit_code, duration_ms: *duration_ms, }), + Event::AgentCliCancelled { + stdout, + stderr, + duration_ms, + .. + } => EventBody::AgentCliCancelled(fabro_types::AgentCliCancelledProps { + stdout: stdout.clone(), + stderr: stderr.clone(), + duration_ms: *duration_ms, + }), + Event::AgentCliTimedOut { + stdout, + stderr, + duration_ms, + .. + } => EventBody::AgentCliTimedOut(fabro_types::AgentCliTimedOutProps { + stdout: stdout.clone(), + stderr: stderr.clone(), + duration_ms: *duration_ms, + }), Event::PullRequestCreated { pr_url, pr_number, @@ -1809,6 +1829,48 @@ mod tests { }); } + #[test] + fn agent_cli_cancelled_maps_to_event_body_with_node_id() { + let stored = to_run_event(&fixtures::RUN_1, &Event::AgentCliCancelled { + node_id: "code".to_string(), + stdout: "out".to_string(), + stderr: "err".to_string(), + duration_ms: 42, + }); + + assert_eq!(stored.event_name(), "agent.cli.cancelled"); + assert_eq!(stored.node_id.as_deref(), Some("code")); + match &stored.body { + EventBody::AgentCliCancelled(props) => { + assert_eq!(props.stdout, "out"); + assert_eq!(props.stderr, "err"); + assert_eq!(props.duration_ms, 42); + } + other => panic!("expected AgentCliCancelled, got {other:?}"), + } + } + + #[test] + fn agent_cli_timed_out_maps_to_event_body_with_node_id() { + let stored = to_run_event(&fixtures::RUN_1, &Event::AgentCliTimedOut { + node_id: "code".to_string(), + stdout: "out".to_string(), + stderr: "err".to_string(), + duration_ms: 99, + }); + + assert_eq!(stored.event_name(), "agent.cli.timed_out"); + assert_eq!(stored.node_id.as_deref(), Some("code")); + match &stored.body { + EventBody::AgentCliTimedOut(props) => { + assert_eq!(props.stdout, "out"); + assert_eq!(props.stderr, "err"); + assert_eq!(props.duration_ms, 99); + } + other => panic!("expected AgentCliTimedOut, got {other:?}"), + } + } + #[test] fn stall_watchdog_timeout_populates_watchdog_actor() { let stored = to_run_event(&fixtures::RUN_1, &Event::StallWatchdogTimeout { diff --git a/lib/crates/fabro-workflow/src/event/events.rs b/lib/crates/fabro-workflow/src/event/events.rs index a3b2e2b5b..eb6956ec5 100644 --- a/lib/crates/fabro-workflow/src/event/events.rs +++ b/lib/crates/fabro-workflow/src/event/events.rs @@ -530,6 +530,18 @@ pub enum Event { exit_code: i32, duration_ms: u64, }, + AgentCliCancelled { + node_id: String, + stdout: String, + stderr: String, + duration_ms: u64, + }, + AgentCliTimedOut { + node_id: String, + stdout: String, + stderr: String, + duration_ms: u64, + }, PullRequestCreated { pr_url: String, pr_number: u64, @@ -1247,6 +1259,20 @@ impl Event { } => { debug!(node_id, exit_code, duration_ms, "Agent CLI completed"); } + Self::AgentCliCancelled { + node_id, + duration_ms, + .. + } => { + debug!(node_id, duration_ms, "Agent CLI cancelled"); + } + Self::AgentCliTimedOut { + node_id, + duration_ms, + .. + } => { + debug!(node_id, duration_ms, "Agent CLI timed out"); + } Self::PullRequestCreated { pr_url, pr_number, diff --git a/lib/crates/fabro-workflow/src/event/names.rs b/lib/crates/fabro-workflow/src/event/names.rs index e176b2474..5c69b52fb 100644 --- a/lib/crates/fabro-workflow/src/event/names.rs +++ b/lib/crates/fabro-workflow/src/event/names.rs @@ -116,6 +116,8 @@ pub fn event_name(event: &Event) -> &'static str { Event::CommandCompleted { .. } => "command.completed", Event::AgentCliStarted { .. } => "agent.cli.started", Event::AgentCliCompleted { .. } => "agent.cli.completed", + Event::AgentCliCancelled { .. } => "agent.cli.cancelled", + Event::AgentCliTimedOut { .. } => "agent.cli.timed_out", Event::PullRequestCreated { .. } => "pull_request.created", Event::PullRequestFailed { .. } => "pull_request.failed", Event::DevcontainerResolved { .. } => "devcontainer.resolved", diff --git a/lib/crates/fabro-workflow/src/event/stored_fields.rs b/lib/crates/fabro-workflow/src/event/stored_fields.rs index 3c9fd5872..c0b15d307 100644 --- a/lib/crates/fabro-workflow/src/event/stored_fields.rs +++ b/lib/crates/fabro-workflow/src/event/stored_fields.rs @@ -116,7 +116,9 @@ fn stored_event_fields_for_variant(event: &Event) -> StoredEventFields { | Event::CommandStarted { node_id, .. } | Event::CommandCompleted { node_id, .. } | Event::AgentCliStarted { node_id, .. } - | Event::AgentCliCompleted { node_id, .. } => node_stored_fields(Some(node_id.clone())), + | Event::AgentCliCompleted { node_id, .. } + | Event::AgentCliCancelled { node_id, .. } + | Event::AgentCliTimedOut { node_id, .. } => node_stored_fields(Some(node_id.clone())), Event::Agent { stage, visit, diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 676e2f142..eaebea7ee 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -7,6 +7,7 @@ use fabro_agent::Sandbox; use fabro_graphviz::graph::{Graph, Node}; use fabro_template::{TemplateContext, render as render_template}; use fabro_types::RunId; +use tokio_util::sync::CancellationToken; use super::{EngineServices, Handler}; use crate::context::{Context, WorkflowContext, keys}; @@ -44,6 +45,7 @@ pub trait CodergenBackend: Send + Sync { emitter: &Arc, sandbox: &Arc, tool_hooks: Option>, + cancel_token: CancellationToken, ) -> Result; /// Run a single LLM call with no tools (one_shot mode). @@ -297,6 +299,7 @@ impl Handler for AgentHandler { &services.run.emitter, &services.run.sandbox, tool_hooks, + services.run.cancel_token(), ) .await; match result { @@ -307,6 +310,7 @@ impl Handler for AgentHandler { files_touched, last_file_touched, }) => (text, usage, files_touched, last_file_touched), + Err(Error::Cancelled) => return Err(Error::Cancelled), Err(e) if e.is_retryable() => { return Err(e); } @@ -615,6 +619,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { Ok(CodergenResult::Text { text: @@ -675,6 +680,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { Ok(CodergenResult::Text { text: "Done writing results.".to_string(), @@ -736,6 +742,7 @@ mod tests { emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { let scope = StageScope::for_handler(context, &node.id); emitter.emit_scoped( @@ -847,6 +854,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { *self.captured_thread_id.lock().unwrap() = Some(thread_id.map(String::from)); Ok(CodergenResult::Text { @@ -899,6 +907,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { *self.captured_thread_id.lock().unwrap() = Some(thread_id.map(String::from)); Ok(CodergenResult::Text { @@ -946,6 +955,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { Err(Error::handler("Request timed out".to_string())) } @@ -1093,6 +1103,7 @@ Some text in between. _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { Err(Error::Validation("bad config".to_string())) } @@ -1133,6 +1144,7 @@ Some text in between. _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { *self.captured_prompt.lock().unwrap() = Some(prompt.to_string()); Ok(CodergenResult::Text { @@ -1202,6 +1214,7 @@ Some text in between. _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { *self.captured_prompt.lock().unwrap() = Some(prompt.to_string()); Ok(CodergenResult::Text { diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index 4ce355536..dbb8fa933 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -109,7 +109,7 @@ impl Handler for CommandHandler { } else { Some(&services.env) }; - let cancel_token = services.run.sandbox_cancel_token(); + let cancel_token = services.run.cancel_token().child_token(); let stage_id = stage_scope.stage_id(); let recorder = CommandLogRecorder::create(run_dir, &stage_id).await?; let output_callback: CommandOutputCallback = { @@ -130,16 +130,14 @@ impl Handler for CommandHandler { .sandbox .exec_command_streaming( &command, - timeout_ms, + Some(timeout_ms), None, env_vars, - cancel_token.clone(), + Some(cancel_token.clone()), output_callback, ) .await; - if let Some(token) = cancel_token { - token.cancel(); - } + cancel_token.cancel(); let streaming = match result { Ok(streaming) => streaming, Err(err) => { @@ -237,7 +235,6 @@ fn tail_bytes(text: &str, max_bytes: usize) -> String { #[cfg(test)] mod tests { use std::sync::Arc; - use std::sync::atomic::AtomicBool; use std::time::Duration; use bytes::Bytes; @@ -1126,7 +1123,7 @@ mod tests { let mut services = make_spy_services(spy.clone()); services.run = services .run - .with_cancel_requested(Some(Arc::new(AtomicBool::new(false)))); + .with_cancel_token(tokio_util::sync::CancellationToken::new()); handler .execute(&node, &context, &graph, run_dir.path(), &services) diff --git a/lib/crates/fabro-workflow/src/handler/fan_in.rs b/lib/crates/fabro-workflow/src/handler/fan_in.rs index 48c484726..65c838731 100644 --- a/lib/crates/fabro-workflow/src/handler/fan_in.rs +++ b/lib/crates/fabro-workflow/src/handler/fan_in.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_agent::Sandbox; use fabro_graphviz::graph::{Graph, Node}; +use tokio_util::sync::CancellationToken; use super::agent::{CodergenBackend, CodergenResult}; use super::{EngineServices, Handler}; @@ -86,6 +87,7 @@ impl Handler for FanInHandler { &node.id, &services.run.emitter, &services.run.sandbox, + services.run.cancel_token(), ) .await? } else { @@ -223,6 +225,7 @@ async fn llm_evaluate( node_id: &str, emitter: &Arc, sandbox: &Arc, + cancel_token: CancellationToken, ) -> Result { let results_text = serde_json::to_string_pretty(results).unwrap_or_else(|_| results.to_string()); @@ -259,6 +262,7 @@ async fn llm_evaluate( emitter, sandbox, None, + cancel_token, ) .await { @@ -474,6 +478,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { // Return text that contains the ID "branch_b" Ok(CodergenResult::Text { diff --git a/lib/crates/fabro-workflow/src/handler/human.rs b/lib/crates/fabro-workflow/src/handler/human.rs index 5e3f7bb9d..fd0df72cc 100644 --- a/lib/crates/fabro-workflow/src/handler/human.rs +++ b/lib/crates/fabro-workflow/src/handler/human.rs @@ -325,12 +325,7 @@ impl Handler for HumanHandler { // 5. Handle unanswered / interrupted interview sessions. if answer.value == AnswerValue::Interrupted { - if services - .run - .cancel_requested - .as_ref() - .is_some_and(|flag| flag.load(Ordering::SeqCst)) - { + if services.run.cancel_token().is_cancelled() { return Err(Error::Cancelled); } self.emit( diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index 5d21c7d32..466af0a20 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -14,6 +14,8 @@ use fabro_llm::types::{Message, Request, TokenCounts}; use fabro_mcp::config::McpServerSettings; use fabro_model::{FallbackTarget, Provider}; use tokio::sync::Mutex as TokioMutex; +use tokio::task::JoinHandle; +use tokio_util::sync::CancellationToken; use super::super::agent::{CodergenBackend, CodergenResult}; use crate::context::keys::Fidelity; @@ -22,6 +24,96 @@ use crate::error::Error; use crate::event::{Emitter, Event, StageScope}; use crate::outcome::billed_model_usage_from_llm; +/// Spawn a task that, when the run-level token cancels, sets the agent +/// `Session`'s interrupt reason to `Cancelled` and cancels the session token. +/// +/// Factored out of `SessionCancelBridgeGuard::replace` so it can be unit-tested +/// without constructing a real `Session`. +fn spawn_bridge_task( + run_token: CancellationToken, + interrupt_reason: Arc>>, + session_token: CancellationToken, +) -> JoinHandle<()> { + tokio::spawn(async move { + run_token.cancelled().await; + { + let mut guard = interrupt_reason + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if guard.is_none() { + *guard = Some(fabro_agent::InterruptReason::Cancelled); + } + } + session_token.cancel(); + }) +} + +/// Per-invocation guard that maps a run-level `CancellationToken` to an agent +/// `Session`'s interrupt reason and cancel token. +/// +/// Dropping the guard aborts the spawned bridge task so a still-cached session +/// (after success) is not left wired to a stale run token. +struct SessionCancelBridgeGuard { + handle: Option>, +} + +impl SessionCancelBridgeGuard { + fn new() -> Self { + Self { handle: None } + } + + fn replace(&mut self, run_token: CancellationToken, session: &Session) { + self.abort(); + self.handle = Some(spawn_bridge_task( + run_token, + session.interrupt_reason_handle(), + session.cancel_token(), + )); + } + + fn abort(&mut self) { + if let Some(handle) = self.handle.take() { + handle.abort(); + } + } +} + +impl Drop for SessionCancelBridgeGuard { + fn drop(&mut self) { + self.abort(); + } +} + +/// Classification of an `fabro_agent::Error` for the API backend's `run` path. +enum AgentApiErrorDisposition { + /// Session was interrupted via cancellation; surface as `Error::Cancelled`. + Cancelled, + /// Underlying LLM error eligible for provider failover. + FailoverEligible(fabro_llm::Error), + /// Terminal error; abort the invocation with this workflow `Error`. + Terminal(Error), +} + +fn classify_agent_error(err: fabro_agent::Error, allow_failover: bool) -> AgentApiErrorDisposition { + match err { + fabro_agent::Error::Interrupted(fabro_agent::InterruptReason::Cancelled) => { + AgentApiErrorDisposition::Cancelled + } + fabro_agent::Error::Interrupted(fabro_agent::InterruptReason::WallClockTimeout) => { + AgentApiErrorDisposition::Terminal(Error::Precondition( + "Agent session hit its wall-clock timeout".to_string(), + )) + } + fabro_agent::Error::Llm(err) if allow_failover && err.failover_eligible() => { + AgentApiErrorDisposition::FailoverEligible(err) + } + fabro_agent::Error::Llm(err) => AgentApiErrorDisposition::Terminal(Error::Llm(err)), + other => AgentApiErrorDisposition::Terminal(Error::handler(format!( + "Agent session failed: {other}" + ))), + } +} + fn build_profile(model: &str, provider: Provider) -> Box { match provider { Provider::OpenAi => Box::new(OpenAiProfile::new(model)), @@ -426,6 +518,7 @@ impl CodergenBackend for AgentApiBackend { emitter: &Arc, sandbox: &Arc, tool_hooks: Option>, + cancel_token: CancellationToken, ) -> Result { let actual_model = node.model().unwrap_or(&self.model).to_string(); let _actual_provider = node @@ -440,7 +533,14 @@ impl CodergenBackend for AgentApiBackend { None }; - // Take a cached session if reusing, otherwise create a new one. + let mut bridge = SessionCancelBridgeGuard::new(); + + // Take a cached session if reusing, otherwise create a new one. Cancel + // checks bracket `Client::from_source(...)` so cancellation arriving + // during credential refresh is not lost. + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); + } let (mut session, is_reused) = if let Some(ref key) = reuse_key { let existing = self.sessions.lock().unwrap().remove(key); if let Some(s) = existing { @@ -459,6 +559,10 @@ impl CodergenBackend for AgentApiBackend { false, ) }; + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); + } + bridge.replace(cancel_token.clone(), &session); tracing::info!( node = %node.id, @@ -487,99 +591,157 @@ impl CodergenBackend for AgentApiBackend { // Record turn count before processing so we only aggregate new usage. let turns_before = session.history().turns().len(); - if !is_reused { - session.initialize().await; - } - - let result = session.process_input(prompt).await; - - // On failover-eligible error, try fallback providers. - let result = match result { - Ok(()) => Ok(()), - Err(fabro_agent::Error::Llm(ref sdk_err)) - if sdk_err.failover_eligible() && !self.fallback_chain.is_empty() => - { - let error_msg = sdk_err.to_string(); - let from_provider = self.provider.to_string(); - let from_model = self.model.clone(); - - let mut last_err = Error::Llm(sdk_err.clone()); - let mut succeeded = false; - - for target in &self.fallback_chain { - emitter.emit_scoped( - &Event::Failover { - stage: node.id.clone(), - from_provider: from_provider.clone(), - from_model: from_model.clone(), - to_provider: target.provider.clone(), - to_model: target.model.clone(), - error: error_msg.clone(), - }, - &stage_scope, - ); - - let target_provider: Provider = match target.provider.parse() { - Ok(p) => p, - Err(_) => continue, - }; - - let new_session = match Self::create_session_for( - &target.model, - target_provider, - node, - sandbox, - self.source.as_ref(), - &self.env, - tool_hooks.clone(), - self.mcp_servers.clone(), - ) - .await - { - Ok(s) => s, - Err(e) => { - last_err = e; - continue; - } - }; - session = new_session; - - // Re-subscribe to forward events + track files from the new session - spawn_event_forwarder( - &session, - node.id.clone(), - stage_scope.clone(), - Arc::clone(emitter), - Arc::clone(&file_tracking), - ); - - session.initialize().await; - match session.process_input(prompt).await { - Ok(()) => { - succeeded = true; - break; - } - Err(fabro_agent::Error::Llm(err)) if err.failover_eligible() => { - last_err = Error::Llm(err); - } - Err(fabro_agent::Error::Llm(err)) => return Err(Error::Llm(err)), - Err(fabro_agent::Error::Interrupted(_)) => { - return Err(Error::Cancelled); - } - Err(other) => { - return Err(Error::handler(format!("Agent session failed: {other}"))); - } + let allow_failover_primary = !self.fallback_chain.is_empty(); + let init_result = if is_reused { + Ok(()) + } else { + match session.initialize().await { + Ok(()) => Ok(()), + Err(err) => match classify_agent_error(err, allow_failover_primary) { + AgentApiErrorDisposition::Cancelled => { + bridge.abort(); + return Err(Error::Cancelled); } - } - - if succeeded { Ok(()) } else { Err(last_err) } + AgentApiErrorDisposition::Terminal(err) => { + bridge.abort(); + return Err(err); + } + AgentApiErrorDisposition::FailoverEligible(sdk_err) => { + Err(fabro_agent::Error::Llm(sdk_err)) + } + }, } - Err(fabro_agent::Error::Llm(sdk_err)) => Err(Error::Llm(sdk_err)), - Err(fabro_agent::Error::Interrupted(_)) => Err(Error::Cancelled), - Err(other) => Err(Error::handler(format!("Agent session failed: {other}"))), }; - // On error, drop the session (don't cache failed state). + // If initialize failed with a failover-eligible error, treat as a + // process_input failover trigger; otherwise run process_input. + let result = match init_result { + Ok(()) => session.process_input(prompt).await, + Err(err) => Err(err), + }; + + // On failover-eligible error, try fallback providers. + let result: Result<(), Error> = match result { + Ok(()) => Ok(()), + Err(err) => match classify_agent_error(err, allow_failover_primary) { + AgentApiErrorDisposition::Cancelled => { + bridge.abort(); + return Err(Error::Cancelled); + } + AgentApiErrorDisposition::Terminal(err) => { + bridge.abort(); + return Err(err); + } + AgentApiErrorDisposition::FailoverEligible(sdk_err) => { + let error_msg = sdk_err.to_string(); + let from_provider = self.provider.to_string(); + let from_model = self.model.clone(); + + let mut last_err = Error::Llm(sdk_err); + let mut succeeded = false; + + for (index, target) in self.fallback_chain.iter().enumerate() { + emitter.emit_scoped( + &Event::Failover { + stage: node.id.clone(), + from_provider: from_provider.clone(), + from_model: from_model.clone(), + to_provider: target.provider.clone(), + to_model: target.model.clone(), + error: error_msg.clone(), + }, + &stage_scope, + ); + + let target_provider: Provider = match target.provider.parse() { + Ok(p) => p, + Err(_) => continue, + }; + + // Detach the bridge from the failing session before + // refreshing credentials and building a new one. + bridge.abort(); + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); + } + let new_session = match Self::create_session_for( + &target.model, + target_provider, + node, + sandbox, + self.source.as_ref(), + &self.env, + tool_hooks.clone(), + self.mcp_servers.clone(), + ) + .await + { + Ok(s) => s, + Err(e) => { + last_err = e; + continue; + } + }; + if cancel_token.is_cancelled() { + return Err(Error::Cancelled); + } + session = new_session; + bridge.replace(cancel_token.clone(), &session); + + // Re-subscribe to forward events + track files from the new session + spawn_event_forwarder( + &session, + node.id.clone(), + stage_scope.clone(), + Arc::clone(emitter), + Arc::clone(&file_tracking), + ); + + let allow_failover_next = index + 1 < self.fallback_chain.len(); + if let Err(err) = session.initialize().await { + match classify_agent_error(err, allow_failover_next) { + AgentApiErrorDisposition::Cancelled => { + bridge.abort(); + return Err(Error::Cancelled); + } + AgentApiErrorDisposition::Terminal(err) => { + bridge.abort(); + return Err(err); + } + AgentApiErrorDisposition::FailoverEligible(sdk_err) => { + last_err = Error::Llm(sdk_err); + continue; + } + } + } + match session.process_input(prompt).await { + Ok(()) => { + succeeded = true; + break; + } + Err(err) => match classify_agent_error(err, allow_failover_next) { + AgentApiErrorDisposition::Cancelled => { + bridge.abort(); + return Err(Error::Cancelled); + } + AgentApiErrorDisposition::Terminal(err) => { + bridge.abort(); + return Err(err); + } + AgentApiErrorDisposition::FailoverEligible(sdk_err) => { + last_err = Error::Llm(sdk_err); + } + }, + } + } + + if succeeded { Ok(()) } else { Err(last_err) } + } + }, + }; + + // On error, drop the session (don't cache failed state). The bridge's + // `Drop` will abort the spawned task on early return. result?; // Aggregate token usage only from new turns (prevents double-counting on @@ -622,8 +784,10 @@ impl CodergenBackend for AgentApiBackend { (v, s.last.clone()) }; - // Cache session back for reuse on success. + // Cache session back for reuse on success. Detach the bridge first so + // the cached session is not left wired to this run's cancel token. if let Some(key) = reuse_key { + bridge.abort(); self.sessions.lock().unwrap().insert(key, session); } @@ -640,6 +804,7 @@ impl CodergenBackend for AgentApiBackend { mod tests { use fabro_agent::subagent::SessionFactory; use fabro_auth::{AuthCredential, AuthDetails, VaultCredentialSource}; + use fabro_llm::{Error as LlmError, ProviderErrorDetail, ProviderErrorKind}; use fabro_vault::{SecretType, Vault}; use tokio::sync::RwLock as AsyncRwLock; @@ -820,4 +985,238 @@ mod tests { assert_eq!(client.provider_names(), vec!["anthropic"]); } + + // --- Bridge guard tests --- + + fn failover_eligible_llm_error() -> LlmError { + LlmError::Network { + message: "boom".into(), + source: None, + } + } + + fn non_failover_llm_error() -> LlmError { + LlmError::Provider { + kind: ProviderErrorKind::Authentication, + detail: Box::new(ProviderErrorDetail { + message: "bad key".into(), + provider: "openai".into(), + status_code: Some(401), + error_code: None, + retry_after: None, + raw: None, + }), + } + } + + #[tokio::test] + async fn spawn_bridge_task_sets_cancelled_and_cancels_session_token() { + let run_token = CancellationToken::new(); + let interrupt_reason = Arc::new(Mutex::new(None)); + let session_token = CancellationToken::new(); + + let handle = spawn_bridge_task( + run_token.clone(), + Arc::clone(&interrupt_reason), + session_token.clone(), + ); + + assert!(!session_token.is_cancelled()); + assert!(interrupt_reason.lock().unwrap().is_none()); + + run_token.cancel(); + handle.await.unwrap(); + + assert!(session_token.is_cancelled()); + assert_eq!( + *interrupt_reason.lock().unwrap(), + Some(fabro_agent::InterruptReason::Cancelled) + ); + } + + #[tokio::test] + async fn spawn_bridge_task_preserves_existing_interrupt_reason() { + let run_token = CancellationToken::new(); + let interrupt_reason = Arc::new(Mutex::new(Some( + fabro_agent::InterruptReason::WallClockTimeout, + ))); + let session_token = CancellationToken::new(); + + let handle = spawn_bridge_task( + run_token.clone(), + Arc::clone(&interrupt_reason), + session_token.clone(), + ); + run_token.cancel(); + handle.await.unwrap(); + + // Existing reason wins; the bridge does not overwrite a wall-clock + // timeout already recorded by the session. + assert_eq!( + *interrupt_reason.lock().unwrap(), + Some(fabro_agent::InterruptReason::WallClockTimeout) + ); + assert!(session_token.is_cancelled()); + } + + #[tokio::test] + async fn bridge_guard_drop_aborts_pending_task() { + let run_token = CancellationToken::new(); + let interrupt_reason = Arc::new(Mutex::new(None)); + let session_token = CancellationToken::new(); + + { + let mut guard = SessionCancelBridgeGuard::new(); + guard.handle = Some(spawn_bridge_task( + run_token.clone(), + Arc::clone(&interrupt_reason), + session_token.clone(), + )); + // guard dropped here + } + + // Trigger the run token after the guard has been dropped. The aborted + // task must not write to interrupt_reason or cancel session_token. + run_token.cancel(); + // Yield enough times for any errant task to run. + for _ in 0..10 { + tokio::task::yield_now().await; + } + + assert!(interrupt_reason.lock().unwrap().is_none()); + assert!(!session_token.is_cancelled()); + } + + #[tokio::test] + async fn bridge_guard_replace_aborts_prior_task() { + // First (prior) bridge wiring. + let prior_run_token = CancellationToken::new(); + let prior_interrupt_reason = Arc::new(Mutex::new(None)); + let prior_session_token = CancellationToken::new(); + + // Second (replacement) bridge wiring. + let new_run_token = CancellationToken::new(); + let new_interrupt_reason = Arc::new(Mutex::new(None)); + let new_session_token = CancellationToken::new(); + + let mut guard = SessionCancelBridgeGuard::new(); + guard.handle = Some(spawn_bridge_task( + prior_run_token.clone(), + Arc::clone(&prior_interrupt_reason), + prior_session_token.clone(), + )); + + // Replace with a new task pointing at different handles. + guard.handle = { + // Manually mirror `replace` semantics: abort then install. + if let Some(h) = guard.handle.take() { + h.abort(); + } + Some(spawn_bridge_task( + new_run_token.clone(), + Arc::clone(&new_interrupt_reason), + new_session_token.clone(), + )) + }; + + // Cancelling the prior run token must not affect anything because the + // prior task was aborted by `replace`. + prior_run_token.cancel(); + for _ in 0..10 { + tokio::task::yield_now().await; + } + assert!(prior_interrupt_reason.lock().unwrap().is_none()); + assert!(!prior_session_token.is_cancelled()); + + // The replacement task must still be alive and react to its own token. + new_run_token.cancel(); + guard.handle.take().unwrap().await.unwrap(); + assert_eq!( + *new_interrupt_reason.lock().unwrap(), + Some(fabro_agent::InterruptReason::Cancelled) + ); + assert!(new_session_token.is_cancelled()); + } + + // --- classify_agent_error tests --- + + #[test] + fn classify_interrupted_cancelled_is_cancelled() { + let err = fabro_agent::Error::Interrupted(fabro_agent::InterruptReason::Cancelled); + assert!(matches!( + classify_agent_error(err, true), + AgentApiErrorDisposition::Cancelled + )); + } + + #[test] + fn classify_interrupted_wall_clock_is_terminal_precondition() { + let err = fabro_agent::Error::Interrupted(fabro_agent::InterruptReason::WallClockTimeout); + match classify_agent_error(err, true) { + AgentApiErrorDisposition::Terminal(Error::Precondition(msg)) => { + assert!(msg.contains("wall-clock")); + } + _ => panic!("expected Terminal(Error::Precondition) for WallClockTimeout"), + } + } + + #[test] + fn classify_failover_eligible_llm_returns_failover_when_allowed() { + let err = fabro_agent::Error::Llm(failover_eligible_llm_error()); + assert!(matches!( + classify_agent_error(err, true), + AgentApiErrorDisposition::FailoverEligible(_) + )); + } + + #[test] + fn classify_failover_eligible_llm_returns_terminal_when_not_allowed() { + let err = fabro_agent::Error::Llm(failover_eligible_llm_error()); + match classify_agent_error(err, false) { + AgentApiErrorDisposition::Terminal(Error::Llm(_)) => {} + _ => panic!("expected Terminal(Error::Llm) when failover disallowed"), + } + } + + #[test] + fn classify_non_failover_eligible_llm_is_terminal_llm() { + let err = fabro_agent::Error::Llm(non_failover_llm_error()); + match classify_agent_error(err, true) { + AgentApiErrorDisposition::Terminal(Error::Llm(_)) => {} + _ => panic!("expected Terminal(Error::Llm) for non-failover-eligible LLM error"), + } + } + + #[test] + fn classify_session_closed_is_terminal_handler() { + let err = fabro_agent::Error::SessionClosed; + match classify_agent_error(err, true) { + AgentApiErrorDisposition::Terminal(Error::Handler { message, .. }) => { + assert!(message.contains("Agent session failed")); + } + _ => panic!("expected Terminal(Error::Handler) for SessionClosed"), + } + } + + #[test] + fn classify_invalid_state_is_terminal_handler() { + let err = fabro_agent::Error::InvalidState("oops".into()); + match classify_agent_error(err, true) { + AgentApiErrorDisposition::Terminal(Error::Handler { message, .. }) => { + assert!(message.contains("Agent session failed")); + } + _ => panic!("expected Terminal(Error::Handler) for InvalidState"), + } + } + + #[test] + fn classify_tool_execution_is_terminal_handler() { + let err = fabro_agent::Error::ToolExecution("tool blew up".into()); + match classify_agent_error(err, true) { + AgentApiErrorDisposition::Terminal(Error::Handler { message, .. }) => { + assert!(message.contains("Agent session failed")); + } + _ => panic!("expected Terminal(Error::Handler) for ToolExecution"), + } + } } diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index 5036eec97..5b97c1a04 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -3,14 +3,14 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_agent::Sandbox; -use fabro_agent::sandbox::ExecResult; use fabro_auth::{CliAgentKind, CredentialResolver, CredentialUsage, ResolvedCredential}; use fabro_graphviz::graph::Node; use fabro_llm::types::TokenCounts; use fabro_model::Provider; -use fabro_types::CommandTermination; +use fabro_types::{CommandOutputStream, CommandTermination}; use fabro_util::time::elapsed_ms; -use tokio::time::sleep; +use tokio::sync::Mutex as TokioMutex; +use tokio_util::sync::CancellationToken; use super::super::agent::{CodergenBackend, CodergenResult}; use crate::context::Context; @@ -66,6 +66,7 @@ async fn ensure_cli( provider: Provider, sandbox: &Arc, emitter: &Arc, + cancel_token: &CancellationToken, ) -> Result<(), Error> { let start = std::time::Instant::now(); let cli_name = cli.name(); @@ -84,7 +85,7 @@ async fn ensure_cli( 30_000, None, None, - None, + Some(cancel_token.child_token()), ) .await .map_err(|e| { @@ -112,7 +113,13 @@ async fn ensure_cli( cli.npm_package() ); let install_result = sandbox - .exec_command(&install_cmd, 180_000, None, None, None) + .exec_command( + &install_cmd, + 180_000, + None, + None, + Some(cancel_token.child_token()), + ) .await .map_err(|e| Error::handler_with_source(format!("Failed to install {cli_name}"), &e))?; @@ -477,6 +484,7 @@ impl CodergenBackend for AgentCliBackend { emitter: &Arc, sandbox: &Arc, _tool_hooks: Option>, + cancel_token: CancellationToken, ) -> Result { // 1. Snapshot git state before the CLI run let files_before = self.detect_changed_files(sandbox).await; @@ -485,9 +493,6 @@ impl CodergenBackend for AgentCliBackend { let run_id = uuid::Uuid::new_v4().to_string(); let tmp_prefix = format!("/tmp/fabro_cli_{run_id}"); let prompt_path = format!("{tmp_prefix}_prompt.txt"); - let stdout_path = format!("{tmp_prefix}_stdout.log"); - let stderr_path = format!("{tmp_prefix}_stderr.log"); - let exit_code_path = format!("{tmp_prefix}_exit_code"); let env_path = format!("{tmp_prefix}_env.sh"); sandbox @@ -504,7 +509,7 @@ impl CodergenBackend for AgentCliBackend { // Ensure the CLI tool is installed in the sandbox let cli = AgentCli::for_provider(provider); - ensure_cli(cli, provider, sandbox, emitter).await?; + ensure_cli(cli, provider, sandbox, emitter, &cancel_token).await?; let command = cli_command_for_provider(provider, model, &prompt_path); let stage_scope = StageScope::for_handler(context, &node.id); @@ -521,11 +526,8 @@ impl CodergenBackend for AgentCliBackend { ); // Forward provider API key and custom env vars so the CLI tool can - // authenticate. Build a HashMap to pass via exec_command's env_vars - // parameter — this prepends `export` statements directly into the - // base64-encoded command, avoiding filesystem-to-process race - // conditions that can occur when writing an env file via the fs API and - // sourcing it via the process API. + // authenticate. Resolve credentials and run any pre-login command + // before the main CLI invocation. let cli_agent = match cli { AgentCli::Claude => CliAgentKind::Claude, AgentCli::Codex => CliAgentKind::Codex, @@ -541,7 +543,13 @@ impl CodergenBackend for AgentCliBackend { }; if let Some(login_cmd) = &cli_credential.login_command { let login_result = sandbox - .exec_command(login_cmd, 30_000, None, None, None) + .exec_command( + login_cmd, + 30_000, + None, + None, + Some(cancel_token.child_token()), + ) .await .map_err(|e| Error::handler_with_source("codex login failed", &e))?; if !login_result.is_success() { @@ -566,102 +574,176 @@ impl CodergenBackend for AgentCliBackend { launch_env.insert(name.clone(), val.clone()); } - // Also write env file as fallback for commands that source it (e.g. ensure_cli - // PATH) + // Write env file so the inner shell that runs the CLI command picks up + // PATH and provider env vars; we still pass `launch_env` to + // `exec_command_streaming` for parity. let mut env_lines: Vec = vec!["export PATH=\"$HOME/.local/bin:$PATH\"".to_string()]; env_lines.extend( launch_env .iter() .map(|(k, v)| format!("export {k}={}", shell_quote(v))), ); - { - sandbox - .write_file(&env_path, &env_lines.join("\n")) - .await - .map_err(|e| Error::handler_with_source("Failed to write env file", &e))?; - } + sandbox + .write_file(&env_path, &env_lines.join("\n")) + .await + .map_err(|e| Error::handler_with_source("Failed to write env file", &e))?; - // 3a. Disable auto-stop so the sandbox stays alive during long CLI runs + // Disable auto-stop so the sandbox stays alive during long CLI runs. if let Err(e) = sandbox.set_autostop_interval(0).await { tracing::warn!("Failed to disable sandbox auto-stop: {e}"); } - // 3b. Launch CLI command in background (env file is always written) - let inner_command = format!(". {env_path} && {command}"); - // Use setsid (if available) to create a new session so the child process is - // fully detached from the shell. Without this, Daytona's POST /process/execute - // blocks until ALL descendant processes exit, causing a 60s HTTP timeout. - // $SID is empty on macOS (where setsid doesn't exist but isn't needed since - // the local exec implementation doesn't wait for grandchildren). - let bg_command = format!( - "SID=$(command -v setsid || true)\n$SID sh -c '{inner_command} > {stdout_path} 2>{stderr_path}; echo $? > {exit_code_path}' /dev/null 2>&1 &\necho $!" - ); - let launch_start = std::time::Instant::now(); + // Stream the CLI command directly: the previous detached `setsid &` + // launcher could not be cancelled mid-flight. By running through + // `exec_command_streaming` the run-level cancel token (and node + // timeout, when set) terminate the CLI and its descendants. + let outer_command = format!(". {env_path} && {command}"); + let stdout_buffer: Arc>> = Arc::new(TokioMutex::new(Vec::new())); + let stderr_buffer: Arc>> = Arc::new(TokioMutex::new(Vec::new())); + let stdout_buf_cb = Arc::clone(&stdout_buffer); + let stderr_buf_cb = Arc::clone(&stderr_buffer); + let emitter_for_callback = Arc::clone(emitter); + let output_callback: fabro_agent::CommandOutputCallback = Arc::new(move |stream, bytes| { + let stdout_buf = Arc::clone(&stdout_buf_cb); + let stderr_buf = Arc::clone(&stderr_buf_cb); + let emitter = Arc::clone(&emitter_for_callback); + Box::pin(async move { + // Touch the stall watchdog whenever the CLI emits output + // so long-running invocations don't trip stall timeout. + emitter.touch(); + match stream { + CommandOutputStream::Stdout => { + stdout_buf.lock().await.extend_from_slice(&bytes); + } + CommandOutputStream::Stderr => { + stderr_buf.lock().await.extend_from_slice(&bytes); + } + } + Ok(()) + }) + }); let launch_env_ref = if launch_env.is_empty() { None } else { Some(&launch_env) }; - let launch_result = sandbox - .exec_command(&bg_command, 30_000, None, launch_env_ref, None) - .await - .map_err(|e| Error::handler_with_source("Failed to launch CLI command", &e))?; - let pid = launch_result.stdout.trim(); - tracing::info!(pid, "CLI process launched in background"); + let timeout_ms = node.timeout().map(crate::millis_u64); + let invocation_token = cancel_token.child_token(); + let launch_start = std::time::Instant::now(); + let streaming_result = sandbox + .exec_command_streaming( + &outer_command, + timeout_ms, + None, + launch_env_ref, + Some(invocation_token.clone()), + output_callback, + ) + .await; - // 3c. Poll for completion - let poll_command = - format!("[ -f {exit_code_path} ] && cat {exit_code_path} || echo running"); - let poll_interval = self.poll_interval; - let exit_code: i32 = loop { - sleep(poll_interval).await; - emitter.touch(); // keep the stall watchdog alive while polling - let poll_result = sandbox - .exec_command(&poll_command, 30_000, None, None, None) - .await - .map_err(|e| Error::handler_with_source("Failed to poll CLI command", &e))?; - let status = poll_result.stdout.trim(); - - if status != "running" { - break status.parse::().unwrap_or(-1); + let cleanup_temp_files = || { + let sandbox = Arc::clone(sandbox); + let cleanup_cmd = format!("rm -f {tmp_prefix}_*"); + async move { + let _ = sandbox + .exec_command(&cleanup_cmd, 30_000, None, None, None) + .await; } }; - // 3d. Read results - let duration_ms = u64::try_from(launch_start.elapsed().as_millis()).unwrap_or(u64::MAX); - let stdout_result = sandbox - .exec_command(&format!("cat {stdout_path}"), 60_000, None, None, None) - .await - .map_err(|e| Error::handler_with_source("Failed to read stdout", &e))?; - let stderr_result = sandbox - .exec_command(&format!("cat {stderr_path}"), 60_000, None, None, None) - .await - .map_err(|e| Error::handler_with_source("Failed to read stderr", &e))?; - - let result = ExecResult { - stdout: stdout_result.stdout, - stderr: stderr_result.stdout, - exit_code: Some(exit_code), - termination: CommandTermination::Exited, - duration_ms, + let streaming = match streaming_result { + Ok(streaming) => streaming, + Err(err) => { + cleanup_temp_files().await; + return Err(Error::handler_with_source( + "Failed to run CLI command", + &err, + )); + } }; - emitter.emit_scoped( - &Event::AgentCliCompleted { - node_id: node.id.clone(), - stdout: result.stdout.clone(), - stderr: result.stderr.clone(), - exit_code: result.exit_code.unwrap_or(-1), - duration_ms: result.duration_ms, - }, - &stage_scope, - ); + let result = streaming.result; + // Prefer the buffered streaming output (live chunks); fall back to the + // result struct for sandboxes that bundle output at the end. + let buffered_stdout = String::from_utf8_lossy(&stdout_buffer.lock().await).into_owned(); + let buffered_stderr = String::from_utf8_lossy(&stderr_buffer.lock().await).into_owned(); + let stdout = if buffered_stdout.is_empty() { + result.stdout.clone() + } else { + buffered_stdout + }; + let stderr = if buffered_stderr.is_empty() { + result.stderr.clone() + } else { + buffered_stderr + }; + let duration_ms = u64::try_from(launch_start.elapsed().as_millis()).unwrap_or(u64::MAX); - // 3e. Cleanup temp files - let _ = sandbox - .exec_command(&format!("rm -f {tmp_prefix}_*"), 30_000, None, None, None) - .await; + match result.termination { + CommandTermination::Cancelled => { + emitter.emit_scoped( + &Event::AgentCliCancelled { + node_id: node.id.clone(), + stdout: stdout.clone(), + stderr: stderr.clone(), + duration_ms, + }, + &stage_scope, + ); + cleanup_temp_files().await; + return Err(Error::Cancelled); + } + CommandTermination::TimedOut => { + emitter.emit_scoped( + &Event::AgentCliTimedOut { + node_id: node.id.clone(), + stdout: stdout.clone(), + stderr: stderr.clone(), + duration_ms, + }, + &stage_scope, + ); + cleanup_temp_files().await; + let tail = |s: &str, n: usize| -> String { + s.chars() + .rev() + .take(n) + .collect::>() + .into_iter() + .rev() + .collect() + }; + let stderr_tail = tail(&stderr, 500); + let stdout_tail = tail(&stdout, 500); + let detail = match (stderr_tail.is_empty(), stdout_tail.is_empty()) { + (false, false) => format!("{stderr_tail}\nstdout: {stdout_tail}"), + (false, true) => stderr_tail, + (true, false) => format!("stdout: {stdout_tail}"), + (true, true) => format!("command: {command}"), + }; + return Err(Error::handler(format!( + "CLI command timed out after {duration_ms} ms: {detail}" + ))); + } + CommandTermination::Exited => { + emitter.emit_scoped( + &Event::AgentCliCompleted { + node_id: node.id.clone(), + stdout: stdout.clone(), + stderr: stderr.clone(), + exit_code: result.exit_code.unwrap_or(-1), + duration_ms, + }, + &stage_scope, + ); + } + } - if !result.is_success() { + // Cleanup temp files (Exited path). + cleanup_temp_files().await; + + let exited_success = + result.termination == CommandTermination::Exited && result.exit_code == Some(0); + if !exited_success { let tail = |s: &str, n: usize| -> String { s.chars() .rev() @@ -671,8 +753,8 @@ impl CodergenBackend for AgentCliBackend { .rev() .collect() }; - let stderr_tail = tail(&result.stderr, 500); - let stdout_tail = tail(&result.stdout, 500); + let stderr_tail = tail(&stderr, 500); + let stdout_tail = tail(&stdout, 500); let detail = match (stderr_tail.is_empty(), stdout_tail.is_empty()) { (false, false) => format!("{stderr_tail}\nstdout: {stdout_tail}"), (false, true) => stderr_tail, @@ -681,12 +763,14 @@ impl CodergenBackend for AgentCliBackend { }; return Err(Error::handler(format!( "CLI command exited with code {}: {detail}", - result.display_exit_code(), + result + .exit_code + .map_or_else(|| "".to_string(), |c| c.to_string()), ))); } // 4. Parse the CLI output - let parsed = parse_cli_response(provider, &result.stdout) + let parsed = parse_cli_response(provider, &stdout) .ok_or_else(|| Error::handler("Failed to parse CLI output".to_string()))?; // 5. Detect changed files @@ -789,17 +873,32 @@ impl CodergenBackend for BackendRouter { emitter: &Arc, sandbox: &Arc, tool_hooks: Option>, + cancel_token: CancellationToken, ) -> Result { if self.should_use_cli(node) { self.cli_backend .run( - node, prompt, context, thread_id, emitter, sandbox, tool_hooks, + node, + prompt, + context, + thread_id, + emitter, + sandbox, + tool_hooks, + cancel_token, ) .await } else { self.api_backend .run( - node, prompt, context, thread_id, emitter, sandbox, tool_hooks, + node, + prompt, + context, + thread_id, + emitter, + sandbox, + tool_hooks, + cancel_token, ) .await } @@ -820,6 +919,7 @@ impl CodergenBackend for BackendRouter { mod tests { use std::path::Path; + use fabro_agent::sandbox::ExecResult; use fabro_graphviz::graph::AttrValue; use super::*; @@ -999,7 +1099,14 @@ mod tests { )); let emitter = Arc::new(Emitter::default()); - let result = ensure_cli(AgentCli::Claude, Provider::Anthropic, &sandbox, &emitter).await; + let result = ensure_cli( + AgentCli::Claude, + Provider::Anthropic, + &sandbox, + &emitter, + &CancellationToken::new(), + ) + .await; assert!(result.is_ok()); let commands = commands.lock().unwrap(); @@ -1020,7 +1127,14 @@ mod tests { )); let emitter = Arc::new(Emitter::default()); - let result = ensure_cli(AgentCli::Claude, Provider::Anthropic, &sandbox, &emitter).await; + let result = ensure_cli( + AgentCli::Claude, + Provider::Anthropic, + &sandbox, + &emitter, + &CancellationToken::new(), + ) + .await; assert!(result.is_ok()); let commands = commands.lock().unwrap(); @@ -1045,7 +1159,14 @@ mod tests { move |event| events.lock().unwrap().push(event.clone()) }); - let result = ensure_cli(AgentCli::Claude, Provider::Anthropic, &sandbox, &emitter).await; + let result = ensure_cli( + AgentCli::Claude, + Provider::Anthropic, + &sandbox, + &emitter, + &CancellationToken::new(), + ) + .await; assert!(result.is_err()); let error = result.unwrap_err().to_string(); assert!(error.contains("install exited with code 1")); @@ -1276,6 +1397,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { Ok(CodergenResult::Text { text: "stub".to_string(), @@ -1285,4 +1407,233 @@ mod tests { }) } } + + /// Sandbox stub whose `exec_command_streaming` returns a configurable + /// `CommandTermination` so we can exercise the cancel/timeout paths in + /// `AgentCliBackend::run` without spawning real processes. + struct StreamingCliMock { + commands: Arc>>, + termination: CommandTermination, + exit_code: Option, + } + + #[async_trait] + impl Sandbox for StreamingCliMock { + async fn read_file( + &self, + _path: &str, + _offset: Option, + _limit: Option, + ) -> fabro_sandbox::Result { + Ok(String::new()) + } + async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { + Ok(()) + } + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { + Ok(()) + } + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { + Ok(false) + } + async fn list_directory( + &self, + _path: &str, + _depth: Option, + ) -> fabro_sandbox::Result> { + Ok(vec![]) + } + async fn exec_command( + &self, + command: &str, + _timeout_ms: u64, + _working_dir: Option<&str>, + _env_vars: Option<&std::collections::HashMap>, + _cancel_token: Option, + ) -> fabro_sandbox::Result { + self.commands.lock().unwrap().push(command.to_string()); + // Default: success for git/version/cat/rm/ls. + if command.contains("--version") { + return Ok(ok_result()); + } + Ok(ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: Some(0), + termination: CommandTermination::Exited, + duration_ms: 1, + }) + } + async fn exec_command_streaming( + &self, + command: &str, + _timeout_ms: Option, + _working_dir: Option<&str>, + _env_vars: Option<&std::collections::HashMap>, + _cancel_token: Option, + _output_callback: fabro_agent::CommandOutputCallback, + ) -> fabro_sandbox::Result { + self.commands.lock().unwrap().push(command.to_string()); + Ok(fabro_sandbox::ExecStreamingResult { + result: ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: self.exit_code, + termination: self.termination, + duration_ms: 5, + }, + streams_separated: true, + live_streaming: true, + }) + } + async fn grep( + &self, + _pattern: &str, + _path: &str, + _options: &fabro_agent::sandbox::GrepOptions, + ) -> fabro_sandbox::Result> { + Ok(vec![]) + } + async fn glob( + &self, + _pattern: &str, + _path: Option<&str>, + ) -> fabro_sandbox::Result> { + Ok(vec![]) + } + async fn download_file_to_local(&self, _: &str, _: &Path) -> fabro_sandbox::Result<()> { + Ok(()) + } + async fn upload_file_from_local(&self, _: &Path, _: &str) -> fabro_sandbox::Result<()> { + Ok(()) + } + async fn initialize(&self) -> fabro_sandbox::Result<()> { + Ok(()) + } + async fn cleanup(&self) -> fabro_sandbox::Result<()> { + Ok(()) + } + fn working_directory(&self) -> &str { + "/workspace" + } + fn platform(&self) -> &str { + "linux" + } + fn os_version(&self) -> String { + "Ubuntu 22.04".into() + } + async fn set_autostop_interval(&self, _minutes: i32) -> fabro_sandbox::Result<()> { + Ok(()) + } + } + + fn collect_events(emitter: &Arc) -> Arc>> { + let events = Arc::new(Mutex::new(Vec::new())); + let events_clone = Arc::clone(&events); + emitter.on_event(move |event| events_clone.lock().unwrap().push(event.clone())); + events + } + + #[tokio::test] + async fn agent_cli_backend_run_emits_cancelled_event_and_returns_cancelled() { + let commands = Arc::new(Mutex::new(Vec::new())); + let sandbox: Arc = Arc::new(StreamingCliMock { + commands: Arc::clone(&commands), + termination: CommandTermination::Cancelled, + exit_code: None, + }); + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic); + let node = Node::new("step"); + let context = Context::new(); + let emitter = Arc::new(Emitter::default()); + let events = collect_events(&emitter); + + let result = backend + .run( + &node, + "Do something", + &context, + None, + &emitter, + &sandbox, + None, + CancellationToken::new(), + ) + .await; + + let Err(err) = result else { + panic!("cancelled streaming should bubble Error::Cancelled"); + }; + assert!(matches!(err, Error::Cancelled)); + + let events = events.lock().unwrap(); + let names: Vec = events + .iter() + .map(|e| e.body.event_name().to_string()) + .collect(); + assert!( + names.iter().any(|n| n == "agent.cli.cancelled"), + "expected agent.cli.cancelled, got events: {names:?}" + ); + assert!( + !names.iter().any(|n| n == "agent.cli.completed"), + "should not emit agent.cli.completed on cancellation" + ); + // Cleanup `rm -f` ran. + let cmds = commands.lock().unwrap(); + assert!( + cmds.iter().any(|c| c.starts_with("rm -f /tmp/fabro_cli_")), + "expected temp cleanup, got commands: {cmds:?}" + ); + } + + #[tokio::test] + async fn agent_cli_backend_run_emits_timed_out_event_and_returns_handler_error() { + let commands = Arc::new(Mutex::new(Vec::new())); + let sandbox: Arc = Arc::new(StreamingCliMock { + commands: Arc::clone(&commands), + termination: CommandTermination::TimedOut, + exit_code: None, + }); + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic); + let node = Node::new("step"); + let context = Context::new(); + let emitter = Arc::new(Emitter::default()); + let events = collect_events(&emitter); + + let result = backend + .run( + &node, + "Do something slow", + &context, + None, + &emitter, + &sandbox, + None, + CancellationToken::new(), + ) + .await; + + let Err(err) = result else { + panic!("timeout streaming should produce a handler error"); + }; + assert!( + matches!(err, Error::Handler { .. }), + "expected handler error on timeout, got {err:?}" + ); + + let events = events.lock().unwrap(); + let names: Vec = events + .iter() + .map(|e| e.body.event_name().to_string()) + .collect(); + assert!( + names.iter().any(|n| n == "agent.cli.timed_out"), + "expected agent.cli.timed_out, got events: {names:?}" + ); + assert!( + !names.iter().any(|n| n == "agent.cli.completed"), + "should not emit agent.cli.completed on timeout" + ); + } } diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index 86e340626..8cc366796 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use async_trait::async_trait; @@ -197,13 +196,12 @@ impl Handler for SubWorkflowHandler { let child_logs = run_dir.join(format!("stages/{}@{visit}/child", node.id)); let _ = fs::create_dir_all(&child_logs).await; - let cancel_token = Arc::new(AtomicBool::new(false)); - let child_cancel = Arc::clone(&cancel_token); + let child_run_token = services.run.cancel_token().child_token(); let child_run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: child_logs, - cancel_token: Some(cancel_token), + cancel_token: child_run_token.clone(), // Child workflows are part of the parent run's event stream. run_id: services.run.emitter.run_id(), labels: HashMap::new(), @@ -246,11 +244,14 @@ impl Handler for SubWorkflowHandler { .map_err(|err| Error::engine(err.to_string()))?; let artifact_store = ArtifactStore::new(object_store, "artifacts"); - // Spawn child engine + // Spawn child engine. Child runs receive a derived cancel token from + // the parent run; parent cancellation propagates parent-to-child via + // `child_token()`, but child cancellation does not cancel the parent. + let child_run_token_for_services = child_run_token.clone(); let mut child_handle = tokio::spawn(async move { let child_run = parent_run .with_run_store(run_store.into()) - .with_cancel_requested(None); + .with_cancel_token(child_run_token_for_services); let initialized = Initialized { graph: child_graph, source: String::new(), @@ -319,7 +320,7 @@ impl Handler for SubWorkflowHandler { if !stop_condition.is_empty() { let dummy_outcome = Outcome::success(); if evaluate_condition(stop_condition, &dummy_outcome, context) { - child_cancel.store(true, Ordering::Relaxed); + child_run_token.cancel(); // Give child a moment to wind down let _ = timeout( Duration::from_millis(100), @@ -337,7 +338,7 @@ impl Handler for SubWorkflowHandler { } // Max cycles exceeded — cancel child - child_cancel.store(true, Ordering::Relaxed); + child_run_token.cancel(); let _ = timeout(Duration::from_millis(100), &mut child_handle).await; Ok(Outcome::fail_classify(format!( diff --git a/lib/crates/fabro-workflow/src/handler/mod.rs b/lib/crates/fabro-workflow/src/handler/mod.rs index edd5b3f3a..8dc13ae6e 100644 --- a/lib/crates/fabro-workflow/src/handler/mod.rs +++ b/lib/crates/fabro-workflow/src/handler/mod.rs @@ -23,7 +23,6 @@ use fabro_interview::Interviewer; use crate::context::Context; use crate::error::Error; use crate::outcome::{Outcome, OutcomeExt}; -pub(crate) use crate::services::sandbox_cancel_token; pub use crate::services::{EngineServices, RunServices}; /// The handler interface for node execution. diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 39862586c..4f1d75684 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -463,6 +463,9 @@ impl Handler for ParallelHandler { Ok(Ok(result)) => { results.push(result); } + Ok(Err(Error::Cancelled)) => { + return Err(Error::Cancelled); + } Ok(Err(e)) => { results.push(BranchResult { id: String::new(), diff --git a/lib/crates/fabro-workflow/src/handler/prompt.rs b/lib/crates/fabro-workflow/src/handler/prompt.rs index a76099b80..9d1bad3ad 100644 --- a/lib/crates/fabro-workflow/src/handler/prompt.rs +++ b/lib/crates/fabro-workflow/src/handler/prompt.rs @@ -71,8 +71,10 @@ impl Handler for PromptHandler { working_dir, working_dir, provider, + &services.run.cancel_token(), ) - .await; + .await + .unwrap_or_default(); if docs.is_empty() { None @@ -115,6 +117,7 @@ impl Handler for PromptHandler { files_touched, .. }) => (text, usage, files_touched), + Err(Error::Cancelled) => return Err(Error::Cancelled), Err(e) if e.is_retryable() => { return Err(e); } @@ -185,6 +188,7 @@ mod tests { use fabro_types::fixtures; use object_store::memory::InMemory; use tempfile::TempDir; + use tokio_util::sync::CancellationToken; use super::*; @@ -270,6 +274,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { panic!("run() should not be called for prompt handler"); } @@ -330,6 +335,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { panic!("run() should not be called for prompt handler"); } @@ -387,6 +393,7 @@ mod tests { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: CancellationToken, ) -> Result { panic!("run() should not be called for prompt handler"); } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 0e0e9f195..e8d43d4fe 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -601,7 +601,7 @@ mod tests { Arc::new(RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: fixtures::RUN_1, labels: HashMap::new(), workflow_slug: Some("metadata".to_string()), @@ -998,7 +998,7 @@ mod tests { repo_dir.path().to_path_buf(), )), None, - None, + tokio_util::sync::CancellationToken::new(), fabro_model::Provider::Anthropic, Arc::new(fabro_auth::EnvCredentialSource::new()), Arc::new(SandboxGitRuntime::new()), diff --git a/lib/crates/fabro-workflow/src/operations/fork.rs b/lib/crates/fabro-workflow/src/operations/fork.rs index 8213ab0ca..f2a88c899 100644 --- a/lib/crates/fabro-workflow/src/operations/fork.rs +++ b/lib/crates/fabro-workflow/src/operations/fork.rs @@ -250,6 +250,8 @@ fn replay_event_for_fork_projection(body: &EventBody) -> bool { | EventBody::InterviewInterrupted(_) | EventBody::AgentSessionStarted(_) | EventBody::AgentCliStarted(_) + | EventBody::AgentCliCancelled(_) + | EventBody::AgentCliTimedOut(_) | EventBody::CommandStarted(_) | EventBody::CommandCompleted(_) | EventBody::ParallelCompleted(_) diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 4e623bcec..d519e7fba 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; @@ -30,6 +29,7 @@ use fabro_types::settings::run::{ use fabro_vault::Vault; use tokio::runtime::Handle; use tokio::sync::RwLock as AsyncRwLock; +use tokio_util::sync::CancellationToken; use crate::ManifestPath; use crate::artifact_upload::ArtifactSink; @@ -54,7 +54,7 @@ use crate::runtime_store::RunStoreHandle; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; struct RunSession { - cancel_token: Option>, + cancel_token: CancellationToken, emitter: Arc, sandbox: SandboxSpec, llm: LlmSpec, @@ -86,7 +86,7 @@ struct RunSession { pub struct StartServices { pub run_id: RunId, - pub cancel_token: Option>, + pub cancel_token: CancellationToken, pub emitter: Arc, pub interviewer: Arc, pub run_store: RunStoreHandle, @@ -831,7 +831,7 @@ impl RunSession { struct DetachedRunBootstrapGuard { run_id: RunId, event_sink: RunEventSink, - cancel_token: Option>, + cancel_token: CancellationToken, active: bool, } @@ -840,7 +840,7 @@ impl DetachedRunBootstrapGuard { run_id: RunId, _run_dir: &Path, event_sink: RunEventSink, - cancel_token: Option>, + cancel_token: CancellationToken, ) -> Self { Self { run_id, @@ -858,10 +858,7 @@ impl DetachedRunBootstrapGuard { impl Drop for DetachedRunBootstrapGuard { fn drop(&mut self) { if self.active { - let cancelled = self - .cancel_token - .as_ref() - .is_some_and(|token| token.load(Ordering::SeqCst)); + let cancelled = self.cancel_token.is_cancelled(); let reason = if cancelled { FailureReason::Cancelled } else { @@ -891,12 +888,12 @@ const POSTRUN_CANCELLED_MESSAGE: &str = "Run cancelled before post-run finalizat struct DetachedRunCompletionGuard { event_sink: RunEventSink, run_id: RunId, - cancel_token: Option>, + cancel_token: CancellationToken, active: bool, } impl DetachedRunCompletionGuard { - fn arm(run_id: RunId, event_sink: RunEventSink, cancel_token: Option>) -> Self { + fn arm(run_id: RunId, event_sink: RunEventSink, cancel_token: CancellationToken) -> Self { Self { event_sink, run_id, @@ -916,10 +913,7 @@ impl Drop for DetachedRunCompletionGuard { return; } - let cancelled = self - .cancel_token - .as_ref() - .is_some_and(|token| token.load(Ordering::SeqCst)); + let cancelled = self.cancel_token.is_cancelled(); let reason = if cancelled { FailureReason::Cancelled } else { @@ -1108,7 +1102,7 @@ mod tests { ) -> StartServices { StartServices { run_id: fixtures::RUN_1, - cancel_token: None, + cancel_token: CancellationToken::new(), emitter, interviewer: Arc::new(fabro_interview::AutoApproveInterviewer::engine()), run_store: store.open_run(&fixtures::RUN_1).await.unwrap().into(), diff --git a/lib/crates/fabro-workflow/src/pipeline/execute.rs b/lib/crates/fabro-workflow/src/pipeline/execute.rs index f7912548a..57ed86cfc 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute.rs @@ -243,9 +243,7 @@ pub async fn execute(init: Initialized) -> Executed { let mut builder = ExecutorBuilder::new(handler as Arc>) .lifecycle(Box::new(lifecycle)); - if let Some(ref cancel) = run_options.cancel_token { - builder = builder.cancel_token(cancel.clone()); - } + builder = builder.cancel_token(run_options.cancel_token.clone()); if let Some(token) = stall_token.clone() { builder = builder.stall_token(token); } diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index ff70db28c..31bd95a08 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -7,7 +7,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use std::sync::atomic::{AtomicU32, Ordering}; use std::time::Duration; use async_trait::async_trait; @@ -91,7 +91,7 @@ fn test_emitter_arc(label: &str) -> Arc { fn test_run_options(run_dir: &Path, run_id: &str) -> RunOptions { RunOptions { run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: test_run_id(run_id), settings: WorkflowSettings::default(), git: None, @@ -864,16 +864,16 @@ async fn execute_cancelled_mid_run() { g.edges.push(Edge::new("start", "work")); g.edges.push(Edge::new("work", "exit")); - let cancel_token = Arc::new(AtomicBool::new(false)); - let cancel_token_clone = Arc::clone(&cancel_token); + let cancel_token = tokio_util::sync::CancellationToken::new(); + let cancel_token_clone = cancel_token.clone(); let mut registry = make_registry(); registry.register("slow", Box::new(SlowHandler { sleep_ms: 200 })); let mut run_options = test_run_options(dir.path(), "test-run"); - run_options.cancel_token = Some(cancel_token); + run_options.cancel_token = cancel_token; tokio::spawn(async move { tokio::time::sleep(Duration::from_millis(50)).await; - cancel_token_clone.store(true, Ordering::Relaxed); + cancel_token_clone.cancel(); }); let result = run_graph( @@ -901,16 +901,16 @@ async fn execute_cancelled_mid_run_persists_cancelled_status() { g.edges.push(Edge::new("start", "work")); g.edges.push(Edge::new("work", "exit")); - let cancel_token = Arc::new(AtomicBool::new(false)); - let cancel_token_clone = Arc::clone(&cancel_token); + let cancel_token = tokio_util::sync::CancellationToken::new(); + let cancel_token_clone = cancel_token.clone(); let mut registry = make_registry(); registry.register("slow", Box::new(SlowHandler { sleep_ms: 200 })); let mut run_options = test_run_options(dir.path(), "test-run"); - run_options.cancel_token = Some(cancel_token); + run_options.cancel_token = cancel_token; tokio::spawn(async move { tokio::time::sleep(Duration::from_millis(50)).await; - cancel_token_clone.store(true, Ordering::Relaxed); + cancel_token_clone.cancel(); }); let executed = execute_test_run_with_options(run_options, g, Some(Arc::new(registry))).await; diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 921697369..6e71ca4ca 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -621,7 +621,7 @@ mod tests { RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: test_run_id(), labels: HashMap::new(), workflow_slug: None, @@ -816,7 +816,7 @@ mod tests { emitter, sandbox, None, - None, + tokio_util::sync::CancellationToken::new(), fabro_model::Provider::Anthropic, Arc::new(fabro_auth::EnvCredentialSource::new()), Arc::new(SandboxGitRuntime::new()), @@ -842,7 +842,7 @@ mod tests { std::env::current_dir().unwrap(), )), None, - None, + tokio_util::sync::CancellationToken::new(), fabro_model::Provider::Anthropic, Arc::new(fabro_auth::EnvCredentialSource::new()), Arc::new(SandboxGitRuntime::new()), diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index ce125827b..6f035592e 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -30,7 +30,7 @@ use crate::error::Error; use crate::event::{Emitter, Event, RunNoticeLevel}; use crate::git::RUN_BRANCH_PREFIX; use crate::handler::llm::{AgentApiBackend, AgentCliBackend, BackendRouter}; -use crate::handler::{HandlerRegistry, default_registry, sandbox_cancel_token}; +use crate::handler::{HandlerRegistry, default_registry}; use crate::run_metadata::{ RunMetadataRuntime, build_metadata_writer, metadata_branch_name, mint_token, }; @@ -637,23 +637,21 @@ pub async fn initialize( index, }); let cmd_start = Instant::now(); - let cancel_token = sandbox_cancel_token(options.run_options.cancel_token.clone()); + let cancel_token = options.run_options.cancel_token.child_token(); let result = sandbox .exec_command( command, options.lifecycle.setup_command_timeout_ms, None, None, - cancel_token.clone(), + Some(cancel_token.clone()), ) .await .map_err(|e| Error::engine_with_source("Setup command failed", &e))?; - if let Some(token) = &cancel_token { - if token.is_cancelled() { - return Err(Error::Cancelled); - } - token.cancel(); + if options.run_options.cancel_token.is_cancelled() { + return Err(Error::Cancelled); } + cancel_token.cancel(); let duration_ms = crate::millis_u64(cmd_start.elapsed()); if !result.is_success() { let exit_code = result.display_exit_code(); @@ -753,7 +751,6 @@ pub async fn initialize( mod tests { use std::collections::HashMap; use std::sync::Arc; - use std::sync::atomic::AtomicBool; use std::time::Duration; use fabro_auth::{AuthCredential, AuthDetails}; @@ -846,7 +843,7 @@ mod tests { RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: test_run_id(), labels: HashMap::new(), workflow_slug: None, @@ -1257,9 +1254,10 @@ mod tests { std::fs::create_dir_all(&run_dir).unwrap(); let (graph, source) = simple_graph(); let persisted = test_persisted(graph, source, &run_dir); - let cancel_token = Arc::new(AtomicBool::new(true)); + let cancel_token = tokio_util::sync::CancellationToken::new(); + cancel_token.cancel(); let mut run_options = test_settings(&run_dir); - run_options.cancel_token = Some(cancel_token); + run_options.cancel_token = cancel_token; let result = initialize(persisted, InitOptions { run_id: test_run_id(), @@ -1318,9 +1316,10 @@ mod tests { std::fs::create_dir_all(&run_dir).unwrap(); let (graph, source) = simple_graph(); let persisted = test_persisted(graph, source, &run_dir); - let cancel_token = Arc::new(AtomicBool::new(true)); + let cancel_token = tokio_util::sync::CancellationToken::new(); + cancel_token.cancel(); let mut run_options = test_settings(&run_dir); - run_options.cancel_token = Some(cancel_token); + run_options.cancel_token = cancel_token; let result = initialize(persisted, InitOptions { run_id: test_run_id(), diff --git a/lib/crates/fabro-workflow/src/pipeline/retro.rs b/lib/crates/fabro-workflow/src/pipeline/retro.rs index 649435c2e..47e0070b0 100644 --- a/lib/crates/fabro-workflow/src/pipeline/retro.rs +++ b/lib/crates/fabro-workflow/src/pipeline/retro.rs @@ -304,7 +304,7 @@ mod tests { RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: test_run_id(), labels: HashMap::new(), workflow_slug: None, @@ -340,7 +340,7 @@ mod tests { Arc::clone(&emitter), Arc::clone(&sandbox), None, - None, + tokio_util::sync::CancellationToken::new(), fabro_llm::Provider::Anthropic, test_llm_source(), Arc::new(crate::sandbox_git_runtime::SandboxGitRuntime::new()), @@ -395,7 +395,7 @@ mod tests { std::env::current_dir().unwrap(), )), None, - None, + tokio_util::sync::CancellationToken::new(), fabro_llm::Provider::Anthropic, test_llm_source(), Arc::new(crate::sandbox_git_runtime::SandboxGitRuntime::new()), diff --git a/lib/crates/fabro-workflow/src/run_metadata.rs b/lib/crates/fabro-workflow/src/run_metadata.rs index bcfde102b..de27f743e 100644 --- a/lib/crates/fabro-workflow/src/run_metadata.rs +++ b/lib/crates/fabro-workflow/src/run_metadata.rs @@ -642,7 +642,7 @@ mod tests { RunOptions { settings: WorkflowSettings::default(), run_dir: tempfile::tempdir().unwrap().path().to_path_buf(), - cancel_token: None, + cancel_token: tokio_util::sync::CancellationToken::new(), run_id: fabro_types::fixtures::RUN_1, labels: HashMap::new(), workflow_slug: Some("metadata".to_string()), diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index 705dc6a65..6a9d9f65a 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -1,10 +1,9 @@ use std::collections::HashMap; use std::path::PathBuf; -use std::sync::Arc; -use std::sync::atomic::AtomicBool; use fabro_types::settings::run::RunMode; use fabro_types::{ForkSourceRef, GitContext, RunId, WorkflowSettings}; +use tokio_util::sync::CancellationToken; use crate::git::{GitAuthor, git_author_from_settings}; @@ -21,7 +20,10 @@ pub struct GitCheckpointOptions { pub struct RunOptions { pub settings: WorkflowSettings, pub run_dir: PathBuf, - pub cancel_token: Option>, + /// Cancellation token for this run. Cancelling this token cancels the + /// run and propagates to handlers, sandbox commands, and child runs. + /// Default constructors should use `CancellationToken::new()`. + pub cancel_token: CancellationToken, /// Unique identifier for this workflow run. pub run_id: RunId, /// User-defined key-value labels for this run. diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index ae3912790..5ad0b983b 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; #[cfg(test)] use std::path::PathBuf; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; +#[cfg(test)] use std::time::Duration; use fabro_agent::Sandbox; @@ -11,7 +11,6 @@ use fabro_auth::CredentialSource; use fabro_auth::ResolvedCredentials; use fabro_hooks::{HookContext, HookDecision, HookRunner}; use fabro_model::Provider; -use tokio::time; use tokio_util::sync::CancellationToken; use crate::ManifestPath; @@ -24,13 +23,20 @@ use crate::sandbox_git_runtime::SandboxGitRuntime; use crate::workflow_bundle::WorkflowBundle; /// Services shared across workflow phases. +/// +/// Production construction is expected to happen from pipeline initialization +/// with the run's root cancellation token. Use +/// [`RunServices::with_cancel_token`] only with the same root token or a +/// `child_token()` derived from it. The token semantically means "cancel this +/// run or child run," not a generic shutdown signal — dropping a `RunServices` +/// does NOT count as cancellation. #[derive(Clone)] pub struct RunServices { pub run_store: RunStoreHandle, pub emitter: Arc, pub sandbox: Arc, pub hook_runner: Option>, - pub cancel_requested: Option>, + pub(crate) cancel_token: CancellationToken, pub provider: Provider, pub llm_source: Arc, pub(crate) sandbox_git: Arc, @@ -45,7 +51,7 @@ impl RunServices { emitter: Arc, sandbox: Arc, hook_runner: Option>, - cancel_requested: Option>, + cancel_token: CancellationToken, provider: Provider, llm_source: Arc, sandbox_git: Arc, @@ -57,7 +63,7 @@ impl RunServices { emitter, sandbox, hook_runner, - cancel_requested, + cancel_token, provider, llm_source, sandbox_git, @@ -66,10 +72,11 @@ impl RunServices { }) } - /// Bridge the core executor's atomic cancel flag to sandbox command - /// cancellation. - pub fn sandbox_cancel_token(&self) -> Option { - sandbox_cancel_token(self.cancel_requested.clone()) + /// The run-level cancellation token. Cancel this to terminate the run. + /// Derive child tokens via `cancel_token().child_token()` for sandbox + /// command invocations. + pub fn cancel_token(&self) -> CancellationToken { + self.cancel_token.clone() } /// Run lifecycle hooks and return the merged decision. @@ -107,13 +114,15 @@ impl RunServices { }) } + /// Replace the cancellation token. Use only with the same root token or + /// a child derived from it via `child_token()`. #[must_use] - pub fn with_cancel_requested( + pub(crate) fn with_cancel_token( self: &Arc, - cancel_requested: Option>, + cancel_token: CancellationToken, ) -> Arc { Arc::new(Self { - cancel_requested, + cancel_token, ..self.as_ref().clone() }) } @@ -209,7 +218,7 @@ impl EngineServices { std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), )), None, - None, + CancellationToken::new(), Provider::Anthropic, Arc::new(StubCredentialSource), Arc::new(SandboxGitRuntime::new()), @@ -227,34 +236,6 @@ impl EngineServices { } } -pub(crate) fn sandbox_cancel_token( - cancel_requested: Option>, -) -> Option { - let cancel_requested = cancel_requested?; - let token = CancellationToken::new(); - - if cancel_requested.load(Ordering::Relaxed) { - token.cancel(); - return Some(token); - } - - let token_clone = token.clone(); - tokio::spawn(async move { - loop { - if token_clone.is_cancelled() { - return; - } - if cancel_requested.load(Ordering::Relaxed) { - token_clone.cancel(); - return; - } - time::sleep(Duration::from_millis(10)).await; - } - }); - - Some(token) -} - #[cfg(test)] mod tests { use super::EngineServices; diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index b8188a539..ef32bbc84 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -41,6 +41,7 @@ use fabro_workflow::records::Checkpoint; use fabro_workflow::run_options::{GitCheckpointOptions, RunOptions}; use fabro_workflow::test_support::{WorkflowRunner, test_store_dir}; use object_store::local::LocalFileSystem; +use tokio_util::sync::CancellationToken; use ulid::Ulid; fn test_run_id(label: &str) -> RunId { @@ -249,7 +250,7 @@ async fn daytona_exec_command_cancelled() { let env = create_env_with_github_app(Some(creds)).await; env.initialize().await.unwrap(); - let token = tokio_util::sync::CancellationToken::new(); + let token = CancellationToken::new(); let token_clone = token.clone(); // Cancel the token shortly after starting @@ -513,7 +514,7 @@ async fn daytona_pipeline_artifact_offload_and_sync() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -698,7 +699,7 @@ async fn daytona_git_checkpoint_remote_emits_events() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("git-cp-test"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -871,7 +872,7 @@ async fn daytona_parallel_git_branching_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: run_tmp.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id, labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1078,6 +1079,7 @@ async fn run_daytona_cli_test(provider: Provider, model: &str, install_command: &emitter, &env, None, + CancellationToken::new(), ) .await; @@ -1209,7 +1211,7 @@ async fn daytona_git_checkpoint_with_shadow_branch() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id, labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1366,7 +1368,7 @@ async fn daytona_asset_collection() { ..WorkflowSettings::default() }, run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("artifact-test-daytona"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1634,7 +1636,7 @@ async fn daytona_git_push_run_branch_to_origin() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id, labels: std::collections::HashMap::new(), workflow_slug: None, diff --git a/lib/crates/fabro-workflow/tests/it/git_integration.rs b/lib/crates/fabro-workflow/tests/it/git_integration.rs index 611b2f055..8ff8f342d 100644 --- a/lib/crates/fabro-workflow/tests/it/git_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/git_integration.rs @@ -21,6 +21,7 @@ use fabro_workflow::handler::exit::ExitHandler; use fabro_workflow::handler::start::StartHandler; use fabro_workflow::run_options::{GitCheckpointOptions, RunOptions}; use fabro_workflow::test_support::run_graph; +use tokio_util::sync::CancellationToken; fn assert_success(output: &Output, context: &str) { assert!( @@ -154,7 +155,7 @@ fn make_registry() -> HandlerRegistry { fn test_run_options(run_dir: &Path) -> RunOptions { RunOptions { run_dir: run_dir.to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: fixtures::RUN_2, settings: WorkflowSettings::default(), git: None, diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 002d473da..6ee580bc8 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -54,6 +54,7 @@ use fabro_workflow::test_support::{WorkflowRunner, run_graph_with_hooks, test_st use fabro_workflow::transforms::stylesheet::{apply_stylesheet, parse_stylesheet}; use fabro_workflow::transforms::{StylesheetApplicationTransform, TemplateTransform, Transform}; use object_store::local::LocalFileSystem; +use tokio_util::sync::CancellationToken; use ulid::Ulid; fn local_env() -> Arc { @@ -416,7 +417,7 @@ async fn end_to_end_linear_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -549,7 +550,7 @@ async fn end_to_end_branching_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -669,7 +670,7 @@ async fn end_to_end_human_gate_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -765,7 +766,7 @@ async fn human_gate_interrupted_input_fails_closed_without_fail_route() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -878,7 +879,7 @@ async fn human_gate_interrupted_input_routes_via_outcome_fail_condition() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -992,7 +993,7 @@ async fn goal_gate_routes_to_retry_target_on_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1115,7 +1116,7 @@ async fn goal_gate_routes_to_retry_target_when_present() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1430,7 +1431,7 @@ async fn retry_on_failure_then_succeed() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1505,7 +1506,7 @@ async fn pipeline_with_many_nodes() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1593,6 +1594,7 @@ impl CodergenBackend for MockCodergenBackend { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: tokio_util::sync::CancellationToken, ) -> Result { Ok(CodergenResult::Text { text: format!( @@ -1852,7 +1854,7 @@ async fn smoke_test_with_mock_codergen_backend() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -1954,7 +1956,7 @@ async fn end_to_end_parallel_fan_out_fan_in() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2067,7 +2069,7 @@ async fn resume_from_checkpoint_completes_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2166,7 +2168,7 @@ async fn resume_from_checkpoint_preserves_goal_gate_outcomes() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2209,7 +2211,7 @@ async fn graph_goal_in_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2248,7 +2250,7 @@ async fn event_streaming_lifecycle() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2328,7 +2330,7 @@ async fn context_flow_between_stages() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2384,7 +2386,7 @@ async fn tool_handler_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2455,7 +2457,7 @@ async fn auto_approve_interviewer_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2495,7 +2497,7 @@ async fn codergen_without_backend_simulated() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2600,7 +2602,7 @@ async fn branching_loop_back_on_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2686,7 +2688,7 @@ async fn human_gate_loops_back() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2751,7 +2753,7 @@ async fn scenario_ship_a_feature() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2839,7 +2841,7 @@ async fn scenario_parallel_expert_review() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2926,7 +2928,7 @@ async fn scenario_node_retries_on_retry_status() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -2991,7 +2993,7 @@ async fn scenario_loop_restart_resets_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3059,7 +3061,7 @@ async fn scenario_bug_triage_router() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3121,7 +3123,7 @@ async fn scenario_crash_recovery() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3231,7 +3233,7 @@ async fn manager_loop_stop_condition_satisfied_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3313,7 +3315,7 @@ async fn manager_loop_max_cycles_exceeded_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3456,7 +3458,7 @@ async fn conditional_branching_success_fail_paths() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3512,7 +3514,7 @@ async fn edge_selection_condition_match_wins_over_weight() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3562,7 +3564,7 @@ async fn edge_selection_weight_breaks_ties() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3604,7 +3606,7 @@ async fn edge_selection_lexical_tiebreak() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3665,7 +3667,7 @@ async fn context_updates_visible_across_nodes() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3712,7 +3714,7 @@ async fn stylesheet_applies_model_override() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3768,7 +3770,7 @@ async fn custom_handler_registration_and_execution() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3846,7 +3848,7 @@ async fn integration_smoke_plan_implement_review_done() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -3938,7 +3940,7 @@ async fn manager_loop_runs_child_engine_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4072,7 +4074,7 @@ async fn manager_loop_context_flows_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4148,7 +4150,7 @@ async fn manager_loop_child_dotfile_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4252,7 +4254,7 @@ async fn import_e2e_through_engine() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4407,7 +4409,7 @@ async fn fidelity_default_is_compact() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4464,7 +4466,7 @@ async fn fidelity_graph_default_applied() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4517,7 +4519,7 @@ async fn fidelity_node_overrides_graph_default() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4576,7 +4578,7 @@ async fn fidelity_edge_overrides_node_and_graph() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4625,7 +4627,7 @@ async fn fidelity_full_produces_empty_preamble() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4684,7 +4686,7 @@ async fn fidelity_truncate_preamble_minimal() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4756,7 +4758,7 @@ async fn fidelity_summary_low_mode() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4823,7 +4825,7 @@ async fn fidelity_summary_medium_mode() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4890,7 +4892,7 @@ async fn fidelity_summary_high_mode() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -4950,7 +4952,7 @@ async fn fidelity_full_sets_thread_id_in_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5021,7 +5023,7 @@ async fn fidelity_full_nodes_share_thread_id() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5102,7 +5104,7 @@ async fn fidelity_resume_degrades_full_to_summary_high() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5199,7 +5201,7 @@ async fn fidelity_resume_degrade_only_affects_first_hop() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5283,7 +5285,7 @@ async fn fidelity_resume_no_degrade_when_not_full() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5325,7 +5327,7 @@ async fn fidelity_stored_in_checkpoint_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5418,7 +5420,7 @@ async fn fidelity_precedence_multi_node_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5486,7 +5488,7 @@ async fn fidelity_compact_preamble_includes_completed_stages_and_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5561,7 +5563,7 @@ async fn fidelity_summary_low_excludes_context_values_in_pipeline() { let run_options_low = RunOptions { settings: WorkflowSettings::default(), run_dir: dir_low.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5628,7 +5630,7 @@ async fn fidelity_summary_low_excludes_context_values_in_pipeline() { let run_options_med = RunOptions { settings: WorkflowSettings::default(), run_dir: dir_med.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5700,7 +5702,7 @@ async fn fidelity_thread_id_fallback_to_previous_node_in_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5754,7 +5756,7 @@ async fn fidelity_thread_id_from_node_class_in_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5811,7 +5813,7 @@ async fn fidelity_edge_thread_id_override_in_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5869,7 +5871,7 @@ async fn fidelity_full_without_explicit_thread_id_uses_previous_node() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5937,7 +5939,7 @@ async fn fidelity_from_parsed_dot_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -5986,7 +5988,7 @@ async fn fidelity_checkpoint_roundtrip_preserves_fidelity() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6059,7 +6061,7 @@ async fn fidelity_node_thread_id_overrides_edge_thread_id_in_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6146,7 +6148,7 @@ async fn fidelity_resume_preserves_context_values_across_checkpoint() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6194,6 +6196,7 @@ mod real_llm { use fabro_workflow::context::Context; use fabro_workflow::error::Error; use fabro_workflow::handler::agent::{AgentHandler, CodergenBackend, CodergenResult}; + use tokio_util::sync::CancellationToken; struct LlmCodergenBackend { client: Arc, @@ -6212,6 +6215,7 @@ mod real_llm { _emitter: &Arc, _sandbox: &Arc, _tool_hooks: Option>, + _cancel_token: tokio_util::sync::CancellationToken, ) -> Result { self.complete(prompt).await } @@ -6382,7 +6386,7 @@ mod real_llm { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6491,7 +6495,7 @@ mod real_llm { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6624,7 +6628,7 @@ mod real_llm { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6725,7 +6729,7 @@ mod real_llm { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6863,7 +6867,7 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("vault-only-openai-codex-pr-body"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -6975,7 +6979,7 @@ async fn human_gate_freeform_only_routes_text() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -7106,7 +7110,7 @@ async fn human_gate_freeform_with_fixed_choice_match() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -7223,7 +7227,7 @@ async fn human_gate_freeform_fallback_on_unmatched_text() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -7351,7 +7355,7 @@ async fn human_gate_freeform_sets_allow_freeform_on_question() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -7460,7 +7464,7 @@ async fn human_gate_without_freeform_sets_allow_freeform_false() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -7764,7 +7768,7 @@ fn make_run_options(dir: &std::path::Path) -> RunOptions { RunOptions { settings: WorkflowSettings::default(), run_dir: dir.to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("hook-test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -8703,7 +8707,7 @@ async fn run_fidelity_prompt_pipeline(fidelity: &str) -> String { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -8905,7 +8909,7 @@ async fn large_context_values_are_offloaded_to_artifact_store() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -9110,7 +9114,7 @@ async fn artifact_pointers_rewritten_for_remote_sandbox() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -9198,7 +9202,7 @@ async fn downstream_local_execution_materializes_blob_refs_to_runtime_files() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -9286,7 +9290,7 @@ async fn downstream_remote_execution_materializes_blob_refs_to_sandbox_files() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -9417,7 +9421,7 @@ async fn node_dir_uses_visit_count_on_revisit() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -9566,46 +9570,11 @@ impl fabro_agent::Sandbox for CliTestEnv { }); } - // Background launch: return PID - if command.contains("echo $!") { + // CLI version check during ensure_cli — return success so install path + // is skipped. + if command.contains("--version") { return Ok(fabro_agent::ExecResult { - stdout: "12345\n".into(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // Poll for completion: return exit code 0 immediately - if command.contains("exit_code") && command.contains("echo running") { - return Ok(fabro_agent::ExecResult { - stdout: "0\n".into(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // Read stdout file - if command.starts_with("cat") && command.contains("stdout.log") { - return Ok(fabro_agent::ExecResult { - stdout: self.cli_stdout.clone(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // Read stderr file - if command.starts_with("cat") && command.contains("stderr.log") { - return Ok(fabro_agent::ExecResult { - stdout: String::new(), + stdout: "1.0.0\n".into(), stderr: String::new(), exit_code: Some(0), @@ -9626,7 +9595,21 @@ impl fabro_agent::Sandbox for CliTestEnv { }); } - // Fallback + // ls -t for last_file_touched + if command.starts_with("ls -t ") { + return Ok(fabro_agent::ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: Some(0), + + termination: CommandTermination::Exited, + duration_ms: 1, + }); + } + + // Fallback: this is the streaming CLI invocation. The default trait + // implementation of `exec_command_streaming` delegates to this path + // and replays output through the streaming callback. Ok(fabro_agent::ExecResult { stdout: self.cli_stdout.clone(), stderr: String::new(), @@ -9714,6 +9697,7 @@ async fn cli_backend_run_writes_prompt_and_calls_exec() { &emitter, &env, None, + CancellationToken::new(), ) .await .expect("CLI backend should succeed"); @@ -9731,20 +9715,21 @@ async fn cli_backend_run_writes_prompt_and_calls_exec() { ); assert_eq!(prompt_file.1, "Fix the authentication bug"); - // Verify the CLI command was called (now wrapped in background launch) + // Verify the CLI command was streamed (env file is sourced, then `cat + // | claude -p ...` runs as the inner shell command). let commands = test_env.recorded_commands(); let cli_cmd = commands .iter() - .find(|c| c.contains("claude") && c.contains("echo $!")) - .expect("should launch claude CLI in background"); + .find(|c| c.contains("claude") && c.contains("_prompt.txt")) + .expect("should run claude CLI command"); assert!(cli_cmd.contains("-p"), "should use pipe mode"); assert!( cli_cmd.contains("claude-opus-4-6"), "should use correct model" ); assert!( - cli_cmd.contains("_prompt.txt"), - "should reference prompt file" + cli_cmd.contains(". /tmp/fabro_cli_") && cli_cmd.contains("_env.sh"), + "should source the env file before invoking the CLI: {cli_cmd}" ); // Verify parsed response @@ -9786,6 +9771,7 @@ async fn cli_backend_run_detects_changed_files() { &emitter, &env, None, + CancellationToken::new(), ) .await .expect("CLI backend should succeed"); @@ -9811,16 +9797,25 @@ async fn cli_backend_run_with_codex_provider() { let emitter = Arc::new(Emitter::default()); let result = backend - .run(&node, "Build the API", &context, None, &emitter, &env, None) + .run( + &node, + "Build the API", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("CLI backend should succeed"); - // Verify codex command was called (now wrapped in background launch) + // Verify codex command was streamed. let commands = test_env.recorded_commands(); let cli_cmd = commands .iter() - .find(|c| c.contains("codex") && c.contains("echo $!")) - .expect("should launch codex CLI in background"); + .find(|c| c.contains("codex") && c.contains("_prompt.txt")) + .expect("should run codex CLI command"); assert!(cli_cmd.contains("exec --json"), "should use exec mode"); assert!( cli_cmd.contains("gpt-5.3-codex"), @@ -9888,10 +9883,10 @@ async fn cli_backend_run_fails_on_nonzero_exit() { duration_ms: 0, }); } - // Background launch: return PID - if command.contains("echo $!") { + // CLI version check during ensure_cli — pretend already installed. + if command.contains("--version") { return Ok(fabro_agent::ExecResult { - stdout: "12345\n".into(), + stdout: "1.0.0\n".into(), stderr: String::new(), exit_code: Some(0), @@ -9899,24 +9894,12 @@ async fn cli_backend_run_fails_on_nonzero_exit() { duration_ms: 0, }); } - // Poll: return non-zero exit code - if command.contains("exit_code") && command.contains("echo running") { + // The streaming CLI invocation: return non-zero exit with stderr. + if command.contains("claude") || command.contains("codex") { return Ok(fabro_agent::ExecResult { - stdout: "127\n".into(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 0, - }); - } - // Read stderr file - if command.starts_with("cat") && command.contains("stderr.log") { - return Ok(fabro_agent::ExecResult { - stdout: "command not found: claude".into(), - stderr: String::new(), - exit_code: Some(0), - + stdout: String::new(), + stderr: "command not found: claude".into(), + exit_code: Some(127), termination: CommandTermination::Exited, duration_ms: 0, }); @@ -9990,6 +9973,7 @@ async fn cli_backend_run_fails_on_nonzero_exit() { &emitter, &failing_env, None, + CancellationToken::new(), ) .await; @@ -10019,7 +10003,16 @@ async fn cli_backend_run_fails_on_unparseable_output() { let emitter = Arc::new(Emitter::default()); let result = backend - .run(&node, "do something", &context, None, &emitter, &env, None) + .run( + &node, + "do something", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await; let err = match result { @@ -10052,14 +10045,23 @@ async fn cli_backend_run_uses_node_model_override() { let emitter = Arc::new(Emitter::default()); backend - .run(&node, "test", &context, None, &emitter, &env, None) + .run( + &node, + "test", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("should succeed"); let commands = test_env.recorded_commands(); let cli_cmd = commands .iter() - .find(|c| c.contains("claude") && c.contains("echo $!")) + .find(|c| c.contains("claude") && c.contains("_prompt.txt")) .unwrap(); assert!( cli_cmd.contains("claude-sonnet-4-5"), @@ -10093,14 +10095,23 @@ async fn cli_backend_run_uses_node_provider_override() { let emitter = Arc::new(Emitter::default()); backend - .run(&node, "test", &context, None, &emitter, &env, None) + .run( + &node, + "test", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("should succeed"); let commands = test_env.recorded_commands(); let cli_cmd = commands .iter() - .find(|c| c.contains("codex") && c.contains("echo $!")) + .find(|c| c.contains("codex") && c.contains("_prompt.txt")) .expect("should launch codex based on provider override"); assert!(cli_cmd.contains("gpt-5.3-codex")); } @@ -10118,7 +10129,16 @@ async fn cli_backend_run_returns_text_and_usage() { let emitter = Arc::new(Emitter::default()); let result = backend - .run(&node, "test", &context, None, &emitter, &env, None) + .run( + &node, + "test", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("should succeed"); @@ -10158,7 +10178,16 @@ async fn backend_router_delegates_to_cli_for_cli_node() { let emitter = Arc::new(Emitter::default()); let result = router - .run(&node, "Fix the bug", &context, None, &emitter, &env, None) + .run( + &node, + "Fix the bug", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("router should succeed"); @@ -10192,7 +10221,16 @@ async fn backend_router_delegates_to_api_for_normal_node() { let emitter = Arc::new(Emitter::default()); let result = router - .run(&node, "Plan the work", &context, None, &emitter, &env, None) + .run( + &node, + "Plan the work", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("router should succeed"); @@ -10229,7 +10267,16 @@ async fn backend_router_delegates_to_cli_for_backend_attr() { let emitter = Arc::new(Emitter::default()); let result = router - .run(&node, "Build it", &context, None, &emitter, &env, None) + .run( + &node, + "Build it", + &context, + None, + &emitter, + &env, + None, + CancellationToken::new(), + ) .await .expect("router should succeed"); @@ -10322,7 +10369,7 @@ async fn full_pipeline_with_cli_backend_node() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -10441,7 +10488,7 @@ async fn stylesheet_backend_property_routes_to_cli() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -10638,7 +10685,7 @@ async fn git_checkpoint_host_emits_events_and_diff_patch() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-docker"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -10804,7 +10851,7 @@ async fn git_checkpoint_host_skips_metadata_branch_without_writer_prereqs() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id, labels: std::collections::HashMap::new(), workflow_slug: None, @@ -10995,7 +11042,7 @@ async fn parallel_git_branching_host_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id, labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11245,7 +11292,7 @@ async fn git_checkpoint_host_skips_empty_diff_patch() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("empty-diff"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11616,7 +11663,7 @@ async fn e2e_circuit_breaker_deterministic_self_loop() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-circuit-breaker"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11663,7 +11710,7 @@ async fn e2e_circuit_breaker_custom_limit() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-custom-limit"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11703,7 +11750,7 @@ async fn e2e_circuit_breaker_ignores_transient_failures() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-transient-no-breaker"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11750,7 +11797,7 @@ async fn e2e_circuit_breaker_different_reasons_separate_counters() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-varying-reasons"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11790,7 +11837,7 @@ async fn e2e_circuit_breaker_loop_restart() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-breaker"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11853,7 +11900,7 @@ async fn e2e_failure_signature_persisted_in_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-sig-context"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11917,7 +11964,7 @@ async fn e2e_failure_signature_hint_overrides_reason_in_context() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-sig-hint"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -11975,7 +12022,7 @@ async fn e2e_signature_maps_persist_in_checkpoint() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-sig-persist"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12103,7 +12150,7 @@ async fn e2e_circuit_breaker_emits_events_before_abort() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-events"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12170,7 +12217,7 @@ async fn e2e_circuit_breaker_does_not_fire_below_limit() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-below-limit"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12266,7 +12313,7 @@ async fn e2e_circuit_breaker_multi_stage_impl_verify_cycle() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-impl-verify-cycle"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12364,7 +12411,7 @@ async fn e2e_loop_restart_blocked_for_deterministic_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-blocked-det"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12404,7 +12451,7 @@ async fn e2e_loop_restart_blocked_for_structural_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-blocked-struct"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12444,7 +12491,7 @@ async fn e2e_loop_restart_blocked_for_budget_exhausted_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-blocked-budget"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12484,7 +12531,7 @@ async fn e2e_loop_restart_blocked_for_canceled_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-blocked-canceled"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12521,7 +12568,7 @@ async fn e2e_loop_restart_blocked_for_compilation_loop_failure() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-blocked-comploop"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12562,7 +12609,7 @@ async fn e2e_loop_restart_allowed_for_transient_infra() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("e2e-restart-allowed-transient"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12670,7 +12717,7 @@ async fn e2e_stall_watchdog_triggers_from_dot_parsed_pipeline() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("stall-e2e"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12726,7 +12773,7 @@ async fn e2e_stall_watchdog_kept_alive_by_handler_events() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("stall-alive-e2e"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12772,7 +12819,7 @@ async fn e2e_stall_watchdog_disabled_with_zero_timeout() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("stall-disabled-e2e"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12838,7 +12885,7 @@ async fn e2e_stall_watchdog_with_explicit_timeout_override() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("stall-override-e2e"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -12980,7 +13027,7 @@ async fn asset_collection_local_sandbox_success() { ..WorkflowSettings::default() }, run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("artifact-test-local"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -13116,7 +13163,7 @@ async fn asset_collection_local_sandbox_on_failure() { ..WorkflowSettings::default() }, run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("artifact-test-fail"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -13226,7 +13273,7 @@ async fn asset_collection_docker_sandbox() { ..WorkflowSettings::default() }, run_dir: run_dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("artifact-test-docker"), labels: std::collections::HashMap::new(), workflow_slug: None, @@ -13298,7 +13345,7 @@ async fn wait_timer_e2e() { let run_options = RunOptions { settings: WorkflowSettings::default(), run_dir: dir.path().to_path_buf(), - cancel_token: None, + cancel_token: CancellationToken::new(), run_id: test_run_id("test-run"), labels: std::collections::HashMap::new(), workflow_slug: None,