Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Docker runtimes not stopping #6470

Merged
merged 6 commits into from
Jan 27, 2025
Merged

Conversation

tofarr
Copy link
Collaborator

@tofarr tofarr commented Jan 27, 2025

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below
    no changelog

Give a summary of what the PR does, explaining any non-trivial design decisions

This PR fixes an issue where Docker runtime containers were not being properly stopped when the application shuts down. The changes include:

Changes

  1. Enhanced Shutdown Listener System:

    • Added a new shutdown listener mechanism in shutdown_listener.py that works better with Starlette/Uvicorn shutdown signals
    • Replaced the atexit handler with the new shutdown listener system
    • Added UUID-based listener registration and removal functionality
  2. Docker Runtime Improvements:

    • Removed global _atexit_registered flag and stop_all_runtime_containers function
    • Added class-level _shutdown_listener_id to track the shutdown listener
    • Containers are now properly stopped when the application shuts down
  3. Added Comprehensive Tests:

    • New test file test_shutdown_listener.py with thorough test coverage
    • Tests for listener registration, removal, and shutdown behavior
    • Tests for edge cases like removing listeners during shutdown

Technical Details

  • The new shutdown listener system uses a dictionary to track listeners by UUID
  • Signal handlers are properly chained to preserve existing handlers
  • Listeners are called exactly once during shutdown
  • Added safeguards against recursive shutdown calls

This change ensures that Docker containers are properly cleaned up when the application exits, preventing orphaned containers from consuming system resources.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:26f53e2-nikolaik   --name openhands-app-26f53e2   docker.all-hands.dev/all-hands-ai/openhands:26f53e2

@tofarr tofarr marked this pull request as ready for review January 27, 2025 17:13
@tofarr tofarr merged commit ffdab28 into main Jan 27, 2025
13 checks passed
@tofarr tofarr deleted the fix-docker-runtimes-not-stopping branch January 27, 2025 18:09
zchn pushed a commit to zchn/OpenHands that referenced this pull request Feb 4, 2025
idagelic pushed a commit to idagelic/OpenHands that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants