fix(env): atexit cleanup for DockerEnvironment - #881
Conversation
…) idempotent The cleanup() method was only called via __del__, which is not guaranteed on abnormal exit. Add atexit registration to ensure Docker containers are cleaned up. - Add atexit.register in __init__ - Add _cleaned flag for idempotent cleanup - Add tests for cleanup idempotency
for more information, see https://pre-commit.ci
klieret
left a comment
klieret
left a comment
There was a problem hiding this comment.
Oh, you're right, this is better! But should we remove the __del__ then?
| cmd = f"(timeout 60 {self.config.executable} stop {self.container_id} || {self.config.executable} rm -f {self.container_id}) >/dev/null 2>&1 &" | ||
| subprocess.Popen(cmd, shell=True) | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
Pull request overview
This PR hardens DockerEnvironment resource cleanup by ensuring container teardown runs on normal interpreter exit (via atexit) and making cleanup calls idempotent to avoid double-cleanup across atexit and __del__.
Changes:
- Register
DockerEnvironment.cleanup()withatexitduring initialization. - Add a
_cleanedflag to makecleanup()idempotent. - Add slow integration tests validating repeated
cleanup()calls and_cleanedflag behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/minisweagent/environments/docker.py | Registers cleanup() with atexit and adds _cleaned idempotency guard for container teardown. |
| tests/environments/test_docker_cleanup.py | Adds slow integration tests to validate idempotent cleanup and _cleaned state changes. |
| if self._cleaned: | ||
| return | ||
| self._cleaned = True | ||
| if getattr(self, "container_id", None) is not None: |
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
OK so the bigger problem here is that right now basically we rely on python garbage collection to clean up docker containers. Once the env is GCd |
Problem
The
DockerEnvironment.cleanup()method was only called via__del__, which is not guaranteed to execute on abnormal exit (Ctrl+C, interpreter crash). This could leave Docker containers running as zombie processes.Solution
Register
atexit.register(self.cleanup)in__init__, which guarantees execution on normal Python exit. Add a_cleanedflag to makecleanup()idempotent for the case where bothatexitand__del__fire.Changes
src/minisweagent/environments/docker.py: Addatexit.register, add_cleanedidempotency flagtests/environments/test_docker_cleanup.py: New tests for cleanup idempotencyTesting
test_cleanup_idempotent: Verifies multiplecleanup()calls do not raisetest_cleaned_flag: Verifies_cleanedis set after cleanupNotes for Reviewer
atexitis stdlib, no new dependencies_cleanedflag protects against the atexit +__del__double-call edge case