Skip to content

feat(core): add opt-in kill-on-drop for externally spawned commands - #1244

Merged
reubeno merged 2 commits into
reubeno:mainfrom
hartsock:fix/opt-in-kill-external-commands-on-drop
Jul 25, 2026
Merged

feat(core): add opt-in kill-on-drop for externally spawned commands#1244
reubeno merged 2 commits into
reubeno:mainfrom
hartsock:fix/opt-in-kill-external-commands-on-drop

Conversation

@hartsock

@hartsock hartsock commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Found during practical testing. We were experimenting with spawning shells and the outer process would terminate and leave behind the child shell.

sys::process::spawn never sets kill_on_drop, so a spawned child outlives the shell that started it. That is correct for a real shell — it is what makes job control, disowned jobs, and nohup-style usage work — and remains the default.

It is wrong for an embedded shell, where the host creates and destroys the shell as an object. There a surviving child keeps running unattended and holds a duplicate of the shell's stdout/stderr pipe, so a host draining that output never observes EOF.

Adds a kill_external_commands_on_drop creation option, defaulting to false, plumbed through RuntimeOptions to sys::process::spawn alongside the existing external_cmd_leads_session. The stub backend accepts and ignores it. Process-group behavior is untouched, so this signals the immediate child only.

Tests cover both directions and that the option defaults off. Mutation-checked: forcing kill_on_drop(false) fails the enabled case.

hartsock added 2 commits July 19, 2026 21:50
`sys::process::spawn` never sets `kill_on_drop`, so a spawned child outlives
the shell that started it. That is correct for a real shell -- it is what makes
job control, disowned jobs, and `nohup`-style usage work -- and remains the
default.

It is wrong for an embedded shell, where the host creates and destroys the
shell as an object. There a surviving child keeps running unattended and holds
a duplicate of the shell's stdout/stderr pipe, so a host draining that output
never observes EOF.

Add a `kill_external_commands_on_drop` creation option, defaulting to `false`,
plumbed through `RuntimeOptions` to `sys::process::spawn` alongside the
existing `external_cmd_leads_session`. The stub backend accepts and ignores it.
Process-group behavior is untouched, so this signals the immediate child only.

Assisted-by: Claude Code:claude-opus-4-8
The added doc comments were multi-paragraph prose where brush uses one-line
field docs. Trim each to match its neighbors; drop the `# Arguments` block on
the internal `spawn` (it had no doc before); keep only the non-obvious runtime-
teardown note in the test header.

Assisted-by: Claude Code:claude-opus-4-8
@hartsock

Copy link
Copy Markdown
Contributor Author

Found alongside #1184 and #1242 during practical testing of brush embedded as a shell library.

@github-actions

Copy link
Copy Markdown

Public API changes for crate: brush-core

Added items

+pub brush_core::options::RuntimeOptions::kill_external_commands_on_drop: bool
+pub brush_core::CreateOptions::kill_external_commands_on_drop: bool
+pub type brush_core::ShellBuilderState::KillExternalCommandsOnDrop

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.57 μs 17.47 μs -0.10 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.15 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.68 μs 1.68 μs -0.01 μs ⚪ Unchanged
for_loop 32.05 μs 31.74 μs -0.32 μs ⚪ Unchanged
full_peg_complex 57.01 μs 57.18 μs 0.17 μs ⚪ Unchanged
full_peg_for_loop 6.24 μs 6.22 μs -0.02 μs ⚪ Unchanged
full_peg_nested_expansions 15.99 μs 16.12 μs 0.13 μs ⚪ Unchanged
full_peg_pipeline 4.29 μs 4.25 μs -0.04 μs ⚪ Unchanged
full_peg_simple 1.81 μs 1.80 μs -0.01 μs ⚪ Unchanged
function_call 3.70 μs 3.53 μs -0.17 μs ⚪ Unchanged
instantiate_shell 54.77 μs 55.84 μs 1.08 μs 🟠 +1.97%
instantiate_shell_with_init_scripts 26110.64 μs 26038.27 μs -72.38 μs ⚪ Unchanged
parse_peg_bash_completion 2100.55 μs 2116.29 μs 15.73 μs 🟠 +0.75%
parse_peg_complex 20.98 μs 20.71 μs -0.27 μs ⚪ Unchanged
parse_peg_for_loop 2.04 μs 2.02 μs -0.02 μs 🟢 -1.23%
parse_peg_pipeline 2.16 μs 2.08 μs -0.08 μs 🟢 -3.56%
parse_peg_simple 1.09 μs 1.08 μs -0.01 μs ⚪ Unchanged
run_echo_builtin_command 16.81 μs 16.63 μs -0.18 μs ⚪ Unchanged
tokenize_sample_script 3.33 μs 3.43 μs 0.09 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/options.rs 🟢 92% 🟢 92.06% 🟢 0.06%
brush-core/src/shell.rs 🟢 93.99% 🟢 94.85% 🟢 0.86%
Overall Coverage 🟢 75.77% 🟢 75.78% 🟢 0.01%

Minimum allowed coverage is 70%, this run produced 75.78%
Maximum allowed coverage difference is -5%, this run produced 0.01%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1581 74.96
❗️ Error 17 0.81
❌ Fail 157 7.44
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@reubeno

reubeno commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Thanks for the contribution! Agreed that this is valuable for embedded shell hosts.

@reubeno
reubeno merged commit 07e8708 into reubeno:main Jul 25, 2026
52 checks passed
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