From dbcb619e589e689b9b2c2dd1df7b953ffeca6be7 Mon Sep 17 00:00:00 2001 From: Erich Champion Date: Sun, 6 Sep 2026 18:30:43 -0700 Subject: [PATCH 1/2] core: route the fatal exit(1) sites through ace_fatal(), behind ACESTEP_FATAL_THROWS ace_fatal() centralizes the engine's fatal exits: one function, one format, one place to change. By default it prints and exit(1)s exactly as the sites it replaces did, so the engine's behaviour is unchanged; building with -DACESTEP_FATAL_THROWS=ON makes it throw ace_fatal_error instead, so an embedder can catch a failed model load and show an error rather than dying. The flag is PUBLIC and build-wide (it gates the body of an inline function), the same way GGML_MAX_NAME is: defining it on only some targets is an ODR violation. It stays OFF by default and the CMakeLists says why: the model-store load path frees cleanly on a throw (LoadGuard, GgufCloser, transactional install -- all in this change), but a forward-time fatal still leaks, so the throwing mode is load-path only until that follow-up lands. Also fixes two latent bugs found on the way: GGUFModel's fd sentinel was 0, so a zeroed STFile closed stdin; and backend_release() crashed on a null backend. tests/test-ace-fatal.cpp covers the flag's both modes. --- CMakeLists.txt | 13 ++++ src/ace-fatal.h | 92 +++++++++++++++++++++++ src/backend.h | 36 ++++++--- src/cond-enc.h | 5 +- src/dit.h | 23 +++--- src/fsq-detok.h | 1 + src/fsq-tok.h | 1 + src/gguf-weights.h | 53 +++++++++----- src/model-store.cpp | 153 +++++++++++++++++++++++++++++++-------- src/qwen3-enc.h | 8 +- src/qwen3-lm.h | 14 ++-- src/vae-enc.h | 8 +- src/vae.h | 8 +- tests/CMakeLists.txt | 5 ++ tests/test-ace-fatal.cpp | 95 ++++++++++++++++++++++++ 15 files changed, 423 insertions(+), 92 deletions(-) create mode 100644 src/ace-fatal.h create mode 100644 tests/test-ace-fatal.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index a8792bb3..2c7587c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -121,6 +121,19 @@ add_library(acestep-core STATIC target_link_libraries(acestep-core PUBLIC yyjson) link_ggml_backends(acestep-core) +# ace_fatal() exit(1)s by default; with this ON it throws ace_fatal_error so an +# embedder can catch a failed load instead of dying (see src/ace-fatal.h). It +# gates the body of an inline function, so it is PUBLIC and build-wide -- the one +# correct way to turn it on, uniformly across acestep-core and its consumers, the +# way GGML_MAX_NAME is. Defining it on only some targets is an ODR violation. +# OFF by default, and do not turn it on until the inference path is +# exception-safe: today only the model-store load path frees cleanly on a throw +# (a forward-time fatal still leaks -- see the ACESTEP_FATAL_THROWS follow-up). +option(ACESTEP_FATAL_THROWS "ace_fatal() throws instead of exit(1) (load-path only so far)" OFF) +if(ACESTEP_FATAL_THROWS) + target_compile_definitions(acestep-core PUBLIC ACESTEP_FATAL_THROWS) +endif() + # ace-synth: full pipeline (text-enc + cond + dit + vae + wav) add_executable(ace-synth tools/ace-synth.cpp) target_link_libraries(ace-synth PRIVATE acestep-core) diff --git a/src/ace-fatal.h b/src/ace-fatal.h new file mode 100644 index 00000000..fcb0b449 --- /dev/null +++ b/src/ace-fatal.h @@ -0,0 +1,92 @@ +#pragma once +// One place the engine says "this load cannot continue." +// +// By default ace_fatal() prints the message to stderr and exit(1)s -- byte for +// byte what the ~25 `fprintf(stderr, "[X] FATAL: ...\n", ...); exit(1);` sites +// did inline before. That is correct for the CLIs. +// +// Define ACESTEP_FATAL_THROWS (the app does; the CLIs do not) and the same +// message is thrown as ace_fatal_error instead, so C++ stack unwinding runs: +// ModelStore's LoadGuard frees the half-built module (its backend, scheduler, +// ggml_context and weight buffer) and its std::lock_guard unlocks, rather than +// the process dying mid-load. The message is echoed to stderr *and* carried on +// the exception. (This covers the load path; an inference-time fatal still +// unwinds through code that is not yet exception-safe -- see the follow-up.) +// +// Where the exception ends up depends on who raised it. ModelStore catches +// ace_fatal_error at the load boundary (store_require_*) and turns a failed load +// into the nullptr it already returned on failure -- so load callers are +// unchanged and read the reason from stderr. A fatal raised anywhere without +// such a boundary propagates to the caller carrying the message. +// +// ace_fatal_error is defined even in the default (non-throwing) build so callers +// can `catch (const ace_fatal_error &)` unconditionally; it is simply never +// thrown there. Catching this specific type rather than `...` lets an unrelated +// std::bad_alloc keep propagating instead of being masked as a benign failure. +// +// ACESTEP_FATAL_THROWS gates the *body* of an inline function, so it must be +// defined uniformly for the whole build -- a target-wide compile definition, the +// way GGML_MAX_NAME already is. Defining it for only some translation units in a +// binary is an ODR violation: the two bodies differ, the linker silently keeps +// one, and fatals in the odd TUs would exit(1) instead of throwing. +// +// Why an exception and not the alternatives (see #17): +// - setjmp/longjmp skips C++ destructors -- it would leak the half-built +// module's ggml_context and backend buffer and leave the lock_guard held, +// deadlocking the next call. +// - Propagating a bool up the chain is a ~2000-line diff across seven headers +// for the ~1000 straight-line load calls, and the right behaviour on +// "tensor missing" is to abandon the whole load anyway. +#include +#include +#include +#include +#include +#include + +#if defined(__GNUC__) || defined(__clang__) +# define ACE_FATAL_PRINTF_FMT __attribute__((format(printf, 2, 3))) +#else +# define ACE_FATAL_PRINTF_FMT +#endif + +// Carries the exit code the CLI would have used and the formatted message. +// Defined unconditionally; only thrown under ACESTEP_FATAL_THROWS. +struct ace_fatal_error : std::runtime_error { + int code; + ace_fatal_error(int c, std::string msg) : std::runtime_error(std::move(msg)), code(c) {} +}; + +[[noreturn]] ACE_FATAL_PRINTF_FMT inline void ace_fatal(int code, const char * fmt, ...) { + va_list ap; + va_start(ap, fmt); +#ifdef ACESTEP_FATAL_THROWS + // Format into a string so the message rides with the exception, and echo it + // to stderr so the diagnostic is present whether or not a caller catches. + va_list ap2; + va_copy(ap2, ap); + char stackbuf[1024]; + int n = vsnprintf(stackbuf, sizeof(stackbuf), fmt, ap); + std::string msg; + if (n < 0) { + // Encoding error mid-format: the expanded text is unavailable. Mark it as + // such rather than pass the raw format string (unexpanded %s and all) off + // as if it were the diagnostic. + msg = std::string("ace_fatal: could not format message: ") + fmt; + } else if (static_cast(n) < sizeof(stackbuf)) { + msg.assign(stackbuf, static_cast(n)); + } else { + msg.resize(static_cast(n) + 1); + vsnprintf(&msg[0], msg.size(), fmt, ap2); + msg.resize(static_cast(n)); // drop the NUL vsnprintf wrote + } + va_end(ap2); + va_end(ap); + fputs(msg.c_str(), stderr); + throw ace_fatal_error(code, std::move(msg)); +#else + vfprintf(stderr, fmt, ap); + va_end(ap); + exit(code); +#endif +} diff --git a/src/backend.h b/src/backend.h index 394cbcbb..92075de6 100644 --- a/src/backend.h +++ b/src/backend.h @@ -5,11 +5,13 @@ // keep CPU as fallback. This avoids duplicating init logic across // qwen3.h, qwen3-lm.h, cond.h, dit.h, vae.h. +#include "ace-fatal.h" #include "ggml-backend.h" #include #include #include +#include #include struct BackendPair { @@ -109,19 +111,19 @@ static BackendPair backend_init(const char * label) { if (force_backend) { bp.backend = ggml_backend_init_by_name(force_backend, nullptr); if (!bp.backend) { - fprintf(stderr, "[Load] FATAL: GGML_BACKEND=%s not found. Available:", force_backend); + std::string avail; for (size_t i = 0; i < ggml_backend_dev_count(); i++) { - fprintf(stderr, " %s", ggml_backend_dev_name(ggml_backend_dev_get(i))); + avail += ' '; + avail += ggml_backend_dev_name(ggml_backend_dev_get(i)); } - fprintf(stderr, "\n"); - exit(1); + ace_fatal(1, "[Load] FATAL: GGML_BACKEND=%s not found. Available:%s\n", force_backend, + avail.c_str()); } } else { bp.backend = ggml_backend_init_best(); } if (!bp.backend) { - fprintf(stderr, "[Load] FATAL: no backend available\n"); - exit(1); + ace_fatal(1, "[Load] FATAL: no backend available\n"); } bool best_is_cpu = (strcmp(ggml_backend_name(bp.backend), "CPU") == 0); int n_threads = backend_cpu_n_threads(); @@ -133,8 +135,14 @@ static BackendPair backend_init(const char * label) { bp.cpu_backend = cpu_backend_new(n_threads); } if (!bp.cpu_backend) { - fprintf(stderr, "[Load] FATAL: failed to init CPU backend\n"); - exit(1); + // Under ACESTEP_FATAL_THROWS this throws, and bp is a local that never + // reaches a module or the backend cache -- so free the GPU backend + // allocated above (the best_is_cpu branch already freed it and left + // bp.backend null) before unwinding, or it leaks. exit(1) does not care. + if (bp.backend) { + ggml_backend_free(bp.backend); + } + ace_fatal(1, "[Load] FATAL: failed to init CPU backend\n"); } bp.has_gpu = !best_is_cpu; fprintf(stderr, "[Load] %s backend: %s (CPU threads: %d)\n", label, ggml_backend_name(bp.backend), n_threads); @@ -146,6 +154,15 @@ static BackendPair backend_init(const char * label) { // Release a backend reference. Frees GPU + CPU backends when refcount hits 0. static void backend_release(ggml_backend_t backend, ggml_backend_t cpu_backend) { + // A caller with no backend (m->backend == null) never took a ref -- e.g. a + // load that threw before backend_init under ACESTEP_FATAL_THROWS, whose + // del_* still runs here. Every caller passes m->backend, so null means "no + // ref"; decrementing would corrupt the shared count and could free a backend + // another live module still holds. vae/vae-enc open the GGUF before + // backend_init, so they are the ones that reach here with a null backend. + if (!backend) { + return; + } if (g_backend_refs <= 0) { return; } @@ -180,8 +197,7 @@ static ggml_backend_sched_t backend_sched_new(BackendPair bp, int max_nodes) { ggml_backend_sched_t sched = ggml_backend_sched_new(backends, bufts, n, max_nodes, false, true); if (!sched) { - fprintf(stderr, "[Load] FATAL: failed to create scheduler\n"); - exit(1); + ace_fatal(1, "[Load] FATAL: failed to create scheduler\n"); } return sched; } diff --git a/src/cond-enc.h b/src/cond-enc.h index 8fd28f7d..078d111a 100644 --- a/src/cond-enc.h +++ b/src/cond-enc.h @@ -12,6 +12,7 @@ // Pack: cat(lyric, timbre[0:1], text_proj) -> [2048, S_total] #pragma once +#include "ace-fatal.h" #include "qwen3-enc.h" // Lyric/Timbre encoder configs @@ -100,6 +101,7 @@ static bool cond_ggml_load(CondGGML * m, const char * gguf_path) { fprintf(stderr, "[Load] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); // XL models have encoder_hidden_size in GGUF metadata (2B models omit it). // When present, the timbre encoder prepends a learned CLS token. @@ -288,8 +290,7 @@ static void cond_ggml_forward(CondGGML * m, // Allocate and set inputs if (!ggml_backend_sched_alloc_graph(m->sched, gf)) { - fprintf(stderr, "[CondEncoder] FATAL: failed to allocate graph\n"); - exit(1); + ace_fatal(1, "[CondEncoder] FATAL: failed to allocate graph\n"); } ggml_backend_tensor_set(t_lyric_in, lyric_embed, 0, 1024 * S_lyric * sizeof(float)); diff --git a/src/dit.h b/src/dit.h index 54f7f8f3..32bccd70 100644 --- a/src/dit.h +++ b/src/dit.h @@ -8,6 +8,7 @@ // ggml ops used: rms_norm, mul_mat, rope_ext, flash_attn_ext, swiglu_split, // conv_transpose_1d, add, mul, scale, view, reshape, permute. +#include "ace-fatal.h" #include "adapter-merge.h" #include "backend.h" #include "ggml-backend.h" @@ -147,13 +148,11 @@ static struct ggml_tensor * dit_load_proj_in_w(WeightCtx * wctx, int P) { int64_t idx = gguf_find_tensor(gf.gguf, name.c_str()); if (idx < 0) { - fprintf(stderr, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); } struct ggml_tensor * src = ggml_get_tensor(gf.meta, name.c_str()); if (!src) { - fprintf(stderr, "[GGUF] FATAL: meta tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: meta tensor '%s' not found\n", name.c_str()); } size_t offset = gguf_get_tensor_offset(gf.gguf, idx); const void * raw = gf.mapping + gf.data_offset + offset; @@ -186,8 +185,8 @@ static struct ggml_tensor * dit_load_proj_in_w(WeightCtx * wctx, const float * s = (const float *) raw; cvt([&](int i) { return s[i]; }); } else { - fprintf(stderr, "[GGUF] FATAL: unsupported type %d for '%s' in proj_in pre-permute\n", src->type, name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: unsupported type %d for '%s' in proj_in pre-permute\n", src->type, + name.c_str()); } wctx->pending.push_back({ dst, data, n * sizeof(float), 0 }); wctx->staging.push_back(std::move(buf)); @@ -204,13 +203,11 @@ static struct ggml_tensor * dit_load_proj_out_w(WeightCtx * wctx, int P) { int64_t idx = gguf_find_tensor(gf.gguf, name.c_str()); if (idx < 0) { - fprintf(stderr, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); } struct ggml_tensor * src = ggml_get_tensor(gf.meta, name.c_str()); if (!src) { - fprintf(stderr, "[GGUF] FATAL: meta tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: meta tensor '%s' not found\n", name.c_str()); } size_t offset = gguf_get_tensor_offset(gf.gguf, idx); const void * raw = gf.mapping + gf.data_offset + offset; @@ -243,9 +240,8 @@ static struct ggml_tensor * dit_load_proj_out_w(WeightCtx * wctx, const float * s = (const float *) raw; cvt([&](int i) { return s[i]; }); } else { - fprintf(stderr, "[GGUF] FATAL: unsupported type %d for '%s' in proj_out pre-permute\n", src->type, - name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: unsupported type %d for '%s' in proj_out pre-permute\n", src->type, + name.c_str()); } wctx->pending.push_back({ dst, data, n * sizeof(float), 0 }); wctx->staging.push_back(std::move(buf)); @@ -270,6 +266,7 @@ static bool dit_ggml_load(DiTGGML * m, fprintf(stderr, "[Load] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); // config from GGUF metadata (all keys required) DiTGGMLConfig & cfg = m->cfg; diff --git a/src/fsq-detok.h b/src/fsq-detok.h index b3b5b404..1712185e 100644 --- a/src/fsq-detok.h +++ b/src/fsq-detok.h @@ -79,6 +79,7 @@ static bool detok_ggml_load(DetokGGML * m, const char * gguf_path) { fprintf(stderr, "[Load] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); // FSQ(2) + embed(2) + special(1) + 2 layers x 11(22) + norm(1) + proj(2) = 30 wctx_init(&m->wctx, 30); diff --git a/src/fsq-tok.h b/src/fsq-tok.h index 8f6e6161..c818577b 100644 --- a/src/fsq-tok.h +++ b/src/fsq-tok.h @@ -85,6 +85,7 @@ static bool tok_ggml_load(TokGGML * m, const char * gguf_path) { fprintf(stderr, "[Tok] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); // proj(2) + embed(2) + special(1) + 2 layers x 11(22) + norm(1) + fsq_in(2) = 30 wctx_init(&m->wctx, 30); diff --git a/src/gguf-weights.h b/src/gguf-weights.h index 6adc2b04..920f01f7 100644 --- a/src/gguf-weights.h +++ b/src/gguf-weights.h @@ -13,6 +13,7 @@ // wctx_alloc(&wctx, backend); // gf_close(&gf); // safe after wctx_alloc copied data to GPU +#include "ace-fatal.h" #include "gguf.h" #include "weight-ctx.h" @@ -30,16 +31,19 @@ #endif struct GGUFModel { - struct gguf_context * gguf; // parsed header (KV + tensor metadata) - struct ggml_context * meta; // tensor descriptors (no data) - uint8_t * mapping; // mmapped file - size_t file_size; - size_t data_offset; // gguf_get_data_offset(gguf) + struct gguf_context * gguf = nullptr; // parsed header (KV + tensor metadata) + struct ggml_context * meta = nullptr; // tensor descriptors (no data) + uint8_t * mapping = nullptr; // mmapped file + size_t file_size = 0; + size_t data_offset = 0; // gguf_get_data_offset(gguf) #ifdef _WIN32 - HANDLE fh; - HANDLE mh; + HANDLE fh = nullptr; + HANDLE mh = nullptr; #else - int fd; + // -1, not 0, is "no fd": 0 is a legitimate descriptor (open() returns it when + // stdin is closed), so gf_close must be able to close 0 yet skip the unset + // value. `= {}` and the *gf = {} in gf_close/gf_load both reset fd to -1. + int fd = -1; #endif }; @@ -64,6 +68,9 @@ static void gf_close(GGUFModel * gf) { if (gf->mapping) { munmap(gf->mapping, gf->file_size); } + // fd defaults to -1 (see GGUFModel), so >= 0 closes a real descriptor -- + // including a legitimate fd 0 -- while the -1 sentinel makes this safe to + // call twice, which GgufCloser below relies on. if (gf->fd >= 0) { close(gf->fd); } @@ -71,6 +78,22 @@ static void gf_close(GGUFModel * gf) { *gf = {}; } +// Closes an open GGUFModel when the scope unwinds -- by a return, or (under +// ACESTEP_FATAL_THROWS) by a gf_load_tensor throwing mid-load. Each loader opens +// a local gf, runs throwing tensor reads, and gf_close()s at the end; without +// this a thrown fatal skips that close and leaks the whole mmap + fd + gguf +// context on every failed load. gf_close is idempotent (above), so the loader's +// own end-of-function gf_close stays and this is just a no-op safety net after +// it. Construct it only once gf is open (after gf_load succeeds), never over an +// uninitialised gf. +struct GgufCloser { + GGUFModel * gf; + explicit GgufCloser(GGUFModel * g) : gf(g) {} + ~GgufCloser() { gf_close(gf); } + GgufCloser(const GgufCloser &) = delete; + GgufCloser & operator=(const GgufCloser &) = delete; +}; + static bool gf_load(GGUFModel * gf, const char * path) { *gf = {}; @@ -163,15 +186,13 @@ static struct ggml_tensor * gf_load_tensor(WeightCtx * wctx, int n_dims_override = 0) { int64_t idx = gguf_find_tensor(gf.gguf, name.c_str()); if (idx < 0) { - fprintf(stderr, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); } // Get metadata from the context populated by gguf_init_from_file struct ggml_tensor * src = ggml_get_tensor(gf.meta, name.c_str()); if (!src) { - fprintf(stderr, "[GGUF] FATAL: tensor '%s' not in meta context\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: tensor '%s' not in meta context\n", name.c_str()); } int n_dims; @@ -214,8 +235,7 @@ static struct ggml_tensor * gf_try_load_tensor(WeightCtx * wctx, const GGUFModel static struct ggml_tensor * gf_load_tensor_f32(WeightCtx * wctx, const GGUFModel & gf, const std::string & name) { int64_t idx = gguf_find_tensor(gf.gguf, name.c_str()); if (idx < 0) { - fprintf(stderr, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: tensor '%s' not found\n", name.c_str()); } struct ggml_tensor * src = ggml_get_tensor(gf.meta, name.c_str()); int n_dims = ggml_n_dims(src); @@ -287,9 +307,8 @@ static struct ggml_tensor * gf_load_qkv_fused(WeightCtx * wctx, struct ggml_tensor * k_src = ggml_get_tensor(gf.meta, k_name.c_str()); struct ggml_tensor * v_src = ggml_get_tensor(gf.meta, v_name.c_str()); if (!q_src || !k_src || !v_src) { - fprintf(stderr, "[GGUF] FATAL: QKV tensor not found: %s / %s / %s\n", q_name.c_str(), k_name.c_str(), - v_name.c_str()); - exit(1); + ace_fatal(1, "[GGUF] FATAL: QKV tensor not found: %s / %s / %s\n", q_name.c_str(), k_name.c_str(), + v_name.c_str()); } // All must share ne[0] (input dim) and type - otherwise can't fuse GGML_ASSERT(q_src->ne[0] == k_src->ne[0] && k_src->ne[0] == v_src->ne[0]); diff --git a/src/model-store.cpp b/src/model-store.cpp index 75fda310..badb458f 100644 --- a/src/model-store.cpp +++ b/src/model-store.cpp @@ -12,6 +12,7 @@ #include "model-store.h" +#include "ace-fatal.h" #include "gguf-weights.h" #include "timer.h" @@ -144,7 +145,16 @@ static T * install_entry(ModelStore * s, e.deleter = deleter; e.label = label; s->gpu.emplace(k, e); - s->handle_to_key.emplace(obj, k); + try { + s->handle_to_key.emplace(obj, k); + } catch (...) { + // Roll back the first insert so install_entry is all-or-nothing: on + // failure obj is in no container, and the caller's still-armed LoadGuard + // frees it exactly once. erase() does not run the deleter, so obj is not + // touched here. + s->gpu.erase(k); + throw; + } return obj; } @@ -273,6 +283,47 @@ static size_t bytes_of_fsq_detok(const DetokGGML * m) { return m && m->wctx.buffer ? ggml_backend_buffer_get_size(m->wctx.buffer) : 0; } +// Owns a raw-new'd module until install_entry adopts it. Every store_require_* +// below does `T * m = new T(); load(m); install_entry(...)`, and each load calls +// backend_init / gf_load_tensor -- which exit(1) by default but throw under +// ACESTEP_FATAL_THROWS (see ace-fatal.h). #17 called out only the two void +// loaders, but a throw escaping any of these functions skips its cleanup just +// the same, so the half-built m -- with its backend, scheduler, ggml_context and +// weight buffer already assigned in -- would leak past the frame with the store's +// mutex released mid-load. LoadGuard frees it on the load-returned-false path, +// the ace_fatal_error throw path (the flag), and a std::bad_alloc from the load's +// own STL allocations (which can happen in any build, so this also plugs a +// pre-existing leak, not only a flag-only one). +// +// It runs the module's del_* function -- the same free the store uses on +// eviction -- NOT a bare `delete`. These structs are POD: `delete m` would run a +// trivial destructor and leak the backend/scheduler/context/buffer, since the +// real teardown lives in qw3lm_free()/vae_enc_free()/etc. The del_* frees safely +// on a partial module: every *_free() null-checks each handle, and backend_init +// either fully took a ref (backend_release returns it) or threw before taking one +// (g_backend_refs is 0, so backend_release early-returns). +// +// dismiss() runs after install_entry, which is all-or-nothing: it rolls back its +// first emplace if the second throws, so on any install failure m is in no +// container and the still-armed guard frees it exactly once -- no leak, no +// dangling entry, no double-free. On success dismiss() hands ownership to the +// store. Below, each site catches ace_fatal_error specifically, not `...`, so a +// bad_alloc is not masked as a benign nullptr but propagates (with m already +// freed here). +template struct LoadGuard { + T * p; + void (*free_fn)(void *); + LoadGuard(T * ptr, void (*fn)(void *)) : p(ptr), free_fn(fn) {} + ~LoadGuard() { + if (p) { + free_fn(p); + } + } + void dismiss() { p = nullptr; } + LoadGuard(const LoadGuard &) = delete; + LoadGuard & operator=(const LoadGuard &) = delete; +}; + Qwen3LM * store_require_lm(ModelStore * s, const ModelKey & k) { std::lock_guard lock(s->mtx); if (auto * hit = cache_hit(s, k)) { @@ -282,12 +333,17 @@ Qwen3LM * store_require_lm(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - Qwen3LM * m = new Qwen3LM(); - if (!qw3lm_load(m, k.path.c_str(), k.max_seq, k.n_kv_sets)) { - delete m; - return nullptr; + Qwen3LM * m = new Qwen3LM(); + LoadGuard guard(m, del_lm); + try { + if (!qw3lm_load(m, k.path.c_str(), k.max_seq, k.n_kv_sets)) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_lm(m), "LM", del_lm); + guard.dismiss(); fprintf(stderr, "[Store] Load LM: %.0f ms\n", t.ms()); return m; } @@ -301,12 +357,17 @@ Qwen3GGML * store_require_text_enc(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - Qwen3GGML * m = new Qwen3GGML(); - if (!qwen3_load_text_encoder(m, k.path.c_str())) { - delete m; - return nullptr; + Qwen3GGML * m = new Qwen3GGML(); + LoadGuard guard(m, del_text_enc); + try { + if (!qwen3_load_text_encoder(m, k.path.c_str())) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_text_enc(m), "TextEnc", del_text_enc); + guard.dismiss(); fprintf(stderr, "[Store] Load TextEnc: %.0f ms\n", t.ms()); return m; } @@ -320,12 +381,17 @@ CondGGML * store_require_cond_enc(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - CondGGML * m = new CondGGML(); - if (!cond_ggml_load(m, k.path.c_str())) { - delete m; - return nullptr; + CondGGML * m = new CondGGML(); + LoadGuard guard(m, del_cond_enc); + try { + if (!cond_ggml_load(m, k.path.c_str())) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_cond_enc(m), "CondEnc", del_cond_enc); + guard.dismiss(); fprintf(stderr, "[Store] Load CondEnc: %.0f ms\n", t.ms()); return m; } @@ -339,13 +405,18 @@ DiTGGML * store_require_dit(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - DiTGGML * m = new DiTGGML(); - const char * adapter = k.adapter_path.empty() ? nullptr : k.adapter_path.c_str(); - if (!dit_ggml_load(m, k.path.c_str(), adapter, k.adapter_scale)) { - delete m; - return nullptr; + DiTGGML * m = new DiTGGML(); + LoadGuard guard(m, del_dit); + const char * adapter = k.adapter_path.empty() ? nullptr : k.adapter_path.c_str(); + try { + if (!dit_ggml_load(m, k.path.c_str(), adapter, k.adapter_scale)) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_dit(m), "DiT", del_dit); + guard.dismiss(); fprintf(stderr, "[Store] Load DiT: %.0f ms\n", t.ms()); return m; } @@ -359,9 +430,15 @@ VAEEncoder * store_require_vae_enc(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - VAEEncoder * m = new VAEEncoder(); - vae_enc_load(m, k.path.c_str()); // exit(1) on failure, returns void + VAEEncoder * m = new VAEEncoder(); + LoadGuard guard(m, del_vae_enc); + try { + vae_enc_load(m, k.path.c_str()); // void: exit(1) by default, throws under the flag + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr + } install_entry(s, k, m, bytes_of_vae_enc(m), "VAE-Enc", del_vae_enc); + guard.dismiss(); fprintf(stderr, "[Store] Load VAE-Enc: %.0f ms\n", t.ms()); return m; } @@ -375,9 +452,15 @@ VAEGGML * store_require_vae_dec(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - VAEGGML * m = new VAEGGML(); - vae_ggml_load(m, k.path.c_str()); // exit(1) on failure, returns void + VAEGGML * m = new VAEGGML(); + LoadGuard guard(m, del_vae_dec); + try { + vae_ggml_load(m, k.path.c_str()); // void: exit(1) by default, throws under the flag + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr + } install_entry(s, k, m, bytes_of_vae_dec(m), "VAE-Dec", del_vae_dec); + guard.dismiss(); fprintf(stderr, "[Store] Load VAE-Dec: %.0f ms\n", t.ms()); return m; } @@ -391,12 +474,17 @@ TokGGML * store_require_fsq_tok(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - TokGGML * m = new TokGGML(); - if (!tok_ggml_load(m, k.path.c_str())) { - delete m; - return nullptr; + TokGGML * m = new TokGGML(); + LoadGuard guard(m, del_fsq_tok); + try { + if (!tok_ggml_load(m, k.path.c_str())) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_fsq_tok(m), "FSQ-Tok", del_fsq_tok); + guard.dismiss(); fprintf(stderr, "[Store] Load FSQ-Tok: %.0f ms\n", t.ms()); return m; } @@ -410,12 +498,17 @@ DetokGGML * store_require_fsq_detok(ModelStore * s, const ModelKey & k) { evict_all_except(s, k); } Timer t; - DetokGGML * m = new DetokGGML(); - if (!detok_ggml_load(m, k.path.c_str())) { - delete m; - return nullptr; + DetokGGML * m = new DetokGGML(); + LoadGuard guard(m, del_fsq_detok); + try { + if (!detok_ggml_load(m, k.path.c_str())) { + return nullptr; + } + } catch (const ace_fatal_error &) { + return nullptr; // a failed load is the store's nullptr; reason is on stderr } install_entry(s, k, m, bytes_of_fsq_detok(m), "FSQ-Detok", del_fsq_detok); + guard.dismiss(); fprintf(stderr, "[Store] Load FSQ-Detok: %.0f ms\n", t.ms()); return m; } diff --git a/src/qwen3-enc.h b/src/qwen3-enc.h index 6fd24633..d85079ef 100644 --- a/src/qwen3-enc.h +++ b/src/qwen3-enc.h @@ -11,6 +11,7 @@ // Final: RMSNorm #pragma once +#include "ace-fatal.h" #include "backend.h" #include "ggml-backend.h" #include "ggml.h" @@ -341,6 +342,7 @@ static bool qwen3_load_text_encoder(Qwen3GGML * m, const char * gguf_path) { fprintf(stderr, "[Load] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); // embed(1) + 28 layers * 11 weights + final_norm(1) = 310 int n_tensors = 1 + m->cfg.n_layers * 11 + 1; @@ -411,8 +413,7 @@ static void qwen3_forward(Qwen3GGML * m, const int * token_ids, int S, float * o // Allocate if (!ggml_backend_sched_alloc_graph(m->sched, gf)) { - fprintf(stderr, "[TextEncoder] FATAL: failed to allocate graph (%d tokens)\n", S); - exit(1); + ace_fatal(1, "[TextEncoder] FATAL: failed to allocate graph (%d tokens)\n", S); } // Set inputs @@ -468,8 +469,7 @@ static void qwen3_embed_lookup(Qwen3GGML * m, const int * token_ids, int S, floa ggml_build_forward_expand(gf, out); if (!ggml_backend_sched_alloc_graph(m->sched, gf)) { - fprintf(stderr, "[TextEncoder] FATAL: failed to allocate graph (embed lookup, %d tokens)\n", S); - exit(1); + ace_fatal(1, "[TextEncoder] FATAL: failed to allocate graph (embed lookup, %d tokens)\n", S); } ggml_backend_tensor_set(t_ids, token_ids, 0, S * sizeof(int)); ggml_backend_sched_graph_compute(m->sched, gf); diff --git a/src/qwen3-lm.h b/src/qwen3-lm.h index feec6514..118356f5 100644 --- a/src/qwen3-lm.h +++ b/src/qwen3-lm.h @@ -3,6 +3,7 @@ // Loads from GGUF, supports prefill + decode, tied lm_head #pragma once +#include "ace-fatal.h" #include "graph-arena.h" #include "qwen3-enc.h" // Qwen3Layer, Qwen3Config, layer build helpers #include "static-graph.h" @@ -248,8 +249,7 @@ static void qw3lm_alloc_kv_cache(Qwen3LM * m, int n_sets) { m->kv_buf = ggml_backend_alloc_ctx_tensors(m->kv_ctx, m->backend); if (!m->kv_buf) { - fprintf(stderr, "[LM-KV] FATAL: failed to allocate KV cache\n"); - exit(1); + ace_fatal(1, "[LM-KV] FATAL: failed to allocate KV cache\n"); } // Zero the buffer once: the attention window is padded past kv_pos @@ -290,6 +290,7 @@ static bool qw3lm_load(Qwen3LM * m, const char * gguf_path, int max_seq_len, int fprintf(stderr, "[LM-Load] FATAL: cannot load %s\n", gguf_path); return false; } + GgufCloser gfc(&gf); m->cfg = qw3lm_load_config(gf); if (max_seq_len > 0) { @@ -573,8 +574,7 @@ static void qw3lm_forward(Qwen3LM * m, const int * token_ids, int n_tokens, int // Schedule + allocate ggml_backend_sched_reset(m->sched); if (!ggml_backend_sched_alloc_graph(m->sched, gf)) { - fprintf(stderr, "[LM] FATAL: failed to allocate graph (prefill, %d tokens)\n", n_tokens); - exit(1); + ace_fatal(1, "[LM] FATAL: failed to allocate graph (prefill, %d tokens)\n", n_tokens); } // Set token IDs @@ -646,8 +646,7 @@ static void qw3lm_forward_batch(Qwen3LM * m, max_kv_len = kl; } if (kl > c.max_seq_len) { - fprintf(stderr, "[LM-Batch] FATAL: kv_len %d > max_seq %d (set %d)\n", kl, c.max_seq_len, kv_sets[i]); - exit(1); + ace_fatal(1, "[LM-Batch] FATAL: kv_len %d > max_seq %d (set %d)\n", kl, c.max_seq_len, kv_sets[i]); } } @@ -829,8 +828,7 @@ static void qw3lm_forward_batch(Qwen3LM * m, ggml_build_forward_expand(gf, lgt); if (!static_graph_alloc(&m->batch_graph.graph, m->backend, m->sched, gf)) { - fprintf(stderr, "[LM] FATAL: failed to allocate graph (batch decode, N=%d)\n", N); - exit(1); + ace_fatal(1, "[LM] FATAL: failed to allocate graph (batch decode, N=%d)\n", N); } m->batch_graph.gf = gf; diff --git a/src/vae-enc.h b/src/vae-enc.h index c6aca774..1e432f70 100644 --- a/src/vae-enc.h +++ b/src/vae-enc.h @@ -6,6 +6,7 @@ // Downsample: 2x4x4x6x10 = 1920x (matches decoder upsample) #pragma once +#include "ace-fatal.h" #include "vae.h" // Encoder block: 3xResUnit(in_ch) -> snake(in_ch) -> strided Conv1d(in_ch -> out_ch) @@ -44,9 +45,9 @@ struct VAEEncoder { static void vae_enc_load(VAEEncoder * m, const char * path) { GGUFModel gf = {}; if (!gf_load(&gf, path)) { - fprintf(stderr, "[VAE-Enc] FATAL: cannot load %s\n", path); - exit(1); + ace_fatal(1, "[VAE-Enc] FATAL: cannot load %s\n", path); } + GgufCloser gfc(&gf); // Encoder channel layout (mirror of decoder, bottom-up): // conv1: 2 -> 128 @@ -111,8 +112,7 @@ static void vae_enc_load(VAEEncoder * m, const char * path) { m->sched = backend_sched_new(bp, 8192); m->buf = ggml_backend_alloc_ctx_tensors(ctx, m->backend); if (!m->buf) { - fprintf(stderr, "[VAE-Enc] FATAL: failed to allocate weight buffer\n"); - exit(1); + ace_fatal(1, "[VAE-Enc] FATAL: failed to allocate weight buffer\n"); } fprintf(stderr, "[VAE-Enc] Backend: %s, Weight buffer: %.1f MB\n", ggml_backend_name(m->backend), (float) ggml_backend_buffer_get_size(m->buf) / (1024 * 1024)); diff --git a/src/vae.h b/src/vae.h index 0a85d9fc..abe1e737 100644 --- a/src/vae.h +++ b/src/vae.h @@ -8,6 +8,7 @@ // Upsample: 10x6x4x4x2 = 1920x #pragma once +#include "ace-fatal.h" #include "backend.h" #include "ggml-backend.h" #include "ggml.h" @@ -167,9 +168,9 @@ static void vae_load_bias(struct ggml_tensor * dst, const GGUFModel & gf, const static void vae_ggml_load(VAEGGML * m, const char * path) { GGUFModel gf = {}; if (!gf_load(&gf, path)) { - fprintf(stderr, "[VAE] FATAL: cannot load %s\n", path); - exit(1); + ace_fatal(1, "[VAE] FATAL: cannot load %s\n", path); } + GgufCloser gfc(&gf); static const int strides[] = { 10, 6, 4, 4, 2 }; static const int in_ch[] = { 2048, 1024, 512, 256, 128 }; @@ -220,8 +221,7 @@ static void vae_ggml_load(VAEGGML * m, const char * path) { m->sched = backend_sched_new(bp, 8192); m->buf = ggml_backend_alloc_ctx_tensors(ctx, m->backend); if (!m->buf) { - fprintf(stderr, "[VAE] FATAL: failed to allocate weight buffer\n"); - exit(1); + ace_fatal(1, "[VAE] FATAL: failed to allocate weight buffer\n"); } fprintf(stderr, "[VAE] Backend: %s, Weight buffer: %.1f MB\n", ggml_backend_name(m->backend), (float) ggml_backend_buffer_get_size(m->buf) / (1024 * 1024)); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e940df51..fb8982be 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,3 +1,8 @@ +# test-ace-fatal: the fatal-throws path (ace_fatal / ace_fatal_error, #17). +# Header-only, no engine link: it defines ACESTEP_FATAL_THROWS for itself. +add_executable(test-ace-fatal test-ace-fatal.cpp) +target_include_directories(test-ace-fatal PRIVATE ${CMAKE_SOURCE_DIR}/src) + # test-philox: Philox RNG reference dumper. add_executable(test-philox test-philox.cpp) target_include_directories(test-philox PRIVATE ${CMAKE_SOURCE_DIR}/src) diff --git a/tests/test-ace-fatal.cpp b/tests/test-ace-fatal.cpp new file mode 100644 index 00000000..31e6fada --- /dev/null +++ b/tests/test-ace-fatal.cpp @@ -0,0 +1,95 @@ +// test-ace-fatal: the throw-instead-of-exit path (#17). +// +// Defines ACESTEP_FATAL_THROWS for this TU only, so it exercises the throwing +// build regardless of how the rest of the engine is compiled. Verifies that a +// fatal turns into a catchable ace_fatal_error carrying the formatted message +// and the exit code, that stack unwinding runs destructors (nothing leaks), and +// that a message longer than the on-stack format buffer is reproduced in full. +#define ACESTEP_FATAL_THROWS +#include "ace-fatal.h" + +#include +#include + +static int failures = 0; + +#define CHECK(cond) \ + do { \ + if (!(cond)) { \ + fprintf(stderr, "FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + failures++; \ + } \ + } while (0) + +// Flips a flag in its destructor, so we can prove unwinding ran it. +struct Sentinel { + bool * ran; + explicit Sentinel(bool * r) : ran(r) {} + ~Sentinel() { *ran = true; } +}; + +int main() { + // 1. Message and code ride with the exception. + { + bool caught = false; + try { + ace_fatal(7, "[X] FATAL: tensor '%s' not found (%d)\n", "decoder.proj_out", 42); + } catch (const ace_fatal_error & e) { + caught = true; + CHECK(e.code == 7); + CHECK(std::string(e.what()) == "[X] FATAL: tensor 'decoder.proj_out' not found (42)\n"); + } + CHECK(caught); + } + + // 2. Stack unwinding runs destructors between the throw and the catch, so a + // half-built module's RAII members are released -- "leaks nothing". + { + bool sentinel_ran = false; + bool caught = false; + try { + Sentinel s(&sentinel_ran); + ace_fatal(1, "boom\n"); + CHECK(false); // unreachable: ace_fatal is [[noreturn]] + } catch (const ace_fatal_error &) { + caught = true; + } + CHECK(caught); + CHECK(sentinel_ran); + } + + // 3. A message longer than the 1024-byte stack buffer is reproduced whole + // (exercises the vsnprintf heap-resize branch). + { + std::string big(5000, 'x'); + bool caught = false; + try { + ace_fatal(2, "%s", big.c_str()); + } catch (const ace_fatal_error & e) { + caught = true; + CHECK(std::string(e.what()) == big); + CHECK(e.code == 2); + } + CHECK(caught); + } + + // 4. A generic handler catches it too. (ModelStore's LoadGuard sites catch + // the specific `const ace_fatal_error &`, not `...`, so a std::bad_alloc + // is not masked; this only checks the base-class catch still works.) + { + bool caught = false; + try { + ace_fatal(1, "generic\n"); + } catch (...) { + caught = true; + } + CHECK(caught); + } + + if (failures == 0) { + printf("test-ace-fatal: all checks passed\n"); + return 0; + } + fprintf(stderr, "test-ace-fatal: %d check(s) failed\n", failures); + return 1; +} From 2fc343f75a9a19e05b4bb609ab39e356a73c00cc Mon Sep 17 00:00:00 2001 From: Erich Champion Date: Mon, 7 Sep 2026 12:32:48 -0700 Subject: [PATCH 2/2] fix: RAII guard for ggml contexts in forward paths (#110 review) When ACESTEP_FATAL_THROWS is enabled, ace_fatal throws ace_fatal_error instead of exit(1). The three forward functions allocate a ggml_context with ggml_init and previously freed it with an explicit ggml_free at the end. If ggml_backend_sched_alloc_graph fails, ace_fatal now throws BEFORE that free, leaking the context on each failure. Own the context with a function-local RAII guard so it is released during exception unwinding, preserving the normal cleanup behavior. Addresses review comment on src/cond-enc.h:293, src/qwen3-enc.h:416,472. --- src/cond-enc.h | 3 ++- src/qwen3-enc.h | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/cond-enc.h b/src/cond-enc.h index 078d111a..ca4e7d21 100644 --- a/src/cond-enc.h +++ b/src/cond-enc.h @@ -187,6 +187,7 @@ static void cond_ggml_forward(CondGGML * m, size_t ctx_size = 4096 * ggml_tensor_overhead() + ggml_graph_overhead(); struct ggml_init_params gp = { ctx_size, NULL, true }; struct ggml_context * ctx = ggml_init(gp); + struct GgmlCtxGuard { ggml_context* p; ~GgmlCtxGuard(){ if(p) ggml_free(p); } } _ctx_guard{ctx}; struct ggml_cgraph * gf = ggml_new_graph_custom(ctx, 8192, false); // Positions for lyric (bidirectional, 0..S_lyric-1) @@ -377,7 +378,7 @@ static void cond_ggml_forward(CondGGML * m, S_total); ggml_backend_sched_reset(m->sched); - ggml_free(ctx); + // _ctx_guard frees ctx } // Free diff --git a/src/qwen3-enc.h b/src/qwen3-enc.h index d85079ef..df07f86b 100644 --- a/src/qwen3-enc.h +++ b/src/qwen3-enc.h @@ -380,6 +380,7 @@ static void qwen3_forward(Qwen3GGML * m, const int * token_ids, int S, float * o size_t ctx_size = 2048 * ggml_tensor_overhead() + ggml_graph_overhead(); struct ggml_init_params gp = { ctx_size, NULL, true }; struct ggml_context * ctx = ggml_init(gp); + struct GgmlCtxGuard { ggml_context* p; ~GgmlCtxGuard(){ if(p) ggml_free(p); } } _ctx_guard{ctx}; struct ggml_cgraph * gf = ggml_new_graph_custom(ctx, 4096, false); @@ -445,7 +446,7 @@ static void qwen3_forward(Qwen3GGML * m, const int * token_ids, int S, float * o ggml_backend_tensor_get(out, output, 0, H * S * sizeof(float)); ggml_backend_sched_reset(m->sched); - ggml_free(ctx); + // _ctx_guard frees ctx } // Embedding lookup via ggml graph (reuses text encoder weights + scheduler) @@ -457,6 +458,7 @@ static void qwen3_embed_lookup(Qwen3GGML * m, const int * token_ids, int S, floa size_t ctx_size = 16 * ggml_tensor_overhead() + ggml_graph_overhead(); struct ggml_init_params gp = { ctx_size, NULL, true }; struct ggml_context * ctx = ggml_init(gp); + struct GgmlCtxGuard { ggml_context* p; ~GgmlCtxGuard(){ if(p) ggml_free(p); } } _ctx_guard{ctx}; struct ggml_cgraph * gf = ggml_new_graph(ctx); struct ggml_tensor * t_ids = ggml_new_tensor_1d(ctx, GGML_TYPE_I32, S); @@ -476,7 +478,7 @@ static void qwen3_embed_lookup(Qwen3GGML * m, const int * token_ids, int S, floa ggml_backend_tensor_get(out, output, 0, (size_t) H * S * sizeof(float)); ggml_backend_sched_reset(m->sched); - ggml_free(ctx); + // _ctx_guard frees ctx } // Free