perf(mqtt): share payload and QoS 0 packet encoding across topic subscribers - #2040
perf(mqtt): share payload and QoS 0 packet encoding across topic subscribers#2040kriszyp wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a shared message encoding mechanism to optimize MQTT fan-out delivery by serializing messages once per content type and sharing QoS 0 PUBLISH packets across subscribers of a topic. It includes comprehensive benchmarks, integration tests, and unit tests to validate the performance improvements and ensure message identity contracts. The review feedback suggests two improvements: using a more robust check for thenable objects in sharedMessageEncoding.ts to prevent false positives, and adhering to the repository style guide in subscriptionValueIdentity.test.js by using the node: prefix for builtin imports and including file extensions on relative imports.
| // A pass-through payload IS the message's own data, already reachable from the WeakMap key, so | ||
| // caching it retains nothing extra and must not be billed against the budget. | ||
| if (encoding.serializer === PASS_THROUGH) return; | ||
| if ((payload as any)?.then) |
There was a problem hiding this comment.
When detecting a Promise or thenable object, use typeof value?.then === 'function' instead of a simple truthiness check on value?.then to prevent false positives from non-Promise objects that have a truthy then property.
| if ((payload as any)?.then) | |
| if (typeof (payload as any)?.then === 'function') |
References
- When detecting a Promise or thenable object, use
typeof value?.then === 'function'instead of a simple truthiness check onvalue?.thento prevent false positives from non-Promise objects that have a truthythenproperty.
| const assert = require('assert'); | ||
| const { setupTestDBPath } = require('../testUtils'); |
There was a problem hiding this comment.
Node builtins must use the node: prefix as per the repository style guide constraints. Additionally, relative imports must include the file extension.
| const assert = require('assert'); | |
| const { setupTestDBPath } = require('../testUtils'); | |
| const assert = require('node:assert'); | |
| const { setupTestDBPath } = require('../testUtils.js'); |
References
- Node builtins use the
node:prefix. These are constraints, not style choices. Relative imports must include the file extension. (link)
|
Reviewed; no blockers found. |
…cribers
On a high-fan-out topic every subscriber independently serialized the same
message and generated its own PUBLISH packet, even though the output was
byte-identical. A production heap snapshot found ~7,200 subscribers per
distinct topic, so that work was being redone thousands of times per message,
and a CPU profile showed the delivery path (serialization, packet generation,
and the GC churn from their short-lived allocations) dominating the worker.
The fan-out has a single shared origin — transactionBroadcast dispatches the
same message object instance to every subscription of a key — so a WeakMap
keyed on that object lets the first subscriber to encode do the work for all
of them, with no change to subscription lifecycle, ordering, or the per
subscription iterator architecture.
serializeMessage's output varies only on the negotiated content type: it
resolves a serializer from the request's Accept header and every registered
serializer is a pure function of the message. Nothing in the MQTT delivery
path applies per-user shaping — attribute permissions are a REST query-time
concern, and the subscribe-time checkPermission gate is all-or-nothing — so
the memo key is (message, serializer) and a shared payload can never cross
users.
For QoS 0 the whole PUBLISH packet is shared as well, since a QoS 0 PUBLISH
carries no message identifier. The key includes the protocol version because
MQTT v5 emits a properties field that v3.1.1 omits. QoS 1/2 packets stay per
subscriber for their distinct message identifiers; a follow-up can pass a
small per-subscriber header plus the shared payload as a buffer list to
writev to avoid that copy.
Per-subscriber behaviour is unchanged: bytes-sent analytics are still
recorded per subscriber, the writableNeedDrain backpressure wait is
untouched, and ordering is unaffected. Also drops the Math.random() burned on
every QoS 0 delivery for a message identifier mqtt-packet does not emit.
benchmarks/mqtt-fanout-encoding.js, 200 messages, ms per publish:
QoS 0 1 sub 0.0052 -> 0.0037 1.4x
1000 subs 1.2301 -> 0.0128 96.3x
QoS 1 1 sub 0.0017 -> 0.0022 0.8x
1000 subs 1.3150 -> 0.6917 1.9x
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…codes, retention
Three defects the independent pre-push review found in the shared fan-out
encoding, plus the coverage gap that let the first two through.
Pass-through messages regressed. A message that carries its own
{contentType, data} is delivered verbatim and serializeMessage returns it
before ever negotiating, but getSharedMessageEncoding resolved the serializer
first — so a connection whose Accept header names only unregistered types
threw a 406 inside the outbound listener, and the catch there disconnects the
session. Negotiation is now skipped for those messages, and they share one
entry under a sentinel key since their bytes do not depend on content type.
A failed async serialization was memoized permanently. The derived promise was
stored on the message object with no rejection path, so one transient failure
(a blob read hiccup) disconnected every subscriber that touched that message
for as long as the object stayed in the store's read cache. The rejection now
clears the payload so the next subscriber re-encodes, which is what happened
before this optimization existed.
Retention was scoped to the message object's lifetime rather than the
fan-out's. For record events the message IS the store's cached record, so
serialized copies and whole PUBLISH buffers stayed resident for as long as the
record stayed cached, while sharing is only ever needed for one fan-out. Two
rotating generations now bound it: a fan-out spanning a rotation still hits,
and dropping a generation releases all of it at once.
Also splits getSharedFrame into a lookup and a store so a cache hit — the
whole point on a fan-out — allocates neither the key string nor a generator
closure. That was a per-subscriber allocation on the path the change
advertises as flat, and it brings the single-subscriber case back to parity.
Coverage: the four end-to-end fan-out tests assert bytes, which hold whether
or not the encoding is shared, so none of them could detect a regression to
one serialization per subscriber. The fixture now registers a content type
that stamps the server's serialization counter into the payload, and a new
test asserts four subscribers share one count and two publishes advance it by
exactly one. Verified it fails with sharing disabled and passes with it on,
while the other four pass either way. New unit tests cover the pass-through
bypass and the re-encode after a rejected serialization.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…an-out of one Round 2 of the independent pre-push review found that the round 1 fixes traded one problem for another. Retention was not actually bounded. Rotation was gated on a 256-delivery counter before it would consult the clock, so a batch that fans out and then goes quiet — a nightly job, a deploy, a client disconnecting — left the counter parked below the gate and never rotated again. The bound the design claims only holds if it does not depend on traffic, so rotation now runs on an unref'd interval, started on the first insert and stopped once a rotation finds nothing was retained. Every QoS 0 delivery populated the frame cache, including a fan-out of one. Since the cache keys on the message object, and for record events that object is the store's cached record, a single-subscriber topic pinned a whole PUBLISH buffer per record for the rest of its retention window — a heap regression where the change buys nothing. The encoding now counts reuses, and the packet is only retained once a second subscriber has actually shown up. An undefined payload meant three different things: not yet encoded, failed and should be retried, and serialized to undefined (which JSON of a value whose toJSON returns undefined legitimately produces). The third case silently degraded the cache to worse than no cache, re-serializing on every lookup. Failure is now an explicit state rather than inferred from a value a serializer can legitimately return. The object-identity contract the whole design rests on was load-bearing but undocumented, and only enforced by internal producers happening to allocate a fresh object per version. It is now stated in server/DESIGN.md and pinned by a test that asserts two subscribers of a record write receive the same value object — the record path's identity comes from the store's read cache rather than the audit record's memoized decode, which is the more fragile of the two and had no coverage. Also adds tests for rotation, the reuse counter, and the undefined-payload case, and qualifies getMessageSerializer's docstring: the identity key is only valid because the built-in serializers are pure functions of their argument, which a component registering into server.contentTypes could violate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…g the fan-out with it Round 3 of the independent pre-push review. Retention was bounded in time but not in volume. Every object message's payload is retained on the first delivery, before it is known whether a second subscriber exists, so a single-subscriber topic at high throughput held an entire rotation interval's worth of payloads live at once for no benefit — a ceiling of throughput x size x 20s with nothing capping it. A byte budget now rotates early once the retained bytes pass it, so the high-water mark is fixed rather than proportional to load. The cost is that a fan-out spanning an early rotation falls back to re-encoding: slower, never wrong. fill() cleared the failure flag before serializing rather than after. On the refill path the entry is already retained, so a synchronous throw would have left it claiming success with no payload — and every later subscriber in the retention window would have sent an empty PUBLISH rather than erroring. The flag is now set pessimistically and cleared only once a payload is in hand, which enforces "not failed implies a valid payload" instead of assuming it. One transient serialization failure disconnected the entire fan-out. All subscribers await the same promise, so a flaky blob read on a topic with 200 subscribers dropped all 200 sessions, where previously each serialized independently and most would have succeeded. resolveSharedPayload retries once through a fresh lookup: the first subscriber to arrive re-encodes and the rest of the wave shares that attempt. Writing the test for this caught a real bug in the first version of that retry — it rethrew for every subscriber after the first, because it keyed off a failure flag a peer had already cleared. Tests: the rotation timer's arm/stop/re-arm lifecycle (previously only the rotation function was called directly, so nothing covered the state machine), a concurrent wave recovering from one shared failure, and a failure that a retry cannot fix still propagating. Known and accepted: the hits > 0 gate that decides whether to retain a QoS 0 packet is payload-wide, not per (protocolVersion, topic), so a fan-out of exactly two retains one packet nothing reads, and a mixed-protocol-version topic can retain a packet for a cohort of one. Both are bounded by the retention window and the byte budget; making the counter per-frame-key would add a three-state frame lookup for no correctness gain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
test_export_terminology_test.json is written into the repo root by the export integration test; it is not part of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…imate Round 4 of the independent pre-push review found no majors. The remaining substantive item was that the byte budget under-counted on three axes, so the 4 MB ceiling the comment claimed was really 4-8x that. Strings were charged by character count while the default MQTT serializer is JSONStringify, which returns a string — so the dominant path under-counted about 2x. A second content type negotiated on an already-retained message was linked into the entry and never charged at all. And promotion re-charged only the payload, so a promoted entry's frames rode the second generation free. Rather than patch each site, entries now carry what they hold: `bytes` is the payload plus every frame, and `retained` says whether those bytes are charged against the budget. Charge on the way in, give them back on refill, re-charge the whole chain on promotion. That makes "charged if and only if retained" an invariant the code states rather than an estimate three call sites have to remember to keep in step — which is how all three holes got in. Tests: byte pressure releasing earlier entries, and a refill giving its bytes back (the budget had no coverage at all, which is why the holes shipped). The unit suite's shareFrame helper claimed to mirror the outbound listener but cached unconditionally, where the listener gates on hits > 0 — so the policy that decides whether a QoS 0 packet is retained was asserted nowhere. It now mirrors the real gate, and its test states the actual policy: nothing retained for a fan-out of one, two generates for a fan-out of two, shared from the third subscriber on. Still accepted, unchanged from round 3: the hits > 0 gate is per encoding while frames are keyed per (protocolVersion, topic), so a message delivered on two topics can retain one frame nothing reads. Bounded by both the rotation window and the byte budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…ented contract Round 5 of the independent pre-push review. Its lead finding was that the object-identity rule the sharing rests on had become a newly-imposed constraint on a public API — a custom Resource that yields one mutable envelope, mutates it, and re-sends would have every subscriber receive the first message's bytes, silently, with byte-level tests passing. Documenting it was the right instinct but the wrong mechanism for a footgun with no runtime detection. Sharing is now gated on provenance rather than on the app knowing the rule. Store-sourced events carry a record version and are a fresh object per version by construction; DurableSubscriptionsSession forwards that version to the delivery listener, and the cache is only consulted when it is present. A Resource yielding its own envelope has no version, falls back to today's per-subscriber serialization, and can reuse a mutable object safely — it just does not get the fan-out saving. Nothing user-visible regresses, and the profiled workload is unaffected: the integration test proving one serialization per publish still passes, which is what confirms the broker's publish path carries a version end to end. Also from round 5, all minor: The retained/unretained boolean survived rotation, so a serialization resolving after a rotation was charged to a budget it did not belong to and charged again on promotion — forcing premature rotation under exactly the load this targets. It is now an epoch stamped from the generation counter, so bytes are charged if and only if they belong to the live generation. setSharedFrame charged on every call including overwrites, so concurrent QoS 0 subscribers that each generated before any stored billed the budget N times for one retained buffer. Only the buffer actually retained is charged now. The retry swallowed the first serialization error entirely, so a transient failure that recovered left no trace; it is logged at debug. The MAX_RETAINED_BYTES comment now says it is a per-generation ceiling, so the real steady-state bound is twice it. subscriptionValueIdentity.test.js uses the shared unitTests/waitFor.js helper AGENTS.md mandates rather than a local copy, and gained the other half of the identity contract: consecutive versions of a record must be distinct objects. Removing the listener signature's `as any` cast fell out of typing the listener properly for the new argument. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
The export integration test writes test_export_terminology_test.json into the repo root, so a git add -A after running the suite picks it up. Not part of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…ssion to share Round 6 of the independent pre-push review found that the provenance gate added in round 5 was not provenance. `update.version` is whatever the subscription iterator yielded, and a custom Resource controls it completely — the obvious way to write one, wrapping a table subscription to enrich events, copies the version straight through a spread. Such a producer reusing a mutable envelope would still have been served the first publish's bytes, so the gate rejected safe app producers while not actually stopping unsafe ones. DESIGN.md claiming the contract was "enforced" was wrong. The version is now part of the key rather than a boolean: an entry whose version does not match is treated as a miss and the whole chain is re-encoded, since every content type on it is equally stale. That makes reuse of a mutable envelope with an advancing version correct rather than merely disallowed, and leaves only same-object-and-same-version mutation — indistinguishable from "the same message" by any definition. An event with no version at all is still not shared: nothing then distinguishes one publish from the next. The benchmark was measuring nothing. Round 5's gate made it fall through to the unshared path on every iteration because it never passed a version, so the only committed evidence for a performance change would have reported roughly 1.0x while the byte-equality assertion still passed. It now passes the version exactly as the listener does — 1000 subscribers, QoS 0: 1.38 ms -> 0.031 ms per publish. Also: bytes landing after a rotation (a serialization resolving late, a frame generated by a subscriber whose await spanned one) were charged to the entry but skipped by the budget, so they were live and counted by no generation. The entry's whole footprint is now folded into the live budget in that case, counted exactly once. Tests: the same object arriving with a new version re-encodes, a version advance invalidates every content type on the chain, and a raw TCP subscriber shares an encoding with a WebSocket subscriber negotiating JSON (their serializer identities coincide, which nothing pinned). Two comments record constraints a future change could silently break: the QoS 0 frame key does not include dup/retain/properties because nothing varies them today, and the integration serialization-count assertion is only meaningful because the harness runs Harper single-threaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…cache key Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
…rt artifact Round 7 of the independent pre-push review returned no majors. Three items. The retention epoch started at 0 while the "never retained" sentinel is -1, so for the first rotation interval of a worker's life `UNRETAINED` aliased `currentEpoch - 1` and charge() treated a never-retained entry as one still live in the previous generation — folding its whole footprint into the budget. The effective ceiling was about two thirds of what it claimed until the first rotation. The epoch now starts at 1, which makes the sentinel unreachable. A pass-through payload IS the message's own data, already reachable from the WeakMap key, so charging the budget for it billed bytes that retention does not add. It is no longer charged. test_export_terminology_test.json had been added and removed three times on this branch: the terminology integration test runs export_local with `path: './'`, so the export lands in the repo root and any `git add -A` after a test run commits it. Removing it a fourth time is the bandaid; the gitignore entry alongside it only covered the northwind test's artifact, so the pattern now covers both. Tests: every accounting bug so far — a string charged by character count, a chained content type never charged, a promotion charged twice, and now this sentinel collision — passed tests that asserted only *that* a rotation happened. getRetainedBytes() makes the numbers assertable, and a new test pins them: a new entry charges its payload, a reuse does not re-charge, a chained content type is charged, an overwritten frame is charged once, a version bump gives the whole chain back, and a pass-through charges nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmcGpXxJY2bNtWJQzZfSbw
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
088d470 to
782f142
Compare
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
What
When one message is published to an MQTT topic, every subscriber independently serialized the payload and generated its own PUBLISH packet — even though the output was byte-identical. This computes both once and reuses them across the fan-out.
writev(which already accepts a buffer list) to avoid the copy; out of scope here.Why
Measured, not assumed. A V8 heap snapshot of one worker on a production node at ~127k cluster connections found 157,888 live Subscription objects across only 29 distinct topics — the busiest topics each had ~7,200 subscribers, i.e. ~7,200 structurally identical delivery pipelines per topic. A 6s CPU profile of a worker at 182k connections put essentially every non-GC sample on the per-delivery path (writev 16.4%, the delivery loop 9.8%, buffer
_copyActual9.3%, mqtt.js listener 5.6%, mqtt-packet generate/concat ~2%), with GC at 26.4% — largely the short-lived allocation churn from that duplicated encoding. Harper's CPU scales with deliveries (messages × subscribers), not connections.Is sharing safe?
serializeMessage(data, request)output varies only on the negotiated content type. It resolves a serializer viafindBestSerializer(request), which reads only the Accept header, and every registered serializer (JSONStringify, cborencode, msgpackrpack) is a pure function of the message. Nothing in the MQTT delivery path applies per-user shaping: attribute-level permissions (attribute_permissions→target.select) are a REST query-time concern, and thecheckPermissiongate on subscribe is all-or-nothing eligibility, not per-message redaction. The value delivered isauditRecord.getValue(...)orentry.valuestraight from the store, unshaped by subscriber identity. So a shared payload cannot cross users.Sharing is keyed on the message object's identity, which works because the fan-out has a single shared origin —
transactionBroadcastdispatches one object to every subscription of a key. Because a custom Resource is not bound by that, the event's record version is part of the key: an entry whose version does not match is re-encoded, and an event with no version is not shared at all. So a Resource that reuses one mutable envelope is correct as long as the version advances, rather than merely being told not to.Numbers
benchmarks/mqtt-fanout-encoding.js, 200 messages, ms per publish (JSON, MQTT v3.1.1):QoS 0 per-publish cost goes from linear in N to roughly flat (44x at 1000 subscribers); QoS 1 gets ~2x from payload sharing alone. At N=1 it is break-even within noise. CBOR shows a larger win (~80-130x at 1000). The benchmark measures encoding wall-time only — the socket writes that follow are unchanged and per-subscriber either way.
Retention
Sharing needs the encoding to outlive one delivery but not the message object — for record events that object is the store's cached record, so an unbounded memo would keep serialized copies and PUBLISH buffers resident for as long as the record stays cached. Two rotating WeakMap generations bound it in time (10s interval, unref'd, stopped when nothing is retained), and a 4 MB per-generation byte budget bounds it in volume.
Test notes
unitTests/server/serverHelpers/sharedMessageEncoding.test.js, 36 tests) — sharing per content type, content-type isolation, the version key invalidating a whole chain, pass-through messages skipping negotiation, retry after a failed serialization, rotation and exact byte accounting, and a shared buffer survivingws.sendto five real clients unmutated.unitTests/resources/subscriptionValueIdentity.test.js) — two subscribers of a record write receive the same value object, and consecutive versions receive distinct objects. Without this, a change making each subscription decode its own copy would silently revert the optimization with every other test green.integrationTests/mqtt/mqtt.test.ts, 5 new) — including one that proves the server serializes once per publish regardless of subscriber count, via a fixture content type that stamps the server's serialization counter into the payload. Byte equality holds whether or not sharing works, so this is the only test that can catch a regression to per-subscriber encoding; verified it fails with sharing disabled and passes with it on, while the other four pass either way.test:unit:main,test:unit:resources(1349), server + serverHelpers (531),test:integration:all(1640 pass / 0 fail). Pre-existing environment failures unrelated to this change are noted in the review.Review
Seven rounds of independent cross-model review (Codex + Gemini + Grok + Harper-domain adjudication) ran pre-push; the final round returned no majors. Findings fixed along the way included a 406 regression for pass-through messages, a memoized rejection that turned one transient serialization failure permanent, unbounded retention, and a benchmark that had stopped measuring the shared path. Remaining accepted trade-offs are in the self-review.
Dispatched task: no linked issue.
🤖 Generated with Claude Code
\n\n### CI follow-up\n\nBun 1.3.14 kept a private resolver cache across component redeploys and its CommonJS resolver could not resolve import-only package exports. The follow-up tracks resolution candidates and package entries explicitly. The focused redeploy suite passes 18/18 under Node and Bun; the exact Bun 4/6 shard passes 138 with 0 failures.