fix(docker): container reuse + bounded-sync cleanup + orphan reaper (#20561)

[benbarclay]

Closes #20561.

Problem

The Docker backend docs claim:

Single persistent container — ONE long-lived container shared across sessions, /new, /reset, and delegate_task subagents. Stopped/removed on shutdown.

In practice the code only honored that contract within a single Python process via the in-memory _active_environments[task_id] cache. Every hermes chat invocation spawned a fresh hermes-<hex> container, and three separate bugs let the old containers pile up in Exited state indefinitely:

1. No cross-process container discovery. No labels, no docker ps probe — the only signal that a container belonged to hermes-agent was the hermes-<hex> name prefix, and nothing in the code used it.
2. cleanup() was fire-and-forget. subprocess.Popen("... &", shell=True) raced with parent-process exit — when Python exited promptly (or got SIGKILL'd), the detached shell child got killed mid-docker stop, leaving stopped containers behind.
3. The docker rm step in cleanup was gated on not self._persistent. Default config sets container_persistent: true, so the default happy path skipped rm entirely. The result: stopped containers from every prior hermes chat session, forever, until manual docker container prune.

Fix — three commits, configurable, each one solves one root cause

Commit 1 — tag containers with hermes-agent labels for identification

Adds --label hermes-agent=1 --label hermes-task-id=<sanitized> --label hermes-profile=<sanitized> to docker run. Pure metadata change, zero behavioral change. Sets up the next two commits.

Labels are sanitized to [A-Za-z0-9_.-] (≤63 chars) so profile/task names containing slashes, colons, or unicode round-trip cleanly through docker ps --filter label=key=value. Empty/non-string inputs collapse to "unknown".

Commit 2 — reuse containers across processes + fix cleanup leaks

• Cross-process reuse. New DockerEnvironment.__init__(persist_across_processes=True) (default on, matching the documented contract). Probes docker ps -a --filter label=hermes-task-id=<task> --filter label=hermes-profile=<profile>. Match found → if running, attach; if stopped, docker start then attach; on docker start failure, falls through to a fresh docker run. Multiple matches prefer the running one (stragglers left for the reaper). Reuse matches on labels only — image / mounts / resources are not compared. Operators who change those settings should set the kill-switch or docker rm -f the labeled container.
• Cleanup rewrite. Replaces subprocess.Popen("... &", shell=True) with subprocess.run(..., timeout=30) on a daemon threading.Thread. Drops the _persistent guard on rm — rm now runs whenever persist_across_processes is false, regardless of the bind-mount-persistence setting. New wait_for_cleanup(timeout) method that tools/terminal_tool.py's atexit hook calls on every active env (15s timeout), so docker stop/rm actually completes before the interpreter exits.
• New config terminal.docker_persist_across_processes (default true). Set false for hard per-process isolation (no reuse, container removed on exit). Wired through all four config-bridge sites (cli.py env_mappings, gateway/run.py _terminal_env_map, hermes_cli/config.py _config_to_env_sync, tests/conftest.py env-strip list) and pinned by test_docker_persist_across_processes_is_bridged_everywhere.

Commit 3 — startup orphan reaper for crashed-process containers

The cleanup-fix in commit 2 handles graceful exits. It doesn't help SIGKILL / OOM / terminal-window-close exits that bypass atexit — the leaked container has no surviving Python process to reuse or remove it.

This commit adds the safety net:

• New reap_orphan_containers() in tools/environments/docker.py. Sweeps label=hermes-agent=1 + status=exited containers older than the age threshold. The status=exited filter is load-bearing — a running container may belong to a sibling Hermes process whose reuse path will pick it up; killing it would crash the sibling mid-command. Parses docker inspect State.FinishedAt (with nanosecond-precision trimming for Python's microsecond-bound fromisoformat); zero-value / unparseable timestamps cause the candidate to be skipped (conservative).
• New _maybe_reap_docker_orphans() helper in tools/terminal_tool.py. Gated by terminal.docker_orphan_reaper (default true), a once-per-process flag with double-checked locking (parallel subagents don't trigger N concurrent docker ps storms), and scoped to the current profile (a research profile NEVER reaps default's stragglers). Age threshold is 2 × TERMINAL_LIFETIME_SECONDS with a 60s floor.

Configuration summary

Two new keys, both default-on, both with kill-switches:

terminal:
  docker_persist_across_processes: true  # set false → per-process isolation
  docker_orphan_reaper: true             # set false → no startup sweep

Set either via hermes config set or in ~/.hermes/config.yaml. Both auto-bridge to env vars (TERMINAL_DOCKER_PERSIST_ACROSS_PROCESSES, TERMINAL_DOCKER_ORPHAN_REAPER) and propagate through CLI and gateway entry points.

Behavior change

Default behavior changes with this PR. Before: each hermes chat invocation created a fresh container; old containers accumulated forever. After: each invocation reuses the previously-stopped container (matching the documented "ONE long-lived container shared across sessions"); a startup sweep removes containers from crashed prior processes after 2×lifetime_seconds.

Users who want the old per-process behavior can set terminal.docker_persist_across_processes: false. That path now also fully removes containers on exit (the gating bug that previously prevented docker rm is fixed).

Tests

22 new unit tests + 6 new integration tests + 2 new bridge-invariant regression tests across:

• tests/tools/test_docker_environment.py (was 23 tests, now 48)
  • 5 label tests (commit 1)
  • 5 cross-process reuse tests (commit 2)
  • 8 cleanup-correctness tests (commit 2)
  • 9 orphan-reaper tests (commit 3)
• tests/tools/test_docker_orphan_reaper_integration.py (new, 6 tests) — terminal_tool-side gates: once-per-process behavior, disable-flag respected, lifetime doubling with 60s floor, current-profile filter wiring, exception swallow
• tests/tools/test_terminal_config_env_sync.py (was 6, now 8) — new bridge-invariant regression pins for both config keys, same pattern as existing docker_run_as_host_user / docker_env pins

Full suite of tests/tools/ (~5615 tests across 231 files) — 0 failures.

Implementation notes for reviewers

• tools/environments/docker.py is backend/docker (cross-lane), so this needs Teknium review per hermes-agent-dev scope rule. Flagging @teknium1.
• The cleanup-thread wait_for_cleanup join is bounded at 15s per env in the atexit hook. Worst-case shutdown delay = 15s × number of active envs; in practice 1–2 envs and most cleanups complete in <1s.
• Reuse intentionally does NOT compare image / mounts / resources. The failure mode if a user changes those without flipping the kill-switch is "attached to old container with stale config" — clean and obvious. We could compare them and force-recreate on mismatch in a follow-up, but I'd rather keep the probe cheap and let the operator choose.
• Container labels are immutable post-creation. A user who renames their profile mid-stream won't reuse their old profile's containers — the reaper will eventually clean them up (subject to the same-profile scoping; cross-profile cleanup would need a --force-reap-all flag).

Commits

ff82a8111 fix(docker): startup orphan reaper for crashed-process containers
d09396208 fix(docker): reuse containers across processes + fix cleanup leaks
d66df58ad fix(docker): tag containers with hermes-agent labels for identification

[github-actions]

🔎 Lint report: fix/docker-container-leak-20561 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 9578 on HEAD, 9577 on base (🆕 +1)

🆕 New issues (2):

Rule	Count
invalid-argument-type	1
invalid-assignment	1

First entries

tests/tools/test_docker_environment.py:605: [invalid-argument-type] invalid-argument-type: Argument to function `_sanitize_label_value` is incorrect: Expected `str`, found `None`
tests/gateway/test_agent_cache.py:1149: [invalid-assignment] invalid-assignment: Object of type `(tid) -> None` is not assignable to attribute `cleanup_vm` of type `def cleanup_vm(task_id: str, *, force_remove: bool = False) -> Unknown`

✅ Fixed issues (1):

Rule	Count
invalid-assignment	1

First entries

tests/gateway/test_agent_cache.py:1149: [invalid-assignment] invalid-assignment: Object of type `(tid) -> None` is not assignable to attribute `cleanup_vm` of type `def cleanup_vm(task_id: str) -> Unknown`

Unchanged: 5044 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

[benbarclay]

E2E verified against real Docker daemon

Ran an end-to-end harness against the real docker binary (29.5.1) and python:3.11-slim. All three commits work in practice:

Commit 1 — labels

• docker inspect confirms all three labels on the live container with correct values
• Container is actually usable (execute('echo X') returns the right output with exit 0)

Commit 2 — cross-process reuse + cleanup

• cleanup() in persist mode stops the container without removing it (verified via docker ps -a)
• A second DockerEnvironment(task_id=X) constructed AFTER cleanup attaches to the same container ID
• The reused container is functional (second execute() works)
• persist_across_processes=False cleanly removes the container on cleanup (docker ps -a shows it gone)

Commit 3 — orphan reaper

• reap_orphan_containers(max_age_seconds=3600) against a just-stopped container returns 0 removed (conservative window respected)
• reap_orphan_containers(max_age_seconds=0) removes the same container (sweep works when age threshold is met)
• reap_orphan_containers(profile_filter='some-other-profile') returns 0 removed and leaves our profile's container intact (profile scoping respected)

Test artifact

Harness is at /tmp/e2e_docker_reuse.py on my dev box — happy to commit it as tests/integration/test_docker_reuse_e2e.py (gated on docker availability) if reviewers want the regression coverage. Not in this PR because the unit test suite already pins the behavior with mocks and an always-on Docker-requiring integration test is awkward to run in CI.

[benbarclay]

Commit 4 (8efba9c4a): persist-mode cleanup is a no-op for the container

Ben caught a design problem in the first three commits: persist-mode cleanup() still issued docker stop (only skipping docker rm), which contradicts the documented "ONE long-lived container shared across sessions" promise — stopping the container on every Hermes /quit kills any in-container background processes (npm watchers, pytest watchers, long-running scripts).

Changes

• Persist mode (default) — cleanup() is now a complete no-op for the container. Container stays running. Background processes survive across /quit. Next Hermes process attaches via the existing label probe in ~ms (no docker start needed). Resource reclamation happens via the orphan reaper at next startup (2 × lifetime_seconds threshold), which covers SIGKILL / OOM / abandoned-laptop.
• Opt-out mode (persist_across_processes=False) — unchanged: docker stop + docker rm -f on cleanup as before.
• Explicit teardown — new cleanup(force_remove=True) kwarg overrides persist mode and tears the container down unconditionally. cleanup_vm(task_id) now defaults to force_remove=True since it's the user-driven reset path (called from AIAgent.close() and the per-turn cleanup branch for non-persistent envs).

E2E verification (against real Docker daemon)

Two harnesses, 18 checks total — all green.

Persist-mode no-op behavior (new):

✓ Container is still running after cleanup()
✓ Background process (sleep loop) survived cleanup()
✓ Filesystem state preserved across cleanup()
✓ In-process container_id cleared (next __init__ will re-probe)
✓ Background process visible from reused env (no docker start happened)
✓ force_remove=True removed the container even in persist mode
✓ cleanup_vm() removed the container (defaults to force_remove=True)

Original 3-commit regression checks (still green):

✓ All three labels present on real container
✓ execute() works in the labeled container
✓ Container still running after persist-mode cleanup() (no-op)
✓ Cross-process reuse worked — same container ID
✓ persist_across_processes=False removed the container on cleanup
✓ Reaper spared the running container (sibling-process safety)
✓ Reaper spared the young Exited container
✓ Reaper removed the Exited container with max_age=0
✓ Reaper respected profile_filter (did not touch other profile)

Test updates

• Replaces test_cleanup_with_persist_only_stops_no_rm with test_cleanup_with_persist_is_noop_for_container — asserts neither stop nor rm runs in persist mode, and the in-process handle is still cleared so re-probe works.
• Adds test_cleanup_force_remove_stops_and_rms_even_in_persist_mode — covers the new kwarg.
• Updates test_cleanup_uses_subprocess_run_not_detached_shell and test_wait_for_cleanup_after_cleanup_returns_true to pass force_remove=True so they exercise the actual docker code path (default no-op would trivially pass otherwise).

Net: 62/62 tests passing in the docker-environment + orphan-reaper + config-bridge files.

cleanup_vm forwarding

cleanup_vm() forwards force_remove only to backends whose cleanup() accepts the kwarg (currently just DockerEnvironment) via runtime inspect.signature — Modal/Daytona/SSH cleanup signatures are unchanged, no risk of breaking other backends.

[benbarclay]

Commit 5 (bfc253439): cleanup_vm() default honors persist mode

Ben caught a follow-up bug from commit 4 during real TUI testing: the container was correctly shared between TUI sessions, but killed as soon as any session closed.

Root cause: commit 4 set cleanup_vm()'s force_remove default to True. That function is called from THREE session-lifecycle paths:

• tui_gateway/server.py:2991 — session.close JSON-RPC → agent.close() → cleanup_vm
• gateway/run.py:3569 — gateway session teardown
• agent/chat_completion_helpers.py:1517 — per-turn cleanup branch for non-persistent envs

I conflated "explicit user reset" with "session-end teardown". They're not the same — session close should honor persist mode.

Fix

Flipped cleanup_vm()'s force_remove default back to False. The kwarg remains for future explicit-teardown paths (/reset-style flows, "destroy my sandbox" commands) that haven't been wired up yet.

New unit tests

• test_cleanup_vm_default_honors_persist_mode — asserts cleanup_vm(task_id) does neither docker stop nor docker rm on a persist-mode container (regression pin for Ben's bug).
• test_cleanup_vm_force_remove_tears_down_persist_container — asserts the kwarg still flows through the runtime-signature-inspection plumbing into the backend's cleanup().

64/64 tests passing in the docker-environment + orphan-reaper + config-bridge files (+2 new).

E2E verified

New assertions added to the commit-4 harness:

✓ Default cleanup_vm() leaves persist-mode container running
✓ cleanup_vm(force_remove=True) removed the container

Plus all 17 prior checks across both E2E harnesses still green.