Skip to content

fix(docker): add wait parameter to cleanup and call from /stop command - #20569

Closed
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-20561-docker-cleanup
Closed

fix(docker): add wait parameter to cleanup and call from /stop command#20569
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-20561-docker-cleanup

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

  • Docker containers accumulate and are not reliably cleaned up when using the /stop command.

Root Cause

The /stop command (cli.py) only calls process_registry.kill_all() to kill background processes, but it does not call environment cleanup methods. Additionally, DockerEnvironment.cleanup() uses fire-and-forget async cleanup (subprocess.Popen with &), which:

  • Does not wait for cleanup to complete
  • Loses tracking of container state
  • Allows containers to accumulate across sessions

Related Issue

Fixes #20561

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

1. tools/environments/docker.py

  • Added wait: bool = False parameter to cleanup() method
  • When wait=True: uses subprocess.run() to synchronously stop and remove containers
  • When wait=False (default): preserves original async behavior
  • This allows callers to choose sync vs async cleanup

2. cli.py

  • Added _cleanup_environments() helper method
  • Updated _handle_stop_command() to call cleanup on all environment objects after killing processes
  • Passes wait=True to ensure containers are actually removed before /stop returns

3. tests/tools/test_docker_cleanup_stop.py

  • Added regression tests for the new wait parameter
  • Verifies that cleanup(wait=True) uses subprocess.run() (sync)
  • Verifies that cleanup(wait=False) uses subprocess.Popen() (async)

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

…INSTALL_TIMEOUT

Increase the default npm install timeout for WhatsApp bridge from 60s
to 300s (5 minutes) to accommodate slower systems like Unraid NAS.
Make it configurable via WHATSAPP_NPM_INSTALL_TIMEOUT environment variable
for users who need even longer timeouts.

Closes NousResearch#14980
- Add 'path', 'old_string', 'new_string', and 'patch' to required list
- Update description to clarify mode-specific parameter requirements
- This addresses issue where LLMs would omit these parameters because
  they were not marked as required in the schema, even though they
  are required depending on the mode

Fixes NousResearch#15524
- Add `wait` parameter to DockerEnvironment.cleanup() for synchronous cleanup
- Update /stop command to call cleanup(wait=True) on all environments
- Fixes orphaned Docker containers that accumulate across sessions
- Prevents zombie containers by ensuring cleanup completes on /stop

Tests: tests/tools/test_docker_cleanup_stop.py
@liuhao1024
liuhao1024 force-pushed the fix/issue-20561-docker-cleanup branch from 974462e to 3872687 Compare May 6, 2026 06:11
@alt-glitch alt-glitch added type/bug Something isn't working backend/docker Docker container execution comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels May 6, 2026
@benbarclay

Copy link
Copy Markdown
Collaborator

Thanks for this, @liuhao1024 — closing it, but the real half is carried forward into a focused issue, not dropped. Reviewing against current main, this PR splits into two parts:

Cleanup-synchronization half — now superseded. The wait parameter + replacing the fire-and-forget Popen(... &) cleanup was the right diagnosis, but #33645 (3c6e70ae) has since landed exactly this: bounded subprocess.run cleanup + a wait_for_cleanup() method + an atexit hook that waits for teardown. So that part is now redundant.

/stop wiring half — still a real, live gap. #33645 fixed the crash/idle leak paths but did not touch _handle_stop_command(), which on current main still only calls process_registry.kill_all() and never cleans up Docker environments. I confirmed this against main. I've filed #39489 to track that focused fix (wire cleanup_all_environments() into /stop), crediting you for surfacing it.

Two more reasons to close rather than rebase: this PR also bundled unrelated changes to gateway/platforms/whatsapp.py and tools/file_tools.py, and it's currently conflicting against main. Cleaner to land the /stop fix fresh and scoped via #39489. Note also that the issue it cited, #20561, was closed as completed (the container-accumulation root cause was fixed by #33645's reaper + atexit path).

Closing in favor of #39489 — thanks for getting the /stop leak on the radar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/docker Docker container execution comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Docker containers accumulate - /stop doesn't clean, multiple per session, async race

3 participants