From c2bcb1e6e5b49c0fcf3c04ae4ffe8f670b939218 Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Tue, 28 Jul 2026 16:18:55 +0000 Subject: [PATCH] Guard the backfill, the detail settings entry point and the detail retry Three separate reports, all confirmed against head. listPerModelConfigs reported future-schema records. loadPerModelConfig refuses to apply one and eviction refuses to drop one, because this client cannot interpret that schema, so handing it to the backfill would persist a partial reading of it server-side and let an API-triggered load apply settings the same client will not apply locally. It is skipped there now, matching the other two paths. The detail view's on-device card passes a null variant while its own lookup is pending or after it failed, and this entry point opened the editor anyway. That saves a bare-model config, which the picker never finds because it matches variants exactly, while the API's bare-key fallback would apply it. openModelSettings already refuses with a toast for exactly that reason; this path now refuses the same way. A failed detail fetch was never retried. The revision is recorded when the fetch starts, and on failure the entry stays missing, so selectedIsMissing does not change and a terminal row's updated_at does not advance: nothing was left to re-run the effect, and the full prompt and reply stayed unavailable until another row was selected. The in-flight flag settling is the trigger now, and the attempt count bounds it, because the usual failure is an entry that has aged out of the ring buffer and will never arrive however often it is asked for. --- .../features/api-monitor/api-monitor-page.tsx | 26 ++++++++++++- studio/frontend/src/features/hub/hub-page.tsx | 16 ++++++++ .../model-config/per-model-config.ts | 7 ++++ tests/studio/test_model_picker_contracts.py | 38 +++++++++++++++++++ 4 files changed, 86 insertions(+), 1 deletion(-) diff --git a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx index 4d998c6f88..1558c9c6eb 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx @@ -50,6 +50,9 @@ import { const API_INFERENCE_PREFIX_RE = /^\/api\/inference/; const V1_PREFIX_RE = /^\/v1\//; +// Tries per revision for a detail payload. Bounded because the usual failure +// is an entry that has aged out of the ring buffer and never comes back. +const DETAIL_FETCH_ATTEMPTS = 3; const STATUS_FILTERS: { value: MonitorStatusFilter; label: string }[] = [ { value: "all", label: "All requests" }, @@ -550,9 +553,16 @@ export function ApiMonitorPage(): ReactElement { const selectedUpdatedAt = selected?.updated_at ?? null; const selectedIsMissing = selectedId_ != null && details[selectedId_] == null; const lastFetchedRef = useRef(null); + const attemptsRef = useRef<{ revision: string; count: number }>({ + revision: "", + count: 0, + }); const [retryTick, setRetryTick] = useState(0); + // Flips as a fetch settles, successfully or not, which is what lets a failed + // one be noticed at all. + const detailInFlight = selectedId_ != null && loadingDetails.has(selectedId_); useEffect(() => { - if (selectedId_ == null) { + if (selectedId_ == null || detailInFlight) { return; } // `updated_at` advances per poll while streaming and settles when terminal. @@ -561,6 +571,19 @@ export function ApiMonitorPage(): ReactElement { if (!selectedIsMissing && lastFetchedRef.current === revision) { return; } + // A terminal row's revision never advances, so a fetch that failed had + // nothing left to re-run this effect and the payload stayed unavailable + // until the user picked another row. `loadingDetails` changing as the failed + // fetch settles is the trigger; the count bounds it, because the usual + // failure is an entry that aged out of the ring buffer and will never + // arrive however often it is asked for. + if (attemptsRef.current.revision !== revision) { + attemptsRef.current = { revision, count: 0 }; + } + if (attemptsRef.current.count >= DETAIL_FETCH_ATTEMPTS) { + return; + } + attemptsRef.current.count += 1; // Only remember the revision when a fetch really started; the in-flight // guard can refuse, and recording it anyway skips that revision for good. if (requestDetail(selectedId_)) { @@ -579,6 +602,7 @@ export function ApiMonitorPage(): ReactElement { selectedIsMissing, requestDetail, retryTick, + detailInFlight, ]); // The desktop webview's origin is tauri://, not the API server, and the diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 2f0dcb677d..7965c6b2da 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -1365,6 +1365,22 @@ export function ModelsPage() { const openSelectedModelSettings = useCallback( (ggufVariant: string | null) => { if (!selectedModel) return; + // The card passes null while its own variant lookup is pending or after it + // failed, so this needs the same guard openModelSettings applies: a model + // that needs a quant cannot be configured without one, because the picker + // matches variants exactly and would never find the saved config while the + // API falls back to the bare key and would apply it. + if ( + !ggufVariant && + selectedModel.isGguf && + selectedModel.requiresVariant + ) { + toast.error("Couldn't determine which quant to configure.", { + description: + "Settings for this model are per quant. Check the connection or the model's cache, then try again.", + }); + return; + } // Share the sequence with openModelSettings: a row's variant lookup may // still be pending, and it must not land on top of this one. settingsOpenSeq.current += 1; diff --git a/studio/frontend/src/features/model-picker/model-config/per-model-config.ts b/studio/frontend/src/features/model-picker/model-config/per-model-config.ts index acc8efdefa..049b480f80 100644 --- a/studio/frontend/src/features/model-picker/model-config/per-model-config.ts +++ b/studio/frontend/src/features/model-picker/model-config/per-model-config.ts @@ -690,6 +690,13 @@ export function listPerModelConfigs(): { if (!modelId) { continue; } + // Never report a future-schema record. loadPerModelConfig refuses to apply + // one and eviction refuses to drop one, so handing it to the backfill would + // persist this client's partial reading of it server-side and let an + // API-triggered load apply settings the same client will not apply locally. + if (storedConfigVersion(raw) > STORAGE_SCHEMA_VERSION) { + continue; + } const variant = ggufVariantFromStorageKey(key); out.push({ modelId, diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index 83c155a1ce..06d8a00082 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -786,3 +786,41 @@ def test_override_writes_are_ordered_per_model(): assert "previous .catch(() => {}) .then(() => sendModelOverride(" in src # Only the last writer clears the slot, or a queue still building loses order. assert "if (writesByKey.get(key) === write) { writesByKey.delete(key); }" in src + + +def test_backfill_skips_future_schema_local_records(): + """loadPerModelConfig refuses to apply a record written by a newer Studio and + eviction refuses to drop one, because this client cannot interpret that + schema. The enumeration the backfill uses had no such guard, so it would + persist this client's partial reading server-side and an API-triggered load + would then apply settings the same client will not apply locally. + """ + src = " ".join( + _read("features/model-picker/model-config/per-model-config.ts").split() + ) + listing = src[src.index("export function listPerModelConfigs()") :] + assert "storedConfigVersion(raw) > STORAGE_SCHEMA_VERSION" in listing[:900] + + +def test_detail_settings_need_a_resolved_quant(): + """The on-device card passes a null variant while its own lookup is pending + or after it failed. Opening the editor then saves a bare-model config, which + the picker never finds because it matches variants exactly, while the API's + bare-key fallback would apply it. openModelSettings already refuses; this + entry point has to refuse the same way.""" + src = " ".join(_read("features/hub/hub-page.tsx").split()) + guard = "if ( !ggufVariant && selectedModel.isGguf && selectedModel.requiresVariant )" + assert guard in src + assert src.count("Couldn't determine which quant to configure.") == 2 + + +def test_a_failed_detail_fetch_is_retried(): + """A terminal row's updated_at never advances and selectedIsMissing stays + true, so a fetch that failed had nothing left to re-run the effect and the + payload stayed unavailable until another row was selected. The retry is + bounded because the usual failure is an entry aged out of the ring buffer, + which never arrives however often it is asked for.""" + src = " ".join(_read("features/api-monitor/api-monitor-page.tsx").split()) + assert "const DETAIL_FETCH_ATTEMPTS = 3;" in src + assert "const detailInFlight = selectedId_ != null && loadingDetails.has(selectedId_);" in src + assert "if (attemptsRef.current.count >= DETAIL_FETCH_ATTEMPTS) { return; }" in src