Repository navigation
What's Changed
| PR | Title | Category |
|---|---|---|
| #183 | Grow packet buffers as data arrives instead of pre-allocating | fix (critical) |
| #181 | A partial capabilities block no longer zeroes every default | fix (critical) |
| #180 | Bound repeated MQTT v5 property counts | fix (security) |
| #182 | Cap topic filter depth to stop trie memory amplification | fix (security) |
| #186 | Retry transient Accept errors instead of silently killing a listener | fix |
| #191 | Drop a relayed publish whose declared body is longer than its payload | fix |
| #184 | Break inflight resend order ties so persistent sessions resend in order | fix |
| #185 | Make same-id session takeover atomic | fix |
| #187 | Explain accept queue limits and reconnect bursts | docs |
| #189 | Wait for writes to land instead of sleeping before closing | test |
| #190 | Strengthen the publish assertions so a broken write is caught | test |
Details
#183 fix: grow packet buffers as data arrives instead of pre-allocating
A packet body's size comes from a Remaining Length the peer declares in the fixed header, before a single body byte has arrived. ReadPacket allocated that much up front and then blocked on the read. An unauthenticated peer could declare the protocol maximum of 256 MB, send five bytes, and go quiet, and the broker would hold that much memory for as long as the socket stayed open.
The MaximumPacketSize check could not stop it: it is keyed off the same declared value, the shipped default is 0 ("unlimited"), and ReadFixedHeader skips the check entirely when it is 0. No read deadline is in effect while the CONNECT packet itself is being read either, so nothing else would have timed the connection out.
Bodies over 64 KiB are now grown geometrically as data actually arrives, so broker memory tracks bytes received rather than bytes declared. Bodies up to 64 KiB keep the previous single-allocation path, byte for byte.
Measured on this tree (Windows, Go 1.26, unauthenticated MQTT 5, one connection per 256 MB declaration, process working set):
before after
conn 1: 258.6 MiB 0.3 MiB
conn 8: 2,054.8 MiB (256.9/conn) 6.6 MiB (0.83/conn)
conn 24: 6,160.8 MiB (256.7/conn) 14.3 MiB (0.59/conn)
conn 40: 10,264.4 MiB (256.6/conn) 16.2 MiB (0.41/conn)
Flat, and still flat when the connections are held open. A peer that declares a large body and then sends nothing still reserves 64 KiB while it waits, which is the same order as the goroutine and socket the connection already costs.
The read error semantics now match io.ReadFull exactly, including the case where a read delivers the final bytes together with io.EOF. That case is reachable in production: crypto/tls returns (n, io.EOF) from a single Read when a close_notify arrives behind the last data.
The same change also fixes a leaked registration: a client that could not receive its CONNACK stayed in the client registry forever, holding its buffer and its connection.
+362/−10 across mqtt/clients.go, mqtt/server.go and the tests, including an equivalence test that pins the new read path to the behaviour of the io.ReadFull it replaced, and a test that proves the growth path consumes exactly the declared body and no more.
#181 fix: a partial capabilities block no longer zeroes every default
The presence of any capabilities: key in the config made YAML allocate a zero-valued struct, so Capabilities stopped being nil and New() skipped DefaultServerCapabilities entirely. Declaring a single key then silently zeroed every other capability.
The worst of them was MaximumMessageExpiryInterval: at zero, the once-per-second expiry sweep matched every queued message as expired, so the entire offline queue was discarded and persistent sessions stopped delivering, with nothing logged.
Capabilities.UnmarshalYAML now decodes on top of the defaults, merging per key rather than per field, so a partial block keeps the defaults for the keys it omits while explicit zeros (maximum-packet-size: 0, meaning unlimited) still work.
This is a behaviour change for anyone who relied on a partial block zeroing the other capabilities. If you wanted a capability off, set it explicitly.
+188/−9 across mqtt/server.go and the tests.
#180 fix: bound repeated MQTT v5 property counts
MQTT v5 puts no count limit on repeated User Property or Subscription Identifier entries, and neither has a count limit in the specification. Left unbounded, a single packet amplifies badly: Properties.User is a slice of 32-byte entries, so a 10 MB property section carrying two-byte properties grew to about 46 MB of slice, and Properties.Copy duplicated all of it again on top of the packet buffers. One unauthenticated CONNECT could push a broker's RSS up by two orders of magnitude.
Both are now capped: 128 User Properties and 1024 Subscription Identifiers per packet. The subscription identifier limit is deliberately much higher, because MQTT v5 permits one identifier per filter in a SUBSCRIBE and a conforming client needs as many as it has filters.
+150/−2 across mqtt/packets/, with tests for both limits.
#182 fix: cap topic filter depth to stop trie memory amplification
A retained message allocated one trie node per / separated topic level, so a deeply nested topic name could be amplified into a large structure from a single message. The depth is now capped, and a filter that exceeds it is rejected rather than truncated, because a truncated filter no longer matches what the client asked for. [MQTT-4.7.3-1]
+342 across mqtt/server.go, mqtt/topics.go and the tests.
#186 fix: retry transient Accept errors instead of silently killing a listener
Listeners.Serve calls a listener's Serve exactly once, with nothing to restart it, so returning on a transient accept failure took the listener down for good, and nothing was logged. The OS listening socket stays open, so clients kept completing the TCP handshake and then hung until their own timeout, each occupying an accept queue slot.
Accept now retries with a capped backoff. The most common causes were EMFILE when a burst of connections exhausted the file descriptor table, and ECONNABORTED when a peer disappeared mid-handshake. The retry is bounded and the listener is only torn down when it is actually closed.
+461/−33 across mqtt/listeners/, with a test that a client is served normally after a transient accept error.
#191 fix: drop a relayed publish whose declared body is longer than its payload
A cluster peer chooses the bytes in a relayed publish, including the remaining length it declares. An empty payload made readFixedHeader read b[0], and a payload shorter than the body it declared made the offset negative and the slice out of range. Both panicked inside the relay worker.
The relay now drops a payload it cannot unpack. A malformed relay was already dropped silently when the fixed header could not be decoded, so this keeps the same behaviour and does not add a log line that a hostile peer could use to flood the log.
Note that the gRPC channel between nodes is unauthenticated and unencrypted. That is a separate and larger question, and this change only makes the relay resilient to malformed input.
+105 across cluster/agent.go and the tests.
#184 fix: break inflight resend order ties so persistent sessions resend in order
Created is a unixtime with one-second resolution, so messages published within the same second tie. sort.Slice is unstable and the slice came out of a map with randomised iteration order, so tied messages were left in an arbitrary order that changed on every call, and resends of a persistent session could go out rotated rather than in their original order. [MQTT-4.6.0-1]
Packet ids are handed out in ascending order, are unique, and are preserved across resend and session restore, so they make this a strict total order.
+181/−1 across mqtt/inflight.go and the tests.
#185 fix: make same-id session takeover atomic
Two connections with the same client id could each believe it had won the takeover. The registry now removes the displaced client with a compare-and-delete, so a stale cleanup cannot drop a newer connection with the same id.
+154/−2 across mqtt/clients.go, mqtt/server.go and the tests.
#187 docs: explain accept queue limits and reconnect bursts
A listening TCP socket holds only a limited number of connections that completed the handshake but were not handed to the process. The depth is a system setting, and comqtt does not override it. The README now covers it, with the somaxconn settings for Linux and macOS, and why clients should reconnect with exponential backoff plus jitter.
+35 in README.md.
#189 and #190 test: make the publish tests deterministic
Twelve tests closed the client connection a fixed time after publishing and then asserted on the bytes that arrived, so they passed or failed depending on how quickly the write loop drained the outbound queue. They now wait for the writes to complete, which is a delivery signal rather than a timing guess: a write to a pipe completes only once the reader has taken the bytes.
Three of them had assertions too weak to notice a broken write, and one of them had lost a length check that was implicit in the original. A test that compared the message expiry interval also failed about once in 20000 runs, because the interval is derived from the time the publish was created and the clock can tick between the two readings.
The whole mqtt package now passes 50 consecutive runs under -race with no failures.
Upgrade notes
No configuration change is required. maximum-packet-size is still 0 by default, which means unlimited, and the packet buffer no longer follows the declared length.
Two behaviour changes are worth knowing about:
- A partial
capabilities:block now merges on top of the defaults instead of zeroing them. If you wanted a capability disabled, set it explicitly. - A packet carrying more than 128 User Properties or more than 1024 Subscription Identifiers is rejected. Both limits are far above what conforming clients send.
Thank you
Thanks to everyone who reported an issue or reviewed a change in this cycle.