Skip to content

fix(docker): make terminal cleanup remove containers reliably - #20565

Open
LeonSGP43 wants to merge 2 commits into
NousResearch:mainfrom
LeonSGP43:fix/docker-cleanup-stop-20561
Open

fix(docker): make terminal cleanup remove containers reliably#20565
LeonSGP43 wants to merge 2 commits into
NousResearch:mainfrom
LeonSGP43:fix/docker-cleanup-stop-20561

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • make Docker environment cleanup stop and remove containers synchronously, with docker rm -f as the verified success point
  • keep terminal environments tracked when cleanup reports failure so Hermes does not forget live containers and create duplicates
  • have classic CLI /stop also clean active terminal environments, not only background process registry entries

Addresses #20561.

Verification

  • scripts/run_tests.sh tests/tools/test_docker_environment.py tests/tools/test_terminal_environment_cleanup.py tests/cli/test_stop_command_cleanup.py -> 26 passed, 4 existing warnings
  • python -m py_compile tools/environments/docker.py tools/terminal_tool.py cli.py tests/tools/test_terminal_environment_cleanup.py tests/cli/test_stop_command_cleanup.py
  • git diff --check

Notes

  • ruff check on the touched runtime files still reports pre-existing import-order findings in cli.py and tools/terminal_tool.py; no new lint-only cleanup is included here to keep the PR focused.

@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 tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels May 6, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #20569 — both address #20561 (Docker container cleanup on /stop). This PR additionally handles terminal_tool tracking on cleanup failure.

@LeonSGP43

LeonSGP43 commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Verification update after af02e6d (test: remove trailing blank line in stop cleanup test) on 2026-05-07.

  • git diff --check origin/main...HEAD -> clean
  • scripts/run_tests.sh tests/tools/test_docker_environment.py tests/tools/test_terminal_environment_cleanup.py tests/cli/test_stop_command_cleanup.py -> 26 passed, 4 existing warnings
  • python -m py_compile tools/environments/docker.py tools/terminal_tool.py cli.py tests/tools/test_terminal_environment_cleanup.py tests/cli/test_stop_command_cleanup.py -> clean

Current CI state on af02e6d:

  • check-attribution, Scan PR for critical supply chain risks, e2e, and both nix jobs are passing.
  • ruff + ty diff is red because the workflow's PR-comment step hit 403 Resource not accessible by integration. The generated summary reported 0 new Ruff issues, and the ty diagnostics in that workflow are warning-only.
  • The fresh Tests / test rerun finished red, but the failing set is still broad and unrelated to the touched Docker cleanup/stop paths. I did not reproduce a failure in tests/tools/test_docker_environment.py, tests/tools/test_terminal_environment_cleanup.py, or tests/cli/test_stop_command_cleanup.py from this diff.

The earlier diff-check failure on tests/cli/test_stop_command_cleanup.py is resolved by af02e6d.

@LeonSGP43

LeonSGP43 commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Status: HER-22 verification at repaired head.

  • Current head: af02e6db5cb97b5f7d69b09d940b1554a297e077.
  • Local verification: git diff --check origin/main...HEAD; uv run --python 3.12 --with pytest-xdist pytest tests/cli/test_stop_command_cleanup.py tests/tools/test_docker_environment.py tests/tools/test_terminal_environment_cleanup.py -q => 26 passed.
  • Remaining red ruff + ty diff is blocked_external: the workflow fails in its PR-comment step with GitHub 403 Resource not accessible by integration.
  • Remaining red test job is upstream_baseline: 33 unrelated merge-with-main failures across bedrock, cron, browser, delegate, dockerfile, skill provenance, and environment-wrapper areas, not the repaired blank-line diff boundary.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cleanup work and regression tests. Current main resolved #20561 through a broader Docker persistence design, so this patch cannot be applied as-is.

Problems

  • tools/environments/docker.py:1389-1437 intentionally leaves the default cross-process container running; tests/tools/test_docker_environment.py:958-1004 locks in no docker stop or docker rm for that mode. The PR's unconditional removal direction would undo the documented shared-container/background-process contract.
  • Classic /stop is currently scoped to background processes and async delegations in hermes_cli/cli_commands_mixin.py:228-259. Extending it to terminal teardown needs an explicit current-contract decision, including how it interacts with docker_persist_across_processes.

Suggested changes

Automated hermes-sweeper review.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have and removed P2 Medium — degraded but workaround exists labels Jul 12, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Re-triage note: the container-accumulation bug (#20561) this PR addresses was resolved on main via a broader Docker-persistence design (commit 5c2170a7c), per the maintainer sweeper verdict (keep_open salvageability=low). The remaining live scope — whether classic /stop should tear down terminal environments — needs an explicit persist-mode contract decision, so labeling needs-decision and demoting to P3. Not a duplicate (competing #20569 and issue #20561 are both closed) — linking as related.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 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 comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants