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 d7aecd6c06..ea1ce54462 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 @@ -59,12 +59,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 -// Cadence for re-fetching the container list while the section is -// mounted. OpenAI's container TTL flips at minute granularity, so 30s -// is fast enough that an expired container loses its ACTIVE pill within -// half a minute without hammering /v1/containers. -const REFRESH_POLL_MS = 30_000; +const TTL_MAX = 20; // OpenAI hard cap on expires_after.minutes function ageLabel(epochSeconds: number | null | undefined): string { if (!epochSeconds) return ""; @@ -114,12 +109,13 @@ export function OpenAICodeExecSection({ const [createTtl, setCreateTtl] = useState( provider.openaiContainerTtlMinutes ?? DEFAULT_TTL_MINUTES, ); - // Target row for the destructive confirmation dialog. Held in state - // (rather than blocking with window.confirm) so the dialog sits inside - // the settings sheet instead of a native browser alert. - const [pendingDelete, setPendingDelete] = - useState(null); - const [deleting, setDeleting] = useState(false); + // 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), @@ -127,14 +123,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], ); // First running container by lastActiveAt — the auto-bind target and @@ -217,10 +221,18 @@ export function OpenAICodeExecSection({ // the chat-adapter's lazy-create path will mint the first container // on first send. useEffect(() => { - if (!activeThreadId || activeContainerId || !firstRunningContainer) { + if ( + !activeThreadId || + activeContainerId || + visibleContainers.length === 0 + ) { return; } - const candidateId = firstRunningContainer.id; + const sorted = [...visibleContainers].sort( + (a, b) => (b.lastActiveAt ?? 0) - (a.lastActiveAt ?? 0), + ); + const candidate = sorted[0]; + if (!candidate) return; void (async () => { try { await ensureThreadRecord({ @@ -234,25 +246,7 @@ export function OpenAICodeExecSection({ // Best-effort; the chat-adapter will inherit/create on send. } })(); - }, [activeThreadId, activeContainerId, firstRunningContainer]); - - // Stale-bind sweeper: when the active thread is pinned to a container - // that the latest list no longer reports as running (expired by idle - // TTL, or deleted out-of-band), drop the binding so the auto-bind - // effect above can pick a healthy candidate and the chat adapter's - // lazy-create path can mint a fresh container on the next send. Only - // fires when `containers` has actually been fetched at least once to - // avoid clearing on a transient empty list during initial load. - useEffect(() => { - if (!activeThreadId || !activeContainerId) return; - if (containers.length === 0) return; - if (boundContainer && isContainerRunning(boundContainer)) return; - void db.threads - .update(activeThreadId, { openaiCodeExecContainerId: null }) - .catch(() => { - /* best-effort cleanup */ - }); - }, [activeThreadId, activeContainerId, boundContainer, containers.length]); + }, [activeThreadId, activeContainerId, visibleContainers]); const ttlValue = provider.openaiContainerTtlMinutes ?? DEFAULT_TTL_MINUTES; @@ -304,7 +298,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 @@ -331,6 +324,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(); } }; @@ -343,6 +340,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) @@ -353,14 +358,15 @@ export function OpenAICodeExecSection({ ), ); toast.success(`Deleted container ${name || id}`); - setPendingDelete(null); - await refresh(); } catch (err) { toast.error( `Delete failed: ${err instanceof Error ? err.message : "Unknown"}`, ); } finally { - setDeleting(false); + // 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(); } }; @@ -374,7 +380,7 @@ export function OpenAICodeExecSection({ htmlFor="openai-container-ttl" className="min-w-0 text-[13px] font-medium leading-[1.25] tracking-nav text-nav-fg" > - Idle timeout (min) + New-container idle timeout (min, max 20) - {isLoading && sortedContainers.length === 0 ? ( + {/* When no containers exist yet, render a disabled placeholder + instead of the picker. The first one is created by the + chat-adapter on first send (lazy-create) and will appear + here after the next refresh. */} + {sortedContainers.length === 0 ? ( +
+ (none yet — will be created on first send) +
+ ) : ( + + )} + + + {/* Container list with delete actions — labeled and visually + quieter so it's clearly the "all containers, manage them" + area rather than the active selector above. */} +
+ + All containers + + {isLoading && visibleContainers.length === 0 ? ( - ) : sortedContainers.length > 0 ? ( -
    - {sortedContainers.map((c) => { - const running = isContainerRunning(c); - const isActive = running && c.id === displayActiveId; - const ttlMinutes = c.expiresAfterMinutes ?? DEFAULT_TTL_MINUTES; - const canActivate = - activeThreadId != null && !isActive && running; - const statusLabel = !running - ? (c.status ?? "expired") - : null; + ) : visibleContainers.length > 0 ? ( +
      + {visibleContainers.map((c) => { + const isActive = c.id === activeContainerId; return (
    • = 1 - ? Math.min(raw.openaiContainerTtlMinutes, 10080) + ? Math.min(raw.openaiContainerTtlMinutes, 20) : undefined, }; } diff --git a/studio/install_llama_prebuilt.py b/studio/install_llama_prebuilt.py index 7c933bd612..89322c83ee 100644 --- a/studio/install_llama_prebuilt.py +++ b/studio/install_llama_prebuilt.py @@ -3433,10 +3433,52 @@ def extract_archive(archive_path: Path, destination: Path) -> None: ) from exc return target + def _try_repair_missing_slash( + member_name: str, link_name: str, archive_names: set[str] + ) -> str | None: + """Some upstream llama.cpp Mac releases (e.g. b9165, b9169) ship + symlinks whose linkname is missing the directory separator AND + the leading character of the file basename between the + top-level dir and the rest of the path: + + llama-b9165/libggml-rpc.0.dylib -> llama-b9165ibggml-rpc.0.11.1.dylib + + That cannot be resolved as written. Detect the pattern + (linkname starts with the top-level dir name but no following + slash) and search archive entries under that dir for a real + file whose basename ends with the mangled suffix. Only accept + when the suffix uniquely identifies a real archive entry.""" + if "/" not in member_name or "/" in link_name: + return None + top, _, _ = member_name.partition("/") + if not link_name.startswith(top) or len(link_name) <= len(top): + return None + bad_suffix = link_name[len(top) :] + if not bad_suffix or bad_suffix.startswith("/"): + return None + prefix = f"{top}/" + candidates = [ + name + for name in archive_names + if name.startswith(prefix) + and "/" not in name[len(prefix) :] + and name[len(prefix) :].endswith(bad_suffix) + ] + if len(candidates) != 1: + return None + return candidates[0] + def safe_link_target( - base: Path, member_name: str, link_name: str, target: Path + base: Path, + member_name: str, + link_name: str, + target: Path, + archive_names: set[str], ) -> tuple[str, Path]: normalized = link_name.replace("\\", "/") + repaired = _try_repair_missing_slash(member_name, normalized, archive_names) + if repaired is not None: + normalized = repaired link_path = Path(normalized) if link_path.is_absolute(): raise PrebuiltFallback( @@ -3473,8 +3515,10 @@ def extract_archive(archive_path: Path, destination: Path) -> None: def extract_tar_safely(source: Path, base: Path) -> None: pending_links: list[tuple[tarfile.TarInfo, Path]] = [] + archive_names: set[str] = set() with tarfile.open(source, "r:gz") as archive: for member in archive.getmembers(): + archive_names.add(member.name) target = safe_extract_path(base, member.name) if member.isdir(): target.mkdir(parents = True, exist_ok = True) @@ -3501,7 +3545,7 @@ def extract_archive(archive_path: Path, destination: Path) -> None: progressed = False for member, target in unresolved: normalized_link, resolved_target = safe_link_target( - base, member.name, member.linkname, target + base, member.name, member.linkname, target, archive_names ) if not resolved_target.exists() and not resolved_target.is_symlink(): next_round.append((member, target))