Skip to content

fix: CoAP Ping response uses RST instead of ACK per RFC 7252 §4.2#655

Open
sandrolain wants to merge 3 commits into
plgd-dev:masterfrom
sandrolain:fix/coap-ping-rst
Open

fix: CoAP Ping response uses RST instead of ACK per RFC 7252 §4.2#655
sandrolain wants to merge 3 commits into
plgd-dev:masterfrom
sandrolain:fix/coap-ping-rst

Conversation

@sandrolain
Copy link
Copy Markdown
Contributor

@sandrolain sandrolain commented Apr 8, 2026

Files changed: udp/client/conn.go (+3), udp/client/conn_test.go (+54)

Problem: sendPong() was setting message.Acknowledgement on the response
to a Confirmable empty message (CoAP Ping). RFC 7252 §4.2 requires the receiver
to reply with a matching Reset message, not an Acknowledgement. This caused
interoperability failures with compliant CoAP clients (reported in #603, #486).

Fix: One-line change — message.Acknowledgementmessage.Reset in
udp/client/conn.go. A new test TestConnPingResponseIsReset verifies the
correct message type is sent.

Summary by CodeRabbit

  • Bug Fixes

    • UDP client now replies to confirmable ping requests with a Reset message while preserving the original message ID.
  • Tests

    • Added a test validating that ping responses are Reset messages with the expected code and message ID.

RFC 7252 §4.2 specifies that an Empty Confirmable message (CoAP Ping)
must be rejected with a matching Reset message, not acknowledged.

This fixes the sendPong() function to set message.Reset instead of
message.Acknowledgement for Confirmable empty messages.

Fixes plgd-dev#603, plgd-dev#486
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 37fb32de-242c-4af1-b01c-a085d38920d6

📥 Commits

Reviewing files that changed from the base of the PR and between d1e0008 and 35aadb0.

📒 Files selected for processing (2)
  • udp/client/conn.go
  • udp/client/conn_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • udp/client/conn.go
  • udp/client/conn_test.go

📝 Walkthrough

Walkthrough

Respond to empty Confirmable CoAP pings with Reset (RST) instead of Acknowledgement (ACK) in the UDP client; add a test that sends a raw Confirmable Empty and asserts the server responds with RST and matching MID.

Changes

Cohort / File(s) Summary
Core Logic
udp/client/conn.go
Changed sendPong to set outgoing type to message.Reset when replying to incoming message.Confirmable (keeps same MID). Non-confirmable handling unchanged.
Tests
udp/client/conn_test.go
Added TestConnPingResponseIsReset which sends a raw Confirmable Empty over UDP and verifies the first response is RST with matching message ID; added net import.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

Suggested reviewers

  • jkralik

Poem

🐰 A ping so quiet, a tiny request,
I hop to reply with a Reset — not the rest.
MID kept intact, the message is terse,
Tested and proven: the protocol's terse verse. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and specifically describes the main change: fixing CoAP Ping responses to use RST instead of ACK per RFC 7252, which is the primary fix in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
udp/client/conn_test.go (1)

874-874: Minor: Unnecessary type assertion.

net.Conn interface already includes SetReadDeadline, so the type assertion to *net.UDPConn is not required.

♻️ Suggested simplification
-	err = conn.(*net.UDPConn).SetReadDeadline(time.Now().Add(time.Second * 2))
+	err = conn.SetReadDeadline(time.Now().Add(time.Second * 2))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@udp/client/conn_test.go` at line 874, The line uses an unnecessary
concrete-type assertion on conn before calling SetReadDeadline; replace the
assertion form conn.(*net.UDPConn).SetReadDeadline(...) with the interface call
conn.SetReadDeadline(...) (keeping the same deadline expression
time.Now().Add(time.Second * 2)) so the code uses the net.Conn interface method
directly and removes the needless type cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@udp/client/conn_test.go`:
- Line 874: The line uses an unnecessary concrete-type assertion on conn before
calling SetReadDeadline; replace the assertion form
conn.(*net.UDPConn).SetReadDeadline(...) with the interface call
conn.SetReadDeadline(...) (keeping the same deadline expression
time.Now().Add(time.Second * 2)) so the code uses the net.Conn interface method
directly and removes the needless type cast.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e7255feb-b296-4077-aebf-106af4eebdb7

📥 Commits

Reviewing files that changed from the base of the PR and between 2d71bd2 and d1e0008.

📒 Files selected for processing (2)
  • udp/client/conn.go
  • udp/client/conn_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant