Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 113 additions & 7 deletions src/lib/inference/local.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -619,21 +619,127 @@ describe("local inference helpers", () => {
});

it("fails ollama model validation when Ollama returns an error payload", () => {
const result = validateOllamaModel("gabegoodhart/minimax-m2.1:latest", () =>
JSON.stringify({ error: "model requires more system memory" }),
);
const payload = JSON.stringify({ error: "model requires more system memory" });
const captureEx = () => ({ stdout: payload, exitCode: 0, timedOut: false });
const result = validateOllamaModel("gabegoodhart/minimax-m2.1:latest", () => payload, undefined, captureEx);
expect(result.ok).toBe(false);
expect(result.message).toMatch(/requires more system memory/);
});

it("passes ollama model validation when the probe returns a normal payload", () => {
const result = validateOllamaModel("nemotron-3-nano:30b", () =>
JSON.stringify({ model: "nemotron-3-nano:30b", response: "hello", done: true }),
);
const payload = JSON.stringify({ model: "nemotron-3-nano:30b", response: "hello", done: true });
const captureEx = () => ({ stdout: payload, exitCode: 0, timedOut: false });
const result = validateOllamaModel("nemotron-3-nano:30b", () => payload, undefined, captureEx);
expect(result).toEqual({ ok: true });
});

it("treats non-JSON probe output as success once the model responds", () => {
expect(validateOllamaModel("nemotron-3-nano:30b", () => "ok")).toEqual({ ok: true });
const captureEx = () => ({ stdout: "ok", exitCode: 0, timedOut: false });
expect(validateOllamaModel("nemotron-3-nano:30b", () => "ok", undefined, captureEx)).toEqual({ ok: true });
});

it("passes ollama memory validation when total RAM covers the model on unified-memory hosts", () => {
// Simulate Spark: Ollama returns available-RAM OOM error, but total RAM is 128 GB.
const freeOutput = " total used free\nMem: 131072 120000 1000";
const oomPayload = JSON.stringify({ error: "model requires more system memory (21.2 GiB) than is available (5.6 GiB)" });
const captureEx = () => ({ stdout: oomPayload, exitCode: 0, timedOut: false });
const capture = (cmd: string | string[]) => {
const c = Array.isArray(cmd) ? cmd.join(" ") : cmd;
if (c.includes("free")) return freeOutput;
return oomPayload;
};
const result = validateOllamaModel("nemotron-3-nano:30b", capture, () => true, captureEx);
expect(result.ok).toBe(true);
});

it("fails ollama memory validation when total RAM is also insufficient", () => {
const freeOutput = " total used free\nMem: 16384 15000 100";
const oomPayload = JSON.stringify({ error: "model requires more system memory (21.2 GiB) than is available (5.6 GiB)" });
const captureEx = () => ({ stdout: oomPayload, exitCode: 0, timedOut: false });
const capture = (cmd: string | string[]) => {
const c = Array.isArray(cmd) ? cmd.join(" ") : cmd;
if (c.includes("free")) return freeOutput;
return oomPayload;
};
const result = validateOllamaModel("nemotron-3-nano:30b", capture, () => true, captureEx);
expect(result.ok).toBe(false);
expect(result.message).toMatch(/failed the local probe/);
});

it("does not bypass OOM error on non-Spark hosts even with large total RAM", () => {
const freeOutput = " total used free\nMem: 262144 250000 1000";
const oomPayload = JSON.stringify({ error: "model requires more system memory (21.2 GiB) than is available (5.6 GiB)" });
const captureEx = () => ({ stdout: oomPayload, exitCode: 0, timedOut: false });
const capture = (cmd: string | string[]) => {
const c = Array.isArray(cmd) ? cmd.join(" ") : cmd;
if (c.includes("free")) return freeOutput;
return oomPayload;
};
const result = validateOllamaModel("nemotron-3-nano:30b", capture, () => false, captureEx);
expect(result.ok).toBe(false);
expect(result.message).toMatch(/failed the local probe/);
});

it("retries with extended timeout when first probe returns empty (slow model load on unified-memory host)", () => {
// Simulate Spark: first probe times out (curl exit 28), retry with 300s timeout succeeds.
const commands: string[] = [];
let captureExCallCount = 0;
const captureEx = (cmd: string[]) => {
captureExCallCount++;
commands.push(cmd.join(" "));
// First call: initial probe times out; second call: 300s retry succeeds.
if (captureExCallCount === 1) return { stdout: "", exitCode: 28, timedOut: true };
return { stdout: JSON.stringify({ response: "Hi" }), exitCode: 0, timedOut: false };
};
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => true, captureEx);
expect(result.ok).toBe(true);
expect(captureExCallCount).toBe(2);
expect(commands[1]).toMatch(/--max-time.*300|300.*--max-time/);
});

it("does not retry on non-Spark hosts when first probe returns empty", () => {
let callCount = 0;
const captureEx = () => { callCount++; return { stdout: "", exitCode: 7, timedOut: false }; };
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => false, captureEx);
expect(result.ok).toBe(false);
expect(callCount).toBe(1);
});

it("does not retry on Spark when probe fails fast (connection refused, not a timeout)", () => {
// exit code 7 = curl connection refused — should surface immediately, not stall 300s.
let callCount = 0;
const captureEx = () => { callCount++; return { stdout: "", exitCode: 7, timedOut: false }; };
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => true, captureEx);
expect(result.ok).toBe(false);
expect(callCount).toBe(1);
expect(result.message).toMatch(/did not answer the local probe in time/);
});
Comment on lines +708 to +716

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Non-timeout path is asserting a timeout message.

The test says this is a fast connection-refused case (timedOut: false, exit 7), but it still expects timeout wording. That can lock in misleading diagnostics for users and hide message regressions on this path.

Suggested test assertion adjustment
-    expect(result.message).toMatch(/did not answer the local probe in time/);
+    expect(result.message).toMatch(/connection refused|curl failed|exit 7/i);
+    expect(result.message).not.toMatch(/in time/);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("does not retry on Spark when probe fails fast (connection refused, not a timeout)", () => {
// exit code 7 = curl connection refused — should surface immediately, not stall 300s.
let callCount = 0;
const captureEx = () => { callCount++; return { stdout: "", exitCode: 7, timedOut: false }; };
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => true, captureEx);
expect(result.ok).toBe(false);
expect(callCount).toBe(1);
expect(result.message).toMatch(/did not answer the local probe in time/);
});
it("does not retry on Spark when probe fails fast (connection refused, not a timeout)", () => {
// exit code 7 = curl connection refused — should surface immediately, not stall 300s.
let callCount = 0;
const captureEx = () => { callCount++; return { stdout: "", exitCode: 7, timedOut: false }; };
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => true, captureEx);
expect(result.ok).toBe(false);
expect(callCount).toBe(1);
expect(result.message).toMatch(/connection refused|curl failed|exit 7/i);
expect(result.message).not.toMatch(/in time/);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/inference/local.test.ts` around lines 535 - 543, The test for
validateOllamaModel ("does not retry on Spark when probe fails fast...") is
asserting a timeout-style message even though captureEx returns exitCode: 7 and
timedOut: false (connection refused); update the assertion on result.message to
expect wording that reflects a connection-refused/fast-failure condition (e.g.,
contains "connection refused", "exit code 7", or similar probe failure text)
instead of "did not answer the local probe in time", keep the callCount and
result.ok assertions as-is, and ensure the captureEx/probe simulation remains
the same so the test validates the non-timeout failure path for
validateOllamaModel.


it("fails when both probe attempts return empty (model truly unhealthy or too slow)", () => {
const captureEx = () => ({ stdout: "", exitCode: 28, timedOut: true });
const result = validateOllamaModel("nemotron-3-nano:30b", () => "", () => true, captureEx);
expect(result.ok).toBe(false);
expect(result.message).toMatch(/did not answer the local probe in time/);
});

it("passes when first probe times out then retry returns OOM error but total RAM is sufficient", () => {
// Composite: mode 2 (first probe timeout) + mode 1 (retry returns OOM error).
const freeOutput = " total used free\nMem: 131072 120000 1000";
const oomPayload = JSON.stringify({ error: "model requires more system memory (21.2 GiB) than is available (5.6 GiB)" });
let captureExCallCount = 0;
const captureEx = (cmd: string[]) => {
captureExCallCount++;
// First call: initial probe times out; second call: 300s retry returns OOM error.
if (captureExCallCount === 1) return { stdout: "", exitCode: 28, timedOut: true };
return { stdout: oomPayload, exitCode: 0, timedOut: false };
};
const capture = (cmd: string | string[]) => {
const c = Array.isArray(cmd) ? cmd.join(" ") : cmd;
if (c.includes("free")) return freeOutput;
return "";
};
const result = validateOllamaModel("nemotron-3-nano:30b", capture, () => true, captureEx);
expect(result.ok).toBe(true);
});

});
39 changes: 37 additions & 2 deletions src/lib/inference/local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ import os from "node:os";
import nodePath from "node:path";
import type { CurlProbeResult } from "../adapters/http/probe";
import { runCurlProbe } from "../adapters/http/probe";
import type { CaptureResult } from "../runner";
import { buildSubprocessEnv } from "../subprocess-env";

const { shellQuote, runCapture } = require("../runner");
const { shellQuote, runCapture, runCaptureEx } = require("../runner");

import { OLLAMA_PORT, OLLAMA_PROXY_PORT, VLLM_PORT } from "../core/ports";
import { sleepSeconds } from "../core/wait";

const { isWsl } = require("../platform");
const { detectNvidiaPlatform } = require("./nim");

/** Port containers use to reach Ollama — proxy on non-WSL, direct on WSL2. */
export const OLLAMA_CONTAINER_PORT = isWsl() ? OLLAMA_PORT : OLLAMA_PROXY_PORT;
Expand All @@ -33,6 +35,8 @@ export const LARGE_OLLAMA_MIN_MEMORY_MB = 32768;

export type RunCaptureFn = (cmd: string | string[], opts?: { ignoreError?: boolean }) => string;

export type RunCaptureExFn = (cmd: string[]) => CaptureResult;

// Hosts that the WSL-side onboard CLI tries when probing Ollama. Native Linux
// and macOS only ever reach Ollama on the local loopback. WSL with Docker
// Desktop can also reach a Windows-host Ollama through the docker-desktop
Expand Down Expand Up @@ -720,10 +724,22 @@ export function getOllamaProbeCommand(
export function validateOllamaModel(
model: string,
runCaptureImpl?: RunCaptureFn,
isSparkImpl?: () => boolean,
runCaptureExImpl?: RunCaptureExFn,
): ValidationResult {
const capture = runCaptureImpl ?? runCapture;
const captureEx = runCaptureExImpl ?? runCaptureEx;
const isSpark = isSparkImpl ?? (() => detectNvidiaPlatform() === "spark");
const probeCmd = getOllamaProbeCommand(model);
const output = capture(probeCmd, { ignoreError: true });
const probeResult = captureEx(probeCmd);
let output = probeResult.stdout;
// On DGX Spark (128 GB unified memory), loading a large model from disk can take >2 min.
// Only retry with a 300 s timeout when the initial probe genuinely timed out — fast
// failures (connection refused, Ollama not running) surface immediately. (#3251)
if (isSpark() && probeResult.timedOut) {
const retryResult = captureEx(getOllamaProbeCommand(model, 300));
output = retryResult.stdout;
}
if (!output) {
return {
ok: false,
Expand All @@ -746,6 +762,25 @@ export function validateOllamaModel(
`model's capabilities and pick one whose list includes 'tools'.`,
};
}
// Ollama checks available RAM instead of total; false positive on DGX Spark
// unified-memory hosts where GPU and CPU share the same 128 GB pool. (#3251)
const memMatch = errText.match(
/model requires more system memory \(([0-9.]+)\s*GiB\) than is available \([0-9.]+\s*GiB\)/i,
);
if (memMatch && isSpark()) {
const requiresGiB = parseFloat(memMatch[1]);
const freeOut = capture(["free", "-m"], { ignoreError: true });
if (freeOut) {
const memLine = freeOut.split("\n").find((l: string) => l.includes("Mem:"));
if (memLine) {
const totalMB = parseInt(memLine.trim().split(/\s+/)[1], 10) || 0;
const totalGiB = totalMB / 1024;
if (totalGiB >= requiresGiB) {
return { ok: true };
}
}
}
}
return {
ok: false,
message: `Selected Ollama model '${model}' failed the local probe: ${errText}`,
Expand Down
44 changes: 44 additions & 0 deletions src/lib/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,49 @@ function runCapture(cmd: readonly string[], opts: CaptureOptions = {}): string {
// Unified redaction — see redact.ts (#2381).
const { redact, redactError, writeRedactedResult } = require("./security/redact");

/** Structured result returned by runCaptureEx. */
export interface CaptureResult {
stdout: string;
exitCode: number | null;
/** True when spawnSync sets result.error due to a timeout (ETIMEDOUT). */
timedOut: boolean;
}

/**
* Like runCapture but returns a structured result instead of throwing or
* collapsing errors to an empty string. Use this when the caller needs to
* distinguish a real timeout (curl exit 28 / spawn ETIMEDOUT) from other
* failures such as connection-refused.
*/
function runCaptureEx(cmd: readonly string[], opts: Omit<CaptureOptions, "ignoreError"> = {}): CaptureResult {
if (!Array.isArray(cmd) || cmd.length === 0) {
throw new Error("runCaptureEx: cmd must be a non-empty argv array");
}
const exe = cmd[0];
const args = cmd.slice(1);
const { env: extraEnv, stdio: _stdio, ...spawnOpts } = opts as CaptureOptions;
try {
const result = spawnSync(exe, args, {
...spawnOpts,
cwd: ROOT,
env: { ...process.env, ...extraEnv },
stdio: ["pipe", "pipe", "pipe"],
encoding: "utf-8",
});
const timedOut =
(result.error != null && (result.error as NodeJS.ErrnoException).code === "ETIMEDOUT") ||
result.status === 28;
const stdout = result.stdout || "";
return {
stdout: (typeof stdout === "string" ? stdout : stdout.toString("utf-8")).trim(),
exitCode: result.status,
timedOut,
};
} catch (err) {
throw redactError(err);
}
}

/**
* Shell-quote a value for safe interpolation into bash -c strings.
* Wraps in single quotes and escapes embedded single quotes.
Expand Down Expand Up @@ -295,6 +338,7 @@ export {
run,
runShell,
runCapture,
runCaptureEx,
runFile,
runInteractive,
runInteractiveShell,
Expand Down
6 changes: 5 additions & 1 deletion test/ollama-tools-capability.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ interface LocalInferenceModule {
validateOllamaModel: (
model: string,
capture?: CaptureFn,
isSparkImpl?: () => boolean,
captureExImpl?: (cmd: string[]) => { stdout: string; exitCode: number | null; timedOut: boolean },
) => { ok: boolean; message?: string };
setResolvedOllamaHost: (host: string) => void;
resetOllamaHostCache: () => void;
Expand Down Expand Up @@ -176,7 +178,9 @@ describe("validateOllamaModel — tools-capable error mapping", () => {
}),
},
]);
const result = localInference.validateOllamaModel("phi4", capture);
const payload = JSON.stringify({ error: "registry.ollama.ai/library/phi4 does not support tools" });
const captureEx = () => ({ stdout: payload, exitCode: 0, timedOut: false });
const result = localInference.validateOllamaModel("phi4", capture, () => false, captureEx);
expect(result.ok).toBe(false);
expect(result.message).toBeTruthy();
expect(result.message!).toContain("phi4");
Expand Down
Loading