Fix/agent runtime delete session#426
Conversation
test: remove duplicate TestHandleHealth function
volcano-sh-bot
added
kind/enhancement
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates server-side session management into the AgentRuntimeClient using a new ControlPlaneClient. Key changes include adding configuration for a workload manager, implementing a stop() method to delete owned sessions, and updating examples and tests. The review feedback highlights several critical issues: first, ControlPlaneClient lacks the create_agent_runtime_session method, which will cause an AttributeError on startup; second, an exception in self.close() during stop() can prevent session deletion, risking resource leaks; third, parsing an empty 200 OK response as JSON will trigger a JSONDecodeError; and finally, the unit tests must be updated to mock the correct bootstrapping behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR updates the Python SDK’s AgentRuntimeClient lifecycle management to support server-side session deletion (similar to CodeInterpreterClient), aiming to prevent sandbox/session resource leaks when clients are done.
Changes:
- Add
ControlPlaneClient.delete_agent_runtime_session()forDELETE /v1/agent-runtime/sessions/:sessionId. - Add
AgentRuntimeClient.stop()and switch context-manager cleanup to callstop()instead ofclose(). - Add/update example and tests to cover owned vs attached session behavior and context manager cleanup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| sdk-python/agentcube/agent_runtime.py | Adds control-plane integration, ownership tracking, and stop() cleanup for AgentRuntime sessions. |
| sdk-python/agentcube/clients/control_plane.py | Adds an agent-runtime session delete helper on the control-plane client. |
| sdk-python/examples/agent_runtime_usage.py | Updates example usage to demonstrate stopping clients. |
| sdk-python/tests/test_agent_runtime.py | Adds tests for stop() behavior and context-manager cleanup. |
Signed-off-by: vivek41-glitch <[email protected]>
c3574b0 to
eebdda2
Compare
|
/remove-label do-not-merge/contains-merge-commits |
|
@vivek41-glitch: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
sdk-python/agentcube/agent_runtime.py:106
AgentRuntimeDataPlaneClient.invoke()currently only accepts(session_id, payload, timeout). AddingpathtoAgentRuntimeClient.invoke()and passing it through will raiseTypeError: invoke() got an unexpected keyword argument 'path'.
def invoke(self, payload: Dict[str, Any], timeout: Optional[float] = None, path: str = "") -> Any:
if not self.session_id:
raise ValueError("AgentRuntime session_id is not initialized")
resp = self.dp_client.invoke(
| # Resolve auth: auth param > auth_token > k8s SA token file | ||
| if auth: | ||
| self._auth = auth | ||
| elif auth_token: | ||
| from agentcube.auth import TokenAuth | ||
| self._auth = TokenAuth(auth_token) | ||
| else: | ||
| token_path = "/var/run/secrets/kubernetes.io/serviceaccount/token" | ||
| token = read_token_from_file(token_path) | ||
| if token: | ||
| from agentcube.auth import TokenAuth | ||
| self._auth = TokenAuth(token) | ||
| else: | ||
| self._auth = None |
| def _apply_auth(self, request_kwargs: dict) -> None: | ||
| """Add Authorization header from auth provider if available.""" | ||
| if not self._auth: | ||
| return | ||
| headers = request_kwargs.setdefault("headers", {}) | ||
| headers["Authorization"] = f"Bearer {self._auth.get_token()}" |
| from requests.exceptions import JSONDecodeError | ||
| from agentcube.auth import AuthProvider | ||
| from agentcube.clients.agent_runtime_data_plane import AgentRuntimeDataPlaneClient |
| workload_manager_url: Optional[str] = None, | ||
| auth_token: Optional[str] = None, | ||
| auth: Optional[AuthProvider] = None, | ||
| ): |
| self._auth = auth | ||
| if not self._auth and auth_token: | ||
| from agentcube.auth import TokenAuth | ||
| self._auth = TokenAuth(auth_token) |
| self.dp_client = AgentRuntimeDataPlaneClient( | ||
| router_url=self.router_url, | ||
| namespace=self.namespace, | ||
| agent_name=self.agent_name, | ||
| timeout=self.timeout, | ||
| connect_timeout=self.connect_timeout, | ||
| auth=self._auth, | ||
| ) |
| def stop(self) -> None: | ||
| """Close local connection and delete server-side session if owned.""" | ||
| try: | ||
| self.close() | ||
| except Exception as e: | ||
| self.logger.warning(f"Error closing local connection: {e}") | ||
|
|
||
| if self._owned_session and self.session_id and self._control_plane: | ||
| try: | ||
| self._control_plane.delete_agent_runtime_session(self.session_id) | ||
| self.logger.info(f"Deleted AgentRuntime session: {self.session_id}") | ||
| except Exception as e: | ||
| self.logger.warning(f"Error deleting AgentRuntime session: {e}") |
Signed-off-by: vivek41-glitch <[email protected]>
Signed-off-by: vivek41-glitch <[email protected]>
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
This PR implements server-side session deletion for AgentRuntimeClient, bringing it in line with the existing lifecycle management of CodeInterpreterClient.
Previously, AgentRuntimeClient could create sessions via
bootstrap_session_id()but had no way to delete them server-side. Theclose()method only closed the local HTTP connection, leaving the remote sandbox/session running indefinitely on the cluster. This caused resource leaks and inconsistency with CodeInterpreterClient, which already has a properstop()method.Changes introduced:
delete_agent_runtime_session()toControlPlaneClientto callDELETE /v1/agent-runtime/sessions/:sessionIdstop()method toAgentRuntimeClientthat:__exit__to callstop()instead ofclose()for proper cleanup in context managersworkload_manager_urlandauth_tokenparameters with environment variable fallback (AGENTCUBE_WORKLOAD_MANAGER_URL,AGENTCUBE_AUTH_TOKEN)stop()usageWhich issue(s) this PR fixes:
Fixes #395
Special notes for reviewer:
This is a Python SDK-only change. No Go backend changes are required as the endpoint
DELETE /v1/agent-runtime/sessions/:sessionIdalready exists inpkg/workloadmanager/server.go(sharedhandleDeleteSandboxhandler).The implementation follows the exact same pattern as
CodeInterpreterClient.stop():session_id) are preservedDoes this PR introduce a user-facing change?: