[WIP] HA failover

[alpe]

🚧 WIP - early version

Integration tests are failing and some unit tests are missing

Overview

[github-actions]

The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Nov 10, 2025, 1:27 PM

[codecov]

Codecov Report

❌ Patch coverage is 34.65517% with 758 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.88%. Comparing base (3a9c9ba) to head (c7952fe).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/raft/node.go	0.00%	245 Missing ⚠️
pkg/raft/node_mock.go	50.94%	60 Missing and 18 partials ⚠️
block/internal/syncing/raft_retriever.go	0.00%	74 Missing ⚠️
pkg/rpc/server/http.go	1.66%	58 Missing and 1 partial ⚠️
node/full.go	28.20%	49 Missing and 7 partials ⚠️
node/failover.go	66.66%	38 Missing and 13 partials ⚠️
pkg/rpc/client/raft_cluster.go	0.00%	46 Missing ⚠️
block/internal/executing/executor.go	4.76%	35 Missing and 5 partials ⚠️
block/internal/syncing/syncer.go	29.41%	34 Missing and 2 partials ⚠️
pkg/raft/election.go	75.86%	15 Missing and 6 partials ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2814      +/-   ##
==========================================
- Coverage   64.79%   59.88%   -4.92%     
==========================================
  Files          81       89       +8     
  Lines        7224    10079    +2855     
==========================================
+ Hits         4681     6036    +1355     
- Misses       2003     3470    +1467     
- Partials      540      573      +33     

Flag	Coverage Δ
combined	59.88% <34.65%> (-4.92%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

[alpe]

@claude review this PR

[claude]

Claude finished @alpe's task —— View job

---

Code Review: HA Failover Implementation

Comprehensive code review completed for the HA failover implementation based on Raft consensus.

Executive Summary

This PR implements high-availability failover using HashiCorp's Raft consensus library. The implementation adds Raft-based leader election, state replication, and automatic failover for sequencer nodes. While the architecture is sound, there are several critical issues that need attention, particularly around error handling, testing coverage, security, and performance.

Current Status: Integration tests failing, 630 lines missing test coverage (36% patch coverage)

---

1. Architecture & Code Quality

✅ Strengths

1. Clean separation of concerns - Raft logic isolated in pkg/raft/, integration in node/failover.go
2. Interface-based design - Good use of Runnable interface for leader/follower modes
3. Factory pattern for mode switching - Clean approach to dynamic role changes
4. Modular configuration - Raft config properly separated in pkg/config/config.go

⚠️ Issues & Recommendations

Critical: State Serialization

pkg/raft/node.go:238, pkg/raft/types.go:5

data, err := json.Marshal(state) // todo:use protobuf

Issue: Using JSON for Raft state replication introduces multiple problems:

• No schema evolution support
• Larger payload sizes (impacts network & disk)
• Slower marshaling/unmarshaling
• No forward/backward compatibility guarantees

Recommendation: Implement protobuf serialization ASAP. This is marked as TODO but is critical for production use. Add this to types/pb/evnode/v1/ and update RaftBlockState to use proto.

---

Critical: State Validation Logic

pkg/raft/types.go:16-24

func (s RaftBlockState) assertValid(next RaftBlockState) error {
    if s.Height > next.Height {
        return fmt.Errorf("invalid height: %d > %d", s.Height, next.Height)
    }
    if s.Timestamp > next.Timestamp {
        return fmt.Errorf("invalid timestamp: %d > %d", s.Timestamp, next.Timestamp)
    }
    return nil
}

Issues:

1. Incomplete validation: Comment says "does not ensure that no gaps exist or chain continuity" but gaps could cause catastrophic state divergence
2. Missing hash chain validation: Should verify next properly references s via hash
3. No bounds checking: Heights should increment by exactly 1 (or document why gaps are acceptable)
4. Timestamp validation too weak: Should enforce minimum progression, not just monotonicity

Recommendation:

func (s RaftBlockState) assertValid(next RaftBlockState) error {
    // Height must increment by exactly 1 (no gaps)
    if next.Height != s.Height + 1 {
        return fmt.Errorf("height gap detected: current=%d next=%d", s.Height, next.Height)
    }
    
    // Verify hash chain continuity
    if !bytes.Equal(next.PrevHash, s.Hash) {
        return fmt.Errorf("hash chain broken at height %d", next.Height)
    }
    
    // Enforce meaningful timestamp progression
    if next.Timestamp <= s.Timestamp {
        return fmt.Errorf("timestamp must increase: %d -> %d", s.Timestamp, next.Timestamp)
    }
    
    // Add max timestamp drift check to prevent far-future timestamps
    return nil
}

---

Major: Raft Node Initialization Error Handling

pkg/raft/node.go:56-113

Issues:

1. Resource leaks on error: If NewRaft fails, the transport and stores are not closed
2. Directory permissions too permissive: 0755 for raft data directory
3. Error propagation loses context: Some errors don't wrap original error

Recommendation:

func NewNode(cfg *Config, clusterClient clusterClient, logger zerolog.Logger) (*Node, error) {
    if err := os.MkdirAll(cfg.RaftDir, 0700); err != nil { // More restrictive
        return nil, fmt.Errorf("create raft dir: %w", err)
    }
    
    // ... setup stores and transport ...
    
    // Use deferred cleanup on error
    var cleanup []func()
    defer func() {
        if err != nil {
            for _, fn := range cleanup {
                fn()
            }
        }
    }()
    
    cleanup = append(cleanup, func() { logStore.Close() })
    cleanup = append(cleanup, func() { stableStore.Close() })
    cleanup = append(cleanup, func() { transport.Close() })
    
    // ... rest of initialization
}

---

Major: Race Condition in FSM Apply

pkg/raft/node.go:316-322

if f.applyCh != nil {
    select {
    case f.applyCh <- RaftApplyMsg{Index: log.Index, State: &state}:
    default:
        f.logger.Warn().Msg("apply channel full, dropping message")
    }
}

Issues:

1. Silent message drops: Dropping Raft apply messages can cause state divergence between Raft FSM and application state
2. No backpressure mechanism: Should block or return error instead of dropping
3. Channel capacity not documented: What's the expected capacity?

Recommendation:

// Either block (preferred for correctness):
if f.applyCh != nil {
    f.applyCh <- RaftApplyMsg{Index: log.Index, State: &state}
}

// OR return error to Raft (will cause node to fail):
if f.applyCh != nil {
    select {
    case f.applyCh <- msg:
    case <-time.After(applyTimeout):
        return fmt.Errorf("apply channel blocked for %s", applyTimeout)
    }
}

---

Major: Election Logic - Sync Check Race

pkg/raft/election.go:90-101

if becameLeader && !isCurrentlyLeader {
    if isStarted {
        time.Sleep(d.node.Config().SendTimeout) // Wait for in-flight messages
        if !runnable.IsSynced(d.node.GetState()) {
            if err := d.node.leadershipTransfer(); err != nil && !errors.Is(err, raft.ErrNotLeader) {
                return err
            }
            continue
        }
        // ...
    }
}

Issues:

1. Hard-coded sleep is fragile: SendTimeout may not be sufficient for all in-flight messages
2. TOCTOU race: State could change between IsSynced check and proceeding as leader
3. Infinite transfer loop: If all nodes are unsynced, this could loop forever
4. No transfer timeout: leadershipTransfer() might block indefinitely

Recommendation:

• Add max retry count for leadership transfer
• Use exponential backoff instead of fixed sleep
• Add timeout to leadership transfer
• Consider using Raft log index to verify sync instead of height comparison

---

2. Error Handling & Resilience

⚠️ Critical Issues

Shutdown Error Swallowing

node/failover.go:167-221

The Run method properly collects multiple errors but:

1. HTTP server errors ignored on graceful shutdown - Only ErrServerClosed is expected, but other errors during shutdown should be logged
2. P2P client close error ignored - Line 196: defer f.p2pClient.Close() // nolint: errcheck should collect the error
3. Inconsistent error accumulation - Some places use errors.Join, others silently ignore

Recommendation: Consistently collect all errors and include them in the return value.

---

RPC Cluster Client Error Handling

pkg/rpc/client/raft_cluster.go:39-68

func (r RaftClusterClient) broadcast(ctx context.Context, path string, obj any) error {
    var peerErrs []error
    for _, peer := range r.peers {
        // ... attempt to contact peer ...
        if resp.StatusCode != http.StatusOK {
            peerErrs = append(peerErrs, fmt.Errorf("unexpected status: %s", resp.Status))
            continue
        }
    }
    if len(peerErrs) == len(r.peers) {
        return errors.Join(peerErrs...)
    }
    return nil // SUCCESS if ANY peer succeeds
}

Issues:

1. Dangerous partial success semantics: Returns nil if ANY peer succeeds, even if most fail. This can cause split-brain scenarios.
2. No quorum enforcement: Should require majority (or all) peers to succeed for Raft operations
3. Timeout is global: 3-second timeout applies to all peers sequentially, not per-peer
4. No retry logic: Transient network errors cause permanent failures

Recommendation:

// Broadcast to all peers in parallel with per-peer timeout
func (r RaftClusterClient) broadcast(ctx context.Context, path string, obj any) error {
    type result struct {
        peer string
        err  error
    }
    
    results := make(chan result, len(r.peers))
    ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
    defer cancel()
    
    for _, peer := range r.peers {
        go func(p string) {
            results <- result{peer: p, err: r.sendToPeer(ctx, p, path, obj)}
        }(peer)
    }
    
    var successCount, failCount int
    var errs []error
    for i := 0; i < len(r.peers); i++ {
        res := <-results
        if res.err != nil {
            failCount++
            errs = append(errs, fmt.Errorf("%s: %w", res.peer, res.err))
        } else {
            successCount++
        }
    }
    
    // Require majority for Raft safety
    if successCount < len(r.peers)/2+1 {
        return fmt.Errorf("failed to reach quorum (%d/%d): %w", 
            successCount, len(r.peers), errors.Join(errs...))
    }
    return nil
}

---

3. Security Concerns

🔒 Critical Security Issues

Raft RPC Endpoint Authentication Missing

pkg/rpc/server/http.go:130-194

The /raft/join and /raft/remove endpoints have NO authentication:

mux.HandleFunc("/raft/join", func(w http.ResponseWriter, r *http.Request) {
    // ... anyone can call this and join the cluster
    if err := raftNode.AddPeer(rsp.NodeID, rsp.Address); err != nil {
        http.Error(w, "failed to join peer", http.StatusInternalServerError)
    }
})

Impact: Any attacker with network access can:

1. Join malicious nodes to the Raft cluster
2. Remove legitimate nodes from the cluster
3. Cause denial of service
4. Potentially achieve consensus takeover

Recommendation:

• Add TLS mutual authentication for Raft endpoints
• Require pre-shared secret or JWT token for cluster operations
• Implement IP allowlisting for Raft management endpoints
• Add rate limiting to prevent abuse

---

Raft Directory Permissions

pkg/raft/node.go:57

if err := os.MkdirAll(cfg.RaftDir, 0755); err != nil {

Issue: Directory is world-readable (0755), exposing Raft state including potentially sensitive data.

Recommendation: Use 0700 to restrict access to owner only.

---

Snapshot Store Writes to stderr

pkg/raft/node.go:84,94

snapshotStore, err := raft.NewFileSnapshotStore(cfg.RaftDir, int(cfg.SnapCount), os.Stderr)
transport, err := raft.NewTCPTransport(cfg.RaftAddr, addr, 3, 10*time.Second, os.Stderr)

Issue: Error output goes to stderr which may be logged. Could leak sensitive state information.

Recommendation: Use io.Discard or a dedicated logger with filtering.

---

4. Performance & Resource Management

⚠️ Performance Concerns

Blocking State Operations

block/internal/syncing/raft_retriever.go:82-96

The raftApplyLoop processes messages sequentially:

for {
    select {
    case <-ctx.Done():
        return
    case msg := <-applyCh:
        if err := r.consumeRaftBlock(ctx, msg.State); err != nil {
            r.logger.Error().Err(err).Uint64("height", msg.State.Height).Msg("failed to apply raft block")
        }
    }
}

Issues:

1. Single-threaded processing: One slow block blocks all subsequent blocks
2. No timeout on consumeRaftBlock: Could block indefinitely
3. Errors are only logged: Failed blocks should halt the node or trigger catchup

Recommendation:

• Add timeout to block processing
• Consider parallel processing with ordering guarantees
• Implement circuit breaker for persistent failures

---

Memory Allocation in Hot Path

pkg/raft/node.go:238-248

func (n *Node) Broadcast(_ context.Context, state *RaftBlockState) error {
    data, err := json.Marshal(state) // Allocation every block
    if err != nil {
        return fmt.Errorf("marshal block state: %w", err)
    }
    future := n.raft.Apply(data, n.config.SendTimeout)
    // ...
}

Issue: JSON marshaling allocates on every block broadcast. For high-throughput chains, this creates GC pressure.

Recommendation:

• Use sync.Pool for marshal buffers
• Pre-allocate buffer based on average block size
• Switch to protobuf which is more efficient

---

No Backpressure on Block Production

block/internal/executing/executor.go

When Raft becomes a bottleneck, there's no backpressure mechanism to slow block production. This can lead to:

• Memory exhaustion from buffered blocks
• Raft log growth
• Increased failover time

Recommendation: Monitor Raft lag and implement backpressure when lag exceeds threshold.

---

5. Testing & Reliability

❌ Major Testing Gaps

Coverage Analysis (from codecov report):

File	Coverage	Missing Lines	Priority
pkg/raft/node.go	0.00%	202	🔴 CRITICAL
block/internal/syncing/raft_retriever.go	0.00%	60	🔴 CRITICAL
pkg/rpc/client/raft_cluster.go	0.00%	41	🔴 CRITICAL
pkg/rpc/server/http.go	2.17%	45	🔴 HIGH
node/full.go	30.00%	49	🟡 MEDIUM
node/failover.go	72.00%	35	🟢 ACCEPTABLE

Critical Missing Tests:

1. No unit tests for pkg/raft/node.go - Core Raft wrapper has 0% coverage

   • Bootstrap scenarios
   • Join cluster scenarios
   • Peer add/remove
   • Snapshot/restore
   • Error paths

2. No integration tests for Raft retriever - raft_retriever.go completely untested

   • Block application from Raft
   • Error handling
   • Stop/restart scenarios

3. Missing RPC client tests - raft_cluster.go has no tests

   • Partial failure scenarios
   • Network timeout handling
   • Retry logic

Recommendations:

1. Add unit tests for pkg/raft/node.go:

func TestNode_Bootstrap(t *testing.T)
func TestNode_JoinCluster(t *testing.T)
func TestNode_AddPeer_Idempotent(t *testing.T)
func TestNode_RemovePeer(t *testing.T)
func TestNode_LeaderElection(t *testing.T)
func TestNode_StateReplication(t *testing.T)
func TestNode_SnapshotRestore(t *testing.T)

2. Add integration tests for failover scenarios:

   • Leader crash and recovery
   • Network partition (split brain)
   • Cascading failures
   • Rolling restarts

3. Add chaos testing: Use tools like Jepsen or custom fault injection to test:

   • Random node crashes
   • Network delays/partitions
   • Clock skew
   • Disk failures

---

E2E Test Issues

test/e2e/failover_e2e_test.go:42-244

The E2E test TestLeaseFailoverE2E is good but has gaps:

1. Only tests single leader failure - Should test multiple failures
2. No partition recovery test - What happens with network splits?
3. No concurrent transaction test - During failover, do transactions get lost?
4. Assumes clean shutdown - Should test kill -9 scenarios
5. Fixed timing assumptions - Uses hardcoded sleeps that may be brittle in CI

Recommendations:

• Add test for follower failure (should be no-op)
• Add test for majority failure (cluster should halt)
• Add test for brain split and recovery
• Use polling with timeout instead of fixed sleeps

---

6. Configuration & Documentation

⚠️ Configuration Issues

Missing Raft Configuration Validation

pkg/config/config.go:181

The RaftConfig struct has no validation:

type RaftConfig struct {
    Enable           bool            
    NodeID           string          
    RaftAddr         string          
    RaftDir          string          
    Bootstrap        bool            
    Peers            string          
    SnapCount        uint64          
    SendTimeout      time.Duration   
    HeartbeatTimeout time.Duration   
}

Issues:

• No validation that NodeID is unique and non-empty
• No validation that RaftAddr is valid TCP address
• No validation that Peers format is correct (expects nodeID@addr)
• No defaults for critical timeouts
• Bootstrap can be true on multiple nodes (should prevent this)

Recommendation: Add Validate() method:

func (r *RaftConfig) Validate() error {
    if r.Enable {
        if r.NodeID == "" {
            return errors.New("raft.node_id is required when raft is enabled")
        }
        if r.RaftAddr == "" {
            return errors.New("raft.raft_addr is required")
        }
        if _, _, err := net.SplitHostPort(r.RaftAddr); err != nil {
            return fmt.Errorf("invalid raft.raft_addr: %w", err)
        }
        // Validate peers format
        if r.Peers != "" {
            for _, peer := range strings.Split(r.Peers, ",") {
                if _, err := splitPeerAddr(peer); err != nil {
                    return fmt.Errorf("invalid peer format '%s': %w", peer, err)
                }
            }
        }
        // Validate timeouts
        if r.SendTimeout == 0 {
            r.SendTimeout = 5 * time.Second
        }
        if r.HeartbeatTimeout == 0 {
            r.HeartbeatTimeout = 1 * time.Second
        }
    }
    return nil
}

---

Documentation Gaps:

1. No ADR (Architecture Decision Record) - This is a major architectural change and should have an ADR explaining:

   • Why Raft was chosen over alternatives
   • Tradeoffs and limitations
   • Operational considerations

2. No runbook - Operations team needs documentation for:

   • How to add/remove nodes from cluster
   • How to recover from failures
   • How to debug Raft issues
   • Monitoring and alerting recommendations

3. No API documentation - The /raft/join and /raft/remove endpoints need OpenAPI/Swagger docs

4. Configuration examples missing - Need examples for:

   • 3-node cluster setup
   • 5-node cluster setup
   • Single-node development mode

---

7. Concurrency & Race Conditions

⚠️ Potential Race Conditions

Leader Election State Management

pkg/raft/election.go:53-148

The Run() method has complex state machine with potential races:

var isStarted, isCurrentlyLeader bool
var workerCancel context.CancelFunc = func() {}

Issues:

1. Multiple goroutines access shared state: isStarted, isCurrentlyLeader not protected by mutex
2. Context cancellation races: workerCancel could be called concurrently
3. No happens-before guarantee: Between checking isStarted and starting worker

Recommendation: Use proper synchronization or state machine library.

---

Failover State Races

node/failover.go:167-221

Multiple components started concurrently without clear ordering:

wg.Go(func() error {
    // RPC server
})
// Then separately:
f.p2pClient.Start(ctx)
f.headerSyncService.Start(ctx)
f.dataSyncService.Start(ctx)

Issue: P2P client must be ready before sync services start, but there's no coordination.

Recommendation: Add explicit startup ordering with health checks between stages.

---

8. Additional Observations

Code Smells

1. Magic numbers:

   • pkg/raft/election.go:81 - 300 * time.Millisecond ticker should be configured
   • node/failover.go:148-150 - Hard-coded timeouts (10s, 120s)

2. Copy-paste code:

   • newAggregatorMode and newSyncMode have significant duplication
   • Refactor common logic into shared helper

3. Inconsistent naming:

   • raftNode vs RaftNode vs raft_node
   • leaderElection vs DynamicLeaderElection

4. Commented-out code: None found (good!)

---

Summary of Recommendations by Priority

🔴 CRITICAL (Must Fix Before Production)

1. ✅ Add authentication to Raft RPC endpoints (/raft/join, /raft/remove)
2. ✅ Fix state validation in RaftBlockState.assertValid() - enforce height increments and hash chain
3. ✅ Fix FSM apply channel drops - don't drop messages silently
4. ✅ Add unit tests for pkg/raft/node.go (0% coverage)
5. ✅ Fix RaftClusterClient partial success semantics - require quorum
6. ✅ Implement protobuf serialization for Raft state

🟡 HIGH (Should Fix Soon)

7. Fix resource leaks in NewNode error paths
8. Add timeout and retry to leadership transfer
9. Add integration tests for failover scenarios
10. Add configuration validation for RaftConfig
11. Fix directory permissions (0755 → 0700)
12. Add backpressure mechanism for block production

🟢 MEDIUM (Technical Debt)

13. Reduce code duplication between aggregator/sync mode setup
14. Add ADR document explaining Raft design choices
15. Add operational runbook
16. Use sync.Pool for marshal buffers
17. Add chaos/fault injection testing
18. Improve E2E test coverage

⚪ LOW (Nice to Have)

19. Add Prometheus metrics for Raft operations
20. Add distributed tracing support
21. Refactor magic numbers to constants
22. Add OpenAPI docs for Raft endpoints

---

Positive Aspects ✨

1. Well-structured code - Clean separation of concerns
2. Good use of Go idioms - Contexts, error wrapping, interfaces
3. Comprehensive E2E test - The failover E2E test is well-designed
4. Proper error collection - Multi-error handling in shutdown paths
5. Configuration flexibility - Good balance of configurability

---

Conclusion

This PR implements a solid foundation for HA failover using Raft, but requires significant work before production readiness. The main concerns are:

1. Security - Unprotected Raft endpoints are a critical vulnerability
2. Testing - 630 lines without test coverage including core Raft functionality
3. Error Handling - Several cases where errors are dropped or mishandled
4. State Validation - Insufficient validation could allow state divergence

Recommendation: Address the CRITICAL and HIGH priority issues before merging. Consider breaking this into smaller PRs to improve reviewability and reduce risk.

---

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Nov 12, 2025, 10:33 AM

[github-actions]

PR Preview Action v1.6.2
🚀 View preview at https://evstack.github.io/docs-preview/pr-2814/
Built to branch main at 2025-11-11 11:39 UTC. Preview will be ready when the GitHub Pages deployment is complete.