-
Notifications
You must be signed in to change notification settings - Fork 108
Fix bash command polling to use async sleep in async context #1662
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
base: main
Are you sure you want to change the base?
Conversation
The bash command polling loop was using time.sleep(0.1) which blocks the event loop in async contexts. This caused the polling to stop prematurely after only 2 attempts, leading to agent loops hanging indefinitely. Changes: - Modified _execute_command_generator to yield sleep requests instead of calling time.sleep() directly - Updated sync _execute method in RemoteWorkspace to handle sleep requests with time.sleep() - Updated async _execute method in AsyncRemoteWorkspace to handle sleep requests with asyncio.sleep() - Added test for async sleep handling - Updated existing polling tests to reflect new generator behavior Fixes #1633 Co-authored-by: openhands <[email protected]>
|
@OpenHands |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
Code Review Summary for PR #1662🟡 Taste Rating: Acceptable - Works but could be cleanerThe PR correctly fixes the bug (Issue #1633) where bash command polling stopped after 2 attempts due to Critical FeedbackMain Issue: Magic String Protocol The solution uses Recommended Improvement: from dataclasses import dataclass
@dataclass(frozen=True)
class SleepRequest:
duration: float
# In generator:
yield SleepRequest(POLL_INTERVAL_SECONDS)
# In executor:
if isinstance(request, SleepRequest):
await asyncio.sleep(request.duration)
request = generator.send(None)
else:
response = await self.client.request(**request)
request = generator.send(response)This approach is:
Verdict✅ Worth merging - The fix solves the real problem and doesn't break anything. The tests properly cover the new behavior. However, I'd recommend refactoring to use a proper Key Insight: The solution works but introduces an implicit protocol (magic |
neubig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this but would probably want to defer to an expert on that one.
| return client | ||
|
|
||
| async def _execute(self, generator: Generator[dict[str, Any], httpx.Response, Any]): | ||
| async def _execute(self, generator: Generator[dict[str, Any], Any, Any]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general Any is not great, should this be httpx.Response | None?
| return client | ||
|
|
||
| def _execute(self, generator: Generator[dict[str, Any], httpx.Response, Any]): | ||
| def _execute(self, generator: Generator[dict[str, Any], Any, Any]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
|
switching back to draft because swebench failed |
Summary
Fixes #1633
The bash command polling loop was using
time.sleep(0.1)which blocks the event loop in async contexts. This caused the polling to stop prematurely after only 2 attempts, leading to agent loops hanging indefinitely with 20-minute idle timeouts.Root Cause
The
_execute_command_generatormethod inRemoteWorkspaceMixinused synchronoustime.sleep(0.1)between polling attempts. In async contexts (when usingAsyncRemoteWorkspace), this blocked the event loop, causing issues with the HTTP client and preventing proper polling continuation.Solution
Modified the generator to yield sleep requests instead of calling
time.sleep()directly. The_executemethods in both sync and async workspace implementations now handle these sleep requests appropriately:RemoteWorkspace): Usestime.sleep()for sleep requestsAsyncRemoteWorkspace): Usesawait asyncio.sleep()for sleep requestsThis ensures the polling loop continues correctly in both sync and async contexts without blocking the event loop.
Changes
openhands-sdk/openhands/sdk/workspace/remote/remote_workspace_mixin.py:_execute_command_generatorto yield{"_sleep": duration}requests instead of callingtime.sleep()directlyPOLL_INTERVAL_SECONDSconstant for polling interval configurationopenhands-sdk/openhands/sdk/workspace/remote/base.py:_executemethod to handle sleep requests withtime.sleep()openhands-sdk/openhands/sdk/workspace/remote/async_remote_workspace.py:_executemethod to handle sleep requests withawait asyncio.sleep()Testing
test_async_execute_handles_sleep_requeststo verify async sleep handling@simonrosenberg can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:657cb46-pythonRun
All tags pushed for this build
About Multi-Architecture Support
657cb46-python) is a multi-arch manifest supporting both amd64 and arm64657cb46-python-amd64) are also available if needed