From a2342f80df18dcaa5b97d2c80fa7444d2bac0a69 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 09:32:25 +0000 Subject: [PATCH] Stop an ignored-cancel sd-server, guard deletes during diffusion training, repair unusable managed binaries sd-server does not interrupt an in-flight job, so when it ignores a cancel the grace branch abandoned the poll and reported cancellation while the native job kept a core (or the GPU) busy to completion and held the server's job slot. The comment said the caller stops the server, but only unload does that immediately: a superseding load stops it after its multi-gigabyte download, and a load that then fails never gets there. Stop it here, as the deadline branch already does. DELETE /api/models/delete-finetuned checked only the LLM trainer, so it could rmtree the output directory a live diffusion LoRA run was about to write its adapter into. Consult the diffusion training service too, like the dataset mutation and model-load routes. find_sd_*_binary only checks is_file(), so an interrupted extraction (or a prebuilt for the wrong CPU) left a present-but-unrunnable binary the installer never retried: every load probed it, fell back to diffusers, and native inference stayed off until the directory was deleted by hand. Probe it and reinstall, but only for a copy under the installer-owned root -- SD_CLI_PATH, UNSLOTH_SD_CPP_PATH, an in-tree build and anything on PATH are the user's. --- .../backend/core/inference/sd_cpp_backend.py | 38 +++++++++++--- .../backend/core/inference/sd_cpp_engine.py | 31 ++++++++--- .../backend/core/inference/sd_cpp_server.py | 12 ++++- studio/backend/routes/models.py | 14 +++++ .../test_delete_finetuned_diffusion_guard.py | 26 ++++++++++ studio/backend/tests/test_sd_cpp_install.py | 51 +++++++++++++++++++ studio/backend/tests/test_sd_cpp_server.py | 4 ++ 7 files changed, 162 insertions(+), 14 deletions(-) diff --git a/studio/backend/core/inference/sd_cpp_backend.py b/studio/backend/core/inference/sd_cpp_backend.py index 34024cb72e..469e279939 100644 --- a/studio/backend/core/inference/sd_cpp_backend.py +++ b/studio/backend/core/inference/sd_cpp_backend.py @@ -65,6 +65,7 @@ from core.inference.sd_cpp_engine import ( SdCppEngine, find_sd_cpp_binary, find_sd_server_binary, + is_managed_binary, runtime_env, ) from core.inference.sd_cpp_server import SdCppServer @@ -123,6 +124,31 @@ def _server_binary_runnable(binary: str) -> bool: return proc.returncode >= 0 and proc.returncode not in (126, 127) +def _usable_or_discard_managed(binary: str) -> bool: + """True if ``binary`` can be kept; False if it is an unusable copy WE own (now removed). + + ``find_sd_*_binary`` only checks that the path is a file, so an interrupted extraction (or a + prebuilt for the wrong CPU) left a present-but-unrunnable binary that the installer then never + retried: every load probed it, rejected it, and fell back to diffusers, so native inference + stayed off until the user deleted the directory by hand. Probing here closes that loop. + + Only a copy under the installer-owned root is removed. SD_CLI_PATH, UNSLOTH_SD_CPP_PATH, an + in-tree build and anything on PATH are the user's, so an unrunnable one of those is reported + as-is (the router still rejects it) rather than deleted or reinstalled over.""" + if _server_binary_runnable(binary): + return True + if not is_managed_binary(binary): + logger.warning("sd.cpp binary %s is not runnable; leaving the user-supplied copy alone", binary) + return True # not ours to replace; the router's own probe still refuses it + logger.warning("managed sd.cpp binary %s is not runnable; removing it so it reinstalls", binary) + try: + Path(binary).unlink() + except OSError as exc: + logger.warning("could not remove the unusable managed binary %s: %s", binary, exc) + return True # cannot repair it; don't spin on a reinstall that will find it again + return False + + def ensure_sd_cpp_binary(*, allow_install: bool = True, accelerator: str = "cpu") -> Optional[str]: """Path to a usable ``sd-cli`` binary, installing the prebuilt once if needed. @@ -131,14 +157,14 @@ def ensure_sd_cpp_binary(*, allow_install: bool = True, accelerator: str = "cpu" return is the router's signal to fall back to diffusers. """ found = find_sd_cpp_binary() - if found: + if found and _usable_or_discard_managed(found): return found if not allow_install: - return None + return found with _install_lock: # Re-check inside the lock: a concurrent first-load may have installed it. found = find_sd_cpp_binary() - if found: + if found and _usable_or_discard_managed(found): return found try: import sys @@ -171,13 +197,13 @@ def ensure_sd_server_binary( uses the one-shot fallback. Never raises. """ found = find_sd_server_binary() - if found: + if found and _usable_or_discard_managed(found): return found if not allow_install: - return None + return found with _install_lock: found = find_sd_server_binary() - if found: + if found and _usable_or_discard_managed(found): return found try: import sys diff --git a/studio/backend/core/inference/sd_cpp_engine.py b/studio/backend/core/inference/sd_cpp_engine.py index ebf312178d..ec7779ccf9 100644 --- a/studio/backend/core/inference/sd_cpp_engine.py +++ b/studio/backend/core/inference/sd_cpp_engine.py @@ -142,6 +142,30 @@ def _first_file(paths: list[Path]) -> Optional[str]: return None +def managed_install_root() -> Path: + """The directory the prebuilt installer owns, so callers can tell a Studio-managed binary + from a user-supplied one (SD_CLI_PATH / UNSLOTH_SD_CPP_PATH / PATH / an in-tree build). + + Only a copy under this root may be reinstalled over: replacing anything else would delete + a build the user chose. Honors UNSLOTH_STUDIO_HOME / STUDIO_HOME like the installer, so + side-by-side Studios stay isolated.""" + studio_home = os.environ.get("UNSLOTH_STUDIO_HOME") or os.environ.get("STUDIO_HOME") + if studio_home: + return Path(studio_home).parent / "stable-diffusion.cpp" + return Path.home() / ".unsloth" / "stable-diffusion.cpp" + + +def is_managed_binary(binary: Optional[str]) -> bool: + """True when ``binary`` lives under the installer-owned root (see managed_install_root).""" + if not binary: + return False + try: + Path(binary).resolve().relative_to(managed_install_root().resolve()) + except (OSError, ValueError): + return False + return True + + def _find_binary( *, direct_env: str, path_stems: tuple[str, ...], layout_stem: str ) -> Optional[str]: @@ -165,12 +189,7 @@ def _find_binary( # 3. Default install root. Honors UNSLOTH_STUDIO_HOME / STUDIO_HOME like the installer, so # side-by-side Studios stay isolated; else ~/.unsloth/.... - studio_home = os.environ.get("UNSLOTH_STUDIO_HOME") or os.environ.get("STUDIO_HOME") - default_root = ( - Path(studio_home).parent / "stable-diffusion.cpp" - if studio_home - else Path.home() / ".unsloth" / "stable-diffusion.cpp" - ) + default_root = managed_install_root() hit = _first_file(_layout_candidates(default_root, layout_stem)) if hit: return hit diff --git a/studio/backend/core/inference/sd_cpp_server.py b/studio/backend/core/inference/sd_cpp_server.py index 25a1c4f5cd..8a7c0de7bf 100644 --- a/studio/backend/core/inference/sd_cpp_server.py +++ b/studio/backend/core/inference/sd_cpp_server.py @@ -428,8 +428,16 @@ class SdCppServer: self.cancel(job_id) cancel_sent_at = time.monotonic() elif time.monotonic() - cancel_sent_at > _CANCEL_GRACE_S: - # Cancel not reflected within the grace window; abandon the poll so the caller can stop the - # server instead of holding the generate lock. + # Cancel not reflected within the grace window, so the job is still running and + # sd-server will not interrupt it (cancel_generating=false). Stop the process + # before reporting the cancellation, exactly as the deadline branch below does: + # abandoning the poll alone frees the generate lock while the native job keeps a + # core (or the GPU) busy to completion and holds the server's job slot, so the + # next request queues behind work nobody is waiting for. The caller does stop the + # server on unload, but a SUPERSEDING LOAD only does so after its multi-gigabyte + # download, and a load that then fails never reaches that point at all. Stopping + # here is safe: the backend reloads on the next generate. + self.stop() raise SdCppCancelled("sd-server generation was cancelled.") if not self.is_alive(): # Unwinding a cancel (e.g. unload killed the server): clean cancellation. diff --git a/studio/backend/routes/models.py b/studio/backend/routes/models.py index 71af09f8be..7a59415892 100644 --- a/studio/backend/routes/models.py +++ b/studio/backend/routes/models.py @@ -2526,6 +2526,20 @@ async def delete_finetuned_model( status_code = 409, detail = "Cannot delete trained models while training is running", ) + # The diffusion (Images) trainer is a second, independent run on the same storage root, + # so checking only the LLM backend let a delete rmtree the output directory a live + # diffusion run is about to write its adapter into: the run then dies at export, or + # silently recreates part of the tree, and an expensive experiment is lost. The dataset + # mutation and model-load routes already consult this service. + from core.training.diffusion_training_service import get_diffusion_training_service + if get_diffusion_training_service().is_active(): + raise HTTPException( + status_code = 409, + detail = ( + "Cannot delete trained models while diffusion (Images) training is " + "running" + ), + ) except HTTPException: raise except Exception as e: diff --git a/studio/backend/tests/test_delete_finetuned_diffusion_guard.py b/studio/backend/tests/test_delete_finetuned_diffusion_guard.py index 343d89ba83..f4bbc1e90e 100644 --- a/studio/backend/tests/test_delete_finetuned_diffusion_guard.py +++ b/studio/backend/tests/test_delete_finetuned_diffusion_guard.py @@ -177,3 +177,29 @@ def test_an_unreadable_engine_state_fails_closed(client, monkeypatch): resp = _delete(c, target) assert resp.status_code == 503 assert target.exists() + + +def test_refuses_the_delete_while_a_diffusion_training_run_is_active(client, monkeypatch): + # source="training" checked only the LLM trainer, so a delete could rmtree the output + # directory a live diffusion LoRA run is about to write its adapter into. + import sys + import types + + c, outputs = client + target = _model_dir(outputs) + monkeypatch.setattr(models_module, "_active_diffusion_backend", lambda: None) + monkeypatch.setattr(models_module, "_active_video_backend", lambda: None) + + stub = types.ModuleType("core.training.diffusion_training_service") + stub.get_diffusion_training_service = lambda: types.SimpleNamespace(is_active = lambda: True) + monkeypatch.setitem(sys.modules, "core.training.diffusion_training_service", stub) + + resp = _delete(c, target) + assert resp.status_code == 409 + assert "diffusion" in resp.json()["detail"].lower() + assert target.exists() + + # Idle again: the same delete goes through. + stub.get_diffusion_training_service = lambda: types.SimpleNamespace(is_active = lambda: False) + assert _delete(c, target).status_code == 200 + assert not target.exists() diff --git a/studio/backend/tests/test_sd_cpp_install.py b/studio/backend/tests/test_sd_cpp_install.py index deb43268e9..607df9731c 100644 --- a/studio/backend/tests/test_sd_cpp_install.py +++ b/studio/backend/tests/test_sd_cpp_install.py @@ -653,3 +653,54 @@ def test_print_asset_uses_upstream_fallback(monkeypatch, capsys): out = capsys.readouterr().out assert rc == 0 assert "vulkan" in out and "no matching prebuilt" not in out + + +def test_unrunnable_managed_binary_is_removed_so_it_reinstalls(monkeypatch, tmp_path): + # An interrupted extraction leaves an sd-cli that exists but cannot run. The finder only checks + # is_file(), so without a probe the installer never retried and native inference stayed off for + # the life of the install. + import core.inference.sd_cpp_backend as bk + import core.inference.sd_cpp_engine as eng + + root = tmp_path / "sd-home" / "stable-diffusion.cpp" + root.mkdir(parents = True) + managed = root / "sd-cli" + managed.write_bytes(b"truncated") + monkeypatch.setenv("UNSLOTH_STUDIO_HOME", str(tmp_path / "sd-home" / "studio")) + assert eng.is_managed_binary(str(managed)) is True + + monkeypatch.setattr(bk, "find_sd_cpp_binary", lambda: str(managed) if managed.exists() else None) + monkeypatch.setattr(bk, "_server_binary_runnable", lambda *_a, **_k: False) + installs: list = [] + + def _install(**kwargs): + installs.append(kwargs) + managed.write_bytes(b"good") + return managed + + import sys + import types + + stub = types.ModuleType("install_sd_cpp_prebuilt") + stub.install = _install + monkeypatch.setitem(sys.modules, "install_sd_cpp_prebuilt", stub) + + out = bk.ensure_sd_cpp_binary(accelerator = "cpu") + assert installs, "the unusable managed copy must trigger a reinstall" + assert out == str(managed) + + +def test_an_unrunnable_user_supplied_binary_is_never_deleted(monkeypatch, tmp_path): + # SD_CLI_PATH / PATH / an in-tree build belong to the user: report them and let the router's own + # probe refuse, but never remove or reinstall over them. + import core.inference.sd_cpp_backend as bk + + outside = tmp_path / "mine" / "sd-cli" + outside.parent.mkdir(parents = True) + outside.write_bytes(b"truncated") + monkeypatch.setenv("UNSLOTH_STUDIO_HOME", str(tmp_path / "elsewhere" / "studio")) + monkeypatch.setattr(bk, "find_sd_cpp_binary", lambda: str(outside)) + monkeypatch.setattr(bk, "_server_binary_runnable", lambda *_a, **_k: False) + + assert bk.ensure_sd_cpp_binary(accelerator = "cpu") == str(outside) + assert outside.exists(), "a user-supplied binary must survive" diff --git a/studio/backend/tests/test_sd_cpp_server.py b/studio/backend/tests/test_sd_cpp_server.py index da3df18f8c..0a60970230 100644 --- a/studio/backend/tests/test_sd_cpp_server.py +++ b/studio/backend/tests/test_sd_cpp_server.py @@ -366,6 +366,10 @@ def test_img_gen_abandons_when_cancel_not_honored(patched): s = _server_with(popen, client) with pytest.raises(SdCppCancelled): s.img_gen({"prompt": "x"}, cancel_event = cancel, poll_interval = 0.01) + # And the process is stopped, not left running the abandoned job: sd-server does not interrupt + # an in-flight job, so a server that ignored the cancel would otherwise burn a core (or the GPU) + # to completion and hold its job slot against the next request. + assert not s.is_alive() def test_img_gen_non_dict_submit_json_raises(patched):