Skip to content

Respond to unknown PUBREL with PUBCOMP reason code 0x92#379

Open
guclumhg wants to merge 1 commit into
FreeRTOS:mainfrom
guclumhg:feature/unknown-pubrel-recovery
Open

Respond to unknown PUBREL with PUBCOMP reason code 0x92#379
guclumhg wants to merge 1 commit into
FreeRTOS:mainfrom
guclumhg:feature/unknown-pubrel-recovery

Conversation

@guclumhg

Copy link
Copy Markdown

Description

When a client loses its QoS 2 session state (for example the device reboots after sending a PUBREC) and reconnects with a persistent session, the broker retransmits the pending PUBREL. Because incomingPublishRecords no longer contains the packet identifier, MQTT_UpdateStateAck fails, no response is sent, and every subsequent MQTT_ProcessLoop call returns MQTTBadResponse — the client is permanently stuck, as reported in #271.

MQTT v5 defines the recovery for exactly this situation: the receiver of a PUBREL must respond with a PUBCOMP [MQTT-4.3.3-11], and reason code 0x92 (Packet Identifier not found) exists to signal that the identifier was not known — the specification notes this "is not an error during recovery". The enum value MQTT_REASON_PUBCOMP_PACKET_IDENTIFIER_NOT_FOUND already exists in coreMQTT but was never sent on this path.

This PR makes handlePublishAcks detect the record-not-found case for an incoming PUBREL (MQTT_UpdateStateAck returns MQTTBadResponse for a PUBREL only when no record exists; a known record with an illegal transition yields MQTTIllegalState) and respond with a PUBCOMP carrying reason code 0x92 and no properties, reusing the existing MQTT_GetAckPacketSize and buildAndSendAckWithProps helpers. The process loop then returns successfully and the connection stays usable. The message is lost, which the issue reporter considered acceptable for this corner case. No state record is created for the response, the application callback is not invoked, and the retransmit hooks are not touched. Behavior for every other packet type and failure is unchanged — an unknown PUBACK/PUBREC/PUBCOMP still fails as before, covered by a regression scenario in the new test.

Scope note: on the outgoing side, publish persistence across reboots is now possible through the store/retrieve/clear retransmit hooks, so this PR focuses on the incoming PUBREL failure that rendered the client unusable. The symmetric case (unknown PUBREC answered with PUBREL 0x92) can be added as a follow-up if desired.

Coverage: all 16 new lines and all 18 new branches are covered by the added test (5 scenarios); totals remain at 97.5% lines / 93.8% branches.

Test Steps

Both CI unit-test configurations pass locally (5/5 test suites), including the new test_MQTT_ProcessLoop_UnknownPubrel_RespondsWithPubcompNotFound:

cmake -S test -B build -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug \
  -DBUILD_CLONE_SUBMODULES=ON -DUNITTEST=1 [-DCOV_ANALYSIS=1] \
  -DCMAKE_C_FLAGS='--coverage -Wall -Wextra -Wsign-compare -Werror -DLIBRARY_LOG_LEVEL=LOG_DEBUG'
make -C build all
ctest --test-dir build -E system --output-on-failure

Formatting verified with uncrustify 0.69.0 using the config from FreeRTOS/CI-CD-Github-Actions.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Fixes #271

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

A client that loses its QoS 2 session state (e.g. the device reboots
after sending PUBREC) receives a retransmitted PUBREL on reconnect.
Since no publish record exists for the packet identifier, the state
update fails, no response is sent, and MQTT_ProcessLoop returns
MQTTBadResponse forever, leaving the client unusable.

MQTT v5 requires a PUBCOMP response to every PUBREL [MQTT-4.3.3-11]
and defines reason code 0x92 (Packet Identifier not found) for this
situation, noting it is not an error during recovery. Send that
response instead of failing, without creating a state record.

Fixes FreeRTOS#271
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.

Handle QoS2 messages when the library loses state - store state in NVM

1 participant