Skip to content

fix(jsonrpc): normalize error responses and fatal handling - #6985

Open
waynercheung wants to merge 1 commit into
tronprotocol:release_v4.8.3from
waynercheung:feat/jsonrpc-error-sanitization
Open

waynercheung wants to merge 1 commit into
tronprotocol:release_v4.8.3from
waynercheung:feat/jsonrpc-error-sanitization

Conversation

@waynercheung

@waynercheung waynercheung commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

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):

  • Unmapped non-fatal exceptions -> -32603 "Internal error" with no data. Each occurrence is logged at DEBUG with the Throwable; this log point had no logging on the base branch.
  • Mapped exceptions get a message precedence of annotation > exception message > per-code default (Invalid Request / Method not found / Invalid params / Internal error), so message is never null. data is exception data > annotation data; jsonrpc4j's ErrorData(exceptionClass, message) default is gone.
  • VirtualMachineError, ThreadDeath, LinkageError and java-tron's TronError found 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_chainId keep their documented -32001 through an explicit mapping ("Chain identity unavailable", data "{}"); ethChainId() keeps the cause, rethrows a fatal cause before logging, and logs other failures at DEBUG with the Throwable.
  • ExecutionException / InterruptedException on the asynchronous log query get a fixed "Internal error" message; LogBlockQuery rethrows a fatal cause before logging (the executor wraps a task-thrown Error in ExecutionException), logs other causes and interruptions at DEBUG, and restores the interrupt flag.

Servlet (JsonRpcServlet):

  • Single requests go through handleRequest(InputStream, OutputStream) instead of handle(request, response), whose catch (Throwable) would swallow the rethrown fatal error. Single and batch dispatch catch RuntimeException, IOException and Error, inspect the complete cause chain before logging, and rethrow a classified fatal cause. Other escaped failures become -32603 under the existing ID rules. This also covers non-fatal Errors from interceptor hooks outside jsonrpc4j's method-invocation catch; any partial dispatch output is discarded.
  • Recoverable batch failures in request serialization, dispatch or response parsing produce an element-specific -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 -32003 even if malformed.
  • An outer doPost guard best-effort commits a zero-length HTTP 500 before rethrowing an escaped Error, so the container does not render its default error page. An already committed response is left untouched, and 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.
  • HTTP 200 and application/json-rpc are set explicitly at normal JSON-RPC servlet exits; the custom HttpStatusCodeProvider configuration and the now-unused CachedBodyRequestWrapper are removed.
  • Request envelope types are validated before dispatch. Boolean / object / array IDs get -32600 "Invalid Request" with id: null. Non-null scalar params also gets -32600; a valid id is echoed, while a missing, null or invalid id becomes null. In a batch only the offending element gets the error and the other elements still execute. An explicit id: null on an otherwise valid request is not rejected by these checks; its final semantics, and whether to reject params: null, are decided with [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676.
  • The single-request catch-all preserves a valid request id; when id is absent it keeps the existing empty 200, while explicit id: null gets an error with id: null. A recoverable batch failure without id still produces an error with id: null. Responses returned normally by jsonrpc4j are forwarded, including its existing error responses to some requests without id; 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 @JsonRpcErrors mapping currently produces (Java 8):

{"jsonrpc":"2.0","id":1,"error":{"code":-32001,"message":null,"data":"java.lang.NullPointerException"}}

message: null violates JSON-RPC 2.0 section 5.1 (on Java 17 it becomes a helpful-NPE string echoing internal class and method names), data exposes internal types, and -32001, like -32000, falls in the implementation-defined server-error range (-32000 to -32099); every unmapped exception, including null-parameter input errors, was reported under it, so callers cannot tell their own bad input from a node failure. Fatal errors such as OutOfMemoryError, StackOverflowError and TronError were 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. Scalar params had the same result after a registered method reached argument matching; an unknown method returned -32601 before inspecting params. In a batch, framework exceptions produced only -32603 with id: null and stopped further processing.

-32603 is the Internal error defined by the specification and matches Besu's RpcErrorType.INTERNAL_ERROR classification. Rejecting Boolean IDs is deliberately stricter than go-ethereum, following section 4 (String / Number / Null). Section 4.2 requires structured params; java-tron classifies a non-null scalar as -32600 at the request-envelope layer, matching Besu's error-code classification, while geth classifies it as -32602 during method-argument parsing. For this malformed shape without an id, java-tron returns id: 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 params changes from -32601 to -32600, while the same method with valid params: [] remains -32601. A malformed request without an id is 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 (16 tests) - mapped code / data priority, message defaults, sanitized unmapped exceptions logged at DEBUG with the Throwable, fatal propagation without logging (direct, wrapped, TronError), and fatal-cause scanning over null/ordinary/deep/cyclic chains.

  • JsonRpcErrorSanitizationIntegrationTest (29 tests) - through a real JsonRpcServer and JsonRpcServlet: unmapped exceptions sanitized on the wire, fatal errors escaping the server (direct StackOverflowError and TronError, a fatal cause wrapped in a mapped exception, and fatal errors from an interceptor), servlet best-effort empty-500 handling, fixed asynchronous and chain-identity messages, business messages preserved, batch isolation, transport and request-envelope contracts. A real preHandleJson interceptor 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, scalar params) so a later framework upgrade cannot change behavior silently.

  • JsonRpcServletTest (69 tests) - request validation and pass-through; all three batch recovery points, with a single DEBUG record for the request-serialization and response-parsing recoveries; 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; the empty-500 cleanup discarding partial output, leaving a committed response untouched and surviving cleanup failures.

  • LogBlockQueryFailureTest (4 tests) - a single DEBUG event with the Throwable for an execution failure and for an interruption, interrupt-flag restoration, and fatal causes rethrown without logging, both from a mocked Future and through a real executor's FutureTask.

  • TronJsonRpcImplChainIdentityTest (5 tests) - each failure logged at DEBUG with the Throwable and nothing else logged, including the null-genesis path around a successful lookup, net_version through the same lookup, and wrapped or direct fatal causes rethrown without logging.

  • 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. The resolver, chain-identity and LogBlockQuery tests use a shared ApiLogCapture test helper that captures API logger events from the calling thread only, so their assertions are not affected by other threads logging to the shared logger. CachedBodyRequestWrapperTest is removed together with the class. On 2026-09-08, a cleanTest --no-build-cache JDK 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, and git diff --check also pass. Separately removing the single and batch Error catches 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 onto release_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's Logger, ListAppender and ILoggingEvent hold under logback 1.3.16. On 2026-09-24, after the review changes to fatal ordering and chain-identity logging, the same cleanTest --no-build-cache suite passed 28 test classes / 338 tests (0 failures, 0 errors, 0 skipped) on the same base; the seven focused classes above then contained 139 tests, and both Checkstyle tasks and git diff --check passed. Moving the chain-identity fatal check after logging, removing the LogBlockQuery fatal 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. On 2026-10-05, after rebasing onto release_v4.8.3 (6d5adc4db7, which adds one JsonRpcServletTest case) and removing the logging deduplication, the same cleanTest --no-build-cache run over org.tron.core.services.jsonrpc.* and org.tron.core.jsonrpc.* passed 28 test classes / 337 tests (0 failures, 0 errors, 0 skipped); the seven focused classes above then contained 138 tests, and both Checkstyle tasks and git show --check on the amended commit passed. Seven mutations (WARN instead of DEBUG and a missing Throwable in the resolver, logging before fatal classification in the resolver, in ethChainId and in LogBlockQuery, WARN in LogBlockQuery, and a removed LogBlockQuery fatal check) each failed the intended tests and were reverted byte-for-byte before the final run. On 2026-10-09, after adding DEBUG logs to the two batch recovery points, the same run passed 28 test classes / 337 tests again (the two recovery tests gained log assertions, so the counts are unchanged), and both Checkstyle tasks and git diff --check passed. Deleting the response-parsing log and raising the serialization log to WARN each failed the intended test and were reverted byte-for-byte before the final run. Later on 2026-10-09, after rebasing onto release_v4.8.3 (6f7b83a6d6, which removes CharResponseWrapper and changes the TronJsonRpcImpl constructor) and removing the response unwrapping, the same run passed 31 test classes / 343 tests (0 failures, 0 errors, 0 skipped; the base adds three filter-pipeline classes under org.tron.core.jsonrpc); the seven focused classes above now contain 134 tests, and both Checkstyle tasks and git show --check on the amended commit pass. Deleting the flushBuffer call, the resetBuffer call or the committed-response check from the cleanup each failed the intended tests and was reverted byte-for-byte 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.

Case Before After
Unmapped exception -32001, exception message (may be null), data = class name -32603 "Internal error", no data
net_version / eth_chainId failure -32001, underlying message, data = class name -32001 "Chain identity unavailable", data "{}"
ExecutionException / InterruptedException -32000, cause toString() / null -32000 "Internal error"
Mapped exception with a null or blank message, e.g. a message-less NPE on JDK 8 at a mapped -32000 catch site message is null or blank the default text for the code ("Internal error" for -32000); code / data unchanged
Boolean / object / array request ID Single request: HTTP 200, empty body; batch: only -32603 / id: null, then processing stops -32600 "Invalid Request", id: null; batch siblings continue
Non-null scalar params After a registered method is selected: single request is HTTP 200 with an empty body; batch returns only -32603 / id: null and stops. An unknown method returns -32601 before checking params. -32600 "Invalid Request", no data; valid id preserved, otherwise id: null; batch siblings continue. Envelope validation precedes method lookup, so unknown + scalar changes to -32600, while unknown + params: [] stays -32601.
Fatal Error (VirtualMachineError / ThreadDeath / LinkageError / TronError) converted into -32001 / -32000 propagates after a best-effort empty HTTP 500; the connection may close instead, and a batch loses accumulated results
Single handleRequest throws non-fatal RuntimeException / IOException hidden by the old servlet-level entry point with a valid ID, HTTP 200 / -32603; without id, the existing empty body is retained
Non-fatal Error escapes handleRequest, e.g. from preHandleJson Single: caught by the old handle entry point without guaranteeing a complete JSON-RPC error response (empty HTTP 200 if no bytes have been written). Batch: escapes to outer/container handling. HTTP 200 / -32603 under the existing ID rules; partial output is discarded, batch siblings continue within the existing response budget. A no-ID single catch stays silent; explicit id: null and no-ID batch failures get an error with id: null. Classified fatal causes still propagate.
Recoverable batch request-serialization, dispatch or response-parsing failure Serialization failures, escaped dispatch RuntimeExceptions and response-parsing failures each return a single -32603 with id: null, discarding earlier results and skipping later elements. A dispatch IOException was not caught at all and propagated to outer handlers without this JSON-RPC response guarantee. preserve earlier results, append -32603 for the failed element and continue within the existing overflow rules; missing id still produces id: null

Unchanged 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 / data of the remaining existing error mappings, deliberate business messages such as "filter not found", gRPC and non-JSON-RPC HTTP APIs. The release_v4.8.3 base (6d5adc4db7) has 62 mappings, the same as the develop baseline (4a21592f95). 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:

  • The four affected entries of the public JSON-RPC error catalog (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 from x-tron-error-model in docs/api/openrpc.json.
  • Confirm that gateway error mappings, the official SDKs and monitoring rules do not depend on the old catch-all -32001 behavior or on the previous chain-identity message / data, and that their error classification accounts for the new -32603 responses.
  • No landing-order dependency on [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676; whichever change lands second rebases (see Extra details).

Follow up

  • Validation of the jsonrpc and method members, whether to reject params: null, and the final semantics of an explicit id: null on 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 in JsonRpcServlet and the TronJsonRpc annotation blocks (see Extra details).
  • jsonrpc4j's precision loss when round-tripping large integer or high-precision numeric request IDs is a separate compatibility follow-up; servlet-generated errors in this PR preserve the original JsonNode ID.
  • Notification normalization is deferred to [Feature]Standardize JSON-RPC error handling(revert codes, LiteNode pruned-history responses, request fields validation) #6676 as discussed in [Feature] Standardize JSON-RPC error mapping and exception boundaries #6941. Characterization tests record current behavior: jsonrpc4j-generated unknown-method, arity and unmapped-exception errors without id are forwarded; a single servlet catch without id is silent, explicit id: null gets an error, and a batch failure without id emits id: null. Future normalization must consider forwarding and synthesized-error exits together; this PR does not preselect that policy.
  • Review the five mapped -32000 catch 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. They also drop the cause, so a fatal error wrapped in an Exception there is answered as -32000 instead of propagating.
  • Exercise unmapped-exception and fatal handling through the server that init() builds (composite service proxy, setShouldLogInvocationErrors(false), interceptor list). The error-handling tests in this PR build the server directly, override init() or mock it; the existing end-to-end requests through init() in JsonrpcServiceTest cover successful calls and the request size limit only.
  • jsonrpc4j 1.6 -> 1.7 upgrade, fixing the parameter type mismatch that returns -32700 and loses the request id.
  • Container-wide sanitization of non-413 Jetty error pages remains a separate HTTP-layer hardening topic; this PR protects only Errors that escape JsonRpcServlet.doPost, on a best-effort basis.
  • Process-level handling of fatal Errors on HTTP worker threads, for all HTTP servlets and gRPC: whether a TronError should reach ExitManager (after auditing which TronError sites are reachable from API requests) and whether to terminate on OutOfMemoryError at the JVM level, for example with -XX:+ExitOnOutOfMemoryError.

Extra details

maxResponseSize continues 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 acts on the response the servlet receives. Since #6982 removed the production HttpInterceptor's response wrapper, that is the container response on every JSON-RPC service. The embedded-Jetty regression installs the actual HttpInterceptor; an outer observer only records the response's committed state, status and zero content length, and never commits it on behalf of the servlet.

An object with scalar params and no id is malformed rather than a valid notification, so it receives -32600 with id: null. Envelope validation also intentionally precedes method lookup; clients probing method availability should use a structurally valid params array or object.

This PR overlaps the request-envelope validation planned in #6676 in JsonRpcServlet and the eth_getLogs @JsonRpcErrors block of TronJsonRpc. 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.

Diagnostics. Causes removed from responses for unmapped method exceptions, chain-identity failures, asynchronous log-query failures and the two batch recovery points (request serialization and response parsing) are logged only at DEBUG, so the default INFO configuration does not record them; enabling DEBUG captures only later failures and does not rate-limit them. Making server-side failures on these paths visible at the default level would be a separate logging change.

Fatal classification. VirtualMachineError, LinkageError, ThreadDeath and TronError, found anywhere on the cause chain, form the propagation set. This is a chosen policy, not a proof that every other Error is safe to recover from. Errors without a classified fatal cause follow the existing response-suppression and batch-recovery rules, using the sanitized -32603 where an internal-error response is emitted; that reports a failed call, not a healthy or recovered node. The policy narrows develop, where method-invocation Errors, including OutOfMemoryError, were answered as -32001 with the class name, and applies the same classification at the method-invocation and dispatch boundaries, as discussed in #6941. On embedded Jetty 9.4.58 with the production HttpInterceptor, a TronError, a StackOverflowError and a synthetic OutOfMemoryError thrown from the invoked method each produced HTTP 500 with a zero-length body and a WARN with the Throwable from Jetty's HttpChannel; the default uncaught exception handler was not invoked, so ExitManager is not reached, and a following request returned 200. Process-level handling of such Errors is listed under Follow up.

Local safeguards and replacement conditions.

Pre-submit checklist:

  • Google Java Style; Checkstyle passes on main and test sources
  • No debug code, temporary comments or TODOs
  • No numeric computation or narrowing casts introduced
  • Logging: no log point is raised above its base-branch level. Unmapped exceptions, chain identity, and asynchronous log-query failure and interruption, which had no logging, log at DEBUG with the Throwable, the first three after fatal classification; batch request-serialization and response-parsing recoveries, which had no logging either, log at DEBUG with the element index and the Throwable; single-request dispatch failures keep ERROR; each batch logs at most one full ERROR stack, with further escaped dispatch failures at DEBUG without a stack/message (the batch catch now also covers escaped IOExceptions and non-fatal Errors, which on base left the servlet without an API ERROR log: the full-node HttpInterceptor swallowed an IOException silently, and Jetty logged an escaped Error at WARN); nothing logs on the normal request path
  • No DB, consensus, config or dependency changes
  • Comments explain why handleRequest is required

Closes #6941
Refs #6676

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)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

@waynercheung waynercheung Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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. ethChainId now 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.
  • LogBlockQuery had the same ordering issue, since its executor Future wraps a task-thrown Error in ExecutionException; it now checks for fatal causes before its WARN. findFatalCause is public so that the filters package 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Following up on the suppression part of this comment: 8be801cb0b removes the cross-request first-occurrence deduplication, which the resolver already used and cd012e1d74 extended to chain identity, and takes the simpler path you suggested.

No log point is raised above its base-branch level. These four log points had no logging on the base branch; each now logs at DEBUG with the Throwable on every occurrence, and the cross-request deduplication state for the resolver and chain identity is removed:

  • unmapped exceptions in the resolver;
  • chain-identity failures in ethChainId, which net_version delegates to;
  • LogBlockQuery execution failures;
  • LogBlockQuery interruptions.

Single-request dispatch failures keep ERROR; a batch continues with later elements, logging its first escaped failure at ERROR with the stack and later ones at DEBUG without the stack or message. Fatal causes are classified and rethrown before the resolver, chain-identity and LogBlockQuery execution-failure log calls, so these handlers do not log them.

Trade-off: responses do not carry these causes, and at the default INFO level these log points record nothing. Enabling DEBUG for the API logger captures only later failures, logs every occurrence without rate limiting, and also enables other DEBUG output on that shared logger. Making server-side failures on these paths visible at the default level would be a separate logging change rather than part of this sanitization PR.

The resolver, chain-identity and LogBlockQuery tests now assert DEBUG events carrying the Throwable and no WARN, and their fatal tests assert that these handlers log nothing at any level on the fatal path. The PR description is updated accordingly.

@bladehan1, does this address the suppression part of your comment?

// 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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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":

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Update on the first replacement condition above: #6982 removed the production HttpInterceptor's response wrapper (CharResponseWrapper / ServletOutputStreamCopy), so the missing flush delegation that required unwrapping no longer exists. After rebasing onto release_v4.8.3 (6f7b83a6d6), 025a09aba0 removes the unwrapping: the fatal cleanup now commits the empty 500 on the response the servlet receives, and the embedded-Jetty test with the production HttpInterceptor still requires an empty committed 500 for JSON, HTML and plain-text Accept values. The other two safeguards and their replacement conditions are unchanged, and the PR description is updated accordingly.

@waynercheung
waynercheung force-pushed the feat/jsonrpc-error-sanitization branch from 6ecb02c to cd012e1 Compare September 24, 2026 13:39
@waynercheung
waynercheung force-pushed the feat/jsonrpc-error-sanitization branch 2 times, most recently from 2d2d9df to 8be801c Compare October 5, 2026 14:41
byte[] subBody;
try {
subBody = MAPPER.writeValueAsBytes(subRequest);
} catch (JsonProcessingException e) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] Two new batch recovery points fail silently at every log level

This PR turns recoverable batch failures from abort-the-whole-batch into per-element recovery, and dispatch failures get the per-batch log policy (first failure at ERROR with the Throwable, later ones at DEBUG). The two other new recovery points do not log at any level: a sub-request that fails serialization (catch (JsonProcessingException) at executeBatchRequest, around line 290) and a dispatch response whose bytes cannot be parsed (catch (IOException) around line 251) are silently replaced with -32603. An unparseable response in particular means the framework produced malformed JSON, which is a server-internal signal worth recording.

Impact: operators have no way to observe that either recovery path fired; if a future jsonrpc4j upgrade starts emitting malformed responses, clients see -32603 while the server leaves zero trace.

Suggestion: route both recovery points through the same per-batch log policy (e.g. reuse BatchFailureLog.record), logging at least the element index and exception class at DEBUG.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, agreed that neither recovery point should be silent. Both now log at DEBUG with the element index and the Throwable: request serialization in executeBatchRequest and response parsing in handleBatch. The two existing recovery tests assert a single DEBUG event for the replaced element and nothing at any other level.

I kept them out of BatchFailureLog.record on purpose. Neither point logged anything on the base branch, where each failure ended the whole batch without a log, and this PR does not raise any log point above its base-branch level; going through record would make the first occurrence an ERROR. The escaped-dispatch catch keeps the per-batch ERROR because the base branch already logged those failures at ERROR.

On reachability: the serialization catch is not expected to fire for wire input, since requests are parsed with a nesting limit of 20 and re-serializing such a tree stays within Jackson's default write constraints; only an injected test reaches it. The response-parsing catch fires only if jsonrpc4j emits bytes that OUTBOUND_MAPPER cannot parse, which would be a server-side defect such as a regression after a jsonrpc4j upgrade, the case you describe. That case now leaves a DEBUG record with the parser exception. Making these failures visible at the default level would belong to the separate logging change noted in the description.

protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
try {
doPostInternal(req, resp);
} catch (Error fatal) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[QUESTION] What is the verified terminal behavior of a fatal Error after it leaves doPost?

TronError's documented contract is that it must not be caught and should reach the default uncaught exception handler, which triggers ExitManager / System.exit. This PR rethrows classified fatal Errors out of doPost (after the best-effort empty 500), but what Jetty's worker-thread handling does with an escaped Error — whether it reaches the thread's uncaught exception handler or is swallowed/logged by the container — does not appear to be verified, and the PR description itself notes that this propagation does not terminate the process.

This is still a strict improvement over the base branch (which answered TronError as -32001), so it is not blocking. But if Jetty absorbs the Error, the "stop masking process-level failure" goal is only half delivered: the client gets an empty 500 while the process state after e.g. an OutOfMemoryError remains unknown.

Suggestion: before merge, run one manual or JsonRpcServletJettyTest-based check of where a rethrown fatal Error actually ends up (uncaught handler invoked / thread death logged / swallowed), and record the observed result in the PR description; if it cannot reach ExitManager, consider a follow-up that invokes it explicitly at the servlet boundary.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I checked it on embedded Jetty 9.4.58 with this branch's JsonRpcServlet, the production HttpInterceptor filter, and a recording default uncaught exception handler installed in place of ExitManager's. For a TronError, a StackOverflowError and a synthetic OutOfMemoryError thrown from the invoked method:

  • the client received HTTP 500 with a zero-length body;
  • Jetty's HttpChannel logged the request at WARN with the Throwable on the worker thread;
  • the default uncaught exception handler was not invoked;
  • the server kept serving: a following request on a new connection returned 200.

So Jetty absorbs the rethrown Error, and it does not reach ExitManager. I'll record this in the PR description.

Reaching it would not settle the OutOfMemoryError case either: the handler installed by ExitManager.initExceptionHandler exits only when it finds a TronError in the cause chain, and for any other Throwable it logs "Uncaught exception" and returns. Terminating on OOM is a JVM-level choice, for example -XX:+ExitOnOutOfMemoryError.

I'd keep an explicit ExitManager call out of this PR. Calling it at the servlet boundary would turn any TronError reachable from request handling into a process shutdown triggered by an API call, which needs an audit of which TronError sites are reachable from API paths first. The same question applies to the other HTTP servlets and to gRPC, not only JSON-RPC. I'll open a separate issue for it and include the OOM option there.

@waynercheung
waynercheung force-pushed the feat/jsonrpc-error-sanitization branch from 8be801c to 6171d9f Compare October 9, 2026 08:58
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, make a best-effort attempt to commit an
empty HTTP 500 so the container does not render its default error page.
Preserve the original error if cleanup fails, and leave an already
committed response untouched.

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.

Log unmapped exceptions, chain-identity failures, and asynchronous
log-query failures and interruptions at DEBUG with the Throwable,
classifying fatal causes before logging the failures. Log batch
request-serialization and response-parsing recoveries at DEBUG with the
element index and the Throwable. Log the first escaped dispatch failure
in each batch at ERROR with the Throwable; 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.
@waynercheung
waynercheung force-pushed the feat/jsonrpc-error-sanitization branch from 6171d9f to 025a09a Compare October 9, 2026 09:35
@waynercheung
waynercheung requested a review from bladehan1 October 9, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants