Skip to content

fix(java): stop logging the legacy 'connect' probe failure as a warning - #2310

Open
rinceyuan wants to merge 1 commit into
github:mainfrom
rinceyuan:fix/java-connect-probe-log-level
Open

fix(java): stop logging the legacy 'connect' probe failure as a warning#2310
rinceyuan wants to merge 1 commit into
github:mainfrom
rinceyuan:fix/java-connect-probe-log-level

Conversation

@rinceyuan

Copy link
Copy Markdown
Contributor

Summary

CopilotClient.verifyProtocolVersion probes the connect RPC and falls back to ping when the server does not implement it. That fallback works, but JsonRpcClient.invoke logged every failed request at WARNING with a full stack trace, so the recovered probe printed this on a plain tutorial run (JUL sends WARNING to the console by default):

WARNUNG: JsonRpc.invoke JSON-RPC request finished. Elapsed=PT0.793S, Method=connect, RequestId=1, Status=Failed
java.util.concurrent.CompletionException: com.github.copilot.JsonRpcException: Unhandled method connect

Change

  • JsonRpcClient.invoke gets an internal (package-private) overload that takes the level used for failure logging; the existing 3-arg overload still defaults to WARNING.
  • The protocol-negotiation connect probe passes Level.FINE, since CopilotClient already catches and recovers from that exact failure.

No public API change - JsonRpcClient is package-private. Genuinely unexpected RPC failures still log at WARNING.

Tests

Two cases added to JsonRpcClientTest, both driving a real socket pair and a -32601 Unhandled method connect error response while capturing the JUL records:

  • testInvokeLogsFailureAtWarningByDefault - unexpected failures are still WARNING.
  • testInvokeHonorsCustomFailureLogLevel - an expected/recovered failure produces nothing at WARNING or above.

Verified testInvokeHonorsCustomFailureLogLevel fails when the level argument is ignored.

mvnw.cmd test -Dtest=JsonRpcClientTest -Denforcer.skip=true -Pskip-test-harness   # 25/25 pass
mvnw.cmd spotless:check -DspotlessFiles=... ; mvnw.cmd checkstyle:check           # clean

Fixes #2291.

Copilot AI balanced review requested due to automatic review settings August 11, 2026 03:59
@rinceyuan
rinceyuan requested a review from a team as a code owner August 11, 2026 03:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds configurable RPC failure logging to suppress warnings for legacy connect probes.

Changes:

  • Adds an internal failure-log-level overload.
  • Uses FINE for protocol negotiation.
  • Adds logging-level tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
CopilotClient.java Downgrades connect probe failures.
JsonRpcClient.java Adds configurable failure logging.
JsonRpcClientTest.java Tests default and custom levels.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +323 to +324
var connectResponse = connection.rpc.invoke("connect", connectParams, ConnectResult.class, Level.FINE)
.get(30, TimeUnit.SECONDS);
@rinceyuan
rinceyuan force-pushed the fix/java-connect-probe-log-level branch from 313992a to ddfaecf Compare August 14, 2026 02:30
@rinceyuan

Copy link
Copy Markdown
Contributor Author

Good catch - fixed.

The level is no longer decided up front. JsonRpcClient.invoke now takes a Predicate<Throwable> describing the failure the caller recovers from, evaluates it against the unwrapped cause inside exceptionally, and logs FINE only on a match. Everything else - -32603, deserialization failures, I/O failures, timeouts - still logs at WARNING.

The probe passes exactly the condition verifyProtocolVersion recovers from, so the two can no longer drift:

cause -> cause instanceof JsonRpcException rpcEx && isUnsupportedConnectMethod(rpcEx)

Added testInvokeKeepsWarningForUnexpectedFailure, which drives a -32603 through the same predicate and asserts a WARNING is still emitted. It fails against the previous revision.

mvnw test -Dtest=JsonRpcClientTest 26/26, spotless and checkstyle clean.

CopilotClient probes the 'connect' RPC and falls back to 'ping' when the server does not implement it. JsonRpcClient.invoke logged every failed request at WARNING with a stack trace, so this fully recovered probe printed a scary 'Unhandled method connect' trace on every startup under the JUL default console handler.

Give invoke an internal overload that takes the level used for failures and have the protocol-negotiation probe pass FINE. Unexpected failures still log at WARNING.

Fixes github#2291.
@rinceyuan
rinceyuan force-pushed the fix/java-connect-probe-log-level branch from ddfaecf to 134f55d Compare August 14, 2026 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[java] Warning logged "Unhandled method connect" during the execution of the tutorial

2 participants