Skip to content

feat(core): support cooperative SIGINT cancellation#1189

Open
mtanski wants to merge 4 commits into
reubeno:mainfrom
mtanski:codex/upstream-ctrl-c-cancel
Open

feat(core): support cooperative SIGINT cancellation#1189
mtanski wants to merge 4 commits into
reubeno:mainfrom
mtanski:codex/upstream-ctrl-c-cancel

Conversation

@mtanski

@mtanski mtanski commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Brush currently handles Ctrl-C differently depending on what is running. Bash sends SIGINT to the foreground process group for external commands and pipelines, and it also interrupts shell-native evaluation. If an INT trap is installed, Bash runs the trap and lets it decide control flow; otherwise interrupted shell-native work returns the usual SIGINT status.

This brings Brush closer to that model for both shell-native execution and foreground processes. ExecutionParameters can now carry a host-requested signal, and centralized execution checkpoints let loops, functions, builtins, compound commands, and traps observe cancellation without manually instrumenting every AST node. Foreground process waits can wake on the same signal state and forward the signal to the child process group, including pipelines.

The interactive shell wires Ctrl-C into the active execution parameters. In a PTY, the terminal driver already delivers SIGINT to the foreground process group, so the interactive path avoids double-forwarding and uses the cooperative signal path to make Brush's own shell code see the interrupt.

For embedders that drive a shell over an API, ExecutionParameters::request_host_signal(signal_number) is the hook for injecting a signal into both shell-native execution and foreground children. ExecutionParameters::request_forced_signal(signal_number) is available for timeout/kill-style cancellation that should bypass shell traps.

@reubeno

reubeno commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Thanks for the contribution!

This is an area I've long wanted to improve and appreciate you digging in.

I'll need more time to look through this than your other 2 PRs. One of the main things I'll be looking at is how the design will work for alternate (embedding) hosts of brush-core. As you've noted, our preexisting signal handling is inconsistent and wasn't too thoughtful about ownership of process signal configuration/management in an embedded host -- I'm hoping this will help a bit, given your description.

@mtanski

mtanski commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

I have some additional improvements for signal handling that I plan on submitting later that help a lot with both embedding (at least in our environment) and conformance to bash behavior, but I need to time to rework them to make them upstream friendly.

Hopefully this one is pretty straightforward as it works for interactive login shells (brush interactive) and embedded cases when you need to emulated it (convert an API call to signal). Let me know if you have any questions.

@mtanski mtanski force-pushed the codex/upstream-ctrl-c-cancel branch from dbdd57a to 262b342 Compare June 6, 2026 03:20
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

Public API changes for crate: brush-core

Added items

+pub const fn brush_core::processes::ChildProcess::forwarded_signal(&self) -> bool
+pub brush_core::results::ExecutionControlFlow::Interrupted
+pub const fn brush_core::results::ExecutionResult::interrupted() -> Self
+pub const fn brush_core::results::ExecutionResult::interrupted() -> Self
+pub fn brush_core::results::ExecutionResult::signaled(i32) -> Self
+pub fn brush_core::results::ExecutionResult::signaled(i32) -> Self
+pub fn brush_core::sys::commands::CommandFgControlExt::reset_job_control_signals(&mut self)
+pub fn std::process::Command::reset_job_control_signals(&mut self)
+pub use brush_core::sys::signal::await_ctrl_c
+pub brush_core::ExecutionControlFlow::Interrupted
+pub fn brush_core::ExecutionParameters::clear_pending_signal(&self)
+pub fn brush_core::ExecutionParameters::request_forced_signal(&self, i32)
+pub fn brush_core::ExecutionParameters::request_host_signal(&self, i32)
+pub fn brush_core::ExecutionParameters::request_interactive_sigint(&self)
+pub fn brush_core::ExecutionParameters::request_sigint(&self)

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.54 μs 17.46 μs -0.08 μs ⚪ Unchanged
eval_arithmetic 0.17 μs 0.15 μs -0.02 μs 🟢 -8.93%
expand_one_string 1.66 μs 1.83 μs 0.16 μs 🟠 +9.86%
for_loop 31.55 μs 39.48 μs 7.93 μs 🟠 +25.15%
full_peg_complex 56.66 μs 57.24 μs 0.58 μs ⚪ Unchanged
full_peg_for_loop 6.24 μs 6.33 μs 0.10 μs 🟠 +1.59%
full_peg_nested_expansions 16.55 μs 17.34 μs 0.78 μs 🟠 +4.73%
full_peg_pipeline 4.18 μs 4.28 μs 0.10 μs 🟠 +2.42%
full_peg_simple 1.75 μs 1.78 μs 0.03 μs ⚪ Unchanged
function_call 3.53 μs 4.55 μs 1.03 μs 🟠 +29.07%
instantiate_shell 55.37 μs 54.16 μs -1.21 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 27360.82 μs 28085.58 μs 724.76 μs 🟠 +2.65%
parse_peg_bash_completion 2113.77 μs 2103.92 μs -9.85 μs ⚪ Unchanged
parse_peg_complex 19.25 μs 18.30 μs -0.95 μs 🟢 -4.95%
parse_peg_for_loop 1.87 μs 1.84 μs -0.03 μs ⚪ Unchanged
parse_peg_pipeline 1.93 μs 2.01 μs 0.09 μs 🟠 +4.47%
parse_peg_simple 1.03 μs 1.03 μs 0.00 μs ⚪ Unchanged
run_echo_builtin_command 15.43 μs 16.53 μs 1.10 μs 🟠 +7.14%
tokenize_sample_script 3.52 μs 4.27 μs 0.75 μs 🟠 +21.23%

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins/src/kill.rs 🟠 54.76% 🟠 63.04% 🟢 8.28%
brush-builtins/src/ulimit.rs 🟠 66% 🟠 68.67% 🟢 2.67%
brush-core/src/commands.rs 🟢 91.47% 🟢 91.65% 🟢 0.18%
brush-core/src/interp.rs 🟢 93% 🟢 89.53% 🔴 -3.47%
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/processes.rs 🟠 63.41% 🟠 73.12% 🟢 9.71%
brush-core/src/results.rs 🟢 83.33% 🟢 82.42% 🔴 -0.91%
brush-core/src/shell/fs.rs 🟢 97.39% 🟢 97.58% 🟢 0.19%
brush-core/src/shell/io.rs 🟢 83.67% 🟢 82.98% 🔴 -0.69%
brush-core/src/shell/traps.rs 🟢 92.86% 🟢 95.24% 🟢 2.38%
brush-core/src/sys/unix/commands.rs 🔴 38.64% 🔴 31.88% 🔴 -6.76%
brush-core/src/sys/unix/signal.rs 🟠 53.85% 🟠 58.43% 🟢 4.58%
brush-interactive/src/interactive_shell.rs 🟠 63.83% 🟠 64.37% 🟢 0.54%
brush-parser/src/ast.rs 🔴 46.26% 🔴 48.31% 🟢 2.05%
brush-test-harness/src/comparison.rs 🟢 76.92% 🟢 83.97% 🟢 7.05%
brush-test-harness/src/execution.rs 🟢 80.2% 🟢 83.17% 🟢 2.97%
brush-test-harness/src/reporting.rs 🔴 11.62% 🔴 46.48% 🟢 34.86%
brush-test-harness/src/runner.rs 🟢 84.17% 🟢 84.77% 🟢 0.6%
brush-test-harness/src/util.rs 🔴 47.76% 🟠 71.64% 🟢 23.88%
Overall Coverage 🟢 74.32% 🟢 75.08% 🟢 0.76%

Minimum allowed coverage is 70%, this run produced 75.08%
Maximum allowed coverage difference is -5%, this run produced 0.76%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1583 75.06
❗️ Error 17 0.81
❌ Fail 155 7.35
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

mtanski and others added 4 commits June 6, 2026 09:54
Add host-requested signal state to execution parameters and checkpoint shell-native execution so loops, lists, and traps can respond without manually instrumenting every AST node.

Co-authored-by: Evan Rittenhouse <evan.rittenhouse@magic.dev>
Co-authored-by: Milosz Tanski <milosz@magic.dev>
Assisted-by: AI
Reset job-control signal dispositions for pipeline children that join an existing process group, and cover host signal delivery across a foreground pipeline.

Assisted-by: AI
Forward interactive Ctrl-C into the active shell execution parameters so pure shell loops and INT traps behave like Bash in the built-in interactive shell.

Assisted-by: AI
These cases now pass after cooperative SIGINT cancellation and foreground pipeline signal handling were added.

Assisted-by: AI
@mtanski mtanski force-pushed the codex/upstream-ctrl-c-cancel branch from 262b342 to d5272b9 Compare June 6, 2026 13:57
@mtanski

mtanski commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

@reubeno I ported the signaling changes from our private branch into this PR. As you can see this fixes a bunch bash signalizing/TRAP compatibility. This also should not hurt embedding (based on our 8mo experience), and exposes the cancel API in such a way it helps embedding cases cancel pipelines in a flexible way.

I see I have caused a windows build break, I will TAL when I have some time again.

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