GODRIVER-3858 message size exceeds limit of 48000000#2360
GODRIVER-3858 message size exceeds limit of 48000000#2360koenno wants to merge 2 commits intomongodb:release/2.5from
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect OP_MSG size accounting when appending batched documents, which could allow writes (e.g., large InsertMany) to exceed the server’s maxMessageSizeBytes (48,000,000).
Changes:
- Initialize the running size counter in
AppendBatchSequence/AppendBatchArrayusing the currentdstlength (so pre-written bytes are counted toward the limit). - Update unit tests to pass a larger
totalSizelimit to account for the newly-counted overhead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
x/mongo/driver/batches.go |
Fixes message-size calculation by counting bytes already written into dst before appending documents. |
x/mongo/driver/batches_test.go |
Adjusts test inputs for totalSize to reflect the updated size accounting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| totalMsgSize := len(batches.Documents[0]) + len(batches.Documents[1]) | ||
| var n int | ||
| var err error | ||
| n, got, err = batches.AppendBatchSequence(got, 2, len(batches.Documents[0])) | ||
| n, got, err = batches.AppendBatchSequence(got, 2, totalMsgSize) |
There was a problem hiding this comment.
totalMsgSize is not actually the total message size here (it excludes OP_MSG/section overhead and also includes the size of a document that isn’t expected to be appended). This makes the test’s intent fragile and tied to the current overhead/identifier length. Consider computing the size limit from the expected encoded bytes (e.g., build dst first and use len(dst) +/- 1) and/or rename the variable to reflect that it’s a size limit rather than an actual message size.
| totalMsgSize := len(batches.Documents[0]) + len(batches.Documents[1]) | ||
| var n int | ||
| var err error | ||
| n, got, err = batches.AppendBatchArray(got, 2, len(batches.Documents[0])) | ||
| n, got, err = batches.AppendBatchArray(got, 2, totalMsgSize) |
There was a problem hiding this comment.
Same as above: the totalMsgSize value here isn’t the actual message size limit (it omits BSON array element overhead) and includes bytes for an element that shouldn’t be appended. To keep the test aligned with the totalSize contract, consider deriving the limit from the expected encoded dst length (or renaming the variable to indicate it’s a limit).
🧪 Performance ResultsCommit SHA: 806fe4eThere were no significant changes to the performance to report for version 69df3ecae8cb260007130f1a. For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change ReportNo changes found! |
GODRIVER-3858
Summary
sizeshould not be 0 at very beginning because there are already some bytes written todsttherefore the size should be initialized with dst lengthBackground & Motivation
InsertMany large number of documents that exceeds maxMessageByte limit (48000000).
A bug in calculation of message byte size.