JAVA-5949 preserve connection pool on backpressure errors when establishing connections by nhachicha · Pull Request #1900 · mongodb/mongo-java-driver

nhachicha · 2026-02-25T13:45:19Z

Original PR #1854 accidentally closed, and had no outstanding review comments.

~~This current PR depends on [JAVA-6033] ServerHeartbeatSucceededEvent is not fired for initial POLL monitoring #1856:~~
Update: This PR no longer depends on [JAVA-6033] ServerHeartbeatSucceededEvent is not fired for initial POLL monitoring #1856. Spec PR DRIVERS-3367: Fix racy backpressure-network-* tests specifications#1880 rewrote the backpressure-network-*-fail.yml tests to wait on serverDescriptionChangedEvent instead of serverHeartbeatSucceededEvent.

~~we should merge the latter into main, then merge this PR in backpressure, and leave the necessary TODO comments in #1918 related to the dependency on #1856.~~

then, when there are no outstanding PRs for the backpressure branch, rebase backpressure on top of main to include the changes from the main (see Merge main into backpressure #1917 (comment) for more context on this process).
then address the aforementioned TODO comments.

Specification changes:

DRIVERS-3218 Avoid clearing the connection pool when the server connection rate limiter triggers (#1855)
DRIVERS-3218: update SDAM error handling tests to ignore handshake failures (#1860)
DRIVERS-3218 Clarify interaction between TLS errors and backpressure (#1879) - the change has not yet been addressed in this PR, because it was introduced after creation of the PR.
DRIVERS-3218 clarify label handling (#1892) - the change has not yet been addressed in this PR, because it was introduced after creation of the PR.
DRIVERS-3367: Fix racy backpressure-network-* tests (#1880) - just a unified spec tests (JSON) update; the change has not yet been addressed in this PR, because it was introduced after creation of the PR.
DRIVERS-1785 Add test that PoolClearedEvent is emitted before ConnectionCheckedInEvent/ConnectionCheckOutFailedEvent

Note that there was a change in the description of one of the tests that belong to this PR, but I closed the corresponding JAVA-6095 (see this comment): DRIVERS-3394: adjust spec test description (#1894).

JAVA-5949, JAVA-6056, JAVA-5664

codeowners-service-app · 2026-02-25T16:59:25Z

Assigned stIncMale for team dbx-java because vbabanin is out of office.

stIncMale · 2026-03-04T22:39:28Z

Added the following to the description of the PR (a new piece of work to be done within the PR): DRIVERS-3394: adjust spec test description (#1894)(JAVA-6095).

vbabanin · 2026-03-10T02:39:54Z

+            boolean terminated = executor.awaitTermination(20, SECONDS);
+            assertTrue("Executor did not terminate within timeout", terminated);
+
+            // Assert at least 10 ConnectionCheckOutFailedEvents occurred


Do we need this comment? The assertion message already explains the intent (e.g., “Expected at least 10 ConnectionCheckOutFailedEvents, but got …”).

vbabanin · 2026-03-10T02:40:49Z

+            assertTrue("Expected at least 10 ConnectionCheckOutFailedEvents, but got " + connectionCheckOutFailedEventCount.get(),
+                    connectionCheckOutFailedEventCount.get() >= 10);
+
+            // Assert 0 PoolClearedEvents occurred


The same as in: https://github.com/mongodb/mongo-java-driver/pull/1900/changes#r2908976631

vbabanin · 2026-03-10T02:43:05Z

+            // Teardown: sleep 1 second and reset rate limiter
+            Thread.sleep(1000);
+            adminDatabase.runCommand(new Document("setParameter", 1)
+                    .append("ingressConnectionEstablishmentRateLimiterEnabled", false));


This cleanup is currently conditional on the code above completing successfully. If an assertion or exception happens earlier, this teardown won’t run, which can leak state into subsequent tests.

We should move it this cleanup into @AfterEach/afterEach so it runs reliably regardless of how the test exits.

It's wrapped under a finally block now

vbabanin · 2026-03-10T03:22:21Z

+        AtomicInteger connectionCheckOutFailedEventCount = new AtomicInteger(0);
+        AtomicInteger poolClearedEventCount = new AtomicInteger(0);
+
+        ConnectionPoolListener connectionPoolListener = new ConnectionPoolListener() {
+            @Override
+            public void connectionCheckOutFailed(final ConnectionCheckOutFailedEvent event) {
+                connectionCheckOutFailedEventCount.incrementAndGet();
+            }
+
+            @Override
+            public void connectionPoolCleared(final ConnectionPoolClearedEvent event) {
+                poolClearedEventCount.incrementAndGet();
+            }
+        };


Instead of introducing a new anonymous listener with counters, we can reuse the existing test listener:

TestConnectionPoolListener connectionPoolListener = new TestConnectionPoolListener();

It already provides await helpers that double as assertions and helpers to assert that zero PoolClearedEvents happened, e.g.:

connectionPoolListener.waitForEvent(ConnectionPoolClearedEvent.class, 1, 110, SECONDS);

This keeps the test more concise and reuses established utilities for clarity/consistency.

vbabanin · 2026-03-10T04:24:21Z

+            // clear the pool as they're not related to overload.
+            // TLS configuration errors (certificate validation, protocol mismatches) should also clear the pool
+            // as they indicate configuration issues, not server overload.
+            if (beforeHandshake && !sdamIssue.relatedToAuth() && !sdamIssue.relatedToTlsConfigurationError()) {


Currently we attach SystemOverloadedError and RetryableError labels in the SDAM error-handling path (effectively only for DefaultServer). In load-balanced mode, SDAM isn’t involved: the LB code path invalidates the pool directly (e.g., connectionPool.invalidate(serviceId, generation)), so the labeling logic is bypassed.

This means users running the driver in LB mode (behind an NLB) can still hit network errors, TLS handshake failures, timeouts during connection establishment or hello, but won’t get the labels.

However, these labels are a CMAP requirement, not SDAM. The CMAP spec states: “The pool MUST add the error labels SystemOverloadedError and RetryableError to network errors or network timeouts it encounters during the connection establishment or the hello message.”

Since this is defined as a pool behavior (topology-agnostic), it seems we should implement the labeling in the connection pool layer so it applies consistently in both default and load-balanced modes.

vbabanin · 2026-03-10T04:49:55Z

+            // clear the pool as they're not related to overload.
+            // TLS configuration errors (certificate validation, protocol mismatches) should also clear the pool
+            // as they indicate configuration issues, not server overload.
+            if (beforeHandshake && !sdamIssue.relatedToAuth() && !sdamIssue.relatedToTlsConfigurationError()) {


Currently we don’t distinguish DNS lookup failures (UnknownHostException) from other connection-establishment network errors. As a result, a DNS failure goes through the generic path (same as connection reset/timeout) and gets SystemOverloadedError/RetryableError labels.

The CMAP spec excludes DNS failures from backpressure labeling: `“For errors that the driver can distinguish as never occurring due to server overload, such as DNS lookup failures […] the driver MUST NOT add backpressure error labels for these error types.”.

Proposed change: detect DNS failure by walking the exception cause chain for UnknownHostException (it’s wrapped as MongoSocketException from ServerAddressHelper.getSocketAddresses()), and when present, skipping backpressure label attachment so SDAM follows the normal path (clear the pool and mark the server Unknown).

In that case, we should add coverage to assert that labeling and pool clearing behaviour. If the driver ever changes the wrapper exception type MongoSocketException (or stops wrapping UnknownHostException this way) and starts adding labels the test should fail.

Copilot

Pull request overview

This PR updates SDAM/backpressure handling so that connection-establishment failures caused by server-side backpressure do not clear the connection pool, and adds/updates tests and matchers to align with the SDAM spec test expectations.

Changes:

Adjust SDAM exception handling to avoid invalidating the pool on certain pre-handshake network failures, and add TLS-configuration-error classification.
Add new error label constants (SystemOverloadedError, RetryableError) and apply them for backpressure-related failures.
Extend unified event matching for additional server types and add a prose test for connection pool backpressure behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
driver-sync/src/test/functional/com/mongodb/client/unified/EventMatcher.java	Extends server type matching for SDAM serverDescriptionChangedEvent expectations.
driver-sync/src/test/functional/com/mongodb/client/ServerDiscoveryAndMonitoringProseTests.java	Adds a prose test intended to validate connection pool backpressure behavior via server rate limiter parameters.
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy	Updates unit tests to expect pool preservation on network errors during connection establishment (sync/async).
driver-core/src/main/com/mongodb/internal/connection/SdamServerDescriptionManager.java	Adds TLS configuration error detection helper on SDAM issues.
driver-core/src/main/com/mongodb/internal/connection/DefaultSdamServerDescriptionManager.java	Changes SDAM exception handling to preserve pool/server description for certain pre-handshake failures and apply labels.
driver-core/src/main/com/mongodb/MongoException.java	Introduces new public error label constants used by the backpressure handling path.

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

stIncMale · 2026-03-28T00:20:50Z

+    /**
+     * An error label indicating that the server is overloaded.
+     *
+     * @see #hasErrorLabel(String)
+     * @since 5.7
+     */
+    public static final String SYSTEM_OVERLOADED_ERROR_LABEL = "SystemOverloadedError";
+
+    /**
+     * An error label indicating that the operation is safely retryable.
+     *
+     * @see #hasErrorLabel(String)
+     * @since 5.7
+     */
+    public static final String RETRYABLE_ERROR_LABEL = "RetryableError";
+


I introduced these constants in #1918. There is more to this than the change do in the current PR. Let's revert the change in the current PR to resolve the conflict with backpressure.

stIncMale · 2026-03-28T00:26:09Z

+            // Backpressure spec: Don't clear pool or mark server unknown for connection establishment failures
+            // (network errors or timeouts during handshake). Authentication errors after handshake should still
+            // clear the pool as they're not related to overload.
+            // TLS configuration errors (certificate validation, protocol mismatches) should also clear the pool
+            // as they indicate configuration issues, not server overload.


We don't usually copy the specification prose into code as comments. I suspect, this comment was left by the AI.

Let's remove this comment, unless there is a good reason to leave it, which was not applicable to the rest of the code in this file that existed before the current PR.

stIncMale · 2026-03-28T00:26:19Z

+                // Don't update server description to Unknown
+                // Don't invalidate the connection pool
+                // Apply error labels for backpressure


We don't usually copy the specification prose into code as comments. I suspect, this comment was left by the AI.

Let's remove this comment, unless there is a good reason to leave it, which was not applicable to the rest of the code in this file that existed before the current PR.

stIncMale · 2026-03-28T00:30:49Z

[this comment is left on a file, but has nothing to do with the file; this is done so that we could reply to the comment; commenting on a PR does not allow replies - horrendous GitHub functionality]

The current PR seemingly depends on #1856 (see the description of the current PR). We need to decide what to do with that.

P.S. I originally left my thoughts in the description of this PR, but I don't know if that's what we are going to do.

We no longer depend on this PR see #1900 (comment)

stIncMale · 2026-03-28T00:44:38Z

[this comment is left on a file, but has nothing to do with the file; this is done so that we could reply to the comment; commenting on a PR does not allow replies - horrendous GitHub functionality]

@nhachicha

Could you please confirm that all the specification changes listed in the "Specification changes" part of the description of the current PR have been addressed in the PR?

If they have been addressed, let's update the description correspondingly.

Description updated. All specs are implemented

stIncMale · 2026-03-28T00:59:02Z

[this comment is left on a file, but has nothing to do with the file; this is done so that we could reply to the comment; commenting on a PR does not allow replies - horrendous GitHub functionality]

JAVA-5949 has an old comment, which instructs to re-enable some tests that were previously disabled when we updated the testing/resources/specifications submodule in main. Those tests were not enabled. Let's enable them and make sure they pass.

This PR no longer depends on #1856. Spec PR mongodb/specifications#1880 rewrote the backpressure-network-*-fail.yml tests to wait on serverDescriptionChangedEvent instead of serverHeartbeatSucceededEvent.

…establishing connections

- Fixing static check analysis

…handshake network timeout

- Fixed https://jira.mongodb.org/browse/JAVA-5664 (https://jira.mongodb.org/browse/DRIVERS-1785)

…r PR

nhachicha self-assigned this Feb 25, 2026

nhachicha requested a review from vbabanin February 25, 2026 16:56

nhachicha marked this pull request as ready for review February 25, 2026 16:56

nhachicha requested a review from a team as a code owner February 25, 2026 16:56

codeowners-service-app Bot requested a review from stIncMale February 25, 2026 16:59

stIncMale mentioned this pull request Feb 25, 2026

[JAVA-6033] ServerHeartbeatSucceededEvent is not fired for initial POLL monitoring #1856

Open

katcharov removed the request for review from stIncMale March 3, 2026 16:34

vbabanin requested changes Mar 10, 2026

View reviewed changes

katcharov requested a review from Copilot March 20, 2026 22:58

Copilot started reviewing on behalf of katcharov March 20, 2026 22:59 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

stIncMale mentioned this pull request Mar 23, 2026

JAVA-5949 preserve connection pool on backpressure errors when establishing connections #1854

Closed

stIncMale requested changes Mar 28, 2026

View reviewed changes

nhachicha and others added 10 commits April 20, 2026 17:47

Fixes JAVA-5949 prevent connection churn on backpressure errors when …

21abfe2

…establishing connections

Remove handshake and update submodule including new tests

339297b

Update spec test; fix test runner

2e44072

Add prose test

4cff1b2

Increasing the timeout termination

0a93dd7

- Revert spec timeout

b1f4ba0

- Fixing static check analysis

Update exception checks

1b1558d

Increase timeout for operations to complete

fcdfa23

Simplifying conditions check

d423093

Apply backpressure labels in CMAP layer; align SDAM with spec

bce97d4

nhachicha force-pushed the nh/backpressure/preserve_connection_pool branch from 0be0250 to bce97d4 Compare April 22, 2026 17:16

nhachicha added 4 commits April 23, 2026 11:06

Align Prose Test with Spec

41b2773

Test runner needs to add the SystemOverloadedError label for the pre-…

508d741

…handshake network timeout

- Re-enable JAVA-5949-skipped SDAM/backpressure tests

c5e68fc

- Fixed https://jira.mongodb.org/browse/JAVA-5664 (https://jira.mongodb.org/browse/DRIVERS-1785)

Deferred fixes to https://jira.mongodb.org/browse/JAVA-5664 to anothe…

71c91f0

…r PR

nhachicha requested a review from vbabanin April 23, 2026 16:59

Add DNS-lookup regression test for backpressure pool invalidation

6afa1a8

Conversation

nhachicha commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeowners-service-app Bot commented Feb 25, 2026

Uh oh!

stIncMale commented Mar 4, 2026

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

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!

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!

stIncMale Mar 28, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nhachicha commented Feb 25, 2026 •

edited

Loading

stIncMale Mar 28, 2026 •

edited

Loading