-
Notifications
You must be signed in to change notification settings - Fork 41
Description
Summary
The current authentication pattern in APIRemoteWorkspace requires manually resetting and re-initializing the HTTP client when credentials change. This is error-prone and relies on side effects that are not immediately obvious.
Current Implementation
In openhands-workspace/openhands/workspace/remote_api/workspace.py (lines 114-116):
self.api_key = self._session_api_key
self._client = None # Reset HTTP client with new host and API key
_ = self.client # Initialize client by accessing the propertyProblem
This pattern has several issues:
- Non-obvious side effects: Accessing the
.clientproperty triggers lazy initialization, but this is not apparent from the code - Order dependency: The operations must happen in exact order (set api_key → reset client → access client)
- Fragility: Changes to the parent class
RemoteWorkspacecould break this pattern silently - Code smell: The underscore assignment
_ = self.clientindicates we're calling something purely for side effects - Separation of concerns: We have two different authentication mechanisms mixed together:
- Runtime API auth:
runtime_api_keywithX-API-Keyheader (for managing containers) - Agent Server auth:
session_api_keywithX-Session-API-Keyheader (for workspace operations)
- Runtime API auth:
Why It Works This Way
The parent class RemoteWorkspace has a lazy-initialized client property:
@property
def client(self) -> httpx.Client:
client = self._client
if client is None:
timeout = httpx.Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
client = httpx.Client(base_url=self.host, timeout=timeout, headers=self._headers)
self._client = client
return clientThe _headers property (from RemoteWorkspaceMixin) uses self.api_key to construct authentication headers:
@property
def _headers(self):
headers = {}
if self.api_key:
headers["X-Session-API-Key"] = self.api_key
return headersSo when the runtime starts and we receive a session_api_key, we need to:
- Update
api_keyfield to the new value - Recreate the HTTP client to use the new credentials
- But the client is already created with old (None) credentials
The current workaround is to reset _client = None and then access .client to trigger recreation.
Proposed Solutions
Option 1: Add a Client Update Method
Add a method to RemoteWorkspace or RemoteWorkspaceMixin:
def _update_client_credentials(self, api_key: str | None = None, host: str | None = None):
"""Update client credentials and reinitialize the HTTP client."""
if api_key is not None:
self.api_key = api_key
if host is not None:
self.host = host
self._client = None
# Force client recreation
_ = self.clientThen in APIRemoteWorkspace:
self._update_client_credentials(api_key=self._session_api_key)Option 2: Make Client Dynamically Recreate on Credential Change
Instead of caching the client, check if credentials have changed and recreate if needed:
_client: httpx.Client | None = PrivateAttr(default=None)
_cached_api_key: str | None = PrivateAttr(default=None)
_cached_host: str | None = PrivateAttr(default=None)
@property
def client(self) -> httpx.Client:
# Recreate client if credentials or host changed
if (self._client is None or
self._cached_api_key != self.api_key or
self._cached_host != self.host):
if self._client is not None:
self._client.close()
timeout = httpx.Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
self._client = httpx.Client(
base_url=self.host,
timeout=timeout,
headers=self._headers
)
self._cached_api_key = self.api_key
self._cached_host = self.host
return self._clientOption 3: Separate Runtime API Client from Agent Server Client
Create two distinct HTTP clients with clear responsibilities:
# In APIRemoteWorkspace
_runtime_api_client: httpx.Client # For runtime API calls (start/stop/pause)
_agent_server_client: httpx.Client # For agent operations (via parent class)This would make the authentication separation explicit and avoid mixing concerns.
Option 4: Use Dependency Injection for Headers
Instead of lazy initialization, pass headers explicitly when making requests:
def _send_request(self, method: str, url: str, headers: dict | None = None, **kwargs):
# Merge provided headers with authentication headers
final_headers = {**self._headers, **(headers or {})}
return self.client.request(method, url, headers=final_headers, **kwargs)Recommendation
I recommend Option 2 (dynamic client recreation) as it:
- Requires minimal changes to existing code
- Makes credential updates transparent
- Eliminates the need for manual client reset
- Is backwards compatible
- Handles both credential AND host changes automatically
Option 3 would be ideal for a larger refactoring as it makes the separation of concerns explicit and clearer.
Related Code
openhands-workspace/openhands/workspace/remote_api/workspace.py(lines 114-116, 269-285)openhands-sdk/openhands/sdk/workspace/remote/base.py(lines 34-47)openhands-sdk/openhands/sdk/workspace/remote/remote_workspace_mixin.py(lines 34-39)openhands-sdk/openhands/sdk/workspace/remote/async_remote_workspace.py(similar pattern for async)
Impact
- Current state: Working but fragile
- Risk: Medium - future refactoring could break this pattern
- Priority: Low-Medium - not urgent but should be addressed before the pattern spreads
Context
This issue was identified during PR #1090 review. The current implementation works correctly but could benefit from a cleaner architecture that's more maintainable and less error-prone.