Manual Puback for MQTT5 by sbSteveK · Pull Request #417 · awslabs/aws-c-mqtt

sbSteveK · 2026-02-03T22:22:29Z

Implements manual PUBACK feature for the MQTT5 client.

Provides public API calls:
aws_mqtt5_client_acquire_puback()
aws_mqtt5_client_invoke_puback()

Tests for new API

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2026-02-03T22:45:49Z

Codecov Report

❌ Patch coverage is 83.89262% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.53%. Comparing base (41b6a7d) to head (37741c0).

Files with missing lines	Patch %	Lines
source/v5/mqtt5_client.c	82.70%	23 Missing ⚠️
source/v5/mqtt5_options_storage.c	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
- Coverage   84.55%   84.53%   -0.03%     
==========================================
  Files          26       26              
  Lines       10833    10974     +141     
==========================================
+ Hits         9160     9277     +117     
- Misses       1673     1697      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bretambrose

Looked at the headers

bretambrose · 2026-02-13T22:16:30Z

include/aws/mqtt/manual-puback/manual_puback.h

@@ -0,0 +1,70 @@
+#ifndef AWS_MQTT_MANUAL_PUBACK_H


Wouldn't this make sense to be in v5 and not "manual-puback"

This is a remnant of initial implementation where we were considering MQTT3 support. I considered moving it into the v5 folder but felt like I'd rather keep it within mqtt/manual-puback in case we add it later. I'm fine with moving things into v5. Should be a minimal change of folder structure if we want it in v5 instead.

include/aws/mqtt/manual-puback/manual_puback.h

bretambrose · 2026-02-13T22:18:17Z

include/aws/mqtt/manual-puback/manual_puback.h

+ * @return puback_control_id of the PUBLISH packet. This can be used to schedule a PUBACK for the PUBLISH packet
+ * when used with the aws_mqtt5_manual_puback function call.
+ */
+AWS_MQTT_API uint64_t aws_mqtt5_take_manual_puback_control(


Feels like these declarations should just be in the public mqtt5 client file. Impl can stay separate.

I don't think it needs to be all or nothing but the lines feel blurred between moving these functions into the public mqtt5 client and moving the implementation details out on their own. i.e. current_control_id. I'm beginning to feel like the entire manual puback should simply be a part of the mqtt5 client as it's being handled as a client operation ala Publish, Subscribe, etc.

include/aws/mqtt/manual-puback/manual_puback.h

include/aws/mqtt/private/manual-puback/manual_puback_impl.h

bretambrose · 2026-02-13T22:29:43Z

include/aws/mqtt/private/v5/mqtt5_client_impl.h


+    /*
+     * One more than the most recently used control packet id.
+     */


My suggestion to wall off the implementation as a separate system was intended to encapsulate/insulate the client from this kind of change, ie the puback system is an opaque structure whose logic is completely divorced from the client. The only crossover would be whatever function the impl needs in order to queue a puback.

Essentially, publish received would now be the responsibility of this new system. It would have a clean, private API for use by the client and the client itself would have very few changes in it (just creating/destroying and invoking the system).

Following up on this:

I'd like to see the puback control an opaque subsystem with a clean/clear API. Its usage within the 5 client would be similar to how the request response client embeds and uses the subscription manager.

Advantages:

Separation of concerns - rather than dropping a bunch of new data structures into the 5 client impl and entangling the implementations, they stay independent

Testability - if we just dump everything into the 5 client then all testing requires a 5 client and a real connection to either a mock or real server. If split off into an opaque subsystem with a clear API, the majority of the testing can be done without a client or an active connection; just use the "public"(private) subsystem API and introspect state

Reusability - if we need to add support to the 311 client, we don't need to reimplement everything from scratch; we just embed the subsystem instance into the 311 client and use its API correctly

Keeping the manual puback operation in mqtt5_client.c feels like the cleaner way to handle this feature. The client PUBACK operation, manual or otherwise, feels like it belongs within the client as opposed to a separate subsystem. While it made sense to move it into a separate subsystem when originally considering both 3 and 5 clients, that's no longer the case. With everything already setup, it'd take the same amount of work to separate it now as it would to do so in the future if we ever add 3 support. There may also be thread or state related issues that we are not aware of right now that'd result in us redoing the work to support the 3 client that would render the current implementation unusable for the 3 client. I'd rather do that work when it's needed than unnecessarily do it if it's never needed.

From a testing perspective, I've included tests that use it in the same way we would want to test other client operations against a mock server rather. The mock server we have implemented is up to the task of detecting that PUBACKs are withheld and sent as expected. The test coverage feels like it's covering that the new API is doing what's expected, which with the removal of ordered PUBACKs is pretty simple.

Open to discussion and willing to change my mind but that's where I'm currently at.

Don't agree; dumping everything into one giant system is something we need to get away from.

include/aws/mqtt/manual-puback/manual_puback.h

include/aws/mqtt/private/v5/mqtt5_client_impl.h

source/manual-puback/manual_puback.c

source/v5/mqtt5_client.c

include/aws/mqtt/private/v5/mqtt5_options_storage.h

include/aws/mqtt/v5/mqtt5_client.h

source/v5/mqtt5_client.c

include/aws/mqtt/v5/mqtt5_client.h

source/v5/mqtt5_client.c

xiazhvera

I temporary finished the main implementation part, still working on the tests...

xiazhvera · 2026-03-04T18:57:28Z

include/aws/mqtt/v5/mqtt5_client.h


+/**
+ * Takes manual control of PUBACK for the given PUBLISH packet.
+ *


We should clarify that the function "should only ever be called by the user within the publish received callback" in the comments, otherwise the PUBACK will be sent after the publish callback is invoked.

Also we should also mention that 0 indicates an invalid control.

xiazhvera · 2026-03-04T19:25:14Z

source/v5/mqtt5_client.c

+
+                        s_aws_mqtt5_client_shutdown_channel(client, error_code);
+                    }
+                    return;


trivial: the return seems unnecessary here.

xiazhvera · 2026-03-04T21:43:14Z

source/v5/mqtt5_client.c

+
+    /* Allows lookup of manual puback entries by packet id. We incref here because the packet_id table also has a ref to
+     * the manual_puback */
+    aws_ref_count_acquire(&manual_puback->ref_count);


Similarly, though it is unlikely to happen, if aws_hash_table_put failed, we need release the ref_count.

xiazhvera · 2026-03-04T22:07:10Z

source/v5/mqtt5_client.c

+    size_t in_flight_unacked_publishes =
+        aws_hash_table_get_entry_count(&client->operational_state.manual_puback_control_id_table);
+    if (in_flight_unacked_publishes >= 100) {
+        AWS_LOGF_WARN(


The in-flight message count here might not be accurate, as it may not account for in-flight messages that are waiting for PUBACK in the queue that were not acquired by the user.

sbSteveK added 5 commits January 15, 2026 14:42

puback function api for mqtt5

b42858e

remove error log

847d50b

revert changes

a5205d4

undo comment changes

a946ecf

undo changes

e5ee878

sbSteveK added 5 commits February 4, 2026 08:03

uint32 hash functions

0d40171

the impl of the hashing support funcs

9bd6ecb

hash helper functions from uint32 to uint64

a8a99ea

manual puback implementation

e8c4717

Merge branch 'main' into manual-puback

7f9ca09

sbSteveK marked this pull request as ready for review February 13, 2026 21:09

sbSteveK added 4 commits February 13, 2026 13:18

unsigned long long for logs

d648b0e

void unusued param

88f0051

void again

946d176

why did this compile locally?

bc8ec4f

bretambrose reviewed Feb 13, 2026

View reviewed changes

xiazhvera reviewed Feb 13, 2026

View reviewed changes

sbSteveK added 8 commits February 17, 2026 13:19

PR changes and merge into mqtt5

8c725be

remove internal wrapper functions

09da6c0

move manual puback entry to refcounted struct

dd0d6c2

ref count fixes

f2157f7

Merge branch 'main' into manual-puback

40f0af1

redeclaration warning as error

d9beae5

add and remove refcount to the puback entry for the packet id hash table

173000c

clean temp entry that was used for logging

ce5b425

sfod reviewed Feb 18, 2026

View reviewed changes

include/aws/mqtt/private/v5/mqtt5_options_storage.h Outdated Show resolved Hide resolved

include/aws/mqtt/v5/mqtt5_client.h Outdated Show resolved Hide resolved

include/aws/mqtt/v5/mqtt5_client.h Outdated Show resolved Hide resolved

source/v5/mqtt5_client.c Outdated Show resolved Hide resolved

sfod reviewed Feb 18, 2026

View reviewed changes

source/v5/mqtt5_client.c Show resolved Hide resolved

pr changes

a149efe

sfod reviewed Feb 18, 2026

View reviewed changes

include/aws/mqtt/v5/mqtt5_client.h Show resolved Hide resolved

sbSteveK and others added 3 commits February 18, 2026 13:26

renamed a func w/s_ but didn't apply change elsewhere

7ae6740

more logging

19648ef

tests for manual puback (#419)

e108259

xiazhvera reviewed Feb 25, 2026

View reviewed changes

source/v5/mqtt5_client.c Show resolved Hide resolved

move incref to post aws_hash_table_put

37741c0

xiazhvera reviewed Mar 4, 2026

View reviewed changes

Conversation

sbSteveK commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bretambrose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bretambrose Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiazhvera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbSteveK commented Feb 3, 2026 •

edited

Loading

codecov-commenter commented Feb 3, 2026 •

edited

Loading

bretambrose Feb 13, 2026 •

edited

Loading