From bd5089d62bbcf019261db9d03a443bdec85f561f Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Wed, 29 Jul 2026 05:15:17 +0000 Subject: [PATCH] Judge residency on settled state, not a snapshot Three places read the resident model from state that had moved on. The Hub's variant lookup closed over rowIsActive and activeGgufVariant before awaiting listGgufVariants, while the same click also starts a status refresh, so the two race and the network call usually loses. The residency test now happens after the await against the settled store, and skips an external checkpoint. adoptResidentModelStatus returned early on an empty status, before the external and modelLoading guards, so an unload from another tab or over the API left the store pinned and the settings page went on treating that row as resident. The empty branch now sits after both guards and clears only a local checkpoint. The monitor's Unload targets the resident local model from /status but cleared the store checkpoint unconditionally, and clearCheckpoint drops the persisted external selection too, so unloading wiped an external pick it never touched. It now clears only when the store holds the model that was unloaded. --- .../features/api-monitor/api-monitor-page.tsx | 11 +- studio/frontend/src/features/hub/hub-page.tsx | 29 +++-- .../hub/lib/adopt-inference-status.ts | 23 +++- .../tests/hub-adopt-inference-status.test.ts | 103 ++++++++++++++++++ 4 files changed, 154 insertions(+), 12 deletions(-) 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 de0077952a..48d9dca650 100644 --- a/studio/frontend/src/features/api-monitor/api-monitor-page.tsx +++ b/studio/frontend/src/features/api-monitor/api-monitor-page.tsx @@ -21,6 +21,7 @@ import { getInferenceStatus, unloadModel } from "@/features/chat/api/chat-api"; import { resolveInferenceCheckpointId } from "@/features/chat/lib/apply-inference-status-to-store"; import { useChatRuntimeStore } from "@/features/chat/stores/chat-runtime-store"; import type { ApiMonitorEntry } from "@/features/chat/types/api"; +import { modelIdsMatch } from "@/features/hub/lib/model-identity"; import { useSettingsDialogStore } from "@/features/settings"; import { getApiBase, isTauri } from "@/lib/api-base"; import { copyToClipboard } from "@/lib/copy-to-clipboard"; @@ -515,7 +516,15 @@ export function ApiMonitorPage(): ReactElement { } await unloadModel({ model_path: checkpoint }); // Same as the chat eject flow: the store still holds the freed checkpoint. - useChatRuntimeStore.getState().clearCheckpoint(); + // Only when that IS the model just unloaded, though. Chat can have an + // external provider selected while a local model stays resident, and + // clearCheckpoint calls saveLastExternalCheckpoint(null), so clearing + // unconditionally would delete a selection this button never touched. + const store = useChatRuntimeStore.getState(); + const selected = store.params.checkpoint; + if (selected && modelIdsMatch(selected, checkpoint)) { + store.clearCheckpoint(); + } setUnloadError(null); refresh(); } catch (err: unknown) { diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index b98638104a..4a5c8fc3bc 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -414,6 +414,9 @@ export function ModelsPage() { setCheckpoint: (checkpointId, ggufVariant) => { store.setCheckpoint(checkpointId, ggufVariant); }, + clearCheckpoint: () => { + store.clearCheckpoint(); + }, // Landing here is the one entry point that has applied no status yet, // so the settings page would read this model's live config off a store // still holding defaults. Same call the chat runtime's refresh makes. @@ -1313,15 +1316,13 @@ export function ModelsPage() { const openSeq = ++settingsOpenSeq.current; // loadId is what the loader accepts; repoId is only a display/API alias. const id = row.loadId; - // Whether this row is the loaded model, under any of its names. Gates the - // "prefer the loaded quant" hint below. + // Every name this row answers to. Residency is judged against the store + // AFTER the variant lookup settles, not here, because that lookup and the + // status refresh this same click starts are in flight together. const rowAliases = row.kind === "local" ? [id, row.repoId, row.path] : [id, row.repoId, row.cachePath]; - const rowIsActive = rowAliases.some((alias) => - modelIdsMatch(alias, activeCheckpoint), - ); // Cached repo rows never carry a quant (cache_inventory.py emits one row per // repo with format_variant null). Opening with a null variant keys the config // to `repo::` while the loader reads `repo::Q4_K_M`, so it never applies and @@ -1337,14 +1338,28 @@ export function ModelsPage() { row.kind === "local" ? row.path : (row.cachePath ?? null), }); const downloaded = res.variants.filter((v) => v.downloaded); + // Re-read residency after the await. Opening settings also kicks a + // status refresh, and this request usually hits the network, so the + // refresh routinely lands first; the values closed over above then + // describe whichever model was resident BEFORE an API-driven switch, + // and the loaded-quant branch would silently pick the wrong one. + const settled = useChatRuntimeStore.getState(); + const settledCheckpoint = + settled.params.checkpoint && + !isExternalModelId(settled.params.checkpoint) + ? settled.params.checkpoint + : null; + const settledIsActive = rowAliases.some((alias) => + modelIdsMatch(alias, settledCheckpoint), + ); ggufVariant = // Loaded quant, then the repo default, then whatever is on disk, // mirroring LocalOnDeviceCard's selectedQuant. Only for the loaded // row: Q4_K_M exists in most repos, so an unguarded match would // target the wrong quant of the wrong model. - (rowIsActive + (settledIsActive ? downloaded.find((v) => - ggufVariantsMatch(v.quant, activeGgufVariant), + ggufVariantsMatch(v.quant, settled.activeGgufVariant), )?.quant : undefined) ?? downloaded.find((v) => diff --git a/studio/frontend/src/features/hub/lib/adopt-inference-status.ts b/studio/frontend/src/features/hub/lib/adopt-inference-status.ts index 881a2e9d20..16c28f086f 100644 --- a/studio/frontend/src/features/hub/lib/adopt-inference-status.ts +++ b/studio/frontend/src/features/hub/lib/adopt-inference-status.ts @@ -37,6 +37,12 @@ export interface ResidentStatusFacts { export interface ResidentAdoptionActions { /** Re-pin ``params.checkpoint`` onto the resident model. */ setCheckpoint: (checkpointId: string, ggufVariant: string | null) => void; + /** + * Drop a local checkpoint the server no longer has. + * + * Optional so a caller that only wants the pinning half can leave it out. + */ + clearCheckpoint?: () => void; /** * Apply the rest of the status. Receives the store values from BEFORE * ``setCheckpoint`` ran, which is what applyActiveModelStatusToStore needs to @@ -60,12 +66,10 @@ export function adoptResidentModelStatus( actions: ResidentAdoptionActions, ): boolean { const { checkpointId } = status; - if (!checkpointId) { - return false; - } // An external-provider selection has no local mirror, so stamping the resident // GGUF's capabilities and launch settings onto it would describe a model the - // user is not talking to. + // user is not talking to. It also owns the store, so an empty status must not + // clear it: clearCheckpoint drops the persisted external pick as well. if (state.checkpointIsExternal) { return false; } @@ -74,6 +78,17 @@ export function adoptResidentModelStatus( if (state.modelLoading) { return false; } + if (!checkpointId) { + // The server has nothing loaded, so neither should we. Unloading from another + // tab, from the monitor or over the API leaves this store pinned otherwise, + // and the settings page goes on treating that row as resident and seeding the + // editor from a launch config nothing is running. + if (state.checkpoint) { + actions.clearCheckpoint?.(); + return true; + } + return false; + } const previous = { checkpoint: state.checkpoint, ggufVariant: state.activeGgufVariant, diff --git a/studio/frontend/tests/hub-adopt-inference-status.test.ts b/studio/frontend/tests/hub-adopt-inference-status.test.ts index b21afaa349..e0eee95138 100644 --- a/studio/frontend/tests/hub-adopt-inference-status.test.ts +++ b/studio/frontend/tests/hub-adopt-inference-status.test.ts @@ -149,3 +149,106 @@ test("a load in flight is not fought", () => { assert.equal(adopted, false); assert.deepEqual(calls, []); }); + +test("an empty status drops a local checkpoint the server no longer has", () => { + // Unloading from another tab, from the API monitor or over the API leaves this + // store pinned; the settings page then treats the row as resident and seeds the + // editor from a launch config nothing is running. + const cleared: string[] = []; + const adopted = adoptResidentModelStatus( + { checkpointId: null, ggufVariant: null }, + { + checkpoint: "/models/llama.gguf", + checkpointIsExternal: false, + activeGgufVariant: "Q4_K_M", + modelLoading: false, + }, + { + setCheckpoint: () => { + throw new Error("nothing is resident, so nothing may be pinned"); + }, + clearCheckpoint: () => { + cleared.push("cleared"); + }, + applyStatus: () => { + throw new Error("there is no status to apply"); + }, + }, + ); + assert.equal(adopted, true); + assert.deepEqual(cleared, ["cleared"]); +}); + +test("an empty status leaves an external pick alone", () => { + // clearCheckpoint also drops the persisted external selection, so an empty + // status must not reach it: the local model is not what the user is talking to. + const adopted = adoptResidentModelStatus( + { checkpointId: null, ggufVariant: null }, + { + checkpoint: "gemini/gemini-2.5-pro", + checkpointIsExternal: true, + activeGgufVariant: null, + modelLoading: false, + }, + { + setCheckpoint: () => { + throw new Error("unreachable"); + }, + clearCheckpoint: () => { + throw new Error("an external pick must survive an empty status"); + }, + applyStatus: () => { + throw new Error("unreachable"); + }, + }, + ); + assert.equal(adopted, false); +}); + +test("an empty status does not fight a load this tab started", () => { + const adopted = adoptResidentModelStatus( + { checkpointId: null, ggufVariant: null }, + { + checkpoint: "/models/llama.gguf", + checkpointIsExternal: false, + activeGgufVariant: null, + modelLoading: true, + }, + { + setCheckpoint: () => { + throw new Error("unreachable"); + }, + clearCheckpoint: () => { + throw new Error("the load owns the store until it settles"); + }, + applyStatus: () => { + throw new Error("unreachable"); + }, + }, + ); + assert.equal(adopted, false); +}); + +test("an empty status on an already empty store changes nothing", () => { + const adopted = adoptResidentModelStatus( + { checkpointId: null, ggufVariant: null }, + { + checkpoint: null, + checkpointIsExternal: false, + activeGgufVariant: null, + modelLoading: false, + }, + { + setCheckpoint: () => { + throw new Error("unreachable"); + }, + clearCheckpoint: () => { + throw new Error("there is nothing to clear"); + }, + applyStatus: () => { + throw new Error("unreachable"); + }, + }, + ); + assert.equal(adopted, false); +});