Add auto-ping feature to client

[embhorn]

The core client now tracks outbound idle time and sends a PINGREQ from MqttClient_WaitMessage once the link has been idle for about three quarters of the negotiated keep-alive, so applications no longer have to schedule pings themselves. The time source is the compile-time macro WOLFMQTT_GET_TIME_S() (defaults to time(NULL), overridable; define WOLFMQTT_NO_TIME to compile it out), mirroring the broker. A v5 CONNACK Server Keep Alive override is honored, the scheduler is disarmed on disconnect, the mqttclient example keeps its old manual loop under WOLFMQTT_NO_TIME, and unit tests cover the scheduling paths.

Fixes #501

[Copilot]

Pull request overview

This PR adds automatic MQTT client keep-alive scheduling to wolfMQTT by tracking outbound-idle time and injecting PINGREQ from the MqttClient_WaitMessage(_ex) path, reducing the need for applications/examples to manage ping timers themselves. It also introduces a configurable time source (WOLFMQTT_GET_TIME_S()), supports MQTT v5 Server Keep Alive overrides, and adds unit tests around the new scheduling logic.

Changes:

• Track last outbound control-packet transmit time and automatically send PINGREQ when idle time exceeds a keep-alive threshold.
• Honor MQTT v5 CONNACK Server Keep Alive and disable the scheduler on disconnect / when time support is compiled out.
• Add client unit tests for auto-ping scheduling and update the mqttclient example and changelog accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
wolfmqtt/mqtt_client.h	Adds public configuration/docs for the time source macro and extends MqttClient with keep-alive scheduling state.
src/mqtt_packet.c	Updates MqttPacket_Write to record last_tx_time on full control-packet writes.
src/mqtt_client.c	Implements auto keep-alive scheduler invoked from MqttClient_WaitMessage_ex, latches keep-alive, and handles v5 Server Keep Alive.
examples/mqttclient/mqttclient.c	Adjusts example loop to rely on core auto-ping except when time is compiled out.
tests/test_mqtt_client.c	Adds unit tests for auto keep-alive scheduling behavior.
ChangeLog.md	Documents the new auto keep-alive feature and configuration knobs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #563

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #563

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

[aidangarske]

Should we be injecting a ping into a partially read or written packet as well rather than just a read?

[aidangarske]

should this path be tested?

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 16 total — 16 posted, 0 skipped
12 finding(s) posted as inline comments (see file-level comments below)
4 finding(s) not tied to a diff line (full detail below)

Posted findings

• [High] [bugs] Disconnect strands a partial auto-ping write, leaving client-write.isActive set and livelocking MqttClient_Disconnect forever — src/mqtt_client.c:2904-2922
• [Medium] [review-security] MqttClient_WaitMessage_ex returns MQTT_CODE_CONTINUE in blocking builds when another thread is mid-write — src/mqtt_client.c:3197-3208
• [Medium] [review-security] Keep-alive ping performs an inline round-trip using cmd_timeout_ms, not the caller's timeout_ms — src/mqtt_client.c:3182-3192
• [Medium] [review] No test covers the mid-payload guard — the case that is actually broken — tests/test_mqtt_client.c:2150-2171
• [Low] [bugs] Auto-ping can hide an incoming message from WaitMessage_ex — src/mqtt_client.c:3181-3207
• [Low] [review+bugs] WaitMessage_ex can perform network I/O before rejecting a NULL message object — src/mqtt_client.c:3197-3208
• [Low] [review+review-security] multithread.c comment misstates how the auto-ping and ping_task are reconciled — examples/multithread/multithread.c:684-689
• [Low] [bugs] Auto-ping margin test underflows last_tx_time on monotonic/uptime clocks, silently testing the opposite branch — tests/test_mqtt_client.c:2101-2124
• [Low] [review] Test asserts last_tx_time != 0, which is time-source dependent — tests/test_mqtt_client.c:2243
• [Info] [review-security] New keep-alive tests cover only single-threaded blocking builds — tests/test_mqtt_client.c:2019-2370
• [Info] [review] Redundant rc 0 check in MqttPacket_Write last_tx_time stamp — src/mqtt_packet.c:3933
• [Info] [review] Declaration initialized from a function call at top of MqttClient_WaitMessage_ex — src/mqtt_client.c:3200-3204

Findings not tied to a diff line

Connect/Disconnect reset keep_alive_ping.stat in place instead of cancelling it, orphaning its registered pendResp and…

File: src/mqtt_client.c:2905-2913, src/mqtt_client.c:1809-1821
Function: MqttClient_Disconnect_ex / MqttClient_Connect
Severity: High
Category: State Management

Both new blocks reset the auto keep-alive ping state machine with only two field writes:

client->keep_alive_ping.stat.write = MQTT_MSG_BEGIN;
client->keep_alive_ping.stat.read  = MQTT_MSG_BEGIN;

This is an incomplete cancel. MqttMsgStat also carries isReadActive / isWriteActive, and an in-flight message additionally owns client->read.isActive / client->write.isActive, the held lockRecv / lockSend semaphores (WOLFMQTT_MULTITHREAD), and a pendResp node registered in client->firstPendResp. Before this PR there was no core-owned auto-ping object to clean up; per-thread ping objects were touched only by their owning thread. The codebase already has the canonical routine for exactly this — MqttClient_CancelMessage (src/mqtt_client.c:3219) — which resets the stat, removes the pendResp via MqttClient_RespList_Remove, and calls MqttReadStop/MqttWriteStop to release the locks. Every existing caller that abandons an in-flight object uses it (MqttClient_Ping_ex:2849, MqttClient_Connect:1900, the examples). The new code does not.

The ping is genuinely left mid-exchange. In MqttClient_KeepAlive -> MqttClient_Ping_ex -> MqttClient_WaitType(client, ping, MQTT_PACKET_TYPE_PING_RESP, 0, cmd_timeout_ms), mms_stat is &client->keep_alive_ping.stat. MqttReadStart sets stat->isReadActive = 1 and client->read.isActive = 1. WaitType line 1616 (if (rc == MQTT_CODE_CONTINUE && mms_stat->read > MQTT_MSG_WAIT) return rc;) deliberately skips MqttReadStop to retain the read lock across a partial read, and MqttClient_Ping_ex then returns…

Recommendation: Replace both manual two-field resets with the existing full cancel:

#ifndef WOLFMQTT_NO_TIME
    client->keep_alive_sec = 0;
    (void)MqttClient_CancelMessage(client,
        (MqttObject*)&client->keep_alive_ping);
#endif

Apply in both MqttClient_Connect and MqttClient_Disconnect_ex. Note MqttClient_CancelMessage is static unless WOLFMQTT_MULTITHREAD or WOLFMQTT_NONBLOCK is defined (line 3216); it is in the same translation unit as both call sites, so no visibility change is needed. Add a regression test that drives the ping to a real isReadActive == 1 state (partial read via a mock net layer returning a short read) and then calls Disconnect followed by Connect, asserting client->read.isActive == 0, that no stale PINGRESP entry remains in…

Referenced code: src/mqtt_client.c:2905-2913, src/mqtt_client.c:1809-1823 (15 lines)

---

New keep-alive state (keep_alive_sec, last_tx_time, keep_alive_ping.stat) is read and written across threads with no…

File: src/mqtt_client.c:3133-3194, src/mqtt_client.c:2904-2913, src/mqtt_client.c:1809-1821, src/mqtt_packet.c:3928-3936
Function: MqttClient_KeepAlive / MqttPacket_Write / MqttClient_Disconnect_ex / MqttClient_Connect
Severity: Medium
Category: State Management

All three modes flagged this; they disagree on severity (review-security: Medium, review: Medium for the stat reset and Low for the field accesses, bugs: Info) — the stricter Medium is kept, and both views are recorded here. The PR adds new MqttClient fields but does not place them under any of the client's three existing locks (lockSend, lockRecv, lockClient), unlike the documented conventions in the struct itself (MqttPkRead packet; /* protected by read lock */, MqttSk write; /* protected by write lock */, wolfmqtt/mqtt_client.h:214-217).

Sub-case 1 — scalar field races (the milder view). client->last_tx_time is written in MqttPacket_Write (mqtt_packet.c:3934) by whichever thread holds lockSend, and read — and, on the backward-clock re-baseline path, written — in MqttClient_KeepAlive (mqtt_client.c:3165-3181) on the reader thread with no lock. Same for keep_alive_sec, written by the connecting thread in MqttClient_Connect (line 2040/2048) and by Handle_ConnectAck_Props (line 609), read unlocked in both MqttClient_KeepAlive (line 3143) and MqttPacket_Write (line 3933). Under WOLFMQTT_MULTITHREAD (see examples/multithread/multithread.c, where publish_task writes while the reader thread waits) these are formally data races / UB and will be reported by TSan. On the mainstream 32-bit-aligned targets wolfMQTT supports, word32/word16 loads and stores will not tear, so the practical outcome is at worst one mis-scheduled or one skipped ping — self-correcting on the next WaitMessage. The bugs mode rated this Info for that reason; the…

Recommendation: Take client->lockClient around the keep_alive_sec / last_tx_time accesses in MqttClient_KeepAlive and around the keep_alive_ping.stat resets in both MqttClient_Connect and MqttClient_Disconnect_ex under WOLFMQTT_MULTITHREAD — it is already the lock guarding client->read/client->write bookkeeping in MqttReadStart/MqttWriteStart. Document the ownership in the struct alongside the existing annotations, e.g. word32 last_tx_time; /* last control-pkt TX - written under lockSend */. If a benign race on the scalars is deliberately accepted (a missed ping self-corrects on the next WaitMessage), say so in a one-line comment — but the stat reset must be covered, or the header must document that MqttClient_Disconnect may not race a concurrent MqttClient_WaitMessage on…

Referenced code: src/mqtt_client.c:3133-3194, src/mqtt_client.c:2904-2913, src/mqtt_client.c:1809-1821, src/mqtt_packet.c:3928-3941 (14 lines)

---

Auto keep-alive can inject a PINGREQ into a stalled mid-payload PUBLISH, causing protocol desync (WOLFMQTT_NONBLOCK) or…

File: src/mqtt_client.c:3160-3163, src/mqtt_client.c:3197-3208
Function: MqttClient_KeepAlive / MqttClient_WaitMessage_ex
Severity: Low
Category: Logic

The PR runs MqttClient_KeepAlive() before MqttClient_WaitType() resumes the caller's wait state. The only re-entrancy guard is if (client->packet.stat != MQTT_PK_BEGIN) return MQTT_CODE_SUCCESS;. That flag only covers a partially-read fixed header; it does NOT cover a message stalled mid-payload, which is the common non-blocking case.

Trace (verified against the state machine in this tree):

1. MqttPacket_Read (src/mqtt_packet.c:4064-4065) sets client->packet.stat = MQTT_PK_BEGIN before returning the header + first rx_buf chunk.
2. MqttClient_WaitType advances to MQTT_MSG_PAYLOAD and calls MqttClient_HandlePacket -> MqttClient_Publish_ReadPayload, which reads the remaining payload via MqttSocket_Read directly, not through MqttPacket_Read, and sets publish->stat.read = MQTT_MSG_PAYLOAD2.
3. Under WOLFMQTT_NONBLOCK, MqttSocket_Read returns MQTT_CODE_CONTINUE mid-payload. MqttClient_WaitType then deliberately returns without calling MqttReadStop: if (rc == MQTT_CODE_CONTINUE && mms_stat->read > MQTT_MSG_WAIT) return rc; (src/mqtt_client.c:1615-1619). MQTT_MSG_PAYLOAD2 > MQTT_MSG_WAIT, so the read lock is intentionally retained.
4. Net state on return: client->packet.stat == MQTT_PK_BEGIN, client->read.isActive == 1, lockRecv still held, payload bytes still pending on the socket.
5. The app loops and calls MqttClient_WaitMessage_ex again. The new code runs MqttClient_KeepAlive first. The packet.stat guard passes, and if elapsed >= threshold it calls MqttClient_Ping_ex(client, &client->keep_alive_ping).…

Recommendation: Block merge until auto-ping is suppressed while the current wait object has an active read/ack state. Gate the ping on the wait object being idle, not just on client->packet.stat: pass msg into MqttClient_KeepAlive and bail when ((MqttMsgStat*)msg)->read != MQTT_MSG_BEGIN (equivalently, check client->read.isActive), or move the keep-alive check into the MQTT_MSG_BEGIN path before starting a new packet read. Add a regression test that leaves test_client.msg.publish.stat.read = MQTT_MSG_PAYLOAD2 with the deadline expired and asserts g_pingreq_writes == 0.

Referenced code: src/mqtt_client.c:3160-3163, src/mqtt_client.c:3197-3202 (6 lines)

---

Documented MQTT_CODE_ERROR_NETWORK dead-link signal does not exist in WOLFMQTT_NONBLOCK builds

File: src/mqtt_client.c:3184-3191, wolfmqtt/mqtt_client.h:60-63
Function: MqttClient_KeepAlive
Severity: Low
Category: Logic

Verified against the tree. The header (mqtt_client.h:62-63) and ChangeLog both state: "A ping left unanswered returns MQTT_CODE_ERROR_NETWORK rather than MQTT_CODE_ERROR_TIMEOUT, so a caller can tell a dead link from an idle one." The implementation achieves this by translating MQTT_CODE_ERROR_TIMEOUT from MqttClient_Ping_ex.

In a WOLFMQTT_NONBLOCK build that translation is dead code. MqttSocket_Read's non-blocking arm (src/mqtt_socket.c:318-335) maps a would-block to MQTT_CODE_CONTINUE and never synthesizes MQTT_CODE_ERROR_TIMEOUT; MqttClient_Ping_ex then returns early on MQTT_CODE_CONTINUE (line 2860). So MqttClient_KeepAlive returns MQTT_CODE_CONTINUE indefinitely for an unanswered ping and MQTT_CODE_ERROR_NETWORK is never produced. In non-blocking builds MQTT_CODE_ERROR_TIMEOUT is an application-level construct generated by mqtt_check_timeout (examples/mqttexample.c:566), not by the library.

This matters because examples/nbclient/nbclient.c — the non-blocking example — had its manual PINGREQ removed in this PR in favour of the auto-ping, on the strength of that documented guarantee (nbclient.c:534-539 now just returns MQTT_CODE_CONTINUE on an app-level timeout). Note the old nbclient WMQ_PING path did not detect dead links either (MqttClient_Ping_ex returned MQTT_CODE_CONTINUE forever there too), so this is a documentation-accuracy issue rather than a functional regression — hence Low.

Recommendation: Scope the claim in both wolfmqtt/mqtt_client.h:62-63 and the ChangeLog entry to blocking builds, and state explicitly that WOLFMQTT_NONBLOCK applications remain responsible for their own liveness deadline (as examples/nbclient does via mqtt_check_timeout). If genuine non-blocking dead-link detection is wanted, record a ping_sent_time when the PINGREQ is written and have MqttClient_KeepAlive surface MQTT_CODE_ERROR_NETWORK once WOLFMQTT_GET_TIME_S() - ping_sent_time exceeds the keep-alive interval while keep_alive_ping.stat.write != MQTT_MSG_BEGIN.

Referenced code: src/mqtt_client.c:3184-3191, wolfmqtt/mqtt_client.h:60-65 (6 lines)

---

Review generated by Skoll

[aidangarske]

🔴 [High] Disconnect strands a partial auto-ping write, leaving client-write.isActive set and livelocking MqttClient_Disconnect… · API Contract Violations

Distinct failure mode from the pendResp/read-lock leak above, same root cause. When the auto-ping is parked at stat.write == MQTT_MSG_HEADER after a partial WOLFMQTT_NONBLOCK write, MqttClient_Ping_ex returned early at mqtt_client.c:2838-2845 deliberately keeping the send lock held: keep_alive_ping.stat.isWriteActive == 1 and client->write.isActive == 1.

MqttClient_Disconnect_ex then does, in this order:

1. client->keep_alive_sec = 0 — which makes MqttClient_KeepAlive early-return at mqtt_client.c:3143-3145, permanently disabling the resume path.
2. client->keep_alive_ping.stat.write = MQTT_MSG_BEGIN — which discards the state that would have resumed the write.
3. MqttWriteStart(client, &disconnect->stat) — which sees client->write.isActive == 1 and returns MQTT_CODE_CONTINUE (mqtt_client.c:250-253).

Disconnect returns MQTT_CODE_CONTINUE. The application retries; disconnect->stat.write is still MQTT_MSG_BEGIN, so steps 1-3 repeat identically. Nothing can ever clear client->write.isActive, because the only code that would finish that write is the resume path that step 1 just disabled and step 2 just erased. The application can never disconnect, and has no public handle on the internal client->keep_alive_ping to cancel it.

With WOLFMQTT_ALLOW_NODATA_UNLOCK defined the write.isActive guard is compiled out, but then under WOLFMQTT_MULTITHREAD MqttWriteStart blocks on wm_SemLock(&client->lockSend) — a lock the orphaned ping will now never release. Hard deadlock instead of livelock.

This is PR-introduced: pre-PR, every…

Fix: Same fix as the incomplete-cancel finding — route through MqttClient_CancelMessage, which handles the held write lock explicitly (scrubs tx_buf then calls MqttWriteStop when isWriteActive):

#ifndef WOLFMQTT_NO_TIME
    client->keep_alive_sec = 0;
    (void)MqttClient_CancelMessage(client,
        (MqttObject*)&client->keep_alive_ping);
#endif

Keep it before MqttWriteStart so the send lock is released before Disconnect tries to take it.

[aidangarske]

🟠 [Medium] MqttClient_WaitMessage_ex returns MQTT_CODE_CONTINUE in blocking builds when another thread is mid-write · Logic

Verified against the tree. MqttClient_KeepAlive forwards every non-SUCCESS return from MqttClient_Ping_ex verbatim — only MQTT_CODE_ERROR_TIMEOUT is translated. MqttClient_Ping_ex line 2799 does:

if ((rc = MqttWriteStart(client, &ping->stat)) != 0) { return rc; }

and MqttWriteStart (line 250-253) returns MQTT_CODE_CONTINUE — not an error — when client->write.isActive is already set: if (client->write.isActive) { MQTT_TRACE_MSG("Partial write in progress!"); rc = MQTT_CODE_CONTINUE; }. That check precedes the wm_SemLock(&client->lockSend) acquisition, so it returns immediately rather than blocking. In a WOLFMQTT_MULTITHREAD build without WOLFMQTT_NONBLOCK, a publish thread inside MqttPacket_Write has client->write.isActive == 1, so a reader thread whose ping is due gets MQTT_CODE_CONTINUE back from MqttClient_KeepAlive. MqttClient_WaitMessage_ex then hits if (rc != MQTT_CODE_SUCCESS) return rc; and hands MQTT_CODE_CONTINUE to a caller that never opted into non-blocking semantics.

BEFORE: MqttClient_WaitMessage_ex was a thin wrapper over MqttClient_WaitType, which gates its own MQTT_CODE_CONTINUE returns behind #ifdef WOLFMQTT_NONBLOCK (lines 1614-1638, 1653-1659). A blocking build could not observe MQTT_CODE_CONTINUE from this API.

AFTER: it can. Two consequences: (a) the reader's blocking wait degenerates into a busy-spin returning immediately instead of blocking for timeout_ms, and (b) a blocking-mode loop shaped like the one this very PR ships in examples/mqttclient/mqttclient.c:618 (`else if (rc !=…

Fix: Do not let a deferred ping abort the wait. In MqttClient_KeepAlive, treat MQTT_CODE_CONTINUE from the write-lock acquisition as "ping not sent this round, try again later" and return MQTT_CODE_SUCCESS so WaitMessage_ex still performs its read; or in MqttClient_WaitMessage_ex gate the early return on non-CONTINUE outside WOLFMQTT_NONBLOCK builds:

if (rc != MQTT_CODE_SUCCESS
#ifndef WOLFMQTT_NONBLOCK
        && rc != MQTT_CODE_CONTINUE
#endif
) {
    return rc;
}

[aidangarske]

🟠 [Medium] Keep-alive ping performs an inline round-trip using cmd_timeout_ms, not the caller's timeout_ms · Logic

Verified against the tree. When a ping is due, MqttClient_KeepAlive calls MqttClient_Ping_ex, which performs a full blocking PINGREQ/PINGRESP round-trip via MqttClient_WaitType(client, ping, MQTT_PACKET_TYPE_PING_RESP, 0, client->cmd_timeout_ms) (line 2857). The caller's timeout_ms argument to MqttClient_WaitMessage(_ex) is not consulted. A caller doing MqttClient_WaitMessage(&client, 100) — previously guaranteed to return in ~100ms — can now block for cmd_timeout_ms (30000ms in the bundled examples, DEFAULT_CMD_TIMEOUT_MS in examples/mqttexample.h:77).

Worse, WaitType's goto wait_again (line 1661) restarts MqttPacket_Read with a fresh cmd_timeout_ms every time a non-PINGRESP packet arrives. On a busy subscription the auto-ping call can block for well beyond a single cmd_timeout_ms before returning.

The header does document a version of this at wolfmqtt/mqtt_client.h:60-63 ("the call may take up to cmd_timeout_ms longer to return"), which is why this is Medium rather than High — but the documented bound understates the wait_again case, and this affects all 9 in-tree MqttClient_WaitMessage call sites (examples/wiot, mqttclient, firmware, websocket, azure, pub-sub, nbclient, mqttsimple, aws) plus every downstream blocking application, none of which opted in.

Fix: Plumb the caller's timeout_ms into the keep-alive path so MqttClient_WaitMessage(client, N) keeps its latency contract — e.g. give MqttClient_KeepAlive a timeout_ms parameter and add an internal MqttClient_Ping_ex variant that takes an explicit wait timeout, rather than hardcoding client->cmd_timeout_ms. At minimum, tighten the header note at mqtt_client.h:60-63 to state that the wait_again loop can extend the call past a single cmd_timeout_ms when other packets are arriving.

[aidangarske]

🟠 [Medium] No test covers the mid-payload guard — the case that is actually broken · Missing Tests

wait_message_no_autoping_during_partial_read only exercises client->packet.stat = MQTT_PK_READ_HEAD, i.e. a partially-read fixed header. It does not cover the MQTT_MSG_PAYLOAD / MQTT_MSG_PAYLOAD2 continuation case, where client->packet.stat has already been reset to MQTT_PK_BEGIN by MqttPacket_Read but the message is still mid-transfer and the read lock is still held. That is precisely the path that fails (see the BLOCK finding).

Confirmed by grep: MQTT_MSG_PAYLOAD / PAYLOAD2 appear nowhere in tests/test_mqtt_client.c. The existing suite therefore gives false confidence that the injection guard is complete — every new keep-alive test runs with the wait object effectively idle, so nothing drives the auto-ping while a message is mid-exchange.

Fix: Add a test that sets msg.publish.stat.read = MQTT_MSG_PAYLOAD2 with packet.stat = MQTT_PK_BEGIN and an expired deadline, asserting no PINGREQ is written. This test should fail against the current implementation and pass once the BLOCK finding is fixed.

[aidangarske]

🔵 [Low] Auto-ping can hide an incoming message from WaitMessage_ex · Logic Errors

The PR inserts MqttClient_KeepAlive before the normal MQTT_PACKET_TYPE_ANY wait. When the keep-alive threshold has elapsed, MqttClient_KeepAlive calls MqttClient_Ping_ex, which waits specifically for PINGRESP. If the broker sends an application packet such as PUBLISH before the PINGRESP, MqttClient_WaitType can process that packet while waiting for the ping response — it is routed through client->msg / msg_cb rather than the caller-supplied msg — and then the outer MqttClient_WaitMessage_ex performs a fresh ANY wait that may return timeout/continue without populating the caller's object. Callers that rely on the returned msg rather than the message callback can therefore miss a packet that a pre-PR ANY wait would have surfaced directly. Trigger: a due auto-ping and any non-PINGRESP packet queued ahead of the broker's PINGRESP.

Severity held at Medium: the packet is not lost — it is delivered through the message callback path that MqttClient_WaitType already uses for out-of-band packets — so the impact is a delivery-path change and a possible extra wait cycle rather than data loss.

Fix: Do not run a hidden PINGRESP wait before the caller's ANY wait can observe application packets. Split keep-alive into a PINGREQ send/track step and let the normal wait path read the next packet, or have the keep-alive path return immediately when it consumes a non-PINGRESP packet through the caller's object:

#ifndef WOLFMQTT_NO_TIME
    rc = MqttClient_KeepAlive(client, msg, &delivered);
    if (rc != MQTT_CODE_SUCCESS || delivered) {
        return rc;
    }
#endif
    return MqttClient_WaitType(client, msg, MQTT_PACKET_TYPE_ANY, 0,
        timeout_ms);

[aidangarske]

🔵 [Low] Auto-ping margin test underflows last_tx_time on monotonic/uptime clocks, silently testing the opposite branch · Arithmetic Errors

test_client.last_tx_time = WOLFMQTT_GET_TIME_S() - 90; assumes the clock is a large absolute epoch. The PR's own header documentation (wolfmqtt/mqtt_client.h) states the opposite contract: "WOLFMQTT_GET_TIME_S() returns a clock in seconds; only elapsed differences are used, so a monotonic source is fine", and ships a Zephyr default of ((word32)(k_uptime_get() / 1000)).

On Zephyr — or any user_settings.h override supplying a monotonic/uptime source — the test process can reach this test within seconds of boot, so WOLFMQTT_GET_TIME_S() returns e.g. 5. 5 - 90 evaluates in word32 and wraps to 4294967211. MqttClient_KeepAlive then hits the backward-clock guard at mqtt_client.c:3166-3170 (now < client->last_tx_time), re-baselines, and returns MQTT_CODE_SUCCESS without pinging. ASSERT_EQ(MQTT_CODE_ERROR_NETWORK, rc) and ASSERT_TRUE(g_pingreq_writes >= 1) both fail.

The test is the only coverage for the ~3/4 margin — its stated purpose is "This fails if the trigger is elapsed >= keep_alive_sec". On an uptime clock it exercises the backward-clock guard instead, so a regression of the margin logic to elapsed >= keep_alive_sec would pass unnoticed on those targets. Confidence held at Medium: on the default host builds this test runs on today the clock is epoch-based and the underflow does not occur; the exposure is on targets that take the documented monotonic-clock option.

Fix: Clamp the subtraction so the test holds for any clock satisfying the documented contract:

word32 now = WOLFMQTT_GET_TIME_S();

test_client.keep_alive_sec = 100;
test_client.last_tx_time = (now > 90) ? (now - 90) : 0;

last_tx_time = 0 still yields elapsed >= 75 on an uptime clock, so the margin branch is exercised either way. Declare now at the top of the function per the C89 style rule.

[aidangarske]

🔵 [Low] Test asserts last_tx_time != 0, which is time-source dependent · Missing Tests

ASSERT_TRUE(test_client.last_tx_time != 0); assumes WOLFMQTT_GET_TIME_S() never returns 0. That holds for the default time(NULL) (wolfmqtt/mqtt_client.h:79), but the PR explicitly documents WOLFMQTT_GET_TIME_S() as user-overridable and "only elapsed differences are used, so a monotonic source is fine" (wolfmqtt/mqtt_client.h:65) — and ships a Zephyr default of ((word32)(k_uptime_get() / 1000)) (wolfmqtt/mqtt_client.h:76), which returns 0 for the first second of uptime. Any monotonic override starting near zero makes this test spuriously fail. Same root class as the margin-test underflow above, in a different test function.

Fix: Pre-seed last_tx_time with a sentinel and assert it changed, rather than asserting non-zero.

[aidangarske]

⚪ [Info] New keep-alive tests cover only single-threaded blocking builds · Missing Tests

The 10 new tests are a good baseline for the threshold arithmetic, the keep_alive==0 disable, the partial-read guard, the v5 Server Keep Alive override, and Connect/Disconnect arm/disarm. But the feature's risk is concentrated in exactly the configurations they do not exercise:

• No WOLFMQTT_NONBLOCK coverage. The MQTT_CODE_CONTINUE resume path in MqttClient_KeepAlive (line 3152) is the whole reason keep_alive_ping is a persistent client-owned object, yet wait_message_resumes_inflight_ping fakes the state with a bare test_client.keep_alive_ping.stat.write = MQTT_MSG_WAIT; and never sets stat.isReadActive / client->read.isActive, so it does not model a real mid-exchange ping and cannot catch the incomplete-cancel defect reported above.
• No WOLFMQTT_MULTITHREAD coverage. No test has a second thread writing while the reader auto-pings, so neither the MqttWriteStart -> MQTT_CODE_CONTINUE leak nor the unsynchronized keep_alive_ping.stat reset in Disconnect_ex is reachable from the suite.
• No reset-while-in-flight test. connect_arms_keepalive_and_disconnect_disarms sets stat.write = MQTT_MSG_WAIT before connecting, but again without lock/pendResp state, so it asserts only that the two fields it sets get cleared.

Most tests also poke test_client.keep_alive_sec / last_tx_time directly rather than reaching those states through the public API, which means they would keep passing across a refactor of how the fields are maintained. Distinct from the mid-payload test gap above, which targets a different guard.

Fix: Add (1) a WOLFMQTT_NONBLOCK test using a mock net read that returns a short/partial PINGRESP so the read lock is genuinely retained, then assert Disconnect followed by Connect leaves client->read.isActive == 0, leaves no stale PINGRESP entry in client->firstPendResp, and that a subsequent auto-ping succeeds rather than returning MQTT_CODE_ERROR_BAD_ARG; (2) a WOLFMQTT_MULTITHREAD test asserting MqttClient_WaitMessage_ex never returns MQTT_CODE_CONTINUE in a blocking build while a concurrent write is active. Both are the direct regression tests for the High and first Medium findings.

[aidangarske]

⚪ [Info] Redundant rc 0 check in MqttPacket_Write last_tx_time stamp · Style

if (rc > 0 && rc == tx_buf_len && client->keep_alive_sec != 0) — the rc > 0 test is subsumed by rc == tx_buf_len for every real call site (all callers pass tx_buf_len > 0; a zero-length control packet is not emitted). Error and MQTT_CODE_CONTINUE (-101) returns are already excluded by the equality test. Harmless, but the extra clause suggests a condition the reader must reason about that does not exist.

Fix: Drop the redundant rc > 0, or keep it and drop rc == tx_buf_len if the intent is "any bytes went out" (these are different semantics — pick one deliberately).

[aidangarske]

⚪ [Info] Declaration initialized from a function call at top of MqttClient_WaitMessage_ex · Style

int rc = MqttClient_KeepAlive(client); mixes the declaration with a fallible call. The surrounding file consistently declares first and assigns after (e.g. MqttClient_Ping_ex, MqttClient_Connect, MqttClient_Disconnect_ex all use int rc; followed by rc = ...), matching the project's C89 declaration convention. This also keeps the #ifndef WOLFMQTT_NO_TIME block from being the thing that introduces the only declaration in the function.

Fix: Split the declaration and the assignment to match the file's existing style.