[kareem-wolfssl]

ZD#22494

Thanks to Gwanhyun Lee for the reports!

[Copilot]

Copilot review overview

🟡 Changes recommended

Build failure, replay races, non-terminating cancellation, and duplicate QoS 0 delivery remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1

Open (4)

• Completed QoS 0 writes can be replayed after re-entrant close · New
• Session reset can delete publishes added during connection setup · New
• Missing forced-include allocator header breaks unit-test compilation · New
• Terminal handler errors leave cancellation permanently stuck · New

What changed in this PR

Hardens MQTT client and broker state ownership during concurrent cancellation, session replay, queue hand-off, and Will publication.

Changes:

• Adds synchronization and cancellation ownership safeguards.
• Prevents broker queue and Will reuse during re-entrant callbacks.
• Adds regression tests and allocator instrumentation.

File	Description
wolfmqtt/​mqtt_client.h	Documents cancellation ownership semantics.
src/​mqtt_client.c	Synchronizes replay state and pending-response cancellation.
src/​mqtt_broker.c	Handles re-entrant queue transfer and Will publication.
tests/​test_mqtt_client.c	Tests replay locking and cancellation.
tests/​test_broker_connect.c	Tests queue hand-off and Will re-entry.
tests/​include.am	Injects allocator hooks into unit tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[embhorn]

Review skoll comments and failing tests

[Copilot]

Copilot review overview

🟡 Changes recommended

Replay provenance can be misclassified across connections, and broker persistence can retain stale DUP state.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Replay provenance inferred from packet IDs, causing stale publish replay · New
• Replay includes current-connection publishes and corrupts packet ownership · New
• Persisted orphan retains QUEUED state and misses retransmit DUP flag · New

Resolved since last review (4)

• Missing forced-include allocator header breaks unit-test compilation
• Session reset can delete publishes added during connection setup
• Completed QoS 0 writes can be replayed after re-entrant close
• Terminal handler errors leave cancellation permanently stuck

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 2 of 5 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c, tests/test_mqtt_client.c, wolfmqtt/mqtt_client.h

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 2 of 5 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c, tests/test_mqtt_client.c, wolfmqtt/mqtt_client.h

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

[Copilot]

Copilot review overview

🟡 Changes recommended

Replay tagging and partial-write handoff handling still contain correctness gaps.

Review effort: Balanced
Findings: 2

Open (2)

• Tag replay slots before publish copy operations · New
• Propagate replay reset lock failures · New

Resolved since last review (3)

• Replay includes current-connection publishes and corrupts packet ownership
• Replay provenance inferred from packet IDs, causing stale publish replay
• Persisted orphan retains QUEUED state and misses retransmit DUP flag

Previously missed (1)

In code that hasn't changed since last review

Reconcile moved writes that make partial progress

src/​mqtt_broker.c:2021

A moved write that made partial progress is left unreconciled because only exact completion reaches BrokerOrphan_ReconcileSent. In a nonblocking/custom transport, MqttPacket_Write can return MQTT_CODE_CONTINUE with client.write.pos > 0 after the reentrant handoff. The orphan then retries a partially sent QoS 0 PUBLISH, or retries QoS 1/2 without DUP. Reconcile whenever the moved write either completed or advanced the write position.

[Copilot]

Copilot review overview

🟡 Changes recommended

Cancellation callers and forced cleanup still permit object-lifetime races while another thread owns the response.

Review effort: Balanced
Findings: 1

Open (1)

• Forced cleanup can free objects still being accessed by readers · New

Resolved since last review (2)

• Propagate replay reset lock failures
• Tag replay slots before publish copy operations

Previously missed (1)

In code that hasn't changed since last review

Callers ignore CONTINUE and reuse objects still owned by readers

wolfmqtt/​mqtt_client.h:899

The new MQTT_CODE_CONTINUE contract is not adopted by the repository's multithreaded callers. For example, examples/multithread/multithread.c:719 calls MqttClient_CancelMessage once and then lets the stack-allocated publish[] go out of scope; the connect, subscribe, ping, and unsubscribe paths also ignore the result. If a reader owns one of those responses, cancellation now returns CONTINUE while the pending node still points into that object, so those paths can reuse or release it despite this new requirement. Update all callers to retry/retain the object, or preserve a blocking cancellation contract.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/test_mqtt_client.c

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Not reviewed (no scan target covers them): tests/include.am
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/test_mqtt_client.c

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in broker hand-off, pre-CONNACK behavior, cancellation ownership, replay cleanup, and persistence-test guards.

4 open issues

• Update orphaned entries after partial writes · New
• Block publishes before CONNACK · New
• Guard persistence helpers for persistence builds · New
• Discard replay entries for non-persistent sessions · New

1 resolved since last review

• Forced cleanup can free objects still being accessed by readers

_{Review effort: Lite}

[kareem-wolfssl]

3e56ca7 from #626 superseded commit e04019d from this PR, so e04019d was dropped.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Not reviewed (no scan target covers them): tests/include.am
Coverage: 2 of 5 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c, tests/test_mqtt_client.c, wolfmqtt/mqtt_client.h

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical replay-retention and broker hand-off issues remain, along with moderate transport-buffer and allocator-test issues.

2 open issues

• Reconcile QoS queue state during live client takeover · New
• Retain current-connection entries for non-persistent sessions · New

4 resolved since last review

• Guard persistence helpers for persistence builds
• Block publishes before CONNACK
• Update orphaned entries after partial writes
• Discard replay entries for non-persistent sessions

_{Review effort: Lite}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Not reviewed (no scan target covers them): tests/include.am
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: tests/test_mqtt_client.c

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Fenrir's latest completed scan found no issues; clearing the prior automated change request.