Why use `||` between `stop` and `rm` when cleanup a docker environment?

[hankeke303]

Describe the bug

Whenever I finish running mini-swe-agent, it consistently leaves behind a large number of exited Docker containers.
This appears to be because the commands within the cleanup method of the DockerEnvironment class use the logical OR operator (||) between docker stop and docker rm, resulting in the containers not being removed after their usage is complete.
Is this the intended behavior?

mini-swe-agent/src/minisweagent/environments/docker.py

Lines 153 to 157 in 58ad92f

	def cleanup(self):
	"""Stop and remove the Docker container."""
	if getattr(self, "container_id", None) is not None: # if init fails early, container_id might not be set
	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)

[klieret]

oops, that might be my bad

[klieret]

OK, so the container should have --rm in its run args (see line 30 of the file), so even if we just stop, it should be removed (so I'm somewhat surprised that you have so many left over, let me check if I can reproduced)

[klieret]

So I think the above line should read docker stop .. || docker kill .. and then cleaning up should still happen automatically because of the --rm run flag.

Happy to merge a PR if you create one today!

I prefer to have the removing being controlled by the run_args config setting and then make the cleanup behavior to just stop, because it might sometimes be beneficial to keep stopped containers for debugging after something went wrong.

[hankeke303]

Oh, I'm sorry! That's my bad.

I configured the run_args parameter when launching the Docker environment, resulting in the --rm flag being overwritten.

I am very sorry for wasting your time.

[klieret]

I still think this actually is a tiny bug (like if you remove --rm, then the force killed containers are actually removed because of the ||), I'm gonna fix that at some point

[gauravdhiman]

I also see so many minisweagent-** and hermes-** in exited state, they are not getting removed automatically.

[jane-jhu]

Hi, can I take this issue and open a small PR for the cleanup logic?