diff --git a/CHANGELOG.md b/CHANGELOG.md index f068712..28eded0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,21 +43,31 @@ copy the entire working tree (including `.git` and `bun.lock`) into the install dir and previously routed through Bun, causing ABI mismatches with the Node-built `better-sqlite3` / `sqlite-vec` native modules. -- Darwin Metal: launcher (`bin/qmd`) and `bun test` preload now set - `GGML_METAL_NO_RESIDENCY=1` by default on macOS to disable libggml-metal's - residency-set keep-alive timer. Previously, llama-using commands (`query`, - `vsearch`, `embed`) and the test runner dumped a multi-kB GGML/Metal - backtrace at process exit even when output succeeded - (ggml-org/llama.cpp#17869) — the static `ggml_metal_device` destructor - asserts `[rsets->data count] == 0` during `__cxa_finalize_ranges`, but the - residency set's 180 s keep-alive timer hasn't expired yet. Residency sets - give no measurable speedup for QMD's short-lived CLI workflow (benchmarked - on M3 Pro), so disabling them is a pure win. The Bun preload uses - `bun:ffi` to call libc `setenv()` directly because Bun does not propagate - `process.env` mutations through to native `getenv()` (Node does). Opt back - in with `QMD_METAL_KEEP_RESIDENCY=1` for long-lived qmd processes (e.g. the - MCP daemon may benefit on hot reload) or to triage the upstream fix. - `qmd doctor` now reports the mitigation state. +- Darwin Metal: llama-using commands (`query`, `vsearch`, `embed`) no longer + dump a multi-kB GGML/Metal backtrace at process exit even when output + succeeded. The libggml-metal static `ggml_metal_device` destructor asserts + `[rsets->data count] == 0` during `__cxa_finalize_ranges`, but the + buffer-free path never calls the symmetric `ggml_metal_device_rsets_rm` + to remove released rsets from the device collection (upstream + ggml-org/llama.cpp#22593, one-line fix open as PR #22595). The assertion + only fires when `process.exit()` skips Node's `beforeExit` hook, which is + what node-llama-cpp uses to auto-dispose Metal contexts. Primary fix: + `finishSuccessfulCliCommand` now sets `process.exitCode = 0` and returns + instead of calling `process.exit(0)`, so `beforeExit` fires and the native + binding cleans up before libc's static destructor runs. Defense-in-depth: + the launcher (`bin/qmd`) and the npm test driver (`scripts/test-all.mjs` + + the `test:bun` / `test:unit` package.json scripts) also set + `GGML_METAL_NO_RESIDENCY=1` on darwin before spawning node/bun, covering + error paths and tests that still terminate via `process.exit()`. The env + var must be set before node/bun start — libggml-metal reads it via libc + `getenv` at module-load time, and Bun does not propagate `process.env` + mutations to libc `setenv` — so it lives in the launcher rather than in + test-preload. Residency sets give no measurable speedup for QMD's + short-lived CLI workflow (benchmarked on M3 Pro). Opt back in with + `QMD_METAL_KEEP_RESIDENCY=1` for long-lived qmd processes (e.g. the MCP + daemon may benefit on hot reload) or to triage the upstream fix. + `qmd doctor` reports the mitigation state. Minimal reproduction: + `scripts/repro-metal-rsets-crash.mjs`. ### Docs diff --git a/bin/qmd b/bin/qmd index 3983dc4..47f9764 100755 --- a/bin/qmd +++ b/bin/qmd @@ -33,7 +33,7 @@ if (process.argv[2] === "mcp") { // libggml-metal on macOS uses "residency sets" to keep allocated model memory // resident across inference requests (180-second keep_alive timer). The // process-static device destructor that runs during libc exit() asserts the -// residency set is empty (ggml-org/llama.cpp#17869); the keep_alive hasn't +// residency set is empty (ggml-org/llama.cpp#22593); the keep_alive hasn't // expired by exit, so the assertion fails and ggml_abort dumps a multi-kB // stack trace to stderr even when the user-visible results were already // emitted correctly. No JS-side dispose can prevent it because the static diff --git a/scripts/repro-metal-rsets-crash.mjs b/scripts/repro-metal-rsets-crash.mjs new file mode 100644 index 0000000..87b2e88 --- /dev/null +++ b/scripts/repro-metal-rsets-crash.mjs @@ -0,0 +1,118 @@ +#!/usr/bin/env node +/** + * Minimal reproduction of llama.cpp issue ggml-org/llama.cpp#22593: + * + * ggml-metal-device.m:612: GGML_ASSERT([rsets->data count] == 0) failed + * + * Root cause (per the upstream issue and proposed fix PR #22595): + * `ggml_metal_buffer_rset_free` releases the per-buffer residency set object + * but does NOT call the symmetric `ggml_metal_device_rsets_rm`. So the + * device's `rsets->data` array accumulates dangling references. When the + * process exits and libc fires the process-static `ggml_metal_device` + * destructor in `__cxa_finalize_ranges`, the destructor asserts the + * array is empty — and it isn't. + * + * Observed downstream behavior: + * - With EXPLICIT `dispose()` of every JS handle in order, the assertion + * does NOT fire. node-llama-cpp's dispose path tears the Metal buffers + * down before the static dtor runs, so the device's rsets array is + * empty by exit time. (Tested locally — clean exit.) + * - With NO dispose (the typical real-world case: synchronous `exit()`, + * `--watch` mode, `process.exit()` after results are written, or any + * code path where GC + finalizers race with libc exit), the rset + * references linger until the static dtor fires, and the assertion + * trips. + * + * What this script does: + * 1. Load node-llama-cpp + a small GGUF model on the Metal backend. + * This allocates at least one Metal buffer → calls rsets_add internally. + * 2. Run an inference (creating an embedding context populates buffers + * that the dispose path would normally clean up). + * 3. Skip explicit dispose. Just let the process exit. + * + * Expected behavior on macOS 15+ with Apple Silicon, current llama.cpp + * (bundled in node-llama-cpp 3.18.1, llama.cpp tag b8390): + * - Without GGML_METAL_NO_RESIDENCY: + * Script writes "ok" and main() returns, then ggml_abort fires the + * assertion, prints a multi-kB backtrace, and the process exits with + * SIGABRT (exit code 134). + * - With GGML_METAL_NO_RESIDENCY=1: + * Clean exit code 0. Residency-set code path is skipped entirely. + * - With --dispose flag (manual cleanup): + * Clean exit code 0 even without the env var, as long as JS dispose() + * runs successfully before libc exit. + * + * Usage: + * # Reproduce the crash (no dispose, no env var) + * node scripts/repro-metal-rsets-crash.mjs + * + * # Verify the documented workaround + * GGML_METAL_NO_RESIDENCY=1 node scripts/repro-metal-rsets-crash.mjs + * + * # Verify that explicit dispose also avoids the crash + * node scripts/repro-metal-rsets-crash.mjs --dispose + * + * Refs: + * https://github.com/ggml-org/llama.cpp/issues/22593 (root-cause analysis) + * https://github.com/ggml-org/llama.cpp/pull/22595 (one-line fix, open) + * https://github.com/tobi/qmd/issues/368 (downstream report) + * https://github.com/tobi/qmd/issues/674 (downstream, current) + * https://github.com/tobi/qmd/pull/600 (downstream workaround PR) + */ + +import { existsSync } from "node:fs"; +import { homedir } from "node:os"; +import { resolve } from "node:path"; + +const DEFAULT_MODEL = resolve( + homedir(), + ".cache/qmd/models/hf_ggml-org_embeddinggemma-300M-Q8_0.gguf", +); + +const args = process.argv.slice(2); +const wantsDispose = args.includes("--dispose"); +const modelPath = args.find((a) => !a.startsWith("--")) ?? DEFAULT_MODEL; + +if (!existsSync(modelPath)) { + console.error(`Model not found: ${modelPath}`); + console.error("Pass a path to any local GGUF as argv[1], or run `qmd embed` once to populate the default cache path."); + process.exit(2); +} + +console.error( + `[repro] GGML_METAL_NO_RESIDENCY=${process.env.GGML_METAL_NO_RESIDENCY ?? "(unset)"}`, +); +console.error(`[repro] dispose=${wantsDispose}`); +console.error(`[repro] loading: ${modelPath}`); + +const { getLlama } = await import("node-llama-cpp"); + +const llama = await getLlama(); +const model = await llama.loadModel({ modelPath }); +const context = await model.createEmbeddingContext(); + +console.error(`[repro] backend: ${llama.gpu}`); + +// Run actual inference so the buffer-allocation path is hit. +await context.getEmbeddingFor("repro text"); + +if (wantsDispose) { + console.error("[repro] explicit dispose…"); + await context.dispose(); + await model.dispose(); + await llama.dispose(); +} + +console.error("[repro] main() returning via process.exit(0)"); +console.log("ok"); + +// CRITICAL: use process.exit(), not `return`. node-llama-cpp registers a +// `process.once('beforeExit', …)` hook that auto-disposes WeakRef'd Llama +// instances when the event loop empties naturally. `process.exit()` skips +// `beforeExit`, so the rsets stay populated until libc's `exit()` fires the +// static dtor — which is when the upstream assertion bug trips. +// +// CLI tools (qmd query, qmd vsearch, qmd embed, etc.) all call process.exit() +// after writing results, which is why every real downstream report crashes +// even though the minimal "let main return" version does not. +process.exit(0); diff --git a/scripts/test-all.mjs b/scripts/test-all.mjs index 5dafc4d..bd4f660 100644 --- a/scripts/test-all.mjs +++ b/scripts/test-all.mjs @@ -5,6 +5,17 @@ import { fileURLToPath } from "node:url"; const root = fileURLToPath(new URL("..", import.meta.url)); +// Mirror bin/qmd's darwin Metal residency mitigation for test subprocesses. +// libggml-metal asserts on a non-empty residency set during its static +// destructor (ggml-org/llama.cpp#22593, fix open as #22595) and dumps a +// multi-kB backtrace at process exit even when tests pass. The env var must +// be set BEFORE the subprocess starts because libggml-metal reads it via +// libc getenv at module-load time. Opt out with QMD_METAL_KEEP_RESIDENCY=1. +const darwinMetalEnv = + process.platform === "darwin" && process.env.QMD_METAL_KEEP_RESIDENCY !== "1" + ? { GGML_METAL_NO_RESIDENCY: "1" } + : {}; + function run(label, command, args, options = {}) { console.log(`==> ${label}`); const { env: extraEnv, ...spawnOptions } = options; @@ -12,7 +23,7 @@ function run(label, command, args, options = {}) { cwd: root, stdio: "inherit", shell: process.platform === "win32", - env: { ...process.env, ...(extraEnv ?? {}) }, + env: { ...process.env, ...darwinMetalEnv, ...(extraEnv ?? {}) }, ...spawnOptions, }); if (result.status !== 0) { diff --git a/src/cli/qmd.ts b/src/cli/qmd.ts index b95cfcc..aff0af7 100755 --- a/src/cli/qmd.ts +++ b/src/cli/qmd.ts @@ -238,18 +238,29 @@ async function flushWritable(stream: CliLifecycleWritable): Promise { /** * Finish a successful CLI command after output has been flushed. * - * Best-effort llama disposal (JS-side resources only): if it throws, we still - * exit 0 because the user's output is already on the wire. The libggml-metal - * static-destructor crash on darwin (ggml-org/llama.cpp#17869) is prevented - * by `bin/qmd` exporting `GGML_METAL_NO_RESIDENCY=1` before the native - * binding loads — see `isDarwinMetalMitigationActive` in `src/llm.ts` for - * the runtime check exposed to diagnostics. Every code path that touches - * Metal (query, vsearch, embed, expand) is covered by that single env var, - * with no per-command bypass logic required here. + * We deliberately do NOT call `process.exit(0)`. `process.exit()` skips + * Node's `beforeExit` event, and node-llama-cpp registers a `beforeExit` hook + * that auto-disposes its native handles. On darwin, without that hook firing, + * libggml-metal's static `ggml_metal_device` destructor asserts on a + * non-empty residency-set collection during `__cxa_finalize_ranges` and + * dumps a multi-kB backtrace (upstream ggml-org/llama.cpp#22593, fix open as + * PR #22595). Empirically, even with explicit `disposeDefaultLlamaCpp()` the + * direct `process.exit(0)` path still trips the assertion — letting the + * event loop drain naturally is what actually clears the rsets. + * + * So: set `process.exitCode = 0` and return. The main module finishes, the + * event loop drains, `beforeExit` fires, native resources tear down in + * order, and the process exits cleanly. The `GGML_METAL_NO_RESIDENCY=1` env + * var that `bin/qmd` exports is a defense-in-depth safety net for paths + * that still call `process.exit()` after loading the native binding + * (signal handlers, error paths, `bun test`). + * + * If the caller passes an explicit `exit` for testability, we honor it — + * the lifecycle tests verify the legacy flush → cleanup → exit ordering. + * Production callers must not pass `exit`. */ export async function finishSuccessfulCliCommand(options: FinishSuccessfulCliCommandOptions): Promise { const stderr = options.stderr ?? process.stderr; - const exit = options.exit ?? ((code: number) => process.exit(code)); await flushWritable(options.stdout ?? process.stdout); @@ -261,7 +272,13 @@ export async function finishSuccessfulCliCommand(options: FinishSuccessfulCliCom ); } await flushWritable(stderr); - exit(0); + + if (options.exit) { + options.exit(0); + return; + } + + process.exitCode = 0; } // Ensure cursor is restored on exit @@ -3587,8 +3604,8 @@ function collectEnvironmentOverrides(activeModels: { embed: string; generate: st add("QMD_EMBED_CONTEXT_SIZE", "overrides embed context size; larger values use more memory"); add("QMD_EDITOR_URI", "overrides clickable editor link template in terminal output"); add("QMD_SKILLS_DIR", "overrides where qmd skills are discovered from"); - add("QMD_METAL_KEEP_RESIDENCY", "opts back into libggml-metal residency sets on darwin; restores ~0ms perf wins for long-lived processes but re-exposes the static-destructor backtrace dump at process exit (ggml-org/llama.cpp#17869)"); - add("GGML_METAL_NO_RESIDENCY", "set automatically by the launcher on darwin to disable Metal residency sets (avoids ggml-org/llama.cpp#17869); override via QMD_METAL_KEEP_RESIDENCY=1"); + add("QMD_METAL_KEEP_RESIDENCY", "opts back into libggml-metal residency sets on darwin; restores ~0ms perf wins for long-lived processes but re-exposes the static-destructor backtrace dump at process exit (ggml-org/llama.cpp#22593)"); + add("GGML_METAL_NO_RESIDENCY", "set automatically by the launcher on darwin to disable Metal residency sets (avoids ggml-org/llama.cpp#22593); override via QMD_METAL_KEEP_RESIDENCY=1"); add("NO_COLOR", "disables colored terminal output"); add("CI", "disables real LLM operations inside QMD's LlamaCpp wrapper"); add("HF_ENDPOINT", "changes Hugging Face download endpoint used when pulling models"); @@ -3871,7 +3888,7 @@ async function runDoctorDeviceChecks(nextSteps: string[]): Promise { // Surface the darwin residency-set mitigation. libggml-metal's // process-static device dtor asserts on un-expired residency sets - // during libc exit() (ggml-org/llama.cpp#17869), producing a giant + // during libc exit() (ggml-org/llama.cpp#22593), producing a giant // stderr backtrace after correct output. The bin/qmd launcher exports // GGML_METAL_NO_RESIDENCY=1 on darwin to skip the assertion entirely. // No measurable perf cost on short-lived CLI calls. @@ -3880,13 +3897,13 @@ async function runDoctorDeviceChecks(nextSteps: string[]): Promise { doctorCheck( "darwin metal residency", true, - "GGML_METAL_NO_RESIDENCY=1 set by launcher; clean process exit (avoids ggml-org/llama.cpp#17869). Opt back in with QMD_METAL_KEEP_RESIDENCY=1 if you run long-lived qmd processes." + "GGML_METAL_NO_RESIDENCY=1 set by launcher; clean process exit (avoids ggml-org/llama.cpp#22593). Opt back in with QMD_METAL_KEEP_RESIDENCY=1 if you run long-lived qmd processes." ); } else { doctorCheck( "darwin metal residency", false, - "residency sets active (QMD_METAL_KEEP_RESIDENCY=1 or launcher bypassed); llama-using commands may dump a libggml-metal backtrace at exit (ggml-org/llama.cpp#17869) even when output succeeded." + "residency sets active (QMD_METAL_KEEP_RESIDENCY=1 or launcher bypassed); llama-using commands may dump a libggml-metal backtrace at exit (ggml-org/llama.cpp#22593) even when output succeeded." ); nextSteps.push("Unset `QMD_METAL_KEEP_RESIDENCY` so the launcher can disable Metal residency sets; without this, query/vsearch/embed dump a stack trace at exit even on success."); } diff --git a/src/llm.ts b/src/llm.ts index 4ea0d00..7c2d464 100644 --- a/src/llm.ts +++ b/src/llm.ts @@ -722,7 +722,7 @@ export class LlamaCpp implements LLM { constructor(config: LlamaCppConfig = {}) { // STRUCTURAL INVARIANT: the launcher (bin/qmd) sets GGML_METAL_NO_RESIDENCY=1 // on darwin BEFORE the native binding loads, which prevents the libggml-metal - // static destructor assertion at process exit (ggml-org/llama.cpp#17869). + // static destructor assertion at process exit (ggml-org/llama.cpp#22593). // See isDarwinMetalMitigationActive() for the runtime check exposed to // diagnostics. No constructor-time guard installation is needed. @@ -1960,7 +1960,7 @@ export function canUnloadLLM(): boolean { // destructor fires during libc `exit()` → `__cxa_finalize_ranges` and asserts // `[rsets->data count] == 0` — but the keep_alive hasn't expired, so the // assertion fails and `ggml_abort` dumps a multi-kilobyte stack trace to -// stderr after the user-visible output. See ggml-org/llama.cpp#17869. +// stderr after the user-visible output. See ggml-org/llama.cpp#22593. // // No JS-side dispose call (`llama.dispose()`, `model.dispose()`, etc.) can // prevent it: the static destructor runs after every JS-reachable cleanup, diff --git a/src/test-preload.ts b/src/test-preload.ts index c2e31e2..f10909c 100644 --- a/src/test-preload.ts +++ b/src/test-preload.ts @@ -1,45 +1,20 @@ /** * Test preload file to ensure proper cleanup of native resources. * - * Uses bun:test afterAll to properly dispose of llama.cpp Metal - * resources before the process exits, avoiding GGML_ASSERT failures. + * Uses bun:test afterAll to dispose of llama.cpp Metal resources before + * the process exits — necessary on darwin to avoid the upstream rsets + * destructor assertion (ggml-org/llama.cpp#22593, fix open as #22595). + * + * The runner-level mitigation `GGML_METAL_NO_RESIDENCY=1` must be set + * BEFORE bun/node starts (libggml-metal reads it via libc getenv at + * module load). Bun does not propagate `process.env` writes to libc + * setenv, so setting it from here would be a no-op for the native + * binding. The env var is injected by: + * - bin/qmd for production CLI runs + * - scripts/test-all.mjs for `npm test` + * - package.json test:bun / test:unit scripts for direct invocation + * See CLAUDE.md for invoking `bun test` manually on darwin. */ - -// Mirror bin/qmd's darwin Metal residency mitigation so `bun test` and -// `vitest` runs exit cleanly. The test runners load node-llama-cpp directly -// without going through the launcher, so the libggml-metal static destructor -// asserts on a non-empty residency set during __cxa_finalize_ranges and dumps -// a multi-kB backtrace at process exit (ggml-org/llama.cpp#17869). Opt back -// in with QMD_METAL_KEEP_RESIDENCY=1 if you're triaging the upstream Metal -// teardown bug. -// -// Two-step propagation, because: -// - Native code in libggml-metal reads via C getenv() at module load time. -// - Node syncs process.env mutations to libc via uv_os_setenv automatically. -// - Bun does NOT — `process.env.X = "1"` only updates the JS-level object, -// so getenv() in the C++ binding still sees nothing (verified empirically -// with bun:ffi). We have to call setenv() ourselves on Bun. -if (process.platform === "darwin" && process.env.QMD_METAL_KEEP_RESIDENCY !== "1") { - process.env.GGML_METAL_NO_RESIDENCY = process.env.GGML_METAL_NO_RESIDENCY || "1"; - - if (typeof (globalThis as { Bun?: unknown }).Bun !== "undefined") { - try { - const { dlopen, FFIType, suffix } = await import("bun:ffi"); - const libc = dlopen(`libSystem.${suffix}`, { - setenv: { args: [FFIType.cstring, FFIType.cstring, FFIType.i32], returns: FFIType.i32 }, - }); - libc.symbols.setenv( - Buffer.from("GGML_METAL_NO_RESIDENCY\0", "utf8"), - Buffer.from("1\0", "utf8"), - 1, - ); - } catch { - // FFI unavailable on this Bun build — the backtrace dump at exit is - // cosmetic; tests still pass. - } - } -} - import { afterAll } from "bun:test"; import { disposeDefaultLlamaCpp } from "./llm"; diff --git a/test/cli-exit-lifecycle.test.ts b/test/cli-exit-lifecycle.test.ts index 5468e0b..e2896b5 100644 --- a/test/cli-exit-lifecycle.test.ts +++ b/test/cli-exit-lifecycle.test.ts @@ -26,11 +26,10 @@ describe("CLI successful-exit lifecycle", () => { expect(flushed).toEqual([""]); }); - test("flushes stdout then stderr then exits, disposing along the way", async () => { - // After widening the safe-exit into a process-wide guard installed by the - // LlamaCpp constructor, the per-command 'immediate exit' branch is gone: - // every command takes the same flush → dispose → exit(0) path, and the - // darwin guard catches the C++ static dtor crash at process-exit time. + test("flushes stdout, runs cleanup, flushes stderr, then exits (when exit is provided)", async () => { + // The legacy lifecycle order is preserved for callers that pass an + // explicit `exit` function — primarily this test, which needs an + // observable terminating step. const calls: string[] = []; await finishSuccessfulCliCommand({ @@ -45,6 +44,31 @@ describe("CLI successful-exit lifecycle", () => { expect(calls).toEqual(["stdout-flush", "cleanup", "stderr-flush", "exit:0"]); }); + test("production path: sets process.exitCode=0 and returns instead of calling process.exit", async () => { + // The real CLI does NOT pass `exit` — finishSuccessfulCliCommand should set + // process.exitCode and return, letting Node's `beforeExit` fire so + // node-llama-cpp's auto-dispose runs BEFORE libc's static destructor. + // process.exit() skips `beforeExit`, which is what trips the libggml-metal + // assertion (ggml-org/llama.cpp#22593) even with explicit dispose. + const prevCode = process.exitCode; + process.exitCode = 1; // poison the state to verify we set it + try { + const calls: string[] = []; + await finishSuccessfulCliCommand({ + command: "query", + format: "json", + cleanup: async () => { calls.push("cleanup"); }, + stdout: { write: (_c: string | Uint8Array, cb?: (error?: Error | null) => void) => { calls.push("stdout-flush"); cb?.(); return true; } }, + stderr: { write: (_c: string | Uint8Array, cb?: (error?: Error | null) => void) => { calls.push("stderr-flush"); cb?.(); return true; } }, + }); + + expect(calls).toEqual(["stdout-flush", "cleanup", "stderr-flush"]); + expect(process.exitCode).toBe(0); + } finally { + process.exitCode = prevCode; + } + }); + test("darwin Metal mitigation reflects launcher-exported env on darwin", () => { // The real mitigation lives in bin/qmd, which sets GGML_METAL_NO_RESIDENCY=1 // before Node loads the llama.cpp native binding. The JS-side predicate