diff --git a/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.md b/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.md new file mode 100644 index 00000000..04696035 --- /dev/null +++ b/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.md @@ -0,0 +1,52 @@ +--- +created_at: 2026-10-07T16:54:53Z +generated_by: + harness: opencode + provider: openai + model: gpt-6.1-sol +git_ref: bde2457b +scope: code +substantive: true +raw_file: 20261007T165453Z_bde2457b_prompt_io.raw.md +--- + +## Prompt + +The user requested fixing all seven local PR94 review findings and +exporting the review for human inspection in Tuicr. They asked that a +Tractor pin adjustment be considered before adding Piker workarounds. + +## Response summary + +Correct the endpoint defaults, Click argument shape, daemon binding and +registrar option handling, chart fallback, and stray debugger pause. +Keep the absent datad implementation's test outside this landing +branch. Add offline startup coverage and export original findings +without modifying review states. The checked newer Tractor address +branch still rejects mixed transports, so leave the dependency pin. + +## Files changed + +- `config/conf.toml`: TCP-only default endpoints. +- `piker/cli/__init__.py`: repeatable and intact multiaddresses. +- `piker/service/_actor_runtime.py`: validate daemon listeners. +- `piker/service/_daemon.py`: initialize and consume registrar options. +- `piker/ui/cli.py`: fallback to automatic chart binding. +- `piker/brokers/binance/feed.py`: remove unconditional debugger entry. +- `tests/test_services.py`: defer test requiring missing datad API. +- `tests/test_startup.py`: CLI/config and service lifecycle regressions. + +## Human edits + +The human identified that datad tests arrived through the agent's +mixed cherry-picks and explicitly requested deferring that test. They +requested a chart default for existing configs, expanded the task to +all review findings, and directed checking whether a newer Tractor pin +could reduce the fix scope. No direct manual source edits were observed +during these changes; acceptance remains with the human. + +During the human review pass, the human confirmed removing the Binance +pause and correcting CLI endpoint parsing. They requested an explicit +template note about this pin's single-transport limitation and detailed +comments distinguishing registrar discovery endpoints from daemon bind +endpoints. Those explanations were added at the human's direction. diff --git a/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.raw.md b/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.raw.md new file mode 100644 index 00000000..a50ebaa4 --- /dev/null +++ b/ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.raw.md @@ -0,0 +1,61 @@ +--- +created_at: 2026-10-07T16:54:53Z +generated_by: + harness: opencode + provider: openai + model: gpt-6.1-sol +git_ref: bde2457b +diff_cmd: git diff HEAD~1..HEAD +--- + +## Prompt + +Fix all findings from the local PR94 review, prefer a Tractor pin +change if it reduces work, and export the original review for Tuicr. + +## Generated changes + +> `git diff HEAD~1..HEAD -- config/conf.toml` + +Use TCP-only defaults, retaining commented UDS alternatives. Both +checked Tractor address branch tips still reject mixed transports. + +> `git diff HEAD~1..HEAD -- piker/cli/__init__.py` + +Make Click multiaddresses repeatable and normalize single strings. + +> `git diff HEAD~1..HEAD -- piker/service/_actor_runtime.py` + +Validate requested daemon bind addresses instead of remote registrars. +Normalize accepted endpoints and allow TCP ephemeral port requests. + +> `git diff HEAD~1..HEAD -- piker/service/_daemon.py` + +Resolve explicit or inherited registrars independently of other daemon +options and consume the explicit keyword before forwarding kwargs. + +> `git diff HEAD~1..HEAD -- piker/ui/cli.py` + +Default chart bind endpoints to None when network config is absent. + +> `git diff HEAD~1..HEAD -- piker/brokers/binance/feed.py` + +Remove the unconditional market-info debugger pause. + +> `git diff HEAD~1..HEAD -- tests/test_services.py tests/test_startup.py` + +Remove the prematurely cherry-picked datad test and add offline startup +regressions, including a real registrar/child daemon lifecycle check. + +## Verification + +- Startup and logging tests: 13 passed. +- Existing runtime boot and tagged registrar tests: 2 passed. +- Scoped native Ruff and diff checks pass. +- The first new actor test used a removed nursery API; it was corrected + to the installed start_actor/Portal.run interface before passing. +- Import initially failed without native libraries. Tests use the + existing flake's GCC, zlib, Qt and PyQt paths without executing its + environment-rebuilding shell hook. +- Original seven findings exported locally as Tuicr review-add payloads + bound to bde2457b; no session comments or remote PR were changed. diff --git a/config/conf.toml b/config/conf.toml index 8d208e34..504c569d 100644 --- a/config/conf.toml +++ b/config/conf.toml @@ -1,14 +1,17 @@ [network] +# This Tractor pin supports one transport per actor, not mixed TCP/UDS. +# Use the single TCP endpoint shown in each list for this version. +# Mixed-transport configs need a future Tractor pin with that support. pikerd = [ # TCP localhost loopback '/ip4/127.0.0.1/tcp/6116', - # same but UDS - '/unix/run/user/1000/piker/pikerd.sock', + # UDS alternative: replace TCP and select the UDS transport. + # '/unix/run/user/1000/piker/pikerd.sock', ] chart = [ '/ip4/127.0.0.1/tcp/3003', - '/unix/run/user/1000/piker/chart.sock', + # '/unix/run/user/1000/piker/chart.sock', ] # the service-actor registry endpoint; # other pikerd trees contact this to discover diff --git a/piker/brokers/binance/feed.py b/piker/brokers/binance/feed.py index 75f06a14..072af2ae 100644 --- a/piker/brokers/binance/feed.py +++ b/piker/brokers/binance/feed.py @@ -364,8 +364,6 @@ async def get_mkt_info( if not mkt_mode: mkt_mode: str = f'{venue_lower}_futes' - await tractor.pause() - async with open_cached_client( 'binance', ) as client: diff --git a/piker/cli/__init__.py b/piker/cli/__init__.py index dec11271..d456c613 100644 --- a/piker/cli/__init__.py +++ b/piker/cli/__init__.py @@ -47,7 +47,7 @@ log = get_logger('piker.cli') def load_trans_eps( network: dict|None = None, - maddrs: list[str]|None = None, + maddrs: str|tuple[str, ...]|list[str]|None = None, ) -> dict[str, list]: ''' @@ -60,6 +60,8 @@ def load_trans_eps( return parse_endpoints(network) elif maddrs: + if isinstance(maddrs, str): + maddrs = [maddrs] return parse_endpoints( {'pikerd': list(maddrs)} ) @@ -87,11 +89,11 @@ def load_trans_eps( @click.option( '--maddrs', '-m', - default=None, + multiple=True, help='Multiaddrs to bind or contact', ) def pikerd( - maddrs: list[str]|None, + maddrs: tuple[str, ...], loglevel: str, tl: bool, pdb: bool, diff --git a/piker/service/_actor_runtime.py b/piker/service/_actor_runtime.py index 69fe7cca..205ed7e3 100644 --- a/piker/service/_actor_runtime.py +++ b/piker/service/_actor_runtime.py @@ -214,7 +214,27 @@ async def open_pikerd( tractor.trionics.collapse_eg(), trio.open_nursery() as service_tn, ): - for addr in reg_addrs: + # `reg_addrs` locates the registrar used for actor discovery; + # `tpt_bind_addrs` requests this daemon's own IPC listeners. + # An existing registrar can live elsewhere, so its endpoints + # must not be checked against `root_actor.accept_addrs`. + bind_addrs = tpt_bind_addrs + if bind_addrs is None: + # Only a registrar actor owns the discovery endpoints. + # A client with no explicit bind request lets Tractor pick + # its listener; there is no caller-specified bind to check. + bind_addrs = ( + reg_addrs if root_actor.is_registrar else [] + ) + + # `root_actor.accept_addrs` contains the actual bound endpoints. + # Normalize both sides with Tractor's address API so legacy + # pairs and tagged transport tuples compare in the same form. + accept_addrs = [ + tractor.discovery._addr.wrap_address(addr).unwrap() + for addr in root_actor.accept_addrs + ] + for addr in bind_addrs: # normalize to a wrapped `tractor` addr-type; # entries may be raw `tuple`s when passed in # from (test) client code. @@ -222,9 +242,16 @@ async def open_pikerd( addr, ) uaddr: tuple = wladdr.unwrap() - if ( - uaddr not in root_actor.accept_addrs - ): + bound = uaddr in accept_addrs + if wladdr.proto_key == 'tcp' and uaddr[-1] == 0: + # TCP port zero asks the OS for an ephemeral listener. + # Its actual port differs from the request, but the + # tagged transport and host must still match exactly. + bound = any( + actual[:-1] == uaddr[:-1] + for actual in accept_addrs + ) + if not bound: raise RuntimeError( f'`pikerd` failed to bind on {addr}!\n' 'Maybe you have another daemon already running?' diff --git a/piker/service/_daemon.py b/piker/service/_daemon.py index d5b4a5f1..94a5d2f0 100644 --- a/piker/service/_daemon.py +++ b/piker/service/_daemon.py @@ -78,14 +78,19 @@ async def maybe_spawn_daemon( lock = Services.locks[service_name] await lock.acquire() - if not pikerd_kwargs: - # XXX NOTE, pin to apprope `tractor` branch! - rtvs: dict = tractor.get_runtime_vars() - registry_addrs: list[tuple] = list( - map(tuple, rtvs['_registry_addrs']) - ) - try: + # Discovery and `maybe_open_pikerd()` must use the same + # registrar list, independently of other runtime options. + # Consume an override here: startup forwards it explicitly + # below, so leaving it in `pikerd_kwargs` duplicates the keyword. + registry_addrs = pikerd_kwargs.pop('registry_addrs', None) + if registry_addrs is None: + # Use the registrar addresses inherited by this actor. + rtvs: dict = tractor.get_runtime_vars() + registry_addrs: list[tuple] = list( + map(tuple, rtvs['_registry_addrs']) + ) + async with find_service( service_name, registry_addrs=registry_addrs, diff --git a/piker/ui/cli.py b/piker/ui/cli.py index fec29afe..78981215 100644 --- a/piker/ui/cli.py +++ b/piker/ui/cli.py @@ -199,6 +199,7 @@ def chart( 'registry_addrs', [], ) + chart_eps: list|None = None network: dict = conf.get('network') if network: eps: dict = load_trans_eps( diff --git a/tests/test_services.py b/tests/test_services.py index ce0d490a..7b2f2a7a 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -120,41 +120,6 @@ def test_registry_normalizes_tagged_addresses( _registry.Registry.addrs = [] -def test_datad_spawn( - open_test_pikerd: AsyncContextManager, - loglevel: str, - -) -> None: - ''' - Verify the new (data-feed-only) `datad.` daemon - can be spawned/registered as a `pikerd` sub-service via - the `maybe_spawn_datad()` factory. - - ''' - from piker.service import maybe_spawn_datad - - backend: str = 'kraken' - datad_name: str = f'datad.{backend}' - - async def main(): - async with ( - open_test_pikerd() as (_, _, _, services), - - maybe_spawn_datad( - backend, - loglevel=loglevel, - ) as portal, - ): - assert portal - - async with ensure_service(datad_name): - assert ( - datad_name in services.service_tasks - ) - - trio.run(main) - - def test_ensure_datafeed_actors( open_test_pikerd: AsyncContextManager, loglevel: str, diff --git a/tests/test_startup.py b/tests/test_startup.py new file mode 100644 index 00000000..5e6ce03e --- /dev/null +++ b/tests/test_startup.py @@ -0,0 +1,214 @@ +''' +Offline regressions for endpoint configuration and service startup. + +''' +from contextlib import asynccontextmanager as acm +from pathlib import Path +import sys +import tomllib +from types import ModuleType + +from click.testing import CliRunner +import pytest +import tractor +import trio + +from piker import config +from piker.cli import load_trans_eps, pikerd +from piker.service import _daemon, _actor_runtime + + +@pytest.mark.parametrize('repeat', [False, True]) +def test_maddrs_click_collection(repeat): + ''' + Click previously produced a string for `--maddrs`, which endpoint + loading split into individual characters. Parse one or repeated + options through Click and then the real Tractor parser, without + entering the indefinitely running daemon callback. Every address + must survive intact and in the declared order. + + ''' + addresses = ['/ip4/127.0.0.1/tcp/6116'] + if repeat: + addresses.append('/ip4/127.0.0.1/tcp/6117') + args = [ + arg for address in addresses for arg in ['--maddrs', address] + ] + with pikerd.make_context('pikerd', args) as ctx: + endpoints = load_trans_eps(maddrs=ctx.params['maddrs']) + assert [addr.unwrap() for addr in endpoints['pikerd']] == [ + ('tcp', '127.0.0.1', 6116 + index) + for index in range(len(addresses)) + ] + + +def test_single_string_multiaddr(): + ''' + Direct endpoint callers may provide one string, independently of + Click's collection conversion. That input must denote a single + endpoint rather than a sequence of invalid character endpoints. + + ''' + endpoints = load_trans_eps(maddrs='/ip4/127.0.0.1/tcp/6116') + assert endpoints['pikerd'][0].unwrap() == ( + 'tcp', '127.0.0.1', 6116, + ) + + +def test_template_single_transport(): + ''' + The shipped template enabled TCP and UDS together although the + locked Tractor runtime permits only one transport per actor. + Parse the real template and ensure both daemon and chart defaults + use the TCP transport selected by their startup paths. + + ''' + path = Path(__file__).parents[1] / 'config' / 'conf.toml' + conf = tomllib.loads(path.read_text()) + endpoints = load_trans_eps(conf['network']) + for name in ['pikerd', 'chart']: + assert endpoints[name] + assert {addr.proto_key for addr in endpoints[name]} == {'tcp'} + + +@pytest.mark.parametrize('network', [None, {}]) +def test_chart_without_network(monkeypatch, network): + ''' + Existing configs without a populated network table left chart + bind endpoints unbound, crashing before the UI entrypoint. Invoke + the actual Click callback with absent and empty tables, replacing + only config IO and the UI entrypoint. Automatic binding must be + passed as None while preserving the configured registrar address. + + ''' + from piker.ui.cli import chart + + conf = {} if network is None else {'network': network} + monkeypatch.setattr(config, 'load', lambda **_: (conf, None)) + app = ModuleType('piker.ui._app') + calls = [] + app._main = lambda **kwargs: calls.append(kwargs) + monkeypatch.setitem(sys.modules, 'piker.ui._app', app) + result = CliRunner().invoke( + chart, + ['btcusdt.binance'], + obj={ + 'pdb': False, + 'brokermods': [object()], + 'tractorloglevel': 'warning', + 'loglevel': 'warning', + 'registry_addrs': [('127.0.0.1', 6116)], + }, + ) + assert result.exit_code == 0, result.exception + runtime = calls[0]['tractor_kwargs'] + assert runtime['tpt_bind_addrs'] is None + assert runtime['registry_addrs'] == [('127.0.0.1', 6116)] + + +@pytest.mark.parametrize('explicit_registry', [False, True]) +def test_daemon_options_resolve_registrars( + monkeypatch, + explicit_registry, +): + ''' + Any nonempty pikerd kwargs previously skipped registrar address + initialization. Exercise the full discovery and spawn path with + runtime options, using protocol-boundary fakes instead of remote + actors. An explicit override must reach discovery and startup + exactly once; otherwise the inherited registrar list is used. + The service lock must be released after leaving the context. + + ''' + inherited = [('tcp', '127.0.0.1', 6116)] + override = [('tcp', '127.0.0.1', 6117)] + expected = override if explicit_registry else inherited + calls = [] + portal = object() + + @acm + async def find(name, registry_addrs): + calls.append(('find', registry_addrs)) + yield None + + @acm + async def open_root(**kwargs): + calls.append(('open', kwargs)) + yield None + + @acm + async def wait(name): + yield portal + + async def start(**kwargs): + return True + + monkeypatch.setattr(_daemon, 'find_service', find) + monkeypatch.setattr(_daemon, 'maybe_open_pikerd', open_root) + monkeypatch.setattr(tractor, 'wait_for_actor', wait) + monkeypatch.setattr( + tractor, + 'get_runtime_vars', + lambda: {'_registry_addrs': inherited}, + ) + + async def main(): + lock = trio.Lock() + monkeypatch.setitem(_daemon.Services.locks, 'probe', lock) + kwargs = {'debug_mode': False} + if explicit_registry: + kwargs['registry_addrs'] = override + async with _daemon.maybe_spawn_daemon( + 'probe', start, {}, **kwargs, + ) as found: + assert found is portal + assert not lock.locked() + + trio.run(main) + assert calls[0] == ('find', expected) + assert calls[1][1]['registry_addrs'] == expected + assert calls[1][1]['debug_mode'] is False + + +async def _check_distinct_daemon_bind(): + ''' + Open Piker's service context on a non-registrar child actor. + + ''' + actor = tractor.current_actor() + assert not actor.is_registrar + assert actor.accept_addrs != actor.reg_addrs + async with _actor_runtime.open_pikerd( + registry_addrs=actor.reg_addrs, + tpt_bind_addrs=[('127.0.0.1', 0)], + ): + return actor.accept_addrs + + +def test_daemon_bind_distinct_from_registrar(reg_addr): + ''' + Separate daemon and registrar endpoints were rejected because + startup checked registrar addresses against daemon listeners. + Run a real TCP registrar and a distinct child daemon with an + ephemeral listener. Its service context must start and unwind + without taking ownership of the registrar's listening address. + + ''' + async def main(): + async with ( + tractor.open_root_actor( + registry_addrs=[reg_addr], + enable_transports=['tcp'], + ), + tractor.open_nursery() as nursery, + ): + portal = await nursery.start_actor( + 'daemon_probe', + enable_modules=[__name__], + bind_addrs=[('127.0.0.1', 0)], + ) + addresses = await portal.run(_check_distinct_daemon_bind) + assert addresses + await nursery.cancel() + + trio.run(main)