Fix container not removed when container_persistent=false (Fixes #1679)#1691
Closed
crazywriter1 wants to merge 4 commits intoNousResearch:mainfrom
Closed
Fix container not removed when container_persistent=false (Fixes #1679)#1691crazywriter1 wants to merge 4 commits intoNousResearch:mainfrom
crazywriter1 wants to merge 4 commits intoNousResearch:mainfrom
Conversation
…Research#1679) When terminal.container_persistent is false, cleanup() now runs 'docker rm -f' after the inner stop so the container is removed instead of left as Exited. Added test to assert docker rm -f is called. Made-with: Cursor
…ainer and upstream execute/dotenv tests Made-with: Cursor
…047576) Made-with: Cursor
setup_model_provider() now calls _setup_tts_provider(); tests that mock prompt_choice either no-op _setup_tts_provider or add one more choice for 'Select TTS provider:' (Keep current). Made-with: Cursor
Contributor
|
Merged via PR #1712. Your fix (docker rm -f on cleanup when container_persistent=false) was cherry-picked onto current main with authorship preserved. Also included your test fixes for model_metadata and setup tests. Thanks for the clean, well-tested contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1679
When
terminal.container_persistentisfalse, Docker containers were only stopped by the inner mini-swe-agent cleanup and left as "Exited", so they were never removed.Changes
tools/environments/docker.pyIn
cleanup(), whencontainer_persistentis false and we have a container ID, we now rundocker rm -f <container_id>after the inner cleanup (which only doesdocker stopin the background). Uses a 30s timeout; failures are logged and we clear the stored container ID.tests/tools/test_docker_environment.pyAdded
test_non_persistent_cleanup_removes_container: with a mocked inner Docker andpersistent_filesystem=False, we assert thatcleanup()triggers asubprocess.runcall withdocker rm -f <container_id>.Testing
pytest tests/tools/test_docker_environment.pypasses (including the new test).terminal.backend: dockerandcontainer_persistent: falsein config, run a terminal command via Hermes, thendocker ps -a— the container should no longer appear after cleanup.Note: Commits after the first one are merge conflict resolution and CI fixes
(model metadata + setup/TTS tests) so the branch passes main’s tests. The
behavioral fix for #1679 is only in the first commit (docker cleanup + test).