Skip to content

server: make the element-memory decode budget configurable - #235

Merged
iainmcgin merged 1 commit into
mainfrom
iain/decode-limits
Jul 20, 2026
Merged

server: make the element-memory decode budget configurable#235
iainmcgin merged 1 commit into
mainfrom
iain/decode-limits

Conversation

@iainmcgin

Copy link
Copy Markdown
Collaborator

Summary

buffa 0.9 bounds the memory a single decode may commit to repeated, map, string and bytes elements, defaulting to 32 MiB. It is charged on element footprint rather than contents, which is what makes it catch the amplification a size limit cannot: a few bytes on the wire can ask the decoder to materialize a very large number of small elements, each with its own allocation. A single large payload is unaffected however big it grows.

Since #233 that budget has reached every received request as a constant. A peer that legitimately sends many small elements — and that 0.8 accepted — is rejected, and there is nothing an operator can do about it. This makes it configuration.

ConnectRpcService::new(router)
    .with_limits(Limits::default().element_memory_limit(128 * 1024 * 1024))

Limits::unlimited() lifts it along with the other two.

Both decode paths, not just the obvious one

The value travels to the decoder on RequestContext for the view paths and on Payload for the owned-message ones.

The second half is the part worth reviewing. Generated dispatch is view-based throughout, so threading only decode_borrowed_request_view and decode_message_request_stream looks complete and passes every test. But Router::route_unary and the *_handler_fn registrations decode through Payload::take_message and decode_request, and those would have kept buffa's default silently — on exactly the handlers where a fully materialized owned message costs most. Payload carries the limits so its public take_message signature is unchanged.

The rejection says what to do about it

invalid_argument: failed to decode proto request: element memory limit exceeded; if this peer is trusted, raise Limits::element_memory_limit

Exceeding the budget is the one decode failure a server operator can fix without the peer changing anything, so it names the limit. Every other DecodeError keeps the bare message — pointing at a limit there would send someone chasing a setting that cannot help. A test pins both directions.

Breaking, for generated code only

decode_borrowed_request_view and decode_message_request_stream take the decode limits. Both are #[doc(hidden)] and called only from generated dispatch, so regenerating is the whole migration — which 0.9.0 already requires for buffa 0.9. Doing it now costs nothing; doing it later costs a second forced regeneration.

Limits also becomes #[non_exhaustive], so the next limit is not another break. Struct-literal construction becomes Limits::default().max_request_body_size(a).max_message_size(b); fields stay public for reads.

Incidental: the generated-code check was missing two directories

CI diffed four generated directories, CONTRIBUTING listed five, and there are six. connectrpc-health/src/generated and connectrpc-reflection/src/generated were unverified — and this change regenerates both, so the gap was live rather than theoretical. The check now covers all six and the doc is corrected.

Not in scope

Clients still decode responses under buffa's defaults, with no override. The server is where the budget bites first, so this ships server-only; the Limits docs now say so rather than leaving the asymmetry to be discovered.

Testing

Clippy on the pinned 1.95 toolchain, cargo test -p connectrpc --no-default-features (419), 56 test suites, lint, docs, server conformance 3600/0, and task generate:all regenerated with the CI-pinned protoc 33.5 / buf 1.69.0 and verified idempotent by hashing every generated file across two runs.

Driven live over a socket against a generated service with a repeated field, two servers differing only in this limit: default returns HTTP 400 naming the knob, element_memory_limit(usize::MAX) returns HTTP 200. The unit tests assert both halves too — rejected at the default, accepted when raised — so wiring the limit to nothing cannot pass.

buffa 0.9 bounds the memory a single decode may commit to repeated, map,
string and bytes elements, defaulting to 32 MiB. The budget is charged on
element footprint rather than contents, so it catches the amplification a
size limit cannot: a few bytes on the wire asking the decoder to
materialize a very large number of small elements. A single large payload
is unaffected however big it grows.

That budget reached every received request as a constant. A peer that
legitimately sends many small elements, and that 0.8 accepted, is now
rejected with nothing an operator can do about it. Limits gains
element_memory_limit alongside the two size limits it already carries, and
the value travels to the decoder on RequestContext for the view paths and
on Payload for the owned-message ones.

Both were needed. The generated dispatch is view-based throughout, but
hand-registered owned-message handlers decode through Payload and
decode_request, and threading only the view path would have left the knob
silently inert on exactly the registrations where a fully materialized
message costs most.

A budget rejection now names the limit to raise. It is the one decode
failure a server operator can fix without the peer changing anything;
every other variant keeps the bare message, because naming a limit there
would send someone chasing a setting that cannot help.

Limits becomes non_exhaustive so the next limit is not another break, and
the CI generated-code check grows the two directories it was missing --
connectrpc-health and connectrpc-reflection were regenerated by this
change and neither was covered.

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
@iainmcgin
iainmcgin marked this pull request as ready for review July 20, 2026 22:11
@iainmcgin
iainmcgin enabled auto-merge July 20, 2026 22:11
@iainmcgin
iainmcgin added this pull request to the merge queue Jul 20, 2026
Merged via the queue into main with commit 8b3c3b0 Jul 20, 2026
14 checks passed
@iainmcgin
iainmcgin deleted the iain/decode-limits branch July 20, 2026 22:16
christopherwxyz pushed a commit to christopherwxyz/connect-rust that referenced this pull request Jul 25, 2026
…onnectrpc#239)

The connect-rust half of anthropics/buffa#331, now that buffa 0.9.1 has
shipped.

## What changed since the first version of this PR

buffa 0.9.1 did not just raise its own bound — it **exported the whole
mechanism**. So this no longer keeps a connect-side implementation at
all. The `descriptor_limits` module and its 16 GiB constant are gone,
replaced by delegation to `buffa_codegen::{decode_request,
tooling_decode_options, decode_failure}`.

That matters beyond tidiness: `buf generate` hands the identical
`CodeGeneratorRequest` to every plugin in a run, so a schema large
enough to need raising needs it for `protoc-gen-buffa` and
`protoc-gen-connect-rust` alike. One mechanism means one setting, rather
than a connect-specific twin the user has to discover and set
separately.

## The problem

buffa 0.9 charges its element-memory budget per element on **struct
size**, not on encoded bytes. Descriptor structs are wide, so the 32 MiB
default rejected descriptor sets from ordinary schemas of a few hundred
`.proto` files.

Measured here, one 400-file schema producing a 19,749,990-byte
descriptor set through `connectrpc_build::Config`:

| element-memory budget | result |
|---|---|
| 32 MiB (buffa 0.9.0's default) | `failed to decode FileDescriptorSet:
element memory limit exceeded` |
| 1 GiB (buffa 0.9.1's tooling budget) | `OK — 1600 files generated` |

## What this does

`protoc-gen-connect-rust` decodes through
`buffa_codegen::decode_request`; `connectrpc-build`, which has no plugin
parameter string, through `tooling_decode_options`. Both get the 1 GiB
tooling bound, and both overrides:

- `element_memory_limit=<bytes|unlimited>` as a plugin option, read by
scanning the wire for the parameter *before* the decode it governs, and
- `BUFFA_ELEMENT_MEMORY_LIMIT`, which covers every plugin in the run and
is the only route into a build script.

The bound stays finite rather than lifting entirely, so a truncated or
corrupt set still fails with an error instead of exhausting memory. The
guide recommends a byte count over `unlimited` for that reason.

## Two traps this exposed, both of which would have shipped

**Our own option parser would have rejected the option.** Plugin options
are parsed *after* the decode that consumed this one, so
`element_memory_limit=` reached the `unknown plugin option` arm and
failed the build — for precisely the one user who ever sets it, whose
schema was too large to decode without it. Proven by disabling the new
arm and watching `unknown plugin option:
"element_memory_limit=unlimited"` come back.

**Test fixtures sized by element count had silently stopped testing.**
buffa 0.9.1 took `ValueView` from 48 bytes to 32, so a hardcoded
`800_000` fell from 1.14x the budget to 0.76x. Two tests merged in connectrpc#235
flipped from proving a rejection to decoding successfully. Such fixtures
now derive their count from the live `size_of`, sized by the
**smallest** type any decode in the test materialises — sizing by the
larger leaves a view decode under budget, which I did at first, and the
`stream_message` test then passed with the behaviour it checks deleted
outright.

## Also in here

- `connectrpc-build` declares `rerun-if-env-changed` for the variable it
now reads. Without it, cargo does not re-run the build script when the
variable changes, and the setting looks inert.
- The over-budget failure from a build script corrects buffa's hint,
which offers a plugin option unreachable there, and points at this
repo's guide rather than buffa's.
- `ReflectionError::ElementBudget` reports an over-budget set at runtime
as a large schema rather than as corruption. The `From<DecodeError>`
conversion is written out instead of derived, so a future `?` on a new
decode path cannot silently route around the split. Runtime reflection
stays on the untrusted default deliberately — a reflection service may
be fed descriptors by a peer.
- The generated `descriptor_pool()` picks up buffa's self-scaling bound,
which is why the multiservice example regenerates. That closes the last
carry-forward item from #331 with no code of ours.

## Testing

Both overrides exercised in **both directions**. Raising proves little
on its own — the path could be ignoring the setting and succeeding
anyway. *Lowering* via the plugin option brings the failure back, which
is the only thing that proves the option is read pre-decode.

Four suites: server conformance 3600/0, client Connect 2580/0, client
gRPC-Web 2838/0. The client gRPC `Timeouts` cases fail 3–4 here, and
reproduce identically on `main` (1, 3 and 5 failures across three runs)
— that is connectrpc#210, fixed by connectrpc#238, not this change.

Reflection driven over a real socket against both descriptor sources. 56
test suites, clippy on the pinned 1.95 toolchain, `cargo test -p
connectrpc --no-default-features` (420), fmt on the pinned nightly,
rustdoc, and `task generate:all` idempotent.

## Note for the merge

`buffa = "0.9.1"` is a hard floor — `decode_request` and
`tooling_decode_options` do not exist in 0.9.0. This homespace's
registry mirror was still serving a stale index while I worked, so local
verification ran against a path override; CI resolves against real
crates.io, where buffa 0.9.1 published cleanly, so its dependency job is
the real check on that floor.

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
iainmcgin added a commit to EffortlessSteven/connect-rust that referenced this pull request Aug 21, 2026
…rpc#236)

## Summary

connectrpc#235, now merged, gave server operators control over the element-memory
budget buffa 0.9 applies to received *requests*. Clients had the same
wall on *responses* with no way through it: a server that legitimately
returns very many small elements fails the call at 32 MiB, and the
caller can do nothing about it.

```rust
// default for every call through this client
ClientConfig::new(uri).with_default_element_memory_limit(128 * 1024 * 1024)

// or per call
client.ingest_with_options(req, CallOptions::default().with_element_memory_limit(usize::MAX))
```

Same config-default plus per-call-override pair `max_message_size`
already uses, merged with the same precedence. Not a breaking change —
both types are already `#[non_exhaustive]`, and leaving the limit unset
keeps buffa's default.

## Every response path, both codecs

The value reaches Connect unary, gRPC unary, client-streaming, and
per-message server-streaming through `ServerStream`, which the bidi
receive half also goes through.

The JSON arm inlines `OwnedView::from_owned` so the decode half can take
the caller's limits. `from_owned` uses buffa's defaults, which made the
knob a silent no-op for JSON-codec clients.

**This adds no work.** `from_owned` is `try_encode_to_vec` followed by
`decode`; the inlined version is the same two steps with
`decode_with_options` in place of `decode`. The encode/decode round-trip
on the JSON path predates this change and is inherent to the view design
— a view borrows from proto bytes, and serde hands back an owned
message.

**It is a weaker defence on JSON, and the docs now say so** rather than
claiming parity. `serde_json` has already materialized the owned message
by the time the budget is consulted, so on JSON the budget bounds the
second materialization and makes the knob behave consistently; it does
not bound the parse. What it fixes is the knob being inert: before, a
JSON client hit the same 32 MiB wall and could not raise it.

The inlined encode uses `try_encode_to_vec`, not `encode_to_vec` — the
latter panics past 2 GiB, and on a client that size is chosen by the
peer.

The underlying gap — buffa's JSON path consulting none of the
`DecodeOptions` limits — is tracked upstream as anthropics/buffa#330.

No codegen change and no regeneration: `decode_response_view` is
private, and generated clients only touch `ClientConfig`, `CallOptions`
and `UnaryResponse`.

## `resource_exhausted`, not `internal`

An over-budget response is the same class of failure as the
`max_message_size` overflow a few lines above it, which already uses
`resource_exhausted`. Reporting a limit the caller set as an internal
error says "the library broke" and would route a caller who branches on
the code — raise and retry versus page someone — into the wrong branch.
The message names the setter that raises it:

```
failed to decode response: element memory limit exceeded; if this server is trusted,
raise CallOptions::with_element_memory_limit or ClientConfig::with_default_element_memory_limit
```

Only that one `DecodeError` variant gets the hint. A malformed response
keeps the bare message, because pointing at a limit would send someone
chasing a setting that cannot help; a test pins both directions.

## Docs

The `max_message_size` setters now point at the element budget. That
cross-reference is the one that matters: someone debugging an
element-budget error starts from the size knob they already know, and
without it they raise `max_message_size` and are confused when nothing
changes.

## Follows connectrpc#235

connectrpc#235 merged while this was in progress, so this is rebased onto `main`
and stands alone. The full gate was re-run on the new base rather than
inherited from the stacked one.

## Testing

Clippy on the pinned 1.95 toolchain, `cargo test -p connectrpc
--no-default-features` (423), 56 suites, lint, fmt, docs. Client
conformance: Connect 2580/0 and gRPC-Web 2838/0 clean.

The gRPC client suite shows flaky `Timeouts/...` failures — between 1
and 5 across runs. These are **pre-existing and reproduce on clean
`main`** (verified), and are connectrpc#210: post-deadline transport errors map to
`internal` instead of `deadline_exceeded`.

Driven live: one server returning 800,000 small elements, two clients
differing only in this budget. The default client fails and names the
knob; a per-call `with_element_memory_limit(usize::MAX)` receives all
800,000 items. The unit tests assert both directions too, including one
that drives an over-budget envelope through `ServerStream` — the
streaming path carries its own copy of the budget, so a construction
site that forgot to populate it would otherwise leave streamed messages
on buffa's default while every unary test stayed green.

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
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.

2 participants