diff --git a/tests/test-chat.cpp b/tests/test-chat.cpp index 201cea95d2c8..957771ee2c36 100644 --- a/tests/test-chat.cpp +++ b/tests/test-chat.cpp @@ -7277,6 +7277,46 @@ static int test_train_control(const char * model_path, const char * output_dir) require(trainer.start(req).at("ok").get(), "baseline refused"); const auto baseline = wait_for(done); + // what this catches (continuum card 48029d24): an exception thrown AFTER the claim. "yield" + // is read with body.value(..., true), so a string there throws json::type_error once the run + // is claimed and prepared. The claim must be released (state "error" under this out, and the + // next start accepted), never left "preparing" with every later start refused as "already in + // progress" until a restart. + { + json bad = req; + bad["out"] = "thrown.gguf"; + bad["yield"] = "not a bool"; + bool threw = false; + try { + trainer.start(bad); + } catch (const std::exception &) { // nlohmann's type_error is a std::exception; `json` here is common_json + threw = true; + } + require(threw, "a non-bool yield must throw after the claim (the case under test)"); + const auto after = trainer.status(); + require(after.value("state", "") == "error" && after.value("out", "") == "thrown.gguf", + "the thrown start leaves its error under its own out, not 'preparing'"); + json next = req; + next["out"] = "after-throw.gguf"; + require(trainer.start(next).at("ok").get(), "the claim was released: the next start is accepted"); + wait_for(done); + + // and the refusal path: an empty "examples" is refused by `refuse` AFTER the claim, which + // must release it the same way (and settle the guard only once released) + json refused = req; + refused.erase("text"); + refused["examples"] = json::array(); + refused["out"] = "refused.gguf"; + require(!trainer.start(refused).at("ok").get(), "an empty examples array is refused"); + const auto after_refusal = trainer.status(); + require(after_refusal.value("state", "") == "error" && after_refusal.value("out", "") == "refused.gguf", + "the refusal leaves its error under its own out"); + json again = req; + again["out"] = "after-refusal.gguf"; + require(trainer.start(again).at("ok").get(), "the refused claim was released: the next start is accepted"); + wait_for(done); + } + // Block after two updates, then exercise manual pause separately from serving admission. calls.store(0); hold_at.store(3); diff --git a/tools/server/server-train.cpp b/tools/server/server-train.cpp index 8dd7f4d87426..b8284ce46b5b 100644 --- a/tools/server/server-train.cpp +++ b/tools/server/server-train.cpp @@ -498,6 +498,11 @@ json server_trainer::start(const json & body_in) { // /train answers "preparing" with this out within milliseconds of the POST arriving, so a // caller reconciles by the run's identity instead of guessing from a clock. A refusal after the // claim releases it and leaves the run's own error, under its out, in the state. + // The state a claim released by an exception leaves behind, built BEFORE the claim (`out` + // is validated above) so releasing it later allocates nothing and cannot throw. + json failed_after_claim = json::object({{"state", "error"}, {"out", body.at("out")}, + {"error", "the start failed after it claimed the run (an exception); the claim is released"}, + {"epochs", json::array()}}); { std::lock_guard claim_lock(mu); bool expected = false; @@ -505,6 +510,33 @@ json server_trainer::start(const json & body_in) { return json::object({{"ok", false}, {"error", "a training run is already in progress"}}); } } + // THE CLAIM IS RELEASED ON EVERY EXIT THAT DOES NOT HAND IT TO THE WORKER (continuum card + // 48029d24). `refuse` releases it on a refusal, but an exception thrown after the claim (a + // json::type_error from a body field, std::system_error from std::thread) used to escape with + // `running` still true: /train named this run "preparing" forever and refused every later + // start as "already in progress" until the server restarted. `settled` is set only AFTER a + // release (by `refuse`) or by the hand-off, so this guard never releases a later run's claim + // and never misses one. Its destructor cannot throw (it runs while unwinding; a throw there + // terminates the server): the state is moved in, not built, recording it is best-effort, and + // `running` is released whatever happens. Declared before `lock` below, so `lock` (on `mu`) is + // gone before this destructor takes `mu`. + struct claim_guard { + server_trainer & trainer; + json failed_state; + bool settled = false; + ~claim_guard() noexcept { + if (settled) { + return; + } + try { + std::lock_guard claim_lock(trainer.mu); + trainer.state = std::move(failed_state); + } catch (...) { + // the diagnostic state is best-effort; releasing the claim is not + } + trainer.running.store(false); + } + } claim{*this, std::move(failed_after_claim)}; if (worker.joinable()) { worker.join(); } @@ -518,6 +550,7 @@ json server_trainer::start(const json & body_in) { state = json::object({{"state", "error"}, {"out", body.at("out")}, {"error", why}, {"epochs", json::array()}}); } running.store(false); + claim.settled = true; // only once released: a throw above leaves the guard to release it return json::object({{"ok", false}, {"error", why}}); }; if (body.contains("examples")) { @@ -615,6 +648,7 @@ json server_trainer::start(const json & body_in) { window_ms_samples.clear(); window_ms_max = 0; worker = std::thread([this, body, ex = std::move(prepared)]() mutable { run(std::move(body), std::move(ex)); }); + claim.settled = true; // handed to the worker, which owns `running` from here lock.unlock(); return json::object({{"ok", true}, {"status", status()}}); }