From 352e5e6aab377015cfb4e20b8106cc0f41f80295 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 25 Sep 2026 14:59:41 +0300 Subject: [PATCH] [#1074] Keep what says the connection is gone through the redaction of a connect failure reported() rebuilds a connect failure link by link wherever it may carry the credentials of the backend - and on every chain longer than the walk looking for them, which answers "yes" past its 32 links whether the url has a password or not. The rebuild made every link a plain SQLException and put a bare one with no SQLState where its own budget ran out, so a link saying the connection is gone by its type, or standing past the 32nd, read false through isConnectionFailure(): write() neither replayed the attempt nor distrusted the pool over a catalog connect refused the way a shorter chain is replayed. Every rebuilt link now keeps the standard JDBC type it extends, and the link standing for what the budget cut carries the type, SQLState and vendor code of the first link of the rest that says the connection is gone - and says nothing of it where the rest does not, so the cut makes no failure a drop either. Fixes #1074 --- .../backends/jdbc/CachedConnection.java | 117 ++++++++++++++++-- .../server/backends/jdbc/JDBCStorage.java | 6 +- .../jdbc/CachedConnectionTestCase.java | 69 +++++++++++ .../jdbc/CatalogConnectionTestCase.java | 58 +++++++++ 4 files changed, 236 insertions(+), 14 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java index 814304a7ca..3a0f01f063 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java @@ -258,7 +258,8 @@ static long nextBackoffMs(long backoffMs) { * How many links of the cause and getNextException() chains of a failure the two walks that * have a reason to stop look at: {@link #holdsCredentials} answers "yes" past it, since the * cost of the other answer is the password of the backend in the log, and - * {@link #redactedCopy} rebuilds that far and names the rest in one link. The walk whose verdict + * {@link #redactedCopy} rebuilds that far and names the rest in one link - carrying, where the + * rest says the connection is gone, what says it (#1074). The walk whose verdict * decides something, {@link #isWorthRetrying}, is not one of them: a count does not leave that * question unanswered, it answers it with the verdict of a failure that carries nothing * (issue #1076), and the visited set of every walk here terminates it on its own. @@ -2135,8 +2136,11 @@ static String stallMessage(String connectionString, int attempts, long waitedMs, * prints a failure prints the causes along with it - a debug build of * stackTraceToSingleLineString walks them, the config manager traces them, and * RootContainer.open() makes the message of the cause the message of what it throws - so a - * link left as it stands would carry the password past the wrapper. The SQLState and the - * vendor code of every link survive it: they are what tells a caller what happened. + * link left as it stands would carry the password past the wrapper. The SQLState, the vendor + * code and the standard JDBC type of every link survive it: they are what tells a caller what + * happened, and JDBCStorage.write() asks the type before the state whether the connection is + * gone. Past the budget of the rebuild the one link standing for the rest says that much of it + * where the rest says so (#1074). *

* The whole chain is the causes, the further exceptions of {@code getNextException()} and what * was suppressed on each of them - the last being where this class puts the failure of a close @@ -2223,40 +2227,126 @@ private static void enqueue(Deque pending, Set visited, Th private static SQLException redactedCopy(SQLException e, String connectionString, int[] budget) { budget[0]--; final SQLException copy = - new SQLException(redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode()); + sameKind(e, redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode()); copy.setStackTrace(e.getStackTrace()); if (e.getNextException() != null) { copy.setNextException(budget[0] > 0 - ? redactedCopy(e.getNextException(), connectionString, budget) : droppedTail()); + ? redactedCopy(e.getNextException(), connectionString, budget) + : droppedTail(Collections.singletonList(e.getNextException()))); } if (e.getCause() != null) { - copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget) : droppedTail()); + copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget) + : droppedTail(Collections.singletonList(e.getCause()))); } copySuppressed(e, copy, connectionString, budget); return copy; } + /** + * A new exception of the kind JDBC names the given one as. Not the class of the driver itself, + * which this cannot be sure of building, but the standard type it extends: that type is what + * the JDBC contract gives a driver to say what happened beside the SQLState, and JDBCStorage + * reads it first - a SQLRecoverableException says the connection is gone whatever state it + * carries, and a copy made as a plain SQLException says nothing of the kind (#1074). + */ + private static SQLException sameKind(SQLException e, String message, String sqlState, int vendorCode) { + if (e instanceof SQLRecoverableException) { + return new SQLRecoverableException(message, sqlState, vendorCode); + } + if (e instanceof SQLNonTransientConnectionException) { + return new SQLNonTransientConnectionException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransientConnectionException) { + return new SQLTransientConnectionException(message, sqlState, vendorCode); + } + if (e instanceof SQLTimeoutException) { + return new SQLTimeoutException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransactionRollbackException) { + return new SQLTransactionRollbackException(message, sqlState, vendorCode); + } + if (e instanceof SQLFeatureNotSupportedException) { + return new SQLFeatureNotSupportedException(message, sqlState, vendorCode); + } + if (e instanceof SQLIntegrityConstraintViolationException) { + return new SQLIntegrityConstraintViolationException(message, sqlState, vendorCode); + } + if (e instanceof SQLInvalidAuthorizationSpecException) { + return new SQLInvalidAuthorizationSpecException(message, sqlState, vendorCode); + } + if (e instanceof SQLSyntaxErrorException) { + return new SQLSyntaxErrorException(message, sqlState, vendorCode); + } + if (e instanceof SQLDataException) { + return new SQLDataException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransientException) { + return new SQLTransientException(message, sqlState, vendorCode); + } + if (e instanceof SQLNonTransientException) { + return new SQLNonTransientException(message, sqlState, vendorCode); + } + return new SQLException(message, sqlState, vendorCode); + } + // The suppressed links of a failure are rebuilt with the rest of it and counted against the // same budget. They are not decoration here: the failure of a close that could not be made // rides on the failure being unwound (establish(), #929), and a rebuild that dropped them // would lose it at the one deployment whose log is redacted - which is the deployment whose // url has a password in it, the log that is worth reading. private static void copySuppressed(Throwable from, Throwable to, String connectionString, int[] budget) { - for (final Throwable suppressed : from.getSuppressed()) { + final Throwable[] suppressed = from.getSuppressed(); + for (int i = 0; i < suppressed.length; i++) { if (budget[0] <= 0) { - to.addSuppressed(droppedTail()); + to.addSuppressed(droppedTail(Arrays.asList(suppressed).subList(i, suppressed.length))); return; } - to.addSuppressed(redactedLink(suppressed, connectionString, budget)); + to.addSuppressed(redactedLink(suppressed[i], connectionString, budget)); } } // What stands where the budget ran out. Without it the same failure logs its root cause when // the url of the backend has no password in it and loses it without a word when it has, which // is a report of a connect nobody can read against a report of one they can. - private static SQLException droppedTail() { - return new SQLException("the rest of this failure was left out: a chain of more than " - + MAX_CHAIN_LENGTH + " links is rebuilt only that far"); + // It also says what the rest says about the connection, where the rest says it is gone: that + // is a verdict JDBCStorage.write() reads off every link of the failure - it replays the attempt + // and distrusts the pool on it - and a tail saying nothing would answer it with the "no" of a + // failure that carries nothing (#1074, the #961 thesis on the budget of this rebuild). Only + // that, and only where the rest does say it, so that the cut makes no failure say it either. + private static SQLException droppedTail(List rest) { + final String message = "the rest of this failure was left out: a chain of more than " + + MAX_CHAIN_LENGTH + " links is rebuilt only that far"; + final SQLException gone = firstLinkSayingTheConnectionIsGone(rest); + return gone == null + ? new SQLException(message) + : sameKind(gone, message + "; a link of it says the connection is gone", gone.getSQLState(), + gone.getErrorCode()); + } + + // Every link of the rest, by the edges JDBCStorage.isConnectionFailure() walks - the causes, the + // further exceptions and what was suppressed - and to its end: the visited set terminates it, and + // it builds nothing, so what it costs is a walk of the links the driver has already allocated. + private static SQLException firstLinkSayingTheConnectionIsGone(List rest) { + final Deque pending = new ArrayDeque<>(); + final Set visited = Collections.newSetFromMap(new IdentityHashMap()); + for (final Throwable t : rest) { + enqueue(pending, visited, t); + } + while (!pending.isEmpty()) { + final Throwable t = pending.poll(); + if (t instanceof SQLException) { + final SQLException sql = (SQLException) t; + if (JDBCStorage.saysTheConnectionIsGone(sql)) { + return sql; + } + enqueue(pending, visited, sql.getNextException()); + } + enqueue(pending, visited, t.getCause()); + for (final Throwable suppressed : t.getSuppressed()) { + enqueue(pending, visited, suppressed); + } + } + return null; } // A link that is no SQLException keeps its class name in the message: its type is not one this @@ -2270,7 +2360,8 @@ private static Throwable redactedLink(Throwable t, String connectionString, int[ + (t.getMessage() == null ? "" : ": " + redact(t.getMessage(), connectionString))); copy.setStackTrace(t.getStackTrace()); if (t.getCause() != null) { - copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget) : droppedTail()); + copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget) + : droppedTail(Collections.singletonList(t.getCause()))); } copySuppressed(t, copy, connectionString, budget); return copy; diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index e38d3cfb7f..de316f162c 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -4924,8 +4924,12 @@ private static void walkLinks(Throwable failure, boolean withTheRelease, int lin * a driver that raises one of them has said so whatever state it filled in. Oracle reports ORA-03113, ORA-00028 * and ORA-01089 as {@link SQLRecoverableException} and happens to map them to 08006 as well; the type is what * makes that robust rather than lucky. + *

+ * Package-private for the one other reader it has, the redaction of a connect failure in {@link + * CachedConnection}: what that rebuild leaves in place of a chain past its budget has to say what this reads + * off the links it cuts (#1074), and a second copy of the question would drift from this one. */ - private static boolean saysTheConnectionIsGone(SQLException e) { + static boolean saysTheConnectionIsGone(SQLException e) { if (e instanceof SQLRecoverableException || e instanceof SQLNonTransientConnectionException || e instanceof SQLTransientConnectionException) { return true; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java index ce66a7e434..bb233814f5 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java @@ -40,7 +40,10 @@ import java.sql.DriverPropertyInfo; import java.sql.PreparedStatement; import java.sql.SQLException; +import java.sql.SQLNonTransientConnectionException; +import java.sql.SQLRecoverableException; import java.sql.SQLTimeoutException; +import java.sql.SQLTransientConnectionException; import java.util.ArrayDeque; import java.util.Collections; import java.util.Deque; @@ -941,6 +944,72 @@ public void testTheTailOfALongChainIsNamedRatherThanDropped() throws Exception { "the tail a rebuild had no budget for has to say so: " + last.getMessage()); } + /** + * A link that says the connection is gone by its type alone still says so once it is rebuilt + * (#1074). The type is what the JDBC contract gives a driver to say it - write() asks it before + * the SQLState - so a copy made as a plain SQLException reads as a failure that says nothing of + * the connection, and only at the deployment whose url has a password in it. + */ + @Test(timeOut = 60000) + public void testALinkThatSaysTheConnectionIsGoneByItsTypeKeepsItThroughRedaction() throws Exception { + final String url = "jdbc:postgresql://opendj:S3cretOfTheBackend@127.0.0.1:5432/opendj"; + for (final SQLException gone : new SQLException[] { new SQLRecoverableException("io error"), + new SQLNonTransientConnectionException("closed"), new SQLTransientConnectionException("reset") }) { + final SQLException failure = new SQLException("login to " + url + " failed", "S0001", 18456); + failure.setNextException(gone); + + final SQLException reported = CachedConnection.reported(failure, url); + + assertNoCredentials(reported); + assertTrue(JDBCStorage.isConnectionFailure(reported), + "a rebuilt " + gone.getClass().getSimpleName() + " no longer says the connection is gone"); + } + } + + /** + * ... and so does one standing past the budget of the rebuild, whatever it says it by: the link + * that stands for the rest of the chain there is all that the classification of write() gets to + * read of it. The url has no password, and the chain is rebuilt all the same - it is longer than + * the walk looking for credentials, which answers "yes" past it. + */ + @Test(timeOut = 60000) + public void testALinkThatSaysTheConnectionIsGonePastTheBudgetOfARebuildStillSaysSo() throws Exception { + final String url = "jdbc:postgresql://127.0.0.1:5432/opendj"; + for (final SQLException gone : new SQLException[] { new SQLException("connection reset", "08S01", 10054), + new SQLRecoverableException("io error") }) { + final SQLException failure = plainChain(40); + failure.setNextException(gone); + + final SQLException reported = CachedConnection.reported(failure, url); + + assertTrue(reported != failure, "a chain this long is expected to be rebuilt"); + assertTrue(JDBCStorage.isConnectionFailure(reported), + gone + " past the budget of the rebuild no longer says the connection is gone"); + } + } + + /** + * The link standing for the rest says only what the rest says: a chain that says nothing of the + * connection is not made to say it is gone by being cut. That would replay the write and distrust + * the pool over a database that refused a connection for a reason of its own. + */ + @Test(timeOut = 60000) + public void testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuild() throws Exception { + final SQLException reported = CachedConnection.reported(plainChain(40), "jdbc:postgresql://127.0.0.1:5432/opendj"); + + assertFalse(JDBCStorage.isConnectionFailure(reported), + "the rebuild made a failure that says nothing of the connection say it is gone"); + } + + /** That many plain links of the next exception chain, none of which says the connection is gone. */ + private static SQLException plainChain(int links) { + final SQLException head = new SQLException("error 0", "S0001", 1); + for (int i = 1; i < links; i++) { + head.setNextException(new SQLException("error " + i, "S0001", 1)); + } + return head; + } + /** * A credential named by a suppressed link alone is redacted like any other. The close of a * connection whose set-up failed rides there (establish(), #929) and an interrupt that ended a diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java index 53404b31bc..17af3506b1 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java @@ -685,6 +685,64 @@ public void testAConnectionWhoseSetUpFailsIsClosed() throws Exception { verify(failing).close(); } + /** + * A refusal that says the connection is gone reaches {@code write()} saying so, however deep in its + * chain the driver put the link that says it (#1074). The failure leaves this connect redacted, and + * a chain longer than the walk looking for credentials is rebuilt whether or not anything in it names + * them - this url has no password at all - so what the rebuild leaves past its budget is what {@code + * isConnectionFailure()} reads: the attempt is replayed where the same refusal a few links shorter is. + */ + @Test + public void testARefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception { + final SQLException refusal = deepChain(40, new SQLException("connection reset", "08S01", 10054)); + probeDriver.refusal = refusal; + probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + try { + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60); + fail("a connect that will not clear was retried instead of being reported"); + } catch (SQLException expected) { + assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone"); + assertTrue(JDBCStorage.isConnectionFailure(expected), + "the refusal as it left the connect lost what said the connection is gone: " + expected); + } + } + + /** + * ... and so does the timeout a refusal worth waiting out ends in, whose cause is that refusal + * redacted the same way: the retry changes no classification. + */ + @Test + public void testATimedOutRefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception { + final SQLException refusal = new SQLException("too many clients already", "53300"); + refusal.setNextException(deepChain(40, new SQLException("connection reset", "08S01", 10054))); + probeDriver.refusal = refusal; + probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + try { + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 1); + fail("a connect refused for the whole deadline was not given up on"); + } catch (SQLTimeoutException expected) { + assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone"); + assertTrue(JDBCStorage.isConnectionFailure(expected), + "the timeout lost what its last refusal said about the connection: " + expected); + } + } + + /** + * A driver's failure the way mssql-jdbc chains every error of a message it received: that many + * plain links of the next exception chain, and the given one last. + */ + private static SQLException deepChain(int links, SQLException last) { + final SQLException head = new SQLException("error 0 of the login", "S0001", 1); + SQLException tail = head; + for (int i = 1; i < links; i++) { + final SQLException next = new SQLException("error " + i + " of the login", "S0001", 1); + tail.setNextException(next); + tail = next; + } + tail.setNextException(last); + return head; + } + /** What the dialect of this url declares, at the given number of seconds. */ private static void assertBoundedAt(Properties handed, long seconds) { assertNotNull(handed, "no properties were handed to the driver at all");