Skip to content

fix(docker): remove POSIX shell cleanup commands from DockerEnvironment.cleanup - #29189

Closed
soynchux wants to merge 1 commit into
NousResearch:mainfrom
soynchux:fix/docker-cross-platform-cleanup-hardening
Closed

fix(docker): remove POSIX shell cleanup commands from DockerEnvironment.cleanup#29189
soynchux wants to merge 1 commit into
NousResearch:mainfrom
soynchux:fix/docker-cross-platform-cleanup-hardening

Conversation

@soynchux

Copy link
Copy Markdown
Contributor

Summary

Fix Docker environment cleanup so it no longer depends on POSIX shell syntax or shell interpolation.

DockerEnvironment.cleanup() previously launched cleanup with shell=True and command strings using timeout, sleep, background &, and shell redirects. That is fragile on native Windows shells and also breaks when the resolved Docker executable path contains spaces, such as Docker Desktop install paths.

This PR changes cleanup to use argv-based subprocess.run(...) calls from daemon threads, preserving the existing non-blocking behavior while avoiding shell parsing entirely.

Changes

  • Replace shell-string cleanup commands with list-argv Docker calls.
  • Keep docker stop non-blocking by running it in a daemon thread.
  • Preserve the fallback behavior: if docker stop fails or times out, force-remove the container with docker rm -f.
  • Preserve non-persistent cleanup behavior by scheduling delayed docker rm -f without shell sleep.
  • Update the Docker environment cleanup test to assert argv calls, including a Docker executable path with spaces.

Why

This improves cross-platform reliability and removes shell quoting risk in container cleanup. It is especially relevant for native Windows support and Docker Desktop paths where the Docker binary may live under a directory such as C:\Program Files\....

Testing

uv run --extra dev pytest tests\tools\test_docker_environment.py -q -n 0 --timeout-method=thread
23 passed

uv run --extra dev ruff check tools\environments\docker.py tests\tools\test_docker_environment.py
All checks passed!

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists backend/docker Docker container execution type/security Security vulnerability or hardening and removed type/bug Something isn't working labels May 20, 2026
@teknium1

Copy link
Copy Markdown
Contributor

This looks implemented on current main by a later Docker cleanup fix.

Automated hermes-sweeper review evidence:

  • tools/environments/docker.py:1262 now runs cleanup with list-argv subprocess.run([docker_exe, "stop", "-t", "10", container_id], ...) from a daemon thread.
  • tools/environments/docker.py:1271 does the removal/fallback path with list-argv subprocess.run([docker_exe, "rm", "-f", container_id], ...).
  • tests/tools/test_docker_environment.py:1161 covers the regression that cleanup must not use the old detached shell/Popen pattern.
  • Commit 5c2170a7c62b9cfd18431de78b462116df57d199 (fix(docker): persist-mode cleanup is no-op; add force_remove kwarg (#20561)) contains the implementation, and git tag --contains shows it shipped in v2026.6.5.

Thanks for the cleanup hardening PR; the same core issue is now covered on main.

@teknium1 teknium1 closed this Jun 20, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/docker Docker container execution P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants