Studio: fix the "No model loaded" error, and optionally auto-download a model named in an API request - #7454
Conversation
A /v1 request naming a model that is not downloaded returned the generic "No model loaded. Call POST /inference/load first.", which cannot fix it. Return 404 model_not_found naming the model and listing what can serve, and make the API usage examples name a model the server actually has.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b965a285ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // No local checkpoint (or an external provider, which /v1 cannot serve): | ||
| // name something this server actually holds. | ||
| return ( | ||
| catalog.find((m) => m.loaded)?.id ?? catalog[0]?.id ?? MODEL_FALLBACK |
There was a problem hiding this comment.
When no local checkpoint is loaded, catalog starts as an empty array and /v1/models is fetched only after the first render, so the examples initially expose MODEL_FALLBACK while the filesystem scan is pending. A user who copies the snippet during that interval—potentially several seconds on a large cache—still gets the exact unavailable hardcoded model this change is intended to eliminate; the same happens transiently during a refetch with stale catalog data. Represent the loading state separately and disable/withhold copying until the catalog resolves, using the fallback only after a successful empty result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The catalog is tri-state now (null until /v1/models answers), MODEL_FALLBACK is deleted, and with nothing servable the panel says to load or download a model rather than rendering a copyable snippet that names an id the server cannot serve.
Fixed in 723528d.
| mid for mid in (m.get("id") for m in await _openai_catalog_objects()) | ||
| if isinstance(mid, str) and mid |
There was a problem hiding this comment.
On the GGUF-only /v1/completions, /v1/embeddings, /v1/messages, token-counting, and streaming /v1/responses guards, this helper consumes the full catalog, which also includes the active Transformers model from _openai_model_objects(). If a Transformers model is resident and the caller names that exact model on one of these endpoints, resolve_local_gguf() misses it and the new response falsely says the model is not downloaded while simultaneously listing it as available; naming the suggested entry cannot satisfy these GGUF-only endpoints. Preserve the generic “No GGUF model loaded” diagnosis for a resident non-GGUF model, or filter suggestions according to the endpoint’s backend requirements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. GGUF-only paths keep the generic no-GGUF-model error when the resident model is a Transformers one, instead of a 404 that lists that same model as available. Covered in test_openai_auto_switch.py.
Fixed in 723528d.
| if not available: | ||
| return ( | ||
| f"The model '{requested_model}' is not downloaded on this server, and no " | ||
| "models are downloaded yet. Download one in Unsloth Studio." |
There was a problem hiding this comment.
When the real filesystem scan fails, _cached_local_catalog() catches the exception and returns an empty list, so the outer diagnosis try never sees an error and this branch confidently reports that no models are downloaded. A permissions error, database failure while enumerating custom folders, or other collect_local_models() failure can therefore turn the former generic 400/503 into a false 404 claiming the requested model is absent; the added test only mocks _openai_catalog_objects() to raise and does not exercise the production exception-swallowing path. Preserve a scan-failure signal so this diagnosis can degrade to the original generic error rather than treating failure as an authoritative empty inventory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. It needs an unreadable models dir or HF cache, which is an environment fault rather than input, and the cause named here is already caught inside collect_local_models. Nothing is servable either way, so only the wording differs.
… quant The monitor rendered all 50 retained entries in one scroller: page it 5 at a time, freezing history while paged back so live traffic cannot reorder it. Add model load/unload rows so the feed shows what the server is doing, and stop the header rendering the loaded model as a raw host path. Advertise each model's GGUF quant on /v1/models so the example pins repo:QUANT, and move the auto-switch section above the monitor with shorter copy.
for more information, see https://pre-commit.ci
danielhanchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88f1e5eb4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )} | ||
| </div> | ||
|
|
||
| {ordered.length > PAGE_SIZE ? ( |
There was a problem hiding this comment.
When a user is viewing an older page, frozenIds remains fixed while the backend's 50-entry retention window evicts those IDs as new traffic arrives. Once five or fewer frozen entries remain, this condition hides the entire pager even though frozenIds is still non-null, leaving the console permanently frozen—with an empty grid once all captured IDs expire—and no way to return to the live list without remounting the component. Render a way back to page 1 whenever a snapshot is frozen, regardless of ordered.length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The pager stays mounted while frozen ids are held, and Newer is enabled while a freeze is active, so entries expiring under a reader can no longer leave an empty grid with no way back.
Fixed in 723528d.
Auto-switch only ever loaded models already on disk, so naming one this server does not have either 404s or, when something else is loaded, gets quietly answered by the resident model. Add openai_api_auto_download_model (off by default, gated on auto-switch). When on, a /v1 request naming a GGUF repo that is not downloaded starts a background fetch and returns 503 with Retry-After and a typed model_downloading code. The resident model keeps serving in the meantime, and the retry after the download completes is served by the new model through the existing auto-switch path. The download reuses the Hub manager's service layer, which already does repo-id validation, casing, claim bookkeeping, disk preflight, resume and cancel. The in-loader download is deliberately not used: it silently falls back to a smaller quant under low disk, which is wrong when the caller named an exact one. Admission is narrow, since a request only needs an API key: - namespace/name only, so gpt-4 and other foreign ids fall through to the resident model exactly as before - GGUF only, decided from the remote file list rather than the repo name - anything declaring auto_map is refused, so trust_remote_code stays a deliberate opt-in in the UI and can never be granted over the API - a single download at a time, plus a free-disk reserve - one model_info call answers existence, gating and the quant list, so a missing repo, a gated repo and a wrong quant each get their own error With the setting off every one of these paths is byte-identical to before. Also: - monitor rows for downloads, with a live percentage - public_model_id resolves an HF cache snapshot to its repo id, so a cache-loaded model is no longer labelled with a commit sha; this drops the duplicate helper added for the monitor and fixes the same leak in the inference status response - the unedited sk-unsloth-YOUR_KEY from the copyable examples now says so instead of "Invalid or expired API key"; every other bad key keeps the generic message
danielhanchen
changed the title
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80cb3a121
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _is_mmproj(name) or _is_mtp_drafter(name) or _is_big_endian_gguf_path(name, quant): | ||
| continue | ||
| sizes[quant] = sizes.get(quant, 0) + int(getattr(sibling, "size", 0) or 0) |
There was a problem hiding this comment.
When a GGUF repo includes an mmproj or MTP drafter, this filter excludes those files from expected_bytes, but the download manager's variant plan includes the preferred companion files in the actual download (hub/utils/gguf_plan.py, lines 160-166 and 186-206). A repo with a companion larger than the nominal reserve can therefore pass _enough_disk() and then consume the reserved space or exhaust the filesystem. Calculate the preflight size from the same complete variant plan used by the downloader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Admission sizes each quant from build_gguf_variant_plans().download_size_bytes, so the reserve is measured against the mmproj and MTP companions the worker actually fetches rather than the main files alone.
Fixed in 723528d.
| await downloads.download_model_response( | ||
| DownloadModelRequest(repo_id = repo_id, gguf_variant = variant), hf_token | ||
| ) |
There was a problem hiding this comment.
When an existing same-repo safetensors download, cross-transport variant download, or deletion conflicts with this request, download_model_response() returns a normal response with accepted: false rather than raising (hub/services/models/downloads.py, lines 188-199). Ignoring that response makes this path create a running lifecycle row and return model_downloading even though no worker was launched; the watcher then polls the idle requested key and reports a spurious failure. Inspect accepted/state and return the busy refusal without installing _active when dispatch was rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. _dispatch checks the service's accepted field and returns model_download_busy instead of opening a lifecycle row and a watcher for a worker that was never launched. Test: test_a_refused_dispatch_is_not_reported_as_downloading.
Fixed in 723528d.
| try: | ||
| return await _admit_and_start(repo_id, wanted_variant, requested_model, hf_token) | ||
| except Exception: | ||
| _release(repo_id) | ||
| raise |
There was a problem hiding this comment.
If the request task is cancelled while awaiting the Hub probe, consent check, or dispatch, asyncio.CancelledError bypasses this except Exception handler and leaves _active installed with variant=None. From then on, other repositories always receive model_download_busy, while retries for the same repository report it as downloading even though no watcher or worker may exist, so auto-download remains wedged until the process restarts. Release the provisional slot in cancellation-safe cleanup while still re-raising cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The cleanup catches BaseException, since CancelledError has not been an Exception since 3.8. Test: test_a_cancelled_admission_does_not_wedge_the_slot.
Fixed in 723528d.
| except Exception as exc: | ||
| logger.debug("auto-download: status probe failed for %r: %s", repo_id, exc) | ||
| return "idle", None |
There was a problem hiding this comment.
If the local download-status lookup raises transiently, converting that failure to idle makes _watch() take its “worker vanished” branch, mark the lifecycle row failed, and release the single-flight slot even though the download worker can still be running. A later request may then start another repository download concurrently, defeating the one-download-at-a-time admission guarantee. Preserve an unknown/error probe state and retry it rather than treating an inability to read status as authoritative idleness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. _job_state returns unknown rather than idle when the probe raises, and both the watcher and the request path treat it as still in flight, so a transient read cannot free the slot under a live download.
Fixed in 723528d.
| if quants: | ||
| obj["quant"] = quants[0] |
There was a problem hiding this comment.
When a local repository contains multiple quants, local_gguf_quants() preserves the order from list_local_gguf_variants(), which is sorted by descending file size (utils/models/model_config.py, lines 1941-1950). Selecting quants[0] therefore advertises F16/F32 or another largest quant, and the usage-examples panel appends that quant to the model ID, forcing an unnecessarily large load that can fail VRAM admission even though a normal preferred Q4 quant is present. Choose the quant with the loader's _pick_best_gguf preference rather than by file size.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. quants[0] is what resolve_local_gguf loads for the bare id, so pinning it forces nothing larger, and both the ordering and the pick predate this PR. Switching to _pick_best_gguf would make the advertised quant disagree with what the bare id resolves to.
| status = hf_error_status(exc) | ||
| if status == 403: | ||
| return AutoDownloadRefusal( |
There was a problem hiding this comment.
When the caller-provided Hub token or the stored ambient token is expired or invalid, HfApi.model_info() can fail with HTTP 401 and hf_error_status() preserves that status, but this branch handles only 403 and 404. The 401 therefore falls through to model_lookup_failed with a 503 and a retry instruction, so users repeatedly retry a credential problem instead of being told to replace the token. Handle 401 as an authentication/access refusal with actionable token guidance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise does not hold here. hf_raise_for_status converts repo-API 401s into RepositoryNotFoundError, and hf_error_status maps by class name to 404, so the status fallback this describes is never reached.
The monitor names the loaded model but offered no way to free it. Idle auto-unload is the only existing release path, and it needs a TTL and a wait. The button sits next to Refresh, appears only while a model is loaded and is disabled mid-unload. /unload matches on the internal identifier, which this response deliberately omits because it would be a host path, so the click reads it from /api/inference/status the same way the chat runtime does rather than widening the monitor payload. Also stamp the manual unload row with the quant, read before the teardown clears it, so it reads repo:QUANT like the load row it pairs with.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 658f106fc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # which swallows errors into False. Fine as a UI hint, wrong as an admission. | ||
| from utils.security.consent import _config_has_auto_map | ||
|
|
||
| has_auto_map = await asyncio.to_thread(_config_has_auto_map, repo_id, hf_token) |
There was a problem hiding this comment.
When the repository configs are uncached and Hugging Face is slow, this synchronous check can keep the API request waiting well beyond _MODEL_INFO_TIMEOUT_S: _config_has_auto_map() performs up to five sequential hf_hub_download() calls without the eight-second bound used by the preceding metadata probe. Wrap the check in a bounded wait or otherwise apply a shared deadline so enabling auto-download cannot leave ordinary /v1 requests stalled for the cumulative Hub timeouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The consent probe sits behind a hard 8s model_info whose failure returns 503 before it runs, and it measures 0.16-0.29s in practice. The suggested wait_for(to_thread(...)) would not cancel the thread, it would only leak executor workers.
| <div className="mt-1 truncate text-ui-11 text-muted-foreground"> | ||
| {entry.model} | ||
| </div> |
There was a problem hiding this comment.
When a background load or download fails after the initiating request has already returned model_downloading, the backend puts the only diagnostic in entry.error, but this component renders only the model name and lifecycle rows cannot be expanded. The console therefore reports merely “Model download failed” or “Model load failed,” even though callers were explicitly told to track the operation there; render the error text for failed lifecycle entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The load half of this is not accurate: that failure path writes a contentless message, not a detailed one. The download error is already available from the download-status endpoint and from the 502 an adopted retry receives. Reading this as a feature request rather than a bug.
| if auto_switch_on and not reload_only: | ||
| await _maybe_auto_download_model(requested_model, fastapi_request) |
There was a problem hiding this comment.
When a chat, Responses, or Anthropic request contains image/audio input and names a missing text-only GGUF repository, this miss path starts the multi-gigabyte download before applying the require_vision compatibility guard, which only runs later after the model exists locally. The retry is then rejected as unsupported, so an input that can never be served has already consumed disk and bandwidth; propagate the capability requirement into admission and reject repositories without the required companion before dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The download does happen, but under a double opt-in on a model the user named by hand, and the outcome is a correct 503 then 400 with weights that stay usable for text. That is an efficiency ask rather than a correctness bug.
It only rendered while a model was loaded, which hid the one manual release path at exactly the moment someone goes looking for it. Render it always, disabled with a "No model is loaded" tooltip when there is nothing to free.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0104baa6b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _release(repo_id: str) -> None: | ||
| global _active | ||
| with _lock: | ||
| if _active is not None and _active.repo_id == repo_id: | ||
| _active = None |
There was a problem hiding this comment.
Keying _release() only by repo_id lets a stale operation clear a newer download for the same repository. For example, after variant A enters error, an adopting request releases the slot and a retry can start variant B; when A's still-running watcher reaches its finally, this check matches B's repository and clears B from _active, allowing another repository to be admitted concurrently with B. Pass the expected _Active instance or a generation token to _release() and clear the slot only if it still owns that exact operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. _release compares the _Active object by identity now, and the claim is threaded through _admit_and_start and _dispatch, so a stale watcher can no longer clear a newer owner of the slot. Regression test: test_a_stale_watcher_cannot_release_a_newer_download.
Fixed in 723528d.
| api_monitor.fail_open(active.monitor_id, "Download timed out") | ||
| except asyncio.CancelledError: | ||
| raise | ||
| except Exception as exc: | ||
| logger.warning("auto-download: watcher failed for %r: %s", active.repo_id, exc) | ||
| api_monitor.fail_open(active.monitor_id, "Download tracking failed") | ||
| finally: | ||
| _release(active.repo_id) |
There was a problem hiding this comment.
When a legitimate download remains running beyond _MAX_WATCH_S—for example, a large quant over a slow connection—the loop marks only the monitor row as timed out and then the unconditional finally releases _active; it neither cancels nor waits for the Hub worker. A subsequent request can therefore launch a second multi-gigabyte download while the first continues, violating the advertised single-download admission guarantee. Confirm or cancel the underlying job before freeing the slot, or continue tracking a status that is still authoritatively running.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
_MAX_WATCH_S is 24 hours and the release is a deliberate stall guard, commented as such. Pinning the slot forever instead is the worse failure mode.
| for part in str(path).replace("\\", "/").split("/"): | ||
| if part.startswith("models--"): | ||
| return part[len("models--") :].replace("--", "/") |
There was a problem hiding this comment.
Treating any path segment beginning with models-- as an HF cache repository misidentifies ordinary local paths such as /srv/models--archive/model.gguf: every model beneath that directory is exposed as archive instead of its own filename. This can collapse distinct catalog entries and causes responses and lifecycle rows to report an ID that clients cannot reliably resolve. Only decode the segment when the surrounding path has a valid Hugging Face cache layout (or otherwise validate the decoded repository ID).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The decode is upstream's own (_scan_cached_repo splits on -- the same way), and requiring a namespace would be wrong: the cache docs list models--bert-base-cased as valid. Worst case here is one duplicate id in a listing.
Asking for a model this server is not serving returned 200 from whatever was resident. Requesting gemma-4-E2B-it-GGUF:UD-Q6_K_XL while UD-Q4_K_XL was loaded got a confident answer from the wrong quant, with nothing in the response saying so. A name carrying a namespace (org/model, optionally :QUANT) is a concrete reference, so 404 instead, with the reason: - wrong quant -> names the quants that are actually downloaded - not on disk -> lists what is available - on disk but auto-switch off -> says to turn it on Ids without a namespace (gpt-4, claude-3, default) are foreign labels rather than references, so they still fall through to the resident model and drop-in clients are unaffected. A bare org/model is still satisfied by any loaded quant of that repo; only an explicit :QUANT must match. The check runs whatever the auto-switch and auto-download toggles are, since serving the wrong weights is wrong in every configuration. It is skipped when nothing is loaded, where the existing no-model-loaded error already says the right thing, and when the model is on disk with auto-switch on, where a failed swap should still fall back.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f7bb5877e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| active = getattr(get_inference_backend(), "active_model_name", None) | ||
| if not active: | ||
| return False | ||
| return base in {active.lower(), (public_model_id(active) or "").lower()} |
There was a problem hiding this comment.
When a Transformers model such as org/model is active and the request names org/model:Q4_K_M, this branch compares only the parsed base ID and ignores the requested variant. With auto-download disabled, _maybe_auto_switch_model therefore treats the request as satisfied and serves the Transformers checkpoint even though the caller explicitly selected a GGUF quant. Non-GGUF backends should never satisfy a variant-suffixed reference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, narrowly. _loaded_satisfies refuses an explicit quant on a backend that has no quant identity to compare. Gated on the suffix actually being a quant label, so Ollama-style :latest and :8b tags still match on the repo alone.
Fixed in 723528d.
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [needsCatalog]); |
There was a problem hiding this comment.
When this hook fetches an empty or otherwise incomplete catalog and a model is subsequently downloaded without being loaded, needsCatalog remains true, so this effect never runs again and the examples keep using the hardcoded fallback for the rest of the component's lifetime. The comment promises updates when model availability changes, but the dependency only observes whether the current checkpoint is usable; poll or invalidate/refetch the catalog when local downloads complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The effect depends on the checkpoint and variant now, and retries while /v1/models is still empty, so a download finishing while the panel is open updates the snippet.
Fixed in 723528d.
| await _resolve_and_switch() | ||
| # The switch may have missed (not downloaded, or a swap that failed): refuse | ||
| # rather than let the handler answer as whatever is still resident. | ||
| await _reject_unservable_model(requested_model, fastapi_request) |
There was a problem hiding this comment.
When idle-unload has stashed a model and a request names an undownloaded namespace/model while auto-download is off, the miss path restores the unrelated stashed model before reaching this newly added rejection. The request then returns 404 anyway, but only after a potentially multi-minute model load that consumes VRAM and defeats the purpose of the idle unload. Determine that a concrete model reference is unavailable before restoring the fallback stash, while retaining the reload behavior for bare compatibility aliases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The trace is right, but the client already gets the correct 404 and the sequence is pinned by two existing tests. Cheap-check-first efficiency ask rather than a bug.
"What is Unsloth Studio?" rather than "Can Unsloth Studio do API calling?". One constant feeds all nine snippet tabs.
A namespace alone was treated as a concrete model reference, so a /v1 request naming anthropic/claude-3.5-sonnet, openai/gpt-4o or any other LiteLLM or OpenRouter style vendor/model id started returning 404 instead of being answered by the resident model. Refuse only on evidence the caller meant this server: an explicit GGUF quant label, or a repo that is actually on disk here. gpt-4 and vendor/model alike fall through again, while the wrong-quant and wrong-repo cases this PR exists for still refuse. Also from review: - Release the single download slot by object identity, not repo id. A stale watcher could clear a newer download of the same repo and let a second multi-GB fetch start alongside it. - Catch BaseException around admission: CancelledError is not an Exception, so a cancelled request stranded the slot for the process lifetime. - Honour the download service's accepted=False, which it returns without raising for a cross-variant conflict, instead of promising a download that was never dispatched. - Treat a failed status probe as unknown rather than idle, so a transient read cannot fail the monitor row and free the slot under a live worker. - Check gated repos with auth_check. The Hub serves metadata for a gated repo without granting its files, so the licence gate was being reported as the unrelated custom-code refusal. - Size the disk reserve from the download plan, which includes the mmproj and MTP companions the worker fetches with every quant. - Never fetch under the server's own HF token. The repo is named by whoever holds an API key, so the ambient token let that key pull the owner's private repos. - Refuse an explicit quant on a backend with no quant identity, gated on the suffix really being a quant so Ollama style :latest tags still match. - Raise instead of falling through when the diagnosis fails: the mismatch is already established by then, only the wording is uncertain. - Report a failed switch as 503 model_switch_failed rather than answering as the resident model. - Fail an open monitor row under the same lock as the check, so a finish landing in between cannot stamp an error onto a completed row. - Usage examples never emit a hardcoded model id: the catalog is tri-state and the panel asks for a model to be loaded instead of printing one the server cannot serve. It also refreshes when the loaded model changes. - Keep the monitor pager reachable while frozen entries expire.
for more information, see https://pre-commit.ci
|
@codex review |
|
Review round. 12 independent reviewers plus the review comments above; every item triaged, 10 taken and 9 declined with reasons in the threads. The one that mattered most was mineAn earlier revision of this PR treated any
A namespace is not evidence of intent. Refusal now needs an explicit GGUF quant label, which no foreign id carries, or a repo that is actually on disk here. Both cases this PR exists for still refuse; Ollama style Download coordinatorFour separate ways the single-download guarantee could be broken, all with executed reproductions:
Also
TestsThe two Hub exception fixtures were constructed positionally, which fails on Studio backend suite: 9118 passed, 28 failed, all 28 identical to the pre-existing baseline on this box ( DeclinedNine items, each answered in its own thread. The main ones: the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d76f56d372
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if status == 403: | ||
| return _gated_refusal(repo_id) | ||
| if status == 404: | ||
| _mark_not_servable(repo_id) |
There was a problem hiding this comment.
Do not globally cache this 404 without accounting for hf_token: Hugging Face deliberately returns 404 for a private repository when the caller lacks access, so one unauthenticated request for a bare private GGUF ID marks it unservable for every caller. A retry with a valid X-Unsloth-HF-Token then skips the Hub probe for 10 minutes; if another model is resident, the request can fall through and be answered by the wrong model instead of downloading the requested one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The cache is keyed per credential now (repo id plus a digest of the token, never the token itself), so an anonymous 404 on a private repo cannot silence a caller who has access to it. Tests: test_an_anonymous_404_does_not_silence_an_authorised_caller, test_the_cache_is_per_token.
| await unloadModel({ model_path: checkpoint }); | ||
| setError(null); | ||
| await loadMonitor(); |
There was a problem hiding this comment.
When this new Unload button is used while model auto-switch is disabled, the backend unload succeeds but this path only refreshes the monitor and never clears or reconciles useChatRuntimeStore.params.checkpoint. The chat runtime therefore continues treating the removed checkpoint as loaded, and the usage examples can keep naming it, until another action performs a full model refresh; the existing ejectModel flow explicitly calls clearCheckpoint() after the same API operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The monitor unload calls clearCheckpoint() after the unload, matching the chat eject flow. Without it the runtime kept the freed checkpoint and the usage examples went on naming it.
| f"'{repo_id}' is gated on Hugging Face. Accept its licence and add an " | ||
| "access token in Unsloth Studio, then retry." |
There was a problem hiding this comment.
The suggested recovery cannot work for an API auto-download: this path deliberately ignores Studio's ambient/stored Hugging Face token and only accepts a caller-supplied X-Unsloth-HF-Token header. A user who follows this message by adding a token in Studio and retrying the same API request will therefore receive the same 403 indefinitely; the error should identify the required request header rather than directing them to an unused setting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The message names X-Unsloth-HF-Token and says outright that automatic download never uses the server's own Hugging Face identity, since the previous wording pointed at a setting this path deliberately ignores. Applied the same clarification to the 404, where a private repo also reads as absent.
The Hub answers 404 for a private repo the caller cannot see, so caching that verdict per repo alone let one anonymous request mark a private repo unservable for everyone for the whole TTL. A later caller sending a valid X-Unsloth-HF-Token skipped the probe and fell through to the resident model instead of downloading what it asked for. Keyed on the repo id plus a digest of the token now, so the token itself is never held. Two more from the same review: - Clear the chat runtime checkpoint after unloading from the API monitor, as the chat eject flow already does. The store went on treating the freed checkpoint as loaded and the usage examples kept naming it. - Point gated and not-found callers at the X-Unsloth-HF-Token header. Automatic download deliberately ignores the server's own Hugging Face identity, so telling the user to add a token in Studio sent them round the same 403 forever.
|
@codex review |
…rted The trust probe was the last credential handoff still passing a raw token. huggingface_hub reads None as "use the cached login", so a caller-named repo was read with this server's Hugging Face identity whenever the caller sent none, which is exactly the isolation the metadata probe and the worker already keep. It takes _hub_token now. Enumerated the rest of that path while there: auth_check, model_info and spawn_worker were already correct. finalize_worker_exit is shared with dataset downloads, so the resolver hook fired for every completed dataset, scanning the model directories for nothing and recording the dataset id as local-model evidence, which turns a bare /v1 request naming that id into a refusal instead of a foreign-id fallthrough. Gated on repo_type. _already_serving decided "bare" on the presence of a colon while _loaded_satisfies and the resolver decide it on whether the suffix names a quant, so org/model:latest against a serving Q8_0 read as a mismatch and swapped in the preferred Q4_K_M for a request either one answers. That rule now lives in four places, each fixed in its own round, so this time I looked for the rest and found a fifth: describe_local_miss splits on the bare colon and its docstring claims it splits like the resolver. It no longer did, and would report a missing quant named "latest". Fixed here too, unreported.
for more information, see https://pre-commit.ci
…/unslothai/unsloth into fix/openai-model-not-found-error
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9e8fa3e15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif current is not None: | ||
| return AutoDownloadRefusal( |
There was a problem hiding this comment.
When auto-download is enabled and one repository is already downloading, the first request for any uncached namespaced foreign label (for example anthropic/claude-3.5-sonnet) takes this branch before the Hub probe can determine that the label is not a servable GGUF repository. The route consequently raises model_download_busy instead of falling through to the resident model, so ordinary LiteLLM/OpenRouter-style traffic can be blocked for the entire duration of a multi-hour download. Apply the busy refusal only after establishing positive evidence that the newly requested label is another downloadable model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the drop-in compatibility claim breaking for the second time on this PR, which is the one I care most about not getting wrong. The busy refusal fired before anything established the label was even a model, so any namespaced id a LiteLLM or OpenRouter style client sends was told to wait out an unrelated multi-hour download.
The slot decision is now split: the branch records that something else holds it, then _is_downloadable_model probes before refusing. Only a label the Hub actually serves as GGUF gets the busy refusal; anything else returns None and falls through to the resident model exactly as before. Any probe failure answers False, since stranding ordinary traffic is much worse than missing a busy refusal, and a negative result is cached so repeats are free.
test_a_foreign_label_is_not_told_to_wait_for_someone_elses_download drives a real download, then asserts anthropic/claude-3.5-sonnet falls through while a genuine second GGUF repo still gets the 503.
| warm_index_soon() | ||
| resolved = resolve_local_gguf(requested_model, allow_scan = False) |
There was a problem hiding this comment.
When auto-switch is off, the first request after startup that names a bare downloaded model different from the resident model reaches this check with an empty index. warm_index_soon() only starts a background thread, so the immediate cache-only lookup misses; because a bare ID is not quantified, the function then returns without an error and the request is silently answered by the resident model. A stale index has the same window after a model is added manually. Wait for or synchronously perform the cold lookup before treating the absence of cached evidence as permission to serve different weights.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and you are overruling a trade-off I made deliberately and said so in-thread, so let me be explicit that I now think you are right. I treated an unbuilt index as "no evidence, fall through", to keep the multi-second scan off the request path. That is the performance fix protecting the exact case the PR exists to prevent.
Split by whether the index has ever been built. Cold, it is scanned once, on a thread so it does not block the loop, bounded by a timeout so a pathological install falls through rather than hanging the request open. Built, the request path still never scans, so the original regression stays fixed: the first request after startup pays once, every one after reads the cache.
test_a_cold_index_is_scanned_rather_than_read_as_nothing_here pins the case you describe, a bare downloaded id against a different resident model, and asserts the scan happens exactly once. test_a_cold_scan_that_never_finishes_does_not_hang_the_request pins the bound.
| else: | ||
| api_monitor.fail_open(active.monitor_id, error or f"Download {state}") | ||
| return |
There was a problem hiding this comment.
When the worker enters error, the watcher records the monitor failure and then immediately releases _active in finally. Since the advertised retry interval is 30 seconds while the watcher polls every 2 seconds, the next client retry normally finds no active operation and starts the same download again instead of reaching the model_download_failed branch in maybe_auto_download. A deterministic worker failure can therefore produce an endless sequence of misleading model_downloading responses and repeated jobs; retain a terminal error long enough for the next retry to surface it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. The watcher freed the slot the instant it saw the error, while Retry-After is 30s and the poll is 2s, so the client reliably came back to an empty slot and started the same failing download again, and the model_download_failed branch was effectively unreachable.
The failure is now held on the slot: _Active carries the error and when it failed, the watcher does not release, and the next retry surfaces the 502 and frees it. A client that never returns cannot hold it forever either, since a different repo takes the slot once the hold is older than three retry intervals.
Two tests, because I first wrote one that passed with and without the fix and could not explain why. I replaced it rather than ship a test I did not understand: test_a_failed_download_keeps_the_slot_until_someone_is_told builds the slot state explicitly and drives the real watcher, and test_the_retry_after_a_failure_is_told_instead_of_restarting_it asserts the retry gets 502 and dispatches nothing. Both verified to fail without the change.
| if state == "complete": | ||
| # Drop the resolver cache so the next retry sees the new model. | ||
| await asyncio.to_thread(invalidate_index) |
There was a problem hiding this comment.
On successful completion, finalize_worker_exit already invalidates the resolver and starts warm_index_soon(). The watcher usually observes complete after that work has started or finished, and this second invalidation either waits for the warm scan's lock and then marks its fresh result stale, or invalidates it immediately afterward. Consequently the client's first retry performs another multi-directory scan synchronously in resolve_local_gguf, defeating the completion-time warmup and adding seconds of latency on large caches. Remove or coordinate this duplicate invalidation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Once the invalidation moved to finalize_worker_exit, the watcher copy became a duplicate that lands after the warm has started, so it marks the fresh scan stale and hands the client a synchronous rescan on the retry, undoing the completion-time warm.
Removed from the watcher. The completion hook covers every download, this one included, so nothing is lost.
test_a_completed_download_does_not_restage_the_scan_it_just_warmed asserts the complete branch no longer invalidates.
| keys = { | ||
| candidate.lower() |
There was a problem hiding this comment.
On a case-sensitive filesystem, a legacy request naming /srv/models/foo.gguf is considered satisfied by a resident /srv/models/Foo.gguf because both the request and every backend identifier are lowercased here. _reject_unservable_model returns immediately on this result, bypassing the later _norm_path comparison that deliberately preserves case, so two distinct local models can still be silently treated as the same weights. Use path-aware normcase comparison for path identifiers while retaining case-insensitive matching only for repository aliases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the sixth place that same path-versus-alias rule lives. _loaded_satisfies lowercased the request and every backend identifier, so on a case-sensitive filesystem it returned satisfied before the case-preserving _norm_path compare below ever ran, which made the earlier fix to that compare unreachable for path-shaped requests.
Comparison now goes through one _matches_any helper: a filesystem path is compared with _norm_path, so case matters where the filesystem says it does, and a repo alias stays case-insensitive.
test_two_local_paths_differing_only_in_case_are_not_the_same_model asserts both, including that unsloth/qwen3-4b-gguf still matches unsloth/Qwen3-4B-GGUF.
The busy refusal fired before anything established the requested label was a model at all, so any namespaced id a drop-in client sends was told to wait out an unrelated download for as long as it ran. Probe first and refuse only a label the Hub actually serves as GGUF; anything else falls through to the resident model as before. A probe failure answers "not downloadable", since stranding ordinary traffic costs more than missing a busy refusal. Treating an unbuilt index as "nothing here" let the first request after startup be answered by the resident model under another model's name. That was a deliberate trade to keep the scan off the request path, and it was the wrong one. Cold, the scan now runs once on a thread, bounded so a pathological install falls through rather than hanging the request. Built, the request path still never scans, so the latency fix stands. The watcher freed the slot the moment it saw an error, while Retry-After is thirty times the poll interval, so the client came back to an empty slot and restarted the same failing download instead of being told. Hold the failure on the slot until a retry surfaces it, and let another repo take it after three retry intervals so a client that never returns cannot keep it. The watcher also invalidated on completion, which now lands after finalize_worker_exit's warm and marks that fresh scan stale, pushing a synchronous rescan onto the retry. Removed. _loaded_satisfies lowercased paths as well as aliases, so it returned satisfied before the case-preserving compare below could run. Both now go through one helper: paths compare with normcase, aliases stay case-insensitive.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e32b486170
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| asyncio.to_thread(resolve_local_gguf, requested_model), | ||
| _COLD_INDEX_WAIT_S, | ||
| ) | ||
| except (TimeoutError, asyncio.TimeoutError): |
There was a problem hiding this comment.
When the initial model-directory scan takes longer than 10 seconds, this timeout sets resolved to None; for a bare local model absent from the catalog cache, quantified and here then remain false, so _reject_unservable_model() permits the request and the unrelated resident model answers it. Fresh evidence after the prior cold-index fix is this newly bounded fallback: timing out should conservatively reject the named request while the scan finishes rather than treating an incomplete scan as absence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct: I bounded the wait and then treated the bound being hit as absence, which is the same hole one branch over.
A timeout now answers 503 model_indexing with a Retry-After, and leaves the warm running, rather than guessing. That does mean a foreign label sent in that window gets told to retry instead of falling through, which I considered against the drop-in compatibility point from the previous round. I took it because the window is one request on an install whose scan exceeds ten seconds, it is self-clearing, and a retryable "not yet" is honest where serving the wrong weights is not.
Finding this turned up something worse that you did not flag. All of these checks run inside a broad except Exception whose job is "could not verify, so fall through". My raise landed inside it, so the 503 was logged as a verification failure and the request was answered by the resident model anyway. Any refusal decided in that block would have been swallowed the same way. HTTPException is now re-raised ahead of that handler, and test_a_refusal_is_never_swallowed_by_the_cannot_verify_handler holds it.
I only caught it because the new test passed when it should not have, and I instrumented the path rather than trusting it.
| if wanted and looks_like_quant(wanted): | ||
| lowered = {name.lower(): name for name in variants} | ||
| return lowered.get(wanted.strip().lower()) | ||
| return preferred_quant(variants) |
There was a problem hiding this comment.
When a repository contains multiple generically named GGUFs without recognized quant tokens, such as llama-7b.gguf and llama-13b.gguf, an explicit repo:llama-13b request fails looks_like_quant() and falls through to preferred_quant(), which can dispatch llama-7b instead. Fresh evidence after canonicalizing generic labels is that those labels are now valid worker keys but are still treated as foreign tags here; first case-insensitively match any existing variant label, and only default-select when the suffix matches none and is genuinely a non-quant tag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and a direct consequence of canonicalizing generic labels: those became real variant keys, but the matcher still decided on shape alone, so repo:llama-13b fell past an exact match and default-selected llama-7b.
Exact match now runs first whatever the shape. A quant-shaped suffix that matches nothing is still a miss rather than a swap, which is the rule separating this from the loader low-disk fallback, and a tag that matches nothing and is not quant-shaped still default-selects.
test_an_exact_generic_variant_beats_the_default_pick asserts :llama-13b dispatches llama-13b, and that :Q2_K against a repo without it still refuses without dispatching.
| # The id stays bare for OpenAI compat; a client appends ":<quant>" to pin one. | ||
| if quants: | ||
| obj["quant"] = quants[0] |
There was a problem hiding this comment.
When a non-preferred quant such as Q8 is manually loaded by absolute path and the catalog exposes the same directory under a different alias, the alias is marked loaded: true but this field still publishes the preferred local quant, typically Q4. Fresh evidence after the alias-residency fix is this inconsistent combination: clients requesting the advertised alias:Q4 with switching disabled receive a model-not-found response even though the listing claims that alias and quant are loaded; use the resident backend's variant whenever the path resolves to the resident model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is two of my own fixes meeting badly: marking the alias loaded, then publishing the preferred on-disk quant next to it, so the listing claimed alias:Q4 was loaded while Q8 was serving, and requiring the resident quant to match made pinning it a 404.
When the entry resolves to the resident model the advertised quant is now the resident hf_variant. Unloaded entries keep the preferred on-disk quant, which is a quant that actually exists for them.
test_a_loaded_alias_advertises_the_quant_that_is_actually_loaded pins Q8_0 resident behind a publisher/model alias and asserts the listing says Q8_0.
…ot a failure Bounding the cold scan then reading the bound as "not here" left the same hole one branch over. A timeout now answers 503 model_indexing with a Retry-After and leaves the warm running. A foreign label sent inside that window is asked to retry rather than falling through, which is a real cost, but the window is one request on an install whose scan exceeds ten seconds and it clears itself, where answering with the wrong weights does not. That uncovered a worse one. Every check here runs inside a broad except whose job is "could not verify, so fall through", so an HTTPException raised in the block was logged as a verification failure and the request was answered by the resident model. Any refusal decided in there was being swallowed. Re-raise it ahead of that handler. Canonicalizing generic labels made them real variant keys, but the matcher still decided on shape, so repo:llama-13b fell past an exact match and fetched llama-7b. Match exactly first, whatever the shape; a quant-shaped suffix that matches nothing is still a miss and never a swap. Marking a catalog alias loaded while publishing the preferred on-disk quant claimed alias:Q4 was loaded while Q8 was serving, and requiring the resident quant to match then made pinning it a 404. Advertise the resident variant when the entry resolves to the resident model.
There was a problem hiding this comment.
💡 Codex Review
unsloth/studio/backend/routes/inference.py
Lines 4144 to 4148 in a52f028
Preserve case in the already-serving path comparison
Fresh evidence after the other path-comparison fixes is that _already_serving() still lowercases concrete filesystem paths. On a case-sensitive filesystem, if /srv/models/Foo/model.gguf is resident and an alias resolves to /srv/models/foo/model.gguf with the same quant, this check treats them as identical, records the requested alias on the wrong backend, and _reject_unservable_model() subsequently accepts that alias, so the request is answered by the wrong weights. Compare path identifiers with the case-preserving path normalization used by _resolves_to_resident().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "loaded": False, | ||
| # A manual load keys the resident entry by path basename while the catalog uses | ||
| # the alias, so match on the path or the alias reads as not loaded. | ||
| "loaded": _resolves_to_resident(getattr(info, "path", None)), |
There was a problem hiding this comment.
Fresh evidence after the earlier admission fix is that the catalog still calls _resolves_to_resident() with its default llama_only=False. When a Transformers model is active from a directory containing GGUF exports and the catalog exposes that directory under another alias, the alias is marked loaded: true and given a GGUF quant even though no llama.cpp model is resident; usage examples then select the supposedly loaded alias:quant, which cannot be served while switching is disabled. GGUF catalog entries should determine residency from the llama backend only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c24ed12. Every entry in that loop is advertised as GGUF and carries a GGUF quant, so residency there is llama.cpp residency. Passing llama_only = True applies the same rule the quantified admission path already used, and the regression test covers a Transformers model live from a directory that also holds GGUF exports.
| siblings = list(getattr(info, "siblings", None) or []) | ||
| servable = any( | ||
| str(getattr(s, "rfilename", "") or "").lower().endswith(".gguf") for s in siblings | ||
| ) |
There was a problem hiding this comment.
Fresh evidence after adding the Hub probe before model_download_busy is that this probe treats any .gguf sibling as downloadable, while admission later excludes mmproj, MTP-drafter, and big-endian-only files in _gguf_variants(). Thus, while another download owns the slot, a request naming a repository containing only such companion/non-servable GGUFs gets blocked with model_download_busy for the entire download, even though the normal admission path would classify the bare label as non-servable and fall through to the resident model. Reuse the actual variant filtering here before returning busy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c24ed12. The probe now calls _gguf_variants, the same filter admission uses, so a repo holding only companions falls through to the resident model instead of sitting at model_download_busy for the length of an unrelated download. A repo with a real quant is still refused as a second download.
| text = (requested or "").strip() | ||
| base, sep, suffix = text.rpartition(":") | ||
| if not sep or not base or "/" in suffix or not suffix: | ||
| return text, None | ||
| return base.strip(), suffix.strip() |
There was a problem hiding this comment.
When an unrecognized GGUF lives below a subdirectory, the canonical plan key intentionally includes that path (for example build/llama-13b), and is_valid_gguf_variant() explicitly permits slashes. This condition prevents org/repo:build/llama-13b from being split at all, so the whole string subsequently fails repository-ID validation and callers cannot select that worker-supported variant; with multiple nested generic GGUFs, only whichever one the default ranking chooses can be downloaded. Parse the suffix after first validating that the portion before the colon is a Hub repo rather than rejecting slash-bearing variant keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c24ed12. A slash-bearing suffix is now a variant exactly when a real Hub repo precedes it, so org/repo:build/llama-13b parses while C:/models/x.gguf stays a path with no repo before the colon. Parametrised both, plus a nested key behind a non-repo prefix.
| const entry = catalog?.find((m) => sameBaseModelId(m.id, checkpoint ?? "")); | ||
| const backed = catalog === null || (!!entry && (entry.loaded || autoSwitch)); | ||
| if (usableCheckpoint && checkpoint && backed) { |
There was a problem hiding this comment.
When UNSLOTH_MODEL_IDLE_TTL enables idle unload without enabling auto-switch, the backend deliberately retains and reloads the last GGUF on the next request, but this predicate treats the resulting unloaded catalog entry as unusable because it considers only autoSwitch. After the environment-driven idle unload, both the stored-checkpoint branch and fromCatalog() therefore return null and the UI hides runnable examples even though sending the stored model name restores it. Track the settings response's effective idleUnloadActive state and accept the stored checkpoint when the standalone reload stash is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c24ed12. idleUnloadActive is already on the auto-switch settings response, so the panel reads it and accepts the stored checkpoint after a standalone idle unload. It is tracked apart from autoSwitch on purpose: the stash restores the model it freed and nothing else, so fromCatalog must not start picking catalog[0] on the strength of it.
Both tests deleted asyncio.timeout to force _wall_clock_timeout down its pre-3.11 branch, but monkeypatch.delattr raises when the attribute is already absent. On Python 3.10, the one version the fallback exists for, there is nothing to delete, so the two tests errored with AttributeError before reaching the code they cover. Passing raising=False makes the deletion a no-op there and leaves the assertions running against the same branch on every version. Every other delattr in the repo already passes raising=False for exactly this reason. Verified with asyncio.timeout removed from the interpreter: the two tests fail with the CI AttributeError before this change and pass after, and the file still runs 89 passed on 3.13 where the deletion is real.
…le each Four admission and catalog fixes, each closing a gap between two places that were answering the same question differently. The /v1/models catalog asked _resolves_to_resident without llama_only, so a Transformers model live from a directory that also holds GGUF exports marked a GGUF alias loaded and gave it a GGUF quant. The usage examples then pinned alias:quant that nothing could serve with switching off. Every entry in that loop is advertised as GGUF, so residency there is llama.cpp residency. The busy probe accepted any .gguf sibling while admission excludes mmproj, MTP drafters and big-endian builds. A repo holding only companions is not downloadable, so it was held at model_download_busy for the length of an unrelated download instead of falling through to the resident model as it does when no download is running. It now reuses _gguf_variants, the same filter. split_model_ref refused any slash-bearing suffix, but an unrecognized GGUF below a subdirectory keys on its path (build/llama-13b), which is_valid_gguf_variant allows and the catalog advertises. Pinning such a variant could not parse, so only the default-ranked one was reachable. A slash-bearing suffix is now a variant exactly when a real Hub repo precedes it, which still leaves C:/models/x.gguf a path rather than a quant. The usage examples treated a downloaded-but-unloaded model as runnable only under auto-switch, but a standalone UNSLOTH_MODEL_IDLE_TTL reloads exactly what it freed on the next request. The panel hid runnable examples after an idle unload. Tracked apart from auto-switch, because the stash restores the stored checkpoint only and never an arbitrary catalog entry. Also stub the index walk in the three cold-index tests that missed it: a real multi-root scan inside the cold-wait budget made them time out into a 503 under load rather than assert what they are there for. One of them flaked locally. Verified each fix is load-bearing by reverting it and watching its test fail. Backend CI command: 10195 passed, 0 failed. tsc -b and the frontend build clean.
…found-error # Conflicts: # studio/backend/tests/test_research_runs_hardening.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06849261c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| or current.startswith(f"{target}/") | ||
| or target.startswith(f"{current}/") |
There was a problem hiding this comment.
When separately indexed models have nested paths (for example, registered scan folders /models/A and /models/A/sub/B), loading B makes a request for A pass this test because the current path is a descendant of the target. _reject_unservable_model then treats A as resident and serves the request with B (and the catalog can also mark A as loaded); explicit quant requests are likewise misidentified when both models use the same common quant label. Only exact identities, or a deliberately recognized directory-to-weight-file relationship within one model entry, should count as resident.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d82c3d. A prefix match now counts only when no catalog entry sits deeper than the target, which is the innermost indexed model that actually owns the file. With nothing indexed there is no nesting to tell apart, so the directory-to-weights match this exists for is unchanged; the test covers both.
| # _hub_token, not the raw token: None lets huggingface_hub fall back to a cached | ||
| # server login, so a caller-named repo would be probed with this server's identity. | ||
| # Same rule as the metadata probe and the worker. | ||
| has_auto_map = await asyncio.to_thread(_config_has_auto_map, repo_id, _hub_token(hf_token)) |
There was a problem hiding this comment.
When the Hub becomes slow after model_info succeeds, this custom-code check can block the API request far beyond _MODEL_INFO_TIMEOUT_S: _config_has_auto_map performs up to three hf_hub_download calls without a timeout, and the preceding gated-repo auth_check is likewise unbounded. During that wait the provisional _active entry also occupies the single-flight slot even though no download worker has started, so other valid auto-download requests are reported busy until the probe or client request finally terminates. Apply an overall timeout to these secondary admission probes as well as to model_info.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d82c3d. Both probes are bounded now, and each default errs the safe way: an unchecked repo is not a cleared one, so the custom-code probe refuses on timeout, while a slow gated-repo check stays inconclusive because the download's own auth is the real gate. The slot is released either way.
| listOpenAIModels().catch(() => [] as OpenAIModel[]), | ||
| loadOpenAIAutoSwitchSettings() | ||
| .then((s) => [s.enabled, s.idleUnloadActive] as const) | ||
| .catch(() => [false, false] as const), |
There was a problem hiding this comment.
When either refresh endpoint fails transiently, these catches convert “unknown” into authoritative negative state: a /v1/models failure replaces a previously valid catalog with [], while a settings failure reports auto-switch and idle reload as disabled. The selection logic then rejects the known checkpoint or downloaded catalog entries and replaces every copyable example with “No model” until the next retry, even though the server can still serve them. Keep the prior values (or leave the initial catalog pending) on fetch errors rather than feeding fallback negatives into setCatalog and setAutoSwitch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d82c3d. Both fetches now resolve to null on failure and the state is left alone, so a transient error keeps the last answer and retries instead of asserting the server holds nothing.
…del paths Three review fixes plus a test-isolation one. _resolves_to_resident matched on a path prefix, so two separately indexed models that nest (/models/A alongside /models/A/sub/B) both satisfied it: loading B made a request for A resident and answered it with B's weights, and the catalog marked A loaded. A prefix match now counts only when no catalog entry sits deeper, which is the innermost indexed model that actually owns the file. With nothing indexed there is no nesting to tell apart, so the directory-to-weights match this exists for is unchanged. auth_check and hf_hub_download take no timeout of their own, and both ran while the provisional single-flight slot was held, so an unresponsive Hub stalled the request far past the metadata budget and reported every other model busy for the duration. Both are bounded now. Each default errs the safe way: an unchecked repo is not a cleared one, so the custom-code probe refuses on timeout, while a slow gated-repo check stays inconclusive because the download's own auth is the real gate. The usage examples caught a failed refresh into an empty catalog and a disabled auto-switch, which made a transient error authoritative and blanked every example while the model was still servable. The catalog is deliberately tri-state; a failure now keeps the last answer and retries. Also start the backend tests from a built, empty model index. Stubbing only the background warm still left the cold path walking real caches synchronously inside the admission wait, so on a large install a test asserted against a 503 "still indexing" instead of its subject. _build_index is untouched, so the tests that call it directly still exercise the real walk. Verified each fix is load-bearing by reverting it and watching its test fail. tsc -b clean. Backend CI command green apart from two failures reproduced only on this box (a real model-dir scan and an orphan-process cleanup), neither touched by this PR; staging CI is the gate for those.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d82c3d13e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if current.repo_id != repo_id and time.monotonic() - current.failed_at > _FAILED_HOLD_S: | ||
| _active = current = None |
There was a problem hiding this comment.
When repository A's worker fails, the watcher leaves this terminal operation in _active; until _FAILED_HOLD_S elapses, a request for a downloadable repository B therefore reaches the busy branch and receives model_download_busy even though no download is running. Retaining A's error is useful for an A retry, but it should not occupy the single-flight slot and prevent unrelated downloads for up to 90 seconds.
Useful? React with 👍 / 👎.
| ]) | ||
| .then(([models, settings]) => { | ||
| if (cancelled) return true; | ||
| if (models !== null) setCatalog(models); |
There was a problem hiding this comment.
Fresh evidence beyond the handled fetch rejection is that _cached_local_catalog() catches a filesystem scan exception and returns HTTP 200 with an empty data list. This assignment treats that response as authoritative, so when no model is resident but downloaded models remain switchable (for example after idle unload), a transient scan error replaces the previously valid catalog with [] and hides every usage snippet for at least the next polling interval. The backend needs to preserve or explicitly mark the failed scan so the UI can retain its last successful catalog.
Useful? React with 👍 / 👎.
| loadOpenAIAutoSwitchSettings() | ||
| .then((s) => [s.enabled, s.idleUnloadActive] as const) |
There was a problem hiding this comment.
This periodic update calls loadOpenAIAutoSwitchSettings(), but that helper returns its module-level cachedSettings forever after the first successful read. Consequently, changing auto-switch or idle-reload state from another tab/session is never observed while this page remains open; for example, after auto-switch is disabled remotely with no model resident, the stale true state continues selecting an unloaded catalog model and the copied snippet fails instead of showing that no model is runnable. Use a forced/uncached settings fetch for this polling path or subscribe the hook to settings updates.
Useful? React with 👍 / 👎.
| await unloadModel({ model_path: checkpoint }); | ||
| // Same as the chat eject flow: the store still holds the freed checkpoint. | ||
| useChatRuntimeStore.getState().clearCheckpoint(); |
There was a problem hiding this comment.
When the chat store currently selects an external::... provider model while a local backend remains resident, the monitor's Unload button correctly unloads that local backend but then unconditionally calls clearCheckpoint(). That clears both the active external selection and its persisted LAST_EXTERNAL_CHECKPOINT_KEY, even though the selected model was unrelated to the model just freed. Only clear the store when its checkpoint identifies the unloaded local model; otherwise keep the user's external selection intact.
Useful? React with 👍 / 👎.
One conflict, in the /unload route: #7454 reads the model identity before teardown clears it, this branch cancels the active chats and lets them unwind first. Both are needed, so the identity read stays ahead of the cancel and the drain, and the two comments about a manual unload being deliberate fold into one.
…#7501) Comment-only follow-up to #7454. That change carried 523 comment lines, many of them three and four line preambles where one line says the same thing. This collapses them and drops the ones restating what the code already says, for a net 77 lines. Scope is limited to comments #7454 itself introduced. The files it touched hold about 3,761 comments in total; the rest predate it and are untouched, verified by checking that every removed line is one that commit added. Nothing that records why a non-obvious decision was made was dropped, only compressed. Still stated: the normcase-before-versus-after Windows separator trap, the innermost-indexed-model rule for nested directories, an HTTPException being a decision rather than a failure to decide, that only an explicit False is anonymous to huggingface_hub while None borrows the server owner's login, the fail-closed tri-state custom-code gate, and the regressions each test was written for. Code is provably unchanged: comment_tools.py check reports 17/17 files comments-only. Backend CI command 10337 passed, 0 failed. tsc -b clean.
…8975) * Stop six backend tests waiting on the clock instead of on a signal These six files spend most of their runtime asleep rather than working, and the waiting buys nothing: every one of them is waiting out a fixed window when it could be waiting for the event it actually cares about. Measured on a fresh worktree off main, the six files together: 118.0s and 118.8s before, 56.5s and 55.3s after. 404 passed and 3 skipped in every run, before and after. Backend tests runs on four Python versions, so that is about 4 runner-minutes a run. No production constant is lowered. The diff is six files, all under studio/backend/tests/. Where a delay was reachable from production code the test now passes an existing parameter at the call site: _terminate_validation_server already takes grace, defaulted to 5.0, and the sibling test in the same commit already passes 0.2, so this one passes 1.0 instead of taking the default four times over. The measured margin there is 25 to 50x, since with the kill mutated out the sentinel appears in 20 to 40ms. test_tool_output_streaming.py is the bulk of it, 61.5s to 32.1s. Seven grandchild processes were fused with sleep 3 and the test then waited 4s or 11s for them. The fuse is now a gate file the test owns, so the grandchild lives indefinitely and the assertion is stronger than "3 is greater than 1". test_cloudflare_tunnel.py is a correctness fix that happens to save 5s. monkeypatch.setattr(ct.threading, "Thread", ...) sets the attribute on the threading module itself, so it replaced threading.Thread process-wide rather than just the tunnel's stdout reader. utils/process_lifetime._Spawner then started a helper thread that never ran, burned its full 5s readiness backstop, and fell back to spawning inline, which silently gives up the PR_SET_PDEATHSIG guarantee that helper thread exists to provide. No assertion is masked today because Popen is faked too, but the patch reaches far outside what the test meant to stub. It is now a shim that overrides Thread and proxies the rest. Detection power is unchanged, and that is measured rather than asserted. Neutering _killpg_captured in core/inference/tools.py fails 7 of the 55 tests in test_tool_output_streaming.py on this branch, and the identical mutation on unpatched main fails the same 7 by name. Same for the others: the installer test fails when the post-SIGKILL group escalation goes, the probe test fails when the probe runs inline, the child-lifetime test fails when PDEATHSIG is dropped, and the cloudflare test fails when the pending-stop record is not cleared. Attribution, since these were deliberate in some cases and accidental in others: #7083 added the grandchild fuses, #8170 the installer grace and child lifetime, #6494 the startup probe, #7875 the cloudflare backstop, #7454 the cold scan. Two of those were sound decisions and are strengthened here rather than weakened. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
127 added comment lines to 79. Cut the restatements and collapsed the rationale blocks to the fact each one exists to carry: why the route untracks before encoding, why the permit follows the worker thread rather than the awaiting task, why no -b/-ub is passed, why the limit is not frozen while /props is silent, and unslothai#7454's rule about answering a decisive reference from another embedding space.
… the loaded GGUF cannot (#10315) * Studio: serve /v1/embeddings from the configured embedding model when the loaded GGUF cannot * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Untrack the studio embeddings fallback from the llama keep-warm slot * Hold the embeddings admission permit until the worker thread exits * Pin the embeddings helpers to one model, report its real identity, free the preview guard * Close the embeddings monitor row when the request is cancelled * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make the admitted-tally assertion relative to the suite's shared counter * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Serve the configured embedding model by name and enforce its token limit on the GGUF embedder * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Validate embeddings input before any switch, cap the limit at the running context and default prompt, hide local model paths * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the proxy for a resident embedding GGUF that answers the requested name and hide Windows-shaped model paths * Accept flat token arrays before switching, drop queued embeddings on disconnect, match aliases without probing and keep redacted local names unique * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Reject falsey encoding formats and apply the studio batch cap only on the fallback * Studio: refuse a decisively named model with nothing loaded, and keep local identities unique * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Accept the advertised local identity as an alias, drop disconnected requests after admission and pin the matched model * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: size the embed server's batch to its context, and never advertise past it * Release the embeddings permit when a cancellation lands in the final disconnect probe * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: trust a cold index less, keep the embed batch as shipped, and reject boolean token ids * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Match tagged identities in full, skip the chat-slot restore for default embedding requests, cache the limit only with both bounds * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the embedder's own message when the model just needs downloading Picking a model in Settings and querying before its download finishes raises EmbeddingModelDownloadRequiredError, whose message says exactly that and exactly what to do. The catch-all mapped it through _friendly_error, which is written for llama-server transport faults, so the caller got 502 "Embedding model failed: An internal error occurred". Reproduced live on a real Studio: with #10320 on top, the generated openclaw.json points memory search at this route, so that string was the whole diagnosis the user saw -- openclaw memory status reported "Embeddings: unavailable, an internal error occurred" for a condition the user could have fixed in one click. Switching the Settings model and switching back is enough to reach it, since the new model is marked download-pending. Return 409 with the model's own words for that error and for UnsafeEmbeddingModelError: the request is fine, the server is not ready to serve it yet. Everything else keeps the 502. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Redact the model path from embedder errors, and fix two invalidation gaps Three review items, each reproduced at head before it was touched. Path leak in the 409 I added: _get resolves a cached model to its absolute snapshot directory and hands that to _guard_model_security, which interpolates it into the error, so returning str(exc) verbatim published the server's Hugging Face cache layout through /v1/embeddings. The scan still runs on the snapshot; the message now names the configured model, and the route puts a local model through the same hashed label the identity and limit errors already use. Identity redaction replaced substrings, not segments. A local model can be a bare relative directory named transformers, which is_local_path accepts when it exists, and sentence-transformers:transformers then became sentence-transformers-<hash>:transformers-<hash>. Reproduced: embedding_identity_model returns None on that, so the identity the response advertises is one _names_studio_embedder cannot match on the next request. Parse off the backend tag and rewrite only the model and repo segments. The token limit outlived the server it was measured on. Changing the custom llama.cpp path makes _current stale and respawns, but _resolve_model_path fast-paths on the unchanged repo and never reaches _adopt_model_path, so the cached value survived onto the new binary. It clamps the GGUF context by the server's n_ctx and n_ubatch, whose defaults differ between builds (llama.cpp server: -b 2048, -ub 512, --ctx-size 0 = from model, and n_ctx in /props is per slot with an automatic slot count), so it is cleared next to the binary it came from. The one existing test touched is a stub signature, not an assertion. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Type the GGUF backend's pending-download refusal like the sentence-transformers one _resolve_model_path raises for exactly the condition EmbeddingModelDownloadRequiredError names, with the same wording, but as a bare RuntimeError. So on the llama-server backend the 409 added for this landed only on the sentence-transformers path, and the GGUF one still returned 502 An internal error occurred: the same user-fixable state, answered two different ways depending on which embedder the hardware picked. Raised typed now. The route's redaction covers it: the message interpolates the configured model, which is what the label substitution replaces, so a local model path does not travel out with it. Regression test fails on the untyped raise and passes here. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments this PR added 127 added comment lines to 79. Cut the restatements and collapsed the rationale blocks to the fact each one exists to carry: why the route untracks before encoding, why the permit follows the worker thread rather than the awaiting task, why no -b/-ub is passed, why the limit is not frozen while /props is silent, and #7454's rule about answering a decisive reference from another embedding space. * Guard a non-string model selector and redact a local GGUF repo under a hub model * Let the security-gate stubs take the display argument * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Refuse a stale embedding identity and bound the advert by the batch we launch with * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the physical batch from n_ubatch only --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
1 participant
What this fixes
Copying the curl snippet from Settings > API and pointing it at a model returned:
{"error":{"message":"No model loaded. Call POST /inference/load first.", ...}}Naming a model should be enough. Three separate problems sat behind that message, and this PR works through them.
1. The error blamed the wrong thing. Auto-switch only ever matches models already on disk (
resolve_local_gguf). A miss fell through silently and the handler emitted a generic "call /inference/load", which cannot fix a model that is not on the machine. It now says what is actually wrong, and distinguishes a missing repo from a missing quant:2. The snippet named a model the server could not serve. It hardcoded a repo id when nothing was loaded, so a copied curl could name a model that had never been downloaded, and it omitted the quant. It now reads the servable ids from
/v1/models(the same list the error above cites, so the two cannot disagree) and pins the quant asrepo:QUANT.3. A named model that is not downloaded can now just be downloaded. New opt-in setting, off by default.
Never answer as a different model
Naming a model this server was not serving returned 200 from whatever was resident. Asking for
gemma-4-E2B-it-GGUF:UD-Q6_K_XLwhileUD-Q4_K_XLwas loaded got a confident answer from the wrongquant, with nothing in the response saying so.
A reference this server can tell was meant for it 404s instead, with the reason:
"Meant for it" needs evidence, and a namespace is not evidence. An earlier revision of this PR
treated any
org/modelas a concrete reference, which 404d every LiteLLM and OpenRouter stylevendor/modelid:anthropic/claude-3.5-sonnet,openai/gpt-4oandmeta-llama/llama-3-70b-instructall broke. The rule is now positive evidence only:
:UD-Q6_K_XL), which no foreign id carries, orAnything else falls through to the resident model exactly as before, so
gpt-4andanthropic/claude-3.5-sonnetboth keep working. Ollama style:latestand:8btags are not quantlabels, so they fall through too. A bare
org/modelthat is here is still satisfied by any loadedquant of it; only an explicit
:QUANThas to match.The check runs whatever the toggles are, since serving the wrong weights is wrong in every
configuration. It is skipped when nothing is loaded, where the existing no-model-loaded error
already says the right thing. When the model is on disk with auto-switch on and the swap did not
take, the request gets
503 model_switch_failedrather than an answer from the resident model.Auto-download
openai_api_auto_download_model, off by default and gated on auto-switch. When on, a/v1request naming a GGUF repo this server does not have starts a background fetch and returns a typed 503:The request is not held open. A quant is routinely tens of GB, longer than any client or the Cloudflare edge on
--securewill wait, and the inference lifecycle gate must not be held meanwhile. The resident model keeps serving throughout, and the retry that lands after the download is served by the new model through the ordinary auto-switch path, so there is no new load coordinator.The download reuses the Hub manager's service layer, which already handles repo-id validation, casing, claim bookkeeping, disk preflight, resume and cancel. The in-loader download is deliberately not used: it silently substitutes a smaller quant under low disk, which is wrong when the caller named an exact one.
Admission
A request only needs an API key, so the gate is narrow:
gpt-4,anthropic/claude-3.5-sonnet, any id the Hub does not knowunsloth/gemma-4-31B-it-GGUF:UD-Q5_K_XLunsloth/gemma-4-31B-it-GGUF:Q2_K_NOPEunsloth/does-not-exist:Q4_K_Munsloth/Qwen3-4B-unsloth-bnb-4bitauto_mapGGUF only, decided from the remote file list rather than the repo name, because GGUF runs under llama.cpp and never imports repo Python. Anything declaring
auto_mapis refused outright, sotrust_remote_codestays a deliberate opt-in in the UI and can never be granted over the API. The tri-state_config_has_auto_mapis used rather than_requires_trust_remote_code_for_model, which swallows lookup errors intoFalse; fail-open is fine for a UI hint and wrong for an admission decision, so an unreadable config is also refused.An id the Hub does not know, or that has no GGUF weights, is treated as a foreign label and falls through rather than erroring, unless it carried an explicit quant. That is what keeps drop-in clients working, and the verdict is cached for ten minutes so a client sending the same foreign id on every request does not pay a Hub round trip each time. Requiring
namespace/namealso avoids the bare-nameunsloth/prefixing inModelConfig.from_identifierturning an unrelated label into a real repo. Onemodel_infocall answers existence, gating, GGUF-ness and the quant list; gated repos get anauth_checkon top, since the Hub serves metadata for a gated repo without granting its files. There is a single download at a time, released by claim identity so a stale watcher cannot free a live one, and a free-disk reserve sized from the full download plan on top of the worker's own preflight. Fetches only ever use a token the caller supplied themselves, never the server's own.With the setting off, every one of these paths is byte-identical to before. That is covered by tests and was re-checked against a live server.
Also in this PR
public_model_idresolves an HF cache snapshot to its repo id, so a cache-loaded model is labelledunsloth/gemma-4-31B-it-GGUFinstead ofc1ac76e99d55.... This removes a duplicate helper and fixes the same leak in the inference status response, which was serving an absolute host path over the public tunnel./unloadmatches on the internal identifier, which the monitor response deliberately omits because it would be a host path, so the click reads it from/api/inference/statusthe way the chat runtime does rather than widening the monitor payload. The manual unload row is also stamped with the quant now, so it readsrepo:QUANTlike the load row it pairs with.sk-unsloth-YOUR_KEYfrom the copyable examples now says it is the placeholder rather than "Invalid or expired API key". Still 401, still no access, and every other bad key keeps the generic message so nothing is revealed about which keys exist.Testing
274backend tests pass across the auto-download, auto-switch, monitor, catalog and model-id suites, plus the static UI contracts.tsc -b,npm run buildandnpm run i18n:checkare clean, and the changed files are clean under the repo's own ruff and formatter hooks.Pre-existing failures on my box, reproduced identically on a clean worktree at the base commit and unrelated to this change:
test_training_worker_flash_attn.py(14),test_studio_api.py(13, live e2e),test_gguf_load_cache_reuse.py(1), and atyperversion mismatch that stopstests/studio/test_cli_repo_variant.pycollecting.Verified end to end against a running Studio over a
--secureCloudflare tunnel, using the case that prompted this: with the setting off, an unknown model still falls through to the resident one exactly as before. With it on,unsloth/gemma-4-31B-it-GGUF:UD-Q5_K_XLreturned 503 in 0.4s, the resident E2B kept answering during the fetch, the monitor row stepped 0% to 100%, and the retry was served by the 31B at the exact quant requested. The missing-quant, unknown-repo and non-GGUF paths were checked live too.Known, not addressed here
The chat model picker still shows the snapshot commit sha for a model loaded straight from the HF cache. That comes from
resolveInferenceCheckpointIdpreferring the rawmodel_identifier, predates this change, and is a chat-UI concern rather than an API one, so I left it out of scope.