fix(jsonrpc): normalize error responses and fatal handling - #6985
waynercheung wants to merge 1 commit into
Conversation
| throw new JsonRpcInternalException(e.getMessage()); | ||
| // Mapped errors bypass the resolver's unhandled-exception log, so record the complete | ||
| // cause once per failure episode at the lookup boundary. | ||
| if (chainIdentityLookupFailed.compareAndSet(false, true)) { |
There was a problem hiding this comment.
[SHOULD] Simplify the chain-identity logging state before extending it
This adds a dedicated failure/recovery state machine for one log point, although response sanitization and fatal propagation do not require that state. It also updates chainIdentityLookupFailed and logs before the resolver can inspect the cause chain: a RuntimeException wrapping VirtualMachineError or TronError is therefore treated as a recoverable outage before the actual fatal cause is rethrown. Concurrent success and failure calls can additionally make recovered describe one successful call rather than a stable recovery episode.
Suggestion: first confirm that repeated chain-identity failures cause a real log-flood problem. If not, remove the dedicated state and keep a simple sanitized failure path. If suppression is required, prefer a shared rate-limited logging mechanism; at minimum, classify and rethrow wrapped fatal causes before any state transition or log, with a regression test for that boundary.
There was a problem hiding this comment.
Thanks, confirmed with injected wrapped fatal causes: ethChainId logged and changed state before the resolver rethrew the original cause. I have not identified a production trigger on that lookup path.
Addressed in cd012e1d74:
- Fatal causes are now classified first and rethrown as the original instance, before any logging, first-occurrence tracking or wrapping.
- The failure/recovery flag and the recovery INFO are removed.
ethChainIdnow shares first-occurrence tracking with the resolver: the first occurrence per operation and exception type is logged at WARN with the Throwable, and repeats at DEBUG without the Throwable or message. Both chain-identity methods use one operation key. This is deduplication, not rate limiting; a later recurrence after recovery does not emit another WARN. LogBlockQueryhad the same ordering issue, since its executorFuturewraps a task-thrownErrorinExecutionException; it now checks for fatal causes before its WARN.findFatalCauseis public so that thefilterspackage can use the same classification.
Tests cover: a wrapped or direct fatal cause rethrown as the same instance with no log event at any level; a fatal cause not consuming the first WARN (both calls share the outer exception type); first-occurrence WARN and DEBUG repeats, across a recovery on the real null-block path and shared with net_version; and, for LogBlockQuery, a fatal cause both from a mocked Future and through a real executor's FutureTask.
| // JsonRpcServer.handle catches Throwable and would swallow fatal errors rethrown by the | ||
| // resolver. Use the lower-level entry point so single and batch requests share a boundary. | ||
| rpcServer.handleRequest(new ByteArrayInputStream(body), bufferedResp.getOutputStream()); | ||
| } catch (RuntimeException | IOException | Error e) { |
There was a problem hiding this comment.
[QUESTION] Should unclassified Error types be recoverable?
This catch handles every Error, while rethrowIfFatal propagates only VirtualMachineError, ThreadDeath, LinkageError, and TronError. Every other subtype, including AssertionError and potentially library or JVM invariant failures, is converted into a normal -32603 response. The interceptor AssertionError case is deliberate and tested, but that does not establish that every non-allowlisted Error is safe to continue from.
Suggestion: consider propagating Error by default and explicitly allow only the narrow recoverable subtype(s) required here, or document why the current fatal allowlist is complete for this boundary.
There was a problem hiding this comment.
Agreed: these four categories are a propagation policy, not proof that every other Error is safe to recover from.
The compatibility reason is that method-invocation Errors already enter jsonrpc4j's error-response path (on develop this includes OutOfMemoryError, answered as -32001 with the class name). As @lxcmyf suggested in #6941 (#6941 (comment)), this PR applies the same classification to Errors escaping dispatch. Changing only the servlet would recreate the method/interceptor inconsistency; changing both would alter method-level responses and batch continuation.
I propose retaining this policy here, and I have documented the trade-off in the PR description under "Fatal classification". Errors without a classified fatal cause follow the existing response-suppression and batch-recovery rules, using a sanitized -32603 where an internal-error response is emitted. This reports a failed call, not a healthy or recovered node.
@bladehan1, does keeping this policy with the trade-off documented address your concern? @lxcmyf, since it follows your #6941 suggestion, please weigh in if you see it differently. A stricter default would need a coordinated change across the resolver, the dispatch catches, the cause-chain policy and the compatibility tests, so I would prefer to keep it out of this PR unless you both think it belongs here.
| try { | ||
| rpcServer.handle(cachedReq, bufferedResp); | ||
| } catch (RuntimeException e) { | ||
| // JsonRpcServer.handle catches Throwable and would swallow fatal errors rethrown by the |
There was a problem hiding this comment.
[DISCUSS] Keep independent protocol and HTTP changes separable
Using the lower-level jsonrpc4j entry point is necessary for fatal propagation, but this PR also combines per-element batch recovery, partial request-envelope validation that overlaps #6676, and JSON-RPC-local bare-500/wrapper-unwrapping behavior. These concerns have different ownership and rollback paths; keeping them together makes a future jsonrpc4j or Jetty change harder to evaluate against the core exception fix.
Suggestion: keep error sanitization, fatal propagation, and batch recovery here; move complete envelope semantics to #6676 and handle default-error-page/wrapper behavior in an HTTP/Jetty change. If they must remain together, document these boundaries and the conditions for removing the local workarounds.
There was a problem hiding this comment.
Agreed. I propose keeping these safeguards here, with explicit replacement conditions.
The ID and scalar-params checks prevent framework failures exposed by the handleRequest switch from being misclassified as internal errors. Complete envelope and notification semantics remain with #6676.
The best-effort empty-500 guard prevents an escaped Error from reaching Jetty's detail-bearing default response when cleanup succeeds. Unwrapping is needed because the production response wrapper does not delegate flushBuffer reliably. These protections cannot simply be removed without a replacement.
I have added the replacement conditions to the PR description under "Local safeguards and replacement conditions":
- Unwrapping can be removed after shared flush delegation is fixed and production-filter tests pass without it.
- The guard can be removed or replaced after container handling provides equivalent protection across the affected paths and formats, with the HTTP contract verified.
- The existing checks will be integrated with [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676's validation, in coordination with that change, rather than maintained as duplicate predicates.
6ecb02c to
cd012e1
Compare
Replace jsonrpc4j's unhandled -32001 fallback with a fixed -32603 response so unmapped exception types and raw messages no longer reach clients. Keep the documented -32001 contract for net_version and eth_chainId through explicit mappings with fixed message and data. Propagate VirtualMachineError, ThreadDeath, LinkageError, and TronError through the JSON-RPC boundary. Scan complete cause chains without allocating a visited set, detecting cycles without missing fatal causes. Classify Throwables escaping dispatch before logging or mapping them. Before rethrowing an Error, unwrap response decorators with a bounded walk and make a best-effort attempt to commit an empty HTTP 500 on the underlying response. Preserve the original error if cleanup fails, and abandon cleanup without delegating into cyclic or unresolved wrappers. Route single requests through handleRequest because the servlet handle API catches Throwable. Catch RuntimeException, IOException, and Error at both dispatch boundaries, then propagate a classified fatal cause or return -32603 when a response is appropriate. This also covers non-fatal Errors from interceptor hooks. Discard partial dispatch output on failure. Recover each failed batch element without discarding earlier results or skipping later requests. Share internal-error construction across serialization, dispatch, and response-parsing failures. Count only the replacement when parsing fails, retaining existing overflow rules and the current single/batch handling of requests without an id. Bound unmapped-exception and chain-identity failure logging to the first occurrence per operation and exception type. Classify fatal causes before logging chain-identity and asynchronous log-query failures. Log the first escaped dispatch failure in each batch at ERROR with its cause; repeats use DEBUG with only the index and exception type. Restore interrupted status for asynchronous log queries. Reject Boolean, object, and array request IDs before dispatch with -32600 and id:null. Reject non-null scalar params with -32600, preserve valid IDs, and isolate invalid batch elements so their siblings run. Missing, null, array, and object params keep their existing semantics. Remove the obsolete request replay wrapper and HTTP status provider. Add resolver, servlet, embedded-Jetty, chain-identity, asynchronous failure, dispatch-contract, and request-envelope regression coverage. Exercise the real HTTP interceptor chain and observe commitment without performing it on the servlet's behalf. Test interceptor Errors with a real server and preserve the existing notification response-suppression rules with characterization tests. Add a package-private server injection seam for servlet tests without changing production initialization.
cd012e1 to
2d2d9df
Compare
What does this PR do?
Replaces jsonrpc4j's unhandled-exception fallback with spec-defined responses and stops converting fatal errors into JSON-RPC replies.
Resolver (
JsonRpcErrorResolver):-32603 "Internal error"with nodata. Logging is bounded by(RPC method, exception class): the first occurrence is WARN with the Throwable; repeats are DEBUG without the Throwable or exception message.Invalid Request/Method not found/Invalid params/Internal error), somessageis nevernull.datais exception data > annotation data; jsonrpc4j'sErrorData(exceptionClass, message)default is gone.VirtualMachineError,ThreadDeath,LinkageErrorand java-tron'sTronErrorfound anywhere on the cause chain are rethrown as the actual cause instead of being answered. A shared allocation-free, cycle-safe scan has no cause-depth cutoff; servlet dispatch catches use it before ordinary logging or error mapping as well.net_version/eth_chainIdkeep their documented-32001through an explicit mapping ("Chain identity unavailable",data "{}");ethChainId()keeps the cause, rethrows a fatal cause before logging, and logs the first failure per exception type at WARN with repeats at DEBUG (deduplication, not a time-based limiter; a recurrence after recovery does not warn again).ExecutionException/InterruptedExceptionon the asynchronous log query get a fixed"Internal error"message;LogBlockQueryrethrows a fatal cause before its WARN (the executor wraps a task-thrownErrorinExecutionException), logs other causes and restores the interrupt flag.Servlet (
JsonRpcServlet):handleRequest(InputStream, OutputStream)instead ofhandle(request, response), whosecatch (Throwable)would swallow the rethrown fatal error. Single and batch dispatch catchRuntimeException,IOExceptionandError, inspect the complete cause chain before logging, and rethrow a classified fatal cause. Other escaped failures become-32603under the existing ID rules. This also covers non-fatalErrors from interceptor hooks outside jsonrpc4j's method-invocation catch; any partial dispatch output is discarded.-32603, retaining earlier results and continuing with later requests unless the existing response budget overflows. Each batch logs only its first escaped non-fatal dispatch failure at ERROR with the Throwable; subsequent failures use DEBUG with only the index and exception class. This state is request-local, and fatal inspection precedes logging. Malformed response bytes are discarded; only the replacement error is charged. The original pre-parse size check and strict>boundary remain: bytes exceeding the remaining budget get-32003even if malformed.doPostguard best-effort commits a zero-length HTTP 500 before rethrowing an escapedError. On this cleanup path only, it unwraps up to 16ServletResponseWrapperlayers because the productionHttpInterceptor's response wrapper does not delegateflushBuffer. Self-reference, an unresolved/deeper chain or a non-HTTP inner response abandons cleanup without calling response methods through that wrapper. Cleanup failures never replace the original fatal. The client may receive an empty 500 or a closed connection; if cleanup cannot commit, container fallback remains possible. This propagation does not itself terminate the process.application/json-rpcare set explicitly at normal JSON-RPC servlet exits; the customHttpStatusCodeProviderconfiguration and the now-unusedCachedBodyRequestWrapperare removed.-32600 "Invalid Request"withid: null. Non-null scalarparamsalso gets-32600; a valididis echoed, while a missing, null or invalididbecomesnull. In a batch only the offending element gets the error and the other elements still execute. An explicitid: nullon an otherwise valid request is not rejected by these checks; its final semantics, and whether to rejectparams: null, are decided with [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676.id; whenidis absent it keeps the existing empty 200, while explicitid: nullgets an error withid: null. A recoverable batch failure withoutidstill produces an error withid: null. Responses returned normally by jsonrpc4j are forwarded, including its existing error responses to some requests withoutid; the resolver's new error mapping still applies. Notification response-suppression rules are preserved, not normalized; normalization is deferred to [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676.Why are these changes required?
An exception without an
@JsonRpcErrorsmapping currently produces (Java 8):{"jsonrpc":"2.0","id":1,"error":{"code":-32001,"message":null,"data":"java.lang.NullPointerException"}}message: nullviolates JSON-RPC 2.0 section 5.1 (on Java 17 it becomes a helpful-NPE string echoing internal class and method names),dataexposes internal types, and-32001is the code the public error catalog documents for the chain identity lookup, so callers cannot tell their own bad input from a node failure. Fatal errors such asOutOfMemoryError,StackOverflowErrorandTronErrorwere converted into ordinary error responses, masking a process-level failure; after propagation, the servlet now best-effort commits a detail-free 500 rather than allowing Jetty's default error page to render the Throwable. Invalid request ID types produced HTTP 200 with an empty body for single requests. Scalarparamshad the same result after a registered method reached argument matching; an unknown method returned-32601before inspectingparams. In a batch, framework exceptions produced only-32603withid: nulland stopped further processing.-32603is the Internal error defined by the specification and matches Besu'sRpcErrorType.INTERNAL_ERRORclassification. Rejecting Boolean IDs is deliberately stricter than go-ethereum, following section 4 (String / Number / Null). Section 4.2 requires structuredparams; java-tron classifies a non-null scalar as-32600at the request-envelope layer, matching Besu's error-code classification, while geth classifies it as-32602during method-argument parsing. For this malformed shape without anid, java-tron returnsid: null, while geth sends no response. Full analysis and reproduction steps are in #6941.Request-envelope validation intentionally precedes method lookup: an unknown method with scalar
paramschanges from-32601to-32600, while the same method with validparams: []remains-32601. A malformed request without anidis not a valid notification. Successful notifications remain response-free; this change does not yet unify suppression of error responses across dispatch and servlet catches.This PR has been tested by:
Unit Tests
Manual Testing
JsonRpcErrorResolverTest(17 tests) - mapped code / data priority, message defaults, sanitized unmapped exceptions, bounded logging, first-occurrence tracking per operation and exception type, and fatal propagation including null/ordinary/deep/cyclic cause chains andTronError.JsonRpcErrorSanitizationIntegrationTest(29 tests) - through a realJsonRpcServerandJsonRpcServlet: unmapped exceptions sanitized on the wire, four fatal categories escaping the server, servlet best-effort empty-500 handling, fixed asynchronous and chain-identity messages, business messages preserved, batch isolation, transport and request-envelope contracts. A realpreHandleJsoninterceptor proves that non-fatal Errors escape the raw server and are recovered by the servlet; direct and Error-wrapped fatal causes still propagate. No-ID unknown-method, arity and unmapped-exception responses are characterized on both single and batch paths without changing suppression rules.JsonRpcDispatchContractTest(8 tests) - characterization of jsonrpc4j 1.6 dispatch (arity, null array elements, overload selection, scalarparams) so a later framework upgrade cannot change behavior silently.JsonRpcServletTest(72 tests) - request validation and pass-through; all three batch recovery points; exact-limit, replacement-overflow and missed/double-accounting guards; per-batch logging including non-fatal Errors and reset across requests; fatal-after-failure stops dispatch without another log; missing versus explicit null ID on escaped failures; single/batch partial-output discard checked against the raw markers and with strict trailing-token rejection; direct and wrapped fatal propagation; response-wrapper cycles/depth limits and cleanup failures.LogBlockQueryFailureTest(4 tests) - cause logging, interrupt-flag restoration, and fatal causes rethrown without the failure WARN, both from a mockedFutureand through a real executor'sFutureTask.TronJsonRpcImplChainIdentityTest(6 tests) - first-occurrence WARN with the cause and DEBUG repeats without it, no reset after recovery, a key shared withnet_version, and wrapped or direct fatal causes rethrown without logging or consuming the first WARN.JsonRpcServletJettyTest(3 tests) - filtered and unfiltered fatal paths plus a healthy filtered request. For JSON, HTML and plain-text Accept values, the production-filter test requires server-side evidence of committed status 500 and zero content length; an empty 500 or a closed connection is then accepted. The observation filter never commits the response.A package-private server setter supports servlet tests without changing production initialization.
CachedBodyRequestWrapperTestis removed together with the class. On 2026-09-08, acleanTest --no-build-cacheJDK 17 (arm64) run passed 28 test classes / 330 tests (0 failures, 0 errors, 0 skipped); the seven focused classes above then contained 131 tests.checkstyleMain,checkstyleTest, andgit diff --checkalso pass. Separately removing the single and batchErrorcatches made six distinct new tests fail for each mutation; both catches were restored before the full run. On 2026-09-15 the same suite and both Checkstyle tasks were re-run on the branch rebased ontorelease_v4.8.3(0d19485318, which already carries the slf4j 2.0.17 / logback 1.3.16 / jackson 2.18.10 upgrade from #6950 and the HTTP error sanitization from #6954) with the same 28 classes / 330 tests passing; the logging assertions that read logback'sLogger,ListAppenderandILoggingEventhold under logback 1.3.16. On 2026-09-24, after the review changes to fatal ordering and chain-identity logging, the samecleanTest --no-build-cachesuite passed 28 test classes / 338 tests (0 failures, 0 errors, 0 skipped) on the same base; the seven focused classes above now contain 139 tests, and both Checkstyle tasks andgit diff --checkpass. Moving the chain-identity fatal check after logging, removing theLogBlockQueryfatal check, logging under different wording or at TRACE before either fatal check, and making first-occurrence tracking always report a first occurrence each failed the intended new tests; all four were reverted before the final run. Java 8/x86 execution was not performed on this arm64 machine; CI covers it.Compatibility
Breaking, limited to observable failure-handling paths; no request that succeeds today starts failing.
-32001, exception message (may benull),data= class name-32603 "Internal error", nodatanet_version/eth_chainIdfailure-32001, underlying message,data= class name-32001 "Chain identity unavailable",data "{}"ExecutionException/InterruptedException-32000, causetoString()/null-32000 "Internal error"-32603/id: null, then processing stops-32600 "Invalid Request",id: null; batch siblings continueparams-32603/id: nulland stops. An unknown method returns-32601before checkingparams.-32600 "Invalid Request", nodata; valididpreserved, otherwiseid: null; batch siblings continue. Envelope validation precedes method lookup, so unknown + scalar changes to-32600, while unknown +params: []stays-32601.Error(VirtualMachineError/ThreadDeath/LinkageError/TronError)-32001/-32000handleRequestthrows non-fatalRuntimeException/IOException-32603; withoutid, the existing empty body is retainedErrorescapeshandleRequest, e.g. frompreHandleJsonhandleentry point without guaranteeing a complete JSON-RPC error response (empty HTTP 200 if no bytes have been written). Batch: escapes to outer/container handling.-32603under the existing ID rules; partial output is discarded, batch siblings continue within the existing response budget. A no-ID single catch stays silent; explicitid: nulland no-ID batch failures get an error withid: null. Classified fatal causes still propagate.RuntimeExceptions and response-parsing failures each return a single-32603withid: null, discarding earlier results and skipping later elements. A dispatchIOExceptionwas not caught at all and propagated to outer handlers without this JSON-RPC response guarantee.-32603for the failed element and continue within the existing overflow rules; missingidstill producesid: nullUnchanged by this PR: successful responses, HTTP 200 whenever a normal JSON-RPC response is produced, existing dispatch and method validation for missing / null / Array / Object
params,code/dataof the remaining existing error mappings, deliberate business messages such as"filter not found", gRPC and non-JSON-RPC HTTP APIs. The JSON-RPC code onrelease_v4.8.3is identical to the develop baseline (4a21592f95), which has 62 mappings. Removing the uninstall lookup-miss mapping belongs to #6951; if that change lands first, this PR rebases onto a 61-mapping baseline and adds the two chain-identity mappings, leaving 63 combined. Fatal and genuine transport failures are not normal JSON-RPC responses and do not carry an HTTP-200 guarantee.Before merge:
JSON_RPC_UNDERLYING_INTERNAL_ERROR,JSON_RPC_SERVLET_INTERNAL_ERROR,JSON_RPC_EXECUTION_ERROR,JSON_RPC_INTERRUPTED) need a documentation-en PR; the table is generated fromx-tron-error-modelindocs/api/openrpc.json.-32001behavior or on the previous chain-identitymessage/data, and that their error classification accounts for the new-32603responses.Follow up
jsonrpcandmethodmembers, whether to rejectparams: null, and the final semantics of an explicitid: nullon an otherwise valid request belong to [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676, which overlaps this PR inJsonRpcServletand theTronJsonRpcannotation blocks (see Extra details).JsonNodeID.idare forwarded; a single servlet catch withoutidis silent, explicitid: nullgets an error, and a batch failure withoutidemitsid: null. Future normalization must consider forwarding and synthesized-error exits together; this PR does not preselect that policy.-32000catch sites across three methods (eth_call,eth_estimateGas,buildTransaction) that still forward the underlying exception message; changing those public business-error messages needs its own compatibility review.-32700and loses the request id.JsonRpcServlet.doPost, on a best-effort basis.Extra details
maxResponseSizecontinues to limit dispatched response accumulation. As on existing servlet-generated error paths, protocol error envelopes are still emitted and may make the final body exceed that threshold. Request-body and token limits, together with the batch-size limit when enabled, bound this behavior; redefining the threshold as a hard final-body cap is out of scope for this PR.The per-batch log policy bounds only the servlet's escaped-dispatch failure log point, not business, resolver or container logging, and is not a cross-request rate limiter. Enabling DEBUG exposes repeated failure indices and types but not their Throwable or message. Single-request non-fatal dispatch failures are logged individually at ERROR with the Throwable.
The fatal cleanup bypasses response wrappers locally; it does not modify shared HTTP wrapper semantics. The missing flush delegation in
CharResponseWrapper/ServletOutputStreamCopycan be discussed with #6936 independently. The embedded-Jetty regression installs the actualHttpInterceptor; an outer observer only records the underlying response's committed state, status and zero content length, and never commits it on behalf of the servlet.An object with scalar
paramsand noidis malformed rather than a valid notification, so it receives-32600withid: null. Envelope validation also intentionally precedes method lookup; clients probing method availability should use a structurally validparamsarray or object.This PR overlaps the request-envelope validation planned in #6676 in
JsonRpcServletand theeth_getLogs@JsonRpcErrorsblock ofTronJsonRpc. There is no dependency between the two: this PR can land first and #6676 can build on the pre-dispatch checks added here; if #6676's PR lands first, this one will be rebased.Fatal classification.
VirtualMachineError,LinkageError,ThreadDeathandTronError, found anywhere on the cause chain, form the propagation set. This is a chosen policy, not a proof that every otherErroris safe to recover from. Errors without a classified fatal cause follow the existing response-suppression and batch-recovery rules, using the sanitized-32603where an internal-error response is emitted; that reports a failed call, not a healthy or recovered node. The policy narrowsdevelop, where method-invocation Errors, includingOutOfMemoryError, were answered as-32001with the class name, and applies the same classification at the method-invocation and dispatch boundaries, as discussed in #6941.Local safeguards and replacement conditions.
Error: can be removed or replaced after container handling provides equivalent protection across the affected paths and formats, with the HTTP contract verified.paramschecks: needed becausehandleRequestthrows for these envelopes wherehandle()swallowed them; they will be integrated with [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676's validation, in coordination with that change, rather than maintained as duplicate predicates.Pre-submit checklist:
handleRequestis required and why the chain-identity cause must be retainedCloses #6941
Refs #6676