DRIVERS-2884: CSOT avoid connection churn when operations timeout #1845

sanych-sun · 2025-09-26T00:03:23Z

This PR is based on the Preston's work in another PR that was closed by mistake: #1675

This PR implements the design for connection pooling improvements described in DRIVERS-2884, based on the CSOT (Client-Side Operation Timeout) spec. It addresses connection churn caused by network timeouts during operations, especially in environments with low client-side timeouts and high latency.

When a connection is checked out after a network timeout, the driver now attempts to resume and complete reading any pending server response (instead of closing and discarding the connection). This may require multiple connection to be attempted during the connection check out from the pool.
Each pending response draining is subject to a cumulative 3-second static timeout. The timeout is refreshed after each successful read, acknowledging that progress is being made. If no data is read and the timeout is exceeded, the connection is closed.

This update introduces new CMAP events and logging messages (PendingResponseStarted, PendingResponseSucceeded, PendingResponseFailed) to improve observability of this path.

Please complete the following before merging:

Update changelog.
Test changes in at least one language driver (CSharp Driver implementation PR).
Test these changes against all server versions and topologies (including standalone, replica set, and sharded
clusters).

codeowners-service-app · 2025-09-29T07:05:46Z

Assigned qingyang-hu for team dbx-spec-owners-csot because ShaneHarvey is out of office.

jmikola

I reviewed the unified test schema changes alone and those LGTM. I'll defer to CSOT spec folks for the other files.

baileympearson

@sanych-sun Have we implemented these changes in any driver? Which driver is supposed to be the second implementer? iirc it was originally Python

baileympearson · 2025-10-03T16:56:14Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

          close connection
          connection = Null
+        if connection is in "pending response" state:
+          drain the pending response


Do we have pseudocode for "drain the pending response" somewhere? I don't see it in this PR.

As we have events emission in the current pseudocode, not sure if we need to have pseudocode for "drain the pending response", as it basically "consume the bytes from underling stream/socket and ignore them".

I'd like the pseudocode, it doesn't seem terrible involved to add and it would be nice to codify the calculation of the timeout for the pending read in pseudocode:

read_timeout = timeoutMS set ? csotMin(timeoutMS, remaining static timeout) : waitQueueTimeoutMS set ? csotMin(waitQueueTimeoutMS, remaining static timeout) : remaining static timeout

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

baileympearson · 2025-10-03T17:07:56Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

+  /**
+   *  The driver-generated request ID of the operation that caused the pending response state.
+   */
+  requestId: int64;


Seems odd that the events emitted for an operation include a requestId that corresponds to the request that made the connection pending response. What is the value in this datapoint?

If we decide to keep it - do you think a more precise name would be beneficial? requestId is ambiguous - users could easily assume it refers to the request that is reading the pending response, not the operation that made the connection pending.

The idea of reporting the original requestId is to keep tracking how long/how many draining attempt were made before success or failure. Honestly I would prefer to have BOTH, current request Id and "original timed out requestId", but it could make sense only if other check out event had "current requestId" field, which is not there =(

This might depend on driver internals but in Python the new request ID will not be available at this point because the command has not been serialized (we need to checkout the connection first).

As for including the old requestId, I'm not sure how useful it is but it seems harmless to add. The driver needs to validate requestId == responseTo field on the server reply using the old requestId anyway to make sure the wire protocol isn't violated so that value will be available.

Thank you Shane! I suppose we want to keep the field. @baileympearson is this OK with you?

Yeah, fine to keep it - thoughts on workshopping the name to make it clearer?

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

source/client-side-operations-timeout/tests/pending-response.yml

baileympearson · 2025-10-03T17:37:04Z

source/connection-monitoring-and-pooling/tests/README.md

+    - `sendBytes`: We have 3 possible states here:
+        1. Message size was partially read: random value between 1 and 3 inclusive
+        2. Message size was read, body was not read at all: use 4
+        3. Message size was read, body read partially: random value between 5 and 100 inclusive


The insert is the first command on the connection, so where are the values for sendBytes coming from?

This is the parameter for proxy, which specifies how many bytes should be streamed, before sleeping.

The value of sendBytes is determined based on the state of the connection. But this is the first operation on the connection, so the connection will never have anything in the buffer and it isn't clear what the value of sendBytes should be.

Unless I misunderstand what sub-bullets 1,2, and 3 mean?

That 3 sub-bullets - it's different test cases for the same test scenario. This is parameter for proxy, that define how many bytes of the server's response should be streamed instantly, before delay.
I rephrased a little, hope it's more readable now.

baileympearson · 2025-10-03T17:39:34Z

source/connection-monitoring-and-pooling/tests/README.md

+This test verifies that if only part of a response was read before the timeout, the driver can drain the rest of the
+response and reuse the connection for the next operation.


This doesn't match the actual contents of the test, so either the description or the test needs to be updated.

But if its the description that is inaccurate - with sendAll: true and the events (only one pending read started + finished pair), the full response will be read in the first operation. Don't we have coverage for this scenario from our unified tests?

We are using proxy server here to control when/how server response arrived to the client side. So in step 2 in addition to the regular insert command payload, we have to add additional proxyTest property, which instruct proxy to emulate timeout. It works as following:

it send request to the server

based on the sendBytes parameter it stream requested amount of bytes instantly

it sleeps for delayMS

it streams rest of the response.

I think it makes sense to add this explanation to the test summary.

Okay, maybe my confusion stems from the interaction between sendBytes and sendAll. Does sendAll being enabled not mean that the proxy will forward the full response back to the client?

Yes. I've updated steps with sample of payload, it might help to understand the idea.

Here is the example of proxyTest parameter:

proxyTest: { actions: [ { sendBytes : 2 }, { delayMs : 400 }, { sendAll : true }, ] }

Which can be read as:
Hey proxy, here are steps for you, do it one-by-one:
action 1: stream 2 bytes from the server response
action 2: wait for 400ms
action 3: stream rest of the response to the client.

source/connection-monitoring-and-pooling/tests/README.md

…-and-pooling.md Co-authored-by: Bailey Pearson <[email protected]>

Co-authored-by: Bailey Pearson <[email protected]>

matthewdale · 2025-10-07T04:48:48Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

+          tConnectionDrainingStarted = current instant (use a monotonic clock if possible)
+          emit PendingResponseStartedEvent and equivalent log message
+          drain the pending response
+          if error:


The mix of exception/error paradigms in this pseudocode block is a bit confusing. We should rewrite this new pseudocode to use try/catch like the existing code.

Rewritten, thank you!

ShaneHarvey · 2025-10-15T18:54:29Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

 some equivalent configuration, but this configuration will also require target frameworks higher than or equal to .net
 5.0. The advantage of using Background Thread to manage perished connections is that it will work regardless of
 environment setup.



Could we add a rationale question to cover the motivation for this feature? Like "Why introduce the draining pending responses?" where the answer is to reduce connection churn in cases when the configured maxTimeMS does not allow enough time for the driver to read the MaxTimeMSExpired error and cases where the server or network delays the response.

baileympearson

Nothing major from me! Still waiting on a second implementation.

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

baileympearson · 2025-10-15T19:12:57Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

          close connection
          connection = Null
+        if connection is in "pending response" state:
+          drain the pending response


I'd like the pseudocode, it doesn't seem terrible involved to add and it would be nice to codify the calculation of the timeout for the pending read in pseudocode:

read_timeout = timeoutMS set ? csotMin(timeoutMS, remaining static timeout) : waitQueueTimeoutMS set ? csotMin(waitQueueTimeoutMS, remaining static timeout) : remaining static timeout

baileympearson · 2025-10-15T19:20:31Z

source/client-side-operations-timeout/tests/pending-response-close-connection.yml

+          document: {_id: 3, x: 1}
+        expectError:
+          isTimeoutError: true
+      # Draining pending response should failure because of closed connection,


Suggested change

# Draining pending response should failure because of closed connection,

# Draining pending response should fail because of closed connection,

baileympearson · 2025-10-15T19:22:29Z

source/client-side-operations-timeout/tests/pending-response-close-connection.yml

+tests:
+    # If the connection is closed server-side while draining the response, the
+    # driver must retry with a different connection.
+  - description: "write op retries when connection closes server-side while draining response"


Suggested change

- description: "write op retries when connection closes server-side while draining response"

- description: "op retries when connection closes server-side while draining response"

No need to specify read/write operations now, right?

baileympearson · 2025-10-15T19:23:28Z

source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.md

+  /**
+   *  The driver-generated request ID of the operation that caused the pending response state.
+   */
+  requestId: int64;


Yeah, fine to keep it - thoughts on workshopping the name to make it clearer?

baileympearson · 2025-10-15T19:29:11Z

source/connection-monitoring-and-pooling/tests/README.md

+    - `ConnectionClosedEvent`
+3. Execute `ping` command to populate the connection pool.
+4. Send a command (e.g. an insert) with a 200 millisecond timeout and the following `proxyTest` actions:
+    - `sendBytes`: random value between 1 and 3 inclusive


Suggested change

- `sendBytes`: random value between 1 and 3 inclusive

- `sendBytes`: 2

No reason for random value here - suggest we just choose a number. I chose 2 because that matches your example payload below

baileympearson · 2025-10-15T19:30:11Z

source/connection-monitoring-and-pooling/tests/README.md

+    - `sendAll`: `true` Example of run command payload:
+    ```


This is really minor but GH seems to render the last bullet and the codeblock without any spacing.

Suggested change

- `sendAll`: `true` Example of run command payload:

```

- `sendAll`: `true` Example of run command payload:

```

sanych-sun requested review from a team as code owners September 26, 2025 00:03

sanych-sun requested review from alcaeus, jmikola and stIncMale and removed request for a team September 26, 2025 00:03

DRIVERS-2884: CSOT avoid connection churn when operations timeout

f3d26ba

sanych-sun force-pushed the DRIVERS-2884 branch from f973c84 to f3d26ba Compare September 26, 2025 00:12

pr

3e1f8e8

sanych-sun requested a review from ShaneHarvey September 26, 2025 00:39

Fix unified spec tests

250c123

sanych-sun requested review from baileympearson and matthewdale and removed request for stIncMale September 26, 2025 19:39

codeowners-service-app bot requested a review from qingyang-hu September 29, 2025 07:05

jmikola approved these changes Sep 30, 2025

View reviewed changes

baileympearson requested changes Oct 3, 2025

View reviewed changes

sanych-sun and others added 5 commits October 3, 2025 12:29

Update source/connection-monitoring-and-pooling/connection-monitoring…

1fcc099

…-and-pooling.md Co-authored-by: Bailey Pearson <[email protected]>

Update source/client-side-operations-timeout/tests/pending-response.yml

d97a995

Co-authored-by: Bailey Pearson <[email protected]>

PR

f442cce

Fix test

e44e04c

Fix test

fca4dc0

matthewdale reviewed Oct 7, 2025

View reviewed changes

sanych-sun added 3 commits October 7, 2025 10:16

pr

473503f

pr

8677cbc

Move closed connection tests into separate file.

d33c5c2

sanych-sun requested a review from matthewdale October 14, 2025 18:46

sanych-sun requested a review from baileympearson October 14, 2025 18:46

ShaneHarvey reviewed Oct 15, 2025

View reviewed changes

baileympearson requested changes Oct 15, 2025

View reviewed changes

		This test verifies that if only part of a response was read before the timeout, the driver can drain the rest of the
		response and reuse the connection for the next operation.

	# Draining pending response should failure because of closed connection,
	# Draining pending response should fail because of closed connection,

	- description: "write op retries when connection closes server-side while draining response"
	- description: "op retries when connection closes server-side while draining response"

	- `sendBytes`: random value between 1 and 3 inclusive
	- `sendBytes`: 2

DRIVERS-2884: CSOT avoid connection churn when operations timeout #1845

Are you sure you want to change the base?

DRIVERS-2884: CSOT avoid connection churn when operations timeout #1845

Uh oh!

Conversation

sanych-sun commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeowners-service-app bot commented Sep 29, 2025

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun Oct 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun commented Sep 26, 2025 •

edited

Loading

sanych-sun Oct 8, 2025 •

edited

Loading