fix: reap timed-out execution-check process groups

[SebTardif]

What Problem This Solves

ExecutionCheck.shell defaults to True. Official public tasks such as
t1-bugfix-discount (pytest -q) run through
asyncio.create_subprocess_shell. When timeout_seconds expires,
run_execution_check called process.kill() on the shell only, then
await process.communicate() with no timeout.

A child that still holds stdout or stderr (a hung pytest worker, a
verify script that spawned a helper) keeps those pipes open. Scoring
never returns.

Space EvalWorker and clawbench run both score through
scorer.score_task_run -> verify_completion -> run_execution_check.
One timed-out check wedges the job.

The same timeout handler exists in both clawbench/environment.py and
clawbench/environment_files.py. Background services already start a
new session and killpg the group (clawbench/services.py). Execution
checks did not.

Evidence

Live python on this branch compared the old kill-then-unbounded
communicate() path against the patched run_execution_check on both
public runners. The command was a shell-launched Python parent that
spawns a grandchild sleep inheriting the pipes.

$ python C:\Users\sebta\AppData\Local\Temp\proof-f006.py C:\Users\sebta\AppData\Local\Temp\f006-proof
OLD PATTERN
old-pattern communicate still blocked after 5.00s (pipes held by child)
FIXED gateway run_execution_check
gateway passed=False exit_code=-1 reason='Timed out after 1s' elapsed=1.14s child_pid=32900
FIXED files run_execution_check
files passed=False exit_code=-1 reason='Timed out after 1s' elapsed=1.11s child_pid=7736

The old pattern stayed blocked for the full 5s bound after process.kill().
Both patched runners returned Timed out after 1s in about 1.1s.

Real behavior proof

• Behavior or issue addressed: A timed-out execution check could hang scoring because process.kill() only stopped the shell and communicate() waited forever while children held the pipes. The check now starts a process group, signals the group, and bounds communicate() so verify_completion returns.

• Real environment tested: Windows 11, Python 3.13.15, checkout C:\Users\sebta\.grok\tmp\pr-gate-batch\shellbench-f006 on fix/timeout-reap-group.

• Exact steps or command run after this patch:

  python C:\Users\sebta\AppData\Local\Temp\proof-f006.py C:\Users\sebta\AppData\Local\Temp\f006-proof

• Evidence after fix: terminal output from the live command:

  OLD PATTERN
  old-pattern communicate still blocked after 5.00s (pipes held by child)
  FIXED gateway run_execution_check
  gateway passed=False exit_code=-1 reason='Timed out after 1s' elapsed=1.14s child_pid=32900
  FIXED files run_execution_check
  files passed=False exit_code=-1 reason='Timed out after 1s' elapsed=1.11s child_pid=7736

• Observed result after fix: Scoring returned a timeout result in 1.14s and 1.11s instead of blocking on communicate(). Exit code is -1 and the reason is Timed out after 1s. The old kill-only path was still blocked after 5.00s.

• What was not tested: A live Hugging Face Space EvalWorker scoring t1-bugfix-discount against a hung pytest worker.

What does this PR do?

Reap timed-out execution-check process groups and bound
process.communicate() so scoring can return.

Why?

This hang was introduced in 2e39d5cc (2026-04-09, 149 days ago) and
copied into environment_files.py by #17
(56531fbf, 2026-04-29). Related timeout work:
#66 (docker cleanup),
#77 (fleet inspect/warmup),
#80 (lane prepare hook).
None of those cover execution-check timeout reap.

Python documents that Popen.kill() signals only that process
(docs).
A shell-spawned child that inherits the pipes keeps communicate()
blocked until those writers exit.

Changes

• Start execution-check subprocesses with start_new_session=True on POSIX
• On timeout, signal the process group (killpg on POSIX, taskkill /T on Windows)
• Bound the follow-up communicate() with asyncio.wait_for

Tests

• tests/test_execution_shell_rendering.py::test_execution_check_timeout_reaps_shell_child_process_group
  starts a real shell child that holds the pipes, requires scoring to
  return a timeout result, and on POSIX checks the grandchild is gone
• python -m ruff check clawbench app.py scripts tests passes locally

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 5, 2026, 4:30 AM ET / 08:30 UTC.

ClawSweeper review

What this changes

The PR adds shared process-tree cleanup and a bounded output drain to both execution-check runners, with a subprocess regression test.

Merge readiness

⛔ Blocked before merge - 5 items remain

Keep open: current main still contains the timeout hang, and the supplied live evidence demonstrates useful recovery, but three focused correctness repairs remain.

Priority: P2
Reviewed head: aa4492637bc9168574aba6a9ac0d89943f27cff1

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful real-process recovery evidence supports the fix, but cleanup edge cases and a faulty regression assertion prevent merge readiness.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.
Patch quality	🦐 gold shrimp (3/6)	3 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.
Evidence reviewed	10 items	Verified repository and introduced patch: Origin identifies openclaw/shellbench. The local head matches the pinned PR head, and the inspected base-to-head diff contains exactly the three supplied files. Current main still needs the fix: Both main-branch runners kill only the immediate process and then await communicate without another deadline. Scoring reaches this handler through verify_completion. POSIX cleanup depends on a surviving leader: The patch starts a new session but looks up its group through the leader PID at timeout. Once that leader has exited and been reaped, lookup fails even while descendants retain the output pipes.
Findings	3 actionable findings	[P2] [P2] Signal the original group after its leader exits [P2] [P2] Bound Windows tree termination outside the event loop [P2] [P2] Distinguish terminated zombies from running descendants
Security	None	None.

How this fits together

ShellBench runs task-defined commands to verify an agent’s work, then incorporates their results into benchmark scores. Timeout cleanup must stop those commands and return control so scoring can continue.

flowchart TD
  A[Task verification command] --> B[Execution-check runner]
  B --> C[Shell or direct subprocess]
  C --> D{Completes before deadline?}
  D -->|Yes| E[Evaluate output]
  D -->|No| F[Terminate children and drain output]
  E --> G[Completion score]
  F --> G

Loading

Before merge

• [P2] Signal the original group after its leader exits (P2) - If a shell starts a background child that inherits the pipes and then exits, asyncio can reap the shell before the communication timeout fires. os.getpgid(process.pid) then raises even though the child’s process group still exists. The fallback only targets the vanished shell, so the new bounded drain eventually returns a timeout while the child continues running. Since start_new_session=True establishes the group ID as the spawned PID, retain and signal that identity directly; add a leader-exits-first regression for both runners.
• [P2] Bound Windows tree termination outside the event loop (P2) - This synchronous subprocess.run() executes inside the async timeout handler with no deadline. If taskkill stalls, the event-loop thread blocks before the later wait_for is installed, preventing other evaluations and cancellation timers from progressing too. Run tree termination asynchronously with a bounded lifecycle and retain a bounded fallback for the original process.
• [P2] Distinguish terminated zombies from running descendants (P2) - Group SIGKILL terminates the Python parent and grandchild, but asyncio only waits for its direct subprocess. The orphaned grandchild can still be a zombie when this assertion runs, and os.kill(pid, 0) succeeds for zombies. This makes the new test race with the system reaper and fail persistently under a non-reaping container init even when termination succeeds. Use a bounded liveness check that distinguishes zombie state, or arrange deterministic reaping in the fixture.
• Resolve merge risk (P2) - The supplied trace does not cover stalled Windows tree termination or an already-exited POSIX shell; those paths can freeze parallel work or leave commands running after a timeout result.
• Complete next step (P2) - Repair group targeting after shell exit, bound Windows cleanup without blocking the event loop, and correct the zombie-sensitive liveness assertion.

Findings

• [P2] [P2] Signal the original group after its leader exits — clawbench/environment_files.py:118-125
• [P2] [P2] Bound Windows tree termination outside the event loop — clawbench/environment_files.py:107-112
• [P2] [P2] Distinguish terminated zombies from running descendants — tests/test_execution_shell_rendering.py:203-205

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	Production +61/-6 lines (net +55); tests +75/-0	The production growth implements shared cross-platform cleanup for two existing runners, with one regression scenario parameterized across both.

Merge-risk options

Maintainer options:

1. Complete bounded process cleanup (recommended)
   Preserve the spawned POSIX group identity, bound Windows termination without blocking the event loop, and repair the process-liveness regression test.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Fix the shared execution-check cleanup helper to signal the original POSIX process group even after its leader exits and to bound Windows taskkill asynchronously, including cleanup of that helper process on timeout. Make the regression assertion distinguish terminated zombies from running descendants, and cover both runner entrypoints.

Technical review

Best possible solution:

Keep both verifier entrypoints on one bounded cleanup path that terminates owned descendants after their shell exits and remains responsive on Windows.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a shell-launched descendant retaining stdout or stderr past the check deadline leaves main’s kill-then-communicate path waiting indefinitely. The supplied Windows comparison supports this mechanism; this review did not execute a current-main reproduction.

Is this the best way to solve the issue?

Yes in approach: shared process-group termination and bounded draining address the existing failure without adding configuration, but leader-exit handling, Windows cleanup, and the liveness assertion need correction.

Full review comments:

• [P2] [P2] Signal the original group after its leader exits — clawbench/environment_files.py:118-125
  If a shell starts a background child that inherits the pipes and then exits, asyncio can reap the shell before the communication timeout fires. os.getpgid(process.pid) then raises even though the child’s process group still exists. The fallback only targets the vanished shell, so the new bounded drain eventually returns a timeout while the child continues running. Since start_new_session=True establishes the group ID as the spawned PID, retain and signal that identity directly; add a leader-exits-first regression for both runners.
  Confidence: 0.98
• [P2] [P2] Bound Windows tree termination outside the event loop — clawbench/environment_files.py:107-112
  This synchronous subprocess.run() executes inside the async timeout handler with no deadline. If taskkill stalls, the event-loop thread blocks before the later wait_for is installed, preventing other evaluations and cancellation timers from progressing too. Run tree termination asynchronously with a bounded lifecycle and retain a bounded fallback for the original process.
  Confidence: 0.97
• [P2] [P2] Distinguish terminated zombies from running descendants — tests/test_execution_shell_rendering.py:203-205
  Group SIGKILL terminates the Python parent and grandchild, but asyncio only waits for its direct subprocess. The orphaned grandchild can still be a zombie when this assertion runs, and os.kill(pid, 0) succeeds for zombies. This makes the new test race with the system reaper and fail persistently under a non-reaping container init even when termination succeeds. Use a bounded liveness check that distinguishes zombie state, or arrange deterministic reaping in the fixture.
  Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against c1a79f731541.

Labels

Label changes:

• add P2: This repairs a concrete scoring hang triggered by timed-out commands with surviving descendants, without evidence of an urgent widespread outage.
• add merge-risk: 🚨 availability: The newly added unbounded synchronous Windows cleanup can stall the shared event loop before its timeout guard starts.
• add proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.

Label justifications:

• P2: This repairs a concrete scoring hang triggered by timed-out commands with surviving descendants, without evidence of an urgent widespread outage.
• merge-risk: 🚨 availability: The newly added unbounded synchronous Windows cleanup can stall the shared event loop before its timeout guard starts.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.
• proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows terminal trace exercises both changed production runners with real pipe-holding descendants and observes timeout recovery in 1.14s and 1.11s versus an old path blocked for 5s; the identified POSIX and Windows edge cases remain correctness gaps.

Evidence

Acceptance criteria:

• [P1] python -m pytest -q tests/test_execution_shell_rendering.py tests/test_services.py.
• [P2] python -m pytest -q tests/test_execution_shell_rendering.py -k timeout.
• [P1] python -m pytest -q.
• [P1] python -m ruff check clawbench app.py scripts tests.

What I checked:

• Verified repository and introduced patch: Origin identifies openclaw/shellbench. The local head matches the pinned PR head, and the inspected base-to-head diff contains exactly the three supplied files. (aa4492637bc9)
• Current main still needs the fix: Both main-branch runners kill only the immediate process and then await communicate without another deadline. Scoring reaches this handler through verify_completion. (clawbench/environment.py:172, c1a79f731541)
• POSIX cleanup depends on a surviving leader: The patch starts a new session but looks up its group through the leader PID at timeout. Once that leader has exited and been reaped, lookup fails even while descendants retain the output pipes. (clawbench/environment_files.py:119, aa4492637bc9)
• Windows termination precedes the bounded await: The new synchronous taskkill invocation has no timeout and runs on the event-loop thread before the bounded communicate call. (clawbench/environment_files.py:108, aa4492637bc9)
• Regression assertion confuses zombies with running children: The test immediately expects signal-zero probing of the grandchild to fail, although successful group termination does not synchronously reap that orphaned grandchild. (tests/test_execution_shell_rendering.py:203, aa4492637bc9)
• Supplied real-process recovery evidence: The full supplied PR body, in context sourceRevision 65ab0e140d9c5ef553adfd473171b96ae8e51cb1477b8bcb84f53ad87c315830, reports Windows 11/Python 3.13.15 calls through both production runners against shell-launched pipe-holding descendants. They returned timeout results in 1.14s and 1.11s; the old pattern remained blocked for 5s. This is runtime evidence, not merely a test assertion. (aa4492637bc9)

Likely related people:

• scoootscooob: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Cover a POSIX shell that exits before its pipe-holding descendant and verify descendant termination.
• Exercise bounded Windows termination while another coroutine continues to make progress.
• Make the regression test robust to asynchronous orphan reaping.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.