From 8405067693543f397887cd28527d847f6d28bdd0 Mon Sep 17 00:00:00 2001 From: Jhin Lee Date: Sun, 18 Oct 2026 09:37:52 -0400 Subject: [PATCH 1/2] fix: reject image results after Metal backend failures --- README.md | 11 +++ src/sd_dart_exit.cpp | 38 ++++++++- src/sd_dart_internal.h | 8 ++ src/sd_dart_log.cpp | 20 +++++ src/sd_dart_wrapper.h | 11 ++- tests/native/exit_teardown_runtime_test.cpp | 12 ++- tests/native/exit_teardown_test.cpp | 93 ++++++++++++++++++++- tests/test_exit_teardown_runtime.py | 8 ++ 8 files changed, 193 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 829901a..45169be 100644 --- a/README.md +++ b/README.md @@ -164,6 +164,17 @@ void sd_dart_exit_set_wait_ms(int32_t wait_ms, int32_t work_wait_ms); void sd_dart_exit_teardown(void); ``` +`sd_dart_generate_image()` enables the native log recorder and rejects images +when Metal reports a failed command buffer, including INFO-level diagnostics. +On failure it frees any returned image batch, sets `*images_out` to `NULL` and +`*num_images_out` to zero, and exposes the reason through +`sd_dart_last_error()`. A tracked context with a backend failure rejects later +generations until it is freed with `sd_dart_exit_free()` and replaced by a new +context. Do not replace upstream's log callback after enabling the recorder. +Because upstream diagnostics have no context identity, overlapping generations +conservatively fail together when a Metal failure is reported. This reports +failed GPU work; it does not prevent an operating-system GPU watchdog reset. + ggml-metal aborts in its static destructor (`GGML_ASSERT([rsets->data count] == 0)`) when a process exits while a Metal buffer is still allocated. A caller in a managed runtime cannot always free diff --git a/src/sd_dart_exit.cpp b/src/sd_dart_exit.cpp index 0d559ed..5e83756 100644 --- a/src/sd_dart_exit.cpp +++ b/src/sd_dart_exit.cpp @@ -25,6 +25,7 @@ struct Entry { void (*cancel_fn)(void*) = nullptr; int32_t stage = 0; uint64_t order = 0; + bool backend_failed = false; }; // A thread that just left a call in flight usually still makes short calls on @@ -421,8 +422,43 @@ bool sd_dart_generate_image(sd_ctx_t* sd_ctx, sd_image_t** images_out, int* num_images_out) { Call call(kWorkCall); + sd_dart_log_enable(); ErrorWindow errors; - return generate_image(sd_ctx, sd_img_gen_params, images_out, num_images_out); + if (images_out != nullptr) *images_out = nullptr; + if (num_images_out != nullptr) *num_images_out = 0; + Registry& registry = state(); + { + std::lock_guard lock(registry.mutex); + const auto found = registry.objects.find(sd_ctx); + if (found != registry.objects.end() && found->second.backend_failed) { + sd_dart_log_generation_failed(); + return false; + } + } + const uint64_t before = sd_dart_log_backend_failures(); + // Upstream permits a null count output. Still collect the count so that + // rejecting a returned batch frees every image's pixel allocation. + int local_count = 0; + int* count_out = num_images_out != nullptr ? num_images_out : &local_count; + const bool ok = generate_image(sd_ctx, sd_img_gen_params, images_out, count_out); + const bool backend_failed = sd_dart_log_backend_failures() != before; + if (backend_failed) { + { + std::lock_guard lock(registry.mutex); + const auto found = registry.objects.find(sd_ctx); + if (found != registry.objects.end()) found->second.backend_failed = true; + } + sd_dart_log_generation_failed(); + } + if (!ok || backend_failed) { + if (images_out != nullptr && *images_out != nullptr) { + free_sd_images(*images_out, *count_out); + *images_out = nullptr; + } + if (num_images_out != nullptr) *num_images_out = 0; + return false; + } + return true; } void sd_dart_cancel_generation(sd_ctx_t* sd_ctx, enum sd_cancel_mode_t mode) { diff --git a/src/sd_dart_internal.h b/src/sd_dart_internal.h index acb6467..a07f73d 100644 --- a/src/sd_dart_internal.h +++ b/src/sd_dart_internal.h @@ -1,6 +1,8 @@ #ifndef SD_DART_INTERNAL_H #define SD_DART_INTERNAL_H +#include + // What the wrapper's sources share with each other. Not shipped and not // exported. @@ -35,4 +37,10 @@ class SdDartStaticsCall { void sd_dart_log_call_begin(); void sd_dart_log_call_end(); +// Monotonic backend-failure count, independent of log level, ring eviction, +// and dropped messages. Upstream callbacks have no context identity, so a +// failure conservatively invalidates every generation overlapping it. +uint64_t sd_dart_log_backend_failures(); +void sd_dart_log_generation_failed(); + #endif // SD_DART_INTERNAL_H diff --git a/src/sd_dart_log.cpp b/src/sd_dart_log.cpp index 5951505..15a3a59 100644 --- a/src/sd_dart_log.cpp +++ b/src/sd_dart_log.cpp @@ -57,6 +57,7 @@ struct Log { // The number of the newest error message. Message n is in // errors[n % kErrors] until message n + kErrors replaces it. std::atomic errors_recorded; + std::atomic backend_failures; // The errors have a flag of their own, so that the reason of a failed // call never waits behind the other messages or their reader. @@ -199,6 +200,17 @@ void record(int32_t level, const char* origin, const char* text) { if (text == nullptr) { return; } + // Metal also reports failed command buffers at INFO during graph capture. + // Recognize the pinned backend's diagnostics before filtering or waiting + // for either recorder flag. A lost diagnostic must never permit success. + const bool backend_failed = std::strstr(text, "ggml_metal_") != nullptr && + ((std::strstr(text, "command buffer") != nullptr && + std::strstr(text, "failed with status") != nullptr) || + std::strstr(text, "backend is in error state") != nullptr); + if (backend_failed) { + state.backend_failures.fetch_add(1); + level = SD_LOG_ERROR; + } const bool wanted = level >= SD_LOG_INFO + state.level_above_info.load(std::memory_order_relaxed); const bool is_error = level >= SD_LOG_ERROR; if (!wanted && !is_error) { @@ -272,6 +284,14 @@ void sd_dart_log_call_end() { window_end = state.errors_recorded.load(); } +uint64_t sd_dart_log_backend_failures() { + return state.backend_failures.load(); +} + +void sd_dart_log_generation_failed() { + record(SD_LOG_ERROR, "", "Metal backend failed; discard the image result and recreate the context before generating again."); +} + void sd_dart_log_enable(void) { // Upstream and ggml keep their callbacks in unsynchronized globals, so // they are written once rather than on every call. diff --git a/src/sd_dart_wrapper.h b/src/sd_dart_wrapper.h index 9c0b669..0cf1d61 100644 --- a/src/sd_dart_wrapper.h +++ b/src/sd_dart_wrapper.h @@ -212,8 +212,15 @@ SD_API void sd_dart_exit_teardown(void); // teardown. SD_API sd_ctx_t* sd_dart_new_sd_ctx(const sd_ctx_params_t* sd_ctx_params); -// generate_image() as a call in flight. Takes and returns what generate_image() -// does. Teardown cancels it with SD_CANCEL_ALL, which stable-diffusion.cpp +// generate_image() as a call in flight. Enables native log recording and +// returns false if Metal reports a failed command buffer, even if upstream +// returns success. On failure, frees the returned batch and clears both +// outputs. A tracked context with a backend failure rejects later generations: +// free it with sd_dart_exit_free() and load a new context. Concurrent calls +// conservatively fail together when a backend failure overlaps them, since +// upstream's log callback does not identify a context. Do not replace that +// callback after enabling the wrapper recorder. +// Teardown cancels it with SD_CANCEL_ALL, which stable-diffusion.cpp // honors before a sampling step and before the decode of each image, and // waits for it with the longer bound. Blocks after teardown. SD_API bool sd_dart_generate_image(sd_ctx_t* sd_ctx, diff --git a/tests/native/exit_teardown_runtime_test.cpp b/tests/native/exit_teardown_runtime_test.cpp index 93b5f46..fb369c5 100644 --- a/tests/native/exit_teardown_runtime_test.cpp +++ b/tests/native/exit_teardown_runtime_test.cpp @@ -425,10 +425,11 @@ int test_idle(const char* model, const char* backend, bool tracked) { return tracked ? kNotChecked : 0; } -int test_generate(const char* model, const char* backend) { - sd_ctx_t* context = load(model, backend, false); - CHECK(generate(context, 1, false) == 1); - free_sd_ctx(context); +int test_generate(const char* model, const char* backend, bool guarded = false) { + sd_ctx_t* context = load(model, backend, guarded); + CHECK(generate(context, 1, guarded) == 1); + if (guarded) sd_dart_exit_free(context); + else free_sd_ctx(context); return 0; } @@ -853,6 +854,9 @@ int main(int argc, char** argv) { if (scenario == "generate") { return test_generate(model, backend); } + if (scenario == "generate-guarded") { + return test_generate(model, backend, true); + } if (scenario == "idle") { return test_idle(model, backend, true); } diff --git a/tests/native/exit_teardown_test.cpp b/tests/native/exit_teardown_test.cpp index 24d819e..d91a1d0 100644 --- a/tests/native/exit_teardown_test.cpp +++ b/tests/native/exit_teardown_test.cpp @@ -195,6 +195,8 @@ std::string last_error() { std::atomic fail_load{false}; std::atomic hold_load{nullptr}; std::atomic run_generation{nullptr}; +bool return_images = false; +int image_batches_freed = 0; std::atomic hold_free{nullptr}; std::atomic contexts_freed{0}; std::atomic last_freed{nullptr}; @@ -1240,6 +1242,81 @@ int test_device_query_idle() { // A call that fails leaves the reason upstream logged for it with the thread // that made the call, once the recorder is enabled. +int test_generation_backend_failure() { + sd_ctx_params_t params{}; + sd_img_gen_params_t request{}; + return_images = true; + // The generation wrapper must enable recording itself, even with logs muted. + sd_dart_log_set_level(SD_LOG_ERROR + 1); + for (int level : {SD_LOG_INFO, SD_LOG_ERROR}) { + sd_ctx_t* context = sd_dart_new_sd_ctx(¶ms); + sd_image_t* images = nullptr; + int count = 0; + run_generation.store(level == SD_LOG_INFO ? +[](sd_ctx_t*) { + std::thread([] { + upstream_log(SD_LOG_INFO, "ggml - ggml_metal_graph_compute: command buffer 0 failed with status 5"); + }).join(); + return true; + } : +[](sd_ctx_t*) { + upstream_log(SD_LOG_ERROR, "ggml - ggml_metal_synchronize: error: command buffer 0 failed with status 5"); + return true; + }); + const int before = image_batches_freed; + CHECK(!sd_dart_generate_image(context, &request, &images, &count)); + CHECK(images == nullptr && count == 0 && image_batches_freed == before + 1); + CHECK(last_error().find("command buffer 0 failed") != std::string::npos); + // The context is unusable until disposal/reload; do not call upstream again. + run_generation.store(+[](sd_ctx_t*) -> bool { CHECK(false); return true; }); + CHECK(!sd_dart_generate_image(context, &request, &images, &count)); + CHECK(images == nullptr && count == 0); + CHECK(last_error().find("recreate") != std::string::npos); + sd_dart_exit_free(context); + } + // A fresh context and ordinary CPU success are unaffected by old failures, + // ordinary errors, warnings, and cancellation messages. + sd_ctx_t* context = sd_dart_new_sd_ctx(¶ms); + run_generation.store(+[](sd_ctx_t*) { + upstream_log(SD_LOG_WARN, "low memory"); + upstream_log(SD_LOG_INFO, "ggml_metal_graph_compute: command buffer 0 aborted"); + upstream_log(SD_LOG_ERROR, "an unrelated optional operation failed"); + return true; + }); + sd_image_t* images = nullptr; + int count = 0; + CHECK(sd_dart_generate_image(context, &request, &images, &count)); + CHECK(images != nullptr && count == 2); + free_sd_images(images, count); + // Upstream failure must also discard any partially returned batch. + run_generation.store(+[](sd_ctx_t*) { return false; }); + CHECK(!sd_dart_generate_image(context, &request, &images, &count)); + CHECK(images == nullptr && count == 0); + run_generation.store(nullptr); + CHECK(sd_dart_generate_image(context, &request, &images, &count)); + CHECK(last_error().empty()); + free_sd_images(images, count); + // The already-poisoned backend diagnostic is also a failure signal. + run_generation.store(+[](sd_ctx_t*) { + upstream_log(SD_LOG_ERROR, "ggml_metal_graph_compute: backend is in error state from a previous command buffer failure - recreate the backend to recover"); + return true; + }); + CHECK(!sd_dart_generate_image(context, &request, &images, &count)); + CHECK(images == nullptr && count == 0); + sd_dart_exit_free(context); + context = sd_dart_new_sd_ctx(¶ms); + run_generation.store(+[](sd_ctx_t*) { + upstream_log(SD_LOG_INFO, "ggml_metal_graph_compute: command buffer 0 failed with status 5"); + for (int i = 0; i < 64; ++i) upstream_log(SD_LOG_ERROR, "later error"); + return true; + }); + const int before_null_count = image_batches_freed; + CHECK(!sd_dart_generate_image(context, &request, &images, nullptr)); + CHECK(images == nullptr && image_batches_freed == before_null_count + 1); + CHECK(last_error().find("recreate") != std::string::npos); + sd_dart_exit_free(context); + CHECK(sd_dart_exit_tracked_count() == 0); + return 0; +} + int test_last_error() { sd_ctx_params_t params{}; sd_img_gen_params_t request{}; @@ -1750,7 +1827,13 @@ bool generate_image(sd_ctx_t* sd_ctx, upstream_log(SD_LOG_ERROR, "diffusion_engine.cpp:3 - generate_image failed"); } if (num_images_out != nullptr) { - *num_images_out = 7; + *num_images_out = return_images ? 2 : 7; + } + if (return_images && images_out != nullptr) { + *images_out = static_cast(calloc(2, sizeof(sd_image_t))); + for (int i = 0; i < 2; ++i) { + (*images_out)[i].data = static_cast(malloc(16)); + } } sd_ctx->generating.store(false); return ok; @@ -1761,6 +1844,13 @@ void sd_cancel_generation(sd_ctx_t* sd_ctx, enum sd_cancel_mode_t mode) { sd_ctx->cancel_requests.fetch_add(1); } +void free_sd_images(sd_image_t* images, int count) { + CHECK(images != nullptr && count == 2); + for (int i = 0; i < count; ++i) free(images[i].data); + free(images); + ++image_batches_freed; +} + int main(int argc, char** argv) { const std::string scenario = argc > 1 ? argv[1] : ""; const struct { @@ -1799,6 +1889,7 @@ int main(int argc, char** argv) { {"device-query-timeout", test_device_query_timeout}, {"device-query-idle", test_device_query_idle}, {"last-error", test_last_error}, + {"generation-backend-failure", test_generation_backend_failure}, {"log-at-exit", test_log_at_exit}, #if defined(__APPLE__) {"exit-in-flight", test_exit_in_flight}, diff --git a/tests/test_exit_teardown_runtime.py b/tests/test_exit_teardown_runtime.py index b920ecf..0b07d48 100644 --- a/tests/test_exit_teardown_runtime.py +++ b/tests/test_exit_teardown_runtime.py @@ -254,6 +254,14 @@ def run_scenario(self, scenario: str, backend: str, model: Path | str | None = N timeout=600, errors="replace", env={**os.environ, **SCENARIO_ENV, **load_only}) + def test_guarded_generation_succeeds_on_healthy_backends(self) -> None: + for backend in BACKENDS: + if backend in self.load_only: + continue + with self.subTest(backend=backend): + result = self.run_scenario("generate-guarded", backend) + self.assertEqual(0, result.returncode, result.stdout + result.stderr) + def check_scenarios(self, model: Path | str | None = None, size: str | None = None) -> None: for scenario in SCENARIOS: for backend in BACKENDS: From 1bb395f7540cc35570c307a30f354f267de43a6d Mon Sep 17 00:00:00 2001 From: Jhin Lee Date: Sun, 18 Oct 2026 09:49:02 -0400 Subject: [PATCH 2/2] refactor: release context registry before failure logging --- src/sd_dart_exit.cpp | 12 ++++++++---- tests/native/exit_teardown_test.cpp | 6 ++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/sd_dart_exit.cpp b/src/sd_dart_exit.cpp index 5e83756..55e71be 100644 --- a/src/sd_dart_exit.cpp +++ b/src/sd_dart_exit.cpp @@ -427,13 +427,17 @@ bool sd_dart_generate_image(sd_ctx_t* sd_ctx, if (images_out != nullptr) *images_out = nullptr; if (num_images_out != nullptr) *num_images_out = 0; Registry& registry = state(); + bool context_failed = false; { std::lock_guard lock(registry.mutex); const auto found = registry.objects.find(sd_ctx); - if (found != registry.objects.end() && found->second.backend_failed) { - sd_dart_log_generation_failed(); - return false; - } + context_failed = found != registry.objects.end() && found->second.backend_failed; + } + if (context_failed) { + // The recorder can wait on its own flags. Keep cancellation and + // disposal independent of that wait, as on the failure path below. + sd_dart_log_generation_failed(); + return false; } const uint64_t before = sd_dart_log_backend_failures(); // Upstream permits a null count output. Still collect the count so that diff --git a/tests/native/exit_teardown_test.cpp b/tests/native/exit_teardown_test.cpp index d91a1d0..2f0e5b6 100644 --- a/tests/native/exit_teardown_test.cpp +++ b/tests/native/exit_teardown_test.cpp @@ -1240,8 +1240,8 @@ int test_device_query_idle() { return 0; } -// A call that fails leaves the reason upstream logged for it with the thread -// that made the call, once the recorder is enabled. +// Backend failures invalidate the whole returned batch and its tracked +// context, independently of logging level and retained diagnostic history. int test_generation_backend_failure() { sd_ctx_params_t params{}; sd_img_gen_params_t request{}; @@ -1317,6 +1317,8 @@ int test_generation_backend_failure() { return 0; } +// A call that fails leaves the reason upstream logged for it with the thread +// that made the call, once the recorder is enabled. int test_last_error() { sd_ctx_params_t params{}; sd_img_gen_params_t request{};