Fix endpoint config and daemon startup from PR94 review
Keep the shipped `conf.toml` defaults TCP-only for this `tractor` pin, with an explicit single-transport note and commented UDS alternatives. Preserve repeated Click `--maddrs` options and treat a direct string as one endpoint. Deats, - Validate daemon bind requests against actual listeners, not registrar discovery endpoints; normalize tagged addresses and allow OS-assigned TCP ports. Explain registrar ownership and automatic client binding at the validation site. - Resolve explicit or inherited registrar addresses independently of other daemon kwargs, consuming overrides before forwarding them and keeping discovery and startup on the same registrar. - Default chart binding to `None` for absent or empty network config, and remove the unconditional Binance debugger pause. - Defer the prematurely cherry-picked `test_datad_spawn` until its service API lands. Add offline endpoint/startup regressions and a real distinct-registrar/daemon lifecycle check. Include the human-directed config and registrar/bind explanations and the direct prompt provenance pair from the review fix pass. Prompt-IO: ai/prompt-io/20261007T165453Z_bde2457b_prompt_io.md (this patch was generated in some part by `opencode` using `gpt-6.1-sol` (`openai`))wkt/land_ib_method_proxy
parent
bde2457bdd
commit
1fa45dfa98
|
|
@ -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.
|
||||
|
|
@ -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.
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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?'
|
||||
|
|
|
|||
|
|
@ -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!
|
||||
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'])
|
||||
)
|
||||
|
||||
try:
|
||||
async with find_service(
|
||||
service_name,
|
||||
registry_addrs=registry_addrs,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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.<broker>` 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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
Loading…
Reference in New Issue