#2043 High Availability (HA) Subsystem Improvement

[robfrank]

This pull request addresses several critical issues and improvements in the High Availability (HA) subsystem, focusing on bug fixes, protocol consistency, and test reliability. The main changes include fixing alias resolution for cluster discovery, correcting type mismatches in server removal, re-enabling HTTP address propagation for client redirection, and fixing test assertions in resilience scenarios. Additionally, the CI workflow is updated to properly handle resilience tests.

HA Subsystem Bug Fixes and Improvements:

• Alias Resolution

  • Implemented a resolveAlias() method in HAServer.java to ensure that server addresses containing aliases (e.g., {arcade2}proxy:8667) are correctly resolved to actual host:port values during leader connection, fixing cluster formation issues in Docker/K8s environments.
  • Added comprehensive tests for alias resolution, including edge cases and multiple alias scenarios.

• Server Removal Consistency

  • Updated the removeServer() method in HAServer.java to accept ServerInfo objects instead of String, aligning with the migration to ServerInfo-based replica connection maps. A backward-compatible method accepting String was also added.
  • Added a helper method findByHostAndPort() to HACluster for easier ServerInfo lookups and fixed enum naming conventions for consistency.

• HTTP Address Propagation

  • Re-enabled and updated the mechanism for propagating HTTP addresses from replicas to the leader, allowing clients to be redirected to the correct replica HTTP endpoints. This includes protocol changes where replicas send their HTTP address to the leader during connection, and the leader tracks these addresses in a map.
  • Updated GetServerHandler to return the correct HTTP addresses for client redirection and ensured cleanup of HTTP address mappings when servers are removed.

Test Reliability and Workflow:

• Test Assertion Correction

  • Fixed incorrect assertions in the ThreeInstancesScenarioIT test by removing assertions on a disconnected node (arcade1) and clarifying comments to reflect the actual test scenario, ensuring the test only checks nodes that are connected and can replicate data.

• CI Workflow Update

  • Modified the GitHub Actions workflow to exclude the resilience module from the main integration test job and introduced a dedicated java-resilience-tests job to run resilience-specific tests separately. [1] [2]

These changes collectively improve the robustness, correctness, and maintainability of the HA subsystem and associated tests.

Related issues

#2043

Checklist

• I have run the build using mvn clean package command
• My unit tests cover both failure and success scenarios

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -6.11%	✅ 19.74%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (770d240)	103881	57037	54.91%
Head commit (0576387)	105567 (+1686)	51515 (-5522)	48.80% (-6.11%)

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 (#2062)	2153	425	19.74%

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%

See your quality gate settings    Change summary preferences

[claude]

Pull Request Review: #2062 - High Availability (HA) Subsystem Improvements

Executive Summary

This PR makes significant improvements to ArcadeDB's High Availability (HA) subsystem, addressing critical bugs in cluster discovery, server removal, and HTTP address propagation. The changes span 201 files with 33,374 additions and include new infrastructure for split-brain prevention, circuit breakers, and consistency monitoring. Overall code quality is good, but there are several critical thread safety concerns and incomplete implementations that should be addressed before merging.

---

1. Critical Issues 🔴

1.1 Thread Safety Violation in HAServer.receivedResponse() (HIGH PRIORITY)

File: server/src/main/java/com/arcadedb/server/ha/HAServer.java:641

Issue:

public void receivedResponse(final ServerInfo remoteServerName, final long messageNumber, final Object payload) {
    // ...
    final Leader2ReplicaNetworkExecutor c = replicaConnections.get(remoteServerName);
    if (c != null)
      c.updateStats(msg.sentOn, receivedOn);
}

The method signature changed from String to ServerInfo, but replicaConnections is keyed by String (server name/alias). This will cause c to always be null, breaking latency tracking.

Impact: Latency statistics won't be recorded, breaking monitoring and performance analysis.

Fix Required: Extract the alias from ServerInfo:

final Leader2ReplicaNetworkExecutor c = replicaConnections.get(remoteServerName.alias());

1.2 Race Condition in Cluster Configuration Updates

File: server/src/main/java/com/arcadedb/server/ha/HAServer.java:738-781

Issue: The registerIncomingConnection() method reads this.cluster without synchronization, then uses it in a lambda:

final HACluster currentCluster = this.cluster;  // Line 738 - no synchronization
serverInfoByName.compute(serverName, (name, existingInfo) -> {
    if (currentCluster != null) {  // Line 741 - race condition
        // ...
    }
});

Between line 738 and 741, another thread could call setServerAddresses() and replace this.cluster, causing inconsistencies.

Impact: Cluster configuration could become inconsistent during concurrent replica connections or cluster updates.

Recommendation: Synchronize cluster access or use AtomicReference<HACluster>.

1.3 Incomplete ConsistencyMonitor Implementation

File: server/src/main/java/com/arcadedb/server/ha/ConsistencyMonitor.java:294-327

Issue: The consistency monitor only collects checksums from the leader, not from replicas:

// TODO: Collect replica checksums
// This requires implementing a new replication protocol command

Impact: The feature is non-functional - it cannot detect drift without comparing leader vs replica checksums. Currently enabled by default in some configurations but does nothing useful.

Recommendation: Either:

1. Implement the replica checksum protocol before enabling
2. Disable by default until complete (HA_CONSISTENCY_CHECK_ENABLED = false)
3. Add a startup warning if enabled without replica support

---

2. Thread Safety Analysis ⚠️

2.1 Positive: Good Use of Thread-Safe Patterns

• ✅ ConcurrentHashMap for replicaConnections, serverInfoByName, replicaHTTPAddresses
• ✅ AtomicReference for leaderConnection
• ✅ Separate locks for channel I/O (channelInputLock, channelOutputLock)
• ✅ LeaderFence uses AtomicReference with CAS loop (lines 158-178)
• ✅ ReplicaCircuitBreaker uses AtomicInteger and volatile state

2.2 Concerns:

Potential deadlock risk in Leader2ReplicaNetworkExecutor
The executor has multiple locks (lock, channelInputLock, channelOutputLock). While currently safe, ensure consistent lock ordering to prevent future deadlocks.

---

3. Bug Analysis 🐛

3.1 Fixed Bugs (Verified)

✅ Alias Resolution (Issue #2945)

The resolveAlias() method correctly handles Docker/K8s scenarios where aliases like {arcade2}proxy:8667 need resolution. Implementation looks correct.

✅ Server Removal Type Consistency

The overloaded methods correctly handle both String and ServerInfo.

✅ HTTP Address Propagation

Re-enabled and correctly implemented. Leader tracks replica HTTP addresses for client redirection.

3.2 Potential New Bugs

⚠️ Memory Leak in Circuit Breaker

File: Leader2ReplicaNetworkExecutor.java:93

The ReplicaCircuitBreaker instance is created but never explicitly cleaned up. Since it is stored in the executor and executors are in a map, old circuit breakers could accumulate if replicas reconnect frequently.

Recommendation: Ensure close() method nulls the circuit breaker reference.

⚠️ SQL Injection Risk in ConsistencyMonitor

File: ConsistencyMonitor.java:247-256

While there is validation, it is not exhaustive. The regex allows underscores, which could enable edge cases. More critically, samplesPerType is multiplied but not validated as positive.

Impact: Low risk (internal use only), but should be hardened.

---

4. Performance Considerations ⚡

4.1 Good Optimizations

• ✅ WAL semantics: Replication log written before sending to replicas
• ✅ Back-pressure mechanism with configurable timeouts
• ✅ Circuit breaker prevents cascading failures

4.2 Performance Concerns

Queue Overflow Handling

File: Leader2ReplicaNetworkExecutor.java:582-596

When the queue overflows, the replica is removed from the cluster entirely. This is very aggressive. A temporary spike in load could permanently remove a healthy replica.

Recommendation: Consider:

1. Adding a grace period or retry mechanism
2. Logging a critical alert before removal
3. Allowing automatic rejoin after queue clears

ConsistencyMonitor Sampling

File: ConsistencyMonitor.java:229-282

The sampling algorithm is inefficient: fetches 2x the required data, then randomly discards half. On large types, this wastes I/O.

Recommendation: Use SKIP random() or ORDER BY rand() for better sampling.

---

5. Test Coverage Analysis 🧪

5.1 Strengths

• ✅ 46 HA test classes found in server/src/test/java/com/arcadedb/server/ha/
• ✅ New e2e-ha module for integration tests (9 test classes)
• ✅ Specific tests for new features

5.2 Gaps

Missing Tests

1. LeaderFence split-brain scenarios - No dedicated test found
2. ConcurrentModificationException handling - The forced full resync path needs verification
3. Election cooldown - New feature needs test coverage
4. HTTP address cleanup on removeServer - Should be tested

---

6. Security Analysis 🔒

6.1 Positive Aspects

• ✅ SQL injection protection in ConsistencyMonitor (basic validation)
• ✅ Leader fencing prevents unauthorized writes from old leaders
• ✅ Epoch-based message validation prevents replay attacks

6.2 Concerns

Stack Overflow / OOM Handling

File: Replica2LeaderNetworkExecutor.java:182-206

Printing stack traces to stderr could leak sensitive information in production. Should use structured logging only.

---

7. Code Quality Assessment 📊

7.1 Strengths

• ✅ Excellent documentation (Javadoc on all new classes)
• ✅ Clear separation of concerns (LeaderFence, LeaderEpoch, CircuitBreaker are separate)
• ✅ Good use of Java records (LeaderEpoch, ServerInfo)
• ✅ Consistent error handling patterns
• ✅ Comprehensive logging with appropriate levels

7.2 Improvement Areas

Commented-Out Code

File: e2e-ha/pom.xml:129-182

Large blocks of commented XML should be removed.

Diagnostic Logging

File: Replica2LeaderNetworkExecutor.java:81-86

Diagnostic logging statements should be removed or changed to FINE level before merging.

Magic Numbers

• Line 158 in LeaderFence.java: maxAttempts = 100 (CAS retry limit) - should be configurable
• Line 337 in Replica2LeaderNetworkExecutor.java: maxWaitMs = 10000 - should be a constant

---

8. Configuration Management ⚙️

The PR adds many new GlobalConfiguration settings (HA_CONSISTENCY_CHECK_ENABLED, HA_CIRCUIT_BREAKER_ENABLED, etc.).

Concern: No documentation update found in /docs/ folder for these new settings.

Recommendation: Add configuration documentation before merging.

---

9. Recommendations

Before Merge (CRITICAL)

1. ✅ Fix HAServer.receivedResponse() bug - Change get(remoteServerName) to get(remoteServerName.alias())
2. ⚠️ Synchronize cluster access in registerIncomingConnection()
3. ❌ Disable or complete ConsistencyMonitor - Current implementation is non-functional
4. 🧹 Remove diagnostic logging from production code
5. 📝 Add configuration documentation for new settings

Before Merge (HIGH PRIORITY)

6. 🧪 Add tests for:
   • LeaderFence split-brain scenarios
   • Election cooldown behavior
   • ConcurrentModificationException → full resync path
7. 🔒 Remove e.printStackTrace(System.err) calls
8. 🧹 Clean up commented-out code in e2e-ha/pom.xml

Post-Merge Improvements

9. 📊 Monitor circuit breaker effectiveness in production
10. 🔄 Consider less aggressive queue overflow handling
11. 🎯 Optimize ConsistencyMonitor sampling algorithm
12. 📖 Update architecture documentation with new split-brain prevention

---

10. Overall Assessment

Category	Rating	Notes
Code Quality	8/10	Excellent structure, minor cleanup needed
Thread Safety	6/10	Good patterns but 2-3 critical issues
Performance	8/10	Good optimizations, some aggressive policies
Security	7/10	Leader fencing is great, minor hardening needed
Test Coverage	7/10	Good breadth, some critical paths untested
Documentation	6/10	Excellent inline docs, missing config docs

Overall Recommendation: ⚠️ APPROVE WITH CHANGES

The PR makes substantial improvements to HA reliability and includes well-designed split-brain prevention. However, 3 critical bugs must be fixed before merge:

1. receivedResponse() parameter type mismatch
2. Cluster configuration race condition
3. Non-functional ConsistencyMonitor

Once these are addressed, this will be a strong improvement to ArcadeDB's HA subsystem.

---

11. Positive Highlights ⭐

• Excellent architecture for leader fencing using epochs
• Well-designed circuit breaker pattern
• Comprehensive error categorization for better recovery strategies
• Good separation of concerns with new helper classes
• Extensive test coverage (46 tests)
• WAL semantics properly maintained
• Clear upgrade path from old protocol

Great work overall! The foundation is solid - just needs the critical fixes before merging.