Add kill-tree helper and runtime sidecar PID registry#14443
Conversation
Package Changes Through ed8d624There are 1 changes which include @tauri-apps/api with patch 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 |
Legend-Master
left a comment
Legend-Master
left a comment
There was a problem hiding this comment.
Thanks for working on this!
| /// background tasks). This is guaranteed to run from the thread performing | ||
| /// the app cleanup/exit sequence. | ||
| #[allow(unused_variables)] | ||
| fn cleanup_before_exit(&mut self, app: &AppHandle<R>) {} |
There was a problem hiding this comment.
We would also want to implement this for the TauriPlugin so that tauri::plugin::Builder could have access to this API
There was a problem hiding this comment.
for sure! I’ll add this to the TauriPlugin trait as well so it’s accessible via tauri::plugin::Builder.
| /// protected or system processes, or when the caller lacks sufficient | ||
| /// privileges. Callers should handle and log any errors returned by this | ||
| /// function. | ||
| pub fn kill_process_tree(pid: u32) -> std::io::Result<()> { |
There was a problem hiding this comment.
since this is a generic function and not bound to this app instance imo this should be moved into https://github.com/tauri-apps/plugins-workspace/tree/v2/plugins/process (and in theory that plugin's exit and restart should probably be part of the .app core plugin instead but that's a discussion for another day)
There was a problem hiding this comment.
Would it be okay if I leave a TODO for now and move it in a follow-up PR?
There was a problem hiding this comment.
No, we at least have to decide how to proceed here because once this is added in tauri we cannot remove it without a major release since removing apis is a breaking change.
There was a problem hiding this comment.
Should I move kill_process_tree to the process plugin now as you suggested? I’ll update this PR to avoid a future breaking change.
There was a problem hiding this comment.
i think that would make sense, yes. we can always move it back if needed (that way around works)
|
I think we have now gathered a few different problems around this now:
My brain is basically fried right now, no more processes 😭 |
|
I fixed point 1, 2 in my(our) project. 3 and 4 not test on my machine but has no community report related. With sidecar, I hold a custom strcture to wrap ProcessChild and give impl Drop for it to handle kill when droping. The entire app(which is main process) listen on system signal and excute tauri exiting and custom behavior, it will kill itself and its sidecar. They works well both in tauri dev and release build. |
We found it's hard to fully trust tauri exit behavior, and external listen system signal mannuly in clash-verge-rev/crates/signal. But behavior of macOS system shutdown signal seems hooked by tauri, can only processing with |
|
This PR using system shell. For Windows example in some case, user can not invoke system powershell due to their machine permission setting or even disable powershell usage. We might not want to handle PID with shell diretcly, and there are potential security problems whether if cross-platform. |
I think this is more or less something we should provide in shell plugin?
Could you explain about this a bit more?
Not the biggest fan of this either |
Yes, and the auto-cleanup or likely behavior can be toggled via a field setting whether if spawn or runtime. We did not provide this behavior before, might destroy downstream program behavior if enable by default.
Months ago, did not remember too much details.
More, hanlding with resue |
Hmm, that sounds weird, the tauri async runtime by default should be very much the same as using a tokio one directly |
Our program continuosly handle differents tauri commands, data-related processing, system setting processing. Might be a little more complex than genral program. The async_runtime::spawn /// async_runtime::spawn
pub fn spawn<F>(task: F) -> JoinHandle<F::Output>
where
F: Future + Send + 'static,
F::Output: Send + 'static,
{
let runtime = RUNTIME.get_or_init(default_runtime);
runtime.spawn(task)
} |
Wierd on my test on macOS, signal_hook can not handle shutdown signal, but SIGTERM, SIGTERM interminal or standalone were fine. Macos shutdown handle only works with #[cfg(target_os = "macos")]
tauri::RunEvent::Exit => async_runtime::block_on(async {
some_clean_up()
}),Linux works with competely works with signal_hook. Seemed tauri hooked macOS shutdown signal and use |
Not gonna lie, this sounds like something macOS would. Not sending posix signals in favor of the NSApplication delegate bullshit would be such an Apple move 🙃 |
|
Hi, Severity: action required | Category: reliability How to fix: Handle poisoned mutex gracefully Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
6 participants
Adds a best-effort process-tree killer and a lightweight runtime sidecar PID registry, ensuring that descendant processes spawned by sidecars are properly terminated when their parent sidecar is killed.
This resolves the core issue described in #14360 by introducing both API and runtime cleanup mechanisms for sidecar process management.
Details:
Files Changed:
Testing:
Call app_handle.register_sidecar(child.id() as u32) after spawning.
Fixes #14360