feat: MQTT 5.0 outbound (broker-assigned) Topic Alias (#840)

[BenjaminDobler]

Second of the next #821 batch, per the agreed sequence — the other low-blast-radius win. Broker-core only, cluster-safe, no persistence. Flips test_server_topic_alias from an EXPECTED_GAP toward a pass.

What

Inbound topic aliases (client→broker) already work; this adds the reverse (broker→client). When a client advertises a Topic Alias Maximum > 0 in its CONNECT (§3.1.2.11.5), the broker assigns aliases on outbound PUBLISH (§3.3.2.3.4):

• first PUBLISH on a topic → full topic + Topic Alias (registers the mapping)
• later PUBLISHes on that topic → empty topic + the alias

Per connection, bounded by the client's advertised max. Server MAY, so it's entirely off unless the client opts in.

How

• write.js — withOutboundTopicAlias rewrites a v5 PUBLISH just before serialize. It returns a shallow clone when aliasing, so the caller's packet — which persistence holds by reference for QoS > 0 and must keep its real topic for resend after reconnect — is never mutated. Non-publish / non-v5 / non-advertising clients hit a cheap early guard (no allocation).
• connect.js — parse the client's topicAliasMaximum into client._outboundTopicAliasMaximum; allocate the per-connection alias Map only when > 0.
• Strategy — assign 1..max in first-seen order, never evict; once the table is full, later new topics send the full name (no alias). (Eviction/LRU is a possible future optimization.)

Tests

Register-then-reuse (full topic + alias, then empty topic + same alias); not-advertised (no aliasing); full-table fallback to the full topic.

Note

The alias map is bounded by min(clientMax, distinct topics delivered) — a client advertising 65535 could grow it accordingly (self-inflicted, per connection). A broker-side cap option could be a follow-up if you'd like one.

369 unit tests + lint + tsd green.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.93%. Comparing base (9baa1c1) to head (2e487ae).

Additional details and impacted files

@@           Coverage Diff            @@
##           mqttv5    #1117    +/-   ##
========================================
  Coverage   99.93%   99.93%            
========================================
  Files          16       16            
  Lines        3094     3275   +181     
========================================
+ Hits         3092     3273   +181     
  Misses          2        2            

Flag	Coverage Δ
unittests	99.93% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robertsLando]

deep-review — feat: MQTT 5.0 outbound (broker-assigned) Topic Alias

Verdict: Ship with minor changes. The approach is sound and spec-correct on the wire (verified full-topic+alias and empty-topic+alias round-trip through mqtt-packet 9.0.2; the aliased frame parses back with topic === ""). Aliases are assigned 1..max, contiguous and never evicted, so size + 1 is always the correct next value; the per-connection Map is allocated only when the client opts in (spec-compliant Server MAY, §3.3.2.3.4).

Strength worth calling out: aedes-packet's Packet copies properties by reference into every fanout subscriber packet (this.properties = original.properties), so mutating it in place would corrupt other subscribers. withOutboundTopicAlias correctly returns a shallow clone with a fresh properties object instead — the subtle correctness lynchpin, handled right, and the inline comment explains why.

Major (out-of-diff, so noted here) — reconcile the Paho EXPECTED_GAPS entry

tools/mqtt-compat/run_compat.py:52 still annotates test_server_topic_alias as "broker-assigned (outbound) topic aliases are not implemented… (#840)". This PR implements #840, so that note is now false. Per the compat-harness convention (tools/mqtt-compat/README.md §Expected gaps), when a gap starts passing the harness emits an UNEXPECTED_PASS/🎉 warning and the entry must be removed. The diff doesn't touch run_compat.py, and the PR description itself says it "flips test_server_topic_alias from an EXPECTED_GAP toward a pass" — so this is a known, in-scope loose end.

Fix: run the MQTT-compat job — if the test now passes, delete the entry; if it still fails for a different reason, rewrite the note (don't leave "not implemented"). It only warns rather than fails CI, so it rots silently otherwise.

Line-anchored findings

1 Minor (connect.js), 1 Minor (test/mqtt5.js), 1 FYI (write.js) — see inline threads.

Coverage: correctness, MQTT-5 spec conformance (§3.1.2.11.5, §3.3.2.3.4, §3.3.2-8/11), security (memory amplification), performance (hot-path guard, clone cost), tests, docs. Clean on DRY, naming, doc accuracy.

[BenjaminDobler]

Addressed the review, and rebased onto the current mqttv5 (which clears the stale-base codecov/project failure):

• test/mqtt5.js:144 [Minor] — per-connection reset — added a reconnect test pinning §3.3.2-11: after the subscriber reconnects, the first PUBLISH on a previously-aliased topic carries the full topic + a freshly-assigned alias (proving the fresh Client gets a fresh map, never carrying a mapping across connections).
• connect.js:309 [Minor] — broker-side outbound cap — deferred per your note and tracked in MQTT 5.0: add a broker-side cap for outbound Topic Alias allocation #1124; the code comment now references it. Happy to fold the option in here instead if you'd prefer it in-PR.
• write.js:40 [FYI] — agreed, no action (a write failure tears down the connection and discards the map, so an aliased send never follows a failed full-topic send).

Full suite green, lint + tsd clean.

[github-actions]

📊 MQTT Compatibility Report

Aedes tested against the Eclipse Paho interoperability suite (the vendor-neutral broker conformance suite used by Mosquitto, EMQX, mochi-mqtt, …).

Protocol	Compatibility	Passed	Total
MQTT 5.0	65%	17	26
MQTT 3.1.1	100%	10	10

MQTT 5.0 — 17/26 passed (3 expected gaps)

Test	Result	Notes
test_assigned_clientid	✅
test_basic	✅
test_client_topic_alias	✅
test_dollar_topics	✅
test_flow_control1	❌ ⚠️ expected	receiveMaximum is advertised but not yet enforced outbound (advisory; #829, deferred per #821)
test_flow_control2	⏭️	not evaluable under per-test isolation (reuses the persistent client id / background receiver the Paho suite only sets up in its single-process mode); the Paho reference broker also hangs it here. The receiveMaximum behaviour it probes is still measured by test_flow_control1.
test_keepalive	✅
test_maximum_packet_size	❌	AssertionError: 2 != 1 : [('TopicA', b'................................', 0, False, 0, <mqtt.formats.MQTTV5.MQTTV5.Properties object at 0x7f3284dff230>), ('TopicA', b'................................................................', 1, False, 13861, <mqtt.formats.MQTTV5.MQTTV5.Properties object at 0x7f3284c6e360>)]
test_offline_message_queueing	❌	AssertionError: False is not true : 0
test_overlapping_subscriptions	✅
test_payload_format	✅
test_publication_expiry	❌	AssertionError: 0 != 2 : []
test_redelivery_on_reconnect	✅
test_request_response	✅
test_retained_message	✅
test_server_keep_alive	❌	AssertionError: False is not true
test_server_topic_alias	✅
test_session_expiry	✅
test_shared_subscriptions	❌ ⚠️ expected	aedes advertises sharedSubscriptionAvailable=false (deferred until cluster-aware; lib/handlers/connect.js)
test_subscribe_failure	✅
test_subscribe_identifiers	❌ ⚠️ expected	a delivery matching multiple overlapping subscriptions echoes only one Subscription Identifier (#828 [MQTT-3.3.4-4] — deferred per #821)
test_subscribe_options	✅
test_unsubscribe	✅
test_user_properties	✅
test_will_delay	⏱️	exceeded 45s timeout
test_will_message	❌	AssertionError: 0 != 1 : []
test_zero_length_clientid	✅

MQTT 3.1.1 — 10/10 passed

Test	Result	Notes
testBasic	✅
test_dollar_topics	✅
test_keepalive	✅
test_offline_message_queueing	✅
test_overlapping_subscriptions	✅
test_redelivery_on_reconnect	✅
test_retained_messages	✅
test_subscribe_failure	✅
test_unsubscribe	✅
test_zero_length_clientid	✅

_{⚠️ expected = a feature aedes intentionally does not implement yet, counted as a failure in the raw percentage. 🎉 update gaps = an expected gap that now passes — update EXPECTED_GAPS.}

[robertsLando]

deep-review: MQTT 5.0 outbound Topic Alias

Scope: 4 files, +146 / −2. Base mqttv5, head 7f43786. Sensitive: lib/write.js is the universal serialize path for every outbound packet.

Verdict: Needs work — one crash path introduced on client-controlled input, plus the compat gap list that this PR's own premise invalidates. Both fixes are small.

Top 3 risks

1. A duplicated Topic Alias Maximum property in CONNECT now kills the connection on first delivery (connect.js:311).
2. EXPECTED_GAPS still claims this feature is unimplemented — and the compat run proves otherwise.
3. The alias table is unbounded and sized by the client; measured 19.1 MB per connection for 10k topics.

Strengths

• The clone-not-mutate design is right, and I verified it empirically: persisted QoS packets keep their real topic across reconnect.
• Non-v5 clients are guarded twice (version !== 5 in write.js, and _outboundTopicAliasMaximum = 0 in connect.js).
• The vendor-neutral Paho suite validates the feature in both directions, and the v5 score moves 16/26 → 17/26.
• All three prior review threads are genuinely addressed (#1124 filed and open; reset-on-reconnect test added).

---

Major — tools/mqtt-compat/run_compat.py:52-54 (outside the diff)

test_server_topic_alias is still listed as an expected gap — "broker-assigned (outbound) topic aliases are not implemented" — and this PR is what implements it.

Not theoretical. The compat artifact for this exact head (run 29258181729) records "unexpected_pass": true for that test, so the ::warning …update EXPECTED_GAPS:: annotation fired. The job still concludes success because the step only prints.

Cross-run comparison, so the delta is attributable:

run	branch	v5 score	test_server_topic_alias
29818164110	mqttv5 (base)	16/26	fail [gap]
28871723205	unrelated branch	16/26	fail [gap]
29258181729	this PR	17/26	pass — UNEXPECTED PASS

The other six failures are byte-identical across all three, so nothing regressed.

Fix: delete lines 52-54 in this PR. The v5 dict then holds exactly test_shared_subscriptions, test_flow_control1, test_subscribe_identifiers.

---

Workflow findings (outside the diff)

• [Minor] .github/workflows/mqtt-compat.yml:103-119 — the unexpected-pass guard is advisory. ::warning cannot fail a job, which is precisely how a stale gap list reached a green PR. raise SystemExit(1) inside if xpass: makes the marker do its job.
• [Minor] .github/workflows/mqtt-compat-comment.yml:55-61 — download-artifact hard-fails when no artifact exists, which is reachable since the producer uploads with if-no-files-found: ignore. The later fs.existsSync('compat-result/report.md') guard is unreachable in that case. Add continue-on-error: true to the download step.
• [Minor] .github/workflows/mqtt-compat-comment.yml:53 — checkout@v7 with default persist-credentials: true in the job holding pull-requests: write; ci.yml:24,41 already sets it false. Hardening only — nothing untrusted executes after that checkout.
• [Minor] .github/workflows/ci.yml:23,28,39,45,66 — checkout@v4, setup-node@v4, codecov-action@v5, dependency-review-action@v4 are 1-3 majors behind and run on the deprecated Node 20 action runtime, while mqtt-compat.yml is already on checkout@v7. ci.yml also has no concurrency: block. Pre-existing, not this PR — noted for the branch.
• [Nit] tools/mqtt-compat/README.md:86 — says the suite exercises "inbound topic aliases"; with this PR it exercises broker-assigned outbound aliases too.

The workflow_run pattern in mqtt-compat-comment.yml was audited specifically for the classic escalation bugs and is clean: the PR number comes from trusted workflow_run metadata cross-checked against head_sha, the artifact body reaches github-script via env: rather than [object Object] interpolation, and the $GITHUB_OUTPUT heredoc uses a random delimiter. The only residual is content spoofing — a fork controls report.md verbatim, so it can make the bot post arbitrary markdown under the repo's identity.

---

FYI

• Duplicated-property handling is a general gap, not specific to this PR: receiveMaximum and maximumPacketSize take the same array shape today (connect.js:304-305). Rejecting duplicates with 0x82 at parse time would close the whole class, and would also fix the Blocker.
• v5 compat failures unchanged by this PR: test_maximum_packet_size, test_offline_message_queueing, test_publication_expiry, test_server_keep_alive, test_will_delay (timeout), test_will_message. None are in EXPECTED_GAPS — the list tracks a curated subset, and the workflow warns on unexpected passes but never on unlisted failures.
• lib/client.js:39 allocates the inbound _topicAliases Map unconditionally even when broker.topicAliasMaximum is 0 (the default), so every client pays for a Map it can never use. connect.js:312 shows the better lazy pattern. Out of scope here.

---

Coverage

Ran: correctness/concurrency, compat-workflow/CI, tests + DRY + design, prior-thread reconciliation.

Clean on: DRY (inbound alias→topic bounded by the broker option, outbound topic→alias bounded by the client's advertised max — genuinely different structures, nothing to extract; a shared helper would be worse), types (_-prefixed internals are deliberately undeclared, consistent with _wireVersion/_requestProblemInformation; no broker option ⇒ types/instance.d.ts correctly unchanged), packet-mutation safety, QoS retransmission, init-runs-once lifecycle (a second CONNECT is rejected at handlers/index.js:26-34; takeover installs a new Client), t.plan counts, test isolation.

Not run: the clause-by-clause MQTT 5.0 conformance matrix — that agent died on a session limit. Substituted direct verification against 7f43786: aliases stay in 1..max and never reach 0; absent or 0 topicAliasMaximum produces no alias; v3/v4 subscribers never receive one; mappings reset per Network Connection; retained, will and QoS 1 deliveries all alias correctly; topic: '' + topicAlias serializes correctly in mqtt-packet 9.0.2 — each probed live, plus test_server_topic_alias passing in the Paho suite. No per-statement matrix was produced, and no [MQTT-x.x.x-x] numbers are cited that couldn't be confirmed verbatim.

Local verification: test/mqtt5.js 73/73 pass at 7f43786; all 17 PR checks green.

[robertsLando]

[Minor] · Design / altitude

Per-feature v5 PUBLISH logic on the universal serialize path.

write() has 13 call sites (pubrec.js:29,34; client.js:143,360,555; connect.js:500,567; ping.js:9; publish.js:84,93,99,138; unsubscribe.js:98; subscribe.js:272), and exactly two ever carry a publish: client.js:143 (deliver0) and client.js:555 (writeQoS). This file's own header comment calls it "the broker's universal (v3/v4 included) hot path" and goes out of its way to avoid a per-write allocation — then this adds a call + property load to every PINGRESP, PUBACK and SUBACK.

deliver0/writeQoS is already the "shape the outbound PUBLISH for this client" seam — no-local, dedupe, qos downgrade and authorizeForward all live there (client.js:123-176).

Why: correctness is fine either way; this is placement. It also entangles the commit-after-success fix above with the generic write path — at deliver0/writeQoS the write callback is already in hand, so committing after success is natural there.

[robertsLando]

Still open as of this review: withOutboundTopicAlias is called unconditionally from write(), which is the universal v3/v4/v5 path, with the version check inside the callee rather than at the call site.

[robertsLando]

Still open as of the round-7 review at 0d2f83d — withOutboundTopicAlias is still called from the universal write() path rather than behind a v5 branch.

[BenjaminDobler]

Addressed the review (rebased onto current mqttv5 after #1116 merged). Commit 52a8a38.

Blocker · dup Topic Alias Maximum → crash — connect.js now honors only a numeric topicAliasMaximum; a duplicated property (decoded as an array) disables outbound aliasing for the connection instead of slipping past the guards and crashing on first delivery. New wire test drives the exact repro (duplicated 0x22, then a delivery) and asserts the connection survives with the full topic.

Major · unbounded alias table — clamped the effective per-connection max to min(clientAdvertised, 64) (OUTBOUND_TOPIC_ALIAS_LIMIT); the configurable version stays in #1124.

Major · EXPECTED_GAPS — removed test_server_topic_alias from the gap list, and made the unexpected-pass guard raise SystemExit(1) so a stale gap list fails the job instead of only warning.

Major · test coverage — added the two missing lanes: an aliased QoS 1 message left un-acked and resent after reconnect (raw subscriber so PUBACK is under our control) carries the full topic; and a fanout with two v5 subscribers (independent per-connection alias sequences — the spread-not-mutate proof on the shared properties object) plus a v3/v4 subscriber that never receives an alias.

Minors — Buffer-topic guard (typeof === 'string'); a comment tying the alias-commit-before-write ordering to the planned maximumPacketSize follow-up; _topicAliases → _inboundTopicAliases; a waitFor helper replacing the seven busy-waits; broker-side deregistration wait on reconnect; strictEqual for the aliasing-off assertions; dropped the redundant end()s; docs moved to a connackSent callout; README nit.

Two I did not fold in — flagging for your call rather than guessing:

• write.js:58 (altitude) — left aliasing on the universal write() path. The early-return (!max || cmd !== 'publish' || version !== 5) is O(1) and short-circuits before any publish-specific work, so the per-write cost on PINGRESP/PUBACK/etc. is a single cheap branch. Moving it to deliver0/writeQoS is a reasonable refactor but touches the hot path in two places; happy to do it if you'd prefer.
• Duplicated-property class-fix — your FYI (reject duplicate v5 properties with 0x82 at parse time) also covers receiveMaximum/maximumPacketSize, which are already on mqttv5. That's really a base-branch hardening; I'd rather do it as its own change than smuggle a behavior change for those two into this PR. Want me to open an issue / small PR for it?

Full suite green, lint + tsd clean.

[robertsLando]

deep-review round 2 — 52a8a38

Verdict: Ship with minor changes. Everything from round 1 is genuinely fixed — verified against the code, not the commit message.

Round-1 finding	Status	Evidence
Blocker · dup Topic Alias Maximum	Fixed	connect.js:314-317 numeric guard. The new wire test really does emit 06 22 0005 22 0005 and re-parses to [5,5]; it would have thrown on 7f43786.
Major · unbounded alias table	Fixed	OUTBOUND_TOPIC_ALIAS_LIMIT = 64 at the single assignment site; grep shows one write, one read — no unclamped path. A smaller client value is still honored (min(2,64) = 2).
Major · stale EXPECTED_GAPS	Fixed	Entry removed, remaining four intact, ast.parse clean. SystemExit(1) propagates — heredoc not a pipe, no continue-on-error, and if: always() doesn't mask a step's own exit code.
Major · test coverage	Partial	Both lanes exist and their behavioural assertions are sound, but neither pins the clone — see the line comment.
Minors (Buffer guard, ordering comment, rename, waitFor, dereg wait, strictEqual, docs callout, README)	Fixed	Rename is complete: grep -rnw '_topicAliases' lib/ aedes.js types/ test/ docs/ → zero hits. waitFor throws past a 2 s deadline and replaced all seven alias-lane busy-waits.

test/mqtt5.js 78/78, test/qos2.js 21/21. No rebase fallout — the extra files in 7f43786..52a8a38 are #1116 landing on mqttv5 (merge-base = 0f5fc3f).

Two Minors as line comments above. Your two questions are answered in a separate comment on the PR thread. Plus:

• [Nit] .github/workflows/mqtt-compat.yml:102 — the step is still named "Warn on unexpected passes" although it now fails the job. "Fail on unexpected passes".
• [Nit] waitFor is used only in the alias lanes; ~15 raw while (…) await delay(5) busy-waits remain elsewhere in test/mqtt5.js (:602, :827, :860, :885, …), mostly from the rebase. Not this PR's scope, but the helper exists now and the file is inconsistent.
• [FYI] The cap bounds entry count, not bytes. 64 never-evicted keys with topics up to 65535 B (unbounded unless maximumPacketSize is set) is ≈4.2 MB/connection worst case — vastly better than the ~4 GB ceiling before, but #1124 may want a byte budget rather than an entry count.

---

Coverage

Verified this round: round-1 fix confirmation (adversarial, per-item), regression hunt on all new code, rename completeness, workflow exit-code propagation, mqtt-packet property-decoding domain, and a mutation test of the clone invariant. npm test in full (lint + tsd + all suites) exceeded my 2-minute budget and was not run to completion — the 78/78 and 21/21 figures are node --test on test/mqtt5.js and test/qos2.js only. CI is green on all 17 checks.

[robertsLando]

Answering your two open questions (round-2 review is posted separately with the line-level items).

write.js:58 (altitude) — keep it where it is. My enumeration was wrong, and that changes the answer.

I told you only 2 of 13 write() call sites carry a publish. There's a third: emptyQueueFilter at lib/handlers/connect.js:573 — the offline-queue flush on reconnect — writes queued packets straight through write(), and it explicitly branches on packet.cmd === 'publish' two lines above. It is neither deliver0 nor writeQoS. Moving the transform would have silently dropped aliasing for every queued message replayed at reconnect. Not a correctness bug — an unaliased full-topic PUBLISH is always valid — but a silent, untested behavioural hole, which is exactly what a single serialization funnel prevents.

The altitude argument also runs the other way from how I first put it. Topic aliasing is a pure wire-encoding concern: same message, different bytes. deliver0/writeQoS is the routing and authorization seam — no-local, dedupe, QoS downgrade, authorizeForward — all broker-semantics decisions. Encoding belongs at the serialize boundary. Withdrawing the finding; your reasoning on the O(1) early return was right, and the placement is better than the alternative I proposed.

Duplicated-property class-fix — agreed, separate change. Please open the issue. I searched; there isn't one yet.

Three things worth capturing in it so the scope is deliberate:

1. It's a base-branch behaviour change. receiveMaximum and maximumPacketSize already ship on mqttv5 accepting the array shape, and today aedes silently degrades. The follow-up makes it strict — 0x82, connection refused — for CONNECTs that currently succeed. That deserves its own commit and its own bisect point, not a fold-in here.
2. The spec basis is per-property ("It is a Protocol Error to include X more than once" recurs for each), so the right shape is a general CONNECT-property validation pass, not three special cases.
3. The array shape is not a reliable duplicate signal. mqtt-packet's parser promotes to an array behind a truthiness check (parser.js:772), so a duplicate whose first occurrence is 0 is silently overwritten rather than arrayed. Detecting duplicates properly has to happen in the parser — likely an upstream mqtt-packet change — which is another reason it doesn't belong in this PR.

Nothing couples the two: this PR already closes the Blocker defensively, so the class-fix can land whenever.

[BenjaminDobler]

Thanks — round-2 items addressed in 8cb493c.

connect.js:315 [Minor] — close the class. Switched the guard to advertisedTopicAliasMax > 0 ? Math.min(…) : 0, the same predicate write.js uses (!max), so a negative value yields 0 at both sites and the guards can't diverge. Confirmed your preConnect repro and added it as a regression test (negative max → aliasing off, full topic, no crash).

test/mqtt5.js:261 [Minor] — the clone test now discriminates. You were right that it didn't pin the clone (QoSPacket copies topic by value). Rewrote it around the reference-shared properties object: the publisher sends a User Property, an aliased QoS 1 message is left un-acked, the session reconnects advertising no Topic Alias Maximum, and the resent PUBLISH must carry no topicAlias. I ran your exact mutation patch — the new test fails on it (actual: 1, expected: undefined) and passes on the shipped clone.

[Nit] Renamed the step to "Fail on unexpected passes".

Duplicated-property class-fix — opened #1131, capturing your three scoping points (base-branch behaviour change on receiveMaximum/maximumPacketSize; a general per-property validation pass, not three special cases; and that the array shape isn't a reliable duplicate signal — likely an upstream mqtt-packet change). This PR keeps the defensive Blocker fix, so #1131 can land independently.

FYI (byte budget vs entry count) and the remaining non-alias busy-waits — noted; the byte-budget point is worth adding to #1124, and the busy-wait cleanup is out of scope here now that waitFor exists. Left both.

Full suite green, lint + tsd clean.

[robertsLando]

Deep review — round 3

Verdict: Needs work, but narrowly. No blockers; ~30 LOC of changes, not a rework.

Top 3 risks

1. No broker-side kill switch — outbound aliasing is fully client-triggered and un-tunable.
2. The alias table is bounded by count (64), not bytes, and never evicts — memory amplification plus permanent starvation on churning topic sets.
3. The Paho compat job cannot fail on the regression it was added to catch.

Strengths

• The tests are genuinely discriminating, not tautological. Verified by mutation: reverting withOutboundTopicAlias to mutate in place, and breaking the aliases.size >= max boundary, each produced a failing test.
• Non-alias overhead measured at <0.5% (1.8 ns/packet with no field, 3.5 ns with max = 0, against ~1 us for writeToStream). The !max-first predicate ordering is what buys that — keep it first if the guard grows.
• The EXPECTED_GAPS removal is backed by a real passing run, not a silenced entry: CI run 29898530229 shows test_server_topic_alias: pass.
• Two prior review rounds landed 13 of 15 threads.

Theme. Most of the Majors share one root cause: the resource policy is hardcoded and client-driven rather than an operator-owned broker option. Deferring that to #1124 is what turns a single design gap into four separate findings.

Finding outside the diff

[Minor] · Security — lib/client.js:404 close() releases neither _outboundTopicAliases nor _inboundTopicAliases, unlike the adjacent _parser._queue = null. Audited and this is not a leak — Client is a fresh instance per TCP connection and close() -> unregisterClient drops the broker's reference on every disconnect path (error, timeout, takeover, close), so both maps become collectible with the client. It only shortens the retention window when something still holds a closed client (conn.client backref, in-flight mqemitter callbacks, takeover races). Cheap to do while you are there.

Prior threads

• lib/write.js:70 — still open. Per-feature v5 logic sits on the universal write() hot path rather than deliver0/writeQoS. The placement is defensible (it is the only choke point covering client.publish(), retained and queued sends) — but every other per-client outbound PUBLISH shaping (nl, rap, subscriptionIdentifier, messageExpiryInterval) lives in the delivery wrappers at subscribe.js:225-243 and connect.js:530-537, including the same clone-never-mutate invariant. Worth stating the rationale where the other shaping lives so the split does not read as accidental.
• lib/handlers/connect.js:315 — behaviour now matches (> 0 ? Math.min(...) : 0 closes the -1 case, pinned by test/mqtt5.js:417-441). The thread's textual ask (one shared predicate rather than two expressions) is still literally true; the remaining live gap is the Number.isInteger comment below.

Audited clean

• Commit-before-write is safe as the in-code comment claims: every write() error path (deliver0 -> _onError, writeQoS -> emit('error') via the listener at client.js:116, emptyQueue -> pipeline -> _nextBatch(err)) destroys the socket, and mqtt-packet validates topic/messageId/properties before emitting any bytes. No broker/client alias desync. The caveat about a future packet-dropping path is the right guard to keep.
• No cross-client leakage: resolveTopicAlias clears the publisher's inbound alias (publish.js:174) before fanout, and write.js spreads both packet and packet.properties rather than mutating the reference-shared objects.
• Workflow security: on: pull_request (not pull_request_target), permissions: contents: read, no secrets, pinned Paho ref. No new dependencies.
• Pre-existing, not introduced here: restoreSubs runs before doConnack, so a concurrently published message can be written — and now register an alias — before the CONNACK. Aliasing makes it more visible.

Coverage

Eight specialists run against a dedicated worktree at the PR head (8cb493c): correctness, security, performance, DRY/codebase-fit, design/API/backcompat, tests, operability, readability. None skipped. Repo-wide DRY and sibling-file context intact. Test findings are mutation-verified; performance figures are measured, not estimated.

[robertsLando]

[Major] · Security / Performance / Correctness (flagged independently by three specialists)

The cap bounds entry count, not bytes, and entries are first-come with no eviction. Two distinct failure modes:

(a) Memory. An MQTT topic is up to 65535 bytes, so 64 never-evicted entries is ~4 MB of client-controlled per-connection state — measured 1.9-3.4 KB retained for a realistic full table, ~190-340 MB at 100k v5 connections, and far worse under attack. A client advertises topicAliasMaximum: 64, subscribes #, self-publishes 64 max-length topics, then idles (keepalive is client-chosen, maximumPacketSize defaults to unlimited) — transient upload converts into persistent retention for the connection's life. This is net-new surface: before this PR the broker held no per-connection outbound topic table at all. Note the asymmetry with the inbound table, which is admin-bounded via broker.topicAliasMaximum (default 0); this one is client-bounded.

(b) Starvation. MQTT 5's own request/response idiom uses unique reply topics (resp/<uuid>). A # subscriber — or any wildcard covering churning topics — fills all 64 slots with one-shot topics that never repeat, then aliases nothing for the rest of the connection. Each of those 64 also paid +3 bytes for a registration that never amortizes. The feature becomes pure overhead on exactly the busiest connections.

Fix: "never evict" is a design choice, not a spec constraint — §3.3.2.3.4 explicitly permits reassigning an alias by resending it with a new topic name, so eviction is legal and O(1). LRU on the least-recently-used slot, or registering a topic only on its second send, fixes (b) and raises hit rate; bounding total key bytes fixes (a). The constants.js:15-20 comment currently claims the count bound addresses the amplification vector — it addresses only half of it.

[robertsLando]

Still open, and this round escalates it to a Blocker with numbers.

The table is bounded by entry count (64), never by bytes. validateTopic caps topic levels, not length, so a single topic can be ~64 KB, and the Map keys pin those strings for the whole connection when they would otherwise be garbage right after delivery.

Measured on this branch: 20 connections x 64 distinct 60 KB topics retained 146.9 MB (7.3 MB per connection). The identical run with outboundTopicAliasMaximum: 0 retained 0.3 MB. That extrapolates to roughly 7 GB at 1000 connections — and it is on by default.

A byte budget for the table would close it, or simply skip aliasing topics over a few hundred bytes: long topics are the least likely to recur, so they are the worst candidates for an alias slot anyway.

[robertsLando]

Still open at 0d2f83d, and this round's security pass raised it again independently: aedes.js:95-97 floors outboundTopicAliasMaximum at 0 but sets no ceiling, unlike the sibling maxTopicLevels two lines below. Worth noting the framing in the PR body ("self-inflicted per connection") isn't quite right — the table is keyed by full topic strings chosen by whoever publishes, and one retained copy lands on every subscribed connection, so the broker pays N-times amplification for content a third party picks.

[robertsLando]

[Minor] · DRY & Codebase Fit

waitFor is a third variant of a wait idiom that already exists twice over: this file inlines while (cond) await delay(5) roughly 18 times (e.g. :635, :860, :1054, :1362, :1492) plus a bespoke serverConnected poller at :1567, and shared async test utilities live in test/helper.js (withTimeout, checkNoPacket, nextPacketWithTimeOut).

Why: the real win here — a timeout instead of a 3-minute hang — does not reach any of the existing loops while it stays file-local.

Fix: put it in test/helper.js next to the other wait helpers. Migrating the raw loops can follow separately.

[robertsLando]

Still open: waitFor is now the third wait idiom in this file, and the new test at line 246 uses the older while (!broker.clients[...]) await delay(5) form rather than the helper this PR adds.

[robertsLando]

Still open at 0d2f83d — waitFor is still a third wait idiom alongside the inline while-loop the new tests use.

[robertsLando]

[Nit] · Design / Docs

The outbound-alias callout lives under Event: connackSent, and the option bullet at line 55 links here — but the behaviour it describes concerns outbound PUBLISHes, not the CONNACK event. Optional: move it near Event: publish or give it its own section.

[robertsLando]

Still open (nit): the callout sits under connackSent, which is not where a reader looks for outbound PUBLISH behaviour.

[robertsLando]

Still open at 0d2f83d — the outbound-alias explainer is still under Event: connackSent rather than near the option it documents.

[BenjaminDobler]

Round-3 addressed in 5051430.

Major · broker option (flagged ×3) — landed here rather than deferring to #1124: outboundTopicAliasMaximum (default 64, 0 disables), in the established pattern — defaultOptions, AedesOptions + Aedes types, a docs/Aedes.md bullet, runtime clamp. Effective per-connection max is min(client-advertised, broker option). Tests: a client advertising above the cap is clamped; 0 disables outbound aliasing.

Major · caller-supplied topicAlias — a packet reaching the public client.publish() with properties.topicAlias already set is now caller-owned: forwarded verbatim, not broker-aliased, so the two tables can't disagree and misdeliver.

Major · compat CI can't go red — run_compat.py now sys.exit(1) on any non-gap, non-skipped failure (and on an unexpected pass), so a regression in test_server_topic_alias (removed from EXPECTED_GAPS here) turns the check red, not just a lower percentage.

Minors — Number.isInteger on the advertised max; both outbound alias fields declared in the constructor (monomorphic shape, local > 0 ⇒ Map invariant); close() releases both maps; the max-> 64 clamp now has a test.

Fanout test [Minor] — you're right it wasn't discriminating, and digging in, it can't be on the QoS 0 fanout path: each subscriber's deliver0 re-runs withOutboundTopicAlias synchronously before its own writeToStream, so an in-place properties.topicAlias mutation is overwritten per-subscriber and never leaks across them (only the QoS 1 poison test at :328 catches the mutation, via persistence). I reframed the test to assert what it genuinely proves — distinct per-connection alias numbering (subA gets alias 2, subB alias 1) + v4 exclusion — and moved the clone claim to where it's actually pinned. Happy to hear if you see a fanout-observable angle I missed.

Deferred with reasoning:

• Byte budget / eviction (Major) — the option + min() bound the count; a byte budget and LRU/register-on-second-send are a real improvement but a design change (eviction semantics, hit-rate tuning). I'd rather do it as a focused follow-up on MQTT 5.0: add a broker-side cap for outbound Topic Alias allocation #1124 now that the option exists — the operator can already set outboundTopicAliasMaximum: 0 to opt out entirely.
• QoS 0 clone allocation (Minor) — overlaps the still-open write.js altitude thread; a caller-ownership signal to skip the clone on QoS 0 is worth doing but touches the delivery wrappers, so also better as its own change.
• close() map release — done (you flagged it as cheap-while-here).

Full suite green, lint + tsd clean.

[robertsLando]

Deep review — outbound Topic Alias

Verdict: Needs work — three Blockers, all in the new alias path.

Top 3 risks

1. A client's Will properties can inject a Topic Alias that hijacks an alias slot in other clients' tables.
2. A caller-set alias silently desynchronizes the broker's table, misdelivering later messages under the wrong topic.
3. close() leaves the max/map pair inconsistent, dropping in-flight QoS>0 writes and firing spurious clientError.

Theme: all three share one root cause — properties.topicAlias is treated as broker-owned state, but it is remotely settable and never re-validated. A second theme is teardown: the alias maps are the only per-connection state in client.js that gets nulled, and nothing that reads them was updated to expect null.

Strengths. The non-aliasing path is genuinely cheap (one monomorphic read, then return) — the guard ordering was done right. The clone-not-mutate invariant holds for persistence and MQEmitter, so QoS>0 resend keeps its real topic. Direction separation between inbound (broker.topicAliasMaximum, §3.2.2.3.8) and outbound (§3.1.2.11.5) is correct. Per-connection lifetime is right — the map lives on Client, so it never survives a takeover. Tests are deterministic (no fixed sleeps) and several assert on wire-decoded bytes via raw sockets. The previous review round's 20 addressed threads landed real fixes.

MQTT 5.0 conformance

Mostly right, two real violations.

Correct: direction separation (§3.2.2.3.8 inbound vs §3.1.2.11.5 outbound), per-connection lifetime (§3.3.2.3.4, §4.1 — never crosses a takeover), and §4.4 resend safety (the spread preserves dup/messageId/qos/retain, the stored packet keeps its real topic, and the fresh post-reconnect table forces a full topic).

Violated: [MQTT-3.1.2-27] and [MQTT-3.1.2-26] on the caller-alias path (see the inline Blockers), plus the §3.1.2.11.5 duplicate-property Protocol Error handled leniently (inline Minor).

Compat workflow — up to date, and the PR undersells it

Verified by reading the full files, not just the diff: test_server_topic_alias is cleanly moved out of EXPECTED_GAPS["v5"] into the new EXPECTED_PASSES["v5"], and a failure now sets regressions → sys.exit(1) → red job. It is hard-gated, not merely re-counted. Both directions are checked (a stale gap that starts passing also fails the build), and no other list needed syncing — a repo-wide grep for EXPECTED_GAPS / EXPECTED_PASSES / test_server_topic_alias came back clean.

Actions are all on current majors (checkout@v7, setup-node@v6, setup-python@v6, upload-artifact@v7), PAHO_REF is pinned to a commit SHA, Node 24 matches ci.yml's newest matrix entry and engines.node: ">=20", and permissions / concurrency / timeout-minutes: 20 are all present. It runs on every PR with no path filter — broader than "PRs touching lib/", not narrower.

Findings outside the diff

These are real but anchor to unchanged lines, so they can't be inline comments:

• tools/mqtt-compat/README.md:50-65 — still says "Two categories are handled specially" and "Both lists in run_compat.py". The new CI-gating EXPECTED_PASSES set is undocumented, so a contributor following the README won't learn a must-pass gate exists or how to add to it. Verified: no occurrence of EXPECTED_PASSES anywhere in the README.
• test/types/aedes.test-d.ts:26 — sets topicAliasMaximum but never outboundTopicAliasMaximum. tsd is the only compile-check of the new declaration, so the field can drift unexercised.
• lib/handlers/connect.js:331 — client._will = packet.will stores the Will with its properties intact; this is the source half of the Will-hijack Blocker commented inline on lib/write.js.

Still-open threads from the previous round

31 unresolved threads reconciled against the PR head: 20 genuinely addressed (verified against the code, resolving those separately), 11 still open. Of those 11, two are escalated by this round with new evidence and I've replied on them directly:

• lib/write.js:44 — count-bound cap, no eviction. Now measured: 20 connections × 64 distinct 60 KB topics retained 146.9 MB (7.3 MB/conn) vs 0.3 MB with outboundTopicAliasMaximum: 0. ~7 GB at 1000 connections, on by default.
• lib/write.js:60 — allocation per aliased publish. Now benchmarked: ~2× serialize cost, plus a suspected polymorphic-IC tax on non-aliasing clients.

The rest still stand as written: lib/write.js:77 (v5-only logic on the universal write() path), docs/Aedes.md:55/:237/:235 (the connackSent callout still says outbound aliasing is "automatic, no option" and points at #1124 for a configurable cap — while line 56 of the same file documents the option this PR ships; three specialists flagged this independently), lib/constants.js:21 (no test advertises a maximum above 64), test/mqtt5.js:382 (full-table test never re-verifies a cached alias), test/mqtt5.js:384 (no QoS 2 resend-alias twin), test/mqtt5.js:56 (waitFor is now the third wait idiom), test/mqtt5.js:321 (comment cites the wrong write.js lines).

Coverage

Dimensions run: correctness, security, performance, DRY/codebase-fit, design/API/backcompat, tests, operability, readability, MQTT-5 spec conformance, compat-workflow freshness. Readability came back essentially clean. Review ran against a worktree at the PR head (5e02cf5), so repo-wide checks had full context. Both Blocker claims were re-verified by hand against lib/write.js, lib/handlers/connect.js:331, lib/handlers/publish.js:176, and lib/client.js:430-431.

[robertsLando]

[Minor] · MQTT 5.0 conformance

A repeated Topic Alias Maximum in CONNECT — which mqtt-packet decodes as an array — is silently coerced to "aliasing off".

Why: §3.1.2.11.5 states that including it more than once is a Protocol Error, and §4.13 requires CONNACK reason code 0x82 followed by a close. test/mqtt5.js:591 now pins the non-conformant outcome (reasonCode === 0), so the deviation is locked in by a test. Blast radius is small, but it's worth being deliberate about.

Fix: either treat a non-integer Topic Alias Maximum as a Protocol Error, or document this as a deliberate lenient-parse policy shared with the other v5 properties — the current state reads as an oversight rather than a choice.

[robertsLando]

Still open at 0d2f83d — a duplicated Topic Alias Maximum property still silently disables aliasing instead of returning Protocol Error 0x82.

[robertsLando]

[Minor] · Operability

This "Fail on unexpected passes" step re-derives the same unexpected_pass condition that run_compat.py:384 (if regressions or xpass: sys.exit(1)) already fails on. By the time it runs, the job is red from the earlier "Run compatibility suites" step for the same test names.

Why: not incorrect, just dead weight — but it has an asymmetric side-effect. The regression path (an EXPECTED_PASSES test such as test_server_topic_alias starting to fail) surfaces only as a plain stderr line, with no ::error annotation, while the xpass path gets one. Verified the job still fails correctly either way (run: steps use bash -eo pipefail, so the script's exit code propagates).

Fix: drop the now-redundant step, or extend it to emit a ::error annotation for regressions too.

[robertsLando]

Still open at 0d2f83d — the workflow's inline "fail on unexpected passes" step still re-derives from result.json what run_compat.py already signals via its exit code. Two places to keep in sync for one rule.

[robertsLando]

[FYI] · Operability

EXPECTED_PASSES treats any non-pass status as a regression, including the harness's own timeout status. A transient Paho subprocess timeout therefore turns the check red with no retry.

This reads as intentional — a hard gate is the point — and I'd leave it. Flagging only so the flake surface is known before it fires on someone else's unrelated PR.

[robertsLando]

Still open at 0d2f83d (noted as FYI, no change was requested). Flagging only because this round found a related gap: EXPECTED_PASSES is membership-only, so if test_server_topic_alias ever stops appearing in results — a Paho rename on a PAHO_REF bump, or a load failure — regressions is empty and the job goes green, silently dropping the one hard gate protecting this feature.

[BenjaminDobler]

Round-4 addressed in 3935f52 (+ 0d2f83d for the compat broker). The three Blockers are fixed by shrinking the alias surface rather than adding reconciliation machinery.

Blockers

• Will-property alias hijack (write.js:35) — topicAlias on any outbound PUBLISH is now scrubbed at the write boundary before serialization (covers Will, retained, and caller sources in one choke point), and the Will is also scrubbed at storage (connect.js, the inbound-PUBLISH analogue of resolveTopicAlias). Verified your PoC: the victim now receives the Will with its real topic + a broker-assigned alias, never the hijacked {empty, alias:1}. New test drives a Will with a bogus topicAlias end-to-end.
• Caller-alias desync (write.js:31) — you were right that "forward verbatim" was the wrong call; it caused the divergence. topicAlias is broker-owned on the outbound path, so a caller value is now always stripped and the broker applies its own aliasing. Reworked the oalias-caller test to assert the broker alias wins (1, not the caller's 3) and reuses across publishes. Also fixes [MQTT-3.1.2-26/27] — a stray alias is scrubbed even on the disabled passthrough (new [MQTT-3.1.2-27] test).
• close() map/max inconsistency (client.js:430) — _outboundTopicAliasMaximum is now zeroed in the same statement as the outbound-map null, so withOutboundTopicAlias short-circuits on !max before touching it. Verified the race now matches the non-aliasing baseline (connection closed) instead of the swallowed TypeError. The inbound map is no longer nulled at all — per your own observation that subscriptions/duplicates (larger) aren't nulled either; it's collected with the Client, and resolveTopicAlias has no !max guard to hide behind.

Majors

• Default → opt-in (0) — this one change resolves three findings at once: the backcompat surprise (matches the topicAliasMaximum sibling's opt-in posture), the fail-open input handling (a non-integer/negative value now coerces to 0/disabled, never to an enabled default), and the 146 MB memory concern (off unless an operator turns it on and owns the number). Removed the OUTBOUND_TOPIC_ALIAS_MAXIMUM_DEFAULT constant (your nit — it was a broker-option default in the spec-constants file).
• Retained + Will delivery tests — added both (they build PUBLISH packets off the live-fanout path, which is exactly where the Will hijack hid).
• Docs: the connackSent callout no longer claims "automatic, no option"; option default corrected to 0; README documents the EXPECTED_PASSES CI gate; test-d exercises the new option.

Deferred, with reasoning

• Byte-budget / eviction (write.js:44) — the opt-in default de-fangs the memory vector (off by default; the operator sets and owns the cap). A byte budget + LRU is a real improvement but a distinct design change (eviction semantics, hit-rate tuning); I'd rather do it as a focused follow-up on MQTT 5.0: add a broker-side cap for outbound Topic Alias allocation #1124 now that the option exists, than bundle it here.
• Allocation perf / polymorphic-IC (write.js:60) — worth a fixed-shape clone, but you flagged the IC measurement as noisy and wanting a real-broker benchmark first; deferring so it's done against a confirmed number.
• v5-only logic on write() (write.js:77) — the choke-point placement is deliberate (it's the one path covering client.publish(), retained, and queued sends — and now the security-critical alias scrub); happy to add a rationale comment where the other outbound shaping lives if you'd prefer.
• Telemetry surface for the alias table — reasonable; I'd add it as a small follow-up rather than expand this PR's public surface.
• Dup Topic Alias Maximum lenient-parse (connect.js:315) — this is the one behaviour choice I'd like your call on: reject with 0x82 (strict, but a new close-the-connection path) vs. keep the current lenient "aliasing off" and document it as deliberate. I lean lenient+documented for a MAY feature, but will do strict if you prefer.

Full suite green, lint + tsd clean, compat gate green (test_server_topic_alias re-enabled on the compat broker since the option is now opt-in).

[robertsLando]

Round-7 deep review — outbound (broker-assigned) Topic Alias

Verdict: Ship with minor changes. No blockers.

Top 3 risks

1. The published .d.ts says the feature defaults to on with 64 slots when it defaults to 0 — the opposite of the opt-in posture the whole PR rests on.
2. A non-integer inbound alias bypasses the range check and grows the inbound map without bound (publish.js:153).
3. The compat harness's new hard gate is membership-only and goes green if the test simply disappears.

Strengths. The shallow-clone-vs-persistence invariant genuinely holds — verified that aedes-packet shares properties by reference across the fanout, and every aliasing branch clones both levels, so the packet persistence holds keeps its real topic. Per-connection scoping, 1..max numbering, reconnect reset and close() teardown all check out against §3.3.4; the cleanStart=false + un-acked-QoS1 case is pinned by a real test. Tests are live-socket and byte-level, not tautologies. The Client constructor initialises both new fields unconditionally, so there's no hidden-class split and v3/v4 clients pay one property read with zero allocation.

Measured interop. Downloaded the mqtt-compat-result artifacts: head 0d2f83d (run 33058791964) is v5 65% (17/26); the sibling feat/v5-enhanced-auth branch on the same base (run 33061639309) is v5 62% (16/26). Exactly one test differs — test_server_topic_alias ❌→✅. v3 is 100% (10/10) on both. Nothing regressed: test_client_topic_alias sends TopicAliasMaximum = 0 and no other v5 test sets the property, so broker.js's outboundTopicAliasMaximum: 65535 has zero blast radius. Good result.

Themes. (a) The type/doc surface has drifted from the code in four places. (b) The feature has no operator-facing signal at all.

Prior threads. Reconciled all 22 unresolved threads against this head: 9 were genuinely fixed and I've resolved them; 13 are still open and I've replied on each with what this round found. The biggest carry-overs, all confirmed again independently this round: the unbounded/never-evicting alias table (write.js:59), the two allocations per aliased publish (write.js:76), and the complete absence of an operator surface (client.js:44) — on that last one, when the table fills, write.js silently and permanently falls back to full topic names for the rest of the connection, dropping the entire benefit the operator enabled the feature for, with no event, counter or log. publish.js:130 emits clientError for every other notable v5 outcome, so it also breaks a local convention.

Not anchored to a line — docs/Client.md:133: withOutboundTopicAlias now strips properties.topicAlias from every v5 publish, including when outboundTopicAliasMaximum is 0, so callers of the documented public client.publish() who set it themselves silently lose it. Spec-justified, but it's an undocumented contract change on a public method.

FYI — 6 of the 10 v5 non-passes (test_will_message, test_will_delay, test_maximum_packet_size, test_offline_message_queueing, test_publication_expiry, test_server_keep_alive) are in neither EXPECTED_GAPS nor HARNESS_LIMITED: pre-existing, ungated, unflagged.

Coverage. Eight lanes ran against a worktree at 0d2f83d: correctness, security, performance, DRY/codebase-fit, design/API/backcompat, tests, operability, and MQTT-5 spec + Paho interop. Compat numbers above are measured from CI artifacts, not estimated.

[robertsLando]

[Minor] · Design / API

Third distinct validation style for a numeric option in this file: outboundTopicAliasMaximum silently coerces bad values to 0, the sibling topicAliasMaximum (line 94) takes opts raw, and maxTopicLevels clamps. The same "positive integer, else 0" gate is also written at connect.js:316, with write.js:47's !max and client.js:443 tied to it by prose.

Why: A realistic '64' from env or JSON silently disables the feature with no signal, and docs/Aedes.md:56 doesn't mention it. Four sites hold one invariant together with comments.

Fix: At minimum document that a non-integer or negative value disables it; better, extract one positiveInt() into lib/utils.js (alongside ackProperties/armLongTimer) and use it at both parse sites.

[robertsLando]

[Minor] · DRY & Codebase Fit

The other negotiated CONNECT limits set just above (client.maximumPacketSize, client.receiveMaximum) are public and declared in types/client.d.ts:38,43; the negotiated outbound alias max is _-prefixed and undeclared. Separately, broker-side clamping of a client-requested v5 value has a precedent as a broker method (aedes.js:356 clampSessionExpiry); this one is inlined in the handler.

Why: Same family of values, gratuitously different visibility — plugins and tests can inspect one and not the other.

Fix: Either expose it consistently or note why it differs.

[BenjaminDobler]

Round-7 addressed in 4d8d41d. Thanks for the measured interop numbers — good to see test_server_topic_alias flip with zero blast radius.

Majors

• .d.ts default (instance.d.ts:93) — fixed to (default: 0). That was a straight miss when I flipped the default to opt-in; the IDE-hover contradiction is gone.
• Non-integer inbound alias (publish.js:162) — added the Number.isInteger gate before the range check, the inbound sibling of the outbound guard. A duplicated (array) alias is now rejected 0x94 and never .set()s a fresh key. Test asserts the rejection and that the inbound map doesn't grow.
• EXPECTED_PASSES membership gate (run_compat.py:384) — now also fails when an expected-pass name is absent from the evaluated set (Paho rename / load failure) or is simultaneously in EXPECTED_GAPS/HARNESS_LIMITED. So the one hard gate can't silently vanish.

Minors

• Empty-topic + caller alias (write.js:56) — that packet is now left untouched (the strip only applies to non-empty string topics), so we never emit a zero-length-topic-no-alias frame.
• delete → undefined (write.js:27) — stripTopicAlias now assigns undefined, matching publish.js and avoiding the dictionary-mode deopt.
• write.js:66 comment — corrected to name the real stream.destroy → conn 'error' → _onError path (writeToStream returns false, doesn't throw).
• Clamp to 65535 (aedes.js:96) — added, matching the maxTopicLevels precedent.
• Inbound map (client.js:438) — .clear()'d symmetrically on close (kept a Map, so resolveTopicAlias still can't throw); the asymmetry paragraph is gone.
• QoS1 poison test now asserts resent.topic === 'o/q1'; full-table test re-publishes a cached topic and confirms the alias still resolves; stale write.js:19-21 citation and the [MQTT-3.3.2-11] → §3.3.2.3.4 citation fixed.
• README — the unexpected-pass path now says "fails the build".

Deliberate / deferred

• Duplicated Topic Alias Maximum → lenient (connect.js:315) — took your "document as deliberate" option rather than a behaviour change: outbound aliasing is a broker-side MAY optimisation, so degrading it on a malformed optional property is safer than refusing an otherwise-valid CONNECT (the receiveMaximum/authData Protocol Errors stay strict because they affect correctness). Documented in-code.
• Operator surface / table telemetry (client.js:44), byte-budget + eviction (write.js:59), the two-allocations-per-publish (write.js:76), and moving aliasing off the universal write() path (write.js:93) — all real, but each is a design/perf change I'd rather do as a focused follow-up on MQTT 5.0: add a broker-side cap for outbound Topic Alias allocation #1124 than fold into this PR now that it's opt-in (off by default, operator owns the number). Happy to pick them up next.
• .d.ts visibility of _outboundTopicAliasMaximum / the inline broker clamp — noted; can expose/extract a positiveInt() helper in a follow-up if you'd like the four sites consolidated.

Full suite green, lint + tsd clean, all CI checks green.

[robertsLando]

Round-8 deep review — outbound (broker-assigned) Topic Alias

Reviewed at 4d8d41d in an isolated worktree, across 8 parallel lenses (correctness, security, MQTT-5 conformance, performance, DRY, design/API, tests, operability) plus a prior-thread reconciliation pass.

Verdict: Needs work — narrowly. No Blockers; ~40 LOC plus two test fixes, not a rework.

Top 3 risks

1. Two tests that exist to protect the load-bearing clone contract were mutation-proven not to catch it breaking.
2. A caller-supplied topicAlias escapes the scrub on two reachable packet shapes, so the broker emits an alias the client never negotiated ([MQTT-3.1.2-27]).
3. The alias table is bounded by entry count, not bytes — open since round 5, and the amplification is worse than previously quantified.

Strengths. The spec work is genuinely correct: absent is treated identically to 0, allocation starts at 1, per-connection lifetime survives takeover, and QoS resend keeps the real topic. The Paho test_server_topic_alias flip is substantiated — I fetched client_test5.py at the pinned PAHO_REF 9d7bb80, and the test has no early return: three phases pinning the register/reuse pair, the absent case, and the explicit 0 case. mqtt-packet 9.0.2 behaviour was verified against source (topicAlias id 35 int16; getProperties skips undefined; writeToStream.publish() accepts topic: ''), not assumed.

Theme. The scrub half of this feature — always-on, independent of the option — is under-documented and under-tested relative to the aliasing half, which is opt-in and well covered.

Carry-over

12 of the 25 open threads are still unaddressed. I re-verified each against the head rather than trusting isOutdated. The one that moved is lib/write.js:65 (byte-unbounded table): the amplification is larger than round 5 quantified, because the table is filled by what the broker delivers — one attacker publishing 64 distinct 64 KB topics into a wildcard subscription fills the table of every v5 subscriber at once. Roughly 10,000x amplification: ~4 MB of attacker traffic against ~40 GB retained across 10k subscribers. At the 65535 ceiling that tools/mqtt-compat/broker.js:22 actually sets, it is ~4.3 GB per connection. Detail on the thread.

The other 13 threads are genuinely fixed; I am resolving those.

Not anchorable to the diff

[Minor] - Security / DRY / Spec (flagged independently by three lenses) - aedes.js:94, outside this diff. The inbound topicAliasMaximum still takes opts raw while its new sibling three lines below gets Number.isInteger + Math.min(..., 65535). The value is advertised as an int16 CONNACK property, so 65536, Infinity, or a JSON-config string makes mqtt-packet stream.destroy() every v5 CONNECT — a self-inflicted v5 outage. Infinity additionally lifts the alias > max bound in resolveTopicAlias. Pre-existing, but the asymmetry is newly visible now that one of the two adjacent options is hardened and the other is not.

Coverage

Lenses run: correctness, security, MQTT 5.0 conformance, performance, DRY/codebase-fit, design/API, tests, operability. Suite executed at the head: 86 pass. Readability was not run — tests and DRY covered the large new file, and no non-test source file cleared the size threshold.

[robertsLando]

[Major] - Tests

This test passes with withOutboundTopicAlias mutated to write in place on both branches — I ran it.

Two independent maskings: (1) aedes-packet's Packet constructor copies topic by value on every wrap, so the topic assertion holds regardless of write.js's clone choice; (2) the second connection reconnects with aliasing disabled, so the off-path stripTopicAlias strips the poisoned alias before it could reach the wire — the very defense this test is meant to check independently of.

Why: the shallow-clone contract that this PR's own comments call load-bearing is currently unprotected. An in-place-mutation regression ships undetected.

Fix: have the second connection reconnect still advertising a max, so the disabled-path stripping cannot mask the bug — or assert on object identity / the persisted packet rather than on final wire bytes after a masking code path.

[robertsLando]

[Major] - Tests

This test passes with all three reset lines deleted from lib/client.js close() (_outboundTopicAliasMaximum = 0, _outboundTopicAliases = null, _inboundTopicAliases.clear()) — I ran it.

A full disconnect-then-reconnect always builds a brand-new Client, so the reset-on-close path is never exercised; the new object simply supersedes the old state.

Why: the scenario the PR's comments explicitly worry about — an in-flight deliver0/deliverQoS from a closing client, or a live takeover where the old Client is still referenced — is not what this tests, and no test would catch a regression there.

Fix: force a genuine takeover (connect a duplicate clientId while the first connection is still open, not after end() and deregistration), and/or keep a reference to the old Client and assert close() actually cleared its alias state.

[robertsLando]

[Major] - MQTT 5.0 conformance / Correctness (two lenses)

The "aliasing off / not applicable" passthrough only scrubs a caller-supplied properties.topicAlias when the topic is a non-empty string. Two shapes escape and are written verbatim to a v5 client:

• a Buffer topic + topicAlias: n — callerAlias requires typeof === 'string', so this falls into return packet
• topic: '' + topicAlias: n — documented as intentional

Both are reachable through the public client.publish(), and both are emitted even when the feature is off (the default).

Spec: [MQTT-3.1.2-27] — "If Topic Alias Maximum is absent or zero, the Server MUST NOT send any Topic Aliases to the Client." With aliasing on, the empty-topic form additionally shares one namespace with the broker's own table, so the receiver resolves the alias to the wrong topic ([MQTT-3.3.2-11]/[MQTT-3.3.2-13]) — or, if unregistered, answers 0x94 and drops.

The oalias-27-sub test proves this path is considered reachable; it just doesn't cover these two shapes.

Fix: in the passthrough branch, scrub or fail the write on any caller alias the broker did not assign. An empty-topic packet is unsendable either way, so failing the write is strictly better than a wire violation.

[robertsLando]

[Major] - Operability

When a connection's outbound alias table fills (aliases.size >= max), the broker silently reverts to full-topic PUBLISHes for that client. No event, log, or counter marks it.

Why: this codebase already has the convention for exactly this case. sessionLimitReached exists because, per its own doc (docs/Aedes.md:256), "without it the guard would degrade behavior silently"; rejectPacketTooLarge (lib/client.js:399) emits clientError explicitly as telemetry before acting. Outbound-alias exhaustion breaks that convention — an operator cannot tell that a client's table filled and the connection quietly lost its bandwidth optimization, nor size outboundTopicAliasMaximum for the workload.

Fix: emit a one-shot signal the first time a connection's table fills, latched the way _rejectingPacketTooLarge is.

[robertsLando]

[Major] - Design / API

"Opt-in ... default 0 = off" is not accurate.

lib/write.js:46-58 strips a caller-supplied properties.topicAlias from every v5 non-empty-topic PUBLISH, and lib/handlers/connect.js:346-348 deletes topicAlias from every Will — both regardless of the option. test/mqtt5.js:596 asserts this default-on behaviour, so it is intended, just undocumented.

Why: the back-compat claim doesn't hold. An embedder who never sets outboundTopicAliasMaximum and today calls client.publish({ topic: 'x', properties: { topicAlias: 7 } }) silently loses the alias on upgrade.

Fix: document the always-on scrub — outbound topicAlias is broker-owned, and a caller- or Will-supplied value is dropped — and reword "off" to "no aliases assigned".

[robertsLando]

[Major] - DRY / Codebase fit

_outboundTopicAliases is the only per-connection collection in the codebase allocated outside a constructor (grep "new Map()" gives lib/client.js:39, aedes.js:136/140/144, and this line).

Its direct sibling _inboundTopicAliases is allocated once at lib/client.js:39 and merely .clear()d at teardown. The outbound one is declared null at lib/client.js:45, allocated here, and nulled at lib/client.js:440-441.

Why: the resulting "max > 0 implies the map is a Map" invariant is hand-maintained across four sites with long explanatory comments (lib/client.js:41-45, lib/client.js:430-441, here, and lib/write.js:49-59) — a shape the repo uses nowhere else. Any future write path that reads the map without the !max guard breaks it.

Fix: allocate unconditionally in the Client constructor like the inbound sibling, make _outboundTopicAliasMaximum the sole gate, and make teardown .clear(). The three invariant comments then collapse.

[robertsLando]

[Minor] - Correctness

The EXPECTED_PASSES gate only iterates protocols present in reports. A protocol that never ran — --protocols v3, or a report that failed to materialize — has its expected-pass entries checked zero times, and the script exits 0.

Why: this is precisely the silent-green failure this block's own comment claims to close ("would leave regressions empty and the job green"). The absent check added last round catches a vanished test, not a vanished protocol.

Fix: diff EXPECTED_PASSES.keys() against the protocols actually reported, and fail on any that is missing.

[robertsLando]

[Minor] - Operability

The regression gate does fail the job (sys.exit(1)), but the reason only reaches a plain print(..., file=sys.stderr) — never a ::error annotation, unlike the adjacent xpass check which does get one. The regressed row in report.md and the sticky PR comment render as an indistinguishable failure marker, with nothing separating a hard-gated regression from an ordinary WIP gap.

Why: an operator seeing the red check has to dig through raw logs to find REGRESSION (expected-pass gate): .... The two artifacts people actually look at give no clue why it is red.

Fix: emit a ::error annotation for regressions, mirroring the xpass block, and mark regressed EXPECTED_PASSES rows distinctly in render_markdown.

[robertsLando]

[Minor] - Design / API

Neither this declaration nor docs/Aedes.md:56 states that aedes.js:101-103 silently coerces a non-integer or negative value to 0 (disabled) and clamps the top to 65535.

Why: outboundTopicAliasMaximum: '64' arriving from env or JSON config silently disables the feature with no signal. The repo's own convention is to state clamping inline — types/instance.d.ts:87 says "clamped to [1, 100]".

Fix: add the coerce-to-0 and 65535-ceiling sentence to both the type comment and the doc.

[robertsLando]

[Nit] - Correctness

delete client._will.properties.topicAlias uses delete, while both sibling scrubs (lib/handlers/publish.js:180, lib/write.js:29) deliberately assign undefined to avoid the dictionary-mode transition on a reference-shared properties object.

Behaviourally identical here; inconsistent with the rule the other two sites state explicitly.

[BenjaminDobler]

Round-8 addressed — 0a62365 (the load-bearing three) + 2e487ae (the rest)

Thanks Robert — the two mutation-proven test holes were the important catch. Split across two commits: the top-3 risks first, then the majors/minors/nit.

The two tests that didn't catch their own contract

• QoS 1 poison test (test/mqtt5.js:416). You were right on both maskings. After the caller-alias scrub fix (below), the wire can't reveal a properties-poison at all: an aliasing-ON reconnect overwrites topicAlias with a fresh assignment, and an aliasing-OFF reconnect scrubs it. So the decisive assertion now reads the un-acked packet straight out of persistence.outgoingStream() and checks its properties.topicAlias is undefined and topic intact — where no resend path can launder it. Then it also reconnects advertising a max (per your steer) to pin the realistic resume. Mutation-verified: mutating withOutboundTopicAlias to write properties.topicAlias in place now fails the stored-packet assertion.
• Reset-on-reconnect test (test/mqtt5.js:181). Replaced the tautology (a full disconnect→reconnect always builds a fresh Client, so the reset lines were never exercised) with a genuine session takeover: same clientId, clean:false, overlapping, waiting until broker.clients[id] is the new Client object. That drives the resume path where alias state could leak if it were ever stored on the session rather than the Client.

The caller-alias escape (write.js:55) — [Major]

callerAlias required typeof topic === 'string', so a Buffer topic and an empty-string topic both fell through to return packet with the caller's topicAlias intact — an alias the client never negotiated ([MQTT-3.1.2-27]). Widened the scrub to every shape (packet.properties?.topicAlias !== undefined, no topic-type gate). Scrubbing a Buffer/non-empty topic yields a valid full-topic packet; the degenerate empty-topic form is malformed either way, and dropping the unnegotiated alias beats emitting it.

Batch 2 (2e487ae)

• [Major] Alias-table exhaustion was silent (write.js:67). Added a one-shot outboundTopicAliasExhausted (client, { max }) broker event, emitted the first time a new topic can't be aliased because the per-connection table is full — matching the sessionLimitReached / rejectPacketTooLarge telemetry convention you cited. Latched per connection (fires once, not per PUBLISH), typed, tsd-checked, documented, and tested (fires exactly once; a cached-alias re-publish doesn't re-trip).
• [Major] Map allocated outside a constructor (connect.js:327). _outboundTopicAliases is now allocated once in the Client constructor and .clear()d at teardown, exactly like _inboundTopicAliases — dropping the hand-maintained "max > 0 ⇒ Map" invariant. write()'s hot path always reads a Map now; _outboundTopicAliasMaximum stays the on/off switch (0 ⇒ short-circuit before the map is touched, so an empty map on a non-aliasing client is inert).
• [Major] "opt-in, default 0 = off" inaccuracy (Aedes.md:237). Documented that the option gates alias assignment only; the caller-alias scrub and Will-alias scrub run regardless (the broker owns the outbound namespace). Added the clarification to both the callout and the outboundTopicAliasMaximum option entry.
• [Minor] Undocumented coercion (types/instance.d.ts:93 + Aedes.md). Added the coerce-non-integer/negative→0 and clamp→65535 sentence to both alias-maximum options, in both the .d.ts comments and the docs — matching the repo's inline-clamp convention (maxTopicLevels says "clamped to [1, 100]").
• [Minor] Compat gate blind spots (run_compat.py:378/388). The expected-pass gate now also fails on a wholly-absent gated protocol (a --protocols subset or a run_protocol crash that drops a protocol's report entirely — its gates were checked zero times), and emits a ::error annotation for regressions so a hard-gated regression is distinguishable from an ordinary WIP gap in the Actions UI, symmetric with the unexpected-pass step.
• [Nit] Will scrub (connect.js:344). delete → = undefined, matching the two sibling scrubs (publish.js:180, write.js:29) and avoiding the dictionary-mode deopt on the reference-shared properties object.

Deferred (with reason)

• Byte-bounded alias table. The table is still bounded by entry count (min(client max, outboundTopicAliasMaximum)), not bytes. Each entry is one topic string, so worst-case memory is entries × longest-topic-length — already bounded, opt-in, and now observable via the exhaustion event. A byte cap would need a second accounting dimension on the hot write path; I'd rather land it deliberately (with a benchmark) than fold it into this round. Flagging for your call on whether it blocks.

Full suite + lint + tsd green; codecov patch & project green; every changed line covered.