fix(server): use asyncio.sleep instead of time.sleep in sandbox create#489
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c529d9f1c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR removes event-loop blocking during Kubernetes sandbox creation by making readiness polling fully asynchronous and awaiting sandbox creation from the API layer.
Changes:
- Convert
create_sandbox/_wait_for_sandbox_readytoasync defand replacetime.sleep()withawait asyncio.sleep(). - Update the lifecycle API endpoint to
awaitsandbox creation. - Update unit tests to await async service methods.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/src/services/k8s/kubernetes_service.py | Makes sandbox readiness polling non-blocking via asyncio.sleep and awaits readiness in create_sandbox. |
| server/src/services/sandbox_service.py | Changes the abstract create_sandbox contract to async. |
| server/src/api/lifecycle.py | Awaits the now-async sandbox_service.create_sandbox. |
| server/tests/k8s/test_kubernetes_service.py | Updates tests to run under asyncio and await service methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
skyler.su seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This PR addresses event-loop blocking during Kubernetes sandbox creation by making the sandbox creation path fully async and replacing blocking sleeps with non-blocking asyncio.sleep.
Changes:
- Convert
SandboxService.create_sandbox(and implementations) toasync defand update call sites toawaitit. - Update Kubernetes sandbox readiness polling to use
await asyncio.sleep(...)during polling. - Update affected unit tests to be async (
pytest.mark.asyncio) and to useAsyncMockwhere needed.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/services/k8s/kubernetes_service.py | Makes _wait_for_sandbox_ready / create_sandbox async and switches polling sleep to await asyncio.sleep. |
| server/src/services/sandbox_service.py | Updates the abstract service interface to require an async create_sandbox. |
| server/src/services/docker.py | Updates Docker service create_sandbox signature to async to match the interface. |
| server/src/api/lifecycle.py | Fixes the endpoint to await sandbox_service.create_sandbox(...). |
| server/tests/k8s/test_kubernetes_service.py | Converts tests to async and updates calls to await. |
| server/tests/test_docker_service.py | Converts relevant tests to async and awaits create_sandbox; updates mocking to AsyncMock where appropriate. |
| server/tests/test_routes_create_delete.py | Updates route test stub service to provide an async create_sandbox. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Great fix! Please sign the CLA and run |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Pangjiping
left a comment
There was a problem hiding this comment.
LGTM. I will fix unit test error.
Summary
_wait_for_sandbox_readyinkubernetes_service.pyusedtime.sleep()inside anasync defcall chain, which blocks the entire asyncio event loop thread during each poll interval.With a 3-pod pool and 3 concurrent requests, each request was ~2s slower than a single request because the polls were effectively serialized.
With 4 concurrent requests (exceeding pool size), all 4 timed out — the 4th request's blocking sleep held the event loop for the entire timeout duration, preventing responses to the first 3 from being sent.
Fix: make
_wait_for_sandbox_readyandcreate_sandboxasync def, replacetime.sleepwithawait asyncio.sleep, and add the missingawaitinlifecycle.py.Testing
Breaking Changes
Checklist