From 240fd3611e01dbb0a4b22ac371e495ed9e53d8aa Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Mon, 21 Sep 2026 10:00:06 +0000
Subject: [PATCH] [#961] Walk every link of a failure where the verdict decides something (#1004)
---
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java | 110 +++++++++++++++++++++++++++++++++---
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java | 61 +++++++++++---------
2 files changed, 133 insertions(+), 38 deletions(-)
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 39024b4..6721873 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
@@ -89,29 +89,28 @@
private static final double MAX_SLEEP_ON_RETRY_MS = 1000.0;
/**
- * Number of links walked by the questions that are not asked to the end of the chains: a guard against a chain
- * long enough to matter, at the cost of what truncation costs each of them. One number for three chains at
- * once - the causes, the next exceptions and the suppressed exceptions are walked together and counted
- * together - so it is set well above the depth a wrapped failure of this backend reaches: mssql-jdbc chains
- * every error of one message it received through {@code setNextException}, and a budget spent on those would
- * never reach the cause the wrapper carries.
+ * Number of links walked by the one question of this class whose answer truncation makes less precise rather
+ * than wrong: the fallback of {@link #conflictSummary} that names the first {@link SQLException} of a failure,
+ * where a link past the budget costs a less precise line in the log and no decision. Every question a decision
+ * reads is asked with {@link #EVERY_LINK} instead: a budget does not leave those unanswered, it answers them
+ * with the verdict of a failure that carries nothing - which is what issue #961 was.
* <p>
- * For the fallbacks of {@link #conflictSummary} truncation leaves a question unanswered and nothing more: the
- * line reporting the replay names a less precise link, and no decision moves. For
- * {@link #isConnectionFailure} it does weaken the verdict, which is the test {@link #EVERY_LINK} exists to
- * apply: a class 08 link past this many links leaves {@code dropped} false in {@link #write}, so
- * {@link #distrustPool} is not called and the pool keeps handing out - unvalidated - the connections it had
- * established before the same restart or failover. That is what this walk did before #903 and it is left as it
- * is here rather than widened along with the two below, since nothing about the grant of #903 depends on it;
- * the budget is pinned from both sides by {@code testTheWalkOfAFailureStopsAtItsBudget}, which is where
- * widening it would have to start.
+ * One number for three chains at once - the causes, the next exceptions and the suppressed exceptions are walked
+ * together and counted together - so it is set well above the depth a wrapped failure of this backend reaches:
+ * mssql-jdbc chains every error of one message it received through {@code setNextException}, and a budget spent
+ * on those would never reach the cause the wrapper carries.
*/
private static final int MAX_CHAIN_LINKS = 64;
/**
- * The budget of the two walks whose verdict would weaken rather than go unnoticed under truncation:
- * {@link #failureScope} and {@link #conflictVerdict}. See the comments above them; the {@code seen} set of
- * the walk terminates it either way.
+ * The budget of every walk of this class but one: {@link #failureScope}, {@link #isConnectionFailure},
+ * {@link #conflictVerdict}, {@link #lockNotAvailable} and the lookup of {@link #conflictSummary} that mirrors
+ * {@link #isConnectionFailure} all read to the end of the chains - see the comments above them. A budget does
+ * not leave a question a decision reads unanswered, it answers it with the verdict of a failure that carries
+ * nothing, which is what issue #961 was. The one exception is what {@link #MAX_CHAIN_LINKS} is left for, where
+ * truncation costs the precision of a line of the log and no decision. The {@code seen} set terminates the walk
+ * whatever budget it is given, so this one costs a walk of the links the driver has already allocated and
+ * nothing else.
*/
private static final int EVERY_LINK = Integer.MAX_VALUE;
@@ -3909,9 +3908,16 @@
* 14.20.3.1) rather than as a cause. The walk starts at the failure this class was handed because it reaches it
* wrapped in a {@link StorageRuntimeException}, and a caller such as {@code EntryContainer.addEntry} may wrap
* it once more.
+ * <p>
+ * Every link of them is walked, rather than {@link #MAX_CHAIN_LINKS} of them, for the reason
+ * {@link #failureScope} walks every link: truncation does not leave this question unanswered, it answers it with
+ * the {@code false} of a failure that says nothing about the connection. A class 08 link past a budget leaves
+ * {@code dropped} false in {@link #write} and in {@link #read}, so {@link #distrustPool} is not called and the
+ * pool keeps handing out - unvalidated, for the rest of the alive window - the connections the same restart or
+ * failover took, and {@link #replayReason} fails an attempt that was worth replaying (issue #961).
*/
static boolean isConnectionFailure(Throwable failure) {
- return firstLinkMatching(failure, WITH_THE_RELEASE, JDBCStorage::saysTheConnectionIsGone)!=null;
+ return firstLinkMatching(failure, WITH_THE_RELEASE, EVERY_LINK, JDBCStorage::saysTheConnectionIsGone)!=null;
}
/**
@@ -3928,13 +3934,11 @@
* The first {@link SQLException} of the chains of a failure that answers the given question, or null where none
* does. Every classifier of this class walks the failure this way, so that none of them reads a chain the others
* act on: what makes a write replayable must also be what the pool is told about and what the replay logs.
+ * <p>
+ * The number of links it may look at is named by every caller rather than defaulted, so that a walk whose
+ * verdict decides something cannot inherit a budget without saying so: {@link #EVERY_LINK} for the questions a
+ * decision reads, {@link #MAX_CHAIN_LINKS} for the one that only names a link in a line of the log.
*/
- private static SQLException firstLinkMatching(Throwable failure, boolean withTheRelease,
- Predicate<SQLException> matches) {
- return firstLinkMatching(failure, withTheRelease, MAX_CHAIN_LINKS, matches);
- }
-
- /** The walk above, with the number of links it is allowed to look at. */
private static SQLException firstLinkMatching(Throwable failure, boolean withTheRelease, int links,
Predicate<SQLException> matches) {
final SQLException[] found=new SQLException[1];
@@ -4263,13 +4267,14 @@
// that merely resembles the one the decision was taken on
SQLException named=verdict.link;
if (named==null) {
- named=firstLinkMatching(failure, WITH_THE_RELEASE, JDBCStorage::saysTheConnectionIsGone);
+ named=firstLinkMatching(failure, WITH_THE_RELEASE, EVERY_LINK, JDBCStorage::saysTheConnectionIsGone);
}
if (named==null) {
// without the release, so that the line names the statement that failed rather than the rollback
// behind it: this is the fallback of a replay decided on isClosed(con) alone, where neither chain
- // carries a verdict, and the walk reaches the suppressed exceptions before the cause
- named=firstLinkMatching(failure, WITHOUT_THE_RELEASE, e -> true);
+ // carries a verdict, and the walk reaches the suppressed exceptions before the cause. The one walk
+ // left bounded: no decision reads it, so a link past the budget costs the precision of this line
+ named=firstLinkMatching(failure, WITHOUT_THE_RELEASE, MAX_CHAIN_LINKS, e -> true);
}
return named==null
? String.valueOf(failure)
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java
index 0aff8f7..0a20a37 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java
@@ -591,16 +591,48 @@
}
/**
- * The walk of a failure looks at a bounded number of links: mssql-jdbc chains every error of one message it
- * received through {@code setNextException}, and a budget spent on those would never reach the cause a wrapper
- * carries. Pinned from both sides - a drop on the last link of the budget is found and one link further is not
- * - since a number nothing pins drifts unnoticed in either direction.
+ * The walk of a failure reads every link of it where a decision reads the verdict, however long the chains
+ * are - mssql-jdbc chains every error of one message it received through {@code setNextException}. A budget
+ * does not leave the question unanswered here: it answers it with the {@code false} of a failure that says
+ * nothing about the connection, so the pool is never told of the drop and the attempt is failed to its caller
+ * instead of being replayed (issue #961). What terminates the walk is its {@code seen} set, which the cyclic
+ * chains of the providers above pin.
*/
@Test
- public void testTheWalkOfAFailureStopsAtItsBudget()
+ public void testTheWalkOfAFailureReachesADropPastAnyBudget()
{
- assertTrue(JDBCStorage.isConnectionFailure(chainEndingInADrop(64)), "a drop on the last link of the budget");
- assertFalse(JDBCStorage.isConnectionFailure(chainEndingInADrop(65)), "a drop past the budget was walked to");
+ assertTrue(JDBCStorage.isConnectionFailure(chainEndingIn(65, "08006")), "a drop one link past the budget");
+ assertTrue(JDBCStorage.isConnectionFailure(chainEndingIn(200, "08006")), "a drop far past the budget");
+ assertEquals(replayReason(chainEndingIn(200, "08006"), MSSQL, false, false, false),
+ "a connection the database dropped", "an attempt worth replaying was failed to its caller");
+ }
+
+ /**
+ * The line reporting a replay names the link the replay was decided on however deep it sits: a lookup that
+ * stopped where the verdict does not would name the statement at the head of the chain instead, and describe
+ * a replay that did not happen.
+ */
+ @Test
+ public void testTheSummaryNamesADecidingLinkPastAnyBudget()
+ {
+ final String drop = conflictSummary(chainEndingIn(200, "08006"), MSSQL);
+ assertTrue(drop.contains("08006"), drop);
+ final String conflict = conflictSummary(chainEndingIn(200, "40001"), MSSQL);
+ assertTrue(conflict.contains("40001"), conflict);
+ }
+
+ /**
+ * What the budget is left for: the fallback of the summary that names a link no decision reads, where
+ * truncation costs the precision of one line of the log. Pinned from both sides - the last link of the budget
+ * is named and one link further is not - since a number nothing pins drifts unnoticed in either direction.
+ */
+ @Test
+ public void testTheFallbackOfTheSummaryStopsAtItsBudget()
+ {
+ assertTrue(conflictSummary(wrappedTimes(63, sql(2627, "23000")), POSTGRES).contains("23000"),
+ "the statement on the last link of the budget was not named");
+ assertFalse(conflictSummary(wrappedTimes(64, sql(2627, "23000")), POSTGRES).contains("23000"),
+ "a link past the budget was walked to");
}
/**
@@ -961,6 +993,53 @@
}
/**
+ * A drop the failure reports past any budget reaches the pool and the replay both: the write is replayed on a
+ * connection of its own, and the connections the pool established before the same drop are validated on their
+ * next borrow instead of being trusted for the rest of the alive window (issue #961). The driver has not closed
+ * the connection here - it is the chains of the failure, and only they, that say the database dropped it.
+ */
+ @Test
+ public void testADropDeepInTheChainReachesThePoolAndTheReplay() throws Exception
+ {
+ final long window = CachedConnection.aliveBypassNanos;
+ CachedConnection.aliveBypassNanos = TimeUnit.HOURS.toNanos(1);
+ try
+ {
+ final Connection pooled = mock(Connection.class);
+ when(pooled.isValid(anyInt())).thenReturn(true);
+ final Connection borrowed = mock(Connection.class);
+ when(borrowed.isValid(anyInt())).thenReturn(true);
+
+ final JDBCStorage storage = storageOver(pooled, borrowed);
+ // both are proven alive and back in the pool; the one released last is the one the write borrows
+ final Connection first = storage.getConnection();
+ final Connection second = storage.getConnection();
+ first.close();
+ second.close();
+ verify(borrowed, never()).isValid(anyInt()); // both are inside the window: nothing has validated
+
+ final AtomicInteger attempts = new AtomicInteger();
+ storage.write(txn -> {
+ if (attempts.incrementAndGet() == 1)
+ {
+ // the chain mssql-jdbc reports a message of many errors in, with the one that says the session is
+ // gone at the end of it - past the budget the walk used to stop at
+ throw new StorageRuntimeException(chainEndingIn(200, "08006"));
+ }
+ });
+
+ assertEquals(attempts.get(), 2, "a write the database dropped the connection under was not replayed");
+ // the borrow of the replay validated rather than trusting the last answer of a connection established
+ // before the drop: the pool was told
+ verify(borrowed, times(1)).isValid(CachedConnection.VALIDATION_TIMEOUT_SECONDS);
+ }
+ finally
+ {
+ CachedConnection.aliveBypassNanos = window;
+ }
+ }
+
+ /**
* The create index of the postgres branch is asked of the catalog first, although postgresql has "create index
* if not exists": that statement commits whether it creates anything or not, and unguarded it would take every
* write that opens a tree out of the replay - {@code RootContainer.open()} and its ~25 trees per suffix
@@ -1449,18 +1528,29 @@
return first;
}
- /** A chain of the given number of next exceptions whose last link is a connection that broke. */
- private static SQLException chainEndingInADrop(int links)
+ /** A chain of the given number of next exceptions whose last link carries the given SQLState. */
+ private static SQLException chainEndingIn(int links, String lastState)
{
final SQLException head = sql(2627, "23000");
SQLException tail = head;
for (int link = 2; link <= links; link++)
{
- tail = chained(tail, sql(0, link == links ? "08006" : "23000")).getNextException();
+ tail = chained(tail, sql(0, link == links ? lastState : "23000")).getNextException();
}
return head;
}
+ /** The given failure behind the given number of wrappers, the way a caller of this backend wraps one. */
+ private static Throwable wrappedTimes(int wrappers, Throwable failure)
+ {
+ Throwable wrapped = failure;
+ for (int wrapper = 0; wrapper < wrappers; wrapper++)
+ {
+ wrapped = new IllegalStateException("wrapped", wrapped);
+ }
+ return wrapped;
+ }
+
/** The second failure suppressed into the first, the way a failing close() joins the failure of an operation. */
private static SQLException suppressing(SQLException failure, SQLException onRelease)
{
--
Gitblit v1.10.0