fix(#5043): preserve engine exception types across gRPC (Status mapping + class-name trailer)

[robfrank]

Closes #5043

Summary

The gRPC layer collapsed distinct engine exceptions into a coarse INTERNAL/ABORTED status (or an in-band success=false message), so the client could not reconstruct the original exception type. As a result RemoteDatabase.transaction()'s automatic retry-on-NeedRetryException worked over HTTP but was silently inert over gRPC, and permanent conflicts (a commit-time duplicate key) were mis-typed as retryable.

This PR adopts one consistent server-side mapping and preserves the exception type across the wire the way the HTTP protocol already does. A new GrpcErrorMapper (server) maps each engine exception to the correct io.grpc.Status code and attaches the fully-qualified exception class name (plus duplicate-key index/keys) in a metadata trailer. A new GrpcClientErrorMapper (client) reconstructs the exact type from that trailer, falling back to the legacy status-code mapping when the trailer is absent (backward compatible with older servers). No proto change is needed - trailers are the standard gRPC error-detail channel.

Findings addressed

• COR-3 commitTransaction no longer blanket-maps every commit failure to ABORTED. A commit-time DuplicatedKeyException becomes ALREADY_EXISTS (non-retryable) while genuine ConcurrentModificationException/NeedRetryException stay ABORTED (retryable), so transaction() retry fires over gRPC exactly as over HTTP.
• COR-2 DuplicatedKeyException maps to ALREADY_EXISTS carrying the real index name + keys; the client rebuilds a proper DuplicatedKeyException with correct indexName/keys instead of DuplicatedKeyException(msg, msg, null).
• TX-9 beginTransaction passes an already-mapped UNAUTHENTICATED/PERMISSION_DENIED StatusRuntimeException (from getDatabase) straight through instead of wrapping it as INTERNAL.
• COR-15 executeQuery with an unknown transaction id returns FAILED_PRECONDITION, not INTERNAL.

Deliberately deferred (follow-ups)

• COR-1 (executeCommand returning in-band success=false): flipping it to onError would break existing tests that assert this contract (Issue4794GrpcPerDbAuthorizationIT, Issue5040GrpcTransactionHijackIT, Issue4804GrpcCommandErrorMetricsTest); the "never modify existing tests" rule makes this a separate coordinated change.
• SEC-6 (client-facing message sanitization): broad message rewriting risks breaking message-content assertions across the suite and is better done as a focused pass.

Test plan

• GrpcErrorMapperTest (grpcw, unit) - each engine exception maps to the right Status.Code and trailers; already-mapped statuses pass through; ExecutionException is unwrapped. 10/10 pass.
• Issue5043GrpcErrorTypeReconstructionTest (grpc-client, unit) - client rebuilds the exact type from the trailer: CME stays retryable (NeedRetryException), duplicate-key is reconstructed with correct index/keys and is NOT retryable, unknown class / no trailer falls back to status-code mapping. 9/9 pass.
• Existing grpc-client surefire suite green, incl. the server-backed Issue4533ConcurrentModificationTest (commit CME -> ConcurrentModificationException).
• Existing grpcw surefire suite green (server-backed ArcadeDbGrpcServiceExtendedTest errored only on a fixed-port 50051 bind collision from concurrent local runs, not on any assertion).

Note: backward compatible - a server without the trailer still maps by status code, and a client without this change still reads the status code.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 46 complexity

Metric	Results
Complexity	46

View in Codacy

🟢 Coverage 56.76% diff coverage · -35.67% coverage variation

Metric	Results
Coverage variation	✅ -35.67% coverage variation
Diff coverage	✅ 56.76% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (656373c)	137165	102881	75.01%
Head commit (1ccd166)	169099 (+31934)	66516 (-36365)	39.34% (-35.67%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5184)	74	42	56.76%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[claude]

Review: preserve engine exception types across gRPC (#5184)

Nice, focused fix. Mapping engine exceptions to the right io.grpc.Status and carrying the FQN class name in a trailer is the correct approach, mirrors the HTTP protocol, and the trailer-based fallback keeps it backward compatible without a proto change. Test coverage is solid on both sides (server mapping + client reconstruction, including the unwrap and no-trailer fallback cases). The deliberate deferral of COR-1/SEC-6 with rationale is well documented.

A few things worth addressing:

1. Fallback for ALREADY_EXISTS without a trailer drops the server message (minor backward-compat regression) - GrpcClientErrorMapper.java

In the legacy/trailer-less path, case ALREADY_EXISTS -> new DuplicatedKeyException(dupIndex(trailers), dupKeys(trailers), null): when talking to an older server (no trailer), dupIndex/dupKeys both return empty strings, so the reconstructed exception message becomes Duplicated key found on index ... assigned to record null and the server descriptive msg is discarded entirely. The previous code new DuplicatedKeyException(msg, msg, null) at least preserved the message. Consider falling back to msg when the trailers are absent so diagnostics are not lost against pre-upgrade servers. A small helper that prefers the trailer value and falls back to msg would cover both this branch and reconstructFromClassName.

2. Duplicate-key trailers use the ASCII marshaller but carry arbitrary user data - GrpcErrorMapper.java

DUP_INDEX_KEY / DUP_KEYS_KEY are ASCII_STRING_MARSHALLER, but dup.getKeys() (and to a lesser extent the index name) can contain arbitrary key values: unicode (accented names, non-Latin scripts, emoji in an indexed string field), control characters, etc. gRPC ASCII metadata values are restricted to the printable range (0x20-0x7E); values outside it get lossily coerced or can be rejected by the transport. The status description is safe (gRPC percent-encodes grpc-message), but these custom trailers are not. Consider base64-encoding the keys/index value (or using a -bin binary metadata key) so non-ASCII keys survive the wire intact and cannot break the trailer.

3. Unused import - RemoteGrpcDatabase.java:27 import com.arcadedb.exception.NeedRetryException; is now referenced only in a comment after the handleGrpcException rewrite. Remove it.

Minor / non-blocking

• Reconstructed DuplicatedKeyException renders assigned to record null because currentIndexedRID is not carried in the trailer. Acceptable, just slightly noisy; could optionally add the RID trailer if cheaply available.
• reconstructFromClassName only recognizes an explicit set of class names; any other NeedRetryException subclass falls through to the status-code path and is rebuilt as ConcurrentModificationException. Still retryable and still a NeedRetryException, so behavior is fine, just noting the exact type is lost for uncommon subclasses.
• Re-confirming the deferred SEC-6 point: INTERNAL errors ship cause.getMessage() verbatim to clients. This matches prior behavior (no regression), but the follow-up sanitization pass is worth prioritizing since it can leak internal detail.

Overall: correct direction and good tests. Items 1 and 2 are the ones I would fix before merge; 3 is trivial cleanup.

[codecov]

Codecov Report

❌ Patch coverage is 37.83784% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.99%. Comparing base (656373c) to head (1ccd166).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...om/arcadedb/remote/grpc/GrpcClientErrorMapper.java	35.29%	17 Missing and 5 partials ⚠️
...java/com/arcadedb/server/grpc/GrpcErrorMapper.java	38.23%	12 Missing and 9 partials ⚠️
.../com/arcadedb/server/grpc/ArcadeDbGrpcService.java	40.00%	3 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #5184       +/-   ##
=============================================
- Coverage     66.02%   36.99%   -29.03%     
- Complexity      915     1210      +295     
=============================================
  Files          1694     1696        +2     
  Lines        137165   137254       +89     
  Branches      29328    29354       +26     
=============================================
- Hits          90558    50780    -39778     
- Misses        34324    77005    +42681     
+ Partials      12283     9469     -2814     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robfrank]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses issue #5043 by preserving ArcadeDB engine exception types across gRPC calls. It introduces GrpcErrorMapper on the server side to map exceptions to gRPC statuses and attach the fully-qualified exception class name (and duplicate key details, Base64-encoded) in metadata trailers. On the client side, GrpcClientErrorMapper reconstructs the exact exception type from these trailers, falling back to status-code-only mapping for older servers. This ensures proper exception propagation (such as retrying on NeedRetryException and handling DuplicatedKeyException properly) over gRPC. Unit tests have been added to verify the mapping and reconstruction. No review comments were provided, so there is no feedback to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[claude]

Review: preserve engine exception types across gRPC (#5184 / closes #5043)

Overall this is a clean, well-scoped fix with genuinely good engineering: a single server-side mapper, a symmetric client-side reconstructor, backward compatibility with trailer-less servers, Base64 for non-ASCII index/keys, pass-through of already-mapped statuses, and thoughtful Javadoc. The retryable/non-retryable semantics (ConcurrentModificationException/NeedRetryException -> ABORTED retryable; commit-time DuplicatedKeyException -> ALREADY_EXISTS non-retryable) are correct and address the reported regression. I verified there is no existing IT asserting generic commit failures as ABORTED, so the commitTransaction status change does not regress the suite.

A few points worth considering:

1. Test coverage: no end-to-end round-trip test (moderate)

The two mapper sides are each unit-tested in isolation, but nothing exercises the actual wire path: server onError(StatusRuntimeException + trailers) -> real gRPC transport -> client Status.trailersFromThrowable reconstruction. The headline behavioral guarantee of this PR - that a commit-time DuplicatedKeyException reconstructs as DuplicatedKeyException over the real wire and transaction() retry does NOT fire - is never asserted against a live server. Since custom-trailer propagation through onError is exactly the kind of thing that can silently break, a small server-backed IT (analogous to Issue4533ConcurrentModificationTest) inserting a duplicate key at commit and asserting non-retry + correct getIndexName()/getKeys() would lock in the fix. As written, a future change to trailer wiring could pass both unit suites while breaking production.

2. GrpcExceptionMappingTest now tests a stale copy of the logic (minor)

That test duplicates the old handleGrpcException switch into a private ExceptionMapper helper rather than calling production code. Now that the real logic lives in GrpcClientErrorMapper.toException, the test validates a frozen copy that has already diverged (e.g. it still does new DuplicatedKeyException("", "", null)). Consider repointing it at GrpcClientErrorMapper.toException(...) so it can't rot and actually guards the shipping code.

3. Trailer-less ALREADY_EXISTS fallback produces a nested message (minor)

In the fallback path, new DuplicatedKeyException(dupIndex(trailers, msg), dupKeys(trailers, msg), null) passes the full server message as both index name and keys, yielding getIndexName() == the entire message and a doubly-nested "Duplicated key <msg> found on index '<msg>'...". This is essentially pre-existing behavior for old-server compatibility and harmless, but slightly ugly; passing null for the keys/index (or leaving them unset) would keep getIndexName() from returning a whole sentence.

4. ServerSecurityException is not directly matched (informational)

The mapper's security branch is instanceof SecurityException (java.lang), but ArcadeDB's ServerSecurityException extends ServerException, not SecurityException. This is fine today because getDatabase already pre-maps security failures to a StatusRuntimeException that the mapper passes through - but if a future onError path ever hands a raw ServerSecurityException to the mapper it would silently fall to INTERNAL. A short comment noting the reliance on upstream pre-mapping (or an explicit ServerException branch) would make that assumption robust.

5. Redundant double-unwrap (nit)

Call sites in ArcadeDbGrpcService (e.g. createRecord external-tx, commitTransaction) compute cause from the ExecutionException themselves and then pass it to toStatusRuntimeException, which calls unwrap again. Harmless, but you could pass the raw throwable and let the mapper own unwrapping consistently.

Security

No new leakage beyond what HTTP already does - the FQ exception class name in the trailer mirrors the existing HTTP contract, and SEC-6 message sanitization is explicitly deferred and acknowledged. Reasonable.

Nice work overall; #1 is the only item I'd treat as close-to-blocking, and it's an additive test rather than a code change.

Reviewed against CLAUDE.md conventions (final params, AssertJ style, TDD, no new deps).

[claude]

Review of PR #5184 - preserve engine exception types across gRPC

Thorough, well-scoped fix. The server/client mapper pair is symmetric, backward-compatible (trailer-less fallback), and the test coverage (server unit, client unit, and a full wire round-trip) is genuinely good. The docs file and the honest "deferred findings" section make the scope easy to reason about. A few observations, mostly minor.

Correctness / robustness

1. Hard-coded FQ class-name string literals in GrpcClientErrorMapper.reconstructFromClassName. The switch cases ("com.arcadedb.exception.DuplicatedKeyException", etc.) are string literals that must stay in sync with the actual class names across two modules that don't share code. A switch requires constant labels so .class.getName() can't be used directly, but the failure mode is silent: rename/move a class and reconstruction quietly reverts to the coarse status-code fallback with no compile error and no test failure. Consider a tiny guard test asserting DuplicatedKeyException.class.getName().equals("com.arcadedb.exception.DuplicatedKeyException") (and peers), so a future refactor trips a red test instead of silently regressing the fix this PR just made.

2. IllegalArgumentException type is shipped but not reconstructed. The server attaches arcadedb-exception-class=java.lang.IllegalArgumentException and maps to INVALID_ARGUMENT, but the client reconstructFromClassName has no case for it, so it falls through to the status-code default -> RemoteException. Not a regression (it was RemoteException before too), but the trailer carries information the client throws away. Minor; add a case if IAE-typed client behavior matters.

3. UNAUTHENTICATED is not mapped to a security exception on the client. getDatabase throws Status.UNAUTHENTICATED for bad credentials (pass-through, no class trailer); the client status-code switch handles PERMISSION_DENIED -> SecurityException but not UNAUTHENTICATED, so authn failures surface as RemoteException while authz failures surface as SecurityException. Pre-existing asymmetry (the old switch had the same gap), just flagging since this PR is the natural place to close it.

4. Trailer-less ALREADY_EXISTS fallback duplicates the message. new DuplicatedKeyException(dupIndex(trailers, msg), dupKeys(trailers, msg), null) passes the full server description as both index name and keys, yielding Duplicated key <msg> found on index '<msg>' already assigned to record null. Only hit against an older server, but the doubled message reads oddly. Cosmetic.

Test quality

5. GrpcExceptionMappingTest now tests a private copy of the old logic. Its inner ExceptionMapper.handleGrpcException replicates the pre-PR switch rather than calling the real GrpcClientErrorMapper.toException, so it no longer exercises production code (and even still constructs DuplicatedKeyException("","",null), the old lossy form). Not introduced here, but this PR moved the logic out from under it - worth repointing it at GrpcClientErrorMapper.toException so it guards the shipping path.

6. Round-trip test binds a fixed port 50051. The PR description already notes 50051 bind collisions under concurrent local runs. Issue5043GrpcDuplicatedKeyRoundTripTest isn't tagged @Tag("slow"); given it starts a full server and does a multi-step wire round-trip, consider whether it belongs behind a tag or can use an ephemeral port to avoid CI flakiness.

Security

7. As the class Javadoc and the deferred SEC-6 note acknowledge, the raw exception message (status description) and now the FQ exception class name are shipped to clients. The message was already exposed pre-PR and the class name is low-sensitivity, so this is acceptable for the stated scope - just confirming it's a conscious, documented deferral rather than an oversight.

Nits

• GrpcErrorMapper's comment about ServerSecurityException not extending java.lang.SecurityException (falling through to INTERNAL if ever handed raw) is a good latent-gap callout; a one-line instanceof ServerSecurityException branch would future-proof it cheaply, but current call paths pre-map security so it's not required today.

Overall: correct, minimal, well-tested, and it delivers the stated fix (retry-on-NeedRetryException now fires over gRPC; permanent dup-key conflicts are no longer mis-typed as retryable). Recommend addressing #1 (the sync guard test) before merge since it protects the very behavior this PR adds; the rest are optional follow-ups.

Automated review by Claude. Verified the exception hierarchy (ConcurrentModificationException extends NeedRetryException extends ArcadeDBException; DuplicatedKeyException extends ArcadeDBException, confirming it is correctly non-retryable), the getDatabase security pre-mapping, and the removed-import cleanup. I did not run the build.

[claude]

Review: PR #5184 - preserve engine exception types across gRPC

Overall this is a clean, well-scoped fix. The central GrpcErrorMapper (server) / GrpcClientErrorMapper (client) pair mirrors what the HTTP protocol already does, the Base64 trailer handling is thoughtful, backward compatibility (trailer-less older servers fall back to status-code mapping) is handled on both sides, and the test coverage is excellent (unit tests for each mapper side in isolation plus a real server-backed round-trip). Nice work.

A few observations, none blocking:

1. Unused import (nit) - grpcw/.../GrpcErrorMapper.java:21
import com.arcadedb.exception.ConcurrentModificationException; is no longer referenced in code; CME is handled implicitly by the NeedRetryException branch and only appears in a comment (line 105). It compiles (unused imports are warnings, not errors) but should be dropped for cleanliness, especially since one commit was explicitly "drop unused import."

2. The "non-retryable duplicate key" framing is slightly imprecise
RemoteDatabase.transaction() retries on BOTH exception types (network/.../RemoteDatabase.java:244): catch (final NeedRetryException | DuplicatedKeyException e). So within transaction(), reconstructing a DuplicatedKeyException (vs a ConcurrentModificationException) does NOT change whether the block is retried; both retry, which is exactly what HTTP does too. The real win here is type fidelity: a user own catch (DuplicatedKeyException) outside transaction(), and the reconstructed indexName/keys, now work over gRPC as over HTTP. The description emphasis on "not retried forever" reads as if the type distinction gates the retry loop, which it does not. The code is correct; just worth aligning the narrative so a future reader does not assume mapping a new exception to a non-NeedRetryException type will stop transaction() from retrying it.

3. Trailer-less ALREADY_EXISTS fallback produces a doubly-nested message
In GrpcClientErrorMapper, when a class trailer is absent the fallback builds new DuplicatedKeyException(dupIndex(trailers, msg), dupKeys(trailers, msg), null) with msg (the full server description) passed as BOTH index and keys, producing a message like Duplicated key <full-msg> found on index <full-msg>. This only affects pre-upgrade servers and preserves information, so it is acceptable; a brief inline note that the duplication is intentional (or passing null for one arg) would help.

4. Security: message pass-through can leak internal detail
toStatusRuntimeException sends cause.getMessage() straight into the client-facing status description. This matches existing HTTP behavior and SEC-6 (message sanitization) is explicitly deferred, so it is fine for this PR; just flagging that the deferred item carries a real information-disclosure surface for the follow-up.

5. Round-trip test hardcodes port 50051
Issue5043GrpcDuplicatedKeyRoundTripTest binds a fixed 50051, which the PR body already acknowledges collides under concurrent local runs. Consistent with existing gRPC tests, so not introduced here, but a dynamic/ephemeral port would make the suite more robust.

Positives worth calling out

• Base64-encoding arbitrary index/key values so non-ASCII survives the ASCII-only metadata channel, with a decode fallback that never masks the original error, plus a test asserting the wire bytes stay [0x20-0x7E].
• The guard test reconstructFromClassName_literalsMatchActualClassNames turning a silent class-rename regression (switch on FQ-name string literals) into a red test is a genuinely good touch.
• Deliberately deferred findings (COR-1, SEC-6) are documented with the concrete reason (existing test contracts), the right call under the "never modify existing tests" constraint.

Note: I reviewed statically and did not run the module build myself; the PR reports both grpcw and grpc-client suites green.

[claude]

Review: fix(#5043) preserve engine exception types across gRPC

Solid, well-scoped fix. The central-mapper approach mirrors what HTTP already does, the trailer channel is the idiomatic gRPC way to carry error detail (no proto change), backward compatibility is handled cleanly (class-name trailer + status-code fallback), and the test coverage is genuinely good: unit tests for both mapper sides, a class-name-literal sync guard, non-ASCII round-trips, and a real server-backed wire test. Nicely documented in docs/5043-grpc-error-status-mapping.md. The Base64 encoding of index/keys trailers and the defensive decode-fallback are thoughtful touches.

A few observations, none blocking:

1. "Non-retryable" framing for DuplicatedKeyException is imprecise (docs, not code)

RemoteDatabase.transaction() catches both NeedRetryException and DuplicatedKeyException for retry:

// network/.../RemoteDatabase.java:244
} catch (final NeedRetryException | DuplicatedKeyException e) {
  // RETRY

So a commit-time duplicate key raised inside a transaction() block is still retried up to attempts times regardless of this PR - the reconstructed type does not stop the retry loop. The real, verified win is that (a) direct save()/commit() calls now surface the correct DuplicatedKeyException type (as the round-trip test asserts), and (b) after retries are exhausted, transaction() rethrows the correct type instead of a mis-typed ConcurrentModificationException. That matches HTTP behavior, which is the right goal - but the COR-3 wording ("not retried forever" / "non-retryable") over-claims for the transaction() path. Worth a one-line clarification so a future reader doesn't assume transaction() short-circuits on duplicate keys.

2. IllegalArgumentException now logged at SEVERE in the non-external createRecord path

Previously this branch had a dedicated catch (IllegalArgumentException e) that mapped to INVALID_ARGUMENT without SEVERE logging; the generic catch (Exception) (which logs SEVERE) only handled the rest. After the refactor both collapse into one catch (Exception) that logs SEVERE and then calls the mapper. Net effect: client-caused invalid-argument errors will now generate SEVERE server-log noise. Consider logging at FINE/WARNING for the INVALID_ARGUMENT/client-fault classes, or logging after mapping based on the resolved status code.

3. IllegalArgumentException type is not reconstructed on the client

The server maps IllegalArgumentException -> INVALID_ARGUMENT and ships the class trailer, but GrpcClientErrorMapper.reconstructFromClassName has no case for it, and the status-code fallback switch has no INVALID_ARGUMENT arm - so it becomes a generic RemoteException. This isn't a regression (the old code behaved the same), but since the server now deliberately maps it, adding case "java.lang.IllegalArgumentException" -> new IllegalArgumentException(msg) (and/or an INVALID_ARGUMENT fallback arm) would complete the parity the PR is aiming for.

4. Metadata key-name literals are duplicated across modules without a sync guard

EXCEPTION_CLASS_KEY / DUP_INDEX_KEY / DUP_KEYS_KEY names ("arcadedb-exception-class", etc.) are hard-coded on both the server and client sides. The class-name literals have a nice guard test (reconstructFromClassName_literalsMatchActualClassNames), but the metadata key strings do not - if one side's string is edited, reconstruction silently reverts to status-code mapping with no failing test. A tiny assertion pinning the three key names on each side would close that gap.

5. Minor: garbled message when class trailer present but dup trailers absent

In the mixed case (a server that emits the class-name trailer but not the dup index/keys trailers), dupIndex/dupKeys both fall back to the full msg, so the rebuilt exception message becomes Duplicated key <msg> found on index '<msg>'. Acceptable degradation and unlikely in practice, just noting it.

Nit

• The server-backed round-trip test binds a fixed port 50051 (the PR description itself notes a 50051 bind collision under concurrent local runs). A dynamically-allocated port would make it more robust, though I see this matches the existing gRPC test pattern.

Overall this is a clean, well-tested improvement that correctly restores type-driven retry behavior over gRPC. The points above are refinements rather than blockers.