diff --git a/docs/explain/architecture.rst b/docs/explain/architecture.rst index 97d9fcdc..091647f3 100644 --- a/docs/explain/architecture.rst +++ b/docs/explain/architecture.rst @@ -130,9 +130,10 @@ UDS: same-host, creds included Pass ``enable_transports=['uds']`` and actors instead talk over unix-domain sockets, with socket files placed in the per-user -runtime dir (``$XDG_RUNTIME_DIR/tractor/`` on linux, the -``platformdirs`` equivalent elsewhere). Two perks over tcp on a -single host: +runtime dir: ``$XDG_RUNTIME_DIR/tractor/`` on linux, a short +owner-only ``/tmp/tractor-`` dir on Darwin, and the +``platformdirs`` equivalent elsewhere. Two perks over tcp on a single +host: - no ports to fight over; addrs are just file paths, - the kernel snitches on your peer for free: the listening side diff --git a/docs/guide/discovery.rst b/docs/guide/discovery.rst index cc76a7f3..ea993b52 100644 --- a/docs/guide/discovery.rst +++ b/docs/guide/discovery.rst @@ -44,8 +44,9 @@ clan shares one registry with zero config on your part. The bootstrap rule inside ``open_root_actor()`` is delightfully simple: -- on boot, ping every socket addr in ``registry_addrs``; when none - are passed the per-transport defaults are used: for TCP the +- on boot, probe every addr in ``registry_addrs`` with a bounded + Tractor ``Aid`` handshake; when none are passed the per-transport + defaults are used: for TCP the loopback ``('127.0.0.1', 1616)``, for UDS a ``registry@1616.sock`` file, @@ -53,9 +54,11 @@ simple: actor and register with the *existing* registry; your own IPC server binds random same-transport addrs instead, -- if **nothing answers, congratulations: you just became the - registrar**. Your transport server binds the registry addrs - themselves and you start serving lookups for everyone else. +- if every address is absent, congratulations: you just became the + registrar. Your transport server binds the registry addrs + themselves and you start serving lookups for everyone else, +- if no registrar answers but an address is occupied by a foreign or + non-responsive endpoint, startup fails instead of binding over it. Pass ``ensure_registry=True`` when your program *requires* being the one-and-only registrar; boot then fails loudly with a @@ -196,9 +199,10 @@ the existing registrar: trio.run(main) -Per the bootstrap rules above, if the registrar at those addrs is -*not* reachable this process simply becomes its own (registrar) -root — so the same code works standalone and as a tree-joiner. +Per the bootstrap rules above, if those addrs are absent this process +becomes its own registrar root, so the same code works standalone and +as a tree-joiner. An occupied address that does not complete a Tractor +registrar handshake fails startup instead of being rebound. "Arbiter"? A legacy naming note ------------------------------- diff --git a/nooz/473.bugfix.rst b/nooz/473.bugfix.rst new file mode 100644 index 00000000..1736add6 --- /dev/null +++ b/nooz/473.bugfix.rst @@ -0,0 +1,4 @@ +Fix Unix-domain-socket actor trees and registrar discovery on macOS. +Runtime sockets now use a short, owner-only runtime directory, +generated socket names remain within platform limits, and transient +or reset pre-handshake connections no longer destabilize discovery. diff --git a/tests/discovery/conftest.py b/tests/discovery/conftest.py index 9ed318a0..2a7320c6 100644 --- a/tests/discovery/conftest.py +++ b/tests/discovery/conftest.py @@ -1,12 +1,10 @@ ''' -Discovery-suite fixtures, including the `daemon` -remote-registrar subprocess used by the multi-program -discovery tests. +Discovery-suite fixtures, including the `daemon` remote-registrar +subprocess used by the multi-program discovery tests. Lives here (vs. the parent `tests/conftest.py`) -because `daemon` is a discovery-protocol primitive — -boots a separate `tractor.run_daemon()` process whose -sole purpose is to serve as a registrar peer for +because `daemon` is a discovery-protocol primitive: it boots a child +that enters `open_root_actor()` and waits as a registrar peer for discovery-roundtrip tests. Pytest fixtures inherit DOWNWARD through conftest hierarchy, so anything under `tests/discovery/` automatically picks this up. @@ -49,9 +47,8 @@ def _wait_for_daemon_ready( Raises `TimeoutError` on `deadline` exceeded. If `proc` is given, ALSO raises early if the daemon - process exits non-zero before the deadline (catches - daemon-startup-crash that the blind sleep used to - silently mask). + process exits before the deadline (catches a daemon startup crash + that the blind sleep used to silently mask). ''' end: float = time.monotonic() + deadline @@ -148,9 +145,9 @@ def daemon( **kwargs, ) - # Active-poll the daemon's bind address until it's - # ready to accept connections — replaces the legacy - # blind `time.sleep(2.2)` which was racy under load + # Poll the child's ready sentinel, published after actor startup, + # instead of connecting to its transport socket. This replaces + # the legacy blind `time.sleep(2.2)` which was racy under load # (see # `ai/conc-anal/test_register_duplicate_name_daemon_connect_race_issue.md`). # @@ -174,9 +171,9 @@ def daemon( if proc.poll() is None: 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.. + # NOTE: these blocking reads can hang when descendants retain + # inherited pipe descriptors. Keep teardown signaling above + # them and avoid adding subprocesses outside the actor tree. # # NB, drain happens at TEARDOWN (post-yield), so the # test body has its chance to read `proc.stderr` diff --git a/tests/discovery/test_daemon_fixture.py b/tests/discovery/test_daemon_fixture.py index bf53758c..d8899e42 100644 --- a/tests/discovery/test_daemon_fixture.py +++ b/tests/discovery/test_daemon_fixture.py @@ -15,7 +15,7 @@ def test_daemon_ready_check_does_not_connect( tmp_path, ): ''' - Detect a listening UDS daemon without creating a raw connection. + Observe completed daemon startup without a raw connection. The old UDS readiness helper connected and immediately closed. That entered Tractor's actor-handshake handler with no `Aid` payload and diff --git a/tests/discovery/test_tpt_bind_addrs.py b/tests/discovery/test_tpt_bind_addrs.py index df696888..ca65587e 100644 --- a/tests/discovery/test_tpt_bind_addrs.py +++ b/tests/discovery/test_tpt_bind_addrs.py @@ -132,8 +132,8 @@ def test_transport_only_listener_is_not_registrar(): connect alone. A non-Tractor listener, or a registrar still failing its initial handshake, was therefore selected as the remote registry. This test accepts the probe and closes it without - replying, then proves `open_root_actor()` ignores that endpoint and - elects the local actor registrar instead. + replying, then proves `open_root_actor()` rejects that occupied + endpoint instead of selecting it or binding over it. ''' async def transport_only_handler( diff --git a/tests/test_docs_examples.py b/tests/test_docs_examples.py index 7d9b6418..27ec4146 100644 --- a/tests/test_docs_examples.py +++ b/tests/test_docs_examples.py @@ -81,8 +81,9 @@ def _wait_for_proc( errmsg: str = err.decode(errors='replace') - # XXX, ALWAYS surface the subproc's full stderr - # whenever it exits non-zero! + # NOTE: always include captured stdout and stderr for a non-zero + # exit. Depending on the final stderr line previously hid grouped + # exception diagnostics; see GH #473. # # The prior impl only raised when the LAST stderr # line contained 'Error', swallowing any crash whose @@ -246,9 +247,8 @@ def run_example_in_subproc( str(script_file), ] - # XXX: BE FOREVER WARNED: if you enable lots of tractor logging - # in the subprocess it may cause infinite blocking on the pipes - # due to backpressure!!! + # Captured pipes are drained by `_wait_for_proc()` while the + # example runs. proc = testdir.popen( cmdargs, stdin=subprocess.PIPE, diff --git a/tractor/_root.py b/tractor/_root.py index a8316f49..28696918 100644 --- a/tractor/_root.py +++ b/tractor/_root.py @@ -102,8 +102,8 @@ async def _probe_registry( Confirm an address serves the Tractor actor handshake. Connection and handshake work share `timeout`; each attempt gets - `attempt_timeout`. Shielded channel cleanup may consume at most one - additional `close_timeout` after either deadline fires. + `attempt_timeout`. Shielded cleanup may add up to `close_timeout` + per attempted channel. ''' connected_once: bool = False @@ -525,12 +525,10 @@ async def open_root_actor( timeout: float = 3, ) -> None: ''' - Attempt temporary connection to see if a registry is - listening at the requested address by a tranport layer - ping. + Probe with a bounded Tractor actor handshake. - If a connection can't be made quickly we assume none no - server is listening at that addr. + Classify the address as a registrar, occupied by a + non-registrar, or absent. ''' probe_status = await _probe_registry( diff --git a/tractor/ipc/_uds.py b/tractor/ipc/_uds.py index f074820d..1986ce57 100644 --- a/tractor/ipc/_uds.py +++ b/tractor/ipc/_uds.py @@ -656,7 +656,7 @@ class MsgpackUDSStream(MsgpackTransport): case (bytes(), str()): sock_path: Path = Path(sockname) - # XXX, no-autobind case (macOS): the un-bound end + # NOTE, no-autobind case (macOS): the un-bound end # is `''`, NOT a `bytes` abstract-ns addr; taking # `peername` unconditionally (as prior impl did) # delivers garbage `Path('')` addrs on the accept diff --git a/tractor/runtime/_state.py b/tractor/runtime/_state.py index 154266bb..38b64260 100644 --- a/tractor/runtime/_state.py +++ b/tractor/runtime/_state.py @@ -332,9 +332,9 @@ def get_rt_dir( userspace apps stick their IPC and cache related system util-files. - On linux we use a `${XDG_RUNTIME_DIR}/tractor/` subdir by - default, but equivalents are mapped for each platform using - the lovely `platformdirs` lib. + Linux uses `${XDG_RUNTIME_DIR}/tractor/`; Darwin uses a short, + owner-only `/tmp/tractor-` path; other platforms use the + lovely `platformdirs` lib. ''' # lazy-imported to keep it off the eager diff --git a/tractor/spawn/_reap.py b/tractor/spawn/_reap.py index 5dfff2e7..09358bdd 100644 --- a/tractor/spawn/_reap.py +++ b/tractor/spawn/_reap.py @@ -35,14 +35,14 @@ Future-work TODO — authoritative UDS bind-addr tracking `unlink_uds_bind_addrs()` currently has two cleanup paths: 1. Explicit `bind_addrs` (when parent set them at spawn time) -2. **Convention-based reconstruction** — - `/tractor/@.sock` — for the +2. **Convention-based reconstruction** in the platform default UDS + bindspace — for the common case where the subactor self-assigned a random sock via `UDSAddress.get_random()`. -Path (2) hardcodes the `@.sock` convention from -`tractor.ipc._uds.UDSAddress`. If that convention ever -changes — or the subactor binds to a non-default +Path (2) delegates filename reconstruction to +`tractor.ipc._uds.UDSAddress.get_sockname()`. If the subactor binds to +a non-default `bindspace`/`filedir` — we'll silently fail to unlink. A more authoritative approach would be: @@ -105,7 +105,7 @@ def unlink_uds_bind_addrs( `_serve_ipc_eps` `finally:` block (which normally calls `os.unlink(addr.sockpath)`) never runs. Without this parent-side cleanup, the dead subactor's - `${XDG_RUNTIME_DIR}/tractor/@.sock` file + platform-default UDS socket file accumulates on the filesystem (see issue #454 + the autouse `_track_orphaned_uds_per_test` fixture). @@ -119,7 +119,7 @@ def unlink_uds_bind_addrs( picked its own random sock via `UDSAddress.get_random()`), reconstruct the path from `(subactor.aid.name, proc.pid)` using the - same `@.sock` convention. We can do this + same `UDSAddress.get_sockname()` helper. We can do this because the subactor uses its OWN `os.getpid()` at bind time, which equals `proc.pid` from the parent's view.