fix(ha): reconstruct exact leader exception type on forwarded commands

[lvca]

Follow-up to #5014 (customer report: exceptions from the Leader not propagated as their real type on forwarded commands).

#5014 reconstructs a handful of leader-side exceptions on the Follower, but collapses retryable subtypes onto their NeedRetryException supertype. A caller doing catch (ConcurrentModificationException) (the specific subtype) would then miss it, and - more importantly - the retryable/non-retryable timeout distinction was lost.

What this does

RaftReplicatedDatabase.reconstructLeaderException now rebuilds the exact leader-side type via an explicit registry (LEADER_EXCEPTION_FACTORIES) instead of an if-chain that flattens types:

• ConcurrentModificationException is reconstructed as itself (still a NeedRetryException subtype, so retry logic is unchanged) so specific-type catches match.
• LockTimeoutException (retryable) and TimeoutException (non-retryable) are kept distinct - a query-deadline TimeoutException must not be turned into something retryable.
• Common command types (CommandExecutionException, CommandParsingException, ValidationException, SchemaException) are reconstructed faithfully too.
• DuplicatedKeyException keeps its structured 3-arg reconstruction; unknown classes still fall back to TransactionException.
• No reflection - the registry map is a safe, one-line-per-type extension point (avoids instantiating an arbitrary class name from a network response).

ArcadeDB's ConcurrentModificationException is referenced by FQN in the map because this file imports java.util.ConcurrentModificationException.

Tests

RaftReplicatedDatabaseTest: strengthened the CME test to assert the exact type, added reconstructLeaderExceptionLockTimeoutIsRetryable and reconstructLeaderExceptionTimeoutIsNotRetryable. 16 tests, all passing.

Tests run: 16, Failures: 0, Errors: 0, Skipped: 0 -- in com.arcadedb.server.ha.raft.RaftReplicatedDatabaseTest

Note (not addressed here)

RaftReplicatedDatabase line ~390 does if (e instanceof ConcurrentModificationException) where the name resolves to the JDK java.util type (via the existing import), while the engine throws ArcadeDB's com.arcadedb.exception.ConcurrentModificationException for page-version conflicts. That may be a latent dead branch in the phase-2 "step down" log path - unrelated to this change, flagged for a separate look.

[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 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 100.00% diff coverage · -7.22% coverage variation

Metric	Results
Coverage variation	✅ -7.22% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (10e98c5)	134787	100310	74.42%
Head commit (7083d02)	166601 (+31814)	111958 (+11648)	67.20% (-7.22%)

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 (#5017)	16	16	100.00%

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]

Code Review

This pull request refactors exception reconstruction on the follower side in RaftReplicatedDatabase. It introduces a registry of leader-side exception factories (LEADER_EXCEPTION_FACTORIES) to reconstruct the exact exception types (such as LockTimeoutException and TimeoutException) instead of collapsing them into generic supertypes. This preserves correct retry semantics and allows callers to catch specific exception types. Additionally, unit tests have been added to verify the reconstruction behavior and retry eligibility of these exceptions. I have no feedback to provide.

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: fix(ha): reconstruct exact leader exception type on forwarded commands

Overall this is a clean, well-scoped follow-up to PR 5014. Replacing the flattening if-chain with an explicit LEADER_EXCEPTION_FACTORIES registry is the right call: it preserves both the exact catchable type and the retryable/non-retryable distinction, and it avoids reflectively instantiating an arbitrary class name off a network response (good security instinct). The Javadoc is thorough and the added tests pin the two semantics that matter most (LockTimeoutException retryable, TimeoutException not).

Correctness (verified)

• Every registered type has a public single-String constructor, so the Function<String,RuntimeException> method references are all valid: NeedRetryException, ArcadeDB ConcurrentModificationException, LockTimeoutException, TimeoutException, TransactionException, CommandExecutionException, CommandParsingException, ValidationException, SchemaException. Verified.
• The map keys are FQNs, and the leader serializes exception.getClass().getName() in AbstractServerHttpHandler.error2json (line 566), so the keys match what actually arrives on the wire. Verified.
• LockTimeoutException extends NeedRetryException and ArcadeDB ConcurrentModificationException extends NeedRetryException, while TimeoutException extends ArcadeDBException (not NeedRetryException), so the retry-semantics claims in the tests and Javadoc hold. Verified.
• Ordering is preserved: the DuplicatedKeyException structured branch still runs before the map lookup, and DuplicatedKeyException is intentionally absent from the map. Verified.

The self-flagged line ~397 branch is a real dead branch (worth a tracked follow-up)
The PR note is correct and I confirmed it: at line 397 if (e instanceof ConcurrentModificationException) resolves to java.util.ConcurrentModificationException (import at line 98), but commit2ndPhase page-version conflicts throw ArcadeDB's com.arcadedb.exception.ConcurrentModificationException (thrown from PageManager / TransactionContext). So that phase-2 step-down SEVERE-log branch never fires for the case its message describes. Out of scope for this PR, but please open a follow-up issue so it does not get lost - the fix is a one-token FQN change, and it currently masks a step-down-to-prevent-stale-reads diagnostic that is explicitly about page-version conflicts.

Minor / optional

• Coverage gap (not a regression): a few other common command exceptions still fall back to a generic TransactionException because they are not in the map: CommandSQLParsingException, CommandSemanticException, QueryNotIdempotentException. QueryNotIdempotentException in particular is one a caller might reasonably want to catch by type. Consider adding them (one line each) or noting them as deliberately excluded.
• Map.of limit: Map.of(...) supports at most 10 key/value pairs; this uses 9. If the registry grows past 10 it will need Map.ofEntries(...). A brief comment on the map would save a future contributor a surprise compile error.
• DuplicatedKeyException fallthrough: when exceptionArgs is null/malformed it drops through to a generic TransactionException (loses the specific type). Pre-existing behavior, just flagging that the structured path is the only way to recover that type.

Tests
Good additions - the new tests assert both the concrete type and the (non-)NeedRetryException-ness, which is exactly the behavior this PR protects. The strengthened CME test now asserts the exact subtype. I could not run Maven in this review environment, but the author reports 16/16 passing and the assertions are consistent with the verified constructor/hierarchy facts above.

Nice, surgical change. The only thing I would ask before merge is filing the line-397 follow-up so that latent dead branch stays visible.

[claude]

Review: fix(ha): reconstruct exact leader exception type on forwarded commands

Solid, well-scoped follow-up to #5014. I verified the core claims against the codebase and the change is correct. 👍

Correctness (verified)

• The exception hierarchy the PR relies on checks out:
  • LockTimeoutException extends NeedRetryException → stays retryable ✅
  • ConcurrentModificationException extends NeedRetryException → stays retryable ✅
  • TimeoutException extends ArcadeDBException (not NeedRetryException) → correctly kept non-retryable ✅
• Every type in LEADER_EXCEPTION_FACTORIES has a public single-arg (String) constructor, so the Type::new method references resolve unambiguously to Function<String, RuntimeException> even for the types that also declare (String, Throwable) / (Throwable) overloads (ValidationException, CommandExecutionException, CommandParsingException) - the (Throwable) overload isn't applicable to a String arg.
• Complete coverage of retryable subtypes: the only two NeedRetryException subtypes in the engine are LockTimeoutException and ConcurrentModificationException, and both are now in the registry. So no retryable leader exception can silently collapse to a non-retryable TransactionException on the follower.
• Registry entries are reachable, not dead code: the leader serializes the runtime type via exception.getClass().getName() in AbstractServerHttpHandler.error2json (line 566), matching the FQN keys here.
• Behavior parity with the removed if-chain is preserved: NeedRetryException and TransactionException still use detail != null ? detail : message, and the previous literal-string check for the CME FQN now maps to ...ConcurrentModificationException.class.getName() (same string) but reconstructs the exact type instead of the supertype - the intended improvement.

Design / security

• Preferring an explicit registry map over reflectively instantiating an arbitrary class name from a network response is the right call - it avoids turning an attacker-influenced exception field into a Class.forName(...).newInstance(...) gadget. Good instinct, and the Javadoc documents it well.
• No reflection, one-line-per-type extension point, clear comments - easy to maintain.

Tests

• Good coverage: the strengthened CME test now asserts the exact subtype, and the two new tests lock in the retryable-vs-non-retryable distinction (LockTimeout retryable, Timeout not). These directly guard the regression fix: DuplicatedKeyException not propagated as cause on Follower forwarded commands #5014 introduced.

Minor observations (non-blocking, mostly future-proofing)

1. Other command-path types still collapse to TransactionException. Types the leader can realistically throw but that aren't in the registry (e.g. RecordNotFoundException, SecurityException, IllegalArgumentException, IndexException) still lose their specific type on the follower. Not a regression and out of scope, but worth a follow-up if callers ever catch those across the forward boundary.
2. DuplicatedKeyException with malformed exceptionArgs (null, not 3 parts, or bad RID) still falls through to the generic TransactionException fallback rather than a plain DuplicatedKeyException(message), losing the type. Pre-existing behavior - just flagging it since it's the one registered type that can degrade.
3. The self-flagged JDK-vs-ArcadeDB ConcurrentModificationException ambiguity at ~line 390 (the instanceof referencing java.util via the existing import while the engine throws com.arcadedb.exception.*) is a real latent dead-branch smell - good to have called it out for a separate PR.

Nothing here blocks merge. Nice, careful change with tests that pin the exact semantics.

[claude]

Review: reconstruct exact leader exception type on forwarded commands

Reviewed the diff against RaftReplicatedDatabase and the exception hierarchy. This is a clean, well-motivated follow-up to #5014 - the registry approach is the right call and the JavaDoc is genuinely helpful. A few notes below, mostly confirmations plus one worthwhile flag.

Correctness - verified ✅

• Every type in LEADER_EXCEPTION_FACTORIES has a public single-String constructor, so all the ::new method references resolve: NeedRetryException, ConcurrentModificationException, LockTimeoutException, TimeoutException, TransactionException, CommandExecutionException, CommandParsingException, CommandSQLParsingException, CommandSemanticException, QueryNotIdempotentException, ValidationException, SchemaException.
• Retry semantics are preserved: LockTimeoutException and ConcurrentModificationException both extend NeedRetryException (stay retryable); TimeoutException extends ArcadeDBException directly (stays non-retryable). The two new tests lock this in nicely.
• The leader side genuinely sends the concrete class name: AbstractServerHttpHandler.error2json does json.put("exception", exception.getClass().getName()), so keying the map by getName() matches the exact subtype. Good.
• No regression vs the old if-chain: the previous code collapsed CME to NeedRetryException; now it reconstructs CME itself (still a NeedRetryException subtype), so existing retry loops are unaffected while specific-type catches start matching.

Security ✅

Avoiding reflection here is the correct instinct - never instantiate an arbitrary class name from a network response. The explicit allow-list map is the safe pattern.

The flagged line-435 branch - confirmed a real dead branch

The PR note is accurate and I'd argue it's close enough to fix here rather than defer. Confirmed:

• RaftReplicatedDatabase line 101 imports java.util.ConcurrentModificationException, so the if (e instanceof ConcurrentModificationException) at line 435 tests against the JDK type.
• The engine throws com.arcadedb.exception.ConcurrentModificationException for page-version conflicts (TransactionContext.java:996/999/1052, imports the ArcadeDB type).

So the phase-2 "page version conflict / stepping down" SEVERE log branch can never be taken - it always falls through to the generic message. This is the exact same import-shadowing hazard this PR works around in the map (via FQN). A one-line fix - referencing com.arcadedb.exception.ConcurrentModificationException by FQN at line 435 - would resolve it and is thematically identical to this change. Reasonable to keep separate, but flagging that it is confirmed dead, not just "may be".

Minor / optional

• CommandSemanticException is in the map but has no dedicated test (the SQL-parsing test covers a sibling subtype). Cheap to add one for symmetry, not blocking.
• Types leaders can throw that aren't in the map (e.g. RecordNotFoundException, IndexException) still collapse to TransactionException. Not a regression - same as before - and the map is a trivial extension point, so this is just a "future" note.
• Function<String, RuntimeException> as the value type (rather than Function<String, ArcadeDBException>) is fine given the fallback returns RuntimeException; all current factories are ArcadeDBException subtypes anyway.

Test coverage ✅

Good targeted coverage of the behaviors that matter: exact-type preservation, retryable vs non-retryable timeout distinction, and subtype-catch matching.

Nice, focused change. Only substantive suggestion is to consider folding the line-435 FQN fix into this PR since it's the same class of bug.

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.43%. Comparing base (10e98c5) to head (7083d02).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...rcadedb/server/ha/raft/RaftReplicatedDatabase.java	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5017      +/-   ##
============================================
- Coverage     65.43%   65.43%   -0.01%     
+ Complexity      825      822       -3     
============================================
  Files          1687     1687              
  Lines        134787   134798      +11     
  Branches      28825    28822       -3     
============================================
+ Hits          88203    88205       +2     
- Misses        34514    34520       +6     
- Partials      12070    12073       +3     

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