[#872] Bound the connect of the JDBC pool and report a connect it cannot make

[vharseko]

Problem

CachedConnection.getConnection() — the pool every JDBC-backend operation borrows from — established connections with no bound of its own, and gave up on none of them:

final Connection conNew = DriverManager.getConnection(connectionString);   // no Properties: nothing bounds this
conNew.setAutoCommit(false);
conNew.setTransactionIsolation(TRANSACTION_READ_COMMITTED);
return new CachedConnection(connectionString, conNew);
} catch (SQLException e) { // max_connection server error: try recursion for reuse connection
    return getConnection(connectionString, (waitTime == 0) ? 1 : waitTime * 2);
}

Two shapes of the same symptom — "the operation never returns":

• the database accepts the TCP connection and then says nothing (a moved VIP, a proxy at its connection limit, a host that lost its answer): the first call never comes back at all, and it is not interruptible, so a shutdown does not unstick the thread either;
• the connect fails fast (connection refused, a password that is not accepted, a database that does not exist, a driver that is not in lib/extensions — SQLException: No suitable driver found): every one of those was treated as "the server is at max_connections" and retried recursively, the wait doubling from 1 ms. One cycle of that sums to 2³¹ ms ≈ 24.9 days before the int overflows and the doubling starts over, with nothing logged on the way — indistinguishable from a hang from the outside.

isValid(0) on a pooled connection was unbounded as well: 0 means "no timeout" in the JDBC contract, so validating a connection whose socket is half-open blocks just as long.

Every backend operation borrows through this path — openTree() during a backend open (RootContainer.open() runs on the thread that starts the server) and during dsconfig create-backend-index on a running server, every search and modify, and the import. The only workaround was putting driver timeouts into the db-directory URL by hand, as the reporter of #529 had done — undocumented, per-deployment, and not available on the Oracle thin URL at all.

Change

One connect attempt is bounded, per dialect. The dialect is recognized by the prefix of the connection string — there is no connection yet to ask the driver class of, which is what JDBCStorage does elsewhere — and its properties are handed to DriverManager.getConnection(url, properties):

Dialect	Connect	Login
PostgreSQL	connectTimeout (s), loginTimeout (s)	socketTimeout (s)
MySQL	connectTimeout (ms)	socketTimeout (ms)
Oracle	oracle.net.CONNECT_TIMEOUT (ms)	oracle.jdbc.ReadTimeout (ms)
MS SQL Server	loginTimeout (s)	socketTimeout (ms)

Bounded through org.openidentityplatform.opendj.jdbc.connect.timeout (seconds, 30 by default, 0 for no bound), in the style of the …jdbc.ttl / …jdbc.fetchsize properties this backend already has. DriverManager.setLoginTimeout() is deliberately not used: it is JVM-global. A driver with a range of its own for its connect property is never handed a value beyond it — SQLServerDriverIntProperty.LOGIN_TIMEOUT is validated against [0, 65535], so a larger bound would not widen the connect, it would fail every one of them.

A property the administrator set keeps precedence — it is not passed at all, so the loginTimeout=30&socketTimeout=300 of #529 still governs. The connect side of a dialect is one budget rather than a set of independent knobs, so a bound of theirs under any of its names leaves all of them alone: on PostgreSQL connectTimeout bounds the socket connect and loginTimeout the login behind it, and filling in the one they left open caps the one they set — a ?connectTimeout=300 answered with a loginTimeout of ours is a login pgjdbc gives up on at 30 s, since Driver.connect branches into a thread of its own as soon as loginTimeout is anything but 0. The read bound is a budget of its own and is still set where they left it open. Recognizing it means allowing for the parameter syntax of each dialect: ?a=1&b=2 (PostgreSQL, MySQL), ;a=1;b=2 (SQL Server — whose driver gives a supplied property precedence over the URL, so this matters), and (CONNECT_TIMEOUT=…) inside an Oracle TNS descriptor, where the property also goes by the last segment of its name alone. It is recognized the way its own driver recognizes it: pgjdbc and Connector/J look their properties up by their exact name — PropertyKey.fromValue("SocketTimeout") answers null, and Connector/J then reads no bound out of the URL either — so a parameter of another case is a parameter of nobody and must not pass for a bound of the administrator, while the SQL Server driver (getNormalizedPropertyName) and the keywords of an Oracle descriptor match either way. The read bound of an Oracle descriptor goes by the two names ojdbc8 reads, oracle.jdbc.ReadTimeout and oracle.net.READ_TIMEOUT; RECV_TIMEOUT is a parameter of sqlnet.ora and of the listener, and the name appears in none of the driver's classes, so a descriptor carrying one is not a read bound of the connection and must not take ours off it.

The connection string is not the only channel of the administrator. Some properties of the Oracle driver are read out of the system properties as well — -Doracle.jdbc.ReadTimeout=30000 is how a whole JVM is bounded — and a property supplied to the driver outranks that one without a word, so those names are looked up there too. Which names those are is listed rather than told from the shape of them: GeneratedPhysicalConnection resolves oracle.jdbc.ReadTimeout and oracle.net.CONNECT_TIMEOUT in three tiers — the properties supplied to the driver, then System.getProperty, then the properties of the data source — while oracle.net.READ_TIMEOUT, a dotted name of the same driver and the one the socket option is finally read under, reaches the socket from the connection properties alone: the classes carrying that literal hand it to Properties.get, none of them to System.getProperty. Taken for a bound of the administrator, a -D of it would leave the login with no read bound whatever — theirs not read by the driver, ours not set because we believed theirs was in force. Measured against a listener that completes the handshake and then never speaks, with oracle.net.CONNECT_TIMEOUT pinned at 60 s so it cannot be what ends the wait: no bound anywhere blocks past 20 s, -Doracle.jdbc.ReadTimeout=2000 gives up at 2.5 s, and a Properties value of 2000 on top of -D 20000 takes the timing over — which is the bound that would then be lifted after the login as if it were ours, leaving a connection with no read bound where the administrator had set one. A property set to 0 is not a bound either: every one of these drivers reads 0 as "wait as long as it takes", so it counts as unset rather than as a setting to stay out of the way of, and one of ours goes on top of it — wherever a supplied property outranks the URL. On PostgreSQL it does not: Driver.connect copies what it was handed into a flat map and parseURL then writes the parameters of the URL on top of it, so a ?socketTimeout=0 there is the value the driver uses whatever this class supplies. Measured through the driver's own parser, ?connectTimeout=0&socketTimeout=0&loginTimeout=0 comes out 0/0/0 with a full set of ours in hand, Driver.timeout(props) at 0 and the login running unbounded on the borrowing thread. So on that dialect a parameter of the URL counts as the administrator's whatever its value, and a zero is reported — once per connection string, naming the parameter and the value — rather than written over in a map the driver goes on to ignore: a bound this class believes it has set is worse than one it knows it has not.

Not one of the four drivers bounds the attempt with a single property. The first covers the socket connect; the reads behind it — the prelogin handshake, TLS, authentication, which is exactly the phase left unanswered by a proxy at its limit — need the second. That includes the SQL Server driver, contrary to what its loginTimeout suggests: the test below caught it parked in SQLServerConnection.prelogin() for the full 600 s of the run despite loginTimeout=2, which is why socketTimeout is set there too. And it includes pgjdbc, whose loginTimeout is not a bound of the socket at all: ConnectionFactoryImpl.tryConnect puts an SO_TIMEOUT on the login socket only under if (socketTimeout > 0) — both before and after enableSSL — and socketTimeout defaults to 0. What loginTimeout bounds there is the caller, out of process: Driver.connect runs the login on a daemon thread of its own and Driver$ConnectThread.getResult gives up on the thread rather than on the login, leaving it parked in the read. Without a read bound a borrow against a peer that answers nothing returns on time and leaves that thread — and the ESTABLISHED socket it holds — behind, so socketTimeout is set on PostgreSQL as well, with loginTimeout kept on top of it for a URL naming more than one host, where each host costs a login of its own — the connect is one budget for all of them, taken from the single System.nanoTime() in front of the loop over the hosts. The JDBCStorage.Dialect table of #866 declares the same three for its stamp connection.

That second property is a socket read timeout for the life of the connection on every one of the four, so it is lifted with setNetworkTimeout(…, 0) once the login is through — left in place it would fail every statement slower than it, an import batch or the dbms_stats pass of #866 among them. The two setup round trips (setAutoCommit, setTransactionIsolation) still run under it. A read bound the connection string sets itself is never lifted, because it is never set here — that is what the three names above are for. A connection whose bound cannot be lifted at all serves the borrower waiting for it and is closed rather than pooled: in the pool it would carry that bound into every borrow it was handed to.

A database that takes no connection for the moment is retried, under a deadline. Two states qualify, and only these two:

• it is at its connection limit, and one of our own connections is on its way back to the pool: SQLState 53300 too_many_connections plus the vendor codes of the dialect — MySQL 1040/1203, Oracle ORA-00020 and 12516/12518/12519/12520, SQL Server 17809/10928/10929;
• it is not accepting connections yet, the state a database on its way up reports while it recovers: PostgreSQL 57P03 cannot_connect_now, Oracle ORA-01033/01034/01089, MySQL 1053, SQL Server 921/922/927 and Azure 40613. JDBCStorage.open() has no second attempt of its own, so without this a backend whose database restarted together with the server — Compose, systemd — would stay locked down until the next restart of the server. ORA-12514 is deliberately not among them: a listener that does not know the service is also what a service name of a typo answers, forever.

Both are recognized through the whole chain of the failure, the causes and getNextException() alike. The rest of the insufficient_resources class is not retried — 53100 disk_full is not cured by a connection of ours coming back — and neither is anything else: a password that is not accepted, a database that is down, a driver that is not on the classpath is reported to the caller.

The wait is bounded by org.openidentityplatform.opendj.jdbc.pool.timeout (seconds, 60 by default, 0 for no bound), with the backoff capped at 1 s and a warning — throttled, per connection string, since every operation borrows through here — so the stall reaches the server log. That deadline governs the whole borrow, as its name says: it bounds the connect attempt inside it (an attempt left to run out its own 30 s would overrun a 60 s borrow by half again) and it bounds the drain of the pool, where each connection costs a validation round trip and the pool has no upper bound on the number it holds (#878) — the connection in hand is always validated first, though, since a database at its connection limit has no source of connections other than the ones coming back to the pool. It bounds the attempt even where org.openidentityplatform.opendj.jdbc.connect.timeout is 0: turning the per-attempt bound off must not turn the bound of the whole borrow off with it, and both at 0 is what leaves a connect unbounded. It is not a bound to the millisecond, though: the connection in hand is validated whatever the deadline says, and an attempt is never given less than a second, so a borrow can return one validation and one attempt past it. What it hands a driver stays inside the range an int of milliseconds takes — mssql-jdbc rejects a socketTimeout past Integer.MAX_VALUE outright ("The socketTimeout 3000000000 is not valid"), which with connect.timeout=0 and a pool.timeout of more than 24.9 days would fail every connect of that backend with the name of a property nobody typed.

Nothing that leaves this class carries credentials. The logged connection string is stripped of them first, and neither delimiter is looked for in the whole string: on an Oracle URL the credentials stand between the subprotocol and the first @, which is the delimiter of the descriptor — a password holding an @ has to be quoted for the driver itself — and everywhere else they stand inside the authority, which ends at the path rather than at a ?. So a password holding the parameter separator of another dialect or of its own (scott/pa;ss@, scott/pa?ss@) is not cut in half and left in the log, while an @ inside a parameter value (?user=u@example.com) still leaves the host in the message. Every host of an authority is stripped of credentials of its own, since a URL of Connector/J gives each of them a set (//u:p@h1:3306,u2:p2@h2:3306, and jdbc:mysql:replication:// in front of them); a password=/pwd= left standing anywhere is blanked out, since the key-value host syntax of Connector/J holds one inside the authority itself (//address=(host=h)(user=u)(password=p), //(host=h,user=u,password=p)), where neither a userinfo nor a parameter stands; and a URL that none of this took apart is not logged past its subprotocol — an @ left standing where the credentials of an Oracle URL do not end is one this did not recognize, and the host of a stall report is worth less than a password in the server log. What is no credential stays: the token naming the kind of Oracle driver, since thin against oci is a first question of an Oracle connect, and the parameter naming the database, since two backends of one SQL Server host answer to the same URL up to their databaseName.

That covers a URL this class logs itself. A driver is free to put the connection string it was handed into the message of its own failure, and the JDK itself does — "No suitable driver found for " + url, which is what a driver jar left out of lib/extensions arrives as, the ordinary Oracle misconfiguration. That message travels: JDBCStorage.open() hands the failure to RootContainer, whose StorageRuntimeException(cause) makes the message of the cause its own, BackendImpl wraps that into ERR_OPEN_ENV_FAIL and BackendConfigManager logs it at ERROR and answers a config change with it. So the failure that leaves this class is redacted whole: the connection string replaced by its safe form, the credentials of it blanked wherever a driver quoted a part of them back, and every link of the chain rebuilt that way rather than wrapped — everything that prints a failure prints its causes along with it, so a cause left as it stands would carry the password past the wrapper. A failure naming nothing of the connection string is passed on as it is, with its own type.

A driver outside the four is reported. The properties bounding a connect are the properties of a driver, so a connection string this class knows no names for — an admin-added jdbc:mariadb:, a jdbc:h2: — leaves every attempt unbounded, and the deadline of the borrow cannot reach into a connect already under way, since the driver is the only thing holding the socket. Silently, that is #872 again for a backend nobody thinks of as unbounded, so it is named in the log once along with the prefixes that are bounded. And the timeout of a borrow carries 08001 now rather than no SQLState at all: it is the failure of a connect that did not happen, and monitoring reading the state off what it caught would otherwise see null where the driver's own exception carried one.

Three leaks along the way. A connect whose setup then failed dropped a live connection on the floor and retried — at full speed at first, so a repeated failure could exhaust the database's own connection limit and turn a transient problem into a permanent one; it is closed now, whether the setup failed with a SQLException or with an unchecked one out of the driver. And close(), whose rollback() failed, neither pooled the connection nor closed it; it closes the parent and reports the failure.

The validation of a pooled connection is bounded at the socket. isValid(n) is not that bound on every driver: SQLServerConnection.isValid(int) maps its argument onto setQueryTimeout and runs SELECT 1, an attention-based timeout that needs a working channel to fire — and the read bound of the login was lifted the moment the connection was established. So the socket carries the bound, for the length of the validation only, and a read bound of the connection string, being the tighter one, is left alone. A driver that reports the validation as an error discards the connection rather than failing the operation with it, and so does a connection whose bound cannot be put back afterwards. A driver that takes the call bounding the validation and then fails inside it is told apart from one that never takes a network timeout at all: it is free to have applied the bound before failing, so that connection is discarded rather than validated unbounded and handed out — pooled, it would carry five seconds of ours into every statement for the rest of its life. Nothing is put back on a connection that failed the validation: Connector/J answers that by aborting the connection and the SQL Server driver by terminating it, so the restore would fail too and warn about the statements of a connection that is being closed — over an idle connection the server merely reaped.

Tests

CachedConnectionTestCase — 41 tests, no database, ~23 s: all four JDBC drivers are compile-scope dependencies, so every dialect is exercised for real and a regression fails the build wherever it runs.

• testLoginIsBoundedWhenTheDatabaseNeverAnswers — a bound, never-accepted ServerSocket: the kernel completes the handshake, so the connect succeeds and every read of the login hangs. Each dialect is borrowed on a thread of its own, so a driver that does not honour its bound fails the test in seconds instead of hanging the run — which is how the SQL Server prelogin gap surfaced.
• testTheAttemptIsBoundedByTheDeadlineOfTheBorrow — the same socket with a 600 s connect bound and a 2 s pool timeout: the borrow is over in seconds, not in ten minutes.
• testRefusedConnectIsReportedAtOnce, testMissingDriverIsReportedAtOnce, testRejectedLoginIsNotRetried, testDiskFullIsNotRetried — the failures that used to be retried forever are reported, after exactly one attempt.
• testConnectionLimitIsRetried / testConnectionLimitGivesUpAtTheDeadline, testDatabaseOnItsWayUpIsRetried, testTheWholeChainOfTheFailureIsLookedAt — a driver of the test answering with 53300 and with 57P03 proves the two cases that still wait, that a wrapped failure is recognized, and that the wait gives up at its deadline rather than 25 days later.
• testConnectionIsClosedWhenItsSetupFails, testConnectionIsClosedWhenItsSetupFailsWithAnUncheckedError, testConnectionThatCannotBeRolledBackIsClosed, testBrokenPooledConnectionIsDiscarded — the three leaks and the discarded connection.
• testValidationOfAPooledConnectionIsBoundedAtTheSocket, testValidationLeavesTheBoundOfTheConnectionStringAlone, testDrainOfThePoolStopsAtTheDeadline — the bound around the validation, that a tighter one of the administrator is not widened by it, and that a pool of connections the database no longer answers is not drained past the deadline.
• testTheLoginThreadOfPostgresDoesNotOutliveTheBorrow — the daemon thread pgjdbc abandons at its loginTimeout: the borrow is over, and the thread has to be gone with it. Against a PostgreSQL row without socketTimeout it fails with the thread still alive after the whole window.
• testTheDeadlineBoundsAnAttemptTheConnectPropertyDoesNot — connect.timeout=0 with pool.timeout=2: the borrow is still bounded. Against the previous code this one hangs out the whole run, which is the shape of JDBC backend: the connection pool connects without any timeout and retries a failed connect forever #872 itself.
• testAPooledConnectionIsNotDiscardedUnvalidatedAtTheDeadline, testAConnectionOnItsWayOutIsNotGivenItsBoundBack, testAConnectionStillCarryingTheBoundOfItsLoginIsNotPooled — the connection the deadline used to close unvalidated, the bound that is not put back on a connection the server reaped, and the connection that must not go into the pool.
• testBothPhasesOfTheLoginAreBounded, testConnectBoundStaysInTheRangeTheDriverTakes, testConnectionStringKeepsPrecedence, testTheCaseOfAParameterIsTheOneOfItsDriver, testDialectIsRecognizedByTheConnectionString, testLoggedConnectionStringCarriesNoCredentials — the property table, the range the SQL Server driver takes, the precedence of the URL property by property and by case, and that a stall report carries no password: a password holding its own dialect's separator, one of every host of a failover or replication URL, one inside the key-value host syntax, and a URL this cannot take apart at all.
• testASystemPropertyOfTheAdministratorKeepsPrecedence, testAZeroIsNotABoundOfTheAdministrator, testTheBoundHandedToADriverStaysInTheRangeAnIntTakes, testAPooledConnectionWhoseValidationThrowsIsDiscarded — the bound of -Doracle.jdbc.ReadTimeout that is neither overridden nor lifted, the 0 that is no bound of anybody's, the value no driver would take, and the unchecked failure of a validation that would leave a connection dequeued and unclosed.
• testAReportedConnectCarriesNoCredentials, testAMessageOfADriverCarriesNoCredentials — the failure of a borrow driven through getConnection() with a password in its URL, asserted over every link of the chain and not only the message on top, and the redactor held to the shapes a driver quotes back: the whole URL, the credentials without it, the password alone, and the stall line built apart from the logging of it so that a test can hold it to the same rule.
• testASystemPropertyNoDriverReadsIsNoBound — a -Doracle.net.READ_TIMEOUT no driver reads must not take the read bound off a login, while the same name written into a descriptor still keeps precedence.
• testAConnectBudgetOfTheAdministratorIsNotCappedByThisClass, testAParameterOfAPostgresUrlOutranksTheBoundOfThisClass — both read the effective values back through Driver.parseURL of pgjdbc rather than off the map handed to it, which is the half of the story the bug lived in.
• testAUrlThisBackendCannotBoundIsReportedOnce, testAReadBoundTurnedOffInAPostgresUrlIsReportedOnce, testAPooledConnectionThatCannotBeBoundedForItsValidationIsDiscarded, testTheBoundOfAnAttemptStaysInRangeWithoutADeadline — the two things that must not happen silently, the connection whose validation could not be bounded, and the clamp of the range where the borrow has no deadline to take it from.

TestCase.testLoginBoundDoesNotOutliveTheLogin asserts on every real database that the read bound of the login is in force in the first place — established with the very properties the borrow uses and read back off the socket of that driver, all four at 2000 ms — and only then that it is gone by the time the connection is handed out. Against a relaxReadBound() that lifts nothing it fails with expected [0] but found [2000], where the assertion on its own passed either way.

Every one of the new tests was run against the code it fixes and fails there — each fix was put back one at a time, and the assertion that covers it failed:

the rethrow is not redacted            -> testAReportedConnectCarriesNoCredentials
the message of a driver is not         -> testAMessageOfADriverCarriesNoCredentials
connect properties filled one by one   -> testAConnectBudgetOfTheAdministratorIsNotCappedByThisClass
a postgres url does not outrank ours   -> testAParameterOfAPostgresUrlOutranksTheBoundOfThisClass
a dotted name is a system property     -> testASystemPropertyNoDriverReadsIsNoBound
one sentinel for two outcomes          -> testAPooledConnectionThatCannotBeBoundedForItsValidationIsDiscarded
no clamp without a deadline            -> testTheBoundOfAnAttemptStaysInRangeWithoutADeadline
the subprotocol and parameters dropped -> testLoggedConnectionStringCarriesNoCredentials
relaxReadBound() lifts nothing         -> TestCase.testLoginBoundDoesNotOutliveTheLogin (PgSql container)

All four container suites pass with no skips, re-run against this head: PgSql 54/54, MySql 54/54, Oracle 54/54, MsSql 54/54, plus the JDBC EncryptedTestCase 34/34, JDBCStorageRetryTest 26/26 and StampConnectionTestCase 5/5 — on top of the 41 that need no database.

Out of scope

Left for their own issues, so this one stays reviewable: #877 (no statement gets a setQueryTimeout, so a query that hangs after a successful connect still parks its worker — the validation of a pooled connection is bounded here, the statements of an operation are not), #878 (the pool has no upper bound on its size and its TTL sits on the URL key rather than on each connection, so a burst of connections survives as long as the backend sees any traffic at all) and #879 (a pooled connection is validated on every borrow, at the cost of a database round trip per operation).

#875 reports the same retry loop, filed separately while reviewing #867, so this closes both.

Merged up to master. #866 touches the same method — it added a conNew.close() to the old recursive getConnection — and the conflict is resolved in favour of this branch, which removes that method: the leak that patch closed is closed here by catch (SQLException | RuntimeException e) { closeQuietly(conNew); throw e; }, for an unchecked failure out of the driver as well.

Fixes #872
Fixes #875

[maximthomas]

Solid change that closes a real hang, and the no-database CachedConnectionTestCase is the right way to cover it. No blocker. Three majors below, all in opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java.

Transient "database is starting up" is now permanently fatal (major)

Only isConnectionLimit is retried; every other SQLException is rethrown:

if (!isConnectionLimit(e, dialect)) {
    throw e;
}

That includes PostgreSQL 57P03 cannot_connect_now ("the database system is starting up" / "is shutting down") and Oracle ORA-01033 — states that clear themselves in seconds. JDBCStorage.open() has no retry of its own, so the backend stays down until the server is restarted:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
public void open(AccessMode accessMode) throws Exception {
    try (final Connection con=getConnection()) {   // throws -> storageStatus is never set
        this.accessMode = accessMode;
        storageStatus = StorageStatus.working();
    }
}
private StorageStatus storageStatus = StorageStatus.lockedDown(LocalizableMessage.raw("closed"));

OpenDJ and PostgreSQL restarting together (compose, systemd) is enough: RootContainer.open() reaches the database a second before recovery finishes and the backend is locked down for good. The old loop rode this out. Please add 57P03 and the per-dialect equivalents to the retried set, under the same pool deadline.

The pool deadline is never applied to the drain (major)

poll() validates every pooled entry with no reference to deadline:

private static CachedConnection poll(String connectionString, long waitMs) throws InterruptedException {
    CachedConnection con = cached.get(connectionString).poll(waitMs, TimeUnit.MILLISECONDS);
    while (con != null) {
        if (isUsable(con)) {              // isValid(5) per entry, no deadline check
            return con;
        }
        closeQuietly(con.parent);
        con = cached.get(connectionString).poll();
    }
    return null;
}

deadline / POOL_TIMEOUT_PROPERTY is consulted only in the connection-limit catch of getConnection(). The pool is .build(conStr -> new LinkedBlockingQueue<>()) — no size cap (#878). After a failover that leaves pooled sockets half-open, one borrow costs 5 s x pooled connections, minutes, while the advertised 60 s bound never fires. Check deadline in the drain loop and stop validating once it has passed.

isValid(VALIDATION_TIMEOUT_SECONDS) is not a bound on SQL Server (major)

The comment claims the bound covers the half-open case:

// The validation needs a bound of its own: isValid(0) means "no timeout" in the JDBC
// contract, and a connection whose socket is half-open answers it no sooner than it
// answers anything else.
return con.isValid(VALIDATION_TIMEOUT_SECONDS);

It does not, for MICROSOFT. In mssql-jdbc-13.4.0.jre11, SQLServerConnection.isValid(int) maps its argument to SQLServerStatement.setQueryTimeout(...) and runs SELECT 1 — an attention-based query timeout that needs a working channel, not a socket read timeout. And relaxReadBound() has just cleared the only socket-level bound this connection had:

con.setNetworkTimeout(DIRECT_EXECUTOR, 0);   // MICROSOFT: readBoundOutlivesLogin == true

So a pooled SQL Server connection has SO_TIMEOUT 0 and validating it against a black-holed socket blocks forever — the exact failure the PR description opens with, surviving on the borrow-from-pool path. MySQL, pgjdbc and Oracle do honour isValid(n) at socket level, so this is one dialect out of four.

#877 already owns "a read timeout for an established connection (today unbounded by design)", which would close this. Either fix it here or correct the comment and the PR description and name it explicitly in #877 — right now both claim a bound that is not there.

Nits

• relaxReadBound() can clear a bound the administrator set: readBoundSet comes only from declaredInUrl(..., "oracle.jdbc.ReadTimeout") / readtimeout. An Oracle TNS descriptor spells the read bound (RECV_TIMEOUT=...) (older property oracle.net.READ_TIMEOUT), so we set ours on top of it and then setNetworkTimeout(con, 0) wipes theirs for the life of the connection — contrary to "nothing of the administrator's is lifted along with it". Same shape for (TRANSPORT_CONNECT_TIMEOUT=...) on the connect side.
• sqlState.startsWith("53") matches the whole insufficient_resources class: 53100 disk_full, 53200 out_of_memory and 53400 configuration_limit_exceeded are not fixed by a pooled connection coming back, yet a PostgreSQL server out of disk now makes every borrow spin the full 60 s, a fresh TCP connect every <= 1 s, before reporting. Also the chain is walked with getCause() only, not SQLException.getNextException().
• The leak connect() closes is only closed for SQLException: conNew.setAutoCommit(false) / setTransactionIsolation(...) can throw unchecked (drivers wrap internal failures), and the fresh connection is dropped on the floor exactly as before the fix. catch (SQLException | RuntimeException e) covers it.
• safeUrl() can leak the start of a password: it cuts at the first ?/; before looking for @, so jdbc:oracle:thin:scott/pa;ss@//h:1521/svc is cut inside the password and jdbc:oracle:thin:scott/pa is what reaches the stall warning and the SQLTimeoutException. Strip the userinfo first, then the parameters.
• readBoundWarned is one-shot per JVM: if setNetworkTimeout fails, the first connection warns and every later one silently carries a 30 s read timeout; an import batch then dies with nothing in the log pointing at org.openidentityplatform.opendj.jdbc.connect.timeout. Warn per occurrence (throttled, like warnStall) or discard the connection. Same file: lastStallWarning is a single static shared across all connection strings, so with two JDBC backends one starves the other's stall warnings.
• declaredInUrl is case-insensitive, the drivers are not: pgjdbc and Connector/J parse URL parameters case-sensitively, so ?ConnectTimeout=5 suppresses our bound while the driver ignores the user's.
• Dropped unit in the TTL warning: getNonNegativeProperty logs "using %d" where the old message was "using %d ms".
• opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java: CachedConnection.cached.invalidate(url) discards the queue without closing the connections in it.

Verified as correct, no action needed: the empty-Properties overload is equivalent to the 1-arg DriverManager.getConnection; URL precedence holds in both directions (Connector/J and mssql-jdbc let a supplied property win, pgjdbc lets the URL win, and declaredInUrl never sets a property the URL declares); DIRECT_EXECUTOR is safe with Connector/J's setNetworkTimeout; close() rethrowing a failed rollback() is not a regression; the overflow guards on deadline and timeoutSeconds * unitsPerSecond hold; there are no other callers of the removed getConnection(String,int); and **/*TestCase.java is in the failsafe includes, so CachedConnectionTestCase really does run in the default test job.

[vharseko]

Thanks — a good catch on each of the three, and on most of the nits. All of it is in e83d002, with the description updated to match. Point by point, including the one I did not take.

Transient "database is starting up" (major) — fixed

You are right about the consequence, and it is worse than a slow start: JDBCStorage.open() has no second attempt, RootContainer.open() wraps the failure and BackendImpl.openBackend() throws InitializationException, so nothing above it tries again. The backend is down until the server is restarted.

Retried now, alongside pool exhaustion and under the same pool.timeout: 57P03 plus the per-dialect equivalents — Oracle ORA-01033/01034/01089, MySQL 1053, SQL Server 921/922/927 and Azure 40613.

Two decisions inside that worth flagging:

• ORA-12514 is deliberately not in the set. It is what a listener answers while the instance has not registered yet — and equally what it answers for a service name of a typo, forever. Waiting out 60 s on a first-time misconfiguration seemed the worse trade against a window that ORA-01033/01034 already cover. One line to add if you disagree.
• Class 08 is not in the set either, since 08001 is also what a refused connect and No suitable driver arrive as, which this PR exists to report at once.

One caveat that stands: 60 s of pool.timeout does not cover a PostgreSQL crash recovery of minutes, so a long recovery still leaves the backend down. A longer deadline for the open path specifically, or a lazy re-open of a locked-down storage, would — but that is a change in JDBCStorage/BackendImpl rather than in the pool, so I left it out of this one.

The pool deadline is never applied to the drain (major) — fixed

poll() takes the deadline and stops validating past it, closing the entry it holds and letting a fresh connect answer instead.

While there: the deadline did not cover the connect attempt either, so a 60 s borrow could spend 90 s — the property already says it bounds "every connect attempt and every wait for a pooled connection", and now it does. One attempt gets whatever is left of the deadline, never below 1 s, since 0 means "no bound".

isValid(VALIDATION_TIMEOUT_SECONDS) is not a bound on SQL Server (major) — fixed

Confirmed in the bytecode of mssql-jdbc-13.4.0.jre11: isValid(int) builds a SQLServerStatement, calls setQueryTimeout(n) and runs SELECT 1; the timeout fires through TDSCommand.interrupt(), which sends an attention packet and calls Thread.interrupt() — neither of which unblocks a plain socket read on a channel the peer never answers.

So the socket now carries the bound of the validation rather than the driver: getNetworkTimeout(), setNetworkTimeout(5000), validate, put the previous value back. A read bound of the connection string is tighter than ours and is left untouched; a connection whose previous value cannot be restored is discarded rather than handed out carrying a 5 s bound into every statement. The comment says what it does now.

Nits

• relaxReadBound() clearing a bound of the administrator — right, and the comment claimed the opposite. The read bound of a dialect is a list of names now, so oracle.net.READ_TIMEOUT and a descriptor's RECV_TIMEOUT count as the administrator's: ours is not set on top of either, and nothing of theirs is lifted with ours. TRANSPORT_CONNECT_TIMEOUT is left as it was — we only add an outer bound there, nothing is wiped.
• startsWith("53") — narrowed to 53300 plus the vendor codes, and the walk now follows getNextException() as well as the causes (bounded at 32 links against a cycle).
• The leak closed only for SQLException — catch (SQLException | RuntimeException e), with a test that throws an unchecked failure out of setTransactionIsolation.
• safeUrl() leaking the start of a password — the order is reversed and the separator comes from the dialect (; separates parameters on SQL Server and nothing on Oracle), so jdbc:oracle:thin:scott/pa;ss@//h:1521/svc reaches the log as jdbc:oracle:@//h:1521/svc. As a side effect a ?user=u@example.com no longer costs the host in the message.
• readBoundWarned one-shot / lastStallWarning shared — the first is throttled at 10 s like the stall warning; the second is per connection string now.
• Case sensitivity — right for pgjdbc, and I checked the other three before narrowing it: Driver.parseURL has no toLowerCase at all and PGProperty looks up exact names, while Connector/J matches through PropertyKey.fromValue (equalsIgnoreCase for keys not marked case-sensitive) and mssql-jdbc through getNormalizedPropertyName (equalsIgnoreCase); an Oracle descriptor's keywords are case-insensitive too. So the exact match is applied to PostgreSQL alone.
• Dropped unit in the TTL warning — the helper takes the unit now, so the seconds-valued properties do not inherit an "ms".

The one I did not take

CachedConnection.cached.invalidate(url) discarding the queue without closing the connections in it — it does close them. Caffeine calls removalListener for every removal including an explicit invalidation (only evictionListener is restricted to evictions), and the value handed to it is the queue with its entries. Checked against the 3.2.3 we depend on rather than from the javadoc:

removalListener fired: key=jdbc:x cause=EXPLICIT wasEvicted=false entries=1

The one true caveat is that the notification is asynchronous — it runs on the common pool — so the close is not immediate. That is fine for what the test uses it for. Left as it is.

Also found while fixing the above

org.openidentityplatform.opendj.jdbc.connect.timeout was clamped only against Integer.MAX_VALUE / 1000, but SQLServerDriverIntProperty.LOGIN_TIMEOUT is validated against [0, 65535] — so an administrator raising the property to "effectively disable it" instead of setting 0 would have failed every SQL Server connect with R_invalidTimeOut. The connect property of a dialect now carries the range its driver takes.

Tests

CachedConnectionTestCase is at 23 (from 13), still without a database, ~17 s: the two retried states and the failures that are not, a wrapped failure recognized through its chain, the bound around the validation and a tighter one left alone, the drain stopping at the deadline, the attempt bounded by it, the unchecked leak, the SQL Server range, the case of a parameter, and the two new shapes of safeUrl. Green locally with -Pprecommit verify -Dit.test=CachedConnectionTestCase.

Note for the queue: #884 carries this commit as its base, so it needs a rebase on the updated branch.

[maximthomas]

All three majors from the previous round are genuinely fixed, and I re-checked the load-bearing claims in driver bytecode rather than on trust: SQLServerDriverIntProperty.LOGIN_TIMEOUT really is validated against [0, 65535] while SOCKET_TIMEOUT has no range, so the maxConnectSeconds clamp is right; SQLServerConnection.isValid(int) is new SQLServerStatement + setQueryTimeout(n) + SELECT 1 and never touches the socket, so the new outer setNetworkTimeout(5000) in boundValidation() is the bound it claims to be; ojdbc8 23.7 implements PhysicalConnection.setNetworkTimeout, so relaxReadBound() works on Oracle; and pgjdbc copies the supplied Properties into a flat map before parseURL, so URL precedence holds. The getNextException() walk, the 53300 narrowing, the RuntimeException leak fix and the per-URL stall throttle all check out.

One major left, on the dialect the PR opens with.

PostgreSQL login is bounded only in the caller, and leaks a thread and a socket per borrow (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:130

POSTGRES("jdbc:postgresql:", '?',
    "connectTimeout", 1, 0,
    new String[]{"loginTimeout"}, 1, false,   // <- read bound that does not outlive the login
    new int[]{}, new int[]{}),

pgjdbc never puts an SO_TIMEOUT on the login socket. PGStream.createSocket(int) passes connectTimeout to Socket.connect(addr, timeout) only, and ConnectionFactoryImpl.openConnectionImpl calls newStream.setNetworkTimeout(socketTimeout * 1000) only under if (socketTimeout > 0) (both before and after enableSSL). socketTimeout defaults to 0 and is not set here, so every read of the login is unbounded.

That leaves loginTimeout as the only bound, and it is enforced out of process: Driver$ConnectThread.getResult(timeout) sets abandoned = true, throws PSQLException("Connection attempt timed out.", 08001), and leaves the daemon thread parked in the unbounded read forever.

Against the failure this PR opens with - a VIP or proxy that completes the TCP handshake and never answers - each borrow returns after 30 s and leaks one daemon thread plus one ESTABLISHED socket. tcpKeepAlive is off by default, so nothing reaps them; at a few operations per second the server reaches unable to create new native thread and fd exhaustion within minutes, where the pre-PR code only parked the operation thread.

Fix: treat PostgreSQL like the other three - socketTimeout (seconds) as readProperties[0] with readBoundOutlivesLogin = true, keeping loginTimeout as the outer bound.

Neither new test can see this: testLoginBoundDoesNotOutliveTheLogin reads getNetworkTimeout() == 0 on postgres either way, and testLoginIsBoundedWhenTheDatabaseNeverAnswers only asserts that the caller returns.

poll() destroys a pooled connection it never validated (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:359

while (con != null) {
    if (System.currentTimeMillis() >= deadline) {
        closeQuietly(con.parent);   // <- unvalidated, possibly perfectly good
        return null;
    }

Under connection-limit pressure - the case the retry loop exists for - a connection returned to the pool right at the deadline is closed, and the borrow then fails with SQLTimeoutException anyway. Each timed-out borrow removes one live connection from the only source of connections there is. The deadline should stop the validation, not discard the entry: return con unvalidated, or put it back.

isUsable() restores the network timeout on a connection that just failed validation (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:386

if (restore >= 0 && !setNetworkTimeout(con.parent, restore)) {
    return false;
}
return usable;

On a connection the server has already reaped - MySQL wait_timeout, PG idle_session_timeout, any failover - Connector/J's isValid calls abortInternal() and the mssql driver calls terminate(), so the restore throws and emits the throttled WARN "The read bound of a JDBC connection could not be set to 0 ms ... statements taking longer than the org.openidentityplatform.opendj.jdbc.connect.timeout property may fail". That is a routine idle-reap, the message describes something else, and it points the operator at an unrelated property. Skip the restore when !usable.

connect() ignores the false from relaxReadBound() (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:423

if (readBoundSet) {
    relaxReadBound(conNew);   // return value dropped
}

isUsable() discards a pooled connection whose bound cannot be put back, but a freshly established one whose login bound cannot be lifted is still pooled and handed out carrying a 30 s SO_TIMEOUT for its whole life - exactly what the comment above relaxReadBound() says must not happen, so an import batch or the #866 statistics pass dies mid-statement (and on MySQL a socket read timeout aborts the connection outright). Either discard it or do not pool it.

safeUrl() still leaks a password holding the dialect's own separator (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:530

if (at >= 0 && (firstParameter < 0 || at < firstParameter)) {   // userinfo first

The previous round fixed the cross-dialect case (a ; in an Oracle password), and there is a test for it. The same-dialect case is not covered: the guard sends jdbc:oracle:thin:scott/pa?ss@//h:1521/svc back down the parameters-first path, so the stall warning and the SQLTimeoutException carry jdbc:oracle:thin:scott/pa. Same shape for jdbc:mysql://u:sec?ret@h/db -> jdbc:mysql://u:sec.

connect.timeout=0 also disables pool.timeout (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:343

private static long attemptSeconds(long connectTimeoutSeconds, long deadline) {
    if (connectTimeoutSeconds == 0 || deadline == Long.MAX_VALUE) {
        return connectTimeoutSeconds;   // 0 -> connect() sets no properties at all
    }

pool.timeout documents itself as "Bounds a whole borrow - every connect attempt and every wait for a pooled connection", but with the per-attempt bound turned off the attempt is unbounded, so an administrator who sets connect.timeout=0 and leaves pool.timeout=60 gets the #872 hang back. Clamp the attempt to the remaining deadline even when connectTimeoutSeconds == 0.

Nits

• Timeout message misdiagnoses the newly retried state: at CachedConnection.java:325 the SQLTimeoutException still reads "the database took no further connection and none was returned to the pool" even when the state being retried was 57P03 / ORA-01033 ("starting up"), which is the one case this round added.

[vharseko]

Thanks — all six stand, and the major one was worse than the report says. Fixed in e77c8f7, on a branch merged up to master; the description is updated to match.

PostgreSQL login is bounded only in the caller (major) — fixed

Right down to the mechanism. I checked it in the bytecode of the 42.7.12 we depend on rather than on the source:

• ConnectionFactoryImpl.openConnectionImpl: connectTimeout only reaches the PGStream constructor, which passes it to Socket.connect(addr, timeout). Both calls to newStream.setNetworkTimeout(socketTimeout * 1000) — the one before enableSSL and the one after — sit behind an ifle, that is if (socketTimeout > 0), and PGProperty.SOCKET_TIMEOUT defaults to 0.
• Driver.connect: timeout(props) > 0 branches into new Thread(new Driver$ConnectThread(url, props), "PostgreSQL JDBC driver connection thread"), a daemon; getResult sets abandoned = true and throws PSQLException("Connection attempt timed out.", CONNECTION_UNABLE_TO_CONNECT) while the thread stays in the read it was abandoned in.

So it was not only "bounded out of process": against a peer that completes the handshake and says nothing, the code before this branch parked the operation thread, and this branch returned the operation and leaked a daemon thread plus an ESTABLISHED socket on every borrow. That is a regression on the dialect the PR opens with, not a gap it left.

Taken as you proposed. connectProperty is a list now, so a dialect can name more than one property on the connect side:

POSTGRES("jdbc:postgresql:", '?',
    new String[]{"connectTimeout", "loginTimeout"}, 1, 0,
    new String[]{"socketTimeout"}, 1, true,   // the read bound, lifted once the login is through
    new int[]{}, new int[]{}),

socketTimeout is the read bound of the login and is lifted by relaxReadBound() once the login is through, as on the other three; loginTimeout stays on top of it for a url naming more than one host, where each host costs a connect and a login of its own — and with socketTimeout set, the thread it abandons dies inside that bound rather than never.

Worth noting that master already had this right elsewhere: the JDBCStorage.Dialect table #866 added for its stamp connection declares connectTimeout, loginTimeout and socketTimeout for postgresql. The two tables agree now.

testTheLoginThreadOfPostgresDoesNotOutliveTheBorrow covers it — it borrows against the never-answered socket and then waits for the PostgreSQL JDBC driver connection thread to be gone. Against the previous table it fails with the thread still alive after the whole 62 s window, which is what "forever" looks like from a test.

poll() destroys a pooled connection it never validated (minor) — fixed

The deadline check moved below the validation, so the entry in hand is always validated and only a connection the database no longer answers is closed. The drain still stops at the deadline; the most it can now overrun by is one validation rather than one per pooled connection.

isUsable() restores the network timeout on a connection that just failed validation (minor) — fixed

Early return on !usable. You are right about the drivers, and about the message being the wrong one for a routine idle reap — it named …jdbc.connect.timeout over a connection that was about to be closed either way.

connect() ignores the false from relaxReadBound() (minor) — fixed

I took "do not pool it" over "discard it": throwing would make the backend unusable against a driver that will not take a setNetworkTimeout at all, while poolable = false lets the connection serve the borrower that is waiting for it and closes it in close() instead of putting it back. connect() is package-private now so the test can hand it a dialect without a database behind it.

safeUrl() still leaks a password holding the dialect's own separator (minor) — fixed

Neither delimiter is looked for in the whole string any more. stripCredentials() goes by the shape of the url: on oracle the credentials stand between the subprotocol and the first @, which is the delimiter of the descriptor — a password holding an @ has to be quoted for the driver itself — and everywhere else inside the authority, which ends at the path rather than at a ?. So jdbc:oracle:thin:scott/pa?ss@//h:1521/svc reaches the log as jdbc:oracle:@//h:1521/svc and jdbc:mysql://u:sec?ret@h:3306/db as jdbc:mysql://h:3306/db, while ?user=u@example.com still keeps its host. The // survives the strip now, so the mysql expectation moved from jdbc:mysql:@h:3306/db to jdbc:mysql://h:3306/db.

One shape I deliberately did not add a test for: jdbc:sqlserver://u:sec;ret@h:1433;…. The SQL Server url has no userinfo form at all — jdbc:sqlserver://server[\instanceName][:port][;property=value] — so there is nothing in front of an @ to strip there, and a test asserting a shape the driver cannot take would be asserting an invention of ours.

connect.timeout=0 also disables pool.timeout (minor) — fixed

attemptSeconds() clamps to what is left of the deadline at 0 as well, and the javadoc of the property says what 0 means now: no bound of its own, with the deadline of pool.timeout still bounding the attempt; setting both to 0 is what leaves a connect unbounded. The negative control for this one is the most convincing of the set — against the previous code the new test hung for the full 600 s of the run, which is the shape of #872 itself.

Nit — fixed

The message of the timeout no longer diagnoses a database on its way up as one at its connection limit: "the database took no connection for the moment and none was returned to the pool, last error: …".

Tests

CachedConnectionTestCase is at 28 (from 23), still without a database, ~22 s. The five new ones: the login thread pgjdbc abandons, the pooled connection the deadline used to close unvalidated, the bound that is not put back on a connection the server reaped, the connection that must not be pooled, and a connect attempt the deadline bounds on its own. Each was run against the code it fixes and fails there — the four minors in seconds, the connect.timeout=0 one by hanging out the whole run.

On the merged branch: the four container suites 54/54 each and the JDBC EncryptedTestCase 34/34 — 250 tests, no skips — plus master's own JDBCStorageRetryTest 26/26 and StampConnectionTestCase 5/5.

Merged up to master

#886, #866 and #867 landed in the meantime, and #866 touches the same method: it added if (conNew != null) conNew.close() to the old recursive getConnection. Resolved in favour of this branch, which removes that method — the leak that patch closed is closed here by catch (SQLException | RuntimeException e) { closeQuietly(conNew); throw e; } in connect(), and for an unchecked failure out of the driver as well, which the master-side patch did not cover. TestCase.java merged on its own.

[maximthomas]

Round 3 looks good on the main point: the round-2 major is fixed — POSTGRES now carries socketTimeout as readProperties[0], set before enableSSL, in seconds, and lifted after login. I re-checked the connectProperty → String[] connectProperties refactor and the index/unit/range arithmetic holds for all four dialects.

Two things I'd want in before this merges, plus some minors. Everything below was measured against the pinned driver jars, not read off the docs.

The password reaches the log in cleartext (blocker)

safeUrl() drops only the first userinfo and assumes it ends before the first /. Both assumptions break on URL shapes these drivers accept. Measured with a verbatim copy of safeUrl / stripCredentials / endOfCredentials over 19 shapes:

jdbc:mysql://u:p@h1:3306,u2:SECOND_PW@h2:3306/db
  -> jdbc:mysql://h1:3306,u2:SECOND_PW@h2:3306/db      second host's password kept
jdbc:mysql:replication://master:PW1@h1:3306,slave:PW2@h2:3306/db
  -> jdbc:mysql:@h1:3306,slave:PW2@h2:3306/db          second host's password kept
jdbc:mysql://address=(host=h)(port=3306)(user=u)(password=SECRET)/db
  -> unchanged                                          full password
jdbc:mysql://(host=h,port=3306,user=u,password=SECRET)/db
  -> unchanged                                          full password
jdbc:mysql://u:pa/ss@h:3306/db
  -> unchanged                                          full userinfo ('/' in the password)

It is reached from logger.warn in warnStall (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:561) and from the SQLTimeoutException message at :358, so one slow connect writes the production DB password into the server error log and into an exception the operator sees. JDBCStorage.java:143 passes config.getDBDirectory() as the whole connection string, so the URL is the only place credentials live for this backend.

Worth noting on severity: safeUrl() and the warnStall logging are both new in this PR — neither exists at 0b9c0f63f5 — so this is introduced here, not inherited.

Suggested: after the existing shape strip, also redact by value, and drop every remaining userinfo@ rather than just the first:

url = url.replaceAll("(?i)(password|pwd)\\s*=[^,)&;?]*", "$1=***");

MySQL properties are matched case-insensitively, but Connector/J matches them case-sensitively (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:288:

final boolean exact = this == POSTGRES;

I linked against mysql-connector-j-9.2.0 and called PropertyKey.fromValue() directly:

connectTimeout   fromValue(exact)=connectTimeout   fromValue(lower)=null  fromValue(MiXeD)=null
socketTimeout    fromValue(exact)=socketTimeout    fromValue(lower)=null  fromValue(MiXeD)=null
user             fromValue(exact)=user             fromValue(lower)=user  fromValue(MiXeD)=user

So db-directory = jdbc:mysql://db:3306/opendj?...&SocketTimeout=2000 gets no read bound from either side: our matcher lowercases and decides the administrator declared it, and Connector/J ignores the mis-cased key. A host that completes the handshake and then goes silent parks the borrower — the hang #872 is about.

The comment at :283-286 cites PropertyKey.fromValue as the reason for the opposite behaviour, so it needs correcting too. MICROSOFT (getNormalizedPropertyName → equalsIgnoreCase) and ORACLE (EZConnectResolver lowercases) really are case-insensitive and should stay in the lowercasing branch.

final boolean exact = this == POSTGRES || this == MYSQL;

An Oracle read bound set with -D is invisible here, and this PR now wipes it (major)

declaredInUrl() searches only the connection string, but oracle.jdbc.ReadTimeout is also honoured as a JVM system property. Probed against a listener that completes the TCP handshake and then never speaks, oracle.net.CONNECT_TIMEOUT pinned at 60s so it cannot be what ends the wait (ojdbc8 23.7.0.25.01):

no bound anywhere                                    still blocked at 25000 ms
-Doracle.jdbc.ReadTimeout=2000                       ORA-17002 at 2502 ms, socketOptions {3=2000}
-D...=20000 + Properties oracle.jdbc.ReadTimeout=2000  ORA-17002 at 2501 ms, socketOptions {3=2000}

Row 2 shows the channel is real and declaredInUrl() cannot see it. Row 3 shows our Properties value silently outranks the operator's. Then relaxReadBound() calls setNetworkTimeout(con, 0) — PhysicalConnection.setNetworkTimeout rejects only negatives, so 0 reaches T4CConnection.doSetNetworkTimeout(0) → Communication.setSocketReadTimeout(0), i.e. SO_TIMEOUT infinite.

At base the class never called setNetworkTimeout itself (only the delegating override at :366), so an operator's -Doracle.jdbc.ReadTimeout=30000 used to survive for the life of the connection. It no longer does — this is a regression, not a pre-existing gap.

The comment at :487-488 ("A read bound the connection string sets itself is never touched here") is true as written but too narrow: the connection string is not the only channel. Cheapest fix is to consult System.getProperty() for the dotted names in readProperties inside declaredInUrl().

socketTimeout=0 is accepted as a bound (minor)

containsParameter() returns true as soon as it sees <delim>name=; it never reads the value. Every one of these drivers defines 0 as "block forever", so ?socketTimeout=0 is treated exactly like ?socketTimeout=5000 and bound() declines to set a read bound. Measured:

jdbc:postgresql://h:5432/db?socketTimeout=0   readBoundSet=false, but loginTimeout=30 IS still set
jdbc:mysql://h:3306/db?socketTimeout=0        no read bound
jdbc:sqlserver://h:1433;socketTimeout=0       no read bound

The postgres row is the bad one: loginTimeout alone makes pgjdbc hand the login to Driver$ConnectThread, and on expiry getResult() sets abandoned = true and throws 08001 while that daemon thread stays parked in a read with no timeout — one leaked thread plus one ESTABLISHED socket per borrow, which is the failure the class javadoc says this exists to prevent.

Suggestion: after matching name=, parse the digits and treat 0 as "not declared".

connect.timeout=0 escapes the int clamp and breaks every SQL Server connect (minor)

getConnection() clamps connect.timeout to Integer.MAX_VALUE / 1000 at :328-330, but when it is 0, attemptSeconds() returns remainingSeconds instead (:382-383), derived from pool.timeout — which getNonNegativeProperty() accepts up to Long.MAX_VALUE with no clamp (:107-121, :331). Measured on the pinned mssql-jdbc-13.4.0.jre11:

pool.timeout=60        socketTimeout=60000        normal "connection refused"
pool.timeout=2147483   socketTimeout=2147483000   normal "connection refused"
pool.timeout=3000000   socketTimeout=3000000000   "The socketTimeout 3000000000 is not valid."

The boundary is exactly the clamp the other path already applies. With those two properties set, a SQL Server backend cannot open a single connection, and the error names a property nobody typed. pgjdbc 42.7.12, mysql 9.2.0 and ojdbc8 23.7 all tolerate the value — mssql is the only one affected.

return Math.max(1, Math.min(connectTimeoutSeconds == 0
    ? remainingSeconds : Math.min(connectTimeoutSeconds, remainingSeconds),
    Integer.MAX_VALUE / 1000));

Nits

• RECV_TIMEOUT is not a name ojdbc8 reads: unzip -p ojdbc8-23.7.0.25.01.jar | strings | grep -c gives RECV_TIMEOUT 0, oracle.net.READ_TIMEOUT 6, oracle.jdbc.ReadTimeout 5. It is a sqlnet.ora/listener-side parameter. A TNS descriptor containing it makes declaredInUrl() true and silently drops our read bound, so an administrator who wrote a timeout ends up with less protection than one who wrote nothing. Suggest dropping it from the ORACLE readProperties at :163 and fixing the comment at :159-162.
• isUsable() catches only SQLException: at :420-424, around con.isValid(...). boundValidation (:453) and setNetworkTimeout (:499) both catch SQLException | RuntimeException. An unchecked throw unwinds through poll() — called at :339, outside every try in getConnection — and the connection already dequeued at :397/:406 never reaches closeQuietly at :402. No driver I checked actually does this, so it is hygiene, but it is the odd one out in its own file.
• openConnectionImpl is the wrong pointer: the comment at :140 sends the reader there, but in 42.7.12 both PGStream.setNetworkTimeout calls that read PGProperty.SOCKET_TIMEOUT are in the private tryConnect (javap offsets 141 and 410, SOCKET_TIMEOUT loaded at 119). The behaviour described is right; the method is not.
• connectTimeout is one shared budget, not one per host: :141-145 says a multi-host URL costs "a connect and a login of its own" per host. openConnectionImpl reads System.nanoTime() once at offset 24 — before it obtains the host iterator at offset 99 — and passes that same instant into every tryConnect. The login half of the claim stands; the connect half does not. The conclusion (keep loginTimeout, do not rely on it alone) still holds on the daemon-thread argument in the same comment.
• POOL_TIMEOUT_PROPERTY javadoc overstates the bound: :49 says it bounds a whole borrow, but poll() checks the deadline at :403, below isUsable() at :399, so a connection dequeued 1 ms before the deadline still gets a full 5 s validation past it — and attemptSeconds() then floors the last connect attempt at 1 s. A 60 s pool.timeout can return at ~66 s. The ordering is deliberate and defended at :391-394; it is the one-line description that needs softening.

[vharseko]

Thanks — all eight stand, and I re-measured every one of them against the pinned jars rather than taking the report on trust. Fixed in 625e2f2; the description is updated to match.

The password reaches the log in cleartext (blocker) — fixed

Reproduced with a verbatim copy of safeUrl/stripCredentials/endOfCredentials, and then checked which of the shapes are urls Connector/J 9.2.0 actually takes (ConnectionUrl.getConnectionUrlInstance, reading the password back off the HostInfo):

jdbc:mysql://u:p@h1:3306,u2:SECOND_PW@h2:3306/db            FAILOVER_CONNECTION    h2[user=u2 pwd=SECOND_PW]
jdbc:mysql:replication://master:PW1@h1:3306,slave:PW2@h2/db REPLICATION_CONNECTION h2[user=slave pwd=PW2]
jdbc:mysql://address=(host=h)(port=3306)(user=u)(password=SECRET)/db   SINGLE_CONNECTION  h[pwd=SECRET]
jdbc:mysql://(host=h,port=3306,user=u,password=SECRET)/db             SINGLE_CONNECTION  h[pwd=SECRET]

Four of the five leak and are real urls. The fifth is worth a correction: jdbc:mysql://u:pa/ss@h:3306/db is not a url the driver takes — it answers Failed to parse the host:port pair 'u:pa', and a raw / in a password (like a raw , or @: Malformed database URL, failed to parse the URL authority segment) has to be percent-encoded, which then holds neither delimiter. It changes nothing about the finding, and that shape is covered by the fallback below anyway.

safeUrl() now takes the userinfo off every host of an authority rather than the first — the authority is split on ,, which is where Connector/J puts the next host, and a subprotocol naming the kind of connection in front of the hosts (jdbc:mysql:replication://) is recognized as well. A password=/pwd= left standing anywhere is blanked out, which is what covers the key-value host syntax, where the credentials stand inside the authority itself.

I took the suggestion one step further, because a denylist over url shapes is what failed here in the first place: a url that none of this took apart is not logged past its subprotocol. An @ left standing anywhere but where the credentials of an oracle url end is one this did not recognize, and then the message is jdbc:mysql:<credentials hidden> — the host of a stall report is worth less than a password in the server log. That is what answers the pa/ss row above, and a quoted oracle password holding an @ (scott/"pa@ss"@//h:1521/svc), which the first @ used to cut in the middle.

And you are right that both safeUrl() and the warnStall logging are of this branch: at 0b9c0f63f5 neither exists, so nothing of this ever reached a release.

MySQL properties are matched case-insensitively (major) — fixed

Confirmed end to end rather than at PropertyKey.fromValue alone — the url parsed, the property set built from it, the effective value read back (mysql-connector-j-9.2.0):

?socketTimeout=2000&connectTimeout=1500  ->  effective socketTimeout=2000 connectTimeout=1500
?SocketTimeout=2000&ConnectTimeout=1500  ->  effective socketTimeout=0    connectTimeout=0
?sockettimeout=2000&connecttimeout=1500  ->  effective socketTimeout=0    connectTimeout=0

So a mis-cased parameter bounds nothing on either side. exact is now POSTGRES || MYSQL and the comment says why. The other two stay in the lowercasing branch, and I kept them honest: SQLServerDriver.getPropertyInfo normalizes socketTimeout/SocketTimeout/sockettimeout alike, and there is a test for a descriptor written in lower case on oracle.

An Oracle read bound set with -D (major) — fixed

Reproduced against a listener that completes the handshake and never speaks, oracle.net.CONNECT_TIMEOUT pinned at 60 s (ojdbc8 23.7.0.25.01):

no bound anywhere                                        still blocked at 20000 ms
-Doracle.jdbc.ReadTimeout=2000                           ORA-17002 at 2505 ms
-Doracle.jdbc.ReadTimeout=20000 + Properties 2000        ORA-17002 at 2558 ms

Row 3 is the one that matters: our value takes the timing over, and relaxReadBound() then lifts it as if it were ours. declared() consults System.getProperty() now, for dotted names only — a plain socketTimeout is a name common enough to be somebody else's system property, while oracle.jdbc.ReadTimeout is the name its own driver reads.

One extension beyond the report: I applied it to the connect property as well. -Doracle.net.CONNECT_TIMEOUT was overridden by ours just as silently; it left a bound in place, so it was not a hang, but the rule "an explicit setting of the administrator keeps precedence" should not depend on which of the two properties they chose.

socketTimeout=0 is accepted as a bound (minor) — fixed

containsParameter() reads the value now: 0, and an empty value, count as unset — every one of these drivers reads 0 as "wait as long as it takes", so it is the default this class exists to replace rather than a setting to stay out of the way of. A value that is no number is left alone: it is the driver's to interpret, not ours. The postgres row is the one I care about too — I confirmed in the bytecode of Driver.connect that the daemon thread comes from if (timeout(props) > 0), so our loginTimeout alone is exactly what turns an unbounded read into a leaked thread. The same rule applies to a -D set to 0.

connect.timeout=0 escapes the int clamp (minor) — fixed

Confirmed on the pinned mssql-jdbc-13.4.0.jre11, with a loginTimeout the driver certainly accepts so it cannot be what fails:

socketTimeout=2147483647  ->  normal "connection refused"
socketTimeout=2147483648  ->  "The socketTimeout 2147483648 is not valid."

attemptSeconds() is clamped as you suggested, so both the connect and the read property stay inside the range whatever pool.timeout says. There is a test for it that fails without the clamp: connect.timeout=0, pool.timeout=3000000, a refused connect to SQL Server, and the message must not name socketTimeout.

Nits — all four taken

• RECV_TIMEOUT: same counts here — RECV_TIMEOUT 0, oracle.net.READ_TIMEOUT 6, oracle.jdbc.ReadTimeout 5 files of the 2658 classes in ojdbc8-23.7.0.25.01.jar. Dropped from readProperties, and the test now asserts the opposite of what it did: a descriptor carrying one is not a read bound of the connection, so ours is set on top of it. READ_TIMEOUT, which the driver does read, keeps its precedence.
• isUsable() catches SQLException only: now SQLException | RuntimeException, with the reason in a comment - it would unwind through poll(), outside every try of the borrow, and leave the connection dequeued and closed by nobody. There is a test with a driver that throws an unchecked failure out of isValid.
• openConnectionImpl is the wrong pointer: agreed, and the offsets match — both PGStream.setNetworkTimeout calls that read SOCKET_TIMEOUT are in the private tryConnect (141 and 410, SOCKET_TIMEOUT loaded at 119). Comment fixed.
• connectTimeout is one shared budget: agreed as well. openConnectionImpl reads System.nanoTime() at offset 24, before the host iterator, and tryConnect opens with remainingConnectTimeout(int, long) over that same instant. The comment says "one budget for all of them" now; the conclusion about loginTimeout is unchanged, since it rests on the daemon thread.
• POOL_TIMEOUT_PROPERTY javadoc: softened - it bounds a borrow, but not to the millisecond, since the connection in hand is validated whatever the deadline says and an attempt is never given less than a second.

Tests

CachedConnectionTestCase is at 32, still without a database. Four new ones — the -D bound that is neither overridden nor lifted, the 0 that is nobody's bound, the value no driver would take, and the unchecked failure of a validation — and three rewritten, for the mysql case, for RECV_TIMEOUT, and for the url shapes above.

Each fix was put back one at a time and the assertion that covers it failed:

mysql matched without case            -> testTheCaseOfAParameterIsTheOneOfItsDriver
system properties not consulted       -> testASystemPropertyOfTheAdministratorKeepsPrecedence
a zero counted as a bound             -> testAZeroIsNotABoundOfTheAdministrator
no int clamp in attemptSeconds        -> testTheBoundHandedToADriverStaysInTheRangeAnIntTakes
validation catching SQLException only -> testAPooledConnectionWhoseValidationThrowsIsDiscarded
RECV_TIMEOUT back in readProperties   -> testConnectionStringKeepsPrecedence
only the first userinfo stripped      -> testLoggedConnectionStringCarriesNoCredentials
no redaction and no fallback          -> testLoggedConnectionStringCarriesNoCredentials

All four container suites were re-run against this head and pass with no skips - PgSql 54/54, MySql 54/54, Oracle 54/54, MsSql 54/54, plus the JDBC EncryptedTestCase 34/34 - and the 32 that need no database take 25 s.

[maximthomas]

Seven of the eight fixes land as described — safeUrl() itself is now correct over every shape from the last round, exact = POSTGRES || MYSQL, the int clamp, RECV_TIMEOUT, the widened catch and the four comment corrections all check out. Four things still stand, one of them the same leak as last round through a path the redaction does not cover. Everything below was measured against the pinned jars at 625e2f23.

The password still reaches the log in cleartext (blocker)

safeUrl() guards exactly two call sites, CachedConnection.java:412 and :621. Every other exit carries the raw string. With the driver jar absent the JDK itself throws:

// java.sql.DriverManager
throw new SQLException("No suitable driver found for " + url, "08001");

getErrorCode() is 0, and isWorthRetrying() (:582-604) matches only SQLState 53300/57P03 or a dialect vendor code — none of them 0. So :408 rethrows it as it stands:

if (!isWorthRetrying(e, dialect)) {
  throw e;   // message still holds jdbc:oracle:thin:scott/S3cret@//h:1521/svc
}

and :414 concatenates the cause's message into the one that is redacted:

... + " moment and none was returned to the pool, last error: " + e.getMessage(), e);

It does not stop at traceException. JDBCStorage.open():150 → RootContainer.open():134 (StorageRuntimeException, message = cause.toString()) → BackendImpl.newRootContainer():994 (ERR_OPEN_ENV_FAIL.get(e.getMessage())) → BackendConfigManager.java:1051 (stackTraceToSingleLineString into the ConfigChangeResult) → BackendConfigManager.java:263:

for (LocalizableMessage msg : ccr.getMessages()) { logger.error(msg); }

ERROR in the server error log, and the same string back over LDAP on a config change. ojdbc is not redistributable, so a missing driver jar is the ordinary Oracle misconfiguration — and testMissingDriverIsReportedAtOnce drives exactly this path. It is new here: at 0b9c0f63f5 every SQLException was swallowed by the doubling recursion, so none escaped.

Redact at the boundary rather than at two chosen call sites: wrap the rethrow at :408 in a SQLException carrying safeUrl(connectionString) and the original SQLState/errorCode (keeping the cause attached for traceException), and put the cause's message through the same redactor before concatenating it at :414.

-Doracle.net.READ_TIMEOUT switches off the read bound (major)

CachedConnection.java:305 treats a dotted name as one the driver reads as a system property. ORACLE's readProperties (:178) is {"oracle.jdbc.ReadTimeout", "oracle.net.READ_TIMEOUT"}; the second one is not such a name. Of the six classes in ojdbc8-23.7.0.25.01 carrying the literal (MQLNTAdapter, ConnStrategy, TcpNTAdapter, SdpNTAdapter, SQLnetDef, T4CConnection), none passes it to System.getProperty — the only arguments there are line.separator, socksProxyHost/Port, oracle.jdbc.disablePipeline, mql.Context. T4CConnection@495 only writes the key into the net properties, from thinReadTimeout, itself resolved from oracle.jdbc.ReadTimeout.

So an operator who exports it gets declared() true, :259 never runs, bound() returns false, and connect():524 goes out with only oracle.net.CONNECT_TIMEOUT set. Against a socket that accepts and never speaks:

Properties{oracle.jdbc.ReadTimeout=3000}          -> fails at 3007 ms
empty + -Doracle.net.READ_TIMEOUT=3000           -> still blocked at 25 s (killed)

CONNECT_TIMEOUT covers the TCP connect only; the same socket hung past 40 s with it set. There is no DriverManager.setLoginTimeout anywhere, and :524 is synchronous on the borrowing thread — the deadline is read only before and after it (:395-410), so a thread parked inside getConnection is never abandoned. That is #872 again, through a flag the operator believes is helping.

The rule is right for the other two: oracle.net.CONNECT_TIMEOUT is read as a system property (three tiers in GeneratedPhysicalConnection, offsets 2816-2874, via doPrivileged(System.getProperty) at :723 — measured 21223 ms with no flag, 4230 ms at -D=3000, 10225 ms at -D=9000). Only "contains a dot" is the wrong test; mark per entry which names are system-property channels, and keep oracle.net.READ_TIMEOUT in readProperties, where the driver does honour it.

An explicit PostgreSQL connect budget is silently capped at 30 s (major)

CachedConnection.java:253-257 decides the two connect properties one at a time:

for (final String property : connectProperties) {
  if (!declared(connectionString, property)) {
    properties.setProperty(property, ...);
  }
}

while the javadoc at :242-249 promises "an explicit setting of theirs keeps precedence". On pgjdbc these are not independent knobs: connectTimeout bounds the TCP connect, loginTimeout bounds the whole login. Setting either is the administrator declaring their budget.

jdbc:postgresql://h:5432/db?connectTimeout=300, defaults connect.timeout=30, measured through postgresql-42.7.12's own Driver.parseURL:

head 625e2f23   -> connectTimeout=300 socketTimeout=30 loginTimeout=30, timeout(props)=30000
                   -> ConnectThread created, getResult(30000) abandons the login at 30 s
base 0b9c0f63f5 -> loginTimeout=null, timeout(props)=0 -> no ConnectThread, the 300 s budget ran in full

So the 30 s cap is new, and pool.timeout is not what would have capped it anyway — the deadline is never enforced mid-connect (:399 is a straight synchronous call; it is re-read only in poll():454 and in the catch at :410).

Treat a dialect's connect properties as one group: if the administrator declared any of them, inject none. CachedConnectionTestCase.java:504-507 currently asserts the opposite and would move with the code.

The "a zero is nobody's bound" fix does nothing on PostgreSQL (major)

The reasoning is right and the implementation does what it says — but pgjdbc copies supplied Properties into a flat Properties and then runs parseURL over it, so the URL wins. For jdbc:postgresql://h:5432/db?connectTimeout=0&socketTimeout=0&loginTimeout=0, isBound("0") is false, bound() writes 30/30/30 and returns true, setting readBoundSet = true at :522. Measured through Driver.parseURL with exactly those properties:

effective connectTimeout=0 socketTimeout=0 loginTimeout=0
Driver.timeout(props)=0  -> no ConnectThread

makeConnection then runs on the borrowing thread itself: socket.connect with no timeout, login reads with no SO_TIMEOUT. The borrow hangs and nothing can abandon it, while the class believes it bounded the login and queues relaxReadBound() behind a call that will not return.

Two corrections to keep this in proportion: it is not a regression — at e77c8f7267 containsParameter had no value check, nothing was written, and pgjdbc saw 0/0/0 just the same. And there is no leaked daemon thread or ESTABLISHED socket per borrow here, because timeout()=0 means no ConnectThread exists; the caller simply hangs.

On POSTGRES a zero the administrator wrote has to be removed from the URL, not merely overridden in the properties — or the dialect has to declare that its URL outranks supplied properties and not claim readBoundSet when it does. The new test asserts bound() returned true and that the map holds "7", which is why this survived a round of re-measurement; asserting the driver's effective value via Driver.parseURL would have caught it.

The redaction is pinned by no test (minor)

safeUrl() has two call sites in main, :412 and :621. testLoggedConnectionStringCarriesNoCredentials calls the package-private static directly, 15 times, never through a borrow. The only test reaching :412 uses jdbc:opendj-stub:deadline, which has no credentials; :621 is asserted by nothing — there is no log capture in the file at all. Replacing safeUrl(connectionString) with connectionString at either site leaves the class green.

A driver outside the four gets no bound at all (minor)

ConnectDialect.of() (:231-239) matches only the four shipped prefixes. For anything else connect() takes the false arm of the dialect != null guard:

DriverManager.getConnection(connectionString, new Properties());   // :524, empty map

No connect, login or socket property, and the deadline is unreachable mid-connect. An admin-added jdbc:mariadb: or jdbc:h2: reproduces #872 verbatim, silently. One WARN per connection string naming the four supported prefixes would be enough.

A borrow that exhausted the whole deadline is retried as a failure of the moment (minor)

:412-414 builds the SQLTimeoutException with the (String, Throwable) constructor, so SQLState is null and vendorCode 0, and JDBCStorage.java:626-633 maps e instanceof SQLTimeoutException to FailureScope.MOMENT. A borrow that already spent the full pool.timeout is then retried, each attempt costing another one. Passing "08001" (or the cause's SQLState) explicitly would also stop monitoring keyed on the top-level SQLState from seeing null where base surfaced the driver's own exception.

boundValidation() cannot tell "left it alone" from "it failed" (minor)

:507-515 returns -1 for both. If a driver applies setNetworkTimeout(DIRECT_EXECUTOR, 5000) and then throws before :513, isUsable():492 reads restore >= 0 as "nothing to put back", isValid(5) succeeds, :494 returns true, and close():787 puts the connection back in the pool — permanently carrying a 5 s network timeout, so every statement over 5 s on it fails. The only closeQuietly (:459) is reached solely when isUsable is false. Latent; it needs an apply-then-throw driver, which none of the four pinned ones is known to be. A distinct sentinel from the catch would close it.

Nits

• The read-bound WARN names the wrong property: :566-569 cites CONNECT_TIMEOUT_PROPERTY and warns that "statements taking longer ... may fail", but on the isUsable() restore path the bound in force is VALIDATION_TIMEOUT_SECONDS (5 s), millis is the restore value (usually 0, so it reads "could not be set to 0 ms"), and poll() closes the connection one line later.
• The invalid-value WARN is unthrottled: getNonNegativeProperty (:118-132) is called twice per borrow from :382-385 and its warn at :128 has no guard, while warnStall (:618) and the read-bound warning (:565) both use STALL_WARNING_INTERVAL_MS. A typo'd connect.timeout=30s puts two WARN lines in the log per backend operation.
• safeUrl() over-redacts: scheme stops at the first sub-protocol token, so jdbc:oracle:thin:user/pw@//h:1521/svc logs as jdbc:oracle:@//h:1521/svc — thin-vs-oci is a first question in an Oracle connect diagnosis. And the whole parameter section is dropped, so jdbc:sqlserver://h:1433;databaseName=db;... logs as jdbc:sqlserver://h:1433: two backends on one host produce byte-identical stall lines, though the throttle map at :616 is still keyed on the full string so both do warn.
• attemptSeconds()'s javadoc promises a clamp it does not apply: the deadline == Long.MAX_VALUE branch (:434-442) returns connectTimeoutSeconds unclamped — attemptSeconds(Long.MAX_VALUE, Long.MAX_VALUE) is Long.MAX_VALUE. The clamp lives at the single caller (:382-384), and the method was widened to package-private this round.
• withoutUserinfo() drops a separator: hosts.length() > 0 as the "not first" test means jdbc:mysql://,h2:3306/db logs as jdbc:mysql://h2:3306/db. Cosmetic, no credential impact.
• The new container test has no timeOut: TestCase.java:149, unlike all 32 tests in the sibling file. What it guards against is an unbounded login, so a regression hangs the job instead of failing the test.
• TestCase.java:157 passes against the pre-fix code: assertEquals(con.getNetworkTimeout(), 0, "the read bound of the login is still in force") — no bound was ever set before this PR, so it was already 0; and on the PostgreSQL subclass the login read bound is the socketTimeout connect property, never setNetworkTimeout, so it cannot fail there. Asserting the bound before the lift, or that relaxReadBound() returned true, would fix both.
• testDrainOfThePoolStopsAtTheDeadline has no lower bound: assertTrue(validated.get() < pooled) (CachedConnectionTestCase.java:382) also passes if poll() drained nothing at all. validated.get() > 0 && validated.get() < pooled.
• testTheBoundHandedToADriverStaysInTheRangeAnIntTakes races an ephemeral port: it disables every bound (connect.timeout=0, pool.timeout=3000000) against a port bound and then closed; if anything on the host takes it in between, only the 120 s timeOut is left. A blackhole socket would be deterministic.
• testTheLoginThreadOfPostgresDoesNotOutliveTheBorrow counts JVM-global threads: three sibling tests also open pgjdbc logins against blackhole sockets, so a leak from any of them is attributed here after a 62 s wait, and TestNG method order is not contractual. Capture a delta at method start.

[vharseko]

Thanks — the blocker and the two majors stand, and I re-measured each of them against the pinned jars rather than taking the report on trust. Fixed in 52ca42b; the description is updated to match. One minor I did not take, with the measurement below.

The password still reaches the log in cleartext (blocker) — fixed

Confirmed end to end, including the part that makes it an ERROR line rather than a trace: RootContainer.open() catches into new StorageRuntimeException(e), whose super(cause) makes cause.toString() its own message, so the driver's message is carried into ERR_OPEN_ENV_FAIL as a message and not merely as a cause. BackendConfigManager then logs it and answers the config change with it.

Which is also why I did not take the shape you proposed as written. Wrapping while "keeping the cause attached for traceException" leaves the raw message reachable through two doors: stackTraceToSingleLineString walks the causes and appends throwable.toString() for each of them in its isFullStack branch (a debug build), and logger.traceException prints the chain in full. So the failure that leaves this class is rebuilt, not wrapped:

• every link of the chain gets a redacted message, keeping its SQLState, its vendor code and its stack trace; a link that is no SQLException keeps its class name in the message of the copy;
• the chain is walked first, and a failure naming nothing of the connection string is passed on untouched, with its own type — which is the common case (connection refused, password authentication failed), so nothing changes there;
• getMessage() of the cause is redacted before it goes into the SQLTimeoutException at the deadline, and so is the one in warnStall, which the report did not name but concatenated the driver's message just the same;
• the RuntimeException arm was open as well: DriverManager does not catch one, so a driver reporting a connect it will not make as unchecked went out with the url in hand. It is redacted now too.

The redactor takes the url down to safeUrl(), then blanks the credentials themselves — the userinfo of every host, the password inside it, the value of every password= — so a driver naming the credentials without the url around them is covered too. What it cannot answer for, and I would rather say so than imply otherwise: a driver quoting back a fragment of a url it failed to parse. A whole credential is replaced, a part of one is not, and detecting fragments cheaply means hiding messages like FATAL: password authentication failed for user "opendj" for any administrator whose password happens to share four characters with it.

Two tests, and neither calls the redactor from the side: testAReportedConnectCarriesNoCredentials drives a real borrow with a password in the url and asserts over every link of the chain (getCause() and getNextException() both), and testAMessageOfADriverCarriesNoCredentials holds the redactor to the shapes a driver quotes back. The stall line is built by a stallMessage() of its own now, so the rule it has to keep is a rule a test can hold it to without capturing a log.

-Doracle.net.READ_TIMEOUT switches off the read bound (major) — fixed

Right, and the rule was wrong in the general case rather than in that one entry. javap over ojdbc8-23.7.0.25.01 says what you said: GeneratedPhysicalConnection resolves oracle.jdbc.ReadTimeout in three tiers at offsets 2695 / 2708 (getSystemProperty) / 2727 and oracle.net.CONNECT_TIMEOUT the same way at 2821 / 2834 / 2853, while oracle.net.READ_TIMEOUT appears in that class not at all — the six classes carrying the literal read it out of a Properties: TcpNTAdapter.setReadTimeoutIfRequired(Properties) at offset 1, ConnStrategy.createSocketOptions at 96 (equalsIgnoreCase, then socket option 3), and T4CConnection@495 only writes the key, from thinReadTimeout.

So "contains a dot" is gone and the names a driver reads out of the system properties are listed instead — oracle.jdbc.ReadTimeout and oracle.net.CONNECT_TIMEOUT, the two that are measured to be read there. oracle.net.READ_TIMEOUT stays in readProperties, where the driver does honour it, so a descriptor or a property carrying one still keeps precedence and is still never lifted with ours. testASystemPropertyNoDriverReadsIsNoBound covers both directions.

An explicit PostgreSQL connect budget is silently capped at 30 s (major) — fixed

Reproduced through Driver.parseURL of the pinned 42.7.12, which is also what the test asserts on now:

?connectTimeout=300 + supplied {connectTimeout=30, loginTimeout=30, socketTimeout=30}
  -> connectTimeout=300 loginTimeout=30   timeout(props)=30000   -> ConnectThread, abandoned at 30 s
?connectTimeout=300 + supplied {}                                  (base)
  -> connectTimeout=300 loginTimeout=null timeout(props)=0        -> no ConnectThread, 300 s in full

Taken as you proposed: the connect properties of a dialect are one group, and a bound of the administrator under any of its names leaves all of them alone. It changes POSTGRES alone — the other three name one property on that side. testConnectionStringKeepsPrecedence moved with it, and testAConnectBudgetOfTheAdministratorIsNotCappedByThisClass reads the effective values back through the driver's parser rather than off the map handed to it.

The "a zero is nobody's bound" fix does nothing on PostgreSQL (major) — fixed

Also reproduced, same parser:

?connectTimeout=0&socketTimeout=0&loginTimeout=0 + supplied 30/30/30
  -> effective 0/0/0   timeout(props)=0

And your two corrections are right on both counts — it is not a regression, and there is no leaked thread here, since timeout()=0 means no ConnectThread exists at all.

I took the second of the two options you left open rather than rewriting the administrator's url: the dialect declares that its url outranks a supplied property, so on postgresql a parameter counts as the administrator's whatever its value, bound() returns false and nothing is claimed to have been set. Rewriting the url would have this class editing the one string the operator wrote, to override a 0 they typed on purpose.

What it must not do is stay silent about it, since nothing else can end that borrow: the url is reported once per connection string, naming the parameter and the value, at WARN. The same for the connect side of it.

The redaction is pinned by no test (minor) — fixed

Covered above: a borrow-driven test, a redactor test, and stallMessage() extracted so the second call site is assertable. Replacing safeUrl(connectionString) with connectionString at either site now fails the class.

A driver outside the four gets no bound at all (minor) — fixed

One WARN per connection string, naming the four prefixes that are bounded, exactly as you suggested. testAUrlThisBackendCannotBoundIsReportedOnce.

A borrow that exhausted the whole deadline is retried as a failure of the moment (minor) — half taken

The mapping is real — scopeOf at :632-633 does answer MOMENT for a SQLTimeoutException — but the borrow never reaches it, so the consequence does not follow. failureScope() has one caller in main, JDBCStorage:549, on the stamp path, and the stamp does not borrow from the pool: StampSession.connection() calls newStampConnection(), which is a DriverManager.getConnection(...) of its own at :367, deliberately so per the comment at :361-363 ("a connection of its own for the comment statements, outside the pool"). And in write() a failure of getConnection() is excluded from the replayed region explicitly (if (e != failure) throw e). So there is no path on which a timed-out borrow costs a second pool.timeout.

The other half of the point stands and is taken: the exception carried no SQLState where the driver's own carried one. It is built with 08001 now, and testConnectionLimitGivesUpAtTheDeadline asserts it.

If I have missed a caller, say so and I will fix the mapping instead — I looked for failureScope( across the module and found only that one and the tests.

boundValidation() cannot tell "left it alone" from "it failed" (minor) — fixed

Two sentinels. getNetworkTimeout() throwing is "the driver takes none" and leaves the connection alone; setNetworkTimeout() throwing is "it may well have applied it", and that connection is discarded without being validated — validating it would be the unbounded isValid() this exists to avoid, and pooling it would hand out five seconds of ours for the life of the connection. testAPooledConnectionThatCannotBeBoundedForItsValidationIsDiscarded.

Nits — all taken

• The read-bound WARN named the wrong property: the consequence is the caller's to name now, so the relaxReadBound() path says statements slower than the bound fail on that connection and it is closed rather than pooled, while the restore path says the connection is closed. CONNECT_TIMEOUT_PROPERTY is named only where it is the property in force.
• The invalid-value WARN was unthrottled: reported once per value now, together with the two new reports above — all three are settings rather than events, and every operation of the backend reads them.
• safeUrl() over-redacted: the token naming the kind of oracle driver is kept (it stands in front of the credentials, and it is found behind the subprotocol rather than in front of the @, so a password holding a : is still cut whole), and the parameter naming the database is kept from the parameter section — by an allowlist, since a name this does not know is a name free to carry a secret. Two sql server backends on one host produce different stall lines again.
• attemptSeconds() promised a clamp it did not apply: applied in the Long.MAX_VALUE branch as well, with 0 still standing for "no bound of its own". testTheBoundOfAnAttemptStaysInRangeWithoutADeadline.
• withoutUserinfo() dropped a separator: joined by position now, so an empty first host keeps its comma.
• The container test had no timeOut: 120 s, like its siblings.
• TestCase.java:157 passed against the pre-fix code: this is the one I am most grateful for. It asserts the bound is in force before asserting it is lifted — established with the very properties the borrow uses and read back off the socket, which also proves the dialect table names a property that really bounds the login on each of the four (all four report 2000 ms). Against a relaxReadBound() that lifts nothing, PgSql now fails with expected [0] but found [2000]; before, the assertion held either way.
• testDrainOfThePoolStopsAtTheDeadline had no lower bound: validated.get() > 0 && validated.get() < pooled.
• testTheBoundHandedToADriverStaysInTheRangeAnIntTakes raced an ephemeral port: it uses a socket that accepts the connection and closes it at once, so the failure is deterministic and belongs to nobody else. With every bound of that borrow turned off, a squatter on the released port would have left it on the 120 s timeOut.
• testTheLoginThreadOfPostgresDoesNotOutliveTheBorrow counted JVM-global threads: a delta captured at the start of the method.

Tests

CachedConnectionTestCase is at 41 (from 32), still without a database, ~23 s. Each fix was put back one at a time and the assertion covering it failed:

the rethrow is not redacted            -> testAReportedConnectCarriesNoCredentials
the message of a driver is not         -> testAMessageOfADriverCarriesNoCredentials
connect properties filled one by one   -> testAConnectBudgetOfTheAdministratorIsNotCappedByThisClass
a postgres url does not outrank ours   -> testAParameterOfAPostgresUrlOutranksTheBoundOfThisClass
a dotted name is a system property     -> testASystemPropertyNoDriverReadsIsNoBound
one sentinel for two outcomes          -> testAPooledConnectionThatCannotBeBoundedForItsValidationIsDiscarded
no clamp without a deadline            -> testTheBoundOfAnAttemptStaysInRangeWithoutADeadline
the subprotocol and parameters dropped -> testLoggedConnectionStringCarriesNoCredentials
relaxReadBound() lifts nothing         -> TestCase.testLoginBoundDoesNotOutliveTheLogin (PgSql container)

All four container suites pass against this head with no skips — PgSql 54/54, MySql 54/54, MsSql 54/54, Oracle 54/54 — plus the JDBC EncryptedTestCase 34/34, JDBCStorageRetryTest 26/26 and StampConnectionTestCase 5/5.