Fix - Make Remote runtime use /start for both new and resume
- [ ] This change is worth documenting at https://docs.all-hands.dev/
- [ ] Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below
End-user friendly description of the problem this fixes or functionality this introduces.
Summarize what the PR does, explaining any non-trivial design decisions.
Simplify remote runtime by calling the /start endpoint on both new runtimes and resumes, since runtime-api can handle both cases this was duplicating the server-side logic.
We've recently seen trouble resuming Remote runtimes in OpenHands Cloud, which may be related to multiple being created in a race condition, so the hope is to improve consistency by letting runtime-api decide which existing runtime to use if any.
An error associated is
Error connecting to conversation <ID>: Could not find existing runtime for SID: <ID>"
Logged from here: https://github.com/All-Hands-AI/OpenHands/blob/2693360ad026e6e0f04b4de6d23b026e3cfc0d55/openhands/runtime/impl/remote/remote_runtime.py#L121
This will ignore the attach_to_existing option on create (not on close). That was added in #4329 (Oct 2024) and I don't think it currently adds anything useful on RemoteRuntime create, but @tofarr and @rbren may have more context.
Link of any specific issues this addresses:
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:364c571-nikolaik --name openhands-app-364c571 docker.all-hands.dev/all-hands-ai/openhands:364c571
Looks like there are a few issues preventing this PR from being merged!
- GitHub Actions are failing:
- Run Python Unit Tests
If you'd like me to help, just leave a comment, like
@OpenHands please fix the failing actions on PR #8564
Feel free to include any additional details that might help me get this PR into a better state.
You can manage your notification settings
We have merge conflicts now and I think it's best to redo this after Nested Runtimes is in.