fix: verify connection is still alive before rejecting duplicate identity

[torlando-tech]

Summary

Fixes a bug where legitimate reconnections from the same identity were incorrectly rejected as "duplicates".

Problem: When a peer disconnects, identity_to_address is NOT cleaned up immediately - it's only removed after a 2-second grace period in _process_pending_detaches(). However, _check_duplicate_identity() was not checking if the existing address is still connected before rejecting.

This caused:

• Same identity trying to reconnect with a new MAC (Android MAC rotation) → REJECTED
• Same identity reconnecting after disconnect → REJECTED during grace period

Solution: Before rejecting as duplicate, verify that the old connection is still alive:

1. Check if pending_detach exists for this identity (meaning old connection is already gone)
2. Check if existing address is still in connected_peers or peers dict

If either check fails, the old connection is dead → allow the new connection and clean up stale mappings.

Test plan

• Added TDD-style tests in tests/test_ble_duplicate_identity_stale.py
• Updated existing tests in test_mac_rotation_blacklist_bug.py to set up active connections
• All 306 tests pass

Changes

• src/ble_reticulum/BLEInterface.py: Added connection liveness checks to _check_duplicate_identity()
• tests/test_ble_duplicate_identity_stale.py: New TDD test file demonstrating the bug and fix
• tests/test_mac_rotation_blacklist_bug.py: Updated mock fixtures to include peers, _pending_detach, and connected_peers

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[greptile-apps]

Greptile Summary

Fixed a critical bug where legitimate reconnections from the same identity were incorrectly rejected as duplicates. The issue occurred because identity_to_address cleanup happens after a 2-second grace period, but _check_duplicate_identity() wasn't verifying if the old connection was still alive.

Key Changes

• Connection Liveness Checks: Added three-tier verification in _check_duplicate_identity() to check if existing connection is truly active before rejecting

  • Check 1: Pending detach exists (old connection already gone)
  • Check 2: Address not in connected_peers or peers dict (stale entry)
  • Check 3: Zombie detection (keepalives work but no real data for 30+ seconds)

• Zombie Connection Detection: New feature to handle degraded BLE links where 1-byte keepalives succeed but larger data packets fail

  • Tracks _last_real_data timestamp for each identity
  • 30-second configurable timeout (_zombie_timeout)
  • Automatically disconnects zombie connections when replacement arrives

• Duplicate Handshake Handling: Fixed race condition where Kotlin callback sets identity before 16-byte handshake data arrives through data channel, preventing reassembler errors

Issues Found

• Logic bug in Check 2 (line 1067): Nested conditionals create a gap where addresses in driver.connected_peers but not in self.peers won't trigger cleanup. Should use AND logic instead of nested IF.

Confidence Score: 4/5

• Safe to merge with one logic issue that should be addressed
• The fix addresses a real bug with well-documented behavior and comprehensive test coverage (306 tests passing). However, Check 2's nested conditional logic has a gap that could allow stale connections in edge cases. The zombie detection feature is well-designed and tested. Overall implementation is solid but the logic issue should be fixed before merge.
• Pay close attention to src/ble_reticulum/BLEInterface.py line 1067 - the nested conditional logic needs correction

Important Files Changed

Filename	Overview
src/ble_reticulum/BLEInterface.py	Added connection liveness checks to prevent false duplicate rejections. Implemented zombie connection detection to handle degraded BLE links. Check 2 has a logical issue with nested conditionals.
tests/test_ble_duplicate_identity_stale.py	New TDD test file demonstrating the stale entry bug and verifying the fix with comprehensive test cases for all connection states.
tests/test_zombie_connection_detection.py	New test file with thorough coverage of zombie connection detection feature, including edge cases and threshold testing.

Sequence Diagram

sequenceDiagram
    participant Peer as Peer Device
    participant Driver as BLE Driver
    participant IF as BLEInterface
    participant Maps as State Maps

    Note over Peer,Maps: Scenario 1: Stale Entry After Disconnect

    Peer->>Driver: Connect (MAC_OLD)
    Driver->>IF: _connected_callback(MAC_OLD, identity_A)
    IF->>Maps: identity_to_address[hash_A] = MAC_OLD
    IF->>Maps: address_to_identity[MAC_OLD] = identity_A

    Note over Peer,Maps: Connection established

    Peer->>Driver: Disconnect
    Driver->>IF: _disconnected_callback(MAC_OLD)
    IF->>Maps: _pending_detach[hash_A] = timestamp
    Note over Maps: identity_to_address[hash_A] still exists<br/>(2s grace period)

    Peer->>Driver: Reconnect (MAC_NEW, identity_A)
    Driver->>IF: _check_duplicate_identity(MAC_NEW, identity_A)
    
    alt Check 1: pending_detach exists
        IF->>Maps: Check _pending_detach[hash_A]
        Maps-->>IF: Found (connection gone)
        IF->>IF: _cleanup_stale_address(hash_A, MAC_OLD)
        IF-->>Driver: False (allow connection)
    else Check 2: not in connected_peers
        IF->>Maps: Check driver.connected_peers[MAC_OLD]
        Maps-->>IF: Not found
        IF->>Maps: Check peers[MAC_OLD]
        Maps-->>IF: Not found
        IF->>IF: _cleanup_stale_address(hash_A, MAC_OLD)
        IF-->>Driver: False (allow connection)
    end

    Driver->>IF: Connection proceeds with MAC_NEW
    
    Note over Peer,Maps: Scenario 2: Zombie Connection Detection

    Peer->>IF: Keepalive packets only (1 byte)
    Note over IF,Maps: No real data for 30+ seconds<br/>_last_real_data[hash_A] = old_timestamp

    Peer->>Driver: Reconnect attempt (MAC_NEW, identity_A)
    Driver->>IF: _check_duplicate_identity(MAC_NEW, identity_A)
    
    IF->>Maps: Check driver.connected_peers[MAC_OLD]
    Maps-->>IF: Still connected (keepalives work)
    
    alt Check 3: zombie connection
        IF->>Maps: Check _last_real_data[hash_A]
        Maps-->>IF: 60 seconds ago
        IF->>IF: time_since_data > _zombie_timeout (30s)
        IF->>IF: _cleanup_stale_address(hash_A, MAC_OLD)
        IF->>Driver: disconnect(MAC_OLD)
        IF-->>Driver: False (allow connection)
    end

    Driver->>IF: Connection proceeds with MAC_NEW
    
    Note over Peer,Maps: Scenario 3: Healthy Connection (Reject Duplicate)

    Peer->>IF: Regular data packets
    IF->>Maps: _last_real_data[hash_A] = current_time
    
    Peer->>Driver: Duplicate attempt (MAC_NEW, identity_A)
    Driver->>IF: _check_duplicate_identity(MAC_NEW, identity_A)
    
    IF->>Maps: Check _pending_detach[hash_A]
    Maps-->>IF: Not found
    IF->>Maps: Check driver.connected_peers[MAC_OLD]
    Maps-->>IF: Connected
    IF->>Maps: Check _last_real_data[hash_A]
    Maps-->>IF: Recent (< 30s)
    
    IF-->>Driver: True (reject duplicate)
    Driver->>Peer: Connection rejected

Loading

[torlando-tech]

@greptileai

[greptile-apps]

_{7 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}