fix(cli): surfaced tiny model download errors
Preserved worker-side download errors through TinyTitleClient and included them in tiny-models text and JSON failures. Fixes #3839
This commit is contained in:
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed `omp tiny-models download` JSON/text failures to include the worker-side download error instead of collapsing every worker failure to `ok:false`. ([#3839](https://github.com/can1357/oh-my-pi/issues/3839))
|
||||
|
||||
## [16.2.6] - 2026-06-29
|
||||
|
||||
### Changed
|
||||
|
||||
@@ -28,12 +28,21 @@ interface ProgressReporter {
|
||||
interface DownloadResult {
|
||||
model: TinyLocalModelKey;
|
||||
ok: boolean;
|
||||
error?: string;
|
||||
}
|
||||
|
||||
function writeLine(text = ""): void {
|
||||
process.stdout.write(`${text}\n`);
|
||||
}
|
||||
|
||||
function downloadErrorSummary(error: string | undefined): string | undefined {
|
||||
return error
|
||||
?.split(/\r?\n/)
|
||||
.map(line => line.trim())
|
||||
.find(line => line.length > 0)
|
||||
?.replace(/^Error:\s*/, "");
|
||||
}
|
||||
|
||||
export function resolveModels(model: string | undefined): TinyLocalModelKey[] {
|
||||
if (!model) return [DEFAULT_TINY_TITLE_LOCAL_MODEL_KEY];
|
||||
// `all` is a prefetch convenience: skip models that fail before load (unsupported
|
||||
@@ -101,10 +110,15 @@ async function downloadOne(modelKey: TinyLocalModelKey, json: boolean | undefine
|
||||
const label = getTinyLocalModelSpec(modelKey)?.label ?? modelKey;
|
||||
if (!json && !process.stdout.isTTY) writeLine(`Downloading ${label} (${modelKey})...`);
|
||||
const progress = makeProgressReporter(modelKey, json);
|
||||
const ok = await tinyTitleClient.downloadModel(modelKey, { onProgress: progress.onProgress });
|
||||
progress.finish(ok);
|
||||
if (!json && !process.stdout.isTTY) writeLine(ok ? `Downloaded ${label}.` : `Failed to download ${label}.`);
|
||||
return { model: modelKey, ok };
|
||||
const result = await tinyTitleClient.downloadModel(modelKey, { onProgress: progress.onProgress });
|
||||
progress.finish(result.ok);
|
||||
const error = downloadErrorSummary(result.error);
|
||||
if (!json && !process.stdout.isTTY) {
|
||||
writeLine(result.ok ? `Downloaded ${label}.` : `Failed to download ${label}${error ? `: ${error}` : ""}.`);
|
||||
} else if (!json && !result.ok && error) {
|
||||
writeLine(`${label} failed: ${error}`);
|
||||
}
|
||||
return result.error ? { model: modelKey, ok: result.ok, error: result.error } : { model: modelKey, ok: result.ok };
|
||||
}
|
||||
|
||||
export async function runTinyModelsCommand(command: TinyModelsCommandArgs): Promise<void> {
|
||||
|
||||
@@ -29,7 +29,12 @@ import type { TinyTitleProgressEvent, TinyTitleWorkerInbound, TinyTitleWorkerOut
|
||||
type PendingRequest =
|
||||
| { kind: "generate"; modelKey: TinyTitleLocalModelKey; resolve: (title: string | null) => void }
|
||||
| { kind: "complete"; modelKey: TinyMemoryLocalModelKey; resolve: (text: string | null) => void }
|
||||
| { kind: "download"; modelKey: TinyLocalModelKey; resolve: (ok: boolean) => void };
|
||||
| { kind: "download"; modelKey: TinyLocalModelKey; resolve: (result: TinyTitleDownloadResult) => void };
|
||||
|
||||
export interface TinyTitleDownloadResult {
|
||||
ok: boolean;
|
||||
error?: string;
|
||||
}
|
||||
|
||||
export interface TinyTitleDownloadOptions {
|
||||
signal?: AbortSignal;
|
||||
@@ -269,21 +274,21 @@ export class TinyTitleClient {
|
||||
}
|
||||
}
|
||||
|
||||
async downloadModel(modelKey: string, options: TinyTitleDownloadOptions = {}): Promise<boolean> {
|
||||
if (!isTinyLocalModelKey(modelKey)) return false;
|
||||
if (options.signal?.aborted) return false;
|
||||
async downloadModel(modelKey: string, options: TinyTitleDownloadOptions = {}): Promise<TinyTitleDownloadResult> {
|
||||
if (!isTinyLocalModelKey(modelKey)) return { ok: false };
|
||||
if (options.signal?.aborted) return { ok: false };
|
||||
|
||||
const unsubscribe = options.onProgress ? this.onProgress(options.onProgress) : undefined;
|
||||
try {
|
||||
const worker = this.#ensureWorker();
|
||||
const id = String(++this.#nextRequestId);
|
||||
const { promise, resolve } = Promise.withResolvers<boolean>();
|
||||
const { promise, resolve } = Promise.withResolvers<TinyTitleDownloadResult>();
|
||||
this.#addPending(id, { kind: "download", modelKey, resolve });
|
||||
const abort = (): void => {
|
||||
const pending = this.#pending.get(id);
|
||||
if (pending?.kind !== "download") return;
|
||||
this.#deletePending(id);
|
||||
pending.resolve(false);
|
||||
pending.resolve({ ok: false });
|
||||
};
|
||||
options.signal?.addEventListener("abort", abort, { once: true });
|
||||
try {
|
||||
@@ -294,11 +299,12 @@ export class TinyTitleClient {
|
||||
this.#deletePending(id);
|
||||
}
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
logger.debug("tiny-title: local model download failed", {
|
||||
modelKey,
|
||||
error: error instanceof Error ? error.message : String(error),
|
||||
error: message,
|
||||
});
|
||||
return false;
|
||||
return { ok: false, error: message };
|
||||
} finally {
|
||||
unsubscribe?.();
|
||||
}
|
||||
@@ -314,7 +320,7 @@ export class TinyTitleClient {
|
||||
for (const pending of this.#pending.values()) {
|
||||
this.#emitProgress({ modelKey: pending.modelKey, status: "error" });
|
||||
if (pending.kind === "generate" || pending.kind === "complete") pending.resolve(null);
|
||||
else pending.resolve(false);
|
||||
else pending.resolve({ ok: false });
|
||||
}
|
||||
this.#pending.clear();
|
||||
this.#refed = false;
|
||||
@@ -379,7 +385,7 @@ export class TinyTitleClient {
|
||||
return;
|
||||
}
|
||||
if (message.type === "downloaded") {
|
||||
if (pending.kind === "download") pending.resolve(true);
|
||||
if (pending.kind === "download") pending.resolve({ ok: true });
|
||||
return;
|
||||
}
|
||||
if (message.type === "completion") {
|
||||
@@ -389,8 +395,8 @@ export class TinyTitleClient {
|
||||
logger.debug("tiny-title: worker returned error", { error: message.error });
|
||||
this.#markFailedModel(pending);
|
||||
this.#emitProgress({ modelKey: pending.modelKey, status: "error" });
|
||||
if (pending.kind === "generate" || pending.kind === "complete") pending.resolve(null);
|
||||
else pending.resolve(false);
|
||||
if (pending.kind === "download") pending.resolve({ ok: false, error: message.error });
|
||||
else pending.resolve(null);
|
||||
void this.terminate();
|
||||
}
|
||||
|
||||
@@ -407,7 +413,7 @@ export class TinyTitleClient {
|
||||
for (const pending of this.#pending.values()) {
|
||||
this.#emitProgress({ modelKey: pending.modelKey, status: "error" });
|
||||
if (pending.kind === "generate" || pending.kind === "complete") pending.resolve(null);
|
||||
else pending.resolve(false);
|
||||
else pending.resolve({ ok: false, error: error.message });
|
||||
}
|
||||
this.#pending.clear();
|
||||
void this.terminate();
|
||||
|
||||
@@ -171,10 +171,30 @@ describe("issue #3291 — tiny-model downloads keep the worker referenced", () =
|
||||
|
||||
worker.emit({ type: "downloaded", id: downloadRequestId });
|
||||
|
||||
expect(await download).toBe(true);
|
||||
expect(await download).toEqual({ ok: true });
|
||||
expect(worker.unrefCalls).toBe(1);
|
||||
} finally {
|
||||
await client.terminate();
|
||||
}
|
||||
});
|
||||
|
||||
it("returns the worker error for failed download requests", async () => {
|
||||
let downloadRequestId = "";
|
||||
const worker = new FakeTinyWorker(message => {
|
||||
if (message.type === "download") downloadRequestId = message.id;
|
||||
});
|
||||
const client = new TinyTitleClient(() => worker);
|
||||
|
||||
try {
|
||||
const download = client.downloadModel("lfm2-700m");
|
||||
|
||||
expect(downloadRequestId).not.toBe("");
|
||||
worker.emit({ type: "error", id: downloadRequestId, error: "Error: runtime install failed" });
|
||||
|
||||
expect(await download).toEqual({ ok: false, error: "Error: runtime install failed" });
|
||||
expect(worker.terminated).toBe(true);
|
||||
} finally {
|
||||
await client.terminate();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import { resolveModels } from "@oh-my-pi/pi-coding-agent/cli/tiny-models-cli";
|
||||
import { afterEach, describe, expect, it, spyOn, vi } from "bun:test";
|
||||
import { resolveModels, runTinyModelsCommand } from "@oh-my-pi/pi-coding-agent/cli/tiny-models-cli";
|
||||
import { TINY_LOCAL_MODELS } from "@oh-my-pi/pi-coding-agent/tiny/models";
|
||||
import { tinyTitleClient } from "@oh-my-pi/pi-coding-agent/tiny/title-client";
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe("tiny-models download model resolution", () => {
|
||||
it("excludes load-blocked models from `all` so the bulk prefetch stays green", () => {
|
||||
@@ -25,4 +30,24 @@ describe("tiny-models download model resolution", () => {
|
||||
if (!blocked) return;
|
||||
expect(resolveModels(blocked.key)).toEqual([blocked.key]);
|
||||
});
|
||||
|
||||
it("includes worker error details in JSON failures", async () => {
|
||||
const output: string[] = [];
|
||||
spyOn(process.stdout, "write").mockImplementation((chunk: string | Uint8Array) => {
|
||||
output.push(typeof chunk === "string" ? chunk : new TextDecoder().decode(chunk));
|
||||
return true;
|
||||
});
|
||||
spyOn(tinyTitleClient, "downloadModel").mockResolvedValue({
|
||||
ok: false,
|
||||
error: "Error: runtime install failed\n at worker",
|
||||
});
|
||||
|
||||
await expect(
|
||||
runTinyModelsCommand({ action: "download", model: "lfm2-700m", flags: { json: true } }),
|
||||
).rejects.toThrow("One or more tiny title models failed to download");
|
||||
|
||||
expect(JSON.parse(output.join(""))).toEqual({
|
||||
results: [{ model: "lfm2-700m", ok: false, error: "Error: runtime install failed\n at worker" }],
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user