Skip to content

[BUG](log): Cap the log client's encode size at the log server's decode limit - #7666

Merged
dbeglord merged 1 commit into
mainfrom
dbld/log-client-encode-limit
Sep 1, 2026
Merged

[BUG](log): Cap the log client's encode size at the log server's decode limit#7666
dbeglord merged 1 commit into
mainfrom
dbld/log-client-encode-limit

Conversation

@dbeglord

@dbeglord dbeglord commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What this fixes

When a client writes records to Chroma, the frontend does not write them to storage itself. It batches the records into a single gRPC message and forwards that message to a separate log service, which durably appends it. A whole conditional commit travels as one message, so a large write is one large message rather than many small ones.

Both ends of a gRPC hop set their own size ceilings. The receiver sets a decode limit — the largest message it will accept off the wire. The sender sets an encode limit — the largest message it will put on the wire. These are independent settings, and the sender's is unlimited unless you set it.

The log service sets a decode limit of 32 MB (LogServerConfig::default_max_decoding_message_size in rust/log-service/src/lib.rs, applied to the server in LogServerWrapper::run). Nothing in the repo overrides it.

The frontend's log client set only the decode limit, and left the encode limit unset (rust/log/src/grpc_log.rs, building ClientOptions from rust/memberlist/src/client_manager.rs). Its send ceiling was therefore unlimited.

What that causes

An oversized write leaves the frontend looking healthy and dies on arrival. The frontend serializes the batch, opens a stream, sends 35 MB, and the log service rejects it during decode. The error the client gets back names a decode failure on a gRPC stream, not the write that caused it — there is no collection id, no record count, no size in it. Diagnosing one means correlating the failure back to a request by hand.

The gap is reachable in production. The frontend accepts HTTP request bodies up to 40 MB (default_max_payload_size_bytes in rust/frontend/src/config.rs), so a write between roughly 33 and 40 MB clears the HTTP layer and then fails at the internal hop.

The fix

GrpcLogConfig, the log client's configuration, already carried a max_encoding_message_size field defaulting to 32 MB — the same number the log service decodes at. The field was simply never passed to the client. This wires it through:

ClientOptions::new(Some(my_config.max_decoding_message_size))
    .with_max_encoding_message_size(Some(my_config.max_encoding_message_size)),

Now the frontend refuses the write before sending it, and tonic's local error names the actual byte count and the limit. No new constant is introduced, and the value stays configurable through the same config key it always had.

Why match the server rather than raise it

Raising the log service's decode limit would move the failure, not remove it. Some ceiling always exists, and whatever it is, a client that will send past it produces the same undiagnosable far-side failure. Matching makes the client refuse locally, where the request that caused it is still in hand.

32 MB is also the log service's real capacity decision — it bounds a single buffered append. Raising it to admit larger writes is a capacity change to argue on its own merits, not a fix for an error-reporting problem.

Other gRPC clients

I checked every ClientOptions construction in the repo:

  • Frontend to query/compaction services (rust/frontend/src/executor/distributed.rs) — sets both limits. This is the pattern the log client now follows.
  • Heap service client (rust/s3heap-service/src/client/grpc.rs) — has the same asymmetry, and its config declares an unused max_encoding_message_size default of 32 MB, exactly like the log client did. Left alone: the heap service has no server implementation in this repo yet and the client defaults to disabled, so there is no decode limit to match against. It is worth fixing when that server lands.
  • Garbage collector's Tilt log helper (rust/garbage_collector/src/helper.rs) — same asymmetry, but it is local integration-test scaffolding, not a production path.

Testing

Added log_client_encode_limit_fits_server_decode_limit to the log service's config tests. It asserts the log client's default encode ceiling is at or below the log service's effective decode ceiling, computed the same way the server computes it. The two numbers live in different crates, and this is what keeps them from drifting apart again.

cargo check, cargo clippy -- -D warnings, and cargo fmt --check pass for chroma-log and chroma-log-service. The new test passes. I did not exercise the oversized-write path against a running log service.

…de limit

The frontend's log client set only max_decoding_message_size, leaving tonic's
send limit unlimited, so it would encode a PushLogsRequest larger than the log
service is willing to decode. GrpcLogConfig already carried a
max_encoding_message_size field defaulting to the log service's 32 MB decode
limit; it was never passed to the client. Wire it through so the frontend
rejects an oversized batch locally, with the size named in the error, instead
of failing at decode on the far side.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@rescrv rescrv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description is longer than the code patch.

@dbeglord
dbeglord enabled auto-merge (squash) September 1, 2026 00:15
@blacksmith-sh

blacksmith-sh Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Found 1 test failure on Blacksmith runners:

Failure

Test View Logs
worker/
work_queue::tests::integration::tests::test_k8s_integration_work_queue_fifo_and_filteri
ng
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@dbeglord
dbeglord merged commit 3090b91 into main Sep 1, 2026
175 of 177 checks passed
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