Select action response-header timeouts from the manifest

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: { <engine>: [string] }, loadedByEngine: { <engine>: [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. |

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)
 	}

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

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")
 	}
 }
 

services/nvpair-engine-manager/manifests/lmstudio.json

@@ -77,7 +77,8 @@
       "cmd": ["{cli}", "ls"]
     },
     "chat": {
-      "description": "OpenAI-compatible chat completion (params: {\"model\": \"<name>\", \"messages\": [...]}).",
+      "description": "OpenAI-compatible chat completion (params: {\"model\": \"<name>\", \"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": {

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\": \"<model>\", \"prompt\": \"<text>\", \"stream\": false}).",
+      "description": "Run a one-shot generation (params: {\"model\": \"<model>\", \"prompt\": \"<text>\", \"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": {

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)