fix(sdk-python): correct router port and bootstrap session race#311
fix(sdk-python): correct router port and bootstrap session race#311Abhinav-kodes wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
|
[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 |
|
Welcome @Abhinav-kodes! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
![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 modifies the session bootstrapping logic to allow returning a session ID even when the HTTP response indicates an error, provided the required header is present. It also updates the router port in the usage examples. The review feedback suggests enhancing observability by logging a warning when a session ID is extracted from an error response and providing more diagnostic details in the exception raised when the session header is missing.
| if session_id: | ||
| return session_id |
There was a problem hiding this comment.
While it's correct to return the session ID even on non-2xx responses (as per the PR description), it would be beneficial to log a warning when this happens. This helps in identifying underlying issues with the agent or router that might otherwise be silenced during the bootstrap process.
| if session_id: | |
| return session_id | |
| if session_id: | |
| if resp.status_code >= 400: | |
| self.logger.warning( | |
| f"Bootstrap request returned status {resp.status_code}, " | |
| f"but session ID was found: {session_id}" | |
| ) | |
| return session_id |
| raise ValueError( | ||
| f"Missing required response header: {self.SESSION_HEADER}" | ||
| ) |
There was a problem hiding this comment.
When the session ID header is missing in a successful (2xx) response, providing more context such as the status code and a portion of the response body can significantly aid in debugging protocol mismatches between the SDK and the router.
| raise ValueError( | |
| f"Missing required response header: {self.SESSION_HEADER}" | |
| ) | |
| raise ValueError( | |
| f"Missing required response header: {self.SESSION_HEADER} " | |
| f"(Status: {resp.status_code}, Response: {resp.text[:100]})" | |
| ) |
There was a problem hiding this comment.
Pull request overview
This PR fixes two Python SDK issues encountered when running AgentCube’s getting-started flow end-to-end: an incorrect Router port in an example, and a session-bootstrap bug where the SDK failed to capture x-agentcube-session-id when the upstream agent returned a non-2xx response.
Changes:
- Update the
agent_runtime_usage.pyexample to use Router port8081(matching the documentedkubectl port-forwardmapping). - Adjust
AgentRuntimeDataPlaneClient.bootstrap_session_id()to readx-agentcube-session-idbefore callingraise_for_status(), allowing session bootstrap to succeed even when the agent returns non-2xx.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk-python/examples/agent_runtime_usage.py | Fixes the example Router URL port to match documented local port-forwarding. |
| sdk-python/agentcube/clients/agent_runtime_data_plane.py | Ensures session ID bootstrap succeeds even when the initial GET receives a non-2xx response but includes the session header. |
| def bootstrap_session_id(self) -> str: | ||
| resp = self.session.get( | ||
| self.base_url, | ||
| timeout=(self.connect_timeout, self.timeout), | ||
| ) | ||
| resp.raise_for_status() | ||
|
|
||
| session_id = resp.headers.get(self.SESSION_HEADER) | ||
| if not session_id: | ||
| raise ValueError( | ||
| f"Missing required response header: {self.SESSION_HEADER}" | ||
| ) | ||
| return session_id | ||
| if session_id: | ||
| return session_id | ||
| resp.raise_for_status() | ||
| raise ValueError( | ||
| f"Missing required response header: {self.SESSION_HEADER}" | ||
| ) |
- Fix hardcoded port 18081 → 8081 in agent_runtime_usage.py to match the default kubectl port-forward mapping (8081:8080) - Fix bootstrap_session_id() to read x-agentcube-session-id header before calling raise_for_status(); the router injects the header even when the upstream agent returns a non-2xx response (e.g. 404 for GET / on agents that only handle POST requests) Signed-off-by: Abhinav-kodes <183825080+Abhinav-kodes@users.noreply.github.com>
Abhinav-kodes
force-pushed
the
fix/sdk-python-bootstrap-and-port
branch
from
cf5a0b5 to
1768896
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 47.57% 47.75% +0.18%
==========================================
Files 30 30
Lines 2819 2854 +35
==========================================
+ Hits 1341 1363 +22
- Misses 1338 1343 +5
- Partials 140 148 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Two fixes found while running the getting-started examples end-to-end.
Fix 1 — Wrong port in example
agent_runtime_usage.pyhardcoded port18081but the documentedkubectl port-forwardcommand maps the router to8081. This causedConnection refusedfor anyone following the getting-started guide.Fix 2 — Bootstrap session race on non-2xx agent responses
bootstrap_session_id()calledraise_for_status()before readingthe session header. The AgentCube Router injects
x-agentcube-session-ideven when the upstream agent returns a non-2xx response (e.g.
404for
GET /on agents that only handlePOST). This caused the clientto crash before it could read the valid session ID.
Fix: Read the header first. Only surface the HTTP error if the
header is absent.
Testing
Verified end-to-end with
kindcluster using the getting-started guide.