feat: add job control and signal handling#89
Conversation
📝 WalkthroughWalkthroughThe shell now initializes Unix signal handling, tracks the foreground process group, transfers terminal control during command execution, and routes parsed or fallback commands through the new job-control execution path. ChangesUnix job control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Shell
participant JobControl
participant ChildProcess
participant Terminal
Shell->>JobControl: command and arguments
JobControl->>ChildProcess: spawn process group
JobControl->>Terminal: assign foreground process group
JobControl->>ChildProcess: wait for exit
JobControl->>Terminal: restore shell process group
JobControl-->>Shell: return
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/job_control.rs`:
- Around line 79-89: Update the foreground terminal handoff flow around the
existing tcsetpgrp calls to temporarily ignore SIGTTOU before both terminal
ownership changes, then restore the prior signal disposition afterward. Move
terminal restoration and FG_PGID reset into a cleanup path that runs even if
waiting or handoff handling exits early, ensuring the shell regains control
without being stopped.
- Around line 10-25: Update init_signal_handlers to construct
signal_hook::iterator::Signals for SIGCHLD, SIGINT, and SIGTSTP before calling
thread::spawn, handling registration errors there; then move the successfully
created Signals instance into the worker thread instead of creating it inside
the thread.
- Around line 29-37: Ensure the SIGCHLD handler does not reap the foreground
child, leaving its status for the foreground wait path. Update the foreground
wait logic around the blocking waitpid call to use WUNTRACED, retry on EINTR,
handle ECHILD, and process stopped-child status before restoring terminal
ownership with tcsetpgrp.
In `@src/main.rs`:
- Around line 63-65: Update the parse-error fallback in the Err branch around
split_posix_words to use split_first() and return early when no tokens are
present, preserving the existing spawn_and_wait behavior for non-empty parts
without indexing parts[0] unsafely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdf8f90d-d07a-4f8a-a885-ac0861c3be3c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/job_control.rssrc/main.rs
| pub fn init_signal_handlers() { | ||
| static START: std::sync::Once = std::sync::Once::new(); | ||
| START.call_once(|| { | ||
| // Spawn a thread to handle signals using signal-hook's iterator | ||
| let _ = thread::spawn(|| { | ||
| let mut signals = match signal_hook::iterator::Signals::new(&[ | ||
| libc::SIGCHLD, | ||
| libc::SIGINT, | ||
| libc::SIGTSTP, | ||
| ]) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("Failed to register signal handlers: {}", e); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== job_control.rs outline ==\n'
ast-grep outline src/job_control.rs --view expanded || true
printf '\n== relevant lines in job_control.rs ==\n'
nl -ba src/job_control.rs | sed -n '1,220p'
printf '\n== dependency versions ==\n'
rg -n 'signal-hook|libc' Cargo.toml Cargo.lock --context 2 || true
printf '\n== search for init_signal_handlers usage ==\n'
rg -n 'init_signal_handlers|Signals::new|waitpid|tcsetpgrp|SIGTTOU|WUNTRACED' src --context 3 || trueRepository: TruFoundation/TruShell
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/job_control.rs ==\n'
sed -n '1,220p' src/job_control.rs
printf '\n== Cargo.toml entries ==\n'
rg -n 'signal-hook|libc' Cargo.toml --context 2 || true
printf '\n== Cargo.lock entries ==\n'
rg -n 'name = "signal-hook"|name = "libc"|version = "' Cargo.lock --context 2 || trueRepository: TruFoundation/TruShell
Length of output: 11436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== call sites for init_signal_handlers ==\n'
rg -n 'init_signal_handlers\(' src --context 3 || true
printf '\n== broader job control uses ==\n'
rg -n 'SIGCHLD|SIGINT|SIGTSTP|tcsetpgrp|waitpid|setpgid|FG_PGID' src --context 2 || trueRepository: TruFoundation/TruShell
Length of output: 4578
🌐 Web query:
signal-hook 0.3 iterator::Signals::new signature accepts slice or iterator
💡 Result:
Yes, the signal-hook 0.3 library's Signals::new method accepts a slice iterator. Specifically, the method is defined with a generic signature that accepts any type I that implements IntoIterator where the items S can be borrowed as a c_int [1][2][3]. The signature is: pub fn new<I, S>(signals: I) -> Result<Self, Error> where I: IntoIterator<Item = S>, S: Borrow<c_int>, Because a reference to a slice, such as &[c_int], implements IntoIterator [1][3], you can pass a slice directly to Signals::new as shown in the following example: let mut signals = Signals::new(&[SIGUSR1, SIGUSR2])?; This flexibility allows you to pass arrays, vectors, or slices, as they all satisfy the IntoIterator trait requirement [2][3].
Citations:
- 1: https://docs.rs/signal-hook/0.3.14/signal_hook/iterator/struct.SignalsInfo.html
- 2: https://docs.rs/signal-hook/latest/src/signal_hook/iterator/mod.rs.html
- 3: https://rust.velas.com/signal_hook/iterator/struct.Signals.html
Register the handlers before starting the worker thread. Signals::new runs inside thread::spawn, so init_signal_handlers() can return before SIGCHLD/SIGINT/SIGTSTP are registered. Create Signals first, then move it into the thread.
🧰 Tools
🪛 Clippy (1.96.0)
[warning] 15-15: the borrowed expression implements the required traits
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/job_control.rs` around lines 10 - 25, Update init_signal_handlers to
construct signal_hook::iterator::Signals for SIGCHLD, SIGINT, and SIGTSTP before
calling thread::spawn, handling registration errors there; then move the
successfully created Signals instance into the worker thread instead of creating
it inside the thread.
| libc::SIGCHLD => { | ||
| // Reap any finished children (non-blocking) | ||
| loop { | ||
| let mut status: libc::c_int = 0; | ||
| let pid = unsafe { libc::waitpid(-1, &mut status as *mut i32, libc::WNOHANG) }; | ||
| if pid <= 0 { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' src/job_control.rs | nl -baRepository: TruFoundation/TruShell
Length of output: 200
🏁 Script executed:
sed -n '1,160p' src/job_control.rs | cat -nRepository: TruFoundation/TruShell
Length of output: 4405
🏁 Script executed:
rg -n "waitpid|WUNTRACED|SIGCHLD|SIGTSTP|try_wait|tcsetpgrp|setpgid" srcRepository: TruFoundation/TruShell
Length of output: 1046
Use a single foreground child-status owner src/job_control.rs:29-37, 82-89
The SIGCHLD reaper can consume the foreground child’s status before the blocking waitpid(pid, 0), and that wait never reports SIGTSTP. Ctrl-Z can leave the shell hung with the terminal still assigned to the stopped process group. Let the foreground path own status handling, wait with WUNTRACED, and handle EINTR/ECHILD before restoring tcsetpgrp.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/job_control.rs` around lines 29 - 37, Ensure the SIGCHLD handler does not
reap the foreground child, leaving its status for the foreground wait path.
Update the foreground wait logic around the blocking waitpid call to use
WUNTRACED, retry on EINTR, handle ECHILD, and process stopped-child status
before restoring terminal ownership with tcsetpgrp.
| // Give terminal control to child process group | ||
| unsafe { libc::tcsetpgrp(libc::STDIN_FILENO, pid) }; | ||
|
|
||
| // Wait for the child to exit (blocking) | ||
| let mut status: libc::c_int = 0; | ||
| let _ = unsafe { libc::waitpid(pid, &mut status as *mut i32, 0) }; | ||
|
|
||
| // Restore terminal control to the shell | ||
| let shell_pgid = unsafe { libc::getpgrp() }; | ||
| unsafe { libc::tcsetpgrp(libc::STDIN_FILENO, shell_pgid) }; | ||
| FG_PGID.store(0, Ordering::SeqCst); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== job_control.rs outline ==\n'
ast-grep outline src/job_control.rs --view expanded || true
printf '\n== tcsetpgrp / SIGTTOU references ==\n'
rg -n "tcsetpgrp|SIGTTOU|sigaction|signal|setpgid|getpgrp|FG_PGID" src Cargo.toml . -g '!target' || true
printf '\n== src/job_control.rs relevant lines ==\n'
nl -ba src/job_control.rs | sed -n '1,220p'Repository: TruFoundation/TruShell
Length of output: 3521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/job_control.rs (first 220 lines) ==\n'
sed -n '1,220p' src/job_control.rs
printf '\n== src/main.rs ==\n'
sed -n '1,120p' src/main.rsRepository: TruFoundation/TruShell
Length of output: 7516
Ignore SIGTTOU around tcsetpgrp here. src/job_control.rs:79-89 restores the terminal while TruShell is backgrounded, so the final tcsetpgrp can stop the shell. Check both terminal handoff calls and restore ownership from a cleanup path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/job_control.rs` around lines 79 - 89, Update the foreground terminal
handoff flow around the existing tcsetpgrp calls to temporarily ignore SIGTTOU
before both terminal ownership changes, then restore the prior signal
disposition afterward. Move terminal restoration and FG_PGID reset into a
cleanup path that runs even if waiting or handoff handling exits early, ensuring
the shell regains control without being stopped.
| Err(err) => { | ||
| eprintln!("Parse error: {}", err); | ||
| execute_system_command_from_input(trimmed_input); | ||
| job_control::spawn_and_wait(&parts[0], &parts[1..].to_vec()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/main.rs (around lines 1-140)\n'
cat -n src/main.rs | sed -n '1,140p'
printf '\n## Search for split_posix_words and empty-token guards\n'
rg -n "split_posix_words|split_first\\(|parts\\[0\\]|spawn_and_wait" src -SRepository: TruFoundation/TruShell
Length of output: 6371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/main.rs (split_posix_words and related fallback)\n'
cat -n src/main.rs | sed -n '120,240p'
printf '\n## parser.rs outline\n'
ast-grep outline src/parser.rs --view expanded || true
printf '\n## Search for empty-token handling around command execution\n'
rg -n "is_empty\\(|split_first\\(|spawn_and_wait\\(|parts\\[0\\]" src/main.rs src/job_control.rs src/parser.rs -SRepository: TruFoundation/TruShell
Length of output: 7671
Preserve the empty-token guard in the parse-error fallback. split_posix_words can return [] for malformed input like "" or a trailing \, so parts[0] can panic here. Use split_first() and return early instead of indexing.
🧰 Tools
🪛 Clippy (1.96.0)
[warning] 65-65: unnecessary use of to_vec
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 63 - 65, Update the parse-error fallback in the Err
branch around split_posix_words to use split_first() and return early when no
tokens are present, preserving the existing spawn_and_wait behavior for
non-empty parts without indexing parts[0] unsafely.
1 participant
Summary:
signal-hook,nix,once_cell,libc.Files changed:
Summary by CodeRabbit
New Features
Bug Fixes