fix: terminate timed-out worker process groups - #80
Open
BunsDev wants to merge 1 commit into
Open
Conversation
Signed-off-by: Codex <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the host-backed worker timeout behavior by ensuring coven-code is launched in its own Unix process group and that a timeout terminates the entire group, preventing background descendants from surviving and retaining tokens/resources.
Changes:
- Add a
KillTargetabstraction and host process-group setup/termination path in the worker backend. - Add
libcas a workspace dependency to support Unixkill(-pgid, SIGKILL)signaling. - Strengthen the timeout regression test to detect surviving background descendants after timeout.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Adds libc to workspace dependencies for Unix process-group signaling. |
| Cargo.lock | Locks libc dependency resolution in the workspace. |
| crates/worker/Cargo.toml | Enables libc for the worker crate. |
| crates/worker/src/backend.rs | Launches host sessions in a process group and adds timeout logic to kill process groups or containers based on KillTarget. |
| crates/worker/src/lib.rs | Updates the timeout regression test to spawn a background descendant and assert it cannot outlive timeout cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+211
to
220
| if matches!(kill, KillTarget::ProcessGroup) { | ||
| kill_process_group(&mut child).await; | ||
| } else { | ||
| let _ = child.kill().await; | ||
| } | ||
| let _ = child.wait().await; | ||
| // …then the container itself: killing the docker CLI does not | ||
| // Killing the docker CLI does not | ||
| // reliably stop the container it launched. | ||
| if let Some(kill) = kill { | ||
| if let KillTarget::Container(kill) = kill { | ||
| match Command::new(&kill.docker_bin) |
Comment on lines
+237
to
+244
| // SAFETY: `pid` belongs to the child we spawned as a new process-group | ||
| // leader. A negative pid asks kill(2) to signal that entire group. | ||
| unsafe { | ||
| libc::kill(-(pid as i32), libc::SIGKILL); | ||
| } | ||
| } else { | ||
| let _ = child.kill().await; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
coven-codeby killing only the immediate child, allowing background descendants to survive and retain repository tokens or resources.timeout_secssecurity guarantee and enabling cross-task leakage or resource exhaustion.Description
coven-codein its own Unix process group and terminate the whole group on timeout by adding aProcessGroupkill path and callinglibc::kill(-pid, SIGKILL)on Unix (no-op fallback on non-Unix). (crates/worker/src/backend.rs)KillTargetenum and updateawait_childto handle process-group termination for host runs and retain the existing container kill flow for container backend runs. (crates/worker/src/backend.rs)libcas a workspace dependency and enablelibcfor the worker crate so process-group signaling is available. (Cargo.toml,crates/worker/Cargo.toml)crates/worker/src/lib.rs)Signed-off-by:trailer per project policy; modified files:Cargo.toml,Cargo.lock,crates/worker/Cargo.toml,crates/worker/src/backend.rs, andcrates/worker/src/lib.rs.Testing
cargo check --all-targetsand it completed successfully.cargo clippy --all-targets -- -D warningsand it completed successfully with no warnings.NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost cargo test --allwhich passed; the focused testprocess_tests::coven_code_process_is_stopped_after_configured_timeoutwas also run and passed locally.cargo test --allinvocation in this environment failed due to an HTTP proxy / Wiremock routing issue (HTTP 403) unrelated to the change; re-running tests withNO_PROXYscoped to localhost produced all-green results.Codex Task