Fix tech-debt issues #7, #10; investigate/document #8, #14 - #21
Merged
Conversation
#7 - mqtt.library: connect-phase not abortable (fixed) transport_bsdsocket_connect() now makes the socket non-blocking for the connect() attempt only (issue #7): a host that never responds (down, firewalled, wrong port) used to block for the TCP stack's own connect timeout (often 75s+) with no way to abort it, unlike the rest of the transport (bsdsocket_recv() already gates on WaitSelect+SIGBREAKF_CTRL_C). Added a 30s elapsed-time budget, matching the same pattern (and constant) the AmiSSL/OpenSSL transports already use for their handshake phase. This NDK's bsdsocket.library has no SO_ERROR/SOL_SOCKET to ask "did that connect actually succeed" (checked - genuinely absent, not just unused), so getpeername() is the portable substitute: it only succeeds on an established connection. gethostbyname() itself is still not abortable - noted in a comment as the remaining, harder gap (no async-resolve API on this socket surface). Verified end-to-end on real m68k: net-smoke (plain bsdsocket, both mqtt_pub-static and library-linked mqtt_pub), library-net-smoke (full mqtt.library API), and library-reconnect-smoke (mco_AutoReconnect across a real broker outage - repeated connect() calls through the new code path) all pass with no regression. #10 - PUBLISH decode accepts DUP=1 with QoS 0 (fixed) Decided per the issue's own framing: match the existing precedent (the empty-topic-name rejection for MQTT-4.7.3-1) rather than keep the tolerant-receiver posture. mqtt_decode() now rejects DUP=1 on a QoS 0 PUBLISH as -MQTT_ERR_MALFORMED (MQTT-3.3.1-2), with a test vector and a new "Decode strictness" section in docs/PROTOCOL.md documenting both rejections together. #14 - TLS cipher-suite measurement (investigated, closing with data) Built a throwaway on-target benchmark (not committed - one-off, matching the amissl-spike precedent for this kind of investigation) forcing each TLS 1.3 ciphersuite via SSL_CTX_set_ciphersuites() and timing the handshake against a real broker under Copperline at stock 14MHz. Result: AES-256-GCM 12.92-12.94s, AES-128-GCM 12.22s, ChaCha20- Poly1305 12.18s - a ~6% spread, noise-level against the total, not the dramatic difference a lack-of-AES-acceleration theory might predict. Confirms the original issue #3 reasoning: handshake cost is dominated by asymmetric crypto (the cert/key exchange), which cipher-suite selection doesn't touch - exposing SSL_CTX_set_cipher_list() as a speed lever wouldn't meaningfully help. Recommending closure as answered rather than building the feature. #8 - non-monotonic Amiga clock (investigated, documented, deferred) A real fix needs timer.device's TR_GETSYSTIME, which needs a per-task IORequest - i.e. state threaded through every caller of tool_now_ms(), not a drop-in replacement (statics are off the table for the same multi-connection reason SocketBase/AmiSSLBase are - IORequests aren't safe to share across the concurrent tasks mqtt.library's connection subprocesses are). Given a clock actively changing mid-session is rare in practice, documented the failure mode precisely instead of an invasive API redesign: tool_clock.h's own comment now explains why DateStamp() can't distinguish a clock step from genuine uint32_t wraparound, and docs/PROTOCOL.md's Keepalive section cross-references it. Also fixed a stale comment there claiming the host clock uses time() - it was switched to CLOCK_MONOTONIC during the TLS work and the comment never caught up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWq1CRLCQy9AaD9UtLRSM9
This was referenced Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#7 - mqtt.library connect-phase not abortable (fixed)
transport_bsdsocket_connect()now makes the socket non-blocking for theconnect()attempt only, with a 30s elapsed-time budget (tool_now_ms()), matching the pattern the AmiSSL/OpenSSL transports already use for their handshake phase. This NDK's bsdsocket.library has noSO_ERROR/SOL_SOCKET(checked - genuinely absent), sogetpeername()is the portable substitute for "did that connect actually succeed."gethostbyname()itself remains unabortable - noted in a comment as the harder remaining gap (no async-resolve API on this socket surface).#10 - PUBLISH decode accepts DUP=1 with QoS 0 (fixed)
Decided per the issue's own framing: matched the existing precedent (empty-topic-name rejection for MQTT-4.7.3-1) rather than the tolerant-receiver alternative.
mqtt_decode()now rejects DUP=1 on a QoS 0 PUBLISH as-MQTT_ERR_MALFORMED(MQTT-3.3.1-2), with a test and a new "Decode strictness" section indocs/PROTOCOL.md.#14 - TLS cipher-suite measurement (investigated, recommend closing)
Measured real handshake time against a broker under Copperline at stock 14MHz, forcing each TLS 1.3 ciphersuite: AES-256-GCM ~12.93s, AES-128-GCM 12.22s, ChaCha20-Poly1305 12.18s - a ~6% spread, noise-level against the total. Confirms the original issue #3 reasoning: handshake cost is dominated by asymmetric crypto, not the symmetric cipher. Not building the feature; recommending closure as answered.
#8 - non-monotonic Amiga clock (investigated, documented, deferred)
A real fix needs
timer.device'sTR_GETSYSTIME, which needs per-taskIORequeststate threaded through every caller oftool_now_ms()- not a drop-in replacement, and statics are off the table for the same reasonSocketBase/AmiSSLBaseare. Documented the exact failure mode instead of an invasive redesign, and fixed a stale comment claiming the host clock still usestime()(it moved toCLOCK_MONOTONICduring the TLS work).Test plan
make test- 250/250 (new DUP/QoS0 rejection test included).make lint,make m68k- clean.make test-target(on-target codec test) - clean, no regression from themqtt_packet.cchange.sh tests/net/net-smoke.sh- real m68k, bothmqtt_pub-staticand library-linkedmqtt_pubvia the rewritten connect path.sh tests/library/net-run.sh- fullmqtt.libraryAPI on-target, no regression.sh tests/library/reconn-run.sh-mco_AutoReconnectacross a real broker outage (repeatedconnect()calls through the new code path) - full pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01QWq1CRLCQy9AaD9UtLRSM9