From 394445e89df1894b4146207bcb201069c4e2e706 Mon Sep 17 00:00:00 2001 From: pickx Date: Sat, 17 Oct 2026 17:59:06 +0300 Subject: [PATCH] feat(`job spawn`) `auto-kill` flag (#19141) ## Description closes #18896 when calling `exit`, if the flag is passed, we suppress the warning for quitting that job. if all remaining jobs have this flag enabled, we don't emit a warning. I chose the name `auto-kill` because we already use the terminology "kill" for jobs ## User-facing changes (Release notes) Allow using the flag `-k`/`--auto-kill` with `job spawn` for jobs that are expected to stay alive. This suppresses warnings about that job still running on `exit`. `job list` now shows which jobs have this `auto-kill` flag set, and a hint has been added to `exit`. --- Cargo.lock | 1 + .../nu-command/src/experimental/job_list.rs | 1 + .../nu-command/src/experimental/job_spawn.rs | 29 ++++-- crates/nu-command/tests/commands/job.rs | 26 ++++++ crates/nu-engine/Cargo.toml | 1 + crates/nu-engine/src/exit.rs | 92 ++++++++++++++++--- crates/nu-mcp/src/evaluation.rs | 3 +- crates/nu-protocol/src/engine/engine_state.rs | 8 +- crates/nu-protocol/src/engine/jobs.rs | 22 ++++- .../nu-protocol/src/errors/shell_warning.rs | 11 +++ src/signals.rs | 2 +- 11 files changed, 166 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 57250b8afe..cc605485e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5115,6 +5115,7 @@ dependencies = [ "nu-protocol", "nu-test-support", "nu-utils", + "rstest", ] [[package]] diff --git a/crates/nu-command/src/experimental/job_list.rs b/crates/nu-command/src/experimental/job_list.rs index 5aaf292ea7..fcfd93d730 100644 --- a/crates/nu-command/src/experimental/job_list.rs +++ b/crates/nu-command/src/experimental/job_list.rs @@ -56,6 +56,7 @@ impl Command for JobList { Value::list(vec![ Value::int(unfreeze.pid() as i64, head) ], head) } }, + "auto_kill" => Value::bool(job.is_auto_kill(), head), }; if let Some(description) = job.description() { diff --git a/crates/nu-command/src/experimental/job_spawn.rs b/crates/nu-command/src/experimental/job_spawn.rs index aecd7ae283..f3fb84be92 100644 --- a/crates/nu-command/src/experimental/job_spawn.rs +++ b/crates/nu-command/src/experimental/job_spawn.rs @@ -36,6 +36,11 @@ impl Command for JobSpawn { "An optional description for this job.", Some('d'), ) + .switch( + "auto-kill", + "Kill this job on `exit` without warning that it is still running.", + Some('k'), + ) .required( "closure", SyntaxShape::Closure(Some(vec![SyntaxShape::Any])), @@ -64,6 +69,7 @@ impl Command for JobSpawn { engine_state.get_closure_block(&closure, head)?; let description: Option = call.get_flag(engine_state, stack, "description")?; + let auto_kill = call.has_flag(engine_state, stack, "auto-kill")?; let job_stack = stack.clone(); let mut job_state = engine_state.clone(); @@ -86,7 +92,7 @@ impl Command for JobSpawn { let (send, recv) = mpsc::channel(); let id = { - let thread_job = ThreadJob::new(job_signals, description, send); + let thread_job = ThreadJob::new(job_signals, description, auto_kill, send); let id = jobs.add_job(Job::Thread(thread_job.clone())); @@ -143,11 +149,18 @@ impl Command for JobSpawn { } fn examples(&self) -> Vec> { - vec![Example { - example: "job spawn { sleep 5sec; rm evidence.pdf }", - description: "Spawn a background job to do some time consuming work.", - result: None, - }] + vec![ + Example { + example: "job spawn { sleep 5sec; rm evidence.pdf }", + description: "Spawn a background job to do some time consuming work.", + result: None, + }, + Example { + example: "job spawn --auto-kill { loop { sleep 1sec } }", + description: "Spawn a job that `exit` kills without warning about it.", + result: None, + }, + ] } fn extra_description(&self) -> &str { @@ -155,6 +168,10 @@ impl Command for JobSpawn { and registers this task in the background job table, which can be retrieved with `job list`. This command returns the job id of the newly created job. + +The --auto-kill flag applies to the spawned thread job. If an external process +inside it is suspended and registered as a separate frozen job, it would still +trigger the exit warning. " } } diff --git a/crates/nu-command/tests/commands/job.rs b/crates/nu-command/tests/commands/job.rs index 0c94d64131..11bc068a35 100644 --- a/crates/nu-command/tests/commands/job.rs +++ b/crates/nu-command/tests/commands/job.rs @@ -5,6 +5,32 @@ use nu_test_support::prelude::*; use rstest::rstest; use std::time::Duration; +#[rstest] +#[case(None)] +#[case(Some("-k"))] +#[case(Some("--auto-kill"))] +#[nu_test_support::test] +#[cfg_attr(ci, serial)] +fn job_list_reports_auto_kill(#[case] auto_kill_flag: Option<&str>) -> Result { + let is_auto_kill = auto_kill_flag.is_some(); + + let spawn_command = match auto_kill_flag { + Some(auto_kill_flag) => format!("job spawn {auto_kill_flag} {{ job recv }}"), + None => "job spawn { job recv }".to_owned(), + }; + + let code = format!( + " + let id = {spawn_command} + let auto_kill = job list | where id == $id | get auto_kill.0 + job kill $id + $auto_kill + " + ); + + test().run(&code).expect_value_eq(is_auto_kill) +} + #[test] fn job_spawn_runs_with_empty_pipeline_not_null_value() -> Result { let code = " diff --git a/crates/nu-engine/Cargo.toml b/crates/nu-engine/Cargo.toml index 74ba10c85c..6acdd2cf68 100644 --- a/crates/nu-engine/Cargo.toml +++ b/crates/nu-engine/Cargo.toml @@ -39,3 +39,4 @@ plugin = [ [dev-dependencies] nu-test-support = { path = "../nu-test-support" } +rstest.workspace = true diff --git a/crates/nu-engine/src/exit.rs b/crates/nu-engine/src/exit.rs index eac689aaad..fa85dfb19b 100644 --- a/crates/nu-engine/src/exit.rs +++ b/crates/nu-engine/src/exit.rs @@ -1,12 +1,12 @@ -use std::sync::{Mutex, PoisonError, atomic::Ordering}; - use nu_protocol::engine::{EngineState, Jobs}; +use nu_protocol::{ShellWarning, report_shell_warning}; +use std::sync::{Mutex, PoisonError, atomic::Ordering}; /// Exit the process or clean jobs if appropriate. /// -/// Drops `tag` and exits the current process if there are no running jobs, or if `exit_warning_given` is true. -/// When running in a REPL session, warns the user if there -/// were jobs and sets `exit_warning_given` instead, returning `tag` itself in that case. +/// In a REPL session, warns if any jobs are not `auto_kill` and +/// `exit_warning_given` is false. Sets that flag and returns `tag` in that case. +/// Otherwise, kills all jobs, drops `tag`, and exits the current process. /// // Currently, this `tag` argument exists mostly so that a LineEditor can be dropped before exiting the process. pub fn cleanup_exit(tag: T, engine_state: &EngineState, exit_code: i32) -> T { @@ -19,24 +19,26 @@ pub fn cleanup_exit(tag: T, engine_state: &EngineState, exit_code: i32) -> T /// Clean up jobs before exiting, unless the REPL needs to warn about running jobs. /// -/// On the first REPL exit with running jobs, sets `exit_warning_given` and returns -/// `Some(tag)` so the caller can return to the prompt. Otherwise, kills the jobs, drops -/// `tag`, and returns `None` so the caller can exit. +/// In a REPL session, warns if any jobs are not `auto_kill` and +/// `exit_warning_given` is false. Sets that flag and returns `Some(tag)` in that case. +/// Otherwise, kills all jobs, drops `tag`, and returns `None`. pub fn cleanup(tag: T, engine_state: &EngineState) -> Option { let jobs = engine_state .jobs .lock() .unwrap_or_else(PoisonError::into_inner); + let job_count = jobs.iter().filter(|(_, job)| !job.is_auto_kill()).count(); + if engine_state.is_repl - && jobs.iter().next().is_some() + && job_count > 0 && !engine_state.exit_warning_given.load(Ordering::SeqCst) { - let job_count = jobs.iter().count(); - - println!("There are still background jobs running ({job_count})."); - - println!("Running `exit` a second time will kill all of them."); + report_shell_warning( + None, + engine_state, + &ShellWarning::BackgroundJobsRunning { job_count }, + ); engine_state .exit_warning_given @@ -71,3 +73,65 @@ pub fn kill_all_jobs(jobs: &Mutex) { let mut jobs = jobs.lock().unwrap_or_else(PoisonError::into_inner); let _ = jobs.kill_all(); } + +#[cfg(test)] +mod tests { + use super::*; + use nu_protocol::Signals; + use nu_protocol::engine::Job; + use nu_protocol::engine::ThreadJob; + use rstest::rstest; + use std::ops::Not; + use std::sync::mpsc; + + #[rstest] + #[case(&[true])] + #[case(&[false])] + #[case(&[true, false])] + #[case(&[false, false, true])] + fn interactive_cleanup_respects_auto_kill(#[case] auto_kill_flags: &[bool]) { + let mut engine_state = EngineState::new(); + engine_state.is_repl = true; + + let mut signals_handles = vec![]; + + for auto_kill in auto_kill_flags { + let signals_handle = Signals::new(Default::default()); + signals_handles.push(signals_handle.clone()); + + let (send, _) = mpsc::channel(); + + let job = ThreadJob::new(signals_handle, None, *auto_kill, send); + let job = Job::Thread(job); + engine_state.jobs.lock().unwrap().add_job(job); + } + + let should_warn = auto_kill_flags.contains(&false); + + let exited = cleanup((), &engine_state).is_none(); + assert_eq!(exited, !should_warn); + + let warned = engine_state.exit_warning_given.load(Ordering::SeqCst); + assert_eq!(warned, should_warn); + + if should_warn { + assert_eq!( + engine_state.jobs.lock().unwrap().iter().count(), + signals_handles.len() + ); + + assert!( + signals_handles + .iter() + .all(|signals| signals.interrupted().not()) + ); + + let exited = cleanup((), &engine_state).is_none(); + assert!(exited); + } + + assert!(engine_state.jobs.lock().unwrap().is_empty()); + + assert!(signals_handles.iter().all(Signals::interrupted)); + } +} diff --git a/crates/nu-mcp/src/evaluation.rs b/crates/nu-mcp/src/evaluation.rs index 08f53b19fc..cbb0ee5df2 100644 --- a/crates/nu-mcp/src/evaluation.rs +++ b/crates/nu-mcp/src/evaluation.rs @@ -521,7 +521,8 @@ fn promote_to_background_job( ) -> Result { let signals = Signals::new(interrupt); let (sender, _receiver) = mpsc::channel(); - let thread_job = ThreadJob::new(signals, Some(description), sender); + let auto_kill = false; + let thread_job = ThreadJob::new(signals, Some(description), auto_kill, sender); let job_id = match jobs.lock() { Ok(mut jobs) => jobs.add_job(Job::Thread(thread_job)), diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 22bdda8378..92fd814ca9 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -230,10 +230,10 @@ pub struct EngineState { pub root_job_sender: Sender, - // When there are background jobs running, the interactive behavior of `exit` changes depending on - // the value of this flag: - // - if this is false, then a warning about running jobs is shown and `exit` enables this flag - // - if this is true, then `exit` will `std::process::exit` + // When there are background jobs that are not auto_kill, interactive `exit` checks this flag: + // - if false, it warns about those jobs and sets this flag + // - if true, it kills all jobs and exits + // If all jobs have auto_kill, exit kills them and exits without warning. // // This ensures that running exit twice will terminate the program correctly pub exit_warning_given: Arc, diff --git a/crates/nu-protocol/src/engine/jobs.rs b/crates/nu-protocol/src/engine/jobs.rs index 00a8bba32a..6e0f2f9504 100644 --- a/crates/nu-protocol/src/engine/jobs.rs +++ b/crates/nu-protocol/src/engine/jobs.rs @@ -160,16 +160,23 @@ pub struct ThreadJob { signals: Signals, pids: Arc>>, description: Option, + auto_kill: bool, pub sender: Sender, } impl ThreadJob { - pub fn new(signals: Signals, description: Option, sender: Sender) -> Self { + pub fn new( + signals: Signals, + description: Option, + auto_kill: bool, + sender: Sender, + ) -> Self { ThreadJob { signals, pids: Arc::new(Mutex::new(HashSet::default())), sender, description, + auto_kill, } } @@ -253,6 +260,13 @@ impl Job { } } + pub fn is_auto_kill(&self) -> bool { + match self { + Job::Thread(job) => job.auto_kill, + Job::Frozen(_) => false, + } + } + pub fn assign_description(&mut self, description: Option) { match self { Job::Thread(thread_job) => thread_job.description = description, @@ -452,7 +466,7 @@ mod tests { fn interrupted_job_does_not_spawn() { let (sender, _) = mpsc::channel(); let signals = Signals::new(Arc::new(AtomicBool::new(false))); - let job = ThreadJob::new(signals.clone(), None, sender); + let job = ThreadJob::new(signals.clone(), None, false, sender); signals.trigger(); let child = job.try_spawn(|| panic!("interrupted job must not spawn")); @@ -463,7 +477,7 @@ mod tests { fn spawned_process_is_registered_before_returning() { let (sender, _) = mpsc::channel(); let signals = Signals::new(Arc::new(AtomicBool::new(false))); - let job = ThreadJob::new(signals, None, sender); + let job = ThreadJob::new(signals, None, false, sender); // Listing this test binary's tests needs no platform-specific executable, and the // child terminates by itself if an assertion fails. let mut command = std::process::Command::new(std::env::current_exe().unwrap()); @@ -503,7 +517,7 @@ mod tests { fn spawn_holds_pid_lock_until_registration() { let (sender, _) = mpsc::channel(); let signals = Signals::new(Arc::new(AtomicBool::new(false))); - let job = ThreadJob::new(signals, None, sender); + let job = ThreadJob::new(signals, None, false, sender); let (started_tx, started_rx) = mpsc::channel(); let (release_tx, release_rx) = mpsc::channel(); diff --git a/crates/nu-protocol/src/errors/shell_warning.rs b/crates/nu-protocol/src/errors/shell_warning.rs index 85cce9027d..8d400b7c6a 100644 --- a/crates/nu-protocol/src/errors/shell_warning.rs +++ b/crates/nu-protocol/src/errors/shell_warning.rs @@ -9,6 +9,15 @@ use crate::{ConfigWarning, ReportMode, Reportable}; #[derive(Clone, Debug, Error, Diagnostic)] #[diagnostic(severity(Warning))] pub enum ShellWarning { + /// Running jobs (without auto-kill) prevented interactive shell from exiting. + #[error("Running background jobs that won't be killed automatically: {job_count}.")] + #[diagnostic( + code(nu::shell::background_jobs_running), + help( + "Run `exit` again to kill all jobs.\nUse `job spawn --auto-kill` when spawning thread jobs to exclude them from this warning." + ) + )] + BackgroundJobsRunning { job_count: usize }, /// A parse-time deprecation. Indicates that something will be removed in a future release. /// /// Use [`ParseWarning::Deprecated`](crate::ParseWarning::Deprecated) if this is a deprecation @@ -55,6 +64,7 @@ impl Reportable for ShellWarning { ShellWarning::Deprecated { report_mode, .. } | ShellWarning::LastResultTruncated { report_mode, .. } => *report_mode, ShellWarning::InvalidConfig { .. } => ReportMode::FirstUse, + ShellWarning::BackgroundJobsRunning { .. } => ReportMode::EveryUse, } } } @@ -63,6 +73,7 @@ impl Reportable for ShellWarning { impl Hash for ShellWarning { fn hash(&self, state: &mut H) { match self { + ShellWarning::BackgroundJobsRunning { job_count } => job_count.hash(state), ShellWarning::Deprecated { dep_type, label, .. } => { diff --git a/src/signals.rs b/src/signals.rs index c94c195b46..b1977c43a2 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -55,7 +55,7 @@ mod tests { let jobs = Arc::new(Mutex::new(Jobs::default())); let job_signals = Signals::new(Arc::new(AtomicBool::new(false))); let (sender, _receiver) = mpsc::channel(); - let job = ThreadJob::new(job_signals.clone(), None, sender); + let job = ThreadJob::new(job_signals.clone(), None, false, sender); let id = jobs.lock().unwrap().add_job(Job::Thread(job)); (jobs, id, job_signals)