fix: address review-bot findings on merged PRs (#593)

Squashed from PR #595. Closes #593.

tests/js/prompt-sender.test.js

@@ -255,7 +255,8 @@ test('All is offered only with two or more eligible nodes and is never selected
 test('Apply to All previews every node first, then sends one after another in node id order',async()=>{
     const {createSendSession,ALL_NODES}=await source;const s=await allSession(createSendSession);
     s.session.selectTarget(ALL_NODES);await s.session.apply();
-    assert.deepEqual(order(s.calls),['preview 2','preview 9','preview 10','apply 2','preview 9','apply 9','preview 10','apply 10']);
+    // Each node takes a fresh ticket, the first too: the host keeps only its latest 100 (#593).
+    assert.deepEqual(order(s.calls),['preview 2','preview 9','preview 10','preview 2','apply 2','preview 9','apply 9','preview 10','apply 10']);
     assert.equal(s.session.state.phase,'success');assert.equal(s.session.state.message,'Sent to 3 of 3 nodes in Flow - ComfyUI.');
     await s.session.apply();assert.equal(s.calls.filter(call=>call[0]==='apply').length,3);s.session.dispose();
 });
@@ -363,3 +364,48 @@ test('a changed prepend or append after the review refuses that node',async()=>{
     assert.deepEqual(s.calls.filter(call=>call[0]==='apply').map(call=>call[1]),['2','10']);
     assert.match(s.session.state.message,/^Sent to 2 of 3 nodes in Flow - ComfyUI\. #9 Node 9: Its prompt changed since the preview\./);s.session.dispose();
 });
+
+// #593: Send to All through the real Toolkit bridge and the real adapter, as the review bot reproduced it.
+function realWindow(count) {
+    const listeners=new Set();
+    const channelFactory=()=>{const own=new Set();return{
+        addEventListener(type,listener){if(type==='message'){own.add(listener);listeners.add(listener);}},removeEventListener(type,listener){listeners.delete(listener);},
+        postMessage(data){for(const listener of [...listeners])if(!own.has(listener))queueMicrotask(()=>listener({data:structuredClone(data),origin:'https://comfy.example'}));},close(){},
+    };};
+    // Like LiteGraph, a widget write moves the version of the graph it is in.
+    const graph=()=>{const made={_version:1,setDirtyCanvas(){made._version++;}};made._nodes=Array.from({length:count},(_,index)=>{
+        const widgets=['text','prepend_text','append_text'].map(name=>({name,value:name==='text'?'old':'',inputEl:{value:name==='text'?'old':''}}));
+        return {id:index+1,type:'PromptManager',title:`Node ${index+1}`,widgets,widgets_values:widgets.map(widget=>widget.value),inputs:[],setDirtyCanvas(){}};
+    });return made;};
+    return {channelFactory,graph};
+}
+async function realSend(count,{afterFirstApply}={}) {
+    const {createSendSession,ALL_NODES}=await source;
+    const {createBridgeHost,createBridgeClient}=await import('../../web/js/shared/toolkit/prompt-bridge.js');
+    const {createPromptAdapter}=await import('../../web/js/prompt-bridge-adapter.js');
+    const win=realWindow(count);const app={graph:win.graph(),canvas:{}};
+    const adapter=createPromptAdapter(app,{runtime:{}});let applied=0;
+    const watched={...adapter,apply(snapshot){const result=adapter.apply(snapshot);if(++applied===1)afterFirstApply?.(app,win);return result;}};
+    const options={namespace:'/prompt_manager/',origin:'https://comfy.example',channelFactory:win.channelFactory};
+    const host=createBridgeHost({...options,label:'Flow - ComfyUI',adapter:watched});
+    const client=createBridgeClient(options);
+    const session=createSendSession({client,roles:{positive:'new'},onChange:()=>{}});
+    try {
+        await session.discover();session.selectTarget(ALL_NODES);await session.apply();
+        return {state:session.state,app};
+    } finally { session.dispose();host.dispose(); }
+}
+test('All refuses the later nodes when another workflow is loaded after the first send (#593)',async()=>{
+    let first;
+    const {state,app}=await realSend(3,{afterFirstApply:(target,win)=>{first=target.graph;target.graph=win.graph();target.graph._version=first._version;}});
+    // The new workflow has the same ids, text and composition, and even the same graph version: only node identity differs.
+    assert.equal(first._nodes[0].widgets[0].value,'new');
+    assert.deepEqual(app.graph._nodes.map(node=>node.widgets[0].value),['old','old','old']);
+    assert.equal(state.phase,'error');
+    assert.match(state.message,/^Sent to 1 of 3 nodes in Flow - ComfyUI\. #2 Node 2: The workflow changed since this window was found\./);
+});
+test('All sends to every node of a window with more than 100 of them (#593)',async()=>{
+    const {state,app}=await realSend(101);
+    assert.equal(state.message,'Sent to 101 of 101 nodes in Flow - ComfyUI.');
+    assert.ok(app.graph._nodes.every(node=>node.widgets[0].value==='new'));
+});

tests/test_maintenance_pair_metadata.py

@@ -171,7 +171,7 @@ class TestClassification(PairMetadataCase):
         self.assertEqual(self.preview().targets, [[prompt, True, True]])
 
     def test_category_is_case_sensitive(self):
-        _a, prompt = self.pair("case", "n1", category="Portrait")
+        self.pair("case", "n1", category="Portrait")
         self.pair("case", "n2", category="portrait")
         draft = self.preview()
         self.assertEqual(draft.targets, [])
@@ -292,7 +292,7 @@ class TestClassification(PairMetadataCase):
         )
 
     def test_protected_rows_come_first_within_fifty(self):
-        _c, conflict = self.pair("zz conflict", "n1", category="x")
+        self.pair("zz conflict", "n1", category="x")
         self.pair("zz conflict", "n2", category="y")
         for n in range(60):
             self.pair(f"fill {n:02d}", category="portrait")

tests/test_native_memory.py

@@ -2,7 +2,7 @@
 
 import sys
 import unittest
-from unittest import mock
+import unittest.mock as mock
 
 from utils import native_memory
 
@@ -10,7 +10,9 @@ from utils import native_memory
 class ReleaseFreeMemoryTests(unittest.TestCase):
     def setUp(self):
         # Each test looks the symbol up again.
-        patcher = mock.patch.multiple(native_memory, _trim=None, _looked_up=False)
+        patcher = mock.patch.object(
+            native_memory, "_trim", native_memory._NOT_LOOKED_UP
+        )
         patcher.start()
         self.addCleanup(patcher.stop)
 
@@ -22,7 +24,7 @@ class ReleaseFreeMemoryTests(unittest.TestCase):
     def test_other_platforms_do_nothing(self):
         for platform in ("win32", "darwin", "cygwin"):
             with self.subTest(platform), mock.patch.object(sys, "platform", platform):
-                native_memory._looked_up = False
+                native_memory._trim = native_memory._NOT_LOOKED_UP
                 with mock.patch.object(native_memory.ctypes, "CDLL") as cdll:
                     self.assertFalse(native_memory.release_free_memory())
                 cdll.assert_not_called()

tests/test_saved_instructions.py

@@ -153,7 +153,7 @@ class ConfigTestCase(unittest.TestCase):
             path = os.path.join(tmpdir, "config.json")
             with open(path, "w", encoding="utf-8") as handle:
                 json.dump({"autotag": {"defaults": defaults}}, handle)
-            before = os.stat(path).st_mtime_ns, open(path, "rb").read()
+            before = os.stat(path).st_mtime_ns, self.read_bytes(path)
             if warnings:
                 with self.assertLogs("prompt_manager.config", level="WARNING") as logs:
                     PromptManagerConfig.load_from_file(path)
@@ -162,9 +162,13 @@ class ConfigTestCase(unittest.TestCase):
             else:
                 PromptManagerConfig.load_from_file(path)
             # Nothing is written at load.
-            self.assertEqual(
-                (os.stat(path).st_mtime_ns, open(path, "rb").read()), before
-            )
+            self.assertEqual((os.stat(path).st_mtime_ns, self.read_bytes(path)), before)
+
+    @staticmethod
+    def read_bytes(path):
+        # Closed at once: Windows cannot delete the temp dir with it open.
+        with open(path, "rb") as handle:
+            return handle.read()
 
     def state(self):
         return [dict(e) for e in AutotagConfig.INSTRUCTIONS], dict(

tests/test_utils_parallel.py

@@ -12,10 +12,12 @@ import unittest
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
-from utils import parallel  # noqa: E402
+from utils import exit_signal, parallel  # noqa: E402
 from utils.parallel import JOB_THREAD_PREFIX, map_parallel  # noqa: E402
 
 WAIT = 30
+# A map that has not returned by then is stuck (#593).
+HANG_LIMIT = 5
 
 
 def _job_threads():
@@ -163,5 +165,68 @@ class TestMapParallel(unittest.TestCase):
             map_parallel(boom, [1, 2, 3], workers=3)
 
 
+class TestErrorAfterShutdownCancel(unittest.TestCase):
+    """#593: ``shutdown_job_pool`` cancels the call's queued items without
+    notifying them, so the error path must not wait on them (exit hung)."""
+
+    def setUp(self):
+        exit_signal.reset()
+        self.addCleanup(exit_signal.reset)
+        self.assertTrue(parallel.shutdown_job_pool(WAIT))
+        self.addCleanup(parallel.shutdown_job_pool, WAIT)
+
+    def _run_in_thread(self, func, items):
+        """``map_parallel`` on a daemon thread: (thread, outcome list), so a
+        hang fails the test instead of hanging the suite."""
+        outcome = []
+
+        def call():
+            try:
+                outcome.append(("result", map_parallel(func, items, 1)))
+            except Exception as error:  # the outcome the test checks
+                outcome.append(("error", error))
+
+        thread = threading.Thread(target=call, daemon=True)
+        thread.start()
+        return thread, outcome
+
+    def _hold_first_then_cancel_rest(self, first):
+        """One worker runs item 0 (``first``) while items 1 and 2 queue;
+        shutdown cancels the queued two, then item 0 is released."""
+        entered, release = threading.Event(), threading.Event()
+
+        def item(n):
+            if n == 0:
+                entered.set()
+                self.assertTrue(release.wait(WAIT))
+                return first()
+            return n
+
+        thread, outcome = self._run_in_thread(item, [0, 1, 2])
+        self.assertTrue(entered.wait(WAIT))
+        self.assertFalse(parallel.shutdown_job_pool(0))  # item 0 still runs
+        release.set()
+        thread.join(HANG_LIMIT)
+        self.assertFalse(thread.is_alive(), "map_parallel hung on cancelled items")
+        return outcome
+
+    def test_a_running_item_raising_after_shutdown_cancel_returns(self):
+        def fail():
+            raise ValueError("zero")
+
+        outcome = self._hold_first_then_cancel_rest(fail)
+        self.assertEqual(outcome[0][0], "error")
+        self.assertIsInstance(outcome[0][1], ValueError)
+
+    def test_exit_requested_inline_after_shutdown_cancel_returns(self):
+        def signal_exit():
+            exit_signal.request()  # the cancelled items then run inline
+            return 0
+
+        outcome = self._hold_first_then_cancel_rest(signal_exit)
+        self.assertEqual(outcome[0][0], "error")
+        self.assertIsInstance(outcome[0][1], exit_signal.ExitRequested)
+
+
 if __name__ == "__main__":
     unittest.main()