[#879] Skip the validation of a pooled JDBC connection returned a moment ago by vharseko · Pull Request #883 · OpenIdentityPlatform/OpenDJ

vharseko · 2026-08-19T14:20:40Z

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, at 52ca42bf, because the window belongs inside the isUsable() that #876 introduced. #876 has since moved two commits further, to c588f0a1, so this needs a rebase onto it before it lands; nothing in those two commits touches the window. 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. It is clamped to an hour behind that, since the ttl has no upper bound of its own — with both set high enough the conversion to nanoseconds saturates, nanoTime() - provenAt >= window is never true again, and every connection of the pool stays trusted for the life of the server, which is the outcome the first clamp is there to rule out.

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.

The stamp stands for the moment the connection was asked, not the moment its answer was filed. It is compared against the distrust of the pool as an ordering of two moments, and nothing serializes the two: a validation is allowed five seconds, so a proof taken before another operation reported a drop and filed after it would be the younger of the pair, and the connection — at the head of the deque, where LIFO puts it — would go on being handed out unvalidated for the rest of the window by the check that exists to stop exactly that. Reading the stamp early only ever ages a proof, which costs a validation and never skips one.

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, before the connection goes back. Both read() and write() mark the pool distrusted on such a failure, from the catch that still owns the connection rather than after the release: a rollback that never reaches the server — pgjdbc with an IDLE transaction, the very case the stamp rule below is built around — leaves the connection poolable, so the release returns it to the head of the deque, and a borrow racing the report would be handed it unvalidated. 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. The rollback that unwinds a failed attempt joins its own failure to the one being unwound rather than discarding it: on a driver that reports a killed session as a plain vendor error, that rollback is the only place a class 08 is ever stated.

Which chains answer which question. The suppressed exceptions are read for a question about the connection and not for one about what the engine did with the transaction. replayReason() asks for a conflict before it asks the committing guard, so a class 40 contributed by the release — 40000 is not among the two states excluded from the conflicts — would re-authorise the replay of a commit() whose outcome nobody knows, and apply the write twice. The release runs after the outcome was decided and cannot speak for it.

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 flag is raised by the one helper every statement of the transaction that commits goes through, never once for a method that may issue one. Every one of those statements is guarded by a catalog read — including the create index if not exists of postgresql, which commits whether it creates anything or not — so on an existing backend openTree(name, true) issues nothing at all and the attempt stays replayable. Raising it on the catalog read instead would take a transaction the engine had rolled back whole out of the replay, and RootContainer.open() calls openTree ~25 times per suffix before anything is registered.

Which side of the statement the flag goes up on is the engine's answer. mysql and oracle commit before a DDL statement whether asked to or not, so there the work behind it is committed by the statement itself and the flag has to be up before it is issued: the statement that fails has committed everything before it just as surely as the one that succeeds. postgresql and sql server run DDL inside the transaction, and a DML statement commits of its own accord nowhere — one that fails there has committed nothing, write() rolls the attempt back whole, and a flag raised in front of it would take a conflict the engine itself undid out of the replay. On those the flag goes up in front of the commit instead, which is the call that leaves the outcome of the transaction unknown when it fails. So a statement that fails stays replayable and a commit that fails does not, on every engine.

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, 122 methods, green, driven against mocked connections and the stub driver of #876 — and, for the retry loop, through JDBCStorage.write() end to end rather than against its classifiers alone:

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, but is never read from the release of the connection;
a setting worth warning about still initializes the class — asserted on the class defined again through a loader of the test, since a static initializer runs once per loader;
opening a tree that already exists issues no statement and leaves the attempt replayable, while one that had to be created takes it out of the replay;
a drop stated only by the rollback that unwinds the attempt, or only by the release behind it, gets both the replay and the distrust — and the one about the release fails on the release rather than on the rollback of the attempt, which is what it claimed to pin;
a proof taken while the database was going away does not outlive the distrust that reported it: the connection is validated again on the next borrow rather than trusted;
the window is clamped by the hour as well as by the ttl, so a ttl nothing bounds cannot leave it saturating;
the postgres and mysql branches of openTree() are reached at all — the fixture asserts that the name of its mock names an engine, since driverNameOf() reads the class name and a mock of plain Connection matches no engine — and there a create index the engine rolled back is replayed while one mysql had committed before is not;
the budget of the walk of a failure is pinned from both sides: a drop on its last link is found and one link further is not;
a drop a read ran into reaches the pool, from the release of its connection as much as from the statement;
an unchecked failure of the comment session is suppressed into the failure being unwound rather than replacing it, so the replay still happens.

Each of the tests above fails on the code as it was before this branch changed it: checked by putting every one of those behaviours back and running the suite against it. StampConnectionTestCase covers the scope of a failed stamp read from the suppressed chain as well as from the cause and the next exception, 5 methods, green.

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:

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().
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 · 2026-08-20T10:58:24Z

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 · 2026-08-21T09:17:04Z

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.

maximthomas

issue (blocking): `warnedOnce` is declared below the initializer that reaches it

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

:68   static volatile long aliveBypassNanos = TimeUnit.MILLISECONDS.toNanos(getAliveBypassMillis());
      // ...65 lines...
:133  static final Set<String> warnedOnce = ConcurrentHashMap.newKeySet();

getAliveBypassMillis():190 reaches warnOnce() two ways — :194 when the window exceeds the ttl, and
:221 via getNonNegativeProperty, called for alive.bypass at :191 and for the ttl at
:192 → getCacheTtlMillis():180. warnOnce():229 does warnedOnce.add(key). JLS 12.4.2 runs
class-variable initializers in textual order, so warnedOnce is still null at :68. It compiles only
because the reference sits inside a method body, not in an initializer by simple name.

-Dorg.openidentityplatform.opendj.jdbc.alive.bypass=60000 — larger than the 15000 ms default ttl, which
is exactly the tuning the new javadoc invites — NPEs in <clinit>: ExceptionInInitializerError on first
touch, NoClassDefFoundError with no cause on every later one. No connection can be borrowed, so the
backend cannot open. Same for any non-numeric or negative value of either property.

The ttl half is a regression: at 52ca42bf warnedOnce was at :111 and the first getCacheTtlMillis()
call was the cache field at :116 — after it. -D...jdbc.ttl=30s merely logged the warning the comment at
:219 anticipates.

Fix: move the warnedOnce declaration above :68, or initialize aliveBypassNanos lazily.
testTheWindowIsClampedToTheIdleTimeOfThePool cannot catch this — it calls getAliveBypassMillis() long
after the class is initialized. Pinning it needs a fresh JVM/classloader with the property set.

issue (blocking): `partlyCommitted` is raised before any statement runs

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1329

if (createOnDemand) {
    partlyCommitted=true;              // :1329 — above the guard, before anything is issued
    if (!isExistsTable(treeName)) {    // :1331
        ... create table ...; con.commit();
    }
    ...
}

Steady state, table and index already present:

dialect	what `openTree(name,true)` issues	flag
postgres	`create index if not exists` + `con.commit()`, unconditional (`:1340`)	accurate
mysql	`isExistsIndex()` first (`:1348`) → nothing	wrong
oracle	same shape (`:1358`) → nothing	wrong
mssql	no index branch at all (`:1367`)	wrong

commentTable():1371 runs on StampSession's own connection, so it cannot commit on con.

replayReason returns null on the flag at :1019, before it tests isRetryableConflict at :1021,
so the flag suppresses #867's conflict replay as well as this PR's drop replay.

Restart of an existing backend on mssql/oracle/mysql: RootContainer:135 opens one storage.write whose
first act is PersistentCompressedSchema:143 openTree(ad, true) — flag set, nothing issued. The cursor
walk at PersistentCompressedSchema:148 deadlocks (1205 / ORA-00060); write():946 reads true,
replayReason returns null, and the backend fails to open on a transaction the engine had rolled back
whole with nothing registered. At 52ca42bf the only gate was !isRetryableConflict(failure, driver)
(:883), so that attempt was replayed — and could succeed, because :143..:186 runs entirely before the
first registerEntryContainer.

Fix: move the assignment down to each site that actually commits — inside if (!isExistsTable(...)) before
the create-table commit, and before each create-index statement (unconditional on postgres, inside the
isExistsIndex guard on mysql/oracle). The stated rationale — mysql and oracle commit implicitly before a
DDL statement — justifies setting it before the DDL, not before a catalog read that issues nothing.

Decoration revised to blocking after the verdict: this is a regression against the base branch, not a
missed improvement, and the fix lands in the file already being reopened for the issue above.

issue (non-blocking): `write()` replays a release drop it never distrusts

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:939

:939   dropped=isConnectionFailure(e,con);        // computed BEFORE the implicit close()
:953   if (e!=failure) {                          // skipped when close()'s 08 was SUPPRESSED into failure
:954       if (isConnectionFailure(e)) { distrustPool(); }
:956       throw e;
       }
:965   if (dropped) { distrustPool(); }           // stale flag

replayReason → isConnectionFailure(failure) does walk getSuppressed(), so the attempt is replayed on
evidence it never hands to the pool. read():881 is if (dropped||isConnectionFailure(e)) and has no such
hole. Round-2 finding 4(b) is closed in the classifier, not on write()'s call path — the answer claims
both halves. Self-heals on the next borrow.

Fix: if (dropped || isConnectionFailure(failure)) at :965.

issue (non-blocking): `partlyCommitted`'s computation is covered by nothing

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java:213

git grep partlyCommitted -- src/test returns zero hits. Every use is a boolean literal in
replayReason()'s 4th argument, so only the consumer is pinned; the three assignment sites (:1329,
:1392, :1404) and the partlyCommitted=txn.partlyCommitted read at :946 are not. Deleting the :1329
assignment keeps all 104 tests green — which is also why neither placement in the issue above would be
noticed. Round-2 [15] is not closed, though the answer says it is; round-2 [16] (nothing enters
read()/write()) is not closed either, and this round widened it.

suggestion (non-blocking): the clamp's javadoc argues something the code does not do

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

"a connection in constant use … validated not once per window but never" — expireAfterAccess is refreshed
by every borrow and every return of any connection in the entry, and isKnownAlive validates on age
alone, so a hot connection is still validated once per window. Also aliveBypassNanos is assigned once at
:68, so the clamp does not track a ttl set later — the third assertion of the clamp test describes a
re-read the field never does.

suggestion (non-blocking): `MAX_CAUSE_HOPS=16` now bounds three chains together

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1074 —
while (!pending.isEmpty() && seen.size()<MAX_CAUSE_HOPS). The sibling walk failureScope():629 has no
bound at all. mssql-jdbc chains every error of one message via setNextException, and that chain is pushed
last so it is popped first; a >16-link chain would spend the budget before reaching getCause(). Mechanism
only — no measured real chain length, so this may well be unreachable.

suggestion (non-blocking): the drop-replay log names the wrong SQLState

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:976 →
conflictSummary():1176 walks getCause() only. In the shape above the line reads "replaying the
transaction after a connection the database dropped … SQLState 23000" — the statement's state, not the 08
on getSuppressed() that caused the replay. That line is the only observable record of a drop replay.

nitpick (non-blocking): `Math::max` on raw `nanoTime`

CachedConnection.java:808 poolDistrustedAt.merge(cs, System.nanoTime(), Math::max), against the file's
own "the overflow safe form of the comparison" at :780 and :783. ~292 years of uptime; an internal
inconsistency rather than a bug.

nitpick (non-blocking): `!isClosed(con.parent)` inherits `isValid()`'s TOCTOU

CachedConnection.java:785 — checked before the hand-out, with nothing serialising the removalListener
against pollFirst. No regression, but it is described as answering what the validation it replaces
answered.

nitpick (non-blocking): `clearTree`'s `partlyCommitted` has no caller inside `write()`

JDBCStorage.java:1404. The only in-repo clearTree on this impl is ImporterImpl:2014, which borrows once
at :1745 and never enters the replay loop. Harmless; the comment asserts a protection nothing exercises.

nitpick (non-blocking): the test fixture does not reset `poolDistrustedAt` or the pool cache

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java:97 — safe
today only because every test builds a unique url.

note (non-blocking): `isClosed(con)` makes a pool eviction read as a database drop

JDBCStorage.java:875/:1042. The removalListener closes what it finds in a weakly consistent view of the
deque, so an eviction racing an in-flight operation yields dropped=true → a spurious distrustPool() plus
a full replay. Benign now that partlyCommitted guards the non-idempotent case.

note (non-blocking): neither edge of `isKnownAlive` is tested

CachedConnection.java:774 (>= window) and :778 (provenAt - distrusted <= 0), both rewritten this
round into the overflow-safe form. Flipping either comparison passes all 104 tests. Round-2 [17], untouched
and unmentioned in the answer.

note (non-blocking): a green test row documents a known gap as expected behaviour

JDBCStorageRetryTest.java:172 — { "mssql killed session", sql(596,"S0001"), false } pins the 1-arg
classifier, which is what read():881 and write():957 use on the release path, where the connection is
deliberately not asked. A SQL Server session killed by error token and first seen on the release therefore
still gets neither replay nor distrust.

note (non-blocking): what was verified closed

Round-2 [13] is genuinely fixed — seedPool fills with addFirst in reverse order and all seven old
add() sites plus one new go through it.
The "104 methods" count is accurate: 51 + (11 methods, 2 data-driven, 23+21 rows) = 53.
All six named new tests exist and are discriminating.
openTree(createOnDemand=true) is genuinely never reached from an entry write — entry writes go
BackendImpl → EntryContainer → txn.put/delete/update (:1418, :1487) and resolve tables from the
tree2table catalog. The assertion in the answer holds, which is what keeps the second issue above out of
the hot path.
No SPI, format or upgrade impact: partlyCommitted is a field of the private final
WriteableTransactionTransactionImpl; JE/PDB/Cassandra cannot see it.
Neither new overload pair is the CodeQL confusing-overload shape — both differ in arity, not in parameter
type — and the redactedCopy → redactedSqlCopy rename rebinds no call site.

vharseko · 2026-08-25T17:57:35Z

Round 3 answered in d9c1f3cc. Both blocking findings are fixed, both non-blocking issues with them, and the walk they share was reworked — which turned up a defect of its own that the round-3 review did not reach.

[1] `warnedOnce` below the initializer that reaches it — fixed

Confirmed exactly as described, including that it compiles only because the reference sits in a method body. Reproduced the shape standalone:

Exception in thread "main" java.lang.ExceptionInInitializerError
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.Set.add(Object)" because "T.warned" is null
	at T.<clinit>(T.java:5)

The declaration moved to :45, above every field whose initializer can reach warnOnce(), with the ordering rule written down next to it so it does not drift back.

The observation that testTheWindowIsClampedToTheIdleTimeOfThePool cannot catch this is right, and pinning it does need a fresh loader. testASettingWorthWarningAboutStillInitializesTheClass defines the class again through a loader that delegates everything else to the parent, then reads aliveBypassNanos off the reloaded class — four rows: a window longer than the ttl, a non-numeric window, a negative window, and a non-numeric ttl (the regression half). With the declaration moved back down, all four fail with ExceptionInInitializerError.

[2] `partlyCommitted` raised before any statement runs — fixed

The table is accurate; commentTable() does run on StampSession's own connection and cannot commit on con. The flag now sits at each site that actually commits.

One correction to the prescribed fix: it says "unconditional on postgres", and that leaves a real hole. create index if not exists commits whether it creates anything or not, so on the engine of every default deployment every openTree(name, true) would still raise the flag — and RootContainer.open() calls it ~25 times per suffix, so every write that opens a tree stays outside the #867 conflict replay there. The postgresql branch now takes the same isExistsIndex() guard mysql and oracle already used; PgSqlTestCase is green against postgres in docker with it.

[3] `write()` replays a release drop it never distrusts — fixed, and more than the line proposed

if (dropped || isConnectionFailure(failure)) at :965 closes the reported hole, and that is what I had first. Reworking the walk showed two things behind it:

The report was too late. Both loops computed dropped before the implicit close() but told the pool after it. A rollback that never reaches the server — pgjdbc with an IDLE transaction, which is the case the stamp rule in this PR is built around — leaves the connection poolable, so the release puts the dropped connection back at the head of the deque, and a borrow racing the report gets it unvalidated. Both read() and write() now report from the inner catch, before the release.

The rollback's own failure was thrown away. catch (SQLException ex) {} at :939 discarded the earliest and sometimes only statement that the connection is gone: on a driver that reports a killed session as a plain vendor error, the explicit rollback is the one place a class 08 appears at all, and without it the attempt fell back on isClosed() having flipped already. It is joined to the failure being unwound now. testTheRollbackOfAFailedAttemptIsNotSwallowed pins it — S0001 with isClosed() false, and the drop stated only by that rollback: it gets both the replay and the distrust.

[4] `partlyCommitted`'s computation covered by nothing — fixed

git grep partlyCommitted -- src/test returning nothing was right, and it is why neither placement was noticed. Round-2 [16] was open for the same reason.

JDBCStorageRetryTest now drives JDBCStorage.write() end to end against a stub driver and mocked connections, not just the classifiers: testOpeningAnExistingTreeLeavesTheAttemptReplayable (existing tree, no statement issued, conflict replayed), testCreatingATreeTakesTheAttemptOutOfTheReplay (create table committed, conflict not replayed), testADropReportedByTheReleaseReachesThePool and testTheRollbackOfAFailedAttemptIsNotSwallowed. Deleting the :1329 assignment now fails the first of them.

A defect the rework surfaced

Widening isRetryableConflict to the next-exception and suppressed chains — which is what makes it consistent with isConnectionFailure, and what round 3's MAX_CAUSE_HOPS note points at — is not safe on its own, and I had it wrong before this commit.

replayReason() asks for a conflict before the committing guard. commit() fails with an outcome nobody knows; the release then contributes a class 40 that is not 40002/40003 — 40000 is not in NON_REPLAYABLE_ROLLBACK_STATES — and the conflict branch returns first, past the guard that exists to stop exactly this. The write is replayed and applied twice.

So the shared walk takes a flag: the suppressed exceptions are read for a question about the connection, and not for a question about what the engine did with the transaction. The release runs after the outcome was decided and cannot speak for it. testAConflictIsNotReadFromTheReleaseOfTheConnection pins both halves.

The suggestions and nits

MAX_CAUSE_HOPS bounding three chains — raised to 64 and renamed MAX_CHAIN_LINKS. failureScope() is deliberately left unbounded rather than given the same cap: its verdict weakens under truncation — a SESSION past the budget comes back as TREE, which puts a tree in unstampableTrees for the life of the backend over a connection that broke — while the seen set already terminates it.
The drop-replay log naming the wrong SQLState — conflictSummary() now asks in the order replayReason() asks, and of the same chains, so the line names the link the decision was taken on.
The clamp javadoc — rewritten. You are right that a hot connection is validated once per window rather than never; the argument the clamp actually has is that a window longer than the ttl outlives the connection it was about. The note that aliveBypassNanos is assigned once is now stated in both the method and the test.
Math::max on raw nanoTime — replaced with the overflow safe form the rest of the file uses.

Not addressed, and why

Round-2 [17], neither isKnownAlive edge tested — still open. Both are System.nanoTime() comparisons, and a test that pins them without injecting a clock is timing-dependent, which is what round 1 rightly objected to. Injecting a clock is worth doing; it is not this PR.
LIFO stranding cold connections — unchanged, and the merge order in the description stands: not before [#878] Bound the JDBC connection pool and expire its connections one by one #884.
isClosed() TOCTOU, clearTree's flag having no caller inside write(), the green mssql killed session row — accurate as stated; each is a note rather than a defect, and I have left them as they are.

Full runs, all green: CachedConnectionTestCase 55, JDBCStorageRetryTest 60, PgSqlTestCase against postgres in docker 54. Reverting any of the three fixes above fails its own test and nothing else.

Still needs the rebase onto the current #876 before it can land.

maximthomas

Two blocking issues below, both introduced
or left by this commit; the rest are non-blocking.

Two candidates I chased and refuted — recorded so they don't come back:

A class-40 at commit() replaying past the committing guard. The branch is reachable
(partlyCommitted is false there), but a class-40 reaching the client from commit() means the
server processed it and answered "rolled back" — stated, not in doubt. 40003 is already excluded
at :97, and every genuinely in-doubt outcome arrives as class 08 / SQLRecoverableException /
driver-closed, which !committing covers. Not a hazard.
firstLinkMatching's LIFO drain spending MAX_CHAIN_LINKS on the next-exception chain before the
cause. The ordering and the arithmetic are exactly as they look. The shape isn't producible:
StorageRuntimeException is not an SQLException, so it has no next-chain to drain ahead of its
cause, and every wrap in the file is new StorageRuntimeException(e). Long next-chains come from
driver leaves, where the verdict is in the chain. No addBatch anywhere under backends/jdbc.

issue (blocking): the alive stamp can outlive the distrustPool() meant to invalidate it

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

The stamp is taken after the validation round trip, and compared against the distrust as a
timestamp ordering:

usable = con.isValid(VALIDATION_TIMEOUT_SECONDS);   // :737
...
con.lastKnownAliveNanos = System.nanoTime();        // :754

final long provenAt = con.lastKnownAliveNanos;                        // :782
final Long distrusted = poolDistrustedAt.get(con.connectionString);
if (distrusted != null && provenAt - distrusted <= 0) {               // :787

Thread A validates at T1; the database fails over at Te > T1; thread B's in-flight statement fails
and calls distrustPool() at T2; A writes the stamp at T3 > T2. The distrust is now older than the
proof and :787 does not fire. Nothing in the class serializes this — no synchronized, the borrow
holds no lock, and distrustPool() is a bare ConcurrentHashMap.merge called from worker failure
paths. Because the stamp is never rewritten on release, every borrow bypasses validation for the
full window until a real use fails.

Record when the proof started, not when it was filed:

final long provenAt = System.nanoTime();
usable = con.isValid(VALIDATION_TIMEOUT_SECONDS);
...
con.lastKnownAliveNanos = provenAt;

issue (blocking): on postgres partlyCommitted is raised before a statement that commits nothing when it fails

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1423

if (!isExistsIndex(tableName,"k_"+tableName.substring("opendj_".length()))) {
  partlyCommitted=true;                                   // :1423
  try (final PreparedStatement statement=con.prepareStatement("create index if not exists ...")){
    execute(statement);                                   // :1425 — executeUpdate, no commit of its own
    con.commit();                                         // :1426
  }
}

The pre-statement placement is justified by the mysql/oracle implicit pre-DDL commit (javadoc
:1344-1345). Postgres has no such thing: DDL is transactional, so a 40P01 at :1425 has
committed nothing and con.rollback() at :947 undoes the attempt entirely — but the flag still
reaches replayReason() through the finally at :972 and kills the replay at :1050. A
rollback-safe deadlock becomes a hard write failure, on the path RootContainer.open() walks ~25
times per suffix.

Fail-safe (a lost retry, never a double apply), but it defeats the guard's own purpose. On the
postgres branches, raise the flag after con.commit() returns.

issue (non-blocking): testADropReportedByTheReleaseReachesThePool does not exercise the release

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java:510

// the operation is rejected for its own reasons, and the release that follows it is where the drop surfaces
doThrow(new SQLException("connection reset", "08006")).when(released).rollback();

Unchained, so it fires on the attempt's own con.rollback() at JDBCStorage.java:948 — before any
release. dropped is true and distrustPool() runs in-catch at :964, i.e. the test pins the same
lines its sibling does.

Consequence: JDBCStorage.java:994 and its read() twin at :894 are dead in the suite — reaching
them needs the in-catch rollback to succeed and the release rollback to fail, and :462/:510 are
the only rollback stubs in the class. Nothing calls storage.read() at all. Deleting both branches
leaves the suite green. The relocation itself is also unpinned: verify(pooled).isValid(...) at
:521 passes under the pre-PR post-release placement too.

doNothing().doThrow(new SQLException("connection reset", "08006")).when(released).rollback();

The production code is right; the claim that this test pins it isn't.

issue (non-blocking): failureScope() was not moved onto the shared walk

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:624-657

It pushes getCause() and getNextException() only — no suppressed branch — under a new comment
aligning it with firstLinkMatching. A class 08 arriving only as a suppressed exception scores
TREE, not SESSION: the outcome that same comment says the unbounded walk exists to avoid. Latent
today (the only caller runs on the stamp connection), but two walks now exist that can drift.

issue (non-blocking): conflictSummary's last fallback contradicts its javadoc

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1244

Javadoc says "the first SQLException of the failure"; firstLinkMatching(failure, WITH_THE_RELEASE, e -> true) pops suppressed before cause and returns the release's exception. Log-only, reachable
when the replay was decided on isClosed(con) alone — so the one line a replay leaves names the
release's SQLState instead of the statement's, which is the mis-description the rewrite was for.
Make the last fallback cause-only, or correct the javadoc.

issue (non-blocking): the clamp does not prevent the saturation its javadoc names

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

The window is clamped to ttl, but ttl itself has no upper bound. Set both near Long.MAX_VALUE:
the clamp branch is not taken, toNanos saturates at :79, and System.nanoTime() - provenAt >= window at :783 is never true — every connection trusted for the life of the server, the exact
outcome the javadoc claims to rule out. Caffeine saturates expireAfterAccess rather than throwing.
testTheWindowIsClampedToTheIdleTimeOfThePool only ever puts an extreme value on the window.

issue (non-blocking): the finally that closes the stamp session can replace the failure being unwound

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:972

} finally {
  partlyCommitted=txn.partlyCommitted;
  txn.stampSession.close();   // :973 — an unchecked throw here replaces `e` outright
}

The write then reports the stamp-session failure instead of the drop, and is not replayed.
Pre-existing — flagged because moving distrustPool() into the inner catch fixed the neighbouring
half of exactly this and left this half. Suppress it into the original instead.

todo (non-blocking): the postgres index guard has no test

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1416-1428

driverNameOf(con) is con.getClass().getName(); a Mockito mock matches no engine, so openTree
returns before any index branch in both new write() tests — the test javadoc concedes it ("a mock
of no recognized driver"). The largest behavioural change in openTree, and the per-branch
partlyCommitted assignments, have zero coverage; verify(statements, never()).executeUpdate()
passes vacuously with respect to the index DDL. Stub getMetaData().getDriverName(), or use a
connection class whose name contains postgres.

todo (non-blocking): MAX_CHAIN_LINKS is unpinned in both directions

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:80

The deepest chain in any data provider is depth 3. Reverting 64 to 16 is undetectable by the suite.
One test with a 65-link chain closes it.

question (non-blocking): is getIndexInfo schema-qualified?

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1422

isExistsIndex passes a null schema, so a same-named table in another schema of a shared database
could satisfy the guard and the index would never be created where it is needed. Identifier folding
is not the concern — names are 63-char lowercase hex, no truncation, and postgres folds down.
I could not close this without a real multi-schema pgjdbc check.

praise: testASettingWorthWarningAboutStillInitializesTheClass genuinely pins the field move —
the loader really redefines the class, assertNotSame catches accidental parent delegation, and all
four rows throw ExceptionInInitializerError if warnedOnce moves back below aliveBypassNanos.
Rare to see a <clinit> ordering bug pinned at all.

vharseko · 2026-08-26T19:29:37Z

Both blockers are fixed and the rest with them, in 924b0d5f. One of the two not the way the review proposed, so that one first.

1. The alive stamp outliving the distrust

Fixed, and the race is exactly as traced — nothing serializes the two, and since the stamp is never rewritten on release the connection sits at the head of the deque and is handed out for the whole window.

provenAt is read before the round trip and written after it (CachedConnection.java:769,789). The same in connect() (:896,914), which had the same shape: the stamp of a login was filed once DriverManager.getConnection returned, so a login that answered before a failover and returned after the distrust that failover caused would have been the younger of the two. Reading a proof early only ever ages it, which costs a validation and never skips one; the field says so now.

testAProofTakenWhileTheDatabaseWentAwayIsNotTrusted: the validation reports a drop while it is still in flight — a mock whose isValid calls distrustPool — and the borrow after it validates instead of trusting.

2. `partlyCommitted` raised before a statement that commits nothing

The measurement holds: on postgres a 40P01 at the create index has committed nothing, con.rollback() undoes the attempt whole, and the flag killed a replay that was safe.

The proposed placement does not close it, though. Raising the flag after con.commit() returns leaves the in-doubt case replayable: a commit() that throws leaves the outcome unknown — the server may have committed and died before the answer arrived — and with the flag still down the whole WriteOperation is replayed over work that may be committed. The committing guard does not cover it either: that flag is about the commit write() itself issues, and this commit is inside WriteOperation.run.

The flag goes up in front of the commit instead, which answers both halves: a statement that fails on an engine that has not committed yet is replayable, and a commit that fails is not.

And it is one place now. Every statement of the transaction that commits goes through commitStatement(sql, ddl) (JDBCStorage.java:1376), which raises the flag before the statement only where the engine commits before it — commitsBeforeDdl(), mysql and oracle — and before the commit on every engine. That covers the create table, the three create index branches, deleteTree, and clearTree, whose delete from commits of its own accord nowhere and was over-conservative in the same way.

One note on the reach: the index branch raises the flag only when the index has to be created, so what it costs today is the first open after an upgrade of an existing deployment — where every tree of the suffix is missing its k_<hash>, and the write that opens them is the one RootContainer.open() runs. New backends reach it with the flag already up from the create table.

testACreateIndexPostgresRolledBackLeavesTheAttemptReplayable and testACreateIndexMysqlCommittedBeforeTakesTheAttemptOutOfTheReplay pin the two sides.

3. `testADropReportedByTheReleaseReachesThePool` did not exercise the release

Right on both counts — the unchained stub fired on the in-catch rollback and pinned the same lines as its sibling. doNothing().doThrow(...) now, and verify(released, times(3)).rollback() names all three: the return that seeded the pool, the rollback of the attempt, and the release that reports.

The read() twin at :894 was dead for a simpler reason — nothing in the class called storage.read() at all — so testADropAReadRanIntoReachesThePool covers it: a read rejected for its own reasons, the drop stated only by the rollback of its release, and the borrow behind it validating instead of trusting.

4. `failureScope()` was not moved onto the shared walk

Moved onto it rather than given a suppressed branch of its own, since two walks that can drift was the actual finding: failureScope is now two passes of firstLinkMatching — SESSION, then MOMENT — and cannot read a chain the classifiers of a write act on. The walk takes the number of links it may look at, so the property that comment claims is kept explicitly: EVERY_LINK there, MAX_CHAIN_LINKS for the classifiers, since a SESSION past a budget would come back as TREE. The suppressed case is in testFailureScopeWalksEveryChain.

5. `conflictSummary`'s last fallback

Cause-only (WITHOUT_THE_RELEASE), which is what its javadoc says and what a replay decided on isClosed(con) alone needs to name.

6. The clamp did not prevent the saturation its javadoc names

MAX_ALIVE_BYPASS_MS — an hour — clamps the window behind the ttl, which has no upper bound of its own. A compile-time constant, so it cannot repeat the initializer-order bug of the last round wherever it is read from, and the ttl case has its own warning. testTheWindowIsClampedToTheIdleTimeOfThePool now sets both properties to Long.MAX_VALUE.

7. The `finally` closing the stamp session

Suppressed into the failure being unwound. The other half is the success path, where failure is null: there an unchecked throw turned a transaction that had just committed into a failure of its own, so it is logged at trace — the way StampSession.close() already handles the checked case, and for the same reason: the stamp is a diagnostic aid.

8. The postgres index guard has no test

The reason it had none is the one named: a Mockito mock of Connection matches no engine. Two mocked types named for their engines now (postgresConnection, mysqlConnection — lowercase, since the match is case sensitive and the name of a mock is derived from the type it mocks), and the fixture asserts that driverNameOf(mock) names the engine, so a change in Mockito's naming fails the test instead of silently skipping the branch again.

9. `MAX_CHAIN_LINKS` is unpinned in both directions

testTheWalkOfAFailureStopsAtItsBudget: a drop on link 64 is found, a drop on link 65 is not.

10. Is `getIndexInfo` schema-qualified?

It is not, and it is wider than the index: isExistsTable passes a null schema as well, and the table name is a hash of the tree name, so two instances in two schemas of one database hold the same names. The index case is the silent one — the create is skipped and the cursor batches run unindexed — the table case fails loudly on the first statement. Neither is changed by this branch, so it is #902 rather than a commit here; what a fix has to deal with per engine is written down there.

On the tests. Every one of the tests above was checked against the behaviour it pins: each of the nine changes was put back in turn, in a single patch, and exactly nine tests failed — one per change, none of them anyone else's. CachedConnectionTestCase and JDBCStorageRetryTest, 122 methods green; StampConnectionTestCase, 5.

On the rebase. This still sits on 52ca42bf, and #876 has moved two commits further to c588f0a1 — neither touches the window, and the rebase is due before this lands.

One thing I found here and did not fix. ImporterImpl borrows once and holds that connection for the whole import, and unlike read() and write() it never tells the pool about a drop: it got the getValidatedConnection() half of the compensation and not the distrustPool() half. A database restarted mid-import therefore leaves the rest of that generation trusted for the window. Say if you would rather have it in this PR than in one of its own.

… as its last answer holds Every borrow from the pool validated the connection it took out, and Connection.isValid() is a round trip of its own - an empty query on postgresql, a ping on mysql, a round trip on oracle and sql server. Every operation of this backend borrows, so a read of one entry 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. A connection is now 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. The pool hands connections out from the end it takes them back at, a LinkedBlockingDeque rather than a LinkedBlockingQueue: with FIFO the connection borrowed next is the one reached after a whole cycle of the pool, which has been idle far longer than the window. What proves a connection alive is an answer it actually gave: it is stamped when established and whenever it validates, never on its way back into the pool. pgjdbc short-circuits both rollback() and commit() when the transaction state is IDLE, so a borrow that issued no statement - JDBCStorage.open(), a configuration change that leaves the base DNs alone, an import of nothing - returns a connection without a byte reaching the server, and stamping that return would mark a connection the database had dropped as the freshest one in the pool. A connection that breaks inside the window no longer costs the operation: - JDBCStorage.write() replays it on a connection the next attempt borrows of its own, on SQLState class 08 and on the 57P0x states postgresql announces a connection it is about to drop with - but only while the transaction has not been committed yet, since a drop reported by commit() leaves the outcome unknown and replaying a write that in fact committed applies it twice; - read() and write() both mark the pool distrusted on such a failure, so every connection proven alive before the drop is validated once before it is trusted again. Whatever dropped one connection dropped the whole generation, and a borrow inside the window asks the database nothing - the statement that broke is the only place a drop is ever seen. A failed validation does not mark the pool: an idle connection the server reaped is a routine event. That second point is what makes the window safe to leave on by default. A write of the replication replay that fails is recorded as applied - the ServerState advances past the change and the assured ack reports success, see OpenIdentityPlatform#889 - so a dropped connection there would cost a silently diverged replica rather than an error.

…er, not only by its SQLState, and the rest of what the review found open isConnectionFailure classified a lost connection by SQLState alone, and mssql-jdbc carries none: SQLServerException extends SQLException directly, xopenStates is off by default, and generateStateCode maps neither 596 (session in kill state) nor 3980, 10054, 18456 or 4060 - every one of them comes out as "S"+errorState, measured as S0001. A KILL, a resource governor kill or an availability group transition therefore gave neither the replay nor the distrust, and the window handed out the rest of that generation unvalidated, one failed operation per pooled connection. What the driver does do is close the connection for any error of severity 20 and above, before it throws, so the connection is now asked as well as the failure - while the operation that failed still owns it, since a released one may already be another borrow's. The types the JDBC contract gives a driver to say so are matched too, which is what makes the oracle case robust rather than lucky, and the next-exception and suppressed chains are walked with the causes, the way failureScope already walked both. An attempt that committed part of its own work is no longer replayed at all. 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 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, masks the failure that caused the replay and leaves the indexes of the previous attempt behind with the configuration listeners their constructors registered. The conflict replay of OpenIdentityPlatform#867 could already reach this, so the rule covers any replay rather than only the drop added here. read() and write() borrow outside their try, so that a connect the pool could not make no longer distrusts the pool: Connector/J reports a server at its connection limit as 08004, which is class 08 like a connection that broke, and every failed borrow re-stamped the distrust - an extra round trip per returning connection against a server already refusing connections. A drop reported by the release of a connection now reaches the pool from write() as well, which returned before the distrust call. The three borrows nothing compensates - open(), removeStorageFiles() and the importer - 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, so a connection dropped inside the window surfaced out of the rollback that released it, with nothing to replay it and nothing to tell the pool. One round trip on a path taken once per open, per import or per removal. distrustPool merges its reading with max instead of setting an AtomicLong published holding its initial 0, so that two operations reporting a drop at once cannot move the distrust point backwards. The window is clamped to the ttl an idle pooled connection is kept for, and says so once when it is asked for more: a value the unit conversion saturates on would leave every connection trusted for the life of the server. A connection the pool closed under the borrow - the removal listener iterates a weakly consistent view - is no longer handed out on the strength of its last answer. Also: the comment crediting HikariCP's list undercounted what it leaves out, the seeded pool of the tests filled the end production does not return to, and the bound of the walk covers all three chains.

…zer that reaches it, and commit the flag with the statement Three findings of the third review round. warnedOnce sat below aliveBypassNanos, whose initializer reaches warnOnce() through both properties it reads: class variable initializers run in textual order (JLS 12.4.2), so any value worth a log line - a window longer than the ttl, which is the tuning the javadoc of the property invites, or a non-numeric or negative value of either property - left the class uninitializable. The first borrow got an ExceptionInInitializerError and every one after it a causeless NoClassDefFoundError, so no connection could be borrowed and the backend could not open at all. The ttl half of that was a regression against the base branch. openTree() raised partlyCommitted for the whole method, before the catalog read that most often decides no statement is needed. On an existing backend on mysql, oracle and mssql it issues nothing, so the flag took a transaction the engine had rolled back whole out of the replay - the conflict replay of OpenIdentityPlatform#867 included, since replayReason() reads it before it asks anything else. It is raised at each site that actually commits instead, and the create index of postgresql is now guarded by the catalog read the other engines already used: unguarded it commits on every openTree(), which took every write that opens a tree out of the conflict replay on the engine of every default deployment. write() computed the drop flag before the implicit close(), so a drop the release reported - suppressed into the failure being unwound rather than replacing it - was replayed on evidence the pool was never told about. Both loops now report the drop from the inner catch, before the release returns the connection to the head of the pool where a borrow racing the report would be handed it unvalidated; and the rollback that unwinds a failed attempt joins its own failure to the one being unwound rather than dropping it, since on a driver that reports a killed session as a plain vendor error that rollback is the only place the drop is ever stated. Alongside: the classifiers share one walk of the failure, which reads the suppressed exceptions for a question about the connection and not for a question about what the engine did with the transaction - the release runs after the outcome was decided and cannot speak for it, and a class 40 raised there would otherwise re-authorise the replay of a commit left in doubt. The walk of failureScope() is left unbounded, since its verdict weakens under truncation rather than merely going unnoticed. The replay log names the failure the replay was decided on. The distrust point is merged with the overflow safe comparison the rest of the file uses, and the clamp javadoc argues what the code does. CachedConnectionTestCase and JDBCStorageRetryTest, 115 methods, green - the writes now run through JDBCStorage.write() against a stub driver rather than against the classifiers alone. PgSqlTestCase against postgres in docker, 54 methods, green.

… asked, and raise the commit flag on the side each engine commits on The stamp of a validation was filed once the answer was in, so a proof that started before another operation reported a drop could be younger than the distrust it is compared against - nothing serializes the two - and the connection went on being handed out unvalidated for the rest of the window, from the head of the deque, by the very check that exists to stop that. It is read before the round trip now, in the borrow and in the connect alike: reading it early only ever ages a proof, which costs a validation and never skips one. partlyCommitted was raised in front of every statement that commits, which is what mysql and oracle need and what postgresql and sql server do not: there DDL runs inside the transaction, so a create index the engine rolled back had committed nothing and write() rolls the attempt back whole - and the flag took it out of the replay all the same, on the path RootContainer.open() walks for every tree of a suffix the first time a backend is opened after the index was added. Every such statement now goes through commitStatement(), which raises the flag before the statement only on the engines that commit before it, and before the commit on all of them: a statement that fails is replayable, a commit that fails is not. Also from the review: * failureScope() reads the suppressed exceptions, by sharing the walk of the other classifiers rather than keeping one of its own - a class 08 arriving from the close() of a statement scored TREE, which left the tree unstamped for the life of the backend. * The last fallback of conflictSummary() names the statement rather than the release behind it, as its javadoc says: the walk reaches the suppressed exceptions before the cause. * The alive window is bounded by MAX_ALIVE_BYPASS_MS as well as by the ttl it is clamped to. The ttl has no upper bound of its own, and with both saturating the conversion to nanoseconds every connection of the pool stayed trusted for the life of the server - the outcome the clamp is documented to rule out. * An unchecked failure of the comment session no longer replaces the failure being unwound: it is suppressed into it, so the replay is still decided on the failure that explains it, and a transaction that has just committed is not reported as one that failed. Tests: the postgres and mysql branches of openTree() are reached at all - the fixture asserts that the name of its mock names an engine - the budget of the walk is pinned from both sides, the drop a read runs into reaches the pool, and testADropReportedByTheReleaseReachesThePool now fails on the release rather than on the rollback of the attempt, which is what it claimed to pin. Every one of these fails on the code as it was: checked by putting each behaviour back.

…its connections one by one Nothing limited how many connections a backend opened. The pool was an unbounded queue behind a cache with no maximum size, so a burst of concurrent operations opened as many connections as there were threads asking, and the only ceiling left was the max_connections of the database itself. The TTL sat on the pool rather than on a connection - expireAfterAccess keyed by the connection string, reset by every borrow and every return - so under continuous traffic nothing ever expired and the peak of a burst stayed open for as long as the backend saw any traffic at all. Caffeine was left without a scheduler besides, so an entry was only ever expired by a later cache operation, and a backend gone idle - the one case the TTL exists for - performs none. The cache entry is replaced by a Pool per connection string: a permit per live connection sized once from the new pool.max property, an idle deque handed out from the end it is returned to, and a sweep of its own that closes what nothing has borrowed for the TTL. The pool is reference counted by the storages using it, since it belongs to a database rather than to a backend and two backends may address one. Rebased onto master, which has since taken OpenIdentityPlatform#876, OpenIdentityPlatform#880 and OpenIdentityPlatform#883. Four points of contact this had to decide rather than merge: - pollIdle() is told whether the borrow trusts the alive window of OpenIdentityPlatform#879 and asks isUsable() as master wrote it. The pool of this change and that window are one layer, not two. - A connection carries the poolable flag of OpenIdentityPlatform#872, the proof of life of OpenIdentityPlatform#879 and the pool, the meter and the permit of this change together. close() sends a connection that may not be pooled through Pool.destroy() rather than closing it, or the bound would lose a place for every one of them. - getValidatedConnection() and distrustPool() of OpenIdentityPlatform#879 read db-directory again where this change had already established that the string open() registered with is the one to use: a db-directory changed on a running backend sent each of them to a pool holding none of this storage's connections. All three go through poolKey() now. - A nested borrow takes a permit when the pool has room, and goes on unmetered only when the pool stands at its bound. The exemption a nested borrow carries is from the wait rather than from the pool, which is what Pool.held has always documented; the code exempted it from both, so a connection borrowed inside another was never pooled again and the pool stopped handing out the connection returned last. Not carried over: the validation of a pooled connection is not clamped to what is left of the borrow. Master bounds it at the socket instead, and the pollIdle loop checks the deadline after each connection it discards, so a drain overruns by at most the one validation it is inside - which is what master does today. CachedConnectionTestCase: 83/83.

…ain, and take every borrow through one method The write transaction of a read-only storage stopped throwing when OpenIdentityPlatform#874 replaced the refusal in its constructor with a check per operation - which RootContainer.open() needs, and which an import does not: the merge with master left startImport() handing out an importer for a storage that is not writeable, one that would take a connection, begin its transaction and fail at the first tree it clears rather than at its start. An import writes by definition, so ImporterImpl refuses such a storage where it is built, which is where it was refused before that change. What reaches it is a storage that was already open: import-ldif and rebuild-index close it first, and startImport() opens a closed one READ_WRITE. And both borrows of this storage go through getConnection(boolean): OpenIdentityPlatform#883 moved startImport() onto getValidatedConnection(), which a test standing in for getConnection() no longer intercepted, so the two tests of that path reached for a real database - the connection string of a mock configuration - instead of the connection they had prepared.

vharseko requested a review from maximthomas August 19, 2026 14:20

vharseko added enhancement jdbc performance Performance / concurrency / lock-contention work labels Aug 19, 2026

maximthomas requested changes Aug 19, 2026

View reviewed changes

vharseko mentioned this pull request Aug 20, 2026

Replication replay records a failed operation as applied: the ServerState advances past the change and the assured ack reports success #889

Open

vharseko force-pushed the issues/879-jdbc-alive-bypass branch from 10698d1 to 6a2fa82 Compare August 20, 2026 10:58

vharseko requested a review from maximthomas August 20, 2026 11:26

maximthomas requested changes Aug 21, 2026

View reviewed changes

vharseko force-pushed the issues/879-jdbc-alive-bypass branch from 6a2fa82 to 7bc9294 Compare August 21, 2026 09:15

vharseko added java Pull requests that update java code concurrency Thread-safety / race-condition bugs tests Test suites: fixing, enabling, un-disabling labels Aug 21, 2026

vharseko requested a review from maximthomas August 21, 2026 09:18

vharseko mentioned this pull request Aug 21, 2026

A replayed WriteOperation registers an entry container twice: RootContainer.open fails with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED and masks the failure that caused the replay #896

Open

github-advanced-security Bot found potential problems Aug 21, 2026

View reviewed changes

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

maximthomas requested changes Aug 25, 2026

View reviewed changes

vharseko requested a review from maximthomas August 25, 2026 17:59

maximthomas requested changes Aug 26, 2026

View reviewed changes

vharseko mentioned this pull request Aug 26, 2026

JDBC backend: the catalog reads that guard its create table and create index are not narrowed by schema, so an object of another schema suppresses the create #902

Open

vharseko requested a review from maximthomas August 26, 2026 19:29

maximthomas approved these changes Aug 28, 2026

View reviewed changes

vharseko added 4 commits September 1, 2026 14:13

vharseko force-pushed the issues/879-jdbc-alive-bypass branch from 924b0d5 to 71490e2 Compare September 1, 2026 13:45

vharseko merged commit 0d16f10 into OpenIdentityPlatform:master Sep 1, 2026
13 checks passed

vharseko deleted the issues/879-jdbc-alive-bypass branch September 1, 2026 13:47

This was referenced Sep 1, 2026

[#888] Name the trees of a JDBC backend from a catalog in the database #893

Open

[#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882

Merged

[#873] Give each backend its own compressed schema trees #881

Merged

vharseko mentioned this pull request Sep 1, 2026

[#878] Bound the JDBC connection pool and expire its connections one by one #884

Open

Navigation Menu

Uh oh!

Uh oh!

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

[#879] Skip the validation of a pooled JDBC connection returned a moment ago#883
vharseko merged 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:issues/879-jdbc-alive-bypass

vharseko commented Aug 19, 2026 •

edited

Loading

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Aug 20, 2026

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Aug 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Aug 25, 2026

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Aug 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

vharseko commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The alive window

What happens to a connection that breaks inside the window

Recognizing a dropped connection

What is never replayed

LIFO and the cold end of the pool

Tests

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

Unvalidated connections can silently diverge replicas (major)

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

Virtual attribute reads fail silently (minor)

LIFO handoff strands cold connections (minor)

Nits

Uh oh!

vharseko commented Aug 20, 2026

1. Unvalidated connections and the replay path

2. Liveness stamp fabricated on zero-statement borrows

3. Virtual attribute reads

4. LIFO and the cold end

5. Nits

6. isValid(0) with no network timeout

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

isConnectionFailure misses SQL Server session kills (Major)

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

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

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

distrustPool's update is not atomic (Minor)

Three pool borrows get neither replay nor distrust (Minor)

Nits

Uh oh!

vharseko commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. isConnectionFailure misses SQL Server session kills

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

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

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

5. distrustPool's update is not atomic

6. Three pool borrows get neither replay nor distrust

Nits

Tests

Uh oh!

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

issue (blocking): warnedOnce is declared below the initializer that reaches it

issue (blocking): partlyCommitted is raised before any statement runs

issue (non-blocking): write() replays a release drop it never distrusts

issue (non-blocking): partlyCommitted's computation is covered by nothing

suggestion (non-blocking): the clamp's javadoc argues something the code does not do

suggestion (non-blocking): MAX_CAUSE_HOPS=16 now bounds three chains together

suggestion (non-blocking): the drop-replay log names the wrong SQLState

nitpick (non-blocking): Math::max on raw nanoTime

nitpick (non-blocking): !isClosed(con.parent) inherits isValid()'s TOCTOU

nitpick (non-blocking): clearTree's partlyCommitted has no caller inside write()

nitpick (non-blocking): the test fixture does not reset poolDistrustedAt or the pool cache

note (non-blocking): isClosed(con) makes a pool eviction read as a database drop

note (non-blocking): neither edge of isKnownAlive is tested

note (non-blocking): a green test row documents a known gap as expected behaviour

note (non-blocking): what was verified closed

Uh oh!

vharseko commented Aug 25, 2026

[1] warnedOnce below the initializer that reaches it — fixed

[2] partlyCommitted raised before any statement runs — fixed

[3] write() replays a release drop it never distrusts — fixed, and more than the line proposed

[4] partlyCommitted's computation covered by nothing — fixed

A defect the rework surfaced

The suggestions and nits

Not addressed, and why

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

Uh oh!

vharseko commented Aug 19, 2026 •

edited

Loading

6. `isValid(0)` with no network timeout

`isConnectionFailure` misses SQL Server session kills (Major)

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

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

`distrustPool`'s update is not atomic (Minor)

vharseko commented Aug 21, 2026 •

edited

Loading

1. `isConnectionFailure` misses SQL Server session kills

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

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

5. `distrustPool`'s update is not atomic

issue (blocking): `warnedOnce` is declared below the initializer that reaches it

issue (blocking): `partlyCommitted` is raised before any statement runs

issue (non-blocking): `write()` replays a release drop it never distrusts

issue (non-blocking): `partlyCommitted`'s computation is covered by nothing

suggestion (non-blocking): `MAX_CAUSE_HOPS=16` now bounds three chains together

nitpick (non-blocking): `Math::max` on raw `nanoTime`

nitpick (non-blocking): `!isClosed(con.parent)` inherits `isValid()`'s TOCTOU

nitpick (non-blocking): `clearTree`'s `partlyCommitted` has no caller inside `write()`

nitpick (non-blocking): the test fixture does not reset `poolDistrustedAt` or the pool cache

note (non-blocking): `isClosed(con)` makes a pool eviction read as a database drop

note (non-blocking): neither edge of `isKnownAlive` is tested

[1] `warnedOnce` below the initializer that reaches it — fixed

[2] `partlyCommitted` raised before any statement runs — fixed

[3] `write()` replays a release drop it never distrusts — fixed, and more than the line proposed

[4] `partlyCommitted`'s computation covered by nothing — fixed

2. `partlyCommitted` raised before a statement that commits nothing

3. `testADropReportedByTheReleaseReachesThePool` did not exercise the release

4. `failureScope()` was not moved onto the shared walk

5. `conflictSummary`'s last fallback

7. The `finally` closing the stamp session

9. `MAX_CHAIN_LINKS` is unpinned in both directions

10. Is `getIndexInfo` schema-qualified?