Skip to content

Comments

increase WebSocket attachment size limit from 2KB to 16KB#6141

Open
threepointone wants to merge 1 commit intomainfrom
max-attachment-size
Open

increase WebSocket attachment size limit from 2KB to 16KB#6141
threepointone wants to merge 1 commit intomainfrom
max-attachment-size

Conversation

@threepointone
Copy link

A simple change; as discussed, this kicks up websocket attachment max size from 2kb to 16kb. Couldn't find any existing tests or other references for this value, so sending up just this change.

@threepointone threepointone requested review from a team as code owners February 23, 2026 20:35
@codspeed-hq

This comment was marked as outdated.

@elithrar

This comment was marked as outdated.

@elithrar
Copy link
Contributor

/bonk review this

@ask-bonk
Copy link

ask-bonk bot commented Feb 23, 2026

Here's my review of this PR:

Review Summary

The change is straightforward: MAX_ATTACHMENT_SIZE goes from 1024 * 2 (2KB) to 1024 * 16 (16KB) at src/workerd/api/web-socket.h:413.

Code Correctness

The change is mechanically correct. The constant is only referenced in serializeAttachment() (src/workerd/api/web-socket.c++:745-746) where it gates the maximum size of a serialized V8 attachment blob. Increasing this limit has no downstream effects beyond allowing larger attachments to pass the size check.

CodSpeed Regression

The reported 11.31% regression on Encode_ASCII_32[TextEncoder] is unrelated noise. That benchmark tests TextEncoder on a 32-byte ASCII string -- it has absolutely no connection to WebSocket attachment size limits. This is a constant definition change, not a codepath change, so it cannot affect TextEncoder performance. Safe to dismiss.

Suggestions

  1. Missing space in error message (pre-existing bug, not introduced by this PR): At web-socket.c++:747-748, the error message reads:

    "A WebSocket 'attachment' cannot be larger than ", MAX_ATTACHMENT_SIZE, " bytes."
    "'attachment' was ", released.data.size(), " bytes."
    

    The " bytes." and "'attachment'" are concatenated without a newline or space, producing: ...16384 bytes.'attachment' was 20000 bytes. Consider fixing this while you're in the area (add a space or \n).

  2. Consider a test: The PR description mentions no existing tests for this limit. A simple test in api/tests/ that verifies serializeAttachment rejects payloads above 16KB and accepts ones at/below 16KB would be valuable, especially since this is a user-facing limit that could regress.

  3. Style nit: The existing SUGGESTED_MAX_MESSAGE_SIZE on line 410 uses 1u << 20 (bit-shift style). For consistency, MAX_ATTACHMENT_SIZE could be written as 1u << 14 instead of 1024 * 16, though this is purely cosmetic.

Verdict

Approve -- the change is minimal, correct, and well-scoped. The suggestions above are non-blocking.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants