fix(eval): time out hung docker kill and compose down

[SebTardif]

What Problem This Solves

Native eval already bounds docker exec with asyncio.timeout. After that
timeout fires, cleanup still called run_process(["docker", "kill", ...])
with no deadline. stop() did the same for docker compose down and
docker rm -f.

An enclosing timeout only cancelled the await. run_process had already
started the Docker CLI child and never terminated it, so a hung docker
process leaked for every affected trial.

Evidence

Live python on this branch imported run_process and spawned a real
30-second child (python -c sleep). A 0.4s deadline cancelled the await
and the helper terminated the child.

$ python3 - <<'PY'
# run_process(["python", "-c", "print(pid); sleep(30)"])
# under asyncio.timeout(0.4)
timeout after 0.40s
child pid 13267
reaped
PY

The same helper is used for docker kill, docker compose down, and
docker rm -f.

Real behavior proof

• Behavior or issue addressed: Hung Docker CLI cleanup after an agent timeout leaked the child. run_process now terminates and reaps the subprocess when the enclosing deadline expires.

• Real environment tested: macOS, Python 3.14, branch fix/docker-cleanup-timeout at /tmp/shellbench-66.

• Exact steps or command run after this patch:

  python3 - <<'PY'
  import asyncio, os, sys, time
  from pathlib import Path
  from scripts.native_eval.runtime import run_process
  out = Path("/tmp/sb66-child.out")
  child = [sys.executable, "-c", "import os,time; print(os.getpid(), flush=True); time.sleep(30)"]
  async def main():
      t0 = time.monotonic()
      try:
          async with asyncio.timeout(0.4):
              await run_process(child, stdout_path=out, stderr_path=Path("/tmp/sb66-child.err"))
      except TimeoutError:
          print(f"timeout after {time.monotonic()-t0:.2f}s")
      pid = int(out.read_text().strip())
      print(f"child pid {pid}")
      try:
          os.kill(pid, 0)
          print("ALIVE")
      except OSError:
          print("reaped")
  asyncio.run(main())
  PY

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

  timeout after 0.40s
  child pid 13267
  reaped

• Observed result after fix: Control returns in 0.40s. The child PID is gone. After SIGKILL, wait() is also bounded (2s). A child stuck in uninterruptible I/O cannot pin the 30s cleanup deadline.

• What was not tested: A real dockerd hang on this machine. The live command uses a real long-lived child in place of a stuck Docker CLI.

What does this PR do?

Own the Docker CLI child inside run_process. On cancel or timeout,
terminate, then kill, and bound both wait() calls so a stuck child
cannot pin cleanup. Keep the 30s deadline around docker kill,
compose down, and docker rm -f.

Why?

Introduced in #42
(69f75c6629c4,
2026-07-29). Related wait hardening: #19.
Related 30s bound: #8.

Claw review on c1a5352 asked to terminate and reap the timed-out
client. Review on 636c2d4 asked to bound the wait after SIGKILL.

Changes

• _reap_process on TimeoutError / CancelledError in run_process
• 2s deadline on both terminate-wait and kill-wait
• 30s deadline still wraps the three cleanup call sites
• Real-child hang coverage (sleeping Python process, PID gone after timeout)
• No changelog edit (release-owned)

Tests

• python3 -m pytest -q tests/test_native_eval_runtime.py passes locally
• python3 -m ruff check / ruff format --check on the changed files

[clawsweeper]

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review in progress

ClawSweeper is reviewing this revision. This supersedes any previous blocked status.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 4, 2026, 3:54 PM ET / 19:54 UTC.

ClawSweeper review

What this changes

The PR bounds Docker cleanup commands and makes the native evaluator terminate, kill, and bounded-wait reap a cancelled subprocess so hung Docker clients cannot indefinitely stall a trial.

Merge readiness

⛔ Blocked before merge - 2 items remain

This remains a focused, proof-backed repair for native-evaluation teardown; no merged same-repository change is shown to supersede it, and the patch has no concrete correctness finding.

Priority: P2
Reviewed head: e7e75af5c7867765554b2c647d418ffac342cb15

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused reliability patch with direct live subprocess proof and targeted coverage; its deliberate bounded teardown latency is the remaining merge consideration.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The changed production owner is the native evaluator’s shared subprocess helper. The PR supplies an after-fix macOS terminal trace that runs that helper against a real sleeping Python child under an injected timeout, observes return at 0.40 seconds, and confirms the child PID is gone; the focused tests extend coverage to Docker cleanup call sites and a stuck post-kill wait.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The changed production owner is the native evaluator’s shared subprocess helper. The PR supplies an after-fix macOS terminal trace that runs that helper against a real sleeping Python child under an injected timeout, observes return at 0.40 seconds, and confirms the child PID is gone; the focused tests extend coverage to Docker cleanup call sites and a stuck post-kill wait.
Evidence reviewed	6 items	Introduced cleanup bounds: The PR wraps Docker kill, compose down, and Docker remove cleanup in a 30-second async deadline. Introduced subprocess reaping: The shared subprocess owner terminates a child on timeout or cancellation, escalates to kill after a two-second wait, and bounds the second wait as well. Focused regression coverage: New tests cover hung Docker cleanup calls, a real sleeping child that must disappear after cancellation, and a wait that remains stuck after kill.
Findings	None	None.
Security	None	None.

How this fits together

ShellBench’s native evaluator runs benchmark tasks in Docker containers and records trial output. Its Docker commands flow through a shared async subprocess helper, so cancellation handling determines whether a timed-out trial can complete teardown.

flowchart LR
  A[Benchmark trial] --> B[Docker task environment]
  B --> C[Async subprocess helper]
  C --> D[Docker CLI command]
  D --> E[Trial logs]
  C --> F[Cancellation and reaping]
  F --> G[Bounded teardown]

Loading

Before merge

• Resolve merge risk (P2) - A Docker CLI process that remains unkillable at the OS level can still hold a timeout path for roughly 34 seconds: the 30-second cleanup deadline plus up to two bounded two-second reaping waits.
• Resolve review confidence - ClawSweeper must reach high confidence before merge readiness is known.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed surface	2 files; production +55/-22, tests +148/-0	The production change is confined to the Docker cleanup and shared subprocess path, with focused regression coverage.

Merge-risk options

Maintainer options:

1. Accept the bounded cleanup budget (recommended)
   Merge with the documented maximum cleanup delay because it replaces an unbounded teardown stall with deterministic recovery.
2. Reduce the cleanup budget
   Choose shorter cleanup and reaping bounds only if benchmark operators require faster timeout recovery at the cost of less time for Docker to respond.

Technical review

Best possible solution:

Land the shared child-reaping fix while retaining the explicit bounded cleanup budget, so timed-out trials finish rather than leaving Docker client processes behind.

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

Yes. The supplied live trace uses the changed production subprocess helper with a real long-running child under a 0.4-second cancellation deadline, and the focused test preserves the same PID-reaping scenario.

Is this the best way to solve the issue?

Yes. Centralizing cancellation cleanup in the existing subprocess owner covers all three affected Docker cleanup callers without adding a parallel command runner.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

• P2: This is a bounded reliability repair for native benchmark trial teardown without evidence of a current widespread outage.
• merge-risk: 🚨 availability: The shared subprocess path controls how long timed-out Docker commands can delay a trial before recovery.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the native evaluator’s shared subprocess helper. The PR supplies an after-fix macOS terminal trace that runs that helper against a real sleeping Python child under an injected timeout, observes return at 0.40 seconds, and confirms the child PID is gone; the focused tests extend coverage to Docker cleanup call sites and a stuck post-kill wait.
• proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the native evaluator’s shared subprocess helper. The PR supplies an after-fix macOS terminal trace that runs that helper against a real sleeping Python child under an injected timeout, observes return at 0.40 seconds, and confirms the child PID is gone; the focused tests extend coverage to Docker cleanup call sites and a stuck post-kill wait.

Evidence

What I checked:

• Introduced cleanup bounds: The PR wraps Docker kill, compose down, and Docker remove cleanup in a 30-second async deadline. (scripts/native_eval/runtime.py:422, e7e75af5c786)
• Introduced subprocess reaping: The shared subprocess owner terminates a child on timeout or cancellation, escalates to kill after a two-second wait, and bounds the second wait as well. (scripts/native_eval/runtime.py:1302, e7e75af5c786)
• Focused regression coverage: New tests cover hung Docker cleanup calls, a real sleeping child that must disappear after cancellation, and a wait that remains stuck after kill. (tests/test_native_eval_runtime.py:59, e7e75af5c786)
• Feature history and main drift: The native runtime traces back to the merged native matrix runner, while current main later touched the same runtime in the merged execution-validity repair; neither history entry establishes that this PR’s cleanup behavior is already merged. (scripts/native_eval/runtime.py:1326, 69f75c6629c4)
• After-fix real behavior proof: The PR body records a macOS live run of the changed subprocess helper against a real 30-second Python child: the enclosing 0.4-second deadline returned and the recorded child PID was reaped. This directly exercises the production helper; the author notes a real dockerd hang was not exercised. (e7e75af5c786)
• Not merged as this PR head: The PR head is not contained by a local main branch or release tag; this is not proof that no equivalent implementation exists, but there is no verified merged fixing PR connection for closure. (e7e75af5c786)

Likely related people:

• Vincent Koc: 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)

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.

History

Review history (7 earlier review cycles)

• reviewed 2026-08-15T22:44:08.729Z sha c1a5352 :: needs real behavior proof before merge. :: [P2] Terminate and reap timed-out Docker clients | [P3] Leave the release-owned changelog unchanged
• reviewed 2026-08-20T04:19:59.997Z sha 636c2d4 :: needs changes before merge. :: [P2] Bound the wait after killing a child
• reviewed 2026-08-20T07:34:39.197Z sha e7e75af :: needs maintainer review before merge. :: none
• reviewed 2026-08-26T17:07:34.501Z sha e7e75af :: needs maintainer review before merge. :: none
• reviewed 2026-09-01T21:19:41.783Z sha e7e75af :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T08:35:01.991Z sha e7e75af :: blocked before merge. :: none
• reviewed 2026-09-04T16:54:12.713Z sha e7e75af :: blocked before merge. :: none

[SebTardif]

@clawsweeper re-review

Terminate and reap timed-out Docker clients

Done on 636c2d4. run_process now terminates/kills and waits on cancel. Live child 13267 was reaped after a 0.40s deadline.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[SebTardif]

@clawsweeper re-review

Bound the wait after killing a child

Done on e7e75af. Both terminate-wait and kill-wait use a 2s deadline. A stuck wait() after SIGKILL returns instead of pinning cleanup.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/32345552619
• Updated: 2026-08-20T07:46:36.242Z