diff --git a/studio/backend/core/inference/external_provider.py b/studio/backend/core/inference/external_provider.py index 79c8287e5b..de5f5c5500 100644 --- a/studio/backend/core/inference/external_provider.py +++ b/studio/backend/core/inference/external_provider.py @@ -2870,7 +2870,17 @@ class ExternalProviderClient: response.raise_for_status() data = response.json() containers = data.get("data") if isinstance(data, dict) else None - return list(containers) if isinstance(containers, list) else [] + result = list(containers) if isinstance(containers, list) else [] + logger.info( + "openai_container_list.response count=%s items=%s", + len(result), + [ + {"id": c.get("id"), "status": c.get("status")} + for c in result + if isinstance(c, dict) + ], + ) + return result async def create_openai_container( self, @@ -2901,15 +2911,38 @@ class ExternalProviderClient: async def delete_openai_container(self, container_id: str) -> None: """DELETE /v1/containers/{id}. 404s are surfaced as HTTPError. + Uses a fresh httpx client (not the shared ``_http_client``) so + connection-pool state from earlier chat requests cannot + interfere — observed in the wild that DELETEs over the shared + pool returned ``deleted: true`` while the container persisted + in subsequent /containers list calls, even though the same + DELETE issued from a fresh client genuinely removed it. + Verifies the response body reports ``deleted: true``. OpenAI returns a 2xx ``deleted: true`` body even when the request is silently rejected (e.g. missing OpenAI-Beta header), so a status-only check is not sufficient. """ - response = await _http_client.delete( - f"{self.base_url}/containers/{container_id}", - headers = self._container_headers(), - timeout = self._timeout, + url = f"{self.base_url}/containers/{container_id}" + headers = self._container_headers() + logger.info( + "openai_container_delete.outbound url=%s has_auth=%s openai_beta=%s", + url, + "Authorization" in headers, + headers.get("OpenAI-Beta"), + ) + async with httpx.AsyncClient(timeout = self._timeout) as fresh_client: + response = await fresh_client.delete(url, headers = headers) + logger.info( + "openai_container_delete.response status=%s cf_ray=%s " + "request_id=%s organization=%s project=%s processing_ms=%s body=%s", + response.status_code, + response.headers.get("cf-ray"), + response.headers.get("x-request-id"), + response.headers.get("openai-organization"), + response.headers.get("openai-project"), + response.headers.get("openai-processing-ms"), + response.text[:300], ) response.raise_for_status() try: diff --git a/studio/backend/models/inference.py b/studio/backend/models/inference.py index 6a042d35d7..3aa89cc934 100644 --- a/studio/backend/models/inference.py +++ b/studio/backend/models/inference.py @@ -650,11 +650,11 @@ class CreateOpenAIContainerBody(OpenAIContainerRequest): ttl_minutes: int = Field( 20, ge = 1, - le = 10080, # 1 week + le = 20, description = ( "Idle-timeout TTL the new container will inherit (anchor=" - "last_active_at). OpenAI's default is 20; we cap at one " - "week as a safety bound." + "last_active_at). OpenAI hard-caps this at 20 minutes and " + "rejects larger values with integer_above_max_value." ), ) diff --git a/studio/backend/routes/inference.py b/studio/backend/routes/inference.py index 8f5bd9aab5..76bbb59c94 100644 --- a/studio/backend/routes/inference.py +++ b/studio/backend/routes/inference.py @@ -1716,8 +1716,15 @@ async def list_openai_containers( status_code = 502, detail = f"Failed to reach OpenAI: {exc}", ) + # OpenAI keeps expired containers in /v1/containers indefinitely + # with status="expired" — they're effectively dead but still + # listed. Hide them so the picker only shows usable containers. return ListOpenAIContainersResponse( - containers = [_summarize_container(c) for c in raw if isinstance(c, dict)], + containers = [ + _summarize_container(c) + for c in raw + if isinstance(c, dict) and c.get("status") != "expired" + ], ) finally: await client.close() @@ -1766,17 +1773,38 @@ async def delete_openai_container( current_subject: str = Depends(get_current_subject), ) -> None: """Delete a named container by id.""" + logger.info( + "openai_container_delete.request subject=%s container_id=%s base_url=%s", + current_subject, + body.container_id, + body.provider_base_url, + ) client = _resolve_openai_cloud_client(body) try: try: await client.delete_openai_container(body.container_id) + logger.info( + "openai_container_delete.success container_id=%s", + body.container_id, + ) except httpx.HTTPStatusError as exc: detail = exc.response.text[:500] if exc.response is not None else str(exc) + logger.warning( + "openai_container_delete.openai_rejected container_id=%s status=%s body=%s", + body.container_id, + exc.response.status_code if exc.response else None, + detail, + ) raise HTTPException( status_code = exc.response.status_code if exc.response else 502, detail = f"OpenAI rejected /containers delete: {detail}", ) except httpx.HTTPError as exc: + logger.warning( + "openai_container_delete.transport_error container_id=%s error=%s", + body.container_id, + exc, + ) raise HTTPException( status_code = 502, detail = f"Failed to reach OpenAI: {exc}", diff --git a/studio/backend/tests/test_openai_container_crud.py b/studio/backend/tests/test_openai_container_crud.py index 2965ec6649..fbc0393677 100644 --- a/studio/backend/tests/test_openai_container_crud.py +++ b/studio/backend/tests/test_openai_container_crud.py @@ -149,3 +149,41 @@ def test_delete_propagates_openai_4xx(monkeypatch): with pytest.raises(httpx.HTTPStatusError): _drive(_make_client().delete_openai_container("cntr_missing")) + + +def test_list_route_filters_expired_containers(monkeypatch): + """OpenAI keeps containers in /v1/containers indefinitely with + status="expired" after their idle TTL passes — they can't be + used but still show up. The list route must drop them so the + picker only surfaces usable containers.""" + from routes import inference as inf_mod + from models.inference import OpenAIContainerRequest + + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, + json = { + "data": [ + {"id": "cntr_active", "name": "live", "status": "running"}, + {"id": "cntr_dead", "name": "old", "status": "expired"}, + {"id": "cntr_unknown", "name": "no-status"}, + ], + }, + ) + + _mock_http_client(monkeypatch, handler) + + def fake_resolve(_body): + return _make_client() + + monkeypatch.setattr(inf_mod, "_resolve_openai_cloud_client", fake_resolve) + + body = OpenAIContainerRequest( + encrypted_api_key = "enc", + provider_base_url = "https://api.openai.com/v1", + ) + response = _drive(inf_mod.list_openai_containers(body, current_subject = "u")) + ids = [c.id for c in response.containers] + assert "cntr_active" in ids + assert "cntr_unknown" in ids # missing status is treated as usable + assert "cntr_dead" not in ids diff --git a/studio/frontend/src/features/chat/api/openai-containers.ts b/studio/frontend/src/features/chat/api/openai-containers.ts index ca311cc311..29d292f7cf 100644 --- a/studio/frontend/src/features/chat/api/openai-containers.ts +++ b/studio/frontend/src/features/chat/api/openai-containers.ts @@ -115,7 +115,10 @@ export async function deleteOpenAIContainer( }), }, ); - if (!response.ok && response.status !== 204) { + // 404 = container already gone (deleted elsewhere, or expired-then-purged). + // Treat as idempotent success so a stale list entry doesn't surface as a + // confusing error — the caller will refresh and the entry will disappear. + if (!response.ok && response.status !== 204 && response.status !== 404) { throw new Error(await parseError(response)); } } diff --git a/studio/frontend/src/features/chat/components/openai-code-exec-section.tsx b/studio/frontend/src/features/chat/components/openai-code-exec-section.tsx index 15cb79286c..2d3d225069 100644 --- a/studio/frontend/src/features/chat/components/openai-code-exec-section.tsx +++ b/studio/frontend/src/features/chat/components/openai-code-exec-section.tsx @@ -49,7 +49,7 @@ import { ensureThreadRecord } from "../runtime-provider"; const AUTO_OPTION_VALUE = "__auto__"; const DEFAULT_TTL_MINUTES = 20; const TTL_MIN = 1; -const TTL_MAX = 10080; // one week — matches backend bound +const TTL_MAX = 20; // OpenAI hard cap on expires_after.minutes function ageLabel(epochSeconds: number | null | undefined): string { if (!epochSeconds) return ""; @@ -84,6 +84,13 @@ export function OpenAICodeExecSection({ const [createTtl, setCreateTtl] = useState( provider.openaiContainerTtlMinutes ?? DEFAULT_TTL_MINUTES, ); + // Ids that have been deleted in this session. Once tombstoned, an id + // stays hidden from the picker for the lifetime of the page — OpenAI's + // /containers list can keep returning a freshly-deleted id for an + // undocumented and variable amount of time, and an automatic re-show + // creates more confusion than it solves. Refreshing the page resets + // the tombstone naturally. + const [tombstones, setTombstones] = useState>(() => new Set()); const thread = useLiveQuery( async () => (activeThreadId ? db.threads.get(activeThreadId) : undefined), @@ -91,14 +98,22 @@ export function OpenAICodeExecSection({ ); const activeContainerId = thread?.openaiCodeExecContainerId ?? null; + // Hide just-deleted containers even if OpenAI's list still returns them. + // This is the single chokepoint — every downstream view (sorted picker, + // auto-bind candidate, all-containers list) derives from visibleContainers. + const visibleContainers = useMemo(() => { + if (tombstones.size === 0) return containers; + return containers.filter((c) => !tombstones.has(c.id)); + }, [containers, tombstones]); + // Containers sorted newest-first by lastActiveAt so the dropdown's // default (auto-bind target) shows up first. const sortedContainers = useMemo( () => - [...containers].sort( + [...visibleContainers].sort( (a, b) => (b.lastActiveAt ?? 0) - (a.lastActiveAt ?? 0), ), - [containers], + [visibleContainers], ); // What the dropdown should display right now. We decouple this from @@ -150,10 +165,14 @@ export function OpenAICodeExecSection({ // the chat-adapter's lazy-create path will mint the first container // on first send. useEffect(() => { - if (!activeThreadId || activeContainerId || containers.length === 0) { + if ( + !activeThreadId || + activeContainerId || + visibleContainers.length === 0 + ) { return; } - const sorted = [...containers].sort( + const sorted = [...visibleContainers].sort( (a, b) => (b.lastActiveAt ?? 0) - (a.lastActiveAt ?? 0), ); const candidate = sorted[0]; @@ -171,7 +190,7 @@ export function OpenAICodeExecSection({ // Best-effort; the chat-adapter will inherit/create on send. } })(); - }, [activeThreadId, activeContainerId, containers]); + }, [activeThreadId, activeContainerId, visibleContainers]); const ttlValue = provider.openaiContainerTtlMinutes ?? DEFAULT_TTL_MINUTES; @@ -223,7 +242,6 @@ export function OpenAICodeExecSection({ toast.success(`Created container ${name}`); setCreateName(""); setCreateOpen(false); - await refresh(); // Auto-bind the just-created container to the active thread. // ensureThreadRecord first so the bind lands even when the user // creates a container before sending the first message — without @@ -250,6 +268,10 @@ export function OpenAICodeExecSection({ ); } finally { setCreating(false); + // Refresh even on failure: the request may have partially succeeded + // server-side (created container, lost response), and a re-fetch + // keeps the picker in sync with OpenAI's actual state. + await refresh(); } }; @@ -267,6 +289,14 @@ export function OpenAICodeExecSection({ { apiKey, baseUrl: provider.baseUrl || null }, id, ); + // Tombstone the id so the picker hides it immediately even if + // OpenAI's list keeps returning it for a while. + setTombstones((prev) => { + if (prev.has(id)) return prev; + const next = new Set(prev); + next.add(id); + return next; + }); // Clear any thread bindings pointing at the now-deleted id. const affected = await db.threads .filter((t) => t.openaiCodeExecContainerId === id) @@ -277,11 +307,15 @@ export function OpenAICodeExecSection({ ), ); toast.success(`Deleted container ${name || id}`); - await refresh(); } catch (err) { toast.error( `Delete failed: ${err instanceof Error ? err.message : "Unknown"}`, ); + } finally { + // Always refresh so a stale list entry (e.g. container deleted + // elsewhere, or already expired) is purged from the UI even when + // the delete call itself errored. + await refresh(); } }; @@ -293,7 +327,7 @@ export function OpenAICodeExecSection({ htmlFor="openai-container-ttl" className="min-w-0 text-[13px] font-medium leading-[1.25] tracking-nav text-nav-fg" > - New-container idle timeout (min) + New-container idle timeout (min, max 20) All containers - {isLoading && containers.length === 0 ? ( + {isLoading && visibleContainers.length === 0 ? ( - ) : containers.length > 0 ? ( + ) : visibleContainers.length > 0 ? (
    - {containers.map((c) => { + {visibleContainers.map((c) => { const isActive = c.id === activeContainerId; return (
  • = 1 - ? Math.min(raw.openaiContainerTtlMinutes, 10080) + ? Math.min(raw.openaiContainerTtlMinutes, 20) : undefined, }; }