From 76c707b81c0a780ee5d1d4daaa4bfec0304bba5f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 19:44:53 +0000 Subject: [PATCH] Review fixes: hand-overs with no parent always logged, Upscale refused again at job start, leak label by live parent link, constructor test discriminates, CHANGELOG wording Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Tw9fjX7o2YfeGQE18MEtPu --- CHANGELOG.md | 4 ++-- app/ui/shell/tests/habits.test.js | 2 +- core/leak_report.py | 4 ++-- core/tests/test_leak_report.py | 16 ++++++++++++++++ modules/output/render/__init__.py | 2 +- modules/output/render/tests/test_jobs_routes.py | 8 ++++++++ modules/output/render/upscale.py | 6 ++++++ 7 files changed, 36 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57035dd6..137664b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,8 +23,8 @@ - **Settings of a module that no longer exists are cleared from a project when it opens** (such as the removed Video detail), and saved at once, so the Log stops warning about them. Nothing is cleared while a module failed to load, and a module a model merely hides keeps its settings. - **Save Video carries the sampler's own last picture to the next shot** (it was carrying the Post-processed one). **Interpolate keeps the full length at 3 and 4 times** (it ended up to 75 ms short of the sound). **LoRA training refuses a model it cannot copy with the reason** (MXFP8 or int8 conversion in the loader), and a ticked MXFP8 that was skipped no longer blocks training. LoRA files that spell their matrices lora_down and lora_up load like lora_A and lora_B. - **Model Loader: no more reload on every run, and no half-way eviction.** Two loaders of one file in one graph no longer reload the model each run. When the same file comes back with other settings (SLA method, attention...), ComfyUI frees the earlier copy when the run ends instead of evicting it half way. A ComfyUI that cannot free the earlier copy is said in the Log. -- **Timeline is never shorter than its lanes**, even from a saved height. **LoRA files with Diffusers modules that have no place in H3** are counted and alerted. **A Stop pressed before Upscale or Interpolate gets its turn ends the job at once**, and only those two jobs wait for each other. **A removed watermark or upscaler file no longer refuses a render.** **A second-stage LoRA is said when Two stages is not running.** **Upscale with a model file waits for ComfyUI's queue.** -- **No more "memory leak with model" warning when ComfyUI loses track of a model.** When a model's patcher died and its parent link was already gone, ComfyUI treated the model as leaked on every load although other cached patchers (the loader's, the LoRA's, a modifier's output) still held it. Such an entry is now pointed at the deepest live patcher of its model before ComfyUI's check, so the model is tracked again and is freed normally. The leak report in the Log also states every hand-over and the state of each tracked model, so a real leak is easier to place. +- **Timeline is never shorter than its lanes**, even from a saved height. **LoRA files with Diffusers modules that have no place in H3** are counted and alerted. **A Stop pressed before Upscale or Interpolate gets its turn ends the job at once**, and only those two jobs wait for each other. **A removed watermark or upscaler file no longer refuses a render.** **A second-stage LoRA is said when Two stages is not running.** **Upscale with a model file is refused while ComfyUI has a generation running or queued** (and again when the job starts); RTX Super Resolution is not. +- **No more "memory leak with model" warning when ComfyUI loses track of a model.** When a model's patcher died and its parent link was already gone, ComfyUI treated the model as leaked on every load although other cached patchers (the loader's, the LoRA's, a modifier's output) still held it. Such an entry is now pointed at the deepest live patcher of its model before ComfyUI's check, so the model is tracked again and is freed normally. The leak report in the Log also states each hand-over with no parent (and the first 40 others) and the state of each tracked model, so a real leak is easier to place. - **Build keeps a far-below-best draft's share of the odds**, and the word "constructor" is no weight. - **No more torchsde "Should have ta>=t0" warning** on SDE-type samplers (it appeared whenever the schedule's smallest sigma had a 7th decimal of 5 or more, e.g. with Two stages). ComfyUI's Brownian noise now limits a query to its own span before torchsde sees it, so torchsde's rounding never lands outside it. The noise itself is unchanged and the picture was never affected. diff --git a/app/ui/shell/tests/habits.test.js b/app/ui/shell/tests/habits.test.js index 9cda75d5..e943adab 100644 --- a/app/ui/shell/tests/habits.test.js +++ b/app/ui/shell/tests/habits.test.js @@ -94,5 +94,5 @@ test("the like-guess is odds, not a ban: a far worse draft still comes up, and t const share = (word) => picks.filter((t) => t.includes(word)).length / picks.length; assert.ok(share("fog") > 0.05, `the worst draft keeps a chance: ${share("fog")}`); assert.ok(share("neon") < 0.8, `the best never takes everything: ${share("neon")}`); - assert.ok(share("constructor") > 0.05, `an odd word is not NaN: ${share("constructor")}`); + assert.ok(share("constructor") > 0.05 && share("constructor") < 0.3, `an odd word weighs 0, not NaN (which made it always win): ${share("constructor")}`); }); diff --git a/core/leak_report.py b/core/leak_report.py index a883cb76..ad548106 100644 --- a/core/leak_report.py +++ b/core/leak_report.py @@ -89,7 +89,7 @@ def report(mm, when) -> int: log.warning("FunUI Leak", f"{when}: ComfyUI tracks " + ", ".join( f"{type(l.real_model()).__name__ if callable(l.real_model) and l.real_model() is not None else '?'}" f"({'DEAD' if l.is_dead() else 'patcher alive' if l.model is not None else 'gone'}" - f"{', has parent link' if getattr(l, '_parent_model', None) is not None else ''})" for l in mm.current_loaded_models)) + f"{', has parent link' if callable(getattr(l, '_parent_model', None)) and l._parent_model() is not None else ''})" for l in mm.current_loaded_models)) for m in dead: model = m.real_model() if model is None: @@ -131,7 +131,7 @@ def watch_switch(mm) -> bool: parent = self._parent_model() real = self.real_model() if callable(self.real_model) else None _seen[0] += 1 - if _seen[0] <= 40: # every hand-over, so a missing one shows too + if parent is None or _seen[0] <= 40: # a hand-over with no parent always shows; the others for the first 40 of the session depth, p = 0, parent while p is not None and getattr(p, "parent", None) is not None and depth < 50: p, depth = p.parent, depth + 1 diff --git a/core/tests/test_leak_report.py b/core/tests/test_leak_report.py index fe8c4c81..f551fba7 100644 --- a/core/tests/test_leak_report.py +++ b/core/tests/test_leak_report.py @@ -122,3 +122,19 @@ def test_an_entry_with_no_live_patcher_is_left_for_the_report(comfyui): del keep finally: mm.current_loaded_models.remove(loaded) + + +def test_a_hand_over_with_no_parent_is_logged_after_the_first_forty(monkeypatch): + from core import leak_report + monkeypatch.setattr(leak_report, "_seen", [500]) + lines = [] + monkeypatch.setattr(leak_report.log, "info", lambda *a, **k: lines.append(a)) + mm = type("MM", (), {})() + + class LM: + def __init__(self): self._parent_model = lambda: None; self.real_model = lambda: None + def _switch_parent(self): pass + mm.LoadedModel = LM + leak_report.watch_switch(mm) + LM()._switch_parent() + assert lines and "hand-over" in lines[0][1] and "gone" in lines[0][1] diff --git a/modules/output/render/__init__.py b/modules/output/render/__init__.py index 76da60bf..0e016d25 100644 --- a/modules/output/render/__init__.py +++ b/modules/output/render/__init__.py @@ -102,7 +102,7 @@ def routes(table, base, web): if not isinstance(media, dict) or not isinstance(media.get("filename"), str) or not media["filename"] or not isinstance(model, str) or not model: return bad("Send {media: {filename, subfolder, type}, model, rtx_options}.") if model != rtx.NAME and upscale.comfy_busy(): - return bad("ComfyUI is generating right now. An upscale model shares ComfyUI's model memory, so run Upscale when the generation has finished (RTX Super Resolution can run beside it).") + return bad(upscale.BUSY) job = jobs.new(stoppable=True) asyncio.create_task(jobs.run(job, upscale.run, media, model, body.get("rtx_options") or "{}", error=(RuntimeError, ValueError, files.ClipError), stoppable=True, exclusive=True)) return web.json_response({"job_id": job}) diff --git a/modules/output/render/tests/test_jobs_routes.py b/modules/output/render/tests/test_jobs_routes.py index 1740d3bb..0c10bb9b 100644 --- a/modules/output/render/tests/test_jobs_routes.py +++ b/modules/output/render/tests/test_jobs_routes.py @@ -336,3 +336,11 @@ def test_upscale_with_a_model_file_is_refused_while_comfyui_is_busy_and_rtx_is_n monkeypatch.setattr(upscale, "run", lambda *a, **k: {"media": media_ref}) status, body = call(server, "POST", f"{BASE}/upscale", {"media": media_ref, "model": rtx.NAME}) assert status == 200 and body.get("job_id") # RTX uses no ComfyUI model memory: it may run beside a generation + + +def test_upscale_job_with_a_model_file_stops_if_comfyui_got_busy_after_the_route(monkeypatch): + from modules.output.render import upscale, rtx + monkeypatch.setattr(upscale, "comfy_busy", lambda: True) + import pytest + with pytest.raises(RuntimeError, match="ComfyUI is generating"): + upscale.run({"filename": "x.mp4"}, "4x.safetensors") diff --git a/modules/output/render/upscale.py b/modules/output/render/upscale.py index c7d81a3a..6712879d 100644 --- a/modules/output/render/upscale.py +++ b/modules/output/render/upscale.py @@ -26,6 +26,10 @@ def models() -> list: return [] +BUSY = ("ComfyUI is generating right now. An upscale model shares ComfyUI's model memory, so run Upscale when the generation has finished " + "(RTX Super Resolution can run beside it).") + + def comfy_busy() -> bool: """Whether ComfyUI has a prompt running or waiting. An upscale model is loaded through ComfyUI's model manager, which is not safe to use from two threads at once, so it is not started beside a generation. False when there is no server to ask (tests, a script).""" @@ -42,6 +46,8 @@ def run(media, model, rtx_options="{}", interrupted=lambda: None) -> dict: let go of the workflow's loaders and models (a prompt without them), and the next generation would load them again.""" import folder_paths + if model != rtx.NAME and comfy_busy(): # a generation may have been queued since the route looked + raise RuntimeError(BUSY) src = files.comfy_path(media.get("filename"), media.get("subfolder") or "", media.get("type") or "output") if not src or not os.path.isfile(src): raise RuntimeError(f"{media.get('filename')} is not in the {media.get('type') or 'output'} folder (anymore).")