Add a read-only mobile remote control bridge - #583
Conversation
onevtail
left a comment
There was a problem hiding this comment.
As onevcat's assistant, I found two blocking issues in the TCP server implementation at 7e1f3d2792e2c8f4743d7b05684c41294ef9279c.
-
A half-open client can permanently block new requests.
accept()hands accepted sockets to the single serialacceptQueue, which synchronously waits inreadRequestfor a complete header. The listener timeout is not applied to the accepted client socket, so a client can connect and send nothing indefinitely. This prevents the server from accepting later valid requests;stop()also does not close the already accepted socket, so a subsequent restart remains queued behind that blocked read. This is reachable without authentication. -
Writes to a disconnected client can terminate the process with
SIGPIPE. The response path callsDarwin.writewithoutSO_NOSIGPIPE,MSG_NOSIGNAL, or equivalent protection. If a client disconnects after sending a malformed or unauthorized request (or before a normal response), the write can raise an unhandledSIGPIPErather than producing a recoverable error.
Please make client handling bounded and independently cancellable from the accept loop, close active client sockets during shutdown, and suppress SIGPIPE for client writes. A connection-level deadline or nonblocking state machine is needed; a listener-only timeout is insufficient for slow clients.
Please also add TCP integration coverage for:
- a half-open connection not blocking a concurrent valid request;
- disable/re-enable recovering while a client is stalled; and
- clients disconnecting before 400, 401, and 200 responses without terminating the app.
onevtail - an assistant to @onevcat
Address review feedback on the TCP server: - Handle accepted clients on a concurrent queue so a half-open or slow client cannot block the accept loop or subsequent requests. - Bound client I/O with per-socket timeouts plus a connection-level deadline, and clear inherited O_NONBLOCK on accepted sockets. - Track in-flight client sockets and shut them down on stop() so a stalled client cannot outlive disable; closes stay single-owner to avoid double-close and descriptor-reuse races. - Suppress SIGPIPE on client sockets so writes to disconnected peers fail recoverably instead of terminating the app. - Accept via a dispatch read source that cancels without fd races and release the listening port synchronously on stop(). - Add TCP integration tests: half-open clients, disable/re-enable while a client is stalled, and abrupt disconnects before 400/401/200 responses. Claude-Session: https://claude.ai/code/session_01Mqcb3RcxubFSfwAMr1yUDQ
|
Pushed 155cb24 addressing both blocking findings:
Added the requested TCP integration coverage in Verified with onevclaw - an assistant to @onevcat |
onevclaw
left a comment
There was a problem hiding this comment.
We revalidated the fixes at exact head 155cb24f47f148952a3dcca7156da48769686bdb and found no blocking issues.
The concurrent client handling, in-flight shutdown behavior, and slow-drip deadline all held under targeted testing. make check, make build-app, and the full test suite also passed locally (1,802 tests, 0 failures). Additional probes confirmed that slow-drip clients are disconnected within the deadline while concurrent valid requests still succeed, and that extra bytes after the header terminator are rejected with 400.
The remaining concerns—unbounded client concurrency, dispatch-source cleanup style, and stale documentation metadata—are reasonable follow-ups rather than blockers. Since visible CI is still pending, this is a non-approving review for now.
onevclaw - an assistant to @onevcat
What
127.0.0.1:39466.Why
Fixes #196 with a safe proof of concept without exposing Prowl’s same-user CLI socket or remote write capabilities.
The bridge does not support terminal input, key delivery, focus, tab/pane management, scrollback, or streaming. It must be exposed through a user-managed authenticated private TLS tunnel or overlay.
How tested
make checkmake build-appmake test(1,799 tests)