From c2dc2eb1b128747e8f522d271de16586a07a0837 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Fri, 24 Apr 2026 09:05:31 -0700 Subject: [PATCH] Studio: kill in-flight llama-server before spawning a new one (#5171) * Studio: kill in-flight llama-server before spawning a new one Two rapid Apply clicks in the chat settings panel can race two load_model calls. Both pass the Phase 1 _kill_process (because neither has stored its Popen handle yet), both download / read metadata, and both reach Phase 3 and spawn a server. Only the last reference is tracked in self._process. The first server becomes an orphan that holds the model in RAM until the kernel OOM kicks in. Addresses #5161. Two complementary changes in studio/backend/core/inference/llama_cpp.py: 1. At load_model entry, set the existing _cancel_event so any in-flight load aborts at its next checkpoint, then bind a fresh Event for the new load. Subsequent _kill_process and download phases pick up the new event. 2. Inside the Phase 3 lock, immediately before subprocess.Popen, run a defensive _kill_process that removes any orphan handle a racing load might have stored after the first kill ran. Speculative decoding cleanup (also touched while in this code path): The chat UI used to send "ngram-mod" as the wire value when the speculative dropdown was On, and the backend mapped that to the 4-flag combo --spec-type ngram-mod --spec-ngram-size-n 24 --draft-min 48 --draft-max 64. Switch the wire value to "default" and have the backend pass the single llama-server flag --spec-default. That flag expands to the exact same params (see common/arg.cpp:3905-3914 in llama.cpp). Default-on for non-vision models is preserved. Verified: - Unit test: planted Popen orphan handle is terminated before self._process is overwritten. - All ten spec-cmd mappings produce the expected llama-server args ("default" -> --spec-default, "off" / null -> no flag, vision -> always disabled, manual "ngram-mod" / "ngram-simple" still work). * Address review feedback on PR #5171 The previous attempt at cancelling in-flight loads via ``self._cancel_event.set()`` followed by ``self._cancel_event = threading.Event()`` was broken in two ways (flagged independently by Gemini and Codex): 1. Sub-methods like _download_gguf consult ``self._cancel_event`` on every check. After the rebind, the in-flight thread reads the FRESH unset Event, not the one we just set, so cancellation never propagates. 2. Worse, if unload_model() lands between ``set()`` and the rebind, unload's signal hits the OLD event and is then immediately discarded when load_model swaps in a fresh Event. The user's stop-request silently no-ops. Revert to the original ``self._cancel_event.clear()``. The Phase 3 defensive ``_kill_process()`` introduced in this branch still closes the orphan-process race that #5161 reports: even if two concurrent loads both pass Phase 1 with self._process == None, the loser's Phase 3 kill terminates the winner's Popen handle before overwriting it, so we end up with exactly one llama-server process. * Studio: coerce legacy speculative-type values for the simplified dropdown The Speculative Decoding control was simplified to On (default) / Off, but the backend still accepts and reports the older manual modes (ngram-mod, ngram-simple). When a load response or status refresh comes back with one of those values -- whether from an external API caller, a model loaded before this PR landed, or a not-yet-upgraded backend -- the controlled Select renders with an empty trigger because the value is not in the SelectItem list. Add a tiny normaliser at both entry points (status refresh + post-load) so legacy manual modes coerce to "default". The user sees "On" instead of a blank dropdown, and reapplying lets llama.cpp pick its own preferred strategy via --spec-default. Reviewer-flagged finding on PR #5171. --- studio/backend/core/inference/llama_cpp.py | 26 +++++++++++++++---- .../src/features/chat/chat-settings-sheet.tsx | 2 +- .../chat/hooks/use-chat-model-runtime.ts | 17 ++++++++++-- .../chat/stores/chat-runtime-store.ts | 4 +-- 4 files changed, 39 insertions(+), 10 deletions(-) diff --git a/studio/backend/core/inference/llama_cpp.py b/studio/backend/core/inference/llama_cpp.py index c320f03b2c..800d7fdd8c 100644 --- a/studio/backend/core/inference/llama_cpp.py +++ b/studio/backend/core/inference/llama_cpp.py @@ -1574,11 +1574,21 @@ class LlamaCppBackend: # ref: https://github.com/ggml-org/llama.cpp/blob/master/docs/speculative.md # ref: https://github.com/ggml-org/llama.cpp/pull/19164 # ref: https://github.com/ggml-org/llama.cpp/pull/18471 + # ``"default"`` -> let llama-server pick a sensible spec + # config via ``--spec-default``. Explicit type names are + # passed through with the manual draft tuning we've shipped + # historically so power users keep their overrides. _valid_spec_types = {"ngram-simple", "ngram-mod"} - if speculative_type and speculative_type in _valid_spec_types: - if not is_vision: # spec decoding disabled for vision models - cmd.extend(["--spec-type", speculative_type]) - if speculative_type == "ngram-mod": + normalized_spec = ( + speculative_type.lower().strip() if speculative_type else None + ) + if normalized_spec and normalized_spec != "off" and not is_vision: + if normalized_spec == "default": + cmd.append("--spec-default") + self._speculative_type = "default" + elif normalized_spec in _valid_spec_types: + cmd.extend(["--spec-type", normalized_spec]) + if normalized_spec == "ngram-mod": cmd.extend( [ "--spec-ngram-size-n", @@ -1589,7 +1599,7 @@ class LlamaCppBackend: "64", ] ) - self._speculative_type = speculative_type + self._speculative_type = normalized_spec else: self._speculative_type = None else: @@ -1750,6 +1760,12 @@ class LlamaCppBackend: if gpu_indices is not None: env["CUDA_VISIBLE_DEVICES"] = ",".join(str(i) for i in gpu_indices) + # Defensive kill: if a concurrent load slipped past Phase 1 + # (because its `self._process` was None at the time) and + # already stored a Popen handle here, drop that orphan + # before we overwrite the reference. See issue #5161. + self._kill_process() + self._stdout_lines = [] self._process = subprocess.Popen( cmd, diff --git a/studio/frontend/src/features/chat/chat-settings-sheet.tsx b/studio/frontend/src/features/chat/chat-settings-sheet.tsx index e4574dab74..fc5f097969 100644 --- a/studio/frontend/src/features/chat/chat-settings-sheet.tsx +++ b/studio/frontend/src/features/chat/chat-settings-sheet.tsx @@ -1037,7 +1037,7 @@ export function ChatSettingsPanel({ - On + On Off diff --git a/studio/frontend/src/features/chat/hooks/use-chat-model-runtime.ts b/studio/frontend/src/features/chat/hooks/use-chat-model-runtime.ts index 1281592168..e90cab73fa 100644 --- a/studio/frontend/src/features/chat/hooks/use-chat-model-runtime.ts +++ b/studio/frontend/src/features/chat/hooks/use-chat-model-runtime.ts @@ -24,6 +24,19 @@ import type { InferenceParams, } from "../types/runtime"; +// The simplified Speculative Decoding control surfaces "default" (which +// maps to llama.cpp's --spec-default) and "off". A backend status / load +// response can still report the older manual modes (ngram-mod, +// ngram-simple) when a model is loaded via the API or carried over from an +// older Studio version. The Select would render an empty trigger for those +// values, so coerce them to "default" -- llama.cpp's own --spec-default +// picks an equivalent strategy and keeps the dropdown coherent. +function normalizeSpeculativeType(v: string | null | undefined): string | null { + if (v == null) return null; + if (v === "default" || v === "off") return v; + return "default"; +} + type SelectedModelInput = { id: string; isLora?: boolean; @@ -279,7 +292,7 @@ export function useChatModelRuntime() { const ggufNativeContextLength = statusRes.is_gguf ? (statusRes.native_context_length ?? null) : null; - const currentSpecType = statusRes.speculative_type ?? null; + const currentSpecType = normalizeSpeculativeType(statusRes.speculative_type); useChatRuntimeStore.setState({ supportsReasoning, reasoningAlwaysOn, @@ -492,7 +505,7 @@ export function useChatModelRuntime() { } } const loadedKv = loadResponse.cache_type_kv ?? null; - const loadedSpec = loadResponse.speculative_type ?? null; + const loadedSpec = normalizeSpeculativeType(loadResponse.speculative_type); const nativeCtx = loadResponse.is_gguf ? (loadResponse.context_length ?? 131072) : null; diff --git a/studio/frontend/src/features/chat/stores/chat-runtime-store.ts b/studio/frontend/src/features/chat/stores/chat-runtime-store.ts index 2a1249aea0..ce69d8f6dc 100644 --- a/studio/frontend/src/features/chat/stores/chat-runtime-store.ts +++ b/studio/frontend/src/features/chat/stores/chat-runtime-store.ts @@ -270,7 +270,7 @@ export const useChatRuntimeStore = create((set) => ({ toolCallTimeout: loadInt(TOOL_CALL_TIMEOUT_KEY, 5), kvCacheDtype: null, loadedKvCacheDtype: null, - speculativeType: "ngram-mod", + speculativeType: "default", loadedSpeculativeType: null, customContextLength: null, defaultChatTemplate: null, @@ -365,7 +365,7 @@ export const useChatRuntimeStore = create((set) => ({ toolStatus: null, kvCacheDtype: null, loadedKvCacheDtype: null, - speculativeType: "ngram-mod", + speculativeType: "default", loadedSpeculativeType: null, customContextLength: null, defaultChatTemplate: null,