pr feedback

Author: jeff-hykin

dimos/core/native_module.py

@@ -150,6 +150,7 @@ def start(self) -> None:
             shutdown_timeout=self.config.shutdown_timeout,
             log_json=self.config.log_format == LogFormat.JSON,
         )
+        self._proc.start()
 
     def _resolve_paths(self) -> None:
         """Resolve relative ``cwd`` and ``executable`` against the subclass's source file."""

dimos/utils/test_thread_utils.py

@@ -62,18 +62,6 @@ def test_basic_get_set(self) -> None:
         v.set(99)
         assert v.get() == 99
 
-    def test_bool_truthy(self) -> None:
-        v = ThreadSafeVal(True)
-        assert bool(v) is True
-        v.set(False)
-        assert bool(v) is False
-
-    def test_bool_zero(self) -> None:
-        v = ThreadSafeVal(0)
-        assert bool(v) is False
-        v.set(1)
-        assert bool(v) is True
-
     def test_context_manager_returns_value(self) -> None:
         v = ThreadSafeVal("hello")
         with v as val:
@@ -113,20 +101,6 @@ def do_it() -> None:
         t.join(timeout=2)
         assert result.is_set(), "Deadlocked! get() inside with block hung"
 
-    def test_bool_inside_context_manager_no_deadlock(self) -> None:
-        v = ThreadSafeVal(True)
-        result = threading.Event()
-
-        def do_it() -> None:
-            with v:
-                _ = bool(v)
-            result.set()
-
-        t = threading.Thread(target=do_it)
-        t.start()
-        t.join(timeout=2)
-        assert result.is_set(), "Deadlocked! bool() inside with block hung"
-
     def test_context_manager_blocks_other_threads(self) -> None:
         """While one thread holds the lock via `with`, others should block on set()."""
         v = ThreadSafeVal(0)
@@ -245,6 +219,7 @@ def target() -> None:
             ran.set()
 
         mt = ModuleThread(module=mod, target=target, name="test-basic")
+        mt.start()
         ran.wait(timeout=2)
         assert ran.is_set()
         mt.stop()
@@ -254,14 +229,15 @@ def test_auto_start(self) -> None:
         mod = FakeModule()
         started = threading.Event()
         mt = ModuleThread(module=mod, target=started.set, name="test-autostart")
+        mt.start()
         started.wait(timeout=2)
         assert started.is_set()
         mt.stop()
 
     def test_deferred_start(self) -> None:
         mod = FakeModule()
         started = threading.Event()
-        mt = ModuleThread(module=mod, target=started.set, name="test-deferred", start=False)
+        mt = ModuleThread(module=mod, target=started.set, name="test-deferred")
         time.sleep(0.1)
         assert not started.is_set()
         mt.start()
@@ -275,11 +251,11 @@ def test_stopping_property(self) -> None:
         holder: list[ModuleThread] = []
 
         def target() -> None:
-            while not holder[0].stopping:
+            while holder[0].status.get() == "running":
                 time.sleep(0.01)
             saw_stopping.set()
 
-        mt = ModuleThread(module=mod, target=target, name="test-stopping", start=False)
+        mt = ModuleThread(module=mod, target=target, name="test-stopping")
         holder.append(mt)
         mt.start()
         time.sleep(0.05)
@@ -290,6 +266,7 @@ def target() -> None:
     def test_stop_idempotent(self) -> None:
         mod = FakeModule()
         mt = ModuleThread(module=mod, target=lambda: time.sleep(0.01), name="test-idem")
+        mt.start()
         time.sleep(0.05)
         mt.stop()
         mt.stop()  # second call should not raise
@@ -305,7 +282,7 @@ def target() -> None:
             holder[0].stop()  # stop ourselves — should not deadlock
             result.set()
 
-        mt = ModuleThread(module=mod, target=target, name="test-self-stop", start=False)
+        mt = ModuleThread(module=mod, target=target, name="test-self-stop")
         holder.append(mt)
         mt.start()
         result.wait(timeout=3)
@@ -319,10 +296,10 @@ def test_dispose_stops_thread(self) -> None:
 
         def target() -> None:
             running.set()
-            while not holder[0].stopping:
+            while holder[0].status.get() == "running":
                 time.sleep(0.01)
 
-        mt = ModuleThread(module=mod, target=target, name="test-dispose", start=False)
+        mt = ModuleThread(module=mod, target=target, name="test-dispose")
         holder.append(mt)
         mt.start()
         running.wait(timeout=2)
@@ -336,10 +313,10 @@ def test_concurrent_stop_calls(self) -> None:
         holder: list[ModuleThread] = []
 
         def target() -> None:
-            while not holder[0].stopping:
+            while holder[0].status.get() == "running":
                 time.sleep(0.01)
 
-        mt = ModuleThread(module=mod, target=target, name="test-concurrent-stop", start=False)
+        mt = ModuleThread(module=mod, target=target, name="test-concurrent-stop")
         holder.append(mt)
         mt.start()
         time.sleep(0.05)
@@ -369,6 +346,7 @@ def stubborn_target() -> None:
         mt = ModuleThread(
             module=mod, target=stubborn_target, name="test-timeout", close_timeout=0.2
         )
+        mt.start()
         start = time.monotonic()
         mt.stop()
         elapsed = time.monotonic() - start
@@ -381,10 +359,10 @@ def test_stop_concurrent_with_dispose(self) -> None:
             holder: list[ModuleThread] = []
 
             def target(h: list[ModuleThread] = holder) -> None:
-                while not h[0].stopping:
+                while h[0].status.get() == "running":
                     time.sleep(0.001)
 
-            mt = ModuleThread(module=mod, target=target, name="test-stop-dispose", start=False)
+            mt = ModuleThread(module=mod, target=target, name="test-stop-dispose")
             holder.append(mt)
             mt.start()
             time.sleep(0.02)
@@ -490,6 +468,7 @@ def test_basic_lifecycle(self) -> None:
             args=[PYTHON, "-c", "import time; time.sleep(30)"],
             shutdown_timeout=2.0,
         )
+        mp.start()
         assert mp.is_alive
         assert mp.pid is not None
         mp.stop()
@@ -503,6 +482,7 @@ def test_stop_idempotent(self) -> None:
             args=[PYTHON, "-c", "import time; time.sleep(30)"],
             shutdown_timeout=1.0,
         )
+        mp.start()
         mp.stop()
         mp.stop()  # should not raise
         mp.stop()
@@ -514,6 +494,7 @@ def test_dispose_stops_process(self) -> None:
             args=[PYTHON, "-c", "import time; time.sleep(30)"],
             shutdown_timeout=2.0,
         )
+        mp.start()
         mod.dispose()
         time.sleep(0.5)
         assert not mp.is_alive
@@ -523,23 +504,25 @@ def test_on_exit_fires_on_natural_exit(self) -> None:
         mod = FakeModule()
         exit_called = threading.Event()
 
-        ModuleProcess(
+        mp = ModuleProcess(
             module=mod,
             args=[PYTHON, "-c", "print('done')"],
             on_exit=exit_called.set,
         )
+        mp.start()
         exit_called.wait(timeout=5)
         assert exit_called.is_set(), "on_exit was not called after natural process exit"
 
     def test_on_exit_fires_on_crash(self) -> None:
         mod = FakeModule()
         exit_called = threading.Event()
 
-        ModuleProcess(
+        mp = ModuleProcess(
             module=mod,
             args=[PYTHON, "-c", "import sys; sys.exit(1)"],
             on_exit=exit_called.set,
         )
+        mp.start()
         exit_called.wait(timeout=5)
         assert exit_called.is_set(), "on_exit was not called after process crash"
 
@@ -554,6 +537,7 @@ def test_on_exit_not_fired_on_stop(self) -> None:
             on_exit=exit_called.set,
             shutdown_timeout=2.0,
         )
+        mp.start()
         time.sleep(0.2)  # let watchdog start
         mp.stop()
         time.sleep(1.0)  # give watchdog time to potentially fire
@@ -565,6 +549,7 @@ def test_stdout_logged(self) -> None:
             module=mod,
             args=[PYTHON, "-c", "print('hello from subprocess')"],
         )
+        mp.start()
         time.sleep(1.0)  # let output be read
         mp.stop()
 
@@ -574,6 +559,7 @@ def test_stderr_logged(self) -> None:
             module=mod,
             args=[PYTHON, "-c", "import sys; sys.stderr.write('error msg\\n')"],
         )
+        mp.start()
         time.sleep(1.0)
         mp.stop()
 
@@ -588,6 +574,7 @@ def test_log_json_mode(self) -> None:
             ],
             log_json=True,
         )
+        mp.start()
         time.sleep(1.0)
         mp.stop()
 
@@ -598,6 +585,7 @@ def test_log_json_malformed(self) -> None:
             args=[PYTHON, "-c", "print('not json')"],
             log_json=True,
         )
+        mp.start()
         time.sleep(1.0)
         mp.stop()
 
@@ -614,6 +602,7 @@ def test_stop_process_that_ignores_sigterm(self) -> None:
             shutdown_timeout=0.5,
             kill_timeout=2.0,
         )
+        mp.start()
         time.sleep(0.2)
         start = time.monotonic()
         mp.stop()
@@ -629,6 +618,7 @@ def test_stop_already_dead_process(self) -> None:
             module=mod,
             args=[PYTHON, "-c", "pass"],  # exits immediately
         )
+        mp.start()
         time.sleep(1.0)  # let it die
         mp.stop()  # should not raise
 
@@ -639,6 +629,7 @@ def test_concurrent_stop(self) -> None:
             args=[PYTHON, "-c", "import time; time.sleep(30)"],
             shutdown_timeout=2.0,
         )
+        mp.start()
         errors = []
 
         def stop_it() -> None:
@@ -667,11 +658,12 @@ def fake_module_stop() -> None:
             mod.dispose()
             stop_called.set()
 
-        ModuleProcess(
+        mp = ModuleProcess(
             module=mod,
             args=[PYTHON, "-c", "pass"],  # exits immediately
             on_exit=fake_module_stop,
         )
+        mp.start()
         stop_called.wait(timeout=5)
         assert stop_called.is_set(), "Deadlocked! on_exit -> dispose -> stop chain hung"
 
@@ -685,7 +677,6 @@ def test_deferred_start(self) -> None:
         mp = ModuleProcess(
             module=mod,
             args=[PYTHON, "-c", "import time; time.sleep(30)"],
-            start=False,
         )
         assert not mp.is_alive
         mp.start()
@@ -696,7 +687,7 @@ def test_env_passed(self) -> None:
         mod = FakeModule()
         exit_called = threading.Event()
 
-        ModuleProcess(
+        mp = ModuleProcess(
             module=mod,
             args=[
                 PYTHON,
@@ -706,6 +697,7 @@ def test_env_passed(self) -> None:
             env={**os.environ, "MY_VAR": "42"},
             on_exit=exit_called.set,
         )
+        mp.start()
         exit_called.wait(timeout=5)
         # Process should have exited with 0 (our on_exit fires for all unmanaged exits)
         assert exit_called.is_set()
@@ -717,6 +709,7 @@ def test_cwd_passed(self) -> None:
             args=[PYTHON, "-c", "import os; print(os.getcwd())"],
             cwd="/tmp",
         )
+        mp.start()
         time.sleep(1.0)
         mp.stop()
 
@@ -826,23 +819,25 @@ def test_chain_no_deadlock_fast_exit(self) -> None:
         for _ in range(20):
             mod = FakeModule()
             done = threading.Event()
-            ModuleProcess(
+            mp = ModuleProcess(
                 module=mod,
                 args=[PYTHON, "-c", "pass"],
                 on_exit=self._make_fake_stop(mod, done),
             )
+            mp.start()
             assert done.wait(timeout=5), "Deadlock in dispose chain (fast exit)"
 
     def test_chain_no_deadlock_slow_exit(self) -> None:
         """Process runs briefly then exits."""
         for _ in range(10):
             mod = FakeModule()
             done = threading.Event()
-            ModuleProcess(
+            mp = ModuleProcess(
                 module=mod,
                 args=[PYTHON, "-c", "import time; time.sleep(0.1)"],
                 on_exit=self._make_fake_stop(mod, done),
             )
+            mp.start()
             assert done.wait(timeout=5), "Deadlock in dispose chain (slow exit)"
 
     def test_chain_concurrent_with_external_stop(self) -> None:
@@ -856,6 +851,7 @@ def test_chain_concurrent_with_external_stop(self) -> None:
                 on_exit=self._make_fake_stop(mod, done),
                 shutdown_timeout=1.0,
             )
+            mp.start()
             # Race: the process might exit naturally or we might stop it
             time.sleep(0.03)
             mp.stop()
@@ -874,11 +870,12 @@ def slow_stop(self_mt: ModuleThread) -> None:
             mod = FakeModule()
             done = threading.Event()
             with mock.patch.object(ModuleThread, "stop", slow_stop):
-                ModuleProcess(
+                mp = ModuleProcess(
                     module=mod,
                     args=[PYTHON, "-c", "pass"],
                     on_exit=self._make_fake_stop(mod, done),
                 )
+                mp.start()
                 assert done.wait(timeout=10), "Deadlock with slow ModuleThread.stop()"