From 6e424d4696d1af4be7d87ab9373fe05b8ada26b0 Mon Sep 17 00:00:00 2001 From: goodboy Date: Thu, 13 Aug 2026 20:38:22 -0400 Subject: [PATCH] Use a daemon-ready sentinel in discovery tests The discovery `daemon` fixture probed UDS readiness by connecting and immediately closing. That entered Tractor's actor handshake without an `Aid` payload and destabilized the remote registrar on macOS before test roots attempted discovery. Run the child through a small `open_root_actor()` wrapper and publish a filesystem sentinel only after runtime startup completes. Poll that sentinel with process-liveness checks and guaranteed setup-failure cleanup, without touching the transport socket. Cover transport-free readiness and deterministic polling backoff. Caught-during: review remediation Found-via: `/run-tests` discovery daemon fixture consumers Cause: initial sentinel drafts mishandled `pytest.Testdir`, emitted invalid `python -c` syntax, and misplaced return-code logging. Review: PR #480 (goodboy) https://github.com/goodboy/tractor/pull/480 (this patch was generated in some part by `opencode` using `gpt-5.6-sol` (`openai`)) --- tests/discovery/conftest.py | 174 ++++++++++++------------- tests/discovery/test_daemon_fixture.py | 72 ++++++++++ 2 files changed, 152 insertions(+), 94 deletions(-) create mode 100644 tests/discovery/test_daemon_fixture.py diff --git a/tests/discovery/conftest.py b/tests/discovery/conftest.py index 73749a6d..9ed318a0 100644 --- a/tests/discovery/conftest.py +++ b/tests/discovery/conftest.py @@ -13,9 +13,8 @@ under `tests/discovery/` automatically picks this up. ''' from __future__ import annotations -import os +from pathlib import Path import platform -import socket import subprocess import sys import time @@ -31,27 +30,22 @@ from ..conftest import ( def _wait_for_daemon_ready( - reg_addr: tuple, - tpt_proto: str, + ready_path: Path, *, deadline: float = 10.0, poll_interval: float = 0.05, proc: subprocess.Popen|None = None, ) -> None: ''' - Active-poll the daemon's bind address until it - accepts a connection (proving it has called - `bind() + listen()` and is ready to handle IPC). + Poll until the daemon reports completed actor startup. Replaces the historical blind `time.sleep()` in the `daemon` fixture which was racy under load — see `ai/conc-anal/test_register_duplicate_name_daemon_connect_race_issue.md`. - Uses stdlib `socket` directly (no trio runtime - bootstrap cost) — sufficient because - `tractor.run_daemon()` doesn't return from - bootstrap until the runtime is fully ready to - accept IPC. + The child writes `ready_path` only after entering + `open_root_actor()`, which guarantees all transport listeners are + serving without requiring a raw connection probe. Raises `TimeoutError` on `deadline` exceeded. If `proc` is given, ALSO raises early if the daemon @@ -70,43 +64,25 @@ def _wait_for_daemon_ready( if proc is not None and proc.poll() is not None: raise RuntimeError( f'Daemon proc exited (rc={proc.returncode}) ' - f'before becoming ready to accept on ' - f'{reg_addr!r}' + f'before reporting ready at {ready_path!r}' ) try: - if tpt_proto == 'tcp': - # `socket.create_connection` does the - # `socket() + connect()` dance with a - # builtin timeout — perfect primitive - # for a one-shot probe. - with socket.create_connection( - reg_addr, - timeout=poll_interval, - ): - return - else: - # UDS — `reg_addr` is a `(filedir, sockname)` - # tuple per `tractor.ipc._uds.UDSAddress.unwrap`. - sockpath: str = os.path.join(*reg_addr) - sock = socket.socket(socket.AF_UNIX) - try: - sock.settimeout(poll_interval) - sock.connect(sockpath) - return - finally: - sock.close() + if ready_path.is_file(): + if proc is not None and proc.poll() is not None: + raise RuntimeError( + f'Daemon proc exited (rc={proc.returncode}) ' + f'after reporting ready at {ready_path!r}' + ) + return except ( - ConnectionRefusedError, FileNotFoundError, OSError, - socket.timeout, ) as exc: last_exc = exc - time.sleep(poll_interval) + time.sleep(poll_interval) raise TimeoutError( - f'Daemon never accepted on {reg_addr!r} within ' - f'{deadline}s (last connect-attempt exc: ' - f'{last_exc!r})' + f'Daemon never reported ready at {ready_path!r} within ' + f'{deadline}s (last sentinel-state exc: {last_exc!r})' ) @@ -136,18 +112,27 @@ def daemon( ) loglevel: str = 'info' + ready_path: Path = ( + Path(str(testdir.tmpdir)) + / 'daemon-ready' + ) + ready_path.unlink(missing_ok=True) code: str = ( - "import tractor; " - "tractor.run_daemon([], " - "registry_addrs={reg_addrs}, " - "enable_transports={enable_tpts}, " - "debug_mode={debug_mode}, " - "loglevel={ll})" - ).format( - reg_addrs=str([reg_addr]), - enable_tpts=str([tpt_proto]), - ll="'{}'".format(loglevel) if loglevel else None, - debug_mode=debug_mode, + f'from pathlib import Path\n' + f'import tractor\n' + f'import trio\n' + f'\n' + f'async def main():\n' + f' async with tractor.open_root_actor(\n' + f' registry_addrs={[reg_addr]!r},\n' + f' enable_transports={[tpt_proto]!r},\n' + f' debug_mode={debug_mode!r},\n' + f' loglevel={loglevel!r},\n' + f' ):\n' + f' Path({str(ready_path)!r}).touch()\n' + f' await trio.sleep_forever()\n' + f'\n' + f'trio.run(main)\n' ) cmd: list[str] = [ sys.executable, @@ -176,48 +161,49 @@ def daemon( 15.0 if (_non_linux and ci_env) else 10.0 ) - _wait_for_daemon_ready( - reg_addr=reg_addr, - tpt_proto=tpt_proto, - deadline=deadline, - proc=proc, - ) - - assert not proc.returncode - yield proc - sig_prog(proc, _INT_SIGNAL) - - # XXX! yeah.. just be reaaal careful with this bc - # sometimes it can lock up on the `_io.BufferedReader` - # and hang.. - # - # NB, drain happens at TEARDOWN (post-yield), so the - # test body has its chance to read `proc.stderr` - # FIRST. Reading here AFTER would silently swallow - # the daemon's stderr output and break tests that - # assert on it (e.g. `test_abort_on_sigint`). - stderr: str = proc.stderr.read().decode() - stdout: str = proc.stdout.read().decode() - if ( - stderr - or - stdout - ): - print( - f'Daemon actor tree produced output:\n' - f'{proc.args}\n' - f'\n' - f'stderr: {stderr!r}\n' - f'stdout: {stdout!r}\n' + try: + _wait_for_daemon_ready( + ready_path=ready_path, + deadline=deadline, + proc=proc, ) - if (rc := proc.returncode) != -2: - msg: str = ( - f'Daemon actor tree was not cancelled !?\n' - f'proc.args: {proc.args!r}\n' - f'proc.returncode: {rc!r}\n' - ) - if rc < 0: - raise RuntimeError(msg) + assert not proc.returncode + yield proc + finally: + if proc.poll() is None: + sig_prog(proc, _INT_SIGNAL) - test_log.error(msg) + # XXX! yeah.. just be reaaal careful with this bc + # sometimes it can lock up on the `_io.BufferedReader` + # and hang.. + # + # NB, drain happens at TEARDOWN (post-yield), so the + # test body has its chance to read `proc.stderr` + # FIRST. Reading here AFTER would silently swallow + # the daemon's stderr output and break tests that + # assert on it (e.g. `test_abort_on_sigint`). + stderr: str = proc.stderr.read().decode() + stdout: str = proc.stdout.read().decode() + if ( + stderr + or + stdout + ): + print( + f'Daemon actor tree produced output:\n' + f'{proc.args}\n' + f'\n' + f'stderr: {stderr!r}\n' + f'stdout: {stdout!r}\n' + ) + + if (rc := proc.returncode) != -2: + msg: str = ( + f'Daemon actor tree was not cancelled !?\n' + f'proc.args: {proc.args!r}\n' + f'proc.returncode: {rc!r}\n' + ) + if rc < 0: + raise RuntimeError(msg) + test_log.error(msg) diff --git a/tests/discovery/test_daemon_fixture.py b/tests/discovery/test_daemon_fixture.py new file mode 100644 index 00000000..bf53758c --- /dev/null +++ b/tests/discovery/test_daemon_fixture.py @@ -0,0 +1,72 @@ +''' +Discovery daemon fixture regressions. + +''' +from unittest.mock import ( + call, + Mock, +) + +from .conftest import _wait_for_daemon_ready + + +def test_daemon_ready_check_does_not_connect( + monkeypatch, + tmp_path, +): + ''' + Detect a listening UDS daemon without creating a raw connection. + + The old UDS readiness helper connected and immediately closed. That + entered Tractor's actor-handshake handler with no `Aid` payload and + destabilized the remote registrar on macOS before discovery tests + started. This test creates the child sentinel, forbids all socket + construction and connection helpers, then proves readiness returns + without touching the transport layer. + + ''' + ready_path = tmp_path / 'daemon-ready' + ready_path.touch() + socket_ctor = Mock(side_effect=AssertionError('socket opened')) + connect = Mock(side_effect=AssertionError('socket connected')) + monkeypatch.setattr('socket.socket', socket_ctor) + monkeypatch.setattr('socket.create_connection', connect) + + _wait_for_daemon_ready( + ready_path=ready_path, + deadline=.1, + poll_interval=.01, + ) + + socket_ctor.assert_not_called() + connect.assert_not_called() + + +def test_daemon_ready_check_backs_off(monkeypatch): + ''' + Back off while waiting for the child startup sentinel. + + The sentinel may appear after several parent polling intervals. + A deterministic false/false/true path sequence proves the helper + sleeps between unsuccessful observations instead of hot-spinning + and starving a booting daemon on constrained CI workers. + + ''' + ready_path = Mock() + ready_path.is_file.side_effect = [False, False, True] + sleep = Mock() + monotonic = Mock(side_effect=[0, 0, 0, 0]) + monkeypatch.setattr('time.sleep', sleep) + monkeypatch.setattr('time.monotonic', monotonic) + + _wait_for_daemon_ready( + ready_path=ready_path, + deadline=.2, + poll_interval=.01, + ) + + assert ready_path.is_file.call_count == 3 + assert sleep.call_args_list == [ + call(.01), + call(.01), + ]