feat(shell): add process group spawn option (fix #1332)#3351
Conversation
Add a `processGroup` boolean option to the shell plugin's spawn command. When enabled, the child process is spawned in its own process group (POSIX) or job object (Windows) using the `process-wrap` crate, so that killing the child also kills the entire process tree. This fixes the issue where programs like PyInstaller wrappers spawn a child process that gets orphaned when Tauri kills the parent.
Add end-to-end tests that reproduce the exact scenario from issue tauri-apps#1332: a wrapper process (like PyInstaller's bootloader) spawns a grandchild. - Without process_group: killing the wrapper orphans the grandchild - With process_group: killing the wrapper also kills the grandchild
|
Awesome additions @rterence! May I ask what's holding this up? This would go a long way to simplify one of my projects. |
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Sorry sorry. My backlog is a bit insane ATM. Will try to take a look this week. |
FabianLars
left a comment
There was a problem hiding this comment.
didn't try it yet, just some messy thoughts before going to bed 😅
| #[derive(Debug)] | ||
| pub struct CommandChild { | ||
| inner: Arc<SharedChild>, | ||
| inner: ChildKind, |
There was a problem hiding this comment.
I wonder if process-wrap supports spawning CommandWrap that did not add any wrappers? In that case i assume it should be possible to drop SharedChild completely in favor of CommandWrap? (Edit: Or rather, get rid of just the enum since we still use SharedChild on Unix here?) Then we'd just have to conditionally add the wrapper instead of having the whole child be the switch.
idk if i'm asking for too much here for this pr 😅
There was a problem hiding this comment.
Same constraint as below. The Windows child can't be reduced to a raw Child without losing tree-kill. Happy to tidy the spawn path in a follow-up leaving a TODO?
There was a problem hiding this comment.
I can't follow my own text there lol so let me ask it differently. Can we drop SharedChild on Unix and use the same struct GroupChild as on Windows? If not, what is process-wrap's ProcessGroupChild missing here?
There was a problem hiding this comment.
Addressed in e8d0801. Dropped SharedChild and the ChildKind enum entirely, so there's now a single process-wrap child type for both platforms and process_group is just an optional wrapper. Lmk what you think, happy to tidy up the PR.
There was a problem hiding this comment.
@FabianLars are you happy with this if I tidy up the PR and attach an updated demo?
There was a problem hiding this comment.
@FabianLars if you get a moment could I get another review on this please? Or if this is being fixed elsewhere can you link me? Thanks 😄
Address review feedback on tauri-apps#3351: - Remove `#[cfg(any(unix, windows))]` gates on `ChildKind::ProcessGroup`, the kill/pid match arms, and the spawn block, plus the now-dead `#[cfg(not(any(unix, windows)))]` fallback. That cfg only excluded wasm, which this plugin never builds for. - Set `default-features = false` on process-wrap and enable only the features actually used: std, process-group (unix), creation-flags and job-object (windows). Drops kill-on-drop, process-session, and tracing.
Address review feedback on tauri-apps#3351: - Remove `#[cfg(any(unix, windows))]` gates on `ChildKind::ProcessGroup`, the kill/pid match arms, and the spawn block, plus the now-dead `#[cfg(not(any(unix, windows)))]` fallback. That cfg only excluded wasm, which this plugin never builds for. - Set `default-features = false` on process-wrap and enable only the features actually used: std, process-group (unix), creation-flags and job-object (windows). Drops kill-on-drop, process-session, and tracing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| fn kill(&self) -> std::io::Result<()> { | ||
| // SAFETY: killpg is a standard POSIX syscall. The pgid was obtained from | ||
| // the child's PID, which equals its PGID since it was spawned as a group leader. | ||
| let ret = unsafe { libc::killpg(self.pgid, libc::SIGKILL) }; | ||
| if ret == 0 { | ||
| Ok(()) | ||
| } else { | ||
| Err(std::io::Error::last_os_error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
opposed to https://docs.rs/process-wrap/latest/process_wrap/std/struct.ProcessGroupChild.html in https://github.com/watchexec/process-wrap/blob/main/src/std/process_group.rs#L169-L182 (kill = start_kill + wait) we do not wait for the children to exit here
…d changefile - Add `tracing` feature to process-wrap: with default-features = false its Windows job_object.rs calls `debug\!` unconditionally while the import is gated behind `#[cfg(feature = "tracing")]`, failing the Windows build with "cannot find macro `debug`". - Reformat process/mod.rs assertions to satisfy `cargo fmt --check`. - Reindent the process-wrap features array to 2 spaces for taplo. - Use covector package keys (`shell` / `shell-js`) in the change file so the check-change-files and covector status jobs pass.
Collapse the ChildKind enum and the per-platform GroupChild types into a single process-wrap-backed child. The command always spawns through StdCommandWrap; the process_group flag is now just an optional wrapper rather than a switch into a separate child type. This drops the shared_child dependency and resolves two review notes: the Unix path no longer hand-rolls killpg, and kill() now goes through process-wrap's kill (start_kill + wait), which reaps the entire process group instead of only signalling it. A background thread polls try_wait to surface the exit status, since process-wrap's wrappers only expose &mut self wait methods (the reason SharedChild was used on Unix in the first place).
Package Changes Through 0dc519eThere are 6 changes which include log with minor, log-js with minor, positioner with patch, positioner-js with patch, shell with minor, shell-js with minor Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
e8d0801 to
0dc519e
Compare
Summary
processGroupboolean option to the shell plugin'sspawncommandprocess-wrapcrateCloses #1332
cc. @FabianLars
Changes
process/mod.rs):Command::set_process_group()builder method,GroupChildtype with platform-specific kill (Unix:killpg, Windows: job object termination viaprocess-wrap)commands.rs):processGroupfield onCommandOptions, plumbed throughprepare_cmdindex.ts):processGroup?: booleanonSpawnOptionsprocess-wrap8.2 (std feature),libc0.2 (unix)Testing
processGroupkilling the wrapper orphans the grandchild; withprocessGroupkilling the wrapper also kills the grandchildprocess-wrapJobObjectwith pollingtry_wait)Demo
The video below demonstrates the fix using a real PyInstaller-bundled Python app spawned from the Tauri example app's Shell view.
First half — without
processGroup(the bug):demo_appprocesses persist — the grandchild is orphanedSecond half — with
processGroupenabled (the fix):demo_appprocesses are terminatedprocess-group-demo.mp4