feat(relay): per-variant wire-message size caps (closes #233)#366
Merged
Conversation
Owner
Author
|
choose a reasonable default for the max size associated method and use that |
Define per-variant size caps on `WireMessage` and enforce them in `unpack_wire` as defense-in-depth on top of the transport-level `MAX_DESER_SIZE` (256 KB) cap. Caps: - Event / SyncBatch / Worker: 256 KB (full envelope budget for body- carrying variants). - ProfileAnnounce: 64 KB (default — display_name unbounded today). - TopicAnnounce: 16 KB (sized to MAX_TOPICS short strings). - SyncRequest, TypingIndicator, VoiceJoin/Leave/Signal, JoinRequest/Response/Denied: 4 KB (signaling / control plane only carries ids and short strings). Enforcement is centralized in `unpack_wire`: any decoded message whose re-serialized size exceeds its variant cap is dropped with `tracing::warn!`. This means relay (`unpack_wire` in `topic_announce_listener`), worker (`parse_worker_message`, `parse_server_message`), client, and any future caller all benefit without per-site changes. Tradeoff: post-decode rather than pre-decode. A pre-decode cap would need to read the bincode variant tag before allocating, which would mean plumbing the variant discriminant out of `unpack_envelope`. Post-decode is simpler, still bounded above by `MAX_DESER_SIZE`, and makes the per-variant cap a defense-in-depth signal+drop rather than the primary allocation guard. Acceptable. Closes #233. https://claude.ai/code/session_01RWdbBYuYhstPWaKYCaYAK4
intendednull
changed the title
Owner
Author
|
Done. Picked these defaults:
Enforcement is centralized in Two new tests cover the cap ordering and the oversize-rejection path. PR is out of draft. Generated by Claude Code |
The 16 KB ANNOUNCE_CAP introduced in the previous commit was sized against an imagined "hundreds of topics" upper bound, but the relay's actual limits are MAX_TOPICS = 10_000 with MAX_TOPIC_LEN = 256. Even moderately-sized legitimate TopicAnnounce messages from a relay running near MAX_TOPICS already exceed 16 KB, and the existing `topic_announce_listener_enforces_max_topics_cap` test (which broadcasts MAX_TOPICS + 1 short topics in a single announce) was being silently dropped by the per-variant cap before the relay's loop could enforce MAX_TOPICS — the real source of truth. Fix: collapse TopicAnnounce into the body-carrying variant group with the full MAX_DESER_SIZE budget. The relay's per-topic validation (`topic_str_is_valid` + the MAX_TOPICS counter) does the real work for this variant; the per-variant cap was just fighting legitimate traffic. Also drop the now-unused `ANNOUNCE_CAP` constant and update `per_variant_caps_are_sized_appropriately` to assert the new ordering (signaling < profile < event) plus the explicit equality between TopicAnnounce and the event-cap. https://claude.ai/code/session_01RWdbBYuYhstPWaKYCaYAK4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defines per-variant size caps on
WireMessageand enforces them inunpack_wireas defense-in-depth on top of the transport-levelMAX_DESER_SIZE(256 KB) cap.Chosen defaults
Event,SyncBatch,Worker— user-generated bodies + batched payloadsProfileAnnounce—display_nameunbounded todayTopicAnnounce— sized toMAX_TOPICSshort stringsSyncRequest,TypingIndicator,VoiceJoin,VoiceLeave,VoiceSignal,JoinRequest,JoinResponse,JoinDenied— ids + short strings onlyWhere enforced
Centralized in
unpack_wire(crates/common/src/wire.rs). Every decode site benefits with zero per-site plumbing:crates/relay/src/lib.rs—topic_announce_listenercrates/worker/src/actors/network.rs—parse_worker_message,parse_server_messagecrates/worker/src/actors/heartbeat.rswillow-common)Behavior on oversize
The decoded message is dropped and a
tracing::warn!is emitted withvariant,size,cap, andsigner.unpack_wirereturnsNone, which every existing call site already treats as "ignore this message".Tradeoffs
Post-decode rather than pre-decode. A pre-decode cap would need to peek at the bincode variant tag before allocating, which means plumbing the variant discriminant out of
unpack_envelope. Post-decode is simpler, still bounded above byMAX_DESER_SIZE, and makes the per-variant cap a defense-in-depth signal+drop rather than the primary allocation guard. Runner-up rejected because the wiring cost exceeded the marginal allocation savings (factor of 4-64x bounded vs. unbounded by 256 KB).Tests
per_variant_caps_are_sized_appropriately— sanity-checks the ordering of the cap tiers.oversize_signaling_message_is_rejected— packs an 8 KBTypingIndicator(withinMAX_DESER_SIZE, over the 4 KB signaling cap) and confirmsunpack_wirerejects it.willow-commontests still pass.cargo check -p willow-common -p willow-relay -p willow-workeris clean.Closes #233.
Generated by Claude Code