diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index 8c3dfa3c..d42dc273 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -39,7 +39,7 @@ Requests (caller → service): | `engine:stop` | `{ engine }` | `EngineStatus` | | `engine:restart` | `{ engine }` | `EngineStatus` | | `engine:set-port` | `{ engine, port }` | `EngineStatus` (after rebind) | -| `engine:action` | `{ engine, action, params }` | the engine's raw response. `action:"pull_model"` is streamed: it emits live `engine:pull-progress` notifications and returns the pull's terminal result (see below). An action whose manifest declares `restart_after` (LM Studio's `delete_model`) restarts a running engine before replying, so the response also means the engine is back and healthy | +| `engine:action` | `{ engine, action, params }` | the engine's raw response. `action:"pull_model"` is streamed: it emits live `engine:pull-progress` notifications and returns the pull's terminal result (see below). An action whose manifest declares `restart_after` (LM Studio's `delete_model`) restarts a running engine before replying, so the response also means the engine is back and healthy. An HTTP action may declare `timeout_s` to raise its response-header budget above the 30s default when a cold model load or long prefill can delay the first byte (Ollama's `run_model` and LM Studio's `chat` declare 600); the total call stays bounded by the executor's action timeout | | `engine:logs` | `{ engine }` | `{ lines: [LogLine] }` | | `engine:errors` | — | `{ errors: [ServiceError] }` | | `engine:models` | — | `{ models: [string], modelsByEngine: { : [string] }, loadedByEngine: { : [string] } }` — the flat de-duplicated union of every running engine's models, the per-engine breakdown keyed by engine name, and the per-engine set of models currently **loaded in memory** (all normalized from each engine's `list_models` / `loaded_models` action `result` spec). `modelsByEngine` carries a key for every running engine whose inventory was successfully queried, including an empty list = "running, no models available"; a missing key means not running / not queryable / invalid response. `loadedByEngine` uses the same known-empty distinction for residency and also omits engines with no loaded endpoint. The `/v1/models` HTTP surface returns the same shape. | diff --git a/services/nvpair-engine-manager/actions.go b/services/nvpair-engine-manager/actions.go index 372ee104..d108106d 100644 --- a/services/nvpair-engine-manager/actions.go +++ b/services/nvpair-engine-manager/actions.go @@ -125,11 +125,10 @@ func (e *Executor) dispatchAction(ctx context.Context, st *engineState, engine, req.Header.Set("Content-Type", "application/json") } req.Header.Set(engineIdentityProbeHeader, "1") - client := e.client - if engine == "ollama" && action == "run_model" && e.ollamaLoadClient != nil { - client = e.ollamaLoadClient - } - resp, err := client.Do(req) + // The response-header budget comes from the action's manifest-declared + // timeout_s (default 30s), not from the engine's name — any engine can + // declare a slow action. + resp, err := e.actionClient(act.TimeoutS).Do(req) if err != nil { return nil, fmt.Errorf("action %q: %w", action, err) } diff --git a/services/nvpair-engine-manager/executor.go b/services/nvpair-engine-manager/executor.go index 85ca5536..8123fa94 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -20,8 +20,11 @@ import ( var winEnvRe = regexp.MustCompile(`%([^%]+)%`) const ( - engineResponseHeaderTimeout = 30 * time.Second - ollamaLoadResponseHeaderTimeout = 10 * time.Minute + // engineResponseHeaderTimeout bounds how long a loopback action, download, + // or probe waits for response headers before treating the peer as hung. + // It is also the default for an HTTP action that does not declare its own + // timeout_s in the manifest. + engineResponseHeaderTimeout = 30 * time.Second ) // EngineStatus is the snapshot returned by engine:status and @@ -69,11 +72,15 @@ type engineState struct { // layer runs the long ones (install, start) in goroutines so the read // loop stays responsive. type Executor struct { - reg *Registry - reporter *Reporter - emit func(method string, params any) - client *http.Client - ollamaLoadClient *http.Client + reg *Registry + reporter *Reporter + emit func(method string, params any) + client *http.Client + // actionClients caches one HTTP client per distinct non-default + // response-header timeout declared by an action's timeout_s, so a manifest + // value is honored without allocating a client per call. + actionClientsMu sync.Mutex + actionClients map[time.Duration]*http.Client // progress fans install/pull progress to transient subscribers (the ec // streaming handlers) in addition to the local engine:install-progress // notification path. See progress.go. @@ -115,7 +122,7 @@ func NewExecutor(reg *Registry, reporter *Reporter, emit func(string, any), base reporter: reporter, emit: emit, client: newEngineHTTPClient(engineResponseHeaderTimeout), - ollamaLoadClient: newEngineHTTPClient(ollamaLoadResponseHeaderTimeout), + actionClients: make(map[time.Duration]*http.Client), progress: newProgressHub(), baseDir: baseDir, desired: newDesiredStateStore(baseDir), @@ -143,15 +150,13 @@ func (e *Executor) reservedPortError(port int) error { } // newEngineHTTPClient builds a client used for downloads, loopback actions, -// and probes. The ordinary shared client keeps a 30s response-header bound; -// Ollama's cold-load action gets a separate 10m client. Both set NO total -// http.Client.Timeout: a multi-GB engine download can legitimately run -// for many minutes and every call site already bounds total time with a -// context deadline (download 30m, action actionTimeout, probe 3s). What -// it adds over the zero-value client is (a) a bounded response-header -// wait so a peer that accepts the connection but never replies can't park -// a goroutine even inside a long context, and (b) a redirect policy that -// refuses an https->plaintext downgrade so a checksum-pinned download URL +// and probes. It sets NO total http.Client.Timeout: a multi-GB engine download +// can legitimately run for many minutes and every call site already bounds +// total time with a context deadline (download 30m, action actionTimeout, +// probe 3s). What it adds over the zero-value client is (a) a bounded +// response-header wait so a peer that accepts the connection but never replies +// can't park a goroutine even inside a long context, and (b) a redirect policy +// that refuses an https->plaintext downgrade so a checksum-pinned download URL // can't be silently bounced onto http before its bytes are verified. func newEngineHTTPClient(responseHeaderTimeout time.Duration) *http.Client { tr := http.DefaultTransport.(*http.Transport).Clone() @@ -164,6 +169,28 @@ func newEngineHTTPClient(responseHeaderTimeout time.Duration) *http.Client { } } +// actionClient returns the HTTP client for an action's declared response- +// header timeout: the shared 30s client when the action declares none (or the +// default), otherwise a cached client built for the declared value. The +// timeout is a property of the action in the manifest, not of the engine's +// name, so any engine can declare a slow action (e.g. Ollama's cold run_model +// declares 600). The total call stays bounded by the executor's action +// timeout regardless. +func (e *Executor) actionClient(timeoutS int) *http.Client { + d := time.Duration(timeoutS) * time.Second + if d <= 0 || d == engineResponseHeaderTimeout { + return e.client + } + e.actionClientsMu.Lock() + defer e.actionClientsMu.Unlock() + if c, ok := e.actionClients[d]; ok { + return c + } + c := newEngineHTTPClient(d) + e.actionClients[d] = c + return c +} + // noDowngradeRedirect caps the redirect chain and forbids a redirect that // drops from https to a non-https scheme. Loopback action/probe calls use // http and never redirect, so they are unaffected; only an https download diff --git a/services/nvpair-engine-manager/executor_test.go b/services/nvpair-engine-manager/executor_test.go index 40ebe249..ff42ffda 100644 --- a/services/nvpair-engine-manager/executor_test.go +++ b/services/nvpair-engine-manager/executor_test.go @@ -68,17 +68,19 @@ func responseHeaderTimeout(t *testing.T, client *http.Client) time.Duration { return transport.ResponseHeaderTimeout } -func TestEngineHTTPClientsBoundResponseHeaders(t *testing.T) { +func TestEngineHTTPClientBoundsResponseHeaders(t *testing.T) { ex := newTestExecutor(t, testEngineManifest(fakeEngineBin)) if got := responseHeaderTimeout(t, ex.client); got != engineResponseHeaderTimeout { - t.Fatalf("ordinary response-header timeout = %s, want %s", got, engineResponseHeaderTimeout) - } - if got := responseHeaderTimeout(t, ex.ollamaLoadClient); got != ollamaLoadResponseHeaderTimeout { - t.Fatalf("Ollama load response-header timeout = %s, want %s", got, ollamaLoadResponseHeaderTimeout) + t.Fatalf("shared response-header timeout = %s, want %s", got, engineResponseHeaderTimeout) } } -func TestOnlyOllamaRunModelUsesSlowResponseHeaderBudget(t *testing.T) { +// TestActionTimeoutSFollowsManifestNotEngineName pins the issue #25 fix: the +// response-header budget is a property of the action's manifest entry, not of +// the engine's name. A declared timeout_s must be honored for any engine, and +// an undeclared action — even on an engine named ollama — gets the ordinary +// default. +func TestActionTimeoutSFollowsManifestNotEngineName(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { time.Sleep(100 * time.Millisecond) w.Header().Set("Content-Type", "application/json") @@ -88,49 +90,107 @@ func TestOnlyOllamaRunModelUsesSlowResponseHeaderBudget(t *testing.T) { u, _ := url.Parse(srv.URL) port, _ := strconv.Atoi(u.Port()) - m := testEngineManifest(fakeEngineBin) - m.Engine = "ollama" - platform := m.Platforms[runtime.GOOS+"/"+runtime.GOARCH] - platform.Runtime.Port = port - m.Platforms[runtime.GOOS+"/"+runtime.GOARCH] = platform - m.Actions = map[string]Action{ - "run_model": {HTTP: &ActionHTTP{Method: "POST", Path: "/api/generate"}}, - "delete_model": {HTTP: &ActionHTTP{Method: "DELETE", Path: "/api/delete"}}, + key := runtime.GOOS + "/" + runtime.GOARCH + mkManifest := func(engine string, slow bool) *Manifest { + m := testEngineManifest(fakeEngineBin) + m.Engine = engine + p := m.Platforms[key] + p.Runtime.Port = port + m.Platforms[key] = p + runModel := Action{HTTP: &ActionHTTP{Method: "POST", Path: "/api/generate"}} + if slow { + runModel.TimeoutS = 5 // 5s > the server's 100ms delay + } + m.Actions = map[string]Action{ + "run_model": runModel, + "delete_model": {HTTP: &ActionHTTP{Method: "DELETE", Path: "/api/delete"}}, + } + return m } - ex := newTestExecutor(t, m) - ex.client = newEngineHTTPClient(20 * time.Millisecond) - ex.ollamaLoadClient = newEngineHTTPClient(500 * time.Millisecond) - st, err := ex.state("ollama") - if err != nil { - t.Fatal(err) + + t.Run("declared timeout_s is honored for any engine name", func(t *testing.T) { + ex := newTestExecutor(t, mkManifest("slow", true)) + ex.client = newEngineHTTPClient(20 * time.Millisecond) // < the server's 100ms delay + st, err := ex.state("slow") + if err != nil { + t.Fatal(err) + } + st.running = true + + if _, err := ex.Action(context.Background(), "slow", "run_model", json.RawMessage(`{"model":"tiny"}`)); err != nil { + t.Fatalf("action with declared timeout_s was cut off by the ordinary response-header budget: %v", err) + } + if _, err := ex.Action(context.Background(), "slow", "delete_model", json.RawMessage(`{"name":"tiny"}`)); err == nil || !strings.Contains(err.Error(), "timeout awaiting response headers") { + t.Fatalf("ordinary action error = %v, want bounded response-header timeout", err) + } + }) + + t.Run("no declared timeout_s means the ordinary budget even for ollama", func(t *testing.T) { + ex := newTestExecutor(t, mkManifest("ollama", false)) + ex.client = newEngineHTTPClient(20 * time.Millisecond) + st, err := ex.state("ollama") + if err != nil { + t.Fatal(err) + } + st.running = true + + if _, err := ex.Action(context.Background(), "ollama", "run_model", json.RawMessage(`{"model":"tiny"}`)); err == nil || !strings.Contains(err.Error(), "timeout awaiting response headers") { + t.Fatalf("undeclared ollama run_model error = %v, want ordinary response-header timeout (no name-based special case)", err) + } + }) +} + +// TestBundledLMStudioChatDeclaresResponseHeaderBudget pins issue #25 end to +// end: the engine in the bug report (LM Studio's chat action) must opt into a +// response-header budget beyond the 30s default, or the manifest field is dead +// code for the exact action that timed out. A cold model load or long prefill +// can delay the first byte by minutes; the executor's action timeout still +// bounds the total call. +func TestBundledLMStudioChatDeclaresResponseHeaderBudget(t *testing.T) { + reg := NewRegistry() + if err := reg.LoadFS(bundledManifests, "manifests"); err != nil { + t.Fatalf("LoadFS bundled: %v", err) + } + m, ok := reg.Get("lmstudio") + if !ok { + t.Fatal("bundled lmstudio manifest missing") } - st.running = true + chat, ok := m.Actions["chat"] + if !ok { + t.Fatal("bundled lmstudio chat action missing") + } + if chat.HTTP == nil { + t.Fatal("lmstudio chat must remain an http action for timeout_s to apply") + } + defaultS := int(engineResponseHeaderTimeout / time.Second) + if chat.TimeoutS <= defaultS { + t.Fatalf("lmstudio chat timeout_s = %d, want > %d (issue #25: the reported action would still get the ordinary response-header budget)", chat.TimeoutS, defaultS) + } +} - if _, err := ex.Action(context.Background(), "ollama", "run_model", json.RawMessage(`{"model":"tiny"}`)); err != nil { - t.Fatalf("Ollama load was cut off by the ordinary response-header budget: %v", err) +// TestActionClientCachesPerDeclaredTimeout checks the client-selection helper: +// unset and default values reuse the shared client; distinct declared values +// each get one cached client with the right response-header bound. +func TestActionClientCachesPerDeclaredTimeout(t *testing.T) { + ex := newTestExecutor(t, testEngineManifest(fakeEngineBin)) + + if got := ex.actionClient(0); got != ex.client { + t.Fatal("unset timeout_s must use the shared client") } - if _, err := ex.Action(context.Background(), "ollama", "delete_model", json.RawMessage(`{"name":"tiny"}`)); err == nil || !strings.Contains(err.Error(), "timeout awaiting response headers") { - t.Fatalf("ordinary action error = %v, want bounded response-header timeout", err) + if got := ex.actionClient(30); got != ex.client { + t.Fatal("timeout_s equal to the default must reuse the shared client") } - other := testEngineManifest(fakeEngineBin) - other.Engine = "other" - otherPlatform := other.Platforms[runtime.GOOS+"/"+runtime.GOARCH] - otherPlatform.Runtime.Port = port - other.Platforms[runtime.GOOS+"/"+runtime.GOARCH] = otherPlatform - other.Actions = map[string]Action{ - "run_model": {HTTP: &ActionHTTP{Method: "POST", Path: "/api/generate"}}, + c1 := ex.actionClient(600) + c2 := ex.actionClient(600) + if c1 == nil || c1 != c2 { + t.Fatalf("expected one cached client per declared timeout, got %v and %v", c1, c2) } - otherEx := newTestExecutor(t, other) - otherEx.client = newEngineHTTPClient(20 * time.Millisecond) - otherEx.ollamaLoadClient = newEngineHTTPClient(500 * time.Millisecond) - otherState, err := otherEx.state("other") - if err != nil { - t.Fatal(err) + if got := responseHeaderTimeout(t, c1); got != 10*time.Minute { + t.Fatalf("cached response-header timeout = %s, want %s", got, 10*time.Minute) } - otherState.running = true - if _, err := otherEx.Action(context.Background(), "other", "run_model", json.RawMessage(`{"model":"tiny"}`)); err == nil || !strings.Contains(err.Error(), "timeout awaiting response headers") { - t.Fatalf("non-Ollama run_model error = %v, want ordinary response-header timeout", err) + if got := ex.actionClient(120); got == c1 || got == ex.client { + t.Fatal("a distinct declared timeout must not share another client") } } diff --git a/services/nvpair-engine-manager/manifests/lmstudio.json b/services/nvpair-engine-manager/manifests/lmstudio.json index 887e5068..6efdbc9e 100644 --- a/services/nvpair-engine-manager/manifests/lmstudio.json +++ b/services/nvpair-engine-manager/manifests/lmstudio.json @@ -77,7 +77,8 @@ "cmd": ["{cli}", "ls"] }, "chat": { - "description": "OpenAI-compatible chat completion (params: {\"model\": \"\", \"messages\": [...]}).", + "description": "OpenAI-compatible chat completion (params: {\"model\": \"\", \"messages\": [...]}). A model that is not yet resident is loaded on demand, and long prefills can delay the first byte by minutes, so this action declares a long response-header budget.", + "timeout_s": 600, "http": { "method": "POST", "path": "/v1/chat/completions", "body_schema": { "model": "string", "messages": "array" } } }, "unload_model": { diff --git a/services/nvpair-engine-manager/manifests/ollama.json b/services/nvpair-engine-manager/manifests/ollama.json index 6c925131..1f965fa5 100644 --- a/services/nvpair-engine-manager/manifests/ollama.json +++ b/services/nvpair-engine-manager/manifests/ollama.json @@ -98,7 +98,8 @@ "http": { "method": "POST", "path": "/api/pull", "body_schema": { "name": "string" } } }, "run_model": { - "description": "Run a one-shot generation (params: {\"model\": \"\", \"prompt\": \"\", \"stream\": false}).", + "description": "Run a one-shot generation (params: {\"model\": \"\", \"prompt\": \"\", \"stream\": false}). A cold model load can take minutes before the first byte, so this action declares a long response-header budget.", + "timeout_s": 600, "http": { "method": "POST", "path": "/api/generate", "body_schema": { "model": "string", "prompt": "string", "stream": "bool" } } }, "unload_model": { diff --git a/services/nvpair-engine-manager/registry.go b/services/nvpair-engine-manager/registry.go index b44ec6e0..e5dfc937 100644 --- a/services/nvpair-engine-manager/registry.go +++ b/services/nvpair-engine-manager/registry.go @@ -183,6 +183,13 @@ type Action struct { // only a restart makes the deletion visible to clients. A stopped engine is // left stopped; a restart failure fails the action. RestartAfter bool `json:"restart_after,omitempty"` + // TimeoutS, when > 0 on an HTTP action, bounds how long the runner waits + // for that action's response headers (seconds); 0 means the 30s default. + // It is a property of the action in the manifest, not of the engine's + // name, so any engine can declare a slow action (Ollama's cold run_model + // declares 600). The whole call stays bounded by the executor's action + // timeout regardless. + TimeoutS int `json:"timeout_s,omitempty"` } // ActionResult is the list-extraction spec on an Action (see Action.Result). @@ -639,6 +646,12 @@ func (a *Action) validate(name string) error { if kinds != 1 { return fmt.Errorf("action %q: exactly one of http, cmd, or remove_path is required", name) } + if a.TimeoutS < 0 { + return fmt.Errorf("action %q: timeout_s must be >= 0", name) + } + if a.TimeoutS > 0 && !hasHTTP { + return fmt.Errorf("action %q: timeout_s requires an http action (it bounds the response-header wait)", name) + } if hasRemovePath { if strings.TrimSpace(a.RemovePath.Path) == "" || strings.TrimSpace(a.RemovePath.Root) == "" { return fmt.Errorf("action %q: remove_path.path and remove_path.root are required", name) diff --git a/services/nvpair-engine-manager/registry_test.go b/services/nvpair-engine-manager/registry_test.go index 2ae2c5c6..c1dc7a29 100644 --- a/services/nvpair-engine-manager/registry_test.go +++ b/services/nvpair-engine-manager/registry_test.go @@ -79,6 +79,18 @@ func TestValidateAcceptsUnpinnedFetch(t *testing.T) { } } +func TestValidateAcceptsActionTimeoutS(t *testing.T) { + m := validManifest() + m.Actions["run_model"] = Action{ + Description: "slow one-shot generation", + HTTP: &ActionHTTP{Method: "POST", Path: "/api/generate"}, + TimeoutS: 600, + } + if err := m.Validate(); err != nil { + t.Fatalf("http action with timeout_s rejected: %v", err) + } +} + func TestValidateRejectsBadEngineName(t *testing.T) { for _, bad := range []string{"../evil", "a/b", `a\b`, "..", ".", "a b", ""} { m := validManifest() @@ -139,6 +151,12 @@ func TestValidateRejects(t *testing.T) { {"action missing method", func(m *Manifest) { m.Actions = map[string]Action{"x": {HTTP: &ActionHTTP{Path: "/p"}}} }, "http.method and http.path"}, + {"timeout_s on a cmd action", func(m *Manifest) { + m.Actions = map[string]Action{"x": {Cmd: []string{"lms", "get", "{model}"}, TimeoutS: 60}} + }, "timeout_s requires an http action"}, + {"negative timeout_s", func(m *Manifest) { + m.Actions = map[string]Action{"x": {HTTP: &ActionHTTP{Method: "GET", Path: "/p"}, TimeoutS: -1}} + }, "timeout_s must be >= 0"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -399,6 +417,28 @@ func TestBundledOllamaReadinessBudget(t *testing.T) { } } +// TestBundledOllamaRunModelTimeout pins the response-header budget declared by +// the bundled Ollama run_model action. A cold model load can take minutes +// before the first byte, and dropping the declaration would silently regress +// to the 30s default (issue #25). +func TestBundledOllamaRunModelTimeout(t *testing.T) { + reg := NewRegistry() + if err := reg.LoadFS(bundledManifests, "manifests"); err != nil { + t.Fatal(err) + } + m, ok := reg.Get("ollama") + if !ok { + t.Fatal("ollama manifest not loaded") + } + act, ok := m.Actions["run_model"] + if !ok || act.HTTP == nil { + t.Fatal("ollama run_model action missing or not an http action") + } + if got, want := act.TimeoutS, 600; got != want { + t.Errorf("ollama run_model timeout_s = %d, want %d", got, want) + } +} + func writeManifest(t *testing.T, dir, name string, m Manifest) { t.Helper() data, err := json.MarshalIndent(m, "", " ") diff --git a/services/versions.json b/services/versions.json index 29d8c230..337dec8b 100644 --- a/services/versions.json +++ b/services/versions.json @@ -1,7 +1,7 @@ { "$comment": "Single source of truth for all version numbers. See VERSIONING.md for bump rules.", - "product": "0.91.7", - "installer": "0.91.7", + "product": "0.92.0", + "installer": "0.92.0", "components": { "ollama-proxy": "0.26.2", "lmstudio-proxy": "0.16.2", @@ -12,7 +12,7 @@ "nvpair-errors": "0.7.4", "nvpair-node-settings": "1.0.4", "nvpair-ui-broker": "0.40.2", - "nvpair-engine-manager": "0.17.4", + "nvpair-engine-manager": "0.18.0", "nvpair-cluster-manager": "1.1.4", "nvpair-job-scheduler": "0.4.1", "nvpair-tui": "0.7.2"