refactor(openfiles): address review feedback on fd sharing

Make OpenFile->Stdio conversion fallible (TryFrom) so descriptor
duplication failures surface instead of silently using /dev/null;
remove the now-infallible try_clone and the TryCloneToStdio helper;
gate the fd_management compat tests to non-wasi; tidy comments.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: reubeno

brush-core/src/commands.rs

@@ -221,7 +221,7 @@ pub fn compose_std_command<S: AsRef<OsStr>, SE: extensions::ShellExtensions>(
     match context.try_fd(OpenFiles::STDIN_FD) {
         Some(OpenFile::Stdin(_)) | None => (),
         Some(stdin_file) => {
-            let as_stdio: Stdio = stdin_file.into();
+            let as_stdio: Stdio = stdin_file.try_into()?;
             cmd.stdin(as_stdio);
         }
     }
@@ -230,7 +230,7 @@ pub fn compose_std_command<S: AsRef<OsStr>, SE: extensions::ShellExtensions>(
     match context.try_fd(OpenFiles::STDOUT_FD) {
         Some(OpenFile::Stdout(_)) | None => (),
         Some(stdout_file) => {
-            let as_stdio: Stdio = stdout_file.into();
+            let as_stdio: Stdio = stdout_file.try_into()?;
             cmd.stdout(as_stdio);
         }
     }
@@ -239,7 +239,7 @@ pub fn compose_std_command<S: AsRef<OsStr>, SE: extensions::ShellExtensions>(
     match context.try_fd(OpenFiles::STDERR_FD) {
         Some(OpenFile::Stderr(_)) | None => {}
         Some(stderr_file) => {
-            let as_stdio: Stdio = stderr_file.into();
+            let as_stdio: Stdio = stderr_file.try_into()?;
             cmd.stderr(as_stdio);
         }
     }
@@ -723,12 +723,9 @@ pub(crate) async fn invoke_shell_function(
         &context.params,
     )?;
 
-    // Invoke the function. We pass the context's open files through directly rather than
-    // cloning them; cloning would duplicate every open file descriptor (e.g. via dup()) for
-    // the duration of the function body. With deeply nested function calls -- as in large
-    // scripts like nvm -- those duplicated descriptors accumulate and can exhaust the
-    // process-wide file descriptor limit. The function body shares the same descriptors as
-    // its caller, matching how a function executes in the current shell process.
+    // A function executes within the current shell process and shares its caller's open files,
+    // so the parameters are passed through by shared reference rather than cloned. The shared
+    // borrow also guarantees the body cannot disturb the caller's open-file table.
     let result = body.execute(context.shell, &context.params).await;
 
     // We've come back out, reflect it.

brush-core/src/interp.rs

@@ -1737,7 +1737,7 @@ pub(crate) async fn setup_redirect(
                     let fd_num = specified_fd_num.unwrap_or(default_fd_if_unspecified);
 
                     if let Some(f) = params.try_fd(shell, *fd) {
-                        let target_file = f.try_clone()?;
+                        let target_file = f.clone();
 
                         params.open_files.set_fd(fd_num, target_file);
                     } else {
@@ -1781,7 +1781,7 @@ pub(crate) async fn setup_redirect(
 
                         // Duplicate the fd.
                         let target_file = if let Some(f) = params.try_fd(shell, source_fd_num) {
-                            f.try_clone()?
+                            f.clone()
                         } else {
                             return Err(error::ErrorKind::BadFileDescriptor(source_fd_num).into());
                         };
@@ -1817,7 +1817,7 @@ pub(crate) async fn setup_redirect(
                                 subshell_cmd,
                             )?;
 
-                            let target_file = substitution_file.try_clone()?;
+                            let target_file = substitution_file.clone();
                             params.open_files.set_fd(substitution_fd, substitution_file);
 
                             let fd_num = specified_fd_num
@@ -1895,7 +1895,7 @@ fn setup_redirect_output_and_error_to(
             )
         })?;
 
-    let stderr_file = stdout_file.try_clone()?;
+    let stderr_file = stdout_file.clone();
 
     params.open_files.set_fd(OpenFiles::STDOUT_FD, stdout_file);
     params.open_files.set_fd(OpenFiles::STDERR_FD, stderr_file);

brush-core/src/openfiles.rs

@@ -31,14 +31,14 @@ pub trait Stream: std::io::Read + std::io::Write + Send + Sync {
 
 /// Represents a file open in a shell context.
 ///
-/// File-backed and pipe-backed variants store their handles behind an [`Arc`] so that cloning
-/// an `OpenFile` (which happens whenever the shell forks off a subshell, command substitution,
-/// background job, or function call context) merely bumps a reference count instead of issuing
-/// a `dup(2)` syscall. Because brush runs subshells as in-process tasks (rather than via
-/// `fork(2)` like a traditional shell), every duplicated descriptor consumes a slot in the
-/// single process-wide descriptor table; sharing avoids exhausting that table during deeply
-/// nested or highly concurrent execution. A real duplicate is only materialized when a
-/// descriptor must be handed to an external child process (see [`OpenFile::try_clone_to_owned`]).
+/// File- and pipe-backed variants hold their handle behind an [`Arc`], so cloning an `OpenFile`
+/// shares the underlying descriptor by reference count. The shell opens a fresh context for each
+/// subshell, command substitution, background job, and function call and runs them as in-process
+/// tasks against one process-wide descriptor table (rather than via `fork(2)` like a traditional
+/// shell); sharing the descriptor keeps deeply nested or highly concurrent execution from
+/// exhausting that table. A descriptor is duplicated for real only when an independently owned
+/// copy is needed to hand to an external child process — see [`OpenFile::try_clone_to_owned`] and
+/// the `Stdio` conversion below.
 pub enum OpenFile {
     /// The original standard input this process was started with.
     Stdin(std::io::Stdin),
@@ -133,17 +133,6 @@ impl std::fmt::Display for OpenFile {
 }
 
 impl OpenFile {
-    /// Tries to duplicate the open file.
-    ///
-    /// For file- and pipe-backed open files this shares the underlying handle by reference
-    /// count rather than issuing a `dup(2)`; the resulting `OpenFile` refers to the very same
-    /// kernel descriptor (matching the shared open-file-description semantics that `dup` would
-    /// have provided). Use [`OpenFile::try_clone_to_owned`] when an independent descriptor is
-    /// genuinely required (e.g. to hand to a child process).
-    pub fn try_clone(&self) -> Result<Self, std::io::Error> {
-        Ok(self.clone())
-    }
-
     /// Converts the open file into an `OwnedFd`. For shared file/pipe handles this materializes
     /// a real duplicate via `dup(2)` so the caller receives an independently owned descriptor.
     #[cfg(unix)]
@@ -240,54 +229,25 @@ impl From<std::io::PipeWriter> for OpenFile {
     }
 }
 
-impl From<OpenFile> for Stdio {
-    fn from(open_file: OpenFile) -> Self {
-        // File/pipe handles are shared (behind an `Arc`), so we cannot move the underlying
-        // descriptor out; instead we duplicate it to obtain an owned descriptor for the child.
-        // This is the one place a real `dup` is expected: when wiring up an external process's
-        // standard streams. If the duplication fails (e.g. descriptor exhaustion), fall back to
-        // a null device rather than panicking.
-        fn dup_to_stdio<T: TryCloneToStdio>(handle: &T) -> Stdio {
-            handle.try_clone_to_stdio().unwrap_or_else(|_| Stdio::null())
-        }
+impl TryFrom<OpenFile> for Stdio {
+    type Error = error::Error;
 
+    fn try_from(open_file: OpenFile) -> Result<Self, Self::Error> {
+        // File and pipe handles are shared behind an `Arc`, so the descriptor cannot be moved
+        // out; duplicate it to give the child an independently owned descriptor. Duplication can
+        // fail (e.g. under descriptor exhaustion), so the conversion is fallible and the error is
+        // surfaced to the caller rather than silently degrading the child's streams.
         match open_file {
-            OpenFile::Stdin(_) => Self::inherit(),
-            OpenFile::Stdout(_) => Self::inherit(),
-            OpenFile::Stderr(_) => Self::inherit(),
-            OpenFile::File(f) => dup_to_stdio(f.as_ref()),
-            OpenFile::PipeReader(r) => dup_to_stdio(r.as_ref()),
-            OpenFile::PipeWriter(w) => dup_to_stdio(w.as_ref()),
-            // NOTE: Custom streams cannot be converted to `Stdio`; we do our best here
-            // and return a null device instead.
-            OpenFile::Stream(_) => Self::null(),
+            OpenFile::Stdin(_) | OpenFile::Stdout(_) | OpenFile::Stderr(_) => Ok(Self::inherit()),
+            OpenFile::File(f) => Ok(f.try_clone()?.into()),
+            OpenFile::PipeReader(r) => Ok(r.try_clone()?.into()),
+            OpenFile::PipeWriter(w) => Ok(w.try_clone()?.into()),
+            // Custom streams have no descriptor to hand to a child process.
+            OpenFile::Stream(_) => Ok(Self::null()),
         }
     }
 }
 
-/// Helper trait to duplicate a handle into an owned `Stdio` in a cross-platform way.
-trait TryCloneToStdio {
-    fn try_clone_to_stdio(&self) -> std::io::Result<Stdio>;
-}
-
-impl TryCloneToStdio for std::fs::File {
-    fn try_clone_to_stdio(&self) -> std::io::Result<Stdio> {
-        Ok(self.try_clone()?.into())
-    }
-}
-
-impl TryCloneToStdio for std::io::PipeReader {
-    fn try_clone_to_stdio(&self) -> std::io::Result<Stdio> {
-        Ok(self.try_clone()?.into())
-    }
-}
-
-impl TryCloneToStdio for std::io::PipeWriter {
-    fn try_clone_to_stdio(&self) -> std::io::Result<Stdio> {
-        Ok(self.try_clone()?.into())
-    }
-}
-
 impl std::io::Read for OpenFile {
     fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
         match self {
@@ -300,8 +260,8 @@ impl std::io::Read for OpenFile {
             )),
             // The handle is shared behind an `Arc`; read through a shared reference (`&File`
             // and `&PipeReader` both implement `Read`).
-            Self::File(f) => (&**f).read(buf),
-            Self::PipeReader(reader) => (&**reader).read(buf),
+            Self::File(f) => f.as_ref().read(buf),
+            Self::PipeReader(reader) => reader.as_ref().read(buf),
             Self::PipeWriter(_) => Err(std::io::Error::other(
                 error::ErrorKind::OpenFileNotReadable("pipe writer"),
             )),
@@ -320,11 +280,11 @@ impl std::io::Write for OpenFile {
             Self::Stderr(f) => f.write(buf),
             // The handle is shared behind an `Arc`; write through a shared reference (`&File`
             // and `&PipeWriter` both implement `Write`).
-            Self::File(f) => (&**f).write(buf),
+            Self::File(f) => f.as_ref().write(buf),
             Self::PipeReader(_) => Err(std::io::Error::other(
                 error::ErrorKind::OpenFileNotWritable("pipe reader"),
             )),
-            Self::PipeWriter(writer) => (&**writer).write(buf),
+            Self::PipeWriter(writer) => writer.as_ref().write(buf),
             Self::Stream(s) => s.write(buf),
         }
     }
@@ -334,9 +294,9 @@ impl std::io::Write for OpenFile {
             Self::Stdin(_) => Ok(()),
             Self::Stdout(f) => f.flush(),
             Self::Stderr(f) => f.flush(),
-            Self::File(f) => (&**f).flush(),
+            Self::File(f) => f.as_ref().flush(),
             Self::PipeReader(_) => Ok(()),
-            Self::PipeWriter(writer) => (&**writer).flush(),
+            Self::PipeWriter(writer) => writer.as_ref().flush(),
             Self::Stream(s) => s.flush(),
         }
     }

brush-core/src/shell/fs.rs

@@ -204,7 +204,7 @@ impl<SE: crate::extensions::ShellExtensions> crate::Shell<SE> {
             && let Ok(fd_num) = filename.to_string_lossy().to_string().parse::<ShellFd>()
             && let Some(open_file) = params.try_fd(self, fd_num)
         {
-            return open_file.try_clone();
+            return Ok(open_file.clone());
         }
 
         Ok(options.open(path_to_open)?.into())

brush-core/src/shell/io.rs

@@ -64,9 +64,7 @@ impl<SE: extensions::ShellExtensions> crate::Shell<SE> {
         };
 
         // If we have a valid trace file, write to it.
-        if let Some(trace_file) = trace_file
-            && let Ok(mut trace_file) = trace_file.try_clone()
-        {
+        if let Some(mut trace_file) = trace_file {
             let _ = writeln!(trace_file, "{prefix}{}", command.as_ref());
         }
     }

brush-shell/tests/cases/compat/fd_management.yaml

@@ -3,25 +3,25 @@ name: "File descriptor management"
 # exhaust the process-wide file-descriptor table.
 #
 # brush runs subshells, command substitutions, background jobs, and function
-# call contexts as in-process tasks rather than via fork(2). Each such context
-# inherits the caller's open files. If those open files are duplicated (via
-# dup(2)) on every clone, deeply nested or highly concurrent scripts accumulate
-# descriptors super-linearly and can exhaust the descriptor table -- which is
-# exactly what broke `nvm ls`. A real shell (bash) gives each forked subshell
-# its own descriptor table, so it stays well within the limit.
+# call contexts as in-process tasks rather than via fork(2), so they all share a
+# single process-wide descriptor table. Each context inherits the caller's open
+# files; sharing those descriptors -- rather than giving each context its own
+# copy -- is what keeps deeply nested or highly concurrent scripts within the
+# limit. A real shell gives each forked subshell its own descriptor table and so
+# stays well within the limit too.
 #
-# Each case lowers the descriptor limit with `ulimit -n` and then runs a
-# workload that, with the old duplicate-on-clone behavior, blew past it
-# (producing "Too many open files" / "failed to duplicate open file" errors and
-# truncated output). With proper descriptor sharing the workload stays
-# comfortably within the limit, matching bash. The limits are chosen so the
-# buggy behavior exceeds them with a wide margin while the correct behavior
-# leaves several-fold headroom.
+# Each case lowers the descriptor limit with `ulimit -n` and runs a workload that
+# stays within the limit only when inherited descriptors are shared across
+# contexts; if each context copied them, the workload would exceed the limit and
+# fail with "Too many open files" and truncated output. The limits leave the
+# correct (sharing) behavior several-fold headroom. These cases assume Unix
+# `ulimit -n` semantics.
 cases:
   # Concurrent fan-out: every backgrounded job is alive simultaneously (they are
-  # all launched before `wait`), so duplicating the inherited descriptors per job
-  # multiplies the live descriptor count by the number of jobs.
+  # all launched before `wait`), so copying the inherited descriptors per job
+  # would multiply the live descriptor count by the number of jobs.
   - name: "Many concurrent background jobs each using command substitution"
+    incompatible_platforms: ["wasi"]
     args:
       - "-c"
       - |
@@ -37,6 +37,7 @@ cases:
   # /dev/null via redirects -- the precise shape that exhausted descriptors in
   # `nvm ls`.
   - name: "Subshell of backgrounded jobs with redirects piped to a filter"
+    incompatible_platforms: ["wasi"]
     args:
       - "-c"
       - |
@@ -49,9 +50,10 @@ cases:
         ( for i in $(seq 1 50); do work "$i" 2>/dev/null & done; wait ) | sort | wc -l | tr -d ' '
 
   # Nested depth: each command-substitution level keeps its ancestors' contexts
-  # alive while it runs, so duplicating inherited descriptors grows the live
+  # alive while it runs, so copying inherited descriptors would grow the live
   # count with nesting depth.
   - name: "Deeply nested command substitution with redirects stays within the fd limit"
+    incompatible_platforms: ["wasi"]
     args:
       - "-c"
       - |