Panic when converting other I/O errors into our I/O errors (#16160)

# Description
I added a debug-only check that makes sure only non-other I/O errors get
converted.
Since tests run in debug mode, that should help us catch these errors early.

I left the check out in release mode on purpose so we don't crash users who
have nu as their main shell. It's similar to the debug assertions in the same
file that check for unknown spans.

crates/nu-protocol/src/engine/jobs.rs

@@ -11,7 +11,7 @@ use std::time::{Duration, Instant};
 
 use nu_system::{UnfreezeHandle, kill_by_pid};
 
-use crate::{PipelineData, Signals};
+use crate::{PipelineData, Signals, shell_error};
 
 use crate::JobId;
 
@@ -92,7 +92,7 @@ impl Jobs {
     /// This function tries to forcefully kill a job from this job table,
     /// removes it from the job table. It always succeeds in removing the job
     /// from the table, but may fail in killing the job's active processes.
-    pub fn kill_and_remove(&mut self, id: JobId) -> std::io::Result<()> {
+    pub fn kill_and_remove(&mut self, id: JobId) -> shell_error::io::Result<()> {
         if let Some(job) = self.jobs.get(&id) {
             let err = job.kill();
 
@@ -109,7 +109,7 @@ impl Jobs {
     ///
     /// It returns an error if any of the job killing attempts fails, but always
     /// succeeds in removing the jobs from the table.
-    pub fn kill_all(&mut self) -> std::io::Result<()> {
+    pub fn kill_all(&mut self) -> shell_error::io::Result<()> {
         self.last_frozen_job_id = None;
 
         let first_err = self
@@ -180,7 +180,7 @@ impl ThreadJob {
         lock.iter().copied().collect()
     }
 
-    pub fn kill(&self) -> std::io::Result<()> {
+    pub fn kill(&self) -> shell_error::io::Result<()> {
         // it's okay to make this interrupt outside of the mutex, since it has acquire-release
         // semantics.
 
@@ -205,7 +205,7 @@ impl ThreadJob {
 }
 
 impl Job {
-    pub fn kill(&self) -> std::io::Result<()> {
+    pub fn kill(&self) -> shell_error::io::Result<()> {
         match self {
             Job::Thread(thread_job) => thread_job.kill(),
             Job::Frozen(frozen_job) => frozen_job.kill(),
@@ -233,10 +233,10 @@ pub struct FrozenJob {
 }
 
 impl FrozenJob {
-    pub fn kill(&self) -> std::io::Result<()> {
+    pub fn kill(&self) -> shell_error::io::Result<()> {
         #[cfg(unix)]
         {
-            kill_by_pid(self.unfreeze.pid() as i64)
+            Ok(kill_by_pid(self.unfreeze.pid() as i64)?)
         }
 
         // it doesn't happen outside unix.

crates/nu-protocol/src/errors/shell_error/io.rs

@@ -10,6 +10,16 @@ use crate::Span;
 
 use super::location::Location;
 
+/// Alias for a `Result` with the error type [`ErrorKind`] by default.
+///
+/// This may be used in all situations that would usually return an [`std::io::Error`] but are
+/// already part of the [`nu_protocol`](crate) crate and can therefore interact with
+/// [`shell_error::io`](self) directly.
+///
+/// To make programming inside this module easier, you can pass the `E` type with another error.
+/// This avoids the annoyance of having a shadowed `Result`.
+pub type Result<T, E = ErrorKind> = std::result::Result<T, E>;
+
 /// Represents an I/O error in the [`ShellError::Io`] variant.
 ///
 /// This is the central I/O error for the [`ShellError::Io`] variant.
@@ -158,6 +168,14 @@ pub enum ErrorKind {
     #[allow(private_interfaces)]
     Std(std::io::ErrorKind, Sealed),
 
+    /// Killing a job process failed.
+    ///
+    /// This error is part [`ShellError::Io`](super::ShellError::Io) instead of
+    /// [`ShellError::Job`](super::ShellError::Job) as this error occurs because some I/O operation
+    /// failed on the OS side.
+    /// And not part of our logic.
+    KillJobProcess,
+
     NotAFile,
 
     /// The file or directory is in use by another program.
@@ -375,10 +393,34 @@ impl From<&std::io::Error> for ErrorKind {
             }
         }
 
+        #[cfg(debug_assertions)]
+        if err.kind() == std::io::ErrorKind::Other {
+            panic!(
+                "\
+suspicious conversion:
+    tried to convert `std::io::Error` with `std::io::ErrorKind::Other`
+    into `nu_protocol::shell_error::io::ErrorKind`
+
+I/O errors should always be specific, provide more context
+
+{err:#?}\
+            "
+            )
+        }
+
         ErrorKind::Std(err.kind(), Sealed)
     }
 }
 
+impl From<nu_system::KillByPidError> for ErrorKind {
+    fn from(value: nu_system::KillByPidError) -> Self {
+        match value {
+            nu_system::KillByPidError::Output(error) => error.into(),
+            nu_system::KillByPidError::KillProcess => ErrorKind::KillJobProcess,
+        }
+    }
+}
+
 impl StdError for IoError {}
 impl Display for IoError {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
@@ -400,6 +442,7 @@ impl Display for ErrorKind {
                 let (first, rest) = msg.split_at(1);
                 write!(f, "{}{}", first.to_uppercase(), rest)
             }
+            ErrorKind::KillJobProcess => write!(f, "Killing job process failed"),
             ErrorKind::NotAFile => write!(f, "Not a file"),
             ErrorKind::AlreadyInUse => write!(f, "Already in use"),
             ErrorKind::FileNotFound => write!(f, "File not found"),
@@ -437,6 +480,7 @@ impl Diagnostic for IoError {
                 std::io::ErrorKind::Other => code.push_str("other"),
                 kind => code.push_str(&kind.to_string().to_lowercase().replace(" ", "_")),
             },
+            ErrorKind::KillJobProcess => code.push_str("kill_job_process"),
             ErrorKind::NotAFile => code.push_str("not_a_file"),
             ErrorKind::AlreadyInUse => code.push_str("already_in_use"),
             ErrorKind::FileNotFound => code.push_str("file_not_found"),

crates/nu-system/src/util.rs

@@ -2,16 +2,24 @@ use std::io;
 use std::process::Command as CommandSys;
 
 /// Tries to forcefully kill a process by its PID
-pub fn kill_by_pid(pid: i64) -> io::Result<()> {
+pub fn kill_by_pid(pid: i64) -> Result<(), KillByPidError> {
     let mut cmd = build_kill_command(true, std::iter::once(pid), None);
 
-    let output = cmd.output()?;
+    let output = cmd.output().map_err(KillByPidError::Output)?;
 
-    if !output.status.success() {
+    match output.status.success() {
-        return Err(io::Error::other("failed to kill process"));
+        true => Ok(()),
+        false => Err(KillByPidError::KillProcess),
     }
+}
 
-    Ok(())
+/// Error while killing a process forcefully by its PID.
+pub enum KillByPidError {
+    /// I/O error while capturing the output of the process.
+    Output(io::Error),
+
+    /// Killing the process failed.
+    KillProcess,
 }
 
 /// Create a `std::process::Command` for the current target platform, for killing