Skip to content

chore: apply the 4mb max limit on drpc protocol message size #17771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 13, 2025

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 12, 2025

Respect the 4mb max limit on proto messages

Copy link
Member Author

Emyrk commented May 12, 2025

@Emyrk Emyrk changed the title chore: adjustable drpc protocol message size limit chore: adjust drpc protocol message size limit May 12, 2025
@Emyrk Emyrk force-pushed the stevenmasley/increase_proto_limit branch 2 times, most recently from 14f3977 to 168fe3d Compare May 12, 2025 15:20
@Emyrk Emyrk marked this pull request as ready for review May 12, 2025 15:20
@Emyrk Emyrk requested a review from spikecurtis May 12, 2025 15:26
@Emyrk Emyrk changed the base branch from stevenmasley/larger_proto_limit to graphite-base/17771 May 12, 2025 15:31
@Emyrk Emyrk force-pushed the stevenmasley/increase_proto_limit branch from 168fe3d to 9ba91b6 Compare May 12, 2025 15:31
@Emyrk Emyrk force-pushed the graphite-base/17771 branch from 98815ee to 3783241 Compare May 12, 2025 15:31
@graphite-app graphite-app bot changed the base branch from graphite-base/17771 to main May 12, 2025 15:32
@Emyrk Emyrk force-pushed the stevenmasley/increase_proto_limit branch from 9ba91b6 to 073bedd Compare May 12, 2025 15:32
@Emyrk Emyrk force-pushed the stevenmasley/increase_proto_limit branch from 073bedd to c041459 Compare May 12, 2025 15:58
Copy link
Contributor

I'm not sure I follow the logic. Aren't modules fetched dynamically at import time, which happens after the (up to) 10MB upload? There are no constraints on how big the modules could be during the import build which downloads them from a registry.

@Emyrk
Copy link
Member Author

Emyrk commented May 12, 2025

I'm not sure I follow the logic. Aren't modules fetched dynamically at import time, which happens after the (up to) 10MB upload? There are no constraints on how big the modules could be during the import build which downloads them from a registry.

You are right, modules will have to have a different solution.

I thought a 10mb file could be uploaded as a template version, which would fail at the protobuf layer. Our cli prevents this though, at a 1mb limit. So I will remove the bump to 10mb.

We should still keep the config setting, as we check against 4mb limits here:

if protobuf.Size(protoJob) > drpcsdk.MaxMessageSize {
return nil, failJob(fmt.Sprintf("payload was too big: %d > %d", protobuf.Size(protoJob), drpcsdk.MaxMessageSize))
}

Edit: Reverted to 4mb, but still want the constant to set the settings

@Emyrk Emyrk changed the title chore: adjust drpc protocol message size limit chore: respect drpc protocol message size limit constant May 12, 2025
@Emyrk Emyrk changed the title chore: respect drpc protocol message size limit constant chore: apply the 4mb max limit on drpc protocol message size May 12, 2025
Co-authored-by: Spike Curtis <spike@coder.com>
@Emyrk Emyrk merged commit 64807e1 into main May 13, 2025
35 checks passed
@Emyrk Emyrk deleted the stevenmasley/increase_proto_limit branch May 13, 2025 16:24
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants