net/blockwise/blockwise.go (2)

289-300: Passing 0 for exchangeKey relies on implicit fallback logic.

The WriteMessage function passes 0 as the exchangeKey parameter to startSendingMessage. While this is handled correctly by startSendingMessage (lines 570-572), which computes the key from the token when it's 0, this implicit behavior could be confusing.

Consider making this more explicit by computing the exchangeKey here:

 func (b *BlockWise[C]) WriteMessage(request *pool.Message, maxSZX SZX, maxMessageSize uint32, writeMessage func(r *pool.Message) error) error {
 	startSendingMessageBlock, err := EncodeBlockOption(maxSZX, 0, true)
 	if err != nil {
 		return fmt.Errorf("cannot encode start sending message block option(%v,%v,%v): %w", maxSZX, 0, true, err)
 	}
 
 	w := newWriteRequestResponse(b.cc, request)
-	err = b.startSendingMessage(w, maxSZX, maxMessageSize, startSendingMessageBlock, 0)
+	exchangeKey := request.Token().Hash()
+	err = b.startSendingMessage(w, maxSZX, maxMessageSize, startSendingMessageBlock, exchangeKey)
 	if err != nil {
 		return fmt.Errorf("cannot start writing request: %w", err)
 	}
 	return writeMessage(w.Message())
 }

---

867-879: Consider hash collision implications for remote address keying.

The getExchangeKey function uses FNV-1a to hash the string representation of net.Addr. While FNV-1a is fast and provides reasonable distribution, there are considerations:

1. Address string stability: The function relies on addr.String() being consistent. This is generally safe for net.Addr implementations, but could be fragile if custom address types have non-deterministic string representations.

2. Hash collisions: With a 64-bit hash space, the probability of collision is low but non-zero. For a large number of concurrent connections from different endpoints, collisions could cause cross-contamination of blockwise state between different clients.

3. Empty token semantics: According to RFC 7252, empty tokens are valid when "no other tokens are in use to a destination." The exchange key mechanism assumes one concurrent blockwise exchange per remote address when tokens are empty, which may not hold if the remote sends multiple concurrent requests with empty tokens.

Consider these enhancements:

1. Document the limitations and assumptions in a comment above the function.
2. For production scenarios with high connection counts, consider incorporating additional entropy (e.g., local address, connection ID) to reduce collision probability.
3. Add collision detection/handling in cache operations if feasible.

Example documentation:

+// getExchangeKey returns a cache key for blockwise exchanges derived from the remote address.
+// This is used when the CoAP token is empty per RFC 7252 section 5.3.1.
+// 
+// Limitations:
+// - Relies on addr.String() being deterministic and unique per endpoint
+// - Hash collisions are possible but unlikely with 64-bit FNV-1a
+// - Assumes at most one concurrent empty-token blockwise exchange per remote address
 func getExchangeKey(addr net.Addr) (uint64, error) {
 
 	if addr == nil {
 		return 0, errors.New("cannot get exchange key: addr is nil")
 	}
 
 	h := fnv.New64a()
 	h.Write([]byte(addr.String()))
 	return h.Sum64(), nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

net/blockwise/blockwise.go (3)

354-370: Remove unnecessary trailing newline.

The empty token handling logic is correct, but there's an unnecessary trailing newline at line 370.

Apply this diff:

 		exchangeKey, err = getExchangeKey(remoteAddr)
 		if err != nil {
 			b.errors(fmt.Errorf("cannot get exchange key from request Parameters(%v): %w", r, err))
 			return
 		}
-
 	}

As per static analysis.

---

582-583: Fix formatting issues.

The method signature is correct, but there are formatting issues: unnecessary leading newline and improper formatting.

Apply this diff:

-
-func (b *BlockWise[C]) getSentRequest(exchangeKey uint64, token message.Token) *pool.Message {
-
+func (b *BlockWise[C]) getSentRequest(exchangeKey uint64, token message.Token) *pool.Message {
 	data, ok := b.sendingMessagesCache.LoadWithFunc(exchangeKey, func(value *cache.Element[*pool.Message]) *cache.Element[*pool.Message] {

As per static analysis.

---

869-881: Remove unnecessary leading newline.

The getExchangeKey implementation is correct: FNV-1a provides a fast, non-cryptographic hash suitable for cache keys, and the nil check prevents panics. However, there's an unnecessary leading newline.

Apply this diff to fix formatting:

 }
-
 // getExchangeKey returns a key for the blockwise exchange cache.
 // According to RFC 7252: "[...] An empty token value is appropriate e.g., when no other tokens are in use to a destination"
 // so we need to generate a key from the remote address of the corresponding endpoint.
 func getExchangeKey(addr net.Addr) (uint64, error) {
-
 	if addr == nil {
 		return 0, errors.New("cannot get exchange key: addr is nil")
 	}
-
 	h := fnv.New64a()
 	h.Write([]byte(addr.String()))
 	return h.Sum64(), nil

As per static analysis.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

net/blockwise/blockwise.go (2)

207-229: Consider refactoring to reduce cyclomatic complexity.

The function logic correctly integrates remoteAddr to compute exchangeKey for cache operations. Error handling is appropriate.

The cyclomatic complexity (16) slightly exceeds the threshold (15). Consider extracting some of the block-handling logic into helper functions to improve maintainability:

• Extract the payload size validation and block option encoding (lines 233-258)
• Extract the buffer read logic (lines 259-276)

This would make the function easier to test and maintain, though the current implementation is still readable.

---

541-575: Consider the robustness of using 0 as a sentinel value.

The logic correctly handles the WriteMessage case by treating exchangeKey == 0 as "compute from token." However, there's a theoretical edge case.

The FNV-1a hash could theoretically return 0 for a remote address (probability: 1 in 2^64). If this occurs, lines 565-567 would incorrectly recompute the key using token.Hash().

While extremely unlikely, a more robust approach would be to use a sentinel value outside the valid hash range (e.g., maxUint64) or pass a boolean flag indicating whether to compute from token. However, given the low probability and the limited impact (cache key mismatch), the current implementation is acceptable for practical purposes.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

net/blockwise/blockwise.go (3)

207-216: Improve error message clarity.

The error messages are too generic and don't guide users on what's wrong:

• Line 210: "invalid szx" → should specify the valid range (0-7 or up to SZXBERT)
• Line 213: "invalid token" → should explain the constraint (e.g., "empty token requires valid remoteAddr" if that becomes supported)

Apply this diff to improve the messages:

 func validateDoInput(r *pool.Message, maxSzx SZX) error {
 	if maxSzx > SZXBERT {
-		return errors.New("invalid szx")
+		return fmt.Errorf("invalid szx: must be <= SZXBERT (%d), got %d", SZXBERT, maxSzx)
 	}
 	if len(r.Token()) == 0 {
-		return errors.New("invalid token")
+		return errors.New("empty token not supported in Do method")
 	}
 	return nil
 }

---

619-621: Clarify fallback logic or remove if unnecessary.

The fallback to token.Hash() when exchangeKey is 0 suggests there are code paths where exchangeKey might not be provided. However, looking at the callers of startSendingMessage:

• Line 353 in WriteMessage: passes 0 explicitly
• Line 473 in handleReceivedMessage: passes the computed exchangeKey

The explicit 0 from WriteMessage seems to be the intended use case. Please add a comment explaining this fallback behavior:

+	// If exchangeKey is 0 (e.g., from WriteMessage), fall back to token-based key.
+	// This maintains compatibility with scenarios where remote address tracking isn't needed.
 	if exchangeKey == 0 {
 		exchangeKey = sendingMessage.Token().Hash()
 	}

---

917-932: Well-implemented exchange key derivation.

The function correctly handles both non-empty tokens (using token hash) and empty tokens (using address hash), aligning with RFC 7252's guidance that empty tokens are valid "when no other tokens are in use to a destination."

Minor observation on hash collisions:

Using FNV-1a hash of the address string is simple and practical. While hash collisions are theoretically possible (especially if address strings format inconsistently), they're unlikely in typical deployments. If stricter collision avoidance is needed in the future, consider:

1. Using the raw address bytes instead of the string representation
2. Combining address and port as separate hash inputs
3. Using a cryptographic hash (though FNV is sufficient for cache keys)

For now, the current implementation is acceptable.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

net/blockwise/blockwise.go (2)

290-302: Add early remoteAddr validation in Do when token is empty.
Avoid relying on a later getExchangeKey failure; return a clear error upfront.

Apply this diff:

 func (b *BlockWise[C]) Do(r *pool.Message, maxSzx SZX, maxMessageSize uint32, remoteAddr net.Addr, do func(req *pool.Message) (*pool.Message, error)) (*pool.Message, error) {
   // Input validation
   if err := validateDoInput(maxSzx); err != nil {
     return nil, err
   }
+  // Require remoteAddr when token is empty
+  if len(r.Token()) == 0 && remoteAddr == nil {
+    return nil, errors.New("token required unless remoteAddr is set")
+  }
 
   // Register sending message (token uniqueness + expiration)
   exchangeKey, err := b.registerSendingMessage(r, remoteAddr)
   if err != nil {
     return nil, err
   }

---

407-417: Add early remoteAddr validation in Handle when token is empty.
Mirror Do’s behavior for clearer error handling before getExchangeKey.

Apply this diff:

 func (b *BlockWise[C]) Handle(w *responsewriter.ResponseWriter[C], r *pool.Message, maxSZX SZX, maxMessageSize uint32, remoteAddr net.Addr, next func(w *responsewriter.ResponseWriter[C], r *pool.Message)) {
   if maxSZX > SZXBERT {
     panic("invalid maxSZX")
   }
+  if len(r.Token()) == 0 && remoteAddr == nil {
+    b.errors(errors.New("remoteAddr cannot be nil when token is empty"))
+    return
+  }
 
   exchangeKey, err := getExchangeKey(r.Token(), remoteAddr)
   if err != nil {
     b.errors(fmt.Errorf("cannot get exchange key from request Parameters(%v): %w", r, err))
     return
   }

net/blockwise/blockwise.go (4)

223-235: Clarify collision error message in registerSendingMessage.
When LoadOrStore finds an existing entry, “invalid token” is misleading now that exchangeKey may come from remoteAddr. Prefer an error that reflects an exchangeKey collision.

Apply this diff:

 func (b *BlockWise[C]) registerSendingMessage(r *pool.Message, remoteAddr net.Addr) (uint64, error) {
   expire := b.computeExpire(r)
   exchangeKey, err := getExchangeKey(r.Token(), remoteAddr)
   if err != nil {
     return 0, fmt.Errorf("cannot get exchange key from request Parameters(%v): %w", r, err)
   }
   _, loaded := b.sendingMessagesCache.LoadOrStore(exchangeKey, cache.NewElement(r, expire, nil))
   if loaded {
-    return 0, errors.New("invalid token")
+    return 0, fmt.Errorf("duplicate exchange key (token=%x, addr=%v): another request is already in progress", r.Token(), remoteAddr)
   }
   return exchangeKey, nil
 }

---

554-577: Fix stale error text referencing token in continueSendingMessage.
Lookup is by exchangeKey. Error should reflect that for accurate diagnostics.

Apply this diff:

   if err == nil && sendMessage == nil {
-    err = fmt.Errorf("cannot find sending message for token(%v)", r.Token())
+    err = fmt.Errorf("cannot find sending message for exchangeKey(%d)", exchangeKey)
   }

---

628-649: Minor: naming nit in getSentRequest.
Parameter is an exchangeKey; consider renaming in a follow-up for clarity. No functional change needed.

---

914-929: getExchangeKey approach is reasonable; add minor guard if desired.
FNV-1a over remoteAddr.String() is fine for cache keys. Optionally, guard against empty remoteAddr.String().

Apply this diff:

 func getExchangeKey(token message.Token, remoteAddr net.Addr) (uint64, error) {
   if len(token) != 0 {
     return token.Hash(), nil
   }
 
   if remoteAddr == nil {
     return 0, errors.New("cannot get exchange key: remoteAddr is nil")
   }
 
   h := fnv.New64a()
-  h.Write([]byte(remoteAddr.String()))
+  addr := remoteAddr.String()
+  if addr == "" {
+    return 0, errors.New("cannot get exchange key: remoteAddr string is empty")
+  }
+  _, _ = h.Write([]byte(addr))
   return h.Sum64(), nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

udp/server_libcoap_test.go (14)

96-103: Remove unused client dial.

The Dialed UDP client connection isn’t used; drop it to speed up tests and reduce noise.

- cc, err := Dial(l.LocalAddr().String())
- require.NoError(t, err)
- defer func() {
-   errC := cc.Close()
-   require.NoError(t, errC)
-   <-cc.Done()
- }()

---

111-116: Fix indent-error-flow (revive): drop else after return.

Simplify flow to satisfy linter.

- if tt.wantErr {
-   require.Error(t, err)
-   return
- } else {
-   require.NoError(t, err)
- }
+ if tt.wantErr {
+   require.Error(t, err)
+   return
+ }
+ require.NoError(t, err)

---

77-83: Align content-format with expectations.

Handler sets AppOctets but test table lists TextPlain; either assert the content-format or switch to TextPlain for clarity.

- errS := w.SetResponse(codes.Content, message.AppOctets, bytes.NewReader([]byte(bigPayload10KiB)))
+ errS := w.SetResponse(codes.Content, message.TextPlain, bytes.NewReader([]byte(bigPayload10KiB)))

---

165-169: Skip on missing coap-client (PUT).

Mirror the skip behavior here too.

- _, err := exec.LookPath("coap-client")
- if err != nil {
-   t.Error("coap-client not found in PATH")
-   return
- }
+ if _, err := exec.LookPath("coap-client"); err != nil {
+   t.Skipf("skipping: coap-client not found in PATH")
+ }

---

183-191: Fix err shadowing (govet).

Avoid shadowing outer err when reading body.

- received, err := r.ReadBody()
- assert.NoError(t, err)
+ received, readErr := r.ReadBody()
+ assert.NoError(t, readErr)

---

216-221: Fix indent-error-flow (revive) in PUT test loop.

- if tt.wantErr {
-   require.Error(t, err)
-   return
- } else {
-   require.NoError(t, err)
- }
+ if tt.wantErr {
+   require.Error(t, err)
+   return
+ }
+ require.NoError(t, err)

---

264-269: Skip on missing coap-client (POST).

Mirror the skip behavior here as well.

- _, err := exec.LookPath("coap-client")
- if err != nil {
-   t.Error("coap-client not found in PATH")
-   return
- }
+ if _, err := exec.LookPath("coap-client"); err != nil {
+   t.Skipf("skipping: coap-client not found in PATH")
+ }

---

281-290: Fix err shadowing (govet) in POST handler.

- received, err := r.ReadBody()
+ received, readErr := r.ReadBody()
- assert.NoError(t, err)
+ assert.NoError(t, readErr)

---

315-320: Fix indent-error-flow (revive) in POST test loop.

- if tt.wantErr {
-   require.Error(t, err)
-   return
- } else {
-   require.NoError(t, err)
- }
+ if tt.wantErr {
+   require.Error(t, err)
+   return
+ }
+ require.NoError(t, err)

---

333-336: Prefer errors.New for static error text.

No formatting args used.

- return nil, fmt.Errorf("coap-client not found in PATH")
+ return nil, errors.New("coap-client not found in PATH")

---

341-346: Generate token once and use strconv.FormatUint (perfsprint).

Avoid double rand call; format efficiently.

- if token {
-   rand.Uint64()
-   args = append(args, "-T", fmt.Sprintf("%d", rand.Uint64()))
- }
+ if token {
+   tok := rand.Uint64()
+   args = append(args, "-T", strconv.FormatUint(tok, 10))
+ }

(Optional: seed math/rand or use crypto/rand for better randomness.)

---

345-347: Avoid redundant conversion.

bigPayload10KiB is already a string.

- args = append(args, "-e", string(bigPayload10KiB))
+ args = append(args, "-e", bigPayload10KiB)

---

26-33: Table fields unused/inconsistent; assert what you specify.

wantCode and wantContentFormat are never asserted; GET’s wantPayload type mismatches checks. Either add assertions (parse coap-client output) or drop the fields to avoid confusion.

Would you like me to add minimal parsing to assert response code/content-format from coap-client output?

Also applies to: 35-57, 137-163, 235-262

---

3-17: Add command timeout to avoid hangs.

Use exec.CommandContext with a short timeout when invoking external tools.

+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
- cmd := exec.Command(coapClientPath, args...)
+ cmd := exec.CommandContext(ctx, coapClientPath, args...)

Also applies to: 337-357, 359-363

net/blockwise/blockwise.go (4)

291-301: Validate remoteAddr early in Do.

Return a clear error before attempting register/getExchangeKey.

 if err := validateDoInput(maxSzx); err != nil {
   return nil, err
 }
+ if remoteAddr == nil {
+   return nil, errors.New("remoteAddr cannot be nil")
+ }

---

408-417: Validate remoteAddr early in Handle.

Short-circuit with a clear error when remoteAddr is nil and token is empty.

 func (b *BlockWise[C]) Handle(w *responsewriter.ResponseWriter[C], r *pool.Message, maxSZX SZX, maxMessageSize uint32, remoteAddr net.Addr, next func(w *responsewriter.ResponseWriter[C], r *pool.Message)) {
   if maxSZX > SZXBERT {
     panic("invalid maxSZX")
   }
+  if remoteAddr == nil && len(r.Token()) == 0 {
+    b.errors(errors.New("cannot get exchange key: remoteAddr is nil and token is empty"))
+    return
+  }
 
   exchangeKey, err := getExchangeKey(r.Token(), remoteAddr)

---

231-233: Clarify error on duplicate in-flight request.

“invalid token” is misleading when exchangeKey is address-derived; make the message explicit.

- return 0, errors.New("invalid token")
+ return 0, errors.New("duplicate exchangeKey: in-flight request already registered")

---

574-579: Reference exchangeKey in not-found error.

Current message mentions token, but the cache is keyed by exchangeKey.

- err = fmt.Errorf("cannot find sending message for token(%v)", r.Token())
+ err = fmt.Errorf("cannot find sending message for exchangeKey(%d)", exchangeKey)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

udp/server_libcoap_test.go (1)

113-119: Fix broken payload assertion in GET test.

The test table defines wantPayload as bigPayload10KiB (a string constant at line 350), but the assertion at line 114 only runs when wantPayload is []byte. This means the payload is never actually verified in GET tests.

Apply this diff to fix the assertion:

  if tt.wantPayload != nil {
-   if wantPayloadBytes, ok := tt.wantPayload.([]byte); ok {
-     // add 1 for the extra newline added by coap-client
-     require.Len(t, receivedBody, len(wantPayloadBytes)+1)
-     require.Equal(t, wantPayloadBytes, receivedBody[0:len(wantPayloadBytes)])
-   }
+   want := []byte(tt.wantPayload.(string))
+   // add 1 for the extra newline added by coap-client
+   require.Len(t, receivedBody, len(want)+1)
+   require.Equal(t, want, receivedBody[:len(want)])
  }

mux/router.go (1)

125-136: LGTM! Explicit returns improve readability.

Converting from bare returns to explicit return statements makes the code more readable and maintainable. This is a good practice, especially in functions with multiple exit points.

dtls/server_test.go (1)

88-138: LGTM! Explicit error returns enhance clarity.

Converting all error paths from bare returns to explicit return statements improves code clarity and makes error propagation more obvious. This pattern is now consistent throughout the function and aligns well with the changes in tcp/server_test.go.

tcp/server_test.go (1)

85-133: LGTM! Consistent with DTLS test improvements.

The explicit return statements follow the same refactoring pattern applied in dtls/server_test.go, making error handling more transparent across the test suite. This consistency is valuable for maintainability.

udp/server_libcoap_test.go (1)

325-326: Remove wasteful call to rand.Uint64().

Line 325 calls rand.Uint64() but discards the result. The next line generates a new random value for the token.

Apply this diff:

  if token {
-   rand.Uint64()
    args = append(args, "-T", strconv.FormatUint(rand.Uint64(), 10))
  }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro