Add JSON-RPC methods to connect, disconnect and query connection state

[mcfnord]

Short description of changes

Stacked on #3805 — please review only the last commit here until that one merges.

Adds JSON-RPC control over the client's connection, as a symmetric consumer of the CClient connection state machine introduced in #3805. This closes the largest UI/RPC parity gap named in #3801 (join/leave) and makes a --nogui client fully scriptable — e.g. a local web frontend where clicking a server joins it.

New methods:

• jamulusclient/connect {"address": "host:port", "serverName": "optional display name"} — terminates any current connection first, then connects (asynchronously; progress arrives via notifications).
• jamulusclient/disconnect — idempotent.
• jamulusclient/getConnectionState — returns {state: disconnected|connecting|connected, serverName}.

New notifications: jamulusclient/connecting, jamulusclient/connectingFailed, jamulusclient/connectionStateChanged. Together with the existing connected/disconnected notifications, an RPC consumer can follow the full lifecycle.

Context: Fixes an issue?

Toward #3801 (full UI/RPC parity). Complementary to #3660 (directory getters).

Does this change need documentation? What needs to be documented and how?

docs/JSON-RPC.md regenerated via tools/generate_json_rpc_docs.py (included).

Status of this Pull Request

Draft until #3805 merges; the RPC commit itself is ready for review.

What is missing until this pull request can be merged?

#3805, then review.

Tested end-to-end: --nogui client + local server on Linux/Qt 5.15, driven entirely over JSON-RPC. Scripted checks (16/16 pass): initial state disconnected; connect → connecting notification → connectionStateChanged: connecting → connected (channel ID) → connectionStateChanged: connected → getConnectionState returns server name; connect-while-connected performs a clean reconnect (disconnected → connecting → connected); disconnect → disconnected + connectionStateChanged: disconnected, serverName cleared; double disconnect is a no-op.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

AUTOBUILD: Please build all targets

🤖 Generated with Claude Code

[mcfnord]

Rebased this branch onto #3815 and added an optional directory parameter to jamulusclient/connect.

When directory is supplied, the client hole-punches through that directory (sends CLM_REQ_SERVER_LIST, which makes the directory poke its registered servers toward our socket) before connecting — giving RPC-driven clients the same reachability the GUI directory list has, for servers behind a cloud firewall/NAT. The server address is connected to verbatim and need not be listed by the directory. A non-string directory is rejected with invalid params. docs/JSON-RPC.md was regenerated via tools/generate_json_rpc_docs.py.

The stack is now #3805 → #3815 (--connectdirectory) → this PR, so please review those first. Verified locally: jamulusclient/connect with a directory sends the punch to the directory then connects; without it, behaviour is unchanged; a bogus directory type returns -32602.

Relates to jamulussoftware/jamuluswebsite#1122.

[ann0see]

Please rebase.

[ann0see]

@mcfnord please rebase this. CC @digable1

[ann0see]

Also, I think the directory option should be part of a new PR, not this one.

[mcfnord]

🤖 AI: The directory option is out of this PR — --connectdirectory and the directory parameter on jamulusclient/connect are removed, leaving connect, disconnect and getConnectionState. The branch is also rebased onto current main; #3805 merging accounted for every conflict.

Taking those commits out was treated as cleanup of this PR rather than as opening a new one, so nothing new was opened while the moratorium runs. If it belongs in its own PR now instead, that can be done.

[ann0see]

maybe requestConnection would be better - as this usually returns ok.

[dingodoppelt]

I think this one status message is enough. The notifications are redundant and I don't see why people would only subscribe to for example jamulusclient/connecting. Why wouldn't you want to know about the other states?

[dingodoppelt]

connecting is already part of jamulusclient/connectionStateChanged. Do we need both?

[dingodoppelt]

Could this be refactored into jamulusclient/connectionStateChanged so we don't need to subscribe to multiple endpoints for the same info?

[ann0see]

🤖 AI finding: jamulusclient/connect / disconnect crash the client

I've been stress-testing the client RPC methods. Reliable crash, verified on this branch (ASan build, dummy JACK).

Repro

Run a client with JSON-RPC enabled, keep a server reachable at the address below, then run this (Python 3, no deps):

import socket, json, time

HOST, PORT, SECRET = "127.0.0.1", 22134, "supersecret1234567890"
ADDRESS = "localhost:22124"

def fire(method, params):
    s = socket.create_connection((HOST, PORT), timeout=5)
    s.sendall((json.dumps({"jsonrpc":"2.0","id":1,"method":"jamulus/apiAuth",
                           "params":{"secret":SECRET}})+"\n").encode())
    s.sendall((json.dumps({"jsonrpc":"2.0","id":2,"method":method,
                           "params":params})+"\n").encode())
    s.close()   # fire-and-forget: close without reading the response

# Crash 1: two overlapping connects
fire("jamulusclient/connect", {"address": ADDRESS})
time.sleep(0.003)
fire("jamulusclient/connect", {"address": ADDRESS})

# Crash 2 (same bug): disconnect while the connect is still in flight
# fire("jamulusclient/connect", {"address": ADDRESS})
# time.sleep(0.003)
# fire("jamulusclient/disconnect", {})
# time.sleep(0.003)
# fire("jamulusclient/disconnect", {})

The overlap is the point: each request connects, sends connect/disconnect, and closes immediately — before the client finished handling it. 3 ms apart is enough. I ran crash 1 several times, every run killed the client (SIGSEGV). A single request alone is fine. 8 parallel connections doing connect/disconnect/getConnectionState do it in <10 s.

Why

Connect()/Disconnect() run synchronously in the RPC handler and both go through CClient::Stop() (client.cpp:1122), which runs the event loop ~100 ms — while the RPC readyRead handler is still on the stack. If the peer closes its socket in that window, deleteLater() destroys it and the suspended handler writes to the freed socket in CRpcServer::Send:

#0  QIODevice::write            SEGV (use-after-free)
#2  CRpcServer::Send            rpcserver.cpp:203
#3  readyRead lambda            rpcserver.cpp:189
#17 CClient::Stop()             client.cpp:1122
#20 connect handler             clientrpc.cpp:280
#40 CClient::Stop()             client.cpp:1122  (nested)

Server-side RPC is unaffected (its handlers never call Stop()).

Smaller things I noticed while reviewing

• connect returns "ok" even for an invalid address (FixAddress only strips spaces; a host without a port or a garbage string fails later and surfaces as connectingFailed). Callers need to subscribe to that notification to know it failed.
• serverName of the wrong type is silently ignored (falls back to address), while a non-string address errors out — inconsistent.
• connectionStateChanged duplicates the dedicated notifications: a connect emits connecting and connectionStateChanged: connecting, then connected and connectionStateChanged: connected (same for disconnected). Verified against the running client.

🤖 Used AI: big-pickle, opencode

[ann0see]

Fix idea

Root cause: Connect()/Disconnect() run synchronously in the RPC handler, and Stop() runs the event loop (client.cpp:1122) — re-entrant, so the socket can be freed mid-handler.

1. Defer the call (QTimer::singleShot(0, …))

   • Small diff, stops the UAF
     − Not enough alone: the timer can still fire while Stop() runs the event loop → recursion → stack overflow under churn

2. Re-entrancy guard — one connection operation at a time (reject concurrent ones)

   • Small diff
   • Held up in my testing (both repros ×50, 20 s 8-thread churn, ASan clean)
     − New error code (-32000) callers must handle
     − Mid-connect you can't disconnect to bail out

3. Don't run the event loop in Stop() — the actual root cause

The wait in Stop() is there so no audio packet is still in the network queue (client.cpp:1111):

QTime DieTime = QTime::currentTime().addMSecs ( 100 );
while ( QTime::currentTime() < DieTime )
{
    QCoreApplication::processEvents ( QEventLoop::ExcludeUserInputEvents, 100 );
}

For that it only needs to wait — it doesn't need to process events. processEvents is what lets the RPC readyRead in, so connect/disconnect runs again while we're still inside Stop(). A plain wait closes that window:

// wait for approx. 100 ms so no audio packet is still in the network queue
QThread::msleep ( 100 );

Nothing is lost: the disconnect message is only created after the wait (client.cpp:1130), queued events run once Stop() returns.

• Fixes the root cause, not the symptom
• Also protects any future handler that touches the connection
  − Bigger change; Stop() is called from the UI and network paths, so timing needs care
  − Short block freezes the UI for that window

So — to be clear — the fix itself is small: 3 lines, one function (CClient::Stop()). Not a refactor. It only needs care because Stop() is called from everywhere (UI, network, close, RPC). The code even notes the wait "seems not to gain much" (client.cpp:1113). The downside stays the ~100 ms UI block.

@ann0see probably prefers this — root cause, not a new error code.

🤖 Used AI: big-pickle, opencode

[ann0see]

For option 3 as "fix" - not sure. We'll probably need to iterate over those proposals...

Especially

The downside stays the ~100 ms UI block.

Is not nice.

[dingodoppelt]

For option 3 as "fix" - not sure. We'll probably need to iterate over those proposals...

Especially

The downside stays the ~100 ms UI block.

Is not nice.

A sleep still feels like a workaround. I haven't looked at the code yet, but I'd investigate a possible lock mechanism in case colliding request arrive.

[ann0see]

Yes. Had the same idea but the agent claimed that it's the wrong way to fix it (since it happens in the same thread) - to me this doesn't make too much sense - but it's very likely that I don't fully understand the codebase well enough.