fix(detection): kill leaked subprocess on task timeout, remove dead scan

[Rahul-s-007]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Related issue: Closes #18

What changed?
Two related fixes in Detection/main_benchmark.py, TaskExecutor:

1. _execute_command now kills and reaps (process.kill() + await process.wait()) the claude CLI subprocess when asyncio.wait_for times out, instead of leaving it running. asyncio.wait_for only cancels the await, not the child — this mirrors the kill-on-timeout behavior subprocess.run(..., timeout=...) already provides for the synchronous CLI invocation in guardrail/adr_agent/adr_baseline.py.
2. Removed the dead existing_sessions computation in execute_ads_task — a full recursive rglob("*.jsonl") over the host-wide, ever-growing ~/.claude/projects/ directory, run once per task, whose result was passed into _process_results but never referenced there (confirmed via grep). Removed the now-unused parameter too.

Why?
At benchmark scale (hundreds of tasks per the README's 303 tasks / 133 MCP servers), every timeout leaked a process tree, and the dead scan added wasted I/O to every single task regardless of whether it timed out.

How did you test it?
Added TestTaskExecutorExecuteCommand to tests/test_main_benchmark.py (previously no coverage for this method): timeout-kill behavior, the success path, and non-zero-exit handling. To confirm the timeout test actually catches the original bug (not just passing trivially), I temporarily reverted only the fix, kept the new test, and reran — it failed with the leaked process's returncode still None after the call, then passed again once the fix was restored.

Ran pytest tests/test_main_benchmark.py -v:

13 passed — click to expand

============================= test session starts ==============================
platform darwin -- Python 3.14.0, pytest-9.1.1, pluggy-1.6.0 -- /private/tmp/adr_detection_venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/test4/Desktop/OSS/Uber ADR/Detection
configfile: pyproject.toml
plugins: asyncio-1.4.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 13 items

tests/test_main_benchmark.py::TestConfig::test_default_max_concurrent_tasks PASSED [  7%]
tests/test_main_benchmark.py::TestConfig::test_disallowed_tools_loaded_from_config PASSED [ 15%]
tests/test_main_benchmark.py::TestCommandBuilder::test_builds_claude_command PASSED [ 23%]
tests/test_main_benchmark.py::TestCommandBuilder::test_adds_permission_bypass_flag PASSED [ 30%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_range PASSED [ 38%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_csv_and_range PASSED [ 46%]
tests/test_main_benchmark.py::TestTaskManager::test_validate_task_requires_fields PASSED [ 53%]
tests/test_main_benchmark.py::TestMCPServerManager::test_create_mcp_config_writes_workspace_file PASSED [ 61%]
tests/test_main_benchmark.py::TestMCPServerManager::test_process_arg_template_replaces_workspace_path PASSED [ 69%]
tests/test_main_benchmark.py::TestConcurrencyGuard::test_rejects_zero_concurrency PASSED [ 76%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_kills_process_on_timeout PASSED [ 84%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_returns_parsed_json_on_success PASSED [ 92%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_reports_nonzero_exit_without_leaving_error_message_empty PASSED [100%]

============================== 13 passed in 0.17s ==============================

Potential risks
Low. process.kill() is only reached in the timeout branch, which previously did nothing to the process — this can only reduce leaked processes, not change any success-path behavior. _process_results' signature change has a single call site (verified by grep), updated in the same commit.

[CLAassistant]

All committers have signed the CLA.

[pengyuzhang]

Thanks for this — and for issue #18, which was accurate on both counts. I verified the PR beyond the included unit tests, and the results support the fix but surfaced one behavior that I think needs a small change before merge.

Verified locally (macOS, Python 3.12): 56 tests pass (incl. the 3 new ones); the dead existing_sessions scan is fully removed (no dangling references, single _process_results call site updated); clean merge against current main.

Differential harness. I ran the same scenario against main and this branch: TaskExecutor._execute_command with a 2s timeout, where the child spawns a grandchild subprocess (stand-in for an MCP server), then probing both PIDs after the call returns.

variant	returns after	child alive	grandchild alive
main	2.0s	yes (leak — issue #18 confirmed)	yes
PR #20, grandchild inherits stdio	120s	no ✅	no (died naturally at 120s)
PR #20, grandchild stdio → devnull	2.0s	no ✅	yes (leaked)

Two findings:

1. The kill works, but doesn't reach grandchildren. process.kill() signals the claude CLI only; anything it spawned survives (devnull row). That's the "and any MCP servers it spawned" half of issue #18 — still open after this PR. Not a regression (the subprocess.run(timeout=) pattern in adr_baseline.py has the same limitation), but worth fixing while we're here.

2. await process.wait() can block far past the timeout — potentially forever. asyncio's subprocess transport doesn't report completion until the stdio pipes close, and a grandchild that inherited the child's stdout/stderr holds them open. In the harness this blocked _execute_command for the grandchild's full 120s lifetime; a long-lived orphan would block indefinitely, wedging one of the max_concurrent slots. main never had this failure mode (it leaked, but returned promptly). The new unit tests can't catch it because the test child spawns no grandchild.

Severity caveat on (2): MCP servers spawned over stdio transport get their own pipes and typically don't inherit the CLI's stdout, so the hang needs a descendant with inherited stdio (e.g. a backgrounded shell command run by the agent). Plausible in a benchmark executing arbitrary agent actions, not guaranteed.

Suggested change — bound the wait, and sweep the whole process group so both findings are closed at once:

process = await asyncio.create_subprocess_exec(
    *cmd, ..., start_new_session=True)
...
except asyncio.TimeoutError:
    if process is not None and process.returncode is None:
        try:
            os.killpg(process.pid, signal.SIGTERM)   # reaches MCP servers too
        except ProcessLookupError:
            pass
        try:
            await asyncio.wait_for(process.wait(), timeout=5)
        except asyncio.TimeoutError:
            try:
                os.killpg(process.pid, signal.SIGKILL)
            except ProcessLookupError:
                pass
    return False, "Task execution timed out", None

start_new_session=True puts the CLI and all its descendants in one process group; SIGTERM first gives the CLI a chance to shut its MCP servers down cleanly, with SIGKILL escalation after 5s. Killing the group also closes the inherited pipe FDs, which makes the indefinite wait() impossible. A test for the grandchild case would be a good addition — same spying pattern as test_kills_process_on_timeout, with the test child spawning its own sleep subprocess.

Happy to re-verify with the harness once updated — it's quick to re-run. The existing_sessions removal half of the PR is ready as-is.

[Rahul-s-007]

Thanks for the thorough review and for actually running a differential harness, that's exactly the kind of verification I should've done myself for the descendant-process case. Implemented your suggested fix as-is (start_new_session=True, SIGTERM followed by a bounded 5s wait, then SIGKILL escalation via os.killpg), with one small addition: it falls back to killing just the direct child when os.killpg isn't available (e.g. Windows), since start_new_session/os.killpg are POSIX-only and I didn't want the function to raise on a platform where process groups aren't a thing. Everything else matches your suggestion exactly.

Added test_kills_grandchild_process_holding_inherited_stdio, reproducing your scenario: a child that spawns its own child without redirecting stdout/stderr, so the grandchild inherits the same pipes asyncio set up for the direct child. It asserts _execute_command returns promptly (not hanging toward the grandchild's full sleep duration) and that both the direct child and the grandchild are actually gone afterward, polling via os.kill(pid, 0) rather than a single check, since a not-yet-reaped zombie would otherwise still respond to that (false negative right after sending the kill signal).

Pushed as a new commit (c1a08f3) rather than amending, per the PR template. Full suite, 14 passed:

============================= test session starts ==============================
platform darwin -- Python 3.14.0, pytest-9.1.1, pluggy-1.6.0 -- /private/tmp/adr_detection_venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/test4/Desktop/OSS/Uber ADR/Detection
configfile: pyproject.toml
plugins: asyncio-1.4.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 14 items

tests/test_main_benchmark.py::TestConfig::test_default_max_concurrent_tasks PASSED [  7%]
tests/test_main_benchmark.py::TestConfig::test_disallowed_tools_loaded_from_config PASSED [ 14%]
tests/test_main_benchmark.py::TestCommandBuilder::test_builds_claude_command PASSED [ 21%]
tests/test_main_benchmark.py::TestCommandBuilder::test_adds_permission_bypass_flag PASSED [ 28%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_range PASSED [ 35%]
tests/test_main_benchmark.py::TestTaskManager::test_filter_tasks_by_csv_and_range PASSED [ 42%]
tests/test_main_benchmark.py::TestTaskManager::test_validate_task_requires_fields PASSED [ 50%]
tests/test_main_benchmark.py::TestMCPServerManager::test_create_mcp_config_writes_workspace_file PASSED [ 57%]
tests/test_main_benchmark.py::TestMCPServerManager::test_process_arg_template_replaces_workspace_path PASSED [ 64%]
tests/test_main_benchmark.py::TestConcurrencyGuard::test_rejects_zero_concurrency PASSED [ 71%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_kills_process_on_timeout PASSED [ 78%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_kills_grandchild_process_holding_inherited_stdio PASSED [ 85%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_returns_parsed_json_on_success PASSED [ 92%]
tests/test_main_benchmark.py::TestTaskExecutorExecuteCommand::test_reports_nonzero_exit_without_leaving_error_message_empty PASSED [100%]

============================== 14 passed in 0.62s ==============================

Ready for re-verification against your harness whenever you get a chance.

[pengyuzhang]

Approving — re-verified c1a08f3 with the same differential harness from the previous review. Both findings are closed:

scenario	main	previous rev	this rev
pipe-inheriting grandchild — return time	2s	120s (blocked)	2.0s ✅
pipe-inheriting grandchild — leaked	child+grandchild	none	none ✅
devnull grandchild — return time	2s	2s	2.0s ✅
devnull grandchild — leaked	child+grandchild	grandchild	none ✅

Prompt return at the timeout and a fully swept process tree, in both stdio configurations.

The implementation covers the details I'd have asked about: platform-gated start_new_session, SIGTERM → bounded 5s wait → SIGKILL escalation, both ProcessLookupError races handled, Windows fallback documented, and the comment block accurately explains the pipe-hold/transport mechanism for future readers. uv run pytest tests/ -q → 57 passed; the new grandchild-stdio test pins exactly the scenario the earlier revision couldn't see.

Known residual (non-blocking, same as main): a descendant that double-forks into its own session escapes killpg; nothing at this layer can catch that, and return remains prompt either way. Windows fallback path is untested here (macOS/Linux only) but is identical to the previous revision's behavior.

Thanks for the fast, complete turnaround — including the regression test.