[#879] Skip the validation of a pooled JDBC connection returned a moment ago

[vharseko]

Fixes #879

Every borrow from the pool of the JDBC backend validated the connection it took out, and Connection.isValid() is a round trip to the database — an empty query on postgresql, a ping on mysql, a round trip of its own on oracle and sql server. Every operation of the backend borrows: read(), write(), the cursor of a search, the import. A read of one entry therefore cost three exchanges with the database — the validation, the select, and the rollback that ends the transaction — of which one was the statement the operation came for.

Stacked on #876. This branch carries the connect/pool bounds of #876 underneath (currently 52ca42bf), because the window belongs inside the isUsable() that #876 introduced. Merge #876 first. It should also land no earlier than #884: see LIFO and the cold end of the pool below.

The alive window

A connection is handed out unvalidated while the last answer it gave is younger than org.openidentityplatform.opendj.jdbc.alive.bypass — 500 ms by default, 0 to validate every borrow as before — the way the aliveBypassWindow of HikariCP does it. Beyond the window, a connection that has been sitting in the pool is validated and, if it no longer answers, discarded and replaced exactly as before. The window is clamped to org.openidentityplatform.opendj.jdbc.ttl, the idle time the pool keeps a connection for, and says so once when it is configured higher: a connection trusted for longer than the pool holds it would never be validated at all.

What counts as an answer. The stamp is set when a connection is established — the login and the two round trips that set it up have just answered — and whenever it validates. It is deliberately not set on the way back into the pool. pgjdbc short-circuits both rollback() and commit() when the transaction state is IDLE (PgConnection.rollback: if (getTransactionState() != IDLE)), so a borrow that issued no statement puts a connection back without a byte reaching the server. Stamping that return would mark a connection the database had dropped meanwhile as the freshest one in the pool. Stamping proof rather than use makes the window mean "validated at most once per window", which is a claim the pool can always back.

LIFO handoff. The pool hands connections out from the end it takes them back at — a LinkedBlockingDeque instead of a LinkedBlockingQueue. Without it the window would rarely apply: a FIFO queue reaches a returned connection only after a whole cycle of the pool, and with a pool larger than the load that cycle is far longer than the window.

What happens to a connection that breaks inside the window

It is handed out, and the failure surfaces on the statement rather than on the borrow. That is where a connection breaking mid-operation surfaces anyway — but not every caller of this backend reports such a failure to the client, so the trade is not the caller's alone to bear. Three things take it off them:

• A write is replayed. write() already replays a transaction conflict; it now replays a connection the database dropped as well. The next attempt borrows a connection of its own. Only while the transaction has not been committed yet, though: a drop reported by commit() leaves the outcome unknown — the server may have committed and died before the answer reached us — and replaying a write that in fact committed applies it twice. That is the same reason 40003 is excluded from the conflicts.
• The pool is told. Both read() and write() mark the pool distrusted on such a failure, and every connection proven alive before that moment is validated once before it is trusted again. Whatever dropped one connection — a restart, a failover, a network that went away — dropped every connection established before it, and a borrow inside the window asks the database nothing, so the statement that broke is the only place a drop is ever seen. A validation that fails does not mark the pool: an idle connection the server reaped is a routine event and says nothing about the connection in use. The borrow itself is outside that rule — a connect the pool could not make (Connector/J reports a server at its connection limit as 08004, class 08 like a connection that broke) says nothing about the connections it holds, so it leaves the loop without distrusting anything.
• The borrows nothing compensates are validated. open(AccessMode), removeStorageFiles() and the ImporterImpl constructor ask for a connection the pool validates whatever the window says. They issue their statements far from the borrow, and the open issues none at all — a connection dropped inside the window would surface there out of the rollback() that releases it, with no statement to replay and nothing to tell the pool. Each is one borrow of a cold path.

Recognizing a dropped connection

A SQLState is not enough. mssql-jdbc reports a session killed by KILL, by the resource governor or by an availability group transition as error 596, 3980, 10054, 18456 or 4060, and generateStateCode maps none of them: with xopenStates off, which is its default, every one comes out as "S"+errorState — measured as S0001, indistinguishable from a rejected statement. SQLServerException is final ... extends SQLException, so no exception type tells them apart either.

What the driver does do is close the connection for any error of severity 20 and above before it throws, and Msg 596 is Level 21. So the connection is asked as well as the failure — while the operation that failed still owns it, since a released one is back in the pool and may already be another borrow's. Alongside that, SQLRecoverableException and the two connection exception types of the JDBC contract are matched (which is what makes the oracle mapping of ORA-03113/00028/01089 robust rather than lucky), and the walk covers getNextException() and getSuppressed() next to getCause() — a driver reports what happened as the next exception of a generic failure as readily as it reports it as the cause, and the drop of a close() arrives suppressed into the failure of the operation.

What is never replayed

An attempt that committed part of its own work, whatever the failure says. openTree, clearTree and deleteTree commit inside WriteOperation.run — and mysql and oracle commit before a DDL statement whether asked to or not — so the attempt no longer rolls back as a whole, while a WriteOperation is only idempotent in the database. RootContainer.open opens and registers the entry containers of every base DN in a single storage.write: replayed after the trees of the first base DN were created and committed, it registers that base DN a second time, fails with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED — masking the failure that caused the replay — and leaves the indexes of the previous attempt behind with the configuration listeners their constructors registered.

The rule covers any replay rather than only the drop replay added here: the conflict replay of #867 could already reach the same wall through a deadlock on DDL. That RootContainer.open passes a WriteOperation which is not idempotent, against what the contract asks of it, is a bug of its own and storage-agnostic — #896; this keeps the JDBC backend out of it. It costs nothing on the hot path — openTree(..., createOnDemand=true) is reached from the open of a backend, from an import and from a dsconfig that adds an index, never from an entry write.

That matters beyond one failed operation. A write of the replication replay that fails is recorded as applied — the ServerState advances past the change and the assured ack reports success — so a dropped connection there would cost a silently diverged replica rather than an error. That path is a pre-existing, storage-agnostic bug of its own — #889 — and this PR makes sure the JDBC backend does not walk into it.

LIFO and the cold end of the pool

expireAfterAccess sits on the pool entry, and every borrow and every return touches it, so the 15 s TTL only fires when the backend is fully idle for 15 s. With LIFO the connections below the working set are never borrowed, so nothing validates them and nothing reaps the dead ones — FIFO used to rotate them through. The pool held them all before as well, LIFO does not add connections; what it removes is the only mechanism that pruned dead ones. The per-connection idle expiry of #878 (#884) is what closes this, so this PR should not land before it.

Tests

CachedConnectionTestCase and JDBCStorageRetryTest, 104 methods, green, driven against mocked connections and the stub driver of #876:

• a connection proven alive is not validated again inside the window, and it is the same connection;
• one whose proof has aged past the window is validated, once; a window of 0 validates every borrow;
• the return to the pool is not taken for proof of life — the case the pgjdbc IDLE short-circuit creates;
• the connection returned last is the one borrowed first, and one the pool closed where it lay is not handed out on the strength of its last answer;
• after a drop is reported, the pool is validated once more and then trusted again; and the whole trade end to end;
• a borrow that asks for validation gets it inside the window, and the window is clamped to the ttl;
• the class 08, 57P0x, SQLRecoverableException, next-exception and suppressed cases are recognized through the wrappers they arrive in, while 53300, a deadlock and a bare S0001 are not;
• a killed session is recognized by the connection the driver closed, and replayed — but not from the commit phase;
• an attempt that committed part of its work is replayed for neither a conflict nor a drop;
• a dropped connection is replayable before the commit and not after it; a conflict is replayable from either phase.

PgSqlTestCase against postgres in docker: 54 methods, green.

[maximthomas]

The rewrite of getConnection() is a faithful port of the old loop (the con = null in the old catch was already dead code) and the static-init order is safe. The problem is that the bypass is on by default, and both comments justifying it are false on real paths.

Unvalidated connections can silently diverge replicas (major)

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

static final long DEFAULT_ALIVE_BYPASS_MS = 500;   // opt-out, not opt-in
...
if (bypassNanos > 0 && System.nanoTime() - con.lastKnownAliveNanos < bypassNanos) {
    return true;                                   // isValid() never called
}
return con.isValid(0);

An idle-connection reaper — SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE state='idle' AND state_change < now() - interval '...' — is harmless on master: PgConnection.isValid(int) returns false on a dead socket (its only throw path is timeout < 0), the old loop drains the pool, and DriverManager reconnects because the DB is still reachable. With this patch those same connections are handed out dead and their first statement fails.

On a replica that failure is not visible. StorageRuntimeException → BackendImpl.createDirectoryException → ResultCode 80 (OTHER). LDAPReplicationDomain.replay retries only NO_OPERATION / BUSY / UNAVAILABLE; OTHER falls into solveNamingConflict, which ends in:

// The other type of errors can not be caused by naming conflicts.
// Log a message for the repair tool.
logger.error(ERR_ERROR_REPLAYING_OPERATION, op, ctx.getCSN(), result, op.getErrorMessage());
return true;                                       // replayDone

replayDone → updateError(csn) → RemotePendingChanges.commit(csn) advances the ServerState unconditionally, and the RS resume cursor uses AFTER_MATCHING_KEY, so the change is never resent. replayErrorMsg stays null, so SAFE_READ acks the originating master as if it applied.

Net: silently lost changes, replica reporting fully caught up, unresolved-naming-conflicts at 0 (ModifyDN even increments the resolved counter), one log line. Recovery is a manual dsreplication initialize.

The replay bug itself is pre-existing and storage-agnostic — but this PR turns routine DB maintenance into a trigger for it.

Either fix closes this:

• default the window to 0 (opt-in), or
• evict the pool generation on SQLSTATE 08xxx. That is the piece of HikariCP's machinery this pool lacks: Hikari's window is safe because it hands the SQLException to application code that decides whether to retry — here the caller may be a replay path that records the failure as applied.

Liveness stamp is fabricated on zero-statement borrows (minor)

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

// Stamped after the rollback rather than before it: a transaction the operation opened
// ends in a round trip of its own, so a connection reaching the pool has just answered.
rollback();
lastKnownAliveNanos = System.nanoTime();
cached.get(connectionString).addFirst(this);

pgjdbc short-circuits both rollback() and commit() when the transaction state is IDLE — no bytes reach the server, nothing throws. So on borrows that issue no SQL the stamp proves nothing, and a dead connection goes back to the head of the deque marked alive (verified against postgres:16 + pgjdbc 42.7.12; reproduced repeatedly on the same connection).

Three such paths:

• JDBCStorage.open() — borrows and issues nothing
• BackendImpl.applyConfigurationChange() — its storage.write() body no-ops when the base-DN set is unchanged, so any dsconfig set-backend-prop on a live backend hits it
• ImporterImpl.close() with nothing imported

pgjdbc is the only one of the four bundled drivers that does this, and PostgreSQL is the default dialect. Fix: stamp only when the rollback/commit actually round-tripped, or have open() validate explicitly.

Virtual attribute reads fail silently (minor)

The other justifying comment — "A connection that broke inside the window surfaces as the failure of the statement itself" — does not hold for hasSubordinates / numSubordinates. Each is its own storage.read(), so it borrows its own connection while the search still holds one:

• EntryContainer.hasSubordinates / getNumberOfChildren → StorageRuntimeException
• → BackendImpl.createDirectoryException
• → swallowed in HasSubordinatesVirtualAttributeProvider / NumSubordinatesVirtualAttributeProvider, returning Attributes.empty(...)

The client gets the entry with the attribute missing and resultCode: 0. Filters on them evaluate FALSE, not UNDEFINED. Both providers are ds-cfg-enabled: true in the shipped opendj-server-legacy/resource/config/config.ldif.

Narrow — a plain ldapsearch with no attribute list never reaches this — but tree browsers and monitoring queries request exactly these.

LIFO handoff strands cold connections (minor)

expireAfterAccess sits on the pool entry, and both getConnection() and close() call cached.get(connectionString), so the 15 s TTL only fires when the backend is fully idle for 15 s. With LIFO, connections below the working set are never borrowed, never validated, never closed — a burst that opens 50 connections leaves 49 holding sockets and server-side sessions indefinitely. FIFO used to rotate them through, and isValid() reaped the dead ones.

The PR body defers this to #878, but #884 isn't merged — merging this first introduces the leak on its own. (Note the unbounded pool does not amplify the bypass window: cold connections carry stale stamps and are still validated.)

Nits

• aliveBypassNanos should be volatile: it is a non-final static long read from every backend worker and replay thread; a 64-bit non-volatile write is neither atomic (JLS 17.7) nor visible. The test writes it from the TestNG thread.
• Timing-dependent tests: connectionReturnedWithinTheWindowIsNotValidated and mostRecentlyReturnedConnectionIsBorrowedFirst set a 500 ms window and assert validations() == 0. Whichever runs first also pays for cold class loading, so a loaded CI fork can exceed the window and fail. Use TimeUnit.HOURS.toNanos(1) — the 1 ms window in the "beyond the window" tests is already the right shape.
• StubDriver can't model the failure: breakConnections() flips alive on the driver, not per connection, so the replacement in staleConnectionBeyondTheWindowIsReplaced is also "dead" and the test never checks it is usable. rollback() also proxies to a never-throwing default, which happens to mimic pgjdbc's IDLE no-op — so no test covers "borrow inside the window, connection is dead, close, borrow again", the case that exposes the fabricated stamp.

---

Two pre-existing bugs found while reviewing, both worth their own issues and neither blocking here:

1. The replay path dropping changes on OTHER while advancing the ServerState and sending a clean assured ack. Storage-agnostic — JE and PersistIt hit it on any StorageRuntimeException. Setting server-error-result-code to 52 does not fix it; the state advance is unconditional on every terminal path in replay().
2. isValid(0) never sets a network timeout and the default connection string sets no socketTimeout, so against a black-holed socket the drain loop can block for minutes per connection.

[vharseko]

Thanks — the whole chain you traced holds, I walked it line by line. The branch is rewritten on top of #876, since the window belongs inside the isUsable() that PR introduces, and every point below is answered in code.

1. Unvalidated connections and the replay path

Answered, but not by switching the window off. Two changes in JDBCStorage:

• write() now replays a connection the database dropped — SQLState class 08 plus 57P01/57P02/57P03, the list HikariCP evicts on — on a connection the next attempt borrows of its own. Only while the transaction has not been committed yet: a drop reported by commit() leaves the outcome unknown, and replaying a write that in fact committed applies it twice. The two phases are separated in the loop for exactly that.
• read() and write() both mark the pool distrusted on such a failure. Every connection proven alive before that moment is validated once before it is trusted again, so a dropped connection costs one operation rather than one per pooled connection. This is the piece of HikariCP's machinery you said the pool lacked; a failed validation deliberately does not mark the pool, since an idle connection the server reaped is routine and says nothing about the connection in use.

One correction on the trigger, though. An idle-connection reaper does not reach the window. For an idle session state_change is set when the last statement ended — here, by the rollback of close() that returned the connection to the pool — so a connection returned less than 500 ms ago has an idle time under the window and no state_change < now() - interval '…' predicate selects it. The connections such a reaper does kill are the ones that have been sitting in the pool, and those are past the window and still validated, exactly as on master. The same argument covers a pgbouncer or firewall idle timeout: all of them are seconds or minutes, not sub-second.

What genuinely reaches the window is an event that kills a connection within the window of its return — a restart, a failover, a network partition hitting a busy pool — and your finding #2, which was the one path that put a known-dead connection back at the head of the deque with a fresh stamp. That one is fixed below, and the two together are what made the scenario reachable at all.

The replay path itself — OTHER swallowed into replayDone, the ServerState advanced, a clean assured ack — is now #889; it is storage-agnostic, and nothing in this PR can fix it.

2. Liveness stamp fabricated on zero-statement borrows

Fixed, and it drove the design of the rest. The stamp is now set only by an answer the connection actually gave: when it is established, and whenever it validates. Never on the way back into the pool. So the window means "validated at most once per window" rather than "returned recently", which is a claim the pool can always back — and the pgjdbc IDLE short-circuit of rollback()/commit() stops mattering. testTheReturnToThePoolIsNotTakenForProofOfLife covers it: the proof of the login is aged past the window, the connection is returned by a borrow that issued nothing, and the next borrow still validates it and discards it.

The cost is that a connection in constant use is validated once per window instead of never — one round trip per 500 ms per connection, against one per operation before.

3. Virtual attribute reads

The comment that claimed a broken connection "surfaces as the failure of the statement itself" is rewritten: it now says the failure surfaces on the statement of the caller, that not every caller reports it to the client, and that the trade is therefore taken off them by the replay and the distrust above. In practice a hasSubordinates on a dropped connection now fails at most once — the next borrow validates.

4. LIFO and the cold end

Kept, since without it the window rarely applies, but the dependency is now written down in the code and in the PR description: this must not land before #884. I would put it slightly differently, though — the pool held every one of those 50 connections under FIFO as well; what LIFO removes is not the connections but the only thing that pruned the dead ones, since nothing borrows them and nothing validates them any more. The per-connection idle expiry of #878 is the fix, not something this PR should duplicate.

5. Nits

• aliveBypassNanos is volatile.
• The tests that must not validate use TimeUnit.HOURS.toNanos(1); only the ones that need the window to lapse use a short one, and those sleep past it, so a slow fork can only make them more correct.
• The stub is gone: the new tests use the mocked connections of CachedConnectionTestCase, so a connection breaks on its own rather than the whole driver, and the replacement is a different mock the assertion checks. The case you named — borrow inside the window, connection is dead, close, borrow again — is now testAConnectionDroppedInsideTheWindowIsHandedOutOnceAndThenValidated, end to end: it is handed out unvalidated (the cost of the window), the caller reports the drop, and the pool validates the rest of the generation instead of handing it out the same way.

6. isValid(0) with no network timeout

Already fixed in #876, which this branch now sits on: the validation runs under VALIDATION_TIMEOUT_SECONDS with a network timeout put on the socket around it, since isValid(n) is not a socket bound on every driver.

[maximthomas]

The design holds and every round-1 point is answered in code — the stamp-on-proof-only rule, the pre-commit-only replay and the distrust marker are all implemented as described. Two things still need changing: the compensation is absent on SQL Server, and the replay it leans on can re-run an operation that is idempotent in the database but not in Java. The rest are nits.

Note: this branch is stacked on e77c8f72, and #876 has since moved to 625e2f23 (+346/-83). It needs a rebase.

isConnectionFailure misses SQL Server session kills (Major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:966 classifies a lost connection by SQLState alone:

static boolean isConnectionFailure(Throwable t) {
    for (int hop=0; t!=null && hop<MAX_CAUSE_HOPS; t=t.getCause(), hop++) {
        if (t instanceof SQLException) {
            final String state=String.valueOf(((SQLException) t).getSQLState());
            if (state.startsWith(CONNECTION_FAILURE_CLASS) || CONNECTION_FAILURE_STATES.contains(state)) {

Measured against the pinned mssql-jdbc-13.4.0.jre11:

• SQLServerException is final ... extends java.sql.SQLException — not SQLRecoverableException, not SQLNonTransientConnectionException.
• xopenStates defaults to false (SQLServerDriverBooleanProperty.<clinit>).
• Socket path: terminate() picks 08006/08001, mapFromXopen turns both into 08S01 — class 08, caught.
• Server-error-token path: generateStateCode's default branch maps only 220/515/547/1205/2601/2627/2714/8152/208 and otherwise returns "S"+dbState. Measured S0001 for 596 (session in kill state), 3980, 10054, 18456, 4060.

So a KILL, a resource-governor kill or an AG transition gives neither the replay nor the distrust. Every in-window connection is handed out unvalidated, each first statement fails, the pool is never told — one failed client operation per pooled connection, which is what the distrust exists to prevent. On master every borrow validated and none of them failed. scopeOf at :623 would not catch it either.

Fix — reuse what this file already knows, which also makes the Oracle case robust rather than lucky (ojdbc8 happens to map ORA-03113/00028/01089 to 08006):

if (t instanceof SQLRecoverableException || t instanceof SQLNonTransientConnectionException
        || t instanceof SQLTransientConnectionException) {
    return true;
}

committing == false does not mean nothing was committed (Major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1186 commits on the transaction's own connection, inside writeOperation.run(txn), while committing is still false:

public void openTree(TreeName treeName, boolean createOnDemand) {
    if (createOnDemand) {
        if (!isExistsTable(treeName)) {
            try (final PreparedStatement statement=con.prepareStatement("create table "+...)){
                execute(statement);
                con.commit();          // <-- and again at :1197 for the postgres index

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:135 wraps PersistentCompressedSchema.load plus openAndRegisterEntryContainers — roughly 25 openTree(..., true) calls per suffix — in a single storage.write.

Two base DNs, fresh schema: the trees for base DN #1 are created, committed and registered; the connection drops while creating a tree for base DN #2. committing is false, so replayReason returns "a connection the database dropped" and the whole WriteOperation is replayed. openAndRegisterEntryContainers restarts at base DN #1 and RootContainer.java:193 throws:

EntryContainer ec = this.entryContainers.get(baseDN);
if (ec != null) {
    throw new InitializationException(ERR_ENTRY_CONTAINER_ALREADY_REGISTERED.get(...));
}

That is neither a conflict nor class 08, so it propagates: the backend fails to open and the real drop is masked. Each replay also leaves the previous attempt's AttributeIndex/VLVIndex in place without close() (EntryContainer.open:536/:550), leaking the config listeners their constructors register.

Database-side idempotence genuinely holds — isExistsTable guards the create, postgres uses if not exists, the data writes are upserts, and nextEntryID is an in-memory AtomicLong recomputed from getHighestEntryID each attempt. The non-idempotence is purely Java-side.

Fix: have openTree record that it committed and suppress the drop replay for that attempt, or do not commit mid-transaction. At minimum the replayReason javadoc should not claim a guarantee the code does not have. (#867's conflict replay could already reach this via a deadlock on DDL; this PR widens the trigger to any dropped connection.)

read() distrusts the pool when a new connect is rejected (Minor)

JDBCStorage.java:844 — the try-with-resources initializer is inside the try, so a borrow failure reaches the distrust call:

try(final Connection con=getConnection()) {
    return readOperation.run(new ReadableTransactionImpl(con));
} catch (Exception e) {
    distrustPoolOnConnectionFailure(e);

Connector/J 9.2.0 maps 1040 ER_CON_COUNT_ERROR ("Too many connections") to 08004, and CachedConnection either rethrows it raw (:412) or wraps it as SQLTimeoutException(msg, e) (:416) with the 08004 one cause hop down — matched either way. With MySQL at max_connections, every failed borrow re-stamps poolDistrustedAt (no latch), so every returning connection validates on its next borrow: an extra round trip against a server already refusing connections. 08004 is a rejected new connect and says nothing about the pooled ones — which is the rule CachedConnection.java:106-109 states and this breaks.

Not a regression against master (which validated every borrow anyway); the window just switches itself off under the load it exists for. Fix: scope the distrust to failures raised by the operation, not by the borrow.

A drop seen only on release never reaches the pool (Minor)

Two gaps in JDBCStorage.java:908:

} catch (Exception e) {
    if (e!=failure) { throw e; }     // (a) returns before the distrust call below
}
distrustPoolOnConnectionFailure(failure);

• (a) commit() succeeds, return runs, the implicit close() then raises 08006 from its rollback(). failure is still null, so e != failure and it is rethrown before the distrust. read() has no such guard and does distrust.
• (b) the operation throws, close() then raises 08006 — added via addSuppressed (JLS 14.20.3.1). Now e == failure so the distrust is called, but isConnectionFailure walks getCause() only. It also never walks getNextException(), unlike failureScope() at :614 in the same file, and CachedConnection.java:88 documents both chains.

Both are narrow — a connection dropped while idle throws on its first statement, which lands in the inner catch and gets both the distrust and the replay, and on pgjdbc (a) cannot happen at all since rollback() short-circuits while IDLE after a commit. Worth walking getSuppressed()/getNextException() anyway.

distrustPool's update is not atomic (Minor)

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

poolDistrustedAt.computeIfAbsent(connectionString, url -> new AtomicLong()).set(System.nanoTime());

• Lost update — A reads T1, B reads T2>T1, B sets T2, A sets T1. The distrust point moves backwards, so a connection proven at T1<p<T2 satisfies provenAt - distrusted.get() > 0 and is trusted although it predates B's drop.
• Torn publication — computeIfAbsent installs new AtomicLong() (value 0) before set() runs; a racing borrow reads 0 and provenAt - 0 > 0 holds.

Both collapse into one change:

poolDistrustedAt.merge(connectionString, System.nanoTime(), Math::max);

Three pool borrows get neither replay nor distrust (Minor)

JDBCStorage.java:163 open(AccessMode), :797 removeStorageFiles(), :1596 the ImporterImpl constructor. These are the only such sites — StampSession.newStampConnection (:380) goes to DriverManager, not the pool. A dead in-window connection at :163 issues no statement, and close() → rollback() reaches the server on mysql/oracle/mssql, throwing with no catch in open(). Pre-change, all three validated on borrow and discarded a dead connection.

Nits

• Deque lock swap: LinkedBlockingQueue has separate takeLock/putLock, so a borrow and a return proceed concurrently; LinkedBlockingDeque has a single ReentrantLock, so pollFirst/addFirst now serialise on the handoff path this PR set out to make cheaper. Dwarfed by the round trip removed, but the comment at CachedConnection.java:114-121 justifies LIFO without mentioning it.
• Unclamped window: CachedConnection.java:60 — getNonNegativeProperty accepts any non-negative long and toNanos saturates, so a large value disables validation permanently. Both sibling timeouts (:387, :391) are clamped. Nothing warns when the window exceeds TTL_PROPERTY (15 s).
• No isClosed() on the trusted path: CachedConnection.java:470 — isValid() used to be that check implicitly. The Caffeine removalListener closes connections it finds in the deque and the iterator is weakly consistent. Unreachable at defaults, live if the window is configured >= the TTL.
• Stamp taken after the round trip: CachedConnection.java:497 sets lastKnownAliveNanos after isValid() returns, so the effective window is the configured one plus validation latency. Optimistic, never conservative.
• poolDistrustedAt is never pruned, not even by the removalListener that disposes the pool for that key.
• Seeded-pool tests use the wrong end: CachedConnectionTestCase.java:348/366/390/409/651/652/678 still call add(), which on a Deque is addLast — the opposite end from the addFirst production returns to. Only testTheConnectionReturnedLastIsBorrowedFirst exercises the real path.
• testAWindowOfZeroValidatesEveryBorrow is non-discriminating: identical in body and outcome to the pre-change always-validate path. The other six new tests do flip when the change is reverted.
• committing is never tested as computed: it is only ever passed to replayReason as a literal, and that is the load-bearing half of the "only before commit" claim — see the second issue above.
• The pool wiring is untested: distrustPoolOnConnectionFailure is private, and the pool-key identity (:156 getConnection(config.getDBDirectory()) vs :986 distrustPool(...)) is asserted by nothing.
• Neither isKnownAlive edge is tested: age exactly == window, and provenAt == distrusted.
• Wrapper coverage claim: both wrapper rows in JDBCStorageRetryTest carry class-08 states (08006, 08003); no 57P0x is exercised through a wrapper though the description says so.
• The property is undocumented: nothing outside the source file names org.openidentityplatform.opendj.jdbc.alive.bypass, so an operator hitting stale-connection errors has no documented way to find =0. TTL_PROPERTY has the same gap.
• HikariCP comment: says "minus its two Sybase states"; three are dropped — 01002 as well.
• Read once at class init: unlike connect.timeout/pool.timeout, which are read per borrow — inconsistent within the same property family.
• Prose assertions: the three new replayReason tests assert on English returned by production code.

[vharseko]

Rebased on 52ca42bf, which is where #876 stands now, and every point is answered in code. Two of them not quite the way the review proposed — those two first.

1. isConnectionFailure misses SQL Server session kills

The measurement holds, and I reproduced it: with xopenStates off — its default — the lookupswitch of generateStateCode covers 208/515/547/1205/2601/2627/2714/8152 and nothing else, so everything outside that list comes out as "S"+errorState.

The proposed fix does not close it, though. SQLServerException is final ... extends java.sql.SQLException, exactly as the bullet above the fix says, so none of SQLRecoverableException, SQLNonTransientConnectionException or SQLTransientConnectionException ever matches on that driver. The three types are worth having — they are what makes the oracle case robust rather than lucky — but on their own the killed session still goes unrecognized.

What does close it is the other half of the same driver: SQLServerException.makeFromDatabaseError calls connection.close() for any error of severity 20 and above before it throws, and Msg 596 is Level 21. So the failure is no longer the only witness — the connection is asked as well:

static boolean isConnectionFailure(Throwable failure, Connection con) {
    return isConnectionFailure(failure) || isClosed(con);
}

Asked only while the operation that failed still owns the connection: once the release has returned it to the pool, another borrow may hold it and the driver would be answering about that one. write() takes the reading in its inner catch, read() in an inner try of its own.

The three types are in as well, and the walk now covers getNextException() and getSuppressed() next to getCause() — the way failureScope already walked both chains, and the way mssql-jdbc chains the errors of one message (setNextException, in the constructor that builds the error chain). That is finding 4(b) too.

2. committing == false does not mean nothing was committed

Confirmed all the way down — openTree at :1181, the single storage.write of RootContainer:135, the ERR_ENTRY_CONTAINER_ALREADY_REGISTERED of RootContainer:193, and the listeners the index constructors register (AttributeIndex:447, VLVIndex:144) that a replay leaves behind.

Fixed, but with the rule widened to any replay rather than to the drop replay only. WriteableTransactionTransactionImpl carries a partlyCommitted flag, set by openTree, clearTree and deleteTree before the work rather than after the commit — mysql and oracle commit before a DDL statement whether asked to or not, so a statement that fails has committed everything before it just as surely as one that succeeds — and replayReason returns null for such an attempt whatever the failure says.

Suppressing only the drop replay would have left the conflict replay of #867 walking into the same wall: a deadlock on DDL, the same RootContainer.open, the same already-registered failure. One rule covers both, and it costs nothing on the hot path — openTree(..., createOnDemand=true) is reached from the open of a backend, from an import and from a dsconfig that adds an index, never from an entry write.

RootContainer.open violating the idempotence WriteOperation is documented with is a bug of its own, storage-agnostic like #889 — filed as #896, with PersistIt reaching it through a plain rollback; this makes sure the JDBC backend does not walk into it.

3. read() distrusts the pool when a new connect is rejected

Fixed at the source rather than at the classification: read() and write() borrow outside their try, so a connect the pool could not make no longer reaches the distrust at all. Only a failure of the operation or of the release can mark the pool now, and 08004 keeps meaning what it says without the pool having to guess.

4. A drop seen only on release never reaches the pool

Both halves.

• (a) the outer catch of write() distrusts on a class 08 before it rethrows, so the drop of a close() after a successful commit reaches the pool — the write itself is still not replayed, since it is done.
• (b) the walk covers the suppressed chain, and a row of connectionFailures carries a class 08 suppressed into a plain constraint violation.

Only the chains are asked in that path, not the connection: it has been released by then, and whether it is closed is no longer that attempt's answer.

5. distrustPool's update is not atomic

poolDistrustedAt.merge(connectionString, System.nanoTime(), Math::max);

and the map holds a Long now rather than an AtomicLong that is published holding 0.

6. Three pool borrows get neither replay nor distrust

Given the compensation they can actually use: a borrow the pool validates. CachedConnection.getConnection(url, false) skips the window, and open(AccessMode), removeStorageFiles() and the ImporterImpl constructor take it. Distrust would not have helped there — the open issues no statement, so it has nothing to report a drop from; the drop surfaces out of the rollback() of its release, which is the failure of the open itself. Each of the three is one borrow of a cold path, so the round trip is exactly what master paid on every borrow, and only there.

Nits

Taken: the window is clamped to TTL_PROPERTY and says so once when it is asked for more (getAliveBypassMillis); isKnownAlive ends in !isClosed(con.parent), which is what the validation it replaces also answered; the single lock of the deque is in the comment that justifies LIFO; the HikariCP comment now names what it leaves out and why — 01002 and 0A000 besides the two Sybase states; the seeded pools of the tests fill the end close() returns to, through a seedPool helper that keeps the connection named first the one borrowed first; a 57P0x arrives through a wrapper in the data provider; partlyCommitted is unit-tested through replayReason; the bound of the walk is documented as covering all three chains.

Left alone, with a reason:

• the stamp taken after the round trip — optimistic, never conservative, which is the safe direction for a window;
• poolDistrustedAt is never pruned — one entry per connection string, so per backend;
• testAWindowOfZeroValidatesEveryBorrow — it documents the opt-out rather than a behaviour of its own, and it is the case an operator is told to reach for;
• prose assertions on replayReason — the strings are what the replay log prints, so a test that pins them is pinning something a user sees;
• read once at class init — deliberate, and the comment says why: the borrow is not the place to parse a property, and this one is read on every borrow of every backend;
• the property is undocumented — nothing in the repository documents ttl, connect.timeout or pool.timeout either; it belongs in the wiki, and I would rather add all four there in one go than half of one here;
• committing as computed — still only reachable with a database that drops the connection inside commit(); the half that was load-bearing here is partlyCommitted, and that one is tested.

Tests

CachedConnectionTestCase 51 + JDBCStorageRetryTest 53 — 104 methods, green. New with this round:

• testTheConnectionIsAskedWhetherTheDriverClosedIt — a killed session is recognized by the connection, a rejected statement on a live connection is not, class 08 needs no connection to say so, and a connection that cannot answer is taken as closed;
• testAConnectionTheDriverClosedIsADroppedOne — S0001 alone is not replayed, S0001 on a closed connection is, and not from the commit;
• testAnAttemptThatCommittedPartOfItsWorkIsNotReplayed — neither a conflict nor a drop;
• testTheBorrowsNothingCompensatesAreValidated — getConnection(url, false) validates inside the window;
• testAConnectionThePoolClosedIsNotHandedOut — closed where it lay, not handed out on its last answer;
• testTheWindowIsClampedToTheIdleTimeOfThePool — 500 left alone, Long.MAX_VALUE clamped to the ttl, and the clamp follows the configured ttl;
• plus the wrapper, type, next-exception and suppressed rows of connectionFailures, and the mssql S0001 row that documents what a state cannot tell.

PgSqlTestCase against postgres in docker — 54 methods, green.