Skip to content

fix(openfiles): share descriptors across cloned shell contexts#1181

Merged
reubeno merged 3 commits into
mainfrom
fd-sharing
Jun 4, 2026
Merged

fix(openfiles): share descriptors across cloned shell contexts#1181
reubeno merged 3 commits into
mainfrom
fd-sharing

Conversation

@reubeno

@reubeno reubeno commented Jun 1, 2026

Copy link
Copy Markdown
Owner

Addresses file-descriptor exhaustion in brush by mapping shell "FD" cloning to cloning an Arc-shared OS resource, rather than duplicating the host fd.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

Test Results

    5 files     40 suites   16m 32s ⏱️
2 487 tests 2 487 ✅ 0 💤 0 ❌
7 730 runs  7 730 ✅ 0 💤 0 ❌

Results for commit a4f0e7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

Public API changes for crate: brush-core

Removed items

-pub fn brush_core::openfiles::OpenFile::try_clone(&self) -> core::result::Result<Self, std::io::error::Error>
-impl core::convert::From<brush_core::openfiles::OpenFile> for std::process::Stdio
-pub fn std::process::Stdio::from(brush_core::openfiles::OpenFile) -> Self

Added items

+impl core::convert::TryFrom<brush_core::openfiles::OpenFile> for std::process::Stdio
+pub type std::process::Stdio::Error = brush_core::error::Error
+pub fn std::process::Stdio::try_from(brush_core::openfiles::OpenFile) -> core::result::Result<Self, Self::Error>

Changed items

-pub brush_core::openfiles::OpenFile::File(std::fs::File)
+pub brush_core::openfiles::OpenFile::File(alloc::sync::Arc<std::fs::File>)
-pub brush_core::openfiles::OpenFile::PipeReader(std::io::pipe::PipeReader)
+pub brush_core::openfiles::OpenFile::PipeReader(alloc::sync::Arc<std::io::pipe::PipeReader>)
-pub brush_core::openfiles::OpenFile::PipeWriter(std::io::pipe::PipeWriter)
+pub brush_core::openfiles::OpenFile::PipeWriter(alloc::sync::Arc<std::io::pipe::PipeWriter>)

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.79 μs 17.57 μs -0.21 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.15 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.69 μs 1.72 μs 0.03 μs ⚪ Unchanged
for_loop 30.97 μs 31.14 μs 0.17 μs ⚪ Unchanged
full_peg_complex 56.23 μs 56.49 μs 0.26 μs ⚪ Unchanged
full_peg_for_loop 6.22 μs 6.19 μs -0.03 μs ⚪ Unchanged
full_peg_nested_expansions 16.57 μs 16.54 μs -0.04 μs ⚪ Unchanged
full_peg_pipeline 4.25 μs 4.18 μs -0.07 μs 🟢 -1.60%
full_peg_simple 1.76 μs 1.74 μs -0.02 μs ⚪ Unchanged
function_call 3.58 μs 3.53 μs -0.05 μs ⚪ Unchanged
instantiate_shell 55.77 μs 55.81 μs 0.04 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 28656.67 μs 28164.84 μs -491.83 μs ⚪ Unchanged
parse_peg_bash_completion 2104.35 μs 2086.90 μs -17.45 μs ⚪ Unchanged
parse_peg_complex 18.92 μs 18.96 μs 0.04 μs ⚪ Unchanged
parse_peg_for_loop 1.86 μs 1.85 μs -0.01 μs ⚪ Unchanged
parse_peg_pipeline 1.99 μs 1.93 μs -0.06 μs 🟢 -3.21%
parse_peg_simple 1.06 μs 1.03 μs -0.04 μs ⚪ Unchanged
run_echo_builtin_command 17.37 μs 15.27 μs -2.10 μs 🟢 -12.11%
tokenize_sample_script 3.56 μs 3.48 μs -0.09 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins/src/ulimit.rs 🟠 66% 🟠 68.67% 🟢 2.67%
brush-core/src/commands.rs 🟢 91.47% 🟢 91.37% 🔴 -0.1%
brush-core/src/interp.rs 🟢 93% 🟢 92.99% 🔴 -0.01%
brush-core/src/jobs.rs 🟠 50.68% 🟠 51.13% 🟢 0.45%
brush-core/src/openfiles.rs 🟠 63.64% 🟠 64.15% 🟢 0.51%
brush-core/src/shell/io.rs 🟢 83.67% 🟢 82.98% 🔴 -0.69%
brush-parser/src/ast.rs 🔴 46.26% 🔴 48.31% 🟢 2.05%
Overall Coverage 🟢 74.32% 🟢 74.42% 🟢 0.1%

Minimum allowed coverage is 70%, this run produced 74.42%
Maximum allowed coverage difference is -5%, this run produced 0.1%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1582 75.01
❗️ Error 18 0.85
❌ Fail 155 7.35
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses file-descriptor exhaustion in brush by switching cloned shell contexts to share inherited open file descriptors (instead of duplicating them), which is important because brush runs many “subshell-like” constructs as in-process tasks sharing one process-wide fd table.

Changes:

  • Add compatibility regression tests that stress nested/concurrent constructs under a lowered ulimit -n.
  • Rework OpenFile cloning semantics to share underlying file/pipe descriptors via Arc, only duplicating when needed for external child processes.
  • Adjust redirection and command-spawn plumbing to align with shared-descriptor behavior (including making OpenFile -> Stdio conversion fallible).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
brush-shell/tests/cases/compat/fd_management.yaml Adds regression cases to catch fd exhaustion under concurrency/nesting.
brush-core/src/shell/io.rs Avoids cloning trace fd on each write; writes directly to the selected open file.
brush-core/src/shell/fs.rs Stops duplicating /dev/fd/N opens; reuses the referenced fd’s OpenFile.
brush-core/src/openfiles.rs Switches file/pipe handles to Arc sharing; updates child-process handoff to duplicate explicitly and makes Stdio conversion fallible.
brush-core/src/interp.rs Updates redirection setup to share OpenFiles rather than try_clone()-duplicating them.
brush-core/src/commands.rs Avoids cloning execution parameters for function invocation; executes with shared params reference.

Comment thread brush-shell/tests/cases/compat/fd_management.yaml Outdated
Comment thread brush-shell/tests/cases/compat/fd_management.yaml Outdated
Comment thread brush-shell/tests/cases/compat/fd_management.yaml Outdated
Comment thread brush-core/src/commands.rs Outdated
Comment thread brush-core/src/openfiles.rs
Comment thread brush-core/src/openfiles.rs
Comment thread brush-core/src/interp.rs Outdated
reubeno and others added 3 commits June 1, 2026 15:59
Brush runs subshells, command substitutions, background jobs, and
function-call contexts as in-process clones rather than fork(2) children,
all sharing one process-wide descriptor table. OpenFiles::clone() issued a
real dup(2) for every descriptor on every clone, so descriptors accumulated
super-linearly with nesting depth and concurrent fan-out. A single `nvm ls`
peaked at over 1000 simultaneous descriptors and hit EMFILE ("Too many open
files") on systems with a low limit such as macOS's default of 256.

Store file/pipe handles in OpenFile behind Arc so cloning shares the
descriptor (a refcount bump) with the same open-file-description semantics
dup would have provided; a real dup now happens only when handing a
descriptor to an external child process. Also drop the redundant per-call
params clone in shell-function invocation, which duplicated every inherited
descriptor for the duration of each function body.

nvm ls peak: ~1008 -> ~93 descriptors. As a side effect, OpenFile::clone can
no longer fail, so the "failed to duplicate open file" fallback no longer
triggers under descriptor pressure.

Part of #1173.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@reubeno
reubeno marked this pull request as ready for review June 2, 2026 02:12
@reubeno
reubeno merged commit 428f477 into main Jun 4, 2026
49 checks passed
@reubeno
reubeno deleted the fd-sharing branch June 4, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants