CSHARP-5740: Remove duplicate read concern for operations in snapshot sessions by kyra-rk · Pull Request #1977 · mongodb/mongo-csharp-driver

kyra-rk · 2026-04-28T22:05:57Z

No description provided.

…est coverage

kyra-rk · 2026-04-28T22:06:16Z

+        public void GetReadConcernForSnapshotSession_should_return_expected_result(
+            bool isSnapshot,
+            int? snapshotTime,
+            string expectedResult)


I noticed there wasn't any coverage for GetReadConcernForSnapshotSession, so I added unit tests for it.

kyra-rk · 2026-04-28T22:10:58Z

Side question: The transaction path in CreateType0Section uses AddIfNotAlreadyAdded for readConcern, which silently drops the transaction's readConcern if the command already has one (e.g. via CreateCommand). Ideally shouldn't it use extraElements.Add() like the snapshot path, so the transaction's readConcern always wins? It doesn't hit any errors today because GetReadConcernForCommand returns null for IsInTransaction and there's a separate GetReadConcernForFirstCommandInTransaction, but a raw RunCommand with an explicit readConcern inside a transaction would get the wrong precedence. It's out of scope for this PR but I wanted to ask if this is something worth filing a ticket for?

Code links:

eg. of AddIfNotAlreadyAdded in CreateType0Section
implementation of GetReadConcernForFirstCommandInTransaction

Copilot

Pull request overview

This PR addresses duplicated readConcern fields being sent when executing operations in snapshot sessions, by preventing normal operation-level read concern rendering when session.IsSnapshot is true and adding/adjusting tests to validate the behavior.

Changes:

Update ReadConcernHelper.GetReadConcernForCommand to return null for snapshot sessions (avoids emitting an additional readConcern from operation/collection settings).
Add/extend unit + wire-protocol tests to ensure only a single readConcern is present and that snapshot sessions behave as expected.
Add an integration test covering a snapshot session against a collection configured with a non-default read concern.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/MongoDB.Driver.Tests/ReadConcernIntegrationTests.cs	Adds an integration test ensuring snapshot sessions don’t cause duplicated readConcern on `find`.
tests/MongoDB.Driver.Tests/Core/WireProtocol/CommandWriteProtocolTests.cs	Adds protocol-level assertions that commands contain exactly one `readConcern` in snapshot/transaction scenarios.
tests/MongoDB.Driver.Tests/Core/Operations/ReadConcernHelperTests.cs	Adds coverage for snapshot-session read concern helper behavior and expands connection/session test helpers.
src/MongoDB.Driver/Core/Operations/ReadConcernHelper.cs	Changes `GetReadConcernForCommand` to suppress operation readConcern when `session.IsSnapshot`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kyra-rk · 2026-04-29T15:41:09Z

+            result.Should().HaveCount(1);
+
+            var findEvent = eventCapturer.Events.OfType<CommandStartedEvent>().First();
+            findEvent.Command["readConcern"]["level"].AsString.Should().Be("snapshot");


Hmm this is a good point, I will revise

kyra-rk · 2026-04-29T15:42:39Z

+
+            var command = new BsonDocument
+            {
+                { "find", "testCollection" }


I will edit the name to be Execute_should_send_only_one_readConcern_for_snapshot_session_with_find_command

I actually agree with this comment, and I think that this code could pass even without your fix.
You need to add a read concern in the operation, otherwise there won't be a duplicate in any case.

Reading this more.... I'm not sure we're verifying anything here. The operation does not have a read concern, as we're not adding it (and that's what we want) and we're just verifying that a readConcern is added, but that's a different issue than the one in this ticket.
I think that the unit test you wrote, plus eventually the integration tests would be enough, and these two tests can be removed.

Agreed, will remove!

papafe · 2026-04-29T17:03:10Z

+namespace MongoDB.Driver.Tests
+{
+    [Trait("Category", "Integration")]
+    public class ReadConcernIntegrationTests : LoggableTestClass


I like the idea of this integration test but but sure if we should keep this name for the file/class or we should change it. Or if it makes sense to move this test in some other file, but none of the ones we have seems to be relevant.
This is more of an open question/comment.

Hmm yeah I honestly was struggling to find a good place for it. Maybe we can keep the name and move it under Core so it's next to tests/MongoDB.Driver.Tests/Core/ReadConcernTests.cs? I'm also not apposed to renaming it, but ReadConcern seemed more general than focusing on the Snapshot part

papafe · 2026-04-30T09:47:57Z

Side question: The transaction path in CreateType0Section uses AddIfNotAlreadyAdded for readConcern, which silently drops the transaction's readConcern if the command already has one (e.g. via CreateCommand). Ideally shouldn't it use extraElements.Add() like the snapshot path, so the transaction's readConcern always wins? It doesn't hit any errors today because GetReadConcernForCommand returns null for IsInTransaction and there's a separate GetReadConcernForFirstCommandInTransaction, but a raw RunCommand with an explicit readConcern inside a transaction would get the wrong precedence. It's out of scope for this PR but I wanted to ask if this is something worth filing a ticket for?

Code links:

eg. of AddIfNotAlreadyAdded in CreateType0Section

implementation of GetReadConcernForFirstCommandInTransaction

I think this is worth filing a ticket for. And if you got the time, creating a reproducible test that verifies this issue would be incredibly useful.

papafe · 2026-04-30T09:49:36Z

Overall this PR seems correct to me, added @sanych-sun as the reviewer to get another eye.

skip read concern at operation level if in snapshot session and add t…

097f781

…est coverage

kyra-rk added the bug Fixes issues or unintended behavior. label Apr 28, 2026

kyra-rk commented Apr 28, 2026

View reviewed changes

fix small race with drop collection and snapshot read

95524ed

kyra-rk requested a review from papafe April 29, 2026 14:48

kyra-rk marked this pull request as ready for review April 29, 2026 14:48

kyra-rk requested a review from a team as a code owner April 29, 2026 14:48

Copilot AI review requested due to automatic review settings April 29, 2026 14:48

Copilot started reviewing on behalf of kyra-rk April 29, 2026 14:49 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

kyra-rk marked this pull request as draft April 29, 2026 15:01

acc enforce write majority write concern

d70f074

papafe reviewed Apr 29, 2026

View reviewed changes

papafe requested changes Apr 29, 2026

View reviewed changes

remove tests and add check on readconcern count

5215ade

kyra-rk requested a review from papafe April 29, 2026 18:19

papafe requested a review from sanych-sun April 30, 2026 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-5740: Remove duplicate read concern for operations in snapshot sessions#1977

CSHARP-5740: Remove duplicate read concern for operations in snapshot sessions#1977
kyra-rk wants to merge 4 commits intomongodb:mainfrom
kyra-rk:csharp-5740

kyra-rk commented Apr 28, 2026 •

edited

Loading

Uh oh!

kyra-rk Apr 28, 2026

Uh oh!

kyra-rk commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

kyra-rk Apr 29, 2026

Uh oh!

kyra-rk Apr 29, 2026

Uh oh!

papafe Apr 29, 2026

Uh oh!

papafe Apr 29, 2026 •

edited

Loading

Uh oh!

kyra-rk Apr 29, 2026

Uh oh!

papafe Apr 29, 2026

Uh oh!

kyra-rk Apr 29, 2026

Uh oh!

papafe commented Apr 30, 2026

Uh oh!

papafe commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kyra-rk commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyra-rk Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

kyra-rk commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kyra-rk Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

kyra-rk Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

papafe Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

papafe Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyra-rk Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

papafe Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

kyra-rk Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

papafe commented Apr 30, 2026

Uh oh!

papafe commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kyra-rk commented Apr 28, 2026 •

edited

Loading

papafe Apr 29, 2026 •

edited

Loading