fix: Implement proper cleanup of ACP agent child processes#51
Merged
Conversation
Fixes resource leak where ACP agent processes were being orphaned when AcpAgentRunner was dropped. ## Changes - Added Drop trait implementation for AcpAgentRunner (src/acp_runner.rs:362-403) - Uses libc::kill with SIGTERM to terminate processes synchronously - Includes tracing logs for process cleanup events - Unix-only implementation (uses #[cfg(unix)]) - Added agent_pid() method for testing (src/acp_runner.rs:265-269) - Returns PID of currently running agent process - Enables integration tests to verify actual process termination - Added libc = "0.2" dependency in Cargo.toml - Required for synchronous process termination - Avoids async runtime issues in Drop implementation - Created comprehensive integration tests (tests/acp_process_cleanup_test.rs) - test_process_cleanup_on_runner_drop: Verifies cleanup on drop - test_process_cleanup_on_reuse: Verifies old process killed when spawning new - test_process_cleanup_on_init_failure: Verifies cleanup on errors - Uses OS-level verification via kill -0 <pid> ## Technical Details Used libc::kill instead of tokio::process::Child::kill() because: - Drop must be synchronous - block_on doesn't work inside existing tokio runtime - Direct syscall provides reliable, synchronous cleanup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The test was passing even without the Drop implementation because the mock agent was completing quickly and exiting naturally. Updated the test to use MOCK_AGENT_STREAM_UNTIL_CANCEL environment variable, which makes the agent stream continuously until cancelled, preventing natural exit. ## Changes - Set MOCK_AGENT_STREAM_UNTIL_CANCEL=1 in test_process_cleanup_on_runner_drop - Increased sleep time to allow agent to start streaming - Test now properly fails without Drop implementation - Test passes with Drop implementation ## Verification Tested by temporarily disabling Drop impl - test correctly fails with: "Process PID 1397155 should be terminated after runner drop, but it's still running." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Summary
🤖 Generated with Claude Code
Technical Approach
The implementation uses
libc::killinstead oftokio::process::Child::kill()because Drop requires synchronous execution andblock_oncannot be called from within an existing tokio runtime.Test Plan
kill -0 <pid>Files Changed
src/acp_runner.rs: Added Drop impl and agent_pid() methodCargo.toml: Added libc dependencytests/acp_process_cleanup_test.rs: New integration testssrc/backends/docs.md: Updated documentationtests/docs.md: Updated test documentationShare Nori with your team: https://www.npmjs.com/package/nori-ai