Skip to content

Fix jamulus/pollServerList reporting success on invalid directory address#3820

Open
mcfnord wants to merge 2 commits into
jamulussoftware:mainfrom
mcfnord:fix-3818-directory-socket-error
Open

Fix jamulus/pollServerList reporting success on invalid directory address#3820
mcfnord wants to merge 2 commits into
jamulussoftware:mainfrom
mcfnord:fix-3818-directory-socket-error

Conversation

@mcfnord

@mcfnord mcfnord commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #3818

The jamulus/pollServerList RPC handler set response["error"] when the directory address failed to parse, but then unconditionally set response["result"] = "ok" after the if/else. As a result, an invalid socket address was reported as success, with both "error" and "result" present in the response.

if ( NetworkUtil::ParseNetworkAddress ( ..., false ) )
{
    pClient->CreateCLReqServerListMes ( haDirectoryAddress );
    response["result"] = "ok";
}
else
{
    response["error"] = CRpcServer::CreateJsonRpcError ( ... );
}

response["result"] = "ok";   // <-- always ran, clobbering the error case

Fix

Remove the stray unconditional assignment. The success branch already sets result = "ok", so the handler now reports error on failure and result on success — matching the error-path idiom used by every other handler in this file.

Testing

  • clang-format-14 clean
  • Headless build + server smoke test (clean startup, clean SIGTERM)

The jamulus/pollServerList RPC handler set response["error"] when the
directory address failed to parse, but then unconditionally set
response["result"] = "ok" after the if/else, so an invalid socket
address was reported as success (with both "error" and "result" present
in the response).

Remove the stray unconditional assignment. The success branch already
sets result = "ok", matching the error-and-return idiom used by every
other handler in this file.

Fixes jamulussoftware#3818
Comment thread src/clientrpc.cpp

@ann0see ann0see left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI is probably still working...

@ann0see

ann0see commented Jul 21, 2026

Copy link
Copy Markdown
Member

Testing should obviously test the JSON-RPC api...

@ann0see ann0see added the AI AI generated or potentially AI generated label Jul 21, 2026
@mcfnord

mcfnord commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

AI is probably still working...

The Fable special ended, so today is Opus on X-High Effort. I saw a few examples today of Opus making small errors, but those probably indicate larger errors are more possible, including subtle errors. So in the most complicated instance, I asked Fable to review results.

@dingodoppelt
dingodoppelt requested a review from ann0see July 21, 2026 23:18
@mcfnord
mcfnord marked this pull request as ready for review July 24, 2026 17:45
@mcfnord

mcfnord commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

@ann0see The requested change is in d90fbb6: the handler now uses the guard-clause / early-return style of the other methods in this file — invalid address sets response["error"] and returns, then the success path sets response["result"] = "ok" unconditionally. Ready for re-review whenever you have a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI AI generated or potentially AI generated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing return statement in clientrpc error handling

3 participants