Add Windows Named Pipe transport - #3
Conversation
ltlly
left a comment
There was a problem hiding this comment.
Thanks for this! I reviewed the diff and ran the test suite on Linux. Overall the design is solid (token auth, pipe-name validation, explicit platform errors), but I found two issues to address before merge:
1. tests/test_bridge.py::test_explicit_transport_environment_overrides_gui_setting fails on Linux (breaks CI)
It sets BN_BRIDGE_TRANSPORT=pipe without patching platform.system(). On Linux bridge_transport() raises ValueError: BN_BRIDGE_TRANSPORT=pipe is only supported on Windows, so the test errors out:
258 passed, 2 skipped, 1 failed
FAILED tests/test_bridge.py::test_explicit_transport_environment_overrides_gui_setting
ValueError: BN_BRIDGE_TRANSPORT=pipe is only supported on Windows
Fix options: use BN_BRIDGE_TRANSPORT=tcp (valid on all platforms and still proves "explicit env overrides GUI setting"), or add monkeypatch.setattr("bn.paths.platform.system", lambda: "Windows").
2. _load_instance purges the registry on transient pipe errors
In src/bn/transport.py (_load_instance, the probe block around L232-239), any probe error other than DENIED is treated as stale and calls _purge_stale_registry. But for pipes, ERROR_SEM_TIMEOUT (121) and ERROR_PIPE_BUSY (231) are transient — the same codes _send_request_to_instance retries via TRANSIENT_PIPE_WINERRORS. The probe timeout is 0.2s, so when the bridge is briefly busy, list_instances() will delete the registry file and the CLI reports "No running bridge instances found" even though the daemon is still alive (registry is only written once at start). Suggest keeping the instance and recording the probe error for winerror in TRANSIENT_PIPE_WINERRORS, and only purging on terminal errors like 2/53.
Minor / nits
backlog=64is a no-op for the AF_PIPE listener — CPython'sPipeListenerignoresbacklog(instances arePIPE_UNLIMITED_INSTANCES). Maybe drop it or add a comment.- Unbounded per-connection threads +
PIPE_UNLIMITED_INSTANCES+ a fixed well-known pipe name means a local process can open connections that never send data and pin bridge threads onrecv_bytes(). Arecvtimeout or max-connection cap would harden this. (Pre-existing pattern on TCP, but worth noting for pipes.) _set_gui_port_commandcalls_configured_transport("gui")without thetry/except ValueErrorthat_restart_bridge_commandhas — an invalidBN_BRIDGE_TRANSPORTwould raise unhandled in the plugin command callback.- Please smoke-test the
enum/enumDescriptions/requiresRestartschema keys against a real BN install —_FakePluginSettingscan't catch schema rejection.
Also, thanks for the _fake_headless_instance BN_CACHE_DIR fix — it fixes a real test-isolation issue on master (I hit it locally with a stale registry).
Everything else looks good: the _process_bridge_request refactor also fixes the pre-existing crash when a payload is a JSON list, and the pipe-name validation correctly rejects remote/nested names.
Summary
Tests