Kaynağa Gözat

fix: drop the send-size cap the I1 channel fix accidentally introduced

createChannel() (added to fix I1, the wait-mode response failing over 4 MB)
set SetMaxSendMessageSize alongside the receive limit, sized from the same
server.max_upload_mb-derived number. gRPC's client-side send default is
unlimited, so that was a new cap that did not exist on this branch before -
and it broke a legal upload: a form file field is base64-encoded into the
gRPC request (4/3 the size of the file), so a file only had to exceed about
24 MB to breach a 32 MB send cap, well inside the documented 32 MB
max_upload_mb limit.

Reproduced before this fix: a 26 MiB upload in wait mode failed with
"Sent message larger than max (34953257 vs. 33554432)"; a 20 MiB upload on
the same workflow succeeded. Before the I1 commit, the 26 MiB upload
succeeded too, since gRPC's client send was unbounded and 34.9 MB was well
under the runner's 64 MB receive limit.

SetMaxSendMessageSize is removed. Nothing needs bounding on the send side of
this channel that is not already bounded elsewhere: server.max_upload_mb at
the HTTP layer for what an untrusted client can push into the webserver, and
the runner's own max_message_size_mb receive limit for what the runner will
accept. A send cap here would be a third limit, sized from a number that
does not account for base64 expansion, that only creates a way for a
documented-legal upload to fail. A comment at the call site and in
LoadBalancerConfig now explains why the send side is deliberately left at
gRPC's default.

docs/nodes.md's Upload size section is corrected to match: the channel is
not symmetric, only the receive side is bounded by max_upload_mb, and the
section now explains the base64 expansion so a reader can size these limits
against an actual file size.

Verified: the 26 MiB upload that failed before this commit now returns 200
again; the 6 MB wait-mode response from the I1 verification still returns
200. Full suite: 79 passed / 0 failed / 0 skipped.
fszontagh 1 ay önce
ebeveyn
işleme
c445f9bf48

+ 27 - 14
docs/nodes.md

@@ -358,29 +358,42 @@ failure rather than a silently dropped submission.
 
 ### Upload size
 
+An uploaded file is carried base64-encoded inside the gRPC message the
+webserver sends the runner, not as raw bytes - base64 is 4/3 the size of what
+it encodes, so a 24 MB file becomes roughly a 32 MB request. Keep that
+expansion in mind when sizing any of the limits below against an actual file
+size.
+
 Two limits apply on the way in, and a submission has to pass both:
 
 - `server.max_upload_mb` in `config/webserver.json` bounds every request body
   the webserver accepts, uploads included - 32 MB by default. Anything larger
-  is refused before the form handler ever sees it.
+  is refused before the form handler ever sees it. This is a limit on the
+  HTTP request body, before base64 expansion.
 - The runner's own gRPC message-size limit, `max_message_size_mb` in
-  `config/runner.json`, bounds the same body again on its way from the
-  webserver to the runner as part of the execution payload - 64 MB by default,
-  deliberately set above the upload limit so it never re-imposes a lower cap
-  than the one already enforced at the HTTP layer.
+  `config/runner.json`, bounds the *base64-encoded* body again on its way
+  from the webserver to the runner as part of the execution payload - 64 MB
+  by default, deliberately set above the upload limit (with room for the
+  base64 expansion) so it never re-imposes a lower cap than the one already
+  enforced at the HTTP layer.
 
 A field's own `maxSizeMb` (see Fields, above) can tighten either of these
 further but never raise them.
 
-The way back is a separate, symmetric limit: every gRPC channel the webserver
-opens to a runner - including the one a `wait`-mode form's execution result
-travels back over - is sized off `server.max_upload_mb` as well (both send and
-receive), not off the runner's `max_message_size_mb`. Before this was wired
-up, that channel used gRPC's own 4 MB default for what it would accept back,
-so a `wait`-mode form whose `respond-to-webhook` body exceeded 4 MB failed
-with "Received message larger than max" even though the request that produced
-it was well inside every limit above. A large response is bounded by
-`server.max_upload_mb` the same as a large request is.
+The way back is a separate limit, and it is deliberately not sized off either
+of the above: every gRPC channel the webserver opens to a runner - including
+the one a `wait`-mode form's execution result travels back over - bounds only
+what the webserver will *receive*, from `server.max_upload_mb`. Before this
+was wired up, that channel used gRPC's own 4 MB default for what it would
+accept back, so a `wait`-mode form whose `respond-to-webhook` body exceeded
+4 MB failed with "Received message larger than max". What the webserver
+*sends* to a runner over that same channel is left at gRPC's default
+(unlimited) on purpose: a send cap taken from `max_upload_mb` would be
+smaller than a base64-encoded upload that `max_upload_mb` itself allows (a
+26 MiB file base64-encodes to about 34.7 MB, over a 32 MB cap), and the two
+limits above already bound what the webserver will accept from a client and
+what the runner will accept from the webserver - a third cap on the way out
+would protect nothing and would only break a documented-legal upload.
 
 ### Password
 

+ 16 - 9
src/webserver/runners/load_balancer.cpp

@@ -29,16 +29,23 @@ LoadBalancer::LoadBalancer(RunnerRegistry& registry, const LoadBalancerConfig& c
 std::shared_ptr<::grpc::Channel> LoadBalancer::createChannel(const std::string& address) const {
     ::grpc::ChannelArguments args;
     const int max_bytes = config_.max_message_size_mb * 1024 * 1024;
-    // Both directions: the request can carry a large upload (form file
-    // fields, arbitrary POST bodies) and the response can carry a large
-    // result (a wait-mode form's respond-to-webhook body, a big node
-    // output). gRPC's default is 4 MB for the receive side and unlimited for
-    // send, but leaving send unset here would mean this webserver could ask
-    // a runner for something the runner's own receive limit then rejects -
-    // pinning both to the same number keeps this channel's promise
-    // symmetric.
+    // Only the receive side is bounded here, for what this webserver will
+    // accept back from a runner (a wait-mode form's respond-to-webhook body,
+    // a big node output) - gRPC's default there is 4 MB, which is what I1
+    // fixed. SetMaxSendMessageSize is deliberately NOT set. It defaults to
+    // unlimited, and that is correct: what is worth bounding on the way out
+    // is already bounded elsewhere - server.max_upload_mb at the HTTP layer
+    // for what an untrusted client can push into the webserver, and the
+    // runner's own max_message_size_mb receive limit for what it will accept
+    // - and a form upload is carried base64-encoded inside the gRPC message,
+    // roughly 4/3 the size of the file on disk. A send cap sized from
+    // max_message_size_mb does not account for that expansion, so it would
+    // reject an upload the documented max_upload_mb limit allows (a 26 MiB
+    // file base64-encodes to about 34.7 MB, comfortably under a 32 MB
+    // max_upload_mb but over a 32 MB send cap taken from the same number).
+    // Adding a send limit here would be a third cap that protects nothing -
+    // the two that already exist are the right ones to enforce.
     args.SetMaxReceiveMessageSize(max_bytes);
-    args.SetMaxSendMessageSize(max_bytes);
     return ::grpc::CreateCustomChannel(address, ::grpc::InsecureChannelCredentials(), args);
 }
 

+ 13 - 9
src/webserver/runners/load_balancer.hpp

@@ -28,17 +28,21 @@ struct LoadBalancerConfig {
     LoadBalancingStrategy strategy = LoadBalancingStrategy::LeastConnections;
     double busy_threshold = 0.8;  // Runner considered busy above this load
 
-    // Ceiling for gRPC messages exchanged with a runner over a channel this
-    // class creates - both directions, request and response. Sourced from
-    // server.max_upload_mb (see WebServerService::loadConfig): that is
-    // already the webserver's single stated ceiling for how big a request or
-    // its answer is allowed to be, so reusing it here keeps one size promise
-    // across the HTTP layer and the gRPC hop behind it, instead of a second
-    // knob that could drift out of step with the first. Every webserver ->
-    // runner channel goes through createChannel() precisely so this one
-    // number is what all of them enforce - see the form-response bug this
+    // Ceiling for what this webserver will accept back from a runner over a
+    // channel this class creates - the receive side only, not the send side.
+    // Sourced from server.max_upload_mb (see WebServerService::loadConfig):
+    // that is already the webserver's single stated ceiling for a request's
+    // size, so reusing it here keeps one number as the answer to "how big is
+    // too big" on this side of the gRPC hop, instead of a second knob that
+    // could drift out of step with the first. Every webserver -> runner
+    // channel goes through createChannel() precisely so this one number is
+    // what all of them enforce on receive - see the form-response bug this
     // was written to fix, where only one caller had been widened and its
     // neighbours quietly kept gRPC's 4 MB default.
+    //
+    // Deliberately NOT applied to the send side - see the comment in
+    // createChannel() for why a send cap sized from this number would break
+    // a base64-encoded upload that server.max_upload_mb itself allows.
     int max_message_size_mb = 32;
 };