Skip to content

Conversation

@simonrosenberg
Copy link
Collaborator

@simonrosenberg simonrosenberg commented Jan 2, 2026

Summary

  1. Refactor RemoteConversation.run() to use a single _wait_for_run_completion() call at the end instead of having two separate calls (one in the 409 path and one at the end).
  2. Remove defensive if:else

Fixes #1568

Problem

The original code had two if blocking: self._wait_for_run_completion(...) blocks:

  1. One inside the 409 (already running) path
  2. One at the end

While functionally correct, this duplication made the control flow harder to scan and invited mistaken "cleanup" changes.

Solution

Consolidated the wait logic into a single block at the end:

if resp.status_code == 409:
    logger.info("Conversation is already running; skipping run trigger")
else:
    logger.info(f"run() triggered successfully: {resp}")

if blocking:
    self._wait_for_run_completion(poll_interval, timeout)

Behavior Preserved

This refactor preserves the exact same behavior:

  • For blocking=False: returns immediately after logging (no wait)
  • For 409 already-running: logs "already running", then waits if blocking
  • For success: logs "triggered successfully", then waits if blocking
  • Error handling via ConversationRunError is unchanged

Testing

Added two new tests to verify the refactored behavior:

  • test_remote_conversation_run_wait_called_once_on_409: Verifies _wait_for_run_completion is called exactly once on 409 response
  • test_remote_conversation_run_wait_called_once_on_success: Verifies _wait_for_run_completion is called exactly once on success

All 1576 existing tests pass.

@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

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:5ad6791-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-5ad6791-python \
  ghcr.io/openhands/agent-server:5ad6791-python

All tags pushed for this build

ghcr.io/openhands/agent-server:5ad6791-golang-amd64
ghcr.io/openhands/agent-server:5ad6791-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:5ad6791-golang-arm64
ghcr.io/openhands/agent-server:5ad6791-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:5ad6791-java-amd64
ghcr.io/openhands/agent-server:5ad6791-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:5ad6791-java-arm64
ghcr.io/openhands/agent-server:5ad6791-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:5ad6791-python-amd64
ghcr.io/openhands/agent-server:5ad6791-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:5ad6791-python-arm64
ghcr.io/openhands/agent-server:5ad6791-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:5ad6791-golang
ghcr.io/openhands/agent-server:5ad6791-java
ghcr.io/openhands/agent-server:5ad6791-python

About Multi-Architecture Support

  • Each variant tag (e.g., 5ad6791-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 5ad6791-python-amd64) are also available if needed

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/conversation/impl
   remote_conversation.py47116265%69–75, 82–85, 114, 121, 129, 131–134, 144, 153, 157–158, 163–166, 201, 215, 232, 243, 252–253, 305, 325, 333, 345, 353–356, 359, 364–367, 369, 374–375, 380–384, 389–393, 398–401, 404, 415–416, 420, 424, 427, 514–515, 519–520, 528, 534, 536, 552–553, 558, 560–561, 572, 589–590, 594, 600–601, 605, 610–611, 616–618, 621–625, 627–628, 632, 634–642, 644, 648, 663, 681, 716, 718, 721, 747, 767–768, 775, 784, 787, 793–805, 808–809, 818–819, 828, 836, 841–843, 845, 848, 850–851, 871, 873, 879–880, 895, 902, 908–909, 924, 937, 943–944, 951–952
TOTAL14609691052% 

@simonrosenberg simonrosenberg force-pushed the openhands/refactor-remote-conversation-run-1568 branch 2 times, most recently from 3375249 to 09e7769 Compare January 2, 2026 07:30
…ation.run()

Refactor run() to use a single _wait_for_run_completion() call at the end
instead of having two separate calls (one in the 409 path and one at the end).

This simplifies the control flow while preserving the exact same behavior:
- For blocking=False: returns immediately after logging (no wait)
- For 409 already-running: logs 'already running', then waits if blocking
- For success: logs 'triggered successfully', then waits if blocking
- Error handling via ConversationRunError is unchanged

Also removes unnecessary defensive try-except in _get_last_error_detail().

Fixes #1568

Co-authored-by: openhands <openhands@all-hands.dev>
@simonrosenberg simonrosenberg force-pushed the openhands/refactor-remote-conversation-run-1568 branch from 09e7769 to 48117fe Compare January 2, 2026 07:33
@simonrosenberg simonrosenberg changed the title refactor: consolidate duplicated blocking wait logic in RemoteConversation.run() refactor: consolidate duplicated blocking wait logic in RemoteConversation.run() + remove defensive if else Jan 2, 2026
Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

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

Thank you! Grateful for any bit of cleaner code! ❤️

@simonrosenberg simonrosenberg merged commit cbc8cfb into main Jan 2, 2026
21 checks passed
@simonrosenberg simonrosenberg deleted the openhands/refactor-remote-conversation-run-1568 branch January 2, 2026 13:43
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.

remote_conversation.run: avoid duplicated blocking wait logic

4 participants