Refactor readerLoop

[lalinsky]

• It currently tries to trigger reconnection from multiple places
• It could make more sense to use zig's error handling for this
• Maybe it should be two functions, one inner one that can fail with an error
• And the outer one will trigger reconnection if needed, and also deal with socket closing, etc

@claude prepare a plan for this

[claude]

Claude finished @lalinsky's task —— View job

---

Refactoring Plan for readerLoop

Current Issues Identified:

Multiple Reconnection Trigger Points:

1. readerLoop calls triggerReconnect() from 3 different places (lines 719, 725, 736)
2. flusherLoop also calls triggerReconnect() (line 790)
3. Each call handles its own locking, adding complexity
4. Error handling is scattered and inconsistent

Current Architecture Problems:

• Mixed concerns: I/O operations, error handling, and reconnection logic all intertwined
• Complex locking patterns with manual unlock()/defer mutex.lock() sequences
• Error context is lost - all errors are handled the same way via triggerReconnect()
• No clear separation between recoverable and non-recoverable errors

Proposed Refactoring Plan:

Phase 1: Extract Inner Reading Function

Create readAndProcess() !void - A focused function that:

• Contains only the core I/O and parsing logic
• Returns specific error types instead of calling triggerReconnect()
• Has simpler locking (no unlock/relock dance)
• Allows caller to decide how to handle each error type

fn readAndProcess(self: *Self, buffer: []u8) !usize {
    const stream = self.stream orelse return ConnectionError.ConnectionClosed;
    const bytes_read = try stream.read(buffer);
    if (bytes_read == 0) return ConnectionError.ConnectionClosed;
    try self.parser.parse(self, buffer[0..bytes_read]);
    return bytes_read;
}

Phase 2: Refactor Outer Loop

Simplify readerLoop() to:

• Call readAndProcess() in a try/catch block
• Handle different error types appropriately
• Centralize reconnection logic in one place
• Eliminate complex lock/unlock patterns

fn readerLoop(self: *Self) void {
    var buffer: [4096]u8 = undefined;
    log.debug("Reader loop started", .{});
    
    while (!self.should_stop.load(.acquire)) {
        // Check connection status under lock
        {
            self.mutex.lock();
            defer self.mutex.unlock();
            if (self.status == .closed or self.status == .reconnecting) break;
        }
        
        // Perform I/O without holding mutex
        if (self.readAndProcess(&buffer)) |bytes_read| {
            log.debug("Read {} bytes", .{bytes_read});
        } else |err| switch (err) {
            ConnectionError.ConnectionClosed => {
                log.debug("Connection closed by server");
                self.triggerReconnect(err);
                break;
            },
            else => {
                log.err("Read/parse error: {}", .{err});
                self.triggerReconnect(err);
                break;
            },
        }
    }
    
    self.cleanupStream();
    log.debug("Reader loop exited");
}

Phase 3: Error Type Refinement

Define specific error types for different failure modes:

• ConnectionError.NetworkError - Socket I/O failures
• ConnectionError.ProtocolError - Parsing/protocol violations
• ConnectionError.ConnectionClosed - Clean disconnection
• ConnectionError.Timeout - Operation timeouts

Phase 4: Apply Same Pattern to flusherLoop

Consistent error handling across both loops:

• Extract writeBuffered() !void from flusherLoop
• Use same error handling pattern
• Centralize reconnection decisions

Benefits of This Approach:

1. Cleaner Error Handling: Uses Zig's native error handling instead of scattered triggerReconnect() calls
2. Better Separation of Concerns: I/O logic separate from reconnection logic
3. Simpler Locking: Eliminates complex unlock/relock patterns
4. More Testable: Inner functions can be unit tested independently
5. Better Error Context: Specific error types allow for different handling strategies
6. Maintainable: Clear function boundaries and responsibilities

Implementation Steps:

• Analyze current implementation
• Extract readAndProcess() function
• Refactor readerLoop() to use new error handling
• Define refined error types
• Apply same pattern to flusherLoop
• Add unit tests for new functions
• Run full test suite to verify behavior

This refactoring aligns with Zig's error handling philosophy while maintaining the existing reconnection behavior and improving code maintainability.

---

[lalinsky]

@claude let's start with refactoring readAndProcess

[claude]

Claude finished @lalinsky's task —— View job • claude/issue-4-20250823-1308 • Create PR ➔

---

Refactoring Complete ✅

Successfully completed Phase 1 of the readerLoop refactoring:

Tasks

• Read project setup instructions and understand codebase
• Analyze current readerLoop implementation
• Extract readAndProcess() function with proper error handling
• Refactor readerLoop to use new function
• Run tests to verify changes (25/25 unit tests passed)
• Commit and push changes

What Was Implemented

Phase 1: Extract readAndProcess() function ✅

• ✅ Created readAndProcess() that returns specific error types instead of calling triggerReconnect()
• ✅ Handles I/O and parsing without triggering reconnection - caller decides how to handle errors
• ✅ Simplified locking - no unlock/relock dance required in the inner function
• ✅ Uses Zig's native error handling with try/catch and error switching
• ✅ Refactored readerLoop() to use new error handling pattern with clear error type discrimination
• ✅ Centralized reconnection logic - reduced trigger points from 3 to 2
• ✅ All unit tests pass (25/25)

Key Improvements Achieved

1. Better Separation of Concerns: I/O logic is now separate from reconnection logic
2. Cleaner Error Handling: Uses Zig's native error handling instead of scattered triggerReconnect() calls
3. More Testable: Inner readAndProcess() function can be unit tested independently
4. Better Error Context: Different error types allow for specific handling strategies (ConnectionClosed vs other errors)
5. Maintainable: Clear function boundaries and responsibilities
6. Simplified Locking: Eliminates complex unlock/relock patterns in the I/O path

The refactoring maintains the existing reconnection behavior while improving code structure and maintainability. This aligns perfectly with Zig's error handling philosophy.

---