From 12fab9f7cb500f69fd591911412cc9017b771d6c Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Wed, 23 Sep 2026 09:19:42 -0700 Subject: [PATCH] fix(shell): a busy Work graph no longer kills the command it skipped Refs #6435. When the To-do/Plan lock was busy, ShellSpawnIntentGuard logged "registration skipped; running without a bound operation" and dropped its copy of the lifecycle, but the spawn path kept its own clone, published the new shell to the never-registered operation, treated that failure as fatal, killed the child and reported "Shell execution failed: operation binding shell: is not registered". The reporter's logs show 563 such misses. The lifecycle used after spawn now comes from the guard, so an unbound spawn stays unbound, in both the background and interactive paths. The spawn-time publication is best-effort like every other lifecycle observation: a graph write that fails after the child started must not kill it. The try-lock spin that triggers the skip is unchanged; it now only costs Work-graph bookkeeping for that command, never the command. Evidence: new busy_work_graph_still_runs_the_command holds the To-do lock and runs a foreground and a background command with a lifecycle; against the previous code it fails (0 passed, 1 failed: the background child is killed on the missing binding), with the fix 154 passed, 0 failed across the busy_work_graph and shell:: selections. TUI all-target/all-feature Clippy with CI flags and fmt passed; blocking call budget unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/tui/src/tools/shell.rs | 17 +++++---- crates/tui/src/tools/shell/tests.rs | 57 +++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index ee3e134d50..4705361e87 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -2558,7 +2558,10 @@ impl ShellManager { } = spawn_context; let task_id = format!("shell_{}", &Uuid::new_v4().to_string()[..8]); let mut spawn_guard = - ShellSpawnIntentGuard::new(work_lifecycle.clone(), &task_id, original_command); + ShellSpawnIntentGuard::new(work_lifecycle, &task_id, original_command); + // The guard owns the registration outcome: when bookkeeping could not + // bind this operation, nothing below may publish to it (#6435). + let work_lifecycle = spawn_guard.lifecycle.clone(); let started = Instant::now(); let sandbox_type = exec_env.sandbox_type; let sandboxed = exec_env.is_sandboxed(); @@ -2801,10 +2804,9 @@ impl ShellManager { return Err(err); } - if let Err(err) = bg_shell.publish_lifecycle() { - let _ = bg_shell.kill(); - return Err(err); - } + // Work-graph publication is bookkeeping: a graph write that fails + // now must not kill a command that already started (#6435). + bg_shell.publish_lifecycle_best_effort(); self.processes.insert(task_id.clone(), bg_shell); spawn_guard.disarm(); @@ -5623,8 +5625,9 @@ impl ToolSpec for BashTool { .map_err(|_| ToolError::execution_failed("shell manager lock poisoned"))?; let work_lifecycle = shell_work_lifecycle_from_context(context); let task_id = format!("shell_{}", &Uuid::new_v4().to_string()[..8]); - let mut spawn_guard = - ShellSpawnIntentGuard::new(work_lifecycle.clone(), &task_id, command); + let mut spawn_guard = ShellSpawnIntentGuard::new(work_lifecycle, &task_id, command); + // Only a registered operation is observed (#6435). + let work_lifecycle = spawn_guard.lifecycle.clone(); let result = manager.execute_interactive_with_policy_env( command, working_dir.as_deref(), diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index bfbc6f1e8c..c4f2899937 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -4708,6 +4708,63 @@ async fn busy_work_graph_degrades_the_spawn_intent_instead_of_failing_it() { ); } +/// #6435: the guard went unbound, but its sibling handle still published the +/// spawn to the unregistered operation, killed the child and reported +/// "operation binding shell: is not registered". A busy Work-graph must +/// let the command run end to end. +#[cfg(unix)] +#[tokio::test] +async fn busy_work_graph_still_runs_the_command() { + use crate::tools::plan::new_shared_plan_state; + use crate::tools::todo::new_shared_todo_list; + use crate::work_graph::new_shared_work_runtime; + + let workspace = tempdir().expect("workspace"); + let todos = new_shared_todo_list(); + let plan = new_shared_plan_state(); + let lifecycle = ShellWorkLifecycle { + work: new_shared_work_runtime(todos.clone(), plan.clone()), + session_id: "session-test".to_string(), + }; + let _held = todos.lock().await; + let mut manager = ShellManager::new(workspace.path().to_path_buf()); + for background in [false, true] { + let marker = format!("ran-{background}.txt"); + let result = manager + .execute_with_options_env_for_owner_and_work( + &format!("echo ran > {marker}"), + None, + 10_000, + background, + None, + false, + None, + HashMap::new(), + None, + "session-test".to_string(), + None, + None, + Some(lifecycle.clone()), + None, + false, + (1_000, 60_000), + ) + .unwrap_or_else(|err| panic!("background={background}: {err:#}")); + if background { + let task_id = result.task_id.expect("background task id"); + let deadline = std::time::Instant::now() + Duration::from_secs(10); + while !workspace.path().join(&marker).exists() { + assert!(std::time::Instant::now() < deadline, "{task_id} never ran"); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + assert!( + workspace.path().join(&marker).exists(), + "background={background}: the command ran" + ); + } +} + #[test] fn pty_dimensions_reject_zero_and_unbounded_grid() { assert_eq!(