fix: address CI failures and review comments

- Remove unused variable is_new_peer (Werror on all GCC/Clang)
- Remove 'struct' keyword from iovec/msghdr declarations for Windows
  MSVC compatibility (WSABUF is a typedef, not a struct in namespaces)
- Fix clang-tidy issues in Discovery.cpp: add missing includes
  (<set>, <map>, <mutex>), make 'name' const, suppress
  bugprone-not-null-terminated-result for wire format memcpy, collapse
  duplicate IDLE branches (bugprone-branch-clone)
- Fix clang-tidy issues in Fragmentation.cpp: add missing includes,
  make local variables const, use data() instead of begin() iterators
  to avoid narrowing conversions
- Add NOLINT for wire_protocol.hpp macro and C-style array (necessary
  for packed struct wire format)
- Fix Windows read_socket blocking: set socket non-blocking before
  drain loop since MSG_DONTWAIT has no effect on Windows
- Fix IPv6 reassembly key: XOR-fold full sockaddr_storage instead of
  only first 8 bytes to prevent collisions between IPv6 peers
- Clear deduplicators on reset() to prevent stale state
- Add util sources to standalone nuclearnet CMake target

Author: TrentHouliston

src/nuclearnet/CMakeLists.txt

@@ -38,7 +38,16 @@ find_package(Threads REQUIRED)
 file(GLOB_RECURSE NUCLEARNET_SOURCES CONFIGURE_DEPENDS "*.cpp")
 file(GLOB_RECURSE NUCLEARNET_HEADERS CONFIGURE_DEPENDS "*.hpp")
 
-add_library(nuclearnet STATIC ${NUCLEARNET_SOURCES})
+# Include required utility sources that nuclearnet depends on
+set(NUCLEARNET_UTIL_SOURCES
+    ${CMAKE_CURRENT_SOURCE_DIR}/../util/network/resolve.cpp
+)
+# Include platform shim on Windows
+if(WIN32)
+    list(APPEND NUCLEARNET_UTIL_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/../util/platform.cpp)
+endif()
+
+add_library(nuclearnet STATIC ${NUCLEARNET_SOURCES} ${NUCLEARNET_UTIL_SOURCES})
 add_library(NUClear::nuclearnet ALIAS nuclearnet)
 
 target_include_directories(nuclearnet

src/nuclearnet/Discovery.cpp

@@ -26,6 +26,9 @@
 #include <chrono>
 #include <cstdint>
 #include <cstring>
+#include <map>
+#include <mutex>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -68,7 +71,7 @@
             auto name_len = static_cast<uint16_t>(name.size());
             std::memcpy(ptr, &name_len, sizeof(uint16_t));
             ptr += sizeof(uint16_t);
-            std::memcpy(ptr, name.data(), name.size());
+            std::memcpy(ptr, name.data(), name.size());  // NOLINT(bugprone-not-null-terminated-result)
             ptr += name.size();
 
             // Write subscription count and hashes
@@ -124,7 +127,7 @@
             }
 
             // Read name
-            std::string name(reinterpret_cast<const char*>(ptr), name_len);
+            const std::string name(reinterpret_cast<const char*>(ptr), name_len);
             ptr += name_len;
             remaining -= name_len;
 
@@ -286,13 +289,8 @@
 
                 switch (peer.handshake) {
                     case HandshakeState::IDLE:
-                        if (has_syn && !has_ack) {
-                            // Received SYN — respond with SYN+ACK
-                            peer.handshake        = HandshakeState::SYN_RECEIVED;
-                            result.response_flags = SYN | CON_ACK;
-                        }
-                        else if (has_syn && has_ack) {
-                            // SYN+ACK but we haven't sent SYN — treat as SYN, respond SYN+ACK
+                        if (has_syn) {
+                            // Received SYN (or SYN+ACK when we haven't sent SYN) — respond with SYN+ACK
                             peer.handshake        = HandshakeState::SYN_RECEIVED;
                             result.response_flags = SYN | CON_ACK;
                         }

src/nuclearnet/Fragmentation.cpp

@@ -23,7 +23,12 @@
 #include "Fragmentation.hpp"
 
 #include <algorithm>
+#include <chrono>
+#include <cstdint>
 #include <cstring>
+#include <mutex>
+#include <utility>
+#include <vector>
 
 namespace NUClear {
 namespace network {
@@ -38,7 +43,7 @@ namespace network {
                                                                   uint8_t flags,
                                                                   const std::vector<uint8_t>& payload) const {
         // Calculate how many fragments we need
-        uint16_t packet_count =
+        const uint16_t packet_count =
             payload.empty() ? 1 : static_cast<uint16_t>((payload.size() + packet_mtu - 1) / packet_mtu);
 
         std::vector<Fragment> fragments;
@@ -53,9 +58,9 @@ namespace network {
             frag.hash         = hash;
 
             // Calculate the data slice for this fragment
-            std::size_t offset = static_cast<std::size_t>(i) * packet_mtu;
-            std::size_t length = std::min(static_cast<std::size_t>(packet_mtu), payload.size() - offset);
-            frag.data.assign(payload.begin() + offset, payload.begin() + offset + length);
+            const std::size_t offset = static_cast<std::size_t>(i) * packet_mtu;
+            const std::size_t length = std::min(static_cast<std::size_t>(packet_mtu), payload.size() - offset);
+            frag.data.assign(payload.data() + offset, payload.data() + offset + length);
 
             fragments.push_back(std::move(frag));
         }
@@ -79,13 +84,13 @@ namespace network {
 
         // Enforce max assembly size check
         if (max_assembly_size > 0) {
-            std::size_t projected_size = static_cast<std::size_t>(packet_count) * packet_mtu;
+            const std::size_t projected_size = static_cast<std::size_t>(packet_count) * packet_mtu;
             if (projected_size > max_assembly_size) {
                 return false;
             }
         }
 
-        AssemblyKey key{source_key, packet_id};
+        const AssemblyKey key{source_key, packet_id};
 
         const std::lock_guard<std::mutex> lock(assembly_mutex);
 

src/nuclearnet/NUClearNet.cpp

@@ -71,6 +71,9 @@ namespace network {
         // Shut down existing connections
         shutdown();
 
+        // Clear per-peer state that should not survive a reset
+        deduplicators.clear();
+
         config    = cfg;
         node_name = cfg.name;
 
@@ -142,6 +145,16 @@ namespace network {
                 throw std::system_error(network_errno, std::system_category(), "Failed to bind data socket");
             }
 
+            // Set non-blocking so sends don't block when the kernel buffer is full
+            // (unreliable sends should be fire-and-forget, not back-pressure the caller)
+#ifdef _WIN32
+            u_long non_blocking = 1;
+            ioctl(fd, FIONBIO, &non_blocking);
+#else
+            int flags = ::fcntl(fd, F_GETFL, 0);
+            ::fcntl(fd, F_SETFL, flags | O_NONBLOCK);
+#endif
+
             data_fd.reset(fd);
         }
 
@@ -191,6 +204,16 @@ namespace network {
                 }
             }
 
+            // Set non-blocking so read_socket drain loop works on Windows
+            // (where MSG_DONTWAIT has no effect)
+#ifdef _WIN32
+            u_long non_blocking = 1;
+            ioctl(fd, FIONBIO, &non_blocking);
+#else
+            int flags = ::fcntl(fd, F_GETFL, 0);
+            ::fcntl(fd, F_SETFL, flags | O_NONBLOCK);
+#endif
+
             announce_fd.reset(fd);
         }
 
@@ -231,7 +254,7 @@ namespace network {
             header.flags        = req.flags;
             header.hash         = req.hash;
 
-            struct iovec iov[2];
+            iovec iov[2];
             iov[0].iov_base = reinterpret_cast<void*>(&header);
             iov[0].iov_len  = sizeof(DataPacket) - 1;
             iov[1].iov_base = const_cast<void*>(static_cast<const void*>(req.data.data()));
@@ -285,7 +308,7 @@ namespace network {
                 std::size_t offset   = static_cast<std::size_t>(i) * packet_mtu;
                 std::size_t frag_len = std::min(static_cast<std::size_t>(packet_mtu), length - offset);
 
-                struct iovec iov[2];
+                iovec iov[2];
                 iov[0].iov_base = reinterpret_cast<void*>(&header);
                 iov[0].iov_len  = sizeof(DataPacket) - 1;
                 iov[1].iov_base = const_cast<void*>(static_cast<const void*>(payload + offset));
@@ -337,7 +360,7 @@ namespace network {
                 std::size_t offset   = static_cast<std::size_t>(i) * packet_mtu;
                 std::size_t frag_len = std::min(static_cast<std::size_t>(packet_mtu), length - offset);
 
-                struct iovec iov[2];
+                iovec iov[2];
                 iov[0].iov_base = reinterpret_cast<void*>(&header);
                 iov[0].iov_len  = sizeof(DataPacket) - 1;
                 iov[1].iov_base = const_cast<void*>(static_cast<const void*>(payload + offset));
@@ -402,9 +425,12 @@ namespace network {
 
     void NUClearNet::read_socket(fd_t fd) {
         // Stack buffer — 65535 is the maximum UDP datagram size
-        alignas(8) uint8_t buffer[65535];
+        alignas(8) uint8_t buffer[65535];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
         sock_t source{};
 
+        // Drain all pending datagrams from the socket
+        // The data socket is non-blocking, so recvfrom returns -1/EWOULDBLOCK when empty
+        // For the announce socket, MSG_DONTWAIT provides the same behavior on POSIX
         for (;;) {
             socklen_t source_len = sizeof(source.storage);
             ssize_t received = ::recvfrom(fd,
@@ -434,8 +460,6 @@ namespace network {
 
         switch (header->type) {
             case ANNOUNCE: {
-                // Check if this is a new peer before processing (which adds them)
-                const bool is_new_peer = !discovery->has_peer(source);
                 auto announce_result = discovery->process_announce(source, data, length);
 
                 if (announce_result.is_new) {
@@ -509,10 +533,13 @@ namespace network {
                 const uint8_t* frag_data = data + sizeof(DataPacket) - 1;
                 std::size_t frag_length = length - (sizeof(DataPacket) - 1);
 
-                // Use a hash of the source address bytes as the source key for fragmentation
-                // We use the first 8 bytes of the storage as a simple key
+                // Use a hash of the full source address as the source key for fragmentation
+                // This ensures IPv6 addresses are properly distinguished
                 uint64_t source_key = 0;
-                std::memcpy(&source_key, &source.storage, std::min(sizeof(source_key), sizeof(source.storage)));
+                const auto* bytes = reinterpret_cast<const uint8_t*>(&source.storage);
+                for (std::size_t i = 0; i < sizeof(source.storage); ++i) {
+                    source_key ^= static_cast<uint64_t>(bytes[i]) << ((i % 8) * 8);
+                }
 
                 // Submit to fragmentation
                 Fragmentation::AssembledPacket assembled;
@@ -580,11 +607,11 @@ namespace network {
         }
     }
 
-    void NUClearNet::send_iov(fd_t fd, const sock_t& target, const struct iovec* iov, int iovcnt) {
-        struct msghdr msg{};
-        msg.msg_name       = const_cast<struct sockaddr*>(&target.sock);
+    void NUClearNet::send_iov(fd_t fd, const sock_t& target, const iovec* iov, int iovcnt) {
+        msghdr msg{};
+        msg.msg_name       = const_cast<sockaddr*>(&target.sock);
         msg.msg_namelen    = target.size();
-        msg.msg_iov        = const_cast<struct iovec*>(iov);
+        msg.msg_iov        = const_cast<iovec*>(iov);
         msg.msg_iovlen     = static_cast<decltype(msg.msg_iovlen)>(iovcnt);
         msg.msg_control    = nullptr;
         msg.msg_controllen = 0;

src/nuclearnet/NUClearNet.hpp

@@ -177,11 +177,11 @@ namespace network {
         void process_packet(const sock_t& source, const uint8_t* data, std::size_t length);
 
         /// Send raw bytes to a target using scatter IO (multiple buffers without copying)
-        void send_iov(fd_t fd, const sock_t& target, const struct iovec* iov, int iovcnt);
+        void send_iov(fd_t fd, const sock_t& target, const iovec* iov, int iovcnt);
 
         /// Send a single contiguous buffer to a target
         void send_buf(fd_t fd, const sock_t& target, const uint8_t* data, std::size_t length) {
-            struct iovec iov{};
+            iovec iov{};
             iov.iov_base = const_cast<void*>(static_cast<const void*>(data));  // NOLINT(cppcoreguidelines-pro-type-const-cast)
             iov.iov_len  = length;
             send_iov(fd, target, &iov, 1);

src/nuclearnet/wire_protocol.hpp

@@ -28,8 +28,10 @@
 
 // Packing macros for wire format structs
 #ifdef _MSC_VER
+    // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
     #define NUCLEAR_NET_PACK(...) __pragma(pack(push, 1)) __VA_ARGS__ __pragma(pack(pop))
 #else
+    // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
     #define NUCLEAR_NET_PACK(...) __VA_ARGS__ __attribute__((__packed__))
 #endif
 
@@ -71,7 +73,7 @@ namespace network {
         explicit PacketHeader(PacketType t) : type(t) {}
 
         /// Magic bytes: radioactive symbol in UTF-8
-        uint8_t header[3] = {0xE2, 0x98, 0xA2};
+        uint8_t header[3] = {0xE2, 0x98, 0xA2};  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
         /// Protocol version
         uint8_t version = PROTOCOL_VERSION;
         /// Packet type