Repository navigation
feat: provision ADR-0019 cloud sessions - #115
Merged
Merged
Conversation
Expose provider provisioning and cleanup hooks so HTTP session creation can provision remote workspaces through a shared registry contract. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add docker-backed workspace provisioning and teardown so server-managed cloud sessions create runnable containers and wait for cleanup to finish before returning. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Allow HTTP session creation to request docker-backed cloud workspaces, persist the binding origin, and roll back provisioned resources on endpoint failures. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Persist HTTP session origin metadata so provisioned cloud bindings can be distinguished from explicit bindings and cleaned up across close and clear paths. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
👮 Files not reviewed due to content moderation or server errors (6)
📝 WalkthroughWalkthroughAdds server-side cloud workspace provisioning and cleanup: new provider protocol methods and helpers, Docker provider container lifecycle for provision/cleanup, HTTP server and SessionManager wiring to provision workspaces on session creation and track origin, and tests + API exports for provisioning helpers and types. ChangesCloud Workspace Provisioning
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant HTTP as HTTPServer
participant SM as SessionManager
participant WP as WorkspaceProvider
participant Docker as Docker
Client->>HTTP: POST /sessions (workspace_source: docker)
HTTP->>HTTP: _provisioned_execution_binding_from_request()
HTTP->>WP: provision_cloud_binding_from_config(config, source)
WP->>Docker: docker run (mount workspace, start container)
Docker-->>WP: container started
WP-->>HTTP: CloudWorkspaceBinding
HTTP->>SM: create_session(..., binding=CloudWorkspaceBinding, origin=...)
SM-->>HTTP: session created
Client->>HTTP: DELETE /sessions/{id}
HTTP->>SM: remove_session(id)
SM->>SM: detect provisioned binding
SM->>WP: cleanup_cloud_binding_from_config(config, binding)
WP->>Docker: docker rm -f
Docker-->>WP: container removed
WP-->>SM: cleanup complete
SM-->>HTTP: session removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 355-366: cleanup_cloud_workspace_binding currently calls
_remove_docker_workspace_container which swallows docker failures and may return
without ensuring container is gone, then proceeds to delete the workspace
directory; change the logic so that _remove_docker_workspace_container raises an
exception if it cannot confirm container removal (e.g., returns False or times
out) and update cleanup_cloud_workspace_binding to only remove the host
workspace (use _workspace_root_for_id and shutil.rmtree) when container teardown
succeeded, or catch the raised exception and skip deleting the workspace while
re-raising or logging the failure so the caller can handle rollback; ensure
references to CloudWorkspaceBinding.workspace_id are used to identify the
container and include retries/timeouts in _remove_docker_workspace_container to
deterministically confirm removal.
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 128-132: The function _cleanup_provisioned_cloud_binding currently
returns early when cloud_workspace_config.get("enabled") is not True, preventing
teardown of already-provisioned sessions; remove that early return so
cleanup_cloud_binding_from_config(cloud_workspace_config, binding) is always
invoked for cleanup of persisted CloudWorkspaceBinding instances even if new
provisioning has been disabled, ensuring containers/workspaces are torn down;
keep the call to _load_cloud_workspace_config() and use
cleanup_cloud_binding_from_config as-is (no change to its signature).
- Around line 727-752: The try/except can leak a provisioned workspace if
_provisioned_execution_binding_from_request() creates one but an exception
occurs before provisioned_binding is assigned; immediately capture the result of
_provisioned_execution_binding_from_request() into provisioned_binding (or
another local) right after the call, and use that saved reference in the except
block; also make cleanup_cloud_binding_from_config(cloud_workspace_config,
provisioned_binding) best-effort by guarding it in its own try/except so any
cleanup failure does not mask or replace the original exception from
session_manager.create_session or other errors.
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 910-915: The synchronous cleanup hook
_cleanup_provisioned_cloud_binding is being called inline inside
_remove_session_async_no_lock and can block the event loop; change the call to
run off the event loop (e.g., via asyncio.to_thread or loop.run_in_executor) so
the teardown runs in a worker thread before continuing with _session_cache.pop
and await self._run_store_io(self._store.delete, session_id); also ensure any
exceptions from the threaded call are caught/logged to avoid crashing the
coroutine.
In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 548-557: The test mutates the global session_manager by assigning
session_manager._binding_resolver directly which can leak state; instead use
monkeypatch.setattr to replace session_manager._binding_resolver with the
resolver returned by _build_binding_resolver() so pytest will restore the
original automatically—locate the assignment to
session_manager._binding_resolver in the test and change it to
monkeypatch.setattr("session_manager._binding_resolver",
_build_binding_resolver()) (or equivalent using the session_manager object
reference) so the resolver is scoped to the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b105089-54a5-4c1f-be6b-058adfc39f3a
📒 Files selected for processing (11)
coding-agent/src/coding_agent/environment/__init__.pycoding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/src/coding_agent/environment/workspace_provider.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/schemas.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/coding_agent/environment/test_docker_workspace_provider.pycoding-agent/tests/ui/test_execution_binding.pycoding-agent/tests/ui/test_http_server.pycoding-agent/tests/ui/test_session_manager_public_api.pycoding-agent/tests/ui/test_session_manager_runtime.py
Fail teardown when docker container removal cannot be confirmed so host workspaces are only deleted after container cleanup succeeds.\n\nUltraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)\n\nCo-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep provisioned workspace rollback best-effort, clean disabled-but-configured workspaces, offload async close teardown, and scope test resolver mutation.\n\nUltraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)\n\nCo-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
coding-agent/src/coding_agent/environment/docker_workspace_provider.py (1)
550-562:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat
docker inspectfailures as proof the container is gone.Lines 560-561 currently return
Falsefor any inspect-side subprocess failure. That makes_remove_docker_workspace_container()think teardown is complete and allowscleanup_cloud_workspace_binding()to delete the host workspace even when Docker is unavailable and removal was never verified.Suggested fix
def _docker_container_exists( provider_config: _DockerWorkspaceProviderConfig, container_name: str, ) -> bool: @@ - except (OSError, subprocess.SubprocessError): - return False + except (OSError, subprocess.SubprocessError) as exc: + raise RuntimeError( + f"failed to verify docker workspace container state: {container_name}" + ) from exc return result.returncode == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py` around lines 550 - 562, The current subprocess.run wrapper swallows OSError/SubprocessError and returns False, causing _remove_docker_workspace_container() to assume removal succeeded; change this so inspect failures are not treated as "gone": remove the broad except that returns False (or re-raise the caught exception) so the error propagates to _remove_docker_workspace_container()/cleanup_cloud_workspace_binding(), or explicitly return a third sentinel (e.g. None) for "unknown" instead of False; adjust callers to treat exceptions/None as "unable to verify removal" and avoid deleting the host workspace. Ensure the change targets the subprocess.run block shown (the command/env/capture_output call) and update _remove_docker_workspace_container and cleanup_cloud_workspace_binding code paths accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 347-351: The current try/except only removes workspace_root but
can leave a live container if _start_docker_workspace_container(provider_config,
binding) fails after Docker accepted run -d; modify the flow so the container is
cleaned up on failure: change _start_docker_workspace_container to return the
created container id (or include it on raised exceptions), and in the caller
catch block inspect the exception/return to detect a created container id and
perform a forced stop/remove (e.g., docker rm -f <id>) before calling
workspace_root.rmdir() and re-raising; also ensure any timeout path in
_start_docker_workspace_container triggers the same cleanup behavior.
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 745-755: The except block currently catches Exception so
asyncio.CancelledError (a BaseException subclass) skips the rollback and leaks
provisioned_binding; add a new outer except for (asyncio.CancelledError,
KeyboardInterrupt, SystemExit) that performs the same rollback logic (await
asyncio.to_thread(_cleanup_provisioned_cloud_binding, provisioned_binding) if
provisioned_binding is not None) and then re-raises the cancellation, leaving
the existing except Exception as exc block unchanged; reference the
provisioned_binding variable and the _cleanup_provisioned_cloud_binding helper
when making this change.
---
Duplicate comments:
In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 550-562: The current subprocess.run wrapper swallows
OSError/SubprocessError and returns False, causing
_remove_docker_workspace_container() to assume removal succeeded; change this so
inspect failures are not treated as "gone": remove the broad except that returns
False (or re-raise the caught exception) so the error propagates to
_remove_docker_workspace_container()/cleanup_cloud_workspace_binding(), or
explicitly return a third sentinel (e.g. None) for "unknown" instead of False;
adjust callers to treat exceptions/None as "unable to verify removal" and avoid
deleting the host workspace. Ensure the change targets the subprocess.run block
shown (the command/env/capture_output call) and update
_remove_docker_workspace_container and cleanup_cloud_workspace_binding code
paths accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19382d67-8629-4fda-8aad-d05eb2687aef
📒 Files selected for processing (6)
coding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/coding_agent/environment/test_docker_workspace_provider.pycoding-agent/tests/ui/test_http_server.pycoding-agent/tests/ui/test_session_manager_runtime.py
Clean up possible Docker containers on startup failure, preserve workspaces when inspect cannot verify removal, and roll back HTTP-provisioned workspaces on cancellation. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Capture the rollback and teardown checks so future cloud workspace changes preserve host state until cleanup is verified. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /sessionsto provision docker cloud workspaces, persist HTTP session origin metadata, and clean up provisioned resources across rollback, close, and clear pathsVerification
uv run pytest tests/coding_agent/environment/test_docker_workspace_provider.py tests/ui/test_execution_binding.py tests/ui/test_session_manager_public_api.py tests/ui/test_session_manager_runtime.py tests/ui/test_http_server.py tests/ui/test_http_server_failover.py -qPOST /sessionswithworkspace_source={kind:docker},POST /sessions/{id}/promptSSE returned/workspace\nqa-from-cloud,DELETE /sessions/{id}returned closed, subsequentGETreturned 404, and the exact docker container/workspace path disappearedSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Documentation