Local apps should terminate themselves when dropped#78
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic termination of local applications to prevent orphaned or zombie processes. It adds defensive cleanup mechanisms by setting kill_on_drop(true) on UV processes and implementing a Drop trait for LocalApp that calls terminate() when the app is dropped.
- Added
kill_on_drop(true)to all UV command executions - Implemented
Droptrait forLocalAppto automatically terminate processes on cleanup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/tower-uv/src/lib.rs | Added kill_on_drop(true) to four UV command invocations to ensure child processes are terminated when dropped |
| crates/tower-runtime/src/local.rs | Implemented Drop trait for LocalApp to call terminate() when the app instance is dropped |
| impl Drop for LocalApp { | ||
| fn drop(&mut self) { | ||
| // We want to ensure that we cancel the process if it is still running. | ||
| let _ = self.terminate(); |
There was a problem hiding this comment.
Silently ignoring the result of terminate() may hide important errors. Consider logging the error or at least adding a comment explaining why the error is intentionally ignored.
socksy
left a comment
socksy
left a comment
There was a problem hiding this comment.
Confused why "kill_on_drop" isn't the default. Docs say it's because it's the case in the standard library... but why is that so?
Sometimes we end up with orphaned or zombied apps, despite our best efforts. This PR tries to be more aggressive about getting rid of zombie apps in the event of a failure.