hayssem/Add Tenki Sandboxes provider - #1
Conversation
Implement TenkiProvider, a SandboxProvider for the Tenki managed microVM sandbox platform (https://tenki.cloud). It talks to Tenki's REST API over httpx and streams command output over WebSocket, authenticating with TENKI_API_KEY. - Full lifecycle: create/get/list/destroy sandboxes - Command execution with env var support and duration tracking - WebSocket streaming with graceful fallback to chunked replay when the optional websockets dependency is unavailable - Base64 file upload/download with path-traversal validation - Label-based reuse via find_sandbox / get_or_create_sandbox - snapshot + streaming + file_upload + persistent capabilities Wire the provider into the provider registry, CLI, and auto-configuration (registered when TENKI_API_KEY is set). Add unit tests covering the HTTP/WS layer via httpx.MockTransport plus a skipped live integration test, and update the capability matrix, README, and packaging metadata. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TxSEKG1sHCFex3uGsKnqCg
|
Review Complete Files Reviewed: 8 By Severity:
Critical resource leak, error masking, and HTTP client anti-patterns in the Tenki sandbox provider. The new provider implementation drops WebSocket disconnect handling, creates a new HTTP client per request, and silently swallows errors via synthetic UUID generation. Files Reviewed (8 files) |
There was a problem hiding this comment.
Risk: 🟠 High (78/100) — 4 high findings, 3 medium · 892 LOC across 8 files
Summary
The PR introduces a new Tenki sandbox provider (sandboxes/providers/tenki.py) with material correctness, resource-leak, and resilience issues.
High-Severity Issues
- Orphaned sandbox on setup failure (
tenki.py:168): whensetup_commandsfail duringcreate, the sandbox is left running — a resource leak. - WS disconnect data corruption (
tenki.py:287): when the WebSocket disconnects mid-stream,_connect_sandboxre-executes the command via REST, duplicating output and losing stdout/stderr boundaries. - Synthetic UUID masking API errors (
tenki.py:408):_parse_sandboxgenerates auuid4()when the API response lacks anid, hiding upstream bugs and making debugging impossible. - Non-JSON 2xx returns None (
tenki.py:492):_requestreturnsNonefor non-JSON 2xx responses, causingAttributeErrorin every caller that accesses.get()on the result.
Medium-Severity Issues
- Connection pool destroyed per request (
tenki.py:473): a newhttpx.AsyncClientper_requestcall defeats TCP connection reuse. - Falsy
oron timeout (tenki.py:471):request_timeout or REQUEST_TIMEOUTmapstimeout=0to the default, breaking zero-than-zero-disabled semantics. - Silent WS event discarding (
tenki.py:327):_decode_ws_messagediscardserror,timeout, andexitevents without forwarding them, so callers never see the command exit code.
Recommendation
Address the three resource-leak and error-masking issues before merge. The HTTP client pooling fix is lower urgency but will degrade under load.
| url = f"{self.base_url}{path}" | ||
| timeout = httpx.Timeout(request_timeout or self.timeout) | ||
|
|
||
| async with httpx.AsyncClient(timeout=timeout, transport=self._transport) as client: |
There was a problem hiding this comment.
🟡 New httpx.AsyncClient created per request destroys connection pooling (performance)
The _request method at line 473 creates a new httpx.AsyncClient for every API call via async with httpx.AsyncClient(...) as client:. In production (no shared transport provided), this means each request gets its own connection pool — TCP and TLS connections are never reused. Every lifecycle operation (create, get, list, execute, destroy, upload, download, health check) incurs a full TCP+TLS handshake, adding latency and imposing unnecessary load on the Tenki API servers.
💡 Suggestion: Create a single httpx.AsyncClient instance at provider initialization and reuse it across all _request calls. Add a close() method for cleanup.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py, add self._client: httpx.AsyncClient | None = None in __init__. Create a property or lazy-init method that instantiates the client once with timeout, transport, and base headers. In _request at line 473, replace the async with httpx.AsyncClient(...) block with response = await self._client.request(...). Add a close() method that calls await self._client.aclose().
| request_timeout: float | None = None, | ||
| ) -> Any: | ||
| url = f"{self.base_url}{path}" | ||
| timeout = httpx.Timeout(request_timeout or self.timeout) |
There was a problem hiding this comment.
🟡 Falsy or on request_timeout silently drops timeout=0 (bug)
At line 471, timeout = httpx.Timeout(request_timeout or self.timeout) uses Python's or operator. Since 0 is falsy, passing timeout=0 to execute_command (which forwards to _post → _request) causes the HTTP request timeout to fall back to the default 60s instead of using 0. In httpx, Timeout(0) means 'no timeout', but this code replaces it with 60s. The command payload correctly receives timeout_seconds: 0 (line 227 checks if timeout is not None), creating a mismatch between the API-level timeout and the HTTP client timeout.
💡 Suggestion: Use an explicit None check: httpx.Timeout(self.timeout if request_timeout is None else request_timeout).
| timeout = httpx.Timeout(request_timeout or self.timeout) | |
| timeout = httpx.Timeout(self.timeout if request_timeout is None else request_timeout) |
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py at line 471, change timeout = httpx.Timeout(request_timeout or self.timeout) to timeout = httpx.Timeout(self.timeout if request_timeout is None else request_timeout). This prevents the falsy-zero trap where timeout=0 is incorrectly replaced with the default 60s timeout.
| # Run any setup commands sequentially. | ||
| for command in config.setup_commands: | ||
| await self.execute_command(sandbox.id, command) |
There was a problem hiding this comment.
🟠 Orphaned sandbox resource leak when setup_commands fail during create (bug)
In sandboxes/providers/tenki.py, create_sandbox() creates the sandbox on the Tenki platform via _post (line 153) before running setup commands. If any setup_commands execution raises (network error, auth error, SandboxError), the exception propagates and the Sandbox object is never returned to the caller. The _SandboxAsyncContextManager in sandbox.py:34-37 requires a successful Sandbox object to invoke destroy() on exit. The orphaned sandbox continues running on Tenki's platform with no local reference for cleanup, consuming quota and incurring cost.
💡 Suggestion: Wrap the setup_commands loop in a try/except that calls await self.destroy_sandbox(sandbox.id) on failure before re-raising.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py, wrap the for loop at lines 168-170 in a try/except: on any exception, await self.destroy_sandbox(sandbox.id) before re-raising. Example: try: for command in config.setup_commands: await self.execute_command(sandbox.id, command)\nexcept Exception:\n await self.destroy_sandbox(sandbox.id)\n raise
| except Exception as exc: # noqa: BLE001 - any WS failure should fall back | ||
| logger.warning("Tenki WebSocket streaming failed (%s); falling back", exc) | ||
| async for chunk in self._simulated_stream(sandbox_id, command, timeout, env_vars): | ||
| yield chunk |
There was a problem hiding this comment.
🟠 WebSocket disconnect mid-stream causes command re-execution and output duplication (bug)
In TenkiProvider.stream_execution (lines 274-290), if the WebSocket connection drops after some chunks have already been yielded to the consumer, the except Exception block calls _simulated_stream, which invokes execute_command to run the command again from scratch. The consumer receives partial output from the WebSocket stream followed by the full output from the REST re-execution, producing duplicated/corrupted output. Additionally, the command is executed twice in the sandbox, doubling any side effects (file writes, installations, state mutations).
💡 Suggestion: Do not fall back to simulated stream when partial output has already been yielded. Instead, raise an exception or yield an error sentinel chunk so the consumer can detect the interruption.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py, modify the except block at lines 287-290: add a flag (e.g., _chunks_yielded counter) and, when set, re-raise the exception instead of falling back to _simulated_stream. If fallback is desired, either only replay un-yielded output or raise a custom StreamingInterruptedError to alert the caller.
| # ------------------------------------------------------------------ | ||
| def _parse_sandbox(self, data: dict[str, Any], labels: dict[str, str]) -> Sandbox: | ||
| """Convert a Tenki API sandbox object into a canonical Sandbox.""" | ||
| sandbox_id = data.get("id") or data.get("sandbox_id") or f"tenki-{uuid.uuid4().hex[:12]}" |
There was a problem hiding this comment.
🟠 _parse_sandbox generates synthetic UUID when API response lacks an ID, masking errors (bug)
In sandboxes/providers/tenki.py line 408, the _parse_sandbox method falls back to generating a UUID-based ID when the API response is missing both 'id' and 'sandbox_id': sandbox_id = data.get('id') or data.get('sandbox_id') or f'tenki-{uuid.uuid4().hex[:12]}'. This is called from create_sandbox (line 154), get_sandbox (line 181), and list_sandboxes (line 198). If the API returns a valid response without an id field, the resulting Sandbox object has a fake ID. Any subsequent destroy or execute calls get 404 errors. In create_sandbox, the real server-side sandbox is leaked with no programmatic way to destroy it.
💡 Suggestion: Raise a SandboxError when the API response lacks an id field, rather than silently substituting a synthetic UUID.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py, line 408, change the sandbox_id assignment to validate the API response. Replace the UUID fallback with a check that raises SandboxError if both 'id' and 'sandbox_id' are missing. For example: id_val = data.get('id') or data.get('sandbox_id'); if not id_val: raise SandboxError('Tenki API response missing sandbox id'); sandbox_id = str(id_val).
| if response.headers.get("content-type", "").startswith("application/json"): | ||
| return response.json() | ||
| return None |
There was a problem hiding this comment.
🟠 _request returns None for non-JSON 2xx responses, causing AttributeError in callers (bug)
In sandboxes/providers/tenki.py, the internal _request method at lines 492-494 returns None when a success response has a non-application/json content-type. Five critical callers consume this return value without a None check: create_sandbox (line 154), get_sandbox (line 181), list_sandboxes (line 187-188), execute_command (line 239), and download_file (line 366). Each will raise AttributeError: 'NoneType' object has no attribute 'get' if the Tenki API returns a non-JSON success response — possible with misconfigured proxies, CDN error pages, or API gateway transformations.
💡 Suggestion: Raise a SandboxError when a success response lacks a JSON body, instead of returning None.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py, modify the _request method around lines 492-494: replace return None with raise SandboxError(f'Tenki API returned unexpected content type: {response.headers.get("content-type", "unknown")}') or attempt response.json() wrapped in try/except to produce a meaningful error.
| event_type = event.get("type") or event.get("event") | ||
| if event_type in {"done", "exit", "close", "eof"}: | ||
| return None | ||
|
|
||
| if "stdout" in event: | ||
| return event["stdout"] | ||
| if "stderr" in event: | ||
| return f"[stderr]: {event['stderr']}" | ||
| return event.get("data", "") |
There was a problem hiding this comment.
🟡 _decode_ws_message silently discards error/timeout/exit events; streaming loses exit code (bug)
The WebSocket message decoder at lines 327-335 recognizes only done/exit/close/eof as terminal events (returning None to break the loop). Server-sent error events (e.g., {"type": "error", "message": "..."}) and timeout events are not in the terminal set, so they return event.get('data', '') which is typically empty string, and the loop continues polling. Exit codes present in terminal events ({"type": "done", "exit_code": 0}) are discarded. The streaming consumer has no way to determine command success/failure, unlike the REST path which returns full ExecutionResult.
💡 Suggestion: Add 'error' and 'timeout' to the terminal event set. Log exit codes from terminal events and consider yielding a structured sentinel or raising a StreamError on error/timeout events.
📋 Prompt for AI Agents
In sandboxes/providers/tenki.py at line 328, extend the terminal event set to include 'error' and 'timeout': if event_type in {"done", "exit", "close", "eof", "error", "timeout"}:. Additionally, for error/timeout events, log event.get('message') and consider either raising a SandboxError or yielding a sentinel like f'[STREAM_ERROR]: {event.get("message", "unknown")}' before the break.
|
@hveluxor re-implemented this PR using the Python Async SDK here francoluxor#1 Verified e2e against prod. |
Summary
httpxand streams command output over WebSocket; auth viaTENKI_API_KEY, no extra runtime bridge neededFeatures
SandboxProviderimplementation (create / get / list / destroy)websocketsdependency is unavailableget_or_create_sandbox/find_sandbox)TENKI_API_KEYis set (registered at priority 8), plusSandbox.configure(tenki_api_key=...)persistent,snapshot,streaming,file_uploadRequirements
TENKI_API_KEYenvironment variableTest plan
ruff checkon all changed files)tests/test_tenki_provider.py(13 tests) drive the HTTP/WS layer viahttpx.MockTransport; capability-matrix contract updatedTENKI_API_KEY(live test is included but skipped without a key)🤖 Generated with Claude Code
https://claude.ai/code/session_01TxSEKG1sHCFex3uGsKnqCg
Generated by Claude Code