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 for issue where retries continue on a closed runtime #6564

Merged
merged 9 commits into from
Feb 3, 2025
Merged

Conversation

tofarr
Copy link
Collaborator

@tofarr tofarr commented Jan 31, 2025

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

This PR fixes an issue where retry attempts would continue even after a runtime was closed, which could lead to unnecessary resource usage and potential errors.

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

The PR makes the following changes:

  1. Adds a _stop_if_closed method to the RemoteRuntime class that checks if the runtime is closed
  2. Integrates this check into the retry logic for both _wait_until_alive and _send_action_server_request methods
  3. Improves error logging in the standalone conversation manager by adding stack traces for better debugging

Key changes:

  • Added self._stop_if_closed to the stop conditions in retry decorators
  • Enhanced error logging in _cleanup_stale method to include stack traces

This change ensures that retry attempts are properly terminated when a runtime is closed, preventing unnecessary resource usage and potential errors from retrying operations on a closed runtime.

Link of any specific issues this addresses

N/A


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:aa87fe2-nikolaik   --name openhands-app-aa87fe2   docker.all-hands.dev/all-hands-ai/openhands:aa87fe2

@tofarr tofarr marked this pull request as ready for review January 31, 2025 16:27
@tofarr tofarr requested a review from rbren January 31, 2025 20:24
@tofarr tofarr requested a review from xingyaoww February 3, 2025 14:27
Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM and I just pushed a commit that add remote_runtime_enable_retries to all eval scripts

@tofarr tofarr merged commit bbfdc62 into main Feb 3, 2025
14 checks passed
@tofarr tofarr deleted the fix-fd-leaks branch February 3, 2025 15:44
zchn pushed a commit to zchn/OpenHands that referenced this pull request Feb 4, 2025
…I#6564)

Co-authored-by: Xingyao Wang <xingyao6@illinois.edu>
adityasoni9998 pushed a commit to adityasoni9998/OpenHands that referenced this pull request Feb 7, 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